Skip to content

v3: yield the version flag's -v alias to a user-defined flag#2330

Merged
dearchap merged 1 commit into
urfave:mainfrom
c-tonneslan:fix/version-alias-conflict-with-user-flag
Jun 28, 2026
Merged

v3: yield the version flag's -v alias to a user-defined flag#2330
dearchap merged 1 commit into
urfave:mainfrom
c-tonneslan:fix/version-alias-conflict-with-user-flag

Conversation

@c-tonneslan

Copy link
Copy Markdown
Contributor

Fixes #2229.

The default version flag carries `-v` as an alias. A user-defined flag that also wants `-v` (canonical case: `--verbose / -v`) wasn't actually broken at parse time: `-v` ended up setting the user flag. But `checkVersion` went through `cmd.Bool`, which resolves aliases, so it asked for the value of `"v"` and got back the user flag's value. The end result: passing `-v` was silently treated as "print version and exit."

Two narrow changes:

  • During root setup, drop any aliases from the local copy of the version flag that are already claimed by a user-defined flag's name or alias. Keeps the user flag the sole owner of the short form.
  • `checkVersion` now finds the actual version flag attached to the command (matching against the canonical primary name from the global VersionFlag) and checks `IsSet` directly, so it can't be fooled by a same-named alias on a different flag.

Added `TestVersionFlagYieldsAliasToUserFlag` covering the reporter's reproducer.

@c-tonneslan c-tonneslan requested a review from a team as a code owner May 16, 2026 22:51
@dearchap

Copy link
Copy Markdown
Contributor

@c-tonneslan Can you rebase off main and fix test cov ?

@dearchap dearchap added the status/waiting-for-response Waiting for response from original requester label Jun 28, 2026
@dearchap

Copy link
Copy Markdown
Contributor

Review Summary

1. dropClashingAliases in command_setup.go — ✅ Merge with fix

Good improvement over the existing flagNamesInUse approach. Instead of dropping the entire --version flag when -v clashes, it only removes the clashing -v alias, preserving --version.

Minor issue: dropClashingAliases is called with cmd.Flags but flagNamesInUse just below uses cmd.allFlags() (which also includes MutuallyExclusiveFlags). If a mutually exclusive flag group claims -v, dropClashingAliases won't see it. Pass cmd.allFlags() instead of cmd.Flags for consistency.

2. checkVersion rewrite in help.go — ❌ Drop this change

Keep the current cmd.versionFlag.IsSet() implementation. The proposed name-based matching (names[0] == primary) is fragile — it could match a user-defined flag whose primary name happens to be "version", causing --version to trigger ShowVersion based on the wrong flag's state.

3. New test — ⚠️ Redundant; consider expanding

The existing TestCommand_Run_CustomFlagCanUseVersionAlias already covers the same scenario. Consider testing that --version still works after -v is yielded to a user flag — neither test currently covers that.

The default version flag carries -v as an alias. A user-defined flag
that also wants -v (canonical case: --verbose / -v) wasn't actually
broken at parse time: -v ended up setting the user flag. But
checkVersion went through cmd.Bool, which resolves aliases, so it
asked for the value of "v" and got back the user flag's value. The
end result: -v was silently treated as "print version and exit".

Two changes, both narrow:

  - During root setup, drop any aliases from the local copy of the
    version flag that are already claimed by a user-defined flag's
    name or alias. Keeps the user flag the sole owner of the
    short form.

  - checkVersion now looks for the actual version flag attached to
    the command (matching against the canonical primary name from
    the global VersionFlag) and checks its IsSet directly, so it
    can't be fooled by a same-named alias on a different flag.

Fixes urfave#2229.

Signed-off-by: Charlie Tonneslan <cst0520@gmail.com>
@c-tonneslan c-tonneslan force-pushed the fix/version-alias-conflict-with-user-flag branch from 9180b31 to 54d38c9 Compare June 28, 2026 01:06
@c-tonneslan

Copy link
Copy Markdown
Contributor Author

Rebased on main, conflict is gone.

Addressed the review:

  • dropClashingAliases now takes cmd.allFlags() instead of cmd.Flags, so a mutually-exclusive group claiming -v is also seen.
  • Dropped the checkVersion rewrite. Once the clashing alias is stripped in setup, cmd.versionFlag.IsSet() already does the right thing, so help.go is untouched now.
  • Replaced my redundant test (the existing TestCommand_Run_CustomFlagCanUseVersionAlias already covers -v) with one that checks --version still prints after -v is yielded. That case fails on main today. Added a small table test for dropClashingAliases to cover the helper.

@dearchap dearchap merged commit 49e84c0 into urfave:main Jun 28, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

status/waiting-for-response Waiting for response from original requester

Projects

None yet

Development

Successfully merging this pull request may close these issues.

--verbose flag gets interpreted as "print version and exit"

2 participants