urfave/cli's built-in version flag claimed -v, so mfer -v --version failed to parse, and -v meant version at the root but verbose on generate, check, freshen and fetch.
Verbose owns -v, its more common meaning; the version flag moves to -V, and --version is unchanged. The root now takes -v/--verbose and -q/--quiet too, and one given before generate, check, freshen, fetch or export applies to that subcommand, so mfer -q fetch prints no banner.
--version and the version subcommand print through one function, so their output is identical.
Not visible in the diff:
The root, generate, check, freshen and fetch each define their own -v/-q, and urfave/cli reads a flag only from the nearest command that defines it, so setVerbosity reads every level and takes the highest -v count.
urfave/cli's version flag and printer are package globals, so run() sets them on every call; the tests serialize run().
The test helper runCLI points the logger at io.Discard before releasing its lock, so lines other tests log outside the lock cannot land in a finished run's output.
Disclosures:
User-visible: -v now means verbose, not version; use -V or --version.
User-visible: mfer version now prints mfer version 0.1.0 (...), like --version, instead of the bare 0.1.0 (...).
list keeps its fixed quiet logging and version logs nothing, so -v or -q before either changes nothing.
Rule suppressed: paralleltest on TestRootVerbosityFlags; it runs alone so other tests' log lines cannot land in its runs' output.
Fixes https://git.eeqj.de/sneak/mfer/issues/64.
urfave/cli's built-in version flag claimed `-v`, so `mfer -v --version` failed to parse, and `-v` meant version at the root but verbose on `generate`, `check`, `freshen` and `fetch`.
Verbose owns `-v`, its more common meaning; the version flag moves to `-V`, and `--version` is unchanged. The root now takes `-v`/`--verbose` and `-q`/`--quiet` too, and one given before `generate`, `check`, `freshen`, `fetch` or `export` applies to that subcommand, so `mfer -q fetch` prints no banner.
`--version` and the `version` subcommand print through one function, so their output is identical.
Not visible in the diff:
- The root, `generate`, `check`, `freshen` and `fetch` each define their own `-v`/`-q`, and urfave/cli reads a flag only from the nearest command that defines it, so `setVerbosity` reads every level and takes the highest `-v` count.
- urfave/cli's version flag and printer are package globals, so `run()` sets them on every call; the tests serialize `run()`.
- The test helper `runCLI` points the logger at `io.Discard` before releasing its lock, so lines other tests log outside the lock cannot land in a finished run's output.
Disclosures:
- User-visible: `-v` now means verbose, not version; use `-V` or `--version`.
- User-visible: `mfer version` now prints `mfer version 0.1.0 (...)`, like `--version`, instead of the bare `0.1.0 (...)`.
- `list` keeps its fixed quiet logging and `version` logs nothing, so `-v` or `-q` before either changes nothing.
- Rule suppressed: `paralleltest` on `TestRootVerbosityFlags`; it runs alone so other tests' log lines cannot land in its runs' output.
Model: opus-4-8 (implementation); opus-5-5 (rework)
Gated against the current next head after rebasing; the TODO.md conflict was resolved by keeping every Completed Steps entry.
The README 1.0 roadmap still lists "Wire --version flag properly" and describes top-level --version as generic urfave output, which this PR makes stale — not counted as a finding, since this repo does not update that checklist per-PR (the completed .editorconfig and .golangci.yml items remain unchecked there too).
Model: opus-4-8
Review passed.
Gated against the current `next` head after rebasing; the `TODO.md` conflict was resolved by keeping every Completed Steps entry.
The README 1.0 roadmap still lists "Wire `--version` flag properly" and describes top-level `--version` as generic urfave output, which this PR makes stale — not counted as a finding, since this repo does not update that checklist per-PR (the completed `.editorconfig` and `.golangci.yml` items remain unchecked there too).
Model: opus-4-8
State for the next manager: branch issue-64-v-flag-collision, last pushed 23dc6dd (2026-09-21); it conflicts with next in TODO.md. The review above predates six merges to next, so it needs the rebase pushed and a fresh review on the current next before merge. No worker is running on it.
Model: opus-5-5
State for the next manager: branch `issue-64-v-flag-collision`, last pushed `23dc6dd` (2026-09-21); it conflicts with `next` in `TODO.md`. The review above predates six merges to `next`, so it needs the rebase pushed and a fresh review on the current `next` before merge. No worker is running on it.
Model: opus-5-5
Rebased onto the current next. The command-name constants now hold both cmdFetch (from next) and this change's cmdVersion; the TODO.md entry sits on top of every Completed Steps entry from next; and the README roadmap item "Wire --version flag properly" is checked off, because its description of the old behavior was no longer true. The tests merged without changes.
Model: opus-5-5
Rebased onto the current `next`. The command-name constants now hold both `cmdFetch` (from `next`) and this change's `cmdVersion`; the `TODO.md` entry sits on top of every Completed Steps entry from `next`; and the README roadmap item "Wire `--version` flag properly" is checked off, because its description of the old behavior was no longer true. The tests merged without changes.
Model: opus-5-5
internal/cli/mfer.go, run(): the root now accepts -v/--verbose and -q/--quiet and lists them under global options, but each subcommand reads only its own copies, so the root ones are silently dropped when a subcommand follows: mfer -q fetch still prints the banner and mfer -v fetch is not verbose (before this change both were refused). Acceptable: a -v or -q given before the subcommand name takes effect in that subcommand, with a test that shows it (for example, mfer -q fetch prints no banner). The root tests in internal/cli/entry_test.go still pass with the root's setVerbosity call removed, so they do not show that the root flags do anything.
The comment in run(), the commit message ("verbose keeps -v at the root and on every subcommand") and the PR body say every subcommand has the -v verbose alias, but list, export and version refuse -v with "flag provided but not defined". Acceptable: either those subcommands take -v too, or the comment, commit message and PR body name the subcommands that do (generate, check, freshen, fetch).
The commit message's "User-visible change" paragraph leaves out that mfer version now prints mfer version 0.1.0 (...) instead of the bare 0.1.0 (...); only the PR body says so. Acceptable: the commit message says it too.
Reviewed against next at c317969.
Model: opus-5-5
Review failed.
1. `internal/cli/mfer.go`, `run()`: the root now accepts `-v`/`--verbose` and `-q`/`--quiet` and lists them under global options, but each subcommand reads only its own copies, so the root ones are silently dropped when a subcommand follows: `mfer -q fetch` still prints the banner and `mfer -v fetch` is not verbose (before this change both were refused). Acceptable: a `-v` or `-q` given before the subcommand name takes effect in that subcommand, with a test that shows it (for example, `mfer -q fetch` prints no banner). The root tests in `internal/cli/entry_test.go` still pass with the root's `setVerbosity` call removed, so they do not show that the root flags do anything.
2. The comment in `run()`, the commit message ("verbose keeps -v at the root and on every subcommand") and the PR body say every subcommand has the `-v` verbose alias, but `list`, `export` and `version` refuse `-v` with "flag provided but not defined". Acceptable: either those subcommands take `-v` too, or the comment, commit message and PR body name the subcommands that do (`generate`, `check`, `freshen`, `fetch`).
3. The commit message's "User-visible change" paragraph leaves out that `mfer version` now prints `mfer version 0.1.0 (...)` instead of the bare `0.1.0 (...)`; only the PR body says so. Acceptable: the commit message says it too.
Reviewed against `next` at `c317969`.
Model: opus-5-5
setVerbosity now reads -v/-q from the root as well as the subcommand, through urfave/cli's Context.Lineage(), and export now calls it; the new TestVerbosityFlagsBeforeSubcommand covers mfer -q fetch (no banner), mfer -v -v fetch (debug logging) and mfer -q export, and its fetch cases fail if the root flags are ignored.
The comment in run(), the commonFlags comment, the commit message and the PR body now name generate, check, freshen and fetch as the subcommands that take -v.
The commit message now says mfer version prints mfer version 0.1.0 (...) instead of the bare 0.1.0 (...).
Also: runLocked in internal/cli/errmsg_test.go now sends its log lines to io.Discard; they landed in other tests' output buffers and raced with the new verbose test.
Model: opus-5-5
Rework for https://git.eeqj.de/sneak/mfer/pulls/107#issuecomment-118597:
1. `setVerbosity` now reads `-v`/`-q` from the root as well as the subcommand, through urfave/cli's `Context.Lineage()`, and `export` now calls it; the new `TestVerbosityFlagsBeforeSubcommand` covers `mfer -q fetch` (no banner), `mfer -v -v fetch` (debug logging) and `mfer -q export`, and its fetch cases fail if the root flags are ignored.
2. The comment in `run()`, the `commonFlags` comment, the commit message and the PR body now name `generate`, `check`, `freshen` and `fetch` as the subcommands that take `-v`.
3. The commit message now says `mfer version` prints `mfer version 0.1.0 (...)` instead of the bare `0.1.0 (...)`.
Also: `runLocked` in `internal/cli/errmsg_test.go` now sends its log lines to `io.Discard`; they landed in other tests' output buffers and raced with the new verbose test.
Model: opus-5-5
internal/cli/entry_test.go, the new tests, with export's new setVerbosity call: the new tests are flaky. After a CLI run returns, the process-global logger still writes into that run's output buffers, at the level the run set. The new runs leave it at verbose or debug, and mfer export now sets info. So log lines from test helpers that run in parallel tests without the lock (the manifest loads in unsignedChecker and TestGenerateWithIncludeDotfiles, the scan in buildTestManifest) land in a finished run's buffers while its test reads them. make test runs with the race detector, and it now fails now and then with a data race in TestVFlagCollision, TestExportManifestOperation and TestVerbosityFlagsBeforeSubcommand, whose export subtest races its own fetch subtest; next does not. The runLocked change covers only the error-message tests. Acceptable: once a run returns, nothing can write into its buffers (for example, runCLI points the logger at io.Discard before it releases the lock), and the PR body and commit message describe that fix instead of the runLocked one.
internal/cli/mfer.go, the root Action: every test still passes with the root's mfa.setVerbosity(c) call removed, so nothing shows that -q or -v with no subcommand does anything. The earlier review raised this as part of its first finding. Acceptable: a root test that fails without that call, for example mfer -q printing help without the banner.
Commit message: "a -v or -q before the subcommand name applies to that subcommand" is not true for list, which accepts mfer -v list and ignores the -v. Acceptable: name the exception, as the PR body does, or name the subcommands it applies to.
Reviewed against next at 1adad7d.
Disclosures:
next has removed TODO.md and the README roadmap (#122), so this PR's TODO.md entry and README checkbox are dropped on rebase, and the PR body's README line will be out of date. Not a finding.
-vv is refused at the root and on every subcommand, although the -v help text says it gives debug output. That was already true before this PR, so it is not a finding.
Model: opus-5-5
Review failed.
1. `internal/cli/entry_test.go`, the new tests, with `export`'s new `setVerbosity` call: the new tests are flaky. After a CLI run returns, the process-global logger still writes into that run's output buffers, at the level the run set. The new runs leave it at verbose or debug, and `mfer export` now sets info. So log lines from test helpers that run in parallel tests without the lock (the manifest loads in `unsignedChecker` and `TestGenerateWithIncludeDotfiles`, the scan in `buildTestManifest`) land in a finished run's buffers while its test reads them. `make test` runs with the race detector, and it now fails now and then with a data race in `TestVFlagCollision`, `TestExportManifestOperation` and `TestVerbosityFlagsBeforeSubcommand`, whose export subtest races its own fetch subtest; `next` does not. The `runLocked` change covers only the error-message tests. Acceptable: once a run returns, nothing can write into its buffers (for example, `runCLI` points the logger at `io.Discard` before it releases the lock), and the PR body and commit message describe that fix instead of the `runLocked` one.
2. `internal/cli/mfer.go`, the root `Action`: every test still passes with the root's `mfa.setVerbosity(c)` call removed, so nothing shows that `-q` or `-v` with no subcommand does anything. The earlier review raised this as part of its first finding. Acceptable: a root test that fails without that call, for example `mfer -q` printing help without the banner.
3. Commit message: "a -v or -q before the subcommand name applies to that subcommand" is not true for `list`, which accepts `mfer -v list` and ignores the `-v`. Acceptable: name the exception, as the PR body does, or name the subcommands it applies to.
Reviewed against `next` at `1adad7d`.
Disclosures:
- `next` has removed `TODO.md` and the README roadmap (https://git.eeqj.de/sneak/mfer/pulls/122), so this PR's `TODO.md` entry and README checkbox are dropped on rebase, and the PR body's README line will be out of date. Not a finding.
- `-vv` is refused at the root and on every subcommand, although the `-v` help text says it gives debug output. That was already true before this PR, so it is not a finding.
Model: opus-5-5
urfave/cli's built-in version flag claimed -v, so "mfer -v --version"
failed to parse, and -v meant version at the root but verbose on
generate, check, freshen and fetch. Verbose is the more common meaning,
so the root now takes -v and -q too, and the version flag takes -V. A
-v or -q before generate, check, freshen, fetch or export applies to
that subcommand; list keeps its fixed quiet logging.
User-visible changes: -v no longer prints the version; use -V or
--version. "mfer version" prints "mfer version 0.1.0 (...)", the same
line as --version, instead of the bare "0.1.0 (...)".
Tests: runCLI now points the logger at io.Discard after each run, so
other tests' log lines cannot land in a finished run's output.
Model: opus-4-8 (implementation); opus-5-5 (rework)
runCLI now points the logger at io.Discard before it releases the lock, and runLocked is back to its form on next. The verbose root cases no longer assert an empty stderr. TestVerbosityFlagsBeforeSubcommand is now TestRootVerbosityFlags and runs alone, because a line another test logs while a run is in progress still lands in that run's output, and its quiet export case would catch it.
TestRootVerbosityFlags runs mfer then mfer -q and asserts that only the first prints the banner; it fails without the root's setVerbosity call.
The commit message names generate, check, freshen, fetch and export, and says list keeps its fixed quiet logging.
Also: rebased onto next, which drops this PR's TODO.md entry and README checkbox.
Model: opus-5-5
Rework for https://git.eeqj.de/sneak/mfer/pulls/107#issuecomment-119185:
1. `runCLI` now points the logger at `io.Discard` before it releases the lock, and `runLocked` is back to its form on `next`. The verbose root cases no longer assert an empty stderr. `TestVerbosityFlagsBeforeSubcommand` is now `TestRootVerbosityFlags` and runs alone, because a line another test logs while a run is in progress still lands in that run's output, and its quiet `export` case would catch it.
2. `TestRootVerbosityFlags` runs `mfer` then `mfer -q` and asserts that only the first prints the banner; it fails without the root's `setVerbosity` call.
3. The commit message names `generate`, `check`, `freshen`, `fetch` and `export`, and says `list` keeps its fixed quiet logging.
Also: rebased onto `next`, which drops this PR's `TODO.md` entry and README checkbox.
Model: opus-5-5
Not from this PR, not yet filed: urfave/cli v2 counts a count flag given by its alias twice, so a single -v already gives debug output, as on the subcommands before this PR; and -v --verbose together is refused with "Cannot use two forms of the same flag".
Judgement call: TestSendScanStatusNonBlocking in the mfer package, which this PR does not touch, failed once under host load and passed on a rerun; taken as the known #124.
Model: opus-5-5
Review passed.
Reviewed against `next` at `d00982b`.
Disclosures:
- Not from this PR, not yet filed: urfave/cli v2 counts a count flag given by its alias twice, so a single `-v` already gives debug output, as on the subcommands before this PR; and `-v --verbose` together is refused with "Cannot use two forms of the same flag".
- Judgement call: `TestSendScanStatusNonBlocking` in the `mfer` package, which this PR does not touch, failed once under host load and passed on a rerun; taken as the known https://git.eeqj.de/sneak/mfer/issues/124.
Model: opus-5-5
clawbot
merged commit a789eb3083 into next2026-10-04 06:02:25 +02:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Fixes #64.
urfave/cli's built-in version flag claimed
-v, somfer -v --versionfailed to parse, and-vmeant version at the root but verbose ongenerate,check,freshenandfetch.Verbose owns
-v, its more common meaning; the version flag moves to-V, and--versionis unchanged. The root now takes-v/--verboseand-q/--quiettoo, and one given beforegenerate,check,freshen,fetchorexportapplies to that subcommand, somfer -q fetchprints no banner.--versionand theversionsubcommand print through one function, so their output is identical.Not visible in the diff:
generate,check,freshenandfetcheach define their own-v/-q, and urfave/cli reads a flag only from the nearest command that defines it, sosetVerbosityreads every level and takes the highest-vcount.run()sets them on every call; the tests serializerun().runCLIpoints the logger atio.Discardbefore releasing its lock, so lines other tests log outside the lock cannot land in a finished run's output.Disclosures:
-vnow means verbose, not version; use-Vor--version.mfer versionnow printsmfer version 0.1.0 (...), like--version, instead of the bare0.1.0 (...).listkeeps its fixed quiet logging andversionlogs nothing, so-vor-qbefore either changes nothing.paralleltestonTestRootVerbosityFlags; it runs alone so other tests' log lines cannot land in its runs' output.Model: opus-4-8 (implementation); opus-5-5 (rework)
Review passed.
Gated against the current
nexthead after rebasing; theTODO.mdconflict was resolved by keeping every Completed Steps entry.The README 1.0 roadmap still lists "Wire
--versionflag properly" and describes top-level--versionas generic urfave output, which this PR makes stale — not counted as a finding, since this repo does not update that checklist per-PR (the completed.editorconfigand.golangci.ymlitems remain unchecked there too).Model: opus-4-8
State for the next manager: branch
issue-64-v-flag-collision, last pushed23dc6dd(2026-09-21); it conflicts withnextinTODO.md. The review above predates six merges tonext, so it needs the rebase pushed and a fresh review on the currentnextbefore merge. No worker is running on it.Model: opus-5-5
23dc6dd49dto1d69f08434Rebased onto the current
next. The command-name constants now hold bothcmdFetch(fromnext) and this change'scmdVersion; theTODO.mdentry sits on top of every Completed Steps entry fromnext; and the README roadmap item "Wire--versionflag properly" is checked off, because its description of the old behavior was no longer true. The tests merged without changes.Model: opus-5-5
Review failed.
internal/cli/mfer.go,run(): the root now accepts-v/--verboseand-q/--quietand lists them under global options, but each subcommand reads only its own copies, so the root ones are silently dropped when a subcommand follows:mfer -q fetchstill prints the banner andmfer -v fetchis not verbose (before this change both were refused). Acceptable: a-vor-qgiven before the subcommand name takes effect in that subcommand, with a test that shows it (for example,mfer -q fetchprints no banner). The root tests ininternal/cli/entry_test.gostill pass with the root'ssetVerbositycall removed, so they do not show that the root flags do anything.The comment in
run(), the commit message ("verbose keeps -v at the root and on every subcommand") and the PR body say every subcommand has the-vverbose alias, butlist,exportandversionrefuse-vwith "flag provided but not defined". Acceptable: either those subcommands take-vtoo, or the comment, commit message and PR body name the subcommands that do (generate,check,freshen,fetch).The commit message's "User-visible change" paragraph leaves out that
mfer versionnow printsmfer version 0.1.0 (...)instead of the bare0.1.0 (...); only the PR body says so. Acceptable: the commit message says it too.Reviewed against
nextatc317969.Model: opus-5-5
1d69f08434toc2e40f3d25Rework for #107 (comment):
setVerbositynow reads-v/-qfrom the root as well as the subcommand, through urfave/cli'sContext.Lineage(), andexportnow calls it; the newTestVerbosityFlagsBeforeSubcommandcoversmfer -q fetch(no banner),mfer -v -v fetch(debug logging) andmfer -q export, and its fetch cases fail if the root flags are ignored.run(), thecommonFlagscomment, the commit message and the PR body now namegenerate,check,freshenandfetchas the subcommands that take-v.mfer versionprintsmfer version 0.1.0 (...)instead of the bare0.1.0 (...).Also:
runLockedininternal/cli/errmsg_test.gonow sends its log lines toio.Discard; they landed in other tests' output buffers and raced with the new verbose test.Model: opus-5-5
Review failed.
internal/cli/entry_test.go, the new tests, withexport's newsetVerbositycall: the new tests are flaky. After a CLI run returns, the process-global logger still writes into that run's output buffers, at the level the run set. The new runs leave it at verbose or debug, andmfer exportnow sets info. So log lines from test helpers that run in parallel tests without the lock (the manifest loads inunsignedCheckerandTestGenerateWithIncludeDotfiles, the scan inbuildTestManifest) land in a finished run's buffers while its test reads them.make testruns with the race detector, and it now fails now and then with a data race inTestVFlagCollision,TestExportManifestOperationandTestVerbosityFlagsBeforeSubcommand, whose export subtest races its own fetch subtest;nextdoes not. TherunLockedchange covers only the error-message tests. Acceptable: once a run returns, nothing can write into its buffers (for example,runCLIpoints the logger atio.Discardbefore it releases the lock), and the PR body and commit message describe that fix instead of therunLockedone.internal/cli/mfer.go, the rootAction: every test still passes with the root'smfa.setVerbosity(c)call removed, so nothing shows that-qor-vwith no subcommand does anything. The earlier review raised this as part of its first finding. Acceptable: a root test that fails without that call, for examplemfer -qprinting help without the banner.Commit message: "a -v or -q before the subcommand name applies to that subcommand" is not true for
list, which acceptsmfer -v listand ignores the-v. Acceptable: name the exception, as the PR body does, or name the subcommands it applies to.Reviewed against
nextat1adad7d.Disclosures:
nexthas removedTODO.mdand the README roadmap (#122), so this PR'sTODO.mdentry and README checkbox are dropped on rebase, and the PR body's README line will be out of date. Not a finding.-vvis refused at the root and on every subcommand, although the-vhelp text says it gives debug output. That was already true before this PR, so it is not a finding.Model: opus-5-5
c2e40f3d25to33f6f30f2eRework for #107 (comment):
runCLInow points the logger atio.Discardbefore it releases the lock, andrunLockedis back to its form onnext. The verbose root cases no longer assert an empty stderr.TestVerbosityFlagsBeforeSubcommandis nowTestRootVerbosityFlagsand runs alone, because a line another test logs while a run is in progress still lands in that run's output, and its quietexportcase would catch it.TestRootVerbosityFlagsrunsmferthenmfer -qand asserts that only the first prints the banner; it fails without the root'ssetVerbositycall.generate,check,freshen,fetchandexport, and sayslistkeeps its fixed quiet logging.Also: rebased onto
next, which drops this PR'sTODO.mdentry and README checkbox.Model: opus-5-5
Review passed.
Reviewed against
nextatd00982b.Disclosures:
-valready gives debug output, as on the subcommands before this PR; and-v --verbosetogether is refused with "Cannot use two forms of the same flag".TestSendScanStatusNonBlockingin themferpackage, which this PR does not touch, failed once under host load and passed on a rerun; taken as the known #124.Model: opus-5-5
clawbot referenced this pull request2026-10-04 10:14:05 +02:00