[PATCH 1/2] Move run_hook() from builtin-commit.c into run-command.c (libgit)

[PATCH 1/2] Move run_hook() from builtin-commit.c into run-command.c (libgit)

[Stephan Beyer]

A function that runs a hook is used in several Git commands.
builtin-commit.c has the one that is most general for cases without
piping. This patch moves it into libgit and lets the other builtins
use this libified run_hook().

Note: The run_hook() in receive-pack.c feeds the standard input of
the pre-receive or post-receive hook. So that function is renamed
to run_receive_hook().

Mentored-by: Daniel Barkalow <barkalow@iabervon.org>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Stephan Beyer <s-beyer@gmx.net>
---

	This patch exists because I needed some run_hook() in sequencer
	and I noticed that slight variations are used across other
	builtins. :-)
	Stripping out a libified version seemed better to me than
	copy and paste.

 builtin-commit.c       |   34 ----------------------------------
 builtin-gc.c           |   30 +-----------------------------
 builtin-merge.c        |   31 +------------------------------
 builtin-receive-pack.c |    6 +++---
 run-command.c          |   35 +++++++++++++++++++++++++++++++++++
 run-command.h          |    2 ++
 6 files changed, 42 insertions(+), 96 deletions(-)

diff --git a/builtin-commit.c b/builtin-commit.c
index e88b78f..6f8d9fb 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -361,40 +361,6 @@ static int run_status(FILE *fp, const char *index_file, const char *prefix, int
 	return s.commitable;
 }
 
-static int run_hook(const char *index_file, const char *name, ...)
-{
-	struct child_process hook;
-	const char *argv[10], *env[2];
-	char index[PATH_MAX];
-	va_list args;
-	int i;
-
-	va_start(args, name);
-	argv[0] = git_path("hooks/%s", name);
-	i = 0;
-	do {
-		if (++i >= ARRAY_SIZE(argv))
-			die ("run_hook(): too many arguments");
-		argv[i] = va_arg(args, const char *);
-	} while (argv[i]);
-	va_end(args);
-
-	snprintf(index, sizeof(index), "GIT_INDEX_FILE=%s", index_file);
-	env[0] = index;
-	env[1] = NULL;
-
-	if (access(argv[0], X_OK) < 0)
-		return 0;
-
-	memset(&hook, 0, sizeof(hook));
-	hook.argv = argv;
-	hook.no_stdin = 1;
-	hook.stdout_to_stderr = 1;
-	hook.env = env;
-
-	return run_command(&hook);
-}
-
 static int is_a_merge(const unsigned char *sha1)
 {
 	struct commit *commit = lookup_commit(sha1);
diff --git a/builtin-gc.c b/builtin-gc.c
index f8eae4a..a201438 100644
--- a/builtin-gc.c
+++ b/builtin-gc.c
@@ -144,34 +144,6 @@ static int too_many_packs(void)
 	return gc_auto_pack_limit <= cnt;
 }
 
-static int run_hook(void)
-{
-	const char *argv[2];
-	struct child_process hook;
-	int ret;
-
-	argv[0] = git_path("hooks/pre-auto-gc");
-	argv[1] = NULL;
-
-	if (access(argv[0], X_OK) < 0)
-		return 0;
-
-	memset(&hook, 0, sizeof(hook));
-	hook.argv = argv;
-	hook.no_stdin = 1;
-	hook.stdout_to_stderr = 1;
-
-	ret = start_command(&hook);
-	if (ret) {
-		warning("Could not spawn %s", argv[0]);
-		return ret;
-	}
-	ret = finish_command(&hook);
-	if (ret == -ERR_RUN_COMMAND_WAITPID_SIGNAL)
-		warning("%s exited due to uncaught signal", argv[0]);
-	return ret;
-}
-
 static int need_to_gc(void)
 {
 	/*
@@ -194,7 +166,7 @@ static int need_to_gc(void)
 	else if (!too_many_loose_objects())
 		return 0;
 
-	if (run_hook())
+	if (run_hook(NULL, "pre-auto-gc", NULL))
 		return 0;
 	return 1;
 }
diff --git a/builtin-merge.c b/builtin-merge.c
index cf86975..e4555b0 100644
--- a/builtin-merge.c
+++ b/builtin-merge.c
@@ -300,35 +300,6 @@ static void squash_message(void)
 	strbuf_release(&out);
 }
 
-static int run_hook(const char *name)
-{
-	struct child_process hook;
-	const char *argv[3], *env[2];
-	char index[PATH_MAX];
-
-	argv[0] = git_path("hooks/%s", name);
-	if (access(argv[0], X_OK) < 0)
-		return 0;
-
-	snprintf(index, sizeof(index), "GIT_INDEX_FILE=%s", get_index_file());
-	env[0] = index;
-	env[1] = NULL;
-
-	if (squash)
-		argv[1] = "1";
-	else
-		argv[1] = "0";
-	argv[2] = NULL;
-
-	memset(&hook, 0, sizeof(hook));
-	hook.argv = argv;
-	hook.no_stdin = 1;
-	hook.stdout_to_stderr = 1;
-	hook.env = env;
-
-	return run_command(&hook);
-}
-
 static void finish(const unsigned char *new_head, const char *msg)
 {
 	struct strbuf reflog_message = STRBUF_INIT;
@@ -374,7 +345,7 @@ static void finish(const unsigned char *new_head, const char *msg)
 	}
 
 	/* Run a post-merge hook */
-	run_hook("post-merge");
+	run_hook(NULL, "post-merge", squash ? "1" : "0", NULL);
 
 	strbuf_release(&reflog_message);
 }
diff --git a/builtin-receive-pack.c b/builtin-receive-pack.c
index db67c31..6564a97 100644
--- a/builtin-receive-pack.c
+++ b/builtin-receive-pack.c
@@ -136,7 +136,7 @@ static int hook_status(int code, const char *hook_name)
 	}
 }
 
-static int run_hook(const char *hook_name)
+static int run_receive_hook(const char *hook_name)
 {
 	static char buf[sizeof(commands->old_sha1) * 2 + PATH_MAX + 4];
 	struct command *cmd;
@@ -358,7 +358,7 @@ static void execute_commands(const char *unpacker_error)
 		return;
 	}
 
-	if (run_hook(pre_receive_hook)) {
+	if (run_receive_hook(pre_receive_hook)) {
 		while (cmd) {
 			cmd->error_string = "pre-receive hook declined";
 			cmd = cmd->next;
@@ -627,7 +627,7 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)
 			unlink(pack_lockfile);
 		if (report_status)
 			report(unpack_status);
-		run_hook(post_receive_hook);
+		run_receive_hook(post_receive_hook);
 		run_update_post_hook(commands);
 	}
 	return 0;
diff --git a/run-command.c b/run-command.c
index c90cdc5..602fe85 100644
--- a/run-command.c
+++ b/run-command.c
@@ -342,3 +342,38 @@ int finish_async(struct async *async)
 #endif
 	return ret;
 }
+
+int run_hook(const char *index_file, const char *name, ...)
+{
+	struct child_process hook;
+	const char *argv[10], *env[2];
+	char index[PATH_MAX];
+	va_list args;
+	int i;
+
+	va_start(args, name);
+	argv[0] = git_path("hooks/%s", name);
+	i = 0;
+	do {
+		if (++i >= ARRAY_SIZE(argv))
+			die("run_hook(): too many arguments");
+		argv[i] = va_arg(args, const char *);
+	} while (argv[i]);
+	va_end(args);
+
+	if (access(argv[0], X_OK) < 0)
+		return 0;
+
+	memset(&hook, 0, sizeof(hook));
+	hook.argv = argv;
+	hook.no_stdin = 1;
+	hook.stdout_to_stderr = 1;
+	if (index_file) {
+		snprintf(index, sizeof(index), "GIT_INDEX_FILE=%s", index_file);
+		env[0] = index;
+		env[1] = NULL;
+		hook.env = env;
+	}
+
+	return run_command(&hook);
+}
diff --git a/run-command.h b/run-command.h
index a8b0c20..0211e1d 100644
--- a/run-command.h
+++ b/run-command.h
@@ -49,6 +49,8 @@ int start_command(struct child_process *);
 int finish_command(struct child_process *);
 int run_command(struct child_process *);
 
+extern int run_hook(const char *index_file, const char *name, ...);
+
 #define RUN_COMMAND_NO_STDIN 1
 #define RUN_GIT_CMD	     2	/*If this is to be git sub-command */
 #define RUN_COMMAND_STDOUT_TO_STDERR 4
-- 
1.6.1.160.gecdb

[PATCH 2/2] api-run-command.txt: talk about run_hook()

[Stephan Beyer]

Signed-off-by: Stephan Beyer <s-beyer@gmx.net>
---

	I noticed that the last patch is lacking some documentation.
	Here it is. Should perhaps be squashed.

 Documentation/technical/api-run-command.txt |   16 ++++++++++++++++
 1 files changed, 16 insertions(+), 0 deletions(-)

diff --git a/Documentation/technical/api-run-command.txt b/Documentation/technical/api-run-command.txt
index 82e9e83..1241f48 100644
--- a/Documentation/technical/api-run-command.txt
+++ b/Documentation/technical/api-run-command.txt
@@ -52,6 +52,22 @@ Functions
 	Wait for the completion of an asynchronous function that was
 	started with start_async().
 
+`run_hook`::
+
+	Run a hook.
+	The first argument is a string to an index file or NULL
+	if the default index file or no index is used at all.
+	The second argument is the name of the hook.
+	The further variable number (up to 9) of arguments correspond
+	to the hook arguments.
+	The last argument has to be NULL to terminate the variable
+	arguments list.
+	If the hook is not executable, the return value is zero.
+	If it is executable, the hook will be executed and the exit
+	status of the hook is returned.
+	On execution, .stdout_to_stderr and .no_stdin will be set.
+	(See below.)
+
 
 Data structures
 ---------------
-- 
1.6.1.160.gecdb

Re: [PATCH 1/2] Move run_hook() from builtin-commit.c into run-command.c (libgit)

[Johannes Schindelin]

Hi,

On Thu, 15 Jan 2009, Stephan Beyer wrote:

> 	Stripping out a libified version seemed better to me than
> 	copy and paste.

Oh, definitely.

> -	ret = start_command(&hook);
> -	if (ret) {
> -		warning("Could not spawn %s", argv[0]);
> -		return ret;
> -	}
> -	ret = finish_command(&hook);
> -	if (ret == -ERR_RUN_COMMAND_WAITPID_SIGNAL)
> -		warning("%s exited due to uncaught signal", argv[0]);

What are the side effects of replacing this with "ret = 
run_command(&hook);"?  This has to be discussed and defended in the commit 
message.

> diff --git a/run-command.c b/run-command.c
> index c90cdc5..602fe85 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -342,3 +342,38 @@ int finish_async(struct async *async)
>  #endif
>  	return ret;
>  }
> +
> +int run_hook(const char *index_file, const char *name, ...)
> +{
> +	struct child_process hook;
> +	const char *argv[10], *env[2];
> +	char index[PATH_MAX];
> +	va_list args;
> +	int i;
> +
> +	va_start(args, name);
> +	argv[0] = git_path("hooks/%s", name);
> +	i = 0;
> +	do {
> +		if (++i >= ARRAY_SIZE(argv))
> +			die("run_hook(): too many arguments");
> +		argv[i] = va_arg(args, const char *);
> +	} while (argv[i]);
> +	va_end(args);
> +
> +	if (access(argv[0], X_OK) < 0)
> +		return 0;
> +
> +	memset(&hook, 0, sizeof(hook));
> +	hook.argv = argv;
> +	hook.no_stdin = 1;
> +	hook.stdout_to_stderr = 1;
> +	if (index_file) {
> +		snprintf(index, sizeof(index), "GIT_INDEX_FILE=%s", index_file);
> +		env[0] = index;
> +		env[1] = NULL;
> +		hook.env = env;
> +	}
> +
> +	return run_command(&hook);
> +}

Lots of improvements possible (I agree; _after_ this patch):

- deuglify the loop,

- use ALLOC_GROW instead of having a fixed size argv,

- use an strbuf for the index file

- checking executability of argv[0] before filling argv,

and possibly others, too.

Ciao,
Dscho

Re: [PATCH 2/2] api-run-command.txt: talk about run_hook()

[Jakub Narebski]

Stephan Beyer <s-beyer@gmx.net> writes:

> +`run_hook`::
> +
> +	Run a hook.
> +	The first argument is a string to an index file or NULL
> +	if the default index file or no index is used at all.

Errr...

        The first argument is a filename path to an index file,
        or NULL if hook uses default index file or no index is
        used at all. 

> +	The second argument is the name of the hook.

O.K.

> +	The further variable number (up to 9) of arguments correspond
> +	to the hook arguments.
> +	The last argument has to be NULL to terminate the variable
> +	arguments list.

Why the limitation of maximum of 9 hook arguments?

> +	If the hook is not executable, the return value is zero.

Or the hook does not exist, I assume.

> +	If it is executable, the hook will be executed and the exit
> +	status of the hook is returned.
> +	On execution, .stdout_to_stderr and .no_stdin will be set.
> +	(See below.)
> +
>  
>  Data structures
>  ---------------
> -- 
> 1.6.1.160.gecdb
> 

-- 
Jakub Narebski
Poland
ShadeHawk on #git

Re: [PATCH 2/2] api-run-command.txt: talk about run_hook()

[Miklos Vajna]

[-- Attachment #1: Type: text/plain, Size: 469 bytes --]

On Thu, Jan 15, 2009 at 07:49:51AM -0800, Jakub Narebski <jnareb@gmail.com> wrote:
> > +	The further variable number (up to 9) of arguments correspond
> > +	to the hook arguments.
> > +	The last argument has to be NULL to terminate the variable
> > +	arguments list.
> 
> Why the limitation of maximum of 9 hook arguments?

That's how it is in builtin-commit at the moment, and I agree with Dscho
about it could be a separate patch to remove this limitation.

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH 1/2] Move run_hook() from builtin-commit.c into run-command.c (libgit)

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> On Thu, 15 Jan 2009, Stephan Beyer wrote:
>
>> 	Stripping out a libified version seemed better to me than
>> 	copy and paste.
>
> Oh, definitely.
>
>> -	ret = start_command(&hook);
>> -	if (ret) {
>> -		warning("Could not spawn %s", argv[0]);
>> -		return ret;
>> -	}
>> -	ret = finish_command(&hook);
>> -	if (ret == -ERR_RUN_COMMAND_WAITPID_SIGNAL)
>> -		warning("%s exited due to uncaught signal", argv[0]);
>
> What are the side effects of replacing this with "ret = 
> run_command(&hook);"?  This has to be discussed and defended in the commit 
> message.

I think the answer is "Lost warnings here and there".

Re: [PATCH 1/2] Move run_hook() from builtin-commit.c into run-command.c (libgit)

[Stephan Beyer]

Hi,

> > -	ret = start_command(&hook);
> > -	if (ret) {
> > -		warning("Could not spawn %s", argv[0]);
> > -		return ret;
> > -	}
> > -	ret = finish_command(&hook);
> > -	if (ret == -ERR_RUN_COMMAND_WAITPID_SIGNAL)
> > -		warning("%s exited due to uncaught signal", argv[0]);
> 
> What are the side effects of replacing this with "ret = 
> run_command(&hook);"?  This has to be discussed and defended in the commit 
> message.

This is a very good point.
The consequences are that two warnings are missing, but these are
warnings that are useful enough to be included for all those hooks,
imho.

Thanks!

> Lots of improvements possible (I agree; _after_ this patch):
[...]
> - use ALLOC_GROW instead of having a fixed size argv,

Agreed.

> - use an strbuf for the index file

Is that useful in some way?
Currently it would only adds code to generate strbufs at the caller
side. And in the case the caller has a strbuf for the index file, it
can simply use the .buf member.

> - checking executability of argv[0] before filling argv,

Agreed.

Patch series v2 will follow.

Thanks,
  Stephan

-- 
Stephan Beyer <s-beyer@gmx.net>, PGP 0x6EDDD207FCC5040F

[PATCH v2 1/5] checkout: don't crash on file checkout before running post-checkout hook

[Stephan Beyer]

In the case of

	git init
	echo exit >.git/hooks/post-checkout
	chmod +x .git/hooks/post-checkout
	touch foo
	git add foo
	rm foo
	git checkout -- foo

git-checkout resulted in a Segmentation fault, because there is no new
branch set for the post-checkout hook.

This patch makes use of the null SHA as it is set for the old branch.

While at it, I removed the xstrdup() around the sha1_to_hex(...) calls
in builtin-checkout.c/post_checkout_hook() because sha1_to_hex()
uses four buffers for the hex-dumped SHA and we only need two.
(Duplicating one buffer is only needed if we need more than four.)

Signed-off-by: Stephan Beyer <s-beyer@gmx.net>
---

	I checked if all run_hook()-like functions in the code are
	addressed with my patch and found that the one in builtin-checkout.c
	isn't.  Then I stumbled over this rare-case bug.

 builtin-checkout.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/builtin-checkout.c b/builtin-checkout.c
index b5dd9c0..149343e 100644
--- a/builtin-checkout.c
+++ b/builtin-checkout.c
@@ -47,8 +47,10 @@ static int post_checkout_hook(struct commit *old, struct commit *new,
 
 	memset(&proc, 0, sizeof(proc));
 	argv[0] = name;
-	argv[1] = xstrdup(sha1_to_hex(old ? old->object.sha1 : null_sha1));
-	argv[2] = xstrdup(sha1_to_hex(new->object.sha1));
+	argv[1] = sha1_to_hex(old ? old->object.sha1 : null_sha1);
+	argv[2] = sha1_to_hex(new ? new->object.sha1 : null_sha1);
+	/* "new" can be NULL when checking out from the index before
+	   a commit exists. */
 	argv[3] = changed ? "1" : "0";
 	argv[4] = NULL;
 	proc.argv = argv;
-- 
1.6.1.160.gecdb

[PATCH v2 2/5] Move run_hook() from builtin-commit.c into run-command.c (libgit)

[Stephan Beyer]

A function that runs a hook is used in several Git commands.
builtin-commit.c has the one that is most general for cases without
piping. The one in builtin-gc.c prints some useful warnings.
This patch moves a merged version of these variants into libgit and
lets the other builtins use this libified run_hook().

The run_hook() function used in receive-pack.c feeds the standard
input of the pre-receive or post-receive hooks. This function is
renamed to run_receive_hook() because the libified run_hook() cannot
handle this.

Mentored-by: Daniel Barkalow <barkalow@iabervon.org>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Stephan Beyer <s-beyer@gmx.net>
---

	A quick comparison to v1:
	 - builtin-checkout.c/post_checkout_hook() makes use of
	   run_hook() now.
	 - The lost warnings Dscho pointed at are in the libified
	   run_hook() now.

 builtin-checkout.c     |   22 +++++-----------------
 builtin-commit.c       |   34 ----------------------------------
 builtin-gc.c           |   30 +-----------------------------
 builtin-merge.c        |   31 +------------------------------
 builtin-receive-pack.c |    6 +++---
 run-command.c          |   45 +++++++++++++++++++++++++++++++++++++++++++++
 run-command.h          |    2 ++
 7 files changed, 57 insertions(+), 113 deletions(-)

diff --git a/builtin-checkout.c b/builtin-checkout.c
index 149343e..275176d 100644
--- a/builtin-checkout.c
+++ b/builtin-checkout.c
@@ -38,25 +38,13 @@ struct checkout_opts {
 static int post_checkout_hook(struct commit *old, struct commit *new,
 			      int changed)
 {
-	struct child_process proc;
-	const char *name = git_path("hooks/post-checkout");
-	const char *argv[5];
-
-	if (access(name, X_OK) < 0)
-		return 0;
-
-	memset(&proc, 0, sizeof(proc));
-	argv[0] = name;
-	argv[1] = sha1_to_hex(old ? old->object.sha1 : null_sha1);
-	argv[2] = sha1_to_hex(new ? new->object.sha1 : null_sha1);
+	return run_hook(NULL, "post-checkout",
+			sha1_to_hex(old ? old->object.sha1 : null_sha1),
+			sha1_to_hex(new ? new->object.sha1 : null_sha1),
+			changed ? "1" : "0", NULL);
 	/* "new" can be NULL when checking out from the index before
 	   a commit exists. */
-	argv[3] = changed ? "1" : "0";
-	argv[4] = NULL;
-	proc.argv = argv;
-	proc.no_stdin = 1;
-	proc.stdout_to_stderr = 1;
-	return run_command(&proc);
+
 }
 
 static int update_some(const unsigned char *sha1, const char *base, int baselen,
diff --git a/builtin-commit.c b/builtin-commit.c
index e88b78f..6f8d9fb 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -361,40 +361,6 @@ static int run_status(FILE *fp, const char *index_file, const char *prefix, int
 	return s.commitable;
 }
 
-static int run_hook(const char *index_file, const char *name, ...)
-{
-	struct child_process hook;
-	const char *argv[10], *env[2];
-	char index[PATH_MAX];
-	va_list args;
-	int i;
-
-	va_start(args, name);
-	argv[0] = git_path("hooks/%s", name);
-	i = 0;
-	do {
-		if (++i >= ARRAY_SIZE(argv))
-			die ("run_hook(): too many arguments");
-		argv[i] = va_arg(args, const char *);
-	} while (argv[i]);
-	va_end(args);
-
-	snprintf(index, sizeof(index), "GIT_INDEX_FILE=%s", index_file);
-	env[0] = index;
-	env[1] = NULL;
-
-	if (access(argv[0], X_OK) < 0)
-		return 0;
-
-	memset(&hook, 0, sizeof(hook));
-	hook.argv = argv;
-	hook.no_stdin = 1;
-	hook.stdout_to_stderr = 1;
-	hook.env = env;
-
-	return run_command(&hook);
-}
-
 static int is_a_merge(const unsigned char *sha1)
 {
 	struct commit *commit = lookup_commit(sha1);
diff --git a/builtin-gc.c b/builtin-gc.c
index f8eae4a..a201438 100644
--- a/builtin-gc.c
+++ b/builtin-gc.c
@@ -144,34 +144,6 @@ static int too_many_packs(void)
 	return gc_auto_pack_limit <= cnt;
 }
 
-static int run_hook(void)
-{
-	const char *argv[2];
-	struct child_process hook;
-	int ret;
-
-	argv[0] = git_path("hooks/pre-auto-gc");
-	argv[1] = NULL;
-
-	if (access(argv[0], X_OK) < 0)
-		return 0;
-
-	memset(&hook, 0, sizeof(hook));
-	hook.argv = argv;
-	hook.no_stdin = 1;
-	hook.stdout_to_stderr = 1;
-
-	ret = start_command(&hook);
-	if (ret) {
-		warning("Could not spawn %s", argv[0]);
-		return ret;
-	}
-	ret = finish_command(&hook);
-	if (ret == -ERR_RUN_COMMAND_WAITPID_SIGNAL)
-		warning("%s exited due to uncaught signal", argv[0]);
-	return ret;
-}
-
 static int need_to_gc(void)
 {
 	/*
@@ -194,7 +166,7 @@ static int need_to_gc(void)
 	else if (!too_many_loose_objects())
 		return 0;
 
-	if (run_hook())
+	if (run_hook(NULL, "pre-auto-gc", NULL))
 		return 0;
 	return 1;
 }
diff --git a/builtin-merge.c b/builtin-merge.c
index cf86975..e4555b0 100644
--- a/builtin-merge.c
+++ b/builtin-merge.c
@@ -300,35 +300,6 @@ static void squash_message(void)
 	strbuf_release(&out);
 }
 
-static int run_hook(const char *name)
-{
-	struct child_process hook;
-	const char *argv[3], *env[2];
-	char index[PATH_MAX];
-
-	argv[0] = git_path("hooks/%s", name);
-	if (access(argv[0], X_OK) < 0)
-		return 0;
-
-	snprintf(index, sizeof(index), "GIT_INDEX_FILE=%s", get_index_file());
-	env[0] = index;
-	env[1] = NULL;
-
-	if (squash)
-		argv[1] = "1";
-	else
-		argv[1] = "0";
-	argv[2] = NULL;
-
-	memset(&hook, 0, sizeof(hook));
-	hook.argv = argv;
-	hook.no_stdin = 1;
-	hook.stdout_to_stderr = 1;
-	hook.env = env;
-
-	return run_command(&hook);
-}
-
 static void finish(const unsigned char *new_head, const char *msg)
 {
 	struct strbuf reflog_message = STRBUF_INIT;
@@ -374,7 +345,7 @@ static void finish(const unsigned char *new_head, const char *msg)
 	}
 
 	/* Run a post-merge hook */
-	run_hook("post-merge");
+	run_hook(NULL, "post-merge", squash ? "1" : "0", NULL);
 
 	strbuf_release(&reflog_message);
 }
diff --git a/builtin-receive-pack.c b/builtin-receive-pack.c
index db67c31..6564a97 100644
--- a/builtin-receive-pack.c
+++ b/builtin-receive-pack.c
@@ -136,7 +136,7 @@ static int hook_status(int code, const char *hook_name)
 	}
 }
 
-static int run_hook(const char *hook_name)
+static int run_receive_hook(const char *hook_name)
 {
 	static char buf[sizeof(commands->old_sha1) * 2 + PATH_MAX + 4];
 	struct command *cmd;
@@ -358,7 +358,7 @@ static void execute_commands(const char *unpacker_error)
 		return;
 	}
 
-	if (run_hook(pre_receive_hook)) {
+	if (run_receive_hook(pre_receive_hook)) {
 		while (cmd) {
 			cmd->error_string = "pre-receive hook declined";
 			cmd = cmd->next;
@@ -627,7 +627,7 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)
 			unlink(pack_lockfile);
 		if (report_status)
 			report(unpack_status);
-		run_hook(post_receive_hook);
+		run_receive_hook(post_receive_hook);
 		run_update_post_hook(commands);
 	}
 	return 0;
diff --git a/run-command.c b/run-command.c
index c90cdc5..49810a8 100644
--- a/run-command.c
+++ b/run-command.c
@@ -342,3 +342,48 @@ int finish_async(struct async *async)
 #endif
 	return ret;
 }
+
+int run_hook(const char *index_file, const char *name, ...)
+{
+	struct child_process hook;
+	const char *argv[10], *env[2];
+	char index[PATH_MAX];
+	va_list args;
+	int ret;
+	int i;
+
+	va_start(args, name);
+	argv[0] = git_path("hooks/%s", name);
+	i = 0;
+	do {
+		if (++i >= ARRAY_SIZE(argv))
+			die("run_hook(): too many arguments");
+		argv[i] = va_arg(args, const char *);
+	} while (argv[i]);
+	va_end(args);
+
+	if (access(argv[0], X_OK) < 0)
+		return 0;
+
+	memset(&hook, 0, sizeof(hook));
+	hook.argv = argv;
+	hook.no_stdin = 1;
+	hook.stdout_to_stderr = 1;
+	if (index_file) {
+		snprintf(index, sizeof(index), "GIT_INDEX_FILE=%s", index_file);
+		env[0] = index;
+		env[1] = NULL;
+		hook.env = env;
+	}
+
+	ret = start_command(&hook);
+	if (ret) {
+		warning("Could not spawn %s", argv[0]);
+		return ret;
+	}
+	ret = finish_command(&hook);
+	if (ret == -ERR_RUN_COMMAND_WAITPID_SIGNAL)
+		warning("%s exited due to uncaught signal", argv[0]);
+
+	return ret;
+}
diff --git a/run-command.h b/run-command.h
index a8b0c20..0211e1d 100644
--- a/run-command.h
+++ b/run-command.h
@@ -49,6 +49,8 @@ int start_command(struct child_process *);
 int finish_command(struct child_process *);
 int run_command(struct child_process *);
 
+extern int run_hook(const char *index_file, const char *name, ...);
+
 #define RUN_COMMAND_NO_STDIN 1
 #define RUN_GIT_CMD	     2	/*If this is to be git sub-command */
 #define RUN_COMMAND_STDOUT_TO_STDERR 4
-- 
1.6.1.160.gecdb

[PATCH v2 3/5] api-run-command.txt: talk about run_hook()

[Stephan Beyer]

Signed-off-by: Stephan Beyer <s-beyer@gmx.net>
---

	Jakub, thanks for your notes.
	I still rephrased it some more.

 Documentation/technical/api-run-command.txt |   15 +++++++++++++++
 1 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/Documentation/technical/api-run-command.txt b/Documentation/technical/api-run-command.txt
index 82e9e83..13e7b63 100644
--- a/Documentation/technical/api-run-command.txt
+++ b/Documentation/technical/api-run-command.txt
@@ -52,6 +52,21 @@ Functions
 	Wait for the completion of an asynchronous function that was
 	started with start_async().
 
+`run_hook`::
+
+	Run a hook.
+	The first argument is a pathname to an index file, or NULL
+	if the hook uses the default index file or no index is needed.
+	The second argument is the name of the hook.
+	The further arguments (up to 9) correspond to the hook arguments.
+	The last argument has to be NULL to terminate the arguments list.
+	If the hook does not exist or is not executable, the return
+	value will be zero.
+	If it is executable, the hook will be executed and the exit
+	status of the hook is returned.
+	On execution, .stdout_to_stderr and .no_stdin will be set.
+	(See below.)
+
 
 Data structures
 ---------------
-- 
1.6.1.160.gecdb

[PATCH v2 4/5] run_hook(): check the executability of the hook before filling argv

[Stephan Beyer]

Signed-off-by: Stephan Beyer <s-beyer@gmx.net>
---

	I hope this does not need any more words.
	Thanks to Dscho for the hint: it also saves a "free(argv)"
	and some { } for patch 5/5. :-)

 run-command.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/run-command.c b/run-command.c
index 49810a8..fc54c07 100644
--- a/run-command.c
+++ b/run-command.c
@@ -352,6 +352,9 @@ int run_hook(const char *index_file, const char *name, ...)
 	int ret;
 	int i;
 
+	if (access(git_path("hooks/%s", name), X_OK) < 0)
+		return 0;
+
 	va_start(args, name);
 	argv[0] = git_path("hooks/%s", name);
 	i = 0;
@@ -362,9 +365,6 @@ int run_hook(const char *index_file, const char *name, ...)
 	} while (argv[i]);
 	va_end(args);
 
-	if (access(argv[0], X_OK) < 0)
-		return 0;
-
 	memset(&hook, 0, sizeof(hook));
 	hook.argv = argv;
 	hook.no_stdin = 1;
-- 
1.6.1.160.gecdb

[PATCH 5/5] run_hook(): allow more than 9 hook arguments

[Stephan Beyer]

This is accomplished using the ALLOC_GROW macro.

5 cells are initially allocated for the argv array, allowing
four actual arguments without reallocating memory.

Signed-off-by: Stephan Beyer <s-beyer@gmx.net>
---

	I was a little unsure if it looks better to
	initialize i = 0, alloc = 0
	and do:
		ALLOC_GROW(argv, i + 1, alloc);
		argv[i++] = git_path("hooks/%s", name);
	instead of the xmalloc().

	Do some people care about details like that?

 Documentation/technical/api-run-command.txt |    2 +-
 run-command.c                               |   16 ++++++++--------
 2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/Documentation/technical/api-run-command.txt b/Documentation/technical/api-run-command.txt
index 13e7b63..2efe7a4 100644
--- a/Documentation/technical/api-run-command.txt
+++ b/Documentation/technical/api-run-command.txt
@@ -58,7 +58,7 @@ Functions
 	The first argument is a pathname to an index file, or NULL
 	if the hook uses the default index file or no index is needed.
 	The second argument is the name of the hook.
-	The further arguments (up to 9) correspond to the hook arguments.
+	The further arguments correspond to the hook arguments.
 	The last argument has to be NULL to terminate the arguments list.
 	If the hook does not exist or is not executable, the return
 	value will be zero.
diff --git a/run-command.c b/run-command.c
index fc54c07..22abd09 100644
--- a/run-command.c
+++ b/run-command.c
@@ -346,23 +346,22 @@ int finish_async(struct async *async)
 int run_hook(const char *index_file, const char *name, ...)
 {
 	struct child_process hook;
-	const char *argv[10], *env[2];
+	const char **argv, *env[2];
 	char index[PATH_MAX];
 	va_list args;
 	int ret;
-	int i;
+	size_t i = 1, alloc = 5;
 
 	if (access(git_path("hooks/%s", name), X_OK) < 0)
 		return 0;
 
 	va_start(args, name);
+	argv = xmalloc(alloc * sizeof(const char *));
 	argv[0] = git_path("hooks/%s", name);
-	i = 0;
-	do {
-		if (++i >= ARRAY_SIZE(argv))
-			die("run_hook(): too many arguments");
-		argv[i] = va_arg(args, const char *);
-	} while (argv[i]);
+	while (argv[i-1]) {
+		ALLOC_GROW(argv, i + 1, alloc);
+		argv[i++] = va_arg(args, const char *);
+	}
 	va_end(args);
 
 	memset(&hook, 0, sizeof(hook));
@@ -377,6 +376,7 @@ int run_hook(const char *index_file, const char *name, ...)
 	}
 
 	ret = start_command(&hook);
+	free(argv);
 	if (ret) {
 		warning("Could not spawn %s", argv[0]);
 		return ret;
-- 
1.6.1.160.gecdb

Re: [PATCH 1/2] Move run_hook() from builtin-commit.c into run-command.c (libgit)

[Johannes Schindelin]

Hi,

On Fri, 16 Jan 2009, Stephan Beyer wrote:

> Dscho wrote:
>
> > Lots of improvements possible (I agree; _after_ this patch):
> [...]
> 
> > - use an strbuf for the index file
> 
> Is that useful in some way?
> Currently it would only adds code to generate strbufs at the caller
> side. And in the case the caller has a strbuf for the index file, it
> can simply use the .buf member.

Of course I meant the buffer into which "GIT_INDEX_FILE=..." is written, 
which is fixed size ATM.

Ciao,
Dscho

Re: [PATCH 5/5] run_hook(): allow more than 9 hook arguments

[Johannes Schindelin]

Hi,

On Fri, 16 Jan 2009, Stephan Beyer wrote:

> This is accomplished using the ALLOC_GROW macro.
> 
> 5 cells are initially allocated for the argv array, allowing
> four actual arguments without reallocating memory.
> 
> Signed-off-by: Stephan Beyer <s-beyer@gmx.net>
> ---
> 
> 	I was a little unsure if it looks better to
> 	initialize i = 0, alloc = 0
> 	and do:
> 		ALLOC_GROW(argv, i + 1, alloc);
> 		argv[i++] = git_path("hooks/%s", name);
> 	instead of the xmalloc().
> 
> 	Do some people care about details like that?

	Actually, you need to do it before setting argv[0], and I think it 
	would look better to let ALLOC_GROW() handle the allocation: less 
	opportunity for bugs to lurk.

	Besides that, ALLOC_GROW() will allocate 16 entries initially, 
	plenty enough for anyone, I guess.

Ciao,
Dscho

[PATCH v2 5/5] run_hook(): allow more than 9 hook arguments

[Stephan Beyer]

This is done using the ALLOC_GROW macro.

Signed-off-by: Stephan Beyer <s-beyer@gmx.net>
---

	Ok, Dscho :-)

	The interdiff based on [PATCH 5/5] is...

	--- a/run-command.c
	+++ b/run-command.c
	@@ -350,14 +350,14 @@ int run_hook(const char *index_file, const char *name, ...)
		char index[PATH_MAX];
		va_list args;
		int ret;
	-	size_t i = 1, alloc = 5;
	+	size_t i = 0, alloc = 0;
	 
		if (access(git_path("hooks/%s", name), X_OK) < 0)
			return 0;
	 
		va_start(args, name);
	-	argv = xmalloc(alloc * sizeof(const char *));
	-	argv[0] = git_path("hooks/%s", name);
	+	ALLOC_GROW(argv, i + 1, alloc);
	+	argv[i++] = git_path("hooks/%s", name);
		while (argv[i-1]) {
			ALLOC_GROW(argv, i + 1, alloc);
			argv[i++] = va_arg(args, const char *);

 Documentation/technical/api-run-command.txt |    2 +-
 run-command.c                               |   18 +++++++++---------
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/Documentation/technical/api-run-command.txt b/Documentation/technical/api-run-command.txt
index 13e7b63..2efe7a4 100644
--- a/Documentation/technical/api-run-command.txt
+++ b/Documentation/technical/api-run-command.txt
@@ -58,7 +58,7 @@ Functions
 	The first argument is a pathname to an index file, or NULL
 	if the hook uses the default index file or no index is needed.
 	The second argument is the name of the hook.
-	The further arguments (up to 9) correspond to the hook arguments.
+	The further arguments correspond to the hook arguments.
 	The last argument has to be NULL to terminate the arguments list.
 	If the hook does not exist or is not executable, the return
 	value will be zero.
diff --git a/run-command.c b/run-command.c
index fc54c07..d2f1262 100644
--- a/run-command.c
+++ b/run-command.c
@@ -346,23 +346,22 @@ int finish_async(struct async *async)
 int run_hook(const char *index_file, const char *name, ...)
 {
 	struct child_process hook;
-	const char *argv[10], *env[2];
+	const char **argv, *env[2];
 	char index[PATH_MAX];
 	va_list args;
 	int ret;
-	int i;
+	size_t i = 0, alloc = 0;
 
 	if (access(git_path("hooks/%s", name), X_OK) < 0)
 		return 0;
 
 	va_start(args, name);
-	argv[0] = git_path("hooks/%s", name);
-	i = 0;
-	do {
-		if (++i >= ARRAY_SIZE(argv))
-			die("run_hook(): too many arguments");
-		argv[i] = va_arg(args, const char *);
-	} while (argv[i]);
+	ALLOC_GROW(argv, i + 1, alloc);
+	argv[i++] = git_path("hooks/%s", name);
+	while (argv[i-1]) {
+		ALLOC_GROW(argv, i + 1, alloc);
+		argv[i++] = va_arg(args, const char *);
+	}
 	va_end(args);
 
 	memset(&hook, 0, sizeof(hook));
@@ -377,6 +376,7 @@ int run_hook(const char *index_file, const char *name, ...)
 	}
 
 	ret = start_command(&hook);
+	free(argv);
 	if (ret) {
 		warning("Could not spawn %s", argv[0]);
 		return ret;
-- 
1.6.1.160.gecdb

Re: [PATCH v2 1/5] checkout: don't crash on file checkout before running post-checkout hook

[Junio C Hamano]

All looked good to me except one thing; you need to initialize argv to
NULL as ALLOC_GROW() calls xrealloc on it in the last one.

Will queue with an amend.

Re: [PATCH v2 1/5] checkout: don't crash on file checkout before running post-checkout hook

[Stephan Beyer]

Hi,

Junio C Hamano wrote:
> All looked good to me except one thing; you need to initialize argv to
> NULL as ALLOC_GROW() calls xrealloc on it in the last one.

This is true. Sorry.

> Will queue with an amend.

Thank you.

  Stephan

-- 
Stephan Beyer <s-beyer@gmx.net>, PGP 0x6EDDD207FCC5040F