diff --git a/api/queries_repo.go b/api/queries_repo.go index d1bc2df1e29..14b1ffc2a2e 100644 --- a/api/queries_repo.go +++ b/api/queries_repo.go @@ -1685,6 +1685,26 @@ func RepoExists(client *Client, repo ghrepo.Interface) (bool, error) { } } +func CommitExists(client *Client, repo ghrepo.Interface, ref string) (bool, error) { + path := fmt.Sprintf("repos/%s/%s/commits/%s", repo.RepoOwner(), repo.RepoName(), ref) + + resp, err := client.HTTP().Head(ghinstance.RESTPrefix(repo.RepoHost()) + path) + if err != nil { + return false, err + } + + defer resp.Body.Close() + + switch resp.StatusCode { + case 200: + return true, nil + case 404, 422: + return false, nil + default: + return false, ghAPI.HandleHTTPError(resp) + } +} + // RepoLicenses fetches available repository licenses. // It uses API v3 because licenses are not supported by GraphQL. func RepoLicenses(httpClient *http.Client, hostname string) ([]License, error) { diff --git a/api/queries_repo_test.go b/api/queries_repo_test.go index 4fe7074f187..123e39ef8f2 100644 --- a/api/queries_repo_test.go +++ b/api/queries_repo_test.go @@ -823,6 +823,79 @@ func TestRepoExists(t *testing.T) { } } +func TestCommitExists(t *testing.T) { + tests := []struct { + name string + httpStub func(*httpmock.Registry) + existCheck bool + wantErrMsg string + }{ + { + name: "commit exists", + httpStub: func(r *httpmock.Registry) { + r.Register( + httpmock.REST("HEAD", "repos/OWNER/REPO/commits/309628980"), + httpmock.StringResponse("{}"), + ) + }, + existCheck: true, + }, + { + name: "commit not found", + httpStub: func(r *httpmock.Registry) { + r.Register( + httpmock.REST("HEAD", "repos/OWNER/REPO/commits/1234567"), + httpmock.StatusStringResponse(404, "Not Found"), + ) + }, + existCheck: false, + }, + { + name: "unprocessable ref", + httpStub: func(r *httpmock.Registry) { + r.Register( + httpmock.REST("HEAD", "repos/OWNER/REPO/commits/1234567"), + httpmock.StatusStringResponse(422, "Unprocessable Entity"), + ) + }, + existCheck: false, + }, + { + name: "http error", + httpStub: func(r *httpmock.Registry) { + r.Register( + httpmock.REST("HEAD", "repos/OWNER/REPO/commits/1234567"), + httpmock.StatusStringResponse(500, "Internal Server Error"), + ) + }, + existCheck: false, + wantErrMsg: "HTTP 500 (https://api.github.com/repos/OWNER/REPO/commits/1234567)", + }, + } + for _, tt := range tests { + reg := &httpmock.Registry{} + if tt.httpStub != nil { + tt.httpStub(reg) + } + + client := newTestClient(reg) + ref := "309628980" + if !tt.existCheck || tt.wantErrMsg != "" { + ref = "1234567" + } + + t.Run(tt.name, func(t *testing.T) { + exist, err := CommitExists(client, ghrepo.New("OWNER", "REPO"), ref) + if tt.wantErrMsg != "" { + assert.Equal(t, tt.wantErrMsg, err.Error()) + } else { + assert.NoError(t, err) + } + assert.Equal(t, tt.existCheck, exist) + }) + } +} + func TestForkRepoReturnsErrorWhenForkIsNotPossible(t *testing.T) { // Given our API returns 202 with a Fork that is the same as // the repo we provided diff --git a/pkg/cmd/browse/browse.go b/pkg/cmd/browse/browse.go index d057139565a..a6ea66408fe 100644 --- a/pkg/cmd/browse/browse.go +++ b/pkg/cmd/browse/browse.go @@ -250,6 +250,22 @@ func parseSection(baseRepo ghrepo.Interface, opts *BrowseOptions) (string, error return "", nil } if isNumber(opts.SelectorArg) { + // A selector without a "#" prefix that satisfies both isNumber and + // isCommit is ambiguous (e.g. "309628980"). Disambiguate via API. + if !strings.HasPrefix(opts.SelectorArg, "#") && isCommit(opts.SelectorArg) { + httpClient, err := opts.HttpClient() + if err != nil { + return "", err + } + apiClient := api.NewClientFromHTTP(httpClient) + exists, err := api.CommitExists(apiClient, baseRepo, opts.SelectorArg) + if err != nil { + return "", err + } + if exists { + return fmt.Sprintf("commit/%s", opts.SelectorArg), nil + } + } return fmt.Sprintf("issues/%s", strings.TrimPrefix(opts.SelectorArg, "#")), nil } if isCommit(opts.SelectorArg) { diff --git a/pkg/cmd/browse/browse_test.go b/pkg/cmd/browse/browse_test.go index c321fbdbb57..233b45f6a40 100644 --- a/pkg/cmd/browse/browse_test.go +++ b/pkg/cmd/browse/browse_test.go @@ -674,6 +674,45 @@ func Test_runBrowse(t *testing.T) { expectedURL: "https://github.com/owner/repo/blame/abc123/src/app.js#L50", wantsErr: false, }, + { + name: "decimal-only short SHA that is a valid commit", + opts: BrowseOptions{ + SelectorArg: "309628980", + }, + httpStub: func(r *httpmock.Registry) { + r.Register( + httpmock.REST("HEAD", "repos/cli/cli/commits/309628980"), + httpmock.StatusStringResponse(200, ""), + ) + }, + baseRepo: ghrepo.New("cli", "cli"), + expectedURL: "https://github.com/cli/cli/commit/309628980", + wantsErr: false, + }, + { + name: "decimal-only string that is not a valid commit", + opts: BrowseOptions{ + SelectorArg: "1234567", + }, + httpStub: func(r *httpmock.Registry) { + r.Register( + httpmock.REST("HEAD", "repos/cli/cli/commits/1234567"), + httpmock.StatusStringResponse(404, ""), + ) + }, + baseRepo: ghrepo.New("cli", "cli"), + expectedURL: "https://github.com/cli/cli/issues/1234567", + wantsErr: false, + }, + { + name: "decimal-only short SHA with # prefix forces issue", + opts: BrowseOptions{ + SelectorArg: "#309628980", + }, + baseRepo: ghrepo.New("cli", "cli"), + expectedURL: "https://github.com/cli/cli/issues/309628980", + wantsErr: false, + }, } for _, tt := range tests {