Fix diff fragment header expansion issues.

Review Request #6889 — Created Feb. 2, 2015 and submitted

brennie
Review Board
master
6a71060...
reviewboard

Previously diff fragments would sometimes expand to the incorrect
place. This is beacuse the generated header information was using
original/patched line numbers instead of the virtual (unified original
and patched) line numbers. Header generation has been modified to use
these line numbers and the issue has been fixed.

Headers also used to sometimes select headers to expand to. The code
that searched for headers wasn't taking into account the difference
between the original/patched line numbers, which are what the headers
use, and the virtual line numbers. This would happen if a deletion or
insertion chunk was above the flagged comment lines.

Update the diff fragment template to allow the left header (which
corresponds to the original file) to be expanded to.

Remove dead code for generation headers from the
get_file_chunks_in_range function. The logic for header generation
wasn't quite right and the only caller of the function is using better
logic to find header information.

Add a unit test to test that the line numbers generated by the new
get_last_header_before_line function are both before the requested
line and have the correct unified line number.

Ran unit tests.

Manually tested the diff fragments with lines flagged that had deletes
and inserts above them. They expanded to the proper locations.

Tested expansion of the left-side of the fragment. It was expandable
and expanded to the correct position.

  • 0
  • 0
  • 5
  • 0
  • 5
Description From Last Updated
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/templatetags/reviewtags.py
        reviewboard/diffviewer/diffutils.py
    
    Ignored Files:
        reviewboard/templates/reviews/diff_comment_fragment.html
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/templatetags/reviewtags.py
        reviewboard/diffviewer/diffutils.py
    
    Ignored Files:
        reviewboard/templates/reviews/diff_comment_fragment.html
    
    
  2. 
      
brennie
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/templatetags/reviewtags.py
        reviewboard/diffviewer/diffutils.py
    
    Ignored Files:
        reviewboard/templates/reviews/diff_comment_fragment.html
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/templatetags/reviewtags.py
        reviewboard/diffviewer/diffutils.py
    
    Ignored Files:
        reviewboard/templates/reviews/diff_comment_fragment.html
    
    
  2. 
      
david
  1. Looks good, pending your investigation of the bug in your testing done.

  2. 
      
brennie
chipx86
  1. Can you craft a test case reproducing your original problem?

    Does meta no longer have header information?

    1. The chunk 'meta' field still has 'left_header' and 'right_header entries' (if they were generated).

      The header information generation was removed from get_file_chunks_in_range because the information wasn't useful there. The header information was only being generated for chunks that were in the range specified. It also generated information for all those chunks, whereas we really only need the information for chunks we are going to expand into (i.e. the previous chunks). The function also did not take into account the difference between original/modified line numbers (which are what the 'meta' field uses) and unified line numbers (what the diffviewer and diff fragment viewer use).

      This behaviour has been corrected and put into get_last_header_before_line.

    2. A unit test has been added.

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

    One entry per line, like:

    header = {
        'left': None,
        'right': None,
    }
    
  3. reviewboard/diffviewer/diffutils.py (Diff revision 2)
     
     

    We use lines[0][0] several times, so let's pull it out into a variable.

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

    The second line's indentation should match the 'right_headers' on the previous line.

  5. 
      
brennie
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/templatetags/reviewtags.py
        reviewboard/diffviewer/tests.py
        reviewboard/diffviewer/diffutils.py
    
    Ignored Files:
        reviewboard/templates/reviews/diff_comment_fragment.html
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/templatetags/reviewtags.py
        reviewboard/diffviewer/tests.py
        reviewboard/diffviewer/diffutils.py
    
    Ignored Files:
        reviewboard/templates/reviews/diff_comment_fragment.html
    
    
  2. 
      
brennie
david
  1. 
      
  2. reviewboard/diffviewer/tests.py (Diff revision 3)
     
     
     

    Can you add a blank line here?

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

    Can you add a blank line here?

  4. 
      
brennie
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to master (567f659)
Loading...