Summary

Add a better display for patch errors.

Review Request #8563 — Created Dec. 6, 2016 and submitted Dec. 16, 2016, noon

Information

Owner

david

Repository

Review Board

Branch

release-3.0.x

Bugs

Depends On

Commit

79b6177...

Reviewers

Groups

reviewboard

People

Description

When a file fails to patch, we would show a pretty terrible error message. It
would give the location of the temp directory with the original and rejects
files, and then theoretically show the error output from the patch command
(although in the case where hunks couldn't be applied, that error output
wouldn't be shown because it's displayed to stdout instead of stderr).

This replaces that with a new display that actually shows the patch output
(with temporary directory names filtered out), and instead of forcing the
administrator to dig through that directory, allows the user to either show the
rejects inline on the page or download a .zip bundle containing the original
file, the new file, the diff, and the rejects.

Based on work by Tien Vu.

Testing Done


Uploaded a hand-modified patch that would fail to apply cleanly. Saw the new

  output. Viewed rejects inline and checked the contents of the downloaded

  error bundle.
Ran unit tests.

Files

Issues

Description	From	Last Updated
'format_html' imported but unused	reviewbot	Dec. 6, 2016, 4:06 p.m.
'local_site_reverse' imported but unused	reviewbot	Dec. 6, 2016, 4:06 p.m.
local variable 'response' is assigned to but never used	reviewbot	Dec. 6, 2016, 4:07 p.m.
Col: 17 E127 continuation line over-indented for visual indent	reviewbot	Dec. 6, 2016, 4:07 p.m.
This will also catch, for example, KeyboardInterrupt, RumtimeError and other non-Exceptions that are meant to terminate the program. We should …	brennie	Dec. 13, 2016, 3:14 p.m.
Put the ) on the following line for symmetry?	brennie	Dec. 13, 2016, 3:14 p.m.
Can you put args starting on the next line indented by 4?	brennie	Dec. 13, 2016, 3:15 p.m.
Same here.	brennie	Dec. 13, 2016, 3:16 p.m.
And here.	brennie	Dec. 13, 2016, 3:16 p.m.
Can we add Args, Returns, and Raises while here?	chipx86	Dec. 13, 2016, 3:52 p.m.
I missed this one. except Exception here too.	brennie	Dec. 13, 2016, 3:24 p.m.
In the try/except above, we set new_file = None in the except. For rejects, we're setting it before-hand. Let's move …	chipx86	Dec. 13, 2016, 3:53 p.m.
Can we move this into the __init__ call?	chipx86	Dec. 13, 2016, 3:53 p.m.
Swap these.	chipx86	Dec. 13, 2016, 3:54 p.m.
Swap these.	chipx86	Dec. 13, 2016, 3:54 p.m.
Is this change related to your work? Raising Http404 actually presents a 404 page, whereas returning HttpResponseNotFound returns an empty …	chipx86	Dec. 13, 2016, 3:54 p.m.
Can we use local_site_reverse here?	chipx86	Dec. 13, 2016, 4:15 p.m.
Let's give these names.	chipx86	Dec. 13, 2016, 4:14 p.m.
Alphabetical.	chipx86	Dec. 13, 2016, 4:15 p.m.
This can be removed.	chipx86	Dec. 13, 2016, 4:15 p.m.

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/views.py
    reviewboard/webapi/resources/base_patched_file.py
    reviewboard/reviews/urls.py
    reviewboard/diffviewer/errors.py
    reviewboard/diffviewer/views.py
    reviewboard/diffviewer/diffutils.py

Ignored Files:
    reviewboard/templates/diffviewer/diff_fragment_patch_error.html
    reviewboard/templates/diffviewer/diff_fragment_error.html
    reviewboard/templates/diffviewer/diff_fragment_error_base.html
    reviewboard/static/rb/css/pages/diffviewer.less



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/views.py
    reviewboard/webapi/resources/base_patched_file.py
    reviewboard/reviews/urls.py
    reviewboard/diffviewer/errors.py
    reviewboard/diffviewer/views.py
    reviewboard/diffviewer/diffutils.py

Ignored Files:
    reviewboard/templates/diffviewer/diff_fragment_patch_error.html
    reviewboard/templates/diffviewer/diff_fragment_error.html
    reviewboard/templates/diffviewer/diff_fragment_error_base.html
    reviewboard/static/rb/css/pages/diffviewer.less

reviewboard/diffviewer/errors.py (Diff revision 1)
The issue has been resolved. Show all issues
```
 'format_html' imported but unused
```
reviewboard/diffviewer/views.py (Diff revision 1)
The issue has been resolved. Show all issues
```
 'local_site_reverse' imported but unused
```
reviewboard/diffviewer/views.py (Diff revision 1)
The issue has been resolved. Show all issues
```
 local variable 'response' is assigned to but never used
```
reviewboard/diffviewer/views.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 17
 E127 continuation line over-indented for visual indent
```

Change Summary:: Credit where credit is due.
Description:: When a file fails to patch, we would show a pretty terrible error message. It
would give the location of the temp directory with the original and rejects
files, and then theoretically show the error output from the patch command
(although in the case where hunks couldn't be applied, that error output
wouldn't be shown because it's displayed to stdout instead of stderr).

This replaces that with a new display that actually shows the patch output
(with temporary directory names filtered out), and instead of forcing the
administrator to dig through that directory, allows the user to either show the
rejects inline on the page or download a .zip bundle containing the original
file, the new file, the diff, and the rejects.
+
+
Based on work by Tien Vu.

Commit:

19d22d1ae987ad35120fd29ba7bd69c8e849f9ee

79366c1e9be6f60d67ad978b6bafa40359472c83

Diff:

Revision 2 (+457 -152)

Show changes

	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/errors.py
	reviewboard/diffviewer/views.py
	reviewboard/reviews/urls.py
	reviewboard/reviews/views.py
	reviewboard/static/rb/css/pages/diffviewer.less
	reviewboard/templates/diffviewer/diff_fragment_error.html
	reviewboard/templates/diffviewer/diff_fragment_error_base.html
	2 more

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/views.py
    reviewboard/webapi/resources/base_patched_file.py
    reviewboard/reviews/urls.py
    reviewboard/diffviewer/errors.py
    reviewboard/diffviewer/views.py
    reviewboard/diffviewer/diffutils.py

Ignored Files:
    reviewboard/templates/diffviewer/diff_fragment_patch_error.html
    reviewboard/templates/diffviewer/diff_fragment_error.html
    reviewboard/templates/diffviewer/diff_fragment_error_base.html
    reviewboard/static/rb/css/pages/diffviewer.less



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/views.py
    reviewboard/webapi/resources/base_patched_file.py
    reviewboard/reviews/urls.py
    reviewboard/diffviewer/errors.py
    reviewboard/diffviewer/views.py
    reviewboard/diffviewer/diffutils.py

Ignored Files:
    reviewboard/templates/diffviewer/diff_fragment_patch_error.html
    reviewboard/templates/diffviewer/diff_fragment_error.html
    reviewboard/templates/diffviewer/diff_fragment_error_base.html
    reviewboard/static/rb/css/pages/diffviewer.less

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

The issue has been resolved. Show all issues

This will also catch, for example, KeyboardInterrupt, RumtimeError and other non-Exceptions that are meant to terminate the program. We should be more specific here or just catch Exception.

reviewboard/diffviewer/diffutils.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Put the ) on the following line for symmetry?
```

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

The issue has been resolved. Show all issues

Can you put args starting on the next line indented by 4?

reviewboard/diffviewer/views.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Same here.
```
reviewboard/diffviewer/views.py (Diff revision 2)
The issue has been resolved. Show all issues
```
And here.
```

Commit:

79366c1e9be6f60d67ad978b6bafa40359472c83

c659ea072c4b500db373d3c0e29b1d987ee11c34

Diff:

Revision 3 (+463 -157)

Show changes

	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/errors.py
	reviewboard/diffviewer/views.py
	reviewboard/reviews/urls.py
	reviewboard/reviews/views.py
	reviewboard/static/rb/css/pages/diffviewer.less
	reviewboard/templates/diffviewer/diff_fragment_error.html
	reviewboard/templates/diffviewer/diff_fragment_error_base.html
	2 more

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/views.py
    reviewboard/webapi/resources/base_patched_file.py
    reviewboard/reviews/urls.py
    reviewboard/diffviewer/errors.py
    reviewboard/diffviewer/views.py
    reviewboard/diffviewer/diffutils.py

Ignored Files:
    reviewboard/templates/diffviewer/diff_fragment_patch_error.html
    reviewboard/templates/diffviewer/diff_fragment_error.html
    reviewboard/templates/diffviewer/diff_fragment_error_base.html
    reviewboard/static/rb/css/pages/diffviewer.less



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/views.py
    reviewboard/webapi/resources/base_patched_file.py
    reviewboard/reviews/urls.py
    reviewboard/diffviewer/errors.py
    reviewboard/diffviewer/views.py
    reviewboard/diffviewer/diffutils.py

Ignored Files:
    reviewboard/templates/diffviewer/diff_fragment_patch_error.html
    reviewboard/templates/diffviewer/diff_fragment_error.html
    reviewboard/templates/diffviewer/diff_fragment_error_base.html
    reviewboard/static/rb/css/pages/diffviewer.less

Ship it!

reviewboard/diffviewer/diffutils.py (Diff revision 3)
The issue has been resolved. Show all issues
```
I missed this one. except Exception here too.
```

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

The issue has been resolved. Show all issues

Can we add Args, Returns, and Raises while here?

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

The issue has been resolved. Show all issues

In the try/except above, we set new_file = None in the except. For rejects, we're setting it before-hand. Let's move it into the except here.

reviewboard/diffviewer/errors.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Can we move this into the __init__ call?
```
reviewboard/diffviewer/views.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Swap these.
```
reviewboard/diffviewer/views.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Swap these.
```

reviewboard/diffviewer/views.py (Diff revision 3)

The issue has been resolved. Show all issues

Is this change related to your work?

Raising Http404 actually presents a 404 page, whereas returning HttpResponseNotFound returns an empty 404.

reviewboard/diffviewer/views.py (Diff revision 3)

The issue has been resolved. Show all issues

Can we use local_site_reverse here?

david Dec. 13, 2016, 4:15 p.m.

I banged my head against that for days before going this route. I did get it to work this time but it's a little ugly.

chipx86 Dec. 15, 2016, 3:02 p.m.

Ah, I wasn't thinking about how complicated that would be. Thanks for figuring it out :)

reviewboard/reviews/urls.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Let's give these names.
```

reviewboard/static/rb/css/pages/diffviewer.less (Diff revision 3)

The issue has been resolved. Show all issues

Alphabetical.

reviewboard/static/rb/css/pages/diffviewer.less (Diff revision 3)
The issue has been resolved. Show all issues
```
This can be removed.
```

Commit:

c659ea072c4b500db373d3c0e29b1d987ee11c34

79b61777eff73691fcbdb6693370f851a7b75893

Diff:

Revision 4 (+491 -158)

Show changes

	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/errors.py
	reviewboard/diffviewer/views.py
	reviewboard/reviews/urls.py
	reviewboard/reviews/views.py
	reviewboard/static/rb/css/pages/diffviewer.less
	reviewboard/templates/diffviewer/diff_fragment_error.html
	reviewboard/templates/diffviewer/diff_fragment_error_base.html
	2 more

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/views.py
    reviewboard/webapi/resources/base_patched_file.py
    reviewboard/reviews/urls.py
    reviewboard/diffviewer/errors.py
    reviewboard/diffviewer/views.py
    reviewboard/diffviewer/diffutils.py

Ignored Files:
    reviewboard/templates/diffviewer/diff_fragment_patch_error.html
    reviewboard/templates/diffviewer/diff_fragment_error.html
    reviewboard/templates/diffviewer/diff_fragment_error_base.html
    reviewboard/static/rb/css/pages/diffviewer.less



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/views.py
    reviewboard/webapi/resources/base_patched_file.py
    reviewboard/reviews/urls.py
    reviewboard/diffviewer/errors.py
    reviewboard/diffviewer/views.py
    reviewboard/diffviewer/diffutils.py

Ignored Files:
    reviewboard/templates/diffviewer/diff_fragment_patch_error.html
    reviewboard/templates/diffviewer/diff_fragment_error.html
    reviewboard/templates/diffviewer/diff_fragment_error_base.html
    reviewboard/static/rb/css/pages/diffviewer.less

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to release-3.0.x (a9f7e42)

		When a file fails to patch, we would show a pretty terrible error message. It
		would give the location of the temp directory with the original and rejects
		files, and then theoretically show the error output from the `patch` command
		(although in the case where hunks couldn't be applied, that error output
		wouldn't be shown because it's displayed to stdout instead of stderr).

		This replaces that with a new display that actually shows the patch output
		(with temporary directory names filtered out), and instead of forcing the
		administrator to dig through that directory, allows the user to either show the
		rejects inline on the page or download a .zip bundle containing the original
		file, the new file, the diff, and the rejects.
	+
	+	Based on work by Tien Vu.