[PATCH 1/4] git-commit: support variable number of hook arguments

[PATCH 1/4] git-commit: support variable number of hook arguments

[Paolo Bonzini]

This is a preparatory patch to allow using run_hook for the
prepare-commit-msg hook.

Signed-off-by: Paolo Bonzini <bonzini@gnu.org>

---
 builtin-commit.c |   59 ++++++++++++++++++++++++++++++-------------------------
 1 files changed, 33 insertions(+), 26 deletions(-)

diff --git a/builtin-commit.c b/builtin-commit.c
index 0227936..fc59660 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -339,6 +339,38 @@ 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 {
+		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 const char sign_off_header[] = "Signed-off-by: ";
 
 static int prepare_log_message(const char *index_file, const char *prefix)
@@ -673,31 +705,6 @@ int cmd_status(int argc, const char **argv, const char *prefix)
 	return commitable ? 0 : 1;
 }
 
-static int run_hook(const char *index_file, const char *name, const char *arg)
-{
-	struct child_process hook;
-	const char *argv[3], *env[2];
-	char index[PATH_MAX];
-
-	argv[0] = git_path("hooks/%s", name);
-	argv[1] = arg;
-	argv[2] = NULL;
-	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 void print_summary(const char *prefix, const unsigned char *sha1)
 {
 	struct rev_info rev;
@@ -872,7 +879,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		launch_editor(git_path(commit_editmsg), NULL, env);
 	}
 	if (!no_verify &&
-	    run_hook(index_file, "commit-msg", git_path(commit_editmsg))) {
+	    run_hook(index_file, "commit-msg", git_path(commit_editmsg), NULL)) {
 		rollback_index_files();
 		exit(1);
 	}

[PATCH 2/4] git-commit: set GIT_EDITOR=: if editor will not be launched

[Paolo Bonzini]

This is a preparatory patch that provides a simple way for the future
prepare-commit-msg hook to discover if the editor will be launched.

Signed-off-by: Paolo Bonzini <bonzini@gnu.org>

---
 Documentation/hooks.txt |    4 ++++
 builtin-commit.c        |    2 ++
 2 files changed, 6 insertions(+)

diff --git a/Documentation/hooks.txt b/Documentation/hooks.txt
index f110162..e8d80cf 100644
--- a/Documentation/hooks.txt
+++ b/Documentation/hooks.txt
@@ -61,6 +61,10 @@ The default 'pre-commit' hook, when enabled, catches introduction
 of lines with trailing whitespaces and aborts the commit when
 such a line is found.
 
+All the `git-commit` hooks are invoked with the environment
+variable `GIT_EDITOR=:` if the command will not bring up an editor
+to modify the commit message.
+
 commit-msg
 ----------
 
diff --git a/builtin-commit.c b/builtin-commit.c
index fc59660..955cc84 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -593,6 +593,8 @@ static int parse_and_validate_options(int argc, const char *argv[],
 		use_editor = 0;
 	if (edit_flag)
 		use_editor = 1;
+	if (!use_editor)
+		setenv("GIT_EDITOR", ":", 1);
 
 	if (get_sha1("HEAD", head_sha1))
 		initial_commit = 1;

[PATCH] git-commit: add a prepare-commit-msg hook

[Paolo Bonzini]

This series of patches adds a prepare-commit-msg hook.
The prepare-commit-msg hook is run whenever a "fresh" commit message
is prepared, just before it is shown in the editor (if it is).
It can modify the commit message in-place and/or abort the commit.
I implemented Alex Riesen's suggestion to tell the hook where the
message came from, and now run the hook even if the editor is not
run.

Patches 1 and 2 are small changes.  Patch 1 changes run_hook to
accept a variable-length NULL-terminated list of arguments.  Patch 2
forces GIT_EDITOR to : if editor will not be launched; this is the
simplest way I found to tell the prepare-commit-msg hook whether
the editor will be launched.

Patch 3 is bigger; it refactors parts of git-commit to do all the
log message processing at the same time.  Currently the message
is prepared soon, but only edited after the first part of the commit
object is prepared.  This simplifies a little the code for part 4.

Part 4 actually adds the hook, including documentation and testcases.
The hook takes two parameters.  The first is the source of the commit
message (detailed more in the commit message and in the docs), which
is either an English word or a commit SHA1.  The second
parameter if the name of the file that the commit log message.

Signed-off-by: Paolo Bonzini <bonzini@gnu.org>

[PATCH 4/4] git-commit: add a prepare-commit-msg hook

[Paolo Bonzini]

The prepare-commit-msg hook is run whenever a "fresh" commit message
is prepared, just before it is shown in the editor (if it is).
It can modify the commit message in-place and/or abort the commit.

It takes two parameters.  The first is the source of the commit
message, and can be: `message` (if a `\-m` or `\-F` option was
given); `template` (if a `\-t` option was given or the
configuration option `commit.template` is set); `merge` (if the
commit is a merge or a `.git/MERGE_MSG file exists); `squash`
(if a `.git/SQUASH_MSG file exists); or a commit id (if a
`\-c`, `\-C` or `\--amend` option was given).  The second
parameter if the name of the file that the commit log message.

The hook is not suppressed by the `\--no-verify` option.  However,
exiting with non-zero status only aborts the commit if said option
is not given to `git-commit`.

While the default hook just adds a Signed-Off-By line at the bottom
of the commit messsage, the hook is more intended to build a template
for the commit message following project standards, that the user
can then edit or discard altogether.

Signed-off-by: Paolo Bonzini <bonzini@gnu.org>

---
 Documentation/git-commit.txt        |    4 
 Documentation/hooks.txt             |   33 +++
 builtin-commit.c                    |   21 ++
 t/t7505-prepare-commit-msg-hook.sh  |  303 ++++++++++++++++++++++++++++++++++++
 templates/hooks--commit-msg         |    3 
 templates/hooks--prepare-commit-msg |   14 +
 6 files changed, 375 insertions(+), 3 deletions(-)

	The whole patch series is against the `next' branch.

diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index c3725b2..b4ae61f 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt
@@ -280,8 +280,8 @@ order).
 
 HOOKS
 -----
-This command can run `commit-msg`, `pre-commit`, and
-`post-commit` hooks.  See link:hooks.html[hooks] for more
+This command can run `commit-msg`, `prepare-commit-msg`, `pre-commit`,
+and `post-commit` hooks.  See link:hooks.html[hooks] for more
 information.
 
 
diff --git a/Documentation/hooks.txt b/Documentation/hooks.txt
index e8d80cf..f25bcd5 100644
--- a/Documentation/hooks.txt
+++ b/Documentation/hooks.txt
@@ -55,7 +55,8 @@ This hook is invoked by `git-commit`, and can be bypassed
 with `\--no-verify` option.  It takes no parameter, and is
 invoked before obtaining the proposed commit log message and
 making a commit.  Exiting with non-zero status from this script
-causes the `git-commit` to abort.
+causes the `git-commit` to abort.  This hook can also modify
+the index.
 
 The default 'pre-commit' hook, when enabled, catches introduction
 of lines with trailing whitespaces and aborts the commit when
@@ -65,6 +66,36 @@ All the `git-commit` hooks are invoked with the environment
 variable `GIT_EDITOR=:` if the command will not bring up an editor
 to modify the commit message.
 
+prepare-commit-msg
+------------------
+
+This hook is invoked by `git-commit` right after preparing the
+default log message, and before the editor is started.
+
+It takes two parameters.  The first is the source of the commit
+message, and can be: `message` (if a `\-m` or `\-F` option was
+given); `template` (if a `\-t` option was given or the
+configuration option `commit.template` is set); `merge` (if the
+commit is a merge or a `.git/MERGE_MSG file exists); `squash`
+(if a `.git/SQUASH_MSG file exists); or a commit id (if a
+`\-c`, `\-C` or `\--amend` option was given).  The second
+parameter if the name of the file that the commit log message.
+
+The hook is not suppressed by the `\--no-verify` option.  However,
+exiting with non-zero status only aborts the commit if said option
+is not given to `git-commit`.
+
+The hook is allowed to edit the message file in place, and
+can be used to augment the default commit message with some
+project standard information.  It can also be used for the same
+purpose as the pre-commit message, if the verification has
+to be skipped for automatic commits (e.g. during rebasing).
+
+The default 'prepare-commit-msg' hook adds a Signed-Off-By line
+(doing it with a hook is not necessarily a good idea, but doing
+it in 'commit-msg' is worse because you are not reminded in
+the editor).
+
 commit-msg
 ----------
 
diff --git a/builtin-commit.c b/builtin-commit.c
index fed549e..1a083f7 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -388,36 +388,49 @@ static int prepare_log_message(const char *index_file, const char *prefix)
 	struct strbuf sb;
 	char *buffer;
 	FILE *fp;
+	const char *hook_arg = NULL;
 
 	strbuf_init(&sb, 0);
 	if (message.len) {
 		strbuf_addbuf(&sb, &message);
+		hook_arg = "message";
 	} else if (logfile && !strcmp(logfile, "-")) {
 		if (isatty(0))
 			fprintf(stderr, "(reading log message from standard input)\n");
 		if (strbuf_read(&sb, 0, 0) < 0)
 			die("could not read log from standard input");
+		hook_arg = "message";
 	} else if (logfile) {
 		if (strbuf_read_file(&sb, logfile, 0) < 0)
 			die("could not read log file '%s': %s",
 			    logfile, strerror(errno));
+		hook_arg = "message";
 	} else if (use_message) {
 		buffer = strstr(use_message_buffer, "\n\n");
 		if (!buffer || buffer[2] == '\0')
 			die("commit has empty message");
 		strbuf_add(&sb, buffer + 2, strlen(buffer + 2));
+		hook_arg = use_message;
 	} else if (!stat(git_path("MERGE_MSG"), &statbuf)) {
 		if (strbuf_read_file(&sb, git_path("MERGE_MSG"), 0) < 0)
 			die("could not read MERGE_MSG: %s", strerror(errno));
+		hook_arg = "merge";
 	} else if (!stat(git_path("SQUASH_MSG"), &statbuf)) {
 		if (strbuf_read_file(&sb, git_path("SQUASH_MSG"), 0) < 0)
 			die("could not read SQUASH_MSG: %s", strerror(errno));
+		hook_arg = "squash";
 	} else if (template_file && !stat(template_file, &statbuf)) {
 		if (strbuf_read_file(&sb, template_file, 0) < 0)
 			die("could not read %s: %s",
 			    template_file, strerror(errno));
+		hook_arg = "template";
 	}
 
+	/* This final case does not modify the template message, it just sets
+	   the argument to the prepare-commit-msg hook.  */
+	else if (in_merge)
+		hook_arg = "merge";
+
 	fp = fopen(git_path(commit_editmsg), "w");
 	if (fp == NULL)
 		die("could not open %s", git_path(commit_editmsg));
@@ -511,6 +524,14 @@ static int prepare_log_message(const char *index_file, const char *prefix)
 		return 0;
 	}
 
+	/* Note that we always run the hook, even if no_verify! */
+	if (run_hook(index_file, "prepare-commit-msg",
+		     git_path(commit_editmsg), hook_arg, NULL) &&
+	    !no_verify) {
+		rollback_index_files();
+		return 0;
+	}
+
 	if (use_editor) {
 		char index[PATH_MAX];
 		const char *env[2] = { index, NULL };
diff --git a/t/t7505-prepare-commit-msg-hook.sh b/t/t7505-prepare-commit-msg-hook.sh
new file mode 100755
index 0000000..e632bbe
--- /dev/null
+++ b/t/t7505-prepare-commit-msg-hook.sh
@@ -0,0 +1,303 @@
+#!/bin/sh
+
+test_description='prepare-commit-msg hook'
+
+. ./test-lib.sh
+
+test_expect_success 'with no hook' '
+
+	echo "foo" > file &&
+	git add file &&
+	git commit -m "first"
+
+'
+
+# set up fake editor for interactive editing
+cat > fake-editor <<'EOF'
+#!/bin/sh
+exit 0
+EOF
+chmod +x fake-editor
+FAKE_EDITOR="$(pwd)/fake-editor"
+export FAKE_EDITOR
+
+# now install hook that always succeeds and adds a message
+HOOKDIR="$(git rev-parse --git-dir)/hooks"
+HOOK="$HOOKDIR/prepare-commit-msg"
+mkdir -p "$HOOKDIR"
+cat > "$HOOK" <<'EOF'
+#!/bin/sh
+if test "$2" = HEAD; then
+  set "$1" $(git-rev-parse HEAD)
+fi
+if test "$GIT_EDITOR" = :; then
+  sed -e "1s/.*/${2-default} (no editor)/" "$1" > msg.tmp
+else
+  sed -e "1s/.*/${2-default}/" "$1" > msg.tmp
+fi
+mv msg.tmp "$1"
+exit 0
+EOF
+chmod +x "$HOOK"
+
+echo dummy template > "$(git rev-parse --git-dir)/template"
+
+test_expect_success 'with succeeding hook (-m)' '
+
+	echo "more" >> file &&
+	git add file &&
+	git commit -m "more" &&
+	test "`git log -1 --pretty=format:%s`" = "message (no editor)"
+
+'
+
+test_expect_success 'with succeeding hook (-m editor)' '
+
+	echo "more" >> file &&
+	git add file &&
+	GIT_EDITOR="$FAKE_EDITOR" git commit -e -m "more more" &&
+	test "`git log -1 --pretty=format:%s`" = message
+
+'
+
+test_expect_success 'with succeeding hook (-t)' '
+
+	echo "more" >> file &&
+	git add file &&
+	git commit -t "$(git rev-parse --git-dir)/template" &&
+	test "`git log -1 --pretty=format:%s`" = template
+
+'
+
+test_expect_success 'with succeeding hook (-F)' '
+
+	echo "more" >> file &&
+	git add file &&
+	(echo more | git commit -F -) &&
+	test "`git log -1 --pretty=format:%s`" = "message (no editor)"
+
+'
+
+test_expect_success 'with succeeding hook (-F editor)' '
+
+	echo "more" >> file &&
+	git add file &&
+	(echo more more | GIT_EDITOR="$FAKE_EDITOR" git commit -e -F -) &&
+	test "`git log -1 --pretty=format:%s`" = message
+
+'
+
+test_expect_success 'with succeeding hook (-C)' '
+
+	head=`git rev-parse HEAD` &&
+	echo "more" >> file &&
+	git add file &&
+	git commit -C $head &&
+	test "`git log -1 --pretty=format:%s`" = "$head (no editor)"
+
+'
+
+test_expect_success 'with succeeding hook (editor)' '
+
+	echo "more more" >> file &&
+	git add file &&
+	GIT_EDITOR="$FAKE_EDITOR" git commit &&
+	test "`git log -1 --pretty=format:%s`" = default
+
+'
+
+test_expect_success 'with succeeding hook (--amend)' '
+
+	head=`git rev-parse HEAD` &&
+	echo "more" >> file &&
+	git add file &&
+	GIT_EDITOR="$FAKE_EDITOR" git commit --amend &&
+	test "`git log -1 --pretty=format:%s`" = "$head"
+
+'
+
+test_expect_success 'with succeeding hook (-c)' '
+
+	head=`git rev-parse HEAD` &&
+	echo "more" >> file &&
+	git add file &&
+	GIT_EDITOR="$FAKE_EDITOR" git commit -c $head &&
+	test "`git log -1 --pretty=format:%s`" = "$head"
+
+'
+
+# now a hook that fails
+cat > "$HOOK" << 'EOF'
+#!/bin/sh
+if test "$2" = HEAD; then
+  set "$1" $(git-rev-parse HEAD)
+fi
+if test "$GIT_EDITOR" = :; then
+  sed -e "1s/.*/${2-default} (no editor)/" "$1" > msg.tmp
+else
+  sed -e "1s/.*/${2-default}/" "$1" > msg.tmp
+fi
+mv msg.tmp "$1"
+exit 1
+EOF
+
+test_expect_success 'with failing hook and --no-verify (-m)' '
+
+	echo "more" >> file &&
+	git add file &&
+	git commit --no-verify -m "more" &&
+	test "`git log -1 --pretty=format:%s`" = "message (no editor)"
+
+'
+
+test_expect_success 'with failing hook and --no-verify (-m editor)' '
+
+	echo "more" >> file &&
+	git add file &&
+	GIT_EDITOR="$FAKE_EDITOR" git commit --no-verify -e -m "more more" &&
+	test "`git log -1 --pretty=format:%s`" = message
+
+'
+
+test_expect_success 'with failing hook and --no-verify (-t)' '
+
+	echo "more" >> file &&
+	git add file &&
+	git commit --no-verify -t "$(git rev-parse --git-dir)/template" &&
+	test "`git log -1 --pretty=format:%s`" = template
+
+'
+
+test_expect_success 'with failing hook and --no-verify (-F)' '
+
+	echo "more" >> file &&
+	git add file &&
+	(echo more | git commit --no-verify -F -) &&
+	test "`git log -1 --pretty=format:%s`" = "message (no editor)"
+
+'
+
+test_expect_success 'with failing hook and --no-verify (-F editor)' '
+
+	echo "more" >> file &&
+	git add file &&
+	(echo more more | GIT_EDITOR="$FAKE_EDITOR" git commit --no-verify -e -F -) &&
+	test "`git log -1 --pretty=format:%s`" = message
+
+'
+
+test_expect_success 'with failing hook and --no-verify (-C)' '
+
+	head=`git rev-parse HEAD` &&
+	echo "more" >> file &&
+	git add file &&
+	git commit --no-verify -C $head &&
+	test "`git log -1 --pretty=format:%s`" = "$head (no editor)"
+
+'
+
+test_expect_success 'with failing hook and --no-verify (editor)' '
+
+	echo "more more" >> file &&
+	git add file &&
+	GIT_EDITOR="$FAKE_EDITOR" git commit --no-verify &&
+	test "`git log -1 --pretty=format:%s`" = default
+
+'
+
+test_expect_success 'with failing hook and --no-verify (--amend)' '
+
+	head=`git rev-parse HEAD` &&
+	echo "more" >> file &&
+	git add file &&
+	GIT_EDITOR="$FAKE_EDITOR" git commit --no-verify --amend &&
+	test "`git log -1 --pretty=format:%s`" = "$head"
+
+'
+
+test_expect_success 'with failing hook and --no-verify (-c)' '
+
+	head=`git rev-parse HEAD` &&
+	echo "more" >> file &&
+	git add file &&
+	GIT_EDITOR="$FAKE_EDITOR" git commit --no-verify -c $head &&
+	test "`git log -1 --pretty=format:%s`" = "$head"
+
+'
+
+test_expect_failure 'with failing hook (-m)' '
+
+	echo "more" >> file &&
+	git add file &&
+	git commit -m "more"
+
+'
+
+test_expect_failure 'with failing hook (-m editor)' '
+
+	echo "more" >> file &&
+	git add file &&
+	GIT_EDITOR="$FAKE_EDITOR" git commit -e -m "more more"
+
+'
+
+test_expect_failure 'with failing hook (-t)' '
+
+	echo "more" >> file &&
+	git add file &&
+	git commit -t "$(git rev-parse --git-dir)/template"
+
+'
+
+test_expect_failure 'with failing hook (-F)' '
+
+	echo "more" >> file &&
+	git add file &&
+	(echo more | git commit -F -)
+
+'
+
+test_expect_failure 'with failing hook (-F editor)' '
+
+	echo "more" >> file &&
+	git add file &&
+	(echo more more | GIT_EDITOR="$FAKE_EDITOR" git commit -e -F -)
+
+'
+
+test_expect_failure 'with failing hook (-C)' '
+
+	head=`git rev-parse HEAD` &&
+	echo "more" >> file &&
+	git add file &&
+	git commit -C $head
+
+'
+
+test_expect_failure 'with failing hook (editor)' '
+
+	echo "more more" >> file &&
+	git add file &&
+	GIT_EDITOR="$FAKE_EDITOR" git commit
+
+'
+
+test_expect_failure 'with failing hook (--amend)' '
+
+	echo "more" >> file &&
+	git add file &&
+	GIT_EDITOR="$FAKE_EDITOR" git commit --amend
+
+'
+
+test_expect_failure 'with failing hook (-c)' '
+
+	head=`git rev-parse HEAD` &&
+	echo "more" >> file &&
+	git add file &&
+	GIT_EDITOR="$FAKE_EDITOR" git commit -c $head
+
+'
+
+
+test_done
diff --git a/templates/hooks--commit-msg b/templates/hooks--commit-msg
index c5cdb9d..4ef86eb 100644
--- a/templates/hooks--commit-msg
+++ b/templates/hooks--commit-msg
@@ -9,6 +9,9 @@
 # To enable this hook, make this file executable.
 
 # Uncomment the below to add a Signed-off-by line to the message.
+# Doing this in a hook is a bad idea in general, but the prepare-commit-msg
+# hook is more suited to it.
+#
 # SOB=$(git var GIT_AUTHOR_IDENT | sed -n 's/^\(.*>\).*$/Signed-off-by: \1/p')
 # grep -qs "^$SOB" "$1" || echo "$SOB" >> "$1"
 
diff --git a/templates/hooks--prepare-commit-msg b/templates/hooks--prepare-commit-msg
new file mode 100644
index 0000000..176283b
--- /dev/null
+++ b/templates/hooks--prepare-commit-msg
@@ -0,0 +1,14 @@
+#!/bin/sh
+#
+# An example hook script to prepare the commit log message.
+# Called by git-commit with one argument, the name of the file
+# that has the commit message.  The hook should exit with non-zero
+# status after issuing an appropriate message if it wants to stop the
+# commit.  The hook is allowed to edit the commit message file.
+#
+# To enable this hook, make this file executable.
+
+# This example adds a Signed-off-by line to the message, that can
+# still be edited.
+SOB=$(git var GIT_AUTHOR_IDENT | sed -n 's/^\(.*>\).*$/Signed-off-by: \1/p')
+grep -qs "^$SOB" "$1" || echo "$SOB" >> "$1"

[PATCH 3/4] git-commit: Refactor creation of log message.

[Paolo Bonzini]

This patch moves around some code from run_commit to prepare_log_message.
This simplifies a little the code for the next patch.  The biggest change
is that the editor and the commit-msg hook are invoked before building
the prolog of the commit object.

This means that: 1) the commit may be aborted after editing the message
if there is a problem writing out the tree object (slight disadvantage);
2) no tree will be written out if the commit-msg hook fails (slight
advantage).

Signed-off-by: Paolo Bonzini <bonzini@gnu.org>

---
 builtin-commit.c |  114 ++++++++++++++++++++++++++++---------------------------
 1 files changed, 60 insertions(+), 54 deletions(-)

diff --git a/builtin-commit.c b/builtin-commit.c
index 955cc84..fed549e 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -371,6 +371,14 @@ static int run_hook(const char *index_file, const char *name, ...)
 	return run_command(&hook);
 }
 
+static int is_a_merge(const unsigned char *sha1)
+{
+	struct commit *commit = lookup_commit(sha1);
+	if (!commit || parse_commit(commit))
+		die("could not parse HEAD commit");
+	return !!(commit->parents && commit->parents->next);
+}
+
 static const char sign_off_header[] = "Signed-off-by: ";
 
 static int prepare_log_message(const char *index_file, const char *prefix)
@@ -441,13 +449,38 @@ static int prepare_log_message(const char *index_file, const char *prefix)
 
 	strbuf_release(&sb);
 
-	if (!use_editor) {
+	if (use_editor) {
+		if (in_merge)
+			fprintf(fp,
+				"#\n"
+				"# It looks like you may be committing a MERGE.\n"
+				"# If this is not correct, please remove the file\n"
+				"#	%s\n"
+				"# and try again.\n"
+				"#\n",
+				git_path("MERGE_HEAD"));
+
+		fprintf(fp,
+			"\n"
+			"# Please enter the commit message for your changes.\n"
+			"# (Comment lines starting with '#' will ");
+		if (cleanup_mode == CLEANUP_ALL)
+			fprintf(fp, "not be included)\n");
+		else /* CLEANUP_SPACE, that is. */
+			fprintf(fp, "be kept.\n"
+				"# You can remove them yourself if you want to)\n");
+		if (only_include_assumed)
+			fprintf(fp, "# %s\n", only_include_assumed);
+
+		saved_color_setting = wt_status_use_color;
+		wt_status_use_color = 0;
+		commitable = run_status(fp, index_file, prefix, 1);
+		wt_status_use_color = saved_color_setting;
+	} else {
 		struct rev_info rev;
 		unsigned char sha1[20];
 		const char *parent = "HEAD";
 
-		fclose(fp);
-
 		if (!active_nr && read_cache() < 0)
 			die("Cannot read index");
 
@@ -464,39 +499,31 @@ static int prepare_log_message(const char *index_file, const char *prefix)
 		DIFF_OPT_SET(&rev.diffopt, EXIT_WITH_STATUS);
 		run_diff_index(&rev, 1 /* cached */);
 
-		return !!DIFF_OPT_TST(&rev.diffopt, HAS_CHANGES);
+		commitable = !!DIFF_OPT_TST(&rev.diffopt, HAS_CHANGES);
 	}
 
-	if (in_merge)
-		fprintf(fp,
-			"#\n"
-			"# It looks like you may be committing a MERGE.\n"
-			"# If this is not correct, please remove the file\n"
-			"#	%s\n"
-			"# and try again.\n"
-			"#\n",
-			git_path("MERGE_HEAD"));
-
-	fprintf(fp,
-		"\n"
-		"# Please enter the commit message for your changes.\n"
-		"# (Comment lines starting with '#' will ");
-	if (cleanup_mode == CLEANUP_ALL)
-		fprintf(fp, "not be included)\n");
-	else /* CLEANUP_SPACE, that is. */
-		fprintf(fp, "be kept.\n"
-			"# You can remove them yourself if you want to)\n");
-	if (only_include_assumed)
-		fprintf(fp, "# %s\n", only_include_assumed);
-
-	saved_color_setting = wt_status_use_color;
-	wt_status_use_color = 0;
-	commitable = run_status(fp, index_file, prefix, 1);
-	wt_status_use_color = saved_color_setting;
-
 	fclose(fp);
 
-	return commitable;
+	if (!commitable && !in_merge && !allow_empty &&
+	    !(amend && is_a_merge(head_sha1))) {
+		run_status(stdout, index_file, prefix, 0);
+		unlink(commit_editmsg);
+		return 0;
+	}
+
+	if (use_editor) {
+		char index[PATH_MAX];
+		const char *env[2] = { index, NULL };
+		snprintf(index, sizeof(index), "GIT_INDEX_FILE=%s", index_file);
+		launch_editor(git_path(commit_editmsg), NULL, env);
+	}
+
+	if (!no_verify &&
+	    run_hook(index_file, "commit-msg", git_path(commit_editmsg), NULL)) {
+		return 0;
+	}
+
+	return 1;
 }
 
 /*
@@ -755,14 +782,6 @@ int git_commit_config(const char *k, const char *v)
 	return git_status_config(k, v);
 }
 
-static int is_a_merge(const unsigned char *sha1)
-{
-	struct commit *commit = lookup_commit(sha1);
-	if (!commit || parse_commit(commit))
-		die("could not parse HEAD commit");
-	return !!(commit->parents && commit->parents->next);
-}
-
 static const char commit_utf8_warn[] =
 "Warning: commit message does not conform to UTF-8.\n"
 "You may want to amend it after fixing the message, or set the config\n"
@@ -799,11 +818,9 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		return 1;
 	}
 
-	if (!prepare_log_message(index_file, prefix) && !in_merge &&
-	    !allow_empty && !(amend && is_a_merge(head_sha1))) {
-		run_status(stdout, index_file, prefix, 0);
+	/* Prepare, let the user edit, and validate the log message.  */
+	if (!prepare_log_message(index_file, prefix)) {
 		rollback_index_files();
-		unlink(commit_editmsg);
 		return 1;
 	}
 
@@ -872,19 +889,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		strbuf_addf(&sb, "encoding %s\n", git_commit_encoding);
 	strbuf_addch(&sb, '\n');
 
-	/* Get the commit message and validate it */
+	/* Finally, get the commit message */
 	header_len = sb.len;
-	if (use_editor) {
-		char index[PATH_MAX];
-		const char *env[2] = { index, NULL };
-		snprintf(index, sizeof(index), "GIT_INDEX_FILE=%s", index_file);
-		launch_editor(git_path(commit_editmsg), NULL, env);
-	}
-	if (!no_verify &&
-	    run_hook(index_file, "commit-msg", git_path(commit_editmsg), NULL)) {
-		rollback_index_files();
-		exit(1);
-	}
 	if (strbuf_read_file(&sb, git_path(commit_editmsg), 0) < 0) {
 		rollback_index_files();
 		die("could not read commit message");

Re: [PATCH 1/4] git-commit: support variable number of hook arguments

[Johannes Schindelin]

Hi,

On Mon, 21 Jan 2008, Paolo Bonzini wrote:

> +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 {

Here, an

		if (++i >= ARRAY_SIZE(argv))
			die ("run_hook(): too many arguments");

is missing.  Even nicer, you could have

	int argc = 1;
	char **argv = malloc(sizeof(*argv) * 2);

and

		if (++i >= argc)
			argc = ALLOC_GROW(argv, i + 1, argc);

in the loop.  Of course, you would have to change the "++i" to "i" in this 
line:

> +		argv[++i] = va_arg(args, const char *);

Ciao,
Dscho

Re: [PATCH 3/4] git-commit: Refactor creation of log message.

[Johannes Schindelin]

Hi,

On Mon, 21 Jan 2008, Paolo Bonzini wrote:

> This means that: 1) the commit may be aborted after editing the message
> if there is a problem writing out the tree object (slight disadvantage);

I consider this more than a slight disadvantage.  I regularly take ages 
coming up with a good commit message, because I think that the overall 
time balance is better with me spending more time on the message, but 
every reader spending less time to guess what I meant.

So I would be quite annoyed to edit a message, only to find out that for 
whatever reason the commit was not successful.

Are you _sure_ you need 3/4 for 4/4?

Ciao,
Dscho

Re: [PATCH 3/4] git-commit: Refactor creation of log message.

[Paolo Bonzini]

Johannes Schindelin wrote:
> Hi,
> 
> On Mon, 21 Jan 2008, Paolo Bonzini wrote:
> 
>> This means that: 1) the commit may be aborted after editing the message
>> if there is a problem writing out the tree object (slight disadvantage);
> 
> I consider this more than a slight disadvantage.  I regularly take ages 
> coming up with a good commit message, because I think that the overall 
> time balance is better with me spending more time on the message, but 
> every reader spending less time to guess what I meant.
> 
> So I would be quite annoyed to edit a message, only to find out that for 
> whatever reason the commit was not successful.

Just to make it clearer, the piece of code that would have to fail, for 
the behavior to change, is this:

         discard_cache();
         read_cache_from(index_file);
         if (!active_cache_tree)
                 active_cache_tree = cache_tree();
         if (cache_tree_update(active_cache_tree,
                               active_cache, active_nr, 0, 0) < 0) {
                 rollback_index_files();
                 die("Error building trees");
         }

There are a couple of failure modes hidden in update_one (in cache-tree.c):

         sub = find_subtree(it, path + baselen, entlen, 0);
         if (!sub)
                 die("cache-tree.c: '%.*s' in '%s' not found",
                     entlen, path + baselen, path);

         ...

         if (mode != S_IFGITLINK && !missing_ok && !has_sha1_file(sha1))
                 return error("invalid object %s", sha1_to_hex(sha1));

but I don't really understand how they could happen.  If you do, I 
appreciate being taught.  :-)

Also note that some problems writing the tree object (I cannot think of 
anything but running out of diskspace -- permission errors could be 
caught creating the index and logmessage too) could also happen writing 
the commit object.  I don't think in practice the disadvantage I 
mentioned can happen.

> Are you _sure_ you need 3/4 for 4/4?

Probably no, but it makes it much easier to avoid duplicated code (all 
the cases are already enumerated in prepare_log_message).  I tried it 
first and did not finish because the code was really really ugly.

Thanks for the review.

Paolo

Re: [PATCH 3/4] git-commit: Refactor creation of log message.

[Junio C Hamano]

Paolo Bonzini <bonzini@gnu.org> writes:

> Johannes Schindelin wrote:
>>
>> On Mon, 21 Jan 2008, Paolo Bonzini wrote:
>>
>>> This means that: 1) the commit may be aborted after editing the message
>>> if there is a problem writing out the tree object (slight disadvantage);
>>
>> I consider this more than a slight disadvantage.  I regularly take
>> ages coming up with a good commit message, because I think that the
>> overall time balance is better with me spending more time on the
>> message, but every reader spending less time to guess what I meant.
>>
>> So I would be quite annoyed to edit a message, only to find out that
>> for whatever reason the commit was not successful.
>
> Just to make it clearer, the piece of code that would have to fail,
> for the behavior to change, is this:

I suspect Dscho was worried about the case where he says "git
commit", types message and then write-tree finds out that the
index is still unmerged and the tree cannot be written out.

And I'd be majorly annoyed if the "slight disadvange" was about
that.

>         discard_cache();
>         read_cache_from(index_file);
>         if (!active_cache_tree)
>                 active_cache_tree = cache_tree();
>         if (cache_tree_update(active_cache_tree,
>                               active_cache, active_nr, 0, 0) < 0) {
>                 rollback_index_files();
>                 die("Error building trees");
>         }

I think this _could_ error out if your index is unmerged.

However, if you have other code to error out early upon unmeregd
index before you collect the message from the editor, I think
you are Ok.

Re: [PATCH 4/4] git-commit: add a prepare-commit-msg hook

[Junio C Hamano]

Paolo Bonzini <bonzini@gnu.org> writes:

> It takes two parameters.  The first is the source of the commit
> message, and can be: `message` (if a `\-m` or `\-F` option was
> given); `template` (if a `\-t` option was given or the
> configuration option `commit.template` is set); `merge` (if the
> commit is a merge or a `.git/MERGE_MSG file exists); `squash`
> (if a `.git/SQUASH_MSG file exists); or a commit id (if a
> `\-c`, `\-C` or `\--amend` option was given).  The second
> parameter if the name of the file that the commit log message.

Please do without funny mark-ups.  The commit log is not
AsciiDoc.

> The hook is not suppressed by the `\--no-verify` option.  However,
> exiting with non-zero status only aborts the commit if said option
> is not given to `git-commit`.

I do not understand why.  I do understand that you do not want
to bypass prepare-commit-msg with or without --no-verify,
because this hook is not about input validation but about input
preparation.  But I do not understand why a failure exit from it
should be treated any differently with or without --no-verify?

If you want to be strict and be safe catching a breakage in the
prepare-commit-msg hook, you should always abort.  You could
also choose to ignore.  In either case, shouldn't the validation
be left to pre-commit hook (for the tree) and commit-msg hook
(for the message, that is left after this hook)?

> While the default hook just adds a Signed-Off-By line at the bottom

It's "s/[OB]/ob/;", and ...

> of the commit messsage, the hook is more intended to build a template
> for the commit message following project standards, that the user
> can then edit or discard altogether.
>
> Signed-off-by: Paolo Bonzini <bonzini@gnu.org>

... you know it ;-)

Re: [PATCH 1/4] git-commit: support variable number of hook arguments

[Junio C Hamano]

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

> Hi,
>
> On Mon, 21 Jan 2008, Paolo Bonzini wrote:
>
>> +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 {
>
> Here, an
>
> 		if (++i >= ARRAY_SIZE(argv))
> 			die ("run_hook(): too many arguments");
>
> is missing.  Even nicer, you could have
>
> 	int argc = 1;
> 	char **argv = malloc(sizeof(*argv) * 2);
>
> and
>
> 		if (++i >= argc)
> 			argc = ALLOC_GROW(argv, i + 1, argc);

The sanity check to make sure we do not feed too many is
definitely needed.  For this application I think ALLOC_GROW() is
overkill, as we do not pass unbound number of arguments to the
hook.

Re: [PATCH 3/4] git-commit: Refactor creation of log message.

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> I suspect Dscho was worried about the case where he says "git
> commit", types message and then write-tree finds out that the
> index is still unmerged and the tree cannot be written out.
>
> And I'd be majorly annoyed if the "slight disadvange" was about
> that.
>
>>         discard_cache();
>>         read_cache_from(index_file);
>>         if (!active_cache_tree)
>>                 active_cache_tree = cache_tree();
>>         if (cache_tree_update(active_cache_tree,
>>                               active_cache, active_nr, 0, 0) < 0) {
>>                 rollback_index_files();
>>                 die("Error building trees");
>>         }
>
> I think this _could_ error out if your index is unmerged.

I just tried.  This would annoy me.

It is common to do something like this:

	$ git cherry-pick -n somethingelse
        $ git add this-and-that-path.c
        $ git commit

to get partial changes from somethingelse for paths you only
care about, and commit the result.  And you often get conflicts
to paths that do not matter (think of backporting trivial part
of fixes).  You need to reset the paths you do not care about
that conflicted, but you can forget that before running "git
commit".

With our "git commit", you first get "foo: unmerged" and you do
not see the editor.  With this change, you edit the message and
then see "foo: ummerged".

Re: [PATCH 3/4] git-commit: Refactor creation of log message.

[Paolo Bonzini]

>> I think this _could_ error out if your index is unmerged.
> 
> I just tried.  This would annoy me.

Right, you beat me to it.  It would annoy me too.

Paolo