updateref: Recognize locking errors when committing transactions

When a reference is locked already, then git-update-ref(1) will fail and
tell us. This information is getting parsed by Gitaly and gets returned
to the caller via a special `AlreadyLockedError` so that they can handle
this case more gracefully.

The regular expression we use to parse the error message is lacking
though: while the error may happen in the `prepare` phase, it can also
happen in the `commit` phase when the caller skips explicit preparation
of the transaction. And as Git will prefix the phase to the error, we
need to recognize both of them. Right now, we only recognize `prepare`
errors though.

Fix the issue by extending the regular expression to also detect
`commit` phase errors.

2 files changed, 47 insertions, 32 deletions

diff --git a/internal/git/updateref/updateref.go b/internal/git/updateref/updateref.go
index 16fe02e18..0627451d4 100644
--- a/internal/git/updateref/updateref.go
+++ b/internal/git/updateref/updateref.go
@@ -327,7 +327,7 @@ func (u *Updater) write(format string, args ...interface{}) error {
 }
 
 var (
-	refLockedRegex               = regexp.MustCompile(`^fatal: prepare: cannot lock ref '(.+?)': Unable to create '.*': File exists.`)
+	refLockedRegex               = regexp.MustCompile(`^fatal: (prepare|commit): cannot lock ref '(.+?)': Unable to create '.*': File exists.`)
 	refInvalidFormatRegex        = regexp.MustCompile(`^fatal: invalid ref format: (.*)\n$`)
 	referenceExistsConflictRegex = regexp.MustCompile(`^fatal: .*: cannot lock ref '(.*)': '(.*)' exists; cannot create '.*'\n$`)
 	inTransactionConflictRegex   = regexp.MustCompile(`^fatal: .*: cannot lock ref '.*': cannot process '(.*)' and '(.*)' at the same time\n$`)
@@ -381,8 +381,8 @@ func (u *Updater) handleIOError(fallbackErr error) error {
 	stderr := u.stderr.Bytes()
 
 	matches := refLockedRegex.FindSubmatch(stderr)
-	if len(matches) > 1 {
-		return AlreadyLockedError{ReferenceName: string(matches[1])}
+	if len(matches) > 2 {
+		return AlreadyLockedError{ReferenceName: string(matches[2])}
 	}
 
 	matches = refInvalidFormatRegex.FindSubmatch(stderr)
diff --git a/internal/git/updateref/updateref_test.go b/internal/git/updateref/updateref_test.go
index fd005a6c9..77ef4bc8f 100644
--- a/internal/git/updateref/updateref_test.go
+++ b/internal/git/updateref/updateref_test.go
@@ -3,7 +3,6 @@ package updateref
 import (
 	"context"
 	"encoding/hex"
-	"errors"
 	"fmt"
 	"io"
 	"os"
@@ -408,31 +407,52 @@ func TestUpdater_concurrentLocking(t *testing.T) {
 		SkipCreationViaService: true,
 	})
 	repo := localrepo.NewTestRepo(t, cfg, repoProto, git.WithSkipHooks())
-
 	commitID := gittest.WriteCommit(t, cfg, repoPath)
 
-	// Create the first updater that prepares the reference transaction so that the reference
-	// we're about to update is locked.
-	firstUpdater, err := New(ctx, repo)
-	require.NoError(t, err)
-	require.NoError(t, firstUpdater.Start())
-	require.NoError(t, firstUpdater.Update("refs/heads/master", commitID, ""))
-	require.NoError(t, firstUpdater.Prepare())
+	for _, tc := range []struct {
+		desc string
+		lock func(*Updater) error
+	}{
+		{
+			desc: "prepare",
+			lock: func(u *Updater) error {
+				return u.Prepare()
+			},
+		},
+		{
+			desc: "commit",
+			lock: func(u *Updater) error {
+				return u.Commit()
+			},
+		},
+	} {
+		t.Run(tc.desc, func(t *testing.T) {
+			ref := git.ReferenceName("refs/heads/" + tc.desc)
 
-	// Now we create a second updater that tries to update the same reference.
-	secondUpdater, err := New(ctx, repo)
-	require.NoError(t, err)
-	require.NoError(t, secondUpdater.Start())
-	require.NoError(t, secondUpdater.Update("refs/heads/master", commitID, ""))
+			// Create the first updater that prepares the reference transaction so that the reference
+			// we're about to update is locked.
+			firstUpdater, err := New(ctx, repo)
+			require.NoError(t, err)
+			require.NoError(t, firstUpdater.Start())
+			require.NoError(t, firstUpdater.Update(ref, commitID, ""))
+			require.NoError(t, firstUpdater.Prepare())
 
-	// Preparing this second updater should fail though because we notice that the reference is
-	// locked.
-	require.Equal(t, AlreadyLockedError{
-		ReferenceName: "refs/heads/master",
-	}, secondUpdater.Prepare())
+			// Now we create a second updater that tries to update the same reference.
+			secondUpdater, err := New(ctx, repo)
+			require.NoError(t, err)
+			require.NoError(t, secondUpdater.Start())
+			require.NoError(t, secondUpdater.Update(ref, commitID, ""))
+
+			// Try locking the reference via the second updater. This should fail now because the reference
+			// is locked already.
+			require.Equal(t, AlreadyLockedError{
+				ReferenceName: string(ref),
+			}, tc.lock(secondUpdater))
 
-	// Whereas committing the first transaction should succeed.
-	require.NoError(t, firstUpdater.Commit())
+			// Whereas committing the first transaction should succeed.
+			require.NoError(t, firstUpdater.Commit())
+		})
+	}
 }
 
 func TestUpdater_bulkOperation(t *testing.T) {
@@ -518,14 +538,9 @@ func TestUpdater_cancel(t *testing.T) {
 
 	require.NoError(t, failingUpdater.Start())
 	require.NoError(t, failingUpdater.Delete(git.ReferenceName("refs/heads/main")))
-
-	err = failingUpdater.Commit()
-	require.EqualError(t, err, fmt.Sprintf("state update to %q failed: %v", "commit", io.EOF))
-	var structErr structerr.Error
-	require.True(t, errors.As(err, &structErr))
-	// The error message returned by git-update-ref(1) is simply too long to fully verify it, so
-	// we just check that it matches a specific substring.
-	require.Contains(t, structErr.Metadata()["stderr"], "fatal: commit: cannot lock ref 'refs/heads/main'")
+	require.Equal(t, AlreadyLockedError{
+		ReferenceName: "refs/heads/main",
+	}, failingUpdater.Commit())
 
 	// We now cancel the initial updater. Afterwards, it should be possible again to update the
 	// ref because locks should have been released.