Subject: Re: [PATCH v2 5/6] hook: include hooks from the config

From: Emily Shaffer <[email-masked]>
Date: Wed, 18 Aug 2021 17:09:36 -0700

On Thu, Aug 12, 2021 at 01:48:00PM -0700, Junio C Hamano wrote:
> 
> Emily Shaffer <[email-masked]> writes:
> 
> > Teach the hook.[hc] library to parse configs to populare the list of
> > hooks to run for a given event.
> >
> > Multiple commands can be specified for a given hook by providing
> > multiple "hook.<friendly-name>.command = <path-to-hook>" and
> > "hook.<friendly-name>.event = <hook-event>" lines. Hooks will be run in
> > config order of the "hook.<name>.event" lines.
> >
> > For example:
> >
> >   $ git config --list | grep ^hook
> >   hook.bar.command=~/bar.sh
> >   hook.bar.event=pre-commit
> 
> Your answer might be "read the design doc", but it is unclear to me
> why "bar" (friendly-name) is needed in this picture at all.  Is it
> because you may want to fire more than one command for pre-commit
> event?  IOW,
> 
> 	[hook "bar"]
> 		command = bar1.sh
> 		command = bar2.sh
> 		event = pre-commit
> 
> is easier to manage with an extra level of redirection?  I doubt it
> as 
> 
> 	[hook "pre-commit"]
> 		command = bar1.sh
> 		command = bar2.sh
> 
> would be equally expressive and shorter.  Or would it help use case
> for multiple "friendly-name" to refer to the same "event", e.g.
> 
> 	[hook "xyzzy"]
> 		event = pre-commit
> 		command = xyzzy1
> 
> 	[hook "frotz"]
> 		event = pre-commit
>                 command = frotz1
>                 command = frotz2
> 
> or something?  I am not sure if this gives us useful extra
> flexibility, and if so, the extra flexibility helps us more than it
> confuses us.
> 
> And moving the "event" to the second level in the configuration
> hierarchy, getting rid of "friendly-name" from the design, would not
> make this example unworkable, either:
> 
> >   $ git hook run
> >   # Runs ~/bar.sh
> >   # Runs .git/hooks/pre-commit
> 
> Again, this is not an objection wrapped in a rhetorical question.
> It just is that I do not see how the extra level of redirection
> helps us.

Please have a look at
https://lore.kernel.org/git/87fswey5wd.fsf%40evledraar.gmail.com and
replies, where Ævar and I discussed the schema change at length. I know
it is a lot of back and forth but I think it is useful to understand why
I ended up changing the schema this way.

> 
> > diff --git a/Documentation/config/hook.txt b/Documentation/config/hook.txt
> > index 96d3d6572c..a97b980cca 100644
> > --- a/Documentation/config/hook.txt
> > +++ b/Documentation/config/hook.txt
> > @@ -1,3 +1,8 @@
> > +hook.<command>.command::
> > +	A command to execute during the <command> hook event. This can be an
> > +	executable on your device, a oneliner for your shell, or the name of a
> > +	hookcmd. See linkgit:git-hook[1].
> 
> Please make sure you use the terminology consistently.  If the
> second level is "friendly name", hook.<name>.command should be
> described, instead of hook.<command>.command.

Thanks, this is an oversight. Will update the config/hook.txt doc in
next reroll.

> 
> Also, to help those who are familiar with the current Git from their
> use in the past 10 years or so, giving an example name from the
> current system may help, e.g. when describing hook.<name>.event,
> you may want to say the values are things like "pre-commit",
> "receive", etc.

Sure.

> 
> > +This command parses the default configuration files for pairs of configs like
> > +so:
> > +
> > +  [hook "linter"]
> > +    event = pre-commit
> > +    command = ~/bin/linter --c
> 
> The above addition of .command should also have hook.<name>.event
> next to it, no?

I don't understand the question. Doesn't this config snippet equate to
"""
  hook.linter.event=pre-commit
  hook.linter.command=~/bin/linter --c
"""
? So in this case, '<name>' is 'linter', as that's not a native Git hook.

> 
> > +Conmmands are run in the order Git encounters their associated
> 
> "Conmmands -> Commands", I would think.
ACK
> 
> > +`hook.<name>.event` configs during the configuration parse (see
> > +linkgit:git-config[1]).
> 
> Here you use <name>, which should be matched by the description in
> the first hunk of the patch to this file.
Yep.
> 
> > +In general, when instructions suggest adding a script to
> > +`.git/hooks/<hook-event>`, you can specify it in the config instead by running
> > +`git config --add hook.<some-name>.command <path-to-script> && git config --add
> > +hook.<some-name>.event <hook-event>` - this way you can share the script between
> > +multiple repos. That is, `cp ~/my-script.sh ~/project/.git/hooks/pre-commit`
> > +would become `git config --add hook.my-script.command ~/my-script.sh && git
> > +config --add hook.my-script.event pre-commit`.
> 
> One repository may use a friendly name "xyzzy" while the other may
> use "frotz" to group the hooks that trigger upon "pre-commit" event,
> but unless one of the repositories change the friendly name to
> match, they cannot share these configurations, no?  It seems that an
> extra level of indirection is hindering sharing, rather than
> helping.

Ah, I think this means the documentation isn't sufficient, if you are
asking that. Instead of explaining it in ephemeral email, I think it is
better for me to explain it in documentation reroll, and for you to then
tell me how you interpret it. I expect to send the reroll before I go
home today, since I didn't receive comments from anybody besides you so
far.

Thanks very much for the feedback on the doc - this is very useful.

 - Emily