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