diff --git a/docs/migration-1.0.md b/docs/migration-1.0.md index 6d8c5044..14e27f3e 100644 --- a/docs/migration-1.0.md +++ b/docs/migration-1.0.md @@ -282,6 +282,7 @@ changed meaning: | `label edit\|delete` | `--id ID` | label name or ID | | `team view\|edit\|delete`, `team member\|repo ...` | team ID | `ORG/TEAM` or team ID | | `repo delete` | `--name NAME --owner OWNER` | optional `OWNER/REPO` argument (or `--repo`) | +| `pulls close\|reopen\|edit` | several indexes | `pr close\|reopen\|edit` take one pull request (number, URL or branch); loop to change several | | `issue comment` | optional body argument | `--body`, `--body-file` or `--editor` | | `secret set`, `variable set` | ` []`, `--file`, `--stdin` | value from `--body`, `--body-file FILE\|-` or piped standard input | diff --git a/internal/app/pull_create.go b/internal/app/pull_create.go index cf74284c..e71ef8e3 100644 --- a/internal/app/pull_create.go +++ b/internal/app/pull_create.go @@ -7,6 +7,7 @@ import ( "context" "errors" "fmt" + "net/url" "path/filepath" "strconv" "strings" @@ -116,7 +117,7 @@ is not on the remote yet is pushed first, with its upstream set.`, Args: argsExa } } if web { - return a.openBrowser(cmd, repositoryURL(t)+"/compare/"+options.Base+"..."+options.Head) + return a.openBrowser(cmd, repositoryURL(t)+"/compare/"+compareRef(options.Base)+"..."+compareRef(options.Head)) } var commitBody string if options.Title == "" { @@ -373,3 +374,9 @@ func (a *application) reportCreatedPull(cmd *cobra.Command, t target, item servi a.done(cmd, "Created pull request %s", refLabel(t.Repo, item.Number, item.Title)) return a.created(cmd, item, item.URL) } + +// compareRef escapes a branch name, or OWNER:BRANCH, for a compare URL, where a '#' or '%' in the +// name would otherwise start a fragment or an invalid escape. +func compareRef(ref string) string { + return strings.ReplaceAll(url.PathEscape(ref), "%2F", "/") +} diff --git a/internal/app/pull_create_test.go b/internal/app/pull_create_test.go index f84eaa63..1970e494 100644 --- a/internal/app/pull_create_test.go +++ b/internal/app/pull_create_test.go @@ -307,3 +307,9 @@ func TestPullCreateHelpListsDefaultsOnce(t *testing.T) { require.Contains(t, out.String(), "(default true)") require.NotContains(t, out.String(), "(default true) (default true)") } + +func TestCompareRefEscapesBranchNames(t *testing.T) { + require.Equal(t, "feature/login", compareRef("feature/login")) + require.Equal(t, "fix%2312", compareRef("fix#12")) + require.Equal(t, "bob:fix%25x", compareRef("bob:fix%x")) +} diff --git a/internal/app/pull_edit.go b/internal/app/pull_edit.go index 686ede69..6ea89f59 100644 --- a/internal/app/pull_edit.go +++ b/internal/app/pull_edit.go @@ -194,6 +194,9 @@ removes or adds that marker.`, Annotations: pullArgumentsAnnotation(nil), Args: if item, err = service.Edit(cmd.Context(), n, services.PullEdit{Title: &title}); err != nil { return err } + if item.Draft != undo { + return errors.New("the title was changed, but Gitea still " + map[bool]string{true: "does not treat", false: "treats"}[undo] + " the pull request as a draft; this instance may use a different draft prefix, so edit the title with: tea pr edit " + strconv.FormatInt(n, 10) + " --title") + } if undo { a.done(cmd, "Converted pull request %s to a draft", refLabel(t.Repo, n, item.Title)) } else { diff --git a/internal/app/pull_edit_test.go b/internal/app/pull_edit_test.go index 6c75cdf7..24973c2c 100644 --- a/internal/app/pull_edit_test.go +++ b/internal/app/pull_edit_test.go @@ -67,3 +67,14 @@ func TestPullEditAddsAssigneesWithoutExtraReads(t *testing.T) { require.NoError(t, Execute(context.Background(), deps, []string{"pr", "edit", "7", "-R", "alice/project", "--add-assignees", "bob,Alice", "--json=number"})) require.Equal(t, []string{"GET /api/v1/repos/alice/project/issues/7", "PATCH /api/v1/repos/alice/project/pulls/7"}, calls) } + +func TestPullReadyFailsWhenGiteaStillSeesADraft(t *testing.T) { + server := newAuthenticatedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _, _ = w.Write([]byte(`{"number":7,"title":"Draft: x","draft":true}`)) + })) + defer server.Close() + deps, _, diagnostics := serverDeps(t, server.URL) + err := Execute(context.Background(), deps, []string{"pr", "ready", "7", "-R", "alice/project"}) + require.ErrorContains(t, err, "still treats the pull request as a draft") + require.NotContains(t, diagnostics.String(), "Marked") +} diff --git a/internal/app/pull_merge_form_test.go b/internal/app/pull_merge_form_test.go index 958faa64..d736579d 100644 --- a/internal/app/pull_merge_form_test.go +++ b/internal/app/pull_merge_form_test.go @@ -245,3 +245,19 @@ func TestPullMergeRequiresNumberWithoutInteractive(t *testing.T) { require.Error(t, err) require.Contains(t, err.Error(), "pull request number") } + +func TestPullMergeReportsNoBranchDeletionForAGitPullRequests(t *testing.T) { + agit := `{"number":3,"title":"Add login","state":"open","base":{"ref":"main"},"head":{"ref":"refs/pull/3/head","label":"","sha":"abc123"}}` + server := newAuthenticatedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch { + case r.URL.Path == "/api/v1/repos/alice/project": + _, _ = w.Write([]byte(mergeRepository)) + case r.Method == http.MethodGet: + _, _ = w.Write([]byte(agit)) + } + })) + defer server.Close() + deps, _, diagnostics := serverDeps(t, server.URL) + require.NoError(t, Execute(context.Background(), deps, []string{"pr", "merge", "3", "-R", "alice/project", "--yes", "--delete-branch"})) + require.NotContains(t, diagnostics.String(), "emote branch") +} diff --git a/internal/app/pull_mutations.go b/internal/app/pull_mutations.go index b9dfb60a..85bd185a 100644 --- a/internal/app/pull_mutations.go +++ b/internal/app/pull_mutations.go @@ -128,6 +128,8 @@ $ tea pr close 13 -c "Superseded by #15" --delete-branch` // head branch of another open pull request, which deleting it would close. func (a *application) deleteHead(cmd *cobra.Command, service services.Pulls, item services.Pull, merging, local bool) { switch { + case item.Head == "": + // An AGit pull request has no head branch to delete. case item.HeadRemoteDeleted && merging: a.done(cmd, "Deleted remote branch %s", item.Head) case item.HeadRemoteDeleted: @@ -358,7 +360,7 @@ the base branch.`, Annotations: pullArgumentsAnnotation(map[string]string{"tea.c } else { a.done(cmd, "%s pull request %s", mergeVerbs[method], ref) if deleteBranch { - a.deleteHead(cmd, service, merged, true, deleteLocal) + a.deleteHead(cmd, service, merged, !item.HeadRemoteDeleted, deleteLocal) } } return a.jsonResult(cmd, merged) diff --git a/internal/app/pull_mutations_test.go b/internal/app/pull_mutations_test.go index b8232bc8..03d72a4a 100644 --- a/internal/app/pull_mutations_test.go +++ b/internal/app/pull_mutations_test.go @@ -6,6 +6,7 @@ package app import ( "context" "net/http" + "slices" "strings" "testing" @@ -54,3 +55,60 @@ func TestPullDiffNameOnly(t *testing.T) { require.NoError(t, Execute(context.Background(), deps, []string{"pr", "diff", "2", "-R", "alice/project", "--name-only"})) require.Equal(t, "one.txt\ndocs/two.md\n", out.String()) } + +func TestPullCloseCommentsFirstAndDeletesTheBranchOnlyWhenAsked(t *testing.T) { + for _, tc := range []struct { + flags []string + deleted bool + }{ + {nil, false}, + {[]string{"--delete-branch"}, true}, + } { + var requests []string + server := newAuthenticatedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + requests = append(requests, r.Method+" "+r.URL.Path) + switch { + case r.Method == http.MethodGet: + _, _ = w.Write([]byte(`{"number":3,"title":"Add login","state":"open","base":{"ref":"main"},"head":{"ref":"feature","sha":"abc123","repo":{"full_name":"alice/project","permissions":{"push":true}}}}`)) + case r.Method == http.MethodPatch: + _, _ = w.Write([]byte(`{"number":3,"title":"Add login","state":"closed","base":{"ref":"main"},"head":{"ref":"feature","sha":"abc123","repo":{"full_name":"alice/project","permissions":{"push":true}}}}`)) + case r.Method == http.MethodPost: + w.WriteHeader(http.StatusCreated) + _, _ = w.Write([]byte(`{"id":1}`)) + default: + w.WriteHeader(http.StatusNoContent) + } + })) + deps, _, _ := serverDeps(t, server.URL) + deps.Git = &fakeGit{results: map[string]string{"symbolic-ref --short HEAD": "main"}} + require.NoError(t, Execute(context.Background(), deps, append([]string{"pr", "close", "3", "-R", "alice/project", "-c", "Superseded"}, tc.flags...))) + server.Close() + comment := slices.Index(requests, "POST /api/v1/repos/alice/project/issues/3/comments") + edit := slices.Index(requests, "PATCH /api/v1/repos/alice/project/pulls/3") + require.True(t, comment >= 0 && comment < edit, requests) + require.Equal(t, tc.deleted, slices.Contains(requests, "DELETE /api/v1/repos/alice/project/branches/feature"), requests) + } +} + +func TestPullCloseAndReopenRefuseMergedPullRequests(t *testing.T) { + server := newAuthenticatedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + require.Equal(t, http.MethodGet, r.Method) + _, _ = w.Write([]byte(`{"number":3,"title":"Add login","state":"closed","merged":true}`)) + })) + defer server.Close() + for verb, want := range map[string]string{"close": "cannot be closed", "reopen": "cannot be reopened"} { + deps, _, _ := serverDeps(t, server.URL) + require.ErrorContains(t, Execute(context.Background(), deps, []string{"pr", verb, "3", "-R", "alice/project"}), want) + } +} + +func TestPullReopenWarnsWhenAlreadyOpen(t *testing.T) { + server := newAuthenticatedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + require.Equal(t, http.MethodGet, r.Method) + _, _ = w.Write([]byte(`{"number":3,"title":"Add login","state":"open"}`)) + })) + defer server.Close() + deps, _, diagnostics := serverDeps(t, server.URL) + require.NoError(t, Execute(context.Background(), deps, []string{"pr", "reopen", "3", "-R", "alice/project"})) + require.Contains(t, diagnostics.String(), "is already open") +} diff --git a/internal/app/pulls.go b/internal/app/pulls.go index 26faf194..ffa6d3d3 100644 --- a/internal/app/pulls.go +++ b/internal/app/pulls.go @@ -40,7 +40,7 @@ func (a *application) pullListCommand() *cobra.Command { var web bool cmd := &cobra.Command{Use: "list", Short: "List pull requests", Long: `List the pull requests of a repository, newest first. -The reviewers, reviewer_teams and merged_by JSON fields come from separate requests for +The reviewers, reviewer_teams, review_decision and merged_by JSON fields come from separate requests for each pull request, which tea only makes when --json names them.`, Args: argsExact(0), RunE: func(cmd *cobra.Command, _ []string) error { if id, err := strconv.ParseInt(milestone, 10, 64); err == nil && id < 1 { return clierr.Wrap(clierr.Usage, errors.New("--milestone must be a milestone title or a positive ID")) @@ -78,10 +78,14 @@ each pull request, which tea only makes when --json names them.`, Args: argsExac return err } items := window.Items - if a.jsonSelects("reviewers", "reviewer_teams", "merged_by") { - if items, err = service.ViewAll(cmd.Context(), items); err != nil { - return err - } + switch { + case a.jsonSelects("review_decision"): + items, err = service.DecideAll(cmd.Context(), items) + case a.jsonSelects("reviewers", "reviewer_teams", "merged_by"): + items, err = service.ViewAll(cmd.Context(), items) + } + if err != nil { + return err } noun := map[string]string{"open": "open pull requests", "closed": "closed pull requests", "merged": "merged pull requests", "all": "pull requests"}[state] what := noun + " in " + t.Repo diff --git a/internal/app/pulls_test.go b/internal/app/pulls_test.go index cccc4e58..c7ead84f 100644 --- a/internal/app/pulls_test.go +++ b/internal/app/pulls_test.go @@ -90,3 +90,24 @@ func TestPullListFillsReviewersWhenSelected(t *testing.T) { require.JSONEq(t, `[{"number":13,"reviewers":["bob"]}]`, out.String()) require.Equal(t, 1, views) } + +func TestPullListFillsReviewDecisionWhenSelected(t *testing.T) { + server := newAuthenticatedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/api/v1/repos/alice/project/pulls/13/reviews": + if r.URL.Query().Get("page") > "1" { + _, _ = w.Write([]byte(`[]`)) + return + } + _, _ = w.Write([]byte(`[{"id":1,"state":"APPROVED","user":{"login":"bob"}}]`)) + case "/api/v1/repos/alice/project/pulls/13": + _, _ = w.Write([]byte(`{"number":13}`)) + default: + _, _ = w.Write([]byte(`[{"number":13}]`)) + } + })) + defer server.Close() + deps, out, _ := serverDeps(t, server.URL) + require.NoError(t, Execute(context.Background(), deps, []string{"pr", "list", "-R", "alice/project", "--json", "number,review_decision"})) + require.JSONEq(t, `[{"number":13,"review_decision":"approved"}]`, out.String()) +} diff --git a/internal/services/checks.go b/internal/services/checks.go index 1b79645f..e96aed44 100644 --- a/internal/services/checks.go +++ b/internal/services/checks.go @@ -124,6 +124,7 @@ func (s Pulls) WatchChecks(ctx context.Context, n int64, interval time.Duration, return nil, errors.New("check polling interval must be positive") } var previous []Check + reported := false for { sha, err := s.checkHead(ctx, n) if err != nil { @@ -133,7 +134,9 @@ func (s Pulls) WatchChecks(ctx context.Context, n int64, interval time.Duration, if err != nil { return nil, err } - if !slices.Equal(items, previous) { + // The first poll is reported even when empty, so a watcher can say it is waiting. + if !reported || !slices.Equal(items, previous) { + reported = true if changed != nil { if err := changed(items); err != nil { return nil, err diff --git a/internal/services/checks_test.go b/internal/services/checks_test.go index 0acea7cb..ca0da486 100644 --- a/internal/services/checks_test.go +++ b/internal/services/checks_test.go @@ -58,6 +58,35 @@ func TestCheckWatchRevalidatesHeadBeforeReturning(t *testing.T) { require.Equal(t, 5, heads) } +func TestCheckWatchReportsTheEmptyStateOnce(t *testing.T) { + polls := 0 + client := newTestClient(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if strings.Contains(r.URL.Path, "/pulls/") { + _, _ = w.Write([]byte(`{"head":{"sha":"abc"}}`)) + return + } + if r.URL.Query().Get("page") != "1" { + _, _ = w.Write([]byte(`[]`)) + return + } + polls++ + if polls < 3 { + _, _ = w.Write([]byte(`[]`)) + return + } + _, _ = w.Write([]byte(`[{"id":1,"context":"build","status":"success"}]`)) + })) + ctx, cancel := context.WithTimeout(context.Background(), time.Second) + defer cancel() + var reported []int + _, err := (Pulls{Client: client, Repository: "owner/repo"}).WatchChecks(ctx, 7, time.Millisecond, func(items []Check) error { + reported = append(reported, len(items)) + return nil + }) + require.NoError(t, err) + require.Equal(t, []int{0, 1}, reported) +} + func TestCheckWatchTreatsSkippedAsPassing(t *testing.T) { snapshots := 0 client := newTestClient(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { diff --git a/internal/services/errors.go b/internal/services/errors.go index 9f7f97dc..ae0a2a28 100644 --- a/internal/services/errors.go +++ b/internal/services/errors.go @@ -159,6 +159,9 @@ var ( unknownUser = regexp.MustCompile(`user does not exist \[uid: \d+, name: ([^\]]+)\]`) // pullTargets finds the branches in "pull request already exists for these targets [... head_branch: x, base_branch: y]". pullTargets = regexp.MustCompile(`head_branch: ([^,\]]+), base_branch: ([^,\]]+)`) + // missingBranch finds the branch in the 404 messages `ref "x" does not exist` (create pull + // request) and `new base 'x' not exist` (edit pull request). + missingBranch = regexp.MustCompile(`^(?:ref "([^"]+)" does not exist|new base '([^']+)' not exist)$`) // wikiTitle finds the page in "wiki page already exists [title: Home.md]". wikiTitle = regexp.MustCompile(`\[title: (.+?)(?:\.md)?\]`) ) @@ -186,6 +189,9 @@ func Classify(err error, kind, key, scope string) error { } return conflict } + if match := missingBranch.FindStringSubmatch(apiErr.Message); apiErr.Status == http.StatusNotFound && match != nil { + return &NotFoundError{Kind: "branch", Key: strconv.Quote(cmp.Or(match[1], match[2])), Scope: scope, Err: err} + } if apiErr.Status == http.StatusNotFound && kind != "" { return &NotFoundError{Kind: kind, Key: key, Scope: scope, Err: err} } diff --git a/internal/services/errors_test.go b/internal/services/errors_test.go index e52293e1..72dcf708 100644 --- a/internal/services/errors_test.go +++ b/internal/services/errors_test.go @@ -64,3 +64,10 @@ func TestConflictHintsFollowTheReason(t *testing.T) { err := &ConflictError{Reason: ReasonArchived, Scope: "alice/old", Hint: "unarchive it with: tea repo edit alice/old --archived=false"} require.EqualError(t, err, "alice/old is archived (read-only); unarchive it with: tea repo edit alice/old --archived=false") } + +func TestClassifyNamesAMissingBranchNotThePullRequest(t *testing.T) { + for _, message := range []string{`ref "develp" does not exist`, `new base 'develp' not exist`} { + err := Classify(&transport.APIError{Status: http.StatusNotFound, Message: message}, "pull request", "#3", "alice/x") + require.EqualError(t, err, `branch "develp" not found in alice/x`) + } +} diff --git a/internal/services/pulls.go b/internal/services/pulls.go index e2fb2944..f291655c 100644 --- a/internal/services/pulls.go +++ b/internal/services/pulls.go @@ -241,6 +241,26 @@ func (s Pulls) ViewAll(ctx context.Context, items []Pull) ([]Pull, error) { return viewed, err } +// DecideAll is ViewAll that also fills each pull request's ReviewDecision from its reviews. +func (s Pulls) DecideAll(ctx context.Context, items []Pull) ([]Pull, error) { + viewed := make([]Pull, len(items)) + reviews := Reviews{Client: s.Client, Repository: s.Repository} + err := concurrently(len(items), func(i int) error { + item, err := s.View(ctx, items[i].Number) + if err != nil { + return err + } + all, err := reviews.All(ctx, item.Number) + if err != nil { + return err + } + item.ReviewDecision = ReviewDecision(LatestReviews(all), len(item.Reviewers)+len(item.ReviewerTeams) > 0) + viewed[i] = item + return nil + }) + return viewed, err +} + func (s Pulls) get(ctx context.Context, n int64) (*gitea.PullRequest, error) { item, err := repoCall(ctx, s.Client, s.Repository, func(ctx context.Context, c *gitea.Client, owner, name string) (*gitea.PullRequest, *gitea.Response, error) { return c.PullRequests.GetPullRequest(ctx, owner, name, n)