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

On Tue, Aug 24, 2021 at 05:08:25PM +0200, Ævar Arnfjörð Bjarmason wrote:

On Wed, Aug 18 2021, Emily Shaffer wrote:

quoted

+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?

Yeah, I think you are right. Will switch.

quoted

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

I was curious, because today I learned about puts() ;), so I checked
(sorry for escape gore):

  $ gg puts\( | wc -l
  217
  $ gg "printf(\"%s\(\\\\n\)\?\"" | wc -l
  96

So looks like it is indeed more idiomatic by about 2x to just use
puts(). Will switch.

quoted

+	}
+
+	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...

Sounds like you talked yourself out of it before I could. Noop ;)

quoted

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

Ah, leftover WIP code indeed. Will drop it. I do think, though, that
"hook from hookdir" is an ugly thing to say in list(), so any better
suggestions welcome.

quoted

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

..ditto...

ACK

quoted

 
 	if (!hookname)

@@ -103,8 +104,6 @@ struct list_head *list_hooks(const char *hookname)
 
 	if (have_git_dir()) {
 		const char *hook_path = find_hook(hookname);
-

... earlier notes about whitespace churn...

quoted

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

Thanks.
 - Emily