Summary

Fix interdiff post-processing with replace chunks overflowing ranges.

Review Request #6030 — Created June 25, 2014 and submitted July 5, 2014, 1:23 a.m.

Information

Owner

chipx86

Repository

Review Board

Branch

release-2.0.x

Bugs

3440

Depends On

Commit

44201b8...

Reviewers

Groups

reviewboard

People

Description

When post-processing interdiffs in order to filter out unrelated code,
we had issues where replace chunks could be generated where its
orig/modified ranges weren't equal. This would trigger an assert, which
is probably good, because removing the assert resulted in a
corrupted-looking diff.

This same sort of bug could also affect equal chunks in theory, though
in practice, they weren't in as bad a situation.

The reason this was failing was that we attempted to cap each range to
the chunk's boundaries, without any regard to whether or not the lengths
needed to be consistent.

Now, for equal and replace chunks, we get the lengths of the
orig/modified ranges for the chunk, figure out the longest of the two,
cap that within the valid range boundary, and use that as the end
positions for the chunk.

This ensures that we only show the replace/equal range for the section
represented in the union of the two diffs. Anything else outside of that
will turn into a filtered-equal.

I added a new unit test that previously broke but is fixed with this
change. However, I then noticed that another unit test broke. Turns out,
it was already broken. It had resulting replace/equal chunks that didn't
have consistent ranges, and chunks overflowing the bounds. After looking
at the original chunks and throwing away the unit test's bad resulting
chunks, I created a new set based on what they should have been. This
ended up matching the results of the unit test.

I added some new sanity-checking in the unit tests to make sure that any
opcode lists we provide in the future will meet certain criteria, to
help catch such issues in the future.

Testing Done

All unit tests pass, including the fixed version of another test (which has
results computed by hand and not copied/pasted from the results).

Tested with a series of interdiffs I had laying around, and with some broken
interdiffs that were identified.

Tool: Pyflakes
Processed Files:
    reviewboard/diffviewer/processors.py
    reviewboard/diffviewer/tests.py



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

Ship it!

```
Ship It!
```

A new diff added to the existing review is now causing the following diff viewer error:

Traceback (most recent call last):
File "/usr/local/lib/python2.7/dist-packages/ReviewBoard-2.0.1-py2.7.egg/reviewboard/diffviewer/views.py", line 236, in get
renderer = self.create_renderer(context, args, kwargs)
File "/usr/local/lib/python2.7/dist-packages/ReviewBoard-2.0.1-py2.7.egg/reviewboard/reviews/views.py", line 1102, in create_renderer
args, **kwargs)
File "/usr/local/lib/python2.7/dist-packages/ReviewBoard-2.0.1-py2.7.egg/reviewboard/diffviewer/views.py", line 326, in create_renderer
self.diff_file = self._get_requested_diff_file()
File "/usr/local/lib/python2.7/dist-packages/ReviewBoard-2.0.1-py2.7.egg/reviewboard/diffviewer/views.py", line 367, in _get_requested_diff_file
request=self.request)
File "/usr/local/lib/python2.7/dist-packages/ReviewBoard-2.0.1-py2.7.egg/reviewboard/diffviewer/diffutils.py", line 373, in populate_diff_chunks
chunks = generator.get_chunks()
File "/usr/local/lib/python2.7/dist-packages/ReviewBoard-2.0.1-py2.7.egg/reviewboard/diffviewer/chunk_generator.py", line 148, in get_chunks
large_data=True)
File "/usr/local/lib/python2.7/dist-packages/Djblets-0.8.2-py2.7.egg/djblets/cache/backend.py", line 109, in cache_memoize
data = lookup_callable()
File "/usr/local/lib/python2.7/dist-packages/ReviewBoard-2.0.1-py2.7.egg/reviewboard/diffviewer/chunk_generator.py", line 147, in <lambda>
lambda: list(self._get_chunks_uncached()),
File "/usr/local/lib/python2.7/dist-packages/ReviewBoard-2.0.1-py2.7.egg/reviewboard/diffviewer/chunk_generator.py", line 253, in _get_chunks_uncached
for tag, i1, i2, j1, j2, meta in opcodes_generator:
File "/usr/local/lib/python2.7/dist-packages/ReviewBoard-2.0.1-py2.7.egg/reviewboard/diffviewer/opcode_generator.py", line 70, in iter
self._group_opcodes(opcodes)
File "/usr/local/lib/python2.7/dist-packages/ReviewBoard-2.0.1-py2.7.egg/reviewboard/diffviewer/opcode_generator.py", line 141, in _group_opcodes
for group_index, group in enumerate(opcodes):
File "/usr/local/lib/python2.7/dist-packages/ReviewBoard-2.0.1-py2.7.egg/reviewboard/diffviewer/opcode_generator.py", line 137, in _apply_meta_processors
for opcode in opcodes:
File "/usr/local/lib/python2.7/dist-packages/ReviewBoard-2.0.1-py2.7.egg/reviewboard/diffviewer/processors.py", line 214, in post_process_filtered_equals
for tag, i1, i2, j1, j2, meta in opcodes:
File "/usr/local/lib/python2.7/dist-packages/ReviewBoard-2.0.1-py2.7.egg/reviewboard/diffviewer/opcode_generator.py", line 87, in _generate_opcode_meta
for tag, i1, i2, j1, j2 in opcodes:
File "/usr/local/lib/python2.7/dist-packages/ReviewBoard-2.0.1-py2.7.egg/reviewboard/diffviewer/opcode_generator.py", line 83, in _apply_processors
for opcode in opcodes:
File "/usr/local/lib/python2.7/dist-packages/ReviewBoard-2.0.1-py2.7.egg/reviewboard/diffviewer/processors.py", line 125, in filter_interdiff_opcodes
cap_i2 = orig_range[1] + 1
TypeError: 'NoneType' object has no attribute 'getitem'

chipx86 June 26, 2014, 11:49 a.m.

Thanks for testing this out. I'll see about getting a fix for it today or tomorrow.

Just a tip for 2.0: You can wrap all that like:

```
content here
```

It will then format it nicely and not interpret as Markdown.

chipx86 July 1, 2014, 12:09 a.m.

I was out, but I have a change going up shortly. I would like to know more about the diff in question, though. Are either part of the diff new, deleted, or reverted files? Any major file size differences?

bcran July 1, 2014, 9:14 a.m.

It's failing on files that were deleted in the first diff.
Also, it seems to get the icons in the diff summary confused with interdiffs - either leaving them blank when files have been changed, or using the wrong icons.

chipx86 July 1, 2014, 10:53 a.m.

We should have a separate bug filed for the icons.

Were you able to test the latest patch?

bcran July 1, 2014, 11:10 a.m.

I've just tested the 2nd diff, and the same 'IndexError: list index out of range' exception occurs with files that were deleted in the first diff.

chipx86 July 1, 2014, 11:12 a.m.

When you say deleted, do you mean that diff revision 1 deletes a file, and diff revision 2 doesn't have that file deleted?

BC

bcran July 1, 2014, 11:16 a.m.
```
Both diff versions delete files.
```

chipx86 July 1, 2014, 11:34 a.m.

Were the files different lengths in both of the diffs?

I might need to see both of the diffs.

chipx86 July 1, 2014, 11:50 a.m.

Actually, can you provide a new backtrace? There's no way that you're getting the same exact error for this, since the original failure of trying to access orig_range when None is now wrapped in a conditional checking if it's not None first.

bcran July 1, 2014, 11:53 a.m.

Sorry, it is a different error:

Traceback (most recent call last):
  File "/usr/local/lib/python2.7/dist-packages/ReviewBoard-2.0.1-py2.7.egg/reviewboard/diffviewer/views.py", line 236, in get
    renderer = self.create_renderer(context, *args, **kwargs)
  File "/usr/local/lib/python2.7/dist-packages/ReviewBoard-2.0.1-py2.7.egg/reviewboard/reviews/views.py", line 1102, in create_renderer
    *args, **kwargs)
  File "/usr/local/lib/python2.7/dist-packages/ReviewBoard-2.0.1-py2.7.egg/reviewboard/diffviewer/views.py", line 326, in create_renderer
    self.diff_file = self._get_requested_diff_file()
  File "/usr/local/lib/python2.7/dist-packages/ReviewBoard-2.0.1-py2.7.egg/reviewboard/diffviewer/views.py", line 367, in _get_requested_diff_file
    request=self.request)
  File "/usr/local/lib/python2.7/dist-packages/ReviewBoard-2.0.1-py2.7.egg/reviewboard/diffviewer/diffutils.py", line 373, in populate_diff_chunks
    chunks = generator.get_chunks()
  File "/usr/local/lib/python2.7/dist-packages/ReviewBoard-2.0.1-py2.7.egg/reviewboard/diffviewer/chunk_generator.py", line 148, in get_chunks
    large_data=True)
  File "/usr/local/lib/python2.7/dist-packages/Djblets-0.8.2-py2.7.egg/djblets/cache/backend.py", line 109, in cache_memoize
    data = lookup_callable()
  File "/usr/local/lib/python2.7/dist-packages/ReviewBoard-2.0.1-py2.7.egg/reviewboard/diffviewer/chunk_generator.py", line 147, in <lambda>
    lambda: list(self._get_chunks_uncached()),
  File "/usr/local/lib/python2.7/dist-packages/ReviewBoard-2.0.1-py2.7.egg/reviewboard/diffviewer/chunk_generator.py", line 283, in _get_chunks_uncached
    yield self._new_chunk(lines, 0, num_lines, False, tag, meta)
  File "/usr/local/lib/python2.7/dist-packages/ReviewBoard-2.0.1-py2.7.egg/reviewboard/diffviewer/chunk_generator.py", line 558, in _new_chunk
    compute_chunk_last_header(lines, num_lines, meta, self._last_header)
  File "/usr/local/lib/python2.7/dist-packages/ReviewBoard-2.0.1-py2.7.egg/reviewboard/diffviewer/chunk_generator.py", line 642, in compute_chunk_last_header
    line = lines[0]
IndexError: list index out of range

chipx86 July 1, 2014, 11:54 a.m.

Oh okay, so that's an entirely separate issue. If the issue for this change is fixed, we'll be able to at least get it in.

I will need diffs and original source files for that one though.

chipx86 July 1, 2014, 11:57 a.m.

(It actually may be related, but I don't have enough information to go on right now.)

bcran July 1, 2014, 11:58 a.m.

Great - thanks, and apologies for the confusion.

chipx86 July 1, 2014, 12:01 p.m.

For those files, I don't actually need the content, but if you can tell me this information, I'll see if it's fallout from this change. I'll need these from both revisions of the diff for the failed file:

The number of lines in the file.
Each line in the diff starting with "@@" (there won't be any confidential data there, and there should probably only be one such line in each).

chipx86 July 3, 2014, 12:41 p.m.

Bruce, would you be able to send me that info? I'm hoping to figure out a solution before we do a release with this patch.

bcran July 3, 2014, 12:43 p.m.

Sorry for the delay - I'll get the info this afternoon. FWIW I reset the installation back to plain 2.0.1 and the "list index out of range" error still occurred, so it's not caused by this patch.

bcran July 5, 2014, 10:52 a.m.

If it's still useful, this is from a review that had 5 revisions and failed to display changes for some files between revisions 4 and 5.

diff 4:
number of lines: 43
@@ -1,39 +0,0 @@
diff 5:
number of lines: 43
@@ -1,39 +0,0 @@

So there were apparently no changes to that file between the two revisions.

chipx86 July 6, 2014, 8:44 p.m.

Thanks Bruce. Is this for the error this review request was addressing, or the one in the most recent traceback above?

bcran July 6, 2014, 9:02 p.m.

That's for the most recent traceback ("list index out of range").

Change Summary:

Fixed issues with replace and equal lines when one side starts past its diff's last range.

Commit:

-	617f432b03102a5e80d67b50884c46ed3ffb91bf
+	44201b8a2284514123cc906057cf0ced0f85f752

Diff:

Revision 2 (+164 -15)

Show changes

	reviewboard/diffviewer/processors.py
	reviewboard/diffviewer/tests.py

Tool: Pyflakes
Processed Files:
    reviewboard/diffviewer/processors.py
    reviewboard/diffviewer/tests.py



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

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-2.0.x (626335f)