Fix "List index out of range" errors with interdiffing deleted files.

Review Request #6473 — Created Oct. 20, 2014 and submitted

Information

Review Board
release-2.0.x
62eb033...

Reviewers

It's possible to hit an IndexError when processing an interdiff between two
changes that delete a file. The wrinkle here is that between the two revisions
of the diff, some additional changes to that file were made (and synced to the
local developer's tree).

The result of this is that we didn't filter out the file from the list of
things to show for the interdiff, and then things went wrong when we tried to
process the opcodes for diffing two empty strings.

While I was doing this, I saw some warnings about a unicode-unaware comparison
occurring, so I've slipped the fix for that in as well. This doesn't actually
have any correctness implications, it just speeds up things slightly when
dealing with empty files.

  • Created a reproduction case for the bug and tested before and after the
    change. Before, I saw an entry for this file with the error inside. After,
    the deleted files don't show up at all in the interdiff.
  • Ran unit tests.
Description From Last Updated

Col: 17 E201 whitespace after '('

reviewbotreviewbot

Every Python linter complains about this format, and it's just not common in Python. Can we go back to the …

chipx86chipx86

Col: 17 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 17 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 22 E201 whitespace after '('

reviewbotreviewbot

Col: 22 E128 continuation line under-indented for visual indent

reviewbotreviewbot

What happens for legitimately empty files? Do we want to do this just for interdiffs? Also, can we add a …

chipx86chipx86
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/diffviewer/diffutils.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/diffviewer/diffutils.py
    
    
  2. reviewboard/diffviewer/diffutils.py (Diff revision 1)
     
     
    Show all issues
    Col: 17
     E201 whitespace after '('
    
  3. reviewboard/diffviewer/diffutils.py (Diff revision 1)
     
     
    Show all issues
    Col: 17
     E128 continuation line under-indented for visual indent
    
  4. reviewboard/diffviewer/diffutils.py (Diff revision 1)
     
     
    Show all issues
    Col: 17
     E128 continuation line under-indented for visual indent
    
  5. reviewboard/diffviewer/diffutils.py (Diff revision 1)
     
     
    Show all issues
    Col: 22
     E201 whitespace after '('
    
  6. reviewboard/diffviewer/diffutils.py (Diff revision 1)
     
     
    Show all issues
    Col: 22
     E128 continuation line under-indented for visual indent
    
  7. 
      
chipx86
  1. 
      
  2. reviewboard/diffviewer/diffutils.py (Diff revision 1)
     
     
     
     
     
    Show all issues

    Every Python linter complains about this format, and it's just not common in Python. Can we go back to the format we use everywhere else?

  3. 
      
david
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/diffviewer/diffutils.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/diffviewer/diffutils.py
    
    
  2. 
      
chipx86
  1. 
      
  2. reviewboard/diffviewer/diffutils.py (Diff revision 2)
     
     

    Do we want/need to compare against b'\r' here, or is this sufficient?

  3. 
      
david
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/diffviewer/chunk_generator.py
        reviewboard/diffviewer/diffutils.py
    
    
  2. 
      
chipx86
  1. 
      
  2. reviewboard/diffviewer/chunk_generator.py (Diff revision 3)
     
     
     
    Show all issues

    What happens for legitimately empty files?

    Do we want to do this just for interdiffs?

    Also, can we add a comment here detailing why we're skipping?

    1. Adding or deleting legitimately empty files won't end up with any opcodes to process, so it won't get here. I'll add a comment.

  3. 
      
david
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/diffviewer/chunk_generator.py
        reviewboard/diffviewer/diffutils.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/diffviewer/chunk_generator.py
        reviewboard/diffviewer/diffutils.py
    
    
  2. 
      
david
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/diffviewer/diffutils.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/diffviewer/diffutils.py
    
    
  2. 
      
chipx86
  1. Ship It!

  2. 
      
david
Review request changed
Status:
Completed
Change Summary:
Pushed to release-2.0.x (2b23541)