Open
Conversation
Welcome to GitGitGadgetHi @shatachandra, and welcome to GitGitGadget, the GitHub App to send patch series to the Git mailing list from GitHub Pull Requests. Please make sure that either:
You can CC potential reviewers by adding a footer to the PR description with the following syntax: NOTE: DO NOT copy/paste your CC list from a previous GGG PR's description, Also, it is a good idea to review the commit messages one last time, as the Git project expects them in a quite specific form:
It is in general a good idea to await the automated test ("Checks") in this Pull Request before contributing the patches, e.g. to avoid trivial issues such as unportable code. Contributing the patchesBefore you can contribute the patches, your GitHub username needs to be added to the list of permitted users. Any already-permitted user can do that, by adding a comment to your PR of the form Both the person who commented An alternative is the channel Once on the list of permitted usernames, you can contribute the patches to the Git mailing list by adding a PR comment If you want to see what email(s) would be sent for a After you submit, GitGitGadget will respond with another comment that contains the link to the cover letter mail in the Git mailing list archive. Please make sure to monitor the discussion in that thread and to address comments and suggestions (while the comments and suggestions will be mirrored into the PR by GitGitGadget, you will still want to reply via mail). If you do not want to subscribe to the Git mailing list just to be able to respond to a mail, you can download the mbox from the Git mailing list archive (click the curl -g --user "<EMailAddress>:<Password>" \
--url "imaps://imap.gmail.com/INBOX" -T /path/to/raw.txtTo iterate on your change, i.e. send a revised patch or patch series, you will first want to (force-)push to the same branch. You probably also want to modify your Pull Request description (or title). It is a good idea to summarize the revision by adding something like this to the cover letter (read: by editing the first comment on the PR, i.e. the PR description): To send a new iteration, just add another PR comment with the contents: Need help?New contributors who want advice are encouraged to join git-mentoring@googlegroups.com, where volunteers who regularly contribute to Git are willing to answer newbie questions, give advice, or otherwise provide mentoring to interested contributors. You must join in order to post or view messages, but anyone can join. You may also be able to find help in real time in the developer IRC channel, |
|
/allow |
|
User shatachandra is now allowed to use GitGitGadget. WARNING: shatachandra has no public email address set on GitHub; GitGitGadget needs an email address to Cc: you on your contribution, so that you receive any feedback on the Git mailing list. Go to https://github.com/settings/profile to make your preferred email public to let GitGitGadget know which email address to use. |
|
Error: Could not determine full name of shatachandra |
Author
|
/preview |
|
Preview email sent as pull.2045.git.1770737366590.gitgitgadget@gmail.com |
Author
|
/submit |
|
Submitted as pull.2045.git.1770737573475.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
There are issues in commit 10a5d1a:
|
shatachandra
force-pushed
the
pre-add-hooks
branch
from
10a5d1a to
7edbac4
Compare
Author
|
Fixed the issue causing the build to fail for the CI checks, my apologies for the oversight. /submit |
|
Junio C Hamano wrote on the Git mailing list (how to reply to this email): "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 deny 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 wrote on the Git mailing list (how to reply to this email): 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 deny 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 original 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. |
gitgitgadget
bot
force-pushed
the
seen
branch
2 times, most recently
from
c6d7b35 to
c9d0931
Compare
shatachandra
force-pushed
the
pre-add-hooks
branch
from
7edbac4 to
1024415
Compare
Author
|
/submit |
|
Submitted as pull.2045.v2.git.1770822312474.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
Chandra wrote on the Git mailing list (how to reply to this email): 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 original 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 deny 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 original 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.
>
> |
|
Junio C Hamano wrote on the Git mailing list (how to reply to this email): "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 original 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 original 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"));
> + } |
gitgitgadget
bot
force-pushed
the
seen
branch
from
c9d0931 to
b6057ef
Compare
|
Chandra wrote on the Git mailing list (how to reply to this email): > 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 original 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 original 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 original 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"));
> > + }
>
> |
|
Junio C Hamano wrote on the Git mailing list (how to reply to this email): 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. |
|
Chandra wrote on the Git mailing list (how to reply to this email): 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.
|
gitgitgadget
bot
force-pushed
the
seen
branch
4 times, most recently
from
3b32210 to
368609b
Compare
gitgitgadget
bot
force-pushed
the
seen
branch
from
368609b to
f07777c
Compare
gitgitgadget
bot
force-pushed
the
seen
branch
4 times, most recently
from
7287b02 to
22c0122
Compare
"git add" has no hook that lets users inspect what is about to be staged. Users who want to reject certain paths or content must wrap the command in a shell alias or wait for pre-commit, which fires too late to prevent staging. Introduce a "pre-add" hook that runs after "git add" computes the new index state but before committing it to disk. The hook receives two positional arguments: $1 -- index path used by this invocation (may not exist yet) $2 -- lockfile path containing proposed staged index state While the lockfile is active the current index path remains readable and unchanged, so a seperate copy is unnecessary. Hook authors can inspect the computed result with ordinary tools: GIT_INDEX_FILE="$2" git diff --cached --name-only HEAD without needing to interpret pathspec or mode flags as the proposed index already reflects their effect. At the finish label, write_locked_index() writes the proposed index to the lockfile without COMMIT_LOCK so commit_lock_file() can be called seperately after the hook runs. However, do_write_locked_index() unconditionally fires post-index-change after every write, and the existing test suite (t7113) asserts that index.lock does not exist when that hook fires. Tying the hook to COMMIT_LOCK would suppress it for other callers that depend on it after a non-committed write (e.g., prepare_to_commit() in builtin/commit.c). A new SKIP_INDEX_CHANGE_HOOK flag lets builtin/add.c suppress the automatic notification on just this call, then emit post-index-change manually after commit_lock_file() publishes the new index. If the hook rejects, rollback_lock_file() discards the lockfile and the original index is left unchanged. When no hook is installed the existing write_locked_index(COMMIT_LOCK | SKIP_IF_UNCHANGED) path is taken. The hook gate checks cache_changed regardless of exit_status so that mixed-result adds (e.g., a tracked modification combined with an ignored path) still run the hook when index content changes. The hook is bypassed with "--no-verify" and is not invoked for --interactive, --patch, --edit, or --dry-run, nor by "git commit -a" which stages through its own code path. Signed-off-by: Chandra Kethi-Reddy <chandrakr@pm.me>
shatachandra
force-pushed
the
pre-add-hooks
branch
from
1024415 to
d0fb5f9
Compare
|
Chandra wrote on the Git mailing list (how to reply to this email): Thanks again for the review. I prepared v3 with the lockfile argument model, a mixed-result gating fix, a 'post-index-change' contract fix, and docs updates.
Chandra Kethi-Reddy
@archonphronesis:matrix.org
Sent from Proton Mail for iOS.
-------- Original Message --------
On Thursday, 02/12/26 at 03:25 Chandra <Chandrakr@pm.me> wrote:
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.
|
gitgitgadget
bot
force-pushed
the
seen
branch
9 times, most recently
from
994379e to
60f55de
Compare
Author
|
/submit |
|
Submitted as pull.2045.v3.git.1772171692465.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
gitgitgadget
bot
force-pushed
the
seen
branch
4 times, most recently
from
6267072 to
bfe49b6
Compare
|
Junio C Hamano wrote on the Git mailing list (how to reply to this email): "Chandra Kethi-Reddy via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> "git add" has no hook that lets users inspect what is about to be
> staged. Users who want to reject certain paths or content must
> wrap the command in a shell alias or wait for pre-commit, which
> fires too late to prevent staging.
I do not think the above would convince readers that "preventing to
add" is a worthy goal in the first place. If you "git add foo" by
mistake and wish you had this hook to prevent 'foo' from getting
added ever, you can easily "git reset foo" to undo it.
> Introduce a "pre-add" hook that runs after "git add" computes the
> new index state but before committing it to disk. The hook
> receives two positional arguments:
>
> $1 -- index path used by this invocation (may not exist yet)
> $2 -- lockfile path containing proposed staged index state
OK, perhaps.
> While the lockfile is active the current index path remains readable
> and unchanged, so a seperate copy is unnecessary.
Unless readers may think that it is needed to make a separate copy
in order to prevent "git add" from happening, and I am not sure why
we would expect readers to do so, this is not something we need to
say here, is it, even thought it might not be telling any lies?
What is more important would be to tell readers that these two index
files are meant to be read-only and hooks are not expected to modify
them.
> Hook authors can
> inspect the computed result with ordinary tools:
>
> GIT_INDEX_FILE="$2" git diff --cached --name-only HEAD
>
> without needing to interpret pathspec or mode flags as the proposed
> index already reflects their effect.
Good.
> At the finish label, write_locked_index() writes the proposed index
> to the lockfile without COMMIT_LOCK so commit_lock_file() can be
> called seperately after the hook runs. However, do_write_locked_index()
> unconditionally fires post-index-change after every write, and ...
Are these implementation details really needed to be described here
for future developers to understand what this change was while they
read the "git log -p" output and find this commit?
> the
> existing test suite (t7113) asserts that index.lock does not exist when
> that hook fires. Tying the hook to COMMIT_LOCK would suppress it for
> other callers that depend on it after a non-committed write (e.g.,
> prepare_to_commit() in builtin/commit.c). A new SKIP_INDEX_CHANGE_HOOK
> flag lets builtin/add.c suppress the automatic notification on just this
> call, then emit post-index-change manually after commit_lock_file()
> publishes the new index. If the hook rejects, rollback_lock_file()
> discards the lockfile and the original index is left unchanged. When
> no hook is installed the existing write_locked_index(COMMIT_LOCK |
> SKIP_IF_UNCHANGED) path is taken.
IOW, what does it help the reader to read the above wall of text?
> The hook gate checks cache_changed regardless of exit_status so that
> mixed-result adds (e.g., a tracked modification combined with an
> ignored path) still run the hook when index content changes.
>
> The hook is bypassed with "--no-verify" and is not invoked for
> --interactive, --patch, --edit, or --dry-run, nor by "git commit -a"
> which stages through its own code path.
>
> Signed-off-by: Chandra Kethi-Reddy <chandrakr@pm.me>
> ---
>
> Documentation/git-add.adoc | 11 +-
> Documentation/githooks.adoc | 30 ++++
> builtin/add.c | 47 +++++-
> read-cache-ll.h | 1 +
> read-cache.c | 13 +-
> t/meson.build | 1 +
> t/t3706-pre-add-hook.sh | 289 ++++++++++++++++++++++++++++++++++++
> 7 files changed, 381 insertions(+), 11 deletions(-)
> create mode 100755 t/t3706-pre-add-hook.sh
>
> diff --git a/Documentation/git-add.adoc b/Documentation/git-add.adoc
> index 6192daeb03..b47751acca 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]
Avoid making the line that is already overly long even worse.
> @@ -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].
I think the above (as with everything else you wrote in the patch,
including a part of the proposed commit log message) stresses too
much more on the implementation detail than what would help your
intended readers. How about writing it more like this?
The `pre-add` hook, if exists, is run with a temporary index
file that shows the result of proposed `git add` to inspect. By
exiting with non-zero status, the hook can reject the proposed
changes. If the hook exits with zero status, this temporary
index file will become the final result.
The readers do not have to know 'lockfile' or 'final index'. They
would want to know how to accept or reject the proposed result.
Or we can leave all the details to linkgit:githooks[5] and say
only something like this
A `pre-add` hook can be used to reject `git add`; see
linkgit:githooks[5].
and nothing else.
> diff --git a/Documentation/githooks.adoc b/Documentation/githooks.adoc
> index 056553788d..657e14d306 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`.
> +
> +It takes two parameters: the path to the index file for this invocation
Elsewhere you called these two files "arguments" but here you say
"parameters". Let's be consistent.
> +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.
> +
> +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.
Good write-up.
> +Git does not set `GIT_INDEX_FILE` for this hook.
I am not sure what the point of mentioning GIT_INDEX_FILE here. If
the user did
$ GIT_INDEX_FILE=.git/alt-index git add files...
the "git" process has the environment variable in place, pointing at
the file as _the_ index file to add to. We do not unset and
unexport the environment variable before invoking the hook, do we?
We simply do not do anything special or strange. It makes it less
confusing if we refrain from saying "we do not do this unusual thing
or that special thing", doesn't it?
> Hook authors may
> +set `GIT_INDEX_FILE="$1"` to inspect current index state and
> +`GIT_INDEX_FILE="$2"` to inspect proposed index state.
Explaining this one does make sense. "current" -> "the current"
and "proposed" -> "the proposed", I think.
Somewhere around here, it would be necessary to say that these two
files should be treated as read-only by this hook.
> + int run_pre_add = 0;
> + char *orig_index_path = NULL;
>
> repo_config(repo, add_config, NULL);
>
> @@ -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));
> + }
> +
> transaction = odb_transaction_begin(repo->objects);
>
> ps_matched = xcalloc(pathspec.nr, 1);
> @@ -587,8 +598,10 @@ int cmd_add(int argc,
> include_sparse, flags);
>
> if (take_worktree_changes && !add_renormalize && !ignore_add_errors &&
> - report_path_error(ps_matched, &pathspec))
> + report_path_error(ps_matched, &pathspec)) {
> + free(orig_index_path);
> exit(128);
> + }
Hmph, we are not releasing ps_matched or transaction and nothing is
leaking (the on-stack variables do hold references to these
resources). I do not see much point in releasing orig_index_path
here.
> @@ -598,9 +611,35 @@ int cmd_add(int argc,
> odb_transaction_commit(transaction);
>
> finish:
> - if (write_locked_index(repo->index, &lock_file,
> - COMMIT_LOCK | SKIP_IF_UNCHANGED))
> - die(_("unable to write new index file"));
> + if (run_pre_add && repo->index->cache_changed) {
> + struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT;
> +
> + if (write_locked_index(repo->index, &lock_file,
> + SKIP_INDEX_CHANGE_HOOK))
> + die(_("unable to write proposed index"));
As we _may_ allow the pre-add hook to reject it, we do not know if
the index has changed. So delaying the post-index-change hook until
we know for sure that we will commit to the index change does make
perfect sense.
> + strvec_push(&opt.args, 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;
And then we ask the new hook, which may reject the update, in which
case we leave here. Otherwise ...
> + } else if (commit_lock_file(&lock_file)) {
> + die(_("unable to write new index file"));
> + } else {
... we commit the index file to the final place and then invoke the
post-index-change hook ourselves, as we told write_locked_index()
not to do that earlier. Makes sense.
> + run_hooks_l(repo, "post-index-change",
> + repo->index->updated_workdir ? "1" : "0",
> + repo->index->updated_skipworktree ? "1" : "0",
> + NULL);
> + }
> + repo->index->updated_workdir = 0;
> + repo->index->updated_skipworktree = 0;
Doesn't these two belong to the "run post-index-change hook" block?
I think all the contents in the final "else {}" block that you
copied from do_write_locked_index() should be refactored into a
small helper function and called from here and also from
do_write_locked_index(). Otherwise, you'll be forced to maintain
the details of what needs to happen when running "post-index-change"
at multiple places and they must be kept in sync.
> + } else {
> + if (write_locked_index(repo->index, &lock_file,
> + COMMIT_LOCK | SKIP_IF_UNCHANGED))
> + die(_("unable to write new index file"));
> + }
> +
> + free(orig_index_path);
>
> free(ps_matched);
> dir_clear(&dir); |
|
Ben Knoble wrote on the Git mailing list (how to reply to this email): > Le 3 mars 2026 à 18:06, Junio C Hamano <gitster@pobox.com> a écrit :
>
> "Chandra Kethi-Reddy via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
>> "git add" has no hook that lets users inspect what is about to be
>> staged. Users who want to reject certain paths or content must
>> wrap the command in a shell alias or wait for pre-commit, which
>> fires too late to prevent staging.
>
> I do not think the above would convince readers that "preventing to
> add" is a worthy goal in the first place. If you "git add foo" by
> mistake and wish you had this hook to prevent 'foo' from getting
> added ever, you can easily "git reset foo" to undo it.
It’s also not clear to me how the proposed hook could inspect “git add A B” and reject A but permit B, but maybe that’s a non-goals.
>> diff --git a/Documentation/githooks.adoc b/Documentation/githooks.adoc
>> index 056553788d..657e14d306 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`.
>> +
>> +It takes two parameters: the path to the index file for this invocation
>
> Elsewhere you called these two files "arguments" but here you say
> "parameters". Let's be consistent.
>
>> +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.
Saying “it [the hook] does not read from standard in” feels proscriptive rather than descriptive. Why couldn’t I write a short script that asked for confirmation of the paths being added via stdin?
Or perhaps we mean that Git does not write anything to the hook’s stdin… at which point I wonder if Junio’s “let’s not mention that we don’t do this unusual thing” applies? I haven’t looked at how the rest of our documentation describes hooks that aren’t fed input via stdin. |
|
User |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
pre-addinputs to stable paths ($1index,$2lockfile) and removes copy-specific tempfile logicgit addreturned non-zeroSKIP_INDEX_CHANGE_HOOKflag towrite_locked_index()so thatpost-index-changeis not fired while the lockfile is still on diskNotes
cc: Ben Knoble ben.knoble@gmail.com