Re: [PATCH v3 3/6] hook: introduce "git hook list"

On Wed, Aug 18 2021, Emily Shaffer wrote:

+static int list(int argc, const char **argv, const char *prefix)
+{
+	struct list_head *head, *pos;
+	const char *hookname = NULL;
+	struct strbuf hookdir_annotation = STRBUF_INIT;
+
+	struct option list_options[] = {
+		OPT_END(),
+	};
+
+	argc = parse_options(argc, argv, prefix, list_options,
+			     builtin_hook_list_usage, 0);
+
+	if (argc < 1)
+		usage_msg_opt(_("You must specify a hook event name to list."),
+			      builtin_hook_list_usage, list_options);

Untested, but aren't we silently ignoring:

    git hook list pre-receive some extra gar bage here

I.e. shouldn't this be an "argc != 1" check?

+
+	hookname = argv[0];
+
+	head = hook_list(hookname);
+
+	if (list_empty(head))
+		return 1;
+
+	list_for_each(pos, head) {
+		struct hook *item = list_entry(pos, struct hook, list);
+		item = list_entry(pos, struct hook, list);
+		if (item)
+			printf("%s\n", item->hook_path);

Nit/suggestion: use puts(x) instead of printf("%s\n", x), but that's
also a bikeshedding/style preference, so ignore if you disagree...

+	}
+
+	clear_hook_list(head);

Nit/API suggestion: Maybe s/list_for_each/list_for_each_safe/ and
remove_hook() in the loop would make more sense for this one-shot caller
than iterating over the list twice?

Anyway, currently remove_hook() is static, and it's probably good to not
peek behind the curtain here, so on second thought clear_hook_list() is
probably best...

+	strbuf_release(&hookdir_annotation);

This function did nothing with hookdir_annotation. Looks like leftover
WIP code, but maybe it's used in (and should be moved to) a later
commit, let's keep reading...

[...]
 	struct list_head *hook_head = xmalloc(sizeof(struct list_head));
 
+
 	INIT_LIST_HEAD(hook_head);

..ditto...

... earlier notes about whitespace churn...

-		/* Add the hook from the hookdir */
 		if (hook_path) {
 			struct hook *to_add = xmalloc(sizeof(*to_add));
 			to_add->hook_path = hook_path;