Feature request: hooks directory
Hello all, I would like to ask if it is worth my time looking into the following solution to a problem we have at work. Problem: We want to have some git-hooks and we want to provide them to the user. In a most recent example we have a post-checkout hook that deals with some Docker things. However, if we update that post-checkout hook my local overrides in that post-checkout hook are going to be overwritten. Solution: We discussed this at work and we thought about making a .d directory for the hooks, eg. $GIT_DIR/hooks/post-commit.d, where a user can put the post-commit hooks in. This allows us to provide post commit hooks and allows the user to add additional hooks him/herself. We could implement this in our own code base. But we were wondering if this approach could be shared with the git community and if this behavior is wanted in git itself. Cheers, Wesley -- Wesley Schwengle, Developer Mintlab B.V., https://www.zaaksysteem.nl E: wes...@mintlab.nl T: +31 20 737 00 05
Re: Feature request: hooks directory
Hop, 2018-08-30 16:45 GMT+02:00 Ævar Arnfjörð Bjarmason : >> Solution: >> We discussed this at work and we thought about making a .d directory >> for the hooks, eg. $GIT_DIR/hooks/post-commit.d, where a user can put >> the post-commit hooks in. This allows us to provide post commit hooks >> and allows the user to add additional hooks him/herself. We could >> implement this in our own code base. But we were wondering if this >> approach could be shared with the git community and if this behavior >> is wanted in git itself. > > There is interest in this. This E-Mail of mine gives a good summary of > prior discussions about this: > https://public-inbox.org/git/877eqqnq22@evledraar.gmail.com/ > > I.e. it's something I've personally been interested in doing in the > past, there's various bolt-on solutions to do it (basically local hook > runners) used by various projects. Thank you for the input. Do you by any chance still have that branch? Or would you advise me to start fresh, if so, do you have any pointers on where to look as I'm brand new to the git source code? >From the thread I've extracted three stories: 1) As a developer I want to have 'hooks.multiHooks' to enable multi-hook support in git Input is welcome for another name. 2) As a developer I want natural sort order executing for my githooks so I can predict executions See https://public-inbox.org/git/CACBZZX6AYBYeb5S4nEBhYbx1r=icJ81JGYBx5=h4wacphhj...@mail.gmail.com/ for more information 3) As a developer I want to run $GIT_DIR/hooks/ before $GIT_DIR/hooks/.d/* Reference: https://public-inbox.org/git/cacbzzx6j6q2dun_z-pnent1u714dvnpfbrl_pieqylmczlu...@mail.gmail.com/ The following story would be.. nice to have I think. I'm not sure I would want to implement this from the get go as I don't have a use case for it. 4) As a developer I want a way to have a hook report an error and let another hook decide if we want to pass or not. Reference: https://public-inbox.org/git/xmqq60v4don1@gitster.mtv.corp.google.com/ 2018-08-31 5:16 GMT+02:00 Jonathan Nieder : > A few unrelated thoughts, to expand on this. > > Separately from that, in [1] I mentioned that I want to revamp how > hooks work somewhat, to avoid the attack described there (or the more > common attack also described there that involves a zip file). Such a > revamp would be likely to also handle this multiple-hook use case. > > [1] > https://public-inbox.org/git/20171002234517.gv19...@aiede.mtv.corp.google.com/ The zip file attack vector doesn't change with adding a hook.d directory structure? If I have one file or multiple files, the attack stays the same? I think I'm asking if this would be a show stopper for the feature. Cheers, Wesley -- Wesley Schwengle, Developer Mintlab B.V., https://www.zaaksysteem.nl E: wes...@mintlab.nl T: +31 20 737 00 05
Re: Feature request: hooks directory
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/.
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.
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?
Cheers,
Wesley
format-patch:
>From 129d8aff8257b22210beadc155cdbcae99b0fc4b Mon Sep 17 00:00:00 2001
From: Wesley Schwengle
Date: Sun, 2 Sep 2018 02:40:04 +0200
Subject: [PATCH] WIP: Add hook.d support in git
Add a generic mechanism to find and execute one or multiple hooks found
in $GIT_DIR/hooks/ and/or $GIT_DIR/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/.d directory and the hooks.multiHook flag isn't
set we make use of that directory.
* If we find a hooks/.d and we also have hooks/ 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/ and all
the entries found in hooks/.d
* All the scripts are executed and fail on the first error
---
Makefile | 1 +
TODO-hooks.md | 38
builtin/am.c | 4 +-
builtin/commit.c | 4 +-
builtin/receive-pack.c | 10 +--
builtin/worktree.c | 3 +-
cache.h| 1 +
config.c | 5 ++
environment.c | 1 +
hooks.c| 134 +
hooks.h| 35 +++
run-command.c | 36 +--
run-command.h | 6 --
sequencer.c| 7 ++-
transport.c| 3 +-
15 files changed, 237 insertions(+), 51 deletions(-)
create mode 100644 TODO-hooks.md
create mode 100644 hooks.c
create mode 100644 hooks.h
diff --git a/Makefile b/Makefile
index 5a969f583..f5eddf1d7 100644
--- a/Makefile
+++ b/Makefile
@@ -810,6 +810,7 @@ LIB_H = $(shell $(FIND) . \
-name Documentation -prune -o \
-name '*.h' -print)
+LIB_OBJS += hooks.o
LIB_OBJS += abspath.o
LIB_OBJS += advice.o
LIB_OBJS += alias.o
diff --git a/TODO-hooks.md b/TODO-hooks.md
new file mode 100644
index 0..13b15bffb
--- /dev/null
+++ b/TODO-hooks.md
@@ -0,0 +1,38 @@
+# All hooks
+# See Documentation/githooks.txt for more information about each and every hook
+# that git knows about
+commit-msg
+fsmoninor-watchman
+p4-pre-submit
+post-applypatch
+post-checkout
+post-commit
+post-merge
+post-receive
+post-rewrite
+post-update
+pre-applypatch
+pre-auto-gc
+pre-commit
+pre-push
+pre-rebase
+pre-receive
+prepare-commit-msg
+push-to-checkout
+sendemail-validate
+update
+
+# builtin/receive-pack.c
+feed_recieve_hook
+find_hook
+find_receive_hook
+push_to_checkout_hook
+receive_hook_feed_state
+run_and_feed_hook
+run_hook_le
+run_receive_hook
+run_update_hook
+
+
+# run-command.c
+find_hook
diff --git a/builtin/am.c b/builtin/am.c
index 9f7ecf6ec..d1276dd47 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -34,6 +34,8 @@
#include "packfile.h"
#include "repository.h"
+#include "hooks.h"
+
/**
* Returns 1 if the file is empty or does not exist, 0 otherwise.
*/
@@ -509,7 +511,7 @@ static int run_applypatch_msg_hook(struct am_state *state)
static int run_post_rewrite_hook(const struct am_state *state)
{
struct child_process cp = CHILD_PROCESS_INIT;
- const char *hook = find_hook("post-rewrite");
+ const char *hook = find_hooks("post-rewrite");
int ret;
if (!hook)
diff --git a/builtin/commit.c b/builtin/commit.c
index 0d9828e29..389224d66 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -34,6 +34,8 @@
#include "mailmap.h"
#include "help.h"
+#include "hooks.h"
+
static const char * const builtin_commit_usage[] = {
N_("git commit [] [--] ..."),
NULL
@@ -932,7 +934,7 @@ static int prepare_to_commit(const char
*index_file, const char *prefix,
return 0;
}
- if (!no_verify && find_hook("pre-commit")) {
+ if (!no_verify && find_hooks("pre-commit")) {
/*
* Re-read the index as pre-commit hook could have updated it,
* and write it out as a tree. We must do this before we invoke
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index c17ce94e1..dd
Re: Feature request: hooks directory
Hello Christian, 2018-09-03 6:00 GMT+02:00 Christian Couder : > Hi Wesley, > > On Sun, Sep 2, 2018 at 11:38 PM, Wesley Schwengle 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 >> 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/ and/or $GIT_DIR/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 0..278d63344 >> --- /dev/null >> +++ b/hooks.h >> @@ -0,0 +1,35 @@ >> +#ifndef HOOKS_H >> +#define HOOKS_H >> + >> +#ifndef NO_PTHREADS >> +#include >> +#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: wes...@mintlab.nl T: +31 20 737 00 05
