[PATCH 0/3] builtin-commit fixes

[PATCH 0/3] builtin-commit fixes

[Wincent Colaiuta]

A little series to bring built-in commit back in line with the
behaviour as implemented by git-commit.sh and described in the
documentation.

1/3 Allow --no-verify to bypass commit-msg hook

- git-commit.sh used to do this, but builtin-commit wasn't.

2/3 Documentation: fix --no-verify documentation for "git commit"

- The "git commit" man page should mention the commit-msg hook to
bring it in line with what's in the hook notes and in
Documentation/hooks.txt.

3/3 Fix commit-msg hook to allow editing

- Again, git-commit.sh allowed this, but builtin-commit wasn't
doing so.

I have manually tested these changes and they seem to work, but
the fact that the test suite didn't break when the behaviour of
"git commit" changed is indicative of a hole in the suite. I am
not very familiar yet with the test machinery, but I am going
to see if I can whip something up.

Cheers,
Wincent

[PATCH 1/3] Allow --no-verify to bypass commit-msg hook

[Wincent Colaiuta]

At the moment the --no-verify switch to "git commit" instructs it to
skip over the pre-commit hook. Here we teach "git commit --no-verify"
to skip over the commit-msg hook as well. This brings the behaviour
of builtin-commit back in line with git-commit.sh.

Signed-off-by: Wincent Colaiuta <win@wincent.com>
---
 builtin-commit.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/builtin-commit.c b/builtin-commit.c
index 2ec8223..df9377e 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -791,7 +791,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		rollback_index_files();
 		die("could not read commit message");
 	}
-	if (run_hook(index_file, "commit-msg", git_path(commit_editmsg))) {
+	if (!no_verify &&
+	    run_hook(index_file, "commit-msg", git_path(commit_editmsg))) {
 		rollback_index_files();
 		exit(1);
 	}
-- 
1.5.3.7.1115.gaa595

[PATCH 2/3] Documentation: fix --no-verify documentation for "git commit"

[Wincent Colaiuta]

The documentation for the --no-verify switch should mention the
commit-msg hook, not just the pre-commit hook.

Signed-off-by: Wincent Colaiuta <win@wincent.com>
---
 Documentation/git-commit.txt |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index 4bb2791..4261384 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt
@@ -86,7 +86,7 @@ OPTIONS
 	Add Signed-off-by line at the end of the commit message.
 
 --no-verify::
-	This option bypasses the pre-commit hook.
+	This option bypasses the pre-commit and commit-msg hooks.
 	See also link:hooks.html[hooks].
 
 --allow-empty::
-- 
1.5.3.7.1115.gaa595

[PATCH 3/3] Fix commit-msg hook to allow editing

[Wincent Colaiuta]

The old git-commit.sh script allowed the commit-msg hook to not only
prevent a commit from proceding, but also to edit the commit message
on the fly and allow it to proceed. So here we teach builtin-commit
to do the same.

Signed-off-by: Wincent Colaiuta <win@wincent.com>
---
 builtin-commit.c |   16 ++++++++++------
 1 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/builtin-commit.c b/builtin-commit.c
index df9377e..a6223d2 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -787,14 +787,18 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		const char *env[2] = { index, NULL };
 		snprintf(index, sizeof(index), "GIT_INDEX_FILE=%s", index_file);
 		launch_editor(git_path(commit_editmsg), &sb, env);
-	} else if (strbuf_read_file(&sb, git_path(commit_editmsg), 0) < 0) {
-		rollback_index_files();
-		die("could not read commit message");
 	}
-	if (!no_verify &&
-	    run_hook(index_file, "commit-msg", git_path(commit_editmsg))) {
+	if (!no_verify) {
+		if (run_hook(index_file, "commit-msg", git_path(commit_editmsg))) {
+			rollback_index_files();
+			exit(1);
+		}
+		strbuf_setlen(&sb, header_len);
+	}
+	if ((no_edit || !no_verify) &&
+	    strbuf_read_file(&sb, git_path(commit_editmsg), 0) < 0) {
 		rollback_index_files();
-		exit(1);
+		die("could not read commit message");
 	}
 
 	/* Truncate the message just before the diff, if any. */
-- 
1.5.3.7.1115.gaa595

[PATCH 4/4] Add tests for pre-commit and commit-msg hooks

[Wincent Colaiuta]

As desired, these pass for git-commit.sh, fail for builtin-commit (prior
to the fixes), and succeeded for builtin-commit (after the fixes).

Signed-off-by: Wincent Colaiuta <win@wincent.com>
---
 t/t7501-commit.sh          |    2 +-
 t/t7503-pre-commit-hook.sh |   64 ++++++++++++++++++++++++++++++++
 t/t7504-commit-msg-hook.sh |   88 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 153 insertions(+), 1 deletions(-)
 create mode 100755 t/t7503-pre-commit-hook.sh
 create mode 100755 t/t7504-commit-msg-hook.sh

diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
index 19c4b2c..05aa97d 100755
--- a/t/t7501-commit.sh
+++ b/t/t7501-commit.sh
@@ -4,7 +4,7 @@
 #
 
 # FIXME: Test the various index usages, -i and -o, test reflog,
-# signoff, hooks
+# signoff
 
 test_description='git-commit'
 . ./test-lib.sh
diff --git a/t/t7503-pre-commit-hook.sh b/t/t7503-pre-commit-hook.sh
new file mode 100755
index 0000000..c8097a7
--- /dev/null
+++ b/t/t7503-pre-commit-hook.sh
@@ -0,0 +1,64 @@
+#!/bin/sh
+
+test_description='pre-commit hook'
+
+. ./test-lib.sh
+
+test_expect_success "with no hook" \
+	"echo 'foo' > file &&
+	 git add file &&
+	 git commit -m 'first'"
+
+test_expect_success "--no-verify with no hook" \
+	"echo 'bar' > file &&
+	 git add file &&
+	 git commit --no-verify -m 'bar'"
+
+# now install hook that always succeeds
+HOOKDIR="$(git rev-parse --git-dir)/hooks"
+HOOK="$HOOKDIR/pre-commit"
+mkdir -p "$HOOKDIR"
+cat > "$HOOK" <<EOF
+#!/bin/sh
+exit 0
+EOF
+chmod +x "$HOOK"
+
+test_expect_success "with succeeding hook" \
+	"echo 'more' >> file &&
+	 git add file &&
+	 git commit -m 'more'"
+
+test_expect_success "--no-verify with succeeding hook" \
+	"echo 'even more' >> file &&
+	 git add file &&
+	 git commit --no-verify -m 'even more'"
+
+# now a hook that fails
+cat > "$HOOK" <<EOF
+#!/bin/sh
+exit 1
+EOF
+
+test_expect_failure "with failing hook" \
+	"echo 'another' >> file &&
+	 git add file &&
+	 git commit -m 'another'"
+
+test_expect_success "--no-verify with failing hook" \
+	"echo 'stuff' >> file &&
+	 git add file &&
+	 git commit --no-verify -m 'stuff'"
+
+chmod -x "$HOOK"
+test_expect_success "with non-executable hook" \
+	"echo 'content' >> file &&
+	 git add file &&
+	 git commit -m 'content'"
+
+test_expect_success "--no-verify with non-executable hook" \
+	"echo 'more content' >> file &&
+	 git add file &&
+	 git commit --no-verify -m 'more content'"
+
+test_done
diff --git a/t/t7504-commit-msg-hook.sh b/t/t7504-commit-msg-hook.sh
new file mode 100755
index 0000000..17aad7c
--- /dev/null
+++ b/t/t7504-commit-msg-hook.sh
@@ -0,0 +1,88 @@
+#!/bin/sh
+
+test_description='commit-msg hook'
+
+. ./test-lib.sh
+
+test_expect_success "with no hook" \
+	"echo 'foo' > file &&
+	 git add file &&
+	 git commit -m 'first'"
+
+test_expect_success "--no-verify with no hook" \
+	"echo 'bar' > file &&
+	 git add file &&
+	 git commit --no-verify -m 'bar'"
+
+# now install hook that always succeeds
+HOOKDIR="$(git rev-parse --git-dir)/hooks"
+HOOK="$HOOKDIR/commit-msg"
+mkdir -p "$HOOKDIR"
+cat > "$HOOK" <<EOF
+#!/bin/sh
+exit 0
+EOF
+chmod +x "$HOOK"
+
+test_expect_success "with succeeding hook" \
+	"echo 'more' >> file &&
+	 git add file &&
+	 git commit -m 'more'"
+
+test_expect_success "--no-verify with succeeding hook" \
+	"echo 'even more' >> file &&
+	 git add file &&
+	 git commit --no-verify -m 'even more'"
+
+# now a hook that fails
+cat > "$HOOK" <<EOF
+#!/bin/sh
+exit 1
+EOF
+
+test_expect_failure "with failing hook" \
+	"echo 'another' >> file &&
+	 git add file &&
+	 git commit -m 'another'"
+
+test_expect_success "--no-verify with failing hook" \
+	"echo 'stuff' >> file &&
+	 git add file &&
+	 git commit --no-verify -m 'stuff'"
+
+chmod -x "$HOOK"
+test_expect_success "with non-executable hook" \
+	"echo 'content' >> file &&
+	 git add file &&
+	 git commit -m 'content'"
+
+test_expect_success "--no-verify with non-executable hook" \
+	"echo 'more content' >> file &&
+	 git add file &&
+	 git commit --no-verify -m 'more content'"
+
+# now a hook that edits the commit message
+cat > "$HOOK" <<'EOF'
+#!/bin/sh
+echo "new message" > "$1"
+exit 0
+EOF
+chmod +x "$HOOK"
+
+commit_msg_is () {
+	test "`git log --pretty=format:%s%b -1`" = "$1"
+}
+
+test_expect_success "hook edits commit message" \
+	"echo 'additional' >> file &&
+	 git add file &&
+	 git commit -m 'additional' &&
+	 commit_msg_is 'new message'"
+
+test_expect_success "hook doesn't edit commit message" \
+	"echo 'plus' >> file &&
+	 git add file &&
+	 git commit --no-verify -m 'plus' &&
+	 commit_msg_is 'plus'"
+
+test_done
-- 
1.5.3.7.1115.g3d1c

Re: [PATCH 4/4] Add tests for pre-commit and commit-msg hooks

[Wincent Colaiuta]

El 8/12/2007, a las 13:29, Wincent Colaiuta escribió:

> As desired, these pass for git-commit.sh, fail for builtin-commit  
> (prior
> to the fixes), and succeeded for builtin-commit (after the fixes).

Actually, that's a slight lie.

The commit-msg tests 9 and 10 (editing the commit message from the  
hook) fail for git-commit.sh (tested 1.5.3.4 and 1.5.3.7).

By the looks of it, this discrepancy isn't actually a bug in git- 
commit.sh, but is because of stuff that was in commit.c back in  
1.5.3.7 and prior which causes a commit message like "foo" to yield  
"foo<unknown>" when you do a "git log --pretty=format:%s%b -1"; post  
1.5.3.7 (on master) it instead yields "foo", which is what I was  
testing for.

Cheers,
Wincent

Re: [PATCH 3/3] Fix commit-msg hook to allow editing

[Junio C Hamano]

Wincent Colaiuta <win@wincent.com> writes:

> The old git-commit.sh script allowed the commit-msg hook to not only
> prevent a commit from proceding, but also to edit the commit message
> on the fly and allow it to proceed. So here we teach builtin-commit
> to do the same.

This is a bit unfortunate.

> @@ -787,14 +787,18 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
>  		const char *env[2] = { index, NULL };
>  		snprintf(index, sizeof(index), "GIT_INDEX_FILE=%s", index_file);
>  		launch_editor(git_path(commit_editmsg), &sb, env);

Here, launch-editor lets the user edit an external file, and reads it
back into &sb, but ...

>  	}
> +	if (!no_verify) {
> +		if (run_hook(index_file, "commit-msg", git_path(commit_editmsg))) {
> +			rollback_index_files();
> +			exit(1);
> +		}
> +		strbuf_setlen(&sb, header_len);
> +	}

we need to discard it (I would prefer setlen to be done before running
the hook, just to make the logic flow more natural), because we need to
read it back again anyway.  On the other hand, if we do not start the
editor, we used to read the contents before running the hook, which was
wrong, and the call to read the file is moved after, like this:

> +	if ((no_edit || !no_verify) &&
> +	    strbuf_read_file(&sb, git_path(commit_editmsg), 0) < 0) {
>  		rollback_index_files();
> +		die("could not read commit message");
>  	}

which makes the logic to decide if we read the file back again
unnecessarily convoluted.  The reason why (no_edit || !no_verify) is
there is very closely tied to the fact that you happened to have already
read in the launch_editor() codepath but not yet in no_edit codepath.
This feels very error prone.

	Side note. an unrelated reason where this convolution comes from
	is the unfortunate naming of many options and double-negations
	they bring in.  A normal human being needs to read expressions
	like "if (!no_foo || no_foo)" three times before understanding
	what is being checked.

I would suggest doing the above differently.

 * Allow launch_editor() not to read back the edited result into strbuf
   at all (perhaps pass NULL to signal that), and make "if (!no_edit)"
   codepath use it;

 * The codeflow would become:

	if (!no_edit) {
		launch editor, in "no readback" mode;
	}
	if (!no_verify) {
        	run hook, let it interfere;
	}
	read the file to &sb, no matter what no_edit/no_verify says.

IOW, how about something like this?

---

 builtin-commit.c |    9 +++++----
 builtin-tag.c    |    2 ++
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/builtin-commit.c b/builtin-commit.c
index 2032ca3..30a9deb 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -787,16 +787,17 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		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), &sb, env);
-	} else if (strbuf_read_file(&sb, git_path(commit_editmsg), 0) < 0) {
-		rollback_index_files();
-		die("could not read commit message");
+		launch_editor(git_path(commit_editmsg), NULL, env);
 	}
 	if (!no_verify &&
 	    run_hook(index_file, "commit-msg", git_path(commit_editmsg))) {
 		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");
+	}
 
 	/* Truncate the message just before the diff, if any. */
 	p = strstr(sb.buf, "\ndiff --git a/");
diff --git a/builtin-tag.c b/builtin-tag.c
index 729389b..9f966fc 100644
--- a/builtin-tag.c
+++ b/builtin-tag.c
@@ -53,6 +53,8 @@ void launch_editor(const char *path, struct strbuf *buffer, const char *const *e
 			die("There was a problem with the editor %s.", editor);
 	}
 
+	if (!buffer)
+		return;
 	if (strbuf_read_file(buffer, path, 0) < 0)
 		die("could not read message file '%s': %s",
 		    path, strerror(errno));


                

Re: [PATCH 3/3] Fix commit-msg hook to allow editing

[Wincent Colaiuta]

El 9/12/2007, a las 8:21, Junio C Hamano escribió:

> IOW, how about something like this?
>
> ---
>
> builtin-commit.c |    9 +++++----
> builtin-tag.c    |    2 ++
> 2 files changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/builtin-commit.c b/builtin-commit.c
> index 2032ca3..30a9deb 100644
> --- a/builtin-commit.c
> +++ b/builtin-commit.c
> @@ -787,16 +787,17 @@ int cmd_commit(int argc, const char **argv,  
> const char *prefix)
> 		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), &sb, env);
> -	} else if (strbuf_read_file(&sb, git_path(commit_editmsg), 0) < 0) {
> -		rollback_index_files();
> -		die("could not read commit message");
> +		launch_editor(git_path(commit_editmsg), NULL, env);
> 	}
> 	if (!no_verify &&
> 	    run_hook(index_file, "commit-msg", git_path(commit_editmsg))) {
> 		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");
> +	}
>
> 	/* Truncate the message just before the diff, if any. */
> 	p = strstr(sb.buf, "\ndiff --git a/");
> diff --git a/builtin-tag.c b/builtin-tag.c
> index 729389b..9f966fc 100644
> --- a/builtin-tag.c
> +++ b/builtin-tag.c
> @@ -53,6 +53,8 @@ void launch_editor(const char *path, struct strbuf  
> *buffer, const char *const *e
> 			die("There was a problem with the editor %s.", editor);
> 	}
>
> +	if (!buffer)
> +		return;
> 	if (strbuf_read_file(buffer, path, 0) < 0)
> 		die("could not read message file '%s': %s",
> 		    path, strerror(errno));

Excellent. Reads much better. I hadn't considered changing the strbuf  
API, but looking at the callers of launch_editor this is a safe  
change. Ack.

Cheers,
Wincent