On Wed, Sep 23, 2020 at 04:47:34PM -0700, Jonathan Tan wrote:
>
> > - 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.
OK. I will send 'skip' in the next reroll. Thanks for pointing it out!
>
> - 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.
I personally prefer (3) - I think the user experience with (2) in a
release (or even in 'next', which all Googlers use) is pretty bad. The
downside, of course, is that a large topic gets merged all at once and
makes some pretty nasty reviewer overhead.
Junio, I wonder if you can give any advice here? What would be really
ideal for me would be to do something like Stolee has been doing with
his maintenance series - config-based hooks pt. I containing the library
code and config-based hooks pt. II containing the conversion of
preexisting hooks. Does that make the overhead for you significantly
worse?
- Emily
