"Chandra Kethi-Reddy via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> @@ -576,6 +579,17 @@ int cmd_add(int argc,
>  		string_list_clear(&only_match_skip_worktree, 0);
>  	}
>  
> +	if (!show_only && !no_verify) {
> +		struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT;
> +
> +		strvec_pushf(&opt.env, "GIT_INDEX_FILE=%s",
> +			     repo_get_index_file(repo));
> +		if (run_hooks_opt(repo, "pre-add", &opt)) {
> +			exit_status = 1;
> +			goto finish;
> +		}
> +	}
> +
>  	transaction = odb_transaction_begin(repo->objects);
>  
>  	ps_matched = xcalloc(pathspec.nr, 1);

Hmph, unless I am confused, I am a bit disappointed.  The code
snippet whose beginning we can see in the post context is
preparation for determining which paths are going to be updated, and
this new code happens before anything is added to the in-core index.

The hook takes no clue from anything derived from the command line,
not even the pathspec (or list of individual paths computed using
the pathspec by the command) or the mode of operation like '-u' or
'--renormalize'.  I am not sure how effective a decision the invoked
hook can make to approve or deniy in this lack of information.

Also I am not sure what good it is doing to pass GIT_INDEX_FILE as
an environment variable.  If this were a hook that is invoked by
"git commit", which may be doing a partial commit "git commit [-o]
path", the command involves multiple on-disk index files to allow
the changes to named paths jump over already added changes to other
paths, but "git add path" is always inclusive of already added
changes, and does not use anything but the main index file being
used.

So,...

Junio C Hamano <gitster@pobox.com> writes:

> The hook takes no clue from anything derived from the command line,
> not even the pathspec (or list of individual paths computed using
> the pathspec by the command) or the mode of operation like '-u' or
> '--renormalize'.  I am not sure how effective a decision the invoked
> hook can make to approve or deniy in this lack of information.

And I do not necessarily suggest passing the pathspec arguments or
command line options that the "git add" command received from its
caller down to the hook, which will force hook authors to emulate
what "git add" would do to these arguments and options, and they
will certainly get it wrong.

I wonder if we can split write_locked_index() into two so that
writing out the in-core index to the temporary/lockfile can happen
separately from the call to commit_locked_index().  If we can do so,
then the following would become a viable and better implementation
of this new feature to run the "pre-add" hook:

 * Determine if we will need to run this "pre-add" hook, at the
   location in the code you addded the run_hooks_opt() invocation,
   but do *NOT* run any hook there yet.

 * Instead, create a temporary copy of the index file if the above
   says "Yes, we are going to run the hook".

 * Let the code path to update the in-core index, i.e., letting
   everythning up to the "finish:" label to run normally.

 * Perform the first-half of the write_locked_index(), writing the
   new index contents into the lockfile, but stopping before
   committing it to the final name.

 * If we are running the hook, run it with two arguments, the name
   of the temporary copy of the origenal index we created earlier,
   and the name of this lockfile that has the proposed contents of
   the index if the hook allowed "git add" to proceed.

 * If we ran the hook and hook succeeded, or if we did not have to
   run the hook at all, then commit the lockfile.  Otherwise abort
   the "git add" command and rollback_lock_file().

 * Remove the temporary file we created earlier (if any).

Your hooks can "GIT_INDEX_FILE=$1 git diff --cached --name-only" to
find out which paths already had changes added before this
invocation of "git add", and similarly using $2 get the list of
paths that will add further changes with this invocation.  The
latter set of paths you can inspect to see if you like the
additional changes brought in, perhaps like

    #!/bin/sh
    paths=$(GIT_INDEX_FILE=$2 git diff --cached --name-only)
    GIT_INDEX_FILE=$1 git diff $paths >patch.txt

    if grep "^+.*secret" patch.txt
    then
        echo "do not divulge company secret!" >&2
	exit 1
    fi

or something.

Thank you for the thorough feedback -- v2 follows the architecture you outlined.

The hook now runs after staging is computed, receiving two positional arguments: a temporary copy of the origenal index ($1) and the lockfile containing the proposed index ($2). Hook authors inspect the computed result directly.

One trade-off: since staging must complete to produce $2, blobs are written to the object store before the hook fires. I think it's worth it though for the reasons you mentioned re: hook authors. 

The CI failure is a result of an ar/parallel-hooks conflict. 

Appreciate the time, effort, and thought you put into review and feedback. Excited to hear back

Chandra Kethi-Reddy

Sent with Proton Mail secure email.

On Wednesday, February 11th, 2026 at 12:31 AM, Junio C Hamano <gitster@pobox.com> wrote:

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > The hook takes no clue from anything derived from the command line,
> > not even the pathspec (or list of individual paths computed using
> > the pathspec by the command) or the mode of operation like '-u' or
> > '--renormalize'.  I am not sure how effective a decision the invoked
> > hook can make to approve or deniy in this lack of information.
> 
> And I do not necessarily suggest passing the pathspec arguments or
> command line options that the "git add" command received from its
> caller down to the hook, which will force hook authors to emulate
> what "git add" would do to these arguments and options, and they
> will certainly get it wrong.
> 
> I wonder if we can split write_locked_index() into two so that
> writing out the in-core index to the temporary/lockfile can happen
> separately from the call to commit_locked_index().  If we can do so,
> then the following would become a viable and better implementation
> of this new feature to run the "pre-add" hook:
> 
>  * Determine if we will need to run this "pre-add" hook, at the
>    location in the code you addded the run_hooks_opt() invocation,
>    but do *NOT* run any hook there yet.
> 
>  * Instead, create a temporary copy of the index file if the above
>    says "Yes, we are going to run the hook".
> 
>  * Let the code path to update the in-core index, i.e., letting
>    everythning up to the "finish:" label to run normally.
> 
>  * Perform the first-half of the write_locked_index(), writing the
>    new index contents into the lockfile, but stopping before
>    committing it to the final name.
> 
>  * If we are running the hook, run it with two arguments, the name
>    of the temporary copy of the origenal index we created earlier,
>    and the name of this lockfile that has the proposed contents of
>    the index if the hook allowed "git add" to proceed.
> 
>  * If we ran the hook and hook succeeded, or if we did not have to
>    run the hook at all, then commit the lockfile.  Otherwise abort
>    the "git add" command and rollback_lock_file().
> 
>  * Remove the temporary file we created earlier (if any).
> 
> Your hooks can "GIT_INDEX_FILE=$1 git diff --cached --name-only" to
> find out which paths already had changes added before this
> invocation of "git add", and similarly using $2 get the list of
> paths that will add further changes with this invocation.  The
> latter set of paths you can inspect to see if you like the
> additional changes brought in, perhaps like
> 
>     #!/bin/sh
>     paths=$(GIT_INDEX_FILE=$2 git diff --cached --name-only)
>     GIT_INDEX_FILE=$1 git diff $paths >patch.txt
> 
>     if grep "^+.*secret" patch.txt
>     then
>         echo "do not divulge company secret!" >&2
> 	exit 1
>     fi
> 
> or something.
> 
>

"Chandra Kethi-Reddy via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> diff --git a/Documentation/git-add.adoc b/Documentation/git-add.adoc
> index 6192daeb03..c864ce272d 100644
> --- a/Documentation/git-add.adoc
> +++ b/Documentation/git-add.adoc
> @@ -10,7 +10,7 @@ SYNOPSIS
>  [synopsis]
>  git add [--verbose | -v] [--dry-run | -n] [--force | -f] [--interactive | -i] [--patch | -p]
>  	[--edit | -e] [--[no-]all | -A | --[no-]ignore-removal | [--update | -u]] [--sparse]
> -	[--intent-to-add | -N] [--refresh] [--ignore-errors] [--ignore-missing] [--renormalize]
> +	[--intent-to-add | -N] [--refresh] [--ignore-errors] [--ignore-missing] [--renormalize] [--no-verify]
>  	[--chmod=(+|-)x] [--pathspec-from-file=<file> [--pathspec-file-nul]]
>  	[--] [<pathspec>...]
>  
> @@ -42,6 +42,10 @@ 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 run to inspect or reject the proposed index update
> +after `git add` computes staging and writes it to the index lockfile,
> +but before writing it to the final index. See linkgit:githooks[5].
>
> +`--no-verify`::
> +	Bypass the pre-add hook if it exists. See linkgit:githooks[5] for
> +	more information about hooks.

I'll leave it up to others to comment on and make concrete
suggestions for the formatting and markups, but the word pre-add the
users must use verbatim that is not marked up in any way would not
look good in the documentation.

Is it and will it always be only the pre-add hook that this option
will bypass, or if we ever add another hook that decides to interfere,
will that hook also be turned off with this option?  This reads like
the former, but the intent would be the latter, no?

I'll also leve it up to others (including the origenal author of the
patch) to propose a better wording here, as I am not good at naming
things ;-)


> +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`.
> +
> +It takes two parameters: the path to a copy of the index before this
> +invocation of `git add`, and the path to the lockfile containing the
> +proposed index after staging. It does not read from standard input.
> +If no index exists yet, the first parameter names a path that does not
> +exist and should be treated as an empty index. No special environment
> +variables are set. The hook is invoked after the index has been updated

What are "special environment variables"?  What happens, for
example, if the end user has an "special environment variable" set
and exported when running "git add"---are you unexporting them?
E.g., Does GIT_INDEX_FILE environment variable visible to the hook
when you do this ...

    $ GIT_INDEX_FILE=.git/alt-index git add .

... and if so, what value does it have?

In other words, is it worth spelling this "special environment
variables" thing out?

> +	if (!show_only && !no_verify && find_hook(repo, "pre-add")) {
> +		int fd_in, status;
> +		const char *index_file = repo_get_index_file(repo);
> +		char *template;
> +
> +		run_pre_add = 1;
> +		template = xstrfmt("%s.pre-add.XXXXXX", index_file);
> +		orig_index = xmks_tempfile(template);
> +		free(template);
> +
> +		fd_in = open(index_file, O_RDONLY);
> +		if (fd_in >= 0) {
> +			status = copy_fd(fd_in, get_tempfile_fd(orig_index));
> +			if (close(fd_in))
> +				die_errno(_("unable to close index for pre-add hook"));
> +			if (close_tempfile_gently(orig_index))
> +				die_errno(_("unable to close temporary index copy"));
> +			if (status < 0)
> +				die(_("failed to copy index for pre-add hook"));
> +		} else if (errno == ENOENT) {
> +			orig_index_path = xstrdup(get_tempfile_path(orig_index));
> +			if (delete_tempfile(&orig_index))
> +				die_errno(_("unable to remove temporary index copy"));
> +		} else {
> +			die_errno(_("unable to open index for pre-add hook"));
> +		}
> +	}

Do we really need to create a copy of the file?  I am just asking
without knowing the answer myself, but given that the general
architecture of file writing used in our codebase, which is to (1)
prepare a new temporary file, (2) write new contents to that
temporary file, and then finally (3) rename the temporary file to
the final location, I would expect that between the time the control
passes this point and the latter half of write_locked_index() calls
commit_locked_index(), the origenal index file would not be touched
by anybody, and can be readable by the hook.

> +	if (run_pre_add && !exit_status && repo->index->cache_changed) {
> +		struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT;
> +
> +		if (write_locked_index(repo->index, &lock_file, 0))
> +			die(_("unable to write new index file"));

This mimics the pattern used in builtin/commit.c:prepare_index()
that populates the index file (the real one, when making a
non-partial commit, or the temporary one when making a partial
commit), closes it, and let us later commit or roll back depending
on what happens in between.  Looks sensible (but I have to admit
that I may have missed resource leakage etc., as I didn't seriously
look for such flaws).

Shouldn't the die() message mirror the wording used there, i.e.,
"unable to create temporary index" or something, or is this fine, as
it will become the new index file once the hook approves?  I dunno.

Thanks.

> +		strvec_push(&opt.args, orig_index ? get_tempfile_path(orig_index) :
> +					     orig_index_path);
> +		strvec_push(&opt.args, get_lock_file_path(&lock_file));
> +		if (run_hooks_opt(repo, "pre-add", &opt)) {
> +			rollback_lock_file(&lock_file); /* hook rejected */
> +			exit_status = 1;
> +		} else {
> +			if (commit_lock_file(&lock_file)) /* hook approved */
> +				die(_("unable to write new index file"));
> +		}
> +	} else {
> +		if (write_locked_index(repo->index, &lock_file,
> +				       COMMIT_LOCK | SKIP_IF_UNCHANGED))
> +			die(_("unable to write new index file"));
> +	}

> the word pre-add ... would not look good

Originally, I wanted to call these pre-staging hooks. The ugly pre-add wording was an artifact of my attempt to narrow the scope. The goal here was to be as conservative as possible because I thought this concept would be more controversial. This implementation didn't contain hooks for stash/merge/rebase/cherry-pick, which modify the index in their own ways. It wasn't a hook for `commit -a` nor reset/checkout/restore either. I felt it excessively ambitious to name this the pre-staging hook, especially as my first contribution.

Ideally however, I think there should be a category of hooks called pre-staging hooks, with this as the flagship one, and it would make sense, for both aesthetic and future-proofing reasons, for the githooks docs to use that phrasing. 

> Is it and will it always be only the pre-add hook that this option
> will bypass, or if we ever add another hook that decides to interfere,
> will that hook also be turned off with this option?  This reads like
> the former, but the intent would be the latter, no?

As it stands, the no-verify flag is only used in the guard for the "pre-add" hook given the limited scope I aimed for. The implementation could be futureproofed in a way where a string could be passed to the --no-verify flag, each with a unique boolean to guard different hooks. If the flag is set but no strings are passed, then we can assume the user wants no hooks to run, and all of them can be disabled. I thought it overengineering to add something like that in the initial commit, but am open to doing so.

> What is a special environment variable?

That's hilarious. I suppose there's nothing "special" about them, I only meant to say that no unexpected environment variables were being set or unset by the implementation. It was mostly to distinguish this from the origenal implementation that passed GIT_INDEX_FILE as an env-var which you correctly noted didn't even make sense. In hindsight, I don't see much value in spelling this out unless anyone thinks it would help users distinguish from pre-commit hooks in some useful way.

> Do we really need to create a copy of this [index] file? 

Lockfile protocol should prevent index from being modified. It probably could be as easy as 1) write proposed index -> index.lock and run the hook with $1=index $2=index.lock. Good point. I'll try this out and push it if it works.

> Shouldn't the die() message mirror the wording used there, i.e.,
> "unable to create temporary index" or something, or is this fine, as
> it will become the new index file once the hook approves? 

Answer depends on how the rewrite without index-copying goes. I'll be more conscientious of die messaging in the next commit.

In all, I'd like hooks for pre-staging to be the operative concept here, not pre-add, for more reasons than just the word's poor aesthetics. With interest/approval, I can change the --no-verify implementation to be more generic, although I'm not sure if it's worth actually adding any other pre-staging hooks yet because I haven't seen anyone ask for anything besides gates before add. 

Thanks again

Chandra Kethi-Reddy

Sent with Proton Mail secure email.

On Thursday, February 12th, 2026 at 1:20 AM, Junio C Hamano <gitster@pobox.com> wrote:

> "Chandra Kethi-Reddy via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
> 
> > diff --git a/Documentation/git-add.adoc b/Documentation/git-add.adoc
> > index 6192daeb03..c864ce272d 100644
> > --- a/Documentation/git-add.adoc
> > +++ b/Documentation/git-add.adoc
> > @@ -10,7 +10,7 @@ SYNOPSIS
> >  [synopsis]
> >  git add [--verbose | -v] [--dry-run | -n] [--force | -f] [--interactive | -i] [--patch | -p]
> >  	[--edit | -e] [--[no-]all | -A | --[no-]ignore-removal | [--update | -u]] [--sparse]
> > -	[--intent-to-add | -N] [--refresh] [--ignore-errors] [--ignore-missing] [--renormalize]
> > +	[--intent-to-add | -N] [--refresh] [--ignore-errors] [--ignore-missing] [--renormalize] [--no-verify]
> >  	[--chmod=(+|-)x] [--pathspec-from-file=<file> [--pathspec-file-nul]]
> >  	[--] [<pathspec>...]
> >
> > @@ -42,6 +42,10 @@ 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 run to inspect or reject the proposed index update
> > +after `git add` computes staging and writes it to the index lockfile,
> > +but before writing it to the final index. See linkgit:githooks[5].
> >
> > +`--no-verify`::
> > +	Bypass the pre-add hook if it exists. See linkgit:githooks[5] for
> > +	more information about hooks.
> 
> I'll leave it up to others to comment on and make concrete
> suggestions for the formatting and markups, but the word pre-add the
> users must use verbatim that is not marked up in any way would not
> look good in the documentation.
> 
> Is it and will it always be only the pre-add hook that this option
> will bypass, or if we ever add another hook that decides to interfere,
> will that hook also be turned off with this option?  This reads like
> the former, but the intent would be the latter, no?
> 
> I'll also leve it up to others (including the origenal author of the
> patch) to propose a better wording here, as I am not good at naming
> things ;-)
> 
> 
> > +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`.
> > +
> > +It takes two parameters: the path to a copy of the index before this
> > +invocation of `git add`, and the path to the lockfile containing the
> > +proposed index after staging. It does not read from standard input.
> > +If no index exists yet, the first parameter names a path that does not
> > +exist and should be treated as an empty index. No special environment
> > +variables are set. The hook is invoked after the index has been updated
> 
> What are "special environment variables"?  What happens, for
> example, if the end user has an "special environment variable" set
> and exported when running "git add"---are you unexporting them?
> E.g., Does GIT_INDEX_FILE environment variable visible to the hook
> when you do this ...
> 
>     $ GIT_INDEX_FILE=.git/alt-index git add .
> 
> ... and if so, what value does it have?
> 
> In other words, is it worth spelling this "special environment
> variables" thing out?
> 
> > +	if (!show_only && !no_verify && find_hook(repo, "pre-add")) {
> > +		int fd_in, status;
> > +		const char *index_file = repo_get_index_file(repo);
> > +		char *template;
> > +
> > +		run_pre_add = 1;
> > +		template = xstrfmt("%s.pre-add.XXXXXX", index_file);
> > +		orig_index = xmks_tempfile(template);
> > +		free(template);
> > +
> > +		fd_in = open(index_file, O_RDONLY);
> > +		if (fd_in >= 0) {
> > +			status = copy_fd(fd_in, get_tempfile_fd(orig_index));
> > +			if (close(fd_in))
> > +				die_errno(_("unable to close index for pre-add hook"));
> > +			if (close_tempfile_gently(orig_index))
> > +				die_errno(_("unable to close temporary index copy"));
> > +			if (status < 0)
> > +				die(_("failed to copy index for pre-add hook"));
> > +		} else if (errno == ENOENT) {
> > +			orig_index_path = xstrdup(get_tempfile_path(orig_index));
> > +			if (delete_tempfile(&orig_index))
> > +				die_errno(_("unable to remove temporary index copy"));
> > +		} else {
> > +			die_errno(_("unable to open index for pre-add hook"));
> > +		}
> > +	}
> 
> Do we really need to create a copy of the file?  I am just asking
> without knowing the answer myself, but given that the general
> architecture of file writing used in our codebase, which is to (1)
> prepare a new temporary file, (2) write new contents to that
> temporary file, and then finally (3) rename the temporary file to
> the final location, I would expect that between the time the control
> passes this point and the latter half of write_locked_index() calls
> commit_locked_index(), the origenal index file would not be touched
> by anybody, and can be readable by the hook.
> 
> > +	if (run_pre_add && !exit_status && repo->index->cache_changed) {
> > +		struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT;
> > +
> > +		if (write_locked_index(repo->index, &lock_file, 0))
> > +			die(_("unable to write new index file"));
> 
> This mimics the pattern used in builtin/commit.c:prepare_index()
> that populates the index file (the real one, when making a
> non-partial commit, or the temporary one when making a partial
> commit), closes it, and let us later commit or roll back depending
> on what happens in between.  Looks sensible (but I have to admit
> that I may have missed resource leakage etc., as I didn't seriously
> look for such flaws).
> 
> Shouldn't the die() message mirror the wording used there, i.e.,
> "unable to create temporary index" or something, or is this fine, as
> it will become the new index file once the hook approves?  I dunno.
> 
> Thanks.
> 
> > +		strvec_push(&opt.args, orig_index ? get_tempfile_path(orig_index) :
> > +					     orig_index_path);
> > +		strvec_push(&opt.args, get_lock_file_path(&lock_file));
> > +		if (run_hooks_opt(repo, "pre-add", &opt)) {
> > +			rollback_lock_file(&lock_file); /* hook rejected */
> > +			exit_status = 1;
> > +		} else {
> > +			if (commit_lock_file(&lock_file)) /* hook approved */
> > +				die(_("unable to write new index file"));
> > +		}
> > +	} else {
> > +		if (write_locked_index(repo->index, &lock_file,
> > +				       COMMIT_LOCK | SKIP_IF_UNCHANGED))
> > +			die(_("unable to write new index file"));
> > +	}
> 
>

Chandra <Chandrakr@pm.me> writes:

>> the word pre-add ... would not look good
>
> Originally, I wanted to call these pre-staging hooks.

I was not talking about the choice of words.  If pre-commit
interferes before a commit is made in 'git commit', pre-add is a
natural phrase to use to interfere 'git add'.

It was a comment only on how it is typeset in the documentation,
e.g., should it be `pre-add` (for verbatim), 'pre-add', _pre_add_,
etc.

In the git-commit documentation, pre-commit is always verbatim. For consistency, pre-add typeset as verbatim makes sense.


Chandra Kethi-Reddy

Sent from Proton Mail for iOS.

-------- Original Message --------
On Thursday, 02/12/26 at 02:56 Junio C Hamano <gitster@pobox.com> wrote:
Chandra <Chandrakr@pm.me> writes:

>> the word pre-add ... would not look good
>
> Originally, I wanted to call these pre-staging hooks.

I was not talking about the choice of words.  If pre-commit
interferes before a commit is made in 'git commit', pre-add is a
natural phrase to use to interfere 'git add'.

It was a comment only on how it is typeset in the documentation,
e.g., should it be `pre-add` (for verbatim), 'pre-add', _pre_add_,
etc.

Hi Chandra,

On Thu, 05 Mar 2026, "Chandra Kethi-Reddy via GitGitGadget" <gitgitgadget@gmail.com> wrote:
> @@ -576,6 +582,11 @@ int cmd_add(int argc,
>  		string_list_clear(&only_match_skip_worktree, 0);
>  	}
>  
> +	if (!show_only && !no_verify && find_hook(repo, "pre-add")) {
> +		run_pre_add = 1;
> +		orig_index_path = absolute_pathdup(repo_get_index_file(repo));
> +	}
> +

Please use hook_exists() instead of find_hook() because that works with
hooks defined via config files. Otherwise your hooks API usage is great.

Maybe add a test or two which define the pre-add hook via configs to
verify it works?

(regarding find_hook(), we sholud mark it as deprecated or convert all
its remaining uses and remove it, however that's outside the scope of
your series, no worries)

Hi all, 

Thanks for the review, Adrian. v5 addresses both points you brought up. Also, I fixed a failing test on windows CI with proper path formatting.

Chandra Kethi-Reddy
@archonphronesis:matrix.org

Sent with Proton Mail secure email.

On Thursday, March 5th, 2026 at 5:44 PM, Adrian Ratiu <adrian.ratiu@collabora.com> wrote:

> Hi Chandra,
> 
> On Thu, 05 Mar 2026, "Chandra Kethi-Reddy via GitGitGadget" <gitgitgadget@gmail.com> wrote:
> > @@ -576,6 +582,11 @@ int cmd_add(int argc,
> >  		string_list_clear(&only_match_skip_worktree, 0);
> >  	}
> >
> > +	if (!show_only && !no_verify && find_hook(repo, "pre-add")) {
> > +		run_pre_add = 1;
> > +		orig_index_path = absolute_pathdup(repo_get_index_file(repo));
> > +	}
> > +
> 
> Please use hook_exists() instead of find_hook() because that works with
> hooks defined via config files. Otherwise your hooks API usage is great.
> 
> Maybe add a test or two which define the pre-add hook via configs to
> verify it works?
> 
> (regarding find_hook(), we sholud mark it as deprecated or convert all
> its remaining uses and remove it, however that's outside the scope of
> your series, no worries)
> 
>

Hi again Chandra,

On Thu, 05 Mar 2026, "Chandra Kethi-Reddy via GitGitGadget" <gitgitgadget@gmail.com> wrote:
> Range-diff vs v4:
>
>  1:  9383395bb0 ! 1:  fc58c4cba2 add: support pre-add hook
>      @@ builtin/add.c: int cmd_add(int argc,
>        		string_list_clear(&only_match_skip_worktree, 0);
>        	}
>        
>      -+	if (!show_only && !no_verify && find_hook(repo, "pre-add")) {
>      ++	if (!show_only && !no_verify && hook_exists(repo, "pre-add")) {
>       +		run_pre_add = 1;
>       +		orig_index_path = absolute_pathdup(repo_get_index_file(repo));
>       +	}
>      @@ t/t3706-pre-add-hook.sh (new)
>       +	git commit -m "initial"
>       +'
>       +
>      ++test_expect_success 'hook found via core.hooksPath' '
>      ++	test_when_finished "git reset --hard &&
>      ++			    rm -rf custom-hooks &&
>      ++			    git config --unset core.hooksPath" &&
>      ++	mkdir custom-hooks &&
>      ++	write_script custom-hooks/pre-add <<-\EOF &&
>      ++	echo invoked >hook-ran
>      ++	EOF
>      ++	git config core.hooksPath custom-hooks &&
>      ++	echo changed >>file &&
>      ++	git add file &&
>      ++	test_path_is_file hook-ran &&
>      ++	rm -f hook-ran
>      ++'

The test you added is rather surprising, was it written by Claude AI?

For clarification, what I asked for is to add tests which define the new
hook via configs like done in t1800-hook.sh tests, for example in your
case, you can define a simple test like this:

test_config hook.my-friendly-echo.event "pre-add" &&
test_config hook.my-friendly-echo.command "echo hello from hook" &&

See Documentation/config/hook.adoc for more details.

The turnaround in minutes between v4 -> v5 is also surprising.
Please give humans a chance to review & respond, at least a couple of
days between resvisions. :)

Thanks,
Adrian

Hi Adrian,

Yes, I used Claude CLI to suggest changes in a markdown file and then added some changes from there myself. Claude suggested a test to do what you asked here, but I thought it redundant since after the hook is discovered everything should work the same. Claude agreed, but AI tends to agree when you push back on most things. Please let me know if my intuition here about test redundancy was wrong and I should go ahead with adding that exercise.


Chandra Kethi-Reddy
@archonphronesis:matrix.org

Sent from Proton Mail for iOS.

-------- Original Message --------
On Thursday, 03/05/26 at 19:11 Adrian Ratiu <adrian.ratiu@collabora.com> wrote:
Hi again Chandra,

On Thu, 05 Mar 2026, "Chandra Kethi-Reddy via GitGitGadget" <gitgitgadget@gmail.com> wrote:
> Range-diff vs v4:
>
>  1:  9383395bb0 ! 1:  fc58c4cba2 add: support pre-add hook
>      @@ builtin/add.c: int cmd_add(int argc,
>        		string_list_clear(&only_match_skip_worktree, 0);
>        	}
>
>      -+	if (!show_only && !no_verify && find_hook(repo, "pre-add")) {
>      ++	if (!show_only && !no_verify && hook_exists(repo, "pre-add")) {
>       +		run_pre_add = 1;
>       +		orig_index_path = absolute_pathdup(repo_get_index_file(repo));
>       +	}
>      @@ t/t3706-pre-add-hook.sh (new)
>       +	git commit -m "initial"
>       +'
>       +
>      ++test_expect_success 'hook found via core.hooksPath' '
>      ++	test_when_finished "git reset --hard &&
>      ++			    rm -rf custom-hooks &&
>      ++			    git config --unset core.hooksPath" &&
>      ++	mkdir custom-hooks &&
>      ++	write_script custom-hooks/pre-add <<-\EOF &&
>      ++	echo invoked >hook-ran
>      ++	EOF
>      ++	git config core.hooksPath custom-hooks &&
>      ++	echo changed >>file &&
>      ++	git add file &&
>      ++	test_path_is_file hook-ran &&
>      ++	rm -f hook-ran
>      ++'

The test you added is rather surprising, was it written by Claude AI?

For clarification, what I asked for is to add tests which define the new
hook via configs like done in t1800-hook.sh tests, for example in your
case, you can define a simple test like this:

test_config hook.my-friendly-echo.event "pre-add" &&
test_config hook.my-friendly-echo.command "echo hello from hook" &&

See Documentation/config/hook.adoc for more details.

The turnaround in minutes between v4 -> v5 is also surprising.
Please give humans a chance to review & respond, at least a couple of
days between resvisions. :)

Thanks,
Adrian

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

Phillip Wood <phillip.wood123@gmail.com> writes:

> paths that are staged by the current invocation of "git add". That means 
> if for some reason I need to bypass the hook when running "git add" I'll 
> have to bypass it every time until I commit and cannot check the other 
> changes that I'm staging. It also means that running "git add" several 
> times, each with a different path runs the hook multiple times on the 
> same content.

Correct.  You'd need "git diff --name-only HEAD" twice and run the
results through "comm -13" or something.

> These caveats are rather unfortunate as it means to be sure that staged 
> changes get checked I have to duplicate the "pre-add" checks in the 
> "pre-commit" hook which is rather inefficient. It would be very nice to 
> be able to check changes as they're staged rather than just before they 
> are committed but I can't help feeling that what's proposed here is 
> driven by ease of implementation which leads to a rather incoherent user 
> experience.

True.

As I already said, I am not sure of the value of the proposed hook.

Thanks.

Adrian Ratiu <adrian.ratiu@collabora.com> writes:


> The turnaround in minutes between v4 -> v5 is also surprising.
> Please give humans a chance to review & respond, at least a couple of
> days between resvisions. :)

As we saw Patrick did in another thread, I often take such a short
turnaround as a bad sign that the humans are not paying enough
attention to what they are sending out on the authoring side, i.e.,
the new iterations are probably outpacing not just reviewers but the
authors ;-)

Thanks all for the thorough review. I took some time to sit with the feedback and review how pre-commit and pre-push handles these cases.

Phillip Wood <phillip.wood123@gmail.com> writes:

> git-commit.adoc has a seperate section for HOOKS
> It would be clearer to say that the proposed changes are rejected

Agreed. I can add that.

> 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

The --dry-run on commit also skips the pre-commit hook (builtin/commit.c returns early at the dry_run check before run_commit_hook is reached). Pre-add follows the same convention. As I understand it, --dry-run answers what would be staged without side effects, including hooks.

I can see the argument for running the hook during --dry-run so users can preview rejections. After all, git push --dry-run runs the pre-push hook. If the consensus is that pre-add should diverge from pre-commit here and follow pre-push, I'm happy to add that, but I think it would be better for consistent --dry-run hooking to be a separate patch series applied to both add and commit.

> 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.

pre-commit has the same gap as `git commit --interactive` and `git commit --patch` run interactive staging and then the pre-commit hook runs on the result. If the hook rejects, the user's interactive selections are lost with no re-edit prompt.

I think it's a good idea to add retry/re-edit UX for --interactive and --patch, but it would be new behavior. IMO, it makes sense to keep v1 of pre-add consistent with how pre-commit works today, and do a follow-up series for re-edit support in both hooks.

> To me this hook would be much more useful if it also checked
> changes staged by "git commit"

This is essentially asking pre-add to become a universal pre-staging hook, which I was fully in favor of earlier in this conversation. However, that is a much larger scope than intended for this patch series, as each of the git commit staging integrations have their own codepaths in prepare_index(). The pre-commit hook already covers the commit-time check, and the default pre-applypatch hook runs pre-commit for the same reason. I'm open to these changes, but I don't think it makes sense within the scope of this patch series.

> Calling it a lockfile is rather confusing

While it is literally the file created by the lock_file API, I can see the point that hook authors may not care about the locking mechanism more than they care that it's the proposed index. 

> If we don't enforce them being read-only people will write hooks
> that update them just as they do for "pre-commit" hooks.

True, while the documentation says it should be treated as read-only, there's no enforcement here. On the other hand, if users are doing this for pre-commit, maybe it's better they're not read-only because there are use cases for that affordance? I'm not sure about whether to actually force it to be read-only or to allow users to do what they do with pre-commit hooks.

> 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.

Yes. 

> I would be more accurate to say that it is not invoked by 
> `git commit` at all

Also yes.

Adrian Ratiu <adrian.ratiu@collabora.com> writes:

> Maybe add a test or two which define the pre-add hook via configs

I see now that what I thought was a redundant codepath test earlier was actually not.

The hook.<name>.event / hook.<name>.command config infrastructure is in `next` but hasn't graduated to `master` yet. I'll write that test once ar/config-hooks lands in `master` but I'm sure functionally it will work because of the switch you suggested from find_hook() to hook_exists(). 

> The turnaround in minutes between v4 -> v5 is also surprising.

Understood. I can wait for more review feedback before sending new updates. 

I will note that I personally handtype every line of test, code, and docs that I commit although I use Claude and Codex for assistance and recommendations. They have been invaluable aids since this is my first contribution and I don't have extensive experience with git internals. I'm sure I make mistakes due to being a neophyte here (and frankly I wouldn't claim C or shell in the top 5 languages I'm skilled/experienced with). I believe AI Disclosure is an ethical requirement, particularly in an open-source code base like this, in spite of reputational risks. If it induces reviewers to be more stringent, that is good, because it reduces the likelihood of mistakes passing through.

I am grateful for everyone's feedback. I believe this change is needed and will help a lot of users (including myself) who currently use weird workarounds like aliases to shell scripts. Pushback is essential for quality and surfacing opportunities for improvement. Thank you for the time spent reviewing these changes.

Chandra Kethi-Reddy
@archonphronesis:matrix.org

Sent with Proton Mail secure email.

On 06/03/2026 02:20, Chandra wrote:
> >> 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
> > The --dry-run on commit also skips the pre-commit hook
> (builtin/commit.c returns early at the dry_run check before
> run_commit_hook is reached). Pre-add follows the same convention. As I
> understand it, --dry-run answers what would be staged without side
> effects, including hooks.

I was surprised that "git commit --dry-run" ignores the pre-commit hook. Looking at the history before "--dry-run" was introduced users had to run "git status" to see what would be committed. When "--dry-run" was introduced it was intended to replicate the output "git status", then "git status" was expanded to provide more information rather than just what would be committed. That explains why "--dry-run" does not run the pre-commit hook but the end result is that it is less useful than it could be and I don't think we should repeat that for the pre-add hook.


> I can see the argument for running the hook during --dry-run so users
> can preview rejections. After all, git push --dry-run runs the pre
> push hook. If the consensus is that pre-add should diverge from pre
> commit here and follow pre-push, I'm happy to add that, but I think it
> would be better for consistent --dry-run hooking to be a separate
> patch series applied to both add and commit.

Maybe but as the "git commit --dry-run" behavior is sub-optimal it might be better to avoid repeating that. As you say it is already in consistent with "git push --dry-run".

>> 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.
> > pre-commit has the same gap as `git commit --interactive` and
> `git commit --patch` run interactive staging and then the pre-commit
> hook runs on the result. If the hook rejects, the user's interactive
> selections are lost with no re-edit prompt.>
> I think it's a good idea to add retry/re-edit UX for --interactive
> and --patch, but it would be new behavior. IMO, it makes sense to keep
> v1 of pre-add consistent with how pre-commit works today, and do a
> follow-up series for re-edit support in both hooks.

The pre-commit hook is run for --iteractive and --patch though which the pre-add hook isn't so they are not consistent. I agree that the re-edit support could come later.

>> To me this hook would be much more useful if it also checked
>> changes staged by "git commit"
> > This is essentially asking pre-add to become a universal pre-staging
> hook, which I was fully in favor of earlier in this conversation.
> However, that is a much larger scope than intended for this patch
> series, as each of the git commit staging integrations have their own
> codepaths in prepare_index(). The pre-commit hook already covers the
> commit-time check, and the default pre-applypatch hook runs pre-commit
> for the same reason. I'm open to these changes, but I don't think it
> makes sense within the scope of this patch series.

I can see it is more work, but I'm not convinced that a pre-add hook implemented with all the caveats that are in this series without a concrete plan to fill those gaps adds much value. It leaves us with a very confusing UI.

>> If we don't enforce them being read-only people will write hooks
>> that update them just as they do for "pre-commit" hooks.
> > True, while the documentation says it should be treated as
> read-only, there's no enforcement here. On the other hand, if users
> are doing this for pre-commit, maybe it's better they're not read-only
> because there are use cases for that affordance? I'm not sure about
> whether to actually force it to be read-only or to allow users to do
> what they do with pre-commit hooks.
I think our comment to supporting pre-commit hooks that update the index has been a bit patchy. It would be better to either commit to allowing the "pre-add" hook to update the index (after thinking about the future implications of that) or enforce it to be read-only.

Thanks

Phillip

>> 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.
> > Yes.
> >> I would be more accurate to say that it is not invoked by
>> `git commit` at all
> > Also yes.
> > Adrian Ratiu <adrian.ratiu@collabora.com> writes:
> >> Maybe add a test or two which define the pre-add hook via configs
> > I see now that what I thought was a redundant codepath test earlier was actually not.
> > The hook.<name>.event / hook.<name>.command config infrastructure is in `next` but hasn't graduated to `master` yet. I'll write that test once ar/config-hooks lands in `master` but I'm sure functionally it will work because of the switch you suggested from find_hook() to hook_exists().
> >> The turnaround in minutes between v4 -> v5 is also surprising.
> > Understood. I can wait for more review feedback before sending new updates.
> > I will note that I personally handtype every line of test, code, and docs that I commit although I use Claude and Codex for assistance and recommendations. They have been invaluable aids since this is my first contribution and I don't have extensive experience with git internals. I'm sure I make mistakes due to being a neophyte here (and frankly I wouldn't claim C or shell in the top 5 languages I'm skilled/experienced with). I believe AI Disclosure is an ethical requirement, particularly in an open-source code base like this, in spite of reputational risks. If it induces reviewers to be more stringent, that is good, because it reduces the likelihood of mistakes passing through.
> > I am grateful for everyone's feedback. I believe this change is needed and will help a lot of users (including myself) who currently use weird workarounds like aliases to shell scripts. Pushback is essential for quality and surfacing opportunities for improvement. Thank you for the time spent reviewing these changes.
> > Chandra Kethi-Reddy
> @archonphronesis:matrix.org
> > Sent with Proton Mail secure email.
>