Subject: Re: [PATCH v4 8/9] commit: use config-based hooks

From: Jonathan Tan <[email-masked]>
Date: Wed, 23 Sep 2020 16:47:34 -0700

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