Compute pre-patch files for FileDiffs in commit histories

Review Request #10119 — Created Aug. 13, 2018 and submitted

Information

Review Board
release-4.0.x
d1c92e1...

Reviewers

We can now generate the pre-patch version of a file that is modified
multiple times in a commit history. This allows the diffviewer to show
side-by-side diffs in these cases, however this behaviour is not correct
as it will show files with the same name multiple times when it should
show a condensed diff of the entire history. This will be addressed in a
later patch.

This also allows the original and patched file API endpoints to work for
files modified multiple times in a commit history.

  • Ran unit tests.
  • Was able to view diffs in the diffviewer that depended on previous
    FileDiffs.
Description From Last Updated

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

reviewbotreviewbot

F841 local variable 'to_apply' is assigned to but never used

reviewbotreviewbot

F841 local variable 'diffset' is assigned to but never used

reviewbotreviewbot

F821 undefined name 'orig_filediff'

reviewbotreviewbot

F821 undefined name 'orig_filediff'

reviewbotreviewbot

E501 line too long (80 > 79 characters)

reviewbotreviewbot

E501 line too long (81 > 79 characters)

reviewbotreviewbot

F841 local variable 'TypeError' is assigned to but never used

reviewbotreviewbot

W391 blank line at end of file

reviewbotreviewbot

F401 'base64' imported but unused

reviewbotreviewbot

F401 'json' imported but unused

reviewbotreviewbot

W503 line break before binary operator

reviewbotreviewbot

Missing trailing period.

chipx86chipx86

What happens if patching fails? We should document this situation more clearly in the docstring.

chipx86chipx86

We probably shouldn't Shortest Middle Snake this.

chipx86chipx86

Grammaro: "that is belongs to a commit"

chipx86chipx86

We can avoid the function calls by making this a generator expression instead of using filter.

chipx86chipx86

No need for .spy anymore. You can call directly on the function.

chipx86chipx86

These unit tests were never added.

chipx86chipx86

Alphabetical order.

chipx86chipx86

No () after the function names in any of these test docstrings.

chipx86chipx86

W503 line break before binary operator

reviewbotreviewbot

E302 expected 2 blank lines, found 1

reviewbotreviewbot

F811 redefinition of unused 'GetOriginalFileTests' from line 2527

reviewbotreviewbot

W391 blank line at end of file

reviewbotreviewbot

E501 line too long (85 > 79 characters)

reviewbotreviewbot

E501 line too long (81 > 79 characters)

reviewbotreviewbot

E501 line too long (85 > 79 characters)

reviewbotreviewbot

E501 line too long (81 > 79 characters)

reviewbotreviewbot

Alphabetical order.

chipx86chipx86

Here, too.

chipx86chipx86

Here, too.

chipx86chipx86

We might want to check explicitly against None here, so that if a caller were to pass in a queryset, …

chipx86chipx86

Here as well. In fact, should this be within the previous else, since we've already handled the empty filediffs issue …

chipx86chipx86

Where does the 8 come from? This might be more self-documenting if we can do math on len(self.filediffs) here. Are …

chipx86chipx86

Same question here.

chipx86chipx86

This seems like a really good opportunity for caching. If we're applying a big stack of patches, it seems likely …

daviddavid

I don't see any reason why we can't just have everything use get_original_file_from_history. It looks like it should do the …

daviddavid
brennie
Review request changed

Change Summary:

Major cleanup and refactor. Unit tests still required for get_original_file_*

Testing Done:

   
  • Ran unit tests.
   
  • Was able to view diffs in the diffviewer that depended on previous
    FileDiffs.
-  
  • TODO: Add tests to cover new behaviour.

Commit:

-384436f155f5278e267ff675728a2286b2ef4579
+ba054b74376bf307453130260248096c06f008d3

Diff:

Revision 2 (+504 -9)

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
chipx86
  1. 
      
  2. reviewboard/diffviewer/diffutils.py (Diff revision 4)
     
     

    Missing trailing period.

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

    What happens if patching fails? We should document this situation more clearly in the docstring.

  4. reviewboard/diffviewer/models/filediff.py (Diff revision 4)
     
     
     

    We probably shouldn't Shortest Middle Snake this.

  5. reviewboard/diffviewer/models/filediff.py (Diff revision 4)
     
     
     

    Grammaro: "that is belongs to a commit"

  6. reviewboard/diffviewer/models/filediff.py (Diff revision 4)
     
     
     
     

    We can avoid the function calls by making this a generator expression instead of using filter.

  7. No need for .spy anymore. You can call directly on the function.

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

    These unit tests were never added.

  9. reviewboard/diffviewer/tests/test_filediff.py (Diff revision 4)
     
     
     
     
     

    Alphabetical order.

  10. No () after the function names in any of these test docstrings.

  11. 
      
brennie
Review request changed

Change Summary:

Added unit tests.
Rebased off /r/10122/

Depends On:

+10122 - Allow empty parent diffs

Commit:

-4eec0c3900299b10aae3d404857020a1e7748245
+44e69f4bd8b341606100337c0cbc30e2ce9fd0f2

Diff:

Revision 5 (+651 -8)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

brennie
brennie
Review request changed

Change Summary:

Simplify get_ancestors() call paths

Commit:

-a7bb09d4314f58b6f0e9ddd32dcaf7d529fc686c
+a9a629f53f65ba452b63b06f1a14eb0097e4ba4c

Diff:

Revision 7 (+620 -41)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

brennie
Review request changed

Change Summary:

Correctly compute original file for non-new filediffs with no ancestors in RRs with history.

Commit:

-a9a629f53f65ba452b63b06f1a14eb0097e4ba4c
+0d41ee1d153bad81928f66de778d36f7eb6dfae0

Diff:

Revision 8 (+624 -41)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

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

    Alphabetical order.

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

    Here, too.

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

    Here, too.

  5. We might want to check explicitly against None here, so that if a caller were to pass in a queryset, we wouldn't be querying unnecessarily.

  6. Here as well.

    In fact, should this be within the previous else, since we've already handled the empty filediffs issue in the first if?

  7. Where does the 8 come from? This might be more self-documenting if we can do math on len(self.filediffs) here. Are there 8 filediffs, or is it len * something?

    I'm partially wondering because I think we can maybe do better on the query count, but I want to better understand the numbers.

  8. Same question here.

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

    This seems like a really good opportunity for caching. If we're applying a big stack of patches, it seems likely that patch N+1 will use patch N as its original file.

    You don't need to add the caching in this change, but can you at least add a TODO comment (and maybe asana task) for it?

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

    I don't see any reason why we can't just have everything use get_original_file_from_history. It looks like it should do the right thing for the legacy case.

  4. 
      
brennie
brennie
david
  1. Ship It!
  2. 
      
brennie
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-4.0.x (590f8ae)
Loading...