cmd/tailscale/cli: improve ShortHelp/ShortUsage unit test, fix new errors

Updates #11364

Signed-off-by: Paul Scott <paul@tailscale.com>

cmd/tailscale/cli/cli.go

@@ -103,8 +103,9 @@ func Run(args []string) (err error) {
 	}
 
 	if envknob.Bool("TS_DUMP_HELP") {
-		walkCommands(rootCmd, func(c *ffcli.Command) {
+		walkCommands(rootCmd, func(w cmdWalk) bool {
 			fmt.Println("===")
+			c := w.cmd
 			// UsageFuncs are typically called during Command.Run which ensures
 			// FlagSet is not nil.
 			if c.FlagSet == nil {
@@ -115,6 +116,7 @@ func Run(args []string) (err error) {
 			} else {
 				fmt.Println(ffcli.DefaultUsageFunc(c))
 			}
+			return true
 		})
 		return
 	}
@@ -192,10 +194,11 @@ change in the future.
 		rootCmd.Subcommands = append(rootCmd.Subcommands, configureHostCmd)
 	}
 
-	walkCommands(rootCmd, func(c *ffcli.Command) {
-		if c.UsageFunc == nil {
-			c.UsageFunc = usageFunc
+	walkCommands(rootCmd, func(w cmdWalk) bool {
+		if w.cmd.UsageFunc == nil {
+			w.cmd.UsageFunc = usageFunc
 		}
+		return true
 	})
 	return rootCmd
 }
@@ -216,11 +219,28 @@ var rootArgs struct {
 	socket string
 }
 
-func walkCommands(cmd *ffcli.Command, f func(*ffcli.Command)) {
-	f(cmd)
+type cmdWalk struct {
+	cmd     *ffcli.Command
+	parents []*ffcli.Command
+}
+
+// walkCommands calls f for root and all of its nested subcommands until f
+// returns false or all have been visited.
+func walkCommands(root *ffcli.Command, f func(w cmdWalk) (more bool)) {
+	var walk func(cmd *ffcli.Command, parents []*ffcli.Command, f func(cmdWalk) bool) bool
+	walk = func(cmd *ffcli.Command, parents []*ffcli.Command, f func(cmdWalk) bool) bool {
+		if !f(cmdWalk{cmd, parents}) {
+			return false
+		}
+		parents = append(parents, cmd)
 		for _, sub := range cmd.Subcommands {
-		walkCommands(sub, f)
+			if !walk(sub, parents, f) {
+				return false
+			}
+		}
+		return true
 	}
+	walk(root, nil, f)
 }
 
 // usageFuncNoDefaultValues is like usageFunc but doesn't print default values.

cmd/tailscale/cli/cli_test.go

@@ -16,7 +16,6 @@ import (
 
 	qt "github.com/frankban/quicktest"
 	"github.com/google/go-cmp/cmp"
-	"github.com/peterbourgon/ff/v3/ffcli"
 	"tailscale.com/envknob"
 	"tailscale.com/health/healthmsg"
 	"tailscale.com/ipn"
@@ -34,28 +33,106 @@ func TestPanicIfAnyEnvCheckedInInit(t *testing.T) {
 	envknob.PanicIfAnyEnvCheckedInInit()
 }
 
-func TestShortUsage_FullCmd(t *testing.T) {
+func TestShortUsage(t *testing.T) {
 	t.Setenv("TAILSCALE_USE_WIP_CODE", "1")
 	if !envknob.UseWIPCode() {
 		t.Fatal("expected envknob.UseWIPCode() to be true")
 	}
 
-	// Some commands have more than one path from the root, so investigate all
-	// paths before we report errors.
-	ok := make(map[*ffcli.Command]bool)
-	root := newRootCmd()
-	walkCommands(root, func(c *ffcli.Command) {
-		if !ok[c] {
-			ok[c] = strings.HasPrefix(c.ShortUsage, "tailscale ") && (c.Name == "tailscale" || strings.Contains(c.ShortUsage, " "+c.Name+" ") || strings.HasSuffix(c.ShortUsage, " "+c.Name))
+	walkCommands(newRootCmd(), func(w cmdWalk) bool {
+		c, parents := w.cmd, w.parents
+
+		// Words that we expect to be in the usage.
+		words := make([]string, len(parents)+1)
+		for i, parent := range parents {
+			words[i] = parent.Name
 		}
-	})
-	walkCommands(root, func(c *ffcli.Command) {
-		if !ok[c] {
-			t.Errorf("subcommand %s should show full usage ('tailscale ... %s ...') in ShortUsage (%q)", c.Name, c.Name, c.ShortUsage)
+		words[len(parents)] = c.Name
+
+		// Check the ShortHelp starts with a capital letter.
+		if prefix, help := trimPrefixes(c.ShortHelp, "HIDDEN: ", "[ALPHA] ", "[BETA] "); help != "" {
+			if 'a' <= help[0] && help[0] <= 'z' {
+				if len(help) > 20 {
+					help = help[:20] + "…"
+				}
+				caphelp := string(help[0]-'a'+'A') + help[1:]
+				t.Errorf("command: %s: ShortHelp %q should start with a capital letter %q", strings.Join(words, " "), prefix+help, prefix+caphelp)
 			}
+		}
+
+		// Check all words appear in the usage.
+		usage := c.ShortUsage
+		for _, word := range words {
+			var ok bool
+			usage, ok = cutWord(usage, word)
+			if !ok {
+				full := strings.Join(words, " ")
+				t.Errorf("command: %s: usage %q should contain the full path %q", full, c.ShortUsage, full)
+				return true
+			}
+		}
+		return true
 	})
 }
 
+func trimPrefixes(full string, prefixes ...string) (trimmed, remaining string) {
+	s := full
+start:
+	for _, p := range prefixes {
+		var ok bool
+		s, ok = strings.CutPrefix(s, p)
+		if ok {
+			goto start
+		}
+	}
+	return full[:len(full)-len(s)], s
+}
+
+// cutWord("tailscale debug scale 123", "scale") returns (" 123", true).
+func cutWord(s, w string) (after string, ok bool) {
+	var p string
+	for {
+		p, s, ok = strings.Cut(s, w)
+		if !ok {
+			return "", false
+		}
+		if p != "" && isWordChar(p[len(p)-1]) {
+			continue
+		}
+		if s != "" && isWordChar(s[0]) {
+			continue
+		}
+		return s, true
+	}
+}
+
+func isWordChar(r byte) bool {
+	return r == '_' ||
+		('0' <= r && r <= '9') ||
+		('A' <= r && r <= 'Z') ||
+		('a' <= r && r <= 'z')
+}
+
+func TestCutWord(t *testing.T) {
+	tests := []struct {
+		in   string
+		word string
+		out  string
+		ok   bool
+	}{
+		{"tailscale debug", "debug", "", true},
+		{"tailscale debug", "bug", "", false},
+		{"tailscale debug", "tail", "", false},
+		{"tailscale debug scaley scale 123", "scale", " 123", true},
+	}
+	for _, test := range tests {
+		out, ok := cutWord(test.in, test.word)
+		if out != test.out || ok != test.ok {
+			t.Errorf("cutWord(%q, %q) = (%q, %t), wanted (%q, %t)", test.in, test.word, out, ok, test.out, test.ok)
+		}
+	}
+}
+
 // geese is a collection of gooses. It need not be complete.
 // But it should include anything handled specially (e.g. linux, windows)
 // and at least one thing that's not (darwin, freebsd).

cmd/tailscale/cli/debug.go

@@ -227,8 +227,8 @@ var debugCmd = &ffcli.Command{
 		},
 		{
 			Name: "via",
-			ShortUsage: "tailscale via <site-id> <v4-cidr>\n" +
-				"tailscale via <v6-route>",
+			ShortUsage: "tailscale debug via <site-id> <v4-cidr>\n" +
+				"tailscale debug via <v6-route>",
 			Exec:      runVia,
 			ShortHelp: "Convert between site-specific IPv4 CIDRs and IPv6 'via' routes",
 		},

cmd/tailscale/cli/drive.go

@@ -36,24 +36,24 @@ var driveCmd = &ffcli.Command{
 			Name:       "share",
 			ShortUsage: driveShareUsage,
 			Exec:       runDriveShare,
-			ShortHelp:  "[ALPHA] create or modify a share",
+			ShortHelp:  "[ALPHA] Create or modify a share",
 		},
 		{
 			Name:       "rename",
 			ShortUsage: driveRenameUsage,
-			ShortHelp:  "[ALPHA] rename a share",
+			ShortHelp:  "[ALPHA] Rename a share",
 			Exec:       runDriveRename,
 		},
 		{
 			Name:       "unshare",
 			ShortUsage: driveUnshareUsage,
-			ShortHelp:  "[ALPHA] remove a share",
+			ShortHelp:  "[ALPHA] Remove a share",
 			Exec:       runDriveUnshare,
 		},
 		{
 			Name:       "list",
 			ShortUsage: driveListUsage,
-			ShortHelp:  "[ALPHA] list current shares",
+			ShortHelp:  "[ALPHA] List current shares",
 			Exec:       runDriveList,
 		},
 	},

cmd/tailscale/cli/exitnode.go

@@ -55,13 +55,13 @@ func exitNodeCmd() *ffcli.Command {
 					{
 						Name:       "connect",
 						ShortUsage: "tailscale exit-node connect",
-						ShortHelp:  "connect to most recently used exit node",
+						ShortHelp:  "Connect to most recently used exit node",
 						Exec:       exitNodeSetUse(true),
 					},
 					{
 						Name:       "disconnect",
 						ShortUsage: "tailscale exit-node disconnect",
-						ShortHelp:  "disconnect from current exit node, if any",
+						ShortHelp:  "Disconnect from current exit node, if any",
 						Exec:       exitNodeSetUse(false),
 					},
 				}