Re: [PATCH v7 04/17] hook: include hookdir hook in list
- From
- Jonathan Tan <jonathantanmy@google.com>
- Date
- Jan 31, 2021, 03:20 UTC
- Message-ID
- <20210131032022.1018820-1-jonathantanmy@google.com>
- In-Reply-To
- <20201222000220.1491091-5-emilyshaffer@google.com>
> Historically, hooks are declared by placing an executable into > $GIT_DIR/hooks/$HOOKNAME (or $HOOKDIR/$HOOKNAME). Although hooks taken > from the config are more featureful than hooks placed in the $HOOKDIR, > those hooks should not stop working for users who already have them.
Maybe explicitly add that we're listing them in the list with a "hookdir:" prefix.
> Legacy hooks should be run directly, not in shell. We know that they are > a path to an executable, not a oneliner script - and running them > directly takes care of path quoting concerns for us for free.
Not sure what this paragraph is doing here.
Show 9 quoted lines
> diff --git a/builtin/hook.c b/builtin/hook.c > index 4d36de52f8..a0013ae4d7 100644 > --- a/builtin/hook.c > +++ b/builtin/hook.c > @@ -16,6 +16,7 @@ static int list(int argc, const char **argv, const char *prefix) > struct list_head *head, *pos; > struct hook *item; > struct strbuf hookname = STRBUF_INIT; > + struct strbuf hookdir_annotation = STRBUF_INIT;
Right now this is never set? Maybe hold off on adding this until we set something.
Show 12 quoted lines
> @@ -110,6 +113,18 @@ struct list_head* hook_list(const struct strbuf* hookname)
>
> git_config(hook_config_lookup, (void*)&cb_data);
>
> + if (have_git_dir())
> + legacy_hook_path = find_hook(hookname->buf);
> +
> + /* Unconditionally add legacy hook, but annotate it. */
> + if (legacy_hook_path) {
> + struct hook *legacy_hook;
> +
> + append_or_move_hook(hook_head, absolute_path(legacy_hook_path));Both find_hook() and absolute_path() use static buffers to hold their return values, which makes me a bit nervous. Perhaps put them all under the same "if (have_git_dir())" so that it's clearer that we're not supposed to insert code arbitrarily between their invocation and their usage.
Show 9 quoted lines
> diff --git a/hook.h b/hook.h > index 8ffc4f14b6..5750634c83 100644 > --- a/hook.h > +++ b/hook.h > @@ -12,6 +12,7 @@ struct hook > enum config_scope origin; > /* The literal command to run. */ > struct strbuf command; > + int from_hookdir;
unsigned from_hookdir : 1?
The tests look good.