diff --git a/README.md b/README.md index 6149709..c92f421 100644 --- a/README.md +++ b/README.md @@ -276,10 +276,6 @@ account's trusted device, and each asking for confirmation unless you pass `--yes`. Remove the default and, if a single account is left, it takes over; if several remain, dwshell asks you to pick one rather than choosing for you. -Flags follow their subcommand — `dwshell list --account …`, not -`dwshell --account … list` — except in the shell shortcut, where -`dwshell --account … myserver` works too. - #### Agent name vs subcommand `dwshell ` is a convenience shortcut: the first argument is treated as an diff --git a/cmd/dwshell/account.go b/cmd/dwshell/account.go index 897d4d5..49a41df 100644 --- a/cmd/dwshell/account.go +++ b/cmd/dwshell/account.go @@ -15,18 +15,21 @@ import ( // who log in with more than one account. With a single account there is nothing // here worth running. func cmdAccount(ctx context.Context, args []string) int { - if len(args) == 0 { + // The verb is the first positional, so flags may sit on either side of it. + flagArgs, pos := partitionArgs(args) + if len(pos) == 0 { return fail("usage: dwshell account ") } - switch args[0] { + rest := append(append([]string{}, flagArgs...), pos[1:]...) + switch pos[0] { case "list": - return cmdAccountList(args[1:]) + return cmdAccountList(rest) case "default": - return cmdAccountDefault(args[1:]) + return cmdAccountDefault(rest) case "rm": - return cmdAccountRemove(ctx, args[1:]) + return cmdAccountRemove(ctx, rest) default: - return fail("unknown account subcommand %q", args[0]) + return fail("unknown account subcommand %q", pos[0]) } } @@ -42,7 +45,8 @@ func cmdAccountList(args []string) int { asJSON := false fs.StringVar(&configPath, "config", "", "config file path") fs.BoolVar(&asJSON, "json", false, "machine-readable output") - if err := fs.Parse(args); err != nil { + flagArgs, _ := partitionArgs(args) + if err := fs.Parse(flagArgs); err != nil { return 2 } cfg, err := config.Load(configPath) @@ -80,23 +84,24 @@ func cmdAccountDefault(args []string) int { fs := newFlags("account default") var configPath string fs.StringVar(&configPath, "config", "", "config file path") - if err := fs.Parse(args); err != nil { + flagArgs, pos := partitionArgs(args) + if err := fs.Parse(flagArgs); err != nil { return 2 } - if fs.NArg() != 1 { + if len(pos) != 1 { return fail("usage: dwshell account default ") } cfg, err := config.Load(configPath) if err != nil { return fail("%v", err) } - if err := cfg.SetDefault(fs.Arg(0)); err != nil { + if err := cfg.SetDefault(pos[0]); err != nil { return fail("%v", err) } if err := cfg.Save(); err != nil { return fail("%v", err) } - fmt.Fprintf(os.Stderr, "default account is now %s\n", fs.Arg(0)) + fmt.Fprintf(os.Stderr, "default account is now %s\n", pos[0]) return 0 } @@ -106,10 +111,14 @@ func cmdAccountRemove(ctx context.Context, args []string) int { assumeYes := false fs.StringVar(&configPath, "config", "", "config file path") fs.BoolVar(&assumeYes, "yes", false, "do not ask for confirmation") - email, flagArgs := extractPositional(args) + flagArgs, pos := partitionArgs(args) if err := fs.Parse(flagArgs); err != nil { return 2 } + email := "" + if len(pos) > 0 { + email = pos[0] + } if email == "" { return fail("usage: dwshell account rm [--yes]") } @@ -130,28 +139,6 @@ func cmdAccountRemove(ctx context.Context, args []string) int { return 0 } -// extractPositional pulls the first non-flag token out of a subcommand's args, -// returning it with the flags that surrounded it. Go's flag package stops at -// the first positional, so `account rm a@b --yes` would otherwise drop --yes; -// this mirrors what extractAgent does for the shell shortcut. -func extractPositional(args []string) (pos string, flagArgs []string) { - i := 0 - for i < len(args) { - a := args[i] - if len(a) > 0 && a[0] == '-' { - flagArgs = append(flagArgs, a) - if !containsEq(a) && valueFlags[trimDashes(a)] && i+1 < len(args) { - i++ - flagArgs = append(flagArgs, args[i]) - } - i++ - continue - } - return a, append(flagArgs, args[i+1:]...) - } - return "", flagArgs -} - // confirm gates a change that cannot be undone. With a terminal it asks; with // none — a script, a CI job — it refuses unless the caller passed --yes, so // automation cannot drop an account by accident. diff --git a/cmd/dwshell/agent.go b/cmd/dwshell/agent.go index 990293b..004fc84 100644 --- a/cmd/dwshell/agent.go +++ b/cmd/dwshell/agent.go @@ -65,22 +65,25 @@ func printAgentJSON(a agentJSON) { // cmdAgentManage dispatches the `dwshell agent ` family. It is separate // from cmdAgent, which is the shell shortcut for `dwshell `. func cmdAgentManage(ctx context.Context, args []string) int { - if len(args) == 0 { + // The verb is the first positional, so flags may sit on either side of it. + flagArgs, pos := partitionArgs(args) + if len(pos) == 0 { return fail("usage: dwshell agent …") } - switch args[0] { + rest := append(append([]string{}, flagArgs...), pos[1:]...) + switch pos[0] { case "create": - return cmdAgentCreate(ctx, args[1:]) + return cmdAgentCreate(ctx, rest) case "code": - return cmdAgentCode(ctx, args[1:]) + return cmdAgentCode(ctx, rest) case "rm": - return cmdAgentLifecycle(ctx, args[1:], "rm") + return cmdAgentLifecycle(ctx, rest, "rm") case "reinstall": - return cmdAgentLifecycle(ctx, args[1:], "reinstall") + return cmdAgentLifecycle(ctx, rest, "reinstall") case "group": - return cmdAgentGroup(ctx, args[1:]) + return cmdAgentGroup(ctx, rest) default: - return fail("unknown agent subcommand %q", args[0]) + return fail("unknown agent subcommand %q", pos[0]) } } @@ -94,11 +97,15 @@ func cmdAgentCreate(ctx context.Context, args []string) int { fs.StringVar(&description, "description", "", "free-text description") fs.StringVar(&group, "group", "", "existing group to place the agent in") fs.BoolVar(&asJSON, "json", false, "machine-readable output") - name, flagArgs := extractPositional(args) + flagArgs, pos := partitionArgs(args) if err := fs.Parse(flagArgs); err != nil { return 2 } - if name == "" || fs.NArg() != 0 { + name := "" + if len(pos) > 0 { + name = pos[0] + } + if name == "" || len(pos) != 1 { return fail("usage: dwshell agent create [--group G] [--description D] [--json]") } @@ -194,10 +201,14 @@ func cmdAgentCode(ctx context.Context, args []string) int { fs.StringVar(&configPath, "config", "", "config file path") fs.StringVar(&account, "account", "", "account to use when several are logged in") fs.BoolVar(&asJSON, "json", false, "machine-readable output") - name, flagArgs := extractPositional(args) + flagArgs, pos := partitionArgs(args) if err := fs.Parse(flagArgs); err != nil { return 2 } + name := "" + if len(pos) > 0 { + name = pos[0] + } if name == "" { return fail("usage: dwshell agent code [--json]") } @@ -228,10 +239,14 @@ func cmdAgentLifecycle(ctx context.Context, args []string, verb string) int { fs.StringVar(&configPath, "config", "", "config file path") fs.StringVar(&account, "account", "", "account to use when several are logged in") fs.BoolVar(&assumeYes, "yes", false, "do not ask for confirmation") - name, flagArgs := extractPositional(args) + flagArgs, pos := partitionArgs(args) if err := fs.Parse(flagArgs); err != nil { return 2 } + name := "" + if len(pos) > 0 { + name = pos[0] + } if name == "" { return fail("usage: dwshell agent %s [--yes]", verb) } @@ -284,16 +299,20 @@ func cmdAgentGroup(ctx context.Context, args []string) int { fs.StringVar(&configPath, "config", "", "config file path") fs.StringVar(&account, "account", "", "account to use when several are logged in") fs.BoolVar(&none, "none", false, "remove the agent from its group") - name, flagArgs := extractPositional(args) + flagArgs, pos := partitionArgs(args) if err := fs.Parse(flagArgs); err != nil { return 2 } + name := "" + if len(pos) > 0 { + name = pos[0] + } groupName := "" if !none { - if fs.NArg() != 1 { + if len(pos) != 2 { return fail("usage: dwshell agent group (or --none)") } - groupName = fs.Arg(0) + groupName = pos[1] } if name == "" { return fail("usage: dwshell agent group (or --none)") diff --git a/cmd/dwshell/args.go b/cmd/dwshell/args.go new file mode 100644 index 0000000..2521e0c --- /dev/null +++ b/cmd/dwshell/args.go @@ -0,0 +1,50 @@ +package main + +import "strings" + +// valueFlags names every flag that consumes a following token as its value. +// +// This is what tells a value apart from a positional: given `--account a@b +// list`, only this registry says that `a@b` belongs to `--account` and `list` +// is the command. A value flag missing from here would make dwshell take the +// value for a command name and quietly do the wrong thing, so a test walks the +// package's own source and holds this list to the flags actually registered. +var valueFlags = map[string]bool{ + "account": true, + "c": true, + "config": true, + "description": true, + "group": true, + "term": true, + "timeout": true, + "user": true, +} + +// partitionArgs splits arguments into flags (each with its value) and +// positionals, in their original relative order, so that flags may be written +// anywhere: `dwshell list --account a@b` and `dwshell --account a@b list` are +// the same command. +// +// Everything after a bare "--" is positional, which is how a path or an agent +// name that begins with a dash can still be passed. +func partitionArgs(args []string) (flags, positionals []string) { + for i := 0; i < len(args); i++ { + a := args[i] + if a == "--" { + positionals = append(positionals, args[i+1:]...) + return flags, positionals + } + if len(a) > 1 && a[0] == '-' { + flags = append(flags, a) + // --flag=value carries its value; a bare value flag takes the next + // token, which must not then be read as a positional. + if !strings.Contains(a, "=") && valueFlags[trimDashes(a)] && i+1 < len(args) { + i++ + flags = append(flags, args[i]) + } + continue + } + positionals = append(positionals, a) + } + return flags, positionals +} diff --git a/cmd/dwshell/args_test.go b/cmd/dwshell/args_test.go new file mode 100644 index 0000000..0378862 --- /dev/null +++ b/cmd/dwshell/args_test.go @@ -0,0 +1,110 @@ +package main + +import ( + "go/ast" + "go/parser" + "go/token" + "path/filepath" + "reflect" + "testing" +) + +func TestPartitionArgsSeparatesFlagsFromPositionals(t *testing.T) { + cases := []struct { + name string + args []string + flags []string + pos []string + }{ + {"flags first", []string{"--account", "a@b", "list"}, []string{"--account", "a@b"}, []string{"list"}}, + {"flags last", []string{"list", "--account", "a@b"}, []string{"--account", "a@b"}, []string{"list"}}, + {"flags around", []string{"-c", "ls", "GHE", "--term", "xterm"}, + []string{"-c", "ls", "--term", "xterm"}, []string{"GHE"}}, + {"equals form needs no lookahead", []string{"--account=a@b", "list"}, []string{"--account=a@b"}, []string{"list"}}, + {"boolean flags consume nothing", []string{"list", "--json"}, []string{"--json"}, []string{"list"}}, + {"several positionals keep their order", []string{"get", "-r", "a:b", "c"}, + []string{"-r"}, []string{"get", "a:b", "c"}}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + flags, pos := partitionArgs(tc.args) + if !reflect.DeepEqual(flags, tc.flags) { + t.Errorf("flags = %v, want %v", flags, tc.flags) + } + if !reflect.DeepEqual(pos, tc.pos) { + t.Errorf("positionals = %v, want %v", pos, tc.pos) + } + }) + } +} + +// Everything after "--" is positional, so a path or command that looks like a +// flag can still be passed. +func TestPartitionArgsStopsAtADoubleDash(t *testing.T) { + flags, pos := partitionArgs([]string{"rm", "--yes", "--", "-weird-name"}) + if !reflect.DeepEqual(flags, []string{"--yes"}) { + t.Errorf("flags = %v", flags) + } + if !reflect.DeepEqual(pos, []string{"rm", "-weird-name"}) { + t.Errorf("positionals = %v", pos) + } +} + +// valueFlags decides where a positional begins, so a flag that takes a value +// and is missing from it makes dwshell mistake that value for a command name — +// silently doing the wrong thing rather than failing. This walks the package's +// own source and holds the registry to what is actually registered. +func TestValueFlagsMatchesTheFlagsActuallyRegistered(t *testing.T) { + files, err := filepath.Glob("*.go") + if err != nil { + t.Fatal(err) + } + found := map[string]string{} // flag name -> the FlagSet method that registers it + fset := token.NewFileSet() + for _, f := range files { + file, err := parser.ParseFile(fset, f, nil, 0) + if err != nil { + t.Fatalf("parse %s: %v", f, err) + } + ast.Inspect(file, func(n ast.Node) bool { + call, ok := n.(*ast.CallExpr) + if !ok { + return true + } + sel, ok := call.Fun.(*ast.SelectorExpr) + if !ok || len(call.Args) < 2 { + return true + } + method := sel.Sel.Name + // Var methods take (ptr, name, default, usage); Bool ones consume no + // following token, every other kind does. + if len(method) < 4 || method[len(method)-3:] != "Var" { + return true + } + lit, ok := call.Args[1].(*ast.BasicLit) + if !ok || lit.Kind != token.STRING { + return true + } + name := lit.Value[1 : len(lit.Value)-1] + if method == "BoolVar" { + return true + } + found[name] = method + return true + }) + } + if len(found) == 0 { + t.Fatal("no flags found: the walk is not seeing the source") + } + for name, method := range found { + if !valueFlags[name] { + t.Errorf("--%s takes a value (%s) but is missing from valueFlags: "+ + "a positional after it would be mistaken for the flag's value", name, method) + } + } + for name := range valueFlags { + if _, ok := found[name]; !ok { + t.Errorf("valueFlags lists --%s, which no command registers as a value flag", name) + } + } +} diff --git a/cmd/dwshell/files.go b/cmd/dwshell/files.go index a6ab012..14a0235 100644 --- a/cmd/dwshell/files.go +++ b/cmd/dwshell/files.go @@ -105,7 +105,11 @@ func cmdLs(ctx context.Context, args []string) int { fs.BoolVar(&own, "own", false, "owned agents only") fs.BoolVar(&shared, "shared", false, "incoming shares only") - endpoint, rest := extractAgent(args) + rest, pos := partitionArgs(args) + endpoint := "" + if len(pos) > 0 { + endpoint = pos[0] + } if endpoint == "" { return fail("usage: dwshell ls [:]") } diff --git a/cmd/dwshell/files_test.go b/cmd/dwshell/files_test.go index 8f5458f..8ea68fd 100644 --- a/cmd/dwshell/files_test.go +++ b/cmd/dwshell/files_test.go @@ -1,9 +1,6 @@ package main -import ( - "strings" - "testing" -) +import "testing" func TestParseRemote(t *testing.T) { tests := []struct { @@ -91,17 +88,3 @@ func TestIsRemoteEndpoint(t *testing.T) { } } } - -// --account takes a value, so the shell shortcut has to skip that value when -// looking for the agent name — otherwise `dwshell --account a@b GHE` would take -// the email for the agent. -func TestExtractAgentSkipsTheAccountValue(t *testing.T) { - agent, flags := extractAgent([]string{"--account", "a@b", "GHE", "-c", "ls"}) - if agent != "GHE" { - t.Fatalf("agent = %q, want GHE", agent) - } - joined := strings.Join(flags, " ") - if !strings.Contains(joined, "--account a@b") || !strings.Contains(joined, "-c ls") { - t.Fatalf("flags = %v", flags) - } -} diff --git a/cmd/dwshell/main.go b/cmd/dwshell/main.go index 78c20f1..240b67d 100644 --- a/cmd/dwshell/main.go +++ b/cmd/dwshell/main.go @@ -29,35 +29,6 @@ func newFlags(name string) *flag.FlagSet { return fs } -// valueFlags are agent-command flags that consume a following value token. -var valueFlags = map[string]bool{"c": true, "term": true, "config": true, "timeout": true, "account": true, "description": true, "group": true} - -// extractAgent pulls the first positional (the agent) out of args, returning it -// plus the remaining flag arguments in order. It skips the value token that -// follows a value flag so `dwshell GHE -c "ls"` and `dwshell -c "ls" GHE` both -// work. -func extractAgent(args []string) (agent string, flagArgs []string) { - i := 0 - for i < len(args) { - a := args[i] - if len(a) > 0 && a[0] == '-' { - flagArgs = append(flagArgs, a) - name := trimDashes(a) - // -flag=value is self-contained; a bare value flag consumes the next. - if !containsEq(a) && valueFlags[name] && i+1 < len(args) { - i++ - flagArgs = append(flagArgs, args[i]) - } - i++ - continue - } - agent = a - flagArgs = append(flagArgs, args[i+1:]...) - return agent, flagArgs - } - return "", flagArgs -} - func trimDashes(s string) string { for len(s) > 0 && s[0] == '-' { s = s[1:] @@ -136,6 +107,20 @@ func run() int { ctx, stop := signal.NotifyContext(context.Background(), os.Interrupt) defer stop() + // Flags may be written anywhere, so the command is the first positional + // rather than simply os.Args[1]: `dwshell --account a@b list` and + // `dwshell list --account a@b` are the same command. What follows it is + // handed on with the surrounding flags reattached. + globalFlags, pos := partitionArgs(os.Args[1:]) + command := "" + if len(pos) > 0 { + command = pos[0] + } + sub := func() []string { + rest := append([]string{}, globalFlags...) + return append(rest, pos[1:]...) + } + switch os.Args[1] { case "-h", "--help", "help": fmt.Print(usage) @@ -143,30 +128,39 @@ func run() int { case "-V", "--version", "version": fmt.Println("dwshell " + versionString()) return 0 + } + + switch command { + case "help": + fmt.Print(usage) + return 0 + case "version": + fmt.Println("dwshell " + versionString()) + return 0 case "login": - return cmdLogin(ctx, os.Args[2:]) + return cmdLogin(ctx, sub()) case "account": - return cmdAccount(ctx, os.Args[2:]) + return cmdAccount(ctx, sub()) case "logout": - return cmdLogout(ctx, os.Args[2:]) + return cmdLogout(ctx, sub()) case "list": - return cmdList(ctx, os.Args[2:]) + return cmdList(ctx, sub()) case "ls": - return cmdLs(ctx, os.Args[2:]) + return cmdLs(ctx, sub()) case "get": - return cmdGet(ctx, os.Args[2:]) + return cmdGet(ctx, sub()) case "put": - return cmdPut(ctx, os.Args[2:]) + return cmdPut(ctx, sub()) case "rm": - return cmdRm(ctx, os.Args[2:]) + return cmdRm(ctx, sub()) case "sync": - return cmdSync(ctx, os.Args[2:]) + return cmdSync(ctx, sub()) case "agent": - return cmdAgentManage(ctx, os.Args[2:]) + return cmdAgentManage(ctx, sub()) case "shell": // Explicit form: the next argument is always an agent, even if it happens // to be named like a subcommand (e.g. `dwshell shell version`). - return cmdAgent(ctx, os.Args[2:]) + return cmdAgent(ctx, sub()) default: // Shortcut form: `dwshell [flags]`. If your agent is named like a // subcommand, use the explicit `dwshell shell ` form above. @@ -190,7 +184,8 @@ func cmdLogin(ctx context.Context, args []string) int { fs.StringVar(&configPath, "config", "", "config path") fs.StringVar(&account, "account", "", "account to use when several are logged in") fs.BoolVar(&noTrusted, "no-trusted", false, "do not register a trusted device") - if err := fs.Parse(args); err != nil { + flagArgs, _ := partitionArgs(args) + if err := fs.Parse(flagArgs); err != nil { return 2 } @@ -254,7 +249,8 @@ func cmdLogout(ctx context.Context, args []string) int { fs.StringVar(&configPath, "config", "", "config path") fs.StringVar(&account, "account", "", "account to use when several are logged in") fs.BoolVar(&all, "all", false, "forget every account") - if err := fs.Parse(args); err != nil { + flagArgs, _ := partitionArgs(args) + if err := fs.Parse(flagArgs); err != nil { return 2 } if all { @@ -291,7 +287,8 @@ func cmdList(ctx context.Context, args []string) int { fs.StringVar(&configPath, "config", "", "config path") fs.StringVar(&account, "account", "", "account to use when several are logged in") fs.BoolVar(&asJSON, "json", false, "JSON output") - if err := fs.Parse(args); err != nil { + flagArgs, _ := partitionArgs(args) + if err := fs.Parse(flagArgs); err != nil { return 2 } c, err := client.New(configPath, account) @@ -346,7 +343,11 @@ func cmdAgent(ctx context.Context, args []string) int { fs.BoolVar(&noTerm, "no-term", false, "do not send TERM") fs.DurationVar(&timeout, "timeout", 0, "command timeout for -c (0 = no timeout)") - agentArg, rest := extractAgent(args) + rest, pos := partitionArgs(args) + agentArg := "" + if len(pos) > 0 { + agentArg = pos[0] + } if agentArg == "" { return fail("an agent is required (see `dwshell --help`)") }