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

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

> 
> > +{
> > +	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.

> 
> > +	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?

> 
> > +	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 :)

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

> 
> > -
> > +	struct strbuf hook_key = STRBUF_INIT;
> > +	struct hook_config_cb cb_data = { &hook_key, hook_head };
> 
> Let's use designated initializers.

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

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