Compute pre-patch files for FileDiffs in commit histories

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

brennie
Review Board
release-4.0.x
10122
9920, 10123
d1c92e1...
reviewboard

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