Summary

Add better support for added and deleted empty files.

Review Request #5785 — Created May 8, 2014 and submitted May 28, 2014, 2:40 p.m.

Information

Owner

anselina

Repository

Review Board

Branch

master

Bugs

895

Depends On

~~5802~~

Commit

307acd1...

Reviewers

Groups

reviewboard

People

Description

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

Testing Done

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.

Files

Issues

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 …	david	May 9, 2014, 4:15 p.m.
I think this would be easier to read if we rearranged it a little bit: if (empty_change and file_info.origInfo != …	david	May 9, 2014, 4:18 p.m.
It would be nice to mention if this is a deleted empty file.	david	May 9, 2014, 4:36 p.m.
This should go into SVNDiffParser (reviewboard/scmtools/svn/__init.py__) so that this behavior doesn't interfere with diffs from other SCMs.	david	May 12, 2014, 9:21 p.m.
Col: 13 E129 visually indented line with same indent as next logical line	reviewbot	May 16, 2014, 4:45 a.m.
redefinition of unused 'Client' from line 19	reviewbot	May 16, 2014, 4:46 a.m.
Col: 13 E129 visually indented line with same indent as next logical line	reviewbot	May 24, 2014, 10:51 p.m.
redefinition of unused 'Client' from line 19	reviewbot	May 24, 2014, 10:51 p.m.

Change Summary:

Add handling of deleted empty files in Git diffs, and change the background color of new empty file diffs.

Summary:

-	Add better handling of new files with no content in Git diffs.
+	Add better support for added and deleted 0-length files in Git diffs.

Description:

~		New files with no content were not included in the diffs of review requests.
~		This was especially troublesome when we needed to add an empty `__init__.py`
~		file, for example.
	~	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 empty files in review requests for
~		Git only. Specifically, these new empty files will be displayed with the text
	~	This change adds support for including new and deleted empty files in review
	~	requests for Git only. New empty files will be displayed with the text
		"This is an empty file." (see the attached screenshots).

Testing Done:

		I used both rbt post and the "New Review Request" page to create review
		requests with different Git 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 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).
	~	empty file failed. (Since a new empty file is now included, the number of files
	~	from parse() has increased by 1.)

~		After updating the unit tests to reflect this change, all unit tests pass.
	~	After updating the 3 unit tests to reflect this change, all unit tests pass.
	+	The 2 new unit tests that test the deletion of empty files also pass.

Commit:

-	9795eab19eda5f54d5b438a75b6dba0bf9cc66ae
+	a4b74d2e9b03aeb16ceae1cb63f7c53d7245891d

Diff:

Revision 2 (+186 -56)

Show changes

	reviewboard/diffviewer/chunk_generator.py
	reviewboard/scmtools/git.py
	reviewboard/scmtools/tests.py
	reviewboard/static/rb/css/diffviewer.less
	reviewboard/templates/diffviewer/diff_file_fragment.html

Removed Files:

Added Files:

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

The issue has been resolved. Show all issues

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)):

reviewboard/scmtools/git.py (Diff revision 2)

The issue has been resolved. Show all issues

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)):

reviewboard/templates/diffviewer/diff_file_fragment.html (Diff revision 2)

The issue has been resolved. Show all issues

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

anselina May 9, 2014, 5:14 p.m.

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!

Description:

		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 only. New empty files will be displayed with the text
~		"This is an empty file." (see the attached screenshots).
	~	"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).

Commit:

-	a4b74d2e9b03aeb16ceae1cb63f7c53d7245891d
+	d789b37ec82110020d50eaab676a70ebdc4f9b2b

Diff:

Revision 3 (+195 -58)

Show changes

	reviewboard/diffviewer/chunk_generator.py
	reviewboard/scmtools/git.py
	reviewboard/scmtools/tests.py
	reviewboard/static/rb/css/diffviewer.less
	reviewboard/templates/diffviewer/diff_file_fragment.html

Removed Files:

Added Files:

Change Summary:

Add support for SVN diffs.

Summary:

-	Add better support for added and deleted 0-length files in Git diffs.
+	Add better support for added and deleted 0-length files in Git and SVN diffs.

Description:

		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 only. New empty files will be displayed with the text
	~	requests for Git and SVN. 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).

Testing Done:

~		I used both rbt post and the "New Review Request" page to create review
~		requests with different Git diffs:
	~	I used rbt post and the "New Review Request" page to create review
	~	requests with different Git diffs, and rbt post for SVN 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 2 new unit tests that test the deletion of empty files also pass.
	~	The new unit tests also pass.

Depends On:

+	~~5802 - Add extra context for added and deleted empty files.~~

Commit:

-	d789b37ec82110020d50eaab676a70ebdc4f9b2b
+	5c12502d671da25857e41008d57ac5afad81c215

Diff:

Revision 4 (+237 -58)

Show changes

	reviewboard/diffviewer/chunk_generator.py
	reviewboard/diffviewer/parser.py
	reviewboard/scmtools/git.py
	reviewboard/scmtools/tests.py
	reviewboard/static/rb/css/diffviewer.less
	reviewboard/templates/diffviewer/diff_file_fragment.html

Added Files:

reviewboard/diffviewer/parser.py (Diff revision 4)

The issue has been resolved. Show all issues

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

Change Summary:

Move handling of deleted empty files for SVN to SVNDiffParser.

Commit:

-	5c12502d671da25857e41008d57ac5afad81c215
+	f99741861f10bcfcc3ffbc8cd72667e87a2be78a

Diff:

Revision 5 (+237 -58)

Show changes

	reviewboard/diffviewer/chunk_generator.py
	reviewboard/scmtools/git.py
	reviewboard/scmtools/tests.py
	reviewboard/scmtools/svn/__init__.py
	reviewboard/static/rb/css/diffviewer.less
	reviewboard/templates/diffviewer/diff_file_fragment.html

Change Summary:

Add support for Mercurial.

Summary:

-	Add better support for added and deleted 0-length files in Git and SVN diffs.
+	Add better support for added and deleted empty files.

Description:

		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 and SVN. 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).
	~	requests for Git, SVN, and Mercurial. 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).

Testing Done:

		I used rbt post and the "New Review Request" page to create review
~		requests with different Git diffs, and rbt post for SVN diffs:
	~	requests with different Git diffs, and rbt post for SVN and Mercurial 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.

Commit:

-	f99741861f10bcfcc3ffbc8cd72667e87a2be78a
+	1a6be733ce235a593a7512d5d2e6020936dea85c

Diff:

Revision 6 (+242 -58)

Show changes

	reviewboard/diffviewer/chunk_generator.py
	reviewboard/scmtools/git.py
	reviewboard/scmtools/hg.py
	reviewboard/scmtools/tests.py
	reviewboard/scmtools/svn/__init__.py
	reviewboard/static/rb/css/diffviewer.less
	reviewboard/templates/diffviewer/diff_file_fragment.html

Added Files:

Change Summary:

Add unit tests for Mercurial diffs.

Commit:

-	1a6be733ce235a593a7512d5d2e6020936dea85c
+	5d936e43170ca8ddb3a9be8a989075a28345dc45

Diff:

Revision 7 (+274 -58)

Show changes

	reviewboard/diffviewer/chunk_generator.py
	reviewboard/scmtools/git.py
	reviewboard/scmtools/hg.py
	reviewboard/scmtools/tests.py
	reviewboard/scmtools/svn/__init__.py
	reviewboard/static/rb/css/diffviewer.less
	reviewboard/templates/diffviewer/diff_file_fragment.html

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

reviewboard/scmtools/git.py (Diff revision 7)
The issue has been dropped. Show all issues
```
Col: 13
 E129 visually indented line with same indent as next logical line
```

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

reviewboard/scmtools/svn/__init__.py (Diff revision 7)
The issue has been dropped. Show all issues
```
 redefinition of unused 'Client' from line 19
```

Change Summary:

Add support for Perforce.

Description:

		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, and Mercurial. 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).
	~	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).

Testing Done:

		I used rbt post and the "New Review Request" page to create review
~		requests with different Git diffs, and rbt post for SVN and Mercurial diffs:
	~	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.

Added Files:

Change Summary:

Add server capability to indicate support for added/deleted empty files.

Commit:

-	5d936e43170ca8ddb3a9be8a989075a28345dc45
+	307acd1942bb5544b730d0271c96485c569e87d6

Diff:

Revision 8 (+284 -58)

Show changes

	reviewboard/diffviewer/chunk_generator.py
	reviewboard/scmtools/git.py
	reviewboard/scmtools/hg.py
	reviewboard/scmtools/tests.py
	reviewboard/scmtools/svn/__init__.py
	reviewboard/static/rb/css/pages/diffviewer.less
	reviewboard/templates/diffviewer/diff_file_fragment.html
	reviewboard/webapi/server_info.py

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

reviewboard/scmtools/git.py (Diff revision 8)
The issue has been dropped. Show all issues
```
Col: 13
 E129 visually indented line with same indent as next logical line
```

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

reviewboard/scmtools/svn/__init__.py (Diff revision 8)
The issue has been dropped. Show all issues
```
 redefinition of unused 'Client' from line 19
```

Ship it!

This looks good to me. Was there anything else you were planning on adding?

anselina May 27, 2014, 7:58 p.m.
```
Nothing to add here as well.
```

Status: Closed (submitted)

Change Summary:

Pushed to master (836665a)