Skip to content

Do not diff paths changed in merge commits

Joe Woodward requested to merge fix/lfs-lock-issue into master

What does this MR do and why?

When a merge commit passes through our diff checks we would previously check each path changed in the merge commit. However, our diffing logic would return any path that was changed in any parent, meaning changes that are already in history were being detected as new changes.

Gitaly recently introduced a new flag for the FindChangedPaths GRPC to allow us to diff against all_parents, meaning, only new changes in a merge commit would be detected. With new changes meaning a change that did not exist in all of the parents i.e. if a user merges and fixes a merge conflict with code different to any of the parents, or they manually edited, created, or deleted a file during the merge.

This allows us to correctly execute push rules against new changes only.

Fixes #23625

Changelog: fixed

How to set up and validate locally

Numbered steps to set up and validate the change are strongly suggested.

  1. Create a project in you GDK
  2. Configure LFS both locally and in GDK
  3. As user1 create a branch named test
  4. Create a commit with file foo.text
  5. As user2 create a commit with a file bar.text, configure lfs to track text files, lock it in lfs
  6. As user2 push a commit to the target branch with an LFS file and lock it (you can also mock this by creating the LfsFileLock record manually after pushing a commit yourself but lock it by another user)
  7. As user1 merge the target branch into your branch, then push to GDK

This push should be rejected saying the file user2 pushed is locked

Enable the feature flag:

Feature.enable(:merge_commit_diff_modes)

Push again, now it should be accepted

MR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

Edited by Vasilii Iakliushin

Merge request reports