Do not diff paths changed in merge commits
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.
- Create a project in you GDK
- Configure LFS both locally and in GDK
- As user1 create a branch named test
- Create a commit with file foo.text
- As user2 create a commit with a file bar.text, configure lfs to track text files, lock it in lfs
- 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)
- 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.
-
I have evaluated the MR acceptance checklist for this MR.
Merge request reports
Activity
changed milestone to %16.1
assigned to @jwoodwardgl
1 Warning This MR changes code in ee/
, but its Changelog commit is missing theEE: true
trailer. Consider adding it to your Changelog commits.Reviewer roulette
Changes that require review have been detected!
Please refer to the table below for assigning reviewers and maintainers suggested by Danger in the specified category:
Category Reviewer Maintainer backend Valery Sizov (
@vsizov
) (UTC+1, same timezone as@jwoodwardgl
)Douglas Barbosa Alexandre (
@dbalexandre
) (UTC+0, 1 hour behind@jwoodwardgl
)To spread load more evenly across eligible reviewers, Danger has picked a candidate for each review slot, based on their timezone. Feel free to override these selections if you think someone else would be better-suited or use the GitLab Review Workload Dashboard to find other available reviewers.
To read more on how to use the reviewer roulette, please take a look at the Engineering workflow and code review guidelines. Please consider assigning a reviewer or maintainer who is a domain expert in the area of the merge request.
Once you've decided who will review this merge request, assign them as a reviewer! Danger does not automatically notify them for you.
If needed, you can retry the
danger-review
job that generated this comment.Generated by
DangerAllure report
allure-report-publisher
generated test report!e2e-test-on-gdk:
test report for 42f0de60expand test summary
+-----------------------------------------------------------------------+ | suites summary | +------------------+--------+--------+---------+-------+-------+--------+ | | passed | failed | skipped | flaky | total | result | +------------------+--------+--------+---------+-------+-------+--------+ | Create | 8 | 0 | 1 | 6 | 9 | ❗ | | Data Stores | 2 | 0 | 0 | 1 | 2 | ❗ | | Monitor | 4 | 0 | 0 | 4 | 4 | ❗ | | Govern | 2 | 0 | 0 | 0 | 2 | ✅ | | Framework sanity | 0 | 0 | 1 | 0 | 1 | ➖ | | Plan | 4 | 0 | 0 | 3 | 4 | ❗ | | Manage | 1 | 0 | 0 | 0 | 1 | ✅ | +------------------+--------+--------+---------+-------+-------+--------+ | Total | 21 | 0 | 2 | 14 | 23 | ❗ | +------------------+--------+--------+---------+-------+-------+--------+
- Resolved by Ash McKenzie
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
-
8f5289fa...7ab62a20 - 821 commits from branch
mentioned in issue #23625 (closed)
@jwoodwardgl Some end-to-end (E2E) tests have been selected based on the stage label on this MR. If not run already, please run thee2e:package-and-test-ee
job in theqa
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
emoji on this comment.For any questions or help in reviewing the E2E test results, please reach out on the internal #quality Slack channel.
requested review from @ashmckenzie
- Resolved by Ash McKenzie
@ashmckenzie I've assigned you as reviewer. If you could review ASAP on Monday that would be great as I'm hoping (with some doubt) that I can sneak it into %16.1
If anyone else sees this in the meantime please go ahead and review.
Thanks!
- Resolved by Ash McKenzie
- Resolved by Ash McKenzie
LGTM @jwoodwardgl
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 McKenziemarked the checklist item I have evaluated the MR acceptance checklist for this MR. as completed
@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:
added pipeline:mr-approved label
enabled an automatic merge when the pipeline for b60fb2a0 succeeds
mentioned in commit 353e82ea
added workflowstaging-canary label
added workflowcanary label and removed workflowstaging-canary label
added workflowstaging label and removed workflowcanary label
added workflowproduction label and removed workflowstaging label
added workflowpost-deploy-db-production label and removed workflowproduction label
mentioned in merge request gitlab-com/www-gitlab-com!122228 (merged)
added releasedcandidate label
mentioned in merge request kubitus-project/kubitus-installer!2224 (merged)
mentioned in issue #415579 (closed)
mentioned in merge request !129512 (merged)
added releasedpublished label and removed releasedcandidate label
mentioned in issue #423270 (closed)