> - if (!no_verify && find_hook("pre-commit")) {
> + if (!no_verify && hook_exists("pre-commit")) {
A reviewer would probably need to look at all instances of "pre-commit"
(and likewise for the other hooks) but if the plan is to convert all
hooks, then the reviewer wouldn't need to do this since we could just
delete the "find_hook" function.
Overall comments about the design and scope of the patch set:
- I think that the abilities of the current patch set regarding
overriding order of globally-set hook commands is sufficient. We
should also have some way of disabling globally-set hooks, perhaps
by implementing the "skip" variable mentioned in patch 1 or by
allowing the redefinition of hookcmd sections (e.g. by redefining a
command to "/usr/bin/true"). To me, these provide substantial
user-facing value, and would be sufficient for a first version - and
other things like parallelization can come later.
- As for the UI that should be exposed through the "git hook" command,
I think that "git hook list" and "git hook run" are sufficient.
Editing the config files are not too difficult, and "git hook add"
etc. can be added later.
- As for whether (1) it is OK for none of the hooks to be converted (and
instead rely on the user to edit their hook scripts to call "git hook
run ???"), or if (2) we should require some hooks to be
converted, or if (3) we should require all hooks to be converted: I'd
rather have (2) or (3) so that we don't have dead code. I prefer (3),
especially since a reviewer wouldn't have to worry about leftover
usages of old functions like find_hook() (as I mentioned at the start
of this email), but I'm not fully opposed to (2) either.
