Introduce new mergeability check to ensure MR does not contain locked files
This is a follow-up from spike Merge Requests should check if any files are lo... (#423270 - closed)
Context
The issue happened when a user creates a branch, commits some changes, then before pushing to GitLab the user pulls in the latest changes from the source branch. If the latest changes include any locked files GitLab will reject the push even though the user has not actually changed these files.
To solve this we have added a new GRPC in Gitaly to allow us to exclude merge commits when getting a list of changed paths. This allows us to only show changes that the user has made, including any additional merged/amended changes that don't exist in the source branch.
This feature works, however, when we enabled the feature by default on production our QA suite failed. We discovered that the merge request's mergeability checks do not check if there are any locked files. Instead, when a user clicks on "Merge" a merge commit is created and then "pushed" to the repository. Prior to the changes, this merge commit would be rejected and the MR UI would display an error. However, now we exclude merge commits from the list of changed paths no error is raised.
Outcome of investigation
We need to introduce a new mergeability check before allowing a user to merge, however, there are a few different ways to handle this and this will affect the UX so we are currently looking for input from other teams/members to determine the best solution.
The mergeability check will be similar to our other mergeability checks e.g. MergeRequests::Mergeability::CheckDraftStatusService. We need to check if there are any LfsFileLock or PathLock records for the MR's changed paths.
Logically the user who created the lock should be able to modify the files and merge the changes without unlocking the files.
If a user creates a branch, commits, and creates an MR we can fairly easily determine if the locked files can be merged, we would simply list all of the changed paths and then check if any of those paths are locked by any other user.
If a user collaborates on a branch this becomes more complicated as we will need to match committed changes and locks against more than one user.
We could:
- Treat the MR author as the owner and prevent merging if the changed paths are locked by anyone else.
- Cycle through each commit and check if the anyone other than the committer has locked a changed path for that commit.
- Only allow merging locked files if there's a single committer.
My thoughts:
- Likely makes sense but we need to ensure that we're not bypassing these locks e.g. I create an MR, then anther user modifies a locked file, adds it to my MR and then has their partner in crime approve and merge it.
- Feels like it might introduce the least friction but is also the most complex solution. We could split this into a multifaceted solution where we first check if there are more than one committers, if there's only one we can batch the check against all the changes, if there are more than one we would need perform the checks on each commit (or group them by committer)
- Causes some friction if users are collaborating on a branch but simple to explain behavior to the users.