diff --git a/pkg/github/repositories.go b/pkg/github/repositories.go index 5cb7769b0..69aaaa899 100644 --- a/pkg/github/repositories.go +++ b/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/"). If that +// returns a 404 Not Found error, it then attempts to resolve it as a tag +// ("refs/tags/"). +// +// 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 } diff --git a/pkg/github/repositories_test.go b/pkg/github/repositories_test.go index 2e522b426..36ef66d87 100644 --- a/pkg/github/repositories_test.go +++ b/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)