Fix a variety of bad bugs with diff comments and commit ranges.
Review Request #13547 — Created Feb. 16, 2024 and submitted
When making comments on diffs for review requests that have multiple
commits, there were a handful of bugs of varying severity. These all
occurred when adding comments to subsets of the commit range, rather
than the cumulative diff.
- Medium bad: The link URL for comments added to commit ranges went to
the regular diff page without including any commit info in the query
string. We therefore wouldn't load the correct view. - Bad: When looking at commit ranges, we'd add comment flags to the diff
view for all comments where the tip filediff matched. This would end
up showing comment flags even if the base commit didn't match. - Very bad: The diff fragment shown for comments on commit ranges wasn't
taking the base commit into account, instead always showing the diff
between the base of the entire change and the selected tip commit. In
most cases this wouldn't matter but if there are repeated changes to
the same file across many commits, the shown diff fragment would be
completely wrong.
These bugs all more or less come down to the same underlying problem:
not plumbing through the right commit information. We thankfully are
correctly storing the "base filediff" in the comment extra_data
, which
is the ID of the filediff for the given file in the selected base commit
of the range. We therefore need to plumb that through so that we're
always getting the correct things from get_diff_files
and other
diffutils methods.
In order to make this testable, I've pulled some commit-specific test
infrastructure out of test_diffutils.py and made it available from
our main testcase file.
- Ran unit tests.
- Posted a review request with a number of commits that all changed the
same file. Added comments across various ranges of those commits. Saw
the following:
- In the review box, the link for comments always went to the correct
commit range for the comment. - In the review box, the diff fragment for the comment was the same as
what was selected when making the comment. - When viewing commit ranges in the diff viewer, I only saw comments
that matched the currently visible commit range.
Summary | ID |
---|---|
c25fc449ef5a023255793d12fe3e60f84b26cff0 |
Description | From | Last Updated |
---|---|---|
'typing.Dict' imported but unused Column: 1 Error code: F401 |
reviewbot | |
'typing.List' imported but unused Column: 1 Error code: F401 |
reviewbot | |
'typing.Tuple' imported but unused Column: 1 Error code: F401 |
reviewbot | |
line too long (81 > 79 characters) Column: 80 Error code: E501 |
reviewbot | |
One blank line here (this is still import block territory) |
chipx86 | |
While we're doing all this, can we make these keyword-only and add @deprecate_non_keyword_only_args? There's a lot of important-to-get-right arguments in … |
chipx86 | |
Should be List[str] until we drop Python 3.8. I think, given the short timeframe for RB7, we should keep the … |
chipx86 | |
Should be List[Dict]. |
chipx86 | |
Dict[str, Any] |
chipx86 | |
Dict[str, Any] |
chipx86 | |
Dict[str, Any] |
chipx86 | |
Dict[str, Any] |
chipx86 | |
Dict (though if we're going to type this, we should try to be more specific). |
chipx86 | |
Dict[str, Any] |
chipx86 | |
Dict[str, Any] |
chipx86 | |
Can we make these keyword-only? |
chipx86 | |
Dict[Tuple, Sequence[Comment]] I'll leave it here for these. There are more in the change, but I don't want to bombard … |
chipx86 | |
We should type these, so they don't appear as literal Nones. |
chipx86 | |
This is missing an explicit return value. |
chipx86 | |
We should type this. |
chipx86 | |
TYPE_CHECKING sorts before Tuple alphabetically. |
chipx86 | |
We should type these. |
chipx86 |
- Commits:
-
Summary ID 989d1d7764876e4951873beef1dbc932d1864b95 852a2adc988a790040487cb9217d5d120b87d42a - Diff:
-
Revision 2 (+1868 -730)
Checks run (2 succeeded)
-
-
-
While we're doing all this, can we make these keyword-only and add
@deprecate_non_keyword_only_args
? There's a lot of important-to-get-right arguments in here. -
Should be
List[str]
until we drop Python 3.8.I think, given the short timeframe for RB7, we should keep the 3.8 compatibility.
-
-
-
-
-
-
-
-
-
-
Dict[Tuple, Sequence[Comment]]
I'll leave it here for these. There are more in the change, but I don't want to bombard you with comments to close.
-
-
-
-
- Commits:
-
Summary ID 852a2adc988a790040487cb9217d5d120b87d42a c25fc449ef5a023255793d12fe3e60f84b26cff0 - Diff:
-
Revision 3 (+1878 -732)