fix: address review findings in selfupdate, secret, cobrax, live and pick

cobrax/boolargs.go

@@ -29,33 +29,33 @@ func AllowSpacedBool(flags *pflag.FlagSet, names ...string) {
 }
 
 // JoinSpacedBoolValues rewrites "--flag false" into "--flag=false" for the
-// boolean flags of root and its subcommands that [AllowSpacedBool] marked,
-// so the space form parses. Pass the result to root.SetArgs.
+// boolean flags that [AllowSpacedBool] marked, so the space form parses.
+// Pass the result to root.SetArgs.
 //
 // pflag gives a boolean flag NoOptDefVal "true", which lets "--flag" stand
 // alone and leaves a following "false" behind as a positional argument.
-// Only the marked long flags are joined, only when the next argument is
-// exactly "true" or "false" (in any case), and never after "--". For a
-// shell completion request the word being completed is left alone.
+// Only long flags are joined, and only when the command the arguments
+// select has a marked flag of that name, its own or inherited: an unmarked
+// flag of the same name on another command keeps its meaning. The next
+// argument must be exactly "true" or "false" (in any case), and nothing
+// after "--" is touched. For a shell completion request the word being
+// completed is left alone.
 func JoinSpacedBoolValues(root *cobra.Command, args []string) []string {
-	marked := map[string]bool{}
+	anyMarked := false
 	walk(root, func(cmd *cobra.Command) {
-		collect := func(flag *pflag.Flag) {
-			if _, ok := flag.Annotations[SpacedBoolAnnotation]; ok {
-				marked[flag.Name] = true
-			}
-		}
-		cmd.LocalNonPersistentFlags().VisitAll(collect)
-		cmd.PersistentFlags().VisitAll(collect)
+		cmd.LocalFlags().VisitAll(func(flag *pflag.Flag) {
+			anyMarked = anyMarked || spaced(flag)
+		})
 	})
-	if len(marked) == 0 {
+	if !anyMarked {
 		return args
 	}
 
-	limit := len(args)
+	first, limit := 0, len(args)
 	if len(args) > 0 && (args[0] == cobra.ShellCompRequestCmd || args[0] == cobra.ShellCompNoDescRequestCmd) {
-		limit-- // The last word is the one being completed.
+		first, limit = 1, limit-1 // The last word is the one being completed.
 	}
+	cmd := selected(root, args[first:limit])
 	joined := make([]string, 0, len(args))
 	for i := 0; i < len(args); i++ {
 		arg := args[i]
@@ -65,7 +65,7 @@ func JoinSpacedBoolValues(root *cobra.Command, args []string) []string {
 		name, long := strings.CutPrefix(arg, "--")
 		// limit never exceeds len(args); the second bound states it for
 		// readers and for gosec.
-		if long && marked[name] && i+1 < limit && i+1 < len(args) && isBoolLiteral(args[i+1]) {
+		if long && i >= first && i+1 < limit && i+1 < len(args) && isBoolLiteral(args[i+1]) && spaced(lookupFlag(cmd, name)) {
 			joined = append(joined, arg+"="+strings.ToLower(args[i+1]))
 			i++
 			continue
@@ -75,6 +75,64 @@ func JoinSpacedBoolValues(root *cobra.Command, args []string) []string {
 	return joined
 }
 
+// selected returns the command args select, the way cobra's Find walks
+// them, except that "true" or "false" after a marked flag is that flag's
+// value rather than a command name.
+func selected(root *cobra.Command, args []string) *cobra.Command {
+	cmd := root
+	for i := 0; i < len(args); i++ {
+		arg := args[i]
+		switch {
+		case arg == "--":
+			return cmd
+		case strings.HasPrefix(arg, "--") && !strings.Contains(arg, "="):
+			flag := lookupFlag(cmd, arg[2:])
+			// As in cobra, a flag that is unknown here or needs a value takes
+			// the next argument.
+			if flag == nil || flag.NoOptDefVal == "" || spaced(flag) && i+1 < len(args) && isBoolLiteral(args[i+1]) {
+				i++
+			}
+		case len(arg) == 2 && arg[0] == '-' && arg[1] != '-':
+			if flag := lookupShorthand(cmd, arg[1:]); flag == nil || flag.NoOptDefVal == "" {
+				i++
+			}
+		case strings.HasPrefix(arg, "-"):
+			// Joined short flags and --flag=value carry their values.
+		default:
+			child, _, _ := cmd.Find([]string{arg})
+			if child == nil || child == cmd {
+				return cmd // The first positional argument ends the walk.
+			}
+			cmd = child
+		}
+	}
+	return cmd
+}
+
+// lookupFlag finds a long flag of cmd, its own or inherited from a parent.
+func lookupFlag(cmd *cobra.Command, name string) *pflag.Flag {
+	if flag := cmd.LocalFlags().Lookup(name); flag != nil {
+		return flag
+	}
+	return cmd.InheritedFlags().Lookup(name)
+}
+
+func lookupShorthand(cmd *cobra.Command, name string) *pflag.Flag {
+	if flag := cmd.LocalFlags().ShorthandLookup(name); flag != nil {
+		return flag
+	}
+	return cmd.InheritedFlags().ShorthandLookup(name)
+}
+
+// spaced reports whether [AllowSpacedBool] marked flag; nil is not marked.
+func spaced(flag *pflag.Flag) bool {
+	if flag == nil {
+		return false
+	}
+	_, ok := flag.Annotations[SpacedBoolAnnotation]
+	return ok
+}
+
 func isBoolLiteral(arg string) bool {
 	switch strings.ToLower(arg) {
 	case "true", "false":

cobrax/cobrax_test.go

@@ -369,8 +369,14 @@ func spacedRoot() *cobra.Command {
 	}
 	deploy.Flags().String("previous", "", "previous version")
 	root.PersistentFlags().BoolP("verbose", "v", false, "verbose")
+	root.PersistentFlags().Bool("dry-run", false, "dry run")
+	root.PersistentFlags().String("output", "", "output format")
 	AllowSpacedBool(deploy.Flags(), "active", "stable", "hidden")
-	root.AddCommand(deploy)
+	AllowSpacedBool(root.PersistentFlags(), "dry-run")
+	// status has a --stable flag of its own that is not marked.
+	status := &cobra.Command{Use: "status", RunE: func(*cobra.Command, []string) error { return nil }}
+	status.Flags().Bool("stable", false, "stable only")
+	root.AddCommand(deploy, status)
 	return root
 }
 
@@ -385,17 +391,35 @@ func TestJoinSpacedBoolValues(t *testing.T) {
 			[]string{"deploy", "app-1.2", "--stable", "false", "--apply"},
 			[]string{"deploy", "app-1.2", "--stable=false", "--apply"}},
 		{"several at once",
-			[]string{"--active", "false", "--stable", "true"},
-			[]string{"--active=false", "--stable=true"}},
+			[]string{"deploy", "--active", "false", "--stable", "true"},
+			[]string{"deploy", "--active=false", "--stable=true"}},
 		{"bare flag stands alone", []string{"--stable", "--apply"}, []string{"--stable", "--apply"}},
 		{"equals form untouched", []string{"--stable=false"}, []string{"--stable=false"}},
 		{"flag at the end", []string{"deploy", "--stable"}, []string{"deploy", "--stable"}},
 		{"unmarked flags keep their positional", []string{"--previous", "false", "--apply", "true"}, []string{"--previous", "false", "--apply", "true"}},
 		{"non-boolean value not swallowed", []string{"--stable", "app-1.2"}, []string{"--stable", "app-1.2"}},
-		{"case is normalized", []string{"--active", "FALSE"}, []string{"--active=false"}},
+		{"case is normalized", []string{"deploy", "--active", "FALSE"}, []string{"deploy", "--active=false"}},
 		{"nothing after -- is touched",
-			[]string{"--stable", "false", "--", "--active", "false"},
-			[]string{"--stable=false", "--", "--active", "false"}},
+			[]string{"deploy", "--stable", "false", "--", "--active", "false"},
+			[]string{"deploy", "--stable=false", "--", "--active", "false"}},
+		{"flags before the command name",
+			[]string{"--stable", "false", "deploy"},
+			[]string{"--stable=false", "deploy"}},
+		{"flags of the selected command only",
+			[]string{"status", "--stable", "false"},
+			[]string{"status", "--stable", "false"}},
+		{"marked flags are not joined on the root",
+			[]string{"--active", "false"},
+			[]string{"--active", "false"}},
+		{"marked persistent flag before a command",
+			[]string{"--dry-run", "false", "deploy", "--stable", "true"},
+			[]string{"--dry-run=false", "deploy", "--stable=true"}},
+		{"marked persistent flag in a command",
+			[]string{"status", "--dry-run", "true"},
+			[]string{"status", "--dry-run=true"}},
+		{"flag values do not select a command",
+			[]string{"--output", "deploy", "status", "--stable", "false"},
+			[]string{"--output", "deploy", "status", "--stable", "false"}},
 		{"short flags are not candidates", []string{"-v", "false"}, []string{"-v", "false"}},
 		{"completion keeps the word being completed",
 			[]string{cobra.ShellCompRequestCmd, "deploy", "--active", "false", "--stable", "fal"},

live/live.go

@@ -3,6 +3,7 @@ package live
 import (
 	"fmt"
 	"io"
+	"slices"
 	"strings"
 
 	"github.com/mattn/go-runewidth"
@@ -87,7 +88,9 @@ func (l *Live) Draw(lines []string) error {
 	l.last = frame
 	l.drawn = nil
 	if l.Fits(lines) {
-		l.drawn = lines
+		// A copy: the caller may reuse its slice for the next frame, while
+		// the screen still shows these lines.
+		l.drawn = slices.Clone(lines)
 	}
 	return nil
 }

live/live_test.go

@@ -49,6 +49,23 @@ func TestLiveCountsRowsAtTheCurrentWidth(t *testing.T) {
 	}
 }
 
+// A caller that reuses its slice for the next frame does not change what
+// the next draw moves up over: the screen still shows the old lines.
+func TestLiveKeepsItsOwnCopyOfTheDrawnLines(t *testing.T) {
+	var out bytes.Buffer
+	live := New(&out)
+	live.size = func() (int, int) { return 4, 24 }
+
+	frame := []string{"abcdefgh"}
+	_ = live.Draw(frame)
+	out.Reset()
+	frame[0] = "x"
+	_ = live.Draw(frame)
+	if got, want := out.String(), "\r\x1b[2A\x1b[Jx\n"; got != want {
+		t.Errorf("redraw = %q, want %q", got, want)
+	}
+}
+
 // Lines taller than the screen lose their first lines, so the next draw can
 // still move back up to where this one started; the cursor's row counts too.
 func TestLiveKeepsToTheScreen(t *testing.T) {

pick/list.go

@@ -55,6 +55,9 @@ type list struct {
 	loaded   []bool
 	failures []string
 	pending  bool
+	// pendingKey is the key of the row selected when the pending view was
+	// switched to, for the cursor to land on once its rows are loaded.
+	pendingKey string
 
 	typed     func(string) bool
 	typedNote string
@@ -126,8 +129,8 @@ func newList(opts Options, height int, color bool) *list {
 			}
 		}
 	}
+	l.applyViewTitle()
 	if l.scoped {
-		l.title = firstNonEmpty(l.views[l.view].Title, l.fallback)
 		l.setItems(l.views[l.view].Items)
 		l.pending = l.needsLoad()
 	} else {
@@ -351,6 +354,7 @@ func (l *list) switchView(delta int) {
 	n := len(l.views)
 	if !l.scoped {
 		l.view = (l.view + delta + n) % n
+		l.applyViewTitle()
 		l.setFilter(l.filter)
 		return
 	}
@@ -360,14 +364,26 @@ func (l *list) switchView(delta int) {
 	}
 	l.view = (l.view + delta + n) % n
 	l.pending = l.needsLoad()
+	l.pendingKey = ""
+	if l.pending {
+		l.pendingKey = key
+	}
 	l.loadView(key)
 }
 
+// applyViewTitle shows the current view's title, or the options' title for
+// a view without one.
+func (l *list) applyViewTitle() {
+	if len(l.views) > 0 {
+		l.title = firstNonEmpty(l.views[l.view].Title, l.fallback)
+	}
+}
+
 // loadView makes the current view's own rows the ones on screen. The filter
 // carries over, and the cursor lands on the row carrying key, or at the top.
 func (l *list) loadView(key string) {
 	view := l.views[l.view]
-	l.title = firstNonEmpty(view.Title, l.fallback)
+	l.applyViewTitle()
 	l.setItems(view.Items)
 	l.cursor, l.top = 0, 0
 	l.applyFilter()
@@ -394,6 +410,8 @@ func (l *list) loadPending() bool { return l.pending }
 // the fetch blocks.
 func (l *list) runPendingLoad() {
 	l.pending = false
+	key := l.pendingKey
+	l.pendingKey = ""
 	view := l.views[l.view]
 	if view.Load == nil || l.loaded[l.view] {
 		return
@@ -409,7 +427,7 @@ func (l *list) runPendingLoad() {
 		}
 	}
 	l.views[l.view].Items = items
-	l.loadView("")
+	l.loadView(key)
 }
 
 // toggleMark marks the row under the cursor or unmarks it. The cursor stays,