fix(args)!: stop greedy multi-value options from swallowing the positional range - #1592
Open
cds-amal wants to merge 3 commits into
Open
fix(args)!: stop greedy multi-value options from swallowing the positional range#1592cds-amal wants to merge 3 commits into
cds-amal wants to merge 3 commits into
Conversation
`--include-path` and `--exclude-path` were declared `num_args(1..)` so they could take space-separated patterns. clap reads that greedily, so in `git-cliff --include-path 'pkg/**' v1.0.0..v1.1.0` the trailing range is absorbed as one more glob: `opt.range` comes back `None` and git-cliff emits the whole (path-filtered) history. Nothing warns, and the output is plausible enough to ship, which is the bad kind of bug. Both options now take exactly one value per occurrence. The two multi-pattern forms that were actually documented still work: repeat the flag, or pass one quoted space-delimited value (`value_delimiter = ' '` still splits it, which is also how the `GIT_CLIFF_INCLUDE_PATH` env var keeps working). The trade-off is spelled out in `cli_include_path_unquoted_second_token_falls_to_range_slot`: the unquoted multi-token form (`--include-path a b`, no quotes) used to be absorbed and now puts `b` in the RANGE slot, where it fails loudly at revparse. Loud failure on an undocumented form beats a silently wrong changelog on a documented one. The fixture suite gains a second run of `test-monorepo-include-path` with the range moved after `--include-path`, sharing the existing `expected.md`: both orderings must now produce the same changelog. BREAKING CHANGE: `--include-path` and `--exclude-path` accept one value per occurrence. Unquoted multi-pattern invocations such as `--include-path 'a/**' 'b/**'` must become repeated flags (`--include-path 'a/**' --include-path 'b/**'`) or a single quoted value (`--include-path 'a/** b/**'`).
…range
The same greedy `num_args(1..)` declaration fixed for the path flags in
the previous commit sits on `--with-commit` and `--skip-commit`, with
the same result: a trailing positional range is consumed as one more
message or SHA, `opt.range` comes back `None`, and the changelog covers
all of history. Both fail silently, because any string is a plausible
commit message or SHA and nothing rejects the range once it lands in the
wrong slot.
$ git cliff --with-commit "feat: synthetic" v2.12.0..v2.13.0
...whole history
$ git cliff v2.12.0..v2.13.0 --with-commit "feat: synthetic"
...the range
Both orderings now produce identical output.
`--repository` shares the declaration but not the defect: its
`value_parser = Opt::parse_dir` rejects a range as a path and errors
loudly, so it is left alone.
Neither flag gets a `value_delimiter`, so repeated flags become the only
multi-value form. That is deliberate rather than an oversight. A commit
message legitimately contains spaces, and `--with-commit "<sha> feat:
add X"` is a documented form (see website/docs/usage/adding-commits.md),
so splitting on whitespace would corrupt it;
`cli_with_commit_keeps_whitespace_inside_one_value` pins that. A
delimiter would be safe on `--skip-commit`, where SHA1s contain no
spaces, but that is new behavior rather than a fix, so it stays out of
scope here.
This is deliberately a separate commit from the path-flag fix. The path
flags are where the bug was reported and where the range/path
combination is a documented workflow; these two are the same defect
found by inspection. If the wider scope is unwanted, reverting this
commit alone leaves the reported fix intact.
BREAKING CHANGE: `--with-commit` and `--skip-commit` accept one value
per occurrence. Unquoted multi-value invocations such as `--with-commit
"feat: a" "feat: b"` must become repeated flags (`--with-commit "feat:
a" --with-commit "feat: b"`).
|
Thanks for opening this pull request! Please check out our contributing guidelines! ⛰️ |
17 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Declares the greedy multi-value options as one value per occurrence (
num_args(1)), so a trailing positionalRANGElands in theRANGEslot instead of being absorbed as one more value.Two commits, no new surface:
--include-path/--exclude-path(keepingvalue_delimiter(' ')): the flags the bug was reported against, where combining a range with a path filter is a documented monorepo workflow. Six parser-level regression tests and one extra CI fixture entry.--with-commit/--skip-commit: the same defect, found by inspection rather than by report. Four more tests.The split is deliberate. If the wider scope is unwanted, reverting the second commit leaves the reported fix intact and green; nothing in the first commit depends on it.
Motivation and Context
Fixes #1591.
num_args(1..)makes clap greedy, sogit cliff --include-path 'pkg/**' pkg/v1.0.0..pkg/v1.1.0parses the range as a glob.opt.rangeisNone, git-cliff walks all of history, path-filters it, and emits a changelog. Nothing warns. The parse misses the stated intent and returns a wrong result that is plausible enough to ship: in a monorepo, where this flag combination is the whole point, it reaches published release notes before anyone catches it.The reordered invocation (
git cliff pkg/v1.0.0..pkg/v1.1.0 --include-path 'pkg/**') parses correctly, which is what makes the defect hard to see: the same arguments in a different order mean different things to clap while meaning the same thing to the person who typed them. Call that property intent-idempotence, the guarantee that reordering arguments never changes the intent they express; greedy multi-value options break it, and they break it silently, because a glob pattern and a revision range are both just strings to clap.Restoring that guarantee is worth a breaking parse change. Every other class of range mistake in git-cliff fails loudly at revparse; this one doesn't fail at all.
Surfaced by @leoweyr in review of #1489 (comment). Split out of that PR deliberately: the compatibility trade-off below deserves its own decision, on its own diff, rather than riding along inside a feature.
The same declaration appears on three more options
--with-commitand--skip-commitare alsonum_args(1..)and swallow a trailing range identically. Verified on this repository, before the second commit:Both fail silently, for the same reason as the path flags: any string is a plausible commit message or SHA, so nothing rejects the range once it lands in the wrong slot. After the second commit, both orderings produce byte-identical output.
--repositoryshares the declaration but not the defect, so it is untouched: itsvalue_parser = Opt::parse_dirrejects a range as a path and errors loudly rather than silently.The compatibility trade-off
For the path flags, every documented multi-pattern form still works, and each already has a CI fixture pinning it, from the work on #123, the
value_delimiterfix in #1450, and the fixture set #1455:test-include-path-repeated-flagtest-include-path-space-separatedtest-include-path-configvalue_delimiter, notnum_args):test-include-path-env-varAll four stay green here, unmodified, so the multi-value behavior #123 asked for and #1450 delivered is provably intact rather than merely believed intact.
The second commit's surface is sharper, and worth deciding on separately. Neither
--with-commitnor--skip-commitcarries avalue_delimiter, and neither should:--with-commit "8f55e69… feat: add X"is a documented form (website/docs/usage/adding-commits.md) whose single value contains spaces, so splitting on whitespace would corrupt it.cli_with_commit_keeps_whitespace_inside_one_valuepins that. The consequence is that repeated flags become the only multi-value form for those two, where the path flags keep both forms. (A delimiter would be safe on--skip-commit, since SHA1s contain no spaces, but that is new behavior rather than a fix, so it stays out of scope.)The one form that changes for the path flags is unquoted multiple tokens:
That form appears in no docs, fixtures, or tests, though the rendered help (
--include-path <PATTERN>..., now<PATTERN>) did advertise it. Hencefix!with aBREAKING CHANGE:footer.How Has This Been Tested?
Ten parser-level tests in
args.rs, no repository required.First commit, the path flags:
cli_include_path_does_not_swallow_trailing_positional_rangeand its--exclude-pathtwin: the reported invocation now parses to the right intent.cli_include_path_multiple_patterns_still_parse: repeated flags and the quoted space-delimited value both yield two patterns.cli_include_path_unquoted_second_token_falls_to_range_slot: characterizes the trade-off above, so the compatibility change is visible in the test suite rather than only in this description.cli_positional_range_before_include_path_is_not_swallowedandcli_double_dash_protects_trailing_positional_range: the two orderings that already worked keep working.Second commit, the commit flags:
cli_with_commit_does_not_swallow_trailing_positional_rangeandcli_skip_commit_does_not_swallow_trailing_positional_range: the same defect, the same fix.cli_with_commit_keeps_whitespace_inside_one_value: the documented"<sha> <message>"form stays a single value, which is why neither flag gets avalue_delimiter.cli_repeated_flags_remain_the_multi_value_form: repeated occurrences still accumulate for both.The pre-existing
path_env_vars_are_split_into_multiple_patternstest covers the env-var path and is unchanged, which is the evidence thatvalue_delimiterstill carries the multi-pattern case.End to end, the CI fixture matrix now runs
test-monorepo-include-pathtwice against oneexpected.md, with the range before and after--include-path. The path-first ordering is exactly the invocation that used to lose its range, so the two runs must now agree.cargo test --workspacepasses at each commit, not just at the tip (repo::test::git_upstream_remoteneeds a branch with an upstream configured)cargo test --workspacepasses with the second commit reverted, confirming the split is realcargo +nightly fmt --all --checkcleancargo clippy --tests -- -D warnings: no new warnings frommaintest-monorepo-include-pathgreen in both argument orderstest-include-path-*fixtures from Add test fixtures forGIT_CLIFF_INCLUDE_PATHmultiple-value handling #1455 unchanged and greenScreenshots / Logs (if applicable)
Before and after, using
--dry-runfrom #1489 to make the parse visible (the fix itself does not depend on that branch):Types of Changes
Checklist:
Note for reviewers: the second commit is separable by design.
git revert <sha>on this branch applies cleanly and leaves a green tree, so narrowing the scope to the reported flags costs one command rather than a rewrite.#1489 (range-endpoint flags) is stacked on this branch. Until this merges, #1489's diff includes this commit as an ancestor; once it lands, that diff shrinks to the feature alone.