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 <bircni@icloud.com>
This commit is contained in:
bircni committed 2026-10-18 20:38:01 +02:00
1 parent 6224190bd9
commit 339527b26c
11 files changed
+131 -23

No files matched your search

+1 -1
View File
@@ -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) == "" { if cmd.Flags().Changed("branch") && strings.TrimSpace(branch) == "" {
return clierr.Wrap(clierr.Usage, errors.New("--branch cannot be empty")) 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 { if err != nil {
return err return err
} }
+1 -2
View File
@@ -293,9 +293,8 @@ func TestReviewLifecycleEditorRejectsEmptyRequiredText(t *testing.T) {
for _, verb := range []string{"submit", "dismiss"} { for _, verb := range []string{"submit", "dismiss"} {
t.Run(verb, func(t *testing.T) { t.Run(verb, func(t *testing.T) {
server := newAuthenticatedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { server := newAuthenticatedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
require.Equal(t, "submit", verb)
require.Equal(t, http.MethodGet, r.Method) 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() defer server.Close()
deps, out, _ := testDeps(t) deps, out, _ := testDeps(t)
+11 -5
View File
@@ -4,6 +4,7 @@
package app package app
import ( import (
"cmp"
"context" "context"
"fmt" "fmt"
"net/http" "net/http"
@@ -97,11 +98,15 @@ func TestPullCommandsFindClosedPullsByBranch(t *testing.T) {
name string name string
args []string args []string
err 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"}, ""}, {"view", []string{"pr", "view", "alice:gone", "--json=number"}, "", ""},
{"checks", []string{"pr", "checks", "alice:gone", "--web", "--no-input"}, ""}, {"checks", []string{"pr", "checks", "alice:gone", "--web", "--no-input"}, "", ""},
{"reopen", []string{"pr", "reopen", "alice:gone"}, "already merged"}, {"reopen", []string{"pr", "reopen", "alice:gone"}, "already merged", ""},
{"clean", []string{"pr", "clean", "alice:gone", "--no-input"}, ""}, {"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) { t.Run(tc.name, func(t *testing.T) {
var requests []string var requests []string
@@ -116,7 +121,8 @@ func TestPullCommandsFindClosedPullsByBranch(t *testing.T) {
if err != nil { if err != nil {
require.NotContains(t, err.Error(), "no open pull request") 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)
}) })
} }
} }
+1 -1
View File
@@ -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 { 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)")) 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 { if err != nil {
return err return err
} }
+26 -4
View File
@@ -153,10 +153,9 @@ func (a *application) pullReviewDismissCommand(dismiss bool) *cobra.Command {
var draft terminal.Draft var draft terminal.Draft
defer finishDraft(&draft, &runErr) defer finishDraft(&draft, &runErr)
var err error var err error
if dismiss { // The reason is collected before any request when it comes from flags or cannot be
if err := validateBodySources(cmd); err != nil { // prompted for; an editor or prompt waits until the review is known to be dismissable.
return err collect := func() error {
}
switch { switch {
case useEditor: case useEditor:
draft, err = a.editDraft(cmd, "") draft, err = a.editDraft(cmd, "")
@@ -172,6 +171,24 @@ func (a *application) pullReviewDismissCommand(dismiss bool) *cobra.Command {
if strings.TrimSpace(body) == "" { if strings.TrimSpace(body) == "" {
return clierr.Wrap(clierr.Usage, errors.New("a dismissal needs a reason; pass --body, --body-file or --editor")) 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) t, n, id, err := a.pullAndID(cmd, args)
if err != nil { if err != nil {
@@ -191,6 +208,11 @@ func (a *application) pullReviewDismissCommand(dismiss bool) *cobra.Command {
a.warn(cmd, "%s is not dismissed", subject) a.warn(cmd, "%s is not dismissed", subject)
return a.jsonResult(cmd, current) return a.jsonResult(cmd, current)
} }
if dismiss && !collected {
if err := collect(); err != nil {
return err
}
}
if dismiss { if dismiss {
if err := a.confirmAction(cmd, terminal.Confirmation{Prompt: "Dismiss " + subject + "?"}); err != nil { if err := a.confirmAction(cmd, terminal.Confirmation{Prompt: "Dismiss " + subject + "?"}); err != nil {
return err return err
+15
View File
@@ -288,6 +288,21 @@ func TestReviewDismissChecksTheReviewStateFirst(t *testing.T) {
require.Zero(t, mutations) 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) { func TestReviewDeleteAndDismissRequireYesWithoutInput(t *testing.T) {
mutations := 0 mutations := 0
server := newAuthenticatedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { server := newAuthenticatedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+2 -2
View File
@@ -416,7 +416,7 @@ $ tea release edit v1.0.0-rc1 v1.0.0-rc2 --prerelease=false`,
return err return err
} }
if opts.Body != nil && *opts.Body == "" { 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 cmd.Flags().Changed("tag") {
if len(args) != 1 || strings.TrimSpace(fields.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 opts.Body = &editorDraft.Body
} }
if editorDraft.Body == "" && fields.editor { 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) { 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) item, err := service.View(cmd.Context(), tag)
+13 -4
View File
@@ -106,7 +106,7 @@ $ tea time list --mine --since 168h`, Args: argsExact(0), RunE: func(cmd *cobra.
} }
spec := listSpec[services.TrackedTime]{ spec := listSpec[services.TrackedTime]{
Columns: []terminal.Column{{Header: "ID"}, {Header: "Issue", Flex: true}, {Header: "User"}, {Header: "Spent"}, {Header: "When"}}, 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) 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 sum += item.Seconds
rows[i] = spec.Row(item) 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 { if d.TTY {
text = showing(len(page.Items), servicePage(page, number), spec.Context) + "\n\n" + text 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) 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. // 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 { return func(item services.TrackedTime) []terminal.Cell {
issue := "#" + strconv.FormatInt(item.IssueNumber, 10) issue := "#" + strconv.FormatInt(item.IssueNumber, 10)
if !strings.EqualFold(item.Repository, repo) { 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: strconv.FormatInt(item.ID, 10)},
{Text: strings.TrimSpace(issue + " " + item.IssueTitle)}, {Text: strings.TrimSpace(issue + " " + item.IssueTitle)},
{Text: item.User}, {Text: item.User},
{Text: spent(item.Seconds)}, {Text: spentFor(exact, item.Seconds)},
{Time: terminal.ParseTime(item.CreatedAt)}, {Time: terminal.ParseTime(item.CreatedAt)},
} }
} }
+15 -4
View File
@@ -24,9 +24,9 @@ func TestTimeListShowsRepositoryAndDurations(t *testing.T) {
defer server.Close() defer server.Close()
deps, out, _ := serverDeps(t, server.URL) deps, out, _ := serverDeps(t, server.URL)
require.NoError(t, Execute(context.Background(), deps, []string{"time", "list", "--mine", "--total"})) 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"+ require.Equal(t, "1\talice/project#3\talice\t1h25m0s\t2026-10-01T10:00:00Z\n"+
"2\tbob/tools#3\talice\t15m\t2026-10-02T10:00:00Z\n"+ "2\tbob/tools#3\talice\t15m0s\t2026-10-02T10:00:00Z\n"+
"Total: 1h 40m\n", out.String()) "Total: 1h40m0s\n", out.String())
out.Reset() out.Reset()
require.NoError(t, Execute(context.Background(), deps, []string{"time", "list", "--mine", "--json=repository,issue_number"})) 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()) 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() out.Reset()
empty = false empty = false
require.NoError(t, Execute(context.Background(), deps, []string{"time", "list", "--mine", "--total", "--limit", "1"})) 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()) 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.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()) 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))
}
+27
View File
@@ -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 fetchRemote := options.Remote
fetchRef := fmt.Sprintf("refs/pull/%d/head", options.Number) fetchRef := fmt.Sprintf("refs/pull/%d/head", options.Number)
tracking := "" 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 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
}
+19
View File
@@ -17,6 +17,7 @@ type checkoutRunner struct {
dirty, collision bool dirty, collision bool
localCommit string localCommit string
diverged bool diverged bool
worktrees string
} }
func (r *checkoutRunner) Output(_ context.Context, dir string, prompt bool, args ...string) (string, error) { 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 "true", nil
} }
return "abc123", nil return "abc123", nil
case "worktree":
return r.worktrees, nil
case "status": case "status":
if r.dirty { if r.dirty {
return "?? file", nil return "?? file", nil
@@ -172,3 +175,19 @@ func TestPullCheckoutTracksHeadBranch(t *testing.T) {
require.NotContains(t, call, "refs/pull/") 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)
}