Summary

Show squashed diffs in the diffviewer

Review Request #7135 — Created March 28, 2015 and submitted May 19, 2015, 7:12 p.m.

Information

Owner

brennie

Repository

Review Board

Branch

dvcs

Bugs

Depends On

Commit

af9f908...

Reviewers

Groups

reviewboard

People

Description

Previously we would show multiple FileDiffs for the same file in the
diff viewer if the file was modified in multiple commits. Now we
determine what FileDiffs are ancestors of other FileDiffs in the
commit history and filter out all the ancestors. The result is a
squashed diff between the original file and the resulting file.

Testing Done

Ran unit tests.

Manually verified that the squashed diffs were correct.

Still able to view the FileDiffs of review requests without
commit history.

Issues

Description	From	Last Updated
Col: 12 E111 indentation is not a multiple of four	reviewbot	May 4, 2015, 2:36 p.m.
We should probably just import logging at the top of the file.	david	May 7, 2015, 1:58 p.m.
This should use exc_info=True (we have =1 elsewhere, but I'm slowly cleaning that up).	david	May 7, 2015, 1:58 p.m.
I think I liked your approach up above of changing all of these to add the - and the beginning …	david	May 7, 2015, 1:59 p.m.
Unindent.	brennie	May 8, 2015, 11:49 p.m.
"In this case" twice is awkward phrasing.	brennie	May 8, 2015, 11:50 p.m.
diffviewer.commitutils before diffviewer.differ	david	May 11, 2015, 4:06 p.m.
Blank line after this.	david	May 11, 2015, 4:07 p.m.
This should also have a '-'	david	May 11, 2015, 4:07 p.m.
Can you maybe describe this option in the docstring above?	chipx86	May 11, 2015, 5 p.m.
Does not check if there is a diff_commit.	brennie	May 11, 2015, 4:21 p.m.
This has the potential to be cheaper by doing: self.filediff.diff_commit_id. Reduces the possibility of a lookup.	chipx86	May 11, 2015, 4:25 p.m.
Alignment is inconsistent between these.	chipx86	May 11, 2015, 4:52 p.m.
Maybe flesh this out a bit so its description stands alone without needing to know more of the architecture as …	chipx86	May 11, 2015, 4:58 p.m.
Can you use a list comprehension instead of map? They're generally easier to read and are faster, since they don't …	chipx86	May 11, 2015, 4:54 p.m.
Since this code is all basically the same as the above, we should factor this out into another function.	chipx86	May 11, 2015, 5:20 p.m.
Does not check if there is a diff_commit	brennie	May 11, 2015, 4:21 p.m.
As above, we should use diff_commit_id here instead.	chipx86	May 11, 2015, 4:53 p.m.
No need for six.text_type anymore.	chipx86	May 11, 2015, 4:53 p.m.
Col: 5 E303 too many blank lines (2)	reviewbot	May 11, 2015, 6:59 p.m.
local variable 'have_interdiffset_commits' is assigned to but never used	reviewbot	May 11, 2015, 6:59 p.m.
"FileDiffs" and "DiffSet".	chipx86	May 13, 2015, 4:44 p.m.
Looks like we're doing this twice now?	chipx86	May 13, 2015, 4:44 p.m.

Tool: Pyflakes
Processed Files:
    reviewboard/diffviewer/models.py
    reviewboard/diffviewer/tests.py
    reviewboard/diffviewer/diffutils.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/diffviewer/models.py
    reviewboard/diffviewer/tests.py
    reviewboard/diffviewer/diffutils.py

Summary:

[WIP] Do not show duplicate files in DiffViewer

Show squashed diffs in the diffviewer

Description:

~		Previously we would show multiple `FileDiff`s for the same file in the diff viewer if the file was modified in multiple commits. Now we determine what `FileDiff`s are ancestors of other `FileDiff`s in the commit history and filter out all the ancestors.
	~	Previously we would show multiple `FileDiff`s for the same file in the diff viewer if the file was modified in multiple commits. Now we determine what `FileDiff`s are ancestors of other `FileDiff`s in the commit history and filter out all the ancestors. The result is a squashed diff between the original file and the resulting file.
-
-		TODO: Show correct source file and revision. It currently only shows the previous source file and revision.

Testing Done:

	+	Ran unit tests.
	+
	+	Manually verified that the squashed diffs were correct.

Commit:

f5955cc199fb706a4e1f472297c8ad083922c290

fc09644ddbe85164826249ee3f046a89afba1404

Diff:

Revision 2 (+207 -20)

Show changes

	reviewboard/diffviewer/chunk_generator.py
	reviewboard/diffviewer/commitutils.py
	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/models.py
	reviewboard/diffviewer/renderers.py
	reviewboard/diffviewer/tests.py
	reviewboard/diffviewer/views.py

Tool: Pyflakes
Processed Files:
    reviewboard/diffviewer/chunk_generator.py
    reviewboard/diffviewer/renderers.py
    reviewboard/diffviewer/models.py
    reviewboard/diffviewer/tests.py
    reviewboard/diffviewer/diffutils.py
    reviewboard/diffviewer/views.py
    reviewboard/diffviewer/commitutils.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/diffviewer/chunk_generator.py
    reviewboard/diffviewer/renderers.py
    reviewboard/diffviewer/models.py
    reviewboard/diffviewer/tests.py
    reviewboard/diffviewer/diffutils.py
    reviewboard/diffviewer/views.py
    reviewboard/diffviewer/commitutils.py

reviewboard/diffviewer/chunk_generator.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 12
 E111 indentation is not a multiple of four
```

Change Summary:: wrap description
Description:: ~
Previously we would show multiple FileDiffs for the same file in the diff viewer if the file was modified in multiple commits. Now we determine what FileDiffs are ancestors of other FileDiffs in the commit history and filter out all the ancestors. The result is a squashed diff between the original file and the resulting file.
~
Previously we would show multiple FileDiffs for the same file in the
+ diff viewer if the file was modified in multiple commits. Now we
+ determine what FileDiffs are ancestors of other FileDiffs in the
+ commit history and filter out all the ancestors. The result is a
+ squashed diff between the original file and the resulting file.

Commit:

fc09644ddbe85164826249ee3f046a89afba1404

e65c801746c41b2c5b4960e2e7dba60c2f25d7b2

Diff:

Revision 3 (+207 -20)

Show changes

	reviewboard/diffviewer/chunk_generator.py
	reviewboard/diffviewer/commitutils.py
	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/models.py
	reviewboard/diffviewer/renderers.py
	reviewboard/diffviewer/tests.py
	reviewboard/diffviewer/views.py

Tool: Pyflakes
Processed Files:
    reviewboard/diffviewer/chunk_generator.py
    reviewboard/diffviewer/renderers.py
    reviewboard/diffviewer/models.py
    reviewboard/diffviewer/tests.py
    reviewboard/diffviewer/diffutils.py
    reviewboard/diffviewer/views.py
    reviewboard/diffviewer/commitutils.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/diffviewer/chunk_generator.py
    reviewboard/diffviewer/renderers.py
    reviewboard/diffviewer/models.py
    reviewboard/diffviewer/tests.py
    reviewboard/diffviewer/diffutils.py
    reviewboard/diffviewer/views.py
    reviewboard/diffviewer/commitutils.py

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

The issue has been resolved. Show all issues

We should probably just import logging at the top of the file.

brennie May 7, 2015, 1:57 p.m.

Oops, this was left over from debugging where I broke something else :)

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

The issue has been resolved. Show all issues

This should use exc_info=True (we have =1 elsewhere, but I'm slowly cleaning that up).

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

The issue has been resolved. Show all issues

I think I liked your approach up above of changing all of these to add the - and the beginning of each appended string.

Commit:

e65c801746c41b2c5b4960e2e7dba60c2f25d7b2

97e4a7de42a0e884a0a7b86a32e949f4826f46f3

Diff:

Revision 4 (+207 -25)

Show changes

	reviewboard/diffviewer/chunk_generator.py
	reviewboard/diffviewer/commitutils.py
	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/models.py
	reviewboard/diffviewer/renderers.py
	reviewboard/diffviewer/tests.py
	reviewboard/diffviewer/views.py

Tool: Pyflakes
Processed Files:
    reviewboard/diffviewer/chunk_generator.py
    reviewboard/diffviewer/renderers.py
    reviewboard/diffviewer/models.py
    reviewboard/diffviewer/tests.py
    reviewboard/diffviewer/diffutils.py
    reviewboard/diffviewer/views.py
    reviewboard/diffviewer/commitutils.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/diffviewer/chunk_generator.py
    reviewboard/diffviewer/renderers.py
    reviewboard/diffviewer/models.py
    reviewboard/diffviewer/tests.py
    reviewboard/diffviewer/diffutils.py
    reviewboard/diffviewer/views.py
    reviewboard/diffviewer/commitutils.py

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

Would it be worth it to build a list of parts for the key and use ''.join versus all the appends we're doing, or does that matter? Here and in the renderer.

chipx86 May 11, 2015, 4:31 p.m.

Not really. Joins are better when you have a lot of strings you're working with (for, say, text files), but there's not that many here. In fact, it can have the opposite effect and actually take longer for smaller sets of strings.

To check, I ran comparisons using the timeit module, comparing the above with and without using join (and with hard-coded values for all PKs and such). 100,000 runs of each.

Without joins, 100,000 runs took 0.0366 seconds.

With joins, 100,000 runs took 0.0422 seconds.

reviewboard/diffviewer/models.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Unindent.
```
reviewboard/diffviewer/models.py (Diff revision 4)
The issue has been resolved. Show all issues
```
"In this case" twice is awkward phrasing.
```

Commit:

97e4a7de42a0e884a0a7b86a32e949f4826f46f3

9f75921701894ff464d80e50d58344c799a6a5ae

Diff:

Revision 5 (+206 -25)

Show changes

	reviewboard/diffviewer/chunk_generator.py
	reviewboard/diffviewer/commitutils.py
	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/models.py
	reviewboard/diffviewer/renderers.py
	reviewboard/diffviewer/tests.py
	reviewboard/diffviewer/views.py

Tool: Pyflakes
Processed Files:
    reviewboard/diffviewer/chunk_generator.py
    reviewboard/diffviewer/renderers.py
    reviewboard/diffviewer/models.py
    reviewboard/diffviewer/tests.py
    reviewboard/diffviewer/diffutils.py
    reviewboard/diffviewer/views.py
    reviewboard/diffviewer/commitutils.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/diffviewer/chunk_generator.py
    reviewboard/diffviewer/renderers.py
    reviewboard/diffviewer/models.py
    reviewboard/diffviewer/tests.py
    reviewboard/diffviewer/diffutils.py
    reviewboard/diffviewer/views.py
    reviewboard/diffviewer/commitutils.py

reviewboard/diffviewer/chunk_generator.py (Diff revision 5)
The issue has been resolved. Show all issues
```
diffviewer.commitutils before diffviewer.differ
```
reviewboard/diffviewer/models.py (Diff revision 5)
The issue has been resolved. Show all issues
```
Blank line after this.
```
reviewboard/diffviewer/renderers.py (Diff revision 5)
The issue has been resolved. Show all issues
```
This should also have a '-'
```

Commit:

9f75921701894ff464d80e50d58344c799a6a5ae

4b94c2e41f08018c7e43a5e4b3f556774257e1b0

Diff:

Revision 6 (+208 -26)

Show changes

	reviewboard/diffviewer/chunk_generator.py
	reviewboard/diffviewer/commitutils.py
	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/models.py
	reviewboard/diffviewer/renderers.py
	reviewboard/diffviewer/tests.py
	reviewboard/diffviewer/views.py

Tool: Pyflakes
Processed Files:
    reviewboard/diffviewer/chunk_generator.py
    reviewboard/diffviewer/renderers.py
    reviewboard/diffviewer/models.py
    reviewboard/diffviewer/tests.py
    reviewboard/diffviewer/diffutils.py
    reviewboard/diffviewer/views.py
    reviewboard/diffviewer/commitutils.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/diffviewer/chunk_generator.py
    reviewboard/diffviewer/renderers.py
    reviewboard/diffviewer/models.py
    reviewboard/diffviewer/tests.py
    reviewboard/diffviewer/diffutils.py
    reviewboard/diffviewer/views.py
    reviewboard/diffviewer/commitutils.py

reviewboard/diffviewer/chunk_generator.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Does not check if there is a diff_commit.
```
reviewboard/diffviewer/renderers.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Does not check if there is a diff_commit
```

Change Summary:

Turn off cumulative diffing if there is not history.

Testing Done:

		Ran unit tests.

		Manually verified that the squashed diffs were correct.
	+
	+	Still able to view the `FileDiff`s of review requests without
	+	commit history.

Commit:

4b94c2e41f08018c7e43a5e4b3f556774257e1b0

bf89fe1530eb1ec8db737d24804c3c3e06e67f55

Diff:

Revision 7 (+217 -26)

Show changes

	reviewboard/diffviewer/chunk_generator.py
	reviewboard/diffviewer/commitutils.py
	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/models.py
	reviewboard/diffviewer/renderers.py
	reviewboard/diffviewer/tests.py
	reviewboard/diffviewer/views.py

reviewboard/diffviewer/chunk_generator.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Can you maybe describe this option in the docstring above?
```

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

The issue has been resolved. Show all issues

This has the potential to be cheaper by doing: self.filediff.diff_commit_id. Reduces the possibility of a lookup.

reviewboard/diffviewer/chunk_generator.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Alignment is inconsistent between these.
```

reviewboard/diffviewer/commitutils.py (Diff revision 6)

The issue has been resolved. Show all issues

Maybe flesh this out a bit so its description stands alone without needing to know more of the architecture as a whole.

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

The issue has been resolved. Show all issues

Can you use a list comprehension instead of map? They're generally easier to read and are faster, since they don't require the extra function calls.

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

The issue has been resolved. Show all issues

Since this code is all basically the same as the above, we should factor this out into another function.

reviewboard/diffviewer/renderers.py (Diff revision 6)
The issue has been resolved. Show all issues
```
As above, we should use diff_commit_id here instead.
```
reviewboard/diffviewer/renderers.py (Diff revision 6)
The issue has been resolved. Show all issues
```
No need for six.text_type anymore.
```

Tool: Pyflakes
Processed Files:
    reviewboard/diffviewer/chunk_generator.py
    reviewboard/diffviewer/renderers.py
    reviewboard/diffviewer/models.py
    reviewboard/diffviewer/tests.py
    reviewboard/diffviewer/diffutils.py
    reviewboard/diffviewer/views.py
    reviewboard/diffviewer/commitutils.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/diffviewer/chunk_generator.py
    reviewboard/diffviewer/renderers.py
    reviewboard/diffviewer/models.py
    reviewboard/diffviewer/tests.py
    reviewboard/diffviewer/diffutils.py
    reviewboard/diffviewer/views.py
    reviewboard/diffviewer/commitutils.py

Commit:

bf89fe1530eb1ec8db737d24804c3c3e06e67f55

5ca8f82367a7eeb83d3c393fd2d9b3ed580821dc

Diff:

Revision 8 (+244 -27)

Show changes

	reviewboard/diffviewer/chunk_generator.py
	reviewboard/diffviewer/commitutils.py
	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/models.py
	reviewboard/diffviewer/renderers.py
	reviewboard/diffviewer/tests.py
	reviewboard/diffviewer/views.py

Tool: Pyflakes
Processed Files:
    reviewboard/diffviewer/chunk_generator.py
    reviewboard/diffviewer/renderers.py
    reviewboard/diffviewer/models.py
    reviewboard/diffviewer/tests.py
    reviewboard/diffviewer/diffutils.py
    reviewboard/diffviewer/views.py
    reviewboard/diffviewer/commitutils.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/diffviewer/chunk_generator.py
    reviewboard/diffviewer/renderers.py
    reviewboard/diffviewer/models.py
    reviewboard/diffviewer/tests.py
    reviewboard/diffviewer/diffutils.py
    reviewboard/diffviewer/views.py
    reviewboard/diffviewer/commitutils.py

reviewboard/diffviewer/diffutils.py (Diff revision 8)
The issue has been resolved. Show all issues
```
Col: 5
 E303 too many blank lines (2)
```
reviewboard/diffviewer/diffutils.py (Diff revision 8)
The issue has been resolved. Show all issues
```
 local variable 'have_interdiffset_commits' is assigned to but never used
```

Change Summary:

PEP8

Commit:

5ca8f82367a7eeb83d3c393fd2d9b3ed580821dc

098fec9d421c77d4a1e97ec467758155ac4bcbb1

Diff:

Revision 9 (+240 -27)

Show changes

	reviewboard/diffviewer/chunk_generator.py
	reviewboard/diffviewer/commitutils.py
	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/models.py
	reviewboard/diffviewer/renderers.py
	reviewboard/diffviewer/tests.py
	reviewboard/diffviewer/views.py

Tool: Pyflakes
Processed Files:
    reviewboard/diffviewer/chunk_generator.py
    reviewboard/diffviewer/renderers.py
    reviewboard/diffviewer/models.py
    reviewboard/diffviewer/tests.py
    reviewboard/diffviewer/diffutils.py
    reviewboard/diffviewer/views.py
    reviewboard/diffviewer/commitutils.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/diffviewer/chunk_generator.py
    reviewboard/diffviewer/renderers.py
    reviewboard/diffviewer/models.py
    reviewboard/diffviewer/tests.py
    reviewboard/diffviewer/diffutils.py
    reviewboard/diffviewer/views.py
    reviewboard/diffviewer/commitutils.py

reviewboard/diffviewer/commitutils.py (Diff revision 9)
The issue has been resolved. Show all issues
```
"FileDiffs" and "DiffSet".
```
reviewboard/diffviewer/diffutils.py (Diff revision 9)
The issue has been resolved. Show all issues
```
Looks like we're doing this twice now?
```

Change Summary:

Address Christian's issues.

Commit:

098fec9d421c77d4a1e97ec467758155ac4bcbb1

af9f90812602244307588e61bb2153c13c5cd823

Diff:

Revision 10 (+234 -27)

Show changes

	reviewboard/diffviewer/chunk_generator.py
	reviewboard/diffviewer/commitutils.py
	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/models.py
	reviewboard/diffviewer/renderers.py
	reviewboard/diffviewer/tests.py
	reviewboard/diffviewer/views.py

Tool: Pyflakes
Processed Files:
    reviewboard/diffviewer/chunk_generator.py
    reviewboard/diffviewer/renderers.py
    reviewboard/diffviewer/models.py
    reviewboard/diffviewer/tests.py
    reviewboard/diffviewer/diffutils.py
    reviewboard/diffviewer/views.py
    reviewboard/diffviewer/commitutils.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/diffviewer/chunk_generator.py
    reviewboard/diffviewer/renderers.py
    reviewboard/diffviewer/models.py
    reviewboard/diffviewer/tests.py
    reviewboard/diffviewer/diffutils.py
    reviewboard/diffviewer/views.py
    reviewboard/diffviewer/commitutils.py

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to dvcs (dcaca07)

~		Previously we would show multiple `FileDiff`s for the same file in the diff viewer if the file was modified in multiple commits. Now we determine what `FileDiff`s are ancestors of other `FileDiff`s in the commit history and filter out all the ancestors. The result is a squashed diff between the original file and the resulting file.
	~	Previously we would show multiple `FileDiff`s for the same file in the
	+	diff viewer if the file was modified in multiple commits. Now we
	+	determine what `FileDiff`s are ancestors of other `FileDiff`s in the
	+	commit history and filter out all the ancestors. The result is a
	+	squashed diff between the original file and the resulting file.