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@xxxxxxxxxx>
> > ---
> > 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
