Merge Requests should check if any files are locked before allowing the user to merge
Why is this issue being created?
Recently we resolved a bug with Git LFS locks and GitLab path locks. The issue occurred when user 1 had locked a file and user 2 then attempted to push a branch that included a merge commit with the locked file in it. Even though the locked file had not been modified, it was included in the list of changes so would reject the push.
To fix this gitaly implemented a new flag for the findChangedPaths GRPC endpoint to allow the client to specify if merge commits should be diffed against the current branch only or diffed against all parents. When diffing against all parents we only return files that have are different on all parents so merge commits will only return files which were manually added during the merge, amended after the merge, or a merge conflict resolution.
The problem occurs when this has been enabled. In fact, it already is an issue but is masked by the way our QA tests test the feature. When we create a merge request some checks are performed to ensure that the MR can be merged. If it cannot be merged yet we disable/hide the merge
button. However, during these checks we do not check if there are any locked files so we will allow the user to click the merge button when the changes include a locked file. One of the QA tests checks if after clicking merge an error is returned that the file is locked.
What is actually happening?
When the user clicks merge the backend optionally squashes and then merges the changes. This new merge commit is then pushed to the repository. During this push our DiffCheck logic checks if the changes include locked paths. Prior to the merge_commit_diff_modes
flag, this merge commit would include all files that have been changes in the MR. Now, we the changes will be empty because technically nothing has changed between the two branches, we've just merged them together. This is intentional behavior and is working correctly. The QA test fails because it expects to see an error.
What should happen?
Rather than showing the error after merging, we should check if any files are locked before allowing the user to click on the merge button.
Related Issues and MRs
Related Issues | |
---|---|
QA Failure | Failure in ee/browser_ui/3_create/repository/fi... (#422062 - closed) |
Feature Issue | Git LFS lock in GitLab prevents pushes of new f... (#23625) |
Feature MR in Gitaly | FindChangedPaths returns paths that are not cha... (gitaly#4827 - closed) |
GRPC MR in GitLab | Introduce find_changed_paths with merge_commit_... (!123501 - merged) |
GitLab Repository changes MR | Update Gitlab::Git::Repository#find_changed_pat... (!123548 - merged) |
GitLab DiffCheck changes MR | Do not diff paths changed in merge commits (!123947 - merged) |
FF enabled by default MR | Only check valid changes during diff checks (!129465 - merged) |
Revert FF enabled by default MR | Revert "Only check valid changes during diff ch... (!129512 - merged) |