• 
      

    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)
       
       
       
       
       
      Show all issues

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

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

      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)
       
       
      Show all issues

      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)
       
       
      Show all issues

      Should be List[Dict].

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

      Dict[str, Any]

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

      Dict[str, Any]

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

      Dict[str, Any]

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

      Dict[str, Any]

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

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

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

      Dict[str, Any]

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

      Dict[str, Any]

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

      Can we make these keyword-only?

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

      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)
       
       
       
      Show all issues

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

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

      This is missing an explicit return value.

    17. Show all issues

      We should type this.

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

      We should type these.

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

      TYPE_CHECKING sorts before Tuple alphabetically.

    3. 
        
    david
    Review request changed
    Status:
    Completed
    Change Summary:
    Pushed to release-7.x (f54b0e0)