From ae41772608150df383ff658ddeadbbc07d0e2335 Mon Sep 17 00:00:00 2001 From: Amey Pawar Date: Thu, 6 Aug 2026 20:46:13 +0530 Subject: [PATCH 1/5] Restore commit hooks in the CLI `but commit` ran the commit hooks until 45a8387f40 replaced it with commit2, which carried none of them across. A repository whose quality gates live in `pre-commit` has had them silently skipped since, and the CLI is the surface the bundled agent skill directs agents to. Restores `pre-commit` and `post-commit` along with `-n`/`--no-hooks`, keeping the flag declaration and failure wording of the code commit2 replaced. `commit-msg` needs the message resolved before the commit is created, which commit2 inverted, so it is left for its own change. Separately, `join_output` tested `stderr.is_ascii()` where it meant `is_empty()`, discarding every ASCII-only hook message and reporting "hook produced no output" instead. --- crates/but/skill/references/reference.md | 6 ++ crates/but/src/args/commit.rs | 4 + crates/but/src/command/legacy/commit.rs | 93 ++++++++++++++++++- .../legacy/status/tui/app/commit_mode.rs | 1 + .../src/command/legacy/status/tui/app/mod.rs | 6 ++ crates/but/tests/but/command/commit.rs | 61 ++++++++++++ crates/gitbutler-repo/src/hooks.rs | 41 +++++++- 7 files changed, 209 insertions(+), 3 deletions(-) diff --git a/crates/but/skill/references/reference.md b/crates/but/skill/references/reference.md index 76c1a734a67..7045ffe71fe 100644 --- a/crates/but/skill/references/reference.md +++ b/crates/but/skill/references/reference.md @@ -165,6 +165,7 @@ but commit --above -m "message" # Place the commit above a commit but commit --below -m "message" # Place the commit below a commit or branch but commit -b --no-message # Commit without a message but commit --empty -b -m "message" # Insert an empty commit +but commit -b -m "message" --no-hooks # Commit without running commit hooks ``` **Where the commit goes:** `-b`/`--branch`, `-A`/`--above`, and `-B`/`--below` are mutually exclusive. @@ -175,6 +176,11 @@ but commit --empty -b -m "message" # Insert an empty commit **Important:** `but commit -b -m "msg"` with no IDs commits ALL uncommitted changes. Pass IDs to commit only specific files or hunks. +**Hooks:** the repository's `pre-commit` hook runs before the commit is written, and `post-commit` +after it. A failing `pre-commit` aborts the commit and its output is shown; `--no-hooks` (alias +`--no-verify`) skips both, matching `but push`. A failing `post-commit` is reported but does not +undo the commit. + `but commit` is not supported from linked worktrees. Use Git directly for the worktree-local commit, and do not run `but setup` there. **Committing specific files or hunks:** Start with `but diff` for selective dirty commits, then pass CLI IDs as positional arguments: diff --git a/crates/but/src/args/commit.rs b/crates/but/src/args/commit.rs index 90434893c76..5b7a6a1213d 100644 --- a/crates/but/src/args/commit.rs +++ b/crates/but/src/args/commit.rs @@ -83,6 +83,10 @@ pub struct Platform { #[clap(short, long, group = "changes_to_commit")] pub interactive: bool, + /// Bypass commit hooks + #[clap(short = 'n', long = "no-hooks", visible_alias = "no-verify")] + pub no_hooks: bool, + /// One or more changes to commit. /// /// A change can either be a file or a hunk. diff --git a/crates/but/src/command/legacy/commit.rs b/crates/but/src/command/legacy/commit.rs index f1c979e1d96..f9aa3852fa4 100644 --- a/crates/but/src/command/legacy/commit.rs +++ b/crates/but/src/command/legacy/commit.rs @@ -10,6 +10,7 @@ use but_rebase::graph_rebase::mutate::{InsertSide, RelativeTo}; use but_transaction::{IntermediateCommitCreateResult, Transaction}; use but_workspace::{RefInfo, branch::create_reference::Anchor, commit::ChangeSource}; use gitbutler_oplog::entry::{OperationKind, SnapshotDetails}; +use gitbutler_repo::hooks::{ErrorData, HookResult}; use gix::refs::FullName; use nonempty::NonEmpty; use serde::Serialize; @@ -119,7 +120,7 @@ pub fn commit( let mut meta = ctx.meta()?; let id_map = IdMap::new_from_context(ctx, guard.read_permission())?; - let (mut guard, commit_op, commit_selection, reword_op) = { + let (mut guard, commit_op, commit_selection, reword_op, run_hooks) = { let head_info = but_api::legacy::workspace::head_info(ctx)?; resolve(guard, ctx, args, &mut out, &head_info, &id_map)? }; @@ -130,6 +131,7 @@ pub fn commit( commit_op, commit_selection, reword_op, + run_hooks, )?) } @@ -145,6 +147,7 @@ fn resolve( CommitOperation, CommitSelection, CommitMessageSource, + RunHooks, )> { let Platform { no_message, @@ -154,6 +157,7 @@ fn resolve( above, below, interactive, + no_hooks, changes, allow_merged, } = args; @@ -236,7 +240,13 @@ fn resolve( let reword_op = CommitMessageSource::from_args(no_message, message)?; - Ok((guard, commit_op, commit_selection, reword_op)) + Ok(( + guard, + commit_op, + commit_selection, + reword_op, + RunHooks::from_flag(no_hooks), + )) } /// The retired syntax put the target branch in positional position @@ -268,6 +278,7 @@ pub fn run( commit_op: CommitOperation, commit_selection: CommitSelection, reword_op: CommitMessageSource, + run_hooks: RunHooks, ) -> anyhow::Result { let changes = { let context_lines = ctx.settings.context_lines; @@ -290,6 +301,10 @@ pub fn run( builder.into_diff_specs() }; + if run_hooks.is_yes() { + run_pre_commit_hook(ctx, perm, &changes)?; + } + let rejection_target = commit_op.rejection_target(); let snapshot_details = SnapshotDetails::new(OperationKind::CreateCommit); let ((new_commit, branch_name), _ws) = but_transaction::with_transaction_with_perm( @@ -321,12 +336,86 @@ pub fn run( ) .map_err(|err| rejection::explain_after_rollback(ctx, perm, "commit", rejection_target, err))?; + if run_hooks.is_yes() { + run_post_commit_hook(ctx); + } + Ok(CommitOutcome { new_commit, branch_name, }) } +/// Whether the commit hooks should run, as decided by `--no-hooks`. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum RunHooks { + Yes, + No, +} + +impl RunHooks { + fn from_flag(no_hooks: bool) -> Self { + if no_hooks { Self::No } else { Self::Yes } + } + + fn is_yes(self) -> bool { + self == Self::Yes + } +} + +/// Run `pre-commit` against the tree the commit is about to have, and fail the commit if the hook +/// does. +/// +/// The tree is built the same way the desktop builds it for this hook, so both surfaces show the +/// hook the same thing: `HEAD^{tree}` with the changes being committed applied on top. +fn run_pre_commit_hook( + ctx: &mut Context, + perm: &mut RepoExclusive, + changes: &[DiffSpec], +) -> anyhow::Result<()> { + let context_lines = ctx.settings.context_lines; + let tree = { + let (repo, ..) = ctx.workspace_and_db_mut_with_perm(perm.read_permission())?; + let head = repo + .head_tree_id_or_empty() + .context("Failed to get head tree")?; + let mut changes = changes.iter().cloned().map(Ok).collect::>(); + let (tree, ..) = but_core::tree::apply_worktree_changes( + head.detach(), + &repo, + &mut changes, + context_lines, + )?; + tree.detach() + }; + + match gitbutler_repo::hooks::pre_commit_with_tree(ctx, tree)? { + HookResult::Success | HookResult::NotConfigured => Ok(()), + HookResult::Failure(ErrorData { error }) => Err(hook_failed("pre-commit", error)), + } +} + +/// Run `post-commit`, which cannot fail the commit that already exists. +fn run_post_commit_hook(ctx: &Context) { + let outcome = gitbutler_repo::hooks::post_commit(ctx); + let error = match outcome { + Ok(HookResult::Success | HookResult::NotConfigured) => return, + Ok(HookResult::Failure(ErrorData { error })) => error, + Err(err) => format!("{err:#}"), + }; + tracing::warn!("post-commit hook failed: {error}"); +} + +fn hook_failed(name: &str, error: String) -> anyhow::Error { + // Wording kept from before the `commit2` rewrite dropped hook support, so anyone who saw + // this message on an older build sees the same one again. + // Hook output usually ends in a newline of its own; trimming keeps one blank line here. + anyhow::anyhow!( + "{name} hook failed:\n{}\n\nTo bypass the hook, run: but commit --no-hooks", + error.trim_end() + ) +} + /// Targeting modes for committing. pub enum CommitOperationTargetIsh { /// Target the branch if it exists, or create it at the newest base if it does not. diff --git a/crates/but/src/command/legacy/status/tui/app/commit_mode.rs b/crates/but/src/command/legacy/status/tui/app/commit_mode.rs index 58de9a61088..189084104db 100644 --- a/crates/but/src/command/legacy/status/tui/app/commit_mode.rs +++ b/crates/but/src/command/legacy/status/tui/app/commit_mode.rs @@ -507,6 +507,7 @@ where commit_op, commit_selection, reword_op, + commit::RunHooks::Yes, )?; drop(_suspend_guard); diff --git a/crates/but/src/command/legacy/status/tui/app/mod.rs b/crates/but/src/command/legacy/status/tui/app/mod.rs index 01e3349643a..0c682c5026e 100644 --- a/crates/but/src/command/legacy/status/tui/app/mod.rs +++ b/crates/but/src/command/legacy/status/tui/app/mod.rs @@ -1440,6 +1440,9 @@ impl App { }), CommitSelection::Nothing, CommitMessageSource::Empty, + // A blank commit carries no changes, so there is nothing for a content + // gate to inspect and nothing it could sensibly refuse. + commit::RunHooks::No, )?; messages.push(Message::Reload( @@ -1467,6 +1470,9 @@ impl App { }), CommitSelection::Nothing, CommitMessageSource::Empty, + // A blank commit carries no changes, so there is nothing for a content + // gate to inspect and nothing it could sensibly refuse. + commit::RunHooks::No, )?; messages.push(Message::Reload( diff --git a/crates/but/tests/but/command/commit.rs b/crates/but/tests/but/command/commit.rs index 2ba506d7acf..470131a9bf5 100644 --- a/crates/but/tests/but/command/commit.rs +++ b/crates/but/tests/but/command/commit.rs @@ -2340,3 +2340,64 @@ For more information, try '--help'. "#]]); } + +/// Writes an executable `pre-commit` hook that runs `body`. +#[cfg(unix)] +fn write_pre_commit_hook(env: &Sandbox, body: &str) { + env.invoke_bash(format!( + "mkdir -p .git/hooks && printf '#!/bin/sh\\n{body}\\n' > .git/hooks/pre-commit && chmod +x .git/hooks/pre-commit" + )); +} + +#[cfg(unix)] +#[test] +fn pre_commit_hook_runs_for_a_commit() { + let env = Sandbox::init_scenario_with_target_and_default_settings("one-stack"); + env.setup_metadata(&["A"]); + write_pre_commit_hook(&env, "echo ran > hook-ran.txt"); + env.file("file.txt", "Some text"); + + env.but("commit --no-message").assert().success(); + + assert_eq!( + env.read_file("hook-ran.txt").expect("the hook wrote it"), + "ran\n", + "the pre-commit hook runs when committing through the CLI" + ); +} + +#[cfg(unix)] +#[test] +fn a_failing_pre_commit_hook_blocks_the_commit_and_says_why() { + let env = Sandbox::init_scenario_with_target_and_default_settings("one-stack"); + env.setup_metadata(&["A"]); + write_pre_commit_hook(&env, "echo \"lint found 3 problems\" >&2\\nexit 1"); + env.file("file.txt", "Some text"); + + env.but("commit --no-message") + .assert() + .failure() + .stderr_eq(snapbox::str![[r#" +Error: pre-commit hook failed: +lint found 3 problems + +To bypass the hook, run: but commit --no-hooks + +"#]]); +} + +#[cfg(unix)] +#[test] +fn no_hooks_commits_past_a_failing_pre_commit_hook() { + let env = Sandbox::init_scenario_with_target_and_default_settings("one-stack"); + env.setup_metadata(&["A"]); + write_pre_commit_hook(&env, "echo ran > hook-ran.txt\\nexit 1"); + env.file("file.txt", "Some text"); + + env.but("commit --no-hooks --no-message").assert().success(); + + assert!( + !env.projects_root().join("hook-ran.txt").exists(), + "--no-hooks skips the hook rather than running it and ignoring its verdict" + ); +} diff --git a/crates/gitbutler-repo/src/hooks.rs b/crates/gitbutler-repo/src/hooks.rs index 80df23f178d..8acdcad4e60 100644 --- a/crates/gitbutler-repo/src/hooks.rs +++ b/crates/gitbutler-repo/src/hooks.rs @@ -330,7 +330,7 @@ fn join_output(stdout: String, stderr: String, code: Option) -> String { let code = code .map(|code| format!(" (Exit Code {code})")) .unwrap_or_default(); - if stdout.is_empty() && stderr.is_ascii() { + if stdout.is_empty() && stderr.is_empty() { return format!("hook produced no output{code}"); } else if stdout.is_empty() { return stderr; @@ -339,3 +339,42 @@ fn join_output(stdout: String, stderr: String, code: Option) -> String { } format!("stdout:\n{stdout}\n\nstderr:\n{stderr}{code}") } + +#[cfg(test)] +mod tests { + use super::join_output; + + /// A hook that explains its refusal on stderr says nothing otherwise, so the explanation is + /// the whole of what the user has to go on. + #[test] + fn stderr_only_output_is_reported() { + assert_eq!( + join_output(String::new(), "gate says no\n".into(), Some(1)), + "gate says no\n" + ); + } + + #[test] + fn stdout_only_output_is_reported() { + assert_eq!( + join_output("formatted 3 files\n".into(), String::new(), Some(1)), + "formatted 3 files\n" + ); + } + + #[test] + fn both_streams_are_labelled() { + assert_eq!( + join_output("out".into(), "err".into(), Some(2)), + "stdout:\nout\n\nstderr:\nerr (Exit Code 2)" + ); + } + + #[test] + fn a_silent_hook_says_so_with_its_exit_code() { + assert_eq!( + join_output(String::new(), String::new(), Some(1)), + "hook produced no output (Exit Code 1)" + ); + } +} From 6dc17537c5dd97c01ed7a7b54963ff501f080a5f Mon Sep 17 00:00:00 2001 From: Amey Pawar Date: Thu, 6 Aug 2026 21:28:05 +0530 Subject: [PATCH 2/5] Keep a hook's own edits out of the commit it invalidates Every diff spec carries hunk headers, even when no ids are given, and `DiffSpec` drops headers that no longer match the worktree without saying so. A `pre-commit` hook that reformats a file therefore moved the headers computed before it ran, and the commit silently carried less than was asked for. With nothing singled out, the specs are rebuilt from the worktree as the hook left it, which is the result `git` gives a hook that stages its own edits. When files were singled out, the commit is refused rather than guessed at, and the worktree is left untouched. --- crates/but/src/command/legacy/commit.rs | 136 +++++++++++++++++++----- crates/but/tests/but/command/commit.rs | 60 +++++++++++ 2 files changed, 169 insertions(+), 27 deletions(-) diff --git a/crates/but/src/command/legacy/commit.rs b/crates/but/src/command/legacy/commit.rs index f9aa3852fa4..9fccbb4aa32 100644 --- a/crates/but/src/command/legacy/commit.rs +++ b/crates/but/src/command/legacy/commit.rs @@ -11,9 +11,11 @@ use but_transaction::{IntermediateCommitCreateResult, Transaction}; use but_workspace::{RefInfo, branch::create_reference::Anchor, commit::ChangeSource}; use gitbutler_oplog::entry::{OperationKind, SnapshotDetails}; use gitbutler_repo::hooks::{ErrorData, HookResult}; +use gix::bstr::{BString, ByteSlice}; use gix::refs::FullName; use nonempty::NonEmpty; use serde::Serialize; +use std::path::{Path, PathBuf}; use crate::{ CliError, CliId, CliResult, CliResultExt, IdMap, @@ -280,29 +282,23 @@ pub fn run( reword_op: CommitMessageSource, run_hooks: RunHooks, ) -> anyhow::Result { - let changes = { - let context_lines = ctx.settings.context_lines; - let (repo, ..) = ctx.workspace_and_db_mut_with_perm(perm.read_permission())?; - let mut builder = DiffSpecBuilder::new(&repo, context_lines); + let takes_whole_worktree = matches!(commit_selection, CommitSelection::AllChanges); + let (mut changes, _) = build_diff_specs(ctx, perm, commit_selection)?; - match commit_selection { - CommitSelection::AllChanges => { - builder.push_changes_from_uncommitted_area()?; - } - CommitSelection::Changes(changes) => { - for change in *changes { - builder.push_changes_from_uncommitted(&change)?; - } - - builder.reconcile_worktree_diff_specs()?; + if run_hooks.is_yes() { + let touched = run_pre_commit_hook(ctx, perm, &changes)?; + if !touched.is_empty() { + // The hook rewrote files this commit is made of. Every spec carries hunk headers even + // when no ids were given, and `DiffSpec` drops headers that no longer match without + // saying so, which would quietly commit less than was asked for. + if takes_whole_worktree { + // Nothing was singled out, so what the hook left behind is what to commit - the + // same result `git` gives for a hook that stages its own edits. + changes = build_diff_specs(ctx, perm, CommitSelection::AllChanges)?.0; + } else { + return Err(hook_changed_selected_files(&touched)); } - CommitSelection::Nothing => {} } - - builder.into_diff_specs() - }; - if run_hooks.is_yes() { - run_pre_commit_hook(ctx, perm, &changes)?; } let rejection_target = commit_op.rejection_target(); @@ -372,29 +368,115 @@ fn run_pre_commit_hook( ctx: &mut Context, perm: &mut RepoExclusive, changes: &[DiffSpec], -) -> anyhow::Result<()> { +) -> anyhow::Result> { let context_lines = ctx.settings.context_lines; - let tree = { + let (tree, selections) = { let (repo, ..) = ctx.workspace_and_db_mut_with_perm(perm.read_permission())?; let head = repo .head_tree_id_or_empty() .context("Failed to get head tree")?; - let mut changes = changes.iter().cloned().map(Ok).collect::>(); + let mut specs = changes.iter().cloned().map(Ok).collect::>(); let (tree, ..) = but_core::tree::apply_worktree_changes( head.detach(), &repo, - &mut changes, + &mut specs, context_lines, )?; - tree.detach() + let workdir = repo.workdir().context("non-bare repository")?.to_owned(); + (tree.detach(), HunkSelections::of(changes, &workdir)) }; match gitbutler_repo::hooks::pre_commit_with_tree(ctx, tree)? { - HookResult::Success | HookResult::NotConfigured => Ok(()), - HookResult::Failure(ErrorData { error }) => Err(hook_failed("pre-commit", error)), + HookResult::Success | HookResult::NotConfigured => {} + HookResult::Failure(ErrorData { error }) => return Err(hook_failed("pre-commit", error)), + } + + Ok(selections.changed_since()) +} + +/// The files a commit takes only part of, remembered as they were before a hook ran. +/// +/// A whole-file change survives a hook that rewrites it, because the commit re-reads the file +/// once the hook is done. Chosen hunks do not: they were matched against the file as it looked +/// when the command started, and a hook that reformats it leaves some of them matching nothing. +/// Those are dropped without a word, so a commit meant to carry two hunks quietly carries one. +struct HunkSelections { + files: Vec<(BString, PathBuf, Option>)>, +} + +impl HunkSelections { + fn of(changes: &[DiffSpec], workdir: &Path) -> Self { + let mut seen = std::collections::BTreeSet::new(); + let files = changes + .iter() + .filter(|spec| seen.insert(spec.path.clone())) + .map(|spec| { + let path = workdir.join(gix::path::from_bstr(spec.path.as_bstr())); + let before = std::fs::read(&path).ok(); + (spec.path.clone(), path, before) + }) + .collect(); + Self { files } + } + + fn changed_since(self) -> Vec { + self.files + .into_iter() + .filter(|(_, path, before)| std::fs::read(path).ok().as_ref() != before.as_ref()) + .map(|(rela_path, ..)| rela_path.to_string()) + .collect() } } +/// Build the changes to commit, and the paths that were singled out to make them. +/// +/// `selected_paths` is empty when nothing was singled out, which is what tells a hook's own edits +/// apart from edits that invalidate a selection the user made. +fn build_diff_specs( + ctx: &mut Context, + perm: &mut RepoExclusive, + commit_selection: CommitSelection, +) -> anyhow::Result<(Vec, Vec)> { + let context_lines = ctx.settings.context_lines; + let (repo, ..) = ctx.workspace_and_db_mut_with_perm(perm.read_permission())?; + let mut builder = DiffSpecBuilder::new(&repo, context_lines); + let mut selected_paths = Vec::new(); + + match commit_selection { + CommitSelection::AllChanges => { + builder.push_changes_from_uncommitted_area()?; + } + CommitSelection::Changes(changes) => { + for change in *changes { + selected_paths.extend(change.hunks.iter().map(|hunk| hunk.hunk.path.clone())); + builder.push_changes_from_uncommitted(&change)?; + } + + builder.reconcile_worktree_diff_specs()?; + } + CommitSelection::Nothing => {} + } + + Ok((builder.into_diff_specs(), selected_paths)) +} + +/// Refuse a commit whose singled-out files a hook rewrote underneath it. +fn hook_changed_selected_files(files: &[String]) -> anyhow::Error { + let first = files.first().map(String::as_str).unwrap_or_default(); + let files = if files.len() == 1 { + first.to_owned() + } else { + format!("{first} and {} more", files.len() - 1) + }; + anyhow::anyhow!( + "the pre-commit hook changed {files}, which this commit was told to take part of.\n\ + \n\ + Nothing was committed and the hook's changes are still in the worktree. Re-run \ + `but commit` to choose from the files as they are now, or commit without ids to take \ + the worktree as the hook left it." + ) +} + /// Run `post-commit`, which cannot fail the commit that already exists. fn run_post_commit_hook(ctx: &Context) { let outcome = gitbutler_repo::hooks::post_commit(ctx); diff --git a/crates/but/tests/but/command/commit.rs b/crates/but/tests/but/command/commit.rs index 470131a9bf5..fc510b2f6be 100644 --- a/crates/but/tests/but/command/commit.rs +++ b/crates/but/tests/but/command/commit.rs @@ -2401,3 +2401,63 @@ fn no_hooks_commits_past_a_failing_pre_commit_hook() { "--no-hooks skips the hook rather than running it and ignoring its verdict" ); } + +/// A formatter-style hook: inserts a line in the middle of a file that is already tracked, +/// shifting every line below it and so invalidating hunk headers computed before it ran. +#[cfg(unix)] +fn write_line_shifting_hook(env: &Sandbox) { + // `sed -i` is spelled differently on BSD and GNU, so splice the line with head/tail instead. + env.invoke_bash( + "mkdir -p .git/hooks && cat > .git/hooks/pre-commit <<'HOOK'\n\ + #!/bin/sh\n\ + { head -9 many.txt; echo INSERTED; tail -n +10 many.txt; } > many.tmp\n\ + mv many.tmp many.txt\n\ + HOOK\n\ + chmod +x .git/hooks/pre-commit", + ); +} + +/// With nothing singled out, the worktree as the hook left it is what gets committed - the same +/// result `git` gives a hook that stages its own edits. +/// +/// Committing the changes computed before the hook would drop every hunk whose header the hook +/// moved, silently committing less than was asked for, so `status` would still show changes. +#[cfg(unix)] +#[test] +fn a_hook_that_shifts_lines_does_not_strand_the_changes_below_it() { + let env = Sandbox::init_scenario_with_target_and_default_settings("one-stack"); + env.setup_metadata(&["A"]); + + let lines: String = (1..=20).map(|n| format!("l{n:02}\n")).collect(); + env.file("many.txt", &lines); + env.but("commit --no-message").assert().success(); + + // Two changes far enough apart to be separate hunks, with the hook's insertion between them. + env.file( + "many.txt", + lines + .replace("l02\n", "l02-top\n") + .replace("l19\n", "l19-bottom\n"), + ); + write_line_shifting_hook(&env); + + env.but("commit --no-message").assert().success(); + + env.but("status") + .assert() + .success() + .stdout_eq(snapbox::str![[r#" +╭┄ zz [uncommitted] (no changes) +┊ +┊╭┄ g0 [A] +┊● 1#0 (no commit message) +┊● 1#1 (no commit message) +┊● tpm add A +├╯ +┊ +┴ 0dc3733 (common base) 2000-01-02 add M + +Hint: run `but help` for all commands + +"#]]); +} From 3a501f7c10d79c8428e156931bfd13c51906cf52 Mon Sep 17 00:00:00 2001 From: Amey Pawar Date: Thu, 6 Aug 2026 22:03:09 +0530 Subject: [PATCH 3/5] Only guard the hunks a hook can invalidate Watching every path in the commit refused commits that were never at risk: additions and deletions carry no hunk headers, so the file is read whole while the commit is built, and the same is true of symlinks, binaries and files too large to diff. Only specs with hunk headers can go stale, which also keeps the largest files out of the snapshot. Rebuilding from the worktree also reached past what was being committed. A hook is free to write elsewhere, and files it created or merely dirtied were landing in the commit; `git` commits neither. The rebuild is now limited to the paths that were already going in. Adds the missing test for the refusal, which had none. --- crates/but/src/command/legacy/commit.rs | 28 +++++++++------- crates/but/tests/but/command/commit.rs | 44 +++++++++++++++++++++++++ 2 files changed, 60 insertions(+), 12 deletions(-) diff --git a/crates/but/src/command/legacy/commit.rs b/crates/but/src/command/legacy/commit.rs index 9fccbb4aa32..26c2a44a499 100644 --- a/crates/but/src/command/legacy/commit.rs +++ b/crates/but/src/command/legacy/commit.rs @@ -283,18 +283,22 @@ pub fn run( run_hooks: RunHooks, ) -> anyhow::Result { let takes_whole_worktree = matches!(commit_selection, CommitSelection::AllChanges); - let (mut changes, _) = build_diff_specs(ctx, perm, commit_selection)?; + let mut changes = build_diff_specs(ctx, perm, commit_selection)?; if run_hooks.is_yes() { + let committed_paths: std::collections::BTreeSet<_> = + changes.iter().map(|spec| spec.path.clone()).collect(); let touched = run_pre_commit_hook(ctx, perm, &changes)?; if !touched.is_empty() { // The hook rewrote files this commit is made of. Every spec carries hunk headers even // when no ids were given, and `DiffSpec` drops headers that no longer match without // saying so, which would quietly commit less than was asked for. if takes_whole_worktree { - // Nothing was singled out, so what the hook left behind is what to commit - the - // same result `git` gives for a hook that stages its own edits. - changes = build_diff_specs(ctx, perm, CommitSelection::AllChanges)?.0; + // Nothing was singled out, so take these files as the hook left them. Only these + // files: a hook is free to write elsewhere, and committing what it happened to + // touch would put files in the commit that were never going to be in it. + changes = build_diff_specs(ctx, perm, CommitSelection::AllChanges)?; + changes.retain(|spec| committed_paths.contains(&spec.path)); } else { return Err(hook_changed_selected_files(&touched)); } @@ -409,6 +413,11 @@ impl HunkSelections { let mut seen = std::collections::BTreeSet::new(); let files = changes .iter() + // Only hunk headers go stale. A whole-file spec - an addition, a deletion, a binary + // or too-large file - is read from disk while the commit is built, so a hook's edits + // to it are picked up rather than lost, and watching it would refuse commits that + // were never at risk. It also keeps the biggest files out of the snapshot below. + .filter(|spec| !spec.hunk_headers.is_empty()) .filter(|spec| seen.insert(spec.path.clone())) .map(|spec| { let path = workdir.join(gix::path::from_bstr(spec.path.as_bstr())); @@ -428,19 +437,15 @@ impl HunkSelections { } } -/// Build the changes to commit, and the paths that were singled out to make them. -/// -/// `selected_paths` is empty when nothing was singled out, which is what tells a hook's own edits -/// apart from edits that invalidate a selection the user made. +/// Build the changes a commit is made of. fn build_diff_specs( ctx: &mut Context, perm: &mut RepoExclusive, commit_selection: CommitSelection, -) -> anyhow::Result<(Vec, Vec)> { +) -> anyhow::Result> { let context_lines = ctx.settings.context_lines; let (repo, ..) = ctx.workspace_and_db_mut_with_perm(perm.read_permission())?; let mut builder = DiffSpecBuilder::new(&repo, context_lines); - let mut selected_paths = Vec::new(); match commit_selection { CommitSelection::AllChanges => { @@ -448,7 +453,6 @@ fn build_diff_specs( } CommitSelection::Changes(changes) => { for change in *changes { - selected_paths.extend(change.hunks.iter().map(|hunk| hunk.hunk.path.clone())); builder.push_changes_from_uncommitted(&change)?; } @@ -457,7 +461,7 @@ fn build_diff_specs( CommitSelection::Nothing => {} } - Ok((builder.into_diff_specs(), selected_paths)) + Ok(builder.into_diff_specs()) } /// Refuse a commit whose singled-out files a hook rewrote underneath it. diff --git a/crates/but/tests/but/command/commit.rs b/crates/but/tests/but/command/commit.rs index fc510b2f6be..0bcda64ea21 100644 --- a/crates/but/tests/but/command/commit.rs +++ b/crates/but/tests/but/command/commit.rs @@ -2461,3 +2461,47 @@ Hint: run `but help` for all commands "#]]); } + +/// Hunks chosen before a hook runs cannot survive the hook rewriting the file they came from, so +/// the commit is refused rather than quietly carrying whichever of them still matched. +#[cfg(unix)] +#[test] +fn choosing_hunks_a_hook_then_rewrites_refuses_the_commit() { + let env = Sandbox::init_scenario_with_target_and_default_settings("one-stack"); + env.setup_metadata(&["A"]); + + let lines: String = (1..=20).map(|n| format!("l{n:02}\n")).collect(); + env.file("many.txt", &lines); + env.but("commit --no-message").assert().success(); + + env.file( + "many.txt", + lines + .replace("l02\n", "l02-top\n") + .replace("l19\n", "l19-bottom\n"), + ); + write_line_shifting_hook(&env); + + // `rr:e` and `rr:c` are this fixture's two hunks in many.txt, as `but diff` shows them. + env.but("commit --no-message rr:e rr:c") + .assert() + .failure() + .stderr_eq(snapbox::str![[r#" +Error: the pre-commit hook changed many.txt, which this commit was told to take part of. + +Nothing was committed and the hook's changes are still in the worktree. Re-run `but commit` to choose from the files as they are now, or commit without ids to take the worktree as the hook left it. + +"#]]); + + let mut expected: String = (1..=20).map(|n| format!("l{n:02}\n")).collect(); + expected = expected + .replace("l02\n", "l02-top\n") + .replace("l19\n", "l19-bottom\n"); + let after_ninth = expected.match_indices('\n').nth(8).expect("twenty lines").0 + 1; + expected.insert_str(after_ninth, "INSERTED\n"); + assert_eq!( + env.read_file("many.txt").expect("still there"), + expected, + "a refused commit leaves the hook's changes in the worktree rather than undoing them" + ); +} From e71c886798957b6302a7484e131a1229a2f8d2ed Mon Sep 17 00:00:00 2001 From: Amey Pawar Date: Sun, 9 Aug 2026 13:48:33 +0530 Subject: [PATCH 4/5] Pin the hooks path so tests use the hook they wrote libgit2 only honours GIT_CONFIG_NOSYSTEM and GIT_CONFIG_GLOBAL when the repository is opened FROM_ENV, which `but` never does, so the sandbox's config isolation does not reach any hook path. A developer with `core.hooksPath` set globally therefore had the hook these tests write skipped and their own hook run instead, and the tests failed. Pinning `core.hooksPath` per repository keeps the hook under test the one that runs. The wider leak - every libgit2-read config key escaping the sandbox - belongs in but-testsupport rather than here. The failure mode was pointed out by @sensei-woo. --- crates/but/tests/but/command/commit.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/crates/but/tests/but/command/commit.rs b/crates/but/tests/but/command/commit.rs index 0bcda64ea21..b6d572972c7 100644 --- a/crates/but/tests/but/command/commit.rs +++ b/crates/but/tests/but/command/commit.rs @@ -2344,6 +2344,9 @@ For more information, try '--help'. /// Writes an executable `pre-commit` hook that runs `body`. #[cfg(unix)] fn write_pre_commit_hook(env: &Sandbox, body: &str) { + // A global `core.hooksPath` would otherwise send `but` to the developer's own hooks, + // skipping the one written here and running theirs during the test run. + env.invoke_git("config core.hooksPath .git/hooks"); env.invoke_bash(format!( "mkdir -p .git/hooks && printf '#!/bin/sh\\n{body}\\n' > .git/hooks/pre-commit && chmod +x .git/hooks/pre-commit" )); @@ -2406,6 +2409,9 @@ fn no_hooks_commits_past_a_failing_pre_commit_hook() { /// shifting every line below it and so invalidating hunk headers computed before it ran. #[cfg(unix)] fn write_line_shifting_hook(env: &Sandbox) { + // A global `core.hooksPath` would otherwise send `but` to the developer's own hooks, + // skipping the one written here and running theirs during the test run. + env.invoke_git("config core.hooksPath .git/hooks"); // `sed -i` is spelled differently on BSD and GNU, so splice the line with head/tail instead. env.invoke_bash( "mkdir -p .git/hooks && cat > .git/hooks/pre-commit <<'HOOK'\n\ From 37321067e6d2f1858a5f52694ebf77a42ace73b2 Mon Sep 17 00:00:00 2001 From: Amey Pawar Date: Sun, 9 Aug 2026 14:07:32 +0530 Subject: [PATCH 5/5] Translate retired `--no-hooks` now that it has an equivalent The retired grammar spells the flag exactly as the modern one does, but it was listed among the forms with no modern equivalent, so a command carrying it was refused rather than translated. `but commit` runs hooks again, so it carries over as it stands. The unit test's refusal list and the CLI test both used this flag as their example of something untranslatable; they now use `--message-file`, which still is. --- crates/but/src/lib.rs | 2 +- crates/but/src/retired_syntax.rs | 22 +++++++++++++++++++--- crates/but/tests/but/command/commit.rs | 4 ++-- 3 files changed, 22 insertions(+), 6 deletions(-) diff --git a/crates/but/src/lib.rs b/crates/but/src/lib.rs index abc7865b426..fc364fb0f93 100644 --- a/crates/but/src/lib.rs +++ b/crates/but/src/lib.rs @@ -133,7 +133,7 @@ fn parse_args_and_output_format(args: Vec, agent_detected: bool) -> (A matches } // Retired syntax we cannot safely translate (e.g. - // `--no-hooks`): teach the new form, then report the + // `--message-file`): teach the new form, then report the // original error. None => { print_err_infallible(retired_syntax::hint(agent_detected)); diff --git a/crates/but/src/retired_syntax.rs b/crates/but/src/retired_syntax.rs index ce0fc909bb0..00ad392f06b 100644 --- a/crates/but/src/retired_syntax.rs +++ b/crates/but/src/retired_syntax.rs @@ -193,7 +193,6 @@ pub(crate) fn translate_commit(args: &[OsString]) -> Translation { let unsupported = message_file.is_some() || before.is_some() || after.is_some() - || no_hooks || ai.is_some() || diff || no_diff; @@ -225,6 +224,10 @@ pub(crate) fn translate_commit(args: &[OsString]) -> Translation { if status_after { translated.push("--status-after".into()); } + if no_hooks { + // Spelled the same in both grammars, so it carries over as it stands. + translated.push("--no-hooks".into()); + } if let Some(branch) = &branch { // The `=`-attached form binds unambiguously to the optional-value flag. translated.push(format!("--branch={branch}").into()); @@ -715,8 +718,7 @@ mod tests { #[test] fn retired_flags_without_modern_equivalent_are_refused() { for retired in [ - &["commit", "my-branch", "-c", "--no-hooks", "-m", "msg"][..], - &["commit", "my-branch", "-c", "-m", "msg", "--before", "ab"], + &["commit", "my-branch", "-c", "-m", "msg", "--before", "ab"][..], &["commit", "my-branch", "--changes", "ab", "--ai"], &["commit", "my-branch", "--changes", "ab", "-i=prompt"], &["commit", "my-branch", "-c", "--diff"], @@ -725,6 +727,20 @@ mod tests { } } + #[test] + fn retired_no_hooks_carries_over() { + // Spelled the same in both grammars since `but commit` runs hooks again, so the + // retired form translates rather than being refused. + let translated = translate(&["commit", "b", "-c", "-m", "msg", "--no-hooks"]); + let Translation::Translated(args) = translated else { + panic!("expected a translation, got {translated:?}"); + }; + assert!( + args.iter().any(|arg| arg == "--no-hooks"), + "the flag reaches the modern command line: {args:?}" + ); + } + #[test] fn hyphen_leading_message_still_binds() { // The retired parser accepted `--message=-hello`; the attached form diff --git a/crates/but/tests/but/command/commit.rs b/crates/but/tests/but/command/commit.rs index b6d572972c7..39973b064c0 100644 --- a/crates/but/tests/but/command/commit.rs +++ b/crates/but/tests/but/command/commit.rs @@ -2318,9 +2318,9 @@ fn retired_syntax_without_modern_equivalent_hints_and_fails() { env.file("one", "one content"); - // `--no-hooks` has no modern equivalent, so the command still fails with + // `--message-file` has no modern equivalent, so the command still fails with // the original error — but the hint teaches the new syntax first. - env.but("commit my-branch -c -m 'add one' --no-hooks --changes one") + env.but("commit my-branch -c -m 'add one' --message-file msg.txt --changes one") .assert() .failure() .stderr_eq(snapbox::str![[r#"