cmd/tailscale/cli: use double hypens, make default usage func more clear

Mash up some code from ffcli and std's flag package to make a default
usage func that's super explicit for those not familiar with the Go
style flags. Only show double hyphens in usage text (but still accept both),
and show default values, and only show the proper usage of boolean flags.

Fixes #1353
Fixes #1529

Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>

cmd/tailscale/cli/cli.go

@@ -9,6 +9,7 @@ package cli
 import (
 	"context"
 	"flag"
+	"fmt"
 	"log"
 	"net"
 	"os"
@@ -16,6 +17,7 @@ import (
 	"runtime"
 	"strings"
 	"syscall"
+	"text/tabwriter"
 
 	"github.com/peterbourgon/ff/v2/ffcli"
 	"tailscale.com/ipn"
@@ -53,9 +55,7 @@ func Run(args []string) error {
 		ShortUsage: "tailscale [flags] <subcommand> [command flags]",
 		ShortHelp:  "The easiest, most secure way to use WireGuard.",
 		LongHelp: strings.TrimSpace(`
-For help on subcommands, add -help after: "tailscale status -help".
+For help on subcommands, add --help after: "tailscale status --help".
-
-All flags can use single or double hyphen prefixes (-help or --help).
 
 This CLI is still under active development. Commands and flags will
 change in the future.
@@ -68,8 +68,12 @@ change in the future.
 			pingCmd,
 			versionCmd,
 		},
-		FlagSet: rootfs,
+		FlagSet:   rootfs,
-		Exec:    func(context.Context, []string) error { return flag.ErrHelp },
+		Exec:      func(context.Context, []string) error { return flag.ErrHelp },
+		UsageFunc: usageFunc,
+	}
+	for _, c := range rootCmd.Subcommands {
+		c.UsageFunc = usageFunc
 	}
 
 	// Don't advertise the debug command, but it exists.
@@ -147,3 +151,72 @@ func strSliceContains(ss []string, s string) bool {
 	}
 	return false
 }
+
+func usageFunc(c *ffcli.Command) string {
+	var b strings.Builder
+
+	fmt.Fprintf(&b, "USAGE\n")
+	if c.ShortUsage != "" {
+		fmt.Fprintf(&b, "  %s\n", c.ShortUsage)
+	} else {
+		fmt.Fprintf(&b, "  %s\n", c.Name)
+	}
+	fmt.Fprintf(&b, "\n")
+
+	if c.LongHelp != "" {
+		fmt.Fprintf(&b, "%s\n\n", c.LongHelp)
+	}
+
+	if len(c.Subcommands) > 0 {
+		fmt.Fprintf(&b, "SUBCOMMANDS\n")
+		tw := tabwriter.NewWriter(&b, 0, 2, 2, ' ', 0)
+		for _, subcommand := range c.Subcommands {
+			fmt.Fprintf(tw, "  %s\t%s\n", subcommand.Name, subcommand.ShortHelp)
+		}
+		tw.Flush()
+		fmt.Fprintf(&b, "\n")
+	}
+
+	if countFlags(c.FlagSet) > 0 {
+		fmt.Fprintf(&b, "FLAGS\n")
+		tw := tabwriter.NewWriter(&b, 0, 2, 2, ' ', 0)
+		c.FlagSet.VisitAll(func(f *flag.Flag) {
+			var s string
+			name, usage := flag.UnquoteUsage(f)
+			if isBoolFlag(f) {
+				s = fmt.Sprintf("  --%s, --%s=false", f.Name, f.Name)
+			} else {
+				s = fmt.Sprintf("  --%s", f.Name) // Two spaces before --; see next two comments.
+				if len(name) > 0 {
+					s += " " + name
+				}
+			}
+			// Four spaces before the tab triggers good alignment
+			// for both 4- and 8-space tab stops.
+			s += "\n    \t"
+			s += strings.ReplaceAll(usage, "\n", "\n    \t")
+
+			if f.DefValue != "" {
+				s += fmt.Sprintf(" (default %s)", f.DefValue)
+			}
+
+			fmt.Fprintln(&b, s)
+		})
+		tw.Flush()
+		fmt.Fprintf(&b, "\n")
+	}
+
+	return strings.TrimSpace(b.String())
+}
+
+func isBoolFlag(f *flag.Flag) bool {
+	bf, ok := f.Value.(interface {
+		IsBoolFlag() bool
+	})
+	return ok && bf.IsBoolFlag()
+}
+
+func countFlags(fs *flag.FlagSet) (n int) {
+	fs.VisitAll(func(*flag.Flag) { n++ })
+	return n
+}

cmd/tailscale/cli/status.go

@@ -27,7 +27,7 @@ import (
 
 var statusCmd = &ffcli.Command{
 	Name:       "status",
-	ShortUsage: "status [-active] [-web] [-json]",
+	ShortUsage: "status [--active] [--web] [--json]",
 	ShortHelp:  "Show state of tailscaled and its connections",
 	Exec:       runStatus,
 	FlagSet: (func() *flag.FlagSet {

cmd/tailscale/cli/up.go

@@ -49,11 +49,11 @@ specify any flags, options are reset to their default.
 		upf.StringVar(&upArgs.exitNodeIP, "exit-node", "", "Tailscale IP of the exit node for internet traffic")
 		upf.BoolVar(&upArgs.shieldsUp, "shields-up", false, "don't allow incoming connections")
 		upf.BoolVar(&upArgs.forceReauth, "force-reauth", false, "force reauthentication")
-		upf.StringVar(&upArgs.advertiseTags, "advertise-tags", "", "ACL tags to request (comma-separated, e.g. eng,montreal,ssh)")
+		upf.StringVar(&upArgs.advertiseTags, "advertise-tags", "", "ACL tags to request (comma-separated, e.g. \"tag:eng,tag:montreal,tag:ssh\")")
 		upf.StringVar(&upArgs.authKey, "authkey", "", "node authorization key")
 		upf.StringVar(&upArgs.hostname, "hostname", "", "hostname to use instead of the one provided by the OS")
 		if runtime.GOOS == "linux" || isBSD(runtime.GOOS) {
-			upf.StringVar(&upArgs.advertiseRoutes, "advertise-routes", "", "routes to advertise to other nodes (comma-separated, e.g. 10.0.0.0/8,192.168.0.0/24)")
+			upf.StringVar(&upArgs.advertiseRoutes, "advertise-routes", "", "routes to advertise to other nodes (comma-separated, e.g. \"10.0.0.0/8,192.168.0.0/24\")")
 			upf.BoolVar(&upArgs.advertiseDefaultRoute, "advertise-exit-node", false, "offer to be an exit node for internet traffic for the tailnet")
 		}
 		if runtime.GOOS == "linux" {

cmd/tailscale/depaware.txt

@@ -162,7 +162,7 @@ tailscale.com/cmd/tailscale dependencies: (generated by github.com/tailscale/dep
         sync                                                         from compress/flate+
         sync/atomic                                                  from context+
         syscall                                                      from crypto/rand+
-        text/tabwriter                                               from github.com/peterbourgon/ff/v2/ffcli
+        text/tabwriter                                               from github.com/peterbourgon/ff/v2/ffcli+
         time                                                         from compress/gzip+
         unicode                                                      from bytes+
         unicode/utf16                                                from encoding/asn1+