| Message ID | 20210819033450.3382652-6-emilyshaffer@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | config-based hooks restarted | expand |
Emily Shaffer <emilyshaffer@google.com> writes: > diff --git a/Documentation/config/hook.txt b/Documentation/config/hook.txt > index 96d3d6572c..c394756328 100644 > --- a/Documentation/config/hook.txt > +++ b/Documentation/config/hook.txt > @@ -1,3 +1,21 @@ > +hook.<name>.command:: > + A command to execute whenever `hook.<name>` is invoked. `<name>` should > + be a unique "friendly" name which you can use to identify this hook > + command. (You can specify when to invoke this command with > + `hook.<name>.event`.) The value can be an executable on your device or a > + oneliner for your shell. If more than one value is specified for the > + same `<name>`, the last value parsed will be the only command executed. > + See linkgit:git-hook[1]. > + > +hook.<name>.event:: > + The hook events which should invoke `hook.<name>`. `<name>` should be a > + unique "friendly" name which you can use to identify this hook. The > + value should be the name of a hook event, like "pre-commit" or "update". > + (See linkgit:githooks[5] for a complete list of hooks Git knows about.) > + On the specified event, the associated `hook.<name>.command` will be > + executed. More than one event can be specified if you wish for > + `hook.<name>` to execute on multiple events. See linkgit:git-hook[1]. Looking much better. It now gives enough information to readers to understand (if not enough to convince that it is a good idea) why an indirection with "friendly name" between the event and command is there. In short, <name> names the command to be run and without indirection, you'd end up having to write: [hook "check-whitespace && spellcheck-log-message"] event = pre-commit [hook "check-whitespace && spellcheck-log-message"] event = another-hookable-event which may give the same expressiveness (and may even be workable if the configuration were machine generated) but it is typo-prone, and a single typo, or even an insignificant whitespace change in the command, would destroy the grouping of "this command fires upon these events". It becomes much less typo prone with the indirection, i.e. [hook "logcheck"] command = check-whitespace && spellcheck-log-message [hook "logcheck"] event = pre-commit [hook "logcheck"] event = another-hookable-event using the "friendly name", especially if these entries are spread across different configuration files. My original question was primarily because I thought the second-level <name> corresponded to <event>. If that were the case, it can trivially be made simpler without making it typo-prone, i.e. [hook "pre-commit"] command = check-whitespace && spellcheck-log-message [hook "another-hookable-event"] command = check-whitespace && spellcheck-log-message since the name of the event would be much shorter than the command line. But since we are not grouping per hookable event (to apply the "last one wins" rule to determine which single command is the one that gets to run). Thanks.
On Thu, Aug 19, 2021 at 03:39:06PM -0700, Junio C Hamano wrote: > > Emily Shaffer <emilyshaffer@google.com> writes: > > > diff --git a/Documentation/config/hook.txt b/Documentation/config/hook.txt > > index 96d3d6572c..c394756328 100644 > > --- a/Documentation/config/hook.txt > > +++ b/Documentation/config/hook.txt > > @@ -1,3 +1,21 @@ > > +hook.<name>.command:: > > + A command to execute whenever `hook.<name>` is invoked. `<name>` should > > + be a unique "friendly" name which you can use to identify this hook > > + command. (You can specify when to invoke this command with > > + `hook.<name>.event`.) The value can be an executable on your device or a > > + oneliner for your shell. If more than one value is specified for the > > + same `<name>`, the last value parsed will be the only command executed. > > + See linkgit:git-hook[1]. > > + > > +hook.<name>.event:: > > + The hook events which should invoke `hook.<name>`. `<name>` should be a > > + unique "friendly" name which you can use to identify this hook. The > > + value should be the name of a hook event, like "pre-commit" or "update". > > + (See linkgit:githooks[5] for a complete list of hooks Git knows about.) > > + On the specified event, the associated `hook.<name>.command` will be > > + executed. More than one event can be specified if you wish for > > + `hook.<name>` to execute on multiple events. See linkgit:git-hook[1]. > > Looking much better. It now gives enough information to readers to > understand (if not enough to convince that it is a good idea) why an > indirection with "friendly name" between the event and command is > there. In short, <name> names the command to be run and without > indirection, you'd end up having to write: > > [hook "check-whitespace && spellcheck-log-message"] > event = pre-commit > [hook "check-whitespace && spellcheck-log-message"] > event = another-hookable-event > > which may give the same expressiveness (and may even be workable if > the configuration were machine generated) but it is typo-prone, and > a single typo, or even an insignificant whitespace change in the > command, would destroy the grouping of "this command fires upon > these events". > > It becomes much less typo prone with the indirection, i.e. > > [hook "logcheck"] > command = check-whitespace && spellcheck-log-message > > [hook "logcheck"] > event = pre-commit > > [hook "logcheck"] > event = another-hookable-event > > using the "friendly name", especially if these entries are spread > across different configuration files. > > My original question was primarily because I thought the > second-level <name> corresponded to <event>. If that were the case, > it can trivially be made simpler without making it typo-prone, i.e. > > [hook "pre-commit"] > command = check-whitespace && spellcheck-log-message > > [hook "another-hookable-event"] > command = check-whitespace && spellcheck-log-message > > since the name of the event would be much shorter than the command > line. But since we are not grouping per hookable event (to apply > the "last one wins" rule to determine which single command is the > one that gets to run). > To be clear, the config schema did work the way you describe until this revision. Ævar suggested the change and it seemed like a good idea to me (and the rest of the Google folks) so I changed between v2 and v3 of the restart. - Emily
Emily Shaffer <emilyshaffer@google.com> writes: > On Thu, Aug 19, 2021 at 03:39:06PM -0700, Junio C Hamano wrote: >> >> My original question was primarily because I thought the >> second-level <name> corresponded to <event>. If that were the case, >> it can trivially be made simpler without making it typo-prone, i.e. >> >> [hook "pre-commit"] >> command = check-whitespace && spellcheck-log-message >> >> [hook "another-hookable-event"] >> command = check-whitespace && spellcheck-log-message >> >> since the name of the event would be much shorter than the command >> line. But since we are not grouping per hookable event (to apply >> the "last one wins" rule to determine which single command is the >> one that gets to run). >> > > To be clear, the config schema did work the way you describe until this > revision. Ævar suggested the change and it seemed like a good idea to me > (and the rest of the Google folks) so I changed between v2 and v3 of the > restart. To be clear, you do not want to take the above as my suggestion to go back to the previous one, since that is not what I meant. As long as you and others are happy with what you folks ended up with, i.e. the current one that uses <name> that is a short-hand for <command>, that is what matters. Thanks.
On Wed, Aug 18 2021, Emily Shaffer wrote: > Teach the hook.[hc] library to parse configs to populare the list of > hooks to run for a given event. s/populare/populate/ > Multiple commands can be specified for a given hook by providing > multiple "hook.<friendly-name>.command = <path-to-hook>" and > "hook.<friendly-name>.event = <hook-event>" lines. Hooks will be run in > config order of the "hook.<name>.event" lines. The "will be run in order" probably needs some amending to account for --jobs, no? > For example: > > $ git config --list | grep ^hook > hook.bar.command=~/bar.sh > hook.bar.event=pre-commit Perhaps the example should use: git config --get-regexp '^hook\.' > $ git hook run > # Runs ~/bar.sh > # Runs .git/hooks/pre-commit And this "# Runs" is not actual output by git, but just an explanation for what happens, better to reword it somehow so it doesn't give that impression. But the example also seems to be broken, surely it should be "git hook run bar", not "git hook run"? Reading ahead, but presumably no-arg doesn't run all known hooks... > Signed-off-by: Emily Shaffer <emilyshaffer@google.com> > --- > Documentation/config/hook.txt | 18 ++++ > Documentation/git-hook.txt | 57 ++++++++++++- > builtin/hook.c | 3 +- > hook.c | 153 ++++++++++++++++++++++++++++++---- > hook.h | 7 +- > t/t1800-hook.sh | 141 ++++++++++++++++++++++++++++++- > 6 files changed, 357 insertions(+), 22 deletions(-) > > diff --git a/Documentation/config/hook.txt b/Documentation/config/hook.txt > index 96d3d6572c..c394756328 100644 > --- a/Documentation/config/hook.txt > +++ b/Documentation/config/hook.txt > @@ -1,3 +1,21 @@ > +hook.<name>.command:: > + A command to execute whenever `hook.<name>` is invoked. `<name>` should > + be a unique "friendly" name which you can use to identify this hook > + command. (You can specify when to invoke this command with > + `hook.<name>.event`.) The value can be an executable on your device or a > + oneliner for your shell. If more than one value is specified for the > + same `<name>`, the last value parsed will be the only command executed. > + See linkgit:git-hook[1]. Hrm, so here we say "If more than one value is specified for ... the last value parsed will be the only command executed", but in the commit message it's: Multiple commands can be specified for a given hook by providing multiple "hook.<friendly-name>.command = <path-to-hook>" and "hook.<friendly-name>.event = <hook-event>" lines. As we've discussed earlier I've got a preference for the former, but just reading this commit message & doc the for the first time I still don't know what you went for, looks like one or the other needs updating. I'm intentionally not reading ahead as I review this. Saying that it's a "unique name", but also discussing what happens if it's not unique in the sense that we have multiple "hook.<name>.*" is a bit confusing. I think I know what you're going for, perhaps something like this would be better to describe it: For a "hook.<name>.{command,event}" hook entry you'll need to pick a "<name>" that's not shared with any other hook, if you do normal single-value config semantics apply and git will use the last provided value. Or something... > +hook.<name>.event:: > + The hook events which should invoke `hook.<name>`. `<name>` should be a > + unique "friendly" name which you can use to identify this hook. The p> + value should be the name of a hook event, like "pre-commit" or "update". > + (See linkgit:githooks[5] for a complete list of hooks Git knows about.) > + On the specified event, the associated `hook.<name>.command` will be > + executed. More than one event can be specified if you wish for > + `hook.<name>` to execute on multiple events. See linkgit:git-hook[1]. > + > hook.jobs:: > Specifies how many hooks can be run simultaneously during parallelized > hook execution. If unspecified, defaults to the number of processors on > diff --git a/Documentation/git-hook.txt b/Documentation/git-hook.txt > index d1db084e4f..9c6cbdc2eb 100644 > --- a/Documentation/git-hook.txt > +++ b/Documentation/git-hook.txt > @@ -27,12 +27,65 @@ Git is unlikely to use for a native hook later on. For example, Git is much less > likely to create a `mytool-validate-commit` hook than it is to create a > `validate-commit` hook. > > +This command parses the default configuration files for pairs of configs like > +so: > + > + [hook "linter"] > + event = pre-commit > + command = ~/bin/linter --c > + > +In this example, `[hook "linter"]` represents one script - `~/bin/linter --c` - > +which can be shared by many repos, and even by many hook events, if appropriate. OK, so now it seems like "hook.<name>.command" is 1=1 for "hook.<name>" and "command", and "hook.<name>.event" is 1=many, maybe that's correct, but reading on... > +Commands are run in the order Git encounters their associated > +`hook.<name>.event` configs during the configuration parse (see > +linkgit:git-config[1]). Although multiple `hook.linter.event` configs can be > +added, only one `hook.linter.command` event is valid - Git uses "last-one-wins" > +to determine which command to run. ...ah, and confirmed here... > +So if you wanted your linter to run when you commit as well as when you push, > +you would configure it like so: > + > + [hook "linter"] > + event = pre-commit > + event = pre-push > + command = ~/bin/linter --c > + > +With this config, `~/bin/linter --c` would be run by Git before a commit is > +generated (during `pre-commit`) as well as before a push is performed (during > +`pre-push`). Aside: I know we're discussing a presumably imaginary linter, but it's a bit jarring to see "--" for a one-letter short-option, perhaps "-c" or "--check" for the example... > +And if you wanted to run your linter as well as a secret-leak detector during > +only the "pre-commit" hook event, you would configure it instead like so: > + > + [hook "linter"] > + event = pre-commit > + command = ~/bin/linter --c > + [hook "no-leaks"] > + event = pre-commit > + command = ~/bin/leak-detector I think these examples would flow a bit more naturally if we started by discussing how to setup one configured hook, then two unrelated hooks (perhaps a "commit-msg" and "pre-commit" hook), and then finally the cases where a given "hook.<name>.command" has multiple "hook.<name>.event" entries. My assumption in suggesting that is that that's, respectively, the most common to less common use cases. > +With this config, before a commit is generated (during `pre-commit`), Git would > +first start `~/bin/linter --c` and second start `~/bin/leak-detector`. It would > +evaluate the output of each when deciding whether to proceed with the commit. > + > +For a full list of hook events which you can set your `hook.<name>.event` to, > +and how hooks are invoked during those events, see linkgit:githooks[5]. Let's discuss what happens with unknown values for `hook.<name>.event` here, i.e. just "are ignored". [I'll discuss my opinions on the new and revised config schema in another mail, just commenting on the patch here in terms of its stated goals]. > +In general, when instructions suggest adding a script to > +`.git/hooks/<hook-event>`, you can specify it in the config instead by running > +`git config --add hook.<some-name>.command <path-to-script> && git config --add > +hook.<some-name>.event <hook-event>` - this way you can share the script between > +multiple repos. That is, `cp ~/my-script.sh ~/project/.git/hooks/pre-commit` > +would become `git config --add hook.my-script.command ~/my-script.sh && git > +config --add hook.my-script.event pre-commit`. I think this would benefit a lot from being split up into code example prose, so: You can [...] ---- git config -add hook.<some-name>.command <path-to-script> && git config -add hook.<some-name>.event <hook-event> ---- It's more lines, but I think a lot more readable. I think the part of this that says "You can share the script between multiple repos" could really use some elaboration. I.e. if the user is following an example where they'd otherwise edit .git/hooks/ and then just run "git config", they'll add it to .git/config. So then it won't be shared between multiple repos, what I think this paragraph is trying to go for is that instead of say: ln -s ~/my-script.sh .git/hooks/pre-commit You can instead do, with configurable hooks: git config hook.my-pre-commit.command ~/my-script.sh git config --add hook.my-pre-commit.event pre-commit Notice how I omitted the --add for the first one. It's also confusing if we're documenting "*.command" as "last one wins" to use --add with it, in an example that's discussing how to add it to some local repo, no? Or is this example really trying to discuss what would happen if you: cat >~/my-script.sh ... ^C chmod +x ~/my-script.sh git config --global hook.my-pre-commit.command ~/my-script.sh git config hook.my-pre-commit.event pre-commit That's not clear to me, in any case, I think we'd do well to lead with the former, and then discuss the latter. I.e. "here's what you'd symlink before, now it's in config like this", and then discuss how you'd set "global" or perhaps included config, which isn't possible with the .git/hooks/<script> mechanism. > SUBCOMMANDS > ----------- > > run:: > - Run the `<hook-name>` hook. See linkgit:githooks[5] for > - the hook names we support. > + Runs hooks configured for `<hook-name>`, in the order they are > + discovered during the config parse. This and any other things that discuss how hooks are run in detail should really talk about the thing running .git/hooks/<name> *and* any configured hooks, and on the subject of --jobs and ordering we should also talk about what the order of config v.s. .git/hooks/<name> is. > + /* to enable oneliners, let config-specified hooks run in shell. > + * config-specified hooks have a name. */ nit: usual style is multi-line comments like: /* * some text[...] * some more... */ Not: /* text here right away[...] * some more ... */ > + cp->use_shell = !!run_me->name; > + > /* add command */ > - if (hook_cb->options->absolute_path) > - strvec_push(&cp->args, absolute_path(run_me->hook_path)); > - else > - strvec_push(&cp->args, run_me->hook_path); > + if (run_me->name) { > + /* ...from config */ > + struct strbuf cmd_key = STRBUF_INIT; > + char *command = NULL; > + > + strbuf_addf(&cmd_key, "hook.%s.command", run_me->name); Missing strbuf_release() for this later? > + if (git_config_get_string(cmd_key.buf, &command)) { > + /* TODO test me! */ ...seems easy enough to just have a test for..., i.e. an *.event entry with no *.command. > + die(_("'hook.%s.command' must be configured " > + "or 'hook.%s.event' must be removed; aborting.\n"), > + run_me->name, run_me->name); > + } > + > + strvec_push(&cp->args, command); > + } else { > + /* ...from hookdir. */ > + const char *hook_path = NULL; > + /* > + * Nit: Too few \n before the text in a comment earlier, too many here :) > + * At this point we are already running, so don't validate > + * whether the hook name is known or not. ...because it was done earlier somewhere, or...? > + */ > + hook_path = find_hook_gently(hook_cb->hook_name); > + if (!hook_path) > + BUG("hookdir hook in hook list but no hookdir hook present in filesystem"); > + > + if (hook_cb->options->absolute_path) > + hook_path = absolute_path(hook_path); > + > + strvec_push(&cp->args, hook_path); > + } > + > > /* > * add passed-in argv, without expanding - let the user get back > @@ -228,8 +346,11 @@ static int notify_start_failure(struct strbuf *out, > > hook_cb->rc |= 1; > > - strbuf_addf(out, _("Couldn't start hook '%s'\n"), > - attempted->hook_path); > + if (attempted->name) > + strbuf_addf(out, _("Couldn't start hook '%s'\n"), > + attempted->name); > + else > + strbuf_addstr(out, _("Couldn't start hook from hooks directory\n")); > > return 1; > } > diff --git a/hook.h b/hook.h > index 6b7b2d14d2..621bd2cde1 100644 > --- a/hook.h > +++ b/hook.h > @@ -27,8 +27,11 @@ int hook_exists(const char *hookname); > > struct hook { > struct list_head list; > - /* The path to the hook */ > - const char *hook_path; > + /* > + * The friendly name of the hook. NULL indicates the hook is from the > + * hookdir. > + */ > + const char *name; > > /* > * Use this to keep state for your feed_pipe_fn if you are using > diff --git a/t/t1800-hook.sh b/t/t1800-hook.sh > index 217db848b3..ef2432f53a 100755 > --- a/t/t1800-hook.sh > +++ b/t/t1800-hook.sh > @@ -1,13 +1,29 @@ > #!/bin/bash > > -test_description='git-hook command' > +test_description='git-hook command and config-managed multihooks' > > . ./test-lib.sh > > +setup_hooks () { > + test_config hook.ghi.event pre-commit --add > + test_config hook.ghi.command "/path/ghi" --add > + test_config_global hook.def.event pre-commit --add > + test_config_global hook.def.command "/path/def" --add Isn't --add redundant here? Seems no test is stressing multi-value hook.{ghi,def}.* from a quick glance... > +} > + > +setup_hookdir () { > + mkdir .git/hooks > + write_script .git/hooks/pre-commit <<-EOF > + echo \"Legacy Hook\" > + EOF > + test_when_finished rm -rf .git/hooks > +} > + > test_expect_success 'git hook usage' ' > test_expect_code 129 git hook && > test_expect_code 129 git hook run && > test_expect_code 129 git hook run -h && > + test_expect_code 129 git hook list -h && Doesn't this belong in a previous commit that added "git hook list", not here? > test_expect_code 129 git hook run --unknown 2>err && > grep "unknown option" err > ' > @@ -153,4 +169,127 @@ test_expect_success 'stdin to hooks' ' > test_cmp expect actual > ' > > +test_expect_success 'git hook list orders by config order' ' > + setup_hooks && > + > + cat >expected <<-EOF && > + def > + ghi > + EOF > + > + git hook list pre-commit >actual && > + test_cmp expected actual > +' > + > +test_expect_success 'git hook list reorders on duplicate event declarations' ' > + setup_hooks && > + > + # 'def' is usually configured globally; move it to the end by > + # configuring it locally. > + test_config hook.def.event "pre-commit" --add && Ah, well the --add belongs here, but not needed in setup_hooks, right? > + > + cat >expected <<-EOF && > + ghi > + def > + EOF > + > + git hook list pre-commit >actual && > + test_cmp expected actual > +' > + > +test_expect_success 'git hook list shows hooks from the hookdir' ' > + setup_hookdir && > + > + cat >expected <<-EOF && > + hook from hookdir > + EOF > + > + git hook list pre-commit >actual && > + test_cmp expected actual > +' > + > +test_expect_success 'inline hook definitions execute oneliners' ' > + test_config hook.oneliner.event "pre-commit" && > + test_config hook.oneliner.command "echo \"Hello World\"" && > + > + echo "Hello World" >expected && > + > + # hooks are run with stdout_to_stderr = 1 > + git hook run pre-commit 2>actual && > + test_cmp expected actual > +' > + > +test_expect_success 'inline hook definitions resolve paths' ' > + write_script sample-hook.sh <<-EOF && > + echo \"Sample Hook\" > + EOF > + > + test_when_finished "rm sample-hook.sh" && > + > + test_config hook.sample-hook.event pre-commit && > + test_config hook.sample-hook.command "\"$(pwd)/sample-hook.sh\"" && > + > + echo \"Sample Hook\" >expected && > + > + # hooks are run with stdout_to_stderr = 1 > + git hook run pre-commit 2>actual && > + test_cmp expected actual > +' > + > +test_expect_success 'hookdir hook included in git hook run' ' > + setup_hookdir && > + > + echo \"Legacy Hook\" >expected && > + > + # hooks are run with stdout_to_stderr = 1 > + git hook run pre-commit 2>actual && > + test_cmp expected actual > +' > + > +test_expect_success 'stdin to multiple hooks' ' > + test_config hook.stdin-a.event "test-hook" --add && > + test_config hook.stdin-a.command "xargs -P1 -I% echo a%" --add && > + test_config hook.stdin-b.event "test-hook" --add && > + test_config hook.stdin-b.command "xargs -P1 -I% echo b%" --add && > + > + cat >input <<-EOF && > + 1 > + 2 > + 3 > + EOF > + > + cat >expected <<-EOF && > + a1 > + a2 > + a3 > + b1 > + b2 > + b3 > + EOF For any here-docs without variables, use <<-\EOF, note the backslash.
On Tue, Aug 24, 2021 at 09:30:25PM +0200, Ævar Arnfjörð Bjarmason wrote: Disclaimer: I was writing a pretty involved reply to this and my tmux session crashed, so hopefully I can recall it well enough. Sorry if anything is confusing :) > > On Wed, Aug 18 2021, Emily Shaffer wrote: > > > Teach the hook.[hc] library to parse configs to populare the list of > > hooks to run for a given event. > > s/populare/populate/ ack > > > Multiple commands can be specified for a given hook by providing > > multiple "hook.<friendly-name>.command = <path-to-hook>" and > > "hook.<friendly-name>.event = <hook-event>" lines. Hooks will be run in > > config order of the "hook.<name>.event" lines. > > The "will be run in order" probably needs some amending to account for > --jobs, no? I changed it to "started in order", and tacked on "(but may be run in parallel)". > > > For example: > > > > $ git config --list | grep ^hook > > hook.bar.command=~/bar.sh > > hook.bar.event=pre-commit > > Perhaps the example should use: > > git config --get-regexp '^hook\.' Sure, I better not inflict my crappy habits on readers ;) > > > $ git hook run > > # Runs ~/bar.sh > > # Runs .git/hooks/pre-commit > > And this "# Runs" is not actual output by git, but just an explanation > for what happens, better to reword it somehow so it doesn't give that > impression. > > But the example also seems to be broken, surely it should be "git hook > run bar", not "git hook run"? Reading ahead, but presumably no-arg > doesn't run all known hooks... Ah, even the suggestion was wrong - it should be `git hook run pre-commit`. Zzzz. :) > > > Signed-off-by: Emily Shaffer <emilyshaffer@google.com> > > --- > > Documentation/config/hook.txt | 18 ++++ > > Documentation/git-hook.txt | 57 ++++++++++++- > > builtin/hook.c | 3 +- > > hook.c | 153 ++++++++++++++++++++++++++++++---- > > hook.h | 7 +- > > t/t1800-hook.sh | 141 ++++++++++++++++++++++++++++++- > > 6 files changed, 357 insertions(+), 22 deletions(-) > > > > diff --git a/Documentation/config/hook.txt b/Documentation/config/hook.txt > > index 96d3d6572c..c394756328 100644 > > --- a/Documentation/config/hook.txt > > +++ b/Documentation/config/hook.txt > > @@ -1,3 +1,21 @@ > > +hook.<name>.command:: > > + A command to execute whenever `hook.<name>` is invoked. `<name>` should > > + be a unique "friendly" name which you can use to identify this hook > > + command. (You can specify when to invoke this command with > > + `hook.<name>.event`.) The value can be an executable on your device or a > > + oneliner for your shell. If more than one value is specified for the > > + same `<name>`, the last value parsed will be the only command executed. > > + See linkgit:git-hook[1]. > > Hrm, so here we say "If more than one value is specified for ... the > last value parsed will be the only command executed", but in the commit > message it's: > > Multiple commands can be specified for a given hook by providing > multiple "hook.<friendly-name>.command = <path-to-hook>" and > "hook.<friendly-name>.event = <hook-event>" lines. > > As we've discussed earlier I've got a preference for the former, but > just reading this commit message & doc the for the first time I still > don't know what you went for, looks like one or the other needs > updating. I'm intentionally not reading ahead as I review this. > > Saying that it's a "unique name", but also discussing what happens if > it's not unique in the sense that we have multiple "hook.<name>.*" is a > bit confusing. I think I know what you're going for, perhaps something > like this would be better to describe it: > > For a "hook.<name>.{command,event}" hook entry you'll need to pick a > "<name>" that's not shared with any other hook, if you do normal > single-value config semantics apply and git will use the last > provided value. > > Or something... Yeah, I ended up reworking this a lot. I think the manpage needs some structuring work, to be honest - I wrote a lot about how to use 'git hook run' to add hooks to your wrapper around Git, for example, and stuck it in a section. While I'm waiting for your next reroll, I'm planning to send the doc to a tech writer we've got on loan internally and see if they've got any tips for us here. Hopefully they can help us out :) I'm going to snip the rest of the doc comments, because while I know I took action on them last week, I'm not sure I recall what I changed; and I'm hoping to get a third party to take a look. I did read them all, I promise :) > > + /* to enable oneliners, let config-specified hooks run in shell. > > + * config-specified hooks have a name. */ > > nit: usual style is multi-line comments like: > > /* > * some text[...] > * some more... > */ > > Not: > > /* text here right away[...] > * some more ... */ ACK. By the way, anybody have tips for making Vim gracefully transition from /* happily typing a single line comment to /* * happily typing a single line comment that * became super long Wonder if anything is working quite well for anybody, because I mess this up all the time ;) I guess I could just check for this kind of thing at pre-commit time. Maybe with a hook. ;) ;) > > > + cp->use_shell = !!run_me->name; > > + > > /* add command */ > > - if (hook_cb->options->absolute_path) > > - strvec_push(&cp->args, absolute_path(run_me->hook_path)); > > - else > > - strvec_push(&cp->args, run_me->hook_path); > > + if (run_me->name) { > > + /* ...from config */ > > + struct strbuf cmd_key = STRBUF_INIT; > > + char *command = NULL; > > + > > + strbuf_addf(&cmd_key, "hook.%s.command", run_me->name); > > Missing strbuf_release() for this later? Yep, thanks. > > > + if (git_config_get_string(cmd_key.buf, &command)) { > > + /* TODO test me! */ > > ...seems easy enough to just have a test for..., i.e. an *.event entry > with no *.command. Yeah, this TODO was probably for myself, so I wouldn't push the reroll without writing the test. That worked really well.... > > > + die(_("'hook.%s.command' must be configured " > > + "or 'hook.%s.event' must be removed; aborting.\n"), > > + run_me->name, run_me->name); > > + } > > + > > + strvec_push(&cp->args, command); > > + } else { > > + /* ...from hookdir. */ > > + const char *hook_path = NULL; > > + /* > > + * > > Nit: Too few \n before the text in a comment earlier, too many here :) *facepalm* > > > + * At this point we are already running, so don't validate > > + * whether the hook name is known or not. > > ...because it was done earlier somewhere, or...? Yeah, that validation occurs in list_hooks() instead. I'll make the comment more clear. > > > + */ > > + hook_path = find_hook_gently(hook_cb->hook_name); > > + if (!hook_path) > > + BUG("hookdir hook in hook list but no hookdir hook present in filesystem"); > > + > > + if (hook_cb->options->absolute_path) > > + hook_path = absolute_path(hook_path); > > + > > + strvec_push(&cp->args, hook_path); > > + } > > + > > > > /* > > * add passed-in argv, without expanding - let the user get back > > @@ -228,8 +346,11 @@ static int notify_start_failure(struct strbuf *out, > > > > hook_cb->rc |= 1; > > > > - strbuf_addf(out, _("Couldn't start hook '%s'\n"), > > - attempted->hook_path); > > + if (attempted->name) > > + strbuf_addf(out, _("Couldn't start hook '%s'\n"), > > + attempted->name); > > + else > > + strbuf_addstr(out, _("Couldn't start hook from hooks directory\n")); > > > > return 1; > > } > > diff --git a/hook.h b/hook.h > > index 6b7b2d14d2..621bd2cde1 100644 > > --- a/hook.h > > +++ b/hook.h > > @@ -27,8 +27,11 @@ int hook_exists(const char *hookname); > > > > struct hook { > > struct list_head list; > > - /* The path to the hook */ > > - const char *hook_path; > > + /* > > + * The friendly name of the hook. NULL indicates the hook is from the > > + * hookdir. > > + */ > > + const char *name; > > > > /* > > * Use this to keep state for your feed_pipe_fn if you are using > > diff --git a/t/t1800-hook.sh b/t/t1800-hook.sh > > index 217db848b3..ef2432f53a 100755 > > --- a/t/t1800-hook.sh > > +++ b/t/t1800-hook.sh > > @@ -1,13 +1,29 @@ > > #!/bin/bash > > > > -test_description='git-hook command' > > +test_description='git-hook command and config-managed multihooks' > > > > . ./test-lib.sh > > > > +setup_hooks () { > > + test_config hook.ghi.event pre-commit --add > > + test_config hook.ghi.command "/path/ghi" --add > > + test_config_global hook.def.event pre-commit --add > > + test_config_global hook.def.command "/path/def" --add > > Isn't --add redundant here? Seems no test is stressing multi-value > hook.{ghi,def}.* from a quick glance... Sounds like a failing in the test suite ;) Will remove --add from the command configs, and will stick in test for a hook to run in multiple places. I went back and forth over whether to add the extra .event config in the setup vs. in a specific test, and I decided that by adding it in the setup, we get some implicit "this is fine" assurance, as well as the one explicit test (which I added just now) that the hook shows up in both places. > > > +} > > + > > +setup_hookdir () { > > + mkdir .git/hooks > > + write_script .git/hooks/pre-commit <<-EOF > > + echo \"Legacy Hook\" > > + EOF > > + test_when_finished rm -rf .git/hooks > > +} > > + > > test_expect_success 'git hook usage' ' > > test_expect_code 129 git hook && > > test_expect_code 129 git hook run && > > test_expect_code 129 git hook run -h && > > + test_expect_code 129 git hook list -h && > > Doesn't this belong in a previous commit that added "git hook list", not > here? Yes, nice. Thanks. > > > test_expect_code 129 git hook run --unknown 2>err && > > grep "unknown option" err > > ' > > @@ -153,4 +169,127 @@ test_expect_success 'stdin to hooks' ' > > test_cmp expect actual > > ' > > > > +test_expect_success 'git hook list orders by config order' ' > > + setup_hooks && > > + > > + cat >expected <<-EOF && > > + def > > + ghi > > + EOF > > + > > + git hook list pre-commit >actual && > > + test_cmp expected actual > > +' > > + > > +test_expect_success 'git hook list reorders on duplicate event declarations' ' > > + setup_hooks && > > + > > + # 'def' is usually configured globally; move it to the end by > > + # configuring it locally. > > + test_config hook.def.event "pre-commit" --add && > > Ah, well the --add belongs here, but not needed in setup_hooks, right? Addressed above. > > + > > + cat >input <<-EOF && > > + 1 > > + 2 > > + 3 > > + EOF > > + > > + cat >expected <<-EOF && > > + a1 > > + a2 > > + a3 > > + b1 > > + b2 > > + b3 > > + EOF > > For any here-docs without variables, use <<-\EOF, note the backslash. ACK Thanks. - Emily
diff --git a/Documentation/config/hook.txt b/Documentation/config/hook.txt index 96d3d6572c..c394756328 100644 --- a/Documentation/config/hook.txt +++ b/Documentation/config/hook.txt @@ -1,3 +1,21 @@ +hook.<name>.command:: + A command to execute whenever `hook.<name>` is invoked. `<name>` should + be a unique "friendly" name which you can use to identify this hook + command. (You can specify when to invoke this command with + `hook.<name>.event`.) The value can be an executable on your device or a + oneliner for your shell. If more than one value is specified for the + same `<name>`, the last value parsed will be the only command executed. + See linkgit:git-hook[1]. + +hook.<name>.event:: + The hook events which should invoke `hook.<name>`. `<name>` should be a + unique "friendly" name which you can use to identify this hook. The + value should be the name of a hook event, like "pre-commit" or "update". + (See linkgit:githooks[5] for a complete list of hooks Git knows about.) + On the specified event, the associated `hook.<name>.command` will be + executed. More than one event can be specified if you wish for + `hook.<name>` to execute on multiple events. See linkgit:git-hook[1]. + hook.jobs:: Specifies how many hooks can be run simultaneously during parallelized hook execution. If unspecified, defaults to the number of processors on diff --git a/Documentation/git-hook.txt b/Documentation/git-hook.txt index d1db084e4f..9c6cbdc2eb 100644 --- a/Documentation/git-hook.txt +++ b/Documentation/git-hook.txt @@ -27,12 +27,65 @@ Git is unlikely to use for a native hook later on. For example, Git is much less likely to create a `mytool-validate-commit` hook than it is to create a `validate-commit` hook. +This command parses the default configuration files for pairs of configs like +so: + + [hook "linter"] + event = pre-commit + command = ~/bin/linter --c + +In this example, `[hook "linter"]` represents one script - `~/bin/linter --c` - +which can be shared by many repos, and even by many hook events, if appropriate. + +Commands are run in the order Git encounters their associated +`hook.<name>.event` configs during the configuration parse (see +linkgit:git-config[1]). Although multiple `hook.linter.event` configs can be +added, only one `hook.linter.command` event is valid - Git uses "last-one-wins" +to determine which command to run. + +So if you wanted your linter to run when you commit as well as when you push, +you would configure it like so: + + [hook "linter"] + event = pre-commit + event = pre-push + command = ~/bin/linter --c + +With this config, `~/bin/linter --c` would be run by Git before a commit is +generated (during `pre-commit`) as well as before a push is performed (during +`pre-push`). + +And if you wanted to run your linter as well as a secret-leak detector during +only the "pre-commit" hook event, you would configure it instead like so: + + [hook "linter"] + event = pre-commit + command = ~/bin/linter --c + [hook "no-leaks"] + event = pre-commit + command = ~/bin/leak-detector + +With this config, before a commit is generated (during `pre-commit`), Git would +first start `~/bin/linter --c` and second start `~/bin/leak-detector`. It would +evaluate the output of each when deciding whether to proceed with the commit. + +For a full list of hook events which you can set your `hook.<name>.event` to, +and how hooks are invoked during those events, see linkgit:githooks[5]. + +In general, when instructions suggest adding a script to +`.git/hooks/<hook-event>`, you can specify it in the config instead by running +`git config --add hook.<some-name>.command <path-to-script> && git config --add +hook.<some-name>.event <hook-event>` - this way you can share the script between +multiple repos. That is, `cp ~/my-script.sh ~/project/.git/hooks/pre-commit` +would become `git config --add hook.my-script.command ~/my-script.sh && git +config --add hook.my-script.event pre-commit`. + SUBCOMMANDS ----------- run:: - Run the `<hook-name>` hook. See linkgit:githooks[5] for - the hook names we support. + Runs hooks configured for `<hook-name>`, in the order they are + discovered during the config parse. + Any positional arguments to the hook should be passed after an optional `--` (or `--end-of-options`, see linkgit:gitcli[7]). The diff --git a/builtin/hook.c b/builtin/hook.c index 80397d39f5..50233366a8 100644 --- a/builtin/hook.c +++ b/builtin/hook.c @@ -55,7 +55,8 @@ static int list(int argc, const char **argv, const char *prefix) struct hook *item = list_entry(pos, struct hook, list); item = list_entry(pos, struct hook, list); if (item) - printf("%s\n", item->hook_path); + printf("%s\n", item->name ? item->name + : _("hook from hookdir")); } clear_hook_list(head); diff --git a/hook.c b/hook.c index ab1e86ddcf..581d87cbbd 100644 --- a/hook.c +++ b/hook.c @@ -11,6 +11,50 @@ static void free_hook(struct hook *ptr) free(ptr); } +/* + * Walks the linked list at 'head' to check if any hook named 'name' + * already exists. Returns a pointer to that hook if so, otherwise returns NULL. + */ +static struct hook *find_hook_by_name(struct list_head *head, + const char *name) +{ + struct list_head *pos = NULL, *tmp = NULL; + struct hook *found = NULL; + + list_for_each_safe(pos, tmp, head) { + struct hook *it = list_entry(pos, struct hook, list); + if (!strcmp(it->name, name)) { + list_del(pos); + found = it; + break; + } + } + return found; +} + +/* + * Adds a hook if it's not already in the list, or moves it to the tail of the + * list if it was already there. name == NULL indicates it's from the hookdir; + * just append it in this case. + */ +static void append_or_move_hook(struct list_head *head, const char *name) +{ + struct hook *to_add = NULL; + + /* if it's not from hookdir, check if the hook is already in the list */ + if (name) + to_add = find_hook_by_name(head, name); + + if (!to_add) { + /* adding a new hook, not moving an old one */ + to_add = xmalloc(sizeof(*to_add)); + to_add->name = name; + to_add->feed_pipe_cb_data = NULL; + } + + list_add_tail(&to_add->list, head); +} + static void remove_hook(struct list_head *to_remove) { struct hook *hook_to_remove = list_entry(to_remove, struct hook, list); @@ -103,10 +147,50 @@ int hook_exists(const char *name) struct hook_config_cb { - struct strbuf *hook_key; + const char *hook_event; struct list_head *list; }; +/* + * Callback for git_config which adds configured hooks to a hook list. Hooks + * can be configured by specifying both hook.<friend-name>.command = <path> and + * hook.<friendly-name>.event = <hook-event>. + */ +static int hook_config_lookup(const char *key, const char *value, void *cb_data) +{ + struct hook_config_cb *data = cb_data; + const char *subsection, *parsed_key; + size_t subsection_len = 0; + struct strbuf subsection_cpy = STRBUF_INIT; + + /* + * Don't bother doing the expensive parse if there's no + * chance that the config matches 'hook.myhook.event = hook_event'. + */ + if (!value || strcmp(value, data->hook_event)) + return 0; + + /* Looking for "hook.friendlyname.event = hook_event" */ + if (parse_config_key(key, + "hook", + &subsection, + &subsection_len, + &parsed_key) || + strcmp(parsed_key, "event")) + return 0; + + /* + * 'subsection' is a pointer to the internals of 'key', which we don't + * own the memory for. Copy it away to the hook list. + */ + strbuf_add(&subsection_cpy, subsection, subsection_len); + + append_or_move_hook(data->list, strbuf_detach(&subsection_cpy, NULL)); + + + return 0; +} + struct list_head *list_hooks(const char *hookname) { if (!known_hook(hookname)) @@ -119,21 +203,23 @@ struct list_head *list_hooks(const char *hookname) struct list_head *list_hooks_gently(const char *hookname) { struct list_head *hook_head = xmalloc(sizeof(struct list_head)); + struct hook_config_cb cb_data = { + .hook_event = hookname, + .list = hook_head, + }; INIT_LIST_HEAD(hook_head); if (!hookname) BUG("null hookname was provided to hook_list()!"); - if (have_git_dir()) { - const char *hook_path = find_hook_gently(hookname); - if (hook_path) { - struct hook *to_add = xmalloc(sizeof(*to_add)); - to_add->hook_path = hook_path; - to_add->feed_pipe_cb_data = NULL; - list_add_tail(&to_add->list, hook_head); - } - } + /* Add the hooks from the config, e.g. hook.myhook.event = pre-commit */ + git_config(hook_config_lookup, &cb_data); + + /* Add the hook from the hookdir. The placeholder makes it easier to + * allocate work in pick_next_hook. */ + if (find_hook_gently(hookname)) + append_or_move_hook(hook_head, NULL); return hook_head; } @@ -194,11 +280,43 @@ static int pick_next_hook(struct child_process *cp, cp->trace2_hook_name = hook_cb->hook_name; cp->dir = hook_cb->options->dir; + /* to enable oneliners, let config-specified hooks run in shell. + * config-specified hooks have a name. */ + cp->use_shell = !!run_me->name; + /* add command */ - if (hook_cb->options->absolute_path) - strvec_push(&cp->args, absolute_path(run_me->hook_path)); - else - strvec_push(&cp->args, run_me->hook_path); + if (run_me->name) { + /* ...from config */ + struct strbuf cmd_key = STRBUF_INIT; + char *command = NULL; + + strbuf_addf(&cmd_key, "hook.%s.command", run_me->name); + if (git_config_get_string(cmd_key.buf, &command)) { + /* TODO test me! */ + die(_("'hook.%s.command' must be configured " + "or 'hook.%s.event' must be removed; aborting.\n"), + run_me->name, run_me->name); + } + + strvec_push(&cp->args, command); + } else { + /* ...from hookdir. */ + const char *hook_path = NULL; + /* + * + * At this point we are already running, so don't validate + * whether the hook name is known or not. + */ + hook_path = find_hook_gently(hook_cb->hook_name); + if (!hook_path) + BUG("hookdir hook in hook list but no hookdir hook present in filesystem"); + + if (hook_cb->options->absolute_path) + hook_path = absolute_path(hook_path); + + strvec_push(&cp->args, hook_path); + } + /* * add passed-in argv, without expanding - let the user get back @@ -228,8 +346,11 @@ static int notify_start_failure(struct strbuf *out, hook_cb->rc |= 1; - strbuf_addf(out, _("Couldn't start hook '%s'\n"), - attempted->hook_path); + if (attempted->name) + strbuf_addf(out, _("Couldn't start hook '%s'\n"), + attempted->name); + else + strbuf_addstr(out, _("Couldn't start hook from hooks directory\n")); return 1; } diff --git a/hook.h b/hook.h index 6b7b2d14d2..621bd2cde1 100644 --- a/hook.h +++ b/hook.h @@ -27,8 +27,11 @@ int hook_exists(const char *hookname); struct hook { struct list_head list; - /* The path to the hook */ - const char *hook_path; + /* + * The friendly name of the hook. NULL indicates the hook is from the + * hookdir. + */ + const char *name; /* * Use this to keep state for your feed_pipe_fn if you are using diff --git a/t/t1800-hook.sh b/t/t1800-hook.sh index 217db848b3..ef2432f53a 100755 --- a/t/t1800-hook.sh +++ b/t/t1800-hook.sh @@ -1,13 +1,29 @@ #!/bin/bash -test_description='git-hook command' +test_description='git-hook command and config-managed multihooks' . ./test-lib.sh +setup_hooks () { + test_config hook.ghi.event pre-commit --add + test_config hook.ghi.command "/path/ghi" --add + test_config_global hook.def.event pre-commit --add + test_config_global hook.def.command "/path/def" --add +} + +setup_hookdir () { + mkdir .git/hooks + write_script .git/hooks/pre-commit <<-EOF + echo \"Legacy Hook\" + EOF + test_when_finished rm -rf .git/hooks +} + test_expect_success 'git hook usage' ' test_expect_code 129 git hook && test_expect_code 129 git hook run && test_expect_code 129 git hook run -h && + test_expect_code 129 git hook list -h && test_expect_code 129 git hook run --unknown 2>err && grep "unknown option" err ' @@ -153,4 +169,127 @@ test_expect_success 'stdin to hooks' ' test_cmp expect actual ' +test_expect_success 'git hook list orders by config order' ' + setup_hooks && + + cat >expected <<-EOF && + def + ghi + EOF + + git hook list pre-commit >actual && + test_cmp expected actual +' + +test_expect_success 'git hook list reorders on duplicate event declarations' ' + setup_hooks && + + # 'def' is usually configured globally; move it to the end by + # configuring it locally. + test_config hook.def.event "pre-commit" --add && + + cat >expected <<-EOF && + ghi + def + EOF + + git hook list pre-commit >actual && + test_cmp expected actual +' + +test_expect_success 'git hook list shows hooks from the hookdir' ' + setup_hookdir && + + cat >expected <<-EOF && + hook from hookdir + EOF + + git hook list pre-commit >actual && + test_cmp expected actual +' + +test_expect_success 'inline hook definitions execute oneliners' ' + test_config hook.oneliner.event "pre-commit" && + test_config hook.oneliner.command "echo \"Hello World\"" && + + echo "Hello World" >expected && + + # hooks are run with stdout_to_stderr = 1 + git hook run pre-commit 2>actual && + test_cmp expected actual +' + +test_expect_success 'inline hook definitions resolve paths' ' + write_script sample-hook.sh <<-EOF && + echo \"Sample Hook\" + EOF + + test_when_finished "rm sample-hook.sh" && + + test_config hook.sample-hook.event pre-commit && + test_config hook.sample-hook.command "\"$(pwd)/sample-hook.sh\"" && + + echo \"Sample Hook\" >expected && + + # hooks are run with stdout_to_stderr = 1 + git hook run pre-commit 2>actual && + test_cmp expected actual +' + +test_expect_success 'hookdir hook included in git hook run' ' + setup_hookdir && + + echo \"Legacy Hook\" >expected && + + # hooks are run with stdout_to_stderr = 1 + git hook run pre-commit 2>actual && + test_cmp expected actual +' + +test_expect_success 'stdin to multiple hooks' ' + test_config hook.stdin-a.event "test-hook" --add && + test_config hook.stdin-a.command "xargs -P1 -I% echo a%" --add && + test_config hook.stdin-b.event "test-hook" --add && + test_config hook.stdin-b.command "xargs -P1 -I% echo b%" --add && + + cat >input <<-EOF && + 1 + 2 + 3 + EOF + + cat >expected <<-EOF && + a1 + a2 + a3 + b1 + b2 + b3 + EOF + + git hook run --to-stdin=input test-hook 2>actual && + test_cmp expected actual +' + +test_expect_success 'multiple hooks in series' ' + test_config hook.series-1.event "test-hook" && + test_config hook.series-1.command "echo 1" --add && + test_config hook.series-2.event "test-hook" && + test_config hook.series-2.command "echo 2" --add && + mkdir .git/hooks && + write_script .git/hooks/test-hook <<-EOF && + echo 3 + EOF + + cat >expected <<-EOF && + 1 + 2 + 3 + EOF + + git hook run -j1 test-hook 2>actual && + test_cmp expected actual && + + rm -rf .git/hooks +' test_done
Teach the hook.[hc] library to parse configs to populare the list of hooks to run for a given event. Multiple commands can be specified for a given hook by providing multiple "hook.<friendly-name>.command = <path-to-hook>" and "hook.<friendly-name>.event = <hook-event>" lines. Hooks will be run in config order of the "hook.<name>.event" lines. For example: $ git config --list | grep ^hook hook.bar.command=~/bar.sh hook.bar.event=pre-commit $ git hook run # Runs ~/bar.sh # Runs .git/hooks/pre-commit Signed-off-by: Emily Shaffer <emilyshaffer@google.com> --- Documentation/config/hook.txt | 18 ++++ Documentation/git-hook.txt | 57 ++++++++++++- builtin/hook.c | 3 +- hook.c | 153 ++++++++++++++++++++++++++++++---- hook.h | 7 +- t/t1800-hook.sh | 141 ++++++++++++++++++++++++++++++- 6 files changed, 357 insertions(+), 22 deletions(-)