flake8
passed.
JSHint
passed.
Compute pre-patch files for FileDiffs in commit histories
Review Request #10119 — Created Aug. 13, 2018 and submitted
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:
-
- Ran unit tests.
- Was able to view diffs in the diffviewer that depended on previous
FileDiffs.
- - TODO: Add tests to cover new behaviour.
- Commit:
-
384436f155f5278e267ff675728a2286b2ef4579ba054b74376bf307453130260248096c06f008d3
Checks run (1 failed, 1 succeeded)
flake8
failed.
JSHint
passed.
flake8
- Commit:
-
86b9dc282889f9e50230f2d870553e0e5138e51b4eec0c3900299b10aae3d404857020a1e7748245
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
- Change Summary:
-
Added unit tests.
Rebased off /r/10122/ - Depends On:
-
- Commit:
4eec0c3900299b10aae3d404857020a1e774824544e69f4bd8b341606100337c0cbc30e2ce9fd0f2- Diff:
Revision 5 (+651 -8)
- Change Summary:
-
flake8
- Commit:
-
44e69f4bd8b341606100337c0cbc30e2ce9fd0f2a7bb09d4314f58b6f0e9ddd32dcaf7d529fc686c
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
- Change Summary:
-
Simplify get_ancestors() call paths
- Commit:
-
a7bb09d4314f58b6f0e9ddd32dcaf7d529fc686ca9a629f53f65ba452b63b06f1a14eb0097e4ba4c
- Change Summary:
-
Correctly compute original file for non-new filediffs with no ancestors in RRs with history.
- Commit:
-
a9a629f53f65ba452b63b06f1a14eb0097e4ba4c0d41ee1d153bad81928f66de778d36f7eb6dfae0
- Commit:
-
0d41ee1d153bad81928f66de778d36f7eb6dfae097bc96e3ae7ccd9ac1b52169f68e4e868d97b7df
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
- Change Summary:
-
Fix logging in some unit tests
- Commit:
-
97bc96e3ae7ccd9ac1b52169f68e4e868d97b7dfc88f64aeee2fea2ea9be0cbc072a34d39acb89ca
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
-
-
-
-
-
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. -
Here as well.
In fact, should this be within the previous
else, since we've already handled the empty filediffs issue in the firstif? -
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:
-
c88f64aeee2fea2ea9be0cbc072a34d39acb89caf07fe945e6ef42657e73733188b8f0e37e1393d9
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
-
-
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?
-
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:
-
f07fe945e6ef42657e73733188b8f0e37e1393d90506dcce7497c3f0186698bc5ca380d933cfe010
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.