Hi Wesley, On Sun, Sep 2, 2018 at 11:38 PM, Wesley Schwengle <wesley@xxxxxxxxxx> 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@xxxxxxxxxxxxx> > 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.
