Summary

Fix display of diffs of files that were renamed in a parent diff

Review Request #4015 — Created March 29, 2013 and discarded July 11, 2014, 4:48 p.m.

Information

Owner

ccaughie

Repository

Review Board

Branch

master

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

Fix display of diffs of files that were renamed in a parent diff

In order for this to work it was necessary to add a new column to the FileDiff
table: parent_file. This stores the name of the file in the base revision,
which may be different from the names of the file in either or both of the
source and destination revisions.

Testing Done

- Posted several diffs with parent diffs that rename files. Scenarios:
1. Rename in parent diff, no rename in review diff
2. Rename in review diff, no rename in parent diff
3. Rename in both parent diff and review diff

- Added unit tests for looking up files with the key scenarios that were failing before.

Issues

Description	From	Last Updated
I know most of the unit tests in this file use camelCase, but we're trying to move toward underscores. Mind …	chipx86	April 9, 2013, 8:07 a.m.
Unit tests must have their docstrings be 1 line, with no newlines.	chipx86	April 9, 2013, 8:07 a.m.
Col: 27 E128 continuation line under-indented for visual indent	reviewbot	April 9, 2013, 7:28 a.m.
Col: 27 E128 continuation line under-indented for visual indent	reviewbot	April 9, 2013, 8:07 a.m.
We try to avoid this format. Mind changing to standard strings?	chipx86	April 9, 2013, 8:07 a.m.
Only one blank line needed.	chipx86	April 9, 2013, 8:07 a.m.
Col: 5 E303 too many blank lines (2)	reviewbot	April 9, 2013, 8:07 a.m.
Col: 27 E128 continuation line under-indented for visual indent	reviewbot	April 9, 2013, 8:07 a.m.
Col: 27 E128 continuation line under-indented for visual indent	reviewbot	April 9, 2013, 8:07 a.m.
Same here.	chipx86	April 9, 2013, 8:07 a.m.
And here.	chipx86	April 9, 2013, 8:07 a.m.
Col: 80 E501 line too long (85 > 79 characters)	reviewbot	April 9, 2013, 8:15 a.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	May 7, 2013, 11:14 a.m.

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/diffviewer/forms.py
    reviewboard/diffviewer/models.py
    reviewboard/diffviewer/tests.py
    reviewboard/diffviewer/diffutils.py
  Ignored Files:

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

The issue has been resolved. Show all issues

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

ccaughie March 29, 2013, 4:15 p.m.

I'm more than happy to fix these, but could use some guidance on how to format this without going over the 80 column limit. Should I move the first argument to the next line and indent all continuation lines by four spaces? Apologies if this is explained on the coding standards page, I couldn't find it.

david April 4, 2013, 11:19 p.m.

Moving the first argument to the next line and indenting by 4 is correct. For example, in this case:

repo = Repository.objects.create(
    name='Test HG',
    path='scmtools/testdata/hg_repo.bundle',
    tool=Tool.objects.get(name='Mercurial'))

reviewboard/diffviewer/tests.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 27
 E128 continuation line under-indented for visual indent
```
reviewboard/diffviewer/tests.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 5
 E303 too many blank lines (2)
```
reviewboard/diffviewer/tests.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 27
 E128 continuation line under-indented for visual indent
```
reviewboard/diffviewer/tests.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 27
 E128 continuation line under-indented for visual indent
```

This is missing an evolution file and Testing Done.

ccaughie April 8, 2013, 9:21 a.m.

I didn't know about evolution files - I did wonder how database upgrade worked, should have read up on it before submitting. I'll try and get time to do this and the reformatting changes in the next few days.

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

The issue has been resolved. Show all issues

I know most of the unit tests in this file use camelCase, but we're trying to move toward underscores. Mind fixing the tests?

reviewboard/diffviewer/tests.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Unit tests must have their docstrings be 1 line, with no newlines.
```
reviewboard/diffviewer/tests.py (Diff revision 1)
The issue has been resolved. Show all issues
```
We try to avoid this format. Mind changing to standard strings?
```
reviewboard/diffviewer/tests.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Only one blank line needed.
```
reviewboard/diffviewer/tests.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Same here.
```
reviewboard/diffviewer/tests.py (Diff revision 1)
The issue has been resolved. Show all issues
```
And here.
```

Testing Done:: +

Posted several diffs with parent diffs that rename files. Scenarios:
1. Rename in parent diff, no rename in review diff
2. Rename in review diff, no rename in parent diff
3. Rename in both parent diff and review diff

+

Added unit tests for looking up files with the key scenarios that were failing before.

Change Summary:

Fixed formatting issues and added evolution file.

Diff:

Revision 2 (+75 -3)

Show changes

	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/forms.py
	reviewboard/diffviewer/models.py
	reviewboard/diffviewer/tests.py
	reviewboard/diffviewer/evolutions/__init__.py
	reviewboard/diffviewer/evolutions/filediff_parent_file.py

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/diffviewer/models.py
    reviewboard/diffviewer/tests.py
    reviewboard/diffviewer/diffutils.py
    reviewboard/diffviewer/evolutions/__init__.py
    reviewboard/diffviewer/forms.py
    reviewboard/diffviewer/evolutions/filediff_parent_file.py
  Ignored Files:

reviewboard/diffviewer/evolutions/filediff_parent_file.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (85 > 79 characters)
```

Diff:

Revision 3 (+76 -3)

Show changes

	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/forms.py
	reviewboard/diffviewer/models.py
	reviewboard/diffviewer/tests.py
	reviewboard/diffviewer/evolutions/__init__.py
	reviewboard/diffviewer/evolutions/filediff_parent_file.py

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/diffviewer/models.py
    reviewboard/diffviewer/tests.py
    reviewboard/diffviewer/diffutils.py
    reviewboard/diffviewer/evolutions/__init__.py
    reviewboard/diffviewer/forms.py
    reviewboard/diffviewer/evolutions/filediff_parent_file.py
  Ignored Files:

ccaughie May 2, 2013, 4:46 p.m.

[bump]

Just wondering if anyone had had a chance to look at this yet? It would really help us to get this fix in.

chipx86 May 4, 2013, 1:37 p.m.

Hi Colin,

I think it looks fine (we'l need to test this more in the wild to make sure it doesn't break more corner cases). However, it doesn't merge anymore. Mind fixing that up? We'll then get it into a release and test it out.

ccaughie May 8, 2013, 9:58 a.m.

Christian -

I suggest holding off on incorporating this patch until https://code.google.com/p/reviewboard/issues/detail?id=2971 is fixed, as that fix may impact this one.

Change Summary:

Merged with current master and uploaded as diff 4.

Diff:

Revision 4 (+76 -4)

Show changes

	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/forms.py
	reviewboard/diffviewer/models.py
	reviewboard/diffviewer/tests.py
	reviewboard/diffviewer/evolutions/__init__.py
	reviewboard/diffviewer/evolutions/filediff_parent_file.py

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/diffviewer/models.py
    reviewboard/diffviewer/tests.py
    reviewboard/diffviewer/diffutils.py
    reviewboard/diffviewer/evolutions/__init__.py
    reviewboard/diffviewer/forms.py
    reviewboard/diffviewer/evolutions/filediff_parent_file.py
  Ignored Files:

reviewboard/diffviewer/diffutils.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (80 > 79 characters)
```

Diff:

Revision 5 (+76 -3)

Show changes

	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/forms.py
	reviewboard/diffviewer/models.py
	reviewboard/diffviewer/tests.py
	reviewboard/diffviewer/evolutions/__init__.py
	reviewboard/diffviewer/evolutions/filediff_parent_file.py

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/diffviewer/models.py
    reviewboard/diffviewer/tests.py
    reviewboard/diffviewer/diffutils.py
    reviewboard/diffviewer/evolutions/__init__.py
    reviewboard/diffviewer/forms.py
    reviewboard/diffviewer/evolutions/filediff_parent_file.py
  Ignored Files:

Hi Colin,

I went to go try out this patch today, but one of our other unit tests weren't passing after this change. This appears to be due to what's being stored in parent_files. My concern is that this change is going to cause other problems, as switching from f.origFile to f.newFile is a pretty significant change in what data is being stored.

ccaughie May 27, 2013, 5:05 p.m.

Can you let me know which unit test failed? When I run them a lot of them get skipped or have errors, presumably due to me not having all of the required dependencies on my system. Of the ones that were able to run, I got the same number of failures before and after the change so I'm assuming that the failure is in one of the ones I couldn't run.

As for the f.origFile to f.newFile change; this is pretty crucial to fixing the issue at hand. Every way I look at it the changed version is correct - but I'm ready to accept that there's a way of looking at it that I'm not seeing. :)

As far as I can tell my patch doesn't change what is being *stored* (at least in the database), it just changes how we match up files in the parent diff with files in the posted diff (excuse my made-up terminology).

Basically we have "files", which is the set of files in the posted diff, and "parent_files", which is the set of files in the parent diff. Each element of both has an "origFile" field, which is the name of the file before the diff, and a "newFile" field, which is the name of the file after the diff.

So if we have a file that is renamed from A to B in the parent diff, and then from B to C in the posted diff, in order to match them up correctly we have to compare parent_file.newFile with posted_file.origFile. The unpatched code compares parent_file.origFile with posted_file.origFile, which is clearly going to fail in this scenario. (Either version of the code will work if the file was not renamed in the parent diff.)

chipx86 June 4, 2013, 6:16 p.m.

Hi Colin,

I believe it was UploadDiffFormTests.test_parent_diff_filtering that was failing.

Thanks for the detailed explanation. I think you're right in that it's the correct solution. Just hoping there's no major corner cases. We'll find out, though :)

Status:: Discarded

	+	Posted several diffs with parent diffs that rename files. Scenarios: 1. Rename in parent diff, no rename in review diff 2. Rename in review diff, no rename in parent diff 3. Rename in both parent diff and review diff
	+	Added unit tests for looking up files with the key scenarios that were failing before.