Subject: Re: [PATCH v7 05/17] hook: respect hook.runHookDir

From: Emily Shaffer <[email-masked]>
Date: Tue, 9 Feb 2021 14:31:29 -0800

On Sat, Jan 30, 2021 at 07:35:03PM -0800, Jonathan Tan wrote:
> 
> > Include hooks specified in the hook directory in the list of hooks to
> > run. These hooks do need to be treated differently from config-specified
> > ones - they do not need to run in a shell, and later on may be disabled
> > or warned about based on a config setting.
> > 
> > Because they are at least as local as the local config, we'll run them
> > last - to keep the hook execution order from global to local.
> 
> This commit message doesn't seem to match the code change. Firstly,
> we're teaching hook.runHookDir, not respecting it (since it did not
> exist before this commit), and it's about showing it in "list" and not
> about running it at all.

Yeah, thanks for noticing. Now that I'm rereading it, it looks like this
is the old commit message for "include hookdir hook in list". Yikes.
> 
> Perhaps just "hook: teach hook.runHookDir" as the subject and as the
> body:
> 
>   For now, this just affects the output of "git hook list". In the
>   future, this will affect the behavior of "git hook run" and when Git
>   runs hooks before or after its operations.
> 
> > +	switch (should_run_hookdir) {
> > +		case HOOKDIR_NO:
> > +			strbuf_addstr(&hookdir_annotation, _(" (will not run)"));
> > +			break;
> > +		case HOOKDIR_INTERACTIVE:
> > +			strbuf_addstr(&hookdir_annotation, _(" (will prompt)"));
> > +			break;
> > +		case HOOKDIR_WARN:
> > +		case HOOKDIR_UNKNOWN:
> 
> Hmm...UNKNOWN is the same as WARN? This doesn't agree with what is said
> in patch 1:
> 
>   +In case this list is expanded in the future, if a value for `hook.runHookDir` is
>   +given which Git does not recognize, Git should discard that config entry. For
>   +example, if "warn" was specified at system level and "junk" was specified at
>   +global level, Git would resolve the value to "warn"; if the only time the config
>   +was set was to "junk", Git would use the default value of "yes".
> 
> But having said that, I would prefer if Git just errored out in this
> case.

Eh, I think I'd still like it to be tolerant. Thanks for keeping me
honest :) I added a test to enforce the tolerant behavior, too.

> The rest looks good.

Thanks.