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

> 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).

>> > +	/* 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.