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 |
![]() |
|
F841 local variable 'to_apply' is assigned to but never used |
![]() |
|
F841 local variable 'diffset' is assigned to but never used |
![]() |
|
F821 undefined name 'orig_filediff' |
![]() |
|
F821 undefined name 'orig_filediff' |
![]() |
|
E501 line too long (80 > 79 characters) |
![]() |
|
E501 line too long (81 > 79 characters) |
![]() |
|
F841 local variable 'TypeError' is assigned to but never used |
![]() |
|
W391 blank line at end of file |
![]() |
|
F401 'base64' imported but unused |
![]() |
|
F401 'json' imported but unused |
![]() |
|
W503 line break before binary operator |
![]() |
|
Missing trailing period. |
|
|
What happens if patching fails? We should document this situation more clearly in the docstring. |
|
|
We probably shouldn't Shortest Middle Snake this. |
|
|
Grammaro: "that is belongs to a commit" |
|
|
We can avoid the function calls by making this a generator expression instead of using filter. |
|
|
No need for .spy anymore. You can call directly on the function. |
|
|
These unit tests were never added. |
|
|
Alphabetical order. |
|
|
No () after the function names in any of these test docstrings. |
|
|
W503 line break before binary operator |
![]() |
|
E302 expected 2 blank lines, found 1 |
![]() |
|
F811 redefinition of unused 'GetOriginalFileTests' from line 2527 |
![]() |
|
W391 blank line at end of file |
![]() |
|
E501 line too long (85 > 79 characters) |
![]() |
|
E501 line too long (81 > 79 characters) |
![]() |
|
E501 line too long (85 > 79 characters) |
![]() |
|
E501 line too long (81 > 79 characters) |
![]() |
|
Alphabetical order. |
|
|
Here, too. |
|
|
Here, too. |
|
|
We might want to check explicitly against None here, so that if a caller were to pass in a queryset, … |
|
|
Here as well. In fact, should this be within the previous else, since we've already handled the empty filediffs issue … |
|
|
Where does the 8 come from? This might be more self-documenting if we can do math on len(self.filediffs) here. Are … |
|
|
Same question here. |
|
|
This seems like a really good opportunity for caching. If we're applying a big stack of patches, it seems likely … |
|
|
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 … |
|
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
-
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
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 4 (+499 -9) |
Checks run (2 succeeded)
-
-
-
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
.spy
anymore. 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
-
-
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)
Change Summary:
Simplify get_ancestors() call paths
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 7 (+620 -41) |
Checks run (1 failed, 1 succeeded)
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
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 9 (+625 -41) |
Checks run (2 succeeded)
Change Summary:
Fix logging in some unit tests
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 10 (+628 -45) |
Checks run (2 succeeded)
-
-
-
-
-
reviewboard/diffviewer/models/filediff.py (Diff revision 10) We might want to check explicitly against
None
here, 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)
-
-
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)
Change Summary:
Unit test fixes
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 13 (+589 -45) |