diff --git a/README.md b/README.md index 4dabec8..fd0455b 100644 --- a/README.md +++ b/README.md @@ -417,8 +417,8 @@ proto `go_package` option. Which is canonical? matching Scanner - [ ] Add `--deterministic` flag or make it default — omit `createdAt`, sort files (pending design question answer) -- [ ] Wire `--version` flag properly (currently only a `version` subcommand - exists; top-level `--version` shows urfave/cli generic output) +- [x] Wire `--version` flag properly: `--version` and `-V` print the same line + as the `version` subcommand, and `-v` means verbose - [ ] Add retry logic to `fetch` — currently no retries on transient HTTP errors; needs exponential backoff - [ ] `fetch` command uses bare `http.Get` with no timeout — needs `http.Client` diff --git a/TODO.md b/TODO.md index c4e97e2..3207757 100644 --- a/TODO.md +++ b/TODO.md @@ -24,6 +24,8 @@ only thing left of the `chore/align-repo-policies` branch is the list below. # Completed Steps +- 2026-09-21: fixed the `-v` collision between `--verbose` and `--version`; + verbose owns `-v`, and version answers to `--version` and `-V` (#64) - 2026-10-03: pinned the CLI error messages by driving the functions that emit them in `internal/cli/errmsg_test.go`, and made the freshen mtime-presence test distinguish an absent mtime from the epoch (#87) diff --git a/internal/cli/entry_test.go b/internal/cli/entry_test.go index 332becd..e429cb8 100644 --- a/internal/cli/entry_test.go +++ b/internal/cli/entry_test.go @@ -27,6 +27,7 @@ const ( testManifest = "/manifest.mf" testFlagBase = "--base" testFlagNoExtra = "--no-extra-files" + testFlagVersion = "--version" ) var errSimulatedWrite = errors.New("simulated write failure") @@ -102,7 +103,7 @@ func TestVersionCommand(t *testing.T) { t.Parallel() fs := afero.NewMemMapFs() - opts := testOpts([]string{testApp, "version"}, fs) + opts := testOpts([]string{testApp, cmdVersion}, fs) exitCode := runCLI(opts) @@ -113,6 +114,110 @@ func TestVersionCommand(t *testing.T) { assert.Contains(t, stdout, "abc123") } +// TestVFlagCollision covers the -v/--verbose vs --version flag interaction +// (issue #64). Verbose owns -v; version answers to --version and -V. None of +// these invocations may produce a parser error, and the two ways of asking +// for the version must print the same thing. +func TestVFlagCollision(t *testing.T) { + t.Parallel() + + // Invocations that must print the version and exit 0. + versionCases := map[string][]string{ + "long version flag": {testApp, testFlagVersion}, + "short version flag": {testApp, "-V"}, + "verbose then version": {testApp, "-v", testFlagVersion}, + "long verbose and version": {testApp, "--verbose", testFlagVersion}, + } + + for name, args := range versionCases { + t.Run(name, func(t *testing.T) { + t.Parallel() + + opts := testOpts(args, afero.NewMemMapFs()) + exitCode := runCLI(opts) + + assert.Equal(t, 0, exitCode, "stderr: %s", testStderr(t, opts)) + assert.Contains(t, testStdout(t, opts), mfer.Version) + assert.NotContains(t, testStderr(t, opts), "two forms of the same flag") + }) + } + + // Invocations that must enable verbose and exit 0 without a parser error. + verboseCases := map[string][]string{ + "short verbose flag": {testApp, "-v"}, + "long verbose flag": {testApp, "--verbose"}, + } + + for name, args := range verboseCases { + t.Run(name, func(t *testing.T) { + t.Parallel() + + opts := testOpts(args, afero.NewMemMapFs()) + exitCode := runCLI(opts) + + assert.Equal(t, 0, exitCode, "stderr: %s", testStderr(t, opts)) + assert.Contains(t, testStdout(t, opts), cmdGenerate, + "root should show help listing subcommands") + assert.Empty(t, testStderr(t, opts)) + }) + } +} + +// TestVersionFlagAndCommandMatch asserts that "mfer --version" and +// "mfer version" produce identical output (issue #64). +func TestVersionFlagAndCommandMatch(t *testing.T) { + t.Parallel() + + flagOpts := testOpts([]string{testApp, testFlagVersion}, afero.NewMemMapFs()) + require.Equal(t, 0, runCLI(flagOpts)) + + cmdOpts := testOpts([]string{testApp, cmdVersion}, afero.NewMemMapFs()) + require.Equal(t, 0, runCLI(cmdOpts)) + + assert.Equal(t, testStdout(t, flagOpts), testStdout(t, cmdOpts)) +} + +// TestVerbosityFlagsBeforeSubcommand asserts that -q and -v given before the +// subcommand name take effect in that subcommand (issue #64). +func TestVerbosityFlagsBeforeSubcommand(t *testing.T) { + t.Parallel() + + t.Run("quiet before fetch hides the banner", func(t *testing.T) { + t.Parallel() + + opts := testOpts([]string{testApp, "-q", cmdFetch}, afero.NewMemMapFs()) + + assert.Equal(t, 1, runCLI(opts)) + assert.Contains(t, testStderr(t, opts), errURLRequired.Error()) + assert.NotContains(t, testStdout(t, opts), banner) + }) + + t.Run("verbose twice before fetch enables debug logging", func(t *testing.T) { + t.Parallel() + + opts := testOpts([]string{testApp, "-v", "-v", cmdFetch}, afero.NewMemMapFs()) + + assert.Equal(t, 1, runCLI(opts)) + assert.Contains(t, testStderr(t, opts), "fetchManifestOperation()") + }) + + t.Run("quiet before export hides the manifest summary", func(t *testing.T) { + t.Parallel() + + fs := afero.NewMemMapFs() + manifest := buildTestManifest(t, map[string][]byte{"a.txt": []byte("a")}) + require.NoError(t, afero.WriteFile(fs, testManifest, manifest, 0o644)) + + loud := testOpts([]string{testApp, cmdExport, testManifest}, fs) + require.Equal(t, 0, runCLI(loud)) + assert.Contains(t, testStderr(t, loud), "loaded manifest") + + quiet := testOpts([]string{testApp, "-q", cmdExport, testManifest}, fs) + require.Equal(t, 0, runCLI(quiet)) + assert.NotContains(t, testStderr(t, quiet), "loaded manifest") + }) +} + func TestHelpCommand(t *testing.T) { t.Parallel() diff --git a/internal/cli/errmsg_test.go b/internal/cli/errmsg_test.go index 72b4b48..3703194 100644 --- a/internal/cli/errmsg_test.go +++ b/internal/cli/errmsg_test.go @@ -5,6 +5,7 @@ import ( "bytes" "context" "flag" + "io" "net/http" "net/http/httptest" "os" @@ -16,6 +17,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" urfcli "github.com/urfave/cli/v2" + "sneak.berlin/go/mfer/internal/log" "sneak.berlin/go/mfer/mfer" ) @@ -36,12 +38,16 @@ const ( msgFpB = "BBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBB" ) -// runLocked runs fn while holding runMu, so operations that write to the -// process-global logger do not race the other CLI runs. +// runLocked runs fn while holding runMu and with the process-global logger +// pointed at io.Discard, so fn's log lines neither race the other CLI runs +// nor land in the last run's buffers, which its test may still be reading. func runLocked(fn func() error) error { runMu.Lock() defer runMu.Unlock() + log.SetOutput(io.Discard, io.Discard) + log.Init() + return fn() } diff --git a/internal/cli/mfer.go b/internal/cli/mfer.go index 8c4e769..ccd878a 100644 --- a/internal/cli/mfer.go +++ b/internal/cli/mfer.go @@ -19,6 +19,7 @@ const ( cmdCheck = "check" cmdExport = "export" cmdFetch = "fetch" + cmdVersion = "version" flagProgress = "progress" @@ -68,6 +69,12 @@ func (mfa *CLIApp) VersionString() string { return mfer.Version } +// printVersion writes the version line shared by the --version flag and the +// version subcommand, so both produce identical output. +func (mfa *CLIApp) printVersion() { + _, _ = fmt.Fprintf(mfa.Stdout, "%s version %s\n", mfa.appname, mfa.VersionString()) +} + func (mfa *CLIApp) printBanner() { if log.GetLevel() <= log.InfoLevel { _, _ = fmt.Fprintln(mfa.Stdout, banner) @@ -78,20 +85,34 @@ func (mfa *CLIApp) printBanner() { } } +// setVerbosity sets the log level from -v and -q, given before the subcommand +// name (the root's copies), after it (the subcommand's own copies), or both. +// urfave/cli reads a flag from the nearest command that defines it, so each +// command in the lineage is asked. The highest -v count wins rather than the +// sum, because a subcommand without its own copies reads the root's. func (mfa *CLIApp) setVerbosity(c *cli.Context) { _, present := os.LookupEnv("MFER_DEBUG") + verbosity := 0 + quiet := false + + for _, ctx := range c.Lineage() { + verbosity = max(verbosity, ctx.Count("verbose")) + quiet = quiet || ctx.Bool("quiet") + } + switch { case present: log.EnableDebugLogging() - case c.Bool("quiet"): + case quiet: log.SetLevel(log.ErrorLevel) default: - log.SetLevelFromVerbosity(c.Count("verbose")) + log.SetLevelFromVerbosity(verbosity) } } -// commonFlags returns the flags shared by most commands (-v, -q) +// commonFlags returns the -v and -q flags taken by the root and by the +// generate, check, freshen and fetch subcommands. func commonFlags() []cli.Flag { return []cli.Flag{ &cli.BoolFlag{ @@ -259,6 +280,8 @@ func (mfa *CLIApp) exportCommand() *cli.Command { Usage: "Export manifest contents as JSON", ArgsUsage: "[manifest file or URL]", Action: func(c *cli.Context) error { + mfa.setVerbosity(c) + return mfa.exportManifestOperation(c) }, } @@ -266,10 +289,10 @@ func (mfa *CLIApp) exportCommand() *cli.Command { func (mfa *CLIApp) versionCommand() *cli.Command { return &cli.Command{ - Name: "version", + Name: cmdVersion, Usage: "Show version", Action: func(_ *cli.Context) error { - _, _ = fmt.Fprintln(mfa.Stdout, mfa.VersionString()) + mfa.printVersion() return nil }, @@ -325,6 +348,21 @@ func (mfa *CLIApp) run(args []string) { log.SetOutput(mfa.Stdout, mfa.Stderr) log.Init() + // -v means verbose, not version. urfave/cli's built-in version flag + // claims -v by default, which made "mfer -v --version" fail to parse and + // gave -v a different meaning at the root than on the generate, check, + // freshen and fetch subcommands, where it means verbose. Verbose is the + // more common meaning of -v in tools that offer both, so -v means verbose + // at the root too and the version flag takes the capital -V. + // VersionFlag and VersionPrinter are urfave/cli package globals; run() is + // serialized in tests, so assigning them here is safe. + cli.VersionFlag = &cli.BoolFlag{ + Name: cmdVersion, + Aliases: []string{"V"}, + Usage: "print the version", + } + cli.VersionPrinter = func(_ *cli.Context) { mfa.printVersion() } + mfa.app = &cli.App{ Name: mfa.appname, Usage: "Manifest generator", @@ -332,11 +370,13 @@ func (mfa *CLIApp) run(args []string) { EnableBashCompletion: true, Writer: mfa.Stdout, ErrWriter: mfa.Stderr, + Flags: commonFlags(), Action: func(c *cli.Context) error { if c.Args().Len() > 0 { return fmt.Errorf("%w %q", errUnknownCommand, c.Args().First()) } + mfa.setVerbosity(c) mfa.printBanner() return cli.ShowAppHelp(c)