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

From: Emily Shaffer <[email-masked]>
Date: Thu, 22 Jul 2021 15:51:33 -0700

On Fri, Jul 16, 2021 at 11:01:24AM +0200, Ævar Arnfjörð Bjarmason wrote:
> 
> 
> On Thu, Jul 15 2021, Emily Shaffer wrote:
> 
> > +static struct hook * find_hook_by_command(struct list_head *head, const char *command)
> 
> nit: "*find[...]" not "* find[...]", also let's wrap the long line.
ACK
> 
> > +{
> > +	struct list_head *pos = NULL, *tmp = NULL;
> > +	struct hook *found = NULL;
> > +
> > +	list_for_each_safe(pos, tmp, head) {
> > +		struct hook *it = list_entry(pos, struct hook, list);
> > +		if (!strcmp(it->command, command)) {
> > +		    list_del(pos);
> > +		    found = it;
> > +		    break;
> 
> Indented with spaces.

I don't know how I even did this. *facepalm*

> 
> Also is there some subtlety in the list macro here or can we just
> "s/break/return it/" and skip the break/return pattern?

I guess it's probably fine, but we'd need the final return anyway
("otherwise returns NULL"). IMO one return is more readable than two
returns, so I'd rather leave this.
> 
> > +static struct hook * append_or_move_hook(struct list_head *head, const char *command)
> 
> Same whitespace nits.
ACK
> 
> > +	if (!to_add) {
> > +		/* adding a new hook, not moving an old one */
> > +		to_add = xmalloc(sizeof(*to_add));
> > +		to_add->command = command;
> > +		to_add->feed_pipe_cb_data = NULL;
> > +		/* This gets overwritten in hook_list() for hookdir hooks. */
> > +		to_add->from_hookdir = 0;
> 
> I commented on init verbosity elsewhere, i.e. we could do some things
> via macros, but in this case just having an "init" helper make sense,
> but we have at least two places copying the same init of all fields,
> should just be hook_init_hook() or whatever it'll be called. Maybe with
> a second "from hookdir" param?

Hm, where is the second place where we init everything? I think with
this commit we remove anywhere we're putting together a 'struct hook' manually
except during this helper? Hooks from hookdir are initted by
'append_or_move_hook()'ing them to the end of the list and modifying the
from_hookdir field, and builtin/hook.c just calls hook_list() (and some
list.h helpers to find an entry).

> 
> > +	if (!strcmp(key, hook_key)) {
> > +		const char *command = value;
> > +		struct strbuf hookcmd_name = STRBUF_INIT;
> > +
> > +
> 
> Nit: 3x\n, not 2x\n
> 
> > +		if (!command) {
> > +			strbuf_release(&hookcmd_name);
> 
> You don't need to strbuf_release() things that you haven't done anything
> except init'd, but also...
> 
> > +			BUG("git_config_get_value overwrote a string it shouldn't have");
> 
> ...even if that were the case and it called malloc() memory diligence
> when we're calling BUG() is probably going overboard, and I say that as
> someone who'll tend to go overboard with it :)

:) ok.

> 
> > +		}
> > +
> > +		/* TODO: implement skipping hooks */
> > +
> > +		/* TODO: immplement hook aliases */
> > +
> > +		/*
> > +		 * TODO: implement an option-getting callback, e.g.
> > +		 *   get configs by pattern hookcmd.$value.*
> > +		 *   for each key+value, do_callback(key, value, cb_data)
> > +		 */
> 
> I think we should drop the TODO and just let the commit message /
> comments speak to what we actually implement, and subsequent patches can
> add more features.

Ok, sure.

> 
> > -
> > +	struct strbuf hook_key = STRBUF_INIT;
> > +	struct hook_config_cb cb_data = { &hook_key, hook_head };
> 
> Let's use designated initializers.
ACK
> 
> >  
> >  	INIT_LIST_HEAD(hook_head);
> >  
> >  	if (!hookname)
> >  		return NULL;
> >  
> > +	/* Add the hooks from the config, e.g. hook.pre-commit.command */
> > +	strbuf_addf(&hook_key, "hook.%s.command", hookname);
> > +	git_config(hook_config_lookup, &cb_data);
> > +
> > +
> 
> Another 3x\n
ACK
> 
> > +	/* to enable oneliners, let config-specified hooks run in shell */
> > +	cp->use_shell = !run_me->from_hookdir;
> 
> I've lost track at this point, but doesn't that mean we're going to use
> a shell when we run our own do-not-need-a-shell hooks ourselves?
> 
> Isn't isatty() more appropriate here, or actually even interactively why
> is the shell needed (maybe this is answered elswhere...).

use_shell means "conditionally guess whether I need to wrap this thing
in `sh -c`" - it doesn't have anything to do with TTY or not. So we need
this for something like `hook.post-commit.command = echo "made a
commit"`. In this case the entire argv[0] will be the oneliner, which
you will need use_shell set for. If we *do* just do something simple,
like `hook.post-commit.command = /bin/mail`, even though
use_shell is marked, the child_process runner will notice that there's
no reason to wrap in 'sh -c' and so will just run the /bin/mail
executable directly.

 - Emily