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

["brian m. carlson" <sandals@crustytoothpaste.net>]

[-- Attachment #1: Type: text/plain, Size: 1419 bytes --]

On 2019-05-16 at 05:02:00, Jeff King wrote:
> > +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.

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

>   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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]