Allow empty parent diffs

Review Request #10122 — Created Aug. 20, 2018 and submitted

brennie
Review Board
release-3.0.x
10119
397fea1...
reviewboard

If a review request was uploaded with a FileDiff that contained a
parent diff that creates a 0 byte file, we would previously be unable to
generate the pre-patch file due to a limitation of patch(1). To work
around this, when we encounter a specific error from patch (that is,
when it only finds what it considers garbage in the input), we check to
see if the parent diff is empty and if so we return an empty buffer.
Future calls will get_original_file for that file will have no extra
database query overhead as the results of this (which costs at least two
queries) will be cached with the FileDiff.

  • Ran unit tests.
  • Uploaded a commit that contained a FileDiff that created a 0 byte
    file. With this patch applied, I was able to view the diff.
  • 0
  • 0
  • 16
  • 0
  • 16
Description From Last Updated
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

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

    Can you move the parens to be around the string instead? Same effect, but keeps the string nicely grouped together.

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

    I think this would all be better as a method on FileDiff.

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

    No parens around these equality checks.

  5. reviewboard/diffviewer/diffutils.py (Diff revision 2)
     
     
     

    Blank line between these.

  6. Missing a test docstring.

  7. Can you pass these as keyword arguments?

  8. Can you add a comment saying why we're doing this again?

    1. That is, keyword arguments.

  9. 
      
brennie
Review request changed

Change Summary:

Addressed issues.

Commit:

-5d1dca176a720d93d7a2468ab26b476937774797
+d8f3828b243e890fd1e651a38dfa490f2f092347

Diff:

Revision 3 (+116 -3)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

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

    Did you mean for the next clause to be outside of these parens, or should the parens go away?

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

    Given the previous issue, I think we need better test coverage for the various cases here.

    1. If parent_moved is True then is_parent_diff_empty will also be True.

  3. 
      
brennie
Review request changed

Change Summary:

Addressed Christian's feedback.

Commit:

-d2ed2e7e724e621196001231aa7c3c4ea09f6da4
+2082106b7f627bd42347005dd995f2aac70ae747

Diff:

Revision 6 (+123 -7)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

brennie
chipx86
  1. Ship It!
  2. 
      
brennie
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-3.0.x (503b9bf)
Loading...