jj stderr fix

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

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) {

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: "<old-name> <new-name>",
-		ShellComplete: func(ctx context.Context, cmd *cli.Command) {
-			if cmd.Args().Len() == 0 {
-				reposOnlyCompleter(cfgPath)(ctx, cmd)
-			}
-		},
+		Name:          cmdNameRename,
+		Usage:         "rename a repository",
+		ArgsUsage:     "<old-name> <new-name>",
+		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: "<repo> <group>",
-		ShellComplete: func(ctx context.Context, cmd *cli.Command) {
-			if cmd.Args().Len() == 0 {
-				reposOnlyCompleter(cfgPath)(ctx, cmd)
-			}
-		},
+		Name:          name,
+		Usage:         usage,
+		ArgsUsage:     "<repo> <group>",
+		ShellComplete: completeFirstArgWithRepos(cfgPath),
 		Action: func(_ context.Context, cmd *cli.Command) error {
 			if cmd.NArg() != 2 { //nolint:mnd // expects repo and group name
 				return usageErr

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() {

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)
 

internal/ui/table.go

@@ -53,30 +53,30 @@ 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]))
 		}
 
 		cell += strings.Repeat(" ", widths[i]-lipgloss.Width(cell))
 
+		if style != nil {
+			cell = style(cell)
+		}
+
 		if i > 0 {
 			buf.WriteByte(' ')
 		}