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: wesley@mintlab.nl
T: +31 20 737 00 05
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.
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.fsf@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.
From: Jonathan Nieder <hidden> Date: 2018-08-31 03:17:00
Ævar Arnfjörð Bjarmason wrote:
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.fsf@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.
A few unrelated thoughts, to expand on this.
Reports of experience from using local hook runners would be very
welcome so we can benefit from their good ideas and avoid their bad
ones. That was part of the motivation for not building this in for so
long: we want people to experiment so that the result can be something
that works well for a lot of people.
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.
As a word of caution, today we support having multiple credential
helpers in use and it's a nightmare to support. The layering model is
complicated and users don't understand it. So we might want to try to
avoid whatever went wrong there. ;-)
Thanks,
Jonathan
[1] https://public-inbox.org/git/20171002234517.GV19555@aiede.mtv.corp.google.com/
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.fsf@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=H4wacPhHjFbQ@mail.gmail.com/
for more information
3) As a developer I want to run $GIT_DIR/hooks/<hook> before
$GIT_DIR/hooks/<hook>.d/*
Reference: https://public-inbox.org/git/CACBZZX6j6q2DUN_Z-Pnent1u714dVNPFBrL_PiEQyLmCzLUVxg@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.fsf@gitster.mtv.corp.google.com/
2018-08-31 5:16 GMT+02:00 Jonathan Nieder [off-list ref]:
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.GV19555@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: wesley@mintlab.nl
T: +31 20 737 00 05
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.fsf@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?
No, sorry. Start by grepping the hook names found in the githooks
manpage in the C code.
One of the things that's hard, well not hard, just tedious about this,
is that most of them are implementing their own ad-hoc way of doing
stuff. E.g. the *-receive hooks are in receive-pack.c in
run_receive_hook().
There is run_hook_le() and friends, but it's not used by everything.
So e.g. for the pre-receive hook in order to run 2 of them instead of 1
you need to untangle the state where we're feeding the hook with the
input (and potentially buffer it, not stream it), instead of doing it as
a one-off as we're doing now.
Then some hooks get nothing on stdin, some get stuff on stdin, some
produce output on stdout/stderr etc.
As a first approximation, just add a e.g. support for a pre-receive.2
hook, that gets run after pre-receive, to see what needs to be done to
run it twice.
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.
Yeah maybe we should have a setting, but FWIW I think we should just
stat() whether the hooks/$hook_name.d directory exist, and then use it,
although maybe we'll need stuff like hooks.multiHooks to give the likes
of GitLab (which already do that themselves) an upgrade path...
You can see their implementation here:
https://gitlab.com/gitlab-org/gitlab-shell/blob/master/lib/gitlab_custom_hook.rb
For e.g. GitLab the hook/pre-receive is a runner that'll run all
hook/pre-receive.d/*, so this probably makes sense to hide behind a
config setting. I think it's sensible as a default to move to to just
try to move away from hooks/<hook> and use hook/<hook>.d/* instead.
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.fsf@gitster.mtv.corp.google.com/
I think a default that makes more sense is a while ! ret = glob(<hooks>)
loop, i.e. a failure will do early exit. But yeah. Junio seemed to want
this to be configurable.
2018-08-31 5:16 GMT+02:00 Jonathan Nieder [off-list ref]:
quoted
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.GV19555@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.
Yeah I don't see how what Jonathan's talking about there has any
relevance to whether we run 1 or 100 hooks.
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 <redacted>
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/<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
---
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
@@ -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
@@ -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
@@ -0,0 +1,134 @@+#include"hooks.h"+#include"cache.h"+#include"run-command.h"+#include"string-list.h"+#include"config.h"++//#include <sys/types.h>+//#include <sys/stat.h>+//#include <sys/sysmacros.h>++staticconstchar*_get_hook(structstrbuf*path)+{+char*name=path->buf;+interr;+if(access(name,X_OK)>=0){+returnname;+}+err=errno;++// Check for .exe+#ifdef STRIP_EXTENSION+strbuf_addstr(path,STRIP_EXTENSION);+name=path->buf;+if(access(name,X_OK)>=0)+returnname;+if(errno==EACCES)+err=errno;+#endif++if(err==EACCES&&advice_ignored_hook){+staticstructstring_listadvise_given=STRING_LIST_INIT_DUP;++if(!string_list_lookup(&advise_given,name)){+string_list_insert(&advise_given,name);+advise(_("The '%s' hook was ignored because "+"it's not set as executable.\n"+"You can disable this warning with "+"`git config advice.ignoredHook false`."),+name);+}+}+returnNULL;+}++staticvoidget_hooks_from_directory(constchar*name,struct
string_list *list)
+{
+
+ struct strbuf path = STRBUF_INIT;
+ strbuf_git_path(&path, "hooks/%s.d", name);
+
+ struct dirent *de;
+ DIR *hooksd_dir;
+ static struct strbuf hooksd_script = STRBUF_INIT;
+
+ if ((hooksd_dir = opendir(path.buf)) == NULL) {
+ return;
+ }
+
+ while ((de = readdir(hooksd_dir)) != NULL) {
+ struct stat stbuf;
+
+ strbuf_reset(&hooksd_script);
+ strbuf_addf(&hooksd_script, "%s/%s", path.buf, de->d_name);
+
+ if (stat(hooksd_script.buf, &stbuf) == -1) {
+ continue;
+ }
+ else if ((stbuf.st_mode & S_IFMT) == S_IFDIR) {
+ continue;
+ }
+
+ const char *hook = _get_hook(&hooksd_script);
+ if (hook) {
+ //printf("Found hook script %s\n", hook);
+ string_list_append(list, hook);
+ }
+
+ strbuf_release(&hooksd_script);
+ }
+
+ strbuf_release(&hooksd_script);
+ strbuf_release(&path);
+ return;
+}
+
+//const struct string_list *find_hooks(const char *name)
+const char *find_hooks(const char *name)
+{
+
+ struct string_list list = STRING_LIST_INIT_NODUP;
+ const char *hook_path;
+
+ struct strbuf path = STRBUF_INIT;
+ strbuf_git_path(&path, "hooks/%s", name);
+
+ hook_path = _get_hook(&path);
+
+ if (hook_path) {
+ string_list_append(&list, hook_path);
+ }
+
+ if (hookd_enabled == 1) {
+ get_hooks_from_directory(name, &list);
+
+ if (hook_path)
+ return hook_path;
+ return NULL;
+ }
+ else if (hookd_enabled == 0) {
+ if (hook_path)
+ return hook_path;
+ return NULL;
+ }
+ else {
+ static struct string_list advise_given = STRING_LIST_INIT_DUP;
+
+ get_hooks_from_directory(name, &list);
+ if ( hook_path && list.nr > 1
+ && !string_list_lookup(&advise_given, name)) {
+ string_list_insert(&advise_given, name);
+ advise(_("You have a hook plus hook.d dir for %s. This"
+ " behaviour is now supported by git.\nYou can"
+ " disable this warning with `git config"
+ " hooks.multiHook false` to disable reading\n"
+ "the hook.d directory or run `git config"
+ " hooks.multiHook true` to enable the\nhook.d"
+ " directory. Ignoring the hook.d directory"
+ " for now"), name);
+ }
+ if (hook_path)
+ return hook_path;
+ return NULL;
+ }
+}
@@ -1246,46 +1247,13 @@ int finish_async(struct async *async)#endif}-constchar*find_hook(constchar*name)-{-staticstructstrbufpath=STRBUF_INIT;--strbuf_reset(&path);-strbuf_git_path(&path,"hooks/%s",name);-if(access(path.buf,X_OK)<0){-interr=errno;--#ifdef STRIP_EXTENSION-strbuf_addstr(&path,STRIP_EXTENSION);-if(access(path.buf,X_OK)>=0)-returnpath.buf;-if(errno==EACCES)-err=errno;-#endif--if(err==EACCES&&advice_ignored_hook){-staticstructstring_listadvise_given=STRING_LIST_INIT_DUP;--if(!string_list_lookup(&advise_given,name)){-string_list_insert(&advise_given,name);-advise(_("The '%s' hook was ignored because "-"it's not set as executable.\n"-"You can disable this warning with "-"`git config advice.ignoredHook false`."),-path.buf);-}-}-returnNULL;-}-returnpath.buf;-}intrun_hook_ve(constchar*const*env,constchar*name,va_listargs){structchild_processhook=CHILD_PROCESS_INIT;constchar*p;-p=find_hook(name);+p=find_hooks(name);if(!p)return0;
@@ -1019,7 +1020,7 @@ static int run_pre_push_hook(struct transport *transport,structstrbufbuf;constchar*argv[4];-if(!(argv[0]=find_hook("pre-push")))+if(!(argv[0]=find_hooks("pre-push")))return0;argv[1]=transport->remote->name;
From: Christian Couder <hidden> Date: 2018-09-03 04:00:12
Hi Wesley,
On Sun, Sep 2, 2018 at 11:38 PM, Wesley Schwengle [off-list ref] 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.
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 <redacted>
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).
Hello Christian,
2018-09-03 6:00 GMT+02:00 Christian Couder [off-list ref]:
Hi Wesley,
On Sun, Sep 2, 2018 at 11:38 PM, Wesley Schwengle [off-list ref] wrote:
quoted
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!
quoted
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.
Yeah.. sorry about that. Let's just say I was excited to send my
progress to the list.
quoted
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.
quoted
format-patch:
From 129d8aff8257b22210beadc155cdbcae99b0fc4b Mon Sep 17 00:00:00 2001
From: Wesley Schwengle <redacted>
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..
quoted
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.
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.
quoted
+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.
quoted
+/*
+ * 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.
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