| Message ID | 20190514002332.121089-8-sandals@crustytoothpaste.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Multiple hook support | expand |
On Tue, May 14, 2019 at 7:24 AM brian m. carlson <sandals@crustytoothpaste.net> 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 An alternative name is onError, probably more often used for event callbacks. But I don't know, maybe errorBehavior is actually better. > per-hook basis. Provide options for the default behavior (exiting should we fall back to hook.errorBehavior? That allows people to set global policy, then customize just a small set of weird hooks. > early), executing all hooks and succeeding if all hooks succeed, or > executing all hooks and succeeding if any hook succeeds. > Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net> > --- > config.c | 27 +++++++++++++ > run-command.c | 42 +++++++++++++++++--- > run-command.h | 5 +++ > t/lib-hooks.sh | 106 ++++++++++++++++++++++++++++++++++++++++++++++++- > 4 files changed, 173 insertions(+), 7 deletions(-) > > diff --git a/config.c b/config.c > index c2846df3f1..9cba4061a9 100644 > --- a/config.c > +++ b/config.c > @@ -19,6 +19,7 @@ > #include "utf8.h" > #include "dir.h" > #include "color.h" > +#include "run-command.h" > > struct config_source { > struct config_source *prev; > @@ -1093,6 +1094,29 @@ int git_config_color(char *dest, const char *var, const char *value) > return 0; > } > > +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)) { > + hook = xmemdupz(key, key_len); > + if (!strcmp(value, "stop-on-first")) maybe stop-on-first-error (or if you go with the "onError" name, I think "stop" is enough). I know "stop on/after first hook" does not really make any sense when you think about it. Maybe stop-on-first is sufficient. I was going to suggest strcasecmp. But core.whitespace (also has multiple-word-values) already sets a precedent on strcmp. I think we're good. Or mostly good, I don't know, we still accept False, false and FALSE. > + behavior = HOOK_ERROR_STOP_ON_FIRST; This is basically the logical "and" behavior in a C expression. Which makes me think if anybody's crazy enough to need the "or" counterpart (i.e. run hooks, expect failure, keep going until the first success). I guess it's a crazy mode. We should not care about until a real use case shows up. > + else if (!strcmp(value, "report-any-error")) I couldn't guess based on this name alone, whether we continue or stop after the reporting part. The 7/7 document makes it clear though. So all good. > diff --git a/run-command.c b/run-command.c > index 191d6f6f7e..70fb19a55b 100644 > --- a/run-command.c > +++ b/run-command.c > @@ -1308,6 +1308,8 @@ int async_with_fork(void) > #endif > } > > +struct string_list hook_error_behavior = STRING_LIST_INIT_NODUP; Maybe stick this in 'struct repository'. I know most config variables are still global. But I think we have to move/reorganize them at some point. Most may end up in 'struct repository'. > @@ -1401,18 +1403,48 @@ int for_each_hook(const char *name, > void *data) > { > struct string_list paths = STRING_LIST_INIT_DUP; > - int i, ret = 0; > + int i, hret = 0; > + uintptr_t behavior = HOOK_ERROR_STOP_ON_FIRST; > + struct string_list_item *item; > + /* Use -2 as sentinel because failure to exec is -1. */ > + int ret = -2; > + > + item = string_list_lookup(&hook_error_behavior, name); > + if (item) > + behavior = (uintptr_t)item->util; > > find_hooks(name, &paths); > for (i = 0; i < paths.nr; i++) { > const char *p = paths.items[i].string; > > - ret = handler(name, p, data); > - if (ret) > - break; > + hret = handler(name, p, data); > + switch (behavior) { > + case HOOK_ERROR_STOP_ON_FIRST: > + if (hret) { > + ret = hret; > + goto out; > + } > + break; > + case HOOK_ERROR_REPORT_ANY_SUCCESS: > + if (ret == -2) > + ret = 1; > + if (!hret) > + ret = 0; > + break; > + case HOOK_ERROR_REPORT_ANY_ERROR: > + if (ret == -2) > + ret = 0; > + if (hret) > + ret = hret; > + break; > + default: > + BUG("unknown hook error behavior"); maybe BUG(".. behavior %d", behavior); > + } > } > - > +out: > string_list_clear(&paths, 0); > + if (ret == -2) > + return 0; > return ret; > } >
On Tue, May 14, 2019 at 08:20:10PM +0700, Duy Nguyen wrote: > On Tue, May 14, 2019 at 7:24 AM brian m. carlson > <sandals@crustytoothpaste.net> 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 > > An alternative name is onError, probably more often used for event > callbacks. But I don't know, maybe errorBehavior is actually better. I'm going to use "errorStrategy", since we already have submodule.alternateErrorStrategy. > > per-hook basis. Provide options for the default behavior (exiting > > should we fall back to hook.errorBehavior? That allows people to set > global policy, then customize just a small set of weird hooks. Sure, that sounds good. > maybe stop-on-first-error (or if you go with the "onError" name, I > think "stop" is enough). I know "stop on/after first hook" does not > really make any sense when you think about it. Maybe stop-on-first is > sufficient. > > I was going to suggest strcasecmp. But core.whitespace (also has > multiple-word-values) already sets a precedent on strcmp. I think > we're good. Or mostly good, I don't know, we still accept False, false > and FALSE. I think with errorStrategy, "stop" is fine. Simpler is better. I literally picked what Peff had suggested in his email (mostly because I'm terrible at naming things), and I don't get the impression he spent a great deal of time analyzing the ins and outs of the names before sending. I could be wrong, though. > > + behavior = HOOK_ERROR_STOP_ON_FIRST; > > This is basically the logical "and" behavior in a C expression. Which > makes me think if anybody's crazy enough to need the "or" counterpart > (i.e. run hooks, expect failure, keep going until the first success). > > I guess it's a crazy mode. We should not care about until a real use > case shows up. Yeah, I think that's unlikely to be the case. Nothing prevents us from adding it later. > > + else if (!strcmp(value, "report-any-error")) > > I couldn't guess based on this name alone, whether we continue or stop > after the reporting part. The 7/7 document makes it clear though. So > all good. I'm open to hearing better suggestions if anyone has any. > > diff --git a/run-command.c b/run-command.c > > index 191d6f6f7e..70fb19a55b 100644 > > --- a/run-command.c > > +++ b/run-command.c > > @@ -1308,6 +1308,8 @@ int async_with_fork(void) > > #endif > > } > > > > +struct string_list hook_error_behavior = STRING_LIST_INIT_NODUP; > > Maybe stick this in 'struct repository'. I know most config variables > are still global. But I think we have to move/reorganize them at some > point. Most may end up in 'struct repository'. Okay, sounds fine. > > + default: > > + BUG("unknown hook error behavior"); > > maybe BUG(".. behavior %d", behavior); Sure.
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
On Wed, May 15, 2019 at 11:10:17PM +0000, brian m. carlson wrote: > > An alternative name is onError, probably more often used for event > > callbacks. But I don't know, maybe errorBehavior is actually better. > > I'm going to use "errorStrategy", since we already have > submodule.alternateErrorStrategy. That sounds good (and I don't care too much about the name as long as it it is in the per-hook subsection like this). > > should we fall back to hook.errorBehavior? That allows people to set > > global policy, then customize just a small set of weird hooks. > > Sure, that sounds good. I like this, too. > > maybe stop-on-first-error (or if you go with the "onError" name, I > > think "stop" is enough). I know "stop on/after first hook" does not > > really make any sense when you think about it. Maybe stop-on-first is > > sufficient. > > > > I was going to suggest strcasecmp. But core.whitespace (also has > > multiple-word-values) already sets a precedent on strcmp. I think > > we're good. Or mostly good, I don't know, we still accept False, false > > and FALSE. > > I think with errorStrategy, "stop" is fine. Simpler is better. > > I literally picked what Peff had suggested in his email (mostly because > I'm terrible at naming things), and I don't get the impression he spent > a great deal of time analyzing the ins and outs of the names before > sending. I could be wrong, though. No, I didn't. :) I think "stop" is good. If the others are report-any-error and report-any-success, then the matching name for this could be report-first-error. > > > + else if (!strcmp(value, "report-any-error")) > > > > I couldn't guess based on this name alone, whether we continue or stop > > after the reporting part. The 7/7 document makes it clear though. So > > all good. > > I'm open to hearing better suggestions if anyone has any. Maybe report-all-errors would indicate that it was going to run all of the hooks. I dunno. I think the documentation you wrote is plenty clear with the current name. -Peff
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.
On Thu, May 16, 2019 at 05:19:53PM +0000, brian m. carlson wrote: > > > + /* 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. I think you can do: enum HOOK_ERR { HOOK_ERR_NONE = -2, HOOK_ERR_EXEC = -1, /* otherwise it should be a system exit code */ HOOK_ERR_MAX = 255 }; which ensures that the enum can hold any exit status. -Peff
diff --git a/config.c b/config.c index c2846df3f1..9cba4061a9 100644 --- a/config.c +++ b/config.c @@ -19,6 +19,7 @@ #include "utf8.h" #include "dir.h" #include "color.h" +#include "run-command.h" struct config_source { struct config_source *prev; @@ -1093,6 +1094,29 @@ int git_config_color(char *dest, const char *var, const char *value) return 0; } +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)) { + hook = xmemdupz(key, key_len); + if (!strcmp(value, "stop-on-first")) + behavior = HOOK_ERROR_STOP_ON_FIRST; + else if (!strcmp(value, "report-any-error")) + behavior = HOOK_ERROR_REPORT_ANY_ERROR; + else if (!strcmp(value, "report-any-success")) + behavior = HOOK_ERROR_REPORT_ANY_SUCCESS; + else + die(_("invalid mode for hook %s error behavior: %s"), hook, value); + string_list_insert(&hook_error_behavior, hook)->util = (void *)behavior; + return 0; + } + return 0; +} + static int git_default_core_config(const char *var, const char *value, void *cb) { /* This needs a better name */ @@ -1450,6 +1474,9 @@ int git_default_config(const char *var, const char *value, void *cb) starts_with(var, "committer.")) return git_ident_config(var, value, cb); + if (starts_with(var, "hook.")) + return git_default_hook_config(var, value); + if (starts_with(var, "i18n.")) return git_default_i18n_config(var, value); diff --git a/run-command.c b/run-command.c index 191d6f6f7e..70fb19a55b 100644 --- a/run-command.c +++ b/run-command.c @@ -1308,6 +1308,8 @@ int async_with_fork(void) #endif } +struct string_list hook_error_behavior = STRING_LIST_INIT_NODUP; + /* * Return 1 if a hook exists at path (which may be modified) using access(2) * with check (which should be F_OK or X_OK), 0 otherwise. If strip is true, @@ -1401,18 +1403,48 @@ int for_each_hook(const char *name, void *data) { struct string_list paths = STRING_LIST_INIT_DUP; - int i, ret = 0; + int i, hret = 0; + uintptr_t behavior = HOOK_ERROR_STOP_ON_FIRST; + struct string_list_item *item; + /* Use -2 as sentinel because failure to exec is -1. */ + int ret = -2; + + item = string_list_lookup(&hook_error_behavior, name); + if (item) + behavior = (uintptr_t)item->util; find_hooks(name, &paths); for (i = 0; i < paths.nr; i++) { const char *p = paths.items[i].string; - ret = handler(name, p, data); - if (ret) - break; + hret = handler(name, p, data); + switch (behavior) { + case HOOK_ERROR_STOP_ON_FIRST: + if (hret) { + ret = hret; + goto out; + } + break; + case HOOK_ERROR_REPORT_ANY_SUCCESS: + if (ret == -2) + ret = 1; + if (!hret) + ret = 0; + break; + case HOOK_ERROR_REPORT_ANY_ERROR: + if (ret == -2) + ret = 0; + if (hret) + ret = hret; + break; + default: + BUG("unknown hook error behavior"); + } } - +out: string_list_clear(&paths, 0); + if (ret == -2) + return 0; return ret; } diff --git a/run-command.h b/run-command.h index 15974e26d4..879ebb768f 100644 --- a/run-command.h +++ b/run-command.h @@ -63,6 +63,11 @@ int finish_command(struct child_process *); int finish_command_in_signal(struct child_process *); int run_command(struct child_process *); +#define HOOK_ERROR_STOP_ON_FIRST 1 +#define HOOK_ERROR_REPORT_ANY_ERROR 2 +#define HOOK_ERROR_REPORT_ANY_SUCCESS 3 +extern struct string_list hook_error_behavior; + /* * Returns the paths to all hook files, or NULL if all hooks are missing or * disabled. diff --git a/t/lib-hooks.sh b/t/lib-hooks.sh index 721250aea0..c1d7688313 100644 --- a/t/lib-hooks.sh +++ b/t/lib-hooks.sh @@ -121,7 +121,7 @@ test_multiple_hooks () { ! test -f "$OUTPUTDIR/c" ' - test_expect_success "$hook: multiple hooks, all successful" ' + test_expect_success "$hook: multiple hooks, all successful by default" ' test_when_finished "rm -fr \"$OUTPUTDIR\"" && rm -f "$HOOK" && create_multihooks 0 0 0 && @@ -131,7 +131,40 @@ test_multiple_hooks () { test -f "$OUTPUTDIR/c" ' - test_expect_success "$hook: hooks after first failure not executed" ' + test_expect_success "$hook: multiple hooks, all successful with stop-on-first" ' + test_when_finished "rm -fr \"$OUTPUTDIR\"" && + test_config "hook.$hook.errorbehavior" stop-on-first && + rm -f "$HOOK" && + create_multihooks 0 0 0 && + $cmd content-stop-on-first && + test -f "$OUTPUTDIR/a" && + test -f "$OUTPUTDIR/b" && + test -f "$OUTPUTDIR/c" + ' + + test_expect_success "$hook: multiple hooks, all successful with report-any-error" ' + test_when_finished "rm -fr \"$OUTPUTDIR\"" && + test_config "hook.$hook.errorbehavior" report-any-error && + rm -f "$HOOK" && + create_multihooks 0 0 0 && + $cmd content-report-any-error && + test -f "$OUTPUTDIR/a" && + test -f "$OUTPUTDIR/b" && + test -f "$OUTPUTDIR/c" + ' + + test_expect_success "$hook: multiple hooks, all successful with report-any-success" ' + test_when_finished "rm -fr \"$OUTPUTDIR\"" && + test_config "hook.$hook.errorbehavior" report-any-success && + rm -f "$HOOK" && + create_multihooks 0 0 0 && + $cmd content-report-any-success && + test -f "$OUTPUTDIR/a" && + test -f "$OUTPUTDIR/b" && + test -f "$OUTPUTDIR/c" + ' + + test_expect_success "$hook: hooks after first failure not executed by default" ' test_when_finished "rm -fr \"$OUTPUTDIR\"" && create_multihooks 0 1 0 && $must_fail $cmd more-content && @@ -140,6 +173,75 @@ test_multiple_hooks () { ! test -f "$OUTPUTDIR/c" ' + test_expect_success "$hook: hooks after first failure not executed with stop-on-first" ' + test_when_finished "rm -fr \"$OUTPUTDIR\"" && + test_config "hook.$hook.errorbehavior" stop-on-first && + create_multihooks 0 1 0 && + $must_fail $cmd more-content-stop-on-first && + test -f "$OUTPUTDIR/a" && + test -f "$OUTPUTDIR/b" && + ! test -f "$OUTPUTDIR/c" + ' + + test_expect_success "$hook: hooks after first failure executed with report-any-error" ' + test_when_finished "rm -fr \"$OUTPUTDIR\"" && + test_config "hook.$hook.errorbehavior" report-any-error && + create_multihooks 0 1 0 && + $must_fail $cmd more-content-report-any-error && + test -f "$OUTPUTDIR/a" && + test -f "$OUTPUTDIR/b" && + test -f "$OUTPUTDIR/c" + ' + + test_expect_success "$hook: hooks after first failure executed with report-any-success" ' + test_when_finished "rm -fr \"$OUTPUTDIR\"" && + test_config "hook.$hook.errorbehavior" report-any-success && + create_multihooks 0 1 0 && + $cmd more-content-report-any-success && + test -f "$OUTPUTDIR/a" && + test -f "$OUTPUTDIR/b" && + test -f "$OUTPUTDIR/c" + ' + + test_expect_success "$hook: failing hooks by default" ' + test_when_finished "rm -fr \"$OUTPUTDIR\"" && + create_multihooks 1 1 1 && + $must_fail $cmd most-content && + test -f "$OUTPUTDIR/a" && + ! test -f "$OUTPUTDIR/b" && + ! test -f "$OUTPUTDIR/c" + ' + + test_expect_success "$hook: failing hooks with stop-on-first" ' + test_when_finished "rm -fr \"$OUTPUTDIR\"" && + test_config "hook.$hook.errorbehavior" stop-on-first && + create_multihooks 1 1 1 && + $must_fail $cmd most-content-stop-on-first && + test -f "$OUTPUTDIR/a" && + ! test -f "$OUTPUTDIR/b" && + ! test -f "$OUTPUTDIR/c" + ' + + test_expect_success "$hook: failing hooks with report-any-error" ' + test_when_finished "rm -fr \"$OUTPUTDIR\"" && + test_config "hook.$hook.errorbehavior" report-any-error && + create_multihooks 1 1 1 && + $must_fail $cmd most-content-report-any-error && + test -f "$OUTPUTDIR/a" && + test -f "$OUTPUTDIR/b" && + test -f "$OUTPUTDIR/c" + ' + + test_expect_success "$hook: failing hooks with report-any-success" ' + test_when_finished "rm -fr \"$OUTPUTDIR\"" && + test_config "hook.$hook.errorbehavior" report-any-success && + create_multihooks 1 1 1 && + $must_fail $cmd most-content-report-any-success && + test -f "$OUTPUTDIR/a" && + test -f "$OUTPUTDIR/b" && + test -f "$OUTPUTDIR/c" + ' + test_expect_success POSIXPERM "$hook: non-executable hook not executed" ' test_when_finished "rm -fr \"$OUTPUTDIR\"" && create_multihooks 0 1 0 &&
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. Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net> --- config.c | 27 +++++++++++++ run-command.c | 42 +++++++++++++++++--- run-command.h | 5 +++ t/lib-hooks.sh | 106 ++++++++++++++++++++++++++++++++++++++++++++++++- 4 files changed, 173 insertions(+), 7 deletions(-)