fix(hooks/exclude): match multi-token entries against command prefix#1985
Open
YOMXXX wants to merge 1 commit into
Open
fix(hooks/exclude): match multi-token entries against command prefix#1985YOMXXX wants to merge 1 commit into
YOMXXX wants to merge 1 commit into
Conversation
`[hooks].exclude_commands` was implemented as a single anchored regex per
entry (`^<escaped-entry>($|\s)`). That regex is correct for the common
single-token case ("curl", "diff") but its multi-token behaviour was
both subtle (relies on `regex::escape` turning the literal space into
`\ `) and easy to break — a typo in the entry, or an unfortunate
metacharacter, silently produced an invalid regex that the code then
downgraded to a substring `starts_with`, which would happily exclude
`git-difftool` along with `git diff`.
This change splits the two intents apart: entries beginning with `^` are
still compiled as regexes (so power users can keep patterns like
`^docker (compose|exec)`), and everything else is treated as a literal
command prefix. The new `is_excluded` then matches a command iff it
either equals the entry or begins with the entry followed by a
whitespace character — giving the obvious semantics requested in rtk-ai#1919.
Interior whitespace inside an entry is canonicalised so `"git diff"`
(two spaces) keeps matching `git diff HEAD`.
Single-token entries ("diff"), invalid-regex fallback, env-prefixed
commands ("PGPASSWORD=… psql"), the existing two-token case
("git push"), bare `^` and empty patterns, the no-substring-match
contract ("go" vs "golangci-lint") and `^`-anchored regex entries all
keep their previous behaviour — covered by the existing tests plus
eight new regression cases that drive the matcher and `rewrite_command`
through `git diff`, three-token wrappers, exact-equality, sibling
subcommands and the `git diffstat` boundary.
Fixes rtk-ai#1919
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.
Summary
[hooks].exclude_commandsentries with more than one whitespace-separatedtoken (e.g.
"git diff","docker compose up") were matched by ananchored regex built from the user input. The matching itself happened
to work for the well-formed case, but the construction was fragile and
the user-facing contract was undocumented — issue #1919 reports that a
config like
does not reliably stop
git diff HEADfrom being rewritten. This PRmakes the contract explicit and the implementation literal: a
multi-token entry matches a command iff the command equals the entry
exactly or starts with the entry followed by a whitespace
character.
Reproduction (before)
Root cause
src/discover/registry.rs::compile_exclude_patternswrapped every entryin
^<regex::escape>($|\s)and then matched it as a regex. Formulti-token entries this:
regex::escapeturning the literal space into\— adetail that is easy to break in future refactors;
starts_withwhenever the regexfails to compile, which means an entry like
git[diffwould happilyexclude
git-difftoolas well asgit diff;indirection.
Fix approach
^-prefixed entries as regex (keeps the existing^curl-style escape hatch for power users).ExcludePattern::Prefix.is_excluded, aPrefixmatches when the (left-trimmed) commandis either equal to the entry or starts with the entry followed by a
whitespace character — never on a substring boundary.
"git diff"(twospaces, typo) still matches
git diff HEAD.^…regexdegrades to a literal prefix on the un-anchored body rather than
silently doing nothing.
Test plan
test_exclude_single_token_still_matches—"diff"still excludesdiff a btest_exclude_two_token_subcommand_git_diff— headline [hooks] exclude_commands does not cover subcommands (e.g. "git diff") #1919 casetest_exclude_two_token_with_many_trailing_args—git diff --staged path/to/filetest_exclude_three_token_subcommand—docker compose up -dtest_exclude_exact_match_with_no_trailing_args— baregit difftest_exclude_two_token_does_not_match_sibling_subcommand—git statusstill rewritestest_exclude_two_token_does_not_match_subcommand_prefix—git diffstat,git-difftoolnot excludedtest_exclude_extra_whitespace_in_entry_is_normalised—"git diff"typo toleratedcargo fmt --allcleancargo clippy --all-targetszero warningscargo test --bin rtk -- --test-threads=8: 1909 passed / 6 ignored / 0 failedconfig.tomlconfirmsrtk rewrite "git diff HEAD"now exits 1.Fixes #1919