On Tue, May 14, 2019 at 12:23:31AM +0000, brian m. carlson wrote:
> There are a variety of situations in which a user may want an error
> behavior for multiple hooks other than the default. Add a config option,
> hook.<name>.errorBehavior to allow users to customize this behavior on a
> per-hook basis. Provide options for the default behavior (exiting
> early), executing all hooks and succeeding if all hooks succeed, or
> executing all hooks and succeeding if any hook succeeds.
Thanks for using that naming scheme. I think if we do move to allowing
config-based hooks, the config schemes will mesh together well.
> +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;
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;
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?
Aside from these nits, the code looked sensible to me.
-Peff
