From 339527b26c4aad690c41170e8a3a678f4bf222fb Mon Sep 17 00:00:00 2001 From: bircni Date: Sun, 18 Oct 2026 20:38:01 +0200 Subject: [PATCH] fix(pr,time,release): closed PRs for checkout and edit, dismiss state check first, worktree-aware checkout errors, exact piped durations (follow gh) Signed-off-by: bircni --- internal/app/checkout.go | 2 +- internal/app/editor_test.go | 3 +-- internal/app/pull_closed_lookup_test.go | 16 ++++++++----- internal/app/pull_edit.go | 2 +- internal/app/pull_review_lifecycle.go | 30 +++++++++++++++++++++---- internal/app/pull_review_test.go | 15 +++++++++++++ internal/app/releases.go | 4 ++-- internal/app/time.go | 17 ++++++++++---- internal/app/time_test.go | 19 ++++++++++++---- internal/git/checkout.go | 27 ++++++++++++++++++++++ internal/git/checkout_test.go | 19 ++++++++++++++++ 11 files changed, 131 insertions(+), 23 deletions(-) diff --git a/internal/app/checkout.go b/internal/app/checkout.go index 67e7634b..7d2db066 100644 --- a/internal/app/checkout.go +++ b/internal/app/checkout.go @@ -83,7 +83,7 @@ that git push updates it and pr commands without an argument select the pull req if cmd.Flags().Changed("branch") && strings.TrimSpace(branch) == "" { return clierr.Wrap(clierr.Usage, errors.New("--branch cannot be empty")) } - target, n, err := a.pullSelector(cmd, args) + target, n, err := a.pullSelectorAny(cmd, args) if err != nil { return err } diff --git a/internal/app/editor_test.go b/internal/app/editor_test.go index 4b19b480..1ee6fb8e 100644 --- a/internal/app/editor_test.go +++ b/internal/app/editor_test.go @@ -293,9 +293,8 @@ func TestReviewLifecycleEditorRejectsEmptyRequiredText(t *testing.T) { for _, verb := range []string{"submit", "dismiss"} { t.Run(verb, func(t *testing.T) { server := newAuthenticatedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - require.Equal(t, "submit", verb) require.Equal(t, http.MethodGet, r.Method) - _, _ = w.Write([]byte(`{"id":7,"body":"Saved draft"}`)) + _, _ = w.Write([]byte(`{"id":7,"body":"Saved draft","state":"APPROVED","user":{"login":"bob"}}`)) })) defer server.Close() deps, out, _ := testDeps(t) diff --git a/internal/app/pull_closed_lookup_test.go b/internal/app/pull_closed_lookup_test.go index 305ded3a..8157c7be 100644 --- a/internal/app/pull_closed_lookup_test.go +++ b/internal/app/pull_closed_lookup_test.go @@ -4,6 +4,7 @@ package app import ( + "cmp" "context" "fmt" "net/http" @@ -97,11 +98,15 @@ func TestPullCommandsFindClosedPullsByBranch(t *testing.T) { name string args []string err string + // request is the request that proves the closed pull request was found. + request string }{ - {"view", []string{"pr", "view", "alice:gone", "--json=number"}, ""}, - {"checks", []string{"pr", "checks", "alice:gone", "--web", "--no-input"}, ""}, - {"reopen", []string{"pr", "reopen", "alice:gone"}, "already merged"}, - {"clean", []string{"pr", "clean", "alice:gone", "--no-input"}, ""}, + {"view", []string{"pr", "view", "alice:gone", "--json=number"}, "", ""}, + {"checks", []string{"pr", "checks", "alice:gone", "--web", "--no-input"}, "", ""}, + {"reopen", []string{"pr", "reopen", "alice:gone"}, "already merged", ""}, + {"clean", []string{"pr", "clean", "alice:gone", "--no-input"}, "", ""}, + {"edit", []string{"pr", "edit", "alice:gone", "--title", "New"}, "", "PATCH /api/v1/repos/alice/project/pulls/31?state="}, + {"checkout", []string{"pr", "checkout", "alice:gone", "--detach"}, "missing fake", "GET /api/v1/repos/alice/project/pulls?state=closed"}, } { t.Run(tc.name, func(t *testing.T) { var requests []string @@ -116,7 +121,8 @@ func TestPullCommandsFindClosedPullsByBranch(t *testing.T) { if err != nil { require.NotContains(t, err.Error(), "no open pull request") } - require.Contains(t, requests, "GET /api/v1/repos/alice/project/pulls/31?state=", "err: %v", err) + request := cmp.Or(tc.request, "GET /api/v1/repos/alice/project/pulls/31?state=") + require.Contains(t, requests, request, "err: %v", err) }) } } diff --git a/internal/app/pull_edit.go b/internal/app/pull_edit.go index d7932759..4a6f4d37 100644 --- a/internal/app/pull_edit.go +++ b/internal/app/pull_edit.go @@ -76,7 +76,7 @@ pull request, or mark it as a draft or ready for review.`, Annotations: pullArgu if opts == (services.PullEdit{}) && !useEditor && !draft && !ready && !metadata.changed(cmd) && !cmd.Flags().Changed("milestone") && !removeMilestone { return clierr.Wrap(clierr.Usage, errors.New("nothing to edit; pass a flag such as --title (see tea pr edit --help)")) } - t, n, err := a.pullSelector(cmd, args) + t, n, err := a.pullSelectorAny(cmd, args) if err != nil { return err } diff --git a/internal/app/pull_review_lifecycle.go b/internal/app/pull_review_lifecycle.go index da7a6d1a..99036a53 100644 --- a/internal/app/pull_review_lifecycle.go +++ b/internal/app/pull_review_lifecycle.go @@ -153,10 +153,9 @@ func (a *application) pullReviewDismissCommand(dismiss bool) *cobra.Command { var draft terminal.Draft defer finishDraft(&draft, &runErr) var err error - if dismiss { - if err := validateBodySources(cmd); err != nil { - return err - } + // The reason is collected before any request when it comes from flags or cannot be + // prompted for; an editor or prompt waits until the review is known to be dismissable. + collect := func() error { switch { case useEditor: draft, err = a.editDraft(cmd, "") @@ -172,6 +171,24 @@ func (a *application) pullReviewDismissCommand(dismiss bool) *cobra.Command { if strings.TrimSpace(body) == "" { return clierr.Wrap(clierr.Usage, errors.New("a dismissal needs a reason; pass --body, --body-file or --editor")) } + return nil + } + collected := false + if dismiss { + if err := validateBodySources(cmd); err != nil { + return err + } + snapshot, err := a.settings() + if err != nil { + return err + } + flagged := cmd.Flags().Changed("body") || cmd.Flags().Changed("body-file") + if !useEditor && (flagged || !a.policy(snapshot).CanPrompt(a.deps.Streams)) { + if err := collect(); err != nil { + return err + } + collected = true + } } t, n, id, err := a.pullAndID(cmd, args) if err != nil { @@ -191,6 +208,11 @@ func (a *application) pullReviewDismissCommand(dismiss bool) *cobra.Command { a.warn(cmd, "%s is not dismissed", subject) return a.jsonResult(cmd, current) } + if dismiss && !collected { + if err := collect(); err != nil { + return err + } + } if dismiss { if err := a.confirmAction(cmd, terminal.Confirmation{Prompt: "Dismiss " + subject + "?"}); err != nil { return err diff --git a/internal/app/pull_review_test.go b/internal/app/pull_review_test.go index 69487d0e..568c561d 100644 --- a/internal/app/pull_review_test.go +++ b/internal/app/pull_review_test.go @@ -288,6 +288,21 @@ func TestReviewDismissChecksTheReviewStateFirst(t *testing.T) { require.Zero(t, mutations) } +func TestReviewDismissOpensNoEditorForAnUndismissableReview(t *testing.T) { + server := newAuthenticatedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + require.Equal(t, http.MethodGet, r.Method) + _, _ = w.Write([]byte(`{"id":2,"state":"COMMENT","user":{"login":"bob"}}`)) + })) + defer server.Close() + deps, _, _ := testDeps(t) + editor, _ := newDraftEditor(t, "Reason") + deps.Editor = editor + deps.Getenv = serverEnv(server.URL, map[string]string{"EDITOR": "editor"}) + err := Execute(context.Background(), deps, []string{"pr", "review", "dismiss", "1", "2", "-R", "alice/project", "--editor", "--no-input", "--yes"}) + require.ErrorContains(t, err, "only approvals and change requests can be dismissed") + require.Empty(t, editor.command, "the editor must not open") +} + func TestReviewDeleteAndDismissRequireYesWithoutInput(t *testing.T) { mutations := 0 server := newAuthenticatedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { diff --git a/internal/app/releases.go b/internal/app/releases.go index 84310ad7..76b8ac0c 100644 --- a/internal/app/releases.go +++ b/internal/app/releases.go @@ -416,7 +416,7 @@ $ tea release edit v1.0.0-rc1 v1.0.0-rc2 --prerelease=false`, return err } if opts.Body != nil && *opts.Body == "" { - return clierr.Wrap(clierr.Usage, errors.New("release notes cannot be cleared; Gitea ignores empty notes")) + return clierr.Wrap(clierr.Usage, errors.New("release notes cannot be cleared because Gitea ignores empty notes; edit them to a placeholder, or delete and recreate the release")) } if cmd.Flags().Changed("tag") { if len(args) != 1 || strings.TrimSpace(fields.tag) == "" { @@ -443,7 +443,7 @@ $ tea release edit v1.0.0-rc1 v1.0.0-rc2 --prerelease=false`, opts.Body = &editorDraft.Body } if editorDraft.Body == "" && fields.editor { - return clierr.Wrap(clierr.Usage, errors.New("release notes cannot be cleared; Gitea ignores empty notes")) + return clierr.Wrap(clierr.Usage, errors.New("release notes cannot be cleared because Gitea ignores empty notes; edit them to a placeholder, or delete and recreate the release")) } return batchDone(a, cmd, args, func(tag string) string { return "release " + tag }, func(tag string) (services.Release, error) { item, err := service.View(cmd.Context(), tag) diff --git a/internal/app/time.go b/internal/app/time.go index 96a1bae1..db62e57c 100644 --- a/internal/app/time.go +++ b/internal/app/time.go @@ -106,7 +106,7 @@ $ tea time list --mine --since 168h`, Args: argsExact(0), RunE: func(cmd *cobra. } spec := listSpec[services.TrackedTime]{ Columns: []terminal.Column{{Header: "ID"}, {Header: "Issue", Flex: true}, {Header: "User"}, {Header: "Spent"}, {Header: "When"}}, - Row: trackedTimeRow(target.Repo), Empty: "no time tracked " + where, Context: "time records " + where, + Row: trackedTimeRow(target.Repo, !a.display().TTY), Empty: "no time tracked " + where, Context: "time records " + where, } return a.writeTimes(cmd, page, opts.Page, spec, total) }} @@ -135,7 +135,7 @@ func (a *application) writeTimes(cmd *cobra.Command, page services.Page[services sum += item.Seconds rows[i] = spec.Row(item) } - text := terminal.Table(spec.Columns, rows, d) + "Total: " + spent(sum) + "\n" + text := terminal.Table(spec.Columns, rows, d) + "Total: " + spentFor(!d.TTY, sum) + "\n" if d.TTY { text = showing(len(page.Items), servicePage(page, number), spec.Context) + "\n\n" + text } @@ -149,8 +149,17 @@ func (a *application) writeTimes(cmd *cobra.Command, page services.Page[services return a.morePagesHint(cmd, page.More, number) } +// spentFor is spent on a terminal; when exact (piped output) it keeps every second as a Go +// duration such as 1h2m3s, which spent drops from durations of an hour or more. +func spentFor(exact bool, seconds int64) string { + if exact { + return trackedDuration(seconds).String() + } + return spent(seconds) +} + // trackedTimeRow shows a record in repo; records of other repositories name theirs. -func trackedTimeRow(repo string) func(services.TrackedTime) []terminal.Cell { +func trackedTimeRow(repo string, exact bool) func(services.TrackedTime) []terminal.Cell { return func(item services.TrackedTime) []terminal.Cell { issue := "#" + strconv.FormatInt(item.IssueNumber, 10) if !strings.EqualFold(item.Repository, repo) { @@ -160,7 +169,7 @@ func trackedTimeRow(repo string) func(services.TrackedTime) []terminal.Cell { {Text: strconv.FormatInt(item.ID, 10)}, {Text: strings.TrimSpace(issue + " " + item.IssueTitle)}, {Text: item.User}, - {Text: spent(item.Seconds)}, + {Text: spentFor(exact, item.Seconds)}, {Time: terminal.ParseTime(item.CreatedAt)}, } } diff --git a/internal/app/time_test.go b/internal/app/time_test.go index f9f233f7..e1e68f17 100644 --- a/internal/app/time_test.go +++ b/internal/app/time_test.go @@ -24,9 +24,9 @@ func TestTimeListShowsRepositoryAndDurations(t *testing.T) { defer server.Close() deps, out, _ := serverDeps(t, server.URL) require.NoError(t, Execute(context.Background(), deps, []string{"time", "list", "--mine", "--total"})) - require.Equal(t, "1\talice/project#3\talice\t1h 25m\t2026-10-01T10:00:00Z\n"+ - "2\tbob/tools#3\talice\t15m\t2026-10-02T10:00:00Z\n"+ - "Total: 1h 40m\n", out.String()) + require.Equal(t, "1\talice/project#3\talice\t1h25m0s\t2026-10-01T10:00:00Z\n"+ + "2\tbob/tools#3\talice\t15m0s\t2026-10-02T10:00:00Z\n"+ + "Total: 1h40m0s\n", out.String()) out.Reset() require.NoError(t, Execute(context.Background(), deps, []string{"time", "list", "--mine", "--json=repository,issue_number"})) require.JSONEq(t, `[{"repository":"alice/project","issue_number":3},{"repository":"bob/tools","issue_number":3}]`, out.String()) @@ -125,7 +125,7 @@ func TestTimeListTotalPipedCoversEmptyAndPagedListings(t *testing.T) { out.Reset() empty = false require.NoError(t, Execute(context.Background(), deps, []string{"time", "list", "--mine", "--total", "--limit", "1"})) - require.Equal(t, "1\talice/project#3\talice\t1m\t2026-10-01T10:00:00Z\nTotal: 1m\n", out.String()) + require.Equal(t, "1\talice/project#3\talice\t1m0s\t2026-10-01T10:00:00Z\nTotal: 1m0s\n", out.String()) require.Equal(t, "more results available; use --page 2\n", diagnostics.String()) } @@ -138,3 +138,14 @@ func TestCommentListHintsAtTheNextClientSidePage(t *testing.T) { require.NoError(t, Execute(context.Background(), deps, []string{"issue", "comment", "list", "3", "-R", "alice/project", "--limit", "1"})) require.Equal(t, "more results available; use --page 2\n", diagnostics.String()) } + +func TestPipedTimeKeepsExactSeconds(t *testing.T) { + server := newAuthenticatedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _, _ = w.Write([]byte(`[{"id":1,"time":3723,"created":"2026-10-01T10:00:00Z","user_name":"alice","issue":{"number":3,"repository":{"full_name":"alice/project"}}}]`)) + })) + defer server.Close() + deps, out, _ := serverDeps(t, server.URL) + require.NoError(t, Execute(context.Background(), deps, []string{"time", "list", "--mine", "--total"})) + require.Equal(t, "1\talice/project#3\talice\t1h2m3s\t2026-10-01T10:00:00Z\nTotal: 1h2m3s\n", out.String()) + require.Equal(t, "1h 2m", spent(3723)) +} diff --git a/internal/git/checkout.go b/internal/git/checkout.go index c2ba3718..d4242983 100644 --- a/internal/git/checkout.go +++ b/internal/git/checkout.go @@ -99,6 +99,11 @@ func CheckoutPull(ctx context.Context, runner Runner, options CheckoutOptions) ( } } } + if existing { + if err := requireBranchFreeElsewhere(run, options.Branch); err != nil { + return CheckoutResult{}, err + } + } fetchRemote := options.Remote fetchRef := fmt.Sprintf("refs/pull/%d/head", options.Number) tracking := "" @@ -168,3 +173,25 @@ func CheckoutPull(ctx context.Context, runner Runner, options CheckoutOptions) ( } return CheckoutResult{Branch: options.Branch, Commit: commit, Detached: options.Detached, Remote: fetchRemote, Tracking: tracking}, nil } + +// requireBranchFreeElsewhere fails when branch is checked out in a linked worktree other than +// the current one, which git switch would refuse with a message that does not say where. +func requireBranchFreeElsewhere(run func(...string) (string, error), branch string) error { + top, err := run("rev-parse", "--show-toplevel") + if err != nil { + return err + } + listing, err := run("worktree", "list", "--porcelain") + if err != nil { + return err + } + path := "" + for line := range strings.SplitSeq(listing, "\n") { + if value, ok := strings.CutPrefix(line, "worktree "); ok { + path = value + } else if line == "branch refs/heads/"+branch && path != top { + return fmt.Errorf("branch %q is checked out in the worktree %s; pass --detach to check out the pull request without a branch, or choose another branch with --branch", branch, path) + } + } + return nil +} diff --git a/internal/git/checkout_test.go b/internal/git/checkout_test.go index 54a32965..d4fea9ce 100644 --- a/internal/git/checkout_test.go +++ b/internal/git/checkout_test.go @@ -17,6 +17,7 @@ type checkoutRunner struct { dirty, collision bool localCommit string diverged bool + worktrees string } func (r *checkoutRunner) Output(_ context.Context, dir string, prompt bool, args ...string) (string, error) { @@ -38,6 +39,8 @@ func (r *checkoutRunner) Output(_ context.Context, dir string, prompt bool, args return "true", nil } return "abc123", nil + case "worktree": + return r.worktrees, nil case "status": if r.dirty { return "?? file", nil @@ -172,3 +175,19 @@ func TestPullCheckoutTracksHeadBranch(t *testing.T) { require.NotContains(t, call, "refs/pull/") } } + +func TestPullCheckoutRejectsBranchCheckedOutInAnotherWorktree(t *testing.T) { + options := CheckoutOptions{Directory: "/work", Remote: "origin", Number: 7, Branch: "pr/7", ExpectedCommit: "abc123", Update: true} + runner := &checkoutRunner{collision: true, localCommit: "abc123", worktrees: "worktree abc123\nHEAD 1\nbranch refs/heads/main\n\nworktree /other/tree\nHEAD 2\nbranch refs/heads/pr/7\n"} + _, err := CheckoutPull(context.Background(), runner, options) + require.ErrorContains(t, err, `branch "pr/7" is checked out in the worktree /other/tree`) + require.ErrorContains(t, err, "--detach") + for _, call := range runner.calls { + require.NotContains(t, call, "fetch") + require.NotContains(t, call, "switch") + } + // The current worktree holding the branch is fine. + runner = &checkoutRunner{collision: true, localCommit: "abc123", worktrees: "worktree abc123\nHEAD 1\nbranch refs/heads/pr/7\n"} + _, err = CheckoutPull(context.Background(), runner, options) + require.NoError(t, err) +}