diff --git a/backends/jj/jj.go b/backends/jj/jj.go index f4eb502..9343ccf 100644 --- a/backends/jj/jj.go +++ b/backends/jj/jj.go @@ -331,7 +331,7 @@ func runSteps( } func defaultRunJJ(ctx context.Context, path string, args []string) (string, error) { - var buf bytes.Buffer + var stdout, stderr bytes.Buffer //nolint:gosec // controlled command execution, args from user input cmd := exec.CommandContext( @@ -340,15 +340,15 @@ func defaultRunJJ(ctx context.Context, path string, args []string) (string, erro args..., ) cmd.Dir = path - cmd.Stdout = &buf - cmd.Stderr = &buf + cmd.Stdout = &stdout + cmd.Stderr = &stderr err := cmd.Run() if err != nil { - return "", fmt.Errorf("jj %s: %w\n%s", args[0], err, buf.String()) + return "", fmt.Errorf("jj %s: %w\n%s", args[0], err, stderr.String()) } - return buf.String(), nil + return stdout.String(), nil } // parseWorkingCopy decodes detailTmpl's JSON output. An error means the jj diff --git a/backends/jj/jj_test.go b/backends/jj/jj_test.go index 38f664a..b8f285a 100644 --- a/backends/jj/jj_test.go +++ b/backends/jj/jj_test.go @@ -3,6 +3,7 @@ package jj import ( "context" "encoding/json" + "fmt" "os" "os/exec" "path/filepath" @@ -92,6 +93,18 @@ func isolateJJConfig(t *testing.T) { t.Setenv("JJ_CONFIG", cfgPath) } +// stubJJOnPath writes a fake "jj" executable that prints stderrMsg to stderr +// and stdout to stdout, then exits 0, and prepends its directory to PATH so +// exec.CommandContext("jj", ...) resolves to it instead of the real jj. +func stubJJOnPath(t *testing.T, stderrMsg, stdout string) { + t.Helper() + + binDir := t.TempDir() + script := fmt.Sprintf("#!/bin/sh\nprintf '%%s' %q >&2\nprintf '%%s' %q\n", stderrMsg, stdout) + require.NoError(t, os.WriteFile(filepath.Join(binDir, "jj"), []byte(script), 0o755)) + t.Setenv("PATH", binDir+string(os.PathListSeparator)+os.Getenv("PATH")) +} + // initJJRepo initializes a real jj repository and skips the test if jj is not available. func initJJRepo(t *testing.T) string { t.Helper() @@ -693,9 +706,20 @@ func TestBackend_Status_JjLogFailure(t *testing.T) { assert.Contains(t, err.Error(), "jj log") } +func TestDefaultRunJJ_StderrDoesNotCorruptStdout(t *testing.T) { + // Regression test: defaultRunJJ used to write stdout and stderr into the + // same buffer, so a warning jj prints to stderr while re-snapshotting the + // working copy (exit code 0) would corrupt the JSON read from stdout. + stubJJOnPath(t, "Warning: something jj printed to stderr\n", `{"changeId":"abc"}`) + + out, err := defaultRunJJ(t.Context(), t.TempDir(), []string{"log"}) + require.NoError(t, err) + assert.JSONEq(t, `{"changeId":"abc"}`, out) +} + func TestBackend_Status_MalformedWorkingCopyOutput(t *testing.T) { - // jj merges stdout and stderr (defaultRunJJ), so a stray warning line - // can corrupt the JSON even though the process exits 0. Status must + // A stray warning line in runJJFn's output (e.g. from a jj version that + // still emits one on stdout) must corrupt the JSON. Status must // surface this as an error, not silently return a zeroed status. b := &Backend{} b.runJJFn = func(_ context.Context, _ string, args []string) (string, error) { diff --git a/cmd/repo.go b/cmd/repo.go index 0c2ac7d..0fb3d98 100644 --- a/cmd/repo.go +++ b/cmd/repo.go @@ -238,16 +238,22 @@ const ( cmdNameUngroup = "ungroup" ) +// completeFirstArgWithRepos completes the first (and only the first) +// positional arg with repo names. +func completeFirstArgWithRepos(cfgPath *string) func(context.Context, *cli.Command) { + return func(ctx context.Context, cmd *cli.Command) { + if cmd.Args().Len() == 0 { + reposOnlyCompleter(cfgPath)(ctx, cmd) + } + } +} + func repoRenameCmd(cfgPath *string) *cli.Command { return &cli.Command{ - Name: cmdNameRename, - Usage: "rename a repository", - ArgsUsage: " ", - ShellComplete: func(ctx context.Context, cmd *cli.Command) { - if cmd.Args().Len() == 0 { - reposOnlyCompleter(cfgPath)(ctx, cmd) - } - }, + Name: cmdNameRename, + Usage: "rename a repository", + ArgsUsage: " ", + ShellComplete: completeFirstArgWithRepos(cfgPath), Action: func(_ context.Context, cmd *cli.Command) error { if cmd.NArg() != 2 { //nolint:mnd // expects old and new name return errRepoRenameUsage @@ -323,14 +329,10 @@ func groupActionCmd( act func(*config.Config, string, string), ) *cli.Command { return &cli.Command{ - Name: name, - Usage: usage, - ArgsUsage: " ", - ShellComplete: func(ctx context.Context, cmd *cli.Command) { - if cmd.Args().Len() == 0 { - reposOnlyCompleter(cfgPath)(ctx, cmd) - } - }, + Name: name, + Usage: usage, + ArgsUsage: " ", + ShellComplete: completeFirstArgWithRepos(cfgPath), Action: func(_ context.Context, cmd *cli.Command) error { if cmd.NArg() != 2 { //nolint:mnd // expects repo and group name return usageErr diff --git a/internal/tui/model.go b/internal/tui/model.go index 5359305..a6d2851 100644 --- a/internal/tui/model.go +++ b/internal/tui/model.go @@ -363,13 +363,18 @@ func (m *model) initHelpViewport() { } func (m *model) initHistoryList() { - allRepoSet := make(map[string]struct{}, len(m.cfg.Repos)) + items := buildHistoryItems(m.persState.SelectionHistory, m.cfg.Groups, m.allRepoSet()) + m.historyList = initList(defaultItemDelegate(0), items, defaultViewW) +} + +// allRepoSet returns the set of all configured repo names. +func (m *model) allRepoSet() map[string]struct{} { + set := make(map[string]struct{}, len(m.cfg.Repos)) for name := range m.cfg.Repos { - allRepoSet[name] = struct{}{} + set[name] = struct{}{} } - items := buildHistoryItems(m.persState.SelectionHistory, m.cfg.Groups, allRepoSet) - m.historyList = initList(defaultItemDelegate(0), items, defaultViewW) + return set } func (m *model) initGroupList() { diff --git a/internal/tui/screens.go b/internal/tui/screens.go index 4144931..c86a4cf 100644 --- a/internal/tui/screens.go +++ b/internal/tui/screens.go @@ -158,12 +158,7 @@ func openSelHistoryPopup(m *model) { return } - allRepoSet := make(map[string]struct{}, len(m.cfg.Repos)) - for name := range m.cfg.Repos { - allRepoSet[name] = struct{}{} - } - - items := buildHistoryItems(m.persState.SelectionHistory, m.cfg.Groups, allRepoSet) + items := buildHistoryItems(m.persState.SelectionHistory, m.cfg.Groups, m.allRepoSet()) m.historyList.SetItems(items) m.historyList.Select(0) diff --git a/internal/ui/table.go b/internal/ui/table.go index 6f223da..fa8e75b 100644 --- a/internal/ui/table.go +++ b/internal/ui/table.go @@ -53,23 +53,19 @@ func EffectiveWidths(header []string, rows [][]string, maxWidths []int) []int { } func writeHeader(buf *strings.Builder, cells []string, widths []int) { - for i, cell := range cells { - if lipgloss.Width(cell) > widths[i] { - cell = truncate.String(cell, uint(widths[i])) - } - - cell += strings.Repeat(" ", widths[i]-lipgloss.Width(cell)) - cell = "\x1b[1;96m" + cell + "\x1b[0m" - - if i > 0 { - buf.WriteByte(' ') - } - - buf.WriteString(cell) - } + writeCells(buf, cells, widths, func(cell string) string { + return "\x1b[1;96m" + cell + "\x1b[0m" + }) } func writeRow(buf *strings.Builder, cells []string, widths []int) { + writeCells(buf, cells, widths, nil) +} + +// writeCells pads/truncates each cell to its column width and writes it to +// buf, space-separated. style, if non-nil, wraps the padded cell (e.g. in +// ANSI codes) before it's written. +func writeCells(buf *strings.Builder, cells []string, widths []int, style func(string) string) { for i, cell := range cells { if lipgloss.Width(cell) > widths[i] { cell = truncate.String(cell, uint(widths[i])) @@ -77,6 +73,10 @@ func writeRow(buf *strings.Builder, cells []string, widths []int) { cell += strings.Repeat(" ", widths[i]-lipgloss.Width(cell)) + if style != nil { + cell = style(cell) + } + if i > 0 { buf.WriteByte(' ') }