Skip to content

Commit 5b57b2e

Browse files
gokhanarkangokhanarkan
authored
Merge branch 'main' into fix/search-issues-duplicate-filters
2 parents 3c97c6b + 0b80f68 commit 5b57b2e

File tree

2 files changed

+278
-37
lines changed

2 files changed

+278
-37
lines changed

‎pkg/github/repositories.go

Lines changed: 80 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1358,36 +1358,100 @@ func filterPaths(entries []*github.TreeEntry, path string, maxResults int) []str
13581358
return matchedPaths
13591359
}
13601360

1361-
// resolveGitReference resolves git references with the following logic:
1362-
// 1. If SHA is provided, it takes precedence
1363-
// 2. If neither is provided, use the default branch as ref
1364-
// 3. Get commit SHA from the ref
1365-
// Refs can look like `refs/tags/{tag}`, `refs/heads/{branch}` or `refs/pull/{pr_number}/head`
1366-
// The function returns the resolved ref, commit SHA and any error.
1361+
// resolveGitReference takes a user-provided ref and sha and resolves them into a
1362+
// definitive commit SHA and its corresponding fully-qualified reference.
1363+
//
1364+
// The resolution logic follows a clear priority:
1365+
//
1366+
// 1. If a specific commit `sha` is provided, it takes precedence and is used directly,
1367+
// and all reference resolution is skipped.
1368+
//
1369+
// 2. If no `sha` is provided, the function resolves the `ref`
1370+
// string into a fully-qualified format (e.g., "refs/heads/main") by trying
1371+
// the following steps in order:
1372+
// a). **Empty Ref:** If `ref` is empty, the repository's default branch is used.
1373+
// b). **Fully-Qualified:** If `ref` already starts with "refs/", it's considered fully
1374+
// qualified and used as-is.
1375+
// c). **Partially-Qualified:** If `ref` starts with "heads/" or "tags/", it is
1376+
// prefixed with "refs/" to make it fully-qualified.
1377+
// d). **Short Name:** Otherwise, the `ref` is treated as a short name. The function
1378+
// first attempts to resolve it as a branch ("refs/heads/<ref>"). If that
1379+
// returns a 404 Not Found error, it then attempts to resolve it as a tag
1380+
// ("refs/tags/<ref>").
1381+
//
1382+
// 3. **Final Lookup:** Once a fully-qualified ref is determined, a final API call
1383+
// is made to fetch that reference's definitive commit SHA.
1384+
//
1385+
// Any unexpected (non-404) errors during the resolution process are returned
1386+
// immediately. All API errors are logged with rich context to aid diagnostics.
13671387
func resolveGitReference(ctx context.Context, githubClient *github.Client, owner, repo, ref, sha string) (*raw.ContentOpts, error) {
1368-
// 1. If SHA is provided, use it directly
1388+
// 1) If SHA explicitly provided, it's the highest priority.
13691389
if sha != "" {
13701390
return &raw.ContentOpts{Ref: "", SHA: sha}, nil
13711391
}
13721392

1373-
// 2. If neither provided, use the default branch as ref
1374-
if ref == "" {
1393+
originalRef := ref // Keep original ref for clearer error messages down the line.
1394+
1395+
// 2) If no SHA is provided, we try to resolve the ref into a fully-qualified format.
1396+
var reference *github.Reference
1397+
var resp *github.Response
1398+
var err error
1399+
1400+
switch {
1401+
case originalRef == "":
1402+
// 2a) If ref is empty, determine the default branch.
13751403
repoInfo, resp, err := githubClient.Repositories.Get(ctx, owner, repo)
13761404
if err != nil {
13771405
_, _ = ghErrors.NewGitHubAPIErrorToCtx(ctx, "failed to get repository info", resp, err)
13781406
return nil, fmt.Errorf("failed to get repository info: %w", err)
13791407
}
13801408
ref = fmt.Sprintf("refs/heads/%s", repoInfo.GetDefaultBranch())
1409+
case strings.HasPrefix(originalRef, "refs/"):
1410+
// 2b) Already fully qualified. The reference will be fetched at the end.
1411+
case strings.HasPrefix(originalRef, "heads/") || strings.HasPrefix(originalRef, "tags/"):
1412+
// 2c) Partially qualified. Make it fully qualified.
1413+
ref = "refs/" + originalRef
1414+
default:
1415+
// 2d) It's a short name, so we try to resolve it to either a branch or a tag.
1416+
branchRef := "refs/heads/" + originalRef
1417+
reference, resp, err = githubClient.Git.GetRef(ctx, owner, repo, branchRef)
1418+
1419+
if err == nil {
1420+
ref = branchRef // It's a branch.
1421+
} else {
1422+
// The branch lookup failed. Check if it was a 404 Not Found error.
1423+
ghErr, isGhErr := err.(*github.ErrorResponse)
1424+
if isGhErr && ghErr.Response.StatusCode == http.StatusNotFound {
1425+
tagRef := "refs/tags/" + originalRef
1426+
reference, resp, err = githubClient.Git.GetRef(ctx, owner, repo, tagRef)
1427+
if err == nil {
1428+
ref = tagRef // It's a tag.
1429+
} else {
1430+
// The tag lookup also failed. Check if it was a 404 Not Found error.
1431+
ghErr2, isGhErr2 := err.(*github.ErrorResponse)
1432+
if isGhErr2 && ghErr2.Response.StatusCode == http.StatusNotFound {
1433+
return nil, fmt.Errorf("could not resolve ref %q as a branch or a tag", originalRef)
1434+
}
1435+
// The tag lookup failed for a different reason.
1436+
_, _ = ghErrors.NewGitHubAPIErrorToCtx(ctx, "failed to get reference (tag)", resp, err)
1437+
return nil, fmt.Errorf("failed to get reference for tag '%s': %w", originalRef, err)
1438+
}
1439+
} else {
1440+
// The branch lookup failed for a different reason.
1441+
_, _ = ghErrors.NewGitHubAPIErrorToCtx(ctx, "failed to get reference (branch)", resp, err)
1442+
return nil, fmt.Errorf("failed to get reference for branch '%s': %w", originalRef, err)
1443+
}
1444+
}
13811445
}
13821446

1383-
// 3. Get the SHA from the ref
1384-
reference, resp, err := githubClient.Git.GetRef(ctx, owner, repo, ref)
1385-
if err != nil {
1386-
_, _ = ghErrors.NewGitHubAPIErrorToCtx(ctx, "failed to get reference", resp, err)
1387-
return nil, fmt.Errorf("failed to get reference: %w", err)
1447+
if reference == nil {
1448+
reference, resp, err = githubClient.Git.GetRef(ctx, owner, repo, ref)
1449+
if err != nil {
1450+
_, _ = ghErrors.NewGitHubAPIErrorToCtx(ctx, "failed to get final reference", resp, err)
1451+
return nil, fmt.Errorf("failed to get final reference for %q: %w", ref, err)
1452+
}
13881453
}
1389-
sha = reference.GetObject().GetSHA()
13901454

1391-
// Use provided ref, or it will be empty which defaults to the default branch
1455+
sha = reference.GetObject().GetSHA()
13921456
return &raw.ContentOpts{Ref: ref, SHA: sha}, nil
13931457
}

‎pkg/github/repositories_test.go

Lines changed: 198 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import (
66
"encoding/json"
77
"net/http"
88
"net/url"
9+
"strings"
910
"testing"
1011
"time"
1112

@@ -2212,63 +2213,239 @@ func Test_resolveGitReference(t *testing.T) {
22122213
ctx := context.Background()
22132214
owner := "owner"
22142215
repo := "repo"
2215-
mockedClient := mock.NewMockedHTTPClient(
2216-
mock.WithRequestMatchHandler(
2217-
mock.GetReposByOwnerByRepo,
2218-
http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
2219-
w.WriteHeader(http.StatusOK)
2220-
_, _ = w.Write([]byte(`{"name": "repo", "default_branch": "main"}`))
2221-
}),
2222-
),
2223-
mock.WithRequestMatchHandler(
2224-
mock.GetReposGitRefByOwnerByRepoByRef,
2225-
http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
2226-
w.WriteHeader(http.StatusOK)
2227-
_, _ = w.Write([]byte(`{"ref": "refs/heads/main", "object": {"sha": "123sha456"}}`))
2228-
}),
2229-
),
2230-
)
22312216

22322217
tests := []struct {
22332218
name string
22342219
ref string
22352220
sha string
2221+
mockSetup func() *http.Client
22362222
expectedOutput *raw.ContentOpts
2223+
expectError bool
2224+
errorContains string
22372225
}{
22382226
{
22392227
name: "sha takes precedence over ref",
22402228
ref: "refs/heads/main",
22412229
sha: "123sha456",
2230+
mockSetup: func() *http.Client {
2231+
// No API calls should be made when SHA is provided
2232+
return mock.NewMockedHTTPClient()
2233+
},
22422234
expectedOutput: &raw.ContentOpts{
22432235
SHA: "123sha456",
22442236
},
2237+
expectError: false,
22452238
},
22462239
{
22472240
name: "use default branch if ref and sha both empty",
22482241
ref: "",
22492242
sha: "",
2243+
mockSetup: func() *http.Client {
2244+
return mock.NewMockedHTTPClient(
2245+
mock.WithRequestMatchHandler(
2246+
mock.GetReposByOwnerByRepo,
2247+
http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
2248+
w.WriteHeader(http.StatusOK)
2249+
_, _ = w.Write([]byte(`{"name": "repo", "default_branch": "main"}`))
2250+
}),
2251+
),
2252+
mock.WithRequestMatchHandler(
2253+
mock.GetReposGitRefByOwnerByRepoByRef,
2254+
http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
2255+
assert.Contains(t, r.URL.Path, "/git/ref/heads/main")
2256+
w.WriteHeader(http.StatusOK)
2257+
_, _ = w.Write([]byte(`{"ref": "refs/heads/main", "object": {"sha": "main-sha"}}`))
2258+
}),
2259+
),
2260+
)
2261+
},
22502262
expectedOutput: &raw.ContentOpts{
22512263
Ref: "refs/heads/main",
2252-
SHA: "123sha456",
2264+
SHA: "main-sha",
22532265
},
2266+
expectError: false,
22542267
},
22552268
{
2256-
name: "get SHA from ref",
2257-
ref: "refs/heads/main",
2269+
name: "fully qualified ref passed through unchanged",
2270+
ref: "refs/heads/feature-branch",
22582271
sha: "",
2272+
mockSetup: func() *http.Client {
2273+
return mock.NewMockedHTTPClient(
2274+
mock.WithRequestMatchHandler(
2275+
mock.GetReposGitRefByOwnerByRepoByRef,
2276+
http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
2277+
assert.Contains(t, r.URL.Path, "/git/ref/heads/feature-branch")
2278+
w.WriteHeader(http.StatusOK)
2279+
_, _ = w.Write([]byte(`{"ref": "refs/heads/feature-branch", "object": {"sha": "feature-sha"}}`))
2280+
}),
2281+
),
2282+
)
2283+
},
2284+
expectedOutput: &raw.ContentOpts{
2285+
Ref: "refs/heads/feature-branch",
2286+
SHA: "feature-sha",
2287+
},
2288+
expectError: false,
2289+
},
2290+
{
2291+
name: "short branch name resolves to refs/heads/",
2292+
ref: "main",
2293+
sha: "",
2294+
mockSetup: func() *http.Client {
2295+
return mock.NewMockedHTTPClient(
2296+
mock.WithRequestMatchHandler(
2297+
mock.GetReposGitRefByOwnerByRepoByRef,
2298+
http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
2299+
if strings.Contains(r.URL.Path, "/git/ref/heads/main") {
2300+
w.WriteHeader(http.StatusOK)
2301+
_, _ = w.Write([]byte(`{"ref": "refs/heads/main", "object": {"sha": "main-sha"}}`))
2302+
} else {
2303+
t.Errorf("Unexpected path: %s", r.URL.Path)
2304+
w.WriteHeader(http.StatusNotFound)
2305+
}
2306+
}),
2307+
),
2308+
)
2309+
},
22592310
expectedOutput: &raw.ContentOpts{
22602311
Ref: "refs/heads/main",
2261-
SHA: "123sha456",
2312+
SHA: "main-sha",
2313+
},
2314+
expectError: false,
2315+
},
2316+
{
2317+
name: "short tag name falls back to refs/tags/ when branch not found",
2318+
ref: "v1.0.0",
2319+
sha: "",
2320+
mockSetup: func() *http.Client {
2321+
return mock.NewMockedHTTPClient(
2322+
mock.WithRequestMatchHandler(
2323+
mock.GetReposGitRefByOwnerByRepoByRef,
2324+
http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
2325+
switch {
2326+
case strings.Contains(r.URL.Path, "/git/ref/heads/v1.0.0"):
2327+
w.WriteHeader(http.StatusNotFound)
2328+
_, _ = w.Write([]byte(`{"message": "Not Found"}`))
2329+
case strings.Contains(r.URL.Path, "/git/ref/tags/v1.0.0"):
2330+
w.WriteHeader(http.StatusOK)
2331+
_, _ = w.Write([]byte(`{"ref": "refs/tags/v1.0.0", "object": {"sha": "tag-sha"}}`))
2332+
default:
2333+
t.Errorf("Unexpected path: %s", r.URL.Path)
2334+
w.WriteHeader(http.StatusNotFound)
2335+
}
2336+
}),
2337+
),
2338+
)
2339+
},
2340+
expectedOutput: &raw.ContentOpts{
2341+
Ref: "refs/tags/v1.0.0",
2342+
SHA: "tag-sha",
2343+
},
2344+
expectError: false,
2345+
},
2346+
{
2347+
name: "heads/ prefix gets refs/ prepended",
2348+
ref: "heads/feature-branch",
2349+
sha: "",
2350+
mockSetup: func() *http.Client {
2351+
return mock.NewMockedHTTPClient(
2352+
mock.WithRequestMatchHandler(
2353+
mock.GetReposGitRefByOwnerByRepoByRef,
2354+
http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
2355+
assert.Contains(t, r.URL.Path, "/git/ref/heads/feature-branch")
2356+
w.WriteHeader(http.StatusOK)
2357+
_, _ = w.Write([]byte(`{"ref": "refs/heads/feature-branch", "object": {"sha": "feature-sha"}}`))
2358+
}),
2359+
),
2360+
)
2361+
},
2362+
expectedOutput: &raw.ContentOpts{
2363+
Ref: "refs/heads/feature-branch",
2364+
SHA: "feature-sha",
22622365
},
2366+
expectError: false,
2367+
},
2368+
{
2369+
name: "tags/ prefix gets refs/ prepended",
2370+
ref: "tags/v1.0.0",
2371+
sha: "",
2372+
mockSetup: func() *http.Client {
2373+
return mock.NewMockedHTTPClient(
2374+
mock.WithRequestMatchHandler(
2375+
mock.GetReposGitRefByOwnerByRepoByRef,
2376+
http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
2377+
assert.Contains(t, r.URL.Path, "/git/ref/tags/v1.0.0")
2378+
w.WriteHeader(http.StatusOK)
2379+
_, _ = w.Write([]byte(`{"ref": "refs/tags/v1.0.0", "object": {"sha": "tag-sha"}}`))
2380+
}),
2381+
),
2382+
)
2383+
},
2384+
expectedOutput: &raw.ContentOpts{
2385+
Ref: "refs/tags/v1.0.0",
2386+
SHA: "tag-sha",
2387+
},
2388+
expectError: false,
2389+
},
2390+
{
2391+
name: "invalid short name that doesn't exist as branch or tag",
2392+
ref: "nonexistent",
2393+
sha: "",
2394+
mockSetup: func() *http.Client {
2395+
return mock.NewMockedHTTPClient(
2396+
mock.WithRequestMatchHandler(
2397+
mock.GetReposGitRefByOwnerByRepoByRef,
2398+
http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
2399+
// Both branch and tag attempts should return 404
2400+
w.WriteHeader(http.StatusNotFound)
2401+
_, _ = w.Write([]byte(`{"message": "Not Found"}`))
2402+
}),
2403+
),
2404+
)
2405+
},
2406+
expectError: true,
2407+
errorContains: "could not resolve ref \"nonexistent\" as a branch or a tag",
2408+
},
2409+
{
2410+
name: "fully qualified pull request ref",
2411+
ref: "refs/pull/123/head",
2412+
sha: "",
2413+
mockSetup: func() *http.Client {
2414+
return mock.NewMockedHTTPClient(
2415+
mock.WithRequestMatchHandler(
2416+
mock.GetReposGitRefByOwnerByRepoByRef,
2417+
http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
2418+
assert.Contains(t, r.URL.Path, "/git/ref/pull/123/head")
2419+
w.WriteHeader(http.StatusOK)
2420+
_, _ = w.Write([]byte(`{"ref": "refs/pull/123/head", "object": {"sha": "pr-sha"}}`))
2421+
}),
2422+
),
2423+
)
2424+
},
2425+
expectedOutput: &raw.ContentOpts{
2426+
Ref: "refs/pull/123/head",
2427+
SHA: "pr-sha",
2428+
},
2429+
expectError: false,
22632430
},
22642431
}
22652432

22662433
for _, tc := range tests {
22672434
t.Run(tc.name, func(t *testing.T) {
22682435
// Setup client with mock
2269-
client := github.NewClient(mockedClient)
2436+
client := github.NewClient(tc.mockSetup())
22702437
opts, err := resolveGitReference(ctx, client, owner, repo, tc.ref, tc.sha)
2438+
2439+
if tc.expectError {
2440+
require.Error(t, err)
2441+
if tc.errorContains != "" {
2442+
assert.Contains(t, err.Error(), tc.errorContains)
2443+
}
2444+
return
2445+
}
2446+
22712447
require.NoError(t, err)
2448+
require.NotNil(t, opts)
22722449

22732450
if tc.expectedOutput.SHA != "" {
22742451
assert.Equal(t, tc.expectedOutput.SHA, opts.SHA)

0 commit comments

Comments
 (0)