Re: Feature request: hooks directory

[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!

>  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

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

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

> 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/*
>
> The API is as follows:
>
> #include "hooks.h"
>
> array hooks   = find_hooks('pre-receive');
> int hooks_ran = run_hooks(hooks);
>
> The implemented behaviour is:
>
> * If we find a hooks/<hook>.d directory and the hooks.multiHook flag isn't
>   set we make use of that directory.
>
> * If we find a hooks/<hook>.d and we also have hooks/<hook> and the
>   hooks.multiHook isn't set or set to false we don't use the hook.d
>   directory. If the hook isn't set we issue a warning to the user
>   telling him/her that we support multiple hooks via the .d directory
>   structure
>
> * If the hooks.multiHook is set to true we use the hooks/<hook> and all
>   the entries found in hooks/<hook>.d
>
> * 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).

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

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

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

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

Thanks,
Christian.