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?
> +
> + 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...
> [...]
> struct list_head *hook_head = xmalloc(sizeof(struct list_head));
>
> +
> INIT_LIST_HEAD(hook_head);
..ditto...
>
> 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;
