Re: [PATCH v2 1/6] hook: run a list of hooks instead

Emily Shaffer [off-list ref] writes:

Natural.  We used to use the path to the hook because we were
expecting only one. We now use the name to find a list of hooks.

All the caller sees is just list_head without any direct visibility
into it, which feels like a great abstraction.  Presumably everything
will go through the API functions taking this opaque "list of hooks"
thing (or "the first one in the list" if the API function does not
iterate over it, perhaps?).

OK.

+	if (list_empty(hooks)) {
+		/* ... act like run_hooks_oneshot() under --ignore-missing */
+		if (ignore_missing)
+			return 0;

OK.

 		error("cannot find a hook named %s", hook_name);
 		return 1;
 	}

Lose the extra {}, as we do not do more than the above free() even
at the end of the series?

+	free(ptr);
+}
+
+static void remove_hook(struct list_head *to_remove)
+{
+	struct hook *hook_to_remove = list_entry(to_remove, struct hook, list);
+	list_del(to_remove);
+	free_hook(hook_to_remove);
+}
+
+void clear_hook_list(struct list_head *head)
+{
+	struct list_head *pos, *tmp;
+	list_for_each_safe(pos, tmp, head)
+		remove_hook(pos);
+}

OK.

+struct list_head* hook_list(const char* hookname)

Shift both of the asterisks; our asterisks do not stick to the type
but to the identifier.

+{
+	struct list_head *hook_head = xmalloc(sizeof(struct list_head));
+
+	INIT_LIST_HEAD(hook_head);
+
+	if (!hookname)
+		return NULL;

Checking for invalid hookname first would avoid leaking hook_head,
no?  The caller of hook_list() we saw earlier immediately calls
list_empty() which will segfault.  The caller need to be tightened,
but I wonder if it would be a programmer's error to pass a NULL
hookname to this function.  If so, this can simply be a BUG() and
the earlier allocation and initialization of hook_head can stay
where they are.  Otherwise, the caller should see if this returns a
NULL.

+	if (have_git_dir()) {
+		const char *hook_path = find_hook(hookname);
+
+		/* Add the hook from the hookdir */
+		if (hook_path) {
+			struct hook *to_add = xmalloc(sizeof(*to_add));
+			to_add->hook_path = hook_path;
+			to_add->feed_pipe_cb_data = NULL;
+			list_add_tail(&to_add->list, hook_head);
+		}
+	}

Calling this function to grab a list of hooks when we are not in any
repository is not an error but just we get "there is nothing to
run".  Does the design give us a more useful behaviour, compared to
alternatives like "you have to be in a repository or calling this
function is an error"?

Not an objection wrapped in a rhetorical question, but a genuine
question.  "It would help this and that usecase" would be an ideal
answer, "We could do either way, but we happened to have written the
code this way first, and at the end of the series we did not see any
practical downsides" would also be a great answer.

+	return hook_head;
+}
+

+/*
+ * Provides a linked list of 'struct hook' detailing commands which should run
+ * in response to the 'hookname' event, in execution order.
+ */
+struct list_head* hook_list(const char *hookname);

struct list_head *hook_list(const char *hookname);

On Thu, Aug 12, 2021 at 10:25:03AM -0700, Junio C Hamano wrote:

Emily Shaffer [off-list ref] writes:

quoted

To prepare for multihook support, teach hook.[hc] to take a list of
hooks at run_hooks and run_found_hooks. Right now the list is always one
entry, but in the future we will allow users to supply more than one
executable for a single hook event.

Signed-off-by: Emily Shaffer <redacted>
---
 builtin/hook.c |  14 ++++---
 hook.c         | 103 +++++++++++++++++++++++++++++++++++--------------
 hook.h         |  16 +++++++-
 3 files changed, 96 insertions(+), 37 deletions(-)

diff --git a/builtin/hook.c b/builtin/hook.c
index 5eb7cf73a4..4d39c9e75e 100644
--- a/builtin/hook.c
+++ b/builtin/hook.c

@@ -25,7 +25,8 @@ static int run(int argc, const char **argv, const char *prefix)
 	struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT;
 	int ignore_missing = 0;
 	const char *hook_name;
-	const char *hook_path;
+	struct list_head *hooks;
+

Natural.  We used to use the path to the hook because we were
expecting only one. We now use the name to find a list of hooks.

All the caller sees is just list_head without any direct visibility
into it, which feels like a great abstraction.  Presumably everything
will go through the API functions taking this opaque "list of hooks"
thing (or "the first one in the list" if the API function does not
iterate over it, perhaps?).

Hum, I guess that in a later patch builtin/hook.c does learn to take
apart the list_head into a 'struct hook' to print the output of 'git
hook list'. I haven't read your review of that patch yet though.

quoted

diff --git a/hook.c b/hook.c
index ee20b2e365..80e150548c 100644
--- a/hook.c
+++ b/hook.c

@@ -4,6 +4,28 @@
 #include "hook-list.h"
 #include "config.h"
 
+static void free_hook(struct hook *ptr)
+{
+	if (ptr) {
+		free(ptr->feed_pipe_cb_data);
+	}

Lose the extra {}, as we do not do more than the above free() even
at the end of the series?

ACK

quoted

+struct list_head* hook_list(const char* hookname)

Shift both of the asterisks; our asterisks do not stick to the type
but to the identifier.

ACK

quoted

+{
+	struct list_head *hook_head = xmalloc(sizeof(struct list_head));
+
+	INIT_LIST_HEAD(hook_head);
+
+	if (!hookname)
+		return NULL;

Checking for invalid hookname first would avoid leaking hook_head,
no?  The caller of hook_list() we saw earlier immediately calls
list_empty() which will segfault.  The caller need to be tightened,
but I wonder if it would be a programmer's error to pass a NULL
hookname to this function.  If so, this can simply be a BUG() and
the earlier allocation and initialization of hook_head can stay
where they are.  Otherwise, the caller should see if this returns a
NULL.

Ah, good point. I think this makes sense to BUG().

quoted

+	if (have_git_dir()) {
+		const char *hook_path = find_hook(hookname);
+
+		/* Add the hook from the hookdir */
+		if (hook_path) {
+			struct hook *to_add = xmalloc(sizeof(*to_add));
+			to_add->hook_path = hook_path;
+			to_add->feed_pipe_cb_data = NULL;
+			list_add_tail(&to_add->list, hook_head);
+		}
+	}

Calling this function to grab a list of hooks when we are not in any
repository is not an error but just we get "there is nothing to
run".  Does the design give us a more useful behaviour, compared to
alternatives like "you have to be in a repository or calling this
function is an error"?

Later we enable calling hook_list without a gitdir, in patch 6
(https://lore.kernel.org/git/20210812004258.74318-7-emilyshaffer%40google.com).
So maybe the behavior as it is now is premature?

But leaving the hook list empty means we can behave gracefully if
anybody is calling a hook without necessarily being sure they have
gitdir. I would need to audit callsites to check if they are checking
whether they have one or not. I think in most cases they probably are
checking that.

Not an objection wrapped in a rhetorical question, but a genuine
question.  "It would help this and that usecase" would be an ideal
answer, "We could do either way, but we happened to have written the
code this way first, and at the end of the series we did not see any
practical downsides" would also be a great answer.

The main use case, today, for letting this work nicely even outside of
a gitdir, is sendemail-validate hook. But mostly, I was thinking that
when we're freed from dependence on .git/hooks/, there's no reason to
disallow out-of-repo hooks in case someone wants to add a new one in the
future - for example to 'git maintenance' daemon runs.

quoted

+	return hook_head;
+}
+

quoted

+/*
+ * Provides a linked list of 'struct hook' detailing commands which should run
+ * in response to the 'hookname' event, in execution order.
+ */
+struct list_head* hook_list(const char *hookname);

struct list_head *hook_list(const char *hookname);

ACK

Thanks for the thoughts.
 - Emily