Restore commit hooks in the CLI

crates/but/skill/references/reference.md

@@ -165,6 +165,7 @@ but commit --above <target> -m "message" <id>  # Place the commit above a commit
 but commit --below <target> -m "message" <id>  # Place the commit below a commit or branch
 but commit -b <branch> --no-message <id>     # Commit without a message
 but commit --empty -b <branch> -m "message"  # Insert an empty commit
+but commit -b <branch> -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 <branch> -m "message"  # Insert an empty commit
 
 **Important:** `but commit -b <branch> -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:

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.

crates/but/src/command/legacy/commit.rs

@@ -10,9 +10,12 @@ 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::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,
@@ -119,7 +122,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 +133,7 @@ pub fn commit(
         commit_op,
         commit_selection,
         reword_op,
+        run_hooks,
     )?)
 }
 
@@ -145,6 +149,7 @@ fn resolve(
     CommitOperation,
     CommitSelection,
     CommitMessageSource,
+    RunHooks,
 )> {
     let Platform {
         no_message,
@@ -154,6 +159,7 @@ fn resolve(
         above,
         below,
         interactive,
+        no_hooks,
         changes,
         allow_merged,
     } = args;
@@ -236,7 +242,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,28 +280,31 @@ pub fn run(
     commit_op: CommitOperation,
     commit_selection: CommitSelection,
     reword_op: CommitMessageSource,
+    run_hooks: RunHooks,
 ) -> anyhow::Result<CommitOutcome> {
-    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);
-
-        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()?;
+    let takes_whole_worktree = matches!(commit_selection, CommitSelection::AllChanges);
+    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 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));
             }
-            CommitSelection::Nothing => {}
         }
+    }
 
-        builder.into_diff_specs()
-    };
     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,172 @@ 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<Vec<String>> {
+    let context_lines = ctx.settings.context_lines;
+    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 specs = changes.iter().cloned().map(Ok).collect::<Vec<_>>();
+        let (tree, ..) = but_core::tree::apply_worktree_changes(
+            head.detach(),
+            &repo,
+            &mut specs,
+            context_lines,
+        )?;
+        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 => {}
+        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<Vec<u8>>)>,
+}
+
+impl HunkSelections {
+    fn of(changes: &[DiffSpec], workdir: &Path) -> Self {
+        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()));
+                let before = std::fs::read(&path).ok();
+                (spec.path.clone(), path, before)
+            })
+            .collect();
+        Self { files }
+    }
+
+    fn changed_since(self) -> Vec<String> {
+        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 a commit is made of.
+fn build_diff_specs(
+    ctx: &mut Context,
+    perm: &mut RepoExclusive,
+    commit_selection: CommitSelection,
+) -> anyhow::Result<Vec<DiffSpec>> {
+    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);
+
+    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()?;
+        }
+        CommitSelection::Nothing => {}
+    }
+
+    Ok(builder.into_diff_specs())
+}
+
+/// 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);
+    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.

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);

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(

crates/but/src/lib.rs

@@ -133,7 +133,7 @@ fn parse_args_and_output_format(args: Vec<OsString>, 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));