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.
