On Thu, Jul 22 2021, Emily Shaffer wrote:
> On Fri, Jul 16, 2021 at 10:52:27AM +0200, Ævar Arnfjörð Bjarmason wrote:
>>
>>
>> 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.
>
>>
>> > + if (argc < 1) {
>> > + usage_msg_opt(_("You must specify a hook event name to list."),
>> > + builtin_hook_usage, list_options);
>> > + }
>>
>> {} braces not needed.
> ACK
>>
>>
>> > + 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 ;)
*nod*
>>
>> > 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?
I don't think you're missing anything. I replied on that "struct hook"
in another E-Mail, i.e. I think I just misread parts of this. Thanks.
