updateref: Detect mismatched state errors

We don't gracefully handle the case where updating a reference fails
because the expected value the reference points to is different from the
actual value. Detect this case and return a proper error type so that we
provide better error reporting.

9 files changed, 90 insertions, 24 deletions

diff --git a/internal/git/updateref/updateref.go b/internal/git/updateref/updateref.go
index 0627451d4..d61bee5d7 100644
--- a/internal/git/updateref/updateref.go
+++ b/internal/git/updateref/updateref.go
@@ -86,6 +86,31 @@ func (e NonCommitObjectError) Error() string {
 	return fmt.Sprintf("pointed branch %q to a non-commit object %q", e.ReferenceName, e.ObjectID)
 }
 
+// MismatchingStateError is returned when attempting to update a reference where the expected object ID does not match
+// the actual object ID that the reference currently points to.
+type MismatchingStateError struct {
+	// ReferenceName is the name of the reference that was being updated.
+	ReferenceName string
+	// ExpectedObjectID is the expected object ID as specified by the caller.
+	ExpectedObjectID string
+	// ActualObjectID is the actual object ID that the reference was pointing to.
+	ActualObjectID string
+}
+
+func (e MismatchingStateError) Error() string {
+	return "reference does not point to expected object"
+}
+
+// ErrorMetadata implements the `structerr.ErrorMetadater` interface and provides error metadata about the expected and
+// actual object ID of the failed reference update.
+func (e MismatchingStateError) ErrorMetadata() []structerr.MetadataItem {
+	return []structerr.MetadataItem{
+		{Key: "reference", Value: e.ReferenceName},
+		{Key: "expected_object_id", Value: e.ExpectedObjectID},
+		{Key: "actual_object_id", Value: e.ActualObjectID},
+	}
+}
+
 // state represents a possible state the updater can be in.
 type state string
 
@@ -333,6 +358,7 @@ var (
 	inTransactionConflictRegex   = regexp.MustCompile(`^fatal: .*: cannot lock ref '.*': cannot process '(.*)' and '(.*)' at the same time\n$`)
 	nonExistentObjectRegex       = regexp.MustCompile(`^fatal: .*: cannot update ref '.*': trying to write ref '(.*)' with nonexistent object (.*)\n$`)
 	nonCommitObjectRegex         = regexp.MustCompile(`^fatal: .*: cannot update ref '.*': trying to write non-commit object (.*) to branch '(.*)'\n`)
+	mismatchingStateRegex        = regexp.MustCompile(`^fatal: .*: cannot lock ref '(.*)': is at (.*) but expected (.*)\n$`)
 )
 
 func (u *Updater) setState(state string) error {
@@ -422,5 +448,14 @@ func (u *Updater) handleIOError(fallbackErr error) error {
 		}
 	}
 
+	matches = mismatchingStateRegex.FindSubmatch(stderr)
+	if len(matches) > 2 {
+		return MismatchingStateError{
+			ReferenceName:    string(matches[1]),
+			ExpectedObjectID: string(matches[3]),
+			ActualObjectID:   string(matches[2]),
+		}
+	}
+
 	return structerr.New("%w", fallbackErr).WithMetadata("stderr", string(stderr))
 }
diff --git a/internal/git/updateref/updateref_test.go b/internal/git/updateref/updateref_test.go
index 77ef4bc8f..0a409acc2 100644
--- a/internal/git/updateref/updateref_test.go
+++ b/internal/git/updateref/updateref_test.go
@@ -328,9 +328,11 @@ func TestUpdater_update(t *testing.T) {
 	require.NoError(t, updater.Start())
 	require.NoError(t, updater.Update("refs/heads/main", newCommitID, otherCommitID))
 
-	require.Equal(t, structerr.New("%w", fmt.Errorf("state update to %q failed: %w", "commit", io.EOF)).WithMetadata(
-		"stderr", fmt.Sprintf("fatal: commit: cannot lock ref 'refs/heads/main': is at %s but expected %s\n", oldCommitID, otherCommitID),
-	), updater.Commit())
+	require.Equal(t, MismatchingStateError{
+		ReferenceName:    "refs/heads/main",
+		ExpectedObjectID: otherCommitID.String(),
+		ActualObjectID:   oldCommitID.String(),
+	}, updater.Commit())
 	require.Equal(t, invalidStateTransitionError{expected: stateIdle, actual: stateClosed}, updater.Start())
 
 	require.Equal(t, gittest.ResolveRevision(t, cfg, repoPath, "refs/heads/main"), oldCommitID)
diff --git a/internal/gitaly/service/operations/apply_patch_test.go b/internal/gitaly/service/operations/apply_patch_test.go
index 68f1265dc..7bd18fd0f 100644
--- a/internal/gitaly/service/operations/apply_patch_test.go
+++ b/internal/gitaly/service/operations/apply_patch_test.go
@@ -439,10 +439,11 @@ To restore the original branch and stop patching, run "git am --abort".
 				futureCommit := gittest.WriteCommit(t, cfg, repoPath, gittest.WithParents(git.ObjectID(currentCommit)), gittest.WithBranch(git.DefaultBranch))
 				return expected{
 					oldOID: currentCommit,
-					err: testhelper.WithInterceptedMetadata(
+					err: testhelper.WithInterceptedMetadataItems(
 						structerr.NewInternal(`update reference: Could not update %s. Please refresh and try again.`, git.DefaultRef),
-						"stderr",
-						fmt.Sprintf("fatal: prepare: cannot lock ref 'refs/heads/main': is at %s but expected %s\n", futureCommit, currentCommit),
+						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 4501e356a..c31f4ea87 100644
--- a/internal/gitaly/service/operations/branches_test.go
+++ b/internal/gitaly/service/operations/branches_test.go
@@ -608,8 +608,16 @@ func TestUserDeleteBranch(t *testing.T) {
 					repoPath: repoPath,
 					expectedErr: structerr.NewFailedPrecondition("reference update failed: Could not update refs/heads/%s. Please refresh and try again.", branchName).
 						WithDetail(&testproto.ErrorMetadata{
-							Key:   []byte("stderr"),
-							Value: []byte(fmt.Sprintf("fatal: prepare: cannot lock ref 'refs/heads/%s': is at %s but expected %s\n", branchName, secondCommit, firstCommit)),
+							Key:   []byte("actual_object_id"),
+							Value: []byte(secondCommit),
+						}).
+						WithDetail(&testproto.ErrorMetadata{
+							Key:   []byte("expected_object_id"),
+							Value: []byte(firstCommit),
+						}).
+						WithDetail(&testproto.ErrorMetadata{
+							Key:   []byte("reference"),
+							Value: []byte("refs/heads/" + branchName),
 						}).
 						WithDetail(&gitalypb.UserDeleteBranchError{
 							Error: &gitalypb.UserDeleteBranchError_ReferenceUpdate{
diff --git a/internal/gitaly/service/operations/cherry_pick_test.go b/internal/gitaly/service/operations/cherry_pick_test.go
index 0f26d527a..65eccd0e8 100644
--- a/internal/gitaly/service/operations/cherry_pick_test.go
+++ b/internal/gitaly/service/operations/cherry_pick_test.go
@@ -269,10 +269,11 @@ func TestUserCherryPick(t *testing.T) {
 						ExpectedOldOid: data.masterCommit,
 					},
 					&gitalypb.UserCherryPickResponse{},
-					testhelper.WithInterceptedMetadata(
+					testhelper.WithInterceptedMetadataItems(
 						structerr.NewInternal("update reference with hooks: Could not update refs/heads/master. Please refresh and try again."),
-						"stderr",
-						fmt.Sprintf("fatal: prepare: cannot lock ref 'refs/heads/master': is at %s but expected %s\n", commit, data.masterCommit),
+						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 bee801b5b..93f416aaa 100644
--- a/internal/gitaly/service/operations/commit_files_test.go
+++ b/internal/gitaly/service/operations/commit_files_test.go
@@ -1164,10 +1164,11 @@ func testSuccessfulUserCommitFilesRequest(t *testing.T, ctx context.Context) {
 			repoPath:       repoPath,
 			branchName:     "few-commits",
 			expectedOldOID: text.ChompBytes(gittest.Exec(t, cfg, "-C", repoPath, "rev-parse", "refs/heads/few-commits~1")),
-			expectedError: testhelper.WithInterceptedMetadata(
+			expectedError: testhelper.WithInterceptedMetadataItems(
 				structerr.NewFailedPrecondition("Could not update refs/heads/few-commits. Please refresh and try again."),
-				"stderr",
-				"fatal: prepare: cannot lock ref 'refs/heads/few-commits': is at 0031876facac3f2b2702a0e53a26e89939a42209 but expected bf6e164cac2dc32b1f391ca4290badcbe4ffc5fb\n",
+				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 948df2aac..3d4ed3952 100644
--- a/internal/gitaly/service/operations/merge_test.go
+++ b/internal/gitaly/service/operations/merge_test.go
@@ -193,8 +193,16 @@ func testUserMergeBranch(t *testing.T, ctx context.Context) {
 					secondExpectedErr: func(response *gitalypb.UserMergeBranchResponse) error {
 						return structerr.NewFailedPrecondition("Could not update refs/heads/master. Please refresh and try again.").
 							WithDetail(&testproto.ErrorMetadata{
-								Key:   []byte("stderr"),
-								Value: []byte(fmt.Sprintf("fatal: prepare: cannot lock ref 'refs/heads/master': is at %s but expected %s\n", secondCommit, data.masterCommit)),
+								Key:   []byte("actual_object_id"),
+								Value: []byte(secondCommit),
+							}).
+							WithDetail(&testproto.ErrorMetadata{
+								Key:   []byte("expected_object_id"),
+								Value: []byte(data.masterCommit),
+							}).
+							WithDetail(&testproto.ErrorMetadata{
+								Key:   []byte("reference"),
+								Value: []byte("refs/heads/" + data.branch),
 							}).
 							WithDetail(&gitalypb.UserMergeBranchError{
 								Error: &gitalypb.UserMergeBranchError_ReferenceUpdate{
@@ -721,8 +729,16 @@ func testUserMergeBranchConcurrentUpdate(t *testing.T, ctx context.Context) {
 	testhelper.RequireGrpcError(t,
 		structerr.NewFailedPrecondition("Could not update refs/heads/gitaly-merge-test-branch. Please refresh and try again.").
 			WithDetail(&testproto.ErrorMetadata{
-				Key:   []byte("stderr"),
-				Value: []byte("fatal: prepare: cannot lock ref 'refs/heads/gitaly-merge-test-branch': is at 549090fbeacc6607bc70648d3ba554c355e670c5 but expected 281d3a76f31c812dbf48abce82ccf6860adedd81\n"),
+				Key:   []byte("actual_object_id"),
+				Value: []byte(concurrentCommitID),
+			}).
+			WithDetail(&testproto.ErrorMetadata{
+				Key:   []byte("expected_object_id"),
+				Value: []byte("281d3a76f31c812dbf48abce82ccf6860adedd81"),
+			}).
+			WithDetail(&testproto.ErrorMetadata{
+				Key:   []byte("reference"),
+				Value: []byte("refs/heads/" + mergeBranchName),
 			}).
 			WithDetail(&gitalypb.UserMergeBranchError{
 				Error: &gitalypb.UserMergeBranchError_ReferenceUpdate{
diff --git a/internal/gitaly/service/operations/revert_test.go b/internal/gitaly/service/operations/revert_test.go
index 71fa4411e..4f274e58e 100644
--- a/internal/gitaly/service/operations/revert_test.go
+++ b/internal/gitaly/service/operations/revert_test.go
@@ -367,10 +367,11 @@ func TestUserRevert(t *testing.T) {
 						Message:        []byte("Reverting " + secondCommitID),
 						ExpectedOldOid: firstCommitID.String(),
 					},
-					expectedError: testhelper.WithInterceptedMetadata(
+					expectedError: testhelper.WithInterceptedMetadataItems(
 						structerr.NewInternal("update reference with hooks: Could not update refs/heads/%s. Please refresh and try again.", branchName),
-						"stderr",
-						fmt.Sprintf("fatal: prepare: cannot lock ref 'refs/heads/%s': is at %s but expected %s\n", branchName, secondCommitID, firstCommitID),
+						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/tags_test.go b/internal/gitaly/service/operations/tags_test.go
index bccb397fe..77890ee71 100644
--- a/internal/gitaly/service/operations/tags_test.go
+++ b/internal/gitaly/service/operations/tags_test.go
@@ -267,10 +267,11 @@ func TestUserDeleteTag(t *testing.T) {
 						User:           gittest.TestUser,
 						ExpectedOldOid: firstCommit.String(),
 					},
-					expectedError: testhelper.WithInterceptedMetadata(
+					expectedError: testhelper.WithInterceptedMetadataItems(
 						structerr.NewFailedPrecondition("Could not update refs/tags/%s. Please refresh and try again.", tagName),
-						"stderr",
-						fmt.Sprintf("fatal: prepare: cannot lock ref 'refs/tags/%s': is at %s but expected %s\n", tagName, secondCommit, firstCommit),
+						structerr.MetadataItem{Key: "actual_object_id", Value: secondCommit},
+						structerr.MetadataItem{Key: "expected_object_id", Value: firstCommit},
+						structerr.MetadataItem{Key: "reference", Value: "refs/tags/" + tagName},
 					),
 					expectedTags: []string{"ganymede"},
 				}