On Tue, Aug 24, 2021 at 05:08:25PM +0200, Ævar Arnfjörð Bjarmason wrote:
>
>
> 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.
>
> > +
> > + 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.
>
> > + }
> > +
> > + 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 ;)
>
> > + 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.
>
> > [...]
> > struct list_head *hook_head = xmalloc(sizeof(struct list_head));
> >
> > +
> > INIT_LIST_HEAD(hook_head);
>
> ..ditto...
ACK
> >
> > 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
