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

From: Emily Shaffer <emilyshaffer@google.com>
Date: Aug 26, 2021, 21:43 UTC
Message-ID: <YSgK5Q4uqX8RPXXZ@google.com>
In-Reply-To: <87tujeev1h.fsf@evledraar.gmail.com>

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

Show 26 quoted lines

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

Yeah, I think you are right. Will switch.

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

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.

Show 12 quoted lines

> 
> > +	}
> > +
> > +	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 ;)

Show 6 quoted lines

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

Show 8 quoted lines

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

ACK

Show 15 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;
>

Thanks.
 - Emily