Generate diffs between base and tip commits

Review Request #10130 — Created Aug. 27, 2018 and submitted

Information

Review Board
release-4.0.x
45b9779...

Reviewers

The get_diff_files utility can now generate a list of files to diff
that exist between two commits in a diffset. This allows us to generate
diffs between two commits (and therefore to see the diff of any
individual commit). There is currently no UI support for this behaviour,
but that is comming in a future patch.

Ran unit tests.

Description From Last Updated

F811 redefinition of unused 'test_get_diff_files_with_history_tip_commit' from line 1213

reviewbotreviewbot

Missing space between the backtick and "diffsets".

chipx86chipx86

Here, too.

chipx86chipx86

Missing blank line.

chipx86chipx86

No blank line needed here.

chipx86chipx86

Given the repeated complexity of this, it would be nice to make this whole expected_results building into a private utility …

chipx86chipx86

Redundant "diffsets" in this sentence.

chipx86chipx86

Same here.

chipx86chipx86

Can you compare explicitly against 0?

chipx86chipx86

Here, too.

chipx86chipx86

Should be .. code-block:: python (note the ::).

chipx86chipx86

This doesn't match.

chipx86chipx86

Syntax is super wonky here (0and). Does this even run?

daviddavid

Should be "base commit"

daviddavid

This is a little too fancy. How about: for ancestor in reversed(ancestors): if ancestor.commit_id <= base_commit.pk: base_filediff = ancestor break

daviddavid

This looks like a fancy way of just doing by_details.get(base_filediff_details).

chipx86chipx86
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

brennie
chipx86
  1. 
      
  2. reviewboard/diffviewer/diffutils.py (Diff revision 2)
     
     
    Show all issues

    Missing space between the backtick and "diffsets".

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

    Here, too.

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

    Missing blank line.

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

    No blank line needed here.

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

    Given the repeated complexity of this, it would be nice to make this whole expected_results building into a private utility method.

  7. 
      
brennie
chipx86
  1. 
      
  2. reviewboard/diffviewer/diffutils.py (Diff revision 3)
     
     
     
    Show all issues

    Redundant "diffsets" in this sentence.

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

    Same here.

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

    Can you compare explicitly against 0?

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

    Here, too.

  6. Show all issues

    Should be .. code-block:: python (note the ::).

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

    This doesn't match.

    1. Weird, I must've missed committing the docs change because I have it locally.

  8. 
      
brennie
david
  1. 
      
  2. reviewboard/diffviewer/diffutils.py (Diff revision 4)
     
     
    Show all issues

    Syntax is super wonky here (0and). Does this even run?

    1. Looks like I goofed it up before committing. Oops.

  3. 
      
brennie
david
  1. 
      
  2. reviewboard/diffviewer/diffutils.py (Diff revision 5)
     
     
    Show all issues

    Should be "base commit"

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

    This is a little too fancy. How about:

    for ancestor in reversed(ancestors):
        if ancestor.commit_id <= base_commit.pk:
            base_filediff = ancestor
            break
    
  4. 
      
brennie
chipx86
  1. 
      
  2. reviewboard/diffviewer/tests/test_diffutils.py (Diff revision 6)
     
     
     
     
     
     
    Show all issues

    This looks like a fancy way of just doing by_details.get(base_filediff_details).

  3. 
      
brennie
chipx86
  1. Ship It!
  2. 
      
brennie
Review request changed
Status:
Completed
Change Summary:
Pushed to release-4.0.x (5820e10)