Summary

Fix "List index out of range" errors with interdiffing deleted files.

Review Request #6473 — Created Oct. 20, 2014 and submitted Oct. 22, 2014, 1:04 a.m.

Information

Owner

david

Repository

Review Board

Branch

release-2.0.x

Bugs

3481

Depends On

Commit

62eb033...

Reviewers

Groups

reviewboard

People

Description

It's possible to hit an IndexError when processing an interdiff between two
changes that delete a file. The wrinkle here is that between the two revisions
of the diff, some additional changes to that file were made (and synced to the
local developer's tree).

The result of this is that we didn't filter out the file from the list of
things to show for the interdiff, and then things went wrong when we tried to
process the opcodes for diffing two empty strings.

While I was doing this, I saw some warnings about a unicode-unaware comparison
occurring, so I've slipped the fix for that in as well. This doesn't actually
have any correctness implications, it just speeds up things slightly when
dealing with empty files.

Testing Done


Created a reproduction case for the bug and tested before and after the

  change. Before, I saw an entry for this file with the error inside. After,

  the deleted files don't show up at all in the interdiff.
Ran unit tests.

Issues

Description	From	Last Updated
Col: 17 E201 whitespace after '('	reviewbot	Oct. 20, 2014, 11:29 p.m.
Every Python linter complains about this format, and it's just not common in Python. Can we go back to the …	chipx86	Oct. 20, 2014, 11:56 p.m.
Col: 17 E128 continuation line under-indented for visual indent	reviewbot	Oct. 20, 2014, 11:29 p.m.
Col: 17 E128 continuation line under-indented for visual indent	reviewbot	Oct. 20, 2014, 11:29 p.m.
Col: 22 E201 whitespace after '('	reviewbot	Oct. 20, 2014, 11:29 p.m.
Col: 22 E128 continuation line under-indented for visual indent	reviewbot	Oct. 20, 2014, 11:29 p.m.
What happens for legitimately empty files? Do we want to do this just for interdiffs? Also, can we add a …	chipx86	Oct. 21, 2014, 8:28 p.m.

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/diffviewer/diffutils.py



Tool: Pyflakes
Processed Files:
    reviewboard/diffviewer/diffutils.py

reviewboard/diffviewer/diffutils.py (Diff revision 1)
The issue has been dropped. Show all issues
```
Col: 17
 E201 whitespace after '('
```
reviewboard/diffviewer/diffutils.py (Diff revision 1)
The issue has been dropped. Show all issues
```
Col: 17
 E128 continuation line under-indented for visual indent
```
reviewboard/diffviewer/diffutils.py (Diff revision 1)
The issue has been dropped. Show all issues
```
Col: 17
 E128 continuation line under-indented for visual indent
```
reviewboard/diffviewer/diffutils.py (Diff revision 1)
The issue has been dropped. Show all issues
```
Col: 22
 E201 whitespace after '('
```
reviewboard/diffviewer/diffutils.py (Diff revision 1)
The issue has been dropped. Show all issues
```
Col: 22
 E128 continuation line under-indented for visual indent
```

reviewboard/diffviewer/diffutils.py (Diff revision 1)

The issue has been resolved. Show all issues

Every Python linter complains about this format, and it's just not common in Python. Can we go back to the format we use everywhere else?

Commit:

2301e26f2c23ce76166f13372332fa270e9a359c

9830ce5597089c224accda8879c49ad3ba0811ee

Diff:

Revision 2 (+7 -5)

Show changes

reviewboard/diffviewer/diffutils.py

Tool: Pyflakes
Processed Files:
    reviewboard/diffviewer/diffutils.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/diffviewer/diffutils.py

reviewboard/diffviewer/diffutils.py (Diff revision 2)

Do we want/need to compare against b'\r' here, or is this sufficient?

Change Summary:

Cover a couple more cases.

Commit:

9830ce5597089c224accda8879c49ad3ba0811ee

dc18c41a57db0ee6c035de42e9ba1b8323a326be

Diff:

Revision 3 (+12 -7)

Show changes

	reviewboard/diffviewer/chunk_generator.py
	reviewboard/diffviewer/diffutils.py

Tool: Pyflakes
Processed Files:
    reviewboard/diffviewer/chunk_generator.py
    reviewboard/diffviewer/diffutils.py

reviewboard/diffviewer/chunk_generator.py (Diff revision 3)

The issue has been resolved. Show all issues

What happens for legitimately empty files?

Do we want to do this just for interdiffs?

Also, can we add a comment here detailing why we're skipping?

david Oct. 21, 2014, 8:30 p.m.

Adding or deleting legitimately empty files won't end up with any opcodes to process, so it won't get here. I'll add a comment.

Commit:

dc18c41a57db0ee6c035de42e9ba1b8323a326be

d6a8205b7aca6e77719bbbc691f2e61ad94bf5d2

Diff:

Revision 4 (+20 -7)

Show changes

	reviewboard/diffviewer/chunk_generator.py
	reviewboard/diffviewer/diffutils.py

Tool: Pyflakes
Processed Files:
    reviewboard/diffviewer/chunk_generator.py
    reviewboard/diffviewer/diffutils.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/diffviewer/chunk_generator.py
    reviewboard/diffviewer/diffutils.py

Change Summary:

Revert changes to chunk_generator.py

Commit:

d6a8205b7aca6e77719bbbc691f2e61ad94bf5d2

62eb03356bd97f957cd3601dc8500ee6f10aa628

Diff:

Revision 5 (+9 -7)

Show changes

reviewboard/diffviewer/diffutils.py

Tool: Pyflakes
Processed Files:
    reviewboard/diffviewer/diffutils.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/diffviewer/diffutils.py

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to release-2.0.x (2b23541)