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