localrepo: Fix lookup of wrong ref if requesting prefix

When passing a prefix to git-for-each-ref(1), it'll simply print all
branches whose reference name starts with that prefix. We fail to
account for that in `localrepo.GetReference()`, which as a result may
lead us to simply returning the first branch we've found in case a
prefix is handed to us.

Fix this issue by comparing the returned branch name with what was
searched for. If it doesn't match, then we return an error saying the
reference is ambiguous. Ideally, we'd just tell git to ignore non-exact
matches, but seemingly that isn't possible.

4 files changed, 75 insertions, 19 deletions

diff --git a/changelogs/unreleased/pks-localrepo-ambiguous-refs.yml b/changelogs/unreleased/pks-localrepo-ambiguous-refs.yml
new file mode 100644
index 000000000..4ffb33068
--- /dev/null
+++ b/changelogs/unreleased/pks-localrepo-ambiguous-refs.yml
@@ -0,0 +1,5 @@
+---
+title: 'localrepo: Fix lookup of wrong ref if requesting prefix'
+merge_request: 3127
+author:
+type: fixed
diff --git a/internal/git/localrepo/refs.go b/internal/git/localrepo/refs.go
index c23103038..f4e5bd42f 100644
--- a/internal/git/localrepo/refs.go
+++ b/internal/git/localrepo/refs.go
@@ -72,6 +72,9 @@ func (repo *Repo) GetReference(ctx context.Context, reference git.ReferenceName)
 	if len(refs) == 0 {
 		return git.Reference{}, git.ErrReferenceNotFound
 	}
+	if refs[0].Name != reference {
+		return git.Reference{}, git.ErrReferenceAmbiguous
+	}
 
 	return refs[0], nil
 }
diff --git a/internal/git/localrepo/refs_test.go b/internal/git/localrepo/refs_test.go
index dc55b717a..ff9dae561 100644
--- a/internal/git/localrepo/refs_test.go
+++ b/internal/git/localrepo/refs_test.go
@@ -1,7 +1,6 @@
 package localrepo
 
 import (
-	"errors"
 	"testing"
 
 	"github.com/stretchr/testify/require"
@@ -65,9 +64,10 @@ func TestRepo_GetReference(t *testing.T) {
 	repo := New(testRepo, config.Config)
 
 	testcases := []struct {
-		desc     string
-		ref      string
-		expected git.Reference
+		desc        string
+		ref         string
+		expected    git.Reference
+		expectedErr error
 	}{
 		{
 			desc:     "fully qualified master branch",
@@ -75,35 +75,77 @@ func TestRepo_GetReference(t *testing.T) {
 			expected: git.NewReference("refs/heads/master", masterOID.String()),
 		},
 		{
-			desc:     "unqualified master branch fails",
-			ref:      "master",
-			expected: git.Reference{},
+			desc:        "unqualified master branch fails",
+			ref:         "master",
+			expectedErr: git.ErrReferenceNotFound,
 		},
 		{
-			desc:     "nonexistent branch",
-			ref:      "refs/heads/nonexistent",
-			expected: git.Reference{},
+			desc:        "nonexistent branch",
+			ref:         "refs/heads/nonexistent",
+			expectedErr: git.ErrReferenceNotFound,
 		},
 		{
-			desc:     "nonexistent branch",
-			ref:      "nonexistent",
-			expected: git.Reference{},
+			desc:        "prefix returns an error",
+			ref:         "refs/heads",
+			expectedErr: git.ErrReferenceAmbiguous,
+		},
+		{
+			desc:        "nonexistent branch",
+			ref:         "nonexistent",
+			expectedErr: git.ErrReferenceNotFound,
 		},
 	}
 
 	for _, tc := range testcases {
 		t.Run(tc.desc, func(t *testing.T) {
 			ref, err := repo.GetReference(ctx, git.ReferenceName(tc.ref))
-			if tc.expected.Name == "" {
-				require.True(t, errors.Is(err, git.ErrReferenceNotFound))
-			} else {
-				require.NoError(t, err)
-				require.Equal(t, tc.expected, ref)
-			}
+			require.Equal(t, tc.expectedErr, err)
+			require.Equal(t, tc.expected, ref)
 		})
 	}
 }
 
+func TestRepo_GetReferenceWithAmbiguousRefs(t *testing.T) {
+	ctx, cancel := testhelper.Context()
+	defer cancel()
+
+	// Disable hooks
+	defer func(oldValue string) {
+		config.Config.Ruby.Dir = oldValue
+	}(config.Config.Ruby.Dir)
+	config.Config.Ruby.Dir = "/var/empty"
+
+	repoProto, _, cleanup := testhelper.NewTestRepo(t)
+	defer cleanup()
+	repo := New(repoProto, config.Config)
+
+	currentOID, err := repo.ResolveRevision(ctx, "refs/heads/master")
+	require.NoError(t, err)
+
+	prevOID, err := repo.ResolveRevision(ctx, "refs/heads/master~")
+	require.NoError(t, err)
+
+	for _, ref := range []git.ReferenceName{
+		"refs/heads/something/master",
+		"refs/heads/MASTER",
+		"refs/heads/master2",
+		"refs/heads/masterx",
+		"refs/heads/refs/heads/master",
+		"refs/heads/heads/master",
+		"refs/master",
+		"refs/tags/master",
+	} {
+		require.NoError(t, repo.UpdateRef(ctx, ref, prevOID, git.ZeroOID))
+	}
+
+	ref, err := repo.GetReference(ctx, "refs/heads/master")
+	require.NoError(t, err)
+	require.Equal(t, git.Reference{
+		Name:   "refs/heads/master",
+		Target: currentOID.String(),
+	}, ref)
+}
+
 func TestRepo_GetReferences(t *testing.T) {
 	ctx, cancel := testhelper.Context()
 	defer cancel()
diff --git a/internal/git/repository.go b/internal/git/repository.go
index 41a09245b..29c5cfed1 100644
--- a/internal/git/repository.go
+++ b/internal/git/repository.go
@@ -6,7 +6,13 @@ import (
 )
 
 var (
+	// ErrReferenceNotFound represents an error when a reference was not
+	// found.
 	ErrReferenceNotFound = errors.New("reference not found")
+	// ErrReferenceAmbiguous represents an error when a reference couldn't
+	// unambiguously be resolved.
+	ErrReferenceAmbiguous = errors.New("reference is ambiguous")
+
 	// ErrAlreadyExists represents an error when the resource is already exists.
 	ErrAlreadyExists = errors.New("already exists")
 	// ErrNotFound represents an error when the resource can't be found.