Show cumulative diffs in the diffviewer

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

Information

Review Board
release-4.0.x
7e6d551...

Reviewers

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
Change Summary:

Better architecture.

Commit:
6768bdcb44671579184d7ab94dc12708bd0e7b90
1dbe3963e4b8c427f5d2152e83602493bd7f2dad

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

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

brennie
Review request changed
Commit:
80f2911476891ce37c947036b34de2ef1e4d7804
a9fcb81c2bd0a0a3ef140fcae3cffe211c8f882c

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)
     
     
    Show all issues

    Can you explicitly compare against 0?

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

    One blank line.

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

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

    (newfile or
     (base_filediff is not None and ...
      ...)
    
  5. Show all issues

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

  6. Show all issues

    Missing docstring.

  7. Show all issues

    Missing docstring.

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

    Too many blank lines.

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

    Too many blank lines.

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

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

  11. Show all issues

    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)
     
     
    Show all issues

    This is indented 1 too many spaces.

  3. Show all issues

    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:
Completed
Change Summary:
Pushed to release-4.0.x (0dd6930)