Re: [PATCH v2 6/7] config: allow configuration of multiple hook error behavior

From: brian m. carlson <sandals@crustytoothpaste.net>
Date: May 16, 2019, 17:19 UTC
Message-ID: <20190516171953.GA265608@genre.crustytoothpaste.net>
In-Reply-To: <20190516050200.GB7241@sigill.intra.peff.net>

On 2019-05-16 at 05:02:00, Jeff King wrote:

Show 15 quoted lines

> > +static int git_default_hook_config(const char *key, const char *value)
> > +{
> > +	const char *hook;
> > +	size_t key_len;
> > +	uintptr_t behavior;
> > +
> > +	key += strlen("hook.");
> > +	if (strip_suffix(key, ".errorbehavior", &key_len)) {
> 
> There's an undocumented assumption that the caller has confirmed that
> the key starts with "hook." here. Can we be a little more defensive and
> do:
> 
>   if (skip_prefix(key, "hook.", &key))
> 	return 0;

Yeah, the caller checks that, but I think being a little more defensive is fine.

Show 7 quoted lines

> here (we could even drop the check in git_default_config).
> 
> Or we could use parse_key(), which is designed for this:
> 
>   if (parse_key(key, "hook", &subsection, &subsection_len, &key) < 0 ||
>       !subsection)
> 	return 0;

Oh, good, I didn't know we had that. That's exactly what I want.

Show 8 quoted lines

>   if (!strcmp(key, "errorbehavior"))
> 	...
> 
> > +	/* Use -2 as sentinel because failure to exec is -1. */
> > +	int ret = -2;
> 
> Maybe this would be simpler to follow by using an enum for the handler
> return value?

We can't make this variable an enum because we'd have to define 256 entries (well, we can, but it would be a hassle), but I can create an enum and assign it to the int variable, sure.

-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204