Show squashed diffs in the diffviewer
Review Request #7135 — Created March 28, 2015 and submitted
Previously we would show multiple
FileDiffs for the same file in the
diff viewer if the file was modified in multiple commits. Now we
determine whatFileDiffs are ancestors of otherFileDiffs in the
commit history and filter out all the ancestors. The result is a
squashed diff between the original file and the resulting file.
Ran unit tests.
Manually verified that the squashed diffs were correct.
Still able to view the
FileDiffs of review requests without
commit history.
| Description | From | Last Updated |
|---|---|---|
|
Col: 12 E111 indentation is not a multiple of four |
 reviewbot
|
|
|
We should probably just import logging at the top of the file. |
 david
|
|
|
This should use exc_info=True (we have =1 elsewhere, but I'm slowly cleaning that up). |
david
|
|
|
I think I liked your approach up above of changing all of these to add the - and the beginning … |
david
|
|
|
Unindent. |
 brennie
|
|
|
"In this case" twice is awkward phrasing. |
brennie
|
|
|
diffviewer.commitutils before diffviewer.differ |
david
|
|
|
Blank line after this. |
david
|
|
|
This should also have a '-' |
david
|
|
|
Can you maybe describe this option in the docstring above? |
 chipx86
|
|
|
Does not check if there is a diff_commit. |
brennie
|
|
|
This has the potential to be cheaper by doing: self.filediff.diff_commit_id. Reduces the possibility of a lookup. |
chipx86
|
|
|
Alignment is inconsistent between these. |
chipx86
|
|
|
Maybe flesh this out a bit so its description stands alone without needing to know more of the architecture as … |
chipx86
|
|
|
Can you use a list comprehension instead of map? They're generally easier to read and are faster, since they don't … |
chipx86
|
|
|
Since this code is all basically the same as the above, we should factor this out into another function. |
chipx86
|
|
|
Does not check if there is a diff_commit |
brennie
|
|
|
As above, we should use diff_commit_id here instead. |
chipx86
|
|
|
No need for six.text_type anymore. |
chipx86
|
|
|
Col: 5 E303 too many blank lines (2) |
reviewbot
|
|
|
local variable 'have_interdiffset_commits' is assigned to but never used |
reviewbot
|
|
|
"FileDiffs" and "DiffSet". |
chipx86
|
|
|
Looks like we're doing this twice now? |
chipx86
|
Summary: |
|
||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
||||||||||||
Testing Done: |
|
||||||||||||
Commit: |
|
||||||||||||
Diff: |
Revision 2 (+207 -20) |
-
Tool: Pyflakes Processed Files: reviewboard/diffviewer/chunk_generator.py reviewboard/diffviewer/renderers.py reviewboard/diffviewer/models.py reviewboard/diffviewer/tests.py reviewboard/diffviewer/diffutils.py reviewboard/diffviewer/views.py reviewboard/diffviewer/commitutils.py Tool: PEP8 Style Checker Processed Files: reviewboard/diffviewer/chunk_generator.py reviewboard/diffviewer/renderers.py reviewboard/diffviewer/models.py reviewboard/diffviewer/tests.py reviewboard/diffviewer/diffutils.py reviewboard/diffviewer/views.py reviewboard/diffviewer/commitutils.py -
reviewboard/diffviewer/chunk_generator.py (Diff revision 2) Col: 12 E111 indentation is not a multiple of four
Change Summary:
wrap description
Description: |
|
|---|
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 3 (+207 -20) |
-
Tool: Pyflakes Processed Files: reviewboard/diffviewer/chunk_generator.py reviewboard/diffviewer/renderers.py reviewboard/diffviewer/models.py reviewboard/diffviewer/tests.py reviewboard/diffviewer/diffutils.py reviewboard/diffviewer/views.py reviewboard/diffviewer/commitutils.py Tool: PEP8 Style Checker Processed Files: reviewboard/diffviewer/chunk_generator.py reviewboard/diffviewer/renderers.py reviewboard/diffviewer/models.py reviewboard/diffviewer/tests.py reviewboard/diffviewer/diffutils.py reviewboard/diffviewer/views.py reviewboard/diffviewer/commitutils.py
-
-
reviewboard/diffviewer/diffutils.py (Diff revision 3) We should probably just import logging at the top of the file.
-
reviewboard/diffviewer/diffutils.py (Diff revision 3) This should use
exc_info=True(we have=1elsewhere, but I'm slowly cleaning that up). -
reviewboard/diffviewer/renderers.py (Diff revision 3) I think I liked your approach up above of changing all of these to add the - and the beginning of each appended string.
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 4 (+207 -25) |
-
Tool: Pyflakes Processed Files: reviewboard/diffviewer/chunk_generator.py reviewboard/diffviewer/renderers.py reviewboard/diffviewer/models.py reviewboard/diffviewer/tests.py reviewboard/diffviewer/diffutils.py reviewboard/diffviewer/views.py reviewboard/diffviewer/commitutils.py Tool: PEP8 Style Checker Processed Files: reviewboard/diffviewer/chunk_generator.py reviewboard/diffviewer/renderers.py reviewboard/diffviewer/models.py reviewboard/diffviewer/tests.py reviewboard/diffviewer/diffutils.py reviewboard/diffviewer/views.py reviewboard/diffviewer/commitutils.py
-
-
reviewboard/diffviewer/chunk_generator.py (Diff revision 4) Would it be worth it to build a list of parts for the key and use
''.joinversus all the appends we're doing, or does that matter? Here and in the renderer. -
-
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 5 (+206 -25) |
-
Tool: Pyflakes Processed Files: reviewboard/diffviewer/chunk_generator.py reviewboard/diffviewer/renderers.py reviewboard/diffviewer/models.py reviewboard/diffviewer/tests.py reviewboard/diffviewer/diffutils.py reviewboard/diffviewer/views.py reviewboard/diffviewer/commitutils.py Tool: PEP8 Style Checker Processed Files: reviewboard/diffviewer/chunk_generator.py reviewboard/diffviewer/renderers.py reviewboard/diffviewer/models.py reviewboard/diffviewer/tests.py reviewboard/diffviewer/diffutils.py reviewboard/diffviewer/views.py reviewboard/diffviewer/commitutils.py
-
-
reviewboard/diffviewer/chunk_generator.py (Diff revision 5) diffviewer.commitutilsbeforediffviewer.differ -
-
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 6 (+208 -26) |
-
Tool: Pyflakes Processed Files: reviewboard/diffviewer/chunk_generator.py reviewboard/diffviewer/renderers.py reviewboard/diffviewer/models.py reviewboard/diffviewer/tests.py reviewboard/diffviewer/diffutils.py reviewboard/diffviewer/views.py reviewboard/diffviewer/commitutils.py Tool: PEP8 Style Checker Processed Files: reviewboard/diffviewer/chunk_generator.py reviewboard/diffviewer/renderers.py reviewboard/diffviewer/models.py reviewboard/diffviewer/tests.py reviewboard/diffviewer/diffutils.py reviewboard/diffviewer/views.py reviewboard/diffviewer/commitutils.py
-
-
reviewboard/diffviewer/chunk_generator.py (Diff revision 6) Does not check if there is a
diff_commit. -
Change Summary:
Turn off cumulative diffing if there is not history.
Testing Done: |
|
||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Commit: |
|
||||||||||||||||||
Diff: |
Revision 7 (+217 -26) |
-
-
reviewboard/diffviewer/chunk_generator.py (Diff revision 6) Can you maybe describe this option in the docstring above?
-
reviewboard/diffviewer/chunk_generator.py (Diff revision 6) This has the potential to be cheaper by doing:
self.filediff.diff_commit_id. Reduces the possibility of a lookup. -
reviewboard/diffviewer/chunk_generator.py (Diff revision 6) Alignment is inconsistent between these.
-
reviewboard/diffviewer/commitutils.py (Diff revision 6) Maybe flesh this out a bit so its description stands alone without needing to know more of the architecture as a whole.
-
reviewboard/diffviewer/diffutils.py (Diff revision 6) Can you use a list comprehension instead of
map? They're generally easier to read and are faster, since they don't require the extra function calls. -
reviewboard/diffviewer/diffutils.py (Diff revision 6) Since this code is all basically the same as the above, we should factor this out into another function.
-
reviewboard/diffviewer/renderers.py (Diff revision 6) As above, we should use
diff_commit_idhere instead. -
-
Tool: Pyflakes Processed Files: reviewboard/diffviewer/chunk_generator.py reviewboard/diffviewer/renderers.py reviewboard/diffviewer/models.py reviewboard/diffviewer/tests.py reviewboard/diffviewer/diffutils.py reviewboard/diffviewer/views.py reviewboard/diffviewer/commitutils.py Tool: PEP8 Style Checker Processed Files: reviewboard/diffviewer/chunk_generator.py reviewboard/diffviewer/renderers.py reviewboard/diffviewer/models.py reviewboard/diffviewer/tests.py reviewboard/diffviewer/diffutils.py reviewboard/diffviewer/views.py reviewboard/diffviewer/commitutils.py
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 8 (+244 -27) |
-
Tool: Pyflakes Processed Files: reviewboard/diffviewer/chunk_generator.py reviewboard/diffviewer/renderers.py reviewboard/diffviewer/models.py reviewboard/diffviewer/tests.py reviewboard/diffviewer/diffutils.py reviewboard/diffviewer/views.py reviewboard/diffviewer/commitutils.py Tool: PEP8 Style Checker Processed Files: reviewboard/diffviewer/chunk_generator.py reviewboard/diffviewer/renderers.py reviewboard/diffviewer/models.py reviewboard/diffviewer/tests.py reviewboard/diffviewer/diffutils.py reviewboard/diffviewer/views.py reviewboard/diffviewer/commitutils.py -
-
reviewboard/diffviewer/diffutils.py (Diff revision 8) local variable 'have_interdiffset_commits' is assigned to but never used
Change Summary:
PEP8
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 9 (+240 -27) |
-
Tool: Pyflakes Processed Files: reviewboard/diffviewer/chunk_generator.py reviewboard/diffviewer/renderers.py reviewboard/diffviewer/models.py reviewboard/diffviewer/tests.py reviewboard/diffviewer/diffutils.py reviewboard/diffviewer/views.py reviewboard/diffviewer/commitutils.py Tool: PEP8 Style Checker Processed Files: reviewboard/diffviewer/chunk_generator.py reviewboard/diffviewer/renderers.py reviewboard/diffviewer/models.py reviewboard/diffviewer/tests.py reviewboard/diffviewer/diffutils.py reviewboard/diffviewer/views.py reviewboard/diffviewer/commitutils.py
Change Summary:
Address Christian's issues.
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 10 (+234 -27) |
-
Tool: Pyflakes Processed Files: reviewboard/diffviewer/chunk_generator.py reviewboard/diffviewer/renderers.py reviewboard/diffviewer/models.py reviewboard/diffviewer/tests.py reviewboard/diffviewer/diffutils.py reviewboard/diffviewer/views.py reviewboard/diffviewer/commitutils.py Tool: PEP8 Style Checker Processed Files: reviewboard/diffviewer/chunk_generator.py reviewboard/diffviewer/renderers.py reviewboard/diffviewer/models.py reviewboard/diffviewer/tests.py reviewboard/diffviewer/diffutils.py reviewboard/diffviewer/views.py reviewboard/diffviewer/commitutils.py
Tool: Pyflakes Processed Files: reviewboard/diffviewer/models.py reviewboard/diffviewer/tests.py reviewboard/diffviewer/diffutils.py Tool: PEP8 Style Checker Processed Files: reviewboard/diffviewer/models.py reviewboard/diffviewer/tests.py reviewboard/diffviewer/diffutils.py