From a47dbc413c2c28ae339f37502f6a5e25c4baabbe Mon Sep 17 00:00:00 2001 From: bircni Date: Sun, 18 Oct 2026 17:38:52 +0200 Subject: [PATCH] fix: address verified review findings (terminal-output-1) Signed-off-by: bircni --- internal/app/action_runs.go | 6 +++--- internal/app/admin_users.go | 1 + internal/app/branches.go | 2 +- internal/app/checks.go | 22 +++++++++++++++++++++- internal/app/comments.go | 4 ++-- internal/app/discussion_view.go | 2 +- internal/app/human_output.go | 2 +- internal/app/issue_attachments.go | 7 ++++--- internal/app/label_export.go | 2 +- internal/app/notifications.go | 2 +- internal/app/organizations.go | 9 +++++++++ internal/app/organizations_test.go | 6 ++++++ internal/app/screen_rows_test.go | 18 ++++++++++++++++++ internal/app/teams.go | 1 + internal/app/teams_test.go | 21 +++++++++++++++++++++ internal/terminal/format.go | 2 +- internal/terminal/format_test.go | 1 + internal/terminal/output.go | 13 +++++++++++-- internal/terminal/output_test.go | 21 +++++++++++++++++++++ internal/terminal/safe_writer.go | 2 +- internal/terminal/safe_writer_test.go | 23 ++++++++++++++++++++--- internal/terminal/streams.go | 21 +++++++++++++++++++-- internal/terminal/streams_test.go | 2 ++ internal/terminal/view.go | 4 +++- internal/terminal/view_test.go | 5 +++++ 25 files changed, 175 insertions(+), 24 deletions(-) create mode 100644 internal/app/screen_rows_test.go diff --git a/internal/app/action_runs.go b/internal/app/action_runs.go index 18d50ced..2e233a54 100644 --- a/internal/app/action_runs.go +++ b/internal/app/action_runs.go @@ -382,9 +382,9 @@ func runRecord(run services.Run, d terminal.Display, now time.Time) string { {Label: "Event", Value: run.Event}, {Label: "Actor", Value: run.Actor}, {Label: "Attempt", Value: strconv.FormatInt(run.Attempt, 10)}, - {Label: "Created_at", Value: run.CreatedAt}, - {Label: "Started_at", Value: optional(run.StartedAt)}, - {Label: "Completed_at", Value: optional(run.CompletedAt)}, + {Label: "Created_at", Value: pipedTime(run.CreatedAt)}, + {Label: "Started_at", Value: pipedTime(optional(run.StartedAt))}, + {Label: "Completed_at", Value: pipedTime(optional(run.CompletedAt))}, {Label: "Elapsed", Value: runElapsedText(run.StartedAt, run.CompletedAt, now)}, } rows := make([][]terminal.Cell, len(run.Jobs)) diff --git a/internal/app/admin_users.go b/internal/app/admin_users.go index ad3c9b22..e7732efe 100644 --- a/internal/app/admin_users.go +++ b/internal/app/admin_users.go @@ -129,6 +129,7 @@ $ tea admin user view bob --json email,admin`, return terminal.View{ Title: user.Username, ID: fmt.Sprintf("(ID %d)", user.ID), Badges: []terminal.Cell{role, status}, Meta: meta, Fields: []terminal.Field{ + {Label: "ID", Value: strconv.FormatInt(user.ID, 10), PipedOnly: true}, {Label: "Name", Value: user.FullName}, {Label: "Email", Value: user.Email}, {Label: "Role", Value: role.Text, PipedOnly: true}, diff --git a/internal/app/branches.go b/internal/app/branches.go index b3bc15ce..1e4f65a1 100644 --- a/internal/app/branches.go +++ b/internal/app/branches.go @@ -146,7 +146,7 @@ func (a *application) branchViewCommand() *cobra.Command { meta = "updated " + terminal.RelativeTime(a.deps.Now(), updated) } return terminal.View{ - Title: item.Name, Badges: badges, Meta: meta, Body: item.Message, Kind: "branch", URL: branchURL(repository, item.Name), + Title: item.Name, Badges: badges, Meta: meta, Body: item.Message, Plain: true, Kind: "branch", URL: branchURL(repository, item.Name), Fields: []terminal.Field{{Label: "Commit", Value: item.SHA}, {Label: "Protection", Value: protection}, {Label: "You can push", Value: yesNo(item.CanPush)}}, } }) diff --git a/internal/app/checks.go b/internal/app/checks.go index 228abc2a..1fda2449 100644 --- a/internal/app/checks.go +++ b/internal/app/checks.go @@ -8,12 +8,14 @@ import ( "errors" "fmt" "io" + "regexp" "strings" "time" "gitea.dev/tea/internal/clierr" "gitea.dev/tea/internal/services" "gitea.dev/tea/internal/terminal" + "github.com/mattn/go-runewidth" "github.com/spf13/cobra" ) @@ -153,8 +155,26 @@ func (a *application) checksRedraw(cmd *cobra.Command, interval time.Duration) f if lines > 0 { clear = fmt.Sprintf("\x1b[%dA\x1b[J", lines) } - lines = strings.Count(text, "\n") + lines = screenRows(text, a.deps.Streams.Width) _, err := io.WriteString(cmd.OutOrStdout(), clear+text) return err } } + +// displayEscapes matches the color and hyperlink sequences that take no room on screen. +var displayEscapes = regexp.MustCompile("\x1b\\[[0-9;]*m|\x1b\\]8;[^\x07\x1b]*(?:\x07|\x1b\\\\)") + +// screenRows counts the rows the cursor sits below the start of text on a terminal width columns wide, +// where a line wider than the terminal wraps onto more rows; a width of 0 means unlimited. +func screenRows(text string, width int) int { + rows := strings.Count(text, "\n") + if width <= 0 { + return rows + } + for _, line := range strings.Split(text, "\n") { + if cells := runewidth.StringWidth(displayEscapes.ReplaceAllString(line, "")); cells > width { + rows += (cells - 1) / width + } + } + return rows +} diff --git a/internal/app/comments.go b/internal/app/comments.go index 0348d275..26c1fa7d 100644 --- a/internal/app/comments.go +++ b/internal/app/comments.go @@ -203,7 +203,7 @@ func commentView(t target, now time.Time) func(services.Comment) terminal.View { Fields: []terminal.Field{ {Label: "ID", Value: strconv.FormatInt(comment.ID, 10), PipedOnly: true}, {Label: "Author", Value: comment.Author, PipedOnly: true}, - {Label: "Created_at", Value: comment.CreatedAt, PipedOnly: true}, + {Label: "Created_at", Value: pipedTime(comment.CreatedAt), PipedOnly: true}, }, Body: comment.Body, Kind: "comment", URL: comment.URL, } @@ -228,7 +228,7 @@ func (a *application) writeComments(cmd *cobra.Command, t target, n int64, items var text strings.Builder for i, comment := range items { if !d.TTY { - text.WriteString(fmt.Sprintf("id:\t%d\nauthor:\t%s\ncreated_at:\t%s\nurl:\t%s\n--\n", comment.ID, terminal.SafeLine(comment.Author), comment.CreatedAt, terminal.SafeLine(comment.URL))) + text.WriteString(fmt.Sprintf("id:\t%d\nauthor:\t%s\ncreated_at:\t%s\nurl:\t%s\n--\n", comment.ID, terminal.SafeLine(comment.Author), pipedTime(comment.CreatedAt), terminal.SafeLine(comment.URL))) text.WriteString(strings.TrimRight(terminal.SafeText(comment.Body), "\n") + "\n--\n") continue } diff --git a/internal/app/discussion_view.go b/internal/app/discussion_view.go index 4a8b4998..b5615057 100644 --- a/internal/app/discussion_view.go +++ b/internal/app/discussion_view.go @@ -291,7 +291,7 @@ func (a *application) writeDiscussion(cmd *cobra.Command, target target, v discu var b strings.Builder b.WriteString(view.Render(d)) for _, comment := range comments { - fmt.Fprintf(&b, "--\nauthor:\t%s\ncreated_at:\t%s\n", terminal.SafeLine(comment.author), terminal.SafeLine(comment.createdAt)) + fmt.Fprintf(&b, "--\nauthor:\t%s\ncreated_at:\t%s\n", terminal.SafeLine(comment.author), pipedTime(comment.createdAt)) if comment.id > 0 { fmt.Fprintf(&b, "id:\t%d\n", comment.id) } diff --git a/internal/app/human_output.go b/internal/app/human_output.go index c5333fe5..a44bac95 100644 --- a/internal/app/human_output.go +++ b/internal/app/human_output.go @@ -51,7 +51,7 @@ func (a *application) writeHuman(cmd *cobra.Command, text string) error { // writeJSON writes v as JSON, filtered by --json fields and --jq. func (a *application) writeJSON(cmd *cobra.Command, v any) error { pretty := a.deps.Streams.OutTTY - return terminal.JSONOutput{Fields: a.options.json, Query: a.options.jq, Pretty: pretty, Color: pretty && a.colorEnabled()}.Write(cmd.OutOrStdout(), v) + return terminal.JSONOutput{Fields: a.options.json, Allowed: commandOutputFields(cmd), Query: a.options.jq, Pretty: pretty, Color: pretty && a.colorEnabled()}.Write(cmd.OutOrStdout(), v) } // listSpec describes how a list command shows its items to people. diff --git a/internal/app/issue_attachments.go b/internal/app/issue_attachments.go index c2821db7..74609b86 100644 --- a/internal/app/issue_attachments.go +++ b/internal/app/issue_attachments.go @@ -31,13 +31,13 @@ func attachmentRow(item services.IssueAttachment) []terminal.Cell { } } -func attachmentView(item services.IssueAttachment) terminal.View { +func attachmentView(item services.IssueAttachment, d terminal.Display) terminal.View { return terminal.View{ Title: item.Name, ID: "#" + strconv.FormatInt(item.ID, 10), Fields: []terminal.Field{ {Label: "Size", Value: terminal.Bytes(item.Size)}, {Label: "Downloads", Value: strconv.FormatInt(item.DownloadCount, 10)}, - {Label: "Created", Value: item.CreatedAt}, + timeField("Created", item.CreatedAt, d), }, Kind: "attachment", URL: item.URL, } @@ -93,7 +93,8 @@ are addressed by the %s number and the attachment ID that attachment list shows. if err != nil { return err } - return writeView(a, cmd, item, attachmentView) + d := a.display() + return writeView(a, cmd, item, func(item services.IssueAttachment) terminal.View { return attachmentView(item, d) }) }} var uploadName string diff --git a/internal/app/label_export.go b/internal/app/label_export.go index 47c5e91c..0d783255 100644 --- a/internal/app/label_export.go +++ b/internal/app/label_export.go @@ -33,7 +33,7 @@ $ tea label export -R alice/old | tea label import - -R alice/new`, Args: argsEx return err } if a.options.json != "" { - return terminal.WriteJSON(destination, items, a.options.json, a.options.jq) + return terminal.JSONOutput{Fields: a.options.json, Query: a.options.jq, Safe: output == "" && a.deps.Streams.OutTTY}.Write(destination, items) } var text strings.Builder for _, item := range items { diff --git a/internal/app/notifications.go b/internal/app/notifications.go index bf8d4482..66b8b380 100644 --- a/internal/app/notifications.go +++ b/internal/app/notifications.go @@ -222,7 +222,7 @@ With --json it prints one JSON object per line.`, err = s.Watch(ctx, listed, interval, func(items []services.Notification) error { if a.options.json != "" { for _, item := range items { - if err := (terminal.JSONOutput{Fields: a.options.json, Query: a.options.jq}).Write(cmd.OutOrStdout(), item); err != nil { + if err := (terminal.JSONOutput{Fields: a.options.json, Allowed: commandOutputFields(cmd), Query: a.options.jq, Safe: d.TTY}).Write(cmd.OutOrStdout(), item); err != nil { return err } } diff --git a/internal/app/organizations.go b/internal/app/organizations.go index 19446e5c..c2972c1e 100644 --- a/internal/app/organizations.go +++ b/internal/app/organizations.go @@ -37,6 +37,15 @@ func timeField(label, raw string, d terminal.Display) terminal.Field { return terminal.Field{Label: label, Value: terminal.RelativeTime(now, t)} } +// pipedTime normalizes a server timestamp to RFC 3339 UTC like the times of piped tables, and keeps +// a value that is not a timestamp as it is. +func pipedTime(raw string) string { + if t := terminal.ParseTime(raw); !t.IsZero() { + return terminal.Timestamp(t) + } + return raw +} + func (a *application) organizationCommand() *cobra.Command { group := &cobra.Command{Use: "org", Short: "Manage organizations", Long: `Work with Gitea organizations: list the ones you belong to, view, create, edit and delete them, and manage their members. diff --git a/internal/app/organizations_test.go b/internal/app/organizations_test.go index 2cf4e0e4..df1a72cd 100644 --- a/internal/app/organizations_test.go +++ b/internal/app/organizations_test.go @@ -95,3 +95,9 @@ func TestOrganizationViewShowsDetailsAndCounts(t *testing.T) { require.Equal(t, "title:\tACME Corp\nname:\tacme\nvisibility:\tpublic\nwebsite:\thttps://acme.example\nlocation:\t\n"+ "repo admins manage team access:\tno\nmembers:\t2\nteams:\t3\nrepositories:\t1\nurl:\t"+server.URL+"/acme\n--\nTools\n", out.String()) } + +func TestPipedTimeNormalizesToUTC(t *testing.T) { + require.Equal(t, "2026-01-02T01:04:05Z", pipedTime("2026-01-02T03:04:05+02:00")) + require.Equal(t, "", pipedTime("")) + require.Equal(t, "soon", pipedTime("soon")) +} diff --git a/internal/app/screen_rows_test.go b/internal/app/screen_rows_test.go new file mode 100644 index 00000000..d936a742 --- /dev/null +++ b/internal/app/screen_rows_test.go @@ -0,0 +1,18 @@ +// Copyright 2026 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package app + +import ( + "strings" + "testing" + + "github.com/stretchr/testify/require" +) + +func TestScreenRowsCountsWrappedLines(t *testing.T) { + require.Equal(t, 2, screenRows("one\ntwo\n", 80)) + require.Equal(t, 2, screenRows("one\ntwo\n", 0)) + require.Equal(t, 4, screenRows(strings.Repeat("x", 25)+"\nab\n", 10), "25 cells take 3 rows of 10") + require.Equal(t, 2, screenRows("\x1b[31m"+strings.Repeat("x", 10)+"\x1b[0m\n\x1b]8;;http://a/b\x1b\\link\x1b]8;;\x1b\\\n", 10), "escapes take no room") +} diff --git a/internal/app/teams.go b/internal/app/teams.go index 0386da65..270f75c6 100644 --- a/internal/app/teams.go +++ b/internal/app/teams.go @@ -212,6 +212,7 @@ $ tea team view 2 --json units`, return terminal.View{ Title: team.Slug(), ID: fmt.Sprintf("(ID %d)", team.ID), Badges: []terminal.Cell{{Text: team.Access(), Bold: true}}, Fields: []terminal.Field{ + {Label: "ID", Value: strconv.FormatInt(team.ID, 10), PipedOnly: true}, {Label: "Access", Value: team.Access(), PipedOnly: true}, {Label: "Units", Value: strings.Join(units, ", ")}, {Label: "Members", Value: memberNames}, diff --git a/internal/app/teams_test.go b/internal/app/teams_test.go index a0300d43..6f8777f1 100644 --- a/internal/app/teams_test.go +++ b/internal/app/teams_test.go @@ -131,6 +131,27 @@ func TestTeamsAreAddressedAsOrganizationAndName(t *testing.T) { require.EqualError(t, err, `team "ops" not found in acme`) } +func TestTeamViewPrintsTheIDWhenPiped(t *testing.T) { + server := newAuthenticatedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/api/v1/orgs/acme/teams/search": + if r.URL.Query().Get("page") != "1" { + _, _ = w.Write([]byte(`{"ok":true,"data":[]}`)) + return + } + _, _ = w.Write([]byte(`{"ok":true,"data":[{"id":2,"name":"developers","permission":"none"}]}`)) + case "/api/v1/teams/2/members", "/api/v1/teams/2/repos": + _, _ = w.Write([]byte(`[]`)) + default: + t.Errorf("unexpected request %s", r.URL.Path) + } + })) + defer server.Close() + deps, out, _ := serverDeps(t, server.URL) + require.NoError(t, Execute(context.Background(), deps, []string{"team", "view", "acme/developers"})) + require.Contains(t, out.String(), "\nid:\t2\n") +} + func TestTeamListShowsAccessAndSizes(t *testing.T) { totals := map[string]string{"/api/v1/teams/1/members": "1", "/api/v1/teams/1/repos": "4", "/api/v1/teams/2/members": "3", "/api/v1/teams/2/repos": "2"} server := newAuthenticatedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { diff --git a/internal/terminal/format.go b/internal/terminal/format.go index 79d69ec1..84e62c1a 100644 --- a/internal/terminal/format.go +++ b/internal/terminal/format.go @@ -45,7 +45,7 @@ func StatusIcon(status, conclusion string) Cell { switch status { case "success": return Cell{Text: "✓", Color: "green"} - case "failure", "error": + case "failure", "error", "warning": return Cell{Text: "X", Color: "red"} case "cancelled", "canceled", "skipped": //nolint:misspell // Gitea API spelling. return Cell{Text: "-", Muted: true} diff --git a/internal/terminal/format_test.go b/internal/terminal/format_test.go index 2ecae9f4..aff1d3b2 100644 --- a/internal/terminal/format_test.go +++ b/internal/terminal/format_test.go @@ -57,6 +57,7 @@ func TestStatusIcon(t *testing.T) { {"success", "", Cell{Text: "✓", Color: "green"}}, {"completed", "failure", Cell{Text: "X", Color: "red"}}, {"error", "", Cell{Text: "X", Color: "red"}}, + {"warning", "", Cell{Text: "X", Color: "red"}}, {"completed", "cancelled", Cell{Text: "-", Muted: true}}, //nolint:misspell // Gitea API spelling. {"skipped", "", Cell{Text: "-", Muted: true}}, {"queued", "", Cell{Text: "*", Color: "yellow"}}, diff --git a/internal/terminal/output.go b/internal/terminal/output.go index 36c1c6e1..49b7bc9e 100644 --- a/internal/terminal/output.go +++ b/internal/terminal/output.go @@ -19,10 +19,16 @@ import ( type JSONOutput struct { // Fields holds the comma-separated fields to keep; "" and "*" keep every field. Fields string + // Allowed lists the fields the command documents. A listed field that an item omits (an empty + // omitempty value) is selected as null instead of being reported unknown; nil allows none. + Allowed []string // Query is a jq expression applied after field selection. Query string // Pretty indents the JSON for a terminal and strips control characters from raw jq strings. Pretty bool + // Safe strips control characters from raw jq strings and encoded text like Pretty, for terminal + // output that must stay one line per value. + Safe bool // Color highlights pretty JSON keys and values. Color bool } @@ -51,6 +57,9 @@ func (o JSONOutput) Write(w io.Writer, value any) error { selected := map[string]any{} for _, name := range names { v, exists := object[name] + if !exists && slices.Contains(o.Allowed, name) { + v, exists = nil, true + } if !exists { return nil, fmt.Errorf("unknown JSON field %q", name) } @@ -90,7 +99,7 @@ func (o JSONOutput) Write(w io.Writer, value any) error { return fmt.Errorf("evaluate --jq: %w", err) } if str, ok := value.(string); ok { - if o.Pretty { + if o.Pretty || o.Safe { str = SafeText(str) } if _, err := fmt.Fprintln(w, str); err != nil { @@ -114,7 +123,7 @@ func (o JSONOutput) encode(w io.Writer, value any) error { return err } text := buf.String() - if o.Pretty { + if o.Pretty || o.Safe { // The encoder escapes control characters but not format characters such as bidirectional overrides. text = SafeText(text) } diff --git a/internal/terminal/output_test.go b/internal/terminal/output_test.go index bf9fc114..969f643f 100644 --- a/internal/terminal/output_test.go +++ b/internal/terminal/output_test.go @@ -24,6 +24,27 @@ func TestJSONFieldSelectionAndJQ(t *testing.T) { require.EqualError(t, ValidateOutput("number,,title", "", []string{"number", "title"}), "--json field names cannot be empty; use --json alone for every field") } +func TestJSONFieldSelectionKeepsDocumentedFieldsOmittedByOmitempty(t *testing.T) { + type item struct { + Name string `json:"name"` + Parent *string `json:"parent,omitempty"` + } + var out bytes.Buffer + require.NoError(t, JSONOutput{Fields: "name,parent", Allowed: []string{"name", "parent"}}.Write(&out, []item{{Name: "x"}})) + require.Equal(t, "[{\"name\":\"x\",\"parent\":null}]\n", out.String()) + require.EqualError(t, JSONOutput{Fields: "parent", Allowed: []string{"name"}}.Write(&out, []item{{Name: "x"}}), `unknown JSON field "parent"`) +} + +func TestJSONOutputSafeStripsControlsAndStaysOnOneLine(t *testing.T) { + value := map[string]any{"title": "a\x1b]0;x\x07b\u202e"} + var out bytes.Buffer + require.NoError(t, JSONOutput{Query: ".title", Safe: true}.Write(&out, value)) + require.Equal(t, "a]0;xb\n", out.String()) + out.Reset() + require.NoError(t, JSONOutput{Safe: true}.Write(&out, value)) + require.Equal(t, "{\"title\":\"a\\u001b]0;x\\u0007b\"}\n", out.String()) +} + func TestJSONOutputIsIndentedAndColoredForTerminals(t *testing.T) { var out bytes.Buffer value := []map[string]any{{"title": "a \u202e\x1b[31mb", "done": true, "due": nil, "number": 1}} diff --git a/internal/terminal/safe_writer.go b/internal/terminal/safe_writer.go index ee45ed71..3b1eff29 100644 --- a/internal/terminal/safe_writer.go +++ b/internal/terminal/safe_writer.go @@ -10,7 +10,7 @@ import ( ) // maxSGRLength bounds how many bytes of a possible SGR sequence are held back between writes. -const maxSGRLength = 32 +const maxSGRLength = 256 // SafeWriter returns w unchanged when tty is false, so piped output stays byte-exact. // On a terminal it filters streamed server text like SafeText, and with allowSGR it keeps diff --git a/internal/terminal/safe_writer_test.go b/internal/terminal/safe_writer_test.go index 05bb9525..bcda767e 100644 --- a/internal/terminal/safe_writer_test.go +++ b/internal/terminal/safe_writer_test.go @@ -5,13 +5,30 @@ package terminal import ( "bytes" + "strings" "testing" "github.com/stretchr/testify/require" ) +func TestSafeWriterKeepsLongSGRSequences(t *testing.T) { + const colored = "\x1b[38;2;255;255;255;48;2;255;255;255mwhite\x1b[0m\n" + for _, chunk := range []int{len(colored), 1, 3} { + var out bytes.Buffer + w := SafeWriter(&out, true, true) + for data := []byte(colored); len(data) > 0; { + n := min(chunk, len(data)) + _, err := w.Write(data[:n]) + require.NoError(t, err) + data = data[n:] + } + require.Equal(t, colored, out.String(), "chunk size %d", chunk) + } +} + func TestSafeWriter(t *testing.T) { - const hostile = "+hello \x1b]0;PWNED\x07 \x1b[1;31mred\x1b[0m\x1b[2J\r\n\u009b2J\u202eend\t\xff\x1b[1111111111111111111111111111111111m\n" + long := strings.Repeat("1", 300) + hostile := "+hello \x1b]0;PWNED\x07 \x1b[1;31mred\x1b[0m\x1b[2J\r\n\u009b2J\u202eend\t\xff\x1b[" + long + "m\n" var piped bytes.Buffer _, err := SafeWriter(&piped, false, false).Write([]byte(hostile)) require.NoError(t, err) @@ -21,8 +38,8 @@ func TestSafeWriter(t *testing.T) { allowSGR bool want string }{ - {"drops every escape", false, "+hello ]0;PWNED [1;31mred[0m[2J\n2Jend\t\uFFFD[1111111111111111111111111111111111m\n"}, - {"keeps colors", true, "+hello ]0;PWNED \x1b[1;31mred\x1b[0m[2J\n2Jend\t\uFFFD[1111111111111111111111111111111111m\n"}, + {"drops every escape", false, "+hello ]0;PWNED [1;31mred[0m[2J\n2Jend\t\uFFFD[" + long + "m\n"}, + {"keeps colors", true, "+hello ]0;PWNED \x1b[1;31mred\x1b[0m[2J\n2Jend\t\uFFFD[" + long + "m\n"}, } { t.Run(tc.name, func(t *testing.T) { if !tc.allowSGR { diff --git a/internal/terminal/streams.go b/internal/terminal/streams.go index 60393c29..0c6182e9 100644 --- a/internal/terminal/streams.go +++ b/internal/terminal/streams.go @@ -188,15 +188,32 @@ type Confirmation struct { NonTTY string } +// questionEnd returns the index of the first question mark outside double quotes, or -1, so a quoted +// name such as "why? not" does not end the question. +func questionEnd(prompt string) int { + quoted := false + for i, r := range prompt { + switch { + case r == '"': + quoted = !quoted + case r == '?' && !quoted: + return i + } + } + return -1 +} + // ConfirmAction asks c unless yes is set. A declined or aborted confirmation returns ErrCanceled. func ConfirmAction(ctx context.Context, c Confirmation, yes bool, streams Streams, policy Policy, prompt Prompter) error { if yes { return nil } + // Names in the question come from the server, so keep control characters off the terminal. + c.Prompt, c.NonTTY = SafeLine(c.Prompt), SafeLine(c.NonTTY) if !policy.CanPrompt(streams) { action := c.NonTTY - if question, _, ok := strings.Cut(c.Prompt, "?"); action == "" && ok && !c.DefaultYes && question != "" { - action = strings.ToLower(question[:1]) + question[1:] + if question := questionEnd(c.Prompt); action == "" && question > 0 && !c.DefaultYes { + action = strings.ToLower(c.Prompt[:1]) + c.Prompt[1:question] } if action != "" { action = " to " + action diff --git a/internal/terminal/streams_test.go b/internal/terminal/streams_test.go index 08fc092a..d10cd220 100644 --- a/internal/terminal/streams_test.go +++ b/internal/terminal/streams_test.go @@ -99,6 +99,8 @@ func TestConfirmAction(t *testing.T) { {name: "accepted by flag", c: deleteLabel, yes: true, prompt: &askingPrompt{}}, {name: "names the action without a terminal", c: deleteLabel, prompt: &askingPrompt{}, code: clierr.Usage, message: `confirmation required to delete label "bug" from alice/x; rerun with --yes`}, {name: "names the prompted action without a terminal", c: Confirmation{Prompt: "Delete alice/x? It becomes read-only."}, prompt: &askingPrompt{}, code: clierr.Usage, message: "confirmation required to delete alice/x; rerun with --yes"}, + {name: "keeps a quoted question mark", c: Confirmation{Prompt: `Delete label "why? not" from alice/x?`}, prompt: &askingPrompt{}, code: clierr.Usage, message: `confirmation required to delete label "why? not" from alice/x; rerun with --yes`}, + {name: "drops escape sequences from server names", c: Confirmation{Prompt: "Delete attachment \"x\x1b]0;PWNED\x07\"?"}, prompt: &askingPrompt{}, code: clierr.Usage, message: `confirmation required to delete attachment "x]0;PWNED"; rerun with --yes`}, {name: "without a terminal or action", c: Confirmation{Prompt: "Go?", DefaultYes: true}, prompt: &askingPrompt{}, code: clierr.Usage, message: "confirmation required; rerun with --yes"}, {name: "accepted", c: deleteLabel, streams: terminal, prompt: &askingPrompt{accept: true}, defaults: []bool{false}}, {name: "passes the default", c: Confirmation{Prompt: "Create pull request?", DefaultYes: true}, streams: terminal, prompt: &askingPrompt{accept: true}, defaults: []bool{true}}, diff --git a/internal/terminal/view.go b/internal/terminal/view.go index 220d69e8..8d4c9b2c 100644 --- a/internal/terminal/view.go +++ b/internal/terminal/view.go @@ -46,6 +46,8 @@ type View struct { Meta string Fields []Field Body string + // Plain marks the body as plain text, such as a commit message, which is never rendered as markdown. + Plain bool // Bodyless resources, such as webhooks, have no body: the view ends with the fields and URL. Bodyless bool // Kind names the resource in the footer, such as "issue". @@ -113,7 +115,7 @@ func (v View) Render(d Display) string { switch { case body == "": body = muted("No description provided") - case d.Markdown: + case d.Markdown && !v.Plain: body = Markdown(body, d) default: body = SafeText(body) diff --git a/internal/terminal/view_test.go b/internal/terminal/view_test.go index 1a8e2a7a..468076c3 100644 --- a/internal/terminal/view_test.go +++ b/internal/terminal/view_test.go @@ -96,6 +96,11 @@ func TestViewRendersMarkdownOnlyWhenEnabled(t *testing.T) { require.Contains(t, view.Render(Display{TTY: true, Markdown: true, Color: true, Links: true}), "\x1b]8;id=") } +func TestViewKeepsPlainBodiesVerbatim(t *testing.T) { + view := View{Title: "main", Body: "fix __init__\n# not a heading\x1b[2J", Plain: true} + require.Contains(t, view.Render(Display{TTY: true, Markdown: true}), "\nfix __init__\n# not a heading[2J\n") +} + func TestMarkdownSanitizesServerText(t *testing.T) { rendered := Markdown("Hello \x1b]0;PWNED\x07world\n\n`\x1b[2Jcode`", Display{Color: true, Links: true}) require.NotContains(t, rendered, "\x1b]0;")