Pre-receive error messages in /internal/allowed API check are not propagated in UserMergeBranch
From gitlab#30141 (comment 481566895), when a file is locked and a user attempts to merge a locked file/directory, we no longer give proper feedback to the user. Instead, the user sees the incorrect message, Conflicts detected during merge
:
This is actually been a serious problem we worked out in the Ruby implementation because for numerous customers, this mysterious failure costed a lot of engineering time to troubleshoot.
In https://gitlab.com/gitlab-org/gitaly/blob/b0bc161d3f4faa3c4a23da73e4f29632519b8586/internal/gitaly/hook/prereceive.go#L113-116, the GitLab API allowed call receives a message, and this is returned with a clean error message:
GitLab: The path 'test' is locked by Administrator
However, in https://gitlab.com/gitlab-org/gitaly/blob/8e8146470716158e12cf06d7997c47dcb2b96bb9/internal/gitaly/service/operations/update_with_hooks.go#L65-68, we look for the error in the stdout and stderr stream, but these are empty because the error came from the network response, not any external command.
I verified this patch fixes the issue:
diff --git a/internal/gitaly/service/operations/update_with_hooks.go b/internal/gitaly/service/operations/update_with_hooks.go
index 55021714b..299f05516 100644
--- a/internal/gitaly/service/operations/update_with_hooks.go
+++ b/internal/gitaly/service/operations/update_with_hooks.go
@@ -64,6 +64,9 @@ func (s *Server) updateReferenceWithHooks(ctx context.Context, repo *gitalypb.Re
if err := s.hookManager.PreReceiveHook(ctx, repo, nil, env, strings.NewReader(changes), &stdout, &stderr); err != nil {
msg := hookErrorFromStdoutAndStderr(stdout.String(), stderr.String())
+ if msg == "" {
+ msg = err.Error()
+ }
return preReceiveError{message: msg}
}
if err := s.hookManager.UpdateHook(ctx, repo, reference, oldrev, newrev, env, &stdout, &stderr); err != nil {
This is okay because GitLab Rails will filter any messages that don't start with GitLab:
.
Another option may be to create a special error, NotAllowedError
, and then treat that as a special case.