Hello Christian, 2018-09-03 6:00 GMT+02:00 Christian Couder <christian.couder@xxxxxxxxx>: > 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! 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@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. 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@xxxxxxxxxx T: +31 20 737 00 05
