From 46593e16fe4b368f4118bc32aff9e35cb5d3da52 Mon Sep 17 00:00:00 2001 From: sensei-woo <168141084+sensei-woo@users.noreply.github.com> Date: Sat, 8 Aug 2026 23:49:53 -0400 Subject: [PATCH] Restore commit-msg hook in the CLI Resolve the final message before running commit-msg while the workspace transaction is still open. Hook edits are reworded into the same transaction, and failures roll back the commit. Tests cover provided and editor messages, rejection, and --no-hooks. --- crates/but/src/command/legacy/commit.rs | 35 ++++++- crates/but/tests/but/command/commit.rs | 117 ++++++++++++++++++++++++ crates/gitbutler-repo/src/hooks.rs | 43 +++++++-- 3 files changed, 187 insertions(+), 8 deletions(-) diff --git a/crates/but/src/command/legacy/commit.rs b/crates/but/src/command/legacy/commit.rs index 26c2a44a499..ce99ac2e13c 100644 --- a/crates/but/src/command/legacy/commit.rs +++ b/crates/but/src/command/legacy/commit.rs @@ -10,7 +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 gitbutler_repo::hooks::{CommitMsgHook, ErrorData, HookResult, MessageHookResult}; use gix::bstr::{BString, ByteSlice}; use gix::refs::FullName; use nonempty::NonEmpty; @@ -306,6 +306,10 @@ pub fn run( } let rejection_target = commit_op.rejection_target(); + let commit_msg_hook = run_hooks + .is_yes() + .then(|| CommitMsgHook::from_context(ctx)) + .transpose()?; let snapshot_details = SnapshotDetails::new(OperationKind::CreateCommit); let ((new_commit, branch_name), _ws) = but_transaction::with_transaction_with_perm( ctx, @@ -330,6 +334,10 @@ pub fn run( new_commit.context("BUG: rejected_specs is empty yet nothing was committed")?; let reworded_commit = reword_op.execute(new_commit.into(), &mut tx)?; + let reworded_commit = match commit_msg_hook.as_ref() { + Some(hook) => run_commit_msg_hook(hook, reworded_commit, &mut tx)?, + None => reworded_commit, + }; Ok(but_transaction::Commit((reworded_commit, branch_name))) }, @@ -492,6 +500,31 @@ fn run_post_commit_hook(ctx: &Context) { tracing::warn!("post-commit hook failed: {error}"); } +/// Run `commit-msg` after the message source has been resolved but before the transaction is +/// committed. A rejecting hook therefore rolls the entire commit operation back, while a hook +/// that edits its message gets one final reword inside the same transaction. +fn run_commit_msg_hook( + hook: &CommitMsgHook, + commit: CommitId, + tx: &mut Transaction<'_, '_, impl RefMetadata>, +) -> anyhow::Result { + let message = tx + .repo() + .find_commit(commit.commit_id)? + .message_raw()? + .to_string(); + match hook.run(message)? { + MessageHookResult::Success | MessageHookResult::NotConfigured => Ok(commit), + MessageHookResult::Message(message_data) => Ok(tx + .reword_commit( + commit.commit_id, + BString::from(message_data.message).as_ref(), + )? + .into()), + MessageHookResult::Failure(ErrorData { error }) => Err(hook_failed("commit-msg", 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. diff --git a/crates/but/tests/but/command/commit.rs b/crates/but/tests/but/command/commit.rs index 0bcda64ea21..a118881b323 100644 --- a/crates/but/tests/but/command/commit.rs +++ b/crates/but/tests/but/command/commit.rs @@ -2344,6 +2344,7 @@ 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_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" )); @@ -2402,11 +2403,127 @@ fn no_hooks_commits_past_a_failing_pre_commit_hook() { ); } +/// Writes an executable `commit-msg` hook that runs `body`. +#[cfg(unix)] +fn write_commit_msg_hook(env: &Sandbox, body: &str) { + env.invoke_git("config core.hooksPath .git/hooks"); + env.invoke_bash(format!( + "mkdir -p .git/hooks && cat > .git/hooks/commit-msg <<'HOOK'\n#!/bin/sh\n{body}\nHOOK\nchmod +x .git/hooks/commit-msg" + )); +} + +#[cfg(unix)] +#[test] +fn commit_msg_hook_can_update_the_message() { + let env = Sandbox::init_scenario_with_target_and_default_settings("one-stack"); + env.setup_metadata(&["A"]); + write_commit_msg_hook( + &env, + "printf '\\n\\nGenerated-by: commit-msg hook\\n' >> \"$1\"", + ); + env.file("file.txt", "Some text"); + + env.but("commit -m 'add file.txt'").assert().success(); + + snapbox::assert_data_eq!( + env.invoke_git("show -s --format=%B refs/heads/A"), + snapbox::str![[r#" +add file.txt + +Generated-by: commit-msg hook +"#]] + ); +} + +#[cfg(unix)] +#[test] +fn commit_msg_hook_updates_a_message_from_the_editor() { + let env = Sandbox::init_scenario_with_target_and_default_settings("one-stack"); + env.setup_metadata(&["A"]); + write_commit_msg_hook( + &env, + "printf '\\n\\nGenerated-by: commit-msg hook\\n' >> \"$1\"", + ); + env.file("editor.sh", "printf 'message from editor\\n' > \"$1\"\n"); + let editor_command = format!("sh {}", env.projects_root().join("editor.sh").display()); + env.file("file.txt", "Some text"); + + env.but("commit") + .env("GIT_EDITOR", editor_command) + .assert() + .success(); + + snapbox::assert_data_eq!( + env.invoke_git("show -s --format=%B refs/heads/A"), + snapbox::str![[r#" +message from editor + +Generated-by: commit-msg hook +"#]] + ); +} + +#[cfg(unix)] +#[test] +fn a_failing_commit_msg_hook_rolls_back_the_commit() { + let env = Sandbox::init_scenario_with_target_and_default_settings("one-stack"); + env.setup_metadata(&["A"]); + write_commit_msg_hook(&env, "echo 'message policy rejected it' >&2\nexit 1"); + env.file("file.txt", "Some text"); + let branch_before = env.invoke_git("rev-parse refs/heads/A"); + + env.but("commit -m 'add file.txt'") + .assert() + .failure() + .stderr_eq(snapbox::str![[r#" +Error: commit-msg hook failed: +message policy rejected it + +To bypass the hook, run: but commit --no-hooks + +"#]]); + + assert_eq!( + env.invoke_git("rev-parse refs/heads/A"), + branch_before, + "a rejected message must not advance the branch" + ); + assert!( + env.but("status") + .assert() + .success() + .get_output() + .stdout + .windows(b"file.txt".len()) + .any(|window| window == b"file.txt"), + "a rejected commit must leave its changes uncommitted" + ); +} + +#[cfg(unix)] +#[test] +fn no_hooks_skips_commit_msg_hook() { + let env = Sandbox::init_scenario_with_target_and_default_settings("one-stack"); + env.setup_metadata(&["A"]); + write_commit_msg_hook(&env, "echo ran > commit-msg-ran.txt\nexit 1"); + env.file("file.txt", "Some text"); + + env.but("commit --no-hooks -m 'add file.txt'") + .assert() + .success(); + + assert!( + !env.projects_root().join("commit-msg-ran.txt").exists(), + "--no-hooks must skip commit-msg instead of 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_git("config core.hooksPath .git/hooks"); env.invoke_bash( "mkdir -p .git/hooks && cat > .git/hooks/pre-commit <<'HOOK'\n\ #!/bin/sh\n\ diff --git a/crates/gitbutler-repo/src/hooks.rs b/crates/gitbutler-repo/src/hooks.rs index 8acdcad4e60..f5f3631cfe4 100644 --- a/crates/gitbutler-repo/src/hooks.rs +++ b/crates/gitbutler-repo/src/hooks.rs @@ -45,6 +45,30 @@ pub enum MessageHookResult { Failure(ErrorData), } +/// A prepared `commit-msg` hook runner that owns its repository handle. +/// +/// Commit creation resolves editor input inside a workspace transaction. Owning the legacy +/// repository handle here lets the hook run at that point without borrowing the transaction's +/// [`Context`]. +pub struct CommitMsgHook { + repo: git2::Repository, + husky_search_paths: Option<&'static [&'static str]>, +} + +impl CommitMsgHook { + pub fn from_context(ctx: &Context) -> Result { + let repo = git2::Repository::open(&ctx.gitdir)?; + Ok(Self { + repo, + husky_search_paths: husky_search_paths(ctx), + }) + } + + pub fn run(&self, message: String) -> Result { + commit_msg_with_repo(&self.repo, self.husky_search_paths, message) + } +} + fn husky_search_paths(ctx: &Context) -> Option<&'static [&'static str]> { if ctx.legacy_project.husky_hooks_enabled { Some(&["../.husky"]) @@ -53,14 +77,19 @@ fn husky_search_paths(ctx: &Context) -> Option<&'static [&'static str]> { } } -pub fn commit_msg(ctx: &Context, mut message: String) -> Result { - let original_message = message.clone(); +pub fn commit_msg(ctx: &Context, message: String) -> Result { #[expect(deprecated, reason = "libgit2 hook adapter boundary")] - match git2_hooks::hooks_commit_msg( - &*ctx.git2_repo.get()?, - husky_search_paths(ctx), - &mut message, - )? { + let repo = &*ctx.git2_repo.get()?; + commit_msg_with_repo(repo, husky_search_paths(ctx), message) +} + +fn commit_msg_with_repo( + repo: &git2::Repository, + husky_search_paths: Option<&[&str]>, + mut message: String, +) -> Result { + let original_message = message.clone(); + match git2_hooks::hooks_commit_msg(repo, husky_search_paths, &mut message)? { H::NoHookFound => Ok(MessageHookResult::NotConfigured), H::Run(HookRunResponse { stdout,