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
|
Change Summary:
Fix a typo
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 2 (+59 -25) |
-
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.)
Change Summary:
Add unit tests.
Description: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Commit: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 3 (+143 -37) |
-
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 -
Change Summary:
PEP8
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 4 (+144 -37) |
-
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
-
-
-
reviewboard/diffviewer/tests.py (Diff revision 4) Probably not worth having the :py:func: stuff in here, since this won't get turned into documentation.
-
-
reviewboard/diffviewer/tests.py (Diff revision 4) Can you expand this to have one key per line? It'll help with readability and maintainability.
Same with the others.
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 5 (+191 -37) |
-
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