[PATCH 0/3] pre-merge-hook

[PATCH 0/3] pre-merge-hook

[Michael J Gruber]

The pre-commit hook is often used to ensure certain properties of each
comitted tree like formatting or coding standards, validity (lint/make)
or code quality (make test). But merges introduce new commits unless
they are fast forwards, and therefore they can break these properties
because the pre-commit hook is not run by "git merge".

Introduce a pre-merge hook which works for (non ff, automatic) merges
like pre-commit does for commits. Typically this will just call the
pre-commit hook (like in the sample hook), but it does not need to.

Michael J Gruber (3):
  git-merge: Honor pre-merge hook
  merge: --no-verify to bypass pre-merge hook
  t7503: add tests for pre-merge-hook

 Documentation/git-merge.txt       |  2 +-
 Documentation/githooks.txt        |  7 +++++
 Documentation/merge-options.txt   |  4 +++
 builtin/merge.c                   | 15 ++++++++-
 t/t7503-pre-commit-hook.sh        | 66 ++++++++++++++++++++++++++++++++++++++-
 templates/hooks--pre-merge.sample | 13 ++++++++
 6 files changed, 104 insertions(+), 3 deletions(-)
 create mode 100755 templates/hooks--pre-merge.sample

-- 
1.7.12.406.gafd3f81

[PATCH 1/3] git-merge: Honor pre-merge hook

[Michael J Gruber]

git-merge does not honor the pre-commit hook when doing automatic merge
commits, and for compatibility reasons this is going to stay.

Introduce a pre-merge hook which is called for an automatic merge commit
just like pre-commit is called for a non-automatic merge commit (or any
other commit).

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 Documentation/githooks.txt        |  7 +++++++
 builtin/merge.c                   | 13 ++++++++++++-
 templates/hooks--pre-merge.sample | 13 +++++++++++++
 3 files changed, 32 insertions(+), 1 deletion(-)
 create mode 100755 templates/hooks--pre-merge.sample

diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index b9003fe..3fae643 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -86,6 +86,13 @@ 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.
 
+pre-merge
+~~~~~~~~~
+
+This hook is invoked by 'git merge' when doing an automatic merge
+commit; it is equivalent to 'pre-commit' for a non-automatic commit
+for a merge.
+
 prepare-commit-msg
 ~~~~~~~~~~~~~~~~~~
 
diff --git a/builtin/merge.c b/builtin/merge.c
index 0ec8f0d..7c09e0b 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -898,12 +898,23 @@ static void prepare_to_commit(struct commit_list *remoteheads)
 {
 	struct strbuf msg = STRBUF_INIT;
 	const char *comment = _(merge_editor_comment);
+	const char *index_file = get_index_file();
+
+	if (run_hook(index_file, "pre-merge", NULL))
+		abort_commit(remoteheads, NULL);
+	/*
+	 * Re-read the index as pre-merge hook could have updated it,
+	 * and write it out as a tree.  We must do this before we invoke
+	 * the editor and after we invoke run_status above.
+	 */
+	discard_cache();
+	read_cache_from(index_file);
 	strbuf_addbuf(&msg, &merge_msg);
 	strbuf_addch(&msg, '\n');
 	if (0 < option_edit)
 		strbuf_add_lines(&msg, "# ", comment, strlen(comment));
 	write_merge_msg(&msg);
-	run_hook(get_index_file(), "prepare-commit-msg",
+	run_hook(index_file, "prepare-commit-msg",
 		 git_path("MERGE_MSG"), "merge", NULL, NULL);
 	if (0 < option_edit) {
 		if (launch_editor(git_path("MERGE_MSG"), NULL, NULL))
diff --git a/templates/hooks--pre-merge.sample b/templates/hooks--pre-merge.sample
new file mode 100755
index 0000000..a6313e6
--- /dev/null
+++ b/templates/hooks--pre-merge.sample
@@ -0,0 +1,13 @@
+#!/bin/sh
+#
+# An example hook script to verify what is about to be committed.
+# Called by "git merge" with no arguments.  The hook should
+# exit with non-zero status after issuing an appropriate message if
+# it wants to stop the commit.
+#
+# To enable this hook, rename this file to "pre-merge".
+
+. git-sh-setup
+test -x "$GIT_DIR/hooks/pre-commit" &&
+        exec "$GIT_DIR/hooks/pre-commit"
+:
-- 
1.7.12.406.gafd3f81

[PATCH 2/3] merge: --no-verify to bypass pre-merge hook

[Michael J Gruber]

Analogous to commit, introduce a '--no-verify' option which bypasses the
pre-merge hook. The shorthand '-n' is taken by the (non-existing)
'--no-stat' already.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 Documentation/git-merge.txt     | 2 +-
 Documentation/githooks.txt      | 2 +-
 Documentation/merge-options.txt | 4 ++++
 builtin/merge.c                 | 4 +++-
 4 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
index 20f9228..8816865 100644
--- a/Documentation/git-merge.txt
+++ b/Documentation/git-merge.txt
@@ -10,7 +10,7 @@ SYNOPSIS
 --------
 [verse]
 'git merge' [-n] [--stat] [--no-commit] [--squash] [--[no-]edit]
-	[-s <strategy>] [-X <strategy-option>]
+	[--no-verify] [-s <strategy>] [-X <strategy-option>]
 	[--[no-]rerere-autoupdate] [-m <msg>] [<commit>...]
 'git merge' <msg> HEAD <commit>...
 'git merge' --abort
diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index 3fae643..e20bfe0 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -91,7 +91,7 @@ pre-merge
 
 This hook is invoked by 'git merge' when doing an automatic merge
 commit; it is equivalent to 'pre-commit' for a non-automatic commit
-for a merge.
+for a merge, and can be bypassed with the `\--no-verify` option. 
 
 prepare-commit-msg
 ~~~~~~~~~~~~~~~~~~
diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
index 0bcbe0a..93e2e61 100644
--- a/Documentation/merge-options.txt
+++ b/Documentation/merge-options.txt
@@ -70,6 +70,10 @@ merge.
 With --no-squash perform the merge and commit the result. This
 option can be used to override --squash.
 
+--no-verify::
+	This option bypasses the pre-merge and commit-msg hooks.
+	See also linkgit:githooks[5].
+
 -s <strategy>::
 --strategy=<strategy>::
 	Use the given merge strategy; can be supplied more than
diff --git a/builtin/merge.c b/builtin/merge.c
index 7c09e0b..4d36a00 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -60,6 +60,7 @@ static const char *branch;
 static char *branch_mergeoptions;
 static int option_renormalize;
 static int verbosity;
+static int no_verify;
 static int allow_rerere_auto;
 static int abort_current_merge;
 static int show_progress = -1;
@@ -198,6 +199,7 @@ static struct option builtin_merge_options[] = {
 		N_("allow fast-forward (default)")),
 	OPT_BOOLEAN(0, "ff-only", &fast_forward_only,
 		N_("abort if fast-forward is not possible")),
+	OPT_BOOLEAN(0, "no-verify", &no_verify, "bypass pre-merge hook"),
 	OPT_RERERE_AUTOUPDATE(&allow_rerere_auto),
 	OPT_CALLBACK('s', "strategy", &use_strategies, N_("strategy"),
 		N_("merge strategy to use"), option_parse_strategy),
@@ -900,7 +902,7 @@ static void prepare_to_commit(struct commit_list *remoteheads)
 	const char *comment = _(merge_editor_comment);
 	const char *index_file = get_index_file();
 
-	if (run_hook(index_file, "pre-merge", NULL))
+	if (!no_verify && run_hook(index_file, "pre-merge", NULL))
 		abort_commit(remoteheads, NULL);
 	/*
 	 * Re-read the index as pre-merge hook could have updated it,
-- 
1.7.12.406.gafd3f81

[PATCH 3/3] t7503: add tests for pre-merge-hook

[Michael J Gruber]

Add tests which make sure that the pre-merge-hook is called when
present, allows/disallows merge commits depending on its return value
and is suppressed by "--no-verify".

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 t/t7503-pre-commit-hook.sh | 66 +++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 65 insertions(+), 1 deletion(-)

diff --git a/t/t7503-pre-commit-hook.sh b/t/t7503-pre-commit-hook.sh
index 984889b..36ae87f 100755
--- a/t/t7503-pre-commit-hook.sh
+++ b/t/t7503-pre-commit-hook.sh
@@ -1,9 +1,22 @@
 #!/bin/sh
 
-test_description='pre-commit hook'
+test_description='pre-commit and pre-merge hooks'
 
 . ./test-lib.sh
 
+test_expect_success 'root commit' '
+
+	echo "root" > file &&
+	git add file &&
+	git commit -m "zeroth" &&
+	git checkout -b side &&
+	echo "foo" > foo &&
+	git add foo &&
+	git commit -m "make it non-ff" &&
+	git checkout master
+
+'
+
 test_expect_success 'with no hook' '
 
 	echo "foo" > file &&
@@ -12,6 +25,14 @@ test_expect_success 'with no hook' '
 
 '
 
+test_expect_success 'with no hook (merge)' '
+
+	git checkout side &&
+	git merge -m "merge master" master &&
+	git checkout master
+
+'
+
 test_expect_success '--no-verify with no hook' '
 
 	echo "bar" > file &&
@@ -20,15 +41,25 @@ test_expect_success '--no-verify with no hook' '
 
 '
 
+test_expect_success '--no-verify with no hook (merge)' '
+
+	git checkout side &&
+	git merge --no-verify -m "merge master" master &&
+	git checkout master
+
+'
+
 # now install hook that always succeeds
 HOOKDIR="$(git rev-parse --git-dir)/hooks"
 HOOK="$HOOKDIR/pre-commit"
+MERGEHOOK="$HOOKDIR/pre-merge"
 mkdir -p "$HOOKDIR"
 cat > "$HOOK" <<EOF
 #!/bin/sh
 exit 0
 EOF
 chmod +x "$HOOK"
+cp -p "$HOOK" "$MERGEHOOK"
 
 test_expect_success 'with succeeding hook' '
 
@@ -38,6 +69,14 @@ test_expect_success 'with succeeding hook' '
 
 '
 
+test_expect_success 'with succeeding hook (merge)' '
+
+	git checkout side &&
+	git merge -m "merge master" master &&
+	git checkout master
+
+'
+
 test_expect_success '--no-verify with succeeding hook' '
 
 	echo "even more" >> file &&
@@ -46,11 +85,20 @@ test_expect_success '--no-verify with succeeding hook' '
 
 '
 
+test_expect_success '--no-verify with succeeding hook (merge)' '
+
+	git checkout side &&
+	git merge --no-verify -m "merge master" master &&
+	git checkout master
+
+'
+
 # now a hook that fails
 cat > "$HOOK" <<EOF
 #!/bin/sh
 exit 1
 EOF
+cp -p "$HOOK" "$MERGEHOOK"
 
 test_expect_success 'with failing hook' '
 
@@ -68,6 +116,22 @@ test_expect_success '--no-verify with failing hook' '
 
 '
 
+test_expect_success 'with failing hook (merge)' '
+
+	git checkout side &&
+	test_must_fail git merge -m "merge master" master &&
+	git checkout master
+
+'
+
+test_expect_success '--no-verify with failing hook (merge)' '
+
+	git checkout side &&
+	git merge --no-verify -m "merge master" master &&
+	git checkout master
+
+'
+
 chmod -x "$HOOK"
 test_expect_success POSIXPERM 'with non-executable hook' '
 
-- 
1.7.12.406.gafd3f81

Re: [PATCH 1/3] git-merge: Honor pre-merge hook

[Michael Haggerty]

On 09/05/2012 03:39 PM, Michael J Gruber wrote:
> git-merge does not honor the pre-commit hook when doing automatic merge
> commits, and for compatibility reasons this is going to stay.
> 
> Introduce a pre-merge hook which is called for an automatic merge commit
> just like pre-commit is called for a non-automatic merge commit (or any
> other commit).

What exactly is an "automatic merge commit"?  Is it any merge that
doesn't have a conflict?  A merge that doesn't invoke the editor?  A
merge done as part of another operation (e.g., pull)?  I don't see the
term mentioned in the git-merge or githooks man pages.

I think it would be good if you would define this term in the
documentation files that your patch touched, and perhaps in the githooks
section about "pre-commit" as well.

Secondly, though it is impossible (for backwards compatibility reasons)
for the pre-commit hook to be invoked for automatic merges, no such
considerations prohibit the pre-merge commit from being invoked for
non-automatic merges.  In other words, both hooks, pre-commit *and*
pre-merge, could be invoked for non-automatic merges.  Would this be
preferable?

It depends on what pre-merge scripts are likely to be used for.  If they
will tend to be used for merge-specific actions, then it might be more
convenient for *all* merges to be vetted by them.  On the other hand, if
they tend to do the same actions as pre-commit hooks, then having
non-automatic merge commits go through both hooks would tend to be more
annoying than helpful.  Specifically, one of the scripts would probably
have to check whether the merge is a non-automatic merge, and if so do
nothing (i.e., letting the other script take care of it).  This would
also require an easy way for a script to determine whether a commit is a
non-automatic merge commit.

Have you considered this?

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

Re: [PATCH 0/3] pre-merge-hook

[Junio C Hamano]

Michael J Gruber <git@drmicha.warpmail.net> writes:

> The pre-commit hook is often used to ensure certain properties of each
> comitted tree like formatting or coding standards, validity (lint/make)
> or code quality (make test). But merges introduce new commits unless
> they are fast forwards, and therefore they can break these properties
> because the pre-commit hook is not run by "git merge".
>
> Introduce a pre-merge hook which works for (non ff, automatic) merges
> like pre-commit does for commits. Typically this will just call the
> pre-commit hook (like in the sample hook), but it does not need to.

When your merge asks for a help from you to resolve conflict, you
conclude with "git commit", and at that point, pre-commit hook will
have a chance to reject it, presumably.  That means for any project
that wants to audit a merge via hook, their pre-commit hook MUST be
prepared to look at and judge a merge.  Given that, is a separate
hook that "can just call the pre-commit but does not need to" really
needed and useful?

I admit that I haven't thought things through, but adding a boolean
"merge.usePreCommitHook" smells like a more appropriate approach to
me.

I dunno.

Re: [PATCH 0/3] pre-merge-hook

[Michael J Gruber]

Junio C Hamano venit, vidit, dixit 06.09.2012 07:07:
> Michael J Gruber <git@drmicha.warpmail.net> writes:
> 
>> The pre-commit hook is often used to ensure certain properties of each
>> comitted tree like formatting or coding standards, validity (lint/make)
>> or code quality (make test). But merges introduce new commits unless
>> they are fast forwards, and therefore they can break these properties
>> because the pre-commit hook is not run by "git merge".
>>
>> Introduce a pre-merge hook which works for (non ff, automatic) merges
>> like pre-commit does for commits. Typically this will just call the
>> pre-commit hook (like in the sample hook), but it does not need to.
> 
> When your merge asks for a help from you to resolve conflict, you
> conclude with "git commit", and at that point, pre-commit hook will
> have a chance to reject it, presumably.  That means for any project
> that wants to audit a merge via hook, their pre-commit hook MUST be
> prepared to look at and judge a merge.  Given that, is a separate
> hook that "can just call the pre-commit but does not need to" really
> needed and useful?
> 
> I admit that I haven't thought things through, but adding a boolean
> "merge.usePreCommitHook" smells like a more appropriate approach to
> me.
> 
> I dunno.

That would be an option ;)

Seriously, that would make the case where both hooks are the same even
simpler, obviously. On the other hand it would make the other case more
difficult. But, really, since non-automatic merges call pre-commit now,
and that is probably to stay, catching those with a different (part of
the pre-commit) hook will stay difficult anyways. So the question is
really more whether we prefer to bloat the config space or the name
space by this.

Either works for me, and if we don't change the current behaviour
(pre-commit-hook resp. no hook for non-automatic merges resp. automatic
merges) the config option is probably less confusing.

Michael

Re: [PATCH 1/3] git-merge: Honor pre-merge hook

[Michael J Gruber]

Michael Haggerty venit, vidit, dixit 05.09.2012 17:30:
> On 09/05/2012 03:39 PM, Michael J Gruber wrote:
>> git-merge does not honor the pre-commit hook when doing automatic merge
>> commits, and for compatibility reasons this is going to stay.
>>
>> Introduce a pre-merge hook which is called for an automatic merge commit
>> just like pre-commit is called for a non-automatic merge commit (or any
>> other commit).
> 
> What exactly is an "automatic merge commit"?  Is it any merge that
> doesn't have a conflict?  A merge that doesn't invoke the editor?  A
> merge done as part of another operation (e.g., pull)?  I don't see the
> term mentioned in the git-merge or githooks man pages.
> 
> I think it would be good if you would define this term in the
> documentation files that your patch touched, and perhaps in the githooks
> section about "pre-commit" as well.

"git merge" can go three ways:

F: fast forward: no commit is created, only a ref is changed
A: automatic: true merge (non-ff) without conflicts (i.e. chosen
strategy can perform the merge); a new commit is created
C: merge with conflicts: no commit is created but the index is prepared
(partially) for a merge commit

In case F, no commit hook is run (talking only about pre-commit/pre-merge).

In case A, no commit is run so far but my patch proposes pre-merge to be
run.

In case C, pre-commit (!) is run so far and after my patch.

> Secondly, though it is impossible (for backwards compatibility reasons)
> for the pre-commit hook to be invoked for automatic merges, no such
> considerations prohibit the pre-merge commit from being invoked for
> non-automatic merges.  In other words, both hooks, pre-commit *and*
> pre-merge, could be invoked for non-automatic merges.  Would this be
> preferable?
>
> It depends on what pre-merge scripts are likely to be used for.  If they
> will tend to be used for merge-specific actions, then it might be more
> convenient for *all* merges to be vetted by them.  On the other hand, if
> they tend to do the same actions as pre-commit hooks, then having
> non-automatic merge commits go through both hooks would tend to be more
> annoying than helpful.  Specifically, one of the scripts would probably
> have to check whether the merge is a non-automatic merge, and if so do
> nothing (i.e., letting the other script take care of it).  This would
> also require an easy way for a script to determine whether a commit is a
> non-automatic merge commit.
> 
> Have you considered this?

Your second paragraph explains why I did it the way I did. One can
easily have pre-merge call pre-commit, or have them be different. One
can not easily have only pre-merge called for a non-automatic merge
commit, but that is because of backward compatibility. The way *I* would
like it is:

- call pre-merge for any non-ff merge commit (automatic or not)
- call pre-commit for any non-merge commit (#parents <=1)

But that would break compatibility.

So I hope my patch is the best approximation to the above which keeps
compatibility and is simple to handle in most situations.

Cheers
Michael

Re: [PATCH 1/3] git-merge: Honor pre-merge hook

[Michael Haggerty]

On 09/06/2012 10:16 AM, Michael J Gruber wrote:
> Michael Haggerty venit, vidit, dixit 05.09.2012 17:30:
>> On 09/05/2012 03:39 PM, Michael J Gruber wrote:
>>> git-merge does not honor the pre-commit hook when doing automatic merge
>>> commits, and for compatibility reasons this is going to stay.
>>>
>>> Introduce a pre-merge hook which is called for an automatic merge commit
>>> just like pre-commit is called for a non-automatic merge commit (or any
>>> other commit).
>>
>> What exactly is an "automatic merge commit"?  Is it any merge that
>> doesn't have a conflict?  A merge that doesn't invoke the editor?  A
>> merge done as part of another operation (e.g., pull)?  I don't see the
>> term mentioned in the git-merge or githooks man pages.
>>
>> I think it would be good if you would define this term in the
>> documentation files that your patch touched, and perhaps in the githooks
>> section about "pre-commit" as well.
> 
> "git merge" can go three ways:
> 
> F: fast forward: no commit is created, only a ref is changed
> A: automatic: true merge (non-ff) without conflicts (i.e. chosen
> strategy can perform the merge); a new commit is created
> C: merge with conflicts: no commit is created but the index is prepared
> (partially) for a merge commit
> 
> In case F, no commit hook is run (talking only about pre-commit/pre-merge).
> 
> In case A, no commit is run so far but my patch proposes pre-merge to be
> run.
> 
> In case C, pre-commit (!) is run so far and after my patch.

Thanks for the explanation.  I hope you will explain this briefly in the
patch to the docs.

>> Secondly, though it is impossible (for backwards compatibility reasons)
>> for the pre-commit hook to be invoked for automatic merges, no such
>> considerations prohibit the pre-merge commit from being invoked for
>> non-automatic merges.  In other words, both hooks, pre-commit *and*
>> pre-merge, could be invoked for non-automatic merges.  Would this be
>> preferable?
>>
>> It depends on what pre-merge scripts are likely to be used for.  If they
>> will tend to be used for merge-specific actions, then it might be more
>> convenient for *all* merges to be vetted by them.  On the other hand, if
>> they tend to do the same actions as pre-commit hooks, then having
>> non-automatic merge commits go through both hooks would tend to be more
>> annoying than helpful.  Specifically, one of the scripts would probably
>> have to check whether the merge is a non-automatic merge, and if so do
>> nothing (i.e., letting the other script take care of it).  This would
>> also require an easy way for a script to determine whether a commit is a
>> non-automatic merge commit.
>>
>> Have you considered this?
> 
> Your second paragraph explains why I did it the way I did. One can
> easily have pre-merge call pre-commit, or have them be different. One
> can not easily have only pre-merge called for a non-automatic merge
> commit, but that is because of backward compatibility. The way *I* would
> like it is:
> 
> - call pre-merge for any non-ff merge commit (automatic or not)
> - call pre-commit for any non-merge commit (#parents <=1)
> 
> But that would break compatibility.
> 
> So I hope my patch is the best approximation to the above which keeps
> compatibility and is simple to handle in most situations.

I can understand your reasoning and won't object.  But before I shut up,
I will point out a third alternative that is arguably closer to your
"ideal":

- For non-merge commits, call pre-commit
- For automatic merge commits, call pre-merge
- For non-automatic merge commits:
  if pre-merge exists, call pre-merge (only)
  else if pre-commit exists, call pre-commit (for backwards comptibility)

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

[PATCHv2 0/4] pre-commit hook for merges

[Michael J Gruber]

In this second iteration I implement Junio's suggestion: 'git merge' invokes
the pre-commit hook when merge.usePreCommitHook is set to true.

1/4 documents that 'git merge' invokes the prepare-commit-msg hook
unconditionally already.

2-4 are v2 of the previous 1-3.

This leaves aside the issue of commit-msg and post-commit hooks. If we can live
with a bit of incompatibility I would rename the config to merge.useCommitHooks
and call all of them based on that.  This would change the way prepare-commit-msg is
treated now.

We could also introduce 4 config variables, 3 defaulting to false, 1 to true,
of course...

[I had messed up my alias file when adding mhagger, and it seems that tripped up
vger; resending, sorry.]

Michael J Gruber (4):
  merge: document prepare-commit-msg hook usage
  git-merge: Honor pre-commit hook based on config
  merge: --no-verify to bypass pre-commit hook
  t7503: add tests for pre-commit hook (merge)

 Documentation/git-merge.txt     |  7 ++++-
 Documentation/githooks.txt      |  6 ++++
 Documentation/merge-config.txt  |  5 ++++
 Documentation/merge-options.txt |  4 +++
 builtin/merge.c                 | 19 ++++++++++++-
 t/t7503-pre-commit-hook.sh      | 62 +++++++++++++++++++++++++++++++++++++++++
 6 files changed, 101 insertions(+), 2 deletions(-)

-- 
1.7.12.406.gafd3f81

[PATCHv2 1/4] merge: document prepare-commit-msg hook usage

[Michael J Gruber]

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 Documentation/git-merge.txt | 5 +++++
 Documentation/githooks.txt  | 2 ++
 2 files changed, 7 insertions(+)

diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
index 20f9228..b3ba8a8 100644
--- a/Documentation/git-merge.txt
+++ b/Documentation/git-merge.txt
@@ -308,6 +308,11 @@ branch.<name>.mergeoptions::
 	supported options are the same as those of 'git merge', but option
 	values containing whitespace characters are currently not supported.
 
+HOOKS
+-----
+This command can run the `prepare-commit-msg` hook.
+See linkgit:githooks[5] for more information.
+
 SEE ALSO
 --------
 linkgit:git-fmt-merge-msg[1], linkgit:git-pull[1],
diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index b9003fe..b32134c 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -108,6 +108,8 @@ it is not suppressed by the `--no-verify` option.  A non-zero exit
 means a failure of the hook and aborts the commit.  It should not
 be used as replacement for pre-commit hook.
 
+This hook is also called by 'git merge' when it creates a new commit.
+
 The sample `prepare-commit-msg` hook that comes with git comments
 out the `Conflicts:` part of a merge's commit message.
 
-- 
1.7.12.406.gafd3f81

[PATCHv2 2/4] git-merge: Honor pre-commit hook based on config

[Michael J Gruber]

git-merge does not honor the pre-commit hook when doing automatic merge
commits, and for compatibility reasons this is going to stay.

Introduce a merge.usePreCommitHook which controls whether an automatic
merge commit invokes pre-commit.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 Documentation/git-merge.txt    |  2 +-
 Documentation/githooks.txt     |  3 +++
 Documentation/merge-config.txt |  5 +++++
 builtin/merge.c                | 17 ++++++++++++++++-
 4 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
index b3ba8a8..3501ae2 100644
--- a/Documentation/git-merge.txt
+++ b/Documentation/git-merge.txt
@@ -310,7 +310,7 @@ branch.<name>.mergeoptions::
 
 HOOKS
 -----
-This command can run the `prepare-commit-msg` hook.
+This command can run the `prepare-commit-msg` and `pre-commit` hooks.
 See linkgit:githooks[5] for more information.
 
 SEE ALSO
diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index b32134c..d62e02d 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -86,6 +86,9 @@ 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.
 
+If the configuration option `merge.usePreCommitHook` is set to `true`
+then 'git merge' invokes this hook whenever it creates a new commit.
+
 prepare-commit-msg
 ~~~~~~~~~~~~~~~~~~
 
diff --git a/Documentation/merge-config.txt b/Documentation/merge-config.txt
index 861bd6f..727caad 100644
--- a/Documentation/merge-config.txt
+++ b/Documentation/merge-config.txt
@@ -59,6 +59,11 @@ merge.tool::
 	and "xxdiff".  Any other value is treated is custom merge tool
 	and there must be a corresponding mergetool.<tool>.cmd option.
 
+merge.usePreCommitHook::
+	Controls whether the pre-commit hook is invoked when 'git merge'
+	creates a new commit (true merge without conflicts).  False by
+	default.
+
 merge.verbosity::
 	Controls the amount of output shown by the recursive merge
 	strategy.  Level 0 outputs nothing except a final error
diff --git a/builtin/merge.c b/builtin/merge.c
index 0ec8f0d..a2590a9 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -64,6 +64,7 @@ static int allow_rerere_auto;
 static int abort_current_merge;
 static int show_progress = -1;
 static int default_to_upstream;
+static int use_pre_commit_hook;
 static const char *sign_commit;
 
 static struct strategy all_strategy[] = {
@@ -575,6 +576,9 @@ static int git_merge_config(const char *k, const char *v, void *cb)
 	} else if (!strcmp(k, "merge.defaulttoupstream")) {
 		default_to_upstream = git_config_bool(k, v);
 		return 0;
+	} else if (!strcmp(k, "merge.useprecommithook")) {
+		use_pre_commit_hook = git_config_bool(k, v);
+		return 0;
 	}
 
 	status = fmt_merge_msg_config(k, v, cb);
@@ -898,12 +902,23 @@ static void prepare_to_commit(struct commit_list *remoteheads)
 {
 	struct strbuf msg = STRBUF_INIT;
 	const char *comment = _(merge_editor_comment);
+	const char *index_file = get_index_file();
+
+	if (use_pre_commit_hook && run_hook(index_file, "pre-commit", NULL))
+		abort_commit(remoteheads, NULL);
+	/*
+	 * Re-read the index as pre-commit hook could have updated it,
+	 * and write it out as a tree.  We must do this before we invoke
+	 * the editor and after we invoke run_status above.
+	 */
+	discard_cache();
+	read_cache_from(index_file);
 	strbuf_addbuf(&msg, &merge_msg);
 	strbuf_addch(&msg, '\n');
 	if (0 < option_edit)
 		strbuf_add_lines(&msg, "# ", comment, strlen(comment));
 	write_merge_msg(&msg);
-	run_hook(get_index_file(), "prepare-commit-msg",
+	run_hook(index_file, "prepare-commit-msg",
 		 git_path("MERGE_MSG"), "merge", NULL, NULL);
 	if (0 < option_edit) {
 		if (launch_editor(git_path("MERGE_MSG"), NULL, NULL))
-- 
1.7.12.406.gafd3f81

[PATCHv2 3/4] merge: --no-verify to bypass pre-commit hook

[Michael J Gruber]

Analogous to commit, introduce a '--no-verify' option which bypasses the
pre-commit hook. The shorthand '-n' is taken by the (non-existing)
'--no-stat' already.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 Documentation/git-merge.txt     | 2 +-
 Documentation/githooks.txt      | 1 +
 Documentation/merge-options.txt | 4 ++++
 builtin/merge.c                 | 4 +++-
 4 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
index 3501ae2..363fbea 100644
--- a/Documentation/git-merge.txt
+++ b/Documentation/git-merge.txt
@@ -10,7 +10,7 @@ SYNOPSIS
 --------
 [verse]
 'git merge' [-n] [--stat] [--no-commit] [--squash] [--[no-]edit]
-	[-s <strategy>] [-X <strategy-option>]
+	[--no-verify] [-s <strategy>] [-X <strategy-option>]
 	[--[no-]rerere-autoupdate] [-m <msg>] [<commit>...]
 'git merge' <msg> HEAD <commit>...
 'git merge' --abort
diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index d62e02d..c734e2c 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -88,6 +88,7 @@ to modify the commit message.
 
 If the configuration option `merge.usePreCommitHook` is set to `true`
 then 'git merge' invokes this hook whenever it creates a new commit.
+It can be bypassed with the `\--no-verify` option. 
 
 prepare-commit-msg
 ~~~~~~~~~~~~~~~~~~
diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
index 0bcbe0a..5695fc6 100644
--- a/Documentation/merge-options.txt
+++ b/Documentation/merge-options.txt
@@ -70,6 +70,10 @@ merge.
 With --no-squash perform the merge and commit the result. This
 option can be used to override --squash.
 
+--no-verify::
+	This option bypasses the pre-commit hook.
+	See also linkgit:githooks[5].
+
 -s <strategy>::
 --strategy=<strategy>::
 	Use the given merge strategy; can be supplied more than
diff --git a/builtin/merge.c b/builtin/merge.c
index a2590a9..58a848f 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -60,6 +60,7 @@ static const char *branch;
 static char *branch_mergeoptions;
 static int option_renormalize;
 static int verbosity;
+static int no_verify;
 static int allow_rerere_auto;
 static int abort_current_merge;
 static int show_progress = -1;
@@ -199,6 +200,7 @@ static struct option builtin_merge_options[] = {
 		N_("allow fast-forward (default)")),
 	OPT_BOOLEAN(0, "ff-only", &fast_forward_only,
 		N_("abort if fast-forward is not possible")),
+	OPT_BOOLEAN(0, "no-verify", &no_verify, "bypass pre-merge hook"),
 	OPT_RERERE_AUTOUPDATE(&allow_rerere_auto),
 	OPT_CALLBACK('s', "strategy", &use_strategies, N_("strategy"),
 		N_("merge strategy to use"), option_parse_strategy),
@@ -904,7 +906,7 @@ static void prepare_to_commit(struct commit_list *remoteheads)
 	const char *comment = _(merge_editor_comment);
 	const char *index_file = get_index_file();
 
-	if (use_pre_commit_hook && run_hook(index_file, "pre-commit", NULL))
+	if (use_pre_commit_hook && !no_verify && run_hook(index_file, "pre-commit", NULL))
 		abort_commit(remoteheads, NULL);
 	/*
 	 * Re-read the index as pre-commit hook could have updated it,
-- 
1.7.12.406.gafd3f81

[PATCHv2 4/4] t7503: add tests for pre-commit hook (merge)

[Michael J Gruber]

Add tests which make sure that the pre-commit hook is called by 'git
merge' when merge.usePreCommitHook is set, allows/disallows merge
commits depending on its return value and is suppressed by
"--no-verify".

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 t/t7503-pre-commit-hook.sh | 62 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 62 insertions(+)

diff --git a/t/t7503-pre-commit-hook.sh b/t/t7503-pre-commit-hook.sh
index 984889b..e4d324d 100755
--- a/t/t7503-pre-commit-hook.sh
+++ b/t/t7503-pre-commit-hook.sh
@@ -4,6 +4,19 @@ test_description='pre-commit hook'
 
 . ./test-lib.sh
 
+test_expect_success 'root commit' '
+
+	echo "root" > file &&
+	git add file &&
+	git commit -m "zeroth" &&
+	git checkout -b side &&
+	echo "foo" > foo &&
+	git add foo &&
+	git commit -m "make it non-ff" &&
+	git checkout master
+
+'
+
 test_expect_success 'with no hook' '
 
 	echo "foo" > file &&
@@ -12,6 +25,14 @@ test_expect_success 'with no hook' '
 
 '
 
+test_expect_success 'with no hook (merge)' '
+
+	git checkout side &&
+	git merge -m "merge master" master &&
+	git checkout master
+
+'
+
 test_expect_success '--no-verify with no hook' '
 
 	echo "bar" > file &&
@@ -20,6 +41,14 @@ test_expect_success '--no-verify with no hook' '
 
 '
 
+test_expect_success '--no-verify with no hook (merge)' '
+
+	git checkout side &&
+	git merge --no-verify -m "merge master" master &&
+	git checkout master
+
+'
+
 # now install hook that always succeeds
 HOOKDIR="$(git rev-parse --git-dir)/hooks"
 HOOK="$HOOKDIR/pre-commit"
@@ -29,6 +58,7 @@ cat > "$HOOK" <<EOF
 exit 0
 EOF
 chmod +x "$HOOK"
+git config merge.usePreCommitHook true
 
 test_expect_success 'with succeeding hook' '
 
@@ -38,6 +68,14 @@ test_expect_success 'with succeeding hook' '
 
 '
 
+test_expect_success 'with succeeding hook (merge)' '
+
+	git checkout side &&
+	git merge -m "merge master" master &&
+	git checkout master
+
+'
+
 test_expect_success '--no-verify with succeeding hook' '
 
 	echo "even more" >> file &&
@@ -46,6 +84,14 @@ test_expect_success '--no-verify with succeeding hook' '
 
 '
 
+test_expect_success '--no-verify with succeeding hook (merge)' '
+
+	git checkout side &&
+	git merge --no-verify -m "merge master" master &&
+	git checkout master
+
+'
+
 # now a hook that fails
 cat > "$HOOK" <<EOF
 #!/bin/sh
@@ -68,6 +114,22 @@ test_expect_success '--no-verify with failing hook' '
 
 '
 
+test_expect_success 'with failing hook (merge)' '
+
+	git checkout side &&
+	test_must_fail git merge -m "merge master" master &&
+	git checkout master
+
+'
+
+test_expect_success '--no-verify with failing hook (merge)' '
+
+	git checkout side &&
+	git merge --no-verify -m "merge master" master &&
+	git checkout master
+
+'
+
 chmod -x "$HOOK"
 test_expect_success POSIXPERM 'with non-executable hook' '
 
-- 
1.7.12.406.gafd3f81

Re: [PATCH 0/3] pre-merge-hook

[Junio C Hamano]

Michael J Gruber <git@drmicha.warpmail.net> writes:

> Junio C Hamano venit, vidit, dixit 06.09.2012 07:07:
>> Michael J Gruber <git@drmicha.warpmail.net> writes:
>> 
>>> The pre-commit hook is often used to ensure certain properties of each
>>> comitted tree like formatting or coding standards, validity (lint/make)
>>> or code quality (make test). But merges introduce new commits unless
>>> they are fast forwards, and therefore they can break these properties
>>> because the pre-commit hook is not run by "git merge".
>>>
>>> Introduce a pre-merge hook which works for (non ff, automatic) merges
>>> like pre-commit does for commits. Typically this will just call the
>>> pre-commit hook (like in the sample hook), but it does not need to.
>> 
>> When your merge asks for a help from you to resolve conflict, you
>> conclude with "git commit", and at that point, pre-commit hook will
>> have a chance to reject it, presumably.  That means for any project
>> that wants to audit a merge via hook, their pre-commit hook MUST be
>> prepared to look at and judge a merge.  Given that, is a separate
>> hook that "can just call the pre-commit but does not need to" really
>> needed and useful?
>> 
>> I admit that I haven't thought things through, but adding a boolean
>> "merge.usePreCommitHook" smells like a more appropriate approach to
>> me.
>> 
>> I dunno.
>
> That would be an option ;)

I said "I dunno" as others may have opinions on the best direction
to go.

> Either works for me, and if we don't change the current behaviour
> (pre-commit-hook resp. no hook for non-automatic merges resp. automatic
> merges) the config option is probably less confusing.

If we were to go in the "pre-commit has to be prepared to vet a
merge already, so let it handle the automerge case" direction, I
have another suggestion.

Because you need to support "merge --no-verify" to override the hook
anyway, I think it makes sense to introduce "merge --verify" to
force it trigger the hook (and it needs to percolate up to "pull").

People who want it always on may want a boolean merge.verify, but
that should come in a separate step.  The patch that implements it
must make sure all our internal uses of "merge" is updated to pass
"--no-verify", unless there is a very good reason.

Another direction to go would be to deprecate the "commit is the way
to conclude a merge that stopped in the middle due to a conflict
asking for your help" and introduce "merge --continue" [*1*].  That
would open a way to a separate "pre/post-merge" hook much cleanly.
At that point it would be clear that "pre-commit" won't be involved
in "merge" (i.e. the user never will use "git commit" when merging).

I am fairly negative on the current mess imposed on "git commit"
that has to know who called it and behave differently (look for
"whence" in builtin/commit.c), and would rather not to see it made
worse by piling "call pre-merge if exists and in a merge, or call
pre-commit" kind of hack on top of it.



[Footnote]

*1* This has been brought up a few times during discussion on
sequencer and mergy operations, and I think it makes sense in the
longer term.  "am" and "rebase" were first to use "--continue",
instead of having the user to use "commit" to conclude, and later
"cherry-pick" and "revert" have been updated to follow suit, so
"merge" may be the only oddball remaining now.

Re: [PATCH 0/3] pre-merge-hook

[Michael J Gruber]

Junio C Hamano venit, vidit, dixit 06.09.2012 20:34:
> Michael J Gruber <git@drmicha.warpmail.net> writes:
> 
>> Junio C Hamano venit, vidit, dixit 06.09.2012 07:07:
>>> Michael J Gruber <git@drmicha.warpmail.net> writes:
>>> 
>>>> The pre-commit hook is often used to ensure certain properties
>>>> of each comitted tree like formatting or coding standards,
>>>> validity (lint/make) or code quality (make test). But merges
>>>> introduce new commits unless they are fast forwards, and
>>>> therefore they can break these properties because the
>>>> pre-commit hook is not run by "git merge".
>>>> 
>>>> Introduce a pre-merge hook which works for (non ff, automatic)
>>>> merges like pre-commit does for commits. Typically this will
>>>> just call the pre-commit hook (like in the sample hook), but it
>>>> does not need to.
>>> 
>>> When your merge asks for a help from you to resolve conflict,
>>> you conclude with "git commit", and at that point, pre-commit
>>> hook will have a chance to reject it, presumably.  That means for
>>> any project that wants to audit a merge via hook, their
>>> pre-commit hook MUST be prepared to look at and judge a merge.
>>> Given that, is a separate hook that "can just call the pre-commit
>>> but does not need to" really needed and useful?
>>> 
>>> I admit that I haven't thought things through, but adding a
>>> boolean "merge.usePreCommitHook" smells like a more appropriate
>>> approach to me.
>>> 
>>> I dunno.
>> 
>> That would be an option ;)
> 
> I said "I dunno" as others may have opinions on the best direction to
> go.

Sure. I meant to make a pun the config option approach being an option.

>> Either works for me, and if we don't change the current behaviour 
>> (pre-commit-hook resp. no hook for non-automatic merges resp.
>> automatic merges) the config option is probably less confusing.
> 
> If we were to go in the "pre-commit has to be prepared to vet a merge
> already, so let it handle the automerge case" direction, I have
> another suggestion.
> 
> Because you need to support "merge --no-verify" to override the hook 
> anyway, I think it makes sense to introduce "merge --verify" to force
> it trigger the hook (and it needs to percolate up to "pull").
> 
> People who want it always on may want a boolean merge.verify, but 
> that should come in a separate step.  The patch that implements it 
> must make sure all our internal uses of "merge" is updated to pass 
> "--no-verify", unless there is a very good reason.

Hmm. Nothing calls cmd_merge() nor the relevant parts. "git pull" calls
"git merge" and should probably obey that option. All others (am etc.)
call git-merge-${strategy} directly and are not affected by the option.
Am I overlooking something?

> Another direction to go would be to deprecate the "commit is the way 
> to conclude a merge that stopped in the middle due to a conflict 
> asking for your help" and introduce "merge --continue" [*1*].  That 
> would open a way to a separate "pre/post-merge" hook much cleanly. At
> that point it would be clear that "pre-commit" won't be involved in
> "merge" (i.e. the user never will use "git commit" when merging).
> 
> I am fairly negative on the current mess imposed on "git commit" that
> has to know who called it and behave differently (look for "whence"
> in builtin/commit.c), and would rather not to see it made worse by
> piling "call pre-merge if exists and in a merge, or call pre-commit"
> kind of hack on top of it.

That is a mess, yes. Part of it is due to the fact that both
builtin/{commit,merge}.c do a lot of "commit stuff" in parallel, often
with copy & pasted code, and "commit" needs to be aware of other states
(in-am, in-rebase) also.

My feeling is that builtin/merge.c should rather get rid of the stuff
that parallels things that builtin/commit.c does, so that all is in one
place. (I think that redundancy is to blame for the inconsistent hook
behaviour for merges even within builtin/merge.c).

> [Footnote]
> 
> *1* This has been brought up a few times during discussion on 
> sequencer and mergy operations, and I think it makes sense in the 
> longer term.  "am" and "rebase" were first to use "--continue", 
> instead of having the user to use "commit" to conclude, and later 
> "cherry-pick" and "revert" have been updated to follow suit, so 
> "merge" may be the only oddball remaining now.

"git merge --continue" to commit a resolved merge, but code-wise
builtin/commit.c taking over because it's just one more case?

I afraid that restructuring is beyond my available Git capacity and my
self-imposed constraint to avoid anything with backwards compatibility
or migration plan issues. (I took this up because I thought it's within,
to share something I've been using for a while.)

I fully agree that a big-picture-solution is preferable.

Michael