Fix a variety of bad bugs with diff comments and commit ranges.

Review Request #13547 — Created Feb. 16, 2024 and submitted

Information

Review Board
master

Reviewers

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
Fix a variety of bad bugs with diff comments and commit ranges.
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. Testing Done: - 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.
c25fc449ef5a023255793d12fe3e60f84b26cff0
Description From Last Updated

'typing.Dict' imported but unused Column: 1 Error code: F401

reviewbotreviewbot

'typing.List' imported but unused Column: 1 Error code: F401

reviewbotreviewbot

'typing.Tuple' imported but unused Column: 1 Error code: F401

reviewbotreviewbot

line too long (81 > 79 characters) Column: 80 Error code: E501

reviewbotreviewbot

One blank line here (this is still import block territory)

chipx86chipx86

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 …

chipx86chipx86

Should be List[str] until we drop Python 3.8. I think, given the short timeframe for RB7, we should keep the …

chipx86chipx86

Should be List[Dict].

chipx86chipx86

Dict[str, Any]

chipx86chipx86

Dict[str, Any]

chipx86chipx86

Dict[str, Any]

chipx86chipx86

Dict[str, Any]

chipx86chipx86

Dict (though if we're going to type this, we should try to be more specific).

chipx86chipx86

Dict[str, Any]

chipx86chipx86

Dict[str, Any]

chipx86chipx86

Can we make these keyword-only?

chipx86chipx86

Dict[Tuple, Sequence[Comment]] I'll leave it here for these. There are more in the change, but I don't want to bombard …

chipx86chipx86

We should type these, so they don't appear as literal Nones.

chipx86chipx86

This is missing an explicit return value.

chipx86chipx86

We should type this.

chipx86chipx86

TYPE_CHECKING sorts before Tuple alphabetically.

chipx86chipx86

We should type these.

chipx86chipx86
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

david
chipx86
  1. 
      
  2. reviewboard/diffviewer/diffutils.py (Diff revision 2)
     
     
     
     
     

    One blank line here (this is still import block territory)

  3. reviewboard/diffviewer/diffutils.py (Diff revision 2)
     
     

    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.

    1. This change feels unwieldy enough. I think I'd like to do that separately.

  4. reviewboard/diffviewer/diffutils.py (Diff revision 2)
     
     

    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.

    1. With __future__.annotations we can opt-in to this syntax now, at least for anything that's truly an annotation.

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

    Should be List[Dict].

  6. reviewboard/diffviewer/diffutils.py (Diff revision 2)
     
     

    Dict[str, Any]

  7. reviewboard/diffviewer/diffutils.py (Diff revision 2)
     
     

    Dict[str, Any]

  8. reviewboard/diffviewer/diffutils.py (Diff revision 2)
     
     

    Dict[str, Any]

  9. reviewboard/diffviewer/diffutils.py (Diff revision 2)
     
     

    Dict[str, Any]

  10. reviewboard/diffviewer/diffutils.py (Diff revision 2)
     
     

    Dict (though if we're going to type this, we should try to be more specific).

  11. reviewboard/diffviewer/diffutils.py (Diff revision 2)
     
     

    Dict[str, Any]

  12. reviewboard/diffviewer/diffutils.py (Diff revision 2)
     
     

    Dict[str, Any]

  13. reviewboard/reviews/context.py (Diff revision 2)
     
     
     
     
     
     
     
     

    Can we make these keyword-only?

  14. reviewboard/reviews/context.py (Diff revision 2)
     
     

    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.

  15. reviewboard/reviews/context.py (Diff revision 2)
     
     
     

    We should type these, so they don't appear as literal Nones.

  16. reviewboard/reviews/models/diff_comment.py (Diff revision 2)
     
     
     
     

    This is missing an explicit return value.

  17. We should type this.

  18. reviewboard/reviews/views/diff_fragments.py (Diff revision 2)
     
     
     

    We should type these.

  19. 
      
david
maubin
  1. Ship It!
  2. 
      
chipx86
  1. 
      
  2. reviewboard/reviews/views/diff_fragments.py (Diff revisions 2 - 3)
     
     

    TYPE_CHECKING sorts before Tuple alphabetically.

  3. 
      
david
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-7.x (f54b0e0)
Loading...