Skip to content
Snippets Groups Projects

Do not diff paths changed in merge commits

Merged Joe Woodward requested to merge fix/lfs-lock-issue into master
All threads resolved!

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 (closed)

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

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Joe Woodward added 822 commits

    added 822 commits

    • 8f5289fa...7ab62a20 - 821 commits from branch feat/find_changed_paths-with-all_parents-diff-mode
    • fbc365f8 - Do not diff paths changed in merge commits

    Compare with previous version

  • mentioned in issue #23625 (closed)

  • Contributor

    :warning: @jwoodwardgl Some end-to-end (E2E) tests have been selected based on the stage label on this MR. If not run already, please run the e2e:package-and-test-ee job in the qa stage and review the results before merging this MR. (E2E tests are not run automatically on some MRs due to runner resource constraints.)

    If you would like to run all e2e tests, please apply the pipeline:run-all-e2e label and restart the pipeline.

    Once done, please apply the :white_check_mark: emoji on this comment.

    For any questions or help in reviewing the E2E test results, please reach out on the internal #quality Slack channel.

  • Joe Woodward requested review from @ashmckenzie

    requested review from @ashmckenzie

  • Joe Woodward deleted the feat/find_changed_paths-with-all_parents-diff-mode branch. This merge request now targets the master branch

    deleted the feat/find_changed_paths-with-all_parents-diff-mode branch. This merge request now targets the master branch

  • Ash McKenzie changed the description

    changed the description

  • Ash McKenzie resolved all threads

    resolved all threads

  • Ash McKenzie
  • Ash McKenzie added 1 commit

    added 1 commit

    • 42f0de60 - Rename it description to be accurate

    Compare with previous version

  • LGTM @jwoodwardgl :thumbsup: As I'm still getting up-to-speed with this area of the codebase, I'd like another review before merging. I can't approve or merge anyway as I added 42f0de60 which has removed my eligibility for approving.

    Edited by Ash McKenzie
  • Ash McKenzie resolved all threads

    resolved all threads

  • Vasilii Iakliushin approved this merge request

    approved this merge request

  • Vasilii Iakliushin marked the checklist item I have evaluated the MR acceptance checklist for this MR. as completed

    marked the checklist item I have evaluated the MR acceptance checklist for this MR. as completed

  • :wave: @vyaklushin, thanks for approving this merge request.

    This is the first time the merge request is approved. To ensure full test coverage, a new pipeline will be started shortly.

    For more info, please refer to the following links:

  • Thank you for the fix! LGTM :rocket:

  • Vasilii Iakliushin enabled an automatic merge when the pipeline for b60fb2a0 succeeds

    enabled an automatic merge when the pipeline for b60fb2a0 succeeds

  • mentioned in commit 353e82ea

  • added workflowstaging label and removed workflowcanary label

  • mentioned in issue #415579 (closed)

  • Joe Woodward mentioned in merge request !129512 (merged)

    mentioned in merge request !129512 (merged)

  • mentioned in issue #423270 (closed)

  • Please register or sign in to reply
    Loading