Fix diff fragments with rebased interdiffs
Review Request #7188 — Created April 9, 2015 and submitted
Previously diff fragments of a rebased interdiff would sometimes not be
renderable. This was due to there being blank lines in chunks because of
the upstream changes being filtered out. This broke some assumptions
about the existence of line numbers in equals chunks.We are now more careful about our assumptions and check for the presence
of the line numbers themselves instead of checking the chunk's change
type. The diff fragments now render correctly. A test has been added to
ensure that this does not regress.We also are more careful when determining headers that can be
expanded to for a fragment because when chunks get merged together by
the opcode generator, the headers may end up in the wrong chunk. A test
has been added to ensure that the correct headers are returned when this
occurs.Diff header expansion now correctly compares the line numbers of each
header. Previously, thevirtualelement was being compared (instead of
line), which wasn't a key in either dict and so the comparsion was
being ignored silently. This results in left and right expansion headers
with different lines with the same code correctly being seperated.I also noticed that the terms virtual line number and unified number
were being used to refer to the same thing, so all references to unified
line numbers have been changed to refer to virtual line numbers for
consistency's sake.
Ran unit tests.
Uploaded two diffs to my development server. The second diff was rebased
off a later change and then modified. I created an issue on a section
just after a half blank chunk in the interdiff. The fragment was not
rendered without the patch applied and rendered correctly with the patch
applied. The expansion links also worked correctly with the patch
applied.The provided tests fail without this patch applied.
| Description | From | Last Updated |
|---|---|---|
|
Col: 1 E302 expected 2 blank lines, found 1 |
reviewbot
|
|
|
""" on the next line. |
chipx86
|
|
|
Probably not worth having the :py:func: stuff in here, since this won't get turned into documentation. |
chipx86
|
|
|
""" on the next line. |
chipx86
|
|
|
Can you expand this to have one key per line? It'll help with readability and maintainability. Same with the others. |
chipx86
|
Review request changed
- Change Summary:
-
Fix a typo
- Commit:
-
d3b888c7b2df469c1f25679646ef97d136838783ea5ed44460d6a014418569c25e5a4925f816a657
-
Tool: PEP8 Style Checker Processed Files: reviewboard/diffviewer/diffutils.py Ignored Files: reviewboard/templates/reviews/diff_comment_fragment.html Tool: Pyflakes Processed Files: reviewboard/diffviewer/diffutils.py Ignored Files: reviewboard/templates/reviews/diff_comment_fragment.html
-
This looks good. How difficult do you think it'd be to create a unit test for this? (Not always easy, but it'd be nice to make sure this never breaks again.)
Review request changed
- Change Summary:
-
Add unit tests.
- Description:
-
Previously diff fragments of a rebased interdiff would sometimes not be
renderable. This was due to there being blank lines in chunks because of the upstream changes being filtered out. This broke some assumptions about the existence of line numbers in equals chunks. We are now more careful about our assumptions and check for the presence
of the line numbers themselves instead of checking the chunk's change ~ type. The diff fragments now render correctly. ~ type. The diff fragments now render correctly. A test has been added to + ensure that this does not regress. We also are more careful when determining headers that can be
expanded to for a fragment because when chunks get merged together by ~ the opcode generator, the headers may end up in the wrong chunk. ~ the opcode generator, the headers may end up in the wrong chunk. A test + has been added to ensure that the correct headers are returned when this + occurs. Diff header expansion now correctly compares the line numbers of each
header. Previously, the virtualelement was being compared (instead ofline), which wasn't a key in either dict and so the comparsion wasbeing ignored silently. This results in left and right expansion headers with different lines with the same code correctly being seperated. I also noticed that the terms virtual line number and unified number
were being used to refer to the same thing, so all references to unified line numbers have been changed to refer to virtual line numbers for consistency's sake. - Testing Done:
-
Ran unit tests.
Uploaded two diffs to my development server. The second diff was rebased
off a later change and then modified. I created an issue on a section just after a half blank chunk in the interdiff. The fragment was not rendered without the patch applied and rendered correctly with the patch applied. The expansion links also worked correctly with the patch applied. + + The provided tests fail without this patch applied.
- Commit:
-
ea5ed44460d6a014418569c25e5a4925f816a657c687b14ae7bfc3cd9d4fb1ea0b039b1b3229f00d
-
Tool: Pyflakes Processed Files: reviewboard/diffviewer/tests.py reviewboard/diffviewer/diffutils.py Ignored Files: reviewboard/templates/reviews/diff_comment_fragment.html Tool: PEP8 Style Checker Processed Files: reviewboard/diffviewer/tests.py reviewboard/diffviewer/diffutils.py Ignored Files: reviewboard/templates/reviews/diff_comment_fragment.html -
Review request changed
- Change Summary:
-
PEP8
- Commit:
-
c687b14ae7bfc3cd9d4fb1ea0b039b1b3229f00de075525fae6fcd14e5b30120a337ba9ed2df5aad
-
Tool: Pyflakes Processed Files: reviewboard/diffviewer/tests.py reviewboard/diffviewer/diffutils.py Ignored Files: reviewboard/templates/reviews/diff_comment_fragment.html Tool: PEP8 Style Checker Processed Files: reviewboard/diffviewer/tests.py reviewboard/diffviewer/diffutils.py Ignored Files: reviewboard/templates/reviews/diff_comment_fragment.html
Review request changed
- Commit:
-
e075525fae6fcd14e5b30120a337ba9ed2df5aadbf64e657fe06e7c38662fc744c2a624ba6bbe6e4
-
Tool: PEP8 Style Checker Processed Files: reviewboard/diffviewer/tests.py reviewboard/diffviewer/diffutils.py Ignored Files: reviewboard/templates/reviews/diff_comment_fragment.html Tool: Pyflakes Processed Files: reviewboard/diffviewer/tests.py reviewboard/diffviewer/diffutils.py Ignored Files: reviewboard/templates/reviews/diff_comment_fragment.html
Tool: PEP8 Style Checker Processed Files: reviewboard/diffviewer/diffutils.py Ignored Files: reviewboard/templates/reviews/diff_comment_fragment.html Tool: Pyflakes Processed Files: reviewboard/diffviewer/diffutils.py Ignored Files: reviewboard/templates/reviews/diff_comment_fragment.html