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