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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Aug 24, 2021, 15:08 UTC
Message-ID: <87tujeev1h.fsf@evledraar.gmail.com>
In-Reply-To: <20210819033450.3382652-4-emilyshaffer@google.com>

On Wed, Aug 18 2021, Emily Shaffer wrote:

Show 16 quoted lines

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

Show 13 quoted lines

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

Show 5 quoted lines

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

..ditto...

Show 7 quoted lines

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

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