Add better support for added and deleted empty files.

Review Request #5785 — Created May 8, 2014 and submitted

anselina
Review Board
master
895
5802
307acd1...
reviewboard

The addition and deletion of 0-length files were not included in the diffs of
review requests. This was especially troublesome when we needed to add or
delete an empty __init__.py file, for example.

This change adds support for including new and deleted empty files in review
requests for Git, SVN, Mercurial, and Perforce. New empty files will be
displayed with the text "This is an empty file.", and deleted empty files will
be displayed with the text "This empty file was deleted. The content cannot be
displayed." (see the attached screenshots).

I used rbt post and the "New Review Request" page to create review
requests with different Git diffs, and rbt post for SVN, Mercurial, and
Perforce diffs:
- Diff with 1 new empty file
- Diff with 2 new empty files
- Diff with 1 new empty file, 1 new non-empty file
- Diff with 1 new empty file, 2 modified files
- Diff with 1 deleted empty file
- Diff with 1 deleted empty file, 1 modified file
- Diff with 1 deleted empty file, 1 deleted non-empty file, 1 modified file

After this change, the 3 unit tests that tested parsing a Git diff with a new
empty file failed. (Since a new empty file is now included, the number of files
from parse() has increased by 1.)

After updating the 3 unit tests to reflect this change, all unit tests pass.
The new unit tests also pass.


Description From Last Updated

Can we re-indent this a bit? if (self.filediff.binary or self.filediff.deleted or self.filediff.source_revision == '' or (self.filediff.is_new and counts['insert_count'] == 0 ...

daviddavid

I think this would be easier to read if we rearranged it a little bit: if (empty_change and file_info.origInfo != ...

daviddavid

It would be nice to mention if this is a deleted empty file.

daviddavid

This should go into SVNDiffParser (reviewboard/scmtools/svn/__init.py__) so that this behavior doesn't interfere with diffs from other SCMs.

daviddavid

Col: 13 E129 visually indented line with same indent as next logical line

reviewbotreviewbot

redefinition of unused 'Client' from line 19

reviewbotreviewbot

Col: 13 E129 visually indented line with same indent as next logical line

reviewbotreviewbot

redefinition of unused 'Client' from line 19

reviewbotreviewbot
anselina
david
  1. 
      
  2. reviewboard/diffviewer/chunk_generator.py (Diff revision 2)
     
     
     
     
     
     

    Can we re-indent this a bit?

    if (self.filediff.binary or
        self.filediff.deleted or
        self.filediff.source_revision == '' or
        (self.filediff.is_new and
         counts['insert_count'] == 0 and
         counts['delete_count'] == 0)):
    
  3. reviewboard/scmtools/git.py (Diff revision 2)
     
     
     
     

    I think this would be easier to read if we rearranged it a little bit:

    if (empty_change and
        file_info.origInfo != PRE_CREATION and
        not (file_info.moved or file_info.copied or file_info.deleted)):
    
  4. It would be nice to mention if this is a deleted empty file.

    1. My latest revision does this now, but it requires that we get chunks for any deleted file unless it is an empty file (whereas previously, we did not get chunks for all deleted files). Just pointing this out in case this is too expensive!

  5. 
      
anselina
anselina
david
  1. 
      
  2. reviewboard/diffviewer/parser.py (Diff revision 4)
     
     
     
     

    This should go into SVNDiffParser (reviewboard/scmtools/svn/__init.py__) so that this behavior doesn't interfere with diffs from other SCMs.

  3. 
      
anselina
anselina
anselina
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/scmtools/hg.py
        reviewboard/scmtools/tests.py
        reviewboard/scmtools/git.py
        reviewboard/diffviewer/chunk_generator.py
        reviewboard/scmtools/svn/__init__.py
      Ignored Files:
        reviewboard/static/rb/css/diffviewer.less
        reviewboard/templates/diffviewer/diff_file_fragment.html
    
    
  2. reviewboard/scmtools/git.py (Diff revision 7)
     
     
    Col: 13
     E129 visually indented line with same indent as next logical line
    
  3. 
      
reviewbot
  1. This is a review from Review Bot.
      Tool: Pyflakes
      Processed Files:
        reviewboard/scmtools/hg.py
        reviewboard/scmtools/tests.py
        reviewboard/scmtools/git.py
        reviewboard/diffviewer/chunk_generator.py
        reviewboard/scmtools/svn/__init__.py
      Ignored Files:
        reviewboard/static/rb/css/diffviewer.less
        reviewboard/templates/diffviewer/diff_file_fragment.html
    
    
  2. reviewboard/scmtools/svn/__init__.py (Diff revision 7)
     
     
     redefinition of unused 'Client' from line 19
    
  3. 
      
anselina
anselina
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/diffviewer/chunk_generator.py
        reviewboard/webapi/server_info.py
        reviewboard/scmtools/hg.py
        reviewboard/scmtools/tests.py
        reviewboard/scmtools/svn/__init__.py
        reviewboard/scmtools/git.py
      Ignored Files:
        reviewboard/templates/diffviewer/diff_file_fragment.html
        reviewboard/static/rb/css/pages/diffviewer.less
    
    
  2. reviewboard/scmtools/git.py (Diff revision 8)
     
     
    Col: 13
     E129 visually indented line with same indent as next logical line
    
  3. 
      
reviewbot
  1. This is a review from Review Bot.
      Tool: Pyflakes
      Processed Files:
        reviewboard/diffviewer/chunk_generator.py
        reviewboard/webapi/server_info.py
        reviewboard/scmtools/hg.py
        reviewboard/scmtools/tests.py
        reviewboard/scmtools/svn/__init__.py
        reviewboard/scmtools/git.py
      Ignored Files:
        reviewboard/templates/diffviewer/diff_file_fragment.html
        reviewboard/static/rb/css/pages/diffviewer.less
    
    
  2. reviewboard/scmtools/svn/__init__.py (Diff revision 8)
     
     
     redefinition of unused 'Client' from line 19
    
  3. 
      
david
  1. This looks good to me. Was there anything else you were planning on adding?

    1. Nothing to add here as well.

  2. 
      
anselina
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to master (836665a)
Loading...