Subject: Re: [PATCH 8/9] hook: teach 'hookcmd' config to alias hook scripts

From: Emily Shaffer <[email-masked]>
Date: Thu, 22 Jul 2021 16:31:48 -0700

On Fri, Jul 16, 2021 at 11:13:42AM +0200, Ævar Arnfjörð Bjarmason wrote:
> 
> 
> On Thu, Jul 15 2021, Emily Shaffer wrote:
> 
> > To enable fine-grained options which apply to a single hook executable,
> > and to make it easier for a single executable to be run on multiple hook
> > events, teach "hookcmd.<alias>.config". These can be configured as
> > follows:
> > [...]
> > diff --git a/Documentation/config/hook.txt b/Documentation/config/hook.txt
> > index a97b980cca..5b35170664 100644
> > --- a/Documentation/config/hook.txt
> > +++ b/Documentation/config/hook.txt
> > @@ -3,6 +3,11 @@ hook.<command>.command::
> >  	executable on your device, a oneliner for your shell, or the name of a
> >  	hookcmd. See linkgit:git-hook[1].
> >  
> > +hookcmd.<name>.command::
> > +	A command to execute during a hook for which <name> has been specified
> > +	as a command. This can be an executable on your device or a oneliner for
> > +	your shell. See linkgit:git-hook[1].
> > +
> > [...]
> > +Global config
> > +----
> > +  [hook "post-commit"]
> > +    command = "linter"
> > +    command = "~/typocheck.sh"
> > +
> > +  [hookcmd "linter"]
> > +    command = "/bin/linter --c"
> > +----
> > +
> > +Local config
> > +----
> > +  [hook "prepare-commit-msg"]
> > +    command = "linter"
> > +  [hook "post-commit"]
> > +    command = "python ~/run-test-suite.py"
> > +----
> > +
> > +With these configs, you'd then run post-commit hooks in this order:
> > +
> > +  /bin/linter --c
> > +  ~/typocheck.sh
> > +  python ~/run-test-suite.py
> > +  .git/hooks/post-commit (if present)
> > +
> > +and prepare-commit-msg hooks in this order:
> > +
> > +  /bin/linter --c
> > +  .git/hooks/prepare-commit-msg (if present)
> >  
> 
> I still have outstanding feedback on the fundamental design
> here. I.e. why is this not:
> 
>     hook.<name>.event = post-commit
>     hook.<name>.command = <path>
> 
> See:
> 
> https://lore.kernel.org/git/[email-masked]/
> 
> As noted there I don't see why not, and more complexity results from the
> design choice of doing it the way you're proposing. I.e. we can't
> discover hooks based on config prefixes, and we end up sticking full FS
> paths in config keys.

Interesting. My gut says that it would make it harder to neatly write a
config with the same hook running at the very beginning of one event and
the very end of another, but I'm not sure whether that's a case to
optimize for.

I would also need twice as many lines to run a script I wrote as a hook
- that is, the base case which is probably all most people will need. So
with your proposal I *must* name every hook I want to add. Instead of
"hook.pre-commit.command = ~/check-for-debug-strings" I need to
configure "hook.check-debug-strings.event = pre-commit" and
"hook.check-debug-strings.command = ~/check-for-debug-strings". That's a
little frustrating, and if I never want to skip that check or move it
later or something, it will be extra overhead for no gain for me.

I do agree that your approach bans the regrettably awkward
"hookcmd.~/check-for-debug-strings.skip = true", but I'm not sure
whether or not it's worth it.

To help us visualize the change, here's some common and complicated
tasks and how they look with either schema (let mine be A and yours be
B):

#1 - Add a oneliner (not executing a script)
A:
[hook "post-commit"]
	command = echo post commit
B:
[hook "oneliner"]
	event = post-commit
	command = echo post commit

#2 - Execute a script
A:
[hook "post-commit"]
	command = ~/my-post-commit-hook
B:
[hook "script"]
	event = post-commit
	command = ~/my-post-commit-hook

#3 - Run a handful of scripts in a specific order on one event
A:
[hook "post-commit"]
	command = ~/my-post-commit-1
	command = ~/my-post-commit-2
	command = ~/my-post-commit-3
B:
[hook "script 1"]
	event = post-commit
	command = ~/my-post-commit-1
[hook "script 2"]
	event = post-commit
	command = ~/my-post-commit-2
[hook "script 3"]
	event = post-commit
	command = ~/my-post-commit-3

#4 - Skip a script configured more globally
A:
(original config)
[hook "post-commit"]
	command = /corp/stuff/corp-post-commit
(local config)
[hookcmd "/corp/stuff/corp-post-commit"]
	skip = true
OR,
(original config)
[hookcmd "corp-pc"]
	command = /corp/stuff/corp-post-commit
[hook "post-commit"]
	command = corp-pc
(local config)
[hookcmd "corp-pc"]
	skip = true
B:
(original config)
[hook "corp-pc"]
	event = post-commit
	command = /corp/stuff/corp-post-commit
(local config)
[hook "corp-pc"]
	skip = true

#5 - Execute one script for multiple events
A:
[hookcmd "reusable-hook"]
	command = /long/path/reusable-hook
[hook "post-commit"]
	command = reusable-hook
[hook "pre-commit"]
	command = reusable-hook
[hook "prepare-commit-msg"]
	command = reusable-hook
B:
[hook "reusable-hook"]
	command = /long/path/reusable-hook
	event = post-commit
	event = pre-commit
	event = prepare-commit-msg

#6 - Execute the same script early for one event and late for another
event
A:
(global)
[hookcmd "reusable-hook"]
	command = /long/path/reusable-hook
[hook "pre-commit"]
	command = reusable-hook
(local)
[hook "post-commit"]
	command = other
	command = hooks
	command = reusable-hook

B:
(global)
[hook "reusable-hook"]
	command = /long/path/reusable-hook
	event = pre-commit
(local)
[hook "other"]
	event = post-commit
	command = other
[hook "hooks"]
	event = post-commit
	command = hooks
[hook "reusable-hook"]
	event = reusable-hook

I'm not going to comment on which one I like more yet - I think I will
study this for a while and let others weigh in on their preferences too.
I tried to order the cases from most common to less common. Please feel
free to chime in with more use cases that you think would be handy which
I forgot :)

 - Emily