Add a better display for patch errors.

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

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

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.
Loading file attachments...

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)
     
     
     'format_html' imported but unused
    
  3. reviewboard/diffviewer/views.py (Diff revision 1)
     
     
     'local_site_reverse' imported but unused
    
  4. reviewboard/diffviewer/views.py (Diff revision 1)
     
     
     local variable 'response' is assigned to but never used
    
  5. reviewboard/diffviewer/views.py (Diff revision 1)
     
     
    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)
     
     

    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)
     
     

    Put the ) on the following line for symmetry?

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

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

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

    Same here.

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

    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)
     
     

    I missed this one. except Exception here too.

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

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

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

    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)
     
     

    Can we move this into the __init__ call?

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

    Swap these.

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

    Swap these.

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

    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)
     
     

    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)
     
     
     
     

    Let's give these names.

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

    Alphabetical.

  11. 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: Closed (submitted)

Change Summary:

Pushed to release-3.0.x (a9f7e42)
Loading...