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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Jul 16, 2021, 08:52 UTC
Message-ID: <87o8b2y6u1.fsf@evledraar.gmail.com>
In-Reply-To: <20210715232603.3415111-4-emilyshaffer@google.com>

On Thu, Jul 15 2021, Emily Shaffer wrote:

Show 5 quoted lines

>  static const char * const builtin_hook_usage[] = {
>  	N_("git hook <command> [...]"),
> +	N_("git hook list <hookname>"),
>  	N_("git hook run [<args>] <hook-name> [-- <hook-args>]"),
>  	NULL

Uses <hook-name> already, let's use that too. I can't remember if it's something I changed myself, or if your original version used both and I picked one for consistency, or...

Anyway, I can re-roll the base topic or whatever, but let's have the end result use one or the other.

> +	if (argc < 1) {
> +		usage_msg_opt(_("You must specify a hook event name to list."),
> +			      builtin_hook_usage, list_options);
> +	}

{} braces not needed.

> +	if (!strcmp(argv[0], "list"))
> +		return list(argc, argv, prefix);
>  	if (!strcmp(argv[0], "run"))

This should be "else if" now.

(Doesn't matter for code execution, just IMO readability, but I'll leave that to you ... :)

Show 34 quoted lines

>  		return run(argc, argv, prefix);
>  	else
> diff --git a/hook.c b/hook.c
> index 935751fa6c..19138a8290 100644
> --- a/hook.c
> +++ b/hook.c
> @@ -104,22 +104,20 @@ int hook_exists(const char *name)
>  struct list_head* hook_list(const char* hookname)
>  {
>  	struct list_head *hook_head = xmalloc(sizeof(struct list_head));
> +	const char *hook_path = find_hook(hookname);
> +
>  
>  	INIT_LIST_HEAD(hook_head);
>  
>  	if (!hookname)
>  		return 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);
> -		}
> +	/* 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);

Maybe we should have a INIT for "struct hook" too? This also needlessly leaves behind an un-free'd hook struct in a way that it wouldn't if we just had this on the stack, no?