diff --git a/cmd/tailscale/cli/cli.go b/cmd/tailscale/cli/cli.go index fe6288ddf..674c2a390 100644 --- a/cmd/tailscale/cli/cli.go +++ b/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) - for _, sub := range cmd.Subcommands { - walkCommands(sub, f) +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 { + if !walk(sub, parents, f) { + return false + } + } + return true } + walk(root, nil, f) } // usageFuncNoDefaultValues is like usageFunc but doesn't print default values. diff --git a/cmd/tailscale/cli/cli_test.go b/cmd/tailscale/cli/cli_test.go index 52966a4bd..3d7cca51b 100644 --- a/cmd/tailscale/cli/cli_test.go +++ b/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). diff --git a/cmd/tailscale/cli/debug.go b/cmd/tailscale/cli/debug.go index b8ac7bd9b..3c95b75b8 100644 --- a/cmd/tailscale/cli/debug.go +++ b/cmd/tailscale/cli/debug.go @@ -227,8 +227,8 @@ var debugCmd = &ffcli.Command{ }, { Name: "via", - ShortUsage: "tailscale via \n" + - "tailscale via ", + ShortUsage: "tailscale debug via \n" + + "tailscale debug via ", Exec: runVia, ShortHelp: "Convert between site-specific IPv4 CIDRs and IPv6 'via' routes", }, diff --git a/cmd/tailscale/cli/drive.go b/cmd/tailscale/cli/drive.go index 5ee1186dd..f40742091 100644 --- a/cmd/tailscale/cli/drive.go +++ b/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, }, }, diff --git a/cmd/tailscale/cli/exitnode.go b/cmd/tailscale/cli/exitnode.go index 0e36c2b86..fc82fbf0a 100644 --- a/cmd/tailscale/cli/exitnode.go +++ b/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), }, }