flake8
passed.
JSHint
passed.
Compute pre-patch files for FileDiffs in commit histories
Review Request #10119 — Created Aug. 13, 2018 and submitted
| Information | |
|---|---|
| brennie | |
| Review Board | |
| release-4.0.x | |
|
|
|
| 9920, 10123 | |
| d1c92e1... | |
| Reviewers | |
| reviewboard | |
We can now generate the pre-patch version of a file that is modified
multiple times in a commit history. This allows the diffviewer to show
side-by-side diffs in these cases, however this behaviour is not correct
as it will show files with the same name multiple times when it should
show a condensed diff of the entire history. This will be addressed in a
later patch.This also allows the original and patched file API endpoints to work for
files modified multiple times in a commit history.
- Ran unit tests.
- Was able to view diffs in the diffviewer that depended on previous
FileDiffs.
| Description | From | Last Updated |
|---|---|---|
|
F401 'reviewboard.diffviewer.models.FileDiff' imported but unused |
 reviewbot
|
|
|
F841 local variable 'to_apply' is assigned to but never used |
reviewbot
|
|
|
F841 local variable 'diffset' is assigned to but never used |
reviewbot
|
|
|
F821 undefined name 'orig_filediff' |
reviewbot
|
|
|
F821 undefined name 'orig_filediff' |
reviewbot
|
|
|
E501 line too long (80 > 79 characters) |
reviewbot
|
|
|
E501 line too long (81 > 79 characters) |
reviewbot
|
|
|
F841 local variable 'TypeError' is assigned to but never used |
reviewbot
|
|
|
W391 blank line at end of file |
reviewbot
|
|
|
F401 'base64' imported but unused |
reviewbot
|
|
|
F401 'json' imported but unused |
reviewbot
|
|
|
W503 line break before binary operator |
reviewbot
|
|
|
Missing trailing period. |
 chipx86
|
|
|
What happens if patching fails? We should document this situation more clearly in the docstring. |
chipx86
|
|
|
We probably shouldn't Shortest Middle Snake this. |
chipx86
|
|
|
Grammaro: "that is belongs to a commit" |
chipx86
|
|
|
We can avoid the function calls by making this a generator expression instead of using filter. |
chipx86
|
|
|
No need for .spy anymore. You can call directly on the function. |
chipx86
|
|
|
These unit tests were never added. |
chipx86
|
|
|
Alphabetical order. |
chipx86
|
|
|
No () after the function names in any of these test docstrings. |
chipx86
|
|
|
W503 line break before binary operator |
reviewbot
|
|
|
E302 expected 2 blank lines, found 1 |
reviewbot
|
|
|
F811 redefinition of unused 'GetOriginalFileTests' from line 2527 |
reviewbot
|
|
|
W391 blank line at end of file |
reviewbot
|
|
|
E501 line too long (85 > 79 characters) |
reviewbot
|
|
|
E501 line too long (81 > 79 characters) |
reviewbot
|
|
|
E501 line too long (85 > 79 characters) |
reviewbot
|
|
|
E501 line too long (81 > 79 characters) |
reviewbot
|
|
|
Alphabetical order. |
chipx86
|
|
|
Here, too. |
chipx86
|
|
|
Here, too. |
chipx86
|
|
|
We might want to check explicitly against None here, so that if a caller were to pass in a queryset, … |
chipx86
|
|
|
Here as well. In fact, should this be within the previous else, since we've already handled the empty filediffs issue … |
chipx86
|
|
|
Where does the 8 come from? This might be more self-documenting if we can do math on len(self.filediffs) here. Are … |
chipx86
|
|
|
Same question here. |
chipx86
|
|
|
This seems like a really good opportunity for caching. If we're applying a big stack of patches, it seems likely … |
 david
|
|
|
I don't see any reason why we can't just have everything use get_original_file_from_history. It looks like it should do the … |
david
|
Change Summary:
Major cleanup and refactor. Unit tests still required for get_original_file_*
Testing Done: |
|
|||||||||
|---|---|---|---|---|---|---|---|---|---|---|
Commit: |
|
|||||||||
Diff: |
Revision 2 (+504 -9) |
Checks run (1 failed, 1 succeeded)
flake8
failed.
JSHint
passed.
flake8
-
reviewboard/diffviewer/diffutils.py (Diff revision 2) Show all issues -
reviewboard/diffviewer/diffutils.py (Diff revision 2) F841 local variable 'to_apply' is assigned to but never used
-
reviewboard/diffviewer/diffutils.py (Diff revision 2) F841 local variable 'diffset' is assigned to but never used
-
-
-
-
-
reviewboard/diffviewer/models/filediff.py (Diff revision 2) F841 local variable 'TypeError' is assigned to but never used
-
-
-
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 3 (+499 -9) |
Checks run (1 failed, 1 succeeded)
flake8
failed.
JSHint
passed.
flake8
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 4 (+499 -9) |
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
-
-
-
reviewboard/diffviewer/diffutils.py (Diff revision 4) What happens if patching fails? We should document this situation more clearly in the docstring.
-
reviewboard/diffviewer/models/filediff.py (Diff revision 4) We probably shouldn't Shortest Middle Snake this.
-
-
reviewboard/diffviewer/models/filediff.py (Diff revision 4) We can avoid the function calls by making this a generator expression instead of using
filter. -
reviewboard/diffviewer/tests/test_diffutils.py (Diff revision 4) No need for
.spyanymore. You can call directly on the function. -
-
-
reviewboard/diffviewer/tests/test_filediff.py (Diff revision 4) No
()after the function names in any of these test docstrings.
Change Summary:
Added unit tests.
Rebased off /r/10122/
Depends On: |
|
||||
|---|---|---|---|---|---|
Commit: |
|
||||
Diff: |
Revision 5 (+651 -8) |
Checks run (1 failed, 1 succeeded)
flake8
failed.
JSHint
passed.
flake8
-
-
reviewboard/diffviewer/tests/test_diffutils.py (Diff revision 5) E302 expected 2 blank lines, found 1
-
reviewboard/diffviewer/tests/test_diffutils.py (Diff revision 5) F811 redefinition of unused 'GetOriginalFileTests' from line 2527
-
Change Summary:
flake8
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 6 (+620 -41) |
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
Change Summary:
Simplify get_ancestors() call paths
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 7 (+620 -41) |
Checks run (1 failed, 1 succeeded)
flake8
failed.
JSHint
passed.
flake8
Change Summary:
Correctly compute original file for non-new filediffs with no ancestors in RRs with history.
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 8 (+624 -41) |
Checks run (1 failed, 1 succeeded)
flake8
failed.
JSHint
passed.
flake8
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 9 (+625 -41) |
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
Change Summary:
Fix logging in some unit tests
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 10 (+628 -45) |
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
-
-
-
-
-
reviewboard/diffviewer/models/filediff.py (Diff revision 10) We might want to check explicitly against
Nonehere, so that if a caller were to pass in a queryset, we wouldn't be querying unnecessarily. -
reviewboard/diffviewer/models/filediff.py (Diff revision 10) Here as well.
In fact, should this be within the previous
else, since we've already handled the empty filediffs issue in the firstif? -
reviewboard/diffviewer/tests/test_filediff.py (Diff revision 10) Where does the 8 come from? This might be more self-documenting if we can do math on
len(self.filediffs)here. Are there 8 filediffs, or is itlen * something?I'm partially wondering because I think we can maybe do better on the query count, but I want to better understand the numbers.
-
Change Summary:
Addressed Christian's feedback
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 11 (+628 -45) |
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
-
-
reviewboard/diffviewer/diffutils.py (Diff revision 11) This seems like a really good opportunity for caching. If we're applying a big stack of patches, it seems likely that patch N+1 will use patch N as its original file.
You don't need to add the caching in this change, but can you at least add a TODO comment (and maybe asana task) for it?
-
reviewboard/diffviewer/diffutils.py (Diff revision 11) I don't see any reason why we can't just have everything use
get_original_file_from_history. It looks like it should do the right thing for the legacy case.
Change Summary:
Addressed feedback
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 12 (+589 -45) |
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
Change Summary:
Unit test fixes
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 13 (+589 -45) |
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.