Re: [PATCH v7 04/17] hook: include hookdir hook in list

On Sat, Jan 30, 2021 at 07:20:22PM -0800, Jonathan Tan wrote:

quoted

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.

Sure.

quoted

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.

Yep, this is an artifact of the review process (explaining why I didn't
do something weird, which I did in an earlier version, but now it
doesn't make sense to mention it at all). Deleted.

quoted

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.

Yeah, that makes sense. Will do.

quoted

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

Oh, that's a cool way to indicate that. Thanks, I did that, and learned
something new!

quoted

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?

Sure. It doesn't make a difference now but I see that would be nice for
futureproofing.

The tests look good.

Thanks.