Re: [PATCH] [RFC] Make git-update-ref invoke the update and post-update hooks

Re: [PATCH] [RFC] Make git-update-ref invoke the update and post-update hooks

[Brian Vandenberg]

From: Junio C Hamano <junkio@cox.net>
Date: 2006-10-08 15:00:32
Petr Baudis <pasky@suse.cz> writes:

>> Someone raised a concern that the update and post-update hooks are not
>> invoked at fetch time in the similar way as they are invoked at push
>> time, and the idea sort of makes sense. But this patch goes further - it
>> makes Git invoke those hooks each time a ref is updated in a repository
>> using the git-update-ref command, which I believe makes a lot of sense as
>> well - the behaviour is consistent with the current pushing behaviour
>> and you suddenly finally get a hook where you can properly notify even
>> about fast-forwards etc.

>In principle I do not have problem with this approach per-se,
>but I wonder if we were to do this we might want to make
>receive-pack.c::update() and cmd_update_ref() call the same
>underlying function, and make that underlying function implement
>this "ask the hook if updating is ok" dance.  It might even make
>sense to have update-ref honor deny_non_fast_forwards for that
>matter (I am mildly doubtful of this last point, though).

Was this rejected?

I'm tweaking the configuration on our master repo; among other things:
I've enabled reflog and disabled altering history or deleting branches
through receive-pack, instead requiring that someone make history
alterations directly (using update-ref).

update-ref can optionally accept a comment explaining why the history
needed to be altered, but we would prefer to make it mandatory.  Hooks
seem like a natural place to do that, but after running update-ref
under strace/truss & looking through builtin/update-ref.c, it looks as
though it doesn't make use of hooks.

-Brian

Re: [PATCH] [RFC] Make git-update-ref invoke the update and post-update hooks

[Junio C Hamano]

Brian Vandenberg <phantall@gmail.com> writes:

> From: Junio C Hamano <junkio@cox.net>
> Date: 2006-10-08 15:00:32
> Petr Baudis <pasky@suse.cz> writes:
>
>>> Someone raised a concern that the update and post-update hooks are not
>>> invoked at fetch time in the similar way as they are invoked at push
>>> time, and the idea sort of makes sense. But this patch goes further - it
>>> makes Git invoke those hooks each time a ref is updated in a repository
>>> using the git-update-ref command, which I believe makes a lot of sense as
>>> well - the behaviour is consistent with the current pushing behaviour
>>> and you suddenly finally get a hook where you can properly notify even
>>> about fast-forwards etc.
>
>>In principle I do not have problem with this approach per-se,
>>but I wonder if we were to do this we might want to make
>>receive-pack.c::update() and cmd_update_ref() call the same
>>underlying function, and make that underlying function implement
>>this "ask the hook if updating is ok" dance.  It might even make
>>sense to have update-ref honor deny_non_fast_forwards for that
>>matter (I am mildly doubtful of this last point, though).

I am ultra-doubtful at this point ;-)

For one thing, update-ref is not a tool that is exclusive to
receiving object transfer aka receive-pack, so it makes no sense for
it to pay attention to post-update.

Also it is a low-level plumbing; the policy issues should come at a
level higher than that.  I.e. Porcelain scripts implemented using
them as building blocks should be the ones that you do your policy,
e.g.

	if whatever logic you want to use in your policy says OK
        then
		git update-ref ...args...
	else
        	echo >&2 "My policy does not like your arguments"
                exit 1
	fi

Re: [PATCH] [RFC] Make git-update-ref invoke the update and post-update hooks

[Brian Vandenberg]

On Mon, Jun 15, 2015 at 2:22 PM, Junio C Hamano <gitster@pobox.com> wrote:
> I am ultra-doubtful at this point ;-)
>
> For one thing, update-ref is not a tool that is exclusive to
> receiving object transfer aka receive-pack, so it makes no sense for
> it to pay attention to post-update.

I was a little dubious about that as well, though I like the idea of
update-ref having a hook.

> Also it is a low-level plumbing; the policy issues should come at a
> level higher than that.  I.e. Porcelain scripts implemented using
> them as building blocks should be the ones that you do your policy,
> e.g.
>
>         if whatever logic you want to use in your policy says OK
>         then
>                 git update-ref ...args...
>         else
>                 echo >&2 "My policy does not like your arguments"
>                 exit 1
>         fi
>

In principle I agree, though telling people "you should use xyz
script" is one thing; in practice they're more likely to google "git
alter history" and eventually find an explanation for using
update-ref, completely bypassing any policy/protocol we put in place
for altering history on the master repo.

Would you be less doubtful about adding a lower-level hook for
update-ref?  Or in lieu of that, a config option that can affect the
behavior of its "-m" and "-d" options?

-Brian

Re: [PATCH] [RFC] Make git-update-ref invoke the update and post-update hooks

[Junio C Hamano]

Brian Vandenberg <phantall@gmail.com> writes:

> Would you be less doubtful about adding a lower-level hook for
> update-ref?  Or in lieu of that, a config option that can affect the
> behavior of its "-m" and "-d" options?

Not really.

Those who want to bypass your policy can use "vi .git/packed-refs"
and/or "rm .git/refs/heads/foo"; you would not propose to patch "rm"
and "vi" to pay attention to git configuration, because they are not
about "git" at all, and there are cases where these bare-metal level
editing of repositories is necessary.  You would instead tell them
"don't do that".

When I say "update-ref is a low-level plumbing command that should
not enforce policy", I am suggesting you to treat the command just
like you would treat "rm" and "vi".

Re: [PATCH] [RFC] Make git-update-ref invoke the update and post-update hooks

[Brian Vandenberg]

On Mon, Jun 15, 2015 at 3:45 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Those who want to bypass your policy can use "vi .git/packed-refs"
> and/or "rm .git/refs/heads/foo"; you would not propose to patch "rm"
> and "vi" to pay attention to git configuration, because they are not
> about "git" at all, and there are cases where these bare-metal level
> editing of repositories is necessary.  You would instead tell them
> "don't do that".
>
> When I say "update-ref is a low-level plumbing command that should
> not enforce policy", I am suggesting you to treat the command just
> like you would treat "rm" and "vi".

I suppose I see your point, though the line they've crossed at that
point is from benign to malicious.

When you assume they've crossed that line, hooks and configuration
parameters are mostly useless in other git commands as well.  My
assumption here is they want to do something productive and aren't
intentionally seeking to subvert policies, but rather aren't aware of
the policies and/or correct procedure.

Thanks for your responses, I appreciate your time.

-Brian

[PATCH] [RFC] Make git-update-ref invoke the update and post-update hooks

[Petr Baudis]

Someone raised a concern that the update and post-update hooks are not
invoked at fetch time in the similar way as they are invoked at push
time, and the idea sort of makes sense. But this patch goes further - it
makes Git invoke those hooks each time a ref is updated in a repository
using the git-update-ref command, which I believe makes a lot of sense as
well - the behaviour is consistent with the current pushing behaviour
and you suddenly finally get a hook where you can properly notify even
about fast-forwards etc.

Signed-off-by: Petr Baudis <pasky@suse.cz>
---

 Documentation/git-update-ref.txt |    3 ++
 Documentation/hooks.txt          |   12 ++++--
 Makefile                         |    2 +
 builtin-update-ref.c             |   12 ++++++
 hooks.c                          |   68 ++++++++++++++++++++++++++++++++++++
 hooks.h                          |   15 ++++++++
 receive-pack.c                   |   71 +-------------------------------------
 7 files changed, 106 insertions(+), 77 deletions(-)

diff --git a/Documentation/git-update-ref.txt b/Documentation/git-update-ref.txt
index 71bcb79..bd20d05 100644
--- a/Documentation/git-update-ref.txt
+++ b/Documentation/git-update-ref.txt
@@ -51,6 +51,9 @@ for reading but not for writing (so we'l
 ref symlink to some other tree, if you have copied a whole
 archive by creating a symlink tree).
 
+Just before updating the reference, the update hook is invoked.
+After updating the reference, the post-update hook is invoked.
+
 With `-d` flag, it deletes the named <ref> after verifying it
 still contains <oldvalue>.
 
diff --git a/Documentation/hooks.txt b/Documentation/hooks.txt
index 517f49b..53fa270 100644
--- a/Documentation/hooks.txt
+++ b/Documentation/hooks.txt
@@ -99,8 +99,9 @@ update
 This hook is invoked by `git-receive-pack` on the remote repository,
 which happens when a `git push` is done on a local repository.
 Just before updating the ref on the remote repository, the update hook
-is invoked.  Its exit status determines the success or failure of
-the ref update.
+is invoked.  It is also invoked on the local repository anytime a ref
+value is updated (that includes mainly fetches, merges, commits).
+Its exit status determines the success or failure of the ref update.
 
 The hook executes once for each ref to be updated, and takes
 three parameters:
@@ -135,9 +136,10 @@ post-update
 -----------
 
 This hook is invoked by `git-receive-pack` on the remote repository,
-which happens when a `git push` is done on a local repository.
-It executes on the remote repository once after all the refs have
-been updated.
+which happens when a `git push` is done on a local repository
+It is also invoked on the local repository anytime a ref value is
+updated (that includes mainly fetches, merges, commits).  It executes
+on the repository once after all the refs have been updated.
 
 It takes a variable number of parameters, each of which is the
 name of ref that was actually updated.
diff --git a/Makefile b/Makefile
index 2962c2e..8e58585 100644
--- a/Makefile
+++ b/Makefile
@@ -263,7 +263,7 @@ LIB_OBJS = \
 	fetch-clone.o revision.o pager.o tree-walk.o xdiff-interface.o \
 	write_or_die.o trace.o list-objects.o grep.o \
 	alloc.o merge-file.o path-list.o help.o unpack-trees.o $(DIFF_OBJS) \
-	color.o wt-status.o archive-zip.o archive-tar.o
+	color.o wt-status.o archive-zip.o archive-tar.o hooks.o
 
 BUILTIN_OBJS = \
 	builtin-add.o \
diff --git a/builtin-update-ref.c b/builtin-update-ref.c
index b34e598..546cbb1 100644
--- a/builtin-update-ref.c
+++ b/builtin-update-ref.c
@@ -1,6 +1,7 @@
 #include "cache.h"
 #include "refs.h"
 #include "builtin.h"
+#include "hooks.h"
 
 static const char git_update_ref_usage[] =
 "git-update-ref [-m <reason>] (-d <refname> <value> | <refname> <value> [<oldval>])";
@@ -11,6 +12,7 @@ int cmd_update_ref(int argc, const char 
 	struct ref_lock *lock;
 	unsigned char sha1[20], oldsha1[20];
 	int i, delete;
+	struct command *cmd;
 
 	delete = 0;
 	setup_ident();
@@ -63,9 +65,17 @@ int cmd_update_ref(int argc, const char 
 	lock = lock_any_ref_for_update(refname, oldval ? oldsha1 : NULL);
 	if (!lock)
 		return 1;
+	if (run_update_hook(refname, oldval, value)) {
+		unlock_ref(lock);
+		return 1;
+	}
 	if (write_ref_sha1(lock, sha1, msg) < 0)
 		return 1;
-
 	/* write_ref_sha1 always unlocks the ref, no need to do it explicitly */
+
+	cmd = xmalloc(sizeof(struct command) + strlen(refname) + 1);
+	strcpy(cmd->ref_name, refname);
+	run_update_post_hook(cmd);
+
 	return 0;
 }
diff --git a/hooks.c b/hooks.c
new file mode 100644
index 0000000..efade2b
--- /dev/null
+++ b/hooks.c
@@ -0,0 +1,68 @@
+#include "cache.h"
+#include "run-command.h"
+#include "hooks.h"
+
+
+static char update_hook[] = "hooks/update";
+
+int run_update_hook(const char *refname, const char *old_hex, const char *new_hex)
+{
+	char *update_hook_p = git_path("%s", update_hook);
+	int code;
+
+	if (access(update_hook_p, X_OK) < 0)
+		return 0;
+	code = run_command(update_hook_p, refname, old_hex, new_hex, NULL);
+	switch (code) {
+	case 0:
+		return 0;
+	case -ERR_RUN_COMMAND_FORK:
+		return error("hook fork failed");
+	case -ERR_RUN_COMMAND_EXEC:
+		return error("hook execute failed");
+	case -ERR_RUN_COMMAND_WAITPID:
+		return error("waitpid failed");
+	case -ERR_RUN_COMMAND_WAITPID_WRONG_PID:
+		return error("waitpid is confused");
+	case -ERR_RUN_COMMAND_WAITPID_SIGNAL:
+		return error("%s died of signal", update_hook);
+	case -ERR_RUN_COMMAND_WAITPID_NOEXIT:
+		return error("%s died strangely", update_hook);
+	default:
+		error("%s exited with error code %d", update_hook, -code);
+		return -code;
+	}
+}
+
+
+static char update_post_hook[] = "hooks/post-update";
+
+void run_update_post_hook(struct command *cmd)
+{
+	char *update_post_hook_p = git_path("%s", update_post_hook);
+	struct command *cmd_p;
+	int argc;
+	const char **argv;
+
+	if (access(update_post_hook_p, X_OK) < 0)
+		return;
+	for (argc = 1, cmd_p = cmd; cmd_p; cmd_p = cmd_p->next) {
+		if (cmd_p->error_string)
+			continue;
+		argc++;
+	}
+	argv = xmalloc(sizeof(*argv) * (1 + argc));
+	argv[0] = update_post_hook_p;
+
+	for (argc = 1, cmd_p = cmd; cmd_p; cmd_p = cmd_p->next) {
+		char *p;
+		if (cmd_p->error_string)
+			continue;
+		p = xmalloc(strlen(cmd_p->ref_name) + 1);
+		strcpy(p, cmd_p->ref_name);
+		argv[argc] = p;
+		argc++;
+	}
+	argv[argc] = NULL;
+	run_command_v_opt(argc, argv, RUN_COMMAND_NO_STDIO);
+}
diff --git a/hooks.h b/hooks.h
new file mode 100644
index 0000000..8ddeca3
--- /dev/null
+++ b/hooks.h
@@ -0,0 +1,15 @@
+#ifndef HOOKS_H
+#define HOOKS_H
+
+int run_update_hook(const char *refname, const char *old_hex, const char *new_hex);
+
+struct command {
+	struct command *next;
+	const char *error_string;
+	unsigned char old_sha1[20];
+	unsigned char new_sha1[20];
+	char ref_name[FLEX_ARRAY]; /* more */
+};
+void run_update_post_hook(struct command *cmd);
+
+#endif
diff --git a/receive-pack.c b/receive-pack.c
index 1fcf3a9..9d5ece6 100644
--- a/receive-pack.c
+++ b/receive-pack.c
@@ -4,6 +4,7 @@ #include "pkt-line.h"
 #include "run-command.h"
 #include "commit.h"
 #include "object.h"
+#include "hooks.h"
 
 static const char receive_pack_usage[] = "git-receive-pack <git-dir>";
 
@@ -33,47 +34,8 @@ static void write_head_info(void)
 
 }
 
-struct command {
-	struct command *next;
-	const char *error_string;
-	unsigned char old_sha1[20];
-	unsigned char new_sha1[20];
-	char ref_name[FLEX_ARRAY]; /* more */
-};
-
 static struct command *commands;
 
-static char update_hook[] = "hooks/update";
-
-static int run_update_hook(const char *refname,
-			   char *old_hex, char *new_hex)
-{
-	int code;
-
-	if (access(update_hook, X_OK) < 0)
-		return 0;
-	code = run_command(update_hook, refname, old_hex, new_hex, NULL);
-	switch (code) {
-	case 0:
-		return 0;
-	case -ERR_RUN_COMMAND_FORK:
-		return error("hook fork failed");
-	case -ERR_RUN_COMMAND_EXEC:
-		return error("hook execute failed");
-	case -ERR_RUN_COMMAND_WAITPID:
-		return error("waitpid failed");
-	case -ERR_RUN_COMMAND_WAITPID_WRONG_PID:
-		return error("waitpid is confused");
-	case -ERR_RUN_COMMAND_WAITPID_SIGNAL:
-		return error("%s died of signal", update_hook);
-	case -ERR_RUN_COMMAND_WAITPID_NOEXIT:
-		return error("%s died strangely", update_hook);
-	default:
-		error("%s exited with error code %d", update_hook, -code);
-		return -code;
-	}
-}
-
 static int update(struct command *cmd)
 {
 	const char *name = cmd->ref_name;
@@ -127,37 +89,6 @@ static int update(struct command *cmd)
 	return 0;
 }
 
-static char update_post_hook[] = "hooks/post-update";
-
-static void run_update_post_hook(struct command *cmd)
-{
-	struct command *cmd_p;
-	int argc;
-	const char **argv;
-
-	if (access(update_post_hook, X_OK) < 0)
-		return;
-	for (argc = 1, cmd_p = cmd; cmd_p; cmd_p = cmd_p->next) {
-		if (cmd_p->error_string)
-			continue;
-		argc++;
-	}
-	argv = xmalloc(sizeof(*argv) * (1 + argc));
-	argv[0] = update_post_hook;
-
-	for (argc = 1, cmd_p = cmd; cmd_p; cmd_p = cmd_p->next) {
-		char *p;
-		if (cmd_p->error_string)
-			continue;
-		p = xmalloc(strlen(cmd_p->ref_name) + 1);
-		strcpy(p, cmd_p->ref_name);
-		argv[argc] = p;
-		argc++;
-	}
-	argv[argc] = NULL;
-	run_command_v_opt(argc, argv, RUN_COMMAND_NO_STDIO);
-}
-
 /*
  * This gets called after(if) we've successfully
  * unpacked the data payload.

Re: [PATCH] [RFC] Make git-update-ref invoke the update and post-update hooks

[Junio C Hamano]

Petr Baudis <pasky@suse.cz> writes:

> Someone raised a concern that the update and post-update hooks are not
> invoked at fetch time in the similar way as they are invoked at push
> time, and the idea sort of makes sense. But this patch goes further - it
> makes Git invoke those hooks each time a ref is updated in a repository
> using the git-update-ref command, which I believe makes a lot of sense as
> well - the behaviour is consistent with the current pushing behaviour
> and you suddenly finally get a hook where you can properly notify even
> about fast-forwards etc.

In principle I do not have problem with this approach per-se,
but I wonder if we were to do this we might want to make
receive-pack.c::update() and cmd_update_ref() call the same
underlying function, and make that underlying function implement
this "ask the hook if updating is ok" dance.  It might even make
sense to have update-ref honor deny_non_fast_forwards for that
matter (I am mildly doubtful of this last point, though).