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)
Show all issues
```
E127 continuation line over-indented for visual indent
```
reviewboard/diffviewer/diffutils.py (Diff revision 1)
Show all issues
```
E501 line too long (90 > 79 characters)
```
reviewboard/diffviewer/tests/test_diffutils.py (Diff revision 1)
Show all issues
```
E501 line too long (83 > 79 characters)
```
reviewboard/diffviewer/tests/test_diffutils.py (Diff revision 1)
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)

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)

Show all issues

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

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

Change Summary:

Addressed issues.

Commit:

-	5d1dca176a720d93d7a2468ab26b476937774797
+	d8f3828b243e890fd1e651a38dfa490f2f092347

Diff:

Revision 3 (+116 -3)

Show changes

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

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/diffviewer/diffutils.py (Diff revision 3)
Show all issues
```
W503 line break before binary operator
```

Change Summary:

Flake8

Commit:

-	d8f3828b243e890fd1e651a38dfa490f2f092347
+	1de1cb8b86790e93d96705e70472a733bd4d20cc

Diff:

Revision 4 (+116 -3)

Show changes

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

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

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

Show all issues

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

Change Summary:

Addressed Christian's feedback.

Commit:

-	1de1cb8b86790e93d96705e70472a733bd4d20cc
+	d2ed2e7e724e621196001231aa7c3c4ea09f6da4

Diff:

Revision 5 (+116 -3)

Show changes

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

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

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

Show all issues

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

brennie Aug. 28, 2018, 7:20 a.m.

If parent_moved is True then is_parent_diff_empty will also be True.

Change Summary:

Addressed Christian's feedback.

Commit:

-	d2ed2e7e724e621196001231aa7c3c4ea09f6da4
+	2082106b7f627bd42347005dd995f2aac70ae747

Diff:

Revision 6 (+123 -7)

Show changes

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

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/diffviewer/managers.py (Diff revision 6)
Show all issues
```
F821 undefined name 'parent_fiel'
```

Commit:

-	2082106b7f627bd42347005dd995f2aac70ae747
+	397fea1a02668f6a5b6bab598e62403f49175b79

Diff:

Revision 7 (+123 -7)

Show changes

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

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-3.0.x (503b9bf)