diff --git a/internal/app/app_test.go b/internal/app/app_test.go index 79918dd7..5dda3c84 100644 --- a/internal/app/app_test.go +++ b/internal/app/app_test.go @@ -456,19 +456,11 @@ const ( pr attachment list pr attachment upload pr attachment view - pr checkout - pr clean - pr close pr comment create pr comment delete pr comment edit pr comment list pr comment view - pr create - pr diff - pr edit - pr merge - pr reopen pr status search issue search pr diff --git a/internal/app/args_test.go b/internal/app/args_test.go index 003bf0ff..b9e53ea0 100644 --- a/internal/app/args_test.go +++ b/internal/app/args_test.go @@ -88,7 +88,7 @@ func TestArgumentErrorsShowTheUsageLine(t *testing.T) { {[]string{"issue", "view"}, "missing issue number\nUsage: tea issue view [flags]\nRun 'tea issue view --help' for more information."}, {[]string{"issue", "view", "1", "2"}, "too many arguments\nUsage: tea issue view [flags]"}, {[]string{"issue", "list", "extra"}, "unexpected argument \"extra\"\nUsage: tea issue list [flags]"}, - {[]string{"pr", "checkout"}, "missing pull request number\nUsage: tea pr checkout [flags]"}, + {[]string{"pr", "checkout"}, "missing pull request number\nUsage: tea pr checkout { | | } [flags]"}, {[]string{"run", "view"}, "missing run ID\nUsage: tea run view [] [flags]"}, {[]string{"release", "asset", "edit", "v1"}, "missing asset\nUsage: tea release asset edit [flags]"}, {[]string{"repo", "clone"}, "missing repository\nUsage: tea repo clone [] [-- ...] [flags]"}, diff --git a/internal/app/checkout.go b/internal/app/checkout.go index 5cd0e496..535a27de 100644 --- a/internal/app/checkout.go +++ b/internal/app/checkout.go @@ -75,21 +75,19 @@ var errNoMatchingRemote = errors.New("no Git remote matches the selected reposit func (a *application) pullCheckoutCommand() *cobra.Command { var branch string var detached, update bool - cmd := &cobra.Command{Use: "checkout ", Short: "Check out a pull request in a matching branch or detached HEAD", Args: argsExact(1), RunE: func(cmd *cobra.Command, args []string) error { - n, err := number(args[0]) - if err != nil { - return err - } + cmd := &cobra.Command{Use: "checkout { | | }", Aliases: []string{"co"}, Short: "Check out a pull request", Long: `Check out the head of a pull request in a local branch. + +The branch takes the name of the pull request's head branch when that name is free or +already points at the head; otherwise it is OWNER/BRANCH for a fork, or pr/NUMBER. tea +records the pull request in the branch's Git configuration, so that pr commands without an +argument select it, and the branch tracks the head branch, so that git push updates it.`, Annotations: pullArgumentsAnnotation(nil), Args: argsExact(1), RunE: func(cmd *cobra.Command, args []string) error { if detached && (cmd.Flags().Changed("branch") || update) { return clierr.Wrap(clierr.Usage, errors.New("--detach and --branch/--update are mutually exclusive")) } if cmd.Flags().Changed("branch") && strings.TrimSpace(branch) == "" { return clierr.Wrap(clierr.Usage, errors.New("--branch cannot be empty")) } - if branch == "" && !detached { - branch = fmt.Sprintf("pr/%d", n) - } - target, err := a.resolve(cmd.Context(), true) + target, n, err := a.pullSelector(cmd, args) if err != nil { return err } @@ -108,6 +106,9 @@ func (a *application) pullCheckoutCommand() *cobra.Command { if pull.HeadSHA == "" { return errors.New("Gitea did not return the PR head commit") } + if branch == "" && !detached { + branch = a.checkoutBranchName(cmd.Context(), target, pull) + } rawRemote, err := a.git(cmd.Context(), "remote", "get-url", remote) if err != nil { return err @@ -146,22 +147,44 @@ func (a *application) pullCheckoutCommand() *cobra.Command { return a.writeJSON(cmd, result) } if result.Detached { - _, err := fmt.Fprintf(cmd.OutOrStdout(), "Checked out %s in detached HEAD\n", result.Commit) - return err + a.done(cmd, "Checked out #%d at %s (detached HEAD)", n, shortSHA(result.Commit)) + } else { + a.done(cmd, "Checked out #%d into %s", n, result.Branch) } - if result.Tracking != "" { - _, err = fmt.Fprintf(cmd.OutOrStdout(), "Checked out %s (%s) tracking %s\n", result.Branch, result.Commit, result.Tracking) - return err - } - _, err = fmt.Fprintf(cmd.OutOrStdout(), "Checked out %s (%s)\n", result.Branch, result.Commit) - return err + return nil }} cmd.Flags().BoolVar(&update, "update", false, "Fast-forward an existing branch to the PR head; reject divergence") cmd.Flags().BoolVar(&detached, "detach", false, "Check out the PR commit without creating or changing a branch") - cmd.Flags().StringVarP(&branch, "branch", "b", "", "Local branch name (default: pr/NUMBER; existing branch must match PR head)") + cmd.Flags().StringVarP(&branch, "branch", "b", "", "Local branch name (default: the head branch name when free, else OWNER/BRANCH or pr/NUMBER)") + cmd.ValidArgsFunction = a.completePulls("open") + cmd.Example = `$ tea pr checkout 13 +$ tea pr co feature/login +$ tea pr checkout 13 --branch review-13 +$ tea pr checkout 13 --update` return cmd } +// checkoutBranchName names the local branch for a pull request (D13): its head branch name when +// that is free or already at the head, else OWNER/BRANCH for a fork, else pr/NUMBER. A branch that +// an earlier checkout of the same pull request created is reused, so that --update can advance it. +func (a *application) checkoutBranchName(ctx context.Context, t target, pull services.Pull) string { + fallback := fmt.Sprintf("pr/%d", pull.Number) + if pull.HeadRemoteDeleted || pull.Head == "" { + return fallback + } + candidates := []string{pull.Head} + if owner, _, _ := strings.Cut(pull.HeadRepo, "/"); owner != "" && !strings.EqualFold(pull.HeadRepo, t.Repo) { + candidates = append(candidates, owner+"/"+pull.Head) + } + for _, name := range candidates { + tip, err := a.gitProbe().Output(ctx, a.options.cwd, false, "rev-parse", "--verify", "refs/heads/"+name+"^{commit}") + if err != nil || tip == pull.HeadSHA || git.ReadBranchConfig(ctx, a.gitProbe(), a.options.cwd, name).PullNumber == pull.Number { + return name + } + } + return fallback +} + // ensureHeadRemote returns the remote holding the PR head, adding one when none matches. // An empty name means the head repository has no usable clone URL and the base remote's PR ref should be used. func (a *application) ensureHeadRemote(ctx context.Context, target target, pull services.Pull) (name, remoteURL string, undo func(), err error) { diff --git a/internal/app/checkout_test.go b/internal/app/checkout_test.go index 3bc1ec54..ef4fd2c8 100644 --- a/internal/app/checkout_test.go +++ b/internal/app/checkout_test.go @@ -11,6 +11,7 @@ import ( "testing" "gitea.dev/tea/internal/git" + "gitea.dev/tea/internal/services" "gitea.dev/tea/internal/settings" "github.com/stretchr/testify/require" ) @@ -66,15 +67,14 @@ func TestCheckoutMatchesRemoteThroughEnvironmentSSHHost(t *testing.T) { deps.Getenv = serverEnv(server.URL, map[string]string{"GITEA_INSTANCE_SSH_HOST": "ssh.example.org"}) deps.Git = &fakeGit{results: map[string]string{ "remote": "origin", "remote get-url origin": "git@ssh.example.org:alice/project.git", - "rev-parse --is-inside-work-tree": "true", "check-ref-format --branch pr/7": "", - "check-ref-format --branch feature": "", + "rev-parse --is-inside-work-tree": "true", "check-ref-format --branch feature": "", "status --porcelain=v1 --untracked-files=normal": "", "for-each-ref --format=%(refname) refs/heads/": "", - "fetch --no-tags -- origin refs/heads/feature": "", "rev-parse --verify FETCH_HEAD^{commit}": "abc123", "switch --create pr/7 abc123": "", - "config branch.pr/7.remote origin": "", "config branch.pr/7.merge refs/heads/feature": "", - "config branch.pr/7.tea-pr 7": "", "config branch.pr/7.tea-base-remote origin": "", + "fetch --quiet --no-tags -- origin refs/heads/feature": "", "rev-parse --verify FETCH_HEAD^{commit}": "abc123", "switch --quiet --create feature abc123": "", + "config branch.feature.remote origin": "", "config branch.feature.merge refs/heads/feature": "", + "config branch.feature.tea-pr 7": "", "config branch.feature.tea-base-remote origin": "", }} require.NoError(t, Execute(context.Background(), deps, []string{"pr", "checkout", "7", "--repo", "alice/project", "--remote", "origin", "--no-input", "--json"})) - require.JSONEq(t, `{"branch":"pr/7","commit":"abc123","detached":false,"remote":"origin","tracking":"origin/feature"}`, out.String()) + require.JSONEq(t, `{"branch":"feature","commit":"abc123","detached":false,"remote":"origin","tracking":"origin/feature"}`, out.String()) } func TestSSHCheckoutUsesAccountKeyOnlyForFetch(t *testing.T) { @@ -90,21 +90,20 @@ func TestSSHCheckoutUsesAccountKeyOnlyForFetch(t *testing.T) { }}) sshArgs, err := git.SSHArguments("/keys/alice key", false) require.NoError(t, err) - fetch := strings.Join(append(sshArgs, "fetch", "--no-tags", "--", "origin", "refs/heads/feature"), " ") + fetch := strings.Join(append(sshArgs, "fetch", "--quiet", "--no-tags", "--", "origin", "refs/heads/feature"), " ") runner := &fakeGit{results: map[string]string{ "remote": "origin", "remote get-url origin": "ssh://git@git.example.org:2222/alice/project.git", - "rev-parse --is-inside-work-tree": "true", "check-ref-format --branch pr/7": "", - "check-ref-format --branch feature": "", + "rev-parse --is-inside-work-tree": "true", "check-ref-format --branch feature": "", "status --porcelain=v1 --untracked-files=normal": "", "for-each-ref --format=%(refname) refs/heads/": "", - fetch: "", "rev-parse --verify FETCH_HEAD^{commit}": "abc123", "switch --create pr/7 abc123": "", - "config branch.pr/7.remote origin": "", "config branch.pr/7.merge refs/heads/feature": "", - "config branch.pr/7.tea-pr 7": "", "config branch.pr/7.tea-base-remote origin": "", + fetch: "", "rev-parse --verify FETCH_HEAD^{commit}": "abc123", "switch --quiet --create feature abc123": "", + "config branch.feature.remote origin": "", "config branch.feature.merge refs/heads/feature": "", + "config branch.feature.tea-pr 7": "", "config branch.feature.tea-base-remote origin": "", }} deps.Git = runner require.NoError(t, Execute(context.Background(), deps, []string{"pr", "checkout", "7", "--repo", "alice/project", "--remote", "origin", "--no-input", "--json"})) require.Contains(t, runner.calls, fetch) - require.Contains(t, runner.calls, "config branch.pr/7.remote origin") - require.Contains(t, runner.calls, "config branch.pr/7.merge refs/heads/feature") + require.Contains(t, runner.calls, "config branch.feature.remote origin") + require.Contains(t, runner.calls, "config branch.feature.merge refs/heads/feature") configured := 0 for _, call := range runner.calls { if strings.HasPrefix(call, "-c ") { @@ -114,5 +113,31 @@ func TestSSHCheckoutUsesAccountKeyOnlyForFetch(t *testing.T) { } require.Equal(t, 1, configured) require.False(t, runner.prompt) - require.JSONEq(t, `{"branch":"pr/7","commit":"abc123","detached":false,"remote":"origin","tracking":"origin/feature"}`, out.String()) + require.JSONEq(t, `{"branch":"feature","commit":"abc123","detached":false,"remote":"origin","tracking":"origin/feature"}`, out.String()) +} + +func TestCheckoutNamesTheBranchAfterTheHead(t *testing.T) { + same := services.Pull{Number: 7, Head: "feature", HeadSHA: "abc123", HeadRepo: "alice/project"} + fork := services.Pull{Number: 7, Head: "feature", HeadSHA: "abc123", HeadRepo: "bob/project"} + for _, tc := range []struct { + name string + pull services.Pull + branches map[string]string + want string + }{ + {"free head name", same, nil, "feature"}, + {"head name at the head", same, map[string]string{"rev-parse --verify refs/heads/feature^{commit}": "abc123"}, "feature"}, + {"head name of an earlier checkout", same, map[string]string{"rev-parse --verify refs/heads/feature^{commit}": "old", "config --get branch.feature.tea-pr": "7"}, "feature"}, + {"head name taken", same, map[string]string{"rev-parse --verify refs/heads/feature^{commit}": "other"}, "pr/7"}, + {"fork head name taken", fork, map[string]string{"rev-parse --verify refs/heads/feature^{commit}": "other"}, "bob/feature"}, + {"fork names taken", fork, map[string]string{"rev-parse --verify refs/heads/feature^{commit}": "other", "rev-parse --verify refs/heads/bob/feature^{commit}": "other"}, "pr/7"}, + {"deleted head", services.Pull{Number: 7, Head: "feature", HeadRemoteDeleted: true}, nil, "pr/7"}, + } { + t.Run(tc.name, func(t *testing.T) { + deps, _, _ := testDeps(t) + deps.Git = &fakeGit{results: tc.branches} + a := &application{deps: deps} + require.Equal(t, tc.want, a.checkoutBranchName(context.Background(), target{Repo: "alice/project"}, tc.pull)) + }) + } } diff --git a/internal/app/errors.go b/internal/app/errors.go index aac0f4c0..51cbca9b 100644 --- a/internal/app/errors.go +++ b/internal/app/errors.go @@ -177,7 +177,7 @@ func conflictHint(ctx context.Context, apiErr *transport.APIError, conflict *ser } case services.ReasonDraft: if number != conflict.Key { - return "mark it ready with: tea pr edit " + number + " --ready" + return "mark it ready with: tea pr ready " + number } case services.ReasonOwnReview: return "leave a comment instead" diff --git a/internal/app/errors_test.go b/internal/app/errors_test.go index 09eea68d..e5f72b84 100644 --- a/internal/app/errors_test.go +++ b/internal/app/errors_test.go @@ -47,8 +47,8 @@ func TestExplainNamesResourcesAndTellsWhyRepositoriesFail(t *testing.T) { "POST /api/v1/repos/alice/old/issues": {"423", `{"message":"repo is archived"}`}, "POST /api/v1/repos/alice/project/pulls": {"409", `{"message":"pull request already exists for these targets [id: 16, issue_id: 3, head_repo_id: 1, base_repo_id: 1, head_branch: feat, base_branch: main]"}`}, "GET /api/v1/repos/alice/project/pulls/main/feat": {"200", `{"number":3,"html_url":"https://example.org/alice/project/pulls/3"}`}, - "GET /api/v1/repos/alice/project/pulls/3": {"200", `{"number":3,"state":"open","draft":true,"head":{"sha":"abc"}}`}, - "POST /api/v1/repos/alice/project/pulls/3/merge": {"405", `{"message":"Work in progress PRs cannot be merged"}`}, + "GET /api/v1/repos/alice/project/pulls/3": {"200", `{"number":3,"state":"open","mergeable":false,"head":{"sha":"abc"}}`}, + "POST /api/v1/repos/alice/project/pulls/3/merge": {"405", `{"message":"Please try again later"}`}, }) for _, tc := range []struct { name string @@ -63,7 +63,7 @@ func TestExplainNamesResourcesAndTellsWhyRepositoriesFail(t *testing.T) { {"empty repository", []string{"pr", "list", "-R", "alice/empty"}, "alice/empty is empty; it has no pull requests"}, {"archived repository", []string{"issue", "create", "-R", "alice/old", "--title", "x", "--body", "y"}, "alice/old is archived (read-only); unarchive it with: tea repo unarchive alice/old"}, {"existing pull request", []string{"pr", "create", "-R", "alice/project", "--head", "feat", "--base", "main", "--title", "x", "--body", "y"}, "pull request for feat into main already exists in alice/project; see https://example.org/alice/project/pulls/3"}, - {"context added by the command", []string{"pr", "merge", "3", "-R", "alice/project", "--yes"}, "pull request #3 is a draft; mark it ready for review first: Gitea API returned HTTP 405 for POST /api/v1/repos/alice/project/pulls/3/merge: Work in progress PRs cannot be merged"}, + {"context added by the command", []string{"pr", "merge", "3", "-R", "alice/project", "--yes"}, "pull request #3 has conflicting files or is otherwise not mergeable: Gitea API returned HTTP 405 for POST /api/v1/repos/alice/project/pulls/3/merge: Please try again later"}, {"raw api response", []string{"api", "users/nobody"}, "not found (HTTP 404 GET /api/v1/users/nobody)"}, } { t.Run(tc.name, func(t *testing.T) { @@ -85,7 +85,7 @@ func TestExplainAddsTheNextStepToRefusals(t *testing.T) { }{ {"PATCH", "/api/v1/repos/alice/x/pulls/3", "cannot change state of this pull request, it was already merged", 412, "pull request #3 is already merged; its state can no longer change"}, {"POST", "/api/v1/repos/alice/x/pulls/3/merge", "The PR is already merged", 405, "pull request #3 is already merged"}, - {"POST", "/api/v1/repos/alice/x/pulls/5/merge", "Work in progress PRs cannot be merged", 405, "pull request #5 is a draft; mark it ready with: tea pr edit 5 --ready"}, + {"POST", "/api/v1/repos/alice/x/pulls/5/merge", "Work in progress PRs cannot be merged", 405, "pull request #5 is a draft; mark it ready with: tea pr ready 5"}, {"POST", "/api/v1/repos/alice/x/pulls/1/reviews", "approve your own pull is not allowed", 422, "you can't approve or request changes on your own pull request; leave a comment instead"}, {"POST", "/api/v1/repos/alice/x/pulls", "Invalid PullRequest: There are no changes between the head and the base", 422, "no commits between the base and head branches; check out your feature branch or pass --head"}, {"POST", "/api/v1/repos/alice/x/actions/workflows/nightly.yaml/dispatches", "workflow is disabled", 403, "workflow nightly.yaml is disabled; enable it with: tea workflow enable nightly.yaml"}, diff --git a/internal/app/human_output.go b/internal/app/human_output.go index a9f46909..6b53d954 100644 --- a/internal/app/human_output.go +++ b/internal/app/human_output.go @@ -347,3 +347,8 @@ func yesNo(value bool) string { } return "no" } + +// shortSHA abbreviates a commit ID to seven characters, as Gitea and Git show them. +func shortSHA(sha string) string { + return sha[:min(len(sha), 7)] +} diff --git a/internal/app/pull_clean.go b/internal/app/pull_clean.go index 1f35ea4e..1c7c8259 100644 --- a/internal/app/pull_clean.go +++ b/internal/app/pull_clean.go @@ -13,17 +13,14 @@ import ( "gitea.dev/tea/internal/git" "gitea.dev/tea/internal/services" "gitea.dev/tea/internal/settings" + "gitea.dev/tea/internal/terminal" "github.com/spf13/cobra" ) func (a *application) pullCleanCommand() *cobra.Command { var ignoreSHA bool - cmd := &cobra.Command{Use: "clean ", Short: "Delete local and remote feature branches for a closed pull request", Args: argsExact(1), RunE: func(cmd *cobra.Command, args []string) error { - n, err := number(args[0]) - if err != nil { - return err - } - target, err := a.resolve(cmd.Context(), true) + cmd := &cobra.Command{Use: "clean [ | | ]", Short: "Delete the branches of a merged or closed pull request", Annotations: pullArgumentsAnnotation(map[string]string{"tea.confirm": "true"}), Args: usageArgs(cobra.MaximumNArgs(1)), RunE: func(cmd *cobra.Command, args []string) error { + target, n, err := a.pullSelector(cmd, args) if err != nil { return err } @@ -87,14 +84,15 @@ func (a *application) pullCleanCommand() *cobra.Command { return err } } - description := fmt.Sprintf("Delete local branch %q for pull request %s#%d", localBranch, target.Repo, n) - if ignoreSHA { - description += " (matched by name; local commits not in the pull request are lost)" - } + description := fmt.Sprintf("Delete local branch %s of %s", localBranch, refLabel(target.Repo, n, pull.Title)) if deleteRemote { - description += fmt.Sprintf(" and remote branch %q", remote+"/"+pull.Head) + description += fmt.Sprintf(" and remote branch %s/%s", remote, pull.Head) } - if err := a.confirm(cmd, description); err != nil { + prompt := description + "?" + if ignoreSHA { + prompt += " Local commits that are not in the pull request are lost." + } + if err := a.confirmAction(cmd, terminal.Confirmation{Prompt: prompt, NonTTY: strings.ToLower(description[:1]) + description[1:]}); err != nil { return err } result, err := git.CleanPull(cmd.Context(), a.deps.Git, git.CleanOptions{ @@ -108,21 +106,21 @@ func (a *application) pullCleanCommand() *cobra.Command { if a.options.json != "" { return a.writeJSON(cmd, result) } - _, err = fmt.Fprintf(cmd.OutOrStdout(), "Deleted local branch %s\n", result.LocalBranch) - if err != nil { - return err - } - if result.RemoteDeleted { - _, err = fmt.Fprintf(cmd.OutOrStdout(), "Deleted remote branch %s/%s\n", result.Remote, result.RemoteBranch) - return err - } - if result.RemoteSkipped { - _, err = fmt.Fprintf(cmd.ErrOrStderr(), "%s\n", result.RemoteSkipReason) - return err + a.done(cmd, "Deleted local branch %s", result.LocalBranch) + switch { + case result.RemoteDeleted: + a.done(cmd, "Deleted remote branch %s/%s", result.Remote, result.RemoteBranch) + case pull.HeadRemoteDeleted: + a.warn(cmd, "Remote branch %s was already deleted", pull.Head) + case !pull.HeadCanPush: + a.warn(cmd, "Kept remote branch %s: you cannot push to %s", pull.Head, pull.HeadRepo) } return nil }} cmd.Flags().BoolVar(&ignoreSHA, "ignore-sha", false, "Find the local branch by name instead of commit hash") + cmd.ValidArgsFunction = a.completePulls("closed") + cmd.Example = `# After #13 was merged, delete its branch locally and on Gitea +$ tea pr clean 13` return cmd } diff --git a/internal/app/pull_clean_test.go b/internal/app/pull_clean_test.go index 997e1046..39adef9c 100644 --- a/internal/app/pull_clean_test.go +++ b/internal/app/pull_clean_test.go @@ -64,7 +64,7 @@ func TestPullCleanDeletesLocalAndRemoteBranches(t *testing.T) { "rev-parse --verify refs/heads/pr/7^{commit}": "abc123", "rev-parse --verify refs/heads/main^{commit}": "mainsha", "symbolic-ref --short HEAD": "pr/7", - "switch -- main": "", + "switch --quiet -- main": "", "branch -D -- pr/7": "", }} deps.Git = runner @@ -92,7 +92,7 @@ func TestPullCleanDeletesLocalAndRemoteBranches(t *testing.T) { runner.calls = nil require.NoError(t, Execute(context.Background(), deps, []string{"pr", "clean", "7", "--repo", "alice/project", "--no-input", "--json", "--yes"})) require.Contains(t, runner.calls, "branch -D -- pr/7") - require.Contains(t, runner.calls, "switch -- main") + require.Contains(t, runner.calls, "switch --quiet -- main") require.Contains(t, runner.calls, deleteRemote) require.Contains(t, out.String(), `"local_branch":"pr/7"`) require.Contains(t, out.String(), `"remote_deleted":true`) diff --git a/internal/app/pull_create.go b/internal/app/pull_create.go index 0a1bd33b..19218141 100644 --- a/internal/app/pull_create.go +++ b/internal/app/pull_create.go @@ -8,6 +8,7 @@ import ( "errors" "fmt" "path/filepath" + "strconv" "strings" "gitea.dev/tea/internal/clierr" @@ -20,10 +21,15 @@ import ( func (a *application) pullCreateCommand() *cobra.Command { var options services.PullOptions - var bodyFile, deadline, milestoneName, topic string + var bodyFile, deadline, milestone, topic string var labelNames, reviewers, reviewerTeams, attachments []string - var useEditor, draft, agit, allowMaintainerEdits bool - create := &cobra.Command{Use: "create", Short: "Create a pull request", Args: argsExact(0), RunE: func(cmd *cobra.Command, _ []string) (runErr error) { + var useEditor, draft, agit, allowMaintainerEdits, fill, web bool + create := &cobra.Command{Use: "create", Aliases: []string{"new"}, Short: "Create a pull request", Long: `Create a pull request from the current branch, or from --head. + +On a terminal without flags, tea asks for the title and body, then what to do next: submit, +submit as a draft, add metadata or change the branches. --fill skips the questions and uses +the commit message of a single-commit branch, or the branch name, as the title. A branch that +is not on the remote yet is pushed first, with its upstream set.`, Args: argsExact(0), RunE: func(cmd *cobra.Command, _ []string) (runErr error) { var editorDraft terminal.Draft defer finishDraft(&editorDraft, &runErr) if err := validateBodySources(cmd); err != nil { @@ -33,7 +39,7 @@ func (a *application) pullCreateCommand() *cobra.Command { if a.options.json != "" { return clierr.Wrap(clierr.Usage, errors.New("--json/--jq cannot be combined with --agit; AGit creates the pull request through git push")) } - for _, name := range []string{"assignee", "label", "milestone", "milestone-name", "deadline", "reviewer", "reviewer-team", "attachment"} { + for _, name := range []string{"assignee", "label", "milestone", "deadline", "reviewer", "reviewer-team", "attachment"} { if cmd.Flags().Changed(name) { return clierr.Wrap(clierr.Usage, fmt.Errorf("--%s cannot be combined with --agit; AGit pushes carry only a title and body", name)) } @@ -41,8 +47,8 @@ func (a *application) pullCreateCommand() *cobra.Command { } else if topic != "" { return clierr.Wrap(clierr.Usage, errors.New("--topic requires --agit")) } - if err := validateMilestoneFlags(cmd, milestoneName, options.Milestone); err != nil { - return err + if id, err := strconv.ParseInt(milestone, 10, 64); err == nil && id < 0 { + return clierr.Wrap(clierr.Usage, errors.New("--milestone must be a milestone title or a positive ID")) } if err := requireNames("label", "", labelNames); err != nil { return err @@ -83,11 +89,13 @@ func (a *application) pullCreateCommand() *cobra.Command { if err != nil { return err } - if cmd.Flags().Changed("milestone-name") { - if options.Milestone, err = (services.Milestones{Client: t.client, Repository: t.Repo}).ResolveName(cmd.Context(), milestoneName); err != nil { + if milestone != "" && milestone != "0" { + if options.Milestone, err = milestoneFilter(cmd.Context(), t, milestone); err != nil { return err } } + t.expandMeAll(options.Assignees) + t.expandMeAll(reviewers) if options.Head == "" { owner, branch := a.defaultPullHead(cmd.Context()) if branch == "" { @@ -107,6 +115,9 @@ func (a *application) pullCreateCommand() *cobra.Command { return clierr.Wrap(clierr.Usage, errors.New("supply --base; repository has no default branch")) } } + if web { + return a.openBrowser(cmd, repositoryURL(t)+"/compare/"+options.Base+"..."+options.Head) + } var commitBody string if options.Title == "" { if subject, body, ok := a.singleCommitMessage(cmd.Context(), options.Base, options.Head); ok { @@ -167,7 +178,7 @@ func (a *application) pullCreateCommand() *cobra.Command { failures = append(failures, fmt.Errorf("upload %s: %w", file.Name(), err)) } } - if err := writeRecord(a, cmd, item, pullRecord); err != nil { + if err := a.reportCreatedPull(cmd, t, item); err != nil { return err } if len(failures) > 0 { @@ -175,26 +186,42 @@ func (a *application) pullCreateCommand() *cobra.Command { } return nil }} - create.Flags().StringSliceVar(&options.Assignees, "assignee", nil, "Assign users by username") - create.Flags().StringSliceVar(&labelNames, "label", nil, "Apply labels by exact name; repo:/org: disambiguate") - create.Flags().Int64Var(&options.Milestone, "milestone", 0, "Milestone ID") - create.Flags().StringVar(&milestoneName, "milestone-name", "", "Milestone by exact title; conflicts with --milestone") - create.Flags().StringVar(&options.Title, "title", "", "Pull request title (default: derived from the head branch)") + create.Flags().StringSliceVarP(&options.Assignees, "assignee", "a", nil, "Assign users by username; @me for yourself") + create.Flags().StringSliceVarP(&labelNames, "label", "l", nil, "Apply labels by name; repo: or org: picks between same-named labels") + create.Flags().StringVarP(&milestone, "milestone", "m", "", "Milestone title or ID") + flagAliases(create, map[string]string{"milestone-name": "milestone"}) + create.Flags().StringVarP(&options.Title, "title", "t", "", "Pull request title (default: the commit message of a single-commit branch, or the branch name)") create.Flags().StringVar(&deadline, "deadline", "", "Due date: YYYY-MM-DD, RFC3339, or Unix timestamp") - create.Flags().StringVar(&options.Body, "body", "", "Pull request body") - create.Flags().StringVar(&bodyFile, "body-file", "", "Read body from a file, or - for stdin") - create.Flags().BoolVar(&useEditor, "editor", false, "Compose the pull request body in the configured editor") - create.Flags().StringVar(&options.Head, "head", "", "Head branch, optionally user:branch (default: current branch)") - create.Flags().StringVar(&options.Base, "base", "", "Base branch (default: repository default branch)") - create.Flags().BoolVar(&draft, "draft", false, "Prefix title with WIP for Gitea draft behavior") + create.Flags().StringVarP(&options.Body, "body", "b", "", "Pull request body") + create.Flags().StringVarP(&bodyFile, "body-file", "F", "", "Read the body from `file`, or - for stdin") + create.Flags().BoolVarP(&useEditor, "editor", "e", false, "Write the body in your editor, starting from the repository's template") + create.Flags().StringVarP(&options.Head, "head", "H", "", "Head branch, or OWNER:BRANCH for a fork (default: the current branch)") + create.Flags().StringVarP(&options.Base, "base", "B", "", "Base branch (default: the repository's default branch)") + create.Flags().BoolVarP(&draft, "draft", "d", false, "Create a draft pull request") + create.Flags().BoolVarP(&fill, "fill", "f", false, "Skip the questions and take the title and body from the commits") + create.Flags().BoolVarP(&web, "web", "w", false, "Open the comparison page in the browser to create the pull request there") create.Flags().BoolVar(&allowMaintainerEdits, "allow-maintainer-edits", true, "Allow maintainers to push to the head branch") create.Flags().BoolVar(&allowMaintainerEdits, "edits", true, "Alias of --allow-maintainer-edits") _ = create.Flags().MarkHidden("edits") create.Flags().BoolVar(&agit, "agit", false, "Create the pull request through AGit push (refs/for)") - create.Flags().StringSliceVar(&reviewers, "reviewer", nil, "Request reviews from users by username") + create.Flags().StringSliceVarP(&reviewers, "reviewer", "r", nil, "Request reviews from users; @me for yourself") create.Flags().StringSliceVar(&reviewerTeams, "reviewer-team", nil, "Request reviews from teams within the repository's organization") create.Flags().StringArrayVar(&attachments, "attachment", nil, "Attach a file to the pull request (repeatable)") create.Flags().StringVar(&topic, "topic", "", "AGit topic name (default: head branch); requires --agit") + _ = create.RegisterFlagCompletionFunc("assignee", a.completeUsers()) + _ = create.RegisterFlagCompletionFunc("reviewer", a.completeUsers()) + _ = create.RegisterFlagCompletionFunc("label", a.completeLabels()) + _ = create.RegisterFlagCompletionFunc("milestone", a.completeMilestones()) + _ = create.RegisterFlagCompletionFunc("base", a.completeBranches()) + _ = create.RegisterFlagCompletionFunc("head", a.completeBranches()) + create.Example = `# Create a pull request for the current branch interactively +$ tea pr create + +# Take the title and body from the commits, without questions +$ tea pr create --fill + +$ tea pr create -t "Add login" -b "Closes #1" -B develop -r carol -l enhancement +$ tea pr create --draft --fill` return create } @@ -304,7 +331,7 @@ func (a *application) createPullInteractively(cmd *cobra.Command, snapshot setti body = template } choices := terminal.PullFormChoices{ - BaseOwner: baseOwner, Base: repo.DefaultBranch, HeadOwner: headOwner, HeadBranch: headBranch, Title: title, Body: body, + Repo: target.Repo, BaseOwner: baseOwner, Base: repo.DefaultBranch, HeadOwner: headOwner, HeadBranch: headBranch, Title: title, Body: body, AllowMaintainerEdits: true, CanPush: selectables.CanPush, Assignees: selectables.Assignees, EditBody: a.bodyEditor(cmd), } @@ -320,8 +347,8 @@ func (a *application) createPullInteractively(cmd *cobra.Command, snapshot setti if values.Agit && a.options.json != "" { return clierr.Wrap(clierr.Usage, errors.New("--json/--jq cannot be combined with AGit create; AGit creates the pull request through git push")) } - if err := a.confirm(cmd, "Create pull request on "+target.Repo+": "+values.Title); err != nil { - return err + if values.AsDraft { + values.Title = services.AddDraftPrefix(values.Title) } if values.Agit { return a.pushAgit(cmd, target, snapshot, git.AgitOptions{Remote: a.options.remote, Head: values.Head, Base: values.Base, Topic: values.Topic, Title: values.Title, Body: values.Body}) @@ -338,5 +365,14 @@ func (a *application) createPullInteractively(cmd *cobra.Command, snapshot setti if err != nil { return err } - return writeRecord(a, cmd, item, pullRecord) + return a.reportCreatedPull(cmd, target, item) +} + +// reportCreatedPull reports a new pull request and prints its URL, or the pull request as JSON. +func (a *application) reportCreatedPull(cmd *cobra.Command, t target, item services.Pull) error { + a.done(cmd, "Created pull request %s", refLabel(t.Repo, item.Number, item.Title)) + if a.options.json != "" { + return a.writeJSON(cmd, item) + } + return a.printURL(cmd, item.URL) } diff --git a/internal/app/pull_create_form_test.go b/internal/app/pull_create_form_test.go index b2895cdf..c984e2fb 100644 --- a/internal/app/pull_create_form_test.go +++ b/internal/app/pull_create_form_test.go @@ -80,7 +80,8 @@ func TestPullCreateImplicitInteractiveForm(t *testing.T) { require.Equal(t, []string{"alice", "bob"}, form.choices.Assignees) require.Equal(t, []terminal.IssueMilestoneChoice{{Title: "Release", ID: 4}}, form.choices.Milestones) require.Equal(t, []terminal.IssueLabelChoice{{Name: "bug", ID: 2}, {Name: "docs", ID: 3}}, form.choices.Labels) - require.Len(t, prompt.confirmations, 1) + require.Empty(t, prompt.confirmations, "submitting the form creates the pull request") + require.Equal(t, "alice/project", form.choices.Repo) require.Equal(t, "Interactive", createPayload["title"]) require.Equal(t, "Body text", createPayload["body"]) require.Equal(t, "main", createPayload["base"]) diff --git a/internal/app/pull_create_test.go b/internal/app/pull_create_test.go index eaed4bca..5ba86863 100644 --- a/internal/app/pull_create_test.go +++ b/internal/app/pull_create_test.go @@ -30,7 +30,7 @@ func publishCreateEnv(t *testing.T, creates *int) (Dependencies, *fakeGit, strin })) t.Cleanup(server.Close) deps, _, _ := serverDeps(t, server.URL) - pushKey := credentialCall(t, deps, server.URL, "alice", "push", "--", "origin", "refs/heads/feature") + pushKey := credentialCall(t, deps, server.URL, "alice", "push", "--set-upstream", "--quiet", "--", "origin", "refs/heads/feature") runner := &fakeGit{results: map[string]string{ "rev-parse --is-inside-work-tree": "true", "rev-parse --verify refs/heads/feature^{commit}": "localsha", diff --git a/internal/app/pull_edit.go b/internal/app/pull_edit.go index cd25a684..8ff33472 100644 --- a/internal/app/pull_edit.go +++ b/internal/app/pull_edit.go @@ -5,6 +5,7 @@ package app import ( "errors" + "strconv" "strings" "gitea.dev/tea/internal/clierr" @@ -14,47 +15,39 @@ import ( ) func (a *application) pullEditCommand() *cobra.Command { - var title, body, file, base string - var deadline string - var draft, ready, maintainer bool - var useEditor bool - var milestone int64 - var milestoneName string + var title, body, file, base, deadline, milestone string + var draft, ready, maintainer, useEditor, removeMilestone bool var metadata metadataFlags var reviewers services.ReviewRequestChanges - cmd := &cobra.Command{Use: "edit ", Short: "Edit pull request content, base, and draft state", Args: argsExact(1), RunE: func(cmd *cobra.Command, args []string) (runErr error) { + cmd := &cobra.Command{Use: "edit [ | | ]", Short: "Edit a pull request: title, body, base, reviewers, labels, assignees, milestone, due date", Annotations: pullArgumentsAnnotation(nil), Args: usageArgs(cobra.MaximumNArgs(1)), RunE: func(cmd *cobra.Command, args []string) (runErr error) { var editorDraft terminal.Draft defer finishDraft(&editorDraft, &runErr) if err := validateBodySources(cmd); err != nil { return err } - n, err := number(args[0]) - if err != nil { - return err - } if draft && ready { return clierr.Wrap(clierr.Usage, errors.New("--draft and --ready are mutually exclusive")) } - if err := validateMilestoneFlags(cmd, milestoneName, milestone); err != nil { - return err + if removeMilestone && cmd.Flags().Changed("milestone") { + return clierr.Wrap(clierr.Usage, errors.New("--milestone and --remove-milestone are mutually exclusive")) + } + if id, err := strconv.ParseInt(milestone, 10, 64); err == nil && id < 0 { + return clierr.Wrap(clierr.Usage, errors.New("--milestone must be a milestone title or a positive ID, or 0 for none")) } opts := services.PullEdit{} + var err error if cmd.Flags().Changed("deadline") { - opts.Deadline, opts.RemoveDeadline, err = parseDeadline(deadline) - if err != nil { + if opts.Deadline, opts.RemoveDeadline, err = parseDeadline(deadline); err != nil { return err } } - if cmd.Flags().Changed("add-reviewers") || cmd.Flags().Changed("remove-reviewers") || cmd.Flags().Changed("add-reviewer-teams") || cmd.Flags().Changed("remove-reviewer-teams") { + if cmd.Flags().Changed("add-reviewer") || cmd.Flags().Changed("remove-reviewer") || cmd.Flags().Changed("add-reviewer-team") || cmd.Flags().Changed("remove-reviewer-team") { trimNames(reviewers.Add.Users, reviewers.Remove.Users, reviewers.Add.Teams, reviewers.Remove.Teams) if err := reviewers.Validate(); err != nil { return clierr.Wrap(clierr.Usage, err) } opts.ReviewerChanges = &reviewers } - if cmd.Flags().Changed("milestone") { - opts.Milestone = &milestone - } if err := metadata.validate(cmd); err != nil { return err } @@ -65,8 +58,7 @@ func (a *application) pullEditCommand() *cobra.Command { opts.Title = &title } if cmd.Flags().Changed("body") || cmd.Flags().Changed("body-file") { - body, err = a.readBody(cmd, body, file) - if err != nil { + if body, err = a.readBody(cmd, body, file); err != nil { return err } opts.Body = &body @@ -80,49 +72,49 @@ func (a *application) pullEditCommand() *cobra.Command { if cmd.Flags().Changed("allow-maintainer-edit") { opts.AllowMaintainerEdit = &maintainer } - if opts == (services.PullEdit{}) && !useEditor && !draft && !ready && !metadata.changed(cmd) && !cmd.Flags().Changed("milestone-name") { - return clierr.Wrap(clierr.Usage, errors.New("supply an edit flag; see tea pr edit --help")) + 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)")) } - target, err := a.resolve(cmd.Context(), true) + t, n, err := a.pullSelector(cmd, args) if err != nil { return err } - if cmd.Flags().Changed("milestone-name") { - milestone, err = (services.Milestones{Client: target.client, Repository: target.Repo}).ResolveName(cmd.Context(), milestoneName) + switch { + case removeMilestone || milestone == "0" || (cmd.Flags().Changed("milestone") && milestone == ""): + var none int64 + opts.Milestone = &none + case milestone != "": + id, err := milestoneFilter(cmd.Context(), t, milestone) if err != nil { return err } - opts.Milestone = &milestone + opts.Milestone = &id } - if opts.Labels, opts.Assignees, opts.Changes, err = metadata.resolve(cmd, target); err != nil { + if opts.ReviewerChanges != nil { + t.expandMeAll(reviewers.Add.Users) + t.expandMeAll(reviewers.Remove.Users) + } + if opts.Labels, opts.Assignees, opts.Changes, err = metadata.resolve(cmd, t); err != nil { return err } - service := services.Pulls{Client: target.client, Repository: target.Repo} + service := services.Pulls{Client: t.client, Repository: t.Repo} + var current services.Pull + if ((draft || ready) && opts.Title == nil) || useEditor { + if current, err = service.View(cmd.Context(), n); err != nil { + return err + } + } if draft || ready { if opts.Title == nil { - current, err := service.View(cmd.Context(), n) - if err != nil { - return err - } title = current.Title } - if draft { - title = services.AddDraftPrefix(title) - } else { - title = services.StripDraftPrefix(title) - } - if strings.TrimSpace(title) == "" { - return clierr.Wrap(clierr.Usage, errors.New("draft marker removal leaves an empty title; supply --title")) + if title = draftTitle(title, draft); strings.TrimSpace(title) == "" { + return clierr.Wrap(clierr.Usage, errors.New("removing the draft marker leaves an empty title; pass --title")) } opts.Title = &title } if useEditor { - current, err := service.View(cmd.Context(), n) - if err != nil { - return err - } - editorDraft, err = a.editDraft(cmd, current.Body) - if err != nil { + if editorDraft, err = a.editDraft(cmd, current.Body); err != nil { return err } opts.Body = &editorDraft.Body @@ -131,23 +123,97 @@ func (a *application) pullEditCommand() *cobra.Command { if err != nil { return err } - return writeRecord(a, cmd, item, pullRecord) + a.done(cmd, "Edited pull request %s", refLabel(t.Repo, n, item.Title)) + if a.options.json != "" { + return a.writeJSON(cmd, item) + } + return nil }} - cmd.Flags().StringVar(&title, "title", "", "Pull request title") + cmd.Flags().StringVarP(&title, "title", "t", "", "Pull request title") cmd.Flags().StringVar(&deadline, "deadline", "", "Due date: YYYY-MM-DD, RFC3339, or Unix timestamp; empty clears") - cmd.Flags().StringVar(&body, "body", "", "Pull request body (empty clears)") - cmd.Flags().StringVar(&file, "body-file", "", "Read body from file, or - for stdin") - cmd.Flags().BoolVar(&useEditor, "editor", false, "Edit the current pull request body in the configured editor") - cmd.Flags().StringVar(&base, "base", "", "Target branch") - cmd.Flags().BoolVar(&draft, "draft", false, "Mark the pull request as a draft") - cmd.Flags().BoolVar(&ready, "ready", false, "Remove the draft title marker") - cmd.Flags().BoolVar(&maintainer, "allow-maintainer-edit", false, "Allow maintainers to modify the head branch") - cmd.Flags().StringVar(&milestoneName, "milestone-name", "", "Milestone by exact title; conflicts with --milestone") - cmd.Flags().Int64Var(&milestone, "milestone", 0, "Milestone ID; 0 clears") + cmd.Flags().StringVarP(&body, "body", "b", "", "Pull request body (empty clears)") + cmd.Flags().StringVarP(&file, "body-file", "F", "", "Read the body from `file`, or - for stdin") + cmd.Flags().BoolVarP(&useEditor, "editor", "e", false, "Edit the current body in your editor") + cmd.Flags().StringVarP(&base, "base", "B", "", "Base branch") + cmd.Flags().BoolVar(&draft, "draft", false, "Mark the pull request as a draft (same as tea pr ready --undo)") + cmd.Flags().BoolVar(&ready, "ready", false, "Mark the pull request as ready for review (same as tea pr ready)") + cmd.Flags().BoolVar(&maintainer, "allow-maintainer-edit", false, "Allow maintainers to push to the head branch") + cmd.Flags().StringVarP(&milestone, "milestone", "m", "", "Milestone title or ID; 0 removes it") + cmd.Flags().BoolVar(&removeMilestone, "remove-milestone", false, "Remove the milestone") metadata.register(cmd) - cmd.Flags().StringSliceVar(&reviewers.Add.Users, "add-reviewers", nil, "Request reviews from users") - cmd.Flags().StringSliceVar(&reviewers.Remove.Users, "remove-reviewers", nil, "Remove requests for users") - cmd.Flags().StringSliceVar(&reviewers.Add.Teams, "add-reviewer-teams", nil, "Request reviews from organization teams") - cmd.Flags().StringSliceVar(&reviewers.Remove.Teams, "remove-reviewer-teams", nil, "Remove requests for organization teams") + cmd.Flags().StringSliceVar(&reviewers.Add.Users, "add-reviewer", nil, "Request reviews from users; @me for yourself") + cmd.Flags().StringSliceVar(&reviewers.Remove.Users, "remove-reviewer", nil, "Withdraw review requests from users") + cmd.Flags().StringSliceVar(&reviewers.Add.Teams, "add-reviewer-team", nil, "Request reviews from teams of the repository's organization") + cmd.Flags().StringSliceVar(&reviewers.Remove.Teams, "remove-reviewer-team", nil, "Withdraw review requests from teams") + flagAliases(cmd, map[string]string{ + "milestone-name": "milestone", "add-reviewers": "add-reviewer", "remove-reviewers": "remove-reviewer", + "add-reviewer-teams": "add-reviewer-team", "remove-reviewer-teams": "remove-reviewer-team", + }) + for _, name := range []string{"add-reviewer", "remove-reviewer"} { + _ = cmd.RegisterFlagCompletionFunc(name, a.completeUsers()) + } + _ = cmd.RegisterFlagCompletionFunc("base", a.completeBranches()) + _ = cmd.RegisterFlagCompletionFunc("milestone", a.completeMilestones()) + cmd.ValidArgsFunction = a.completePulls("open") + cmd.Example = `$ tea pr edit 13 --title "Add login support" --add-reviewer carol +$ tea pr edit --base develop +$ tea pr edit 13 --editor` + return cmd +} + +// draftTitle marks a title as a draft, the way Gitea recognizes drafts, or removes the marker. +func draftTitle(title string, draft bool) string { + if draft { + return services.AddDraftPrefix(title) + } + return services.StripDraftPrefix(title) +} + +func (a *application) pullReadyCommand() *cobra.Command { + var undo bool + cmd := &cobra.Command{Use: "ready [ | | ]", Short: "Mark a pull request as ready for review", Long: `Mark a draft pull request as ready for review, or with --undo turn it back into a draft. + +Gitea treats a pull request as a draft while its title starts with "WIP:" or "[WIP]"; tea +removes or adds that marker.`, Annotations: pullArgumentsAnnotation(nil), Args: usageArgs(cobra.MaximumNArgs(1)), RunE: func(cmd *cobra.Command, args []string) error { + t, n, err := a.pullSelector(cmd, args) + if err != nil { + return err + } + service := services.Pulls{Client: t.client, Repository: t.Repo} + item, err := service.View(cmd.Context(), n) + if err != nil { + return err + } + ref := refLabel(t.Repo, n, item.Title) + if item.Draft == undo { + if undo { + a.warn(cmd, "Pull request %s is already a draft", ref) + } else { + a.warn(cmd, "Pull request %s is already ready for review", ref) + } + } else { + title := draftTitle(item.Title, undo) + if strings.TrimSpace(title) == "" { + return errors.New("removing the draft marker leaves an empty title; set one with: tea pr edit " + strconv.FormatInt(n, 10) + " --title") + } + if item, err = service.Edit(cmd.Context(), n, services.PullEdit{Title: &title}); err != nil { + return err + } + if undo { + a.done(cmd, "Converted pull request %s to a draft", refLabel(t.Repo, n, item.Title)) + } else { + a.done(cmd, "Marked pull request %s as ready for review", refLabel(t.Repo, n, item.Title)) + } + } + if a.options.json != "" { + return a.writeJSON(cmd, item) + } + return nil + }} + cmd.Flags().BoolVar(&undo, "undo", false, "Turn the pull request back into a draft") + cmd.ValidArgsFunction = a.completePulls("open") + cmd.Example = `$ tea pr ready +$ tea pr ready 15 +$ tea pr ready 15 --undo` return cmd } diff --git a/internal/app/pull_merge_form_test.go b/internal/app/pull_merge_form_test.go index 982ffe1a..8530d7df 100644 --- a/internal/app/pull_merge_form_test.go +++ b/internal/app/pull_merge_form_test.go @@ -5,69 +5,197 @@ package app import ( "context" + "encoding/json" "net/http" "testing" + "gitea.dev/tea/internal/clierr" "gitea.dev/tea/internal/terminal" "github.com/stretchr/testify/require" ) -type mergeFormFunc func(context.Context, []terminal.MergePullChoice) (int64, error) - -func (f mergeFormFunc) SelectPull(ctx context.Context, choices []terminal.MergePullChoice) (int64, error) { - return f(ctx, choices) +// mergeForm answers the merge form: pick selects among pull requests, options returns the merge options. +type mergeForm struct { + pick func([]terminal.MergePullChoice) int64 + options func(terminal.MergeFormChoices) terminal.MergeFormValues + asked []terminal.MergeFormChoices } -func TestPullMergeInteractiveSelectsMatchingBranch(t *testing.T) { - merges := 0 - server := newAuthenticatedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { +func (f *mergeForm) SelectPull(_ context.Context, choices []terminal.MergePullChoice) (int64, error) { + return f.pick(choices), nil +} + +func (f *mergeForm) MergeOptions(_ context.Context, choices terminal.MergeFormChoices) (terminal.MergeFormValues, error) { + f.asked = append(f.asked, choices) + return f.options(choices), nil +} + +// mergeRepository allows merge, rebase and squash merges in alice/project and squashes and deletes +// branches by default. +const mergeRepository = `{"full_name":"alice/project","allow_merge_commits":true,"allow_rebase":true,"allow_squash_merge":true,"default_merge_style":"squash","default_delete_branch_after_merge":true}` + +// mergeServer serves pull request 3, "Add login" from feature into main, and records merges into +// payload. After a merge the pull request reads as merged with its head branch deleted. +func mergeServer(t *testing.T, pull string, payload *map[string]any) http.Handler { + t.Helper() + merged := false + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { switch { - case r.Method == http.MethodGet && r.URL.Path == "/api/v1/repos/alice/project/pulls": - require.Equal(t, "open", r.URL.Query().Get("state")) + case r.Method == http.MethodGet && r.URL.Path == "/api/v1/repos/alice/project": + _, _ = w.Write([]byte(mergeRepository)) + case r.Method == http.MethodGet && r.URL.Path == "/api/v1/repos/alice/project/pulls/3" && merged: + _, _ = w.Write([]byte(`{"number":3,"title":"Add login","state":"closed","merged":true,"base":{"ref":"main"},"head":{"ref":"refs/pull/3/head","label":"feature","sha":"abc123"}}`)) + case r.Method == http.MethodGet && r.URL.Path == "/api/v1/repos/alice/project/pulls/3": + _, _ = w.Write([]byte(pull)) + case r.Method == http.MethodPost && r.URL.Path == "/api/v1/repos/alice/project/pulls/3/merge": + merged = true + require.NoError(t, json.NewDecoder(r.Body).Decode(payload)) + default: + t.Errorf("unexpected %s %s", r.Method, r.URL.Path) + w.WriteHeader(http.StatusNotFound) + } + }) +} + +const openPull = `{"number":3,"title":"Add login","state":"open","base":{"ref":"main"},"head":{"ref":"feature","sha":"abc123","repo":{"full_name":"alice/project","permissions":{"push":true}}}}` + +func TestPullMergeUsesTheRepositoryDefaults(t *testing.T) { + var payload map[string]any + server := newAuthenticatedServer(mergeServer(t, openPull, &payload)) + defer server.Close() + deps, out, diagnostics := serverDeps(t, server.URL) + require.NoError(t, Execute(context.Background(), deps, []string{"pr", "merge", "3", "-R", "alice/project", "--yes"})) + require.Equal(t, "squash", payload["Do"]) + require.Equal(t, true, payload["delete_branch_after_merge"]) + require.Empty(t, out.String()) + require.Empty(t, diagnostics.String()) +} + +func TestPullMergeAsksForTheMethodAndBranchDeletion(t *testing.T) { + var payload map[string]any + server := newAuthenticatedServer(mergeServer(t, openPull, &payload)) + defer server.Close() + form := &mergeForm{options: func(choices terminal.MergeFormChoices) terminal.MergeFormValues { + return terminal.MergeFormValues{Method: "rebase", DeleteBranch: false} + }} + prompt := &askingPrompter{} + deps, _, diagnostics := serverDeps(t, server.URL) + deps.MergeForm, deps.Prompter = form, prompt + deps.Streams.InTTY, deps.Streams.ErrTTY, deps.Streams.OutTTY = true, true, true + deps.Getenv = serverEnv(server.URL, map[string]string{"NO_COLOR": "1"}) + require.NoError(t, Execute(context.Background(), deps, []string{"pr", "merge", "3", "-R", "alice/project"})) + require.Equal(t, []terminal.MergeFormChoices{{ + Header: `Merge #3 "Add login" (feature → main) in alice/project`, + Methods: []string{"merge", "rebase", "squash"}, Method: "squash", DeleteBranch: true, Branch: "feature", + }}, form.asked) + require.Empty(t, prompt.confirmations, "the form is the confirmation") + require.Equal(t, "rebase", payload["Do"]) + require.Equal(t, false, payload["delete_branch_after_merge"]) + require.Equal(t, "✓ Rebased and merged pull request alice/project#3 (Add login)\n", diagnostics.String()) + + diagnostics.Reset() + payload = nil + server2 := newAuthenticatedServer(mergeServer(t, openPull, &payload)) + defer server2.Close() + deps.Getenv = serverEnv(server2.URL, map[string]string{"NO_COLOR": "1"}) + prompt.accept = true + require.NoError(t, Execute(context.Background(), deps, []string{"pr", "merge", "3", "-R", "alice/project", "--squash", "-d"})) + require.Len(t, form.asked, 1, "flags answer the form") + require.Equal(t, []string{`Merge #3 "Add login" (feature → main) in alice/project?`}, prompt.confirmations) + require.Equal(t, []bool{true}, prompt.defaults) + require.Equal(t, "✓ Squashed and merged pull request alice/project#3 (Add login)\n✓ Deleted remote branch feature\n", diagnostics.String()) +} + +func TestPullMergeRefusals(t *testing.T) { + for _, tc := range []struct { + name, pull string + args []string + want string + code int + }{ + {"draft", `{"number":3,"title":"WIP: x","state":"open","draft":true}`, nil, "pull request alice/project#3 (WIP: x) is a draft; mark it ready with: tea pr ready 3", clierr.Failure}, + {"closed", `{"number":3,"title":"Old","state":"closed"}`, nil, "pull request alice/project#3 (Old) is closed; reopen it with: tea pr reopen 3", clierr.Failure}, + {"disallowed method", openPull, []string{"--method", "fast-forward-only"}, "fast-forward-only merges are disabled in alice/project; use one of: merge, rebase, squash", clierr.Usage}, + } { + t.Run(tc.name, func(t *testing.T) { + var payload map[string]any + server := newAuthenticatedServer(mergeServer(t, tc.pull, &payload)) + defer server.Close() + deps, _, _ := serverDeps(t, server.URL) + err := Execute(context.Background(), deps, append([]string{"pr", "merge", "3", "-R", "alice/project", "--yes"}, tc.args...)) + require.EqualError(t, err, tc.want) + require.Equal(t, tc.code, clierr.ExitCode(err)) + require.Nil(t, payload) + }) + } +} + +func TestPullMergeAlreadyMergedIsANoOp(t *testing.T) { + var payload map[string]any + server := newAuthenticatedServer(mergeServer(t, `{"number":3,"title":"Done","state":"closed","merged":true}`, &payload)) + defer server.Close() + deps, _, diagnostics := serverDeps(t, server.URL) + deps.Streams.OutTTY = true + deps.Getenv = serverEnv(server.URL, map[string]string{"NO_COLOR": "1"}) + require.NoError(t, Execute(context.Background(), deps, []string{"pr", "merge", "3", "-R", "alice/project"})) + require.Equal(t, "! Pull request alice/project#3 (Done) is already merged\n", diagnostics.String()) + require.Nil(t, payload) +} + +func TestPullMergeMethodFlags(t *testing.T) { + for _, tc := range []struct { + args []string + want string + }{ + {[]string{"pr", "merge", "3", "--squash", "--rebase"}, "choose one merge method: --merge, --squash, --rebase or --method"}, + {[]string{"pr", "merge", "3", "--squash", "-s", "merge"}, "choose one merge method"}, + {[]string{"pr", "merge", "3", "-s"}, "-s takes a method (merge, rebase, rebase-merge, squash or fast-forward-only); for gh-style use --squash"}, + {[]string{"pr", "merge", "3", "-s", "bogus"}, "--method must be merge, rebase, rebase-merge, squash, or fast-forward-only"}, + } { + t.Run(tc.args[len(tc.args)-1], func(t *testing.T) { + requireUsageBeforeRequests(t, tc.want, tc.args...) + }) + } +} + +func TestPullMergePicksAPullRequestForTheBranch(t *testing.T) { + var payload map[string]any + pulls := mergeServer(t, openPull, &payload) + server := newAuthenticatedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/api/v1/repos/alice/project/pulls" { if r.URL.Query().Get("page") != "1" { _, _ = w.Write([]byte(`[]`)) return } _, _ = w.Write([]byte(`[ - {"number":3,"title":"Other","state":"open","base":{"ref":"main"},"head":{"ref":"other","label":"alice:other"}}, - {"number":7,"title":"Feature","state":"open","base":{"ref":"main"},"head":{"ref":"feature","label":"alice:feature"}}, - {"number":9,"title":"Into feature","state":"open","base":{"ref":"feature"},"head":{"ref":"topic","label":"alice:topic"}} + {"number":3,"title":"Add login","state":"open","base":{"ref":"main"},"head":{"ref":"feature"}}, + {"number":9,"title":"Other","state":"open","base":{"ref":"main"},"head":{"ref":"other"}} ]`)) - case r.Method == http.MethodGet && r.URL.Path == "/api/v1/repos/alice/project/pulls/7": - _, _ = w.Write([]byte(`{"number":7,"head":{"ref":"feature","sha":"abc123"}}`)) - case r.Method == http.MethodPost && r.URL.Path == "/api/v1/repos/alice/project/pulls/7/merge": - merges++ - w.WriteHeader(http.StatusOK) - default: - t.Errorf("unexpected %s %s", r.Method, r.URL.Path) - w.WriteHeader(http.StatusNotFound) + return } + pulls.ServeHTTP(w, r) })) defer server.Close() - calls := 0 - prompt := &scriptedPrompter{accept: true} + form := &mergeForm{ + pick: func(choices []terminal.MergePullChoice) int64 { + require.Equal(t, []terminal.MergePullChoice{{Number: 3, Title: "Add login", Head: "feature"}, {Number: 9, Title: "Other", Head: "other"}}, choices) + return 3 + }, + options: func(choices terminal.MergeFormChoices) terminal.MergeFormValues { + return terminal.MergeFormValues{Method: choices.Method, DeleteBranch: choices.DeleteBranch} + }, + } deps, _, _ := serverDeps(t, server.URL) - deps.MergeForm = mergeFormFunc(func(_ context.Context, choices []terminal.MergePullChoice) (int64, error) { - calls++ - require.Len(t, choices, 2) - require.EqualValues(t, 7, choices[0].Number) - require.EqualValues(t, 9, choices[1].Number) - return 7, nil - }) - deps.Prompter = prompt + deps.Getenv = serverEnv(server.URL, map[string]string{"TEA_REPO": "alice/project"}) + deps.MergeForm, deps.Prompter = form, &askingPrompter{} deps.Streams.InTTY, deps.Streams.ErrTTY = true, true - deps.Git = &fakeGit{results: map[string]string{"symbolic-ref --short HEAD": "feature"}} - require.NoError(t, Execute(context.Background(), deps, []string{"pr", "merge", "-R", "alice/project", "--interactive", "--yes"})) - require.Equal(t, 1, calls) - require.Equal(t, 1, merges) + deps.Git = &fakeGit{results: map[string]string{"symbolic-ref --short HEAD": "main"}} + require.NoError(t, Execute(context.Background(), deps, []string{"pr", "merge"})) + require.Equal(t, "squash", payload["Do"]) } func TestPullMergeRequiresNumberWithoutInteractive(t *testing.T) { deps, _, _ := testDeps(t) - deps.MergeForm = mergeFormFunc(func(context.Context, []terminal.MergePullChoice) (int64, error) { - t.Fatal("merge form should not run") - return 0, nil - }) err := Execute(context.Background(), deps, []string{"pr", "merge", "-R", "alice/project", "--no-input"}) require.Error(t, err) require.Contains(t, err.Error(), "pull request number") diff --git a/internal/app/pull_merge_title_test.go b/internal/app/pull_merge_title_test.go index 13753d38..c5ca728d 100644 --- a/internal/app/pull_merge_title_test.go +++ b/internal/app/pull_merge_title_test.go @@ -5,8 +5,6 @@ package app import ( "context" - "encoding/json" - "net/http" "os" "testing" @@ -16,22 +14,12 @@ import ( func TestPullMergeSendsTitleAndMessage(t *testing.T) { var payload map[string]any - server := newAuthenticatedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if r.Method == http.MethodGet { - require.Equal(t, "/api/v1/repos/alice/project/pulls/3", r.URL.Path) - _, _ = w.Write([]byte(`{"number":3,"head":{"sha":"abc123"}}`)) - return - } - require.Equal(t, http.MethodPost, r.Method) - require.Equal(t, "/api/v1/repos/alice/project/pulls/3/merge", r.URL.Path) - require.NoError(t, json.NewDecoder(r.Body).Decode(&payload)) - w.WriteHeader(http.StatusOK) - })) + server := newAuthenticatedServer(mergeServer(t, openPull, &payload)) defer server.Close() deps, _, _ := serverDeps(t, server.URL) require.NoError(t, Execute(context.Background(), deps, []string{ "pr", "merge", "3", "-R", "alice/project", "--method", "squash", - "--title", "Squash title", "--message", "Squash body", "--yes", + "--title", "Squash title", "--message", "Squash body", "--delete-branch=false", "--yes", })) require.Equal(t, "squash", payload["Do"]) require.Equal(t, "Squash title", payload["MergeTitleField"]) @@ -40,82 +28,40 @@ func TestPullMergeSendsTitleAndMessage(t *testing.T) { require.Equal(t, "abc123", payload["head_commit_id"]) payload = nil - require.NoError(t, Execute(context.Background(), deps, []string{ - "pr", "merge", "3", "-R", "alice/project", "--style", "rebase", "--yes", - })) + server = newAuthenticatedServer(mergeServer(t, openPull, &payload)) + defer server.Close() + deps.Getenv = serverEnv(server.URL, nil) + require.NoError(t, Execute(context.Background(), deps, []string{"pr", "merge", "3", "-R", "alice/project", "--style", "rebase", "--yes"})) require.Equal(t, "rebase", payload["Do"]) } func TestPullMergeMessageFileAndEditor(t *testing.T) { var payload map[string]any - server := newAuthenticatedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if r.Method == http.MethodGet { - _, _ = w.Write([]byte(`{"number":3,"head":{"sha":"abc123"}}`)) - return - } - require.NoError(t, json.NewDecoder(r.Body).Decode(&payload)) - w.WriteHeader(http.StatusOK) - })) - defer server.Close() deps, _, _ := testDeps(t) - deps.Getenv = serverEnv(server.URL, map[string]string{"EDITOR": "chosen-editor"}) file := t.TempDir() + "/message.md" require.NoError(t, os.WriteFile(file, []byte("from file"), 0o600)) - require.NoError(t, Execute(context.Background(), deps, []string{ - "pr", "merge", "3", "-R", "alice/project", "--message-file", file, "--yes", - })) + server := newAuthenticatedServer(mergeServer(t, openPull, &payload)) + defer server.Close() + deps.Getenv = serverEnv(server.URL, map[string]string{"EDITOR": "chosen-editor"}) + require.NoError(t, Execute(context.Background(), deps, []string{"pr", "merge", "3", "-R", "alice/project", "--message-file", file, "--yes"})) require.Equal(t, "from file", payload["MergeMessageField"]) + server = newAuthenticatedServer(mergeServer(t, openPull, &payload)) + defer server.Close() + deps.Getenv = serverEnv(server.URL, map[string]string{"EDITOR": "chosen-editor"}) deps.Editor, _ = newDraftEditor(t, "from editor") - require.NoError(t, Execute(context.Background(), deps, []string{ - "pr", "merge", "3", "-R", "alice/project", "--editor", "--yes", "--no-input", - })) + require.NoError(t, Execute(context.Background(), deps, []string{"pr", "merge", "3", "-R", "alice/project", "--editor", "--yes", "--no-input"})) require.Equal(t, "from editor", payload["MergeMessageField"]) require.Equal(t, clierr.Usage, clierr.ExitCode(Execute(context.Background(), deps, []string{ "pr", "merge", "3", "-R", "alice/project", "--message", "x", "--editor", "--yes", }))) } -func TestPullMergePromptsToDeleteBranch(t *testing.T) { - for _, tc := range []struct { - name string - accept, tty bool - flags []string - want bool - confirmations int - }{ - {"accepted", true, true, nil, true, 2}, - {"explicit flag skips prompt", true, true, []string{"--delete-branch=false"}, false, 1}, - {"no terminal keeps default", true, false, []string{"--yes"}, false, 0}, - } { - t.Run(tc.name, func(t *testing.T) { - var payload map[string]any - views := 0 - server := newAuthenticatedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - switch { - case r.Method == http.MethodGet && r.URL.Path == "/api/v1/repos/alice/project/pulls/3": - views++ - _, _ = w.Write([]byte(`{"number":3,"head":{"ref":"feature","sha":"abc123"}}`)) - case r.Method == http.MethodPost && r.URL.Path == "/api/v1/repos/alice/project/pulls/3/merge": - require.NoError(t, json.NewDecoder(r.Body).Decode(&payload)) - default: - t.Errorf("unexpected %s %s", r.Method, r.URL.Path) - w.WriteHeader(http.StatusNotFound) - } - })) - defer server.Close() - prompt := &scriptedPrompter{accept: tc.accept} - deps, _, _ := serverDeps(t, server.URL) - deps.Prompter = prompt - deps.Streams.InTTY, deps.Streams.ErrTTY = tc.tty, tc.tty - require.NoError(t, Execute(context.Background(), deps, append([]string{"pr", "merge", "3", "-R", "alice/project"}, tc.flags...))) - require.Equal(t, tc.want, payload["delete_branch_after_merge"]) - require.Len(t, prompt.confirmations, tc.confirmations) - promptViews := 0 - if tc.confirmations == 2 { - promptViews = 1 - } - require.Equal(t, 1+promptViews, views, "the merge reads the current head; the prompt reads the branch") - }) - } +func TestPullMergeReturnsTheMergedPullRequest(t *testing.T) { + var payload map[string]any + server := newAuthenticatedServer(mergeServer(t, openPull, &payload)) + defer server.Close() + deps, out, _ := serverDeps(t, server.URL) + require.NoError(t, Execute(context.Background(), deps, []string{"pr", "merge", "3", "-R", "alice/project", "--yes", "--json", "number,merged"})) + require.JSONEq(t, `{"number":3,"merged":true}`, out.String()) } diff --git a/internal/app/pull_mutations.go b/internal/app/pull_mutations.go index 9b8a707b..8a9567f2 100644 --- a/internal/app/pull_mutations.go +++ b/internal/app/pull_mutations.go @@ -4,20 +4,25 @@ package app import ( + "bufio" "errors" "fmt" "io" + "slices" "strings" "gitea.dev/tea/internal/clierr" + "gitea.dev/tea/internal/git" "gitea.dev/tea/internal/services" "gitea.dev/tea/internal/settings" "gitea.dev/tea/internal/terminal" + "gitea.dev/tea/internal/transport" "github.com/spf13/cobra" ) -// ensureHeadPublished prompts to push the head branch when the selected remote tip is missing or differs. -// Cross-repo heads (owner:branch), non-work-trees, and repositories without a matching remote are skipped. +// ensureHeadPublished pushes the head branch, setting its upstream, when the selected remote tip is +// missing or differs. Cross-repo heads (owner:branch), non-work-trees, and repositories without a +// matching remote are skipped. func (a *application) ensureHeadPublished(cmd *cobra.Command, t target, snapshot settings.Snapshot, head string) error { if head == "" || strings.Contains(head, ":") { return nil @@ -43,145 +48,375 @@ func (a *application) ensureHeadPublished(cmd *cobra.Command, t target, snapshot if tipErr == nil && remoteTip == local { return nil } - if err := a.confirm(cmd, fmt.Sprintf("Push branch %q to remote %q before creating the pull request?", head, remote)); err != nil { + confirmation := terminal.Confirmation{ + Prompt: fmt.Sprintf("Push branch %s to %s first?", head, remote), DefaultYes: true, + NonTTY: fmt.Sprintf("push branch %q to remote %q before creating the pull request (or push it first)", head, remote), + } + if err := a.confirmAction(cmd, confirmation); err != nil { return err } credentials, err := a.remoteCredentials(ctx, t, remote) if err != nil { return err } - args := append(append([]string{}, credentials...), "push", "--", remote, "refs/heads/"+head) - _, err = a.git(ctx, args...) - return err -} - -func pullRecord(item services.Pull) terminal.Record { - return numberedRecord(item.Number, item.Title, item.URL) + args := append(append([]string{}, credentials...), "push", "--set-upstream", "--quiet", "--", remote, "refs/heads/"+head) + if _, err = a.git(ctx, args...); err != nil { + return err + } + a.done(cmd, "Pushed %s to %s", head, remote) + return nil } func (a *application) addPullMutations(group *cobra.Command) { - group.AddCommand(a.pullCreateCommand()) - for _, verb := range []string{"close", "reopen"} { - group.AddCommand(&cobra.Command{Use: verb + " ", Short: titleCase(verb) + " a pull request", Args: argsExact(1), RunE: func(cmd *cobra.Command, args []string) error { - n, err := number(args[0]) - if err != nil { - return err - } - t, err := a.resolve(cmd.Context(), true) - if err != nil { - return err - } - state := "closed" - if verb == "reopen" { - state = "open" - } - item, err := (services.Pulls{Client: t.client, Repository: t.Repo}).Edit(cmd.Context(), n, services.PullEdit{State: &state}) - if err != nil { - return err - } - return writeRecord(a, cmd, item, pullRecord) - }}) + group.AddCommand(a.pullCreateCommand(), a.pullStateCommand(true), a.pullStateCommand(false), a.pullDiffCommand(), a.pullReviewCommand(), a.pullMergeCommand()) +} + +// pullStateCommand builds tea pr close, or tea pr reopen when closing is false. +func (a *application) pullStateCommand(closing bool) *cobra.Command { + verb, done, state := "reopen", "Reopened", "open" + if closing { + verb, done, state = "close", "Closed", "closed" } - group.AddCommand(&cobra.Command{Use: "diff ", Short: "Print a pull request diff", Annotations: textOutput("diff returns patch text"), Args: argsExact(1), RunE: func(cmd *cobra.Command, args []string) error { - n, err := number(args[0]) + var comment string + var deleteBranch bool + cmd := &cobra.Command{Use: verb + " [ | | ]", Short: titleCase(verb) + " a pull request", Annotations: pullArgumentsAnnotation(nil), Args: usageArgs(cobra.MaximumNArgs(1)), RunE: func(cmd *cobra.Command, args []string) error { + t, n, err := a.pullSelector(cmd, args) if err != nil { return err } - t, err := a.resolve(cmd.Context(), true) + service := services.Pulls{Client: t.client, Repository: t.Repo} + item, err := service.View(cmd.Context(), n) if err != nil { return err } - body, err := (services.Pulls{Client: t.client, Repository: t.Repo}).Diff(cmd.Context(), n) + ref := refLabel(t.Repo, n, item.Title) + switch { + case item.Merged: + return fmt.Errorf("pull request %s is already merged; it cannot be %s", ref, map[bool]string{true: "closed", false: "reopened"}[closing]) + case item.State == state: + a.warn(cmd, "Pull request %s is already %s", ref, state) + default: + if comment != "" { + if _, err := (services.Comments{Client: t.client, Repository: t.Repo}).Create(cmd.Context(), n, comment); err != nil { + return err + } + } + if item, err = service.Edit(cmd.Context(), n, services.PullEdit{State: &state}); err != nil { + return err + } + a.done(cmd, "%s pull request %s", done, ref) + } + if deleteBranch { + a.deleteHead(cmd, service, item, false) + } + if a.options.json != "" { + return a.writeJSON(cmd, item) + } + return nil + }} + cmd.Flags().StringVarP(&comment, "comment", "c", "", "Leave a comment before changing the state") + cmd.ValidArgsFunction = a.completePulls(map[bool]string{true: "open", false: "closed"}[closing]) + if closing { + cmd.Flags().BoolVarP(&deleteBranch, "delete-branch", "d", false, "Delete the head branch on Gitea and, when it is checked out, locally") + cmd.Example = `$ tea pr close 13 +$ tea pr close 13 -c "Superseded by #15" --delete-branch` + } else { + cmd.Example = `$ tea pr reopen 13` + } + return cmd +} + +// deleteHead deletes the head branch of a merged or closed pull request on Gitea, unless it is +// gone, and then locally, reporting each step. merging tells that Gitea deleted it as part of +// the merge. +func (a *application) deleteHead(cmd *cobra.Command, service services.Pulls, item services.Pull, merging bool) { + switch { + case item.HeadRemoteDeleted && merging: + a.done(cmd, "Deleted remote branch %s", item.Head) + case item.HeadRemoteDeleted: + a.warn(cmd, "Remote branch %s was already deleted", item.Head) + case !item.HeadCanPush: + a.warn(cmd, "Kept remote branch %s: you cannot push to %s", item.Head, item.HeadRepo) + default: + if err := service.DeleteHead(cmd.Context(), item); err != nil { + a.warn(cmd, "Kept remote branch %s: %v", item.Head, err) + } else { + a.done(cmd, "Deleted remote branch %s", item.Head) + } + } + a.deleteLocalHead(cmd, item) +} + +// deleteLocalHead deletes the checked-out branch when it is at the head of the pull request, +// switching to the base branch first, like gh pr merge --delete-branch. +func (a *application) deleteLocalHead(cmd *cobra.Command, item services.Pull) { + ctx := cmd.Context() + branch, err := a.currentBranch(ctx) + if err != nil || branch == item.Base || item.HeadSHA == "" { + return + } + if tip, err := a.gitProbe().Output(ctx, a.options.cwd, false, "rev-parse", "--verify", "refs/heads/"+branch+"^{commit}"); err != nil || tip != item.HeadSHA { + return + } + result, err := git.CleanPull(ctx, a.deps.Git, git.CleanOptions{Directory: a.options.cwd, DefaultBranch: item.Base, LocalBranch: branch, Prompt: a.canPrompt()}) + if err != nil { + a.warn(cmd, "Kept local branch %s: %v", branch, err) + return + } + a.done(cmd, "Switched to %s and deleted local branch %s", result.DefaultBranch, result.LocalBranch) +} + +func (a *application) pullDiffCommand() *cobra.Command { + var nameOnly, web bool + var color string + cmd := &cobra.Command{Use: "diff [ | | ]", Short: "Show the changes of a pull request", Long: `Show the changes of a pull request as a unified diff. + +On a terminal the diff is colored and paged. Piped, it is written unchanged, so that +tea pr diff 13 | git apply works.`, Annotations: pullArgumentsAnnotation(textOutput("diff returns patch text")), Args: usageArgs(cobra.MaximumNArgs(1)), RunE: func(cmd *cobra.Command, args []string) error { + t, n, err := a.pullSelector(cmd, args) + if err != nil { + return err + } + service := services.Pulls{Client: t.client, Repository: t.Repo} + if web { + item, err := service.View(cmd.Context(), n) + if err != nil { + return err + } + return a.openBrowser(cmd, item.URL+"/files") + } + if nameOnly { + files, err := service.Files(cmd.Context(), n) + if err != nil { + return err + } + for i, file := range files { + files[i] = terminal.SafeLine(file) + } + _, err = fmt.Fprint(cmd.OutOrStdout(), strings.Join(append(files, ""), "\n")) + return err + } + body, err := service.Diff(cmd.Context(), n) if err != nil { return err } defer body.Close() - _, err = io.Copy(terminal.SafeWriter(cmd.OutOrStdout(), a.deps.Streams.OutTTY, false), body) - return err - }}) - group.AddCommand(a.pullReviewCommand()) - var method, mergeTitle, mergeMessage, mergeMessageFile string - var deleteBranch, mergeEditor bool - merge := &cobra.Command{Use: "merge [number]", Short: "Merge a pull request", Args: usageArgs(cobra.MaximumNArgs(1)), RunE: func(cmd *cobra.Command, args []string) (runErr error) { + d := a.display() + colored := color == "always" || (color == "auto" && d.Color) + if !d.TTY && !colored { + _, err = io.Copy(cmd.OutOrStdout(), body) + return err + } + text, err := diffText(body, colored) + if err != nil { + return err + } + if !d.TTY { + _, err = io.WriteString(cmd.OutOrStdout(), text) + return err + } + return a.writeHuman(cmd, text) + }} + cmd.Flags().BoolVar(&nameOnly, "name-only", false, "List only the names of the changed files") + enumFlag(cmd, &color, "color", "", "auto", "Color the diff", "auto", "always", "never") + cmd.Flags().BoolVarP(&web, "web", "w", false, "Open the changed files in the browser") + cmd.ValidArgsFunction = a.completePulls("all") + cmd.Example = `$ tea pr diff +$ tea pr diff 13 --name-only +$ tea pr diff 13 | git apply` + return cmd +} + +// diffText sanitizes a diff for a terminal and, when colored is set, colors it like git: added +// lines green, removed lines red, hunk headers cyan and file headers bold. +func diffText(diff io.Reader, colored bool) (string, error) { + var b strings.Builder + scanner := bufio.NewScanner(diff) + scanner.Buffer(make([]byte, 64*1024), 16*1024*1024) + for scanner.Scan() { + line := terminal.SafeText(scanner.Text()) + switch { + case !colored: + case strings.HasPrefix(line, "diff --git"), strings.HasPrefix(line, "index "), strings.HasPrefix(line, "--- "), strings.HasPrefix(line, "+++ "), + strings.HasPrefix(line, "new file mode"), strings.HasPrefix(line, "deleted file mode"): + line = terminal.Emphasis(line, true) + case strings.HasPrefix(line, "@@"): + line = terminal.Colorize("cyan", line, true) + case strings.HasPrefix(line, "+"): + line = terminal.Colorize("green", line, true) + case strings.HasPrefix(line, "-"): + line = terminal.Colorize("red", line, true) + } + b.WriteString(line + "\n") + } + return b.String(), scanner.Err() +} + +// mergeVerbs report a merge by its style: "Squashed and merged pull request alice/x#3 (title)". +var mergeVerbs = map[string]string{ + "merge": "Merged", "rebase": "Rebased and merged", "rebase-merge": "Rebased and merged", "squash": "Squashed and merged", "fast-forward-only": "Merged", +} + +// mergeMethods are the merge styles in Gitea's order. +var mergeMethods = []string{"merge", "rebase", "rebase-merge", "squash", "fast-forward-only"} + +func (a *application) pullMergeCommand() *cobra.Command { + var method, title, message, messageFile string + var deleteBranch, useEditor, auto bool + var style struct{ merge, squash, rebase bool } + cmd := &cobra.Command{Use: "merge [ | | ]", Short: "Merge a pull request", Long: `Merge a pull request on Gitea. + +Without a method, tea uses the repository's default merge style. On a terminal, leaving out +the method or --delete-branch asks for them, preselecting the repository's defaults. With +--delete-branch, tea also deletes the local head branch when it is checked out, switching to +the base branch.`, Annotations: pullArgumentsAnnotation(map[string]string{"tea.confirm": "true"}), Args: usageArgs(cobra.MaximumNArgs(1)), RunE: func(cmd *cobra.Command, args []string) (runErr error) { var editorDraft terminal.Draft defer finishDraft(&editorDraft, &runErr) - if method != "merge" && method != "rebase" && method != "rebase-merge" && method != "squash" && method != "fast-forward-only" { - return clierr.Wrap(clierr.Usage, errors.New("unsupported merge method")) + for name, set := range map[string]bool{"merge": style.merge, "squash": style.squash, "rebase": style.rebase} { + if set && method != "" { + return clierr.Wrap(clierr.Usage, errors.New("choose one merge method: --merge, --squash, --rebase or --method")) + } + if set { + method = name + } } if err := validateTextSources(cmd, "message", "message-file"); err != nil { return err } - if !mergeEditor { + if !useEditor { var err error - mergeMessage, err = a.readTextSource(cmd, mergeMessage, mergeMessageFile, "message", "message-file") - if err != nil { + if message, err = a.readTextSource(cmd, message, messageFile, "message", "message-file"); err != nil { return err } } - snapshot, err := a.deps.Settings.Load() + t, n, err := a.mergeTarget(cmd, args) if err != nil { return err } - policy := a.policy(snapshot) - if len(args) == 0 && (!policy.CanPrompt(a.deps.Streams) || a.deps.MergeForm == nil) { - return clierr.Wrap(clierr.Usage, errors.New("supply a pull request number, or run interactively to choose one")) - } - t, err := a.resolve(cmd.Context(), true) + service := services.Pulls{Client: t.client, Repository: t.Repo} + item, err := service.View(cmd.Context(), n) if err != nil { return err } - var n int64 - if len(args) == 1 { - n, err = number(args[0]) + ref := refLabel(t.Repo, n, item.Title) + switch { + case item.Merged: + a.warn(cmd, "Pull request %s is already merged", ref) + if a.options.json != "" { + return a.writeJSON(cmd, item) + } + return nil + case item.State == "closed": + return fmt.Errorf("pull request %s is closed; reopen it with: tea pr reopen %d", ref, n) + case item.Draft: + return fmt.Errorf("pull request %s is a draft; mark it ready with: tea pr ready %d", ref, n) + } + repoSettings, err := service.MergeSettings(cmd.Context()) + if err != nil { + return err + } + askMethod, askDelete := method == "", !cmd.Flags().Changed("delete-branch") && !item.HeadRemoteDeleted + if method == "" { + method = repoSettings.Default + } else if !slices.Contains(repoSettings.Styles, method) { + return clierr.Wrap(clierr.Usage, fmt.Errorf("%s merges are disabled in %s; use one of: %s", method, t.Repo, strings.Join(repoSettings.Styles, ", "))) + } + if !cmd.Flags().Changed("delete-branch") { + deleteBranch = repoSettings.DeleteBranch + } + header := fmt.Sprintf("Merge #%d %q (%s → %s) in %s", n, item.Title, pullHead(item, t.Repo), item.Base, t.Repo) + if (askMethod || askDelete) && !a.options.yes && a.canPrompt() && a.deps.MergeForm != nil { + choices := terminal.MergeFormChoices{Header: header, Method: method, DeleteBranch: deleteBranch} + if askMethod { + choices.Methods = repoSettings.Styles + } + if askDelete { + choices.Branch = item.Head + } + values, err := a.deps.MergeForm.MergeOptions(cmd.Context(), choices) if err != nil { return err } + method, deleteBranch = values.Method, values.DeleteBranch + } else if err := a.confirmAction(cmd, terminal.Confirmation{Prompt: header + "?", DefaultYes: true, NonTTY: "merge " + t.Repo + "#" + fmt.Sprint(n)}); err != nil { + return err + } + if useEditor { + if editorDraft, err = a.editDraft(cmd, ""); err != nil { + return err + } + message = editorDraft.Body + } + if err := service.Merge(cmd.Context(), n, services.MergeOptions{Method: method, Title: title, Message: message, DeleteBranch: deleteBranch, Auto: auto}); err != nil { + return err + } + merged, err := service.View(cmd.Context(), n) + if err != nil { + return fmt.Errorf("pull request %s was merged, but reading it failed: %w", ref, err) + } + if auto && !merged.Merged { + a.done(cmd, "Pull request %s will be merged when its checks succeed", ref) } else { - n, err = a.selectPullToMerge(cmd, t) - if err != nil { - return err + a.done(cmd, "%s pull request %s", mergeVerbs[method], ref) + if deleteBranch { + a.deleteHead(cmd, service, merged, true) } } - if mergeEditor { - editorDraft, err = a.editDraft(cmd, "") - if err != nil { - return err - } - mergeMessage = editorDraft.Body + if a.options.json != "" { + return a.writeJSON(cmd, merged) } - if err := a.confirm(cmd, fmt.Sprintf("Merge pull request %s#%d", t.Repo, n)); err != nil { - return err - } - if !cmd.Flags().Changed("delete-branch") && !a.options.yes && policy.CanPrompt(a.deps.Streams) { - item, err := (services.Pulls{Client: t.client, Repository: t.Repo}).View(cmd.Context(), n) - if err != nil { - return err - } - if !item.HeadRemoteDeleted { - if deleteBranch, err = a.deps.Prompter.Confirm(cmd.Context(), fmt.Sprintf("Delete branch %s after merging?", item.Head)); err != nil { - return err - } - } - } - return (services.Pulls{Client: t.client, Repository: t.Repo}).Merge(cmd.Context(), n, services.MergeOptions{ - Method: method, Title: mergeTitle, Message: mergeMessage, DeleteBranch: deleteBranch, - }) + return nil }} - merge.Flags().StringVarP(&method, "method", "s", "merge", "Merge strategy") - merge.Flags().StringVar(&method, "style", "merge", "Alias of --method") - _ = merge.Flags().MarkHidden("style") - merge.Flags().StringVar(&mergeTitle, "title", "", "Merge commit title") - merge.Flags().StringVar(&mergeMessage, "message", "", "Merge commit message") - merge.Flags().StringVar(&mergeMessageFile, "message-file", "", "Read merge commit message from file, or - for stdin") - merge.Flags().BoolVar(&mergeEditor, "editor", false, "Compose the merge commit message in the configured editor") - merge.Flags().BoolVar(&deleteBranch, "delete-branch", false, "Delete head branch after merging") - group.AddCommand(merge) + enumFlag(cmd, &method, "method", "s", "", "Merge method (default: the repository's default merge style)", mergeMethods...) + cmd.Flags().StringVar(&method, "style", "", "Alias of --method") + enumValues(cmd, "style", mergeMethods...) + _ = cmd.Flags().MarkHidden("style") + cmd.Flags().BoolVar(&style.merge, "merge", false, "Create a merge commit") + cmd.Flags().BoolVar(&style.squash, "squash", false, "Squash the commits into one commit and merge it") + cmd.Flags().BoolVar(&style.rebase, "rebase", false, "Rebase the commits onto the base branch") + cmd.Flags().BoolVarP(&deleteBranch, "delete-branch", "d", false, "Delete the head branch after merging (default: the repository setting)") + cmd.Flags().BoolVar(&auto, "auto", false, "Merge once the required checks succeed") + cmd.Flags().StringVarP(&title, "title", "t", "", "Merge commit title") + cmd.Flags().StringVarP(&message, "message", "b", "", "Merge commit message") + cmd.Flags().StringVarP(&messageFile, "message-file", "F", "", "Read the merge commit message from `file`, or - for stdin") + cmd.Flags().BoolVarP(&useEditor, "editor", "e", false, "Write the merge commit message in your editor") + cmd.SetFlagErrorFunc(func(c *cobra.Command, err error) error { + if strings.Contains(err.Error(), "flag needs an argument: 's' in -s") { + return clierr.Wrap(clierr.Usage, errors.New("-s takes a method (merge, rebase, rebase-merge, squash or fast-forward-only); for gh-style use --squash")) + } + return c.Root().FlagErrorFunc()(c, err) + }) + cmd.ValidArgsFunction = a.completePulls("open") + cmd.Example = `# Merge the pull request of the current branch with the repository's default style +$ tea pr merge + +# Squash #13 and delete its branch +$ tea pr merge 13 --squash --delete-branch + +# Merge once the checks pass, without prompts +$ tea pr merge 13 --auto --yes` + return cmd +} + +// mergeTarget selects the pull request to merge: the argument, else the current branch's pull +// request, else, on a terminal, one that the user picks among those for the current branch. +func (a *application) mergeTarget(cmd *cobra.Command, args []string) (target, int64, error) { + t, n, err := a.pullSelector(cmd, args) + if err == nil || len(args) > 0 || a.options.repo != "" || a.options.yes || !a.canPrompt() || a.deps.MergeForm == nil || + clierr.ExitCode(err) != clierr.Failure || errors.As(err, new(*transport.APIError)) { + return t, n, err + } + if t, err = a.resolve(cmd.Context(), true); err != nil { + return t, 0, err + } + n, err = a.selectPullToMerge(cmd, t) + return t, n, err } func (a *application) selectPullToMerge(cmd *cobra.Command, t target) (int64, error) { branch, err := a.currentBranch(cmd.Context()) if err != nil { - return 0, clierr.Wrap(clierr.Usage, errors.New("supply a pull number outside a branch checkout")) + return 0, clierr.Wrap(clierr.Usage, errors.New("not on a branch; pass a pull request number, URL or branch")) } items, err := (services.Pulls{Client: t.client, Repository: t.Repo}).ListOpen(cmd.Context()) if err != nil { @@ -189,11 +424,7 @@ func (a *application) selectPullToMerge(cmd *cobra.Command, t target) (int64, er } var headMatches, baseMatches []terminal.MergePullChoice for _, item := range items { - label := item.Head - if owner, _, ok := strings.Cut(item.HeadRepo, "/"); ok && !strings.EqualFold(item.HeadRepo, t.Repo) { - label = owner + ":" + item.Head // fork heads can share a branch name with the local one - } - choice := terminal.MergePullChoice{Number: item.Number, Title: item.Title, Head: label} + choice := terminal.MergePullChoice{Number: item.Number, Title: item.Title, Head: pullHead(item, t.Repo)} switch branch { case item.Head: headMatches = append(headMatches, choice) diff --git a/internal/app/pull_mutations_test.go b/internal/app/pull_mutations_test.go index b393f201..63552cba 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" + "strings" "testing" "github.com/stretchr/testify/require" @@ -18,10 +19,43 @@ func TestPullDiffStripsTerminalControlsOnlyOnATerminal(t *testing.T) { _, _ = w.Write([]byte(diff)) })) defer server.Close() - for tty, want := range map[bool]string{false: diff, true: "+hello ]0;PWNED [31mred[0m\n-old\tline\n"} { + for _, tc := range []struct { + tty bool + args []string + color string + want string + }{ + {false, nil, "", diff}, + {true, nil, "1", "+hello ]0;PWNED [31mred[0m\n-old\tline\n"}, + {true, nil, "", "\x1b[32m+hello ]0;PWNED [31mred[0m\x1b[0m\n\x1b[31m-old\tline\x1b[0m\n"}, + {false, []string{"--color", "always"}, "", "\x1b[32m+hello ]0;PWNED [31mred[0m\x1b[0m\n\x1b[31m-old\tline\x1b[0m\n"}, + {true, []string{"--color", "never"}, "", "+hello ]0;PWNED [31mred[0m\n-old\tline\n"}, + } { deps, out, _ := serverDeps(t, server.URL) - deps.Streams.OutTTY = tty - require.NoError(t, Execute(context.Background(), deps, []string{"pr", "diff", "2", "-R", "alice/project"})) - require.Equal(t, want, out.String(), "tty=%t", tty) + deps.Getenv = serverEnv(server.URL, map[string]string{"NO_COLOR": tc.color}) + deps.Streams.OutTTY = tc.tty + require.NoError(t, Execute(context.Background(), deps, append([]string{"pr", "diff", "2", "-R", "alice/project"}, tc.args...))) + require.Equal(t, tc.want, out.String(), "%+v", tc) } } + +func TestPullDiffColorsHeadersAndHunks(t *testing.T) { + text, err := diffText(strings.NewReader("diff --git a/x b/x\n--- a/x\n+++ b/x\n@@ -1 +1 @@\n context\n"), true) + require.NoError(t, err) + require.Equal(t, "\x1b[1mdiff --git a/x b/x\x1b[0m\n\x1b[1m--- a/x\x1b[0m\n\x1b[1m+++ b/x\x1b[0m\n\x1b[36m@@ -1 +1 @@\x1b[0m\n context\n", text) +} + +func TestPullDiffNameOnly(t *testing.T) { + server := newAuthenticatedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + require.Equal(t, "/api/v1/repos/alice/project/pulls/2/files", r.URL.Path) + if r.URL.Query().Get("page") != "1" { + _, _ = w.Write([]byte(`[]`)) + return + } + _, _ = w.Write([]byte(`[{"filename":"one.txt"},{"filename":"docs/two.md"}]`)) + })) + defer server.Close() + deps, out, _ := serverDeps(t, server.URL) + 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()) +} diff --git a/internal/app/pulls.go b/internal/app/pulls.go index 9441772c..69a83bb1 100644 --- a/internal/app/pulls.go +++ b/internal/app/pulls.go @@ -33,7 +33,7 @@ $ tea pr merge --squash --delete-branch` group.AddCommand(a.pullChecksCommand(), a.pullCheckoutCommand(), a.pullCleanCommand(), a.statusCommand(true)) group.AddCommand(a.pullListCommand(), a.pullViewCommand()) a.addPullMutations(group) - group.AddCommand(a.pullEditCommand()) + group.AddCommand(a.pullEditCommand(), a.pullReadyCommand()) group.AddCommand(a.issueAttachmentCommand(), a.commentCommand()) return group } diff --git a/internal/app/repository_sync.go b/internal/app/repository_sync.go index 4ef46306..ede1aff8 100644 --- a/internal/app/repository_sync.go +++ b/internal/app/repository_sync.go @@ -155,8 +155,3 @@ func (a *application) reportSync(cmd *cobra.Command, result git.SyncResult, sour a.done(cmd, "Synced %s from %s (fast-forward %s..%s)", result.Branch, source, shortSHA(result.Previous), shortSHA(result.Commit)) } } - -// shortSHA abbreviates a commit ID to seven characters, as Gitea and Git show them. -func shortSHA(sha string) string { - return sha[:min(len(sha), 7)] -} diff --git a/internal/app/validation_test.go b/internal/app/validation_test.go index f45e62ea..d9ea5fdb 100644 --- a/internal/app/validation_test.go +++ b/internal/app/validation_test.go @@ -106,11 +106,9 @@ func TestUsageErrorsPrecedeRequests(t *testing.T) { {[]string{"pr", "attachment", "download", "1", "1", "--json"}, "attachment downloads are binary"}, {[]string{"pr", "attachment", "download", "1", "1", "--output-file", "-", "--json=id"}, "attachment downloads are binary"}, {[]string{"pr", "checkout", "7", "--detach", "--branch", "name"}, "--detach and --branch/--update are mutually exclusive"}, - {[]string{"pr", "create", "--milestone=-1"}, "--milestone must be a positive ID, or 0 for none"}, + {[]string{"pr", "create", "--milestone=-1"}, "--milestone must be a milestone title or a positive ID"}, {[]string{"pr", "create", "--assignee", "alice, "}, "assignee names cannot be empty"}, {[]string{"pr", "create", "--label", "bug, "}, "label names cannot be empty"}, - {[]string{"pr", "create", "--milestone-name="}, "--milestone-name cannot be empty"}, - {[]string{"pr", "create", "--milestone-name=Release", "--milestone=1"}, "--milestone and --milestone-name are mutually exclusive"}, {[]string{"pr", "create", "--agit", "--json", "--title", "x", "--no-input"}, "--json/--jq cannot be combined with --agit"}, {[]string{"pr", "create", "--topic", "x", "--title", "x", "--no-input"}, "--topic requires --agit"}, {[]string{"pr", "create", "--reviewer", "bob, "}, "reviewer names cannot be empty"}, @@ -120,8 +118,9 @@ func TestUsageErrorsPrecedeRequests(t *testing.T) { {[]string{"pr", "create", "--agit", "--remote", "origin", "--reviewer-team", "core"}, "--reviewer-team cannot be combined with --agit"}, {[]string{"pr", "create", "--agit", "--remote", "origin", "--attachment", "log.txt"}, "--attachment cannot be combined with --agit"}, {[]string{"pr", "create", "--attachment", "."}, ". is not a regular file"}, - {[]string{"pr", "edit", "7"}, "supply an edit flag"}, - {[]string{"pr", "edit", "7", "--remove-assignees="}, "supply an edit flag"}, + {[]string{"pr", "edit", "7"}, "nothing to edit; pass a flag such as --title (see tea pr edit --help)"}, + {[]string{"pr", "edit", "7", "--remove-assignees="}, "nothing to edit"}, + {[]string{"pr", "edit", "7", "--milestone", "1", "--remove-milestone"}, "--milestone and --remove-milestone are mutually exclusive"}, {[]string{"pr", "edit", "7", "--draft", "--ready"}, "--draft and --ready are mutually exclusive"}, {[]string{"pr", "edit", "7", "--title="}, "pull request title cannot be empty"}, {[]string{"pr", "edit", "7", "--base="}, "--base cannot be empty"}, diff --git a/internal/git/checkout.go b/internal/git/checkout.go index e7eab6f6..c2ba3718 100644 --- a/internal/git/checkout.go +++ b/internal/git/checkout.go @@ -111,7 +111,7 @@ func CheckoutPull(ctx context.Context, runner Runner, options CheckoutOptions) ( tracking = options.HeadRemote + "/" + options.HeadBranch } fetchArgs := append([]string{}, options.CredentialArgs...) - fetchArgs = append(fetchArgs, "fetch", "--no-tags", "--", fetchRemote, fetchRef) + fetchArgs = append(fetchArgs, "fetch", "--quiet", "--no-tags", "--", fetchRemote, fetchRef) if _, err := run(fetchArgs...); err != nil { return CheckoutResult{}, err } @@ -136,11 +136,11 @@ func CheckoutPull(ctx context.Context, runner Runner, options CheckoutOptions) ( } } } - switchArgs := []string{"switch", "--detach", commit} + switchArgs := []string{"switch", "--quiet", "--detach", commit} if !options.Detached { - switchArgs = []string{"switch", "--create", options.Branch, commit} + switchArgs = []string{"switch", "--quiet", "--create", options.Branch, commit} if existing { - switchArgs = []string{"switch", options.Branch} + switchArgs = []string{"switch", "--quiet", options.Branch} } } if _, err := run(switchArgs...); err != nil { diff --git a/internal/git/checkout_test.go b/internal/git/checkout_test.go index 35495973..54a32965 100644 --- a/internal/git/checkout_test.go +++ b/internal/git/checkout_test.go @@ -68,7 +68,7 @@ func TestPullCheckoutValidatesBeforeChangingGitState(t *testing.T) { result, err := CheckoutPull(context.Background(), runner, options) require.NoError(t, err) require.Equal(t, CheckoutResult{Branch: "pr/7", Commit: "abc123", Remote: "origin"}, result) - require.Equal(t, []string{"rev-parse --is-inside-work-tree", "check-ref-format --branch pr/7", "status --porcelain=v1 --untracked-files=normal", "for-each-ref --format=%(refname) refs/heads/", "fetch --no-tags -- origin refs/pull/7/head", "rev-parse --verify FETCH_HEAD^{commit}", "switch --create pr/7 abc123", "config branch.pr/7.tea-pr 7", "config branch.pr/7.tea-base-remote origin"}, runner.calls) + require.Equal(t, []string{"rev-parse --is-inside-work-tree", "check-ref-format --branch pr/7", "status --porcelain=v1 --untracked-files=normal", "for-each-ref --format=%(refname) refs/heads/", "fetch --quiet --no-tags -- origin refs/pull/7/head", "rev-parse --verify FETCH_HEAD^{commit}", "switch --quiet --create pr/7 abc123", "config branch.pr/7.tea-pr 7", "config branch.pr/7.tea-base-remote origin"}, runner.calls) } func TestPullCheckoutRejectsChangedHead(t *testing.T) { @@ -89,7 +89,7 @@ func TestPullCheckoutScopesCredentialArgumentsToFetch(t *testing.T) { for _, call := range runner.calls { if strings.HasPrefix(call, "-c ") { credentialCalls++ - require.Equal(t, "-c credential.https://example.test.helper=!tea-helper fetch --no-tags -- origin refs/pull/7/head", call) + require.Equal(t, "-c credential.https://example.test.helper=!tea-helper fetch --quiet --no-tags -- origin refs/pull/7/head", call) } } require.Equal(t, 1, credentialCalls) @@ -101,7 +101,7 @@ func TestDetachedCheckoutDoesNotCreateOrChangeBranches(t *testing.T) { result, err := CheckoutPull(context.Background(), runner, CheckoutOptions{Directory: "/work", Remote: "origin", Number: 7, Detached: true, ExpectedCommit: "abc123"}) require.NoError(t, err) require.Equal(t, CheckoutResult{Detached: true, Commit: "abc123", Remote: "origin"}, result) - require.Equal(t, "switch --detach abc123", runner.calls[len(runner.calls)-1]) + require.Equal(t, "switch --quiet --detach abc123", runner.calls[len(runner.calls)-1]) for _, call := range runner.calls { require.False(t, strings.HasPrefix(call, "config ")) require.False(t, strings.HasPrefix(call, "for-each-ref")) @@ -127,7 +127,7 @@ func TestPullCheckoutReusesMatchingBranch(t *testing.T) { require.NoError(t, err) require.Equal(t, "abc123", result.Commit) require.Contains(t, runner.calls, "rev-parse --verify refs/heads/pr/7^{commit}") - require.Equal(t, "switch pr/7", runner.calls[len(runner.calls)-3]) + require.Equal(t, "switch --quiet pr/7", runner.calls[len(runner.calls)-3]) runner = &checkoutRunner{collision: true} _, err = CheckoutPull(context.Background(), runner, CheckoutOptions{Directory: "/work", Remote: "origin", Number: 7, Branch: "pr/7", ExpectedCommit: "different"}) require.ErrorContains(t, err, "already exists with a different commit") @@ -150,7 +150,7 @@ func TestPullCheckoutUpdatesOnlyByFastForward(t *testing.T) { } } else { require.NoError(t, err) - require.Equal(t, []string{"switch pr/7", "merge --ff-only --no-edit abc123"}, runner.calls[len(runner.calls)-4:len(runner.calls)-2]) + require.Equal(t, []string{"switch --quiet pr/7", "merge --ff-only --no-edit abc123"}, runner.calls[len(runner.calls)-4:len(runner.calls)-2]) } } } @@ -164,7 +164,7 @@ func TestPullCheckoutTracksHeadBranch(t *testing.T) { require.NoError(t, err) require.Equal(t, CheckoutResult{Branch: "pr/7", Commit: "abc123", Remote: "pulls/bob", Tracking: "pulls/bob/feature"}, result) require.Contains(t, runner.calls, "check-ref-format --branch feature") - require.Contains(t, runner.calls, "fetch --no-tags -- pulls/bob refs/heads/feature") + require.Contains(t, runner.calls, "fetch --quiet --no-tags -- pulls/bob refs/heads/feature") require.Contains(t, runner.calls, "config branch.pr/7.remote pulls/bob") require.Contains(t, runner.calls, "config branch.pr/7.merge refs/heads/feature") require.Equal(t, []string{"config branch.pr/7.tea-pr 7", "config branch.pr/7.tea-base-remote origin"}, runner.calls[len(runner.calls)-2:]) diff --git a/internal/git/clean.go b/internal/git/clean.go index d78c74a1..4706ad5b 100644 --- a/internal/git/clean.go +++ b/internal/git/clean.go @@ -185,7 +185,7 @@ func CleanPull(ctx context.Context, runner Runner, opts CleanOptions) (CleanResu } current, err := run("symbolic-ref", "--short", "HEAD") if err == nil && current == opts.LocalBranch { - if _, err := run("switch", "--", opts.DefaultBranch); err != nil { + if _, err := run("switch", "--quiet", "--", opts.DefaultBranch); err != nil { return CleanResult{}, fmt.Errorf("could not switch to %s before deleting %s: %w", opts.DefaultBranch, opts.LocalBranch, err) } } diff --git a/internal/git/clean_test.go b/internal/git/clean_test.go index 79f200e2..96cf21c0 100644 --- a/internal/git/clean_test.go +++ b/internal/git/clean_test.go @@ -122,7 +122,7 @@ func TestCleanPullDeletesLocalAndRemoteBranches(t *testing.T) { }) require.NoError(t, err) require.Equal(t, CleanResult{LocalBranch: "feature", Remote: "origin", RemoteBranch: "feature", DefaultBranch: "main", RemoteDeleted: true}, result) - require.Contains(t, runner.calls, "switch -- main") + require.Contains(t, runner.calls, "switch --quiet -- main") require.Contains(t, runner.calls, "branch -D -- feature") require.Contains(t, runner.calls, "-c credential.helper=tea push --delete -- origin feature") } diff --git a/internal/services/pulls.go b/internal/services/pulls.go index d3daefeb..2a32361f 100644 --- a/internal/services/pulls.go +++ b/internal/services/pulls.go @@ -5,6 +5,7 @@ package services import ( "bytes" + "cmp" "context" "errors" "fmt" @@ -369,10 +370,79 @@ func (s Pulls) Diff(ctx context.Context, n int64) (io.ReadCloser, error) { return io.NopCloser(bytes.NewReader(diff)), nil } -// MergeOptions supplies merge strategy and optional commit text. +// DeleteHead deletes the head branch of a pull request in its head repository. +func (s Pulls) DeleteHead(ctx context.Context, item Pull) error { + if item.HeadRemoteDeleted || item.Head == "" { + return nil + } + repo := cmp.Or(item.HeadRepo, s.Repository) + _, err := repoCall(ctx, s.Client, repo, func(ctx context.Context, c *gitea.Client, owner, name string) (bool, *gitea.Response, error) { + return c.Repositories.DeleteRepoBranch(ctx, owner, name, item.Head) + }) + return err +} + +// Files lists the paths of the files a pull request changes. +func (s Pulls) Files(ctx context.Context, n int64) ([]string, error) { + files, err := listAll(func(page int) ([]*gitea.ChangedFile, error) { + return repoCall(ctx, s.Client, s.Repository, func(ctx context.Context, c *gitea.Client, owner, name string) ([]*gitea.ChangedFile, *gitea.Response, error) { + return c.PullRequests.ListPullRequestFiles(ctx, owner, name, n, gitea.ListPullRequestFilesOptions{ListOptions: listOptions(100, page)}) + }) + }) + if err != nil { + return nil, err + } + paths := make([]string, 0, len(files)) + for _, file := range files { + if file != nil { + paths = append(paths, file.Filename) + } + } + return paths, nil +} + +// MergeOptions supplies merge strategy and optional commit text. Auto merges once the required +// checks succeed instead of now. type MergeOptions struct { Method, Title, Message string - DeleteBranch bool + DeleteBranch, Auto bool +} + +// MergeSettings are the merge styles a repository allows, in Gitea's order, and its merge defaults. +type MergeSettings struct { + Styles []string + Default string + DeleteBranch bool +} + +// MergeSettings reads the merge styles the repository allows and its default style and branch +// deletion. A default style that is not allowed falls back to the first allowed one. +func (s Pulls) MergeSettings(ctx context.Context) (MergeSettings, error) { + repo, err := repoCall(ctx, s.Client, s.Repository, func(ctx context.Context, c *gitea.Client, owner, name string) (*gitea.Repository, *gitea.Response, error) { + return c.Repositories.GetRepo(ctx, owner, name) + }) + if err != nil { + return MergeSettings{}, err + } + settings := MergeSettings{Default: string(repo.DefaultMergeStyle), DeleteBranch: repo.DefaultDeleteBranchAfterMerge} + for _, style := range []struct { + name gitea.MergeStyle + allowed bool + }{ + {gitea.MergeStyleMerge, repo.AllowMerge}, + {gitea.MergeStyleRebase, repo.AllowRebase}, + {gitea.MergeStyleRebaseMerge, repo.AllowRebaseMerge}, + {gitea.MergeStyleSquash, repo.AllowSquash}, + {gitea.MergeStyleFastForwardOnly, repo.AllowFastForwardOnlyMerge}, + } { + if style.allowed { + settings.Styles = append(settings.Styles, string(style.name)) + } + } + if !slices.Contains(settings.Styles, settings.Default) && len(settings.Styles) > 0 { + settings.Default = settings.Styles[0] + } + return settings, nil } // Merge merges the pull request's current head with the selected strategy; Gitea refuses the @@ -385,6 +455,7 @@ func (s Pulls) Merge(ctx context.Context, n int64, opts MergeOptions) error { } option := gitea.MergePullRequestOption{ Style: gitea.MergeStyle(opts.Method), Title: opts.Title, Message: opts.Message, DeleteBranchAfterMerge: &opts.DeleteBranch, + MergeWhenChecksSucceed: opts.Auto, } if pull.Head != nil { option.HeadCommitId = pull.Head.Sha diff --git a/internal/terminal/merge_form.go b/internal/terminal/merge_form.go index 62b451f4..22973c9f 100644 --- a/internal/terminal/merge_form.go +++ b/internal/terminal/merge_form.go @@ -17,9 +17,35 @@ type MergePullChoice struct { Title, Head string } -// MergeForm collects which pull request to merge. +// MergeMethodLabels describe Gitea's merge styles in the merge form. +var MergeMethodLabels = map[string]string{ + "merge": "Create a merge commit", "rebase": "Rebase and merge", "rebase-merge": "Rebase and create a merge commit", + "squash": "Squash and merge", "fast-forward-only": "Fast-forward only", +} + +// MergeFormChoices describes the pull request to merge and what the form asks about it. +type MergeFormChoices struct { + // Header introduces the form, such as `Merge #3 "Merge me" (feat/merge-me → main) in alice/x`. + Header string + // Methods are the merge styles the repository allows; Method preselects one. Without Methods, + // the form does not ask for one. + Methods []string + Method string + // DeleteBranch preselects whether to delete the head branch; Branch, when set, asks it. + DeleteBranch bool + Branch string +} + +// MergeFormValues are the chosen merge style and whether to delete the head branch. +type MergeFormValues struct { + Method string + DeleteBranch bool +} + +// MergeForm collects which pull request to merge, and how. type MergeForm interface { SelectPull(context.Context, []MergePullChoice) (int64, error) + MergeOptions(context.Context, MergeFormChoices) (MergeFormValues, error) } // SelectPull prompts for a pull request on stderr. @@ -29,11 +55,34 @@ func (p *FormPrompter) SelectPull(ctx context.Context, choices []MergePullChoice } options := make([]huh.Option[int64], 0, len(choices)) for _, choice := range choices { - options = append(options, huh.NewOption(fmt.Sprintf("#%d: %s (%s)", choice.Number, choice.Title, choice.Head), choice.Number)) + options = append(options, huh.NewOption(SafeLine(fmt.Sprintf("#%d %s (%s)", choice.Number, choice.Title, choice.Head)), choice.Number)) } selected := choices[0].Number err := p.runForm(ctx, huh.NewForm(huh.NewGroup( - huh.NewSelect[int64]().Title("Select a PR to merge:").Options(options...).Value(&selected), + huh.NewSelect[int64]().Title("Pull request to merge:").Options(options...).Value(&selected), ))) return selected, err } + +// MergeOptions asks for the merge style and whether to delete the head branch, on stderr. +func (p *FormPrompter) MergeOptions(ctx context.Context, choices MergeFormChoices) (MergeFormValues, error) { + values := MergeFormValues{Method: choices.Method, DeleteBranch: choices.DeleteBranch} + var fields []huh.Field + if len(choices.Methods) > 0 { + options := make([]huh.Option[string], len(choices.Methods)) + for i, method := range choices.Methods { + options[i] = huh.NewOption(MergeMethodLabels[method], method) + } + fields = append(fields, huh.NewSelect[string]().Title("Merge method:").Options(options...).Value(&values.Method)) + } + if choices.Branch != "" { + fields = append(fields, huh.NewConfirm().Title(SafeLine(fmt.Sprintf("Delete branch %s locally and on Gitea?", choices.Branch))).Value(&values.DeleteBranch)) + } + if choices.Header != "" { + fmt.Fprintln(p.Streams.ErrOut, SafeLine(choices.Header)) + } + if len(fields) == 0 { + return values, nil + } + return values, p.runForm(ctx, huh.NewForm(huh.NewGroup(fields...))) +} diff --git a/internal/terminal/pull_form.go b/internal/terminal/pull_form.go index 8580d2b9..9c2b018b 100644 --- a/internal/terminal/pull_form.go +++ b/internal/terminal/pull_form.go @@ -5,6 +5,7 @@ package terminal import ( "context" + "fmt" "strings" "unicode" "unicode/utf8" @@ -14,6 +15,8 @@ import ( // PullFormChoices supplies defaults and selectable options for interactive PR create. type PullFormChoices struct { + // Repo is the base repository, named in the form's header. + Repo string BaseOwner, Base, HeadOwner, HeadBranch string // Title and Body seed the form; Title applies only while HeadBranch is kept. Title, Body string @@ -21,8 +24,9 @@ type PullFormChoices struct { Assignees []string Milestones []IssueMilestoneChoice Labels []IssueLabelChoice - CanPush bool - EditBody BodyEditor + // CanPush offers metadata, which needs push access; without it, changing branches offers AGit. + CanPush bool + EditBody BodyEditor } // PullFormValues holds interactively collected pull request create fields. @@ -31,6 +35,8 @@ type PullFormValues struct { Agit bool Base, Head, Topic string AllowMaintainerEdits bool + // AsDraft submits the pull request as a draft. + AsDraft bool } // PullForm collects interactive pull create input. @@ -38,69 +44,130 @@ type PullForm interface { CreatePull(context.Context, PullFormChoices) (PullFormValues, error) } -// CreatePull runs the interactive pull create form on stderr. +// pullNext is what the user does after writing a pull request's title and body. +type pullNext int + +const ( + pullSubmit pullNext = iota + pullSubmitDraft + pullMetadata + pullBranches + pullCancel +) + +// CreatePull runs the interactive pull create form on stderr, like gh pr create: the title and +// body of a pull request from the current branch, then "What's next?" to submit it, submit it as a +// draft, add metadata, change the branches or cancel. Branches are asked first when unknown. func (p *FormPrompter) CreatePull(ctx context.Context, choices PullFormChoices) (PullFormValues, error) { - agit, err := p.Confirm(ctx, "Do you want to create an agit flow pull request?") - if err != nil { - return PullFormValues{}, err - } - - base := choices.Base - headOwner := choices.HeadOwner - headBranch := choices.HeadBranch - topic := headBranch - allowMaintainerEdits := choices.AllowMaintainerEdits - - fields := []huh.Field{ - huh.NewInput().Title("Target branch (required):").Value(&base).Validate(huh.ValidateNotEmpty()), - } - if agit { - fields = append(fields, huh.NewInput().Title("Topic branch (required):").Value(&topic).Validate(huh.ValidateNotEmpty())) - } else { - headField := huh.NewInput().Title("Source branch:").Value(&headBranch) - if strings.TrimSpace(headBranch) == "" { - headField = headField.Title("Source branch (required):").Validate(huh.ValidateNotEmpty()) + branches := pullBranchFields{base: choices.Base, headOwner: choices.HeadOwner, headBranch: choices.HeadBranch, allowEdits: choices.AllowMaintainerEdits} + branches.topic = branches.headBranch + if strings.TrimSpace(branches.base) == "" || strings.TrimSpace(branches.headBranch) == "" { + if err := p.runForm(ctx, huh.NewForm(huh.NewGroup(branches.fields(!choices.CanPush)...))); err != nil { + return PullFormValues{}, err } - fields = append(fields, - huh.NewInput().Title("Source repo owner:").Value(&headOwner), - headField, - huh.NewConfirm().Title("Allow maintainers to push to the source branch:").Value(&allowMaintainerEdits), - ) - } - if err := p.runForm(ctx, huh.NewForm(huh.NewGroup(fields...))); err != nil { - return PullFormValues{}, err } + fmt.Fprintln(p.Streams.ErrOut, SafeLine(branches.header(choices.Repo, choices.BaseOwner))) - title := DefaultPullTitle(headBranch) - if choices.Title != "" && headBranch == choices.HeadBranch { + title := DefaultPullTitle(branches.headBranch) + if choices.Title != "" && branches.headBranch == choices.HeadBranch { title = choices.Title } - // AGit pushes carry only a title and description. - properties, err := p.collectIssueProperties(ctx, title, choices.Body, IssueFormChoices{ - Assignees: choices.Assignees, Milestones: choices.Milestones, Labels: choices.Labels, CanPush: choices.CanPush && !agit, - EditBody: choices.EditBody, - }) - if err != nil { - return PullFormValues{IssueFormValues: properties}, err + fields := issueFields{title: title, body: choices.Body} + content := []huh.Field{titleField(&fields.title)} + if choices.EditBody == nil { + content = append(content, bodyField(BodyTitle, &fields.body)) } + if err := p.runForm(ctx, huh.NewForm(huh.NewGroup(content...))); err != nil { + return PullFormValues{}, err + } + values := PullFormValues{} + var err error + if values.Body, values.Draft, err = editBody(ctx, choices.EditBody, fields.body); err != nil { + return values, err + } + for next := pullMetadata; next == pullMetadata || next == pullBranches; { + metadata := choices.CanPush && !branches.agit && (len(choices.Assignees)+len(choices.Milestones)+len(choices.Labels) > 0) + options := []huh.Option[pullNext]{huh.NewOption("Submit", pullSubmit)} + if !branches.agit { + options = append(options, huh.NewOption("Submit as draft", pullSubmitDraft)) + } + if metadata { + options = append(options, huh.NewOption("Add metadata (assignees, labels, milestone, due date)", pullMetadata)) + } + options = append(options, huh.NewOption("Change branches", pullBranches), huh.NewOption("Cancel", pullCancel)) + next = pullSubmit + if err := p.runForm(ctx, huh.NewForm(huh.NewGroup(huh.NewSelect[pullNext]().Title("What's next?").Options(options...).Value(&next)))); err != nil { + return values, err + } + switch next { + case pullMetadata: + groups := fields.groups(noBody, true, choices.Assignees, choices.Milestones, choices.Labels)[1:] + if err := p.runForm(ctx, huh.NewForm(groups...)); err != nil { + return values, err + } + case pullBranches: + if err := p.runForm(ctx, huh.NewForm(huh.NewGroup(branches.fields(!choices.CanPush)...))); err != nil { + return values, err + } + fmt.Fprintln(p.Streams.ErrOut, SafeLine(branches.header(choices.Repo, choices.BaseOwner))) + case pullCancel: + return values, ErrCanceled + } + values.AsDraft = next == pullSubmitDraft + } + values.Title, values.Assignees, values.Milestone, values.Labels = strings.TrimSpace(fields.title), fields.assignees, fields.milestone, fields.labels + if values.Deadline, err = parseDueDate(fields.deadline); err != nil { + return values, err + } + values.Agit, values.Base, values.Topic, values.AllowMaintainerEdits = branches.agit, strings.TrimSpace(branches.base), strings.TrimSpace(branches.topic), branches.allowEdits + values.Head = branches.head(choices.BaseOwner) + return values, nil +} - headBranch = strings.TrimSpace(headBranch) - headOwner = strings.TrimSpace(headOwner) - head := headBranch - switch { - case agit && head == "": - head = "HEAD" - case !agit && headOwner != "" && headOwner != choices.BaseOwner: - head = headOwner + ":" + headBranch +// noBody keeps the body out of issueFields.groups, which the pull request form asks separately. +func noBody(context.Context, string) (Draft, error) { return Draft{}, nil } + +// pullBranchFields are the branches of a new pull request. +type pullBranchFields struct { + base, headOwner, headBranch, topic string + allowEdits, agit bool +} + +// fields asks for the branches; offerAgit adds the choice of an AGit push for users who cannot +// push to the base repository. +func (b *pullBranchFields) fields(offerAgit bool) []huh.Field { + fields := []huh.Field{huh.NewInput().Title("Base branch (required):").Value(&b.base).Validate(huh.ValidateNotEmpty())} + if offerAgit { + fields = append(fields, huh.NewConfirm().Title("Push through AGit (refs/for) instead of a branch of your own?").Value(&b.agit)) } - return PullFormValues{ - IssueFormValues: properties, - Agit: agit, - Base: strings.TrimSpace(base), - Head: head, - Topic: strings.TrimSpace(topic), - AllowMaintainerEdits: allowMaintainerEdits, - }, nil + return append(fields, + huh.NewInput().Title("Head repository owner:").Value(&b.headOwner), + huh.NewInput().Title("Head branch (required):").Value(&b.headBranch).Validate(huh.ValidateNotEmpty()), + huh.NewInput().Title("AGit topic (when pushing through AGit):").Value(&b.topic), + huh.NewConfirm().Title("Allow maintainers to push to the head branch?").Value(&b.allowEdits), + ) +} + +// head is the head of the pull request: BRANCH, OWNER:BRANCH for a head in another repository, or +// HEAD for an AGit push without a branch. +func (b *pullBranchFields) head(baseOwner string) string { + branch, owner := strings.TrimSpace(b.headBranch), strings.TrimSpace(b.headOwner) + switch { + case b.agit && branch == "": + return "HEAD" + case !b.agit && owner != "" && owner != baseOwner: + return owner + ":" + branch + } + return branch +} + +// header introduces the pull request: "Creating pull request for feat/one into main in alice/x". +func (b *pullBranchFields) header(repo, baseOwner string) string { + text := fmt.Sprintf("Creating pull request for %s into %s", b.head(baseOwner), strings.TrimSpace(b.base)) + if repo != "" { + text += " in " + repo + } + return text } // DefaultPullTitle turns a branch-like head into a readable pull title seed. diff --git a/internal/terminal/pull_form_test.go b/internal/terminal/pull_form_test.go index bbafaf17..df043f8d 100644 --- a/internal/terminal/pull_form_test.go +++ b/internal/terminal/pull_form_test.go @@ -7,7 +7,6 @@ import ( "context" "errors" "path/filepath" - "sync" "testing" "time" @@ -26,73 +25,73 @@ func TestDefaultPullTitle(t *testing.T) { } } -func TestPullFormMarksRequiredFields(t *testing.T) { +func TestPullFormAsksTitleBodyThenWhatsNext(t *testing.T) { + known := PullFormChoices{Repo: "alice/x", BaseOwner: "alice", Base: "main", HeadOwner: "alice", HeadBranch: "feature", Title: "Seeded", Body: "Seeded body", AllowMaintainerEdits: true} for _, tc := range []struct { - name, input string - choices PullFormChoices - want, without []string + name, input string + choices PullFormChoices + want PullFormValues + err error + rendered []string + hidden []string }{ { - "without head", "n\nmain\nalice\nfeature\ny\nTitle\n", - PullFormChoices{}, - []string{"Target branch (required):", "Source branch (required):", "Title (required):"}, - nil, + name: "submit", input: "Seeded\n\n1\n", choices: known, + want: PullFormValues{IssueFormValues: IssueFormValues{Title: "Seeded", Body: "Seeded body"}, Base: "main", Head: "feature", Topic: "feature", AllowMaintainerEdits: true}, + rendered: []string{"Creating pull request for feature into main in alice/x", "Title (required):", BodyTitle, "What's next?", "Change branches"}, + hidden: []string{"Base branch", "AGit", "Issue", "Add metadata"}, }, { - "with head", "n\n\n\n\n\nTitle\n", - PullFormChoices{Base: "main", HeadOwner: "alice", HeadBranch: "feature"}, - []string{"Source branch:", "Allow maintainers to push to the source branch:"}, - []string{"Source branch (required)"}, + name: "submit as draft", input: "Add login\n\n2\n", choices: known, + want: PullFormValues{IssueFormValues: IssueFormValues{Title: "Add login", Body: "Seeded body"}, Base: "main", Head: "feature", Topic: "feature", AllowMaintainerEdits: true, AsDraft: true}, + }, + {name: "cancel", input: "Seeded\n\n4\n", choices: known, err: ErrCanceled}, + { + name: "unknown head asks the branches first", input: "main\nn\nbob\nfix-typo\n\n\nFix Typo\n\n1\n", choices: PullFormChoices{Repo: "alice/x", BaseOwner: "alice", Base: "main"}, + want: PullFormValues{IssueFormValues: IssueFormValues{Title: "Fix Typo"}, Base: "main", Head: "bob:fix-typo"}, + rendered: []string{"Base branch (required):", "Push through AGit", "Head branch (required):", "Creating pull request for bob:fix-typo into main in alice/x"}, + }, + { + name: "metadata", input: "Seeded\n\n3\n2\n2026-12-24\n1\n", choices: func() PullFormChoices { + c := known + c.CanPush, c.Milestones = true, []IssueMilestoneChoice{{Title: "v1.0", ID: 4}} + return c + }(), + want: PullFormValues{IssueFormValues: IssueFormValues{Title: "Seeded", Body: "Seeded body", Milestone: 4, Deadline: mustDueDate(t, "2026-12-24")}, Base: "main", Head: "feature", Topic: "feature", AllowMaintainerEdits: true}, + rendered: []string{"Add metadata (assignees, labels, milestone, due date)", "Milestone:", "Due date:"}, }, - {"agit", "y\nmain\ntopic\nTitle\n", PullFormChoices{}, []string{"Topic branch (required):"}, nil}, } { t.Run(tc.name, func(t *testing.T) { - p, rendered := accessiblePrompter(t, tc.input) - tc.choices.EditBody = noEdit - _, err := p.CreatePull(context.Background(), tc.choices) + p, rendered := linePrompter(t, tc.input) + values, err := p.CreatePull(context.Background(), tc.choices) + if tc.err != nil { + require.ErrorIs(t, err, tc.err) + return + } require.NoError(t, err) - for _, want := range tc.want { + require.Equal(t, tc.want, values) + for _, want := range tc.rendered { require.Contains(t, rendered.String(), want) } - for _, unwanted := range tc.without { + for _, unwanted := range tc.hidden { require.NotContains(t, rendered.String(), unwanted) } }) } } -// stagedInput feeds one form's keystrokes per stage. A finished form's canceled read loop can still be inside Read, -// and bubbletea drops the byte that Read returns. When a stage runs dry, the finished form's loop blocks first and -// the next form's loop second, so the next stage is handed to that second reader and never to the stale one. -type stagedInput struct { - mu sync.Mutex - cur string - stages []string - idle int -} - -func (s *stagedInput) Read(p []byte) (int, error) { - s.mu.Lock() - for s.cur == "" { - s.idle++ - if s.idle < 2 || len(s.stages) == 0 { - s.mu.Unlock() - select {} // no more input for this reader; the loop is abandoned with its form - } - s.cur, s.stages, s.idle = s.stages[0], s.stages[1:], 0 - } - p[0], s.cur = s.cur[0], s.cur[1:] - s.mu.Unlock() - return 1, nil +func mustDueDate(t *testing.T, raw string) *time.Time { + t.Helper() + due, err := parseDueDate(raw) + require.NoError(t, err) + return due } func TestCreatePullPreservesDraftOnEditorFailure(t *testing.T) { - ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) - defer cancel() path := filepath.Join(t.TempDir(), "draft.md") called := false - p, _ := testPrompter(&stagedInput{cur: "n\r", stages: []string{"main\ralice\rfeature\ry\r", "Title\r"}}) - values, err := p.CreatePull(ctx, PullFormChoices{ + p, _ := linePrompter(t, "Title\n") + values, err := p.CreatePull(context.Background(), PullFormChoices{ BaseOwner: "alice", Base: "main", HeadOwner: "alice", HeadBranch: "feature", EditBody: func(context.Context, string) (Draft, error) { called = true