From 71189fd15f7badf25967c6b8a3bc0cc166f0c590 Mon Sep 17 00:00:00 2001 From: bircni Date: Sun, 11 Oct 2026 22:46:32 +0200 Subject: [PATCH] fix: tighten error classification and pull request selection Only Gitea's archived-repository messages mean the repository is archived, so an archived label is no longer reported as one. Refused POSTs to the organization, team, user and SSH key collections name the resource they would have created, and the existing pull request is looked up with escaped branch names. pullSelector without an argument refuses an explicit --repo, like gh: the checked-out branch says nothing about another repository. Signed-off-by: bircni --- internal/app/args.go | 7 ++++--- internal/app/download.go | 14 +++----------- internal/app/errors.go | 3 ++- internal/app/ref.go | 21 ++++++++++++++------- internal/app/ref_test.go | 5 +++++ internal/services/errors.go | 10 +++++++++- internal/services/errors_test.go | 4 ++++ 7 files changed, 41 insertions(+), 23 deletions(-) diff --git a/internal/app/args.go b/internal/app/args.go index 0da1fbf2..1bc9a1ed 100644 --- a/internal/app/args.go +++ b/internal/app/args.go @@ -295,10 +295,11 @@ func argumentName(cmd *cobra.Command, argument string) string { if group == cmd { return text } - if noun := group.Name(); noun == "pr" { - return "pull request " + text + noun := group.Name() + if noun == "pr" { + noun = "pull request" } - return group.Name() + " " + text + return noun + " " + text } // localFlagsChanged reports whether any command-specific flag was supplied. diff --git a/internal/app/download.go b/internal/app/download.go index 9ed820c7..dbecb1ea 100644 --- a/internal/app/download.go +++ b/internal/app/download.go @@ -5,6 +5,7 @@ package app import ( "bufio" + "cmp" "errors" "fmt" "io" @@ -58,10 +59,10 @@ func (f downloadFlags) destination(name string) (path string, skip bool, err err } path = f.output if path == "" || isDirectory(path) { - if name != filepath.Base(name) || name == "." || name == ".." || strings.ContainsAny(name, `/\`) { + if name == "" || name == "." || name == ".." || strings.ContainsAny(name, `/\`) { return "", false, clierr.Wrap(clierr.Usage, fmt.Errorf("cannot use %q as a file name; pass --output", name)) } - path = filepath.Join(cmpOrDot(path, f.dir), name) + path = filepath.Join(cmp.Or(path, f.dir, "."), name) } if _, err := os.Lstat(path); err == nil { if f.skipExisting { @@ -76,15 +77,6 @@ func (f downloadFlags) destination(name string) (path string, skip bool, err err return path, false, nil } -func cmpOrDot(values ...string) string { - for _, value := range values { - if value != "" { - return value - } - } - return "." -} - func isDirectory(path string) bool { info, err := os.Stat(path) return (err == nil && info.IsDir()) || strings.HasSuffix(path, string(filepath.Separator)) || strings.HasSuffix(path, "/") diff --git a/internal/app/errors.go b/internal/app/errors.go index 197b1083..01ac67e1 100644 --- a/internal/app/errors.go +++ b/internal/app/errors.go @@ -10,6 +10,7 @@ import ( "fmt" "net" "net/http" + "net/url" "strings" "syscall" "time" @@ -206,7 +207,7 @@ func existsHint(ctx context.Context, apiErr *transport.APIError, conflict *servi var pull struct { URL string `json:"html_url"` } - if apiErr.Lookup(ctx, "repos/"+conflict.Scope+"/pulls/"+base+"/"+head, &pull) == http.StatusOK && pull.URL != "" { + if apiErr.Lookup(ctx, "repos/"+conflict.Scope+"/pulls/"+url.PathEscape(base)+"/"+url.PathEscape(head), &pull) == http.StatusOK && pull.URL != "" { return "see " + pull.URL } case "wiki page": diff --git a/internal/app/ref.go b/internal/app/ref.go index ef18437a..d713d16f 100644 --- a/internal/app/ref.go +++ b/internal/app/ref.go @@ -74,11 +74,14 @@ func parseRefURL(raw string, kind refKind) (ref, error) { return ref{}, invalid } segments := strings.Split(strings.Trim(u.Path, "/"), "/") - positive := func(i int) int64 { - if i >= len(segments) { - return 0 + at := func(i int) string { + if i < len(segments) { + return segments[i] } - n, err := strconv.ParseInt(segments[i], 10, 64) + return "" + } + positive := func(i int) int64 { + n, err := strconv.ParseInt(at(i), 10, 64) if err != nil || n < 1 { return 0 } @@ -88,12 +91,12 @@ func parseRefURL(raw string, kind refKind) (ref, error) { for i := 2; i < len(segments); i++ { result := ref{} switch { - case kind == runRef && segments[i] == "actions" && i+2 < len(segments) && segments[i+1] == "runs" && positive(i+2) > 0: + case kind == runRef && at(i) == "actions" && at(i+1) == "runs" && positive(i+2) > 0: result.Number = positive(i + 2) - if i+4 < len(segments) && segments[i+3] == "jobs" { + if at(i+3) == "jobs" { result.Job = positive(i + 4) } - case kind != runRef && (segments[i] == "issues" || segments[i] == "pulls") && positive(i+1) > 0: + case kind != runRef && (at(i) == "issues" || at(i) == "pulls") && positive(i+1) > 0: result.Number = positive(i + 1) default: continue @@ -188,6 +191,10 @@ func (a *application) sameRepository(first, second string) bool { // checked-out branch. func (a *application) pullSelector(cmd *cobra.Command, args []string) (target, int64, error) { if len(args) == 0 || args[0] == "" { + // The checked-out branch says nothing about a repository chosen with --repo. + if a.options.repo != "" { + return target{}, 0, clierr.Wrap(clierr.Usage, errors.New("pass a pull request number, URL or branch with --repo")) + } return a.currentPull(cmd.Context()) } raw := args[0] diff --git a/internal/app/ref_test.go b/internal/app/ref_test.go index 60352a3c..ad79b4c5 100644 --- a/internal/app/ref_test.go +++ b/internal/app/ref_test.go @@ -135,6 +135,11 @@ func TestPullSelectorDefaultsToTheCheckedOutBranch(t *testing.T) { require.Equal(t, tc.want, n) }) } + a, _ := refApplication(t, branch("feat/two", nil)) + a.options.repo = "alice/project" + _, _, err := a.pullSelector(commandContext(), nil) + require.Equal(t, clierr.Usage, clierr.ExitCode(err)) + require.EqualError(t, err, "pass a pull request number, URL or branch with --repo") } func TestPullSelectorAcceptsNumbersURLsAndBranches(t *testing.T) { diff --git a/internal/services/errors.go b/internal/services/errors.go index 73047ac6..b3778bd4 100644 --- a/internal/services/errors.go +++ b/internal/services/errors.go @@ -141,7 +141,7 @@ var refusals = []struct { statuses []int fragments []string }{ - {ReasonArchived, nil, []string{"is archived"}}, + {ReasonArchived, nil, []string{"repo is archived", "repository is archived", "> is archived"}}, {ReasonMerged, nil, []string{"already merged"}}, {ReasonDraft, []int{http.StatusMethodNotAllowed}, []string{"work in progress", "draft"}}, {ReasonOwnReview, nil, []string{"approve your own pull", "reject your own pull"}}, @@ -341,16 +341,24 @@ func instanceResource(parts []string) Resource { return Resource{Kind: "member", Key: strconv.Quote(at(3)), Scope: at(1)} case at(0) == "orgs" && at(1) != "" && at(2) == "repos": return Resource{Kind: "organization", Key: strconv.Quote(at(1)), Collection: "repository"} + case at(0) == "orgs" && at(1) != "" && at(2) == "teams": + return Resource{Kind: "organization", Key: strconv.Quote(at(1)), Collection: "team"} case at(0) == "orgs" && at(1) != "": return Resource{Kind: "organization", Key: strconv.Quote(at(1))} + case at(0) == "orgs": + return Resource{Collection: "organization"} case at(0) == "users" && at(1) != "": return Resource{Kind: "user", Key: strconv.Quote(at(1))} case at(0) == "admin" && at(1) == "users" && at(2) != "": return Resource{Kind: "user", Key: strconv.Quote(at(2))} + case at(0) == "admin" && at(1) == "users": + return Resource{Collection: "user"} case at(0) == "teams" && isNumber(at(1)): return Resource{Kind: "team", Key: at(1)} case at(0) == "user" && at(1) == "keys" && at(2) != "": return Resource{Kind: "SSH key", Key: at(2)} + case at(0) == "user" && at(1) == "keys": + return Resource{Collection: "SSH key"} case at(0) == "user" && at(1) == "repos": return Resource{Collection: "repository"} case at(0) == "notifications" && at(1) == "threads" && at(2) != "": diff --git a/internal/services/errors_test.go b/internal/services/errors_test.go index b63c59e0..97138934 100644 --- a/internal/services/errors_test.go +++ b/internal/services/errors_test.go @@ -51,6 +51,10 @@ func TestClassifyRequestNamesTheResource(t *testing.T) { {"wiki page exists", "POST", "/api/v1/repos/alice/x/wiki/new", "wiki page already exists [title: Home.md]", 400, `wiki page "Home" already exists in alice/x`}, {"repository exists", "POST", "/api/v1/user/repos", "The repository with the same name already exists.", 409, "repository already exists"}, {"org repository exists", "POST", "/api/v1/orgs/acme/repos", "The repository with the same name already exists.", 409, "repository already exists"}, + {"team exists", "POST", "/api/v1/orgs/acme/teams", "team already exists [org_id: 3, name: devs]", 422, "team already exists"}, + {"user exists", "POST", "/api/v1/admin/users", "user already exists [name: bob]", 422, "user already exists"}, + {"organization exists", "POST", "/api/v1/orgs", "user already exists [name: acme]", 422, "organization already exists"}, + {"archived label is no repository", "POST", "/api/v1/repos/alice/x/issues/1/labels", "label is archived", 422, "Gitea API returned HTTP 422 for POST /api/v1/repos/alice/x/issues/1/labels: label is archived"}, {"run completed", "POST", "/api/v1/repos/alice/x/actions/runs/35/cancel", "run is already completed", 409, "run 35 is already completed"}, {"run not done", "DELETE", "/api/v1/repos/alice/x/actions/runs/37", "this workflow run is not done", 400, "run 37 has not finished"}, {"workflow disabled", "POST", "/api/v1/repos/alice/x/actions/workflows/nightly.yaml/dispatches", "workflow is disabled", 403, "workflow nightly.yaml is disabled"},