Re: [PATCH v2] add: support pre-add hook
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Feb 11, 2026, 19:50 UTC
- Message-ID
- <xmqqseb7rre9.fsf@gitster.g>
- In-Reply-To
- <pull.2045.v2.git.1770822312474.gitgitgadget@gmail.com>
"Chandra Kethi-Reddy via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 24 quoted lines
> 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 ;-)
Show 13 quoted lines
> +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?
Show 27 quoted lines
> + 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.
Show 5 quoted lines
> + 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.
Show 15 quoted lines
> + 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"));
> + }