Add a better display for patch errors.

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

Information

Review Board
release-3.0.x
79b6177...

Reviewers

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.

  • 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.

Description From Last Updated

'format_html' imported but unused

reviewbotreviewbot

'local_site_reverse' imported but unused

reviewbotreviewbot

local variable 'response' is assigned to but never used

reviewbotreviewbot

Col: 17 E127 continuation line over-indented for visual indent

reviewbotreviewbot

This will also catch, for example, KeyboardInterrupt, RumtimeError and other non-Exceptions that are meant to terminate the program. We should …

brenniebrennie

Put the ) on the following line for symmetry?

brenniebrennie

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

brenniebrennie

Same here.

brenniebrennie

And here.

brenniebrennie

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

chipx86chipx86

I missed this one. except Exception here too.

brenniebrennie

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

chipx86chipx86

Can we move this into the __init__ call?

chipx86chipx86

Swap these.

chipx86chipx86

Swap these.

chipx86chipx86

Is this change related to your work? Raising Http404 actually presents a 404 page, whereas returning HttpResponseNotFound returns an empty …

chipx86chipx86

Can we use local_site_reverse here?

chipx86chipx86

Let's give these names.

chipx86chipx86

Alphabetical.

chipx86chipx86

This can be removed.

chipx86chipx86
reviewbot
  1. 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
    
    
  2. reviewboard/diffviewer/errors.py (Diff revision 1)
     
     
    Show all issues
     'format_html' imported but unused
    
  3. reviewboard/diffviewer/views.py (Diff revision 1)
     
     
    Show all issues
     'local_site_reverse' imported but unused
    
  4. reviewboard/diffviewer/views.py (Diff revision 1)
     
     
    Show all issues
     local variable 'response' is assigned to but never used
    
  5. reviewboard/diffviewer/views.py (Diff revision 1)
     
     
    Show all issues
    Col: 17
     E127 continuation line over-indented for visual indent
    
  6. 
      
david
david
reviewbot
  1. 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
    
    
  2. 
      
brennie
  1. 
      
  2. reviewboard/diffviewer/diffutils.py (Diff revision 2)
     
     
    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.

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

    Put the ) on the following line for symmetry?

  4. reviewboard/diffviewer/views.py (Diff revision 2)
     
     
     
     
     
     
     
     
     
     
    Show all issues

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

  5. reviewboard/diffviewer/views.py (Diff revision 2)
     
     
    Show all issues

    Same here.

  6. reviewboard/diffviewer/views.py (Diff revision 2)
     
     
    Show all issues

    And here.

  7. 
      
david
reviewbot
  1. 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
    
    
  2. 
      
brennie
  1. 
      
  2. reviewboard/diffviewer/diffutils.py (Diff revision 3)
     
     
    Show all issues

    I missed this one. except Exception here too.

  3. 
      
chipx86
  1. 
      
  2. reviewboard/diffviewer/diffutils.py (Diff revision 3)
     
     
     
     
     
     
     
     
    Show all issues

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

  3. reviewboard/diffviewer/diffutils.py (Diff revision 3)
     
     
     
    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.

  4. reviewboard/diffviewer/errors.py (Diff revision 3)
     
     
    Show all issues

    Can we move this into the __init__ call?

  5. reviewboard/diffviewer/views.py (Diff revision 3)
     
     
     
    Show all issues

    Swap these.

  6. reviewboard/diffviewer/views.py (Diff revision 3)
     
     
     
    Show all issues

    Swap these.

  7. reviewboard/diffviewer/views.py (Diff revision 3)
     
     
     
    Show all issues

    Is this change related to your work?

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

  8. reviewboard/diffviewer/views.py (Diff revision 3)
     
     
    Show all issues

    Can we use local_site_reverse here?

    1. 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.

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

  9. reviewboard/reviews/urls.py (Diff revision 3)
     
     
     
     
    Show all issues

    Let's give these names.

  10. reviewboard/static/rb/css/pages/diffviewer.less (Diff revision 3)
     
     
     
     
     
     
    Show all issues

    Alphabetical.

  11. Show all issues

    This can be removed.

  12. 
      
david
reviewbot
  1. 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
    
    
  2. 
      
chipx86
  1. Ship It!
  2. 
      
david
Review request changed
Status:
Completed
Change Summary:
Pushed to release-3.0.x (a9f7e42)