Summary

Show cumulative diffs in the diffviewer

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

Information

Owner

brennie

Repository

Review Board

Branch

release-4.0.x

Bugs

Depends On

~~10119~~

Blocks

10124

Commit

7e6d551...

Reviewers

Groups

reviewboard

People

Description

Previously, FileDiffs for review requests with history would not show
cumulatively. That is, you would see only the result of applying that
FileDiff, not the result of applying the entire stack of FileDiffs
in the history graph.

We now compute the entire file history, including deletions, and use
that to compute the cumulative diff. In other words, for a review
request with commits A -> B -> C where all three commits modify the
same file, previously you would see the result of A, B, and C
independently. Now the diffviewer shows A, A..B, and A..C.

Files are still repeated (as indicated above) so this is not yet a true
cumulative diff. A future patch will update the diffviewer to only show
the leaf FileDiffs in the history graph.

Testing Done


Ran unit tests.
Uploaded a multi-commit review request and looked at the diff. I

  observed cumulative (but repeated) diffs.

Issues

Description	From	Last Updated
E501 line too long (86 > 79 characters)	reviewbot	Aug. 22, 2018, 12:15 p.m.
E501 line too long (82 > 79 characters)	reviewbot	Aug. 22, 2018, 12:17 p.m.
E501 line too long (91 > 79 characters)	reviewbot	Aug. 22, 2018, 12:17 p.m.
E501 line too long (81 > 79 characters)	reviewbot	Aug. 22, 2018, 12:17 p.m.
E501 line too long (81 > 79 characters)	reviewbot	Aug. 22, 2018, 12:39 p.m.
E303 too many blank lines (2)	reviewbot	Aug. 22, 2018, 12:39 p.m.
F401 'reviewboard.diffviewer.models.DiffSet' imported but unused	reviewbot	Aug. 23, 2018, 2:52 p.m.
E303 too many blank lines (2)	reviewbot	Aug. 23, 2018, 2:52 p.m.
E303 too many blank lines (2)	reviewbot	Aug. 23, 2018, 2:52 p.m.
F811 redefinition of unused 'setUp' from line 2822	reviewbot	Aug. 23, 2018, 2:52 p.m.
F401 'reviewboard.diffviewer.models.DiffSet' imported but unused	reviewbot	Aug. 23, 2018, 2:52 p.m.
E303 too many blank lines (2)	reviewbot	Aug. 23, 2018, 2:52 p.m.
E303 too many blank lines (2)	reviewbot	Aug. 23, 2018, 2:52 p.m.
Can you explicitly compare against 0?	chipx86	Aug. 27, 2018, 9:27 p.m.
One blank line.	chipx86	Aug. 27, 2018, 9:27 p.m.
Can you move the paren for the base_filediff stuff onto the next line, like: (newfile or (base_filediff is not None …	chipx86	Aug. 27, 2018, 9:27 p.m.
You'll need to import range from six, or we get a list here on Python 2.	chipx86	Aug. 27, 2018, 9:28 p.m.
Missing docstring.	chipx86	Aug. 27, 2018, 9:28 p.m.
Missing docstring.	chipx86	Aug. 27, 2018, 9:28 p.m.
Too many blank lines.	chipx86	Aug. 27, 2018, 9:29 p.m.
Too many blank lines.	chipx86	Aug. 27, 2018, 9:29 p.m.
We should compare in the returned order, not as a set, to catch future discrepencies.	chipx86	Aug. 27, 2018, 9:38 p.m.
Since we're looping and asserting stuff in each loop, we should print out something to help identify the diff_file we're …	chipx86	Aug. 27, 2018, 9:39 p.m.
This is indented 1 too many spaces.	david	Sept. 2, 2018, 11:31 a.m.
I personally find it more readable to do for i in reversed(range(len(ancestor_ids))). No biggie though.	david	Sept. 2, 2018, 11:30 a.m.

flake8 failed.

JSHint passed.

flake8

reviewboard/diffviewer/chunk_generator.py (Diff revision 1)
The issue has been resolved. Show all issues
```
E501 line too long (86 > 79 characters)
```
reviewboard/diffviewer/models/filediff.py (Diff revision 1)
The issue has been resolved. Show all issues
```
E501 line too long (82 > 79 characters)
```
reviewboard/diffviewer/models/filediff.py (Diff revision 1)
The issue has been resolved. Show all issues
```
E501 line too long (91 > 79 characters)
```
reviewboard/diffviewer/models/filediff.py (Diff revision 1)
The issue has been resolved. Show all issues
```
E501 line too long (81 > 79 characters)
```

Testing Done:: ~
Uploaded a multi-commit review request and looked at the diff. I
observed cumulative (but repeated) diffs.
~
TODO: Add unit tests.
~
Ran unit tests.
~
Uploaded a multi-commit review request and looked at the diff. I
observed cumulative (but repeated) diffs.
+
TODO: Add unit tests.

Change Summary:

Better architecture.

Commit:

6768bdcb44671579184d7ab94dc12708bd0e7b90

1dbe3963e4b8c427f5d2152e83602493bd7f2dad

Diff:

Revision 2 (+290 -76)

Show changes

	reviewboard/diffviewer/chunk_generator.py
	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/renderers.py
	reviewboard/diffviewer/models/filediff.py
	reviewboard/diffviewer/tests/test_diffutils.py
	reviewboard/diffviewer/tests/test_filediff.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/diffviewer/models/filediff.py (Diff revision 2)
The issue has been resolved. Show all issues
```
E501 line too long (81 > 79 characters)
```
reviewboard/diffviewer/renderers.py (Diff revision 2)
The issue has been resolved. Show all issues
```
E303 too many blank lines (2)
```

Commit:

1dbe3963e4b8c427f5d2152e83602493bd7f2dad

ecec404ab7fb0e45af857d295606ccf66b95fe9f

Diff:

Revision 3 (+290 -76)

Show changes

	reviewboard/diffviewer/chunk_generator.py
	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/renderers.py
	reviewboard/diffviewer/models/filediff.py
	reviewboard/diffviewer/tests/test_diffutils.py
	reviewboard/diffviewer/tests/test_filediff.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Summary:

WIP: Show cumulative diffs in the diffviewer

Show cumulative diffs in the diffviewer

Testing Done:

		Ran unit tests.
		Uploaded a multi-commit review request and looked at the diff. I observed cumulative (but repeated) diffs.
-		TODO: Add unit tests.

Commit:

ecec404ab7fb0e45af857d295606ccf66b95fe9f

80f2911476891ce37c947036b34de2ef1e4d7804

Diff:

Revision 4 (+586 -234)

Show changes

	reviewboard/diffviewer/chunk_generator.py
	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/renderers.py
	reviewboard/diffviewer/models/filediff.py
	reviewboard/diffviewer/tests/test_diffutils.py
	reviewboard/diffviewer/tests/test_filediff.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/diffviewer/tests/test_diffutils.py (Diff revision 4)
The issue has been resolved. Show all issues
```
F401 'reviewboard.diffviewer.models.DiffSet' imported but unused
```
reviewboard/diffviewer/tests/test_diffutils.py (Diff revision 4)
The issue has been resolved. Show all issues
```
E303 too many blank lines (2)
```
reviewboard/diffviewer/tests/test_diffutils.py (Diff revision 4)
The issue has been resolved. Show all issues
```
E303 too many blank lines (2)
```
reviewboard/diffviewer/tests/test_diffutils.py (Diff revision 4)
The issue has been resolved. Show all issues
```
F811 redefinition of unused 'setUp' from line 2822
```

Commit:

80f2911476891ce37c947036b34de2ef1e4d7804

a9fcb81c2bd0a0a3ef140fcae3cffe211c8f882c

Diff:

Revision 5 (+585 -238)

Show changes

	reviewboard/diffviewer/chunk_generator.py
	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/renderers.py
	reviewboard/diffviewer/models/filediff.py
	reviewboard/diffviewer/tests/test_diffutils.py
	reviewboard/diffviewer/tests/test_filediff.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/diffviewer/tests/test_diffutils.py (Diff revision 5)
The issue has been resolved. Show all issues
```
F401 'reviewboard.diffviewer.models.DiffSet' imported but unused
```
reviewboard/diffviewer/tests/test_diffutils.py (Diff revision 5)
The issue has been resolved. Show all issues
```
E303 too many blank lines (2)
```
reviewboard/diffviewer/tests/test_diffutils.py (Diff revision 5)
The issue has been resolved. Show all issues
```
E303 too many blank lines (2)
```

Commit:

a9fcb81c2bd0a0a3ef140fcae3cffe211c8f882c

29b588f387be58f5a1ff781ea99c75a882255acc

Diff:

Revision 6 (+582 -237)

Show changes

	reviewboard/diffviewer/chunk_generator.py
	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/renderers.py
	reviewboard/diffviewer/models/filediff.py
	reviewboard/diffviewer/tests/test_diffutils.py
	reviewboard/diffviewer/tests/test_filediff.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Commit:

29b588f387be58f5a1ff781ea99c75a882255acc

5c0167db0e6081a5dbfb30144a7de89068f5c686

Diff:

Revision 7 (+581 -237)

Show changes

	reviewboard/diffviewer/chunk_generator.py
	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/renderers.py
	reviewboard/diffviewer/models/filediff.py
	reviewboard/diffviewer/tests/test_diffutils.py
	reviewboard/diffviewer/tests/test_filediff.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Change Summary:

Rebase

Commit:

5c0167db0e6081a5dbfb30144a7de89068f5c686

acf4912903c689ef8ac97441e2cb081369e14e35

Diff:

Revision 8 (+577 -233)

Show changes

	reviewboard/diffviewer/chunk_generator.py
	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/renderers.py
	reviewboard/diffviewer/models/filediff.py
	reviewboard/diffviewer/tests/test_diffutils.py
	reviewboard/diffviewer/tests/test_filediff.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Commit:

acf4912903c689ef8ac97441e2cb081369e14e35

2c0f8462989d794deae15244ae6f07e215a8562b

Diff:

Revision 9 (+578 -234)

Show changes

	reviewboard/diffviewer/chunk_generator.py
	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/renderers.py
	reviewboard/diffviewer/models/filediff.py
	reviewboard/diffviewer/tests/test_diffutils.py
	reviewboard/diffviewer/tests/test_filediff.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Change Summary:

Add more unit tests

Commit:

2c0f8462989d794deae15244ae6f07e215a8562b

037cc56cd51f3094e7f61a433b5df80a571c79c8

Diff:

Revision 10 (+635 -234)

Show changes

	reviewboard/diffviewer/chunk_generator.py
	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/renderers.py
	reviewboard/diffviewer/models/filediff.py
	reviewboard/diffviewer/tests/test_diffutils.py
	reviewboard/diffviewer/tests/test_filediff.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/diffviewer/diffutils.py (Diff revision 10)
The issue has been resolved. Show all issues
```
Can you explicitly compare against 0?
```
reviewboard/diffviewer/diffutils.py (Diff revision 10)
The issue has been resolved. Show all issues
```
One blank line.
```

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

The issue has been resolved. Show all issues

Can you move the paren for the base_filediff stuff onto the next line, like:

(newfile or
 (base_filediff is not None and ...
  ...)

reviewboard/diffviewer/models/filediff.py (Diff revision 10)
The issue has been resolved. Show all issues
```
You'll need to import range from six, or we get a list here on Python 2.
```
reviewboard/diffviewer/tests/test_diffutils.py (Diff revision 10)
The issue has been resolved. Show all issues
```
Missing docstring.
```
reviewboard/diffviewer/tests/test_diffutils.py (Diff revision 10)
The issue has been resolved. Show all issues
```
Missing docstring.
```

reviewboard/diffviewer/tests/test_diffutils.py (Diff revision 10)

The issue has been resolved. Show all issues

Too many blank lines.

reviewboard/diffviewer/tests/test_diffutils.py (Diff revision 10)
The issue has been resolved. Show all issues
```
Too many blank lines.
```
reviewboard/diffviewer/tests/test_diffutils.py (Diff revision 10)
The issue has been resolved. Show all issues
```
We should compare in the returned order, not as a set, to catch future discrepencies.
```

reviewboard/diffviewer/tests/test_diffutils.py (Diff revision 10)

The issue has been resolved. Show all issues

Since we're looping and asserting stuff in each loop, we should print out something to help identify the diff_file we're working on in case of assertion.

Change Summary:

Addressed feedback.

Commit:

037cc56cd51f3094e7f61a433b5df80a571c79c8

3aeb572f153406c48a6f8e8c33ccba28c8a7d89f

Diff:

Revision 11 (+643 -235)

Show changes

	reviewboard/diffviewer/chunk_generator.py
	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/renderers.py
	reviewboard/diffviewer/models/filediff.py
	reviewboard/diffviewer/tests/test_diffutils.py
	reviewboard/diffviewer/tests/test_filediff.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Commit:

3aeb572f153406c48a6f8e8c33ccba28c8a7d89f

7e6d55105315dd4111395d35139acf239033db97

Diff:

Revision 12 (+643 -235)

Show changes

	reviewboard/diffviewer/chunk_generator.py
	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/renderers.py
	reviewboard/diffviewer/models/filediff.py
	reviewboard/diffviewer/tests/test_diffutils.py
	reviewboard/diffviewer/tests/test_filediff.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Ship it!

reviewboard/diffviewer/diffutils.py (Diff revision 12)
The issue has been resolved. Show all issues
```
This is indented 1 too many spaces.
```

reviewboard/diffviewer/models/filediff.py (Diff revision 12)

The issue has been dropped. Show all issues

I personally find it more readable to do for i in reversed(range(len(ancestor_ids))). No biggie though.

brennie Sept. 2, 2018, 11:30 a.m.

This is the iterator version of range, ie xrange. reversed() cannot accept iterators, only iterables.

Status:: Completed
Change Summary:: Pushed to release-4.0.x (0dd6930)