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 [off-list ref] 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 [off-list ref] 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.