Tommy/improve-ref-handling

pkg/github/repositories.go

@@ -1358,36 +1358,100 @@ func filterPaths(entries []*github.TreeEntry, path string, maxResults int) []str
 	return matchedPaths
 }
 
-// resolveGitReference resolves git references with the following logic:
-// 1. If SHA is provided, it takes precedence
-// 2. If neither is provided, use the default branch as ref
-// 3. Get commit SHA from the ref
-// Refs can look like `refs/tags/{tag}`, `refs/heads/{branch}` or `refs/pull/{pr_number}/head`
-// The function returns the resolved ref, commit SHA and any error.
+// resolveGitReference takes a user-provided ref and sha and resolves them into a
+// definitive commit SHA and its corresponding fully-qualified reference.
+//
+// The resolution logic follows a clear priority:
+//
+//  1. If a specific commit `sha` is provided, it takes precedence and is used directly,
+//     and all reference resolution is skipped.
+//
+//  2. If no `sha` is provided, the function resolves the `ref`
+//     string into a fully-qualified format (e.g., "refs/heads/main") by trying
+//     the following steps in order:
+//     a). **Empty Ref:** If `ref` is empty, the repository's default branch is used.
+//     b). **Fully-Qualified:** If `ref` already starts with "refs/", it's considered fully
+//     qualified and used as-is.
+//     c). **Partially-Qualified:** If `ref` starts with "heads/" or "tags/", it is
+//     prefixed with "refs/" to make it fully-qualified.
+//     d). **Short Name:** Otherwise, the `ref` is treated as a short name. The function
+//     first attempts to resolve it as a branch ("refs/heads/<ref>"). If that
+//     returns a 404 Not Found error, it then attempts to resolve it as a tag
+//     ("refs/tags/<ref>").
+//
+//  3. **Final Lookup:** Once a fully-qualified ref is determined, a final API call
+//     is made to fetch that reference's definitive commit SHA.
+//
+// Any unexpected (non-404) errors during the resolution process are returned
+// immediately. All API errors are logged with rich context to aid diagnostics.
 func resolveGitReference(ctx context.Context, githubClient *github.Client, owner, repo, ref, sha string) (*raw.ContentOpts, error) {
-	// 1. If SHA is provided, use it directly
+	// 1) If SHA explicitly provided, it's the highest priority.
 	if sha != "" {
 		return &raw.ContentOpts{Ref: "", SHA: sha}, nil
 	}
 
-	// 2. If neither provided, use the default branch as ref
-	if ref == "" {
+	originalRef := ref // Keep original ref for clearer error messages down the line.
+
+	// 2) If no SHA is provided, we try to resolve the ref into a fully-qualified format.
+	var reference *github.Reference
+	var resp *github.Response
+	var err error
+
+	switch {
+	case originalRef == "":
+		// 2a) If ref is empty, determine the default branch.
 		repoInfo, resp, err := githubClient.Repositories.Get(ctx, owner, repo)
 		if err != nil {
 			_, _ = ghErrors.NewGitHubAPIErrorToCtx(ctx, "failed to get repository info", resp, err)
 			return nil, fmt.Errorf("failed to get repository info: %w", err)
 		}
 		ref = fmt.Sprintf("refs/heads/%s", repoInfo.GetDefaultBranch())
+	case strings.HasPrefix(originalRef, "refs/"):
+		// 2b) Already fully qualified. The reference will be fetched at the end.
+	case strings.HasPrefix(originalRef, "heads/") || strings.HasPrefix(originalRef, "tags/"):
+		// 2c) Partially qualified. Make it fully qualified.
+		ref = "refs/" + originalRef
+	default:
+		// 2d) It's a short name, so we try to resolve it to either a branch or a tag.
+		branchRef := "refs/heads/" + originalRef
+		reference, resp, err = githubClient.Git.GetRef(ctx, owner, repo, branchRef)
+
+		if err == nil {
+			ref = branchRef // It's a branch.
+		} else {
+			// The branch lookup failed. Check if it was a 404 Not Found error.
+			ghErr, isGhErr := err.(*github.ErrorResponse)
+			if isGhErr && ghErr.Response.StatusCode == http.StatusNotFound {
+				tagRef := "refs/tags/" + originalRef
+				reference, resp, err = githubClient.Git.GetRef(ctx, owner, repo, tagRef)
+				if err == nil {
+					ref = tagRef // It's a tag.
+				} else {
+					// The tag lookup also failed. Check if it was a 404 Not Found error.
+					ghErr2, isGhErr2 := err.(*github.ErrorResponse)
+					if isGhErr2 && ghErr2.Response.StatusCode == http.StatusNotFound {
+						return nil, fmt.Errorf("could not resolve ref %q as a branch or a tag", originalRef)
+					}
+					// The tag lookup failed for a different reason.
+					_, _ = ghErrors.NewGitHubAPIErrorToCtx(ctx, "failed to get reference (tag)", resp, err)
+					return nil, fmt.Errorf("failed to get reference for tag '%s': %w", originalRef, err)
+				}
+			} else {
+				// The branch lookup failed for a different reason.
+				_, _ = ghErrors.NewGitHubAPIErrorToCtx(ctx, "failed to get reference (branch)", resp, err)
+				return nil, fmt.Errorf("failed to get reference for branch '%s': %w", originalRef, err)
+			}
+		}
 	}
 
-	// 3. Get the SHA from the ref
-	reference, resp, err := githubClient.Git.GetRef(ctx, owner, repo, ref)
-	if err != nil {
-		_, _ = ghErrors.NewGitHubAPIErrorToCtx(ctx, "failed to get reference", resp, err)
-		return nil, fmt.Errorf("failed to get reference: %w", err)
+	if reference == nil {
+		reference, resp, err = githubClient.Git.GetRef(ctx, owner, repo, ref)
+		if err != nil {
+			_, _ = ghErrors.NewGitHubAPIErrorToCtx(ctx, "failed to get final reference", resp, err)
+			return nil, fmt.Errorf("failed to get final reference for %q: %w", ref, err)
+		}
 	}
-	sha = reference.GetObject().GetSHA()
 
-	// Use provided ref, or it will be empty which defaults to the default branch
+	sha = reference.GetObject().GetSHA()
 	return &raw.ContentOpts{Ref: ref, SHA: sha}, nil
 }

pkg/github/repositories_test.go

@@ -6,6 +6,7 @@ import (
 	"encoding/json"
 	"net/http"
 	"net/url"
+	"strings"
 	"testing"
 	"time"
 
@@ -2212,63 +2213,239 @@ func Test_resolveGitReference(t *testing.T) {
 	ctx := context.Background()
 	owner := "owner"
 	repo := "repo"
-	mockedClient := mock.NewMockedHTTPClient(
-		mock.WithRequestMatchHandler(
-			mock.GetReposByOwnerByRepo,
-			http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
-				w.WriteHeader(http.StatusOK)
-				_, _ = w.Write([]byte(`{"name": "repo", "default_branch": "main"}`))
-			}),
-		),
-		mock.WithRequestMatchHandler(
-			mock.GetReposGitRefByOwnerByRepoByRef,
-			http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
-				w.WriteHeader(http.StatusOK)
-				_, _ = w.Write([]byte(`{"ref": "refs/heads/main", "object": {"sha": "123sha456"}}`))
-			}),
-		),
-	)
 
 	tests := []struct {
 		name           string
 		ref            string
 		sha            string
+		mockSetup      func() *http.Client
 		expectedOutput *raw.ContentOpts
+		expectError    bool
+		errorContains  string
 	}{
 		{
 			name: "sha takes precedence over ref",
 			ref:  "refs/heads/main",
 			sha:  "123sha456",
+			mockSetup: func() *http.Client {
+				// No API calls should be made when SHA is provided
+				return mock.NewMockedHTTPClient()
+			},
 			expectedOutput: &raw.ContentOpts{
 				SHA: "123sha456",
 			},
+			expectError: false,
 		},
 		{
 			name: "use default branch if ref and sha both empty",
 			ref:  "",
 			sha:  "",
+			mockSetup: func() *http.Client {
+				return mock.NewMockedHTTPClient(
+					mock.WithRequestMatchHandler(
+						mock.GetReposByOwnerByRepo,
+						http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+							w.WriteHeader(http.StatusOK)
+							_, _ = w.Write([]byte(`{"name": "repo", "default_branch": "main"}`))
+						}),
+					),
+					mock.WithRequestMatchHandler(
+						mock.GetReposGitRefByOwnerByRepoByRef,
+						http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+							assert.Contains(t, r.URL.Path, "/git/ref/heads/main")
+							w.WriteHeader(http.StatusOK)
+							_, _ = w.Write([]byte(`{"ref": "refs/heads/main", "object": {"sha": "main-sha"}}`))
+						}),
+					),
+				)
+			},
 			expectedOutput: &raw.ContentOpts{
 				Ref: "refs/heads/main",
-				SHA: "123sha456",
+				SHA: "main-sha",
 			},
+			expectError: false,
 		},
 		{
-			name: "get SHA from ref",
-			ref:  "refs/heads/main",
+			name: "fully qualified ref passed through unchanged",
+			ref:  "refs/heads/feature-branch",
 			sha:  "",
+			mockSetup: func() *http.Client {
+				return mock.NewMockedHTTPClient(
+					mock.WithRequestMatchHandler(
+						mock.GetReposGitRefByOwnerByRepoByRef,
+						http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+							assert.Contains(t, r.URL.Path, "/git/ref/heads/feature-branch")
+							w.WriteHeader(http.StatusOK)
+							_, _ = w.Write([]byte(`{"ref": "refs/heads/feature-branch", "object": {"sha": "feature-sha"}}`))
+						}),
+					),
+				)
+			},
+			expectedOutput: &raw.ContentOpts{
+				Ref: "refs/heads/feature-branch",
+				SHA: "feature-sha",
+			},
+			expectError: false,
+		},
+		{
+			name: "short branch name resolves to refs/heads/",
+			ref:  "main",
+			sha:  "",
+			mockSetup: func() *http.Client {
+				return mock.NewMockedHTTPClient(
+					mock.WithRequestMatchHandler(
+						mock.GetReposGitRefByOwnerByRepoByRef,
+						http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+							if strings.Contains(r.URL.Path, "/git/ref/heads/main") {
+								w.WriteHeader(http.StatusOK)
+								_, _ = w.Write([]byte(`{"ref": "refs/heads/main", "object": {"sha": "main-sha"}}`))
+							} else {
+								t.Errorf("Unexpected path: %s", r.URL.Path)
+								w.WriteHeader(http.StatusNotFound)
+							}
+						}),
+					),
+				)
+			},
 			expectedOutput: &raw.ContentOpts{
 				Ref: "refs/heads/main",
-				SHA: "123sha456",
+				SHA: "main-sha",
+			},
+			expectError: false,
+		},
+		{
+			name: "short tag name falls back to refs/tags/ when branch not found",
+			ref:  "v1.0.0",
+			sha:  "",
+			mockSetup: func() *http.Client {
+				return mock.NewMockedHTTPClient(
+					mock.WithRequestMatchHandler(
+						mock.GetReposGitRefByOwnerByRepoByRef,
+						http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+							switch {
+							case strings.Contains(r.URL.Path, "/git/ref/heads/v1.0.0"):
+								w.WriteHeader(http.StatusNotFound)
+								_, _ = w.Write([]byte(`{"message": "Not Found"}`))
+							case strings.Contains(r.URL.Path, "/git/ref/tags/v1.0.0"):
+								w.WriteHeader(http.StatusOK)
+								_, _ = w.Write([]byte(`{"ref": "refs/tags/v1.0.0", "object": {"sha": "tag-sha"}}`))
+							default:
+								t.Errorf("Unexpected path: %s", r.URL.Path)
+								w.WriteHeader(http.StatusNotFound)
+							}
+						}),
+					),
+				)
+			},
+			expectedOutput: &raw.ContentOpts{
+				Ref: "refs/tags/v1.0.0",
+				SHA: "tag-sha",
+			},
+			expectError: false,
+		},
+		{
+			name: "heads/ prefix gets refs/ prepended",
+			ref:  "heads/feature-branch",
+			sha:  "",
+			mockSetup: func() *http.Client {
+				return mock.NewMockedHTTPClient(
+					mock.WithRequestMatchHandler(
+						mock.GetReposGitRefByOwnerByRepoByRef,
+						http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+							assert.Contains(t, r.URL.Path, "/git/ref/heads/feature-branch")
+							w.WriteHeader(http.StatusOK)
+							_, _ = w.Write([]byte(`{"ref": "refs/heads/feature-branch", "object": {"sha": "feature-sha"}}`))
+						}),
+					),
+				)
+			},
+			expectedOutput: &raw.ContentOpts{
+				Ref: "refs/heads/feature-branch",
+				SHA: "feature-sha",
 			},
+			expectError: false,
+		},
+		{
+			name: "tags/ prefix gets refs/ prepended",
+			ref:  "tags/v1.0.0",
+			sha:  "",
+			mockSetup: func() *http.Client {
+				return mock.NewMockedHTTPClient(
+					mock.WithRequestMatchHandler(
+						mock.GetReposGitRefByOwnerByRepoByRef,
+						http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+							assert.Contains(t, r.URL.Path, "/git/ref/tags/v1.0.0")
+							w.WriteHeader(http.StatusOK)
+							_, _ = w.Write([]byte(`{"ref": "refs/tags/v1.0.0", "object": {"sha": "tag-sha"}}`))
+						}),
+					),
+				)
+			},
+			expectedOutput: &raw.ContentOpts{
+				Ref: "refs/tags/v1.0.0",
+				SHA: "tag-sha",
+			},
+			expectError: false,
+		},
+		{
+			name: "invalid short name that doesn't exist as branch or tag",
+			ref:  "nonexistent",
+			sha:  "",
+			mockSetup: func() *http.Client {
+				return mock.NewMockedHTTPClient(
+					mock.WithRequestMatchHandler(
+						mock.GetReposGitRefByOwnerByRepoByRef,
+						http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+							// Both branch and tag attempts should return 404
+							w.WriteHeader(http.StatusNotFound)
+							_, _ = w.Write([]byte(`{"message": "Not Found"}`))
+						}),
+					),
+				)
+			},
+			expectError:   true,
+			errorContains: "could not resolve ref \"nonexistent\" as a branch or a tag",
+		},
+		{
+			name: "fully qualified pull request ref",
+			ref:  "refs/pull/123/head",
+			sha:  "",
+			mockSetup: func() *http.Client {
+				return mock.NewMockedHTTPClient(
+					mock.WithRequestMatchHandler(
+						mock.GetReposGitRefByOwnerByRepoByRef,
+						http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+							assert.Contains(t, r.URL.Path, "/git/ref/pull/123/head")
+							w.WriteHeader(http.StatusOK)
+							_, _ = w.Write([]byte(`{"ref": "refs/pull/123/head", "object": {"sha": "pr-sha"}}`))
+						}),
+					),
+				)
+			},
+			expectedOutput: &raw.ContentOpts{
+				Ref: "refs/pull/123/head",
+				SHA: "pr-sha",
+			},
+			expectError: false,
 		},
 	}
 
 	for _, tc := range tests {
 		t.Run(tc.name, func(t *testing.T) {
 			// Setup client with mock
-			client := github.NewClient(mockedClient)
+			client := github.NewClient(tc.mockSetup())
 			opts, err := resolveGitReference(ctx, client, owner, repo, tc.ref, tc.sha)
+
+			if tc.expectError {
+				require.Error(t, err)
+				if tc.errorContains != "" {
+					assert.Contains(t, err.Error(), tc.errorContains)
+				}
+				return
+			}
+
 			require.NoError(t, err)
+			require.NotNil(t, opts)
 
 			if tc.expectedOutput.SHA != "" {
 				assert.Equal(t, tc.expectedOutput.SHA, opts.SHA)