| Message ID | 20210311021037.3001235-6-emilyshaffer@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | config-based hooks | expand |
On Thu, Mar 11 2021, Emily Shaffer wrote: > + switch (should_run_hookdir) { > + case HOOKDIR_NO: Style: case shouldn't be indented > + strbuf_addstr(&hookdir_annotation, _(" (will not run)")); > + break; > + case HOOKDIR_ERROR: > + strbuf_addstr(&hookdir_annotation, _(" (will error and not run)")); > + break; > + case HOOKDIR_INTERACTIVE: > + strbuf_addstr(&hookdir_annotation, _(" (will prompt)")); > + break; > + case HOOKDIR_WARN: > + strbuf_addstr(&hookdir_annotation, _(" (will warn but run)")); > + break; > + case HOOKDIR_YES: > + /* > + * The default behavior should agree with > + * hook.c:configured_hookdir_opt(). HOOKDIR_UNKNOWN should just > + * do the default behavior. > + */ > + case HOOKDIR_UNKNOWN: > + default: > + break; We should avoid this sort of translation lego. > + } > + > list_for_each(pos, head) { > struct hook *item = list_entry(pos, struct hook, list); > item = list_entry(pos, struct hook, list); > if (item) { > /* Don't translate 'hookdir' - it matches the config */ > - printf("%s: %s\n", > + printf("%s: %s%s\n", native speakers in some languages to read the sentance backwards. Because if you concatenate strings like this you force. (We don't currently have a RTL language in po/, still, but let's not create churn for if/when we do if we can help it)> I have a patch on top to fix this, will send it as some general reply of proposed fixup.s > (item->from_hookdir > + git hook list pre-commit >actual && > + # the hookdir annotation is translated > + test_i18ncmp expected actual This (and the rest of test_i18ncmp in this series) can and should just be "test_cmp" or "test_i18ncmp", the poison mode is dead. See my recent patches to search/replace test_i18ncmp. The reason the function isn't gone entirely was to help a series like yours in "seen", but if we're re-rolling...
On Fri, Mar 12, 2021 at 09:33:46AM +0100, Ævar Arnfjörð Bjarmason wrote: > > > On Thu, Mar 11 2021, Emily Shaffer wrote: > > > + switch (should_run_hookdir) { > > + case HOOKDIR_NO: > > Style: case shouldn't be indented Done, thanks. > > > + strbuf_addstr(&hookdir_annotation, _(" (will not run)")); > > + break; > > + case HOOKDIR_ERROR: > > + strbuf_addstr(&hookdir_annotation, _(" (will error and not run)")); > > + break; > > + case HOOKDIR_INTERACTIVE: > > + strbuf_addstr(&hookdir_annotation, _(" (will prompt)")); > > + break; > > + case HOOKDIR_WARN: > > + strbuf_addstr(&hookdir_annotation, _(" (will warn but run)")); > > + break; > > + case HOOKDIR_YES: > > + /* > > + * The default behavior should agree with > > + * hook.c:configured_hookdir_opt(). HOOKDIR_UNKNOWN should just > > + * do the default behavior. > > + */ > > + case HOOKDIR_UNKNOWN: > > + default: > > + break; > > We should avoid this sort of translation lego. > > > + } > > + > > list_for_each(pos, head) { > > struct hook *item = list_entry(pos, struct hook, list); > > item = list_entry(pos, struct hook, list); > > if (item) { > > /* Don't translate 'hookdir' - it matches the config */ > > - printf("%s: %s\n", > > + printf("%s: %s%s\n", > > native speakers in some languages to read the sentance backwards. > Because if you concatenate strings like this you force. > > (We don't currently have a RTL language in po/, still, but let's not > create churn for if/when we do if we can help it)> Yeah, you are absolutely right. I'll take a look at your suggestion, thanks. > > > I have a patch on top to fix this, will send it as some general reply of > proposed fixup.s FWIW, I found this format of suggestions really hard to navigate. I had to go find your fixup (and I think you sent two different ones) and then had to scroll around and find what you're referring to from here. If I were to apply the fixup directly, I'd have to split it up and find the appropriate commits to associate each part of the diff with. I know generating scissors patches for each review is a pain, but I'd even prefer a prose "How about printing in each part of the case instead, so each string can be translated in full" or a non-formatted example inline to the catchall fixups patch you sent. > > > (item->from_hookdir > > + git hook list pre-commit >actual && > > + # the hookdir annotation is translated > > + test_i18ncmp expected actual > > This (and the rest of test_i18ncmp in this series) can and should just > be "test_cmp" or "test_i18ncmp", the poison mode is dead. See my recent > patches to search/replace test_i18ncmp. > > The reason the function isn't gone entirely was to help a series like > yours in "seen", but if we're re-rolling... Oh cool, thanks, will do.
On Wed, Mar 24 2021, Emily Shaffer wrote: > On Fri, Mar 12, 2021 at 09:33:46AM +0100, �var Arnfj�r� Bjarmason wrote: >> >> >> On Thu, Mar 11 2021, Emily Shaffer wrote: >> >> > + switch (should_run_hookdir) { >> > + case HOOKDIR_NO: >> >> Style: case shouldn't be indented > > Done, thanks. > >> >> > + strbuf_addstr(&hookdir_annotation, _(" (will not run)")); >> > + break; >> > + case HOOKDIR_ERROR: >> > + strbuf_addstr(&hookdir_annotation, _(" (will error and not run)")); >> > + break; >> > + case HOOKDIR_INTERACTIVE: >> > + strbuf_addstr(&hookdir_annotation, _(" (will prompt)")); >> > + break; >> > + case HOOKDIR_WARN: >> > + strbuf_addstr(&hookdir_annotation, _(" (will warn but run)")); >> > + break; >> > + case HOOKDIR_YES: >> > + /* >> > + * The default behavior should agree with >> > + * hook.c:configured_hookdir_opt(). HOOKDIR_UNKNOWN should just >> > + * do the default behavior. >> > + */ >> > + case HOOKDIR_UNKNOWN: >> > + default: >> > + break; >> >> We should avoid this sort of translation lego. >> >> > + } >> > + >> > list_for_each(pos, head) { >> > struct hook *item = list_entry(pos, struct hook, list); >> > item = list_entry(pos, struct hook, list); >> > if (item) { >> > /* Don't translate 'hookdir' - it matches the config */ >> > - printf("%s: %s\n", >> > + printf("%s: %s%s\n", >> >> native speakers in some languages to read the sentance backwards. >> Because if you concatenate strings like this you force. >> >> (We don't currently have a RTL language in po/, still, but let's not >> create churn for if/when we do if we can help it)> > > Yeah, you are absolutely right. I'll take a look at your suggestion, > thanks. > >> >> >> I have a patch on top to fix this, will send it as some general reply of >> proposed fixup.s > > FWIW, I found this format of suggestions really hard to navigate. I had > to go find your fixup (and I think you sent two different ones) and then > had to scroll around and find what you're referring to from here. If I > were to apply the fixup directly, I'd have to split it up and find the > appropriate commits to associate each part of the diff with. I know > generating scissors patches for each review is a pain, but I'd even > prefer a prose "How about printing in each part of the case instead, so > each string can be translated in full" or a non-formatted example inline > to the catchall fixups patch you sent. Sorry about that. FWIW I did try, but found myself jumping a bit too much between different commits (as noted in some "maybe squash?" comments elsewhere), and eventually just gave up and produced some RFC-just-for-discussion patch on top of the whole thing and mostly looked at the entire diff forthe series. >> >> > (item->from_hookdir >> > + git hook list pre-commit >actual && >> > + # the hookdir annotation is translated >> > + test_i18ncmp expected actual >> >> This (and the rest of test_i18ncmp in this series) can and should just >> be "test_cmp" or "test_i18ncmp", the poison mode is dead. See my recent >> patches to search/replace test_i18ncmp. >> >> The reason the function isn't gone entirely was to help a series like >> yours in "seen", but if we're re-rolling... > > Oh cool, thanks, will do.
diff --git a/Documentation/config/hook.txt b/Documentation/config/hook.txt index 71449ecbc7..75312754ae 100644 --- a/Documentation/config/hook.txt +++ b/Documentation/config/hook.txt @@ -7,3 +7,8 @@ hookcmd.<name>.command:: A command to execute during a hook for which <name> has been specified as a command. This can be an executable on your device or a oneliner for your shell. See linkgit:git-hook[1]. + +hook.runHookDir:: + Controls how hooks contained in your hookdir are executed. Can be any of + "yes", "warn", "interactive", or "no". Defaults to "yes". See + linkgit:git-hook[1] and linkgit:git-config[1] "core.hooksPath"). diff --git a/builtin/hook.c b/builtin/hook.c index c8fbfbb39d..310f696ebf 100644 --- a/builtin/hook.c +++ b/builtin/hook.c @@ -10,10 +10,13 @@ static const char * const builtin_hook_usage[] = { NULL }; +static enum hookdir_opt should_run_hookdir; + static int list(int argc, const char **argv, const char *prefix) { struct list_head *head, *pos; struct strbuf hookname = STRBUF_INIT; + struct strbuf hookdir_annotation = STRBUF_INIT; struct option list_options[] = { OPT_END(), @@ -38,20 +41,48 @@ static int list(int argc, const char **argv, const char *prefix) return 0; } + switch (should_run_hookdir) { + case HOOKDIR_NO: + strbuf_addstr(&hookdir_annotation, _(" (will not run)")); + break; + case HOOKDIR_ERROR: + strbuf_addstr(&hookdir_annotation, _(" (will error and not run)")); + break; + case HOOKDIR_INTERACTIVE: + strbuf_addstr(&hookdir_annotation, _(" (will prompt)")); + break; + case HOOKDIR_WARN: + strbuf_addstr(&hookdir_annotation, _(" (will warn but run)")); + break; + case HOOKDIR_YES: + /* + * The default behavior should agree with + * hook.c:configured_hookdir_opt(). HOOKDIR_UNKNOWN should just + * do the default behavior. + */ + case HOOKDIR_UNKNOWN: + default: + break; + } + list_for_each(pos, head) { struct hook *item = list_entry(pos, struct hook, list); item = list_entry(pos, struct hook, list); if (item) { /* Don't translate 'hookdir' - it matches the config */ - printf("%s: %s\n", + printf("%s: %s%s\n", (item->from_hookdir ? "hookdir" : config_scope_name(item->origin)), - item->command.buf); + item->command.buf, + (item->from_hookdir + ? hookdir_annotation.buf + : "")); } } clear_hook_list(head); + strbuf_release(&hookdir_annotation); strbuf_release(&hookname); return 0; @@ -59,16 +90,44 @@ static int list(int argc, const char **argv, const char *prefix) int cmd_hook(int argc, const char **argv, const char *prefix) { + const char *run_hookdir = NULL; + struct option builtin_hook_options[] = { + OPT_STRING(0, "run-hookdir", &run_hookdir, N_("option"), + N_("what to do with hooks found in the hookdir")), OPT_END(), }; - if (argc < 2) + + argc = parse_options(argc, argv, prefix, builtin_hook_options, + builtin_hook_usage, 0); + + /* after the parse, we should have "<command> <hookname> <args...>" */ + if (argc < 1) usage_with_options(builtin_hook_usage, builtin_hook_options); git_config(git_default_config, NULL); - if (!strcmp(argv[1], "list")) - return list(argc - 1, argv + 1, prefix); + + /* argument > config */ + if (run_hookdir) + if (!strcmp(run_hookdir, "no")) + should_run_hookdir = HOOKDIR_NO; + else if (!strcmp(run_hookdir, "error")) + should_run_hookdir = HOOKDIR_ERROR; + else if (!strcmp(run_hookdir, "yes")) + should_run_hookdir = HOOKDIR_YES; + else if (!strcmp(run_hookdir, "warn")) + should_run_hookdir = HOOKDIR_WARN; + else if (!strcmp(run_hookdir, "interactive")) + should_run_hookdir = HOOKDIR_INTERACTIVE; + else + die(_("'%s' is not a valid option for --run-hookdir " + "(yes, warn, interactive, no)"), run_hookdir); + else + should_run_hookdir = configured_hookdir_opt(); + + if (!strcmp(argv[0], "list")) + return list(argc, argv, prefix); usage_with_options(builtin_hook_usage, builtin_hook_options); } diff --git a/hook.c b/hook.c index 080e25696b..039ff0a378 100644 --- a/hook.c +++ b/hook.c @@ -102,6 +102,30 @@ static int hook_config_lookup(const char *key, const char *value, void *cb_data) return 0; } +enum hookdir_opt configured_hookdir_opt(void) +{ + const char *key; + if (git_config_get_value("hook.runhookdir", &key)) + return HOOKDIR_YES; /* by default, just run it. */ + + if (!strcmp(key, "no")) + return HOOKDIR_NO; + + if (!strcmp(key, "error")) + return HOOKDIR_ERROR; + + if (!strcmp(key, "yes")) + return HOOKDIR_YES; + + if (!strcmp(key, "warn")) + return HOOKDIR_WARN; + + if (!strcmp(key, "interactive")) + return HOOKDIR_INTERACTIVE; + + return HOOKDIR_UNKNOWN; +} + struct list_head* hook_list(const struct strbuf* hookname) { struct strbuf hook_key = STRBUF_INIT; diff --git a/hook.h b/hook.h index a97d43670d..1c4b953aec 100644 --- a/hook.h +++ b/hook.h @@ -20,6 +20,22 @@ struct hook { */ struct list_head* hook_list(const struct strbuf *hookname); +enum hookdir_opt +{ + HOOKDIR_NO, + HOOKDIR_ERROR, + HOOKDIR_WARN, + HOOKDIR_INTERACTIVE, + HOOKDIR_YES, + HOOKDIR_UNKNOWN, +}; + +/* + * Provides the hookdir_opt specified in the config without consulting any + * command line arguments. + */ +enum hookdir_opt configured_hookdir_opt(void); + /* Free memory associated with a 'struct hook' */ void free_hook(struct hook *ptr); /* Empties the list at 'head', calling 'free_hook()' on each entry */ diff --git a/t/t1360-config-based-hooks.sh b/t/t1360-config-based-hooks.sh index 0f12af4659..66b0b6b7ad 100755 --- a/t/t1360-config-based-hooks.sh +++ b/t/t1360-config-based-hooks.sh @@ -104,4 +104,75 @@ test_expect_success 'git hook list shows hooks from the hookdir' ' test_cmp expected actual ' +test_expect_success 'hook.runHookDir = no is respected by list' ' + setup_hookdir && + + test_config hook.runHookDir "no" && + + cat >expected <<-EOF && + hookdir: $(pwd)/.git/hooks/pre-commit (will not run) + EOF + + git hook list pre-commit >actual && + # the hookdir annotation is translated + test_i18ncmp expected actual +' + +test_expect_success 'hook.runHookDir = error is respected by list' ' + setup_hookdir && + + test_config hook.runHookDir "error" && + + cat >expected <<-EOF && + hookdir: $(pwd)/.git/hooks/pre-commit (will error and not run) + EOF + + git hook list pre-commit >actual && + # the hookdir annotation is translated + test_i18ncmp expected actual +' + +test_expect_success 'hook.runHookDir = warn is respected by list' ' + setup_hookdir && + + test_config hook.runHookDir "warn" && + + cat >expected <<-EOF && + hookdir: $(pwd)/.git/hooks/pre-commit (will warn but run) + EOF + + git hook list pre-commit >actual && + # the hookdir annotation is translated + test_i18ncmp expected actual +' + + +test_expect_success 'hook.runHookDir = interactive is respected by list' ' + setup_hookdir && + + test_config hook.runHookDir "interactive" && + + cat >expected <<-EOF && + hookdir: $(pwd)/.git/hooks/pre-commit (will prompt) + EOF + + git hook list pre-commit >actual && + # the hookdir annotation is translated + test_i18ncmp expected actual +' + +test_expect_success 'hook.runHookDir is tolerant to unknown values' ' + setup_hookdir && + + test_config hook.runHookDir "junk" && + + cat >expected <<-EOF && + hookdir: $(pwd)/.git/hooks/pre-commit + EOF + + git hook list pre-commit >actual && + # the hookdir annotation is translated + test_i18ncmp expected actual +' + test_done
For now, just give a hint about how these hooks will be run in 'git hook list'. Later on, though, we will pay attention to this enum when running the hooks. --- Notes: Since v7, tidied up the behavior of the HOOK_UNKNOWN flag and added a test to enforce it - now it matches the design doc much better. Also, thanks Jonathan Tan for pointing out that the commit message made no sense and was targeted for a different change. Rewrote the commit message now. Plus, added HOOK_ERROR flag per Junio and Jonathan Nieder. - Emily Documentation/config/hook.txt | 5 +++ builtin/hook.c | 69 +++++++++++++++++++++++++++++++--- hook.c | 24 ++++++++++++ hook.h | 16 ++++++++ t/t1360-config-based-hooks.sh | 71 +++++++++++++++++++++++++++++++++++ 5 files changed, 180 insertions(+), 5 deletions(-)