updateref: Drop use of Ruby-style error

When we have migrated the Operations service into Gitaly, many of the
RPCs were embedding errors into a successful response via a special
field. These errors were in fact consumed and often also displayed by
Rails, which meant that we were not in a position to actually change
them. This led to a bunch of error messages that don't really carry a
lot of information.

We have since migrated most of the RPCs to instead use structured errors
and are thus free to change those errors to be more meaningful. One such
instance is the dreaded "Could not update $ref. Please refresh and try
again."  error message. This error can indeed be caused by a multitude
of different errors, but what the actual root cause is is quite unclear.

Refactor the code and get rid of this error, instead propagating the
actual root cause to the caller. This greatly clarifies the reason why a
reference update has failed and should help us debug such cases better
in the future.

Note that there is once instance left with `UserRevert()` where we have
to return the old-style error as it is still getting embedded into a
successful response. This commit simply inlines that error and adds a
TODO comment on top of it.

9 files changed, 17 insertions, 13 deletions

diff --git a/internal/git/updateref/update_with_hooks.go b/internal/git/updateref/update_with_hooks.go
index 6efa375c6..4c8201916 100644
--- a/internal/git/updateref/update_with_hooks.go
+++ b/internal/git/updateref/update_with_hooks.go
@@ -125,7 +125,7 @@ func (e Error) Unwrap() error {
 }
 
 func (e Error) Error() string {
-	return fmt.Sprintf("Could not update %s. Please refresh and try again.", e.Reference)
+	return fmt.Sprintf("reference update: %v", e.Cause.Error())
 }
 
 // NewUpdaterWithHooks creates a new instance of a struct that will update a Git reference.
diff --git a/internal/gitaly/service/operations/apply_patch_test.go b/internal/gitaly/service/operations/apply_patch_test.go
index 7bd18fd0f..b8dffa014 100644
--- a/internal/gitaly/service/operations/apply_patch_test.go
+++ b/internal/gitaly/service/operations/apply_patch_test.go
@@ -440,7 +440,7 @@ To restore the original branch and stop patching, run "git am --abort".
 				return expected{
 					oldOID: currentCommit,
 					err: testhelper.WithInterceptedMetadataItems(
-						structerr.NewInternal(`update reference: Could not update %s. Please refresh and try again.`, git.DefaultRef),
+						structerr.NewInternal("update reference: reference update: reference does not point to expected object"),
 						structerr.MetadataItem{Key: "actual_object_id", Value: futureCommit},
 						structerr.MetadataItem{Key: "expected_object_id", Value: currentCommit},
 						structerr.MetadataItem{Key: "reference", Value: "refs/heads/main"},
diff --git a/internal/gitaly/service/operations/branches_test.go b/internal/gitaly/service/operations/branches_test.go
index f89b58073..970db09b5 100644
--- a/internal/gitaly/service/operations/branches_test.go
+++ b/internal/gitaly/service/operations/branches_test.go
@@ -6,6 +6,7 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"io"
 	"strings"
 	"testing"
 
@@ -390,7 +391,7 @@ func TestUserCreateBranch_Failure(t *testing.T) {
 			startPoint: "master",
 			user:       gittest.TestUser,
 			err: testhelper.WithInterceptedMetadata(
-				structerr.NewFailedPrecondition("Could not update refs/heads/master. Please refresh and try again."),
+				structerr.NewFailedPrecondition("reference update: state update to %q failed: %w", "prepare", io.EOF),
 				"stderr",
 				"fatal: prepare: cannot lock ref 'refs/heads/master': reference already exists\n",
 			),
@@ -402,7 +403,7 @@ func TestUserCreateBranch_Failure(t *testing.T) {
 			startPoint: "master",
 			user:       gittest.TestUser,
 			err: testhelper.WithInterceptedMetadataItems(
-				structerr.NewFailedPrecondition("Could not update refs/heads/improve. Please refresh and try again."),
+				structerr.NewFailedPrecondition("reference update: file directory conflict"),
 				structerr.MetadataItem{Key: "conflicting_reference", Value: "refs/heads/improve"},
 				structerr.MetadataItem{Key: "existing_reference", Value: "refs/heads/improve/awesome"},
 			),
@@ -610,7 +611,7 @@ func TestUserDeleteBranch(t *testing.T) {
 						ExpectedOldOid: firstCommit.String(),
 					},
 					repoPath: repoPath,
-					expectedErr: structerr.NewFailedPrecondition("reference update failed: Could not update refs/heads/%s. Please refresh and try again.", branchName).
+					expectedErr: structerr.NewFailedPrecondition("reference update failed: reference update: reference does not point to expected object").
 						WithDetail(&testproto.ErrorMetadata{
 							Key:   []byte("actual_object_id"),
 							Value: []byte(secondCommit),
@@ -759,7 +760,7 @@ func TestUserDeleteBranch_concurrentUpdate(t *testing.T) {
 		BranchName: []byte("concurrent-update"),
 		User:       gittest.TestUser,
 	})
-	testhelper.RequireGrpcError(t, structerr.NewFailedPrecondition("reference update failed: Could not update refs/heads/concurrent-update. Please refresh and try again.").
+	testhelper.RequireGrpcError(t, structerr.NewFailedPrecondition("reference update failed: reference update: reference is already locked").
 		WithDetail(&testproto.ErrorMetadata{
 			Key:   []byte("reference"),
 			Value: []byte("refs/heads/concurrent-update"),
diff --git a/internal/gitaly/service/operations/cherry_pick_test.go b/internal/gitaly/service/operations/cherry_pick_test.go
index 65eccd0e8..267e56b78 100644
--- a/internal/gitaly/service/operations/cherry_pick_test.go
+++ b/internal/gitaly/service/operations/cherry_pick_test.go
@@ -270,7 +270,7 @@ func TestUserCherryPick(t *testing.T) {
 					},
 					&gitalypb.UserCherryPickResponse{},
 					testhelper.WithInterceptedMetadataItems(
-						structerr.NewInternal("update reference with hooks: Could not update refs/heads/master. Please refresh and try again."),
+						structerr.NewInternal("update reference with hooks: reference update: reference does not point to expected object"),
 						structerr.MetadataItem{Key: "actual_object_id", Value: commit},
 						structerr.MetadataItem{Key: "expected_object_id", Value: data.masterCommit},
 						structerr.MetadataItem{Key: "reference", Value: "refs/heads/master"},
diff --git a/internal/gitaly/service/operations/commit_files_test.go b/internal/gitaly/service/operations/commit_files_test.go
index 93f416aaa..92bef38bb 100644
--- a/internal/gitaly/service/operations/commit_files_test.go
+++ b/internal/gitaly/service/operations/commit_files_test.go
@@ -1165,7 +1165,7 @@ func testSuccessfulUserCommitFilesRequest(t *testing.T, ctx context.Context) {
 			branchName:     "few-commits",
 			expectedOldOID: text.ChompBytes(gittest.Exec(t, cfg, "-C", repoPath, "rev-parse", "refs/heads/few-commits~1")),
 			expectedError: testhelper.WithInterceptedMetadataItems(
-				structerr.NewFailedPrecondition("Could not update refs/heads/few-commits. Please refresh and try again."),
+				structerr.NewFailedPrecondition("reference update: reference does not point to expected object"),
 				structerr.MetadataItem{Key: "actual_object_id", Value: "0031876facac3f2b2702a0e53a26e89939a42209"},
 				structerr.MetadataItem{Key: "expected_object_id", Value: "bf6e164cac2dc32b1f391ca4290badcbe4ffc5fb"},
 				structerr.MetadataItem{Key: "reference", Value: "refs/heads/few-commits"},
diff --git a/internal/gitaly/service/operations/merge_test.go b/internal/gitaly/service/operations/merge_test.go
index 3d4ed3952..43b7e11d5 100644
--- a/internal/gitaly/service/operations/merge_test.go
+++ b/internal/gitaly/service/operations/merge_test.go
@@ -191,7 +191,7 @@ func testUserMergeBranch(t *testing.T, ctx context.Context) {
 					secondRequest:          &gitalypb.UserMergeBranchRequest{Apply: true},
 					secondExpectedResponse: &gitalypb.OperationBranchUpdate{},
 					secondExpectedErr: func(response *gitalypb.UserMergeBranchResponse) error {
-						return structerr.NewFailedPrecondition("Could not update refs/heads/master. Please refresh and try again.").
+						return structerr.NewFailedPrecondition("reference update: reference does not point to expected object").
 							WithDetail(&testproto.ErrorMetadata{
 								Key:   []byte("actual_object_id"),
 								Value: []byte(secondCommit),
@@ -727,7 +727,7 @@ func testUserMergeBranchConcurrentUpdate(t *testing.T, ctx context.Context) {
 
 	secondResponse, err := mergeBidi.Recv()
 	testhelper.RequireGrpcError(t,
-		structerr.NewFailedPrecondition("Could not update refs/heads/gitaly-merge-test-branch. Please refresh and try again.").
+		structerr.NewFailedPrecondition("reference update: reference does not point to expected object").
 			WithDetail(&testproto.ErrorMetadata{
 				Key:   []byte("actual_object_id"),
 				Value: []byte(concurrentCommitID),
diff --git a/internal/gitaly/service/operations/revert_test.go b/internal/gitaly/service/operations/revert_test.go
index 4f274e58e..010e5f183 100644
--- a/internal/gitaly/service/operations/revert_test.go
+++ b/internal/gitaly/service/operations/revert_test.go
@@ -368,7 +368,7 @@ func TestUserRevert(t *testing.T) {
 						ExpectedOldOid: firstCommitID.String(),
 					},
 					expectedError: testhelper.WithInterceptedMetadataItems(
-						structerr.NewInternal("update reference with hooks: Could not update refs/heads/%s. Please refresh and try again.", branchName),
+						structerr.NewInternal("update reference with hooks: reference update: reference does not point to expected object"),
 						structerr.MetadataItem{Key: "actual_object_id", Value: secondCommitID},
 						structerr.MetadataItem{Key: "expected_object_id", Value: firstCommitID},
 						structerr.MetadataItem{Key: "reference", Value: "refs/heads/" + branchName},
diff --git a/internal/gitaly/service/operations/submodules.go b/internal/gitaly/service/operations/submodules.go
index 2de4167d6..036129fea 100644
--- a/internal/gitaly/service/operations/submodules.go
+++ b/internal/gitaly/service/operations/submodules.go
@@ -253,7 +253,10 @@ func (s *Server) userUpdateSubmodule(ctx context.Context, req *gitalypb.UserUpda
 		var updateRefError updateref.Error
 		if errors.As(err, &updateRefError) {
 			return &gitalypb.UserUpdateSubmoduleResponse{
-				CommitError: err.Error(),
+				// TODO: this needs to be converted to a structured error, and once done we should stop
+				// returning this Ruby-esque error message in favor of the actual error that was
+				// returned by `updateReferenceWithHooks()`.
+				CommitError: fmt.Sprintf("Could not update %s. Please refresh and try again.", updateRefError.Reference),
 			}, nil
 		}
 
diff --git a/internal/gitaly/service/operations/tags_test.go b/internal/gitaly/service/operations/tags_test.go
index 77890ee71..ba8a9f756 100644
--- a/internal/gitaly/service/operations/tags_test.go
+++ b/internal/gitaly/service/operations/tags_test.go
@@ -268,7 +268,7 @@ func TestUserDeleteTag(t *testing.T) {
 						ExpectedOldOid: firstCommit.String(),
 					},
 					expectedError: testhelper.WithInterceptedMetadataItems(
-						structerr.NewFailedPrecondition("Could not update refs/tags/%s. Please refresh and try again.", tagName),
+						structerr.NewFailedPrecondition("reference update: reference does not point to expected object"),
 						structerr.MetadataItem{Key: "actual_object_id", Value: secondCommit},
 						structerr.MetadataItem{Key: "expected_object_id", Value: firstCommit},
 						structerr.MetadataItem{Key: "reference", Value: "refs/tags/" + tagName},