Summary

Exclude ancestor FileDiffs from the diffviewer

Review Request #10124 — Created Aug. 22, 2018 and submitted Sept. 2, 2018, 12:04 p.m.

Information

Owner

brennie

Repository

Review Board

Branch

release-4.0.x

Bugs

Depends On

~~10123~~

Blocks

10130

Commit

423f1c4...

Reviewers

Groups

reviewboard

People

Description

When a review request is created with commit history, it is likely that
there will be FileDiffs in multiple commits that modify the same file.
For any FileDiff in a commit, all FileDiffs in previous commits are
known as "ancestor" FileDiffs.

These ancestor FileDiffs are now excluded from the diffviewer. This
removes the duplication of seeing the same file listed multiple times
with incremental changes. However, currently only the latest change for
that file is shown. True cumulative diffs are supported in /r/10130/.

Testing Done


Ran unit tests.
Viewed a review request with commit history and did not see repeated

  files.

Issues

Description	From	Last Updated
The first sentence of the description has some redundancy going on in the first sentence of the description. I'm also …	chipx86	Aug. 27, 2018, 6:12 p.m.
E501 line too long (90 > 79 characters)	reviewbot	Aug. 23, 2018, 12:29 p.m.
F821 undefined name 'diffset'	reviewbot	Aug. 23, 2018, 12:29 p.m.
F821 undefined name 'f'	reviewbot	Aug. 23, 2018, 12:29 p.m.
F841 local variable 'all_interfilediffs' is assigned to but never used	reviewbot	Aug. 23, 2018, 12:29 p.m.
E501 line too long (82 > 79 characters)	reviewbot	Aug. 23, 2018, 12:30 p.m.
E501 line too long (81 > 79 characters)	reviewbot	Aug. 23, 2018, 12:49 p.m.
E501 line too long (96 > 79 characters)	reviewbot	Aug. 23, 2018, 12:49 p.m.
Maybe this should be called get_leaf_filediffs?	chipx86	Aug. 27, 2018, 6:14 p.m.
"FileDiffs" I feel like this could be worded better. Maybe: python """Return only the leafs in the list of FileDiffs.""" …	chipx86	Aug. 27, 2018, 6:16 p.m.
This should be in the description.	chipx86	Aug. 27, 2018, 6:16 p.m.
Can you move the list() to the next line, so it's more tightly coupled to the queryset?	chipx86	Aug. 27, 2018, 6:16 p.m.
Alphabetical order.	chipx86	Aug. 27, 2018, 6:17 p.m.
Missing docstring.	chipx86	Aug. 27, 2018, 6:19 p.m.

flake8 failed.

JSHint passed.

flake8

reviewboard/diffviewer/commit_utils.py (Diff revision 1)
Show all issues
```
E501 line too long (90 > 79 characters)
```
reviewboard/diffviewer/commit_utils.py (Diff revision 1)
Show all issues
```
F821 undefined name 'diffset'
```
reviewboard/diffviewer/commit_utils.py (Diff revision 1)
Show all issues
```
F821 undefined name 'f'
```

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

Show all issues

F841 local variable 'all_interfilediffs' is assigned to but never used

reviewboard/diffviewer/diffutils.py (Diff revision 1)
Show all issues
```
E501 line too long (82 > 79 characters)
```

Summary:

-	WIP: Exclude ancestor FileDiffs from the diffviewer
+	Exclude ancestor FileDiffs from the diffviewer

Testing Done:

~		Viewed a review request with commit history and did not see repeated files.
~		TODO: Add unit tests.
	~	Ran unit tests.
	~	Viewed a review request with commit history and did not see repeated files.

Commit:

-	c099ebcdb374f4577aea2f1cc18afeccc7f24869
+	4c34e77856c8096e5bc3043659e898cfa334cd12

Diff:

Revision 2 (+96 -21)

Show changes

	reviewboard/diffviewer/commit_utils.py
	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/tests/test_commit_utils.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/diffviewer/tests/test_commit_utils.py (Diff revision 2)
Show all issues
```
E501 line too long (81 > 79 characters)
```
reviewboard/diffviewer/tests/test_commit_utils.py (Diff revision 2)
Show all issues
```
E501 line too long (96 > 79 characters)
```

Change Summary:

flake8

Commit:

-	4c34e77856c8096e5bc3043659e898cfa334cd12
+	3f4fdba82254a48bc5aceafb8d1ad18a9a71cffb

Diff:

Revision 3 (+99 -21)

Show changes

	reviewboard/diffviewer/commit_utils.py
	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/tests/test_commit_utils.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Change Summary:

Fix unit test failures

Commit:

-	3f4fdba82254a48bc5aceafb8d1ad18a9a71cffb
+	e5218e339f7b6bca0cf0b8914ea95904247c4adc

Diff:

Revision 4 (+106 -23)

Show changes

	reviewboard/diffviewer/commit_utils.py
	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/tests/test_commit_utils.py
	reviewboard/diffviewer/tests/test_diffutils.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Change Summary:

Fix unit test failures

Commit:

-	e5218e339f7b6bca0cf0b8914ea95904247c4adc
+	2aae5599f6401a0da00bb4a235a80b1c8054f876

Diff:

Revision 5 (+115 -25)

Show changes

	reviewboard/diffviewer/commit_utils.py
	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/tests/test_commit_utils.py
	reviewboard/diffviewer/tests/test_diffutils.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Show all issues

The first sentence of the description has some redundancy going on in the first sentence of the description.

I'm also just having a really hard time understanding this change from the description altogether.

reviewboard/diffviewer/commit_utils.py (Diff revision 5)
Show all issues
```
Maybe this should be called get_leaf_filediffs?
```

reviewboard/diffviewer/commit_utils.py (Diff revision 5)

Show all issues

"FileDiffs"

I feel like this could be worded better. Maybe:

python
"""Return only the leafs in the list of FileDiffs."""

With a description detailing what it means to be a leaf.

reviewboard/diffviewer/commit_utils.py (Diff revision 5)
Show all issues
```
This should be in the description.
```

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

Show all issues

Can you move the list() to the next line, so it's more tightly coupled to the queryset?

reviewboard/diffviewer/tests/test_commit_utils.py (Diff revision 5)
Show all issues
```
Alphabetical order.
```
reviewboard/diffviewer/tests/test_commit_utils.py (Diff revision 5)
Show all issues
```
Missing docstring.
```

Change Summary:

Address feedback

Description:

~		The diffviewer now only includes leaf FileDiffs in the diffviewer. That
~		is, the only FileDiffs that are shown are FileDiffs that are not
~		ancestors of other FileDiffs. This removes the duplication of files
~		appearing for each time they were modified in the history. Now, the
~		diffviewer just shows the real cumulative diff of all commits.
	~	When a review request is created with commit history, it is likely that
	~	there will be `FileDiff`s in multiple commits that modify the same file.
	~	For any `FileDiff` in a commit, all `FileDiff`s in previous commits are
	~	known as "ancestor" `FileDiff`s.
	~
	+	These ancestor `FileDiff`s are now excluded from the diffviewer. This
	+	removes the duplication of seeing the same file listed multiple times
	+	with incremental changes. However, currently only the latest change for
	+	that file is shown. True cumulative diffs are supported in /r/10130/.

Commit:

-	2aae5599f6401a0da00bb4a235a80b1c8054f876
+	423f1c47b4f60eec0e20947d0342121ba0f68f9a

Diff:

Revision 6 (+132 -25)

Show changes

	reviewboard/diffviewer/commit_utils.py
	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/tests/test_commit_utils.py
	reviewboard/diffviewer/tests/test_diffutils.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-4.0.x (5cc921a)