Skip to content

Comment on merge-ref diffs

Problem to solve

Follow up to #27008 (closed)

Merge request diffs are currently calculated by git diff target...source which compares HEAD of target with the merge base of target and source. This works well until changes from the target branch are merged in to the source branch, creating a complete mess of the diff.

In #27008 (closed), we implemented a new diff mode to fix this, but it doesn't support commenting, and is not the default mode because of this.

Proposal

  • Add the ability to view and add comments to the merge-ref diff
  • Add a feature flag so that we can make the merge-ref diff the default diff mode for specific projects

Since the default diff mode is reliable in most instances, very few people will try a new mode. So that we can ultimately make the merge-ref mode the default and remove the current default mode, we need to test it at scale. Using a feature flag will allow us to access performance, scalability, and reliability of this feature.

Availability & Testing

Risk to availability should be sufficiently mitigated by gradually rolling out the change behind a feature flag, and by users having the option to return to the original diff mode.

Given past and present problems with comments disappearing from the discussion history (e.g., after a rebase), we should confirm that we have suitable test coverage of typical and edge cases, and that the coverage if valid (or added) for the new diff mode, including that:

  • comments are still shown on the diff after changes (commit/merge/rebase/squash/revert) to the target or source (assuming the line with the comment wasn't removed)
  • comments are always accessible on the Discussion/Overview tab (even after changes that hide them on the diff)

We only have limited end-to-end test coverage of diff comments, so this would be a good opportunity to add more. However, unit/integration test coverage should be sufficient, so this work shouldn't wait for an end-to-end test to be ready.

  • package-and-qa should be run before merging to check if existing end-to-end tests need to be updated.

Links / references

Edited by Mark Lapierre