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
signature.asc
Description: PGP signature
