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

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Mar 13, 2026, 14:39 UTC
Message-ID: <3b2b67b1-3748-4a95-9882-30e4ba349922@gmail.com>
In-Reply-To: <Mas-XsZDLQf822y8cXTnllJLDJcd9vU8jRd7i4tj-7pCw90hurfkTos1piH-zF-g9A-IPM2sIZoXac1MB2yHn9oU-nX9kaLeuI9bXWp3Fbw=@pm.me>

On 06/03/2026 02:20, Chandra wrote:

Show 5 quoted lines

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

Show 7 quoted lines

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

Show 32 quoted lines

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