Summary

Generate diffs between base and tip commits

Review Request #10130 — Created Aug. 27, 2018 and submitted Oct. 18, 2018, 9:39 p.m.

Information

Owner

brennie

Repository

Review Board

Branch

release-4.0.x

Bugs

Depends On

~~10124~~

Commit

45b9779...

Reviewers

Groups

reviewboard

People

Description

The get_diff_files utility can now generate a list of files to diff
that exist between two commits in a diffset. This allows us to generate
diffs between two commits (and therefore to see the diff of any
individual commit). There is currently no UI support for this behaviour,
but that is comming in a future patch.

Testing Done

Ran unit tests.

Issues

Description	From	Last Updated
F811 redefinition of unused 'test_get_diff_files_with_history_tip_commit' from line 1213	reviewbot	Aug. 27, 2018, 2:58 p.m.
Missing space between the backtick and "diffsets".	chipx86	Aug. 30, 2018, 2:08 p.m.
Here, too.	chipx86	Aug. 30, 2018, 2:08 p.m.
Missing blank line.	chipx86	Aug. 30, 2018, 2:08 p.m.
No blank line needed here.	chipx86	Aug. 30, 2018, 2:08 p.m.
Given the repeated complexity of this, it would be nice to make this whole expected_results building into a private utility …	chipx86	Aug. 30, 2018, 2:20 p.m.
Redundant "diffsets" in this sentence.	chipx86	Sept. 4, 2018, 1:03 p.m.
Same here.	chipx86	Sept. 4, 2018, 1:03 p.m.
Can you compare explicitly against 0?	chipx86	Sept. 4, 2018, 1:03 p.m.
Here, too.	chipx86	Sept. 4, 2018, 1:03 p.m.
Should be .. code-block:: python (note the ::).	chipx86	Sept. 4, 2018, 1:04 p.m.
This doesn't match.	chipx86	Sept. 4, 2018, 1:04 p.m.
Syntax is super wonky here (0and). Does this even run?	david	Sept. 27, 2018, 11:02 a.m.
Should be "base commit"	david	Oct. 15, 2018, 1:17 p.m.
This is a little too fancy. How about: for ancestor in reversed(ancestors): if ancestor.commit_id <= base_commit.pk: base_filediff = ancestor break	david	Oct. 15, 2018, 1:18 p.m.
This looks like a fancy way of just doing by_details.get(base_filediff_details).	chipx86	Oct. 16, 2018, 2:28 p.m.

flake8 failed.

JSHint passed.

flake8

reviewboard/diffviewer/tests/test_diffutils.py (Diff revision 1)
The issue has been resolved. Show all issues
```
F811 redefinition of unused 'test_get_diff_files_with_history_tip_commit' from line 1213
```

Change Summary:

Flake8 and typo fixes

Commit:

-	7503455a150e258de558f0dbf2713c05812efc9e
+	b5834139aaa5eeb373c9206ebb2de545836aa248

Diff:

Revision 2 (+332 -12)

Show changes

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

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/diffviewer/diffutils.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Missing space between the backtick and "diffsets".
```
reviewboard/diffviewer/diffutils.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Here, too.
```
reviewboard/diffviewer/diffutils.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Missing blank line.
```
reviewboard/diffviewer/tests/test_diffutils.py (Diff revision 2)
The issue has been resolved. Show all issues
```
No blank line needed here.
```

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

The issue has been resolved. Show all issues

Given the repeated complexity of this, it would be nice to make this whole expected_results building into a private utility method.

Change Summary:

Addressed feedback.

Commit:

-	b5834139aaa5eeb373c9206ebb2de545836aa248
+	9d8e07f93a138d75da763d2474536df69d9a0c22

Diff:

Revision 3 (+354 -14)

Show changes

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

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/diffviewer/diffutils.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Redundant "diffsets" in this sentence.
```
reviewboard/diffviewer/diffutils.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Same here.
```
reviewboard/diffviewer/diffutils.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Can you compare explicitly against 0?
```
reviewboard/diffviewer/diffutils.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Here, too.
```
reviewboard/diffviewer/tests/test_diffutils.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Should be .. code-block:: python (note the ::).
```
reviewboard/diffviewer/tests/test_diffutils.py (Diff revision 3)
The issue has been resolved. Show all issues
```
This doesn't match.
```
1. brennie Sept. 4, 2018, 1:04 p.m.
  Weird, I must've missed committing the docs change because I have it locally.

Change Summary:

Addressed feedback.

Commit:

-	9d8e07f93a138d75da763d2474536df69d9a0c22
+	5af6c501da5c8b9a449e36bddb2b4af28ece2f78

Diff:

Revision 4 (+352 -12)

Show changes

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

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/diffviewer/diffutils.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Syntax is super wonky here (0and). Does this even run?
```
1. brennie Sept. 26, 2018, 1:01 p.m.
  Looks like I goofed it up before committing. Oops.

Commit:

-	5af6c501da5c8b9a449e36bddb2b4af28ece2f78
+	55caa2adf3df536281b179f65292df1f21406861

Diff:

Revision 5 (+352 -12)

Show changes

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

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/diffviewer/diffutils.py (Diff revision 5)
The issue has been resolved. Show all issues
```
Should be "base commit"
```

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

The issue has been resolved. Show all issues

This is a little too fancy. How about:

for ancestor in reversed(ancestors):
    if ancestor.commit_id <= base_commit.pk:
        base_filediff = ancestor
        break

Change Summary:

Addressed feedback.

Commit:

-	55caa2adf3df536281b179f65292df1f21406861
+	e0d6450582d52547d0402c8818e7dc1983446c8e

Diff:

Revision 6 (+348 -12)

Show changes

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

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/diffviewer/tests/test_diffutils.py (Diff revision 6)
The issue has been resolved. Show all issues
```
This looks like a fancy way of just doing by_details.get(base_filediff_details).
```
1. brennie Oct. 16, 2018, 2:28 p.m.
  Sure is!

Change Summary:

Addressed feedback.

Commit:

-	e0d6450582d52547d0402c8818e7dc1983446c8e
+	45b977927e5a9e82f7f24371f48590cd50ae772d

Diff:

Revision 7 (+345 -12)

Show changes

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

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-4.0.x (5820e10)