On Tue, Aug 24, 2021 at 05:55:13PM +0200, Ævar Arnfjörð Bjarmason wrote:
>
>
> On Wed, Aug 18 2021, Emily Shaffer wrote:
>
> > As the hook architecture and 'git hook run' become more featureful, we
> > may find wrappers wanting to use the hook architecture to run their own
> > hooks, thereby getting nice things like parallelism and idiomatic Git
> > configuration for free. Enable this by letting 'git hook run' bypass the
> > known_hooks() check.
> >
> > We do still want to keep known_hooks() around, though - by die()ing when
> > an internal Git call asks for run_hooks("my-new-hook"), we can remind
> > Git developers to update Documentation/githooks.txt with their new hook,
> > which in turn helps Git users discover this new hook.
> >
> > [...]
> >
> > +It's possible to use this command to refer to hooks which are not native to Git,
> > +for example if a wrapper around Git wishes to expose hooks into its own
> > +operation in a way which is already familiar to Git users. However, wrappers
> > +invoking such hooks should be careful to name their hook events something which
> > +Git is unlikely to use for a native hook later on. For example, Git is much less
> > +likely to create a `mytool-validate-commit` hook than it is to create a
> > +`validate-commit` hook.
> > +
> > SUBCOMMANDS
> > -----------
>
> The goal here makes sense, but...
>
> > diff --git a/builtin/hook.c b/builtin/hook.c
> > index d21f303eca..80397d39f5 100644
> > --- a/builtin/hook.c
> > +++ b/builtin/hook.c
> > @@ -46,7 +46,7 @@ static int list(int argc, const char **argv, const char *prefix)
> >
> > hookname = argv[0];
> >
> > - head = hook_list(hookname);
> > + head = list_hooks_gently(hookname);
> >
> > if (list_empty(head))
> > return 1;
> > @@ -105,7 +105,7 @@ static int run(int argc, const char **argv, const char *prefix)
> > git_config(git_default_config, NULL);
> >
> > hook_name = argv[0];
> > - hooks = list_hooks(hook_name);
> > + hooks = list_hooks_gently(hook_name);
> > if (list_empty(hooks)) {
> > /* ... act like run_hooks_oneshot() under --ignore-missing */
> > if (ignore_missing)
>
> This introduces a bug v.s. the previous state, e.g. before:
>
> $ git hook run --ignore-missing foobar
> fatal: the hook 'foobar' is not known to git, should be in hook-list.h via githooks(5)
>
> But after we'll silently ignore it. I.e. we've conflated
> --ignore-missing with a new and hypothetical (and this is now a synonym
> of) --ignore-missing-and-allow-unknown-hook-names.
>
> So we've conflated the user's one-shot "foobar" script with wanting to
> catch a typo in e.g. git-send-email.perl.
>
> Also instead of the user's typos being caught with a die (here using
> your BUG(...) version):
>
> $ git hook list pre-recive
> BUG: hook.c:115: Don't recognize hook event 'pre-recive'! Is it documented in Documentation/githooks.txt?
> Aborted
>
> We'll now silently return 1, so indistinguishabl from typing it properly
> as pre-receive.
>
> All that being said I think it's arguable that if we're going to allow
> "git hook run blahblah" that the die() in the base topic in my
> "hook-list.h: add a generated list of hooks, like config-list.h" is more
> trouble than it's worth.
>
> I.e. do we really need to be concerned about new hooks being added and
> someone forgetting a githooks.txt update, or a typo in the git.git code
> that nobody notices? Probably not.
>
> But I think the change here is clearly broken vis-a-vis the stated goals
> of its commit message as it stands, i.e. "[...]we do still want to keep
> known_hooks() around, though[...]". Should we fix it by adding a new
> internal-only flag to the command, or just saying we shouldn't have the
> behavior at all? What do you think.
I think it's A) pretty important to make it easy for users to run
whatever not-necessarily-git-native hook they want, and B) useful for
script Git commands to take advantage of the typo check. So, I'll add a
`--enforce-known-hookname` (or maybe a better named one, this isn't my
strong suit) and switch git-send-email and friends to use it. Like we
discussed off-list, I think it's a good idea to drop the envvar for
exceptional test names from the codebase entirely.
>
> Aside from that, this change seems to be untested, I tried making this
> non-gentle for testing, and all tests still passed. I.e. we don't have
> any tests for running such a hook like mytool-validate-commit, but
> should as part of this change.
Sure.
Actually, I was in the middle of typing about how I wouldn't change your
'test-hook' and so on tests, and it occurs to me that it might actually
be a better fit for your series to add this --reject-unknown (or
whatever) flag, instead of the envvar magics. So I'll hold off on making
any changes unless I hear from you.
- Emily
