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

From: Emily Shaffer <emilyshaffer@google.com>
Date: Jul 22, 2021, 22:18 UTC
Message-ID: <YPnuvRQLzKa6h9kk@google.com>
In-Reply-To: <87o8b2y6u1.fsf@evledraar.gmail.com>

On Fri, Jul 16, 2021 at 10:52:27AM +0200, Ævar Arnfjörð Bjarmason wrote:

Show 16 quoted lines

> 
> 
> On Thu, Jul 15 2021, Emily Shaffer wrote:
> 
> >  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.

'hook-name' is fine, I'll use that. Thanks for pointing it out.

Show 7 quoted lines

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

ACK

Show 10 quoted lines

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

Eh, I think it's easier to read in the strcmps line up, so I will leave it this way ;)

Show 39 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?

I can clean it up here, but I don't think we need an initializer for struct hook. This code chunk gets moved into one of the list manipulators (append_or_move() or something) after the config is introduced.

I don't think it leaves an unfreed hook laying around, does it? list_add_tail() uses the malloc'd pointer, and we free() in clear_hook_list(). What am I missing?

 - Emily