Re: [PATCH v5] add: support pre-add hook

[Phillip Wood <phillip.wood123@gmail.com>]

On 05/03/2026 12:37, Chandra Kethi-Reddy via GitGitGadget wrote:
> 
> diff --git a/Documentation/git-add.adoc b/Documentation/git-add.adoc
> index 6192daeb03..a3ff4ced83 100644
> --- a/Documentation/git-add.adoc
> +++ b/Documentation/git-add.adoc
> [...]   
> @@ -42,10 +42,11 @@ use the `--force` option to add ignored files. If you specify the exact
>   filename of an ignored file, `git add` will fail with a list of ignored
>   files. Otherwise it will silently ignore the file.
>   
> +A `pre-add` hook can be used to reject `git add` (see linkgit:githooks[5]).

git-commit.adoc has a separate section for HOOKS, perhaps we should do 
the same here. It would be clearer to say the that the proposed changes 
are rejected rather than `git add` itself.

> diff --git a/Documentation/githooks.adoc b/Documentation/githooks.adoc
> index 056553788d..90945a590e 100644
> --- a/Documentation/githooks.adoc
> +++ b/Documentation/githooks.adoc
> @@ -94,6 +94,36 @@ and is invoked after the patch is applied and a commit is made.
>   This hook is meant primarily for notification, and cannot affect
>   the outcome of `git am`.
>   
> +pre-add
> +~~~~~~~
> +
> +This hook is invoked by linkgit:git-add[1], and can be bypassed with the
> +`--no-verify` option. It is not invoked for `--interactive`, `--patch`,
> +`--edit`, or `--dry-run`.

I'm struggling to see how it is helpful to the user for "git add 
--dry-run $path" to succeed when "git add $path" will be rejected by the 
"pre-add" hook.

The other options all use "git apply" to apply a diff to the index so 
they could apply the patch to a temporary index which is then passed to 
the "pre-add" hook. If the hook fails the user should be given the 
option to re-edit the patch or re-select the hunks so that their work is 
not wasted.

To me this hook would be much more useful if it also checked changes 
staged by "git commit" - it is still staging changes after all.

> +It takes two arguments: the path to the index file for this invocation
> +of `git add`, and the path to the lockfile containing the proposed

Calling it a lockfile is rather confusing - it is just second index file 
that contains the changes that would be staged.

> +index after staging. If no index exists yet, the first argument names
> +a path that does not exist and should be treated as an empty index.
> +
> +The hook is invoked after the index has been updated in memory and
> +written to the lockfile, but before it is committed to the final index
> +path. Exiting with a non-zero status causes `git add` to reject the
> +proposed state, roll back the lockfile, and leave the index unchanged.
> +Exiting with zero status allows the index update to be committed. The
> +hook accepts or rejects the entire proposed update; per-path filtering
> +is not supported. Both files should be treated as read-only by the hook.

If we don't enforce them being read-only people will write hooks that 
update them just as they do for "pre-commit" hooks. Once they start 
relying on that they will complain if we stop supporting it. If we lock 
both index files before running the hook I think that will prevent the 
hook from being able to update them.

> +Hook authors may set `GIT_INDEX_FILE="$1"` to inspect the current index
> +state and `GIT_INDEX_FILE="$2"` to inspect the proposed index state.

We should be explicit that the proposed index state contains all the 
changes that would be committed so staging changes incrementally will 
check them multiple times.

> +This hook can be used to prevent staging of files based on names, content,
> +or sizes (e.g., to block `.env` files, secret keys, or large files).
> +
> +This hook is not invoked by `git commit -a` or `git commit --include`

I would be more accurate to say that it is not invoked by `git commit` 
at all as there are several ways of staging changes including `git 
commit $path`. We should also be explicit that in order to ensure that 
all staged changes are checked the checks in the "pre-add" hook must be 
duplicated by the "pre-commit" hook.

> +which still can run the `pre-commit` hook, providing a control point at
> +commit time.

While I've commented on the documentation, I think it is really the 
design that needs working on. I like the idea of giving feedback earlier 
when staging changes rather than waiting for the user to run "git 
commit" but I think we need a more coherent approach to when the hook is 
run.

Thanks

Phillip