Summary

Allow empty parent diffs

Review Request #10122 — Created Aug. 20, 2018 and submitted Aug. 30, 2018, 1:38 p.m.

Information

Owner

brennie

Repository

Review Board

Branch

release-3.0.x

Bugs

Depends On

Blocks

10119

Commit

397fea1...

Reviewers

Groups

reviewboard

People

Description

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.

Testing Done


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.

Issues

Description	From	Last Updated
E127 continuation line over-indented for visual indent	reviewbot	Aug. 20, 2018, 4:59 p.m.
E501 line too long (90 > 79 characters)	reviewbot	Aug. 20, 2018, 5:08 p.m.
E501 line too long (83 > 79 characters)	reviewbot	Aug. 20, 2018, 4:58 p.m.
W391 blank line at end of file	reviewbot	Aug. 20, 2018, 5:08 p.m.
Can you move the parens to be around the string instead? Same effect, but keeps the string nicely grouped together.	chipx86	Aug. 20, 2018, 10:33 p.m.
I think this would all be better as a method on FileDiff.	chipx86	Aug. 20, 2018, 10:33 p.m.
No parens around these equality checks.	chipx86	Aug. 20, 2018, 10:33 p.m.
Blank line between these.	chipx86	Aug. 20, 2018, 10:33 p.m.
Missing a test docstring.	chipx86	Aug. 20, 2018, 10:35 p.m.
Can you pass these as keyword arguments?	chipx86	Aug. 20, 2018, 10:35 p.m.
Can you add a comment saying why we're doing this again?	chipx86	Aug. 20, 2018, 10:35 p.m.
Here, too.	chipx86	Aug. 20, 2018, 10:35 p.m.
W503 line break before binary operator	reviewbot	Aug. 20, 2018, 10:51 p.m.
Did you mean for the next clause to be outside of these parens, or should the parens go away?	chipx86	Aug. 23, 2018, 4:30 p.m.
Given the previous issue, I think we need better test coverage for the various cases here.	chipx86	Aug. 28, 2018, 7:20 a.m.
F821 undefined name 'parent_fiel'	reviewbot	Aug. 28, 2018, 1:28 p.m.

flake8 failed.

JSHint passed.

flake8

reviewboard/diffviewer/diffutils.py (Diff revision 1)
The issue has been resolved. Show all issues
```
E127 continuation line over-indented for visual indent
```
reviewboard/diffviewer/diffutils.py (Diff revision 1)
The issue has been resolved. Show all issues
```
E501 line too long (90 > 79 characters)
```
reviewboard/diffviewer/tests/test_diffutils.py (Diff revision 1)
The issue has been resolved. Show all issues
```
E501 line too long (83 > 79 characters)
```
reviewboard/diffviewer/tests/test_diffutils.py (Diff revision 1)
The issue has been resolved. Show all issues
```
W391 blank line at end of file
```

Commit:

5f0986b32f9f5d0c959c36a953ae9a48ca3734a9

5d1dca176a720d93d7a2468ab26b476937774797

Diff:

Revision 2 (+76 -3)

Show changes

	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/tests/test_diffutils.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

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

The issue has been resolved. Show all issues

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

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

The issue has been resolved. Show all issues

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

reviewboard/diffviewer/diffutils.py (Diff revision 2)
The issue has been resolved. Show all issues
```
No parens around these equality checks.
```
reviewboard/diffviewer/diffutils.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Blank line between these.
```
reviewboard/diffviewer/tests/test_diffutils.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Missing a test docstring.
```
reviewboard/diffviewer/tests/test_diffutils.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Can you pass these as keyword arguments?
```
reviewboard/diffviewer/tests/test_diffutils.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Can you add a comment saying why we're doing this again?
```
reviewboard/diffviewer/tests/test_diffutils.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Here, too.
```
1. chipx86 Aug. 20, 2018, 9:08 p.m.
  That is, keyword arguments.