diff --git a/AGENTS.md b/AGENTS.md index 1b58a77..2458385 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -15,6 +15,11 @@ - Integration tests should create local upstream/downstream repositories in `t.TempDir()`, configure local user identity, and disable GPG signing unless the test explicitly covers signing config propagation. +- Any change to the CLI interface, including commands, aliases, options, + positional arguments, or help forms, must include both unit-level and + integration completion tests. The tests must exercise completion at every + valid position affected by the change, including before and after positional + arguments where options are accepted. ## Non-Negotiable CI Parity diff --git a/integration/BUILD.bazel b/integration/BUILD.bazel index 41f7a16..32ad81c 100644 --- a/integration/BUILD.bazel +++ b/integration/BUILD.bazel @@ -41,6 +41,7 @@ go_test( data = ["//cmd/braid"], embed = [":test_support"], timeout = "short", + deps = ["//internal/clitest"], ) go_test( diff --git a/integration/completion_test.go b/integration/completion_test.go index 1fae318..98ab957 100644 --- a/integration/completion_test.go +++ b/integration/completion_test.go @@ -7,6 +7,8 @@ import ( "runtime" "strings" "testing" + + "braid/internal/clitest" ) func TestExecutableBashCompletion(t *testing.T) { @@ -25,17 +27,66 @@ func TestExecutableBashCompletion(t *testing.T) { assertCompletionCandidate(t, rootCandidates, "add") assertCompletionCandidate(t, rootCandidates, "pull") assertCompletionCandidate(t, rootCandidates, "completion") - assertNoCompletionCandidate(t, rootCandidates, "update") - assertNoCompletionCandidate(t, rootCandidates, "up") + assertCompletionCandidate(t, rootCandidates, "update") + assertCompletionCandidate(t, rootCandidates, "up") assertNoCompletionCandidate(t, rootCandidates, "__complete") globalFlags := completeExecutable(t, env, root, braid, "--") assertCompletionCandidate(t, globalFlags, "--verbose") assertCompletionCandidate(t, globalFlags, "--quiet") assertCompletionCandidate(t, globalFlags, "--global-cache-dir") + assertCompletionCandidate(t, globalFlags, "--help") + shortGlobalFlags := completeExecutable(t, env, root, braid, "-") + assertCompletionCandidate(t, shortGlobalFlags, "-h") + + completionArgs := completeExecutable(t, env, root, braid, "completion", "") + for _, candidate := range []string{"bash", "help", "--help", "-h"} { + assertCompletionCandidate(t, completionArgs, candidate) + } + + noRepoMirrors := completeExecutable(t, env, root, braid, "status", "") + assertCompletionCandidate(t, noRepoMirrors, "help") +} + +func TestExecutableBashCompletionCoversEveryCommandAndOptionPosition(t *testing.T) { + root := t.TempDir() + env := newProcessEnv(t, root) + braid := braidBinary(t) - noRepoMirrors := runBraid(t, env, root, braid, "__complete", "bash", "--", "status", "") - assertResult(t, noRepoMirrors, 0, "", "") + for _, test := range clitest.CompletionContractCases() { + t.Run(test.Name, func(t *testing.T) { + candidates := completeExecutable(t, env, root, braid, test.Words...) + if test.WantEmpty && len(candidates) != 0 { + t.Fatalf("completion candidates = %#v, want empty", candidates) + } + for _, want := range test.Want { + assertCompletionCandidate(t, candidates, want) + } + for _, unwanted := range test.Unwanted { + assertNoCompletionCandidate(t, candidates, unwanted) + } + }) + } +} + +func TestExecutableCLIEnforcesGeneratedPositionalConstraints(t *testing.T) { + root := t.TempDir() + env := newProcessEnv(t, root) + braid := braidBinary(t) + + for _, test := range clitest.SyntaxConstraintCases() { + t.Run(test.Name, func(t *testing.T) { + result := runBraid(t, env, root, braid, test.Args...) + if test.WantError != "" { + assertExit(t, result, 2) + assertContains(t, result.stderr, test.WantError) + return + } + if result.exitCode == 2 { + t.Fatalf("valid syntax %v returned usage exit: stderr = %q", test.Args, result.stderr) + } + }) + } } func TestExecutableBashCompletionMirrorPathsFromSubdirectory(t *testing.T) { diff --git a/internal/cli/BUILD.bazel b/internal/cli/BUILD.bazel index c9ecb25..ae50ae0 100644 --- a/internal/cli/BUILD.bazel +++ b/internal/cli/BUILD.bazel @@ -2,14 +2,20 @@ load("@rules_go//go:def.bzl", "go_library", "go_test") go_library( name = "cli", - srcs = ["cli.go"], + srcs = [ + "cli.go", + "schema.go", + ], importpath = "braid/internal/cli", visibility = ["//visibility:public"], ) go_test( name = "cli_test", - srcs = ["cli_test.go"], + srcs = [ + "cli_test.go", + "schema_test.go", + ], embed = [":cli"], timeout = "short", ) diff --git a/internal/cli/cli.go b/internal/cli/cli.go index bd598b4..33d9ff2 100644 --- a/internal/cli/cli.go +++ b/internal/cli/cli.go @@ -225,17 +225,10 @@ func Parse(args []string) (Invocation, error) { if len(rest) == 0 { return inv, usageError("missing command") } - if inv.Global.NoCache && inv.Global.GlobalCacheDirSet { - return inv, usageError("--no-cache and --global-cache-dir cannot be used together") - } - if inv.Global.Verbose && inv.Global.Quiet { - return inv, usageError("--quiet and --verbose cannot be used together") - } - commandText := rest[0] - if commandText == "help" || commandText == "--help" || commandText == "-h" { + if Help().Matches(commandText) { inv.Help = true - return inv, requireNoArgs("help", rest[1:]) + return inv, requireNoArgs(Help().Command, rest[1:]) } if strings.HasPrefix(commandText, "-") { return inv, usageError("unknown global flag %s", commandText) @@ -267,7 +260,8 @@ func Parse(args []string) (Invocation, error) { case CommandSync: return inv, parseSync(commandArgs, &inv.Sync) case CommandVersion: - return inv, requireNoArgs("version", commandArgs) + _, err := parseCommandSyntax(CommandVersion, commandArgs) + return inv, err case CommandStatus: return inv, parseStatus(commandArgs, &inv.Status) case CommandCompletion: @@ -282,81 +276,89 @@ func Parse(args []string) (Invocation, error) { } func parseGlobal(args []string, global *GlobalOptions) ([]string, error) { + options := GlobalOptionsSpec() + present := map[string]bool{} i := 0 for i < len(args) { arg := args[i] - switch { - case arg == "--no-cache": - global.NoCache = true - i++ - case arg == "--verbose" || arg == "-v": - global.Verbose = true - i++ - case arg == "--quiet": - global.Quiet = true - i++ - case arg == "--cache-dir" || strings.HasPrefix(arg, "--cache-dir="): + if arg == "--cache-dir" || strings.HasPrefix(arg, "--cache-dir=") { return nil, usageError("--cache-dir has been replaced by --global-cache-dir") - case arg == "--global-cache-dir": + } + name, inlineValue, hasEquals := strings.Cut(arg, "=") + option, matched := matchOption(name, options) + if !matched { + return args[i:], validateOptionConflicts(present, GlobalConflictsSpec()) + } + present[option.Long] = true + if !option.TakesValue() && hasEquals { + return nil, usageError("flag %s does not take a value", option.Long) + } + value := inlineValue + if option.TakesValue() && !hasEquals { if i+1 >= len(args) { - return nil, usageError("--global-cache-dir requires a value") - } - if args[i+1] == "" { - return nil, usageError("--global-cache-dir requires a non-empty value") - } - global.GlobalCacheDir = args[i+1] - global.GlobalCacheDirSet = true - i += 2 - case strings.HasPrefix(arg, "--global-cache-dir="): - value := strings.TrimPrefix(arg, "--global-cache-dir=") - if value == "" { - return nil, usageError("--global-cache-dir requires a non-empty value") + return nil, usageError("%s requires a value", option.Long) } + i++ + value = args[i] + } + if option.TakesValue() && value == "" { + return nil, usageError("%s requires a non-empty value", option.Long) + } + switch option.Long { + case "--no-cache": + global.NoCache = true + case "--verbose": + global.Verbose = true + case "--quiet": + global.Quiet = true + case "--global-cache-dir": global.GlobalCacheDir = value global.GlobalCacheDirSet = true - i++ - default: - return args[i:], nil + } + i++ + } + return nil, validateOptionConflicts(present, GlobalConflictsSpec()) +} + +func validateOptionConflicts(present map[string]bool, conflicts []ConflictSpec) error { + for _, conflict := range conflicts { + all := true + for _, option := range conflict.Options { + all = all && present[option] + } + if all { + return usageError("%s", conflict.Error) + } + } + return nil +} + +func matchOption(name string, options []OptionSpec) (OptionSpec, bool) { + for _, option := range options { + if name == option.Long || (option.Short != "" && name == option.Short) { + return option, true } } - return nil, nil + return OptionSpec{}, false } func parseAdd(args []string, options *AddOptions) error { - var positionals []string - err := parseCommandArgs(CommandAdd, args, []flagSpec{ - valueFlag("--name", "", "name", func(value string) { options.SourceName = value }), - valueFlag("--branch", "-b", "branch", func(value string) { options.Branch = value }), - valueFlag("--tag", "-t", "tag", func(value string) { options.Tag = value }), - valueFlag("--revision", "-r", "revision", func(value string) { options.Revision = value }), - boolFlag("--no-commit", "", func() { options.NoCommit = true }), - boolFlag("--partial-clone", "", func() { options.PartialClone = true }), - }, func(pos []string, _ []string) { - positionals = pos - }) + parsed, err := parseCommandSyntax(CommandAdd, args) if err != nil { return err } - if err := requireArgRange("add", positionals, 1, -1); err != nil { - return err - } - if options.Tag != "" && options.Branch != "" { - return usageError("add cannot combine --tag and --branch") - } - if options.Tag != "" && options.Revision != "" { - return usageError("add cannot combine --tag and --revision") - } + positionals := parsed.positionals + options.SourceName = parsed.value("--name") + options.Branch = parsed.value("--branch") + options.Tag = parsed.value("--tag") + options.Revision = parsed.value("--revision") + options.NoCommit = parsed.has("--no-commit") + options.PartialClone = parsed.has("--partial-clone") if strings.HasPrefix(positionals[0], ":") { options.ExistingSource = strings.TrimPrefix(positionals[0], ":") if options.ExistingSource == "" { return usageError("add source selector requires a name") } - if options.SourceName != "" || options.Branch != "" || options.Tag != "" || options.Revision != "" || options.PartialClone { - return usageError("add to an existing source cannot use --name, --branch, --tag, --revision, or --partial-clone") - } - if len(positionals) == 1 { - return usageError("add to an existing source requires at least one mirror") - } } else { options.URL = positionals[0] } @@ -383,140 +385,96 @@ func parseAdd(args []string, options *AddOptions) error { } func parseUpdate(args []string, options *UpdateOptions) error { - var positionals []string - err := parseCommandArgs(CommandPull, args, []flagSpec{ - valueFlag("--branch", "-b", "branch", func(value string) { options.Branch = value }), - valueFlag("--tag", "-t", "tag", func(value string) { options.Tag = value }), - valueFlag("--revision", "-r", "revision", func(value string) { options.Revision = value }), - boolFlag("--keep", "", func() { options.Keep = true }), - boolFlag("--no-commit", "", func() { options.NoCommit = true }), - }, func(pos []string, _ []string) { - positionals = pos - }) + parsed, err := parseCommandSyntax(CommandPull, args) if err != nil { return err } - if err := requireArgRange("pull", positionals, 0, 1); err != nil { - return err - } + positionals := parsed.positionals + options.Branch = parsed.value("--branch") + options.Tag = parsed.value("--tag") + options.Revision = parsed.value("--revision") + options.Keep = parsed.has("--keep") + options.NoCommit = parsed.has("--no-commit") if len(positionals) == 1 { options.LocalPath = normalizeLocalPathArg(positionals[0]) - } else if options.Branch != "" || options.Tag != "" || options.Revision != "" { - return usageError("pull without local_path cannot use --branch, --tag, or --revision") - } - if options.Tag != "" && options.Branch != "" { - return usageError("pull cannot combine --tag and --branch") - } - if options.Tag != "" && options.Revision != "" { - return usageError("pull cannot combine --tag and --revision") } return nil } func parseRemove(args []string, options *RemoveOptions) error { - var positionals []string - err := parseCommandArgs(CommandRemove, args, []flagSpec{ - boolFlag("--keep", "", func() { options.Keep = true }), - boolFlag("--no-commit", "", func() { options.NoCommit = true }), - }, func(pos []string, _ []string) { - positionals = pos - }) + parsed, err := parseCommandSyntax(CommandRemove, args) if err != nil { return err } - if err := requireArgRange("remove", positionals, 1, 1); err != nil { - return err - } - options.LocalPath = normalizeLocalPathArg(positionals[0]) + options.LocalPath = normalizeLocalPathArg(parsed.positionals[0]) + options.Keep = parsed.has("--keep") + options.NoCommit = parsed.has("--no-commit") return nil } func parseDiff(args []string, options *DiffOptions) error { - var positionals []string - err := parseCommandArgs(CommandDiff, args, []flagSpec{ - boolFlag("--keep", "", func() { options.Keep = true }), - }, func(pos []string, passthrough []string) { - positionals = pos - options.GitDiffArgs = passthrough - }) + parsed, err := parseCommandSyntax(CommandDiff, args) if err != nil { return err } - if err := requireArgRange("diff", positionals, 0, 1); err != nil { - return err - } - if len(positionals) == 1 { - options.LocalPath = normalizeLocalPathArg(positionals[0]) + if len(parsed.positionals) == 1 { + options.LocalPath = normalizeLocalPathArg(parsed.positionals[0]) } + options.Keep = parsed.has("--keep") + options.GitDiffArgs = parsed.passthrough return nil } func parsePush(args []string, options *PushOptions) error { - var positionals []string - err := parseCommandArgs(CommandPush, args, []flagSpec{ - valueFlag("--branch", "-b", "branch", func(value string) { options.Branch = value }), - valueFlag("--message", "-m", "message", func(value string) { options.Message = value }), - boolFlag("--keep", "", func() { options.Keep = true }), - }, func(pos []string, _ []string) { - positionals = pos - }) + parsed, err := parseCommandSyntax(CommandPush, args) if err != nil { return err } - if err := requireArgRange("push", positionals, 1, 1); err != nil { - return err - } + options.Branch = parsed.value("--branch") + options.Message = parsed.value("--message") + options.Keep = parsed.has("--keep") if options.Message != "" && strings.TrimSpace(options.Message) == "" { return usageError("--message requires a non-empty message value") } - options.LocalPath = normalizeLocalPathArg(positionals[0]) + options.LocalPath = normalizeLocalPathArg(parsed.positionals[0]) return nil } func parseSync(args []string, options *SyncOptions) error { - var positionals []string - err := parseCommandArgs(CommandSync, args, []flagSpec{ - boolFlag("--pull-only", "", func() { options.PullOnly = true }), - boolFlag("--autostash", "", func() { options.Autostash = true }), - boolFlag("--keep", "", func() { options.Keep = true }), - }, func(pos []string, _ []string) { - positionals = pos - }) + parsed, err := parseCommandSyntax(CommandSync, args) if err != nil { return err } - options.LocalPaths = make([]string, 0, len(positionals)) - for _, positional := range positionals { + options.PullOnly = parsed.has("--pull-only") + options.Autostash = parsed.has("--autostash") + options.Keep = parsed.has("--keep") + options.LocalPaths = make([]string, 0, len(parsed.positionals)) + for _, positional := range parsed.positionals { options.LocalPaths = append(options.LocalPaths, normalizeLocalPathArg(positional)) } return nil } func parseStatus(args []string, options *StatusOptions) error { - var positionals []string - err := parseCommandArgs(CommandStatus, args, nil, func(pos []string, _ []string) { - positionals = pos - }) + parsed, err := parseCommandSyntax(CommandStatus, args) if err != nil { return err } - if err := requireArgRange("status", positionals, 0, 1); err != nil { - return err - } - if len(positionals) == 1 { - options.LocalPath = normalizeLocalPathArg(positionals[0]) + if len(parsed.positionals) == 1 { + options.LocalPath = normalizeLocalPathArg(parsed.positionals[0]) } return nil } func parseCompletion(args []string, options *CompletionOptions) error { - if err := requireArgRange("completion", args, 1, 1); err != nil { + parsed, err := parseCommandSyntax(CommandCompletion, args) + if err != nil { return err } - if args[0] != "bash" { - return usageError("unknown completion shell %s", args[0]) + if parsed.positionals[0] != "bash" { + return usageError("unknown completion shell %s", parsed.positionals[0]) } - options.Shell = args[0] + options.Shell = parsed.positionals[0] return nil } @@ -524,137 +482,38 @@ func parseComplete(args []string, options *CompleteOptions) error { if len(args) < 2 { return usageError("__complete requires shell and -- separator") } - if args[0] != "bash" { - return usageError("unknown completion shell %s", args[0]) + parsed, err := parseCommandSyntax(CommandComplete, args) + if err != nil { + return err } - if args[1] != "--" { - return usageError("__complete requires -- separator") + if parsed.positionals[0] != "bash" { + return usageError("unknown completion shell %s", parsed.positionals[0]) } - options.Shell = args[0] - options.Args = append([]string(nil), args[2:]...) + options.Shell = parsed.positionals[0] + options.Args = append([]string(nil), parsed.passthrough...) return nil } func parseUpgradeConfig(args []string, options *UpgradeConfigOptions) error { - var positionals []string - err := parseCommandArgs(CommandUpgradeConfig, args, []flagSpec{ - boolFlag("--no-commit", "", func() { options.NoCommit = true }), - }, func(pos []string, _ []string) { - positionals = pos - }) + parsed, err := parseCommandSyntax(CommandUpgradeConfig, args) if err != nil { return err } - return requireArgRange("upgrade-config", positionals, 0, 0) + options.NoCommit = parsed.has("--no-commit") + return nil } func normalizeLocalPathArg(value string) string { return strings.ReplaceAll(value, `\`, "/") } -type flagSpec struct { - long string - short string - valueName string - setValue func(string) - setBool func() -} - -func valueFlag(long, short, valueName string, set func(string)) flagSpec { - return flagSpec{long: long, short: short, valueName: valueName, setValue: set} -} - -func boolFlag(long, short string, set func()) flagSpec { - return flagSpec{long: long, short: short, setBool: set} -} - -func parseCommandArgs(command Command, args []string, flags []flagSpec, done func([]string, []string)) error { - var positionals []string - var passthrough []string - for i := 0; i < len(args); i++ { - arg := args[i] - if command == CommandDiff && arg == "--" { - passthrough = append(passthrough, args[i+1:]...) - break - } - if strings.HasPrefix(arg, "-") { - spec, inlineValue, matched := matchFlag(arg, flags) - if !matched { - return usageError("unknown flag for %s: %s", command, arg) - } - if spec.setBool != nil { - if inlineValue != "" { - return usageError("flag %s does not take a value", spec.long) - } - spec.setBool() - continue - } - value := inlineValue - if value == "" { - if i+1 >= len(args) { - return usageError("%s requires a %s value", spec.long, spec.valueName) - } - value = args[i+1] - i++ - } - if value == "" { - return usageError("%s requires a non-empty %s value", spec.long, spec.valueName) - } - spec.setValue(value) - continue - } - positionals = append(positionals, arg) - } - done(positionals, passthrough) - return nil -} - -func matchFlag(arg string, flags []flagSpec) (flagSpec, string, bool) { - name := arg - inlineValue := "" - if before, after, ok := strings.Cut(arg, "="); ok { - name = before - inlineValue = after - } - for _, spec := range flags { - if name == spec.long || (spec.short != "" && name == spec.short) { - return spec, inlineValue, true - } - } - return flagSpec{}, "", false -} - func parseCommand(value string) (Command, bool) { - switch value { - case string(CommandAdd): - return CommandAdd, true - case string(CommandPull), "update", "up": - return CommandPull, true - case string(CommandRemove): - return CommandRemove, true - case string(CommandDiff): - return CommandDiff, true - case string(CommandPush): - return CommandPush, true - case string(CommandSync): - return CommandSync, true - case string(CommandVersion): - return CommandVersion, true - case string(CommandStatus): - return CommandStatus, true - case string(CommandCompletion): - return CommandCompletion, true - case string(CommandComplete): - return CommandComplete, true - case string(CommandUpgradeConfig): - return CommandUpgradeConfig, true - default: - return "", false - } + spec, ok := CommandSpecForName(value) + return spec.Command, ok } func isCommandHelp(args []string) bool { - return len(args) == 1 && (args[0] == "help" || args[0] == "--help" || args[0] == "-h") + return len(args) == 1 && Help().Matches(args[0]) } func requireNoArgs(command string, args []string) error { @@ -664,65 +523,53 @@ func requireNoArgs(command string, args []string) error { return nil } -func requireArgRange(command string, args []string, min, max int) error { - if len(args) < min { - return usageError("%s requires %d argument(s)", command, min) - } - if max >= 0 && len(args) > max { - return usageError("%s received extra argument(s)", command) - } - return nil -} - func usageError(format string, args ...interface{}) error { return UsageError{Message: fmt.Sprintf(format, args...)} } func Usage() string { - return strings.TrimLeft(` -usage: braid [--verbose|-v | --quiet] [--no-cache | --global-cache-dir ] [options] - -commands: - add Add a source or mirrors to an existing source - pull Pull one source or every eligible source - remove Remove a mirror or source - diff Show local mirror changes - push Push one source's local mirror changes upstream - sync Push local mirror changes, then pull sources - status Show mirror status - version Show braid version - completion - Print Bash completion script - upgrade-config - Upgrade .braids.json to the current version - -Run "braid help" for command-specific usage. -`, "\n") + var groups []string + for _, conflict := range GlobalConflictsSpec() { + var options []string + for _, option := range GlobalOptionsSpec() { + for _, name := range conflict.Options { + if option.Long == name { + options = append(options, option.UsageName()) + } + } + } + groups = append(groups, "["+strings.Join(options, " | ")+"]") + } + var output strings.Builder + fmt.Fprintf(&output, "usage: braid %s [options]\n\ncommands:\n", strings.Join(groups, " ")) + for _, spec := range CommandSpecs() { + if spec.Hidden { + continue + } + if len(spec.Name) <= 7 { + fmt.Fprintf(&output, " %-9s %s\n", spec.Name, spec.Summary) + } else { + fmt.Fprintf(&output, " %s\n %s\n", spec.Name, spec.Summary) + } + } + fmt.Fprintf(&output, "\nRun \"braid %s\" for command-specific usage.\n", Help().Command) + return output.String() } func CommandUsage(command Command) string { - switch command { - case CommandAdd: - return "usage: braid add [local_path[=upstream_path]...] [--name ] [--branch|-b ] [--tag|-t ] [--revision|-r ] [--no-commit] [--partial-clone]\n" - case CommandPull: - return "usage: braid pull [local_path|:source] [--branch|-b ] [--tag|-t ] [--revision|-r ] [--keep] [--no-commit]\n" - case CommandRemove: - return "usage: braid remove [--keep] [--no-commit]\n" - case CommandDiff: - return "usage: braid diff [local_path|:source] [--keep] [-- ...]\n" - case CommandPush: - return "usage: braid push [--branch|-b ] [--message|-m ] [--keep]\n" - case CommandSync: - return "usage: braid sync [local_path|:source ...] [--pull-only] [--autostash] [--keep]\n" - case CommandVersion: - return "usage: braid version\n" - case CommandStatus: - return "usage: braid status [local_path|:source]\n" - case CommandCompletion: - return "usage: braid completion bash\n" - case CommandUpgradeConfig: - return "usage: braid upgrade-config [--no-commit]\n" - default: + spec, ok := CommandSpecForCommand(command) + if !ok || spec.Hidden { return Usage() } + parts := []string{"usage:", "braid", spec.Name} + for _, positional := range spec.Positionals { + parts = append(parts, positional.Usage) + } + for _, option := range spec.Options { + parts = append(parts, "["+option.UsageName()+"]") + } + if spec.Passthrough != nil && spec.Passthrough.Usage != "" { + parts = append(parts, spec.Passthrough.Usage) + } + return strings.Join(parts, " ") + "\n" } diff --git a/internal/cli/cli_test.go b/internal/cli/cli_test.go index 3a84155..45a860d 100644 --- a/internal/cli/cli_test.go +++ b/internal/cli/cli_test.go @@ -183,6 +183,7 @@ func TestParseUsageErrors(t *testing.T) { {name: "old cache dir flag", args: []string{"--cache-dir", "cache", "version"}, want: "--cache-dir has been replaced by --global-cache-dir"}, {name: "old cache dir equals flag", args: []string{"--cache-dir=cache", "version"}, want: "--cache-dir has been replaced by --global-cache-dir"}, {name: "quiet verbose conflict", args: []string{"--quiet", "--verbose", "version"}, want: "--quiet and --verbose cannot be used together"}, + {name: "global boolean inline value", args: []string{"--verbose=true", "version"}, want: "flag --verbose does not take a value"}, {name: "verbose quiet conflict", args: []string{"--verbose", "--quiet", "version"}, want: "--quiet and --verbose cannot be used together"}, {name: "empty global cache dir", args: []string{"--global-cache-dir=", "version"}, want: "--global-cache-dir requires a non-empty value"}, {name: "existing source needs mirror", args: []string{"add", ":source"}, want: "add to an existing source requires at least one mirror"}, diff --git a/internal/cli/schema.go b/internal/cli/schema.go new file mode 100644 index 0000000..ef747d2 --- /dev/null +++ b/internal/cli/schema.go @@ -0,0 +1,449 @@ +package cli + +import ( + "fmt" + "strings" +) + +type CompletionRole string + +const ( + CompletionNone CompletionRole = "" + CompletionFree CompletionRole = "free" + CompletionDirectory CompletionRole = "directory" + CompletionFilesystem CompletionRole = "filesystem" + CompletionMirror CompletionRole = "mirror" + CompletionStatic CompletionRole = "static" +) + +type HelpSpec struct { + Command string + Long string + Short string +} + +func (h HelpSpec) Names() []string { return []string{h.Command, h.Long, h.Short} } + +func (h HelpSpec) Matches(value string) bool { + return value == h.Command || value == h.Long || value == h.Short +} + +type Availability struct { + PositionalIndex int + ForbiddenPrefix string +} + +func (a Availability) Allows(positionals []string) bool { + return a.ForbiddenPrefix == "" || len(positionals) <= a.PositionalIndex || !strings.HasPrefix(positionals[a.PositionalIndex], a.ForbiddenPrefix) +} + +type OptionSpec struct { + Long string + Short string + ValueName string + UsageValue string + Completion CompletionRole + Availability Availability +} + +func (o OptionSpec) TakesValue() bool { return o.ValueName != "" } + +func (o OptionSpec) UsageName() string { + valueName := o.UsageValue + if valueName == "" { + valueName = o.ValueName + } + name := o.Long + if o.Short != "" { + name += "|" + o.Short + } + if valueName != "" { + name += " <" + valueName + ">" + } + return name +} + +func (s CommandSpec) Option(name string) (OptionSpec, bool) { + for _, option := range s.Options { + if name == option.Long || (option.Short != "" && name == option.Short) { + return option, true + } + } + return OptionSpec{}, false +} + +type PositionalSpec struct { + Name string + Usage string + Required bool + Repeatable bool + Completion CompletionRole + Values []string +} + +type PassthroughSpec struct { + Required bool + Usage string +} + +type ConflictSpec struct { + Options []string + Error string +} + +type RequiresPositionalsSpec struct { + Options []string + Minimum int + Error string +} + +type ConditionalPositionalsSpec struct { + Index int + Prefix string + Minimum int + Error string +} + +type CommandSpec struct { + Command Command + Name string + Aliases []string + Summary string + Hidden bool + Options []OptionSpec + Positionals []PositionalSpec + Passthrough *PassthroughSpec + Conflicts []ConflictSpec + RequiresPositionals []RequiresPositionalsSpec + ConditionalPositionals []ConditionalPositionalsSpec + AvailabilityError string +} + +var globalOptions = []OptionSpec{ + {Long: "--verbose", Short: "-v"}, + {Long: "--quiet"}, + {Long: "--no-cache"}, + {Long: "--global-cache-dir", ValueName: "path", Completion: CompletionDirectory}, +} + +var help = HelpSpec{Command: "help", Long: "--help", Short: "-h"} + +var globalConflicts = []ConflictSpec{ + {Options: []string{"--quiet", "--verbose"}, Error: "--quiet and --verbose cannot be used together"}, + {Options: []string{"--no-cache", "--global-cache-dir"}, Error: "--no-cache and --global-cache-dir cannot be used together"}, +} + +var commandSpecs = []CommandSpec{ + { + Command: CommandAdd, Name: "add", Summary: "Add a source or mirrors to an existing source", + Options: []OptionSpec{ + {Long: "--name", ValueName: "name", Availability: Availability{ForbiddenPrefix: ":"}}, + {Long: "--branch", Short: "-b", ValueName: "branch", Availability: Availability{ForbiddenPrefix: ":"}}, + {Long: "--tag", Short: "-t", ValueName: "tag", Availability: Availability{ForbiddenPrefix: ":"}}, + {Long: "--revision", Short: "-r", ValueName: "revision", UsageValue: "rev", Availability: Availability{ForbiddenPrefix: ":"}}, + {Long: "--no-commit"}, + {Long: "--partial-clone", Availability: Availability{ForbiddenPrefix: ":"}}, + }, + Positionals: []PositionalSpec{ + {Name: "url|:source", Usage: "", Required: true, Completion: CompletionFree}, + {Name: "local_path[=upstream_path]", Usage: "[local_path[=upstream_path]...]", Repeatable: true, Completion: CompletionFilesystem}, + }, + Conflicts: []ConflictSpec{ + {Options: []string{"--tag", "--branch"}, Error: "add cannot combine --tag and --branch"}, + {Options: []string{"--tag", "--revision"}, Error: "add cannot combine --tag and --revision"}, + }, + ConditionalPositionals: []ConditionalPositionalsSpec{{Index: 0, Prefix: ":", Minimum: 2, Error: "add to an existing source requires at least one mirror"}}, + AvailabilityError: "add to an existing source cannot use --name, --branch, --tag, --revision, or --partial-clone", + }, + { + Command: CommandPull, Name: "pull", Aliases: []string{"update", "up"}, Summary: "Pull one source or every eligible source", + Options: []OptionSpec{ + {Long: "--branch", Short: "-b", ValueName: "branch"}, + {Long: "--tag", Short: "-t", ValueName: "tag"}, + {Long: "--revision", Short: "-r", ValueName: "revision", UsageValue: "rev"}, + {Long: "--keep"}, + {Long: "--no-commit"}, + }, + Positionals: []PositionalSpec{{Name: "local_path|:source", Usage: "[local_path|:source]", Completion: CompletionMirror}}, + Conflicts: []ConflictSpec{ + {Options: []string{"--tag", "--branch"}, Error: "pull cannot combine --tag and --branch"}, + {Options: []string{"--tag", "--revision"}, Error: "pull cannot combine --tag and --revision"}, + }, + RequiresPositionals: []RequiresPositionalsSpec{{Options: []string{"--branch", "--tag", "--revision"}, Minimum: 1, Error: "pull without local_path cannot use --branch, --tag, or --revision"}}, + }, + { + Command: CommandRemove, Name: "remove", Summary: "Remove a mirror or source", + Options: []OptionSpec{{Long: "--keep"}, {Long: "--no-commit"}}, + Positionals: []PositionalSpec{{Name: "local_path|:source", Usage: "", Required: true, Completion: CompletionMirror}}, + }, + { + Command: CommandDiff, Name: "diff", Summary: "Show local mirror changes", + Options: []OptionSpec{{Long: "--keep"}}, + Positionals: []PositionalSpec{{Name: "local_path|:source", Usage: "[local_path|:source]", Completion: CompletionMirror}}, + Passthrough: &PassthroughSpec{Usage: "[-- ...]"}, + }, + { + Command: CommandPush, Name: "push", Summary: "Push one source's local mirror changes upstream", + Options: []OptionSpec{{Long: "--branch", Short: "-b", ValueName: "branch"}, {Long: "--message", Short: "-m", ValueName: "message"}, {Long: "--keep"}}, + Positionals: []PositionalSpec{{Name: "local_path|:source", Usage: "", Required: true, Completion: CompletionMirror}}, + }, + { + Command: CommandSync, Name: "sync", Summary: "Push local mirror changes, then pull sources", + Options: []OptionSpec{{Long: "--pull-only"}, {Long: "--autostash"}, {Long: "--keep"}}, + Positionals: []PositionalSpec{{Name: "local_path|:source", Usage: "[local_path|:source ...]", Repeatable: true, Completion: CompletionMirror}}, + }, + {Command: CommandStatus, Name: "status", Summary: "Show mirror status", Positionals: []PositionalSpec{{Name: "local_path|:source", Usage: "[local_path|:source]", Completion: CompletionMirror}}}, + {Command: CommandVersion, Name: "version", Summary: "Show braid version"}, + {Command: CommandCompletion, Name: "completion", Summary: "Print Bash completion script", Positionals: []PositionalSpec{{Name: "shell", Usage: "bash", Required: true, Completion: CompletionStatic, Values: []string{"bash"}}}}, + {Command: CommandComplete, Name: "__complete", Hidden: true, Positionals: []PositionalSpec{{Name: "shell", Usage: "", Required: true, Completion: CompletionStatic, Values: []string{"bash"}}}, Passthrough: &PassthroughSpec{Required: true}}, + {Command: CommandUpgradeConfig, Name: "upgrade-config", Summary: "Upgrade .braids.json to the current version", Options: []OptionSpec{{Long: "--no-commit"}}}, +} + +func GlobalOptionsSpec() []OptionSpec { return globalOptions } + +func Help() HelpSpec { return help } + +func GlobalConflictsSpec() []ConflictSpec { return globalConflicts } + +func CommandSpecs() []CommandSpec { return commandSpecs } + +func CommandSpecForName(name string) (CommandSpec, bool) { + for _, spec := range commandSpecs { + if spec.Name == name { + return spec, true + } + for _, alias := range spec.Aliases { + if alias == name { + return spec, true + } + } + } + return CommandSpec{}, false +} + +func CommandSpecForCommand(command Command) (CommandSpec, bool) { + for _, spec := range commandSpecs { + if spec.Command == command { + return spec, true + } + } + return CommandSpec{}, false +} + +type parsedCommandArgs struct { + positionals []string + passthrough []string + values map[string]string + present map[string]bool +} + +func (p parsedCommandArgs) value(name string) string { return p.values[name] } + +func (p parsedCommandArgs) has(name string) bool { return p.present[name] } + +func parseCommandSyntax(command Command, args []string) (parsedCommandArgs, error) { + spec, ok := CommandSpecForCommand(command) + if !ok { + return parsedCommandArgs{}, usageError("unknown command %s", command) + } + parsed := parsedCommandArgs{values: map[string]string{}, present: map[string]bool{}} + separatorFound := false + for i := 0; i < len(args); i++ { + arg := args[i] + if arg == "--" && spec.Passthrough != nil { + separatorFound = true + parsed.passthrough = append(parsed.passthrough, args[i+1:]...) + break + } + if strings.HasPrefix(arg, "-") { + name, inlineValue, hasEquals := strings.Cut(arg, "=") + option, matched := spec.Option(name) + if !matched { + return parsed, usageError("unknown flag for %s: %s", command, arg) + } + parsed.present[option.Long] = true + if !option.TakesValue() { + if hasEquals { + return parsed, usageError("flag %s does not take a value", option.Long) + } + continue + } + value := inlineValue + if !hasEquals { + if i+1 >= len(args) { + return parsed, usageError("%s requires a %s value", option.Long, option.ValueName) + } + i++ + value = args[i] + } + if value == "" { + return parsed, usageError("%s requires a non-empty %s value", option.Long, option.ValueName) + } + parsed.values[option.Long] = value + continue + } + parsed.positionals = append(parsed.positionals, arg) + } + if spec.Passthrough != nil && spec.Passthrough.Required && !separatorFound { + return parsed, usageError("%s requires -- separator", command) + } + min, max := positionalRange(spec.Positionals) + if len(parsed.positionals) < min { + return parsed, usageError("%s requires %d argument(s)", command, min) + } + if max >= 0 && len(parsed.positionals) > max { + return parsed, usageError("%s received extra argument(s)", command) + } + for _, requirement := range spec.ConditionalPositionals { + if len(parsed.positionals) > requirement.Index && strings.HasPrefix(parsed.positionals[requirement.Index], requirement.Prefix) && len(parsed.positionals) < requirement.Minimum { + return parsed, usageError("%s", requirement.Error) + } + } + for _, option := range spec.Options { + if parsed.has(option.Long) && !option.Availability.Allows(parsed.positionals) { + return parsed, usageError("%s", spec.AvailabilityError) + } + } + for _, conflict := range spec.Conflicts { + all := true + for _, option := range conflict.Options { + all = all && parsed.has(option) + } + if all { + return parsed, usageError("%s", conflict.Error) + } + } + for _, requirement := range spec.RequiresPositionals { + if len(parsed.positionals) >= requirement.Minimum { + continue + } + for _, option := range requirement.Options { + if parsed.has(option) { + return parsed, usageError("%s", requirement.Error) + } + } + } + return parsed, nil +} + +func positionalRange(positionals []PositionalSpec) (int, int) { + min := 0 + max := len(positionals) + for _, positional := range positionals { + if positional.Required { + min++ + } + if positional.Repeatable { + max = -1 + } + } + return min, max +} + +func ValidateSchema() error { + names := map[string]bool{} + for _, name := range help.Names() { + if name == "" || names[name] { + return fmt.Errorf("invalid help name %q", name) + } + names[name] = true + } + commands := map[Command]bool{} + for _, spec := range commandSpecs { + if spec.Command == "" || spec.Name == "" { + return fmt.Errorf("command has empty identity") + } + if commands[spec.Command] { + return fmt.Errorf("duplicate command %s", spec.Command) + } + commands[spec.Command] = true + for _, name := range append([]string{spec.Name}, spec.Aliases...) { + if names[name] { + return fmt.Errorf("duplicate command name %s", name) + } + names[name] = true + } + if err := validateOptions(spec.Name, spec.Options); err != nil { + return err + } + for _, conflict := range spec.Conflicts { + if err := validateConstraintOptions(spec, conflict.Options); err != nil { + return err + } + } + for _, requirement := range spec.RequiresPositionals { + if err := validateConstraintOptions(spec, requirement.Options); err != nil { + return err + } + } + for _, requirement := range spec.ConditionalPositionals { + if requirement.Index < 0 || requirement.Index >= len(spec.Positionals) || requirement.Prefix == "" || requirement.Minimum <= requirement.Index || requirement.Error == "" { + return fmt.Errorf("%s has invalid conditional positional requirement", spec.Name) + } + } + for _, option := range spec.Options { + if option.Availability.ForbiddenPrefix != "" && spec.AvailabilityError == "" { + return fmt.Errorf("%s option %s has availability without an error", spec.Name, option.Long) + } + } + for i, positional := range spec.Positionals { + if positional.Name == "" || positional.Usage == "" { + return fmt.Errorf("%s positional %d has incomplete metadata", spec.Name, i) + } + if positional.Repeatable && i != len(spec.Positionals)-1 { + return fmt.Errorf("%s repeatable positional must be last", spec.Name) + } + if positional.Completion == CompletionStatic && len(positional.Values) == 0 { + return fmt.Errorf("%s static positional %s has no values", spec.Name, positional.Name) + } + if i > 0 && positional.Required && !spec.Positionals[i-1].Required { + return fmt.Errorf("%s required positional follows optional positional", spec.Name) + } + } + } + if err := validateOptions("global", globalOptions); err != nil { + return err + } + global := CommandSpec{Name: "global", Options: globalOptions} + for _, conflict := range globalConflicts { + if err := validateConstraintOptions(global, conflict.Options); err != nil { + return err + } + } + return nil +} + +func validateConstraintOptions(spec CommandSpec, options []string) error { + if len(options) == 0 { + return fmt.Errorf("%s has an empty option constraint", spec.Name) + } + for _, name := range options { + option, ok := spec.Option(name) + if !ok || option.Long != name { + return fmt.Errorf("%s constraint references unknown long option %s", spec.Name, name) + } + } + return nil +} + +func validateOptions(owner string, options []OptionSpec) error { + names := map[string]bool{} + for _, option := range options { + if option.Long == "" || !strings.HasPrefix(option.Long, "--") { + return fmt.Errorf("%s option has invalid long name %q", owner, option.Long) + } + if !option.TakesValue() && option.Completion != CompletionNone { + return fmt.Errorf("%s boolean option %s has a completion role", owner, option.Long) + } + for _, name := range []string{option.Long, option.Short} { + if name == "" { + continue + } + if names[name] { + return fmt.Errorf("%s has duplicate option %s", owner, name) + } + names[name] = true + } + } + return nil +} diff --git a/internal/cli/schema_test.go b/internal/cli/schema_test.go new file mode 100644 index 0000000..9e4b0b6 --- /dev/null +++ b/internal/cli/schema_test.go @@ -0,0 +1,48 @@ +package cli + +import "testing" + +func TestSchemaIsValid(t *testing.T) { + if err := ValidateSchema(); err != nil { + t.Fatal(err) + } +} + +func TestSchemaCoversEveryCommand(t *testing.T) { + want := []Command{ + CommandAdd, + CommandPull, + CommandRemove, + CommandDiff, + CommandPush, + CommandSync, + CommandVersion, + CommandStatus, + CommandCompletion, + CommandComplete, + CommandUpgradeConfig, + } + for _, command := range want { + if _, ok := CommandSpecForCommand(command); !ok { + t.Errorf("schema missing command %s", command) + } + } + if got := len(CommandSpecs()); got != len(want) { + t.Fatalf("schema has %d commands, want %d", got, len(want)) + } +} + +func TestSchemaCommandNamesResolveCanonically(t *testing.T) { + for _, spec := range CommandSpecs() { + for _, name := range append([]string{spec.Name}, spec.Aliases...) { + got, ok := CommandSpecForName(name) + if !ok { + t.Errorf("schema name %q does not resolve", name) + continue + } + if got.Command != spec.Command { + t.Errorf("schema name %q resolves to %s, want %s", name, got.Command, spec.Command) + } + } + } +} diff --git a/internal/clitest/BUILD.bazel b/internal/clitest/BUILD.bazel new file mode 100644 index 0000000..87685ab --- /dev/null +++ b/internal/clitest/BUILD.bazel @@ -0,0 +1,17 @@ +load("@rules_go//go:def.bzl", "go_library", "go_test") + +go_library( + name = "clitest", + srcs = ["completion_contract.go"], + importpath = "braid/internal/clitest", + testonly = True, + visibility = ["//visibility:public"], + deps = ["//internal/cli"], +) + +go_test( + name = "clitest_test", + srcs = ["completion_contract_test.go"], + embed = [":clitest"], + deps = ["//internal/cli"], +) diff --git a/internal/clitest/completion_contract.go b/internal/clitest/completion_contract.go new file mode 100644 index 0000000..db4afca --- /dev/null +++ b/internal/clitest/completion_contract.go @@ -0,0 +1,211 @@ +package clitest + +import ( + "fmt" + "strings" + + "braid/internal/cli" +) + +type CompletionCase struct { + Name string + Words []string + Want []string + Unwanted []string + WantEmpty bool +} + +type SyntaxCase struct { + Name string + Args []string + WantError string +} + +func SyntaxConstraintCases() []SyntaxCase { + var cases []SyntaxCase + for _, spec := range cli.CommandSpecs() { + for _, requirement := range spec.RequiresPositionals { + baseMin := requiredPositionals(spec.Positionals) + if baseMin >= requirement.Minimum { + continue + } + for _, optionName := range requirement.Options { + option, _ := spec.Option(optionName) + invalid := []string{spec.Name, option.Long} + if option.TakesValue() { + invalid = append(invalid, "value") + } + for i := 0; i < baseMin; i++ { + invalid = append(invalid, samplePositional(spec.Positionals[i])) + } + valid := append([]string(nil), invalid...) + for i := baseMin; i < requirement.Minimum; i++ { + valid = append(valid, samplePositional(spec.Positionals[i])) + } + cases = append(cases, + SyntaxCase{Name: spec.Name + " requires positional " + option.Long, Args: invalid, WantError: requirement.Error}, + SyntaxCase{Name: spec.Name + " accepts positional " + option.Long, Args: valid}, + ) + } + } + } + return cases +} + +func CompletionContractCases() []CompletionCase { + var cases []CompletionCase + var rootCandidates []string + for _, spec := range cli.CommandSpecs() { + if spec.Hidden { + continue + } + rootCandidates = append(rootCandidates, spec.Name) + rootCandidates = append(rootCandidates, spec.Aliases...) + } + helpSpec := cli.Help() + cases = append(cases, CompletionCase{Name: "root commands", Words: []string{""}, Want: append(rootCandidates, helpSpec.Command), Unwanted: []string{"__complete"}}) + for _, help := range helpSpec.Names() { + cases = append(cases, CompletionCase{Name: "root terminal help " + help, Words: []string{help, ""}, WantEmpty: true}) + } + + for _, option := range cli.GlobalOptionsSpec() { + for _, name := range optionNames(option) { + cases = append(cases, CompletionCase{Name: "global option " + name, Words: []string{name}, Want: []string{name}}) + } + if option.TakesValue() { + cases = append(cases, + CompletionCase{Name: "global separate value " + option.Long, Words: []string{option.Long, ""}, WantEmpty: option.Completion != cli.CompletionDirectory}, + CompletionCase{Name: "global inline value " + option.Long, Words: []string{option.Long + "="}}, + ) + } + } + for _, conflict := range cli.GlobalConflictsSpec() { + if len(conflict.Options) < 2 { + continue + } + first, second := conflict.Options[0], conflict.Options[1] + words := []string{first} + for _, option := range cli.GlobalOptionsSpec() { + if option.Long == first && option.TakesValue() { + words = append(words, "value") + } + } + words = append(words, "--") + cases = append(cases, CompletionCase{Name: "global conflict " + strings.Join(conflict.Options, " "), Words: words, Unwanted: []string{second}}) + } + + for _, spec := range cli.CommandSpecs() { + if spec.Hidden { + continue + } + for _, commandName := range append([]string{spec.Name}, spec.Aliases...) { + cases = append(cases, commandCases(spec, commandName)...) + } + } + return cases +} + +func commandCases(spec cli.CommandSpec, commandName string) []CompletionCase { + var cases []CompletionCase + helpSpec := cli.Help() + for _, help := range helpSpec.Names() { + cases = append(cases, CompletionCase{Name: commandName + " terminal help " + help, Words: []string{commandName, help, ""}, WantEmpty: true}) + } + for boundary, positionals := range positionalBoundaries(spec.Positionals) { + words := append([]string{commandName}, positionals...) + words = append(words, "") + var want []string + for _, option := range spec.Options { + if option.Availability.Allows(positionals) { + want = append(want, optionNames(option)...) + } + } + if spec.Passthrough != nil { + want = append(want, "--") + } + if boundary == 0 { + want = append(want, helpSpec.Names()...) + if len(spec.Positionals) > 0 && spec.Positionals[0].Completion == cli.CompletionStatic { + want = append(want, spec.Positionals[0].Values...) + } + } + cases = append(cases, CompletionCase{Name: fmt.Sprintf("%s boundary %d", commandName, boundary), Words: words, Want: want}) + } + for _, option := range spec.Options { + if option.TakesValue() { + for _, name := range optionNames(option) { + cases = append(cases, + CompletionCase{Name: commandName + " separate value " + name, Words: []string{commandName, name, ""}, WantEmpty: true}, + CompletionCase{Name: commandName + " inline value " + name, Words: []string{commandName, name + "="}, WantEmpty: true}, + ) + } + } + if option.Availability.ForbiddenPrefix != "" { + positionals := make([]string, option.Availability.PositionalIndex+1) + positionals[option.Availability.PositionalIndex] = option.Availability.ForbiddenPrefix + "value" + words := append([]string{commandName}, positionals...) + words = append(words, "--") + cases = append(cases, CompletionCase{Name: commandName + " availability " + option.Long, Words: words, Unwanted: optionNames(option)}) + } + } + for _, conflict := range spec.Conflicts { + if len(conflict.Options) < 2 { + continue + } + first, second := conflict.Options[0], conflict.Options[1] + words := []string{commandName, first} + if option, ok := spec.Option(first); ok && option.TakesValue() { + words = append(words, "value") + } + words = append(words, "--") + option, _ := spec.Option(second) + cases = append(cases, CompletionCase{Name: commandName + " conflict " + strings.Join(conflict.Options, " "), Words: words, Unwanted: optionNames(option)}) + } + return cases +} + +func positionalBoundaries(positionals []cli.PositionalSpec) [][]string { + boundaries := [][]string{{}} + var completed []string + for _, positional := range positionals { + completed = append(completed, samplePositional(positional)) + boundaries = append(boundaries, append([]string(nil), completed...)) + if positional.Repeatable { + completed = append(completed, samplePositional(positional)+"-2") + boundaries = append(boundaries, append([]string(nil), completed...)) + } + } + return boundaries +} + +func samplePositional(positional cli.PositionalSpec) string { + if len(positional.Values) > 0 { + return positional.Values[0] + } + switch positional.Completion { + case cli.CompletionMirror: + return "mirror" + case cli.CompletionFilesystem: + return "path" + default: + return "value" + } +} + +func requiredPositionals(positionals []cli.PositionalSpec) int { + count := 0 + for _, positional := range positionals { + if positional.Required { + count++ + } + } + return count +} + +func optionNames(option cli.OptionSpec) []string { + names := []string{option.Long} + if option.Short != "" { + names = append(names, option.Short) + } + return names +} diff --git a/internal/clitest/completion_contract_test.go b/internal/clitest/completion_contract_test.go new file mode 100644 index 0000000..b8bd579 --- /dev/null +++ b/internal/clitest/completion_contract_test.go @@ -0,0 +1,25 @@ +package clitest + +import ( + "strings" + "testing" + + "braid/internal/cli" +) + +func TestGeneratedSyntaxConstraintCases(t *testing.T) { + for _, test := range SyntaxConstraintCases() { + t.Run(test.Name, func(t *testing.T) { + _, err := cli.Parse(test.Args) + if test.WantError == "" { + if err != nil { + t.Fatalf("Parse(%v) returned error: %v", test.Args, err) + } + return + } + if err == nil || !strings.Contains(err.Error(), test.WantError) { + t.Fatalf("Parse(%v) error = %v, want containing %q", test.Args, err, test.WantError) + } + }) + } +} diff --git a/internal/command/BUILD.bazel b/internal/command/BUILD.bazel index 156ef28..962abab 100644 --- a/internal/command/BUILD.bazel +++ b/internal/command/BUILD.bazel @@ -33,7 +33,8 @@ go_library( ) COMMAND_TEST_DEPS = [ - "//internal/cli", + "//internal/cli", + "//internal/clitest", "//internal/config", "//internal/gitexec", "//internal/source", diff --git a/internal/command/completion.go b/internal/command/completion.go index f9dca43..487e307 100644 --- a/internal/command/completion.go +++ b/internal/command/completion.go @@ -41,9 +41,11 @@ complete -o default -o bashdefault -F _braid braid ` type completionFlag struct { - long string - short string - value bool + long string + short string + value bool + completion cli.CompletionRole + availability cli.Availability } type completionLine struct { @@ -59,63 +61,29 @@ type commandArgState struct { positionals []string } -var globalCompletionFlags = []completionFlag{ - {long: "--verbose", short: "-v"}, - {long: "--quiet"}, - {long: "--no-cache"}, - {long: "--global-cache-dir", value: true}, -} - -var commandCompletionFlags = map[string][]completionFlag{ - string(cli.CommandAdd): { - {long: "--name", value: true}, - {long: "--branch", short: "-b", value: true}, - {long: "--tag", short: "-t", value: true}, - {long: "--revision", short: "-r", value: true}, - {long: "--no-commit"}, - {long: "--partial-clone"}, - }, - string(cli.CommandPull): { - {long: "--branch", short: "-b", value: true}, - {long: "--tag", short: "-t", value: true}, - {long: "--revision", short: "-r", value: true}, - {long: "--keep"}, - {long: "--no-commit"}, - }, - string(cli.CommandRemove): { - {long: "--keep"}, - {long: "--no-commit"}, - }, - string(cli.CommandDiff): { - {long: "--keep"}, - {long: "--"}, - }, - string(cli.CommandPush): { - {long: "--branch", short: "-b", value: true}, - {long: "--message", short: "-m", value: true}, - {long: "--keep"}, - }, - string(cli.CommandSync): { - {long: "--pull-only"}, - {long: "--autostash"}, - {long: "--keep"}, - }, - string(cli.CommandStatus): {}, - string(cli.CommandUpgradeConfig): {{long: "--no-commit"}}, -} - -var rootCompletionCommands = []string{ - string(cli.CommandAdd), - string(cli.CommandPull), - string(cli.CommandRemove), - string(cli.CommandDiff), - string(cli.CommandPush), - string(cli.CommandSync), - string(cli.CommandStatus), - string(cli.CommandVersion), - string(cli.CommandCompletion), - string(cli.CommandUpgradeConfig), - "help", +func globalCompletionFlags() []completionFlag { + help := cli.Help() + return append(completionFlags(cli.GlobalOptionsSpec()), completionFlag{long: help.Long, short: help.Short}) +} + +func completionFlags(options []cli.OptionSpec) []completionFlag { + flags := make([]completionFlag, 0, len(options)) + for _, option := range options { + flags = append(flags, completionFlag{long: option.Long, short: option.Short, value: option.TakesValue(), completion: option.Completion, availability: option.Availability}) + } + return flags +} + +func rootCompletionCommands() []string { + commands := []string{cli.Help().Command} + for _, spec := range cli.CommandSpecs() { + if spec.Hidden { + continue + } + commands = append(commands, spec.Name) + commands = append(commands, spec.Aliases...) + } + return commands } func (h CompletionHandler) Run(inv cli.Invocation, stdout, _ io.Writer) error { @@ -151,93 +119,119 @@ func (h CompleteHandler) completeBash(ctx context.Context, words []string) []str } func (h CompleteHandler) completeRoot(line completionLine) []string { - if flag, ok := valueFlagAwaitingCurrent(line.globalArgs, globalCompletionFlags); ok { - if flag.long == "--global-cache-dir" { - return pathCandidates(h.Options.WorkDir, line.current, true, "") - } + flags := globalCompletionFlags() + if helpPresent(line.globalArgs) { return nil } - if strings.HasPrefix(line.current, "--global-cache-dir=") { - value := strings.TrimPrefix(line.current, "--global-cache-dir=") - return pathCandidates(h.Options.WorkDir, value, true, "--global-cache-dir=") + if flag, ok := valueFlagAwaitingCurrent(line.globalArgs, flags); ok { + return h.optionValueCandidates(flag, line.current, "") + } + if name, value, ok := strings.Cut(line.current, "="); ok { + if flag, _, matched := matchCompletionFlag(name, flags); matched { + return h.optionValueCandidates(flag, value, name+"=") + } } var candidates []string if line.current == "" || strings.HasPrefix(line.current, "-") { - candidates = append(candidates, optionCandidates(line.current, globalCompletionFlags, line.globalArgs, true)...) + candidates = append(candidates, optionCandidates(line.current, flags, line.globalArgs, cli.GlobalConflictsSpec(), nil)...) } if line.current == "" || !strings.HasPrefix(line.current, "-") { - candidates = append(candidates, prefixed(rootCompletionCommands, line.current)...) + candidates = append(candidates, prefixed(rootCompletionCommands(), line.current)...) } return uniqueSorted(candidates) } +func (h CompleteHandler) optionValueCandidates(flag completionFlag, current, prefix string) []string { + switch flag.completion { + case cli.CompletionDirectory: + return pathCandidates(h.Options.WorkDir, current, true, prefix) + case cli.CompletionFilesystem: + return pathCandidates(h.Options.WorkDir, current, false, prefix) + default: + return nil + } +} + func (h CompleteHandler) completeCommand(ctx context.Context, line completionLine) []string { if line.command == string(cli.CommandComplete) { return nil } - if line.command == string(cli.CommandCompletion) { - return completeCompletionCommand(line) - } - - command := canonicalCompletionCommand(line.command) - flags, ok := commandCompletionFlags[command] - if !ok { + spec, ok := cli.CommandSpecForName(line.command) + if !ok || spec.Hidden { return nil } - if _, ok := valueFlagAwaitingCurrent(line.commandArgs, flags); ok { + if helpPresent(line.commandArgs) { return nil } - if command == string(cli.CommandDiff) && commandArgsContainDiffSeparator(line.commandArgs) { + flags := completionFlags(spec.Options) + if spec.Passthrough != nil { + flags = append(flags, completionFlag{long: "--"}) + } + if len(line.commandArgs) == 0 { + help := cli.Help() + flags = append(append([]completionFlag(nil), flags...), completionFlag{long: help.Long, short: help.Short}) + } + if flag, ok := valueFlagAwaitingCurrent(line.commandArgs, flags); ok { + return h.optionValueCandidates(flag, line.current, "") + } + if name, value, hasEquals := strings.Cut(line.current, "="); hasEquals { + if flag, _, matched := matchCompletionFlag(name, flags); matched { + return h.optionValueCandidates(flag, value, name+"=") + } + } + if spec.Passthrough != nil && commandArgsContainDiffSeparator(line.commandArgs) { return pathCandidates(h.Options.WorkDir, line.current, false, "") } - state := parseCompletedCommandArgs(command, line.commandArgs, flags) - optionFlags := flags - if command == string(cli.CommandAdd) && len(state.positionals) > 0 && strings.HasPrefix(state.positionals[0], ":") { - optionFlags = []completionFlag{{long: "--no-commit"}} - } + state := parseCompletedCommandArgs(line.commandArgs, flags) var candidates []string if line.current == "" || strings.HasPrefix(line.current, "-") { - candidates = append(candidates, optionCandidates(line.current, optionFlags, line.commandArgs, false)...) + candidates = append(candidates, optionCandidates(line.current, flags, line.commandArgs, spec.Conflicts, state.positionals)...) } if line.current == "" || !strings.HasPrefix(line.current, "-") { - candidates = append(candidates, h.pathCandidatesForCommand(ctx, command, line, state)...) + candidates = append(candidates, h.positionalCandidates(ctx, spec, line, state)...) + if len(line.commandArgs) == 0 { + candidates = append(candidates, prefixed([]string{cli.Help().Command}, line.current)...) + } } return uniqueSorted(candidates) } -func canonicalCompletionCommand(command string) string { - switch command { - case "update", "up": - return string(cli.CommandPull) - default: - return command +func helpPresent(args []string) bool { + for _, arg := range args { + if cli.Help().Matches(arg) { + return true + } } + return false } -func completeCompletionCommand(line completionLine) []string { - state := parseCompletedCommandArgs(line.command, line.commandArgs, nil) - if len(state.positionals) > 0 || strings.HasPrefix(line.current, "-") { +func (h CompleteHandler) positionalCandidates(ctx context.Context, spec cli.CommandSpec, line completionLine, state commandArgState) []string { + position, ok := positionalAt(spec.Positionals, len(state.positionals)) + if !ok { + return nil + } + switch position.Completion { + case cli.CompletionFilesystem: + return pathCandidates(h.Options.WorkDir, line.current, false, "") + case cli.CompletionMirror: + return h.mirrorPathCandidates(ctx, line.current, state.positionals) + case cli.CompletionStatic: + return prefixed(position.Values, line.current) + default: return nil } - return prefixed([]string{"bash"}, line.current) } -func (h CompleteHandler) pathCandidatesForCommand(ctx context.Context, command string, line completionLine, state commandArgState) []string { - switch command { - case string(cli.CommandAdd): - if len(state.positionals) >= 1 { - return pathCandidates(h.Options.WorkDir, line.current, false, "") - } - case string(cli.CommandPull), string(cli.CommandRemove), string(cli.CommandDiff), string(cli.CommandPush), string(cli.CommandStatus): - if len(state.positionals) == 0 { - return h.mirrorPathCandidates(ctx, line.current, nil) - } - case string(cli.CommandSync): - return h.mirrorPathCandidates(ctx, line.current, state.positionals) +func positionalAt(positionals []cli.PositionalSpec, index int) (cli.PositionalSpec, bool) { + if index < len(positionals) { + return positionals[index], true } - return nil + if len(positionals) > 0 && positionals[len(positionals)-1].Repeatable { + return positionals[len(positionals)-1], true + } + return cli.PositionalSpec{}, false } func parseCompletionLine(words []string) completionLine { @@ -248,9 +242,10 @@ func parseCompletionLine(words []string) completionLine { line.current = words[len(words)-1] completed := words[:len(words)-1] + globalFlags := globalCompletionFlags() for i := 0; i < len(completed); { word := completed[i] - if flag, inlineValue, ok := matchCompletionFlag(word, globalCompletionFlags); ok { + if flag, inlineValue, ok := matchCompletionFlag(word, globalFlags); ok { line.globalArgs = append(line.globalArgs, word) i++ if flag.value && inlineValue == "" && i < len(completed) { @@ -271,11 +266,11 @@ func parseCompletionLine(words []string) completionLine { return line } -func parseCompletedCommandArgs(command string, args []string, flags []completionFlag) commandArgState { +func parseCompletedCommandArgs(args []string, flags []completionFlag) commandArgState { var state commandArgState for i := 0; i < len(args); i++ { arg := args[i] - if command == string(cli.CommandDiff) && arg == "--" { + if arg == "--" { return state } if strings.HasPrefix(arg, "-") { @@ -299,10 +294,13 @@ func commandArgsContainDiffSeparator(args []string) bool { return false } -func optionCandidates(prefix string, flags []completionFlag, args []string, global bool) []string { +func optionCandidates(prefix string, flags []completionFlag, args []string, conflicts []cli.ConflictSpec, positionals []string) []string { var candidates []string for _, flag := range flags { - if suppressFlag(flag, args, global) { + if !flag.availability.Allows(positionals) { + continue + } + if suppressFlag(flag, flags, args, conflicts) { continue } for _, name := range []string{flag.long, flag.short} { @@ -317,26 +315,34 @@ func optionCandidates(prefix string, flags []completionFlag, args []string, glob return uniqueSorted(candidates) } -func suppressFlag(flag completionFlag, args []string, global bool) bool { +func suppressFlag(flag completionFlag, flags []completionFlag, args []string, conflicts []cli.ConflictSpec) bool { if !flag.value && flagPresent(flag, args) { return true } - if !global { - return false + for _, conflict := range conflicts { + contains := false + otherPresent := false + for _, option := range conflict.Options { + if option == flag.long { + contains = true + continue + } + otherPresent = otherPresent || flagPresent(completionFlagForLong(flags, option), args) + } + if contains && otherPresent { + return true + } } + return false +} - switch flag.long { - case "--quiet": - return flagPresent(completionFlag{long: "--verbose", short: "-v"}, args) - case "--verbose": - return flagPresent(completionFlag{long: "--quiet"}, args) - case "--no-cache": - return flagPresent(completionFlag{long: "--global-cache-dir", value: true}, args) - case "--global-cache-dir": - return flagPresent(completionFlag{long: "--no-cache"}, args) - default: - return false +func completionFlagForLong(flags []completionFlag, long string) completionFlag { + for _, flag := range flags { + if flag.long == long { + return flag + } } + return completionFlag{long: long} } func flagPresent(flag completionFlag, args []string) bool { diff --git a/internal/command/completion_test.go b/internal/command/completion_test.go index d332a20..00c0de1 100644 --- a/internal/command/completion_test.go +++ b/internal/command/completion_test.go @@ -7,6 +7,7 @@ import ( "strings" "testing" + "braid/internal/clitest" "braid/internal/testutil" ) @@ -32,8 +33,8 @@ func TestCompleteRootCommandsAndGlobalOptions(t *testing.T) { assertCandidate(t, candidates, "pull") assertCandidate(t, candidates, "completion") assertCandidate(t, candidates, "help") - assertNoCandidate(t, candidates, "update") - assertNoCandidate(t, candidates, "up") + assertCandidate(t, candidates, "update") + assertCandidate(t, candidates, "up") assertNoCandidate(t, candidates, "__complete") candidates = completeCandidates(t, dir, "--") @@ -41,6 +42,9 @@ func TestCompleteRootCommandsAndGlobalOptions(t *testing.T) { assertCandidate(t, candidates, "--quiet") assertCandidate(t, candidates, "--no-cache") assertCandidate(t, candidates, "--global-cache-dir") + assertCandidate(t, candidates, "--help") + candidates = completeCandidates(t, dir, "-") + assertCandidate(t, candidates, "-h") candidates = completeCandidates(t, dir, "--verbose", "") assertNoCandidate(t, candidates, "--verbose") @@ -102,8 +106,27 @@ func TestCompleteCommandOptions(t *testing.T) { assertCandidate(t, candidates, "--no-commit") candidates = completeCandidates(t, dir, "completion", "") - if got, want := strings.Join(candidates, "\n"), "bash"; got != want { - t.Fatalf("completion candidates = %q, want %q", got, want) + assertCandidate(t, candidates, "bash") + assertCandidate(t, candidates, "help") + assertCandidate(t, candidates, "--help") + assertCandidate(t, candidates, "-h") +} + +func TestCompleteEveryCommandOptionAtEveryValidPosition(t *testing.T) { + dir := t.TempDir() + for _, test := range clitest.CompletionContractCases() { + t.Run(test.Name, func(t *testing.T) { + candidates := completeCandidates(t, dir, test.Words...) + if test.WantEmpty && len(candidates) != 0 { + t.Fatalf("candidates = %#v, want empty", candidates) + } + for _, want := range test.Want { + assertCandidate(t, candidates, want) + } + for _, unwanted := range test.Unwanted { + assertNoCandidate(t, candidates, unwanted) + } + }) } } @@ -157,9 +180,7 @@ func TestCompleteMirrorPathsAreSilentOutsideRepository(t *testing.T) { if code != 0 { t.Fatalf("__complete exit = %d, stderr = %q", code, stderr.String()) } - if stdout.String() != "" { - t.Fatalf("stdout = %q, want empty", stdout.String()) - } + assertCandidate(t, splitCompletionLines(stdout.String()), "help") if stderr.String() != "" { t.Fatalf("stderr = %q, want empty", stderr.String()) }