Show cumulative diffs in the diffviewer

Review Request #10123 — Created Aug. 21, 2018 and submitted

brennie
Review Board
release-4.0.x
10119
10124
7e6d551...
reviewboard

Previously, FileDiffs for review requests with history would not show
cumulatively. That is, you would see only the result of applying that
FileDiff, not the result of applying the entire stack of FileDiffs
in the history graph.

We now compute the entire file history, including deletions, and use
that to compute the cumulative diff. In other words, for a review
request with commits A -> B -> C where all three commits modify the
same file, previously you would see the result of A, B, and C
independently. Now the diffviewer shows A, A..B, and A..C.

Files are still repeated (as indicated above) so this is not yet a true
cumulative diff. A future patch will update the diffviewer to only show
the leaf FileDiffs in the history graph.

  • Ran unit tests.
  • Uploaded a multi-commit review request and looked at the diff. I
    observed cumulative (but repeated) diffs.
  • 0
  • 0
  • 24
  • 1
  • 25
Description From Last Updated
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

brennie
brennie
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

brennie
brennie
Review request changed

Summary:

-WIP: Show cumulative diffs in the diffviewer
+Show cumulative diffs in the diffviewer

Testing Done:

   
  • Ran unit tests.
   
  • Uploaded a multi-commit review request and looked at the diff. I
    observed cumulative (but repeated) diffs.
-  
  • TODO: Add unit tests.

Commit:

-ecec404ab7fb0e45af857d295606ccf66b95fe9f
+80f2911476891ce37c947036b34de2ef1e4d7804

Diff:

Revision 4 (+586 -234)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

brennie
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

brennie
brennie
brennie
brennie
brennie
chipx86
  1. 
      
  2. reviewboard/diffviewer/diffutils.py (Diff revision 10)
     
     

    Can you explicitly compare against 0?

  3. reviewboard/diffviewer/diffutils.py (Diff revision 10)
     
     
     
     
     

    One blank line.

  4. reviewboard/diffviewer/diffutils.py (Diff revision 10)
     
     
     
     

    Can you move the paren for the base_filediff stuff onto the next line, like:

    (newfile or
     (base_filediff is not None and ...
      ...)
    
  5. You'll need to import range from six, or we get a list here on Python 2.

  6. Missing docstring.

  7. Missing docstring.

  8. reviewboard/diffviewer/tests/test_diffutils.py (Diff revision 10)
     
     
     
     
     
     

    Too many blank lines.

  9. reviewboard/diffviewer/tests/test_diffutils.py (Diff revision 10)
     
     
     
     
     

    Too many blank lines.

  10. reviewboard/diffviewer/tests/test_diffutils.py (Diff revision 10)
     
     
     
     

    We should compare in the returned order, not as a set, to catch future discrepencies.

  11. Since we're looping and asserting stuff in each loop, we should print out something to help identify the diff_file we're working on in case of assertion.

  12. 
      
brennie
brennie
chipx86
  1. Ship It!
  2. 
      
david
  1. 
      
  2. reviewboard/diffviewer/diffutils.py (Diff revision 12)
     
     

    This is indented 1 too many spaces.

  3. I personally find it more readable to do for i in reversed(range(len(ancestor_ids))). No biggie though.

    1. This is the iterator version of range, ie xrange. reversed() cannot accept iterators, only iterables.

  4. 
      
brennie
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-4.0.x (0dd6930)
Loading...