merge-request-integration icon indicating copy to clipboard operation
merge-request-integration copied to clipboard

Comments made on content of non latest commit are not visible

Open bironran opened this issue 5 years ago • 2 comments

Consider the following timeline in an MR: T0. Baseline commit C0. T1. commit C1, did not touch file A T2. commit C2, touched filed A T3. commit C3, did not touch file A T4. Comment made on the new version of file A.

When gathering the discussions in GitlabDiscussionTransformer.transform() the headSha is going to be C3 as that was the last commit in T4.

When considering which info to display CodeReviewManagerImpl.findChangeInfoByPathAndContent() is going to return:

  1. for the before (left) panel - commit revision is going to be C1 (before we touched A)
  2. for the after (right) panel - commit revision is going to be C2 (after we touched A).

However, in CodeReviewManagerImpl.findCommentPoints(), when comparing the after's panel changeInfo.contentRevision.revisionNumber (which is C2) with the comment.position (which is C3) we can't find a match, thus the comment disappear.

Of course, the same happens for any comment that is made on a file modified in a commit which is not the HEAD when the comment is added.

It's worth to note that none of base_sha, start_sha or head_sha work in this case. I don't know the logic in gitlab when deciding which discussion to show in the panel but I suspect it's similar to "git blame" - see when the line referenced in the discussion was last changed and decide based on that which commit to attach it to.

I consider this a critical issue for me as it blocks all non-trivial MRs that span multiple commits.

bironran avatar Feb 08 '20 06:02 bironran

Thank you for reporting the problem. I'll try to reproduce and fix it.

There is a feature that you can review 1 or some commits by following steps:

  • Select the MR
  • Choose Commits tab
  • Select 1 or some commits
  • After that, the "Code Review" button will display "Code Review xx/yy commits"

I hope the feature unblocks you for now.

nhat-phan avatar Feb 08 '20 10:02 nhat-phan

"Review 1 commit" doesn't actually work as the base_sha of the comment (made at T4, to C3) and the sha of the file (C2) don't match, so the commit won't show. Also, local squash after clicking "review" doesn't work either as the above still happens.

A partial workaround could be to ignore the sha and just display the comments where they fall. perhaps hidden under a flag in settings. I tested this compiling a custom version and removing the if (position.headHash == changeInfo.contentRevision.revisionNumber.asString()) from CodeReviewManagerImpl.kt - comments appear on both facets which is not ideal, but it works better and I don't miss comments on non-renamed files.

Edit: I found out the strategy in https://gitlab.com/gitlab-org/gitlab/-/blob/e5ecbae93589d2cf44f8653669ca7e48fd13701f/lib/gitlab/diff/position_tracer/line_strategy.rb

bironran avatar Feb 10 '20 19:02 bironran