From 25b5901e95286ac55b46886b4958aace31d2288a Mon Sep 17 00:00:00 2001 From: tommaso-moro Date: Mon, 11 Aug 2025 10:51:57 +0100 Subject: [PATCH 1/9] make resolveGitReference function more robust, add comments to follow the logic --- pkg/github/repositories.go | 88 ++++++++++++++++++++++++++++++++------ 1 file changed, 74 insertions(+), 14 deletions(-) diff --git a/pkg/github/repositories.go b/pkg/github/repositories.go index 5cb7769b0..d2abf3e16 100644 --- a/pkg/github/repositories.go +++ b/pkg/github/repositories.go @@ -1358,36 +1358,96 @@ 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 + // 2) If no SHA is provided, we try to resolve the ref into a fully-qualified format. + // 2a) If ref is empty, determine the default branch. if ref == "" { 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()) + ref = repoInfo.GetDefaultBranch() } - // 3. Get the SHA from the ref + originalRef := ref // Keep original ref for clearer error messages down the line. + + if strings.HasPrefix(ref, "refs/") { + // 2b) Already fully qualified. We will use it. + } else if strings.HasPrefix(ref, "heads/") || strings.HasPrefix(ref, "tags/") { + ref = "refs/" + ref // 2c) Partially qualified. Make it fully qualified. + } else { + // 2d) Short name. Try to resolve it as a branch or tag. + _, resp, err := githubClient.Git.GetRef(ctx, owner, repo, "refs/heads/"+ref) + + // try to resolve the ref as a branch first. + if err == nil { + ref = "refs/heads/" + ref // It's a branch. + } else { + // The branch lookup failed. Check if it was a 404 Not Found error. + // If it was, we will try to resolve it as a tag. + ghErr, isGhErr := err.(*github.ErrorResponse) + if isGhErr && ghErr.Response.StatusCode == http.StatusNotFound { + // The branch wasn't found, so try as a tag. + _, resp2, err2 := githubClient.Git.GetRef(ctx, owner, repo, "refs/tags/"+ref) + if err2 == nil { + ref = "refs/tags/" + ref // It's a tag. + } else { + // The tag lookup also failed. Check if it was a 404 Not Found error. + ghErr2, isGhErr2 := err2.(*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)", resp2, err2) + return nil, fmt.Errorf("failed to get reference for tag '%s': %w", originalRef, err2) + } + } 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) + } + } + } + + // Now that 'ref' is a valid, fully-qualified name, we get the definitive reference object. 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) + _, _ = 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 } From 785eb297c3b9fc724686dc5f7d125ead34f9dfd0 Mon Sep 17 00:00:00 2001 From: tommaso-moro Date: Mon, 11 Aug 2025 10:53:00 +0100 Subject: [PATCH 2/9] add tests for new functionality --- pkg/github/repositories_test.go | 316 +++++++++++++++++++++++++------- 1 file changed, 247 insertions(+), 69 deletions(-) diff --git a/pkg/github/repositories_test.go b/pkg/github/repositories_test.go index 2e522b426..cb3d7b7df 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" @@ -2209,73 +2210,250 @@ func Test_filterPaths(t *testing.T) { } 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 - expectedOutput *raw.ContentOpts - }{ - { - name: "sha takes precedence over ref", - ref: "refs/heads/main", - sha: "123sha456", - expectedOutput: &raw.ContentOpts{ - SHA: "123sha456", - }, - }, - { - name: "use default branch if ref and sha both empty", - ref: "", - sha: "", - expectedOutput: &raw.ContentOpts{ - Ref: "refs/heads/main", - SHA: "123sha456", - }, - }, - { - name: "get SHA from ref", - ref: "refs/heads/main", - sha: "", - expectedOutput: &raw.ContentOpts{ - Ref: "refs/heads/main", - SHA: "123sha456", - }, - }, - } - - for _, tc := range tests { - t.Run(tc.name, func(t *testing.T) { - // Setup client with mock - client := github.NewClient(mockedClient) - opts, err := resolveGitReference(ctx, client, owner, repo, tc.ref, tc.sha) - require.NoError(t, err) - - if tc.expectedOutput.SHA != "" { - assert.Equal(t, tc.expectedOutput.SHA, opts.SHA) - } - if tc.expectedOutput.Ref != "" { - assert.Equal(t, tc.expectedOutput.Ref, opts.Ref) - } - }) - } + ctx := context.Background() + owner := "owner" + repo := "repo" + + 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: "main-sha", + }, + expectError: false, + }, + { + 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 { + callCount := 0 + return mock.NewMockedHTTPClient( + mock.WithRequestMatchHandler( + mock.GetReposGitRefByOwnerByRepoByRef, + http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + callCount++ + 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: "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) { + if strings.Contains(r.URL.Path, "/git/ref/heads/v1.0.0") { + w.WriteHeader(http.StatusNotFound) + _, _ = w.Write([]byte(`{"message": "Not Found"}`)) + } else if 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"}}`)) + } else { + 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, r *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(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) + } + if tc.expectedOutput.Ref != "" { + assert.Equal(t, tc.expectedOutput.Ref, opts.Ref) + } + }) + } } From b9e5ea4cfa58ee1cd424ea6dcfa6311ff7fe04f6 Mon Sep 17 00:00:00 2001 From: tommaso-moro Date: Mon, 11 Aug 2025 11:29:24 +0100 Subject: [PATCH 3/9] lint fix 1 --- pkg/github/repositories.go | 26 +- pkg/github/repositories_test.go | 492 ++++++++++++++++---------------- 2 files changed, 259 insertions(+), 259 deletions(-) diff --git a/pkg/github/repositories.go b/pkg/github/repositories.go index d2abf3e16..7a00b35be 100644 --- a/pkg/github/repositories.go +++ b/pkg/github/repositories.go @@ -1363,23 +1363,23 @@ func filterPaths(entries []*github.TreeEntry, path string, maxResults int) []str // // The resolution logic follows a clear priority: // -// 1. If a specific commit `sha` is provided, it takes precedence and is used directly, +// 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` +// 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/"). +// 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 +// 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 @@ -1414,7 +1414,7 @@ func resolveGitReference(ctx context.Context, githubClient *github.Client, owner // try to resolve the ref as a branch first. if err == nil { ref = "refs/heads/" + ref // It's a branch. - } else { + } else { // The branch lookup failed. Check if it was a 404 Not Found error. // If it was, we will try to resolve it as a tag. ghErr, isGhErr := err.(*github.ErrorResponse) diff --git a/pkg/github/repositories_test.go b/pkg/github/repositories_test.go index cb3d7b7df..dc6627340 100644 --- a/pkg/github/repositories_test.go +++ b/pkg/github/repositories_test.go @@ -2210,250 +2210,250 @@ func Test_filterPaths(t *testing.T) { } func Test_resolveGitReference(t *testing.T) { - ctx := context.Background() - owner := "owner" - repo := "repo" - - 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: "main-sha", - }, - expectError: false, - }, - { - 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 { - callCount := 0 - return mock.NewMockedHTTPClient( - mock.WithRequestMatchHandler( - mock.GetReposGitRefByOwnerByRepoByRef, - http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - callCount++ - 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: "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) { - if strings.Contains(r.URL.Path, "/git/ref/heads/v1.0.0") { - w.WriteHeader(http.StatusNotFound) - _, _ = w.Write([]byte(`{"message": "Not Found"}`)) - } else if 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"}}`)) - } else { - 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, r *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(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) - } - if tc.expectedOutput.Ref != "" { - assert.Equal(t, tc.expectedOutput.Ref, opts.Ref) - } - }) - } + ctx := context.Background() + owner := "owner" + repo := "repo" + + 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: "main-sha", + }, + expectError: false, + }, + { + 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 { + callCount := 0 + return mock.NewMockedHTTPClient( + mock.WithRequestMatchHandler( + mock.GetReposGitRefByOwnerByRepoByRef, + http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + callCount++ + 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: "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) { + if strings.Contains(r.URL.Path, "/git/ref/heads/v1.0.0") { + w.WriteHeader(http.StatusNotFound) + _, _ = w.Write([]byte(`{"message": "Not Found"}`)) + } else if 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"}}`)) + } else { + 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, r *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(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) + } + if tc.expectedOutput.Ref != "" { + assert.Equal(t, tc.expectedOutput.Ref, opts.Ref) + } + }) + } } From 564bb7df7a1d9b7bdf6f624cab2af0c76fc6271b Mon Sep 17 00:00:00 2001 From: tommaso-moro Date: Mon, 11 Aug 2025 11:38:10 +0100 Subject: [PATCH 4/9] fix linting by using inverted if instead of empty if block --- pkg/github/repositories.go | 64 ++++++++++++++++----------------- pkg/github/repositories_test.go | 9 ++--- 2 files changed, 37 insertions(+), 36 deletions(-) diff --git a/pkg/github/repositories.go b/pkg/github/repositories.go index 7a00b35be..64266933c 100644 --- a/pkg/github/repositories.go +++ b/pkg/github/repositories.go @@ -1403,45 +1403,45 @@ func resolveGitReference(ctx context.Context, githubClient *github.Client, owner originalRef := ref // Keep original ref for clearer error messages down the line. - if strings.HasPrefix(ref, "refs/") { - // 2b) Already fully qualified. We will use it. - } else if strings.HasPrefix(ref, "heads/") || strings.HasPrefix(ref, "tags/") { - ref = "refs/" + ref // 2c) Partially qualified. Make it fully qualified. - } else { - // 2d) Short name. Try to resolve it as a branch or tag. - _, resp, err := githubClient.Git.GetRef(ctx, owner, repo, "refs/heads/"+ref) - - // try to resolve the ref as a branch first. - if err == nil { - ref = "refs/heads/" + ref // It's a branch. + // Only enter the resolution logic if the ref is NOT already fully qualified. + if !strings.HasPrefix(ref, "refs/") { + if strings.HasPrefix(ref, "heads/") || strings.HasPrefix(ref, "tags/") { + // 2c) It's partially qualified. Make it fully qualified. + ref = "refs/" + ref } else { - // The branch lookup failed. Check if it was a 404 Not Found error. - // If it was, we will try to resolve it as a tag. - ghErr, isGhErr := err.(*github.ErrorResponse) - if isGhErr && ghErr.Response.StatusCode == http.StatusNotFound { - // The branch wasn't found, so try as a tag. - _, resp2, err2 := githubClient.Git.GetRef(ctx, owner, repo, "refs/tags/"+ref) - if err2 == nil { - ref = "refs/tags/" + ref // It's a tag. - } else { - // The tag lookup also failed. Check if it was a 404 Not Found error. - ghErr2, isGhErr2 := err2.(*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) + // 2d) It's a short name; try to resolve it as a branch or tag. + _, resp, err := githubClient.Git.GetRef(ctx, owner, repo, "refs/heads/"+ref) + + if err == nil { + ref = "refs/heads/" + ref // 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 { + // The branch wasn't found, so try as a tag. + _, resp2, err2 := githubClient.Git.GetRef(ctx, owner, repo, "refs/tags/"+ref) + if err2 == nil { + ref = "refs/tags/" + ref // It's a tag. + } else { + // The tag lookup failed. Check if it was a 404 Not Found error. + ghErr2, isGhErr2 := err2.(*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)", resp2, err2) + return nil, fmt.Errorf("failed to get reference for tag '%s': %w", originalRef, err2) } - // The tag lookup failed for a different reason. - _, _ = ghErrors.NewGitHubAPIErrorToCtx(ctx, "failed to get reference (tag)", resp2, err2) - return nil, fmt.Errorf("failed to get reference for tag '%s': %w", originalRef, err2) + } 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) } - } 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) } } } - // Now that 'ref' is a valid, fully-qualified name, we get the definitive reference object. + // Now that 'ref' is fully qualified, we get the definitive reference object. reference, resp, err := githubClient.Git.GetRef(ctx, owner, repo, ref) if err != nil { _, _ = ghErrors.NewGitHubAPIErrorToCtx(ctx, "failed to get final reference", resp, err) diff --git a/pkg/github/repositories_test.go b/pkg/github/repositories_test.go index dc6627340..fd756bab9 100644 --- a/pkg/github/repositories_test.go +++ b/pkg/github/repositories_test.go @@ -2324,13 +2324,14 @@ func Test_resolveGitReference(t *testing.T) { mock.WithRequestMatchHandler( mock.GetReposGitRefByOwnerByRepoByRef, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if strings.Contains(r.URL.Path, "/git/ref/heads/v1.0.0") { + switch { + case strings.Contains(r.URL.Path, "/git/ref/heads/v1.0.0"): w.WriteHeader(http.StatusNotFound) _, _ = w.Write([]byte(`{"message": "Not Found"}`)) - } else if strings.Contains(r.URL.Path, "/git/ref/tags/v1.0.0") { + 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"}}`)) - } else { + default: t.Errorf("Unexpected path: %s", r.URL.Path) w.WriteHeader(http.StatusNotFound) } @@ -2396,7 +2397,7 @@ func Test_resolveGitReference(t *testing.T) { return mock.NewMockedHTTPClient( mock.WithRequestMatchHandler( mock.GetReposGitRefByOwnerByRepoByRef, - http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + 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"}`)) From e7f84bfc43d53559feae69a00f8a6d5f48fe4f69 Mon Sep 17 00:00:00 2001 From: tommaso-moro Date: Mon, 11 Aug 2025 12:08:21 +0100 Subject: [PATCH 5/9] remove unused var --- pkg/github/repositories_test.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/pkg/github/repositories_test.go b/pkg/github/repositories_test.go index fd756bab9..36ef66d87 100644 --- a/pkg/github/repositories_test.go +++ b/pkg/github/repositories_test.go @@ -2292,12 +2292,10 @@ func Test_resolveGitReference(t *testing.T) { ref: "main", sha: "", mockSetup: func() *http.Client { - callCount := 0 return mock.NewMockedHTTPClient( mock.WithRequestMatchHandler( mock.GetReposGitRefByOwnerByRepoByRef, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - callCount++ 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"}}`)) From b6976b884330f02da51ff5d8327a84d389a9ac8d Mon Sep 17 00:00:00 2001 From: tommaso-moro Date: Mon, 11 Aug 2025 15:44:51 +0100 Subject: [PATCH 6/9] refactor --- pkg/github/repositories.go | 79 +++++++++++++++++++++----------------- 1 file changed, 43 insertions(+), 36 deletions(-) diff --git a/pkg/github/repositories.go b/pkg/github/repositories.go index 64266933c..f2db33fda 100644 --- a/pkg/github/repositories.go +++ b/pkg/github/repositories.go @@ -1390,6 +1390,8 @@ func resolveGitReference(ctx context.Context, githubClient *github.Client, owner return &raw.ContentOpts{Ref: "", SHA: sha}, nil } + 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. // 2a) If ref is empty, determine the default branch. if ref == "" { @@ -1401,51 +1403,56 @@ func resolveGitReference(ctx context.Context, githubClient *github.Client, owner ref = repoInfo.GetDefaultBranch() } - originalRef := ref // Keep original ref for clearer error messages down the line. + var reference *github.Reference + var resp *github.Response + var err error // Only enter the resolution logic if the ref is NOT already fully qualified. - if !strings.HasPrefix(ref, "refs/") { - if strings.HasPrefix(ref, "heads/") || strings.HasPrefix(ref, "tags/") { - // 2c) It's partially qualified. Make it fully qualified. - ref = "refs/" + ref + switch { + case strings.HasPrefix(ref, "refs/"): + // 2b) Already fully qualified. The reference will be fetched at the end. + case strings.HasPrefix(ref, "heads/") || strings.HasPrefix(ref, "tags/"): + // 2c) Partially qualified. Make it fully qualified. + ref = "refs/" + ref + default: + // 2d) It's a short name, so we try to resolve it to either a branch or a tag. + branchRef := "refs/heads/" + ref + reference, resp, err = githubClient.Git.GetRef(ctx, owner, repo, branchRef) + + if err == nil { + ref = branchRef // It's a branch. } else { - // 2d) It's a short name; try to resolve it as a branch or tag. - _, resp, err := githubClient.Git.GetRef(ctx, owner, repo, "refs/heads/"+ref) - - if err == nil { - ref = "refs/heads/" + ref // 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 { - // The branch wasn't found, so try as a tag. - _, resp2, err2 := githubClient.Git.GetRef(ctx, owner, repo, "refs/tags/"+ref) - if err2 == nil { - ref = "refs/tags/" + ref // It's a tag. - } else { - // The tag lookup failed. Check if it was a 404 Not Found error. - ghErr2, isGhErr2 := err2.(*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)", resp2, err2) - return nil, fmt.Errorf("failed to get reference for tag '%s': %w", originalRef, err2) - } + // 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/" + ref + reference, resp, err = githubClient.Git.GetRef(ctx, owner, repo, tagRef) + if err == nil { + ref = tagRef // It's a tag. } 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) + // 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) } } } - // Now that 'ref' is fully qualified, we get the definitive reference object. - 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) + 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() From d67db7fcf6b4ad50b1d56912c7abed12a38e871c Mon Sep 17 00:00:00 2001 From: tommaso-moro Date: Mon, 11 Aug 2025 15:52:14 +0100 Subject: [PATCH 7/9] remove comment --- pkg/github/repositories.go | 1 - 1 file changed, 1 deletion(-) diff --git a/pkg/github/repositories.go b/pkg/github/repositories.go index f2db33fda..14fe1791e 100644 --- a/pkg/github/repositories.go +++ b/pkg/github/repositories.go @@ -1407,7 +1407,6 @@ func resolveGitReference(ctx context.Context, githubClient *github.Client, owner var resp *github.Response var err error - // Only enter the resolution logic if the ref is NOT already fully qualified. switch { case strings.HasPrefix(ref, "refs/"): // 2b) Already fully qualified. The reference will be fetched at the end. From 6ddfbed8e9c534e037a475c82b3c8c3fc4c03426 Mon Sep 17 00:00:00 2001 From: tommaso-moro Date: Mon, 11 Aug 2025 16:30:58 +0100 Subject: [PATCH 8/9] small fix --- pkg/github/repositories.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pkg/github/repositories.go b/pkg/github/repositories.go index 14fe1791e..892ddd97b 100644 --- a/pkg/github/repositories.go +++ b/pkg/github/repositories.go @@ -1400,7 +1400,8 @@ func resolveGitReference(ctx context.Context, githubClient *github.Client, owner _, _ = ghErrors.NewGitHubAPIErrorToCtx(ctx, "failed to get repository info", resp, err) return nil, fmt.Errorf("failed to get repository info: %w", err) } - ref = repoInfo.GetDefaultBranch() + ref = fmt.Sprintf("refs/heads/%s", repoInfo.GetDefaultBranch()) + } var reference *github.Reference From ec9bd3124ac660d2c536633bac7341fdece95448 Mon Sep 17 00:00:00 2001 From: tommaso-moro Date: Mon, 11 Aug 2025 17:00:40 +0100 Subject: [PATCH 9/9] add ref == "" case in switch statement, use originalRef instead of ref in case definitions and some of the logic --- pkg/github/repositories.go | 27 ++++++++++++--------------- 1 file changed, 12 insertions(+), 15 deletions(-) diff --git a/pkg/github/repositories.go b/pkg/github/repositories.go index 892ddd97b..69aaaa899 100644 --- a/pkg/github/repositories.go +++ b/pkg/github/repositories.go @@ -1393,30 +1393,27 @@ func resolveGitReference(ctx context.Context, githubClient *github.Client, owner 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. - // 2a) If ref is empty, determine the default branch. - if ref == "" { + 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()) - - } - - var reference *github.Reference - var resp *github.Response - var err error - - switch { - case strings.HasPrefix(ref, "refs/"): + case strings.HasPrefix(originalRef, "refs/"): // 2b) Already fully qualified. The reference will be fetched at the end. - case strings.HasPrefix(ref, "heads/") || strings.HasPrefix(ref, "tags/"): + case strings.HasPrefix(originalRef, "heads/") || strings.HasPrefix(originalRef, "tags/"): // 2c) Partially qualified. Make it fully qualified. - ref = "refs/" + ref + 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/" + ref + branchRef := "refs/heads/" + originalRef reference, resp, err = githubClient.Git.GetRef(ctx, owner, repo, branchRef) if err == nil { @@ -1425,7 +1422,7 @@ func resolveGitReference(ctx context.Context, githubClient *github.Client, owner // 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/" + ref + tagRef := "refs/tags/" + originalRef reference, resp, err = githubClient.Git.GetRef(ctx, owner, repo, tagRef) if err == nil { ref = tagRef // It's a tag.