From: Chandra Date: Wed, 11 Feb 2026 21:11:15 GMT Subject: Re: [PATCH v2] add: support pre-add hook Message-ID: <2kX5wTQeOz3VPzUT6QiH_KyB9RMMtf8L3I8N6WtVWHaVQ1ZguBTaqAqFcFgOGpCqv-RJyALKlsENx-g7E3DMx3TzCfZoaRtPEpoDyx6d9kg=@pm.me> In-Reply-To: > 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 wrote: > "Chandra Kethi-Reddy via GitGitGadget" > 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= [--pathspec-file-nul]] > > [--] [...] > > > > @@ -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")); > > + } > >