Don't lose commit-related comments on rebase
Description
When rebasing a MR, the hashes change, so any comments on the old commits are no longer visible on the MR.
Implementation
We don't need to care about new commit comments on commits that used to be in the MR, just commit comments that were on the MR that we're losing right now. (I realise this is a confusing sentence!)
That means we can look up the MR's commit comments when one of the following happens:
- A push.
- A change to the target branch.
- A change to the source branch.
If any of these happen, we will create a new merge request version. Before we do that, we can take the existing MR version and look at the comments associated with its commits (like we do when displaying the MR). If any of those commits are not in the new MR version that we are creating, we can then link those commit comments to the MR using a new join table, called something like merge_requests_commit_notes
.
When we display the MR's discussion tab after this happens, we can look up commit comments from the join table and display those as collapsed commit comments. They aren't outdated, strictly - the UI should say that they are on commits not present in the current MR version.
One caveat to this is that if a force push adds a commit back to the MR, that was previously in the MR and then removed, we should remove the entries from the join table.
Comments
The core problem here as I see it is that the current behaviour is completely unintuitive to users. Comments never disappear from the 'discussion' tab of the merge request, apart from this one case.
It is very confusing to users to be sure they had a discussion about the merge request, and that they definitely saw that conversation on the merge request discussion tab, but to look later and find that discussion has vanished without trace. (It's really really bad if, as happened to us, the comment was made by the project lead, and said "This code must not be merged"... the person who merged it checked the full discussion history before merging it, and that comment was no longer there due to the rebase, resulting in the code getting merged by another user.)
It doesn't matter if the comments are outdated or not relevant. By comparison, if I make a comment via the changes tab, and then all the relevant code is removed, the full comment still appears on the discussion tab, even though it's completely outdated and irrelevant. This isn't a problem - from the user's perspective, the expectation is that the discussion tab should be a record of all the discussion. If the initial fix turned out to be completely wrong, the reasoning why it's wrong is important information to be kept in the discussion. (github is this case actually adds a couple of ui indications when a comment is on code that's now been removed, which also makes sense to me.)
I've tried to suggesting to our users that they avoid making comments via the commits tab, but they've strongly pushed back on this as in cases of large pull requests that consistent of many commits it is only realistic to review via the commits tab.