browse: disambiguate decimal-only short SHAs from issue numbers#13347
browse: disambiguate decimal-only short SHAs from issue numbers#13347SebTardif wants to merge 2 commits into
Conversation
When a selector argument passed to gh browse consists entirely of decimal digits and is at least 7 characters long, it satisfies both isNumber and isCommit. Previously isNumber was always checked first, so gh browse 3096289 would open an issues URL even when that string referred to an actual commit SHA. Only in the ambiguous case (decimal-only, no # prefix, passes both checks), call the commits API to determine whether the ref resolves to a real commit. If it does, open the commit URL; otherwise fall back to the issues URL. No API call is made for unambiguous inputs. Fixes cli#12357
There was a problem hiding this comment.
Pull request overview
This PR fixes gh browse so ambiguous decimal-only selectors that also look like short commit SHAs are checked against the commits API before deciding between an issue URL and a commit URL.
Changes:
- Added commit-existence lookup logic for ambiguous numeric selectors in
gh browse. - Added an API helper to probe whether a commit ref exists.
- Added browse command tests for valid commit, invalid commit, and
#-prefixed issue cases.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
pkg/cmd/browse/browse.go |
Adds the disambiguation branch that probes the commits API before choosing issues/ vs commit/. |
pkg/cmd/browse/browse_test.go |
Adds end-to-end command tests for the new ambiguous-selector behavior. |
api/queries_repo.go |
Adds CommitExists to check commit existence via a HEAD request. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 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 | ||
| } |
There was a problem hiding this comment.
This is intentional. The maintainer explicitly chose correctness over graceful degradation in the issue discussion:
I think it's rare enough that we should prioritise correctness here and hit the API.
The issue author agreed. Silently guessing the wrong URL during an outage would be a worse user experience than surfacing the error — especially since gh browse already makes API calls on the common path (e.g. to resolve the default branch).
| 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) |
There was a problem hiding this comment.
Addressed — added direct unit tests for CommitExists in api/queries_repo_test.go (second commit). Covers all four status code paths: 200 (exists), 404 (not found), 422 (unprocessable ref), and 500 (server error).
Cover all status code paths: 200 (exists), 404 (not found), 422 (unprocessable ref), and 500 (server error).
|
@williammartin This PR implements the fix per your acceptance criteria. All three scenarios are covered with tests. Happy to adjust anything based on your feedback. |
Fixes #12357
Summary
When a selector argument passed to
gh browseconsists entirely of decimal digits and is at least 7 characters long (e.g.309628980), it satisfies bothisNumberandisCommit. PreviouslyisNumberwas always checked first, sogh browse 309628980would open an issues URL even when that string referred to an actual commit SHA.This PR disambiguates only in the ambiguous case by checking the commits API:
#prefix and satisfies bothisNumberandisCommit, callHEAD /repos/{owner}/{repo}/commits/{ref}#prefix still forces issue behavior#-prefixed args)Acceptance Criteria Coverage
AC1: Ambiguous decimal-only short hash that IS a valid commit
API returns 200 →
parseSectionreturnscommit/309628980. Covered by test case "decimal-only short SHA that is a valid commit".AC2: Ambiguous decimal-only string that is NOT a valid commit
API returns 404 →
parseSectionfalls through toissues/1234567. Covered by test case "decimal-only string that is not a valid commit".AC3:
#prefix forces issueThe
strings.HasPrefix(opts.SelectorArg, "#")check short-circuits before any API call. Covered by test case "decimal-only short SHA with # prefix forces issue".Changes
api/queries_repo.goCommitExistshelper (mirrors existingRepoExistspattern)pkg/cmd/browse/browse.gopkg/cmd/browse/browse_test.goValidation