Repository navigation
feat(generate): let git hooks carry mise flags - #11820
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughGit pre-commit generation now accepts trailing ChangesGit pre-commit argument support
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant CLI
participant GitPreCommit
participant Hook
CLI->>GitPreCommit: pass trailing mise arguments
GitPreCommit->>Hook: generate shell-quoted command
Hook->>Hook: run mise arguments before run
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThe PR allows generated Git hooks to preserve
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains; the new parser-level test now verifies that passthrough tokens reach the nested command without being consumed as global flags. Important Files Changed
Reviews (4): Last reviewed commit: "feat(generate): let git hooks carry mise..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/cli/generate/git_pre_commit.rs (3)
135-141: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMake the no-argument test enforce byte identity.
Line [140] only checks that the expected command appears somewhere in the output. The test still passes if the hook contains extra output or a duplicate command. Compare
outwith a frozen pre-change hook fixture when byte-identical output is required.Suggested assertion
- assert!(out.contains(r#"exec mise run pre-commit "$@""#), "{out}"); + assert_eq!(out, EXPECTED_NO_MISE_ARGS_HOOK);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cli/generate/git_pre_commit.rs` around lines 135 - 141, Update no_mise_args_leaves_the_command_unchanged to assert exact byte-for-byte equality between out and the frozen pre-change hook fixture, rather than checking that the command merely appears via contains. Preserve the existing generate("pre-commit", "pre-commit") invocation and use the fixture’s complete expected output so extra or duplicate content fails the test.
106-111: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a parser-level test for the new
clapcontract.Line [106] constructs
GitPreCommitdirectly. The tests do not parsemise generate git-pre-commit -- -C subdir -E ci. A regression in#[clap(last = true, ...)]could reject this command while the current tests still pass. Add a test through the actual CLI parser and verify the parsedmise_argsvalues. Clap defineslastas a positional argument accepted after--. (docs.rs)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cli/generate/git_pre_commit.rs` around lines 106 - 111, Add a parser-level test for the CLI command that parses `mise generate git-pre-commit -- -C subdir -E ci`, rather than constructing `GitPreCommit` directly through `generate_with_args`. Assert that parsing succeeds and produces `mise_args` containing `-C`, `subdir`, `-E`, and `ci`, covering the `clap` positional `last` behavior after `--`.
154-160: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert shell-argument semantics, not quote spelling.
Line [158] hardcodes single quotes, but
shell_words::joinonly guarantees that output is shell-safe, not that it uses one quoting style. Useshell_words::spliton the generatedexeccommand and compare the parsed argument list so tests cover argument boundaries instead of a specific quote character.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cli/generate/git_pre_commit.rs` around lines 154 - 160, Update the mise_args_are_shell_quoted test to validate shell argument boundaries rather than requiring single-quote syntax. Parse the generated exec command with shell_words::split and compare the resulting arguments, preserving the expected “my dir” argument as one element and the existing "$@" behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/cli/generate/git_pre_commit.rs`:
- Around line 135-141: Update no_mise_args_leaves_the_command_unchanged to
assert exact byte-for-byte equality between out and the frozen pre-change hook
fixture, rather than checking that the command merely appears via contains.
Preserve the existing generate("pre-commit", "pre-commit") invocation and use
the fixture’s complete expected output so extra or duplicate content fails the
test.
- Around line 106-111: Add a parser-level test for the CLI command that parses
`mise generate git-pre-commit -- -C subdir -E ci`, rather than constructing
`GitPreCommit` directly through `generate_with_args`. Assert that parsing
succeeds and produces `mise_args` containing `-C`, `subdir`, `-E`, and `ci`,
covering the `clap` positional `last` behavior after `--`.
- Around line 154-160: Update the mise_args_are_shell_quoted test to validate
shell argument boundaries rather than requiring single-quote syntax. Parse the
generated exec command with shell_words::split and compare the resulting
arguments, preserving the expected “my dir” argument as one element and the
existing "$@" behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 0f68bd09-469d-408d-a81d-d4994879acd9
📒 Files selected for processing (1)
src/cli/generate/git_pre_commit.rs
080985d to
b819a15
Compare
|
Two of the three were worth acting on; the parser one had already landed. Byte identity for the no-argument case — fixed. The comment claimed byte-identical output while the assertion only did a Quote spelling — fixed, and your framing was the right one. assert_eq!(words, ["exec", "mise", "-C", "my dir", "run", "lint", "$@"]);That checks the property the feature actually needs — Parser-level test — already added before this review landed, in response to the same point from Greptile. |
A generated hook runs `mise run <task>` from the repository root, because that is where git invokes hooks. A config kept in a subdirectory is therefore never found, and the only fix was editing the hook by hand. Everything after `--` is now inserted between `mise` and `run`, so `-- -C subdir` gets the hook there. Passing flags through rather than adding a `--cd` of its own keeps every other global flag — `-E`, `--locked`, `-j` — reachable by the same mechanism instead of one option per flag. Arguments are joined with shell_words so one containing a space stays a single word. With none given the command is byte-identical to before.
b819a15 to
9195acc
Compare
Closes #4304.
The problem
git runs hooks from the repository root, so a generated hook's
mise run <task>never finds a config kept in a subdirectory. The reporter's only option was editing the hook by hand.-Clooks like it should help, and it is accepted — but it applies to thegenerateinvocation, not to the hook it writes. Measured on v2026.8.2:GitPreCommitheld onlytask,writeandhook, and the script was a fixed template, so there was nothing to embed with.Why a passthrough rather than a
--cdof its ownAdding a dedicated
--cdwould fix the reported case and leave every neighbouring flag stuck.-Cis one of a dozen global flags —-E/--env,-j/--jobs,--locked,--raw,-y/--yesand so on — and a hook can plausibly want any of them. One option per flag does not scale, and picking-Calone would be arbitrary.So everything after
--is inserted betweenmiseandrun:last = truemeans those tokens are positional, so-Cis not swallowed by the global parser on the way in. That is the same shapeCli::task_args_lastalready uses.Properties
shell_words, so-C "my dir"survives as one word.generate task-stubsalready usesshell_words::quotefor the same reason, so this follows an existing pattern rather than introducing one.run, which is where mise expects global flags.Scope
{task}is still interpolated unquoted, as it was. Running it throughshell_words::quotewould be a small robustness win but produces identical output for ordinary task names, so it is left for a separate change rather than mixed in here.Tests
Three unit tests alongside the existing two:
exec mise run <task> "$@"untouched["-C", "subdir", "-E", "ci"]producesexec mise -C subdir -E ci run lint "$@"["-C", "my dir"]produces-C 'my dir'rather than a bare spaceNo e2e: the generated text is fully determined by these, and firing a real hook would depend on git state for no extra coverage.
Summary by CodeRabbit
New Features
misearguments.-C.Documentation