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.
Description From Last Updated

E501 line too long (86 > 79 characters)

reviewbotreviewbot

E501 line too long (82 > 79 characters)

reviewbotreviewbot

E501 line too long (91 > 79 characters)

reviewbotreviewbot

E501 line too long (81 > 79 characters)

reviewbotreviewbot

E501 line too long (81 > 79 characters)

reviewbotreviewbot

E303 too many blank lines (2)

reviewbotreviewbot

F401 'reviewboard.diffviewer.models.DiffSet' imported but unused

reviewbotreviewbot

E303 too many blank lines (2)

reviewbotreviewbot

E303 too many blank lines (2)

reviewbotreviewbot

F811 redefinition of unused 'setUp' from line 2822

reviewbotreviewbot

F401 'reviewboard.diffviewer.models.DiffSet' imported but unused

reviewbotreviewbot

E303 too many blank lines (2)

reviewbotreviewbot

E303 too many blank lines (2)

reviewbotreviewbot

Can you explicitly compare against 0?

chipx86chipx86

One blank line.

chipx86chipx86

Can you move the paren for the base_filediff stuff onto the next line, like: (newfile or (base_filediff is not None ...

chipx86chipx86

You'll need to import range from six, or we get a list here on Python 2.

chipx86chipx86

Missing docstring.

chipx86chipx86

Missing docstring.

chipx86chipx86

Too many blank lines.

chipx86chipx86

Too many blank lines.

chipx86chipx86

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

chipx86chipx86

Since we're looping and asserting stuff in each loop, we should print out something to help identify the diff_file we're ...

chipx86chipx86

This is indented 1 too many spaces.

daviddavid

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

daviddavid
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...