diff --git a/internal/app/app.go b/internal/app/app.go index d51741b3..4a05a176 100644 --- a/internal/app/app.go +++ b/internal/app/app.go @@ -361,12 +361,21 @@ func builtins(root *cobra.Command) map[string]bool { // Execute expands aliases and executes the fresh root without printing errors. func Execute(ctx context.Context, deps Dependencies, args []string) error { root := NewRoot(deps) - if at := commandIndex(root, args); at == len(args) || (args[at] != cobra.ShellCompRequestCmd && args[at] != cobra.ShellCompNoDescRequestCmd) { - args = joinJSONFields(root, args) - } // Broken settings or aliases must not prevent help or recovery commands. snapshot, loadErr := deps.Settings.Load() names := builtins(root) + if loadErr == nil { + for name, expansion := range snapshot.Preferences.Aliases { + if aliases.Check(name, snapshot.Preferences.Aliases, names) == nil { + root.AddCommand(&cobra.Command{Use: name, Short: fmt.Sprintf("Alias for %q", expansion), GroupID: "alias", Annotations: map[string]string{"tea.alias": "true"}, RunE: func(*cobra.Command, []string) error { + return clierr.Wrap(clierr.Usage, errors.New("alias must be expanded before execution")) + }}) + } + } + } + if at := commandIndex(root, args); at == len(args) || (args[at] != cobra.ShellCompRequestCmd && args[at] != cobra.ShellCompNoDescRequestCmd) { + args = joinJSONFields(root, args) + } if commandAt := commandIndex(root, args); commandAt < len(args) { name := args[commandAt] completion := name == cobra.ShellCompRequestCmd || name == cobra.ShellCompNoDescRequestCmd @@ -394,15 +403,6 @@ func Execute(ctx context.Context, deps Dependencies, args []string) error { args = completeJSONValue(args) } } - if loadErr == nil { - for name, expansion := range snapshot.Preferences.Aliases { - if aliases.Check(name, snapshot.Preferences.Aliases, names) == nil { - root.AddCommand(&cobra.Command{Use: name, Short: fmt.Sprintf("Alias for %q", expansion), GroupID: "alias", Annotations: map[string]string{"tea.alias": "true"}, RunE: func(*cobra.Command, []string) error { - return clierr.Wrap(clierr.Usage, errors.New("alias must be expanded before execution")) - }}) - } - } - } root.SetArgs(args) err := root.ExecuteContext(ctx) if apiErr, ok := errors.AsType[*transport.APIError](err); ok && apiErr.Status == 401 { diff --git a/internal/app/app_test.go b/internal/app/app_test.go index 75261b28..304934c0 100644 --- a/internal/app/app_test.go +++ b/internal/app/app_test.go @@ -84,6 +84,14 @@ func TestAliasesAfterGlobalFlagsAndNestedHelp(t *testing.T) { } } +func TestBareJSONBeforeAnAliasKeepsTheAlias(t *testing.T) { + deps, out, _ := testDeps(t) + require.NoError(t, Execute(context.Background(), deps, []string{"alias", "set", "prefs", "config list"})) + out.Reset() + require.NoError(t, Execute(context.Background(), deps, []string{"--json", "prefs"})) + require.Contains(t, out.String(), "git_protocol") +} + func TestJoinJSONFields(t *testing.T) { for _, tc := range []struct{ args, want []string }{ {[]string{"issue", "list", "--json", "number,title"}, []string{"issue", "list", "--json=number,title"}}, @@ -101,6 +109,7 @@ func TestJoinJSONFields(t *testing.T) { // A leading --json keeps the command word that follows it. {[]string{"--json", "repo", "view", "alice/project"}, []string{"--json", "repo", "view", "alice/project"}}, {[]string{"--json", "pr", "list"}, []string{"--json", "pr", "list"}}, + {[]string{"issue", "--json", "list"}, []string{"issue", "--json", "list"}}, {[]string{"--json", "number", "my-alias"}, []string{"--json=number", "my-alias"}}, // A word that cannot be an argument is a misspelled field, which then lists the available ones. {[]string{"issue", "list", "--json", "Number"}, []string{"issue", "list", "--json=Number"}}, diff --git a/internal/app/args.go b/internal/app/args.go index 68f87259..1f4dc34e 100644 --- a/internal/app/args.go +++ b/internal/app/args.go @@ -78,11 +78,14 @@ func oneOf(flag, value string, allowed ...string) error { // maxLimit bounds --limit; list services page through the server's smaller pages to reach it. const maxLimit = 1000 +// maxPage bounds --page so that the item offset of a page (page * limit) cannot overflow an int. +const maxPage = math.MaxInt / (2 * maxLimit) + // pageFlags registers -L/--limit and --page, which reject out-of-range values while parsing. func pageFlags(cmd *cobra.Command, limit, page *int) { *limit = 30 cmd.Flags().VarP(positiveInt{limit, maxLimit}, "limit", "L", "Maximum number of results (1..1000)") - positiveIntVar(cmd, page, "page", 1, math.MaxInt, "Result page") + positiveIntVar(cmd, page, "page", 1, maxPage, "Result page") } // positiveIntVar registers an int flag that rejects values outside 1..upper while parsing. @@ -103,7 +106,7 @@ func (f positiveInt) Type() string { return "int" } func (f positiveInt) Set(raw string) error { n, err := strconv.Atoi(raw) switch { - case (err != nil || n < 1) && f.upper == math.MaxInt: + case (err != nil || n < 1) && (f.upper == math.MaxInt || f.upper == maxPage): return errors.New("must be a positive integer") case err != nil || n < 1 || n > f.upper: return fmt.Errorf("must be 1..%d", f.upper) @@ -301,12 +304,25 @@ func argumentName(cmd *cobra.Command, argument string) string { return text } noun := group.Name() + // The numbers under `time` and `milestone issue` are issue numbers. + if noun == "time" || slices.ContainsFunc(ancestors(cmd), func(c *cobra.Command) bool { return c.Name() == "issue" }) { + noun = "issue" + } if noun == "pr" { noun = "pull request" } return noun + " " + text } +// ancestors returns the parents of cmd below the root command. +func ancestors(cmd *cobra.Command) []*cobra.Command { + var parents []*cobra.Command + for parent := cmd.Parent(); parent != nil && parent.HasParent(); parent = parent.Parent() { + parents = append(parents, parent) + } + return parents +} + // localFlagsChanged reports whether a command-specific flag other than those in except was supplied. func localFlagsChanged(cmd *cobra.Command, except ...string) bool { changed := false diff --git a/internal/app/args_test.go b/internal/app/args_test.go index 233f23f2..d1ae1689 100644 --- a/internal/app/args_test.go +++ b/internal/app/args_test.go @@ -4,6 +4,8 @@ package app import ( + "math" + "strconv" "testing" "time" @@ -93,6 +95,9 @@ func TestArgumentErrorsShowTheUsageLine(t *testing.T) { {[]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{"time", "create"}, "missing issue number\nUsage: tea time create { | } [flags]"}, + {[]string{"milestone", "issue", "add", "v1.0"}, "missing issue number\nUsage: tea milestone issue add { | }... [flags]"}, + {[]string{"branch", "protect", "main", "--pattern", "release/*"}, "--pattern cannot be combined with branch names\nUsage: tea branch protect ... [flags]"}, {[]string{"repo", "clone"}, "missing repository\nUsage: tea repo clone [] [-- ...] [flags]"}, } { requireUsageBeforeRequests(t, tc.want, tc.args...) @@ -108,3 +113,12 @@ func TestLimitAcceptsShorthandUpToOneThousand(t *testing.T) { require.Equal(t, 1000, limit) require.ErrorContains(t, cmd.Flags().Parse([]string{"-L", "1001"}), "must be 1..1000") } + +func TestPageRejectsValuesThatOverflowTheOffset(t *testing.T) { + var limit, page int + cmd := &cobra.Command{Use: "list"} + pageFlags(cmd, &limit, &page) + require.NoError(t, cmd.Flags().Parse([]string{"--page", strconv.Itoa(maxPage)})) + require.Equal(t, maxPage, page) + require.ErrorContains(t, cmd.Flags().Parse([]string{"--page", strconv.Itoa(math.MaxInt)}), "--page") +} diff --git a/internal/app/auth.go b/internal/app/auth.go index b53dc474..4a68aebd 100644 --- a/internal/app/auth.go +++ b/internal/app/auth.go @@ -10,6 +10,7 @@ import ( "fmt" "io" "maps" + "net/http" "os" "path/filepath" "slices" @@ -22,6 +23,7 @@ import ( "gitea.dev/tea/internal/services" "gitea.dev/tea/internal/settings" "gitea.dev/tea/internal/terminal" + "gitea.dev/tea/internal/transport" "github.com/spf13/cobra" ) @@ -267,7 +269,7 @@ $ tea auth login --host gitea.com --oauth --scopes read:repository,read:user`, } user, err := services.CurrentAccount(cmd.Context(), a.deps.Client(instance, credential.AccessToken, insecure)) if err != nil { - return err + return rejectedLogin(err, "the token", "check it or create a new one under Settings > Applications") } if selectedUser, _ := a.selectedUser(); !services.MatchAccount(selectedUser, user) { return clierr.Wrap(clierr.Usage, errors.New("token account does not match --user or TEA_USER")) @@ -304,6 +306,15 @@ $ tea auth login --host gitea.com --oauth --scopes read:repository,read:user`, return login } +// rejectedLogin words a 401 for the credential that tea auth login was given, so that Explain does +// not blame GITEA_TOKEN or tell the user to run the command they are running. +func rejectedLogin(err error, credential, advice string) error { + if apiErr, ok := errors.AsType[*transport.APIError](err); ok && apiErr.Status == http.StatusUnauthorized { + return explain(err, "%s was rejected by %s; %s", credential, cmp.Or(apiErr.Instance, "the instance"), advice) + } + return err +} + // confirmReplace asks before a login replaces the stored credential of an account that tea already // has, and reports whether it does. func (a *application) confirmReplace(cmd *cobra.Command, instance, user string) (bool, error) { @@ -346,7 +357,7 @@ func (a *application) signedLogin(cmd *cobra.Command, snapshot settings.Snapshot client.Signer = signer user, err := services.CurrentAccount(cmd.Context(), client) if err != nil { - return account, "", err + return account, "", rejectedLogin(err, "the SSH signature", "check that the key is registered for your account") } if selectedUser, _ := a.selectedUser(); !services.MatchAccount(selectedUser, user) { return account, "", clierr.Wrap(clierr.Usage, errors.New("authenticated account does not match --user or TEA_USER")) diff --git a/internal/app/branches.go b/internal/app/branches.go index 1e4f65a1..9ab1564c 100644 --- a/internal/app/branches.go +++ b/internal/app/branches.go @@ -277,7 +277,10 @@ $ tea branch protect --pattern 'release/*' --allow-push acme/release`, Annotations: textOutput("branch protect does not return a resource"), Args: func(cmd *cobra.Command, args []string) error { if pattern != "" { - return usageArgs(cobra.NoArgs)(cmd, args) + if len(args) > 0 { + return clierr.Wrap(clierr.Usage, &argumentError{problem: "--pattern cannot be combined with branch names", cmd: cmd}) + } + return nil } return usageArgs(cobra.MinimumNArgs(1))(cmd, args) }, diff --git a/internal/app/deadline.go b/internal/app/deadline.go index fa9e641b..68a2c30f 100644 --- a/internal/app/deadline.go +++ b/internal/app/deadline.go @@ -16,7 +16,7 @@ import ( // numericDate matches dates such as 03/04/2026, which read as March 4 in the US and April 3 // elsewhere. -var numericDate = regexp.MustCompile(`^\d{1,2}[/.]\d{1,2}[/.]\d{2,4}$`) +var numericDate = regexp.MustCompile(`^\d{1,2}[/.]\d{1,2}[/.]\d{2,4}(?:[ T].*)?$`) // parseDeadline parses a --deadline value; an empty value requests clearing. // Supplied timezone offsets are kept and Unix timestamps are UTC. diff --git a/internal/app/deadline_test.go b/internal/app/deadline_test.go index 6299cabe..4e0feaa1 100644 --- a/internal/app/deadline_test.go +++ b/internal/app/deadline_test.go @@ -43,7 +43,7 @@ func TestDeadlineInputAndExplicitClearing(t *testing.T) { } func TestAmbiguousNumericDeadlinesAreRejected(t *testing.T) { - for _, value := range []string{"03/04/2026", "04/03/2026", "01/15/2028", "15.01.2028", "3/4/26"} { + for _, value := range []string{"03/04/2026", "04/03/2026", "01/15/2028", "15.01.2028", "3/4/26", "04/03/2026 17:00", "4/3/2026 17:00:00", "04/03/2026 5:00 PM"} { for _, command := range [][]string{{"issue", "edit", "1"}, {"issue", "create", "--title", "x"}, {"milestone", "create", "--title", "x"}} { requireUsageBeforeRequests(t, fmt.Sprintf("invalid --deadline %q: the order of day and month is ambiguous; use YYYY-MM-DD", value), append(command, "--deadline="+value)...) } diff --git a/internal/app/editor.go b/internal/app/editor.go index 884576a5..8b5637ca 100644 --- a/internal/app/editor.go +++ b/internal/app/editor.go @@ -26,10 +26,27 @@ func finishDraft(draft *terminal.Draft, result *error) { } } if *result != nil { - *result = fmt.Errorf("%w; draft saved at %s", *result, draft.Path) + *result = &draftError{err: *result, path: draft.Path} } } +// draftError reports where the text of a failed or canceled command was saved. +type draftError struct { + err error + path string +} + +func (e *draftError) Error() string { return fmt.Sprintf("%v; draft saved at %s", e.err, e.path) } +func (e *draftError) Unwrap() error { return e.err } + +// DraftPath returns the file that holds the unsubmitted text of a failed command, or "". +func DraftPath(err error) string { + if draft, ok := errors.AsType[*draftError](err); ok { + return draft.path + } + return "" +} + func (a *application) editor() (terminal.Editor, string, error) { preferences, err := a.preferences() if err != nil { diff --git a/internal/app/errors_test.go b/internal/app/errors_test.go index 3dc1d8de..99877910 100644 --- a/internal/app/errors_test.go +++ b/internal/app/errors_test.go @@ -9,6 +9,7 @@ import ( "fmt" "net/http" "net/http/httptest" + "strings" "testing" "time" @@ -129,3 +130,14 @@ func TestTimeoutErrorsNameTheWaitAndKeepTheExitCode(t *testing.T) { require.Equal(t, other, timeoutError(other, time.Second, "waiting")) require.EqualError(t, Explain(context.Background(), Dependencies{}, err), "timed out before the operation finished") } + +func TestRejectedLoginTokenIsNotBlamedOnTheEnvironment(t *testing.T) { + rejecting := fakeGitea(t, map[string][2]string{"GET /api/v1/user": {"401", `{"message":"token is invalid"}`}}) + deps, _, _ := testDeps(t) + deps.Getenv = envFunc(map[string]string{"GITEA_TOKEN": "valid-environment-token"}) + deps.Streams.In = strings.NewReader("mistyped-token\n") + err := Execute(context.Background(), deps, []string{"auth", "login", "--host", rejecting.URL, "--with-token", "--credential-store", "file", "--no-input"}) + explained := Explain(context.Background(), deps, err) + require.EqualError(t, explained, "the token was rejected by "+rejecting.URL+"; check it or create a new one under Settings > Applications") + require.Equal(t, clierr.Authentication, clierr.ExitCode(explained)) +} diff --git a/main.go b/main.go index c4393305..8bbe5630 100644 --- a/main.go +++ b/main.go @@ -18,6 +18,12 @@ import ( func main() { ctx, stop := signal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGTERM) + // Restore the default handling once the first signal arrived, so that a second Ctrl-C or SIGTERM + // ends tea even while it blocks on something that ignores ctx, such as reading stdin. + go func() { + <-ctx.Done() + stop() + }() code := run(ctx, app.System(), os.Args[1:]) stop() os.Exit(code) @@ -33,11 +39,21 @@ func run(ctx context.Context, deps app.Dependencies, args []string) int { } code := clierr.ExitCode(err) switch { - case err == nil, code == clierr.Interrupted, clierr.IsQuiet(err): + case err == nil, clierr.IsQuiet(err): + case code == clierr.Interrupted: + reportDraft(deps, err) case code == clierr.Canceled && !errors.Is(err, context.DeadlineExceeded): fmt.Fprintln(deps.Streams.ErrOut, "Canceled.") + reportDraft(deps, err) default: fmt.Fprintf(deps.Streams.ErrOut, "Error: %v\n", app.Explain(ctx, deps, err)) } return code } + +// reportDraft tells where the text of a canceled command was saved; the error is otherwise not shown. +func reportDraft(deps app.Dependencies, err error) { + if path := app.DraftPath(err); path != "" { + fmt.Fprintf(deps.Streams.ErrOut, "Draft saved at %s\n", path) + } +} diff --git a/main_test.go b/main_test.go index 845414a4..a6809ffe 100644 --- a/main_test.go +++ b/main_test.go @@ -131,3 +131,65 @@ func TestRunReportsDeclinedConfirmationsAndExplainedFailures(t *testing.T) { require.Equal(t, 1, run(context.Background(), deps, []string{"issue", "view", "9", "-R", "alice/project", "--no-input"})) require.Equal(t, "Error: issue #9 not found in alice/project\n", stderr.String()) } + +type reviewFormStub struct{} + +func (reviewFormStub) CreateReview(context.Context, terminal.ReviewFormChoices) (terminal.ReviewFormValues, error) { + return terminal.ReviewFormValues{Event: "COMMENT", Body: "Self note"}, nil +} + +func TestRunReportsWhereADeclinedReviewWasSaved(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/api/v1/user" { + _, _ = w.Write([]byte(`{"login":"alice"}`)) + return + } + _, _ = w.Write([]byte(`{"number":3,"title":"Add one","user":{"login":"alice"},"head":{"ref":"feat/one"},"base":{"ref":"main"}}`)) + })) + defer server.Close() + tmp := t.TempDir() + t.Setenv("TMPDIR", tmp) + var stderr bytes.Buffer + store := settings.New(t.TempDir()) + deps := app.Dependencies{ + Settings: store, Credentials: &auth.Store{Dir: store.Dir}, Client: transport.New, Prompter: decliningPrompter{}, ReviewForm: reviewFormStub{}, + Streams: terminal.Streams{In: &bytes.Buffer{}, Out: &bytes.Buffer{}, ErrOut: &stderr, InTTY: true, ErrTTY: true}, + Getenv: func(key string) string { return map[string]string{"GITEA_TOKEN": "token", "TEA_HOST": server.URL}[key] }, + } + require.Equal(t, 3, run(context.Background(), deps, []string{"pr", "review", "3", "-R", "alice/project"})) + files, err := filepath.Glob(filepath.Join(tmp, "tea-draft-*.md")) + require.NoError(t, err) + require.Len(t, files, 1) + require.Equal(t, "Canceled.\nDraft saved at "+files[0]+"\n", stderr.String()) +} + +func TestSecondInterruptStopsACommandBlockedOnStdin(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("os.Interrupt delivery is not supported by Windows processes") + } + ctx, cancel := context.WithTimeout(context.Background(), time.Minute) + defer cancel() + binary := filepath.Join(t.TempDir(), "tea") + build := exec.CommandContext(ctx, "go", "build", "-o", binary, ".") + output, err := build.CombinedOutput() + require.NoError(t, err, "build: %s", output) + command := exec.CommandContext(ctx, binary, "auth", "login", "--host", "http://127.0.0.1:1", "--with-token", "--no-input") + command.Env = append(os.Environ(), "TEA_CONFIG_DIR="+t.TempDir(), "GITEA_TOKEN=") + stdin, err := command.StdinPipe() + require.NoError(t, err) + defer stdin.Close() + require.NoError(t, command.Start()) + done := make(chan error, 1) + go func() { done <- command.Wait() }() + // Reading the token from stdin ignores the canceled context, so only a second signal ends tea. + time.Sleep(time.Second) + require.NoError(t, command.Process.Signal(os.Interrupt)) + time.Sleep(500 * time.Millisecond) + require.NoError(t, command.Process.Signal(os.Interrupt)) + select { + case err := <-done: + require.Error(t, err) + case <-time.After(10 * time.Second): + t.Fatal("tea ignored the second interrupt while waiting on stdin") + } +}