Re: Feature request: hooks directory

[Wesley Schwengle <wesley@mintlab.nl>]

Hello Christian,

2018-09-03 6:00 GMT+02:00 Christian Couder <christian.couder@gmail.com>:
> Hi Wesley,
>
> On Sun, Sep 2, 2018 at 11:38 PM, Wesley Schwengle <wesley@mintlab.nl> wrote:
>> Hi all,
>>
>> I've made some progress with the hook.d implementation. It isn't
>> finished, as it is my first C project I'm still somewhat rocky with
>> how pointers and such work, but I'm getting somewhere. I haven't
>> broken any tests \o/.
>
> Great! Welcome to the Git community!

Thank you!

>>  You have a nice testsuite btw. Feel free to comment on the code.
>>
>> I've moved some of the hooks-code found in run-command.c to hooks.c
>>
>> You can see the progress on gitlab: https://gitlab.com/waterkip/git
>> or on github: https://github.com/waterkip/git
>> The output of format-patch is down below.
>
> It would be nicer if you could give links that let us see more
> directly the commits you made, for example:
> https://gitlab.com/waterkip/git/commits/multi-hooks

Yeah.. sorry about that. Let's just say I was excited to send my
progress to the list.

>> I have some questions regarding the following two functions in run-command.c:
>> * run_hook_le
>> * run_hook_ve
>>
>> What do the postfixes le and ve mean?
>
> It's about the arguments the function accepts, in a similar way to
> exec*() functions, see `man execve` and `man execle`.
> In short 'l' means list, 'v' means array of pointer to strings and 'e'
> means environment.

Thanks, I'll have a look at these functions later today.

>> format-patch:
>>
>> From 129d8aff8257b22210beadc155cdbcae99b0fc4b Mon Sep 17 00:00:00 2001
>> From: Wesley Schwengle <wesley@schwengle.net>
>> Date: Sun, 2 Sep 2018 02:40:04 +0200
>> Subject: [PATCH] WIP: Add hook.d support in git
>
> This is not the best way to embed a patch in an email. There is
> Documentation/SubmittingPatches in the code base, that should explain
> better ways to send patches to the mailing list.

I saw that as well, after I've submitted the e-mail and was looking at
the travis documentation. I'll promise I'll do better for my next
patch submission. Sorry about this..

>> Add a generic mechanism to find and execute one or multiple hooks found
>> in $GIT_DIR/hooks/<hook> and/or $GIT_DIR/hooks/<hooks>.d/*
>>
>> [snip]
>>
>> * All the scripts are executed and fail on the first error
>
> Maybe the above documentation should be fully embedded as comments in
> "hooks.h" (or perhaps added to a new file in
> "Documentation/technical/", though it looks like we prefer to embed
> doc in header files these days).

I've added this to "hooks.h". If you guys want some documentation in
"Documentation/technical", I'm ok with adding a new file there as
well.

>> diff --git a/hooks.h b/hooks.h
>> new file mode 100644
>> index 000000000..278d63344
>> --- /dev/null
>> +++ b/hooks.h
>> @@ -0,0 +1,35 @@
>> +#ifndef HOOKS_H
>> +#define HOOKS_H
>> +
>> +#ifndef NO_PTHREADS
>> +#include <pthread.h>
>> +#endif
>> +/*
>> + * Returns all the hooks found in either
>> + * $GIT_DIR/hooks/$hook and/or $GIT_DIR/hooks/$hook.d/
>> + *
>> + * Note that this points to static storage that will be
>> + * overwritten by further calls to find_hooks and run_hook_*.
>> + */
>> +//extern const struct string_list *find_hooks(const char *name);
>
> The above comment is using "//" which we forbid and should probably be
> removed anyway.

Thanks, I have a "//" comment elsewhere, I'll change/remove it.

>> +extern const char *find_hooks(const char *name);
>> +
>> +/* Unsure what this does/is/etc */
>> +//LAST_ARG_MUST_BE_NULL
>
> This is to make it easier for tools like compilers to check that a
> function is used correctly. You should not remove such macros.

Check.

>> +/*
>> + * Run all the runnable hooks found in
>> + * $GIT_DIR/hooks/$hook and/or $GIT_DIR/hooks/$hook.d/
>> + *
>> + */
>> +//extern int run_hooks_le(const char *const *env, const char *name, ...);
>> +//extern int run_hooks_ve(const char *const *env, const char *name,
>> va_list args);
>
> Strange that these functions are commented out.

These two functions are still in "run-command.h" and I want to move
them to "hooks.h" and friends. But I first wanted to make sure
"find_hooks" worked as intended. This is still on my TODO for this
week.

>> +#endif
>> +
>> +/* Workings of hooks
>> + *
>> + * array_of_hooks      = find_hooks(name);
>> + * number_of_hooks_ran = run_hooks(array_of_hooks);
>> + *
>> + */
>
> This kind of documentation should probably be at the beginning of the
> file, see strbuf.h for example.

Since I added the better part of the commit message in "hooks.h" I
removed this bit.

An additional question:

In my patch I've added "hooks.multiHook", which I think I should
rename to "hooks.hooksd". It is wanted to change "core.hooksPath" to
the "hooks" namespace? Or should I rename my new config item to
"core.hooksd"?

Cheers,
Wesley

-- 
Wesley Schwengle, Developer
Mintlab B.V., https://www.zaaksysteem.nl
E: wesley@mintlab.nl
T:  +31 20 737 00 05