Re: [PATCH v7 04/17] hook: include hookdir hook in list
From: Jonathan Tan <hidden>
Date: 2021-01-31 03:28:14
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.
quoted hunk ↗ jump to hunk
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.
quoted hunk ↗ jump to hunk
@@ -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.
quoted hunk ↗ jump to hunk
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.