Merge branch 'smh-parse-invalid-ref-name' into 'master'

Parse invalid reference name errors in Updater

See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5034

Merged-by: Toon Claes <toon@gitlab.com>
Approved-by: Justin Tobler <jtobler@gitlab.com>
Co-authored-by: Sami Hiltunen <shiltunen@gitlab.com>

3 files changed, 42 insertions, 2 deletions

diff --git a/internal/git/updateref/updateref.go b/internal/git/updateref/updateref.go
index 234a9c1fb..29611c1df 100644
--- a/internal/git/updateref/updateref.go
+++ b/internal/git/updateref/updateref.go
@@ -21,6 +21,16 @@ func (e *ErrAlreadyLocked) Error() string {
 	return fmt.Sprintf("reference is already locked: %q", e.Ref)
 }
 
+// ErrInvalidReferenceFormat indicates a reference name was invalid.
+type ErrInvalidReferenceFormat struct {
+	// ReferenceName is the invalid reference name.
+	ReferenceName string
+}
+
+func (e ErrInvalidReferenceFormat) Error() string {
+	return fmt.Sprintf("invalid reference format: %q", e.ReferenceName)
+}
+
 // Updater wraps a `git update-ref --stdin` process, presenting an interface
 // that allows references to be easily updated in bulk. It is not suitable for
 // concurrent use.
@@ -122,7 +132,10 @@ func (u *Updater) Delete(reference git.ReferenceName) error {
 	return u.Update(reference, u.objectHash.ZeroOID, "")
 }
 
-var refLockedRegex = regexp.MustCompile("cannot lock ref '(.+?)'")
+var (
+	refLockedRegex        = regexp.MustCompile("cannot lock ref '(.+?)'")
+	refInvalidFormatRegex = regexp.MustCompile(`invalid ref format: (.*)\\n"`)
+)
 
 // Prepare prepares the reference transaction by locking all references and determining their
 // current values. The updates are not yet committed and will be rolled back in case there is no
@@ -134,6 +147,11 @@ func (u *Updater) Prepare() error {
 			return &ErrAlreadyLocked{Ref: string(matches[1])}
 		}
 
+		matches = refInvalidFormatRegex.FindSubmatch([]byte(err.Error()))
+		if len(matches) > 1 {
+			return ErrInvalidReferenceFormat{ReferenceName: string(matches[1])}
+		}
+
 		return err
 	}
 
diff --git a/internal/git/updateref/updateref_test.go b/internal/git/updateref/updateref_test.go
index aec92090d..b724d70b7 100644
--- a/internal/git/updateref/updateref_test.go
+++ b/internal/git/updateref/updateref_test.go
@@ -124,6 +124,28 @@ func TestUpdater_prepareLocksTransaction(t *testing.T) {
 	require.Contains(t, err.Error(), "fatal: prepared transactions can only be closed")
 }
 
+func TestUpdater_invalidReferenceName(t *testing.T) {
+	t.Parallel()
+
+	ctx := testhelper.Context(t)
+
+	cfg := testcfg.Build(t)
+
+	repoProto, repoPath := gittest.CreateRepository(t, ctx, cfg, gittest.CreateRepositoryConfig{
+		SkipCreationViaService: true,
+	})
+	repo := localrepo.NewTestRepo(t, cfg, repoProto, git.WithSkipHooks())
+	commitID := gittest.WriteCommit(t, cfg, repoPath)
+
+	updater, err := New(ctx, repo)
+	require.NoError(t, err)
+	defer func() { require.ErrorContains(t, updater.Cancel(), "canceling update: exit status 128") }()
+
+	const referenceName = "./refs/heads/master"
+	require.NoError(t, updater.Update(referenceName, commitID, ""))
+	require.Equal(t, ErrInvalidReferenceFormat{ReferenceName: referenceName}, updater.Prepare())
+}
+
 func TestUpdater_concurrentLocking(t *testing.T) {
 	t.Parallel()
 
diff --git a/internal/gitaly/service/ref/delete_refs_test.go b/internal/gitaly/service/ref/delete_refs_test.go
index 8c9614884..39a4d0c4f 100644
--- a/internal/gitaly/service/ref/delete_refs_test.go
+++ b/internal/gitaly/service/ref/delete_refs_test.go
@@ -208,7 +208,7 @@ func testDeleteRefsInvalidRefFormat(t *testing.T, ctx context.Context) {
 		testhelper.RequireGrpcError(t, detailedErr, err)
 	} else {
 		require.NoError(t, err)
-		assert.Contains(t, response.GitError, "invalid ref format")
+		require.Equal(t, response.GitError, `unable to delete refs: invalid reference format: "refs invalid-ref-format"`)
 	}
 }