[PATCH 0/4] pre-merge hook

[PATCH 0/4] pre-merge hook

[Michael J Gruber]

Now that f8b863598c ("builtin/merge: honor commit-msg hook for merges",
2017-09-07) has landed, merge is getting closer to behaving like commit,
which is important because both are used to complete merges (automatic
vs. non-automatic).

This series revives an old suggestion of mine to make merge honor
pre-commit hook or a separate pre-merge hook. Since pre-commit does not
receive any arguments (which the hook could use tell between commit and
merge) and in order to keep existing behaviour (as opposed to the other
patch) this series introduces a pre-merge hook, with a sample hook that
simply calls the pre-commit hook.

commit and merge have very similar code paths. f8b863598c implemented
"--no-verify" for merge differently from the existing implementation in
commit. 2/4 changes that and could be first in this series, with the
remaining 3 squashed into 2 commits or 1. I've been rebasing and using
this series for years now, 2/4 is the new-comer which fixed the breakage
from f8b863598c that I encountered because of the no-verify
implementation. 

Michael J Gruber (4):
  git-merge: Honor pre-merge hook
  merge: do no-verify like commit
  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                   | 17 ++++++++--
 t/t7503-pre-commit-hook.sh        | 66 ++++++++++++++++++++++++++++++++++++++-
 templates/hooks--pre-merge.sample | 13 ++++++++
 6 files changed, 104 insertions(+), 5 deletions(-)
 create mode 100755 templates/hooks--pre-merge.sample

-- 
2.14.1.909.g0fa57a0913

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

[Michael J Gruber]

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

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@grubix.eu>
---
 Documentation/githooks.txt        |  7 +++++++
 builtin/merge.c                   | 11 +++++++++++
 templates/hooks--pre-merge.sample | 13 +++++++++++++
 3 files changed, 31 insertions(+)
 create mode 100755 templates/hooks--pre-merge.sample

diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index 1bb4f92d4d..85bedd208c 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -99,6 +99,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 ab5ffe85e8..de254d466b 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -769,6 +769,17 @@ static void write_merge_heads(struct commit_list *);
 static void prepare_to_commit(struct commit_list *remoteheads)
 {
 	struct strbuf msg = STRBUF_INIT;
+	const char *index_file = get_index_file();
+
+	if (run_commit_hook(0 < option_edit, 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 (squash)
diff --git a/templates/hooks--pre-merge.sample b/templates/hooks--pre-merge.sample
new file mode 100755
index 0000000000..a6313e6d5c
--- /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"
+:
-- 
2.14.1.909.g0fa57a0913

[PATCH 2/4] merge: do no-verify like commit

[Michael J Gruber]

f8b863598c ("builtin/merge: honor commit-msg hook for merges", 2017-09-07)
introduced the no-verify to merge for bypassing the commit-msg hook,
though in a different way from the implementation in commit.c.

Change the implementation in merge.c to be the same as in merge.c so
that both do the same in the same way.

Signed-off-by: Michael J Gruber <git@grubix.eu>
---
 builtin/merge.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index de254d466b..7ba094ee87 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -73,7 +73,7 @@ static int show_progress = -1;
 static int default_to_upstream = 1;
 static int signoff;
 static const char *sign_commit;
-static int verify_msg = 1;
+static int no_verify;
 
 static struct strategy all_strategy[] = {
 	{ "recursive",  DEFAULT_TWOHEAD | NO_TRIVIAL },
@@ -237,7 +237,7 @@ static struct option builtin_merge_options[] = {
 	  N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" },
 	OPT_BOOL(0, "overwrite-ignore", &overwrite_ignore, N_("update ignored files (default)")),
 	OPT_BOOL(0, "signoff", &signoff, N_("add Signed-off-by:")),
-	OPT_BOOL(0, "verify", &verify_msg, N_("verify commit-msg hook")),
+	OPT_BOOL(0, "no-verify", &no_verify, N_("bypass commit-msg hook")),
 	OPT_END()
 };
 
@@ -798,7 +798,7 @@ static void prepare_to_commit(struct commit_list *remoteheads)
 			abort_commit(remoteheads, NULL);
 	}
 
-	if (verify_msg && run_commit_hook(0 < option_edit, get_index_file(),
+	if (!no_verify && run_commit_hook(0 < option_edit, get_index_file(),
 					  "commit-msg",
 					  git_path_merge_msg(), NULL))
 		abort_commit(remoteheads, NULL);
-- 
2.14.1.909.g0fa57a0913

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

[Michael J Gruber]

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

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@grubix.eu>
---
 Documentation/git-merge.txt     | 2 +-
 Documentation/githooks.txt      | 2 +-
 Documentation/merge-options.txt | 4 ++++
 builtin/merge.c                 | 4 ++--
 4 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
index 4df6431c34..5f0e1768e1 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>] [-S[<keyid>]]
+	[--no-verify] [-s <strategy>] [-X <strategy-option>] [-S[<keyid>]]
 	[--[no-]allow-unrelated-histories]
 	[--[no-]rerere-autoupdate] [-m <msg>] [<commit>...]
 'git merge' --abort
diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index 85bedd208c..969441d7a2 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -104,7 +104,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 4e32304301..75019d5919 100644
--- a/Documentation/merge-options.txt
+++ b/Documentation/merge-options.txt
@@ -74,6 +74,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 7ba094ee87..c63510c199 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -237,7 +237,7 @@ static struct option builtin_merge_options[] = {
 	  N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" },
 	OPT_BOOL(0, "overwrite-ignore", &overwrite_ignore, N_("update ignored files (default)")),
 	OPT_BOOL(0, "signoff", &signoff, N_("add Signed-off-by:")),
-	OPT_BOOL(0, "no-verify", &no_verify, N_("bypass commit-msg hook")),
+	OPT_BOOL(0, "no-verify", &no_verify, N_("bypass pre-merge and commit-msg hooks")),
 	OPT_END()
 };
 
@@ -771,7 +771,7 @@ static void prepare_to_commit(struct commit_list *remoteheads)
 	struct strbuf msg = STRBUF_INIT;
 	const char *index_file = get_index_file();
 
-	if (run_commit_hook(0 < option_edit, index_file, "pre-merge", NULL))
+	if (!no_verify && run_commit_hook(0 < option_edit, index_file, "pre-merge", NULL))
 		abort_commit(remoteheads, NULL);
 	/*
 	 * Re-read the index as pre-merge hook could have updated it,
-- 
2.14.1.909.g0fa57a0913

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

[Michael J Gruber]

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

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@grubix.eu>
---
 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 984889b39d..36ae87f7ef 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' '
 
-- 
2.14.1.909.g0fa57a0913

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

[Stefan Beller]

On Fri, Sep 22, 2017 at 5:04 AM, Michael J Gruber <git@grubix.eu> wrote:
> From: Michael J Gruber <git@drmicha.warpmail.net>
>
> 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@grubix.eu>
> ---
>  Documentation/githooks.txt        |  7 +++++++
>  builtin/merge.c                   | 11 +++++++++++
>  templates/hooks--pre-merge.sample | 13 +++++++++++++
>  3 files changed, 31 insertions(+)
>  create mode 100755 templates/hooks--pre-merge.sample
>
> diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
> index 1bb4f92d4d..85bedd208c 100644
> --- a/Documentation/githooks.txt
> +++ b/Documentation/githooks.txt
> @@ -99,6 +99,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 ab5ffe85e8..de254d466b 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -769,6 +769,17 @@ static void write_merge_heads(struct commit_list *);
>  static void prepare_to_commit(struct commit_list *remoteheads)
>  {
>         struct strbuf msg = STRBUF_INIT;
> +       const char *index_file = get_index_file();
> +
> +       if (run_commit_hook(0 < option_edit, 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);

Please see 680ee550d7 (commit: skip discarding the index
if there is no pre-commit hook, 2017-08-14), maybe we can do it similarly.
Dropping and rereading the index may be expensive for large repos.

>         strbuf_addbuf(&msg, &merge_msg);
>         strbuf_addch(&msg, '\n');
>         if (squash)
> diff --git a/templates/hooks--pre-merge.sample b/templates/hooks--pre-merge.sample
> new file mode 100755
> index 0000000000..a6313e6d5c
> --- /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

The message goes to stdout or sterr or both?

> +# it wants to stop the commit.

nit: s/commit/merge commit/ maybe?

Thanks,
Stefan

Re: [PATCH 2/4] merge: do no-verify like commit

[Stefan Beller]

On Fri, Sep 22, 2017 at 5:04 AM, Michael J Gruber <git@grubix.eu> wrote:
> f8b863598c ("builtin/merge: honor commit-msg hook for merges", 2017-09-07)
> introduced the no-verify to merge for bypassing the commit-msg hook,
> though in a different way from the implementation in commit.c.
>
> Change the implementation in merge.c to be the same as in merge.c so
> that both do the same in the same way.
>
> Signed-off-by: Michael J Gruber <git@grubix.eu>

Thanks for spotting.
Junio spotted it as well; I assumed I had reverted it in the last
sent out (accepted) patch.

If we want to further the commit message:
  This changes the output of 'git merge --help' in a way that
  it suggests that the hook is on by default.

Thanks,
Stefan

> ---
>  builtin/merge.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/merge.c b/builtin/merge.c
> index de254d466b..7ba094ee87 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -73,7 +73,7 @@ static int show_progress = -1;
>  static int default_to_upstream = 1;
>  static int signoff;
>  static const char *sign_commit;
> -static int verify_msg = 1;
> +static int no_verify;
>
>  static struct strategy all_strategy[] = {
>         { "recursive",  DEFAULT_TWOHEAD | NO_TRIVIAL },
> @@ -237,7 +237,7 @@ static struct option builtin_merge_options[] = {
>           N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" },
>         OPT_BOOL(0, "overwrite-ignore", &overwrite_ignore, N_("update ignored files (default)")),
>         OPT_BOOL(0, "signoff", &signoff, N_("add Signed-off-by:")),
> -       OPT_BOOL(0, "verify", &verify_msg, N_("verify commit-msg hook")),
> +       OPT_BOOL(0, "no-verify", &no_verify, N_("bypass commit-msg hook")),
>         OPT_END()
>  };
>
> @@ -798,7 +798,7 @@ static void prepare_to_commit(struct commit_list *remoteheads)
>                         abort_commit(remoteheads, NULL);
>         }
>
> -       if (verify_msg && run_commit_hook(0 < option_edit, get_index_file(),
> +       if (!no_verify && run_commit_hook(0 < option_edit, get_index_file(),
>                                           "commit-msg",
>                                           git_path_merge_msg(), NULL))
>                 abort_commit(remoteheads, NULL);
> --
> 2.14.1.909.g0fa57a0913
>

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

[Stefan Beller]

On Fri, Sep 22, 2017 at 5:04 AM, Michael J Gruber <git@grubix.eu> wrote:

> --- a/t/t7503-pre-commit-hook.sh
> +++ b/t/t7503-pre-commit-hook.sh

> -test_description='pre-commit hook'
> +test_description='pre-commit and pre-merge hooks'

Tangent: do we also want to rename the file?

> +test_expect_success '--no-verify with succeeding hook (merge)' '

The return value of the hook ought to not matter, so we'd rather want
to test for 'no side effect by hook on --no-verify' ?


> +test_expect_success '--no-verify with failing hook (merge)' '
> +
> +       git checkout side &&
> +       git merge --no-verify -m "merge master" master &&
> +       git checkout master
> +
> +'

I assume the same (unsolved) issues apply to this hook as
to the commit-msg hook with 'git merge --continue'
Do we want a test for that (I am torn on that) ?

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

[Martin Ågren]

On 22 September 2017 at 14:04, Michael J Gruber <git@grubix.eu> wrote:
> From: Michael J Gruber <git@drmicha.warpmail.net>
>
> 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@grubix.eu>
[...]
> diff --git a/templates/hooks--pre-merge.sample b/templates/hooks--pre-merge.sample
> new file mode 100755
> index 0000000000..a6313e6d5c
> --- /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"
> +:

Won't this always exit successfully? Is that wanted? If this sample hook
is activated but can't find an executable pre-commit hook to call out
to, should it complain in some helpful way rather than exiting silently
(whether successfully or not)?

Martin

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

[Junio C Hamano]

Michael J Gruber <git@grubix.eu> writes:

> From: Michael J Gruber <git@drmicha.warpmail.net>
>
> 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@grubix.eu>
> ---

It appears that some of the pieces in this patch, namely,
D/git-merge.txt and D/merge-options.txt, belong to 2/4, where we are
fixing up an earlier addition of --[no-]verify option to the
command, to be updated to only add mention of pre-merge hook in this
step?

The end result looks good regardless, I would think, though.

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

[Michael J Gruber]

Junio C Hamano venit, vidit, dixit 24.09.2017 01:48:
> Michael J Gruber <git@grubix.eu> writes:
> 
>> From: Michael J Gruber <git@drmicha.warpmail.net>
>>
>> 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@grubix.eu>
>> ---
> 
> It appears that some of the pieces in this patch, namely,
> D/git-merge.txt and D/merge-options.txt, belong to 2/4, where we are
> fixing up an earlier addition of --[no-]verify option to the
> command, to be updated to only add mention of pre-merge hook in this
> step?

Oh, sorry, rebaser error. I should also rewrite all commits to my
current e-mail.

> The end result looks good regardless, I would think, though.

Pending restructuring and attending to the other comments...

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

[Junio C Hamano]

Michael J Gruber <git@grubix.eu> writes:

> Now that f8b863598c ("builtin/merge: honor commit-msg hook for merges",
> 2017-09-07) has landed, merge is getting closer to behaving like commit,
> which is important because both are used to complete merges (automatic
> vs. non-automatic).

Just a gentle ping to the thread to see if it is still alive.

I think people agree that this is a good thing to do, but it seems
that the execution leaves a few loose ends, judging from the review
comments that have yet to be answered.

Thanks.

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

[Stefan Beller]

On Sun, Oct 1, 2017 at 10:54 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Michael J Gruber <git@grubix.eu> writes:
>
>> Now that f8b863598c ("builtin/merge: honor commit-msg hook for merges",
>> 2017-09-07) has landed, merge is getting closer to behaving like commit,
>> which is important because both are used to complete merges (automatic
>> vs. non-automatic).
>
> Just a gentle ping to the thread to see if it is still alive.
>
> I think people agree that this is a good thing to do, but it seems
> that the execution leaves a few loose ends, judging from the review
> comments that have yet to be answered.
>
> Thanks.

I agree. IIRC the series was ok in design and goal, just minor comments
for coding.

Thanks,
Stefan

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

[Junio C Hamano]

Michael J Gruber <git@grubix.eu> writes:

> This series revives an old suggestion of mine to make merge honor
> pre-commit hook or a separate pre-merge hook....

This seems to have become an abandoned loose end, so I'll drop the
topic from my tree for now; revival of the discussion is _not_
unwelcome (aka "dropping without prejudice").

[PATCH v2 0/4] pre-merge hook

[Josh Steadmon]

I would like to revive discussion on this series; I have addressed most
of the review comments on the original series sent by Michael, with the
following exceptions:

* Martin's objection on 1/4 that the sample hook would always exit
  successfully is (I believe) incorrect. To test this, I ran
  "/bin/true && exec test 0 == 1" in a /bin/sh subshell, and it
  correctly had a non-zero exit status.

* I'm not sure that I understand Stefan's objections on 4/4 regarding
  the hook's exit status. I am also not familiar with "the same
  (unsolved) issues [as] commit-msg".

I have noted my changes in each commit message in "[js: ...]" sections.

Michael's original series description is below:

Now that f8b863598c ("builtin/merge: honor commit-msg hook for merges",
2017-09-07) has landed, merge is getting closer to behaving like commit,
which is important because both are used to complete merges (automatic
vs. non-automatic).

This series revives an old suggestion of mine to make merge honor
pre-commit hook or a separate pre-merge hook. Since pre-commit does not
receive any arguments (which the hook could use tell between commit and
merge) and in order to keep existing behaviour (as opposed to the other
patch) this series introduces a pre-merge hook, with a sample hook that
simply calls the pre-commit hook.

commit and merge have very similar code paths. f8b863598c implemented
"--no-verify" for merge differently from the existing implementation in
commit. 2/4 changes that and could be first in this series, with the
remaining 3 squashed into 2 commits or 1. I've been rebasing and using
this series for years now, 2/4 is the new-comer which fixed the breakage
from f8b863598c that I encountered because of the no-verify
implementation.

Michael J Gruber (4):
  git-merge: Honor pre-merge hook
  merge: do no-verify like commit
  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                               | 18 ++++-
 ...> t7503-pre-commit-and-pre-merge-hooks.sh} | 66 ++++++++++++++++++-
 templates/hooks--pre-merge.sample             | 13 ++++
 6 files changed, 105 insertions(+), 5 deletions(-)
 rename t/{t7503-pre-commit-hook.sh => t7503-pre-commit-and-pre-merge-hooks.sh} (67%)
 create mode 100755 templates/hooks--pre-merge.sample

-- 
2.22.0.657.g960e92d24f-goog

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

[Josh Steadmon]

From: Michael J Gruber <git@grubix.eu>

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).

[js: addressed review comments:
     * clarified that hook should write messages to stderr
     * only discard the index if the hook is actually present
]

Signed-off-by: Michael J Gruber <git@grubix.eu>
Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 Documentation/githooks.txt        |  7 +++++++
 builtin/merge.c                   | 12 ++++++++++++
 templates/hooks--pre-merge.sample | 13 +++++++++++++
 3 files changed, 32 insertions(+)
 create mode 100755 templates/hooks--pre-merge.sample

diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index 786e778ab8..dcc6606d44 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -103,6 +103,13 @@ The default 'pre-commit' hook, when enabled--and with the
 `hooks.allownonascii` config option unset or set to false--prevents
 the use of non-ASCII filenames.
 
+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 6e99aead46..5cd7752191 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -816,6 +816,18 @@ static void write_merge_heads(struct commit_list *);
 static void prepare_to_commit(struct commit_list *remoteheads)
 {
 	struct strbuf msg = STRBUF_INIT;
+	const char *index_file = get_index_file();
+
+	if (run_commit_hook(0 < option_edit, 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.
+	 */
+	if (find_hook("pre-merge"))
+		discard_cache();
+	read_cache_from(index_file);
 	strbuf_addbuf(&msg, &merge_msg);
 	if (squash)
 		BUG("the control must not reach here under --squash");
diff --git a/templates/hooks--pre-merge.sample b/templates/hooks--pre-merge.sample
new file mode 100755
index 0000000000..f459b3db44
--- /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 to
+# stderr if it wants to stop the merge 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"
+:
-- 
2.22.0.657.g960e92d24f-goog

[PATCH v2 2/4] merge: do no-verify like commit

[Josh Steadmon]

From: Michael J Gruber <git@grubix.eu>

f8b863598c ("builtin/merge: honor commit-msg hook for merges", 2017-09-07)
introduced the no-verify flag to merge for bypassing the commit-msg
hook, though in a different way from the implementation in commit.c.

Change the implementation in merge.c to be the same as in commit.c so
that both do the same in the same way. This also changes the output of
"git merge --help" to be more clear that the hook return code is
respected by default.

[js: reworded commit message, and moved documentation changes from patch
 3/4 to this commit.]

Signed-off-by: Michael J Gruber <git@grubix.eu>
Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 Documentation/git-merge.txt     | 2 +-
 Documentation/merge-options.txt | 4 ++++
 builtin/merge.c                 | 6 +++---
 3 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
index c01cfa6595..f71ed1d0f9 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>] [-S[<keyid>]]
+	[--no-verify] [-s <strategy>] [-X <strategy-option>] [-S[<keyid>]]
 	[--[no-]allow-unrelated-histories]
 	[--[no-]rerere-autoupdate] [-m <msg>] [-F <file>] [<commit>...]
 'git merge' --abort
diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
index 79a00d2a4a..d6a9f4b96f 100644
--- a/Documentation/merge-options.txt
+++ b/Documentation/merge-options.txt
@@ -105,6 +105,10 @@ option can be used to override --squash.
 +
 With --squash, --commit is not allowed, and will fail.
 
+--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 5cd7752191..5bbef203f3 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -81,7 +81,7 @@ static int show_progress = -1;
 static int default_to_upstream = 1;
 static int signoff;
 static const char *sign_commit;
-static int verify_msg = 1;
+static int no_verify;
 
 static struct strategy all_strategy[] = {
 	{ "recursive",  DEFAULT_TWOHEAD | NO_TRIVIAL },
@@ -287,7 +287,7 @@ static struct option builtin_merge_options[] = {
 	  N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" },
 	OPT_BOOL(0, "overwrite-ignore", &overwrite_ignore, N_("update ignored files (default)")),
 	OPT_BOOL(0, "signoff", &signoff, N_("add Signed-off-by:")),
-	OPT_BOOL(0, "verify", &verify_msg, N_("verify commit-msg hook")),
+	OPT_BOOL(0, "no-verify", &no_verify, N_("bypass commit-msg hook")),
 	OPT_END()
 };
 
@@ -854,7 +854,7 @@ static void prepare_to_commit(struct commit_list *remoteheads)
 			abort_commit(remoteheads, NULL);
 	}
 
-	if (verify_msg && run_commit_hook(0 < option_edit, get_index_file(),
+	if (!no_verify && run_commit_hook(0 < option_edit, get_index_file(),
 					  "commit-msg",
 					  git_path_merge_msg(the_repository), NULL))
 		abort_commit(remoteheads, NULL);
-- 
2.22.0.657.g960e92d24f-goog

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

[Josh Steadmon]

From: Michael J Gruber <git@grubix.eu>

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.

[js: cleaned up trailing whitespace, moved some documentation changes
 from this commit to 2/4.]

Signed-off-by: Michael J Gruber <git@grubix.eu>
Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 Documentation/githooks.txt | 2 +-
 builtin/merge.c            | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index dcc6606d44..6a93478dcf 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -108,7 +108,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/builtin/merge.c b/builtin/merge.c
index 5bbef203f3..5ed3472c84 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -287,7 +287,7 @@ static struct option builtin_merge_options[] = {
 	  N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" },
 	OPT_BOOL(0, "overwrite-ignore", &overwrite_ignore, N_("update ignored files (default)")),
 	OPT_BOOL(0, "signoff", &signoff, N_("add Signed-off-by:")),
-	OPT_BOOL(0, "no-verify", &no_verify, N_("bypass commit-msg hook")),
+	OPT_BOOL(0, "no-verify", &no_verify, N_("bypass pre-merge and commit-msg hooks")),
 	OPT_END()
 };
 
@@ -818,7 +818,7 @@ static void prepare_to_commit(struct commit_list *remoteheads)
 	struct strbuf msg = STRBUF_INIT;
 	const char *index_file = get_index_file();
 
-	if (run_commit_hook(0 < option_edit, index_file, "pre-merge", NULL))
+	if (!no_verify && run_commit_hook(0 < option_edit, index_file, "pre-merge", NULL))
 		abort_commit(remoteheads, NULL);
 	/*
 	 * Re-read the index as pre-merge hook could have updated it,
-- 
2.22.0.657.g960e92d24f-goog

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

[Josh Steadmon]

From: Michael J Gruber <git@grubix.eu>

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".

[js: renamed test as suggested in review comments]

Signed-off-by: Michael J Gruber <git@grubix.eu>
Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 ...> t7503-pre-commit-and-pre-merge-hooks.sh} | 66 ++++++++++++++++++-
 1 file changed, 65 insertions(+), 1 deletion(-)
 rename t/{t7503-pre-commit-hook.sh => t7503-pre-commit-and-pre-merge-hooks.sh} (67%)

diff --git a/t/t7503-pre-commit-hook.sh b/t/t7503-pre-commit-and-pre-merge-hooks.sh
similarity index 67%
rename from t/t7503-pre-commit-hook.sh
rename to t/t7503-pre-commit-and-pre-merge-hooks.sh
index 984889b39d..36ae87f7ef 100755
--- a/t/t7503-pre-commit-hook.sh
+++ b/t/t7503-pre-commit-and-pre-merge-hooks.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' '
 
-- 
2.22.0.657.g960e92d24f-goog

[PATCH v2 0/4] pre-merge hook

[Josh Steadmon]

(I'm resending this cover letter with fixed Message-Id so that threading
@ public-inbox works properly. Sorry for the noise.)

I would like to revive discussion on this series; I have addressed most
of the review comments on the original series sent by Michael, with the
following exceptions:

* Martin's objection on 1/4 that the sample hook would always exit
  successfully is (I believe) incorrect. To test this, I ran
  "/bin/true && exec test 0 == 1" in a /bin/sh subshell, and it
  correctly had a non-zero exit status.

* I'm not sure that I understand Stefan's objections on 4/4 regarding
  the hook's exit status. I am also not familiar with "the same
  (unsolved) issues [as] commit-msg".

I have noted my changes in each commit message in "[js: ...]" sections.

Michael's original series description is below:

Now that f8b863598c ("builtin/merge: honor commit-msg hook for merges",
2017-09-07) has landed, merge is getting closer to behaving like commit,
which is important because both are used to complete merges (automatic
vs. non-automatic).

This series revives an old suggestion of mine to make merge honor
pre-commit hook or a separate pre-merge hook. Since pre-commit does not
receive any arguments (which the hook could use tell between commit and
merge) and in order to keep existing behaviour (as opposed to the other
patch) this series introduces a pre-merge hook, with a sample hook that
simply calls the pre-commit hook.

commit and merge have very similar code paths. f8b863598c implemented
"--no-verify" for merge differently from the existing implementation in
commit. 2/4 changes that and could be first in this series, with the
remaining 3 squashed into 2 commits or 1. I've been rebasing and using
this series for years now, 2/4 is the new-comer which fixed the breakage
from f8b863598c that I encountered because of the no-verify
implementation.

Michael J Gruber (4):
  git-merge: Honor pre-merge hook
  merge: do no-verify like commit
  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                               | 18 ++++-
 ...> t7503-pre-commit-and-pre-merge-hooks.sh} | 66 ++++++++++++++++++-
 templates/hooks--pre-merge.sample             | 13 ++++
 6 files changed, 105 insertions(+), 5 deletions(-)
 rename t/{t7503-pre-commit-hook.sh => t7503-pre-commit-and-pre-merge-hooks.sh} (67%)
 create mode 100755 templates/hooks--pre-merge.sample

-- 
2.22.0.657.g960e92d24f-goog

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

[Martin Ågren]

On Fri, 19 Jul 2019 at 00:57, Josh Steadmon <steadmon@google.com> wrote:
> --- /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 to
> +# stderr if it wants to stop the merge 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"
> +:

I'm pretty certain that my comment here was with respect to the ":",
which made me think "hmmm, won't : always exit successfully?". My
slightly less ignorant self of today answers "sure, but we won't always
get that far" -- the "exec" is pretty important, and it will cause the
current shell to be replaced with the hook invocation. So however the
pre-commit hook decides to exit, that'll be our exit status.

I retract my comment from back then.


Martin

Re: [PATCH v2 3/4] merge: --no-verify to bypass pre-merge hook

[Martin Ågren]

On Fri, 19 Jul 2019 at 00:57, Josh Steadmon <steadmon@google.com> wrote:
> From: Michael J Gruber <git@grubix.eu>
>
> 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.

I don't understand this "(non-existing)". I realize this message is two
years old, maybe lots older still, and I haven't dug. But at least
*now*, "-n" seems to behave like "--no-stat", suppressing "--stat", from
my simple testing.


Martin

Re: [PATCH v2 4/4] t7503: add tests for pre-merge-hook

[Martin Ågren]

On Fri, 19 Jul 2019 at 00:57, Josh Steadmon <steadmon@google.com> wrote:
> +test_expect_success '--no-verify with succeeding hook (merge)' '
> +
> +       git checkout side &&
> +       git merge --no-verify -m "merge master" master &&
> +       git checkout master
> +
> +'

This test doesn't even try to verify that the hook was actually ignored.
That left me puzzled for a while...

> +test_expect_success '--no-verify with failing hook (merge)' '
> +
> +       git checkout side &&
> +       git merge --no-verify -m "merge master" master &&
> +       git checkout master
> +
> +'

... but this would then (most likely) fail, so we would notice
something's wrong.

This script seems to me like if it passes 100%, we can be fairly sure
we're ok, but if some individual test fails, we shouldn't be surprised
if its oneline description is a bit off compared to the bug. Similarly,
quite a few tests could pass, despite their oneline description inducing
the thought of "but surely, if /that/ were the problem, /those/ tests
would fail".

Anyway, I realize this is just following the existing approach. I guess
you could argue it has served us well for a long time.

I would probably prefer seeing the various hunks in this patch being
squashed into the relevant commits (1/4 vs 3/4) to make those patches
more self-describing.


Martin

Re: [PATCH v2 0/4] pre-merge hook

[Martin Ågren]

On Fri, 19 Jul 2019 at 01:56, Josh Steadmon <steadmon@google.com> wrote:
> * Martin's objection on 1/4 that the sample hook would always exit
>   successfully is (I believe) incorrect. To test this, I ran
>   "/bin/true && exec test 0 == 1" in a /bin/sh subshell, and it
>   correctly had a non-zero exit status.

I retract my comment on this. It was incorrect, indeed.

>   git-merge: Honor pre-merge hook

Nit: s/H/h/

>   merge: do no-verify like commit

Nit (maybe just me): this could be patch 1/N, before getting started
with the real focus of this series.


Martin

Re: [PATCH v2 0/4] pre-merge hook

[Martin Ågren]

On Fri, 19 Jul 2019 at 01:56, Josh Steadmon <steadmon@google.com> wrote:
> This series revives an old suggestion [...] to make merge honor
> pre-commit hook or a separate pre-merge hook. Since pre-commit does not
> receive any arguments (which the hook could use tell between commit and
> merge) and in order to keep existing behaviour (as opposed to the other
> patch) this series introduces a pre-merge hook, with a sample hook that
> simply calls the pre-commit hook.

Argh, I wanted to comment on this in the cover letter, but forgot and
just left a bunch of nits.

I wonder if we might ever regret naming this "pre-merge" and not, e.g.,
"pre-merge-commit". There's the pre-rebase hook which runs before
git-rebase even starts actually rebasing, so I could well imagine a
"pre-merge" hook which would get called before merging starts.

I'm also pondering whether there should be an "automatic" in there, but
"pre-automatic-merge-commit" looks a bit awkward. Anyway, I'm not even
sure an "automatic merge commit" is a well-defined thing. I can figure
out what it pretty certainly means, but it's not crystal clear. There
might be a need for some more discussion in the added docs for what this
new hook is for and how it compares to pre-commit. Right now, the
proposed docs suggests they're equivalent in a way, but I think that's a
bit confusing -- they are certainly not synonyms, and they'd never both
be called, for example. They can be used for the same purpose in
different scenarios, sure.

I tried coming up with some proposed docs for githooks.txt, but didn't
feel I achieved much of value. :-/

Martin

Re: [PATCH v2 0/4] pre-merge hook

[Josh Steadmon]

On 2019.07.29 22:09, Martin Ågren wrote:
> On Fri, 19 Jul 2019 at 01:56, Josh Steadmon <steadmon@google.com> wrote:
> > * Martin's objection on 1/4 that the sample hook would always exit
> >   successfully is (I believe) incorrect. To test this, I ran
> >   "/bin/true && exec test 0 == 1" in a /bin/sh subshell, and it
> >   correctly had a non-zero exit status.
> 
> I retract my comment on this. It was incorrect, indeed.
> 
> >   git-merge: Honor pre-merge hook
> 
> Nit: s/H/h/

Fixed in V3.

> >   merge: do no-verify like commit
> 
> Nit (maybe just me): this could be patch 1/N, before getting started
> with the real focus of this series.

Agreed, I found the ordering strange as well, but wanted to keep it the
same as V1 to start with. Fixed in V3.

> 
> 
> Martin

Re: [PATCH v2 3/4] merge: --no-verify to bypass pre-merge hook

[Josh Steadmon]

On 2019.07.29 22:02, Martin Ågren wrote:
> On Fri, 19 Jul 2019 at 00:57, Josh Steadmon <steadmon@google.com> wrote:
> > From: Michael J Gruber <git@grubix.eu>
> >
> > 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.
> 
> I don't understand this "(non-existing)". I realize this message is two
> years old, maybe lots older still, and I haven't dug. But at least
> *now*, "-n" seems to behave like "--no-stat", suppressing "--stat", from
> my simple testing.

Fixed in V3.

Re: [PATCH v2 0/4] pre-merge hook

[Josh Steadmon]

On 2019.07.29 22:29, Martin Ågren wrote:
> On Fri, 19 Jul 2019 at 01:56, Josh Steadmon <steadmon@google.com> wrote:
> > This series revives an old suggestion [...] to make merge honor
> > pre-commit hook or a separate pre-merge hook. Since pre-commit does not
> > receive any arguments (which the hook could use tell between commit and
> > merge) and in order to keep existing behaviour (as opposed to the other
> > patch) this series introduces a pre-merge hook, with a sample hook that
> > simply calls the pre-commit hook.
> 
> Argh, I wanted to comment on this in the cover letter, but forgot and
> just left a bunch of nits.
> 
> I wonder if we might ever regret naming this "pre-merge" and not, e.g.,
> "pre-merge-commit". There's the pre-rebase hook which runs before
> git-rebase even starts actually rebasing, so I could well imagine a
> "pre-merge" hook which would get called before merging starts.

"pre-merge-commit" sounds reasonable to me. I'll wait for a bit before
sending out V3 in case anyone else wants to chime in here though.

> I'm also pondering whether there should be an "automatic" in there, but
> "pre-automatic-merge-commit" looks a bit awkward. Anyway, I'm not even
> sure an "automatic merge commit" is a well-defined thing. I can figure
> out what it pretty certainly means, but it's not crystal clear. There
> might be a need for some more discussion in the added docs for what this
> new hook is for and how it compares to pre-commit. Right now, the
> proposed docs suggests they're equivalent in a way, but I think that's a
> bit confusing -- they are certainly not synonyms, and they'd never both
> be called, for example. They can be used for the same purpose in
> different scenarios, sure.
> 
> I tried coming up with some proposed docs for githooks.txt, but didn't
> feel I achieved much of value. :-/
> 
> Martin

Re: [PATCH v2 4/4] t7503: add tests for pre-merge-hook

[Josh Steadmon]

On 2019.07.29 22:04, Martin Ågren wrote:
> On Fri, 19 Jul 2019 at 00:57, Josh Steadmon <steadmon@google.com> wrote:
> > +test_expect_success '--no-verify with succeeding hook (merge)' '
> > +
> > +       git checkout side &&
> > +       git merge --no-verify -m "merge master" master &&
> > +       git checkout master
> > +
> > +'
> 
> This test doesn't even try to verify that the hook was actually ignored.
> That left me puzzled for a while...
> 
> > +test_expect_success '--no-verify with failing hook (merge)' '
> > +
> > +       git checkout side &&
> > +       git merge --no-verify -m "merge master" master &&
> > +       git checkout master
> > +
> > +'
> 
> ... but this would then (most likely) fail, so we would notice
> something's wrong.
> 
> This script seems to me like if it passes 100%, we can be fairly sure
> we're ok, but if some individual test fails, we shouldn't be surprised
> if its oneline description is a bit off compared to the bug. Similarly,
> quite a few tests could pass, despite their oneline description inducing
> the thought of "but surely, if /that/ were the problem, /those/ tests
> would fail".
> 
> Anyway, I realize this is just following the existing approach. I guess
> you could argue it has served us well for a long time.
> 
> I would probably prefer seeing the various hunks in this patch being
> squashed into the relevant commits (1/4 vs 3/4) to make those patches
> more self-describing.

Will squash these as you said in V3. Will also think about whether
another test approach would make more sense here.

Re: [PATCH v2 4/4] t7503: add tests for pre-merge-hook

[Martin Ågren]

On Tue, 30 Jul 2019 at 01:43, Josh Steadmon <steadmon@google.com> wrote:
>
> On 2019.07.29 22:04, Martin Ågren wrote:
> > This script seems to me like if it passes 100%, we can be fairly sure
> > we're ok, but [...]

> Will squash these as you said in V3. Will also think about whether
> another test approach would make more sense here.

Thinking a bit more about this, this test uses two identical hooks, runs
some commands and verifies that "the" hook was run (or not, with
--no-verify). If the implementation started calling the wrong hook
(pre-commit / pre-merge) or both hooks, we wouldn't notice.

Please forgive my braindump below, I'm on the run so I'm just throwing
this out there:

Perhaps (first do some modernizing of this script, to protect various
setup steps, use "write_script", etc, then) make the existing hook a
tiny bit pre-commit-specific, e.g., by doing something like "echo
pre-commit >>executed-hooks", then at select places check "test_cmp
executed-hooks pre-commit" (against "echo pre-commit >pre-commit"),
"test_path_is_missing executed-hooks", and so on, coupled with some
"test_when_finished 'rm -f executed_hooks'". Then the tests added for
this series would use a very similar hook, appending and checking for
"pre-merge[-commit]", That should make us fairly certain that we're
running precisely the wanted hook, I think.

Martin

Re: [PATCH v2 4/4] t7503: add tests for pre-merge-hook

[Josh Steadmon]

On 2019.07.30 09:13, Martin Ågren wrote:
> On Tue, 30 Jul 2019 at 01:43, Josh Steadmon <steadmon@google.com> wrote:
> >
> > On 2019.07.29 22:04, Martin Ågren wrote:
> > > This script seems to me like if it passes 100%, we can be fairly sure
> > > we're ok, but [...]
> 
> > Will squash these as you said in V3. Will also think about whether
> > another test approach would make more sense here.
> 
> Thinking a bit more about this, this test uses two identical hooks, runs
> some commands and verifies that "the" hook was run (or not, with
> --no-verify). If the implementation started calling the wrong hook
> (pre-commit / pre-merge) or both hooks, we wouldn't notice.
> 
> Please forgive my braindump below, I'm on the run so I'm just throwing
> this out there:
> 
> Perhaps (first do some modernizing of this script, to protect various
> setup steps, use "write_script", etc, then) make the existing hook a
> tiny bit pre-commit-specific, e.g., by doing something like "echo
> pre-commit >>executed-hooks", then at select places check "test_cmp
> executed-hooks pre-commit" (against "echo pre-commit >pre-commit"),
> "test_path_is_missing executed-hooks", and so on, coupled with some
> "test_when_finished 'rm -f executed_hooks'". Then the tests added for
> this series would use a very similar hook, appending and checking for
> "pre-merge[-commit]", That should make us fairly certain that we're
> running precisely the wanted hook, I think.
> 
> Martin

That sounds like a reasonable approach, thank you for the suggestions. I
will work on this for V3.

[PATCH v3 0/4] pre-merge-commit hook

[Josh Steadmon]

This series adds a new pre-merge-commit hook, similar in usage to
pre-commit. It also improves hook testing in t7503, by verifying that
the correct hooks are run or bypassed as expected.

The original series was done by Michael J Gruber <git@grubix.eu>. I have
addressed the outstanding review comments, and noted my changes in the
commit messages in "[js: ...]" blocks.

Changes since V2:
* Renamed the hook from "pre-merge" to "pre-merge-commit".
* Added a new patch (1/4) to improve t7503 by verifying that the
  expected hooks are (or are not) run.
* Squashed test changes (from V2's patch 4/4) into patch 3/4.
  Modified the tests to follow the example set in patch 1/4.
* Reworded commit messages to match with the current state of certain
  flags, which changed in between V1 and V2 of this series.

Josh Steadmon (1):
  t7503: verify proper hook execution

Michael J Gruber (3):
  merge: do no-verify like commit
  git-merge: honor pre-merge-commit hook
  merge: --no-verify to bypass pre-merge-commit hook

 Documentation/git-merge.txt                   |   2 +-
 Documentation/githooks.txt                    |   7 +
 Documentation/merge-options.txt               |   4 +
 builtin/merge.c                               |  18 +-
 ...3-pre-commit-and-pre-merge-commit-hooks.sh | 237 ++++++++++++++++++
 t/t7503-pre-commit-hook.sh                    | 139 ----------
 templates/hooks--pre-merge-commit.sample      |  13 +
 7 files changed, 277 insertions(+), 143 deletions(-)
 create mode 100755 t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
 delete mode 100755 t/t7503-pre-commit-hook.sh
 create mode 100755 templates/hooks--pre-merge-commit.sample

Range-diff against v2:
1:  e8b3fd8a5b < -:  ---------- git-merge: Honor pre-merge hook
-:  ---------- > 1:  f0476b2b1e t7503: verify proper hook execution
2:  36406a85be ! 2:  89ddbf410f merge: do no-verify like commit
    @@ Commit message
         "git merge --help" to be more clear that the hook return code is
         respected by default.
     
    -    [js: reworded commit message, and moved documentation changes from patch
    -     3/4 to this commit.]
    +    [js: * reworded commit message
    +         * squashed documentation changes from original series' patch 3/4
    +    ]
     
         Signed-off-by: Michael J Gruber <git@grubix.eu>
    @@ Documentation/git-merge.txt: SYNOPSIS
     +  [--no-verify] [-s <strategy>] [-X <strategy-option>] [-S[<keyid>]]
        [--[no-]allow-unrelated-histories]
        [--[no-]rerere-autoupdate] [-m <msg>] [-F <file>] [<commit>...]
    - 'git merge' --abort
    + 'git merge' (--continue | --abort | --quit)
     
      ## Documentation/merge-options.txt ##
     @@ Documentation/merge-options.txt: option can be used to override --squash.
3:  2440ad35e4 < -:  ---------- merge: --no-verify to bypass pre-merge hook
4:  69dc3696e7 < -:  ---------- t7503: add tests for pre-merge-hook
-:  ---------- > 3:  61b989ff16 git-merge: honor pre-merge-commit hook
-:  ---------- > 4:  45828c56fc merge: --no-verify to bypass pre-merge-commit hook

-- 
2.22.0.770.g0f2c4a37fd-goog

[PATCH v3 1/4] t7503: verify proper hook execution

[Josh Steadmon]

t7503 did not verify that the expected hooks actually ran during
testing. Fix that by making the hook scripts write their $0 into a file
so that we can compare actual execution vs. expected execution.

While we're at it, do some test style cleanups, such as using
write_script() and doing setup inside a test_expect_success block.

Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 t/t7503-pre-commit-hook.sh | 162 +++++++++++++++++++++----------------
 1 file changed, 94 insertions(+), 68 deletions(-)

diff --git a/t/t7503-pre-commit-hook.sh b/t/t7503-pre-commit-hook.sh
index 984889b39d..500bdd97c2 100755
--- a/t/t7503-pre-commit-hook.sh
+++ b/t/t7503-pre-commit-hook.sh
@@ -4,124 +4,149 @@ test_description='pre-commit hook'
 
 . ./test-lib.sh
 
-test_expect_success 'with no hook' '
+HOOKDIR="$(git rev-parse --git-dir)/hooks"
+PRECOMMIT="$HOOKDIR/pre-commit"
+
+# Prepare sample scripts that write their $0 to actual_hooks
+test_expect_success 'sample script setup' '
+	mkdir -p "$HOOKDIR" &&
+	write_script "$HOOKDIR/success.sample" <<-\EOF &&
+	echo $0 >>actual_hooks
+	exit 0
+	EOF
+	write_script "$HOOKDIR/fail.sample" <<-\EOF &&
+	echo $0 >>actual_hooks
+	exit 1
+	EOF
+	write_script "$HOOKDIR/require-prefix.sample" <<-\EOF &&
+	echo $0 >>actual_hooks
+	test $GIT_PREFIX = "success/"
+	EOF
+	write_script "$HOOKDIR/check-author.sample" <<-\EOF
+	echo $0 >>actual_hooks
+	test "$GIT_AUTHOR_NAME" = "New Author" &&
+	test "$GIT_AUTHOR_EMAIL" = "newauthor@example.com"
+	EOF
+'
 
-	echo "foo" > file &&
+test_expect_success 'with no hook' '
+	test_when_finished "rm -f expected_hooks actual_hooks" &&
+	touch expected_hooks actual_hooks &&
+	echo "foo" >file &&
 	git add file &&
-	git commit -m "first"
-
+	git commit -m "first" &&
+	test_cmp expected_hooks actual_hooks
 '
 
 test_expect_success '--no-verify with no hook' '
-
-	echo "bar" > file &&
+	test_when_finished "rm -f expected_hooks actual_hooks" &&
+	touch expected_hooks actual_hooks &&
+	echo "bar" >file &&
 	git add file &&
-	git commit --no-verify -m "bar"
-
+	git commit --no-verify -m "bar" &&
+	test_cmp expected_hooks actual_hooks
 '
 
-# 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 &&
+	test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks" &&
+	ln -s "success.sample" "$PRECOMMIT" &&
+	touch actual_hooks &&
+	echo "$PRECOMMIT" >expected_hooks &&
+	echo "more" >>file &&
 	git add file &&
-	git commit -m "more"
-
+	git commit -m "more" &&
+	test_cmp expected_hooks actual_hooks
 '
 
 test_expect_success '--no-verify with succeeding hook' '
-
-	echo "even more" >> file &&
+	test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks" &&
+	ln -s "success.sample" "$PRECOMMIT" &&
+	touch expected_hooks actual_hooks &&
+	echo "even more" >>file &&
 	git add file &&
-	git commit --no-verify -m "even more"
-
+	git commit --no-verify -m "even more" &&
+	test_cmp expected_hooks actual_hooks
 '
 
-# now a hook that fails
-cat > "$HOOK" <<EOF
-#!/bin/sh
-exit 1
-EOF
-
 test_expect_success 'with failing hook' '
-
-	echo "another" >> file &&
+	test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks" &&
+	ln -s "fail.sample" "$PRECOMMIT" &&
+	touch actual_hooks &&
+	echo "$PRECOMMIT" >expected_hooks &&
+	echo "another" >>file &&
 	git add file &&
-	test_must_fail git commit -m "another"
-
+	test_must_fail git commit -m "another" &&
+	test_cmp expected_hooks actual_hooks
 '
 
 test_expect_success '--no-verify with failing hook' '
-
-	echo "stuff" >> file &&
+	test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks" &&
+	ln -s "fail.sample" "$PRECOMMIT" &&
+	touch expected_hooks actual_hooks &&
+	echo "stuff" >>file &&
 	git add file &&
-	git commit --no-verify -m "stuff"
-
+	git commit --no-verify -m "stuff" &&
+	test_cmp expected_hooks actual_hooks
 '
 
-chmod -x "$HOOK"
 test_expect_success POSIXPERM 'with non-executable hook' '
-
-	echo "content" >> file &&
+	test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks; chmod +x \"$HOOKDIR/fail.sample\"" &&
+	ln -s "fail.sample" "$PRECOMMIT" &&
+	chmod -x "$HOOKDIR/fail.sample" &&
+	touch expected_hooks actual_hooks &&
+	echo "content" >>file &&
 	git add file &&
-	git commit -m "content"
-
+	git commit -m "content" &&
+	test_cmp expected_hooks actual_hooks
 '
 
 test_expect_success POSIXPERM '--no-verify with non-executable hook' '
-
-	echo "more content" >> file &&
+	test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks; chmod +x \"$HOOKDIR/fail.sample\"" &&
+	ln -s "fail.sample" "$PRECOMMIT" &&
+	chmod -x "$HOOKDIR/fail.sample" &&
+	touch expected_hooks actual_hooks &&
+	echo "more content" >>file &&
 	git add file &&
-	git commit --no-verify -m "more content"
-
+	git commit --no-verify -m "more content" &&
+	test_cmp expected_hooks actual_hooks
 '
-chmod +x "$HOOK"
-
-# a hook that checks $GIT_PREFIX and succeeds inside the
-# success/ subdirectory only
-cat > "$HOOK" <<EOF
-#!/bin/sh
-test \$GIT_PREFIX = success/
-EOF
 
 test_expect_success 'with hook requiring GIT_PREFIX' '
-
-	echo "more content" >> file &&
+	test_when_finished "rm -rf \"$PRECOMMIT\" expected_hooks actual_hooks success" &&
+	ln -s "require-prefix.sample" "$PRECOMMIT" &&
+	echo "$PRECOMMIT" >expected_hooks &&
+	echo "more content" >>file &&
 	git add file &&
 	mkdir success &&
 	(
 		cd success &&
 		git commit -m "hook requires GIT_PREFIX = success/"
 	) &&
-	rmdir success
+	test_cmp expected_hooks actual_hooks
 '
 
 test_expect_success 'with failing hook requiring GIT_PREFIX' '
-
-	echo "more content" >> file &&
+	test_when_finished "rm -rf \"$PRECOMMIT\" expected_hooks actual_hooks fail" &&
+	ln -s "require-prefix.sample" "$PRECOMMIT" &&
+	echo "$PRECOMMIT" >expected_hooks &&
+	echo "more content" >>file &&
 	git add file &&
 	mkdir fail &&
 	(
 		cd fail &&
 		test_must_fail git commit -m "hook must fail"
 	) &&
-	rmdir fail &&
-	git checkout -- file
+	git checkout -- file &&
+	test_cmp expected_hooks actual_hooks
 '
 
 test_expect_success 'check the author in hook' '
-	write_script "$HOOK" <<-\EOF &&
-	test "$GIT_AUTHOR_NAME" = "New Author" &&
-	test "$GIT_AUTHOR_EMAIL" = "newauthor@example.com"
+	test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks" &&
+	ln -s "check-author.sample" "$PRECOMMIT" &&
+	cat >expected_hooks <<-EOF &&
+	$PRECOMMIT
+	$PRECOMMIT
+	$PRECOMMIT
 	EOF
 	test_must_fail git commit --allow-empty -m "by a.u.thor" &&
 	(
@@ -133,7 +158,8 @@ test_expect_success 'check the author in hook' '
 	) &&
 	git commit --author="New Author <newauthor@example.com>" \
 		--allow-empty -m "by new.author via command line" &&
-	git show -s
+	git show -s &&
+	test_cmp expected_hooks actual_hooks
 '
 
 test_done
-- 
2.22.0.770.g0f2c4a37fd-goog

[PATCH v3 2/4] merge: do no-verify like commit

[Josh Steadmon]

f8b863598c ("builtin/merge: honor commit-msg hook for merges", 2017-09-07)
introduced the no-verify flag to merge for bypassing the commit-msg
hook, though in a different way from the implementation in commit.c.

Change the implementation in merge.c to be the same as in commit.c so
that both do the same in the same way. This also changes the output of
"git merge --help" to be more clear that the hook return code is
respected by default.

[js: * reworded commit message
     * squashed documentation changes from original series' patch 3/4
]

Signed-off-by: Michael J Gruber <git@grubix.eu>
Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 Documentation/git-merge.txt     | 2 +-
 Documentation/merge-options.txt | 4 ++++
 builtin/merge.c                 | 6 +++---
 3 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
index 01fd52dc70..092529c619 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>] [-S[<keyid>]]
+	[--no-verify] [-s <strategy>] [-X <strategy-option>] [-S[<keyid>]]
 	[--[no-]allow-unrelated-histories]
 	[--[no-]rerere-autoupdate] [-m <msg>] [-F <file>] [<commit>...]
 'git merge' (--continue | --abort | --quit)
diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
index 79a00d2a4a..d6a9f4b96f 100644
--- a/Documentation/merge-options.txt
+++ b/Documentation/merge-options.txt
@@ -105,6 +105,10 @@ option can be used to override --squash.
 +
 With --squash, --commit is not allowed, and will fail.
 
+--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 e2ccbc44e2..4425a7a12e 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -81,7 +81,7 @@ static int show_progress = -1;
 static int default_to_upstream = 1;
 static int signoff;
 static const char *sign_commit;
-static int verify_msg = 1;
+static int no_verify;
 
 static struct strategy all_strategy[] = {
 	{ "recursive",  DEFAULT_TWOHEAD | NO_TRIVIAL },
@@ -287,7 +287,7 @@ static struct option builtin_merge_options[] = {
 	  N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" },
 	OPT_BOOL(0, "overwrite-ignore", &overwrite_ignore, N_("update ignored files (default)")),
 	OPT_BOOL(0, "signoff", &signoff, N_("add Signed-off-by:")),
-	OPT_BOOL(0, "verify", &verify_msg, N_("verify commit-msg hook")),
+	OPT_BOOL(0, "no-verify", &no_verify, N_("bypass commit-msg hook")),
 	OPT_END()
 };
 
@@ -842,7 +842,7 @@ static void prepare_to_commit(struct commit_list *remoteheads)
 			abort_commit(remoteheads, NULL);
 	}
 
-	if (verify_msg && run_commit_hook(0 < option_edit, get_index_file(),
+	if (!no_verify && run_commit_hook(0 < option_edit, get_index_file(),
 					  "commit-msg",
 					  git_path_merge_msg(the_repository), NULL))
 		abort_commit(remoteheads, NULL);
-- 
2.22.0.770.g0f2c4a37fd-goog

[PATCH v3 3/4] git-merge: honor pre-merge-commit hook

[Josh Steadmon]

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-commit 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).

[js: * renamed hook from "pre-merge" to "pre-merge-commit"
     * only discard the index if the hook is actually present
     * clarified that hook should write messages to stderr
     * squashed test changes from the original series' patch 4/4
     * modified tests to follow new pattern from this series' patch 1/4
     * reworded commit message
]

Signed-off-by: Michael J Gruber <git@grubix.eu>
Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 Documentation/githooks.txt                    |  7 +++
 builtin/merge.c                               | 12 +++++
 ...-pre-commit-and-pre-merge-commit-hooks.sh} | 45 ++++++++++++++++++-
 templates/hooks--pre-merge-commit.sample      | 13 ++++++
 4 files changed, 76 insertions(+), 1 deletion(-)
 rename t/{t7503-pre-commit-hook.sh => t7503-pre-commit-and-pre-merge-commit-hooks.sh} (78%)
 create mode 100755 templates/hooks--pre-merge-commit.sample

diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index 82cd573776..7c4c994858 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -103,6 +103,13 @@ The default 'pre-commit' hook, when enabled--and with the
 `hooks.allownonascii` config option unset or set to false--prevents
 the use of non-ASCII filenames.
 
+pre-merge-commit
+~~~~~~~~~~~~~~~~
+
+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 4425a7a12e..bf0ae68c40 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -816,6 +816,18 @@ static void write_merge_heads(struct commit_list *);
 static void prepare_to_commit(struct commit_list *remoteheads)
 {
 	struct strbuf msg = STRBUF_INIT;
+	const char *index_file = get_index_file();
+
+	if (run_commit_hook(0 < option_edit, index_file, "pre-merge-commit", NULL))
+		abort_commit(remoteheads, NULL);
+	/*
+	 * Re-read the index as pre-merge-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.
+	 */
+	if (find_hook("pre-merge-commit"))
+		discard_cache();
+	read_cache_from(index_file);
 	strbuf_addbuf(&msg, &merge_msg);
 	if (squash)
 		BUG("the control must not reach here under --squash");
diff --git a/t/t7503-pre-commit-hook.sh b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
similarity index 78%
rename from t/t7503-pre-commit-hook.sh
rename to t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
index 500bdd97c2..040dfa0175 100755
--- a/t/t7503-pre-commit-hook.sh
+++ b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
@@ -1,11 +1,12 @@
 #!/bin/sh
 
-test_description='pre-commit hook'
+test_description='pre-commit and pre-merge-commit hooks'
 
 . ./test-lib.sh
 
 HOOKDIR="$(git rev-parse --git-dir)/hooks"
 PRECOMMIT="$HOOKDIR/pre-commit"
+PREMERGE="$HOOKDIR/pre-merge-commit"
 
 # Prepare sample scripts that write their $0 to actual_hooks
 test_expect_success 'sample script setup' '
@@ -29,6 +30,17 @@ test_expect_success 'sample script setup' '
 	EOF
 '
 
+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' '
 	test_when_finished "rm -f expected_hooks actual_hooks" &&
 	touch expected_hooks actual_hooks &&
@@ -38,6 +50,15 @@ test_expect_success 'with no hook' '
 	test_cmp expected_hooks actual_hooks
 '
 
+test_expect_success 'with no hook (merge)' '
+	test_when_finished "rm -f expected_hooks actual_hooks" &&
+	touch expected_hooks actual_hooks &&
+	git checkout side &&
+	git merge -m "merge master" master &&
+	git checkout master &&
+	test_cmp expected_hooks actual_hooks
+'
+
 test_expect_success '--no-verify with no hook' '
 	test_when_finished "rm -f expected_hooks actual_hooks" &&
 	touch expected_hooks actual_hooks &&
@@ -58,6 +79,17 @@ test_expect_success 'with succeeding hook' '
 	test_cmp expected_hooks actual_hooks
 '
 
+test_expect_success 'with succeeding hook (merge)' '
+	test_when_finished "rm -f \"$PREMERGE\" expected_hooks actual_hooks" &&
+	ln -s "success.sample" "$PREMERGE" &&
+	touch actual_hooks &&
+	echo "$PREMERGE" >expected_hooks &&
+	git checkout side &&
+	git merge -m "merge master" master &&
+	git checkout master &&
+	test_cmp expected_hooks actual_hooks
+'
+
 test_expect_success '--no-verify with succeeding hook' '
 	test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks" &&
 	ln -s "success.sample" "$PRECOMMIT" &&
@@ -89,6 +121,17 @@ test_expect_success '--no-verify with failing hook' '
 	test_cmp expected_hooks actual_hooks
 '
 
+test_expect_success 'with failing hook (merge)' '
+	test_when_finished "rm -f \"$PREMERGE\" expected_hooks actual_hooks" &&
+	ln -s "fail.sample" "$PREMERGE" &&
+	touch actual_hooks &&
+	echo "$PREMERGE" >expected_hooks &&
+	git checkout side &&
+	test_must_fail git merge -m "merge master" master &&
+	git checkout master &&
+	test_cmp expected_hooks actual_hooks
+'
+
 test_expect_success POSIXPERM 'with non-executable hook' '
 	test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks; chmod +x \"$HOOKDIR/fail.sample\"" &&
 	ln -s "fail.sample" "$PRECOMMIT" &&
diff --git a/templates/hooks--pre-merge-commit.sample b/templates/hooks--pre-merge-commit.sample
new file mode 100755
index 0000000000..399eab1924
--- /dev/null
+++ b/templates/hooks--pre-merge-commit.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 to
+# stderr if it wants to stop the merge commit.
+#
+# To enable this hook, rename this file to "pre-merge-commit".
+
+. git-sh-setup
+test -x "$GIT_DIR/hooks/pre-commit" &&
+        exec "$GIT_DIR/hooks/pre-commit"
+:
-- 
2.22.0.770.g0f2c4a37fd-goog

[PATCH v3 4/4] merge: --no-verify to bypass pre-merge-commit hook

[Josh Steadmon]

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

[js: * reworded commit message to reflect current state of --no-stat flag
       and new hook name
     * fixed flag documentation to reflect new hook name
     * cleaned up trailing whitespace
     * squashed test changes from the original series' patch 4/4
     * modified tests to follow pattern from this series' patch 1/4
]

Signed-off-by: Michael J Gruber <git@grubix.eu>
Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 Documentation/githooks.txt                    |  2 +-
 builtin/merge.c                               |  4 +--
 ...3-pre-commit-and-pre-merge-commit-hooks.sh | 29 +++++++++++++++++++
 3 files changed, 32 insertions(+), 3 deletions(-)

diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index 7c4c994858..a934553509 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -108,7 +108,7 @@ pre-merge-commit
 
 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/builtin/merge.c b/builtin/merge.c
index bf0ae68c40..c9746e37b8 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -287,7 +287,7 @@ static struct option builtin_merge_options[] = {
 	  N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" },
 	OPT_BOOL(0, "overwrite-ignore", &overwrite_ignore, N_("update ignored files (default)")),
 	OPT_BOOL(0, "signoff", &signoff, N_("add Signed-off-by:")),
-	OPT_BOOL(0, "no-verify", &no_verify, N_("bypass commit-msg hook")),
+	OPT_BOOL(0, "no-verify", &no_verify, N_("bypass pre-merge-commit and commit-msg hooks")),
 	OPT_END()
 };
 
@@ -818,7 +818,7 @@ static void prepare_to_commit(struct commit_list *remoteheads)
 	struct strbuf msg = STRBUF_INIT;
 	const char *index_file = get_index_file();
 
-	if (run_commit_hook(0 < option_edit, index_file, "pre-merge-commit", NULL))
+	if (!no_verify && run_commit_hook(0 < option_edit, index_file, "pre-merge-commit", NULL))
 		abort_commit(remoteheads, NULL);
 	/*
 	 * Re-read the index as pre-merge-commit hook could have updated it,
diff --git a/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
index 040dfa0175..86a375ab3e 100755
--- a/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
+++ b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
@@ -68,6 +68,15 @@ test_expect_success '--no-verify with no hook' '
 	test_cmp expected_hooks actual_hooks
 '
 
+test_expect_success '--no-verify with no hook (merge)' '
+	test_when_finished "rm -f expected_hooks actual_hooks" &&
+	touch expected_hooks actual_hooks &&
+	git checkout side &&
+	git merge --no-verify -m "merge master" master &&
+	git checkout master &&
+	test_cmp expected_hooks actual_hooks
+'
+
 test_expect_success 'with succeeding hook' '
 	test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks" &&
 	ln -s "success.sample" "$PRECOMMIT" &&
@@ -100,6 +109,16 @@ test_expect_success '--no-verify with succeeding hook' '
 	test_cmp expected_hooks actual_hooks
 '
 
+test_expect_success '--no-verify with succeeding hook (merge)' '
+	test_when_finished "rm -f \"$PREMERGE\" expected_hooks actual_hooks" &&
+	ln -s "success.sample" "$PREMERGE" &&
+	touch expected_hooks actual_hooks &&
+	git checkout side &&
+	git merge --no-verify -m "merge master" master &&
+	git checkout master &&
+	test_cmp expected_hooks actual_hooks
+'
+
 test_expect_success 'with failing hook' '
 	test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks" &&
 	ln -s "fail.sample" "$PRECOMMIT" &&
@@ -132,6 +151,16 @@ test_expect_success 'with failing hook (merge)' '
 	test_cmp expected_hooks actual_hooks
 '
 
+test_expect_success '--no-verify with failing hook (merge)' '
+	test_when_finished "rm -f \"$PREMERGE\" expected_hooks actual_hooks" &&
+	ln -s "fail.sample" "$PREMERGE" &&
+	touch expected_hooks actual_hooks &&
+	git checkout side &&
+	git merge --no-verify -m "merge master" master &&
+	git checkout master &&
+	test_cmp expected_hooks actual_hooks
+'
+
 test_expect_success POSIXPERM 'with non-executable hook' '
 	test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks; chmod +x \"$HOOKDIR/fail.sample\"" &&
 	ln -s "fail.sample" "$PRECOMMIT" &&
-- 
2.22.0.770.g0f2c4a37fd-goog

Re: [PATCH v3 1/4] t7503: verify proper hook execution

[Martin Ågren]

On Fri, 2 Aug 2019 at 00:20, Josh Steadmon <steadmon@google.com> wrote:
>
> t7503 did not verify that the expected hooks actually ran during
> testing. Fix that by making the hook scripts write their $0 into a file
> so that we can compare actual execution vs. expected execution.

It could be argued that this test is perfectly fine as long as there is
just one hook to choose from, and as long as we're fine with failing
tests being a bit non-obvious to debug.  Since we'll soon have two hooks
to choose from in this test, we really want to make sure that we're
executing the right one, not just "at least one". (No need to reroll,
this is just me thinking out loud.)


Martin

Re: [PATCH v3 3/4] git-merge: honor pre-merge-commit hook

[Martin Ågren]

On Fri, 2 Aug 2019 at 00:20, Josh Steadmon <steadmon@google.com> wrote:

> diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
> index 82cd573776..7c4c994858 100644
> --- a/Documentation/githooks.txt
> +++ b/Documentation/githooks.txt
> @@ -103,6 +103,13 @@ The default 'pre-commit' hook, when enabled--and with the
>  `hooks.allownonascii` config option unset or set to false--prevents
>  the use of non-ASCII filenames.
>
> +pre-merge-commit
> +~~~~~~~~~~~~~~~~
> +
> +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.
> +

I'm not sure everyone understands what an "automatic merge commit" is.
(Is it an automatic "merge commit", or an "automatic merge" commit? Or
sort of both?) And I'm not sure exactly what to infer from the
"equivalence". I happen to know that the statement about the default
hook can only be half-carried over. And I'm not sure what to infer from
"All the git commit hooks are invoked with the environment variable
...".

Is the below suggestion 1) correct, 2) readable?

  This hook is invoked by linkgit:git-merge[1], and can be bypassed
  with the `--no-verify` option.  It takes no parameters, and is
  invoked after the merge has been carried out successfully and before
  obtaining the proposed commit log message to
  make a commit.  Exiting with a non-zero status from this script
  causes the `git merge` command to abort before creating a commit.

  The default 'pre-merge-commit' hook, when enabled, runs the
  'pre-commit' hook, if the latter is enabled.

  This hook is invoked with the environment variable
  `GIT_EDITOR=:` if the command will not bring up an editor
  to modify the commit message.

  If the merge cannot be carried out automatically, the conflicts
  need to be resolved and the result committed separately (see
  linkgit:git-merge[1]). At that point, this hook will not be executed,
  but the 'pre-commit' hook will, if it is enabled.

(If you use this or something like it, notice how this already mentions
`--no-verify`...)

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

You got rid of loads of "> file" in patch 1/4, so it seems unfortunate
to introduce a few here. ;-)


Martin

Re: [PATCH v3 0/4] pre-merge-commit hook

[Martin Ågren]

[Dropped cc-list the first time around. Apologies to those who receive
this twice...]

On Fri, 2 Aug 2019 at 00:20, Josh Steadmon <steadmon@google.com> wrote:
>
> This series adds a new pre-merge-commit hook, similar in usage to
> pre-commit. It also improves hook testing in t7503, by verifying that
> the correct hooks are run or bypassed as expected.

I really like those test improvements. Now it should be harder to mess
up a future refactoring and run the wrong hook. These hooks are "very
related" so I think this is important.

I've messed with the test a bit and offer these potential improvements
for your consideration. I was lazy and just built this on top of your
series -- if you agree to some or all of these, you'll probably need to
squash them into a few individual patches.

The first four are perhaps more or less a matter of opinion, although I
do think that patch 2/5 is based on an opinion shared by others. ;-)
Patch 5/5 or something like it seems pretty important to me to make
sure that of these two "similar"/"related" hooks with some
"backwards-compatibility-and-code-copying-history" around them, we'd
better pick the right one when they're both available.
 
Feel free to pick and squash as you see fit. (I don't think it makes
sense to have these go in as-are. They really are meant for squashing.)

Martin

Martin Ågren (5):
  t7503: use "&&" in "test_when_finished" rather than ";"
  t7503: avoid touch when mtime doesn't matter
  t7503: simplify file-juggling
  t7503: don't create "actual_hooks" for later appending
  t7503: test failing merge with both hooks available

 ...3-pre-commit-and-pre-merge-commit-hooks.sh | 84 +++++++++++--------
 1 file changed, 50 insertions(+), 34 deletions(-)

-- 
2.23.0.rc0.30.g51cf315870

[PATCH 1/5] t7503: use "&&" in "test_when_finished" rather than ";"

[Martin Ågren]

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 t/t7503-pre-commit-and-pre-merge-commit-hooks.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
index 86a375ab3e..5cc6c98375 100755
--- a/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
+++ b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
@@ -162,7 +162,7 @@ test_expect_success '--no-verify with failing hook (merge)' '
 '
 
 test_expect_success POSIXPERM 'with non-executable hook' '
-	test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks; chmod +x \"$HOOKDIR/fail.sample\"" &&
+	test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks && chmod +x \"$HOOKDIR/fail.sample\"" &&
 	ln -s "fail.sample" "$PRECOMMIT" &&
 	chmod -x "$HOOKDIR/fail.sample" &&
 	touch expected_hooks actual_hooks &&
@@ -173,7 +173,7 @@ test_expect_success POSIXPERM 'with non-executable hook' '
 '
 
 test_expect_success POSIXPERM '--no-verify with non-executable hook' '
-	test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks; chmod +x \"$HOOKDIR/fail.sample\"" &&
+	test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks && chmod +x \"$HOOKDIR/fail.sample\"" &&
 	ln -s "fail.sample" "$PRECOMMIT" &&
 	chmod -x "$HOOKDIR/fail.sample" &&
 	touch expected_hooks actual_hooks &&
-- 
2.23.0.rc0.30.g51cf315870

[PATCH 2/5] t7503: avoid touch when mtime doesn't matter

[Martin Ågren]

AFAIU, we avoid using `touch` unless we really do want to update the
mtime. Here, we just want to create empty files, so write `>foo`
instead.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 ...3-pre-commit-and-pre-merge-commit-hooks.sh | 38 ++++++++++++-------
 1 file changed, 24 insertions(+), 14 deletions(-)

diff --git a/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
index 5cc6c98375..a6f1240aa2 100755
--- a/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
+++ b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
@@ -43,7 +43,8 @@ test_expect_success 'root commit' '
 
 test_expect_success 'with no hook' '
 	test_when_finished "rm -f expected_hooks actual_hooks" &&
-	touch expected_hooks actual_hooks &&
+	>expected_hooks &&
+	>actual_hooks &&
 	echo "foo" >file &&
 	git add file &&
 	git commit -m "first" &&
@@ -52,7 +53,8 @@ test_expect_success 'with no hook' '
 
 test_expect_success 'with no hook (merge)' '
 	test_when_finished "rm -f expected_hooks actual_hooks" &&
-	touch expected_hooks actual_hooks &&
+	>expected_hooks &&
+	>actual_hooks &&
 	git checkout side &&
 	git merge -m "merge master" master &&
 	git checkout master &&
@@ -61,7 +63,8 @@ test_expect_success 'with no hook (merge)' '
 
 test_expect_success '--no-verify with no hook' '
 	test_when_finished "rm -f expected_hooks actual_hooks" &&
-	touch expected_hooks actual_hooks &&
+	>expected_hooks &&
+	>actual_hooks &&
 	echo "bar" >file &&
 	git add file &&
 	git commit --no-verify -m "bar" &&
@@ -70,7 +73,8 @@ test_expect_success '--no-verify with no hook' '
 
 test_expect_success '--no-verify with no hook (merge)' '
 	test_when_finished "rm -f expected_hooks actual_hooks" &&
-	touch expected_hooks actual_hooks &&
+	>expected_hooks &&
+	>actual_hooks &&
 	git checkout side &&
 	git merge --no-verify -m "merge master" master &&
 	git checkout master &&
@@ -80,7 +84,7 @@ test_expect_success '--no-verify with no hook (merge)' '
 test_expect_success 'with succeeding hook' '
 	test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks" &&
 	ln -s "success.sample" "$PRECOMMIT" &&
-	touch actual_hooks &&
+	>actual_hooks &&
 	echo "$PRECOMMIT" >expected_hooks &&
 	echo "more" >>file &&
 	git add file &&
@@ -91,7 +95,7 @@ test_expect_success 'with succeeding hook' '
 test_expect_success 'with succeeding hook (merge)' '
 	test_when_finished "rm -f \"$PREMERGE\" expected_hooks actual_hooks" &&
 	ln -s "success.sample" "$PREMERGE" &&
-	touch actual_hooks &&
+	>actual_hooks &&
 	echo "$PREMERGE" >expected_hooks &&
 	git checkout side &&
 	git merge -m "merge master" master &&
@@ -102,7 +106,8 @@ test_expect_success 'with succeeding hook (merge)' '
 test_expect_success '--no-verify with succeeding hook' '
 	test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks" &&
 	ln -s "success.sample" "$PRECOMMIT" &&
-	touch expected_hooks actual_hooks &&
+	>expected_hooks &&
+	>actual_hooks &&
 	echo "even more" >>file &&
 	git add file &&
 	git commit --no-verify -m "even more" &&
@@ -112,7 +117,8 @@ test_expect_success '--no-verify with succeeding hook' '
 test_expect_success '--no-verify with succeeding hook (merge)' '
 	test_when_finished "rm -f \"$PREMERGE\" expected_hooks actual_hooks" &&
 	ln -s "success.sample" "$PREMERGE" &&
-	touch expected_hooks actual_hooks &&
+	>expected_hooks &&
+	>actual_hooks &&
 	git checkout side &&
 	git merge --no-verify -m "merge master" master &&
 	git checkout master &&
@@ -122,7 +128,7 @@ test_expect_success '--no-verify with succeeding hook (merge)' '
 test_expect_success 'with failing hook' '
 	test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks" &&
 	ln -s "fail.sample" "$PRECOMMIT" &&
-	touch actual_hooks &&
+	>actual_hooks &&
 	echo "$PRECOMMIT" >expected_hooks &&
 	echo "another" >>file &&
 	git add file &&
@@ -133,7 +139,8 @@ test_expect_success 'with failing hook' '
 test_expect_success '--no-verify with failing hook' '
 	test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks" &&
 	ln -s "fail.sample" "$PRECOMMIT" &&
-	touch expected_hooks actual_hooks &&
+	>expected_hooks &&
+	>actual_hooks &&
 	echo "stuff" >>file &&
 	git add file &&
 	git commit --no-verify -m "stuff" &&
@@ -143,7 +150,7 @@ test_expect_success '--no-verify with failing hook' '
 test_expect_success 'with failing hook (merge)' '
 	test_when_finished "rm -f \"$PREMERGE\" expected_hooks actual_hooks" &&
 	ln -s "fail.sample" "$PREMERGE" &&
-	touch actual_hooks &&
+	>actual_hooks &&
 	echo "$PREMERGE" >expected_hooks &&
 	git checkout side &&
 	test_must_fail git merge -m "merge master" master &&
@@ -154,7 +161,8 @@ test_expect_success 'with failing hook (merge)' '
 test_expect_success '--no-verify with failing hook (merge)' '
 	test_when_finished "rm -f \"$PREMERGE\" expected_hooks actual_hooks" &&
 	ln -s "fail.sample" "$PREMERGE" &&
-	touch expected_hooks actual_hooks &&
+	>expected_hooks &&
+	>actual_hooks &&
 	git checkout side &&
 	git merge --no-verify -m "merge master" master &&
 	git checkout master &&
@@ -165,7 +173,8 @@ test_expect_success POSIXPERM 'with non-executable hook' '
 	test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks && chmod +x \"$HOOKDIR/fail.sample\"" &&
 	ln -s "fail.sample" "$PRECOMMIT" &&
 	chmod -x "$HOOKDIR/fail.sample" &&
-	touch expected_hooks actual_hooks &&
+	>expected_hooks &&
+	>actual_hooks &&
 	echo "content" >>file &&
 	git add file &&
 	git commit -m "content" &&
@@ -176,7 +185,8 @@ test_expect_success POSIXPERM '--no-verify with non-executable hook' '
 	test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks && chmod +x \"$HOOKDIR/fail.sample\"" &&
 	ln -s "fail.sample" "$PRECOMMIT" &&
 	chmod -x "$HOOKDIR/fail.sample" &&
-	touch expected_hooks actual_hooks &&
+	>expected_hooks &&
+	>actual_hooks &&
 	echo "more content" >>file &&
 	git add file &&
 	git commit --no-verify -m "more content" &&
-- 
2.23.0.rc0.30.g51cf315870

[PATCH 3/5] t7503: simplify file-juggling

[Martin Ågren]

Rather than creating an empty "expected_hooks" file and another empty
file "actual_hooks" (which we want to not be appended to), just verify
that "actual_hooks" isn't created. Do still clean up the "actual_hooks"
file, so that a failing test won't start affecting later tests too.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 ...3-pre-commit-and-pre-merge-commit-hooks.sh | 60 +++++++------------
 1 file changed, 20 insertions(+), 40 deletions(-)

diff --git a/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
index a6f1240aa2..477207cb5c 100755
--- a/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
+++ b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
@@ -42,43 +42,35 @@ test_expect_success 'root commit' '
 '
 
 test_expect_success 'with no hook' '
-	test_when_finished "rm -f expected_hooks actual_hooks" &&
-	>expected_hooks &&
-	>actual_hooks &&
+	test_when_finished "rm -f actual_hooks" &&
 	echo "foo" >file &&
 	git add file &&
 	git commit -m "first" &&
-	test_cmp expected_hooks actual_hooks
+	test_path_is_missing actual_hooks
 '
 
 test_expect_success 'with no hook (merge)' '
-	test_when_finished "rm -f expected_hooks actual_hooks" &&
-	>expected_hooks &&
-	>actual_hooks &&
+	test_when_finished "rm -f actual_hooks" &&
 	git checkout side &&
 	git merge -m "merge master" master &&
 	git checkout master &&
-	test_cmp expected_hooks actual_hooks
+	test_path_is_missing actual_hooks
 '
 
 test_expect_success '--no-verify with no hook' '
-	test_when_finished "rm -f expected_hooks actual_hooks" &&
-	>expected_hooks &&
-	>actual_hooks &&
+	test_when_finished "rm -f actual_hooks" &&
 	echo "bar" >file &&
 	git add file &&
 	git commit --no-verify -m "bar" &&
-	test_cmp expected_hooks actual_hooks
+	test_path_is_missing actual_hooks
 '
 
 test_expect_success '--no-verify with no hook (merge)' '
-	test_when_finished "rm -f expected_hooks actual_hooks" &&
-	>expected_hooks &&
-	>actual_hooks &&
+	test_when_finished "rm -f actual_hooks" &&
 	git checkout side &&
 	git merge --no-verify -m "merge master" master &&
 	git checkout master &&
-	test_cmp expected_hooks actual_hooks
+	test_path_is_missing actual_hooks
 '
 
 test_expect_success 'with succeeding hook' '
@@ -104,25 +96,21 @@ test_expect_success 'with succeeding hook (merge)' '
 '
 
 test_expect_success '--no-verify with succeeding hook' '
-	test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks" &&
+	test_when_finished "rm -f \"$PRECOMMIT\" actual_hooks" &&
 	ln -s "success.sample" "$PRECOMMIT" &&
-	>expected_hooks &&
-	>actual_hooks &&
 	echo "even more" >>file &&
 	git add file &&
 	git commit --no-verify -m "even more" &&
-	test_cmp expected_hooks actual_hooks
+	test_path_is_missing actual_hooks
 '
 
 test_expect_success '--no-verify with succeeding hook (merge)' '
-	test_when_finished "rm -f \"$PREMERGE\" expected_hooks actual_hooks" &&
+	test_when_finished "rm -f \"$PREMERGE\" actual_hooks" &&
 	ln -s "success.sample" "$PREMERGE" &&
-	>expected_hooks &&
-	>actual_hooks &&
 	git checkout side &&
 	git merge --no-verify -m "merge master" master &&
 	git checkout master &&
-	test_cmp expected_hooks actual_hooks
+	test_path_is_missing actual_hooks
 '
 
 test_expect_success 'with failing hook' '
@@ -137,14 +125,12 @@ test_expect_success 'with failing hook' '
 '
 
 test_expect_success '--no-verify with failing hook' '
-	test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks" &&
+	test_when_finished "rm -f \"$PRECOMMIT\" actual_hooks" &&
 	ln -s "fail.sample" "$PRECOMMIT" &&
-	>expected_hooks &&
-	>actual_hooks &&
 	echo "stuff" >>file &&
 	git add file &&
 	git commit --no-verify -m "stuff" &&
-	test_cmp expected_hooks actual_hooks
+	test_path_is_missing actual_hooks
 '
 
 test_expect_success 'with failing hook (merge)' '
@@ -159,38 +145,32 @@ test_expect_success 'with failing hook (merge)' '
 '
 
 test_expect_success '--no-verify with failing hook (merge)' '
-	test_when_finished "rm -f \"$PREMERGE\" expected_hooks actual_hooks" &&
+	test_when_finished "rm -f \"$PREMERGE\" actual_hooks" &&
 	ln -s "fail.sample" "$PREMERGE" &&
-	>expected_hooks &&
-	>actual_hooks &&
 	git checkout side &&
 	git merge --no-verify -m "merge master" master &&
 	git checkout master &&
-	test_cmp expected_hooks actual_hooks
+	test_path_is_missing actual_hooks
 '
 
 test_expect_success POSIXPERM 'with non-executable hook' '
-	test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks && chmod +x \"$HOOKDIR/fail.sample\"" &&
+	test_when_finished "rm -f \"$PRECOMMIT\" actual_hooks && chmod +x \"$HOOKDIR/fail.sample\"" &&
 	ln -s "fail.sample" "$PRECOMMIT" &&
 	chmod -x "$HOOKDIR/fail.sample" &&
-	>expected_hooks &&
-	>actual_hooks &&
 	echo "content" >>file &&
 	git add file &&
 	git commit -m "content" &&
-	test_cmp expected_hooks actual_hooks
+	test_path_is_missing actual_hooks
 '
 
 test_expect_success POSIXPERM '--no-verify with non-executable hook' '
-	test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks && chmod +x \"$HOOKDIR/fail.sample\"" &&
+	test_when_finished "rm -f \"$PRECOMMIT\" actual_hooks && chmod +x \"$HOOKDIR/fail.sample\"" &&
 	ln -s "fail.sample" "$PRECOMMIT" &&
 	chmod -x "$HOOKDIR/fail.sample" &&
-	>expected_hooks &&
-	>actual_hooks &&
 	echo "more content" >>file &&
 	git add file &&
 	git commit --no-verify -m "more content" &&
-	test_cmp expected_hooks actual_hooks
+	test_path_is_missing actual_hooks
 '
 
 test_expect_success 'with hook requiring GIT_PREFIX' '
-- 
2.23.0.rc0.30.g51cf315870

[PATCH 4/5] t7503: don't create "actual_hooks" for later appending

[Martin Ågren]

If we fail to call the hook, we won't append to "actual_hooks".
"test_cmp" is able to handle a missing file just fine, so these
"pre-creations" are mostly confusing. Let's drop them.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
--
 This "pre-creation" does protect against the file already
 existing, i.e., it could be seen as a "clearing". But since
 we use "test_when_finished" in this script... Also, these
 were "touch" before another of my suggestions, so these
 really were "pre-creating" these files if needed.

 t/t7503-pre-commit-and-pre-merge-commit-hooks.sh | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
index 477207cb5c..f0c73fd58d 100755
--- a/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
+++ b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
@@ -76,7 +76,6 @@ test_expect_success '--no-verify with no hook (merge)' '
 test_expect_success 'with succeeding hook' '
 	test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks" &&
 	ln -s "success.sample" "$PRECOMMIT" &&
-	>actual_hooks &&
 	echo "$PRECOMMIT" >expected_hooks &&
 	echo "more" >>file &&
 	git add file &&
@@ -87,7 +86,6 @@ test_expect_success 'with succeeding hook' '
 test_expect_success 'with succeeding hook (merge)' '
 	test_when_finished "rm -f \"$PREMERGE\" expected_hooks actual_hooks" &&
 	ln -s "success.sample" "$PREMERGE" &&
-	>actual_hooks &&
 	echo "$PREMERGE" >expected_hooks &&
 	git checkout side &&
 	git merge -m "merge master" master &&
@@ -116,7 +114,6 @@ test_expect_success '--no-verify with succeeding hook (merge)' '
 test_expect_success 'with failing hook' '
 	test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks" &&
 	ln -s "fail.sample" "$PRECOMMIT" &&
-	>actual_hooks &&
 	echo "$PRECOMMIT" >expected_hooks &&
 	echo "another" >>file &&
 	git add file &&
@@ -136,7 +133,6 @@ test_expect_success '--no-verify with failing hook' '
 test_expect_success 'with failing hook (merge)' '
 	test_when_finished "rm -f \"$PREMERGE\" expected_hooks actual_hooks" &&
 	ln -s "fail.sample" "$PREMERGE" &&
-	>actual_hooks &&
 	echo "$PREMERGE" >expected_hooks &&
 	git checkout side &&
 	test_must_fail git merge -m "merge master" master &&
-- 
2.23.0.rc0.30.g51cf315870

[PATCH 5/5] t7503: test failing merge with both hooks available

[Martin Ågren]

With a failing automatic merge, we want to make sure that we *don't*
call the pre-merge-commit hook and that we (eventually) *do* call the
pre-commit hook.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 ...3-pre-commit-and-pre-merge-commit-hooks.sh | 30 +++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
index f0c73fd58d..4bf359c097 100755
--- a/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
+++ b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
@@ -41,6 +41,18 @@ test_expect_success 'root commit' '
 	git checkout master
 '
 
+test_expect_success 'setup conflicting branches' '
+	test_when_finished "git checkout master" &&
+	git checkout -b conflicting-a master &&
+	echo a >conflicting &&
+	git add conflicting &&
+	git commit -m conflicting-a &&
+	git checkout -b conflicting-b master &&
+	echo b >conflicting &&
+	git add conflicting &&
+	git commit -m conflicting-b
+'
+
 test_expect_success 'with no hook' '
 	test_when_finished "rm -f actual_hooks" &&
 	echo "foo" >file &&
@@ -93,6 +105,24 @@ test_expect_success 'with succeeding hook (merge)' '
 	test_cmp expected_hooks actual_hooks
 '
 
+test_expect_success 'automatic merge fails; both hooks are available' '
+	test_when_finished "rm -f \"$PREMERGE\" \"$PRECOMMIT\"" &&
+	test_when_finished "rm -f expected_hooks actual_hooks" &&
+	test_when_finished "git checkout master" &&
+	ln -s "success.sample" "$PREMERGE" &&
+	ln -s "success.sample" "$PRECOMMIT" &&
+
+	git checkout conflicting-a &&
+	test_must_fail git merge -m "merge conflicting-b" conflicting-b &&
+	test_path_is_missing actual_hooks &&
+
+	echo "$PRECOMMIT" >expected_hooks &&
+	echo a+b >conflicting &&
+	git add conflicting &&
+	git commit -m "resolve conflict" &&
+	test_cmp expected_hooks actual_hooks
+'
+
 test_expect_success '--no-verify with succeeding hook' '
 	test_when_finished "rm -f \"$PRECOMMIT\" actual_hooks" &&
 	ln -s "success.sample" "$PRECOMMIT" &&
-- 
2.23.0.rc0.30.g51cf315870

Re: [PATCH v3 0/4] pre-merge-commit hook

[Josh Steadmon]

On 2019.08.02 11:56, Martin Ågren wrote:
> [Dropped cc-list the first time around. Apologies to those who receive
> this twice...]
> 
> On Fri, 2 Aug 2019 at 00:20, Josh Steadmon <steadmon@google.com> wrote:
> >
> > This series adds a new pre-merge-commit hook, similar in usage to
> > pre-commit. It also improves hook testing in t7503, by verifying that
> > the correct hooks are run or bypassed as expected.
> 
> I really like those test improvements. Now it should be harder to mess
> up a future refactoring and run the wrong hook. These hooks are "very
> related" so I think this is important.
> 
> I've messed with the test a bit and offer these potential improvements
> for your consideration. I was lazy and just built this on top of your
> series -- if you agree to some or all of these, you'll probably need to
> squash them into a few individual patches.
> 
> The first four are perhaps more or less a matter of opinion, although I
> do think that patch 2/5 is based on an opinion shared by others. ;-)
> Patch 5/5 or something like it seems pretty important to me to make
> sure that of these two "similar"/"related" hooks with some
> "backwards-compatibility-and-code-copying-history" around them, we'd
> better pick the right one when they're both available.
>  
> Feel free to pick and squash as you see fit. (I don't think it makes
> sense to have these go in as-are. They really are meant for squashing.)
> 
> Martin
> 
> Martin Ågren (5):
>   t7503: use "&&" in "test_when_finished" rather than ";"
>   t7503: avoid touch when mtime doesn't matter
>   t7503: simplify file-juggling
>   t7503: don't create "actual_hooks" for later appending
>   t7503: test failing merge with both hooks available
> 
>  ...3-pre-commit-and-pre-merge-commit-hooks.sh | 84 +++++++++++--------
>  1 file changed, 50 insertions(+), 34 deletions(-)
> 
> -- 
> 2.23.0.rc0.30.g51cf315870

These all look good to me, thank you for the suggestions! I'll include
them in V4.

Re: [PATCH v3 3/4] git-merge: honor pre-merge-commit hook

[Josh Steadmon]

On 2019.08.02 11:45, Martin Ågren wrote:
> On Fri, 2 Aug 2019 at 00:20, Josh Steadmon <steadmon@google.com> wrote:
> 
> > diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
> > index 82cd573776..7c4c994858 100644
> > --- a/Documentation/githooks.txt
> > +++ b/Documentation/githooks.txt
> > @@ -103,6 +103,13 @@ The default 'pre-commit' hook, when enabled--and with the
> >  `hooks.allownonascii` config option unset or set to false--prevents
> >  the use of non-ASCII filenames.
> >
> > +pre-merge-commit
> > +~~~~~~~~~~~~~~~~
> > +
> > +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.
> > +
> 
> I'm not sure everyone understands what an "automatic merge commit" is.
> (Is it an automatic "merge commit", or an "automatic merge" commit? Or
> sort of both?) And I'm not sure exactly what to infer from the
> "equivalence". I happen to know that the statement about the default
> hook can only be half-carried over. And I'm not sure what to infer from
> "All the git commit hooks are invoked with the environment variable
> ...".
> 
> Is the below suggestion 1) correct, 2) readable?
> 
>   This hook is invoked by linkgit:git-merge[1], and can be bypassed
>   with the `--no-verify` option.  It takes no parameters, and is
>   invoked after the merge has been carried out successfully and before
>   obtaining the proposed commit log message to
>   make a commit.  Exiting with a non-zero status from this script
>   causes the `git merge` command to abort before creating a commit.
> 
>   The default 'pre-merge-commit' hook, when enabled, runs the
>   'pre-commit' hook, if the latter is enabled.
> 
>   This hook is invoked with the environment variable
>   `GIT_EDITOR=:` if the command will not bring up an editor
>   to modify the commit message.
> 
>   If the merge cannot be carried out automatically, the conflicts
>   need to be resolved and the result committed separately (see
>   linkgit:git-merge[1]). At that point, this hook will not be executed,
>   but the 'pre-commit' hook will, if it is enabled.
> 
> (If you use this or something like it, notice how this already mentions
> `--no-verify`...)

This looks good to me at first glance. I may reword a bit in V4, but
probably not much.

> > +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
> > +'
> 
> You got rid of loads of "> file" in patch 1/4, so it seems unfortunate
> to introduce a few here. ;-)

Thanks for the catch, will fix in V4.

[PATCH v4 0/4] pre-merge-commit hook

[Josh Steadmon]

This series adds a new pre-merge-commit hook, similar in usage to
pre-commit. It also improves hook testing in t7503, by verifying that
the correct hooks are run or bypassed as expected.

The original series was done by Michael J Gruber <git@grubix.eu>. I have
addressed the outstanding review comments, and noted my changes in the
commit messages in "[js: ...]" blocks.

Changes since V3:
* Applied several test style fixes suggested by Martin (thanks!).
* Clarified the documentation for pre-merge-commit hook.
* Fixed a few cases where testing that the merge hook did not run might
  erroneously succeed if we don't have any merge to actually perform.
* Simplified test cleanup by adding a new non-executable sample hook
  script.
* Added test cases for non-executable pre-merge-commit hooks.

Changes since V2:
* Renamed the hook from "pre-merge" to "pre-merge-commit".
* Added a new patch (1/4) to improve t7503 by verifying that the
  expected hooks are (or are not) run.
* Squashed test changes (from V2's patch 4/4) into patch 3/4.
  Modified the tests to follow the example set in patch 1/4.
* Reworded commit messages to match with the current state of certain
  flags, which changed in between V1 and V2 of this series.

Josh Steadmon (1):
  t7503: verify proper hook execution

Michael J Gruber (3):
  merge: do no-verify like commit
  git-merge: honor pre-merge-commit hook
  merge: --no-verify to bypass pre-merge-commit hook

 Documentation/git-merge.txt                   |   2 +-
 Documentation/githooks.txt                    |  22 ++
 Documentation/merge-options.txt               |   4 +
 builtin/merge.c                               |  18 +-
 ...3-pre-commit-and-pre-merge-commit-hooks.sh | 281 ++++++++++++++++++
 t/t7503-pre-commit-hook.sh                    | 139 ---------
 templates/hooks--pre-merge-commit.sample      |  13 +
 7 files changed, 336 insertions(+), 143 deletions(-)
 create mode 100755 t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
 delete mode 100755 t/t7503-pre-commit-hook.sh
 create mode 100755 templates/hooks--pre-merge-commit.sample

Range-diff against v3:
1:  f0476b2b1e ! 1:  5085729095 t7503: verify proper hook execution
    @@ Commit message
         write_script() and doing setup inside a test_expect_success block.
     
    +    Improved-by: Martin Ågren <martin.agren@gmail.com>
    +    Signed-off-by: Martin Ågren <martin.agren@gmail.com>
     
      ## t/t7503-pre-commit-hook.sh ##
     @@ t/t7503-pre-commit-hook.sh: test_description='pre-commit hook'
    @@ t/t7503-pre-commit-hook.sh: test_description='pre-commit hook'
     +	echo $0 >>actual_hooks
     +	exit 1
     +	EOF
    ++	write_script "$HOOKDIR/non-exec.sample" <<-\EOF &&
    ++	echo $0 >>actual_hooks
    ++	exit 1
    ++	EOF
    ++	chmod -x "$HOOKDIR/non-exec.sample" &&
     +	write_script "$HOOKDIR/require-prefix.sample" <<-\EOF &&
     +	echo $0 >>actual_hooks
     +	test $GIT_PREFIX = "success/"
    @@ t/t7503-pre-commit-hook.sh: test_description='pre-commit hook'
      
     -	echo "foo" > file &&
     +test_expect_success 'with no hook' '
    -+	test_when_finished "rm -f expected_hooks actual_hooks" &&
    -+	touch expected_hooks actual_hooks &&
    ++	test_when_finished "rm -f actual_hooks" &&
     +	echo "foo" >file &&
      	git add file &&
     -	git commit -m "first"
     -
     +	git commit -m "first" &&
    -+	test_cmp expected_hooks actual_hooks
    ++	test_path_is_missing actual_hooks
      '
      
      test_expect_success '--no-verify with no hook' '
     -
     -	echo "bar" > file &&
    -+	test_when_finished "rm -f expected_hooks actual_hooks" &&
    -+	touch expected_hooks actual_hooks &&
    ++	test_when_finished "rm -f actual_hooks" &&
     +	echo "bar" >file &&
      	git add file &&
     -	git commit --no-verify -m "bar"
     -
     +	git commit --no-verify -m "bar" &&
    -+	test_cmp expected_hooks actual_hooks
    ++	test_path_is_missing actual_hooks
      '
      
     -# now install hook that always succeeds
    @@ t/t7503-pre-commit-hook.sh: test_description='pre-commit hook'
     -	echo "more" >> file &&
     +	test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks" &&
     +	ln -s "success.sample" "$PRECOMMIT" &&
    -+	touch actual_hooks &&
     +	echo "$PRECOMMIT" >expected_hooks &&
     +	echo "more" >>file &&
      	git add file &&
    @@ t/t7503-pre-commit-hook.sh: test_description='pre-commit hook'
      test_expect_success '--no-verify with succeeding hook' '
     -
     -	echo "even more" >> file &&
    -+	test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks" &&
    ++	test_when_finished "rm -f \"$PRECOMMIT\" actual_hooks" &&
     +	ln -s "success.sample" "$PRECOMMIT" &&
    -+	touch expected_hooks actual_hooks &&
     +	echo "even more" >>file &&
      	git add file &&
     -	git commit --no-verify -m "even more"
     -
     +	git commit --no-verify -m "even more" &&
    -+	test_cmp expected_hooks actual_hooks
    ++	test_path_is_missing actual_hooks
      '
      
     -# now a hook that fails
    @@ t/t7503-pre-commit-hook.sh: test_description='pre-commit hook'
     -	echo "another" >> file &&
     +	test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks" &&
     +	ln -s "fail.sample" "$PRECOMMIT" &&
    -+	touch actual_hooks &&
     +	echo "$PRECOMMIT" >expected_hooks &&
     +	echo "another" >>file &&
      	git add file &&
    @@ t/t7503-pre-commit-hook.sh: test_description='pre-commit hook'
      test_expect_success '--no-verify with failing hook' '
     -
     -	echo "stuff" >> file &&
    -+	test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks" &&
    ++	test_when_finished "rm -f \"$PRECOMMIT\" actual_hooks" &&
     +	ln -s "fail.sample" "$PRECOMMIT" &&
    -+	touch expected_hooks actual_hooks &&
     +	echo "stuff" >>file &&
      	git add file &&
     -	git commit --no-verify -m "stuff"
     -
     +	git commit --no-verify -m "stuff" &&
    -+	test_cmp expected_hooks actual_hooks
    ++	test_path_is_missing actual_hooks
      '
      
     -chmod -x "$HOOK"
      test_expect_success POSIXPERM 'with non-executable hook' '
     -
     -	echo "content" >> file &&
    -+	test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks; chmod +x \"$HOOKDIR/fail.sample\"" &&
    -+	ln -s "fail.sample" "$PRECOMMIT" &&
    -+	chmod -x "$HOOKDIR/fail.sample" &&
    -+	touch expected_hooks actual_hooks &&
    ++	test_when_finished "rm -f \"$PRECOMMIT\" actual_hooks" &&
    ++	ln -s "non-exec.sample" "$PRECOMMIT" &&
     +	echo "content" >>file &&
      	git add file &&
     -	git commit -m "content"
     -
     +	git commit -m "content" &&
    -+	test_cmp expected_hooks actual_hooks
    ++	test_path_is_missing actual_hooks
      '
      
      test_expect_success POSIXPERM '--no-verify with non-executable hook' '
     -
     -	echo "more content" >> file &&
    -+	test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks; chmod +x \"$HOOKDIR/fail.sample\"" &&
    -+	ln -s "fail.sample" "$PRECOMMIT" &&
    -+	chmod -x "$HOOKDIR/fail.sample" &&
    -+	touch expected_hooks actual_hooks &&
    ++	test_when_finished "rm -f \"$PRECOMMIT\" actual_hooks" &&
    ++	ln -s "non-exec.sample" "$PRECOMMIT" &&
     +	echo "more content" >>file &&
      	git add file &&
     -	git commit --no-verify -m "more content"
     -
     +	git commit --no-verify -m "more content" &&
    -+	test_cmp expected_hooks actual_hooks
    ++	test_path_is_missing actual_hooks
      '
     -chmod +x "$HOOK"
     -
2:  89ddbf410f = 2:  3b701a5c41 merge: do no-verify like commit
3:  61b989ff16 ! 3:  9210421fbb git-merge: honor pre-merge-commit hook
    @@ Commit message
     
         [js: * renamed hook from "pre-merge" to "pre-merge-commit"
              * only discard the index if the hook is actually present
    +         * expanded githooks documentation entry
              * clarified that hook should write messages to stderr
              * squashed test changes from the original series' patch 4/4
              * modified tests to follow new pattern from this series' patch 1/4
    +         * added a test case for non-executable merge hooks
    +         * added a test case for failed merges
    +         * when testing that the merge hook did not run, make sure we
    +           actually have a merge to perform (by resetting the "side" branch
    +           to its original state).
              * reworded commit message
         ]
     
    +    Improved-by: Martin Ågren <martin.agren@gmail.com>
         Signed-off-by: Michael J Gruber <git@grubix.eu>
    +    Signed-off-by: Martin Ågren <martin.agren@gmail.com>
     
      ## Documentation/githooks.txt ##
    @@ Documentation/githooks.txt: The default 'pre-commit' hook, when enabled--and wit
     +pre-merge-commit
     +~~~~~~~~~~~~~~~~
     +
    -+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.
    ++This hook is invoked by linkgit:git-merge[1]. It takes no parameters, and is
    ++invoked after the merge has been carried out successfully and before
    ++obtaining the proposed commit log message to
    ++make a commit.  Exiting with a non-zero status from this script
    ++causes the `git merge` command to abort before creating a commit.
    ++
    ++The default 'pre-merge-commit' hook, when enabled, runs the
    ++'pre-commit' hook, if the latter is enabled.
    ++
    ++This hook is invoked with the environment variable
    ++`GIT_EDITOR=:` if the command will not bring up an editor
    ++to modify the commit message.
    ++
    ++If the merge cannot be carried out automatically, the conflicts
    ++need to be resolved and the result committed separately (see
    ++linkgit:git-merge[1]). At that point, this hook will not be executed,
    ++but the 'pre-commit' hook will, if it is enabled.
     +
      prepare-commit-msg
      ~~~~~~~~~~~~~~~~~~
    @@ t/t7503-pre-commit-and-pre-merge-commit-hooks.sh: test_expect_success 'sample sc
      '
      
     +test_expect_success 'root commit' '
    -+	echo "root" > file &&
    ++	echo "root" >file &&
     +	git add file &&
     +	git commit -m "zeroth" &&
     +	git checkout -b side &&
    -+	echo "foo" > foo &&
    ++	echo "foo" >foo &&
     +	git add foo &&
     +	git commit -m "make it non-ff" &&
    ++	git branch side-orig side &&
     +	git checkout master
     +'
    ++
    ++test_expect_success 'setup conflicting branches' '
    ++	test_when_finished "git checkout master" &&
    ++	git checkout -b conflicting-a master &&
    ++	echo a >conflicting &&
    ++	git add conflicting &&
    ++	git commit -m conflicting-a &&
    ++	git checkout -b conflicting-b master &&
    ++	echo b >conflicting &&
    ++	git add conflicting &&
    ++	git commit -m conflicting-b
    ++'
     +
      test_expect_success 'with no hook' '
    - 	test_when_finished "rm -f expected_hooks actual_hooks" &&
    - 	touch expected_hooks actual_hooks &&
    + 	test_when_finished "rm -f actual_hooks" &&
    + 	echo "foo" >file &&
     @@ t/t7503-pre-commit-and-pre-merge-commit-hooks.sh: test_expect_success 'with no hook' '
    - 	test_cmp expected_hooks actual_hooks
    + 	test_path_is_missing actual_hooks
      '
      
     +test_expect_success 'with no hook (merge)' '
    -+	test_when_finished "rm -f expected_hooks actual_hooks" &&
    -+	touch expected_hooks actual_hooks &&
    ++	test_when_finished "rm -f actual_hooks" &&
    ++	git branch -f side side-orig &&
     +	git checkout side &&
     +	git merge -m "merge master" master &&
     +	git checkout master &&
    -+	test_cmp expected_hooks actual_hooks
    ++	test_path_is_missing actual_hooks
     +'
     +
      test_expect_success '--no-verify with no hook' '
    - 	test_when_finished "rm -f expected_hooks actual_hooks" &&
    - 	touch expected_hooks actual_hooks &&
    + 	test_when_finished "rm -f actual_hooks" &&
    + 	echo "bar" >file &&
     @@ t/t7503-pre-commit-and-pre-merge-commit-hooks.sh: test_expect_success 'with succeeding hook' '
      	test_cmp expected_hooks actual_hooks
      '
    @@ t/t7503-pre-commit-and-pre-merge-commit-hooks.sh: test_expect_success 'with succ
     +test_expect_success 'with succeeding hook (merge)' '
     +	test_when_finished "rm -f \"$PREMERGE\" expected_hooks actual_hooks" &&
     +	ln -s "success.sample" "$PREMERGE" &&
    -+	touch actual_hooks &&
     +	echo "$PREMERGE" >expected_hooks &&
     +	git checkout side &&
     +	git merge -m "merge master" master &&
     +	git checkout master &&
     +	test_cmp expected_hooks actual_hooks
     +'
    ++
    ++test_expect_success 'automatic merge fails; both hooks are available' '
    ++	test_when_finished "rm -f \"$PREMERGE\" \"$PRECOMMIT\"" &&
    ++	test_when_finished "rm -f expected_hooks actual_hooks" &&
    ++	test_when_finished "git checkout master" &&
    ++	ln -s "success.sample" "$PREMERGE" &&
    ++	ln -s "success.sample" "$PRECOMMIT" &&
    ++
    ++	git checkout conflicting-a &&
    ++	test_must_fail git merge -m "merge conflicting-b" conflicting-b &&
    ++	test_path_is_missing actual_hooks &&
    ++
    ++	echo "$PRECOMMIT" >expected_hooks &&
    ++	echo a+b >conflicting &&
    ++	git add conflicting &&
    ++	git commit -m "resolve conflict" &&
    ++	test_cmp expected_hooks actual_hooks
    ++'
     +
      test_expect_success '--no-verify with succeeding hook' '
    - 	test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks" &&
    + 	test_when_finished "rm -f \"$PRECOMMIT\" actual_hooks" &&
      	ln -s "success.sample" "$PRECOMMIT" &&
     @@ t/t7503-pre-commit-and-pre-merge-commit-hooks.sh: test_expect_success '--no-verify with failing hook' '
    - 	test_cmp expected_hooks actual_hooks
    + 	test_path_is_missing actual_hooks
      '
      
     +test_expect_success 'with failing hook (merge)' '
     +	test_when_finished "rm -f \"$PREMERGE\" expected_hooks actual_hooks" &&
     +	ln -s "fail.sample" "$PREMERGE" &&
    -+	touch actual_hooks &&
     +	echo "$PREMERGE" >expected_hooks &&
     +	git checkout side &&
     +	test_must_fail git merge -m "merge master" master &&
    @@ t/t7503-pre-commit-and-pre-merge-commit-hooks.sh: test_expect_success '--no-veri
     +'
     +
      test_expect_success POSIXPERM 'with non-executable hook' '
    - 	test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks; chmod +x \"$HOOKDIR/fail.sample\"" &&
    - 	ln -s "fail.sample" "$PRECOMMIT" &&
    + 	test_when_finished "rm -f \"$PRECOMMIT\" actual_hooks" &&
    + 	ln -s "non-exec.sample" "$PRECOMMIT" &&
    +@@ t/t7503-pre-commit-and-pre-merge-commit-hooks.sh: test_expect_success POSIXPERM '--no-verify with non-executable hook' '
    + 	test_path_is_missing actual_hooks
    + '
    + 
    ++test_expect_success POSIXPERM 'with non-executable hook (merge)' '
    ++	test_when_finished "rm -f \"$PREMERGE\" actual_hooks" &&
    ++	ln -s "non-exec.sample" "$PREMERGE" &&
    ++	git branch -f side side-orig &&
    ++	git checkout side &&
    ++	git merge -m "merge master" master &&
    ++	git checkout master &&
    ++	test_path_is_missing actual_hooks
    ++'
    ++
    + test_expect_success 'with hook requiring GIT_PREFIX' '
    + 	test_when_finished "rm -rf \"$PRECOMMIT\" expected_hooks actual_hooks success" &&
    + 	ln -s "require-prefix.sample" "$PRECOMMIT" &&
     
      ## templates/hooks--pre-merge-commit.sample (new) ##
     @@
4:  45828c56fc ! 4:  96c54883d3 merge: --no-verify to bypass pre-merge-commit hook
    @@ Commit message
              * cleaned up trailing whitespace
              * squashed test changes from the original series' patch 4/4
              * modified tests to follow pattern from this series' patch 1/4
    +         * added a test case for --no-verify with non-executable hook
    +         * when testing that the merge hook did not run, make sure we
    +           actually have a merge to perform (by resetting the "side" branch
    +           to its original state).
    +
         ]
     
    +    Improved-by: Martin Ågren <martin.agren@gmail.com>
         Signed-off-by: Michael J Gruber <git@grubix.eu>
    +    Signed-off-by: Martin Ågren <martin.agren@gmail.com>
     
      ## Documentation/githooks.txt ##
    -@@ Documentation/githooks.txt: pre-merge-commit
    - 
    - 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.
    +@@ Documentation/githooks.txt: the use of non-ASCII filenames.
    + pre-merge-commit
    + ~~~~~~~~~~~~~~~~
      
    - prepare-commit-msg
    - ~~~~~~~~~~~~~~~~~~
    +-This hook is invoked by linkgit:git-merge[1]. It takes no parameters, and is
    ++This hook is invoked by linkgit:git-merge[1], and can be bypassed
    ++with the `--no-verify` option.  It takes no parameters, and is
    + invoked after the merge has been carried out successfully and before
    + obtaining the proposed commit log message to
    + make a commit.  Exiting with a non-zero status from this script
     
      ## builtin/merge.c ##
     @@ builtin/merge.c: static struct option builtin_merge_options[] = {
    @@ builtin/merge.c: static void prepare_to_commit(struct commit_list *remoteheads)
     
      ## t/t7503-pre-commit-and-pre-merge-commit-hooks.sh ##
     @@ t/t7503-pre-commit-and-pre-merge-commit-hooks.sh: test_expect_success '--no-verify with no hook' '
    - 	test_cmp expected_hooks actual_hooks
    + 	test_path_is_missing actual_hooks
      '
      
     +test_expect_success '--no-verify with no hook (merge)' '
    -+	test_when_finished "rm -f expected_hooks actual_hooks" &&
    -+	touch expected_hooks actual_hooks &&
    ++	test_when_finished "rm -f actual_hooks" &&
    ++	git branch -f side side-orig &&
     +	git checkout side &&
     +	git merge --no-verify -m "merge master" master &&
     +	git checkout master &&
    -+	test_cmp expected_hooks actual_hooks
    ++	test_path_is_missing actual_hooks
     +'
     +
      test_expect_success 'with succeeding hook' '
      	test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks" &&
      	ln -s "success.sample" "$PRECOMMIT" &&
     @@ t/t7503-pre-commit-and-pre-merge-commit-hooks.sh: test_expect_success '--no-verify with succeeding hook' '
    - 	test_cmp expected_hooks actual_hooks
    + 	test_path_is_missing actual_hooks
      '
      
     +test_expect_success '--no-verify with succeeding hook (merge)' '
    -+	test_when_finished "rm -f \"$PREMERGE\" expected_hooks actual_hooks" &&
    ++	test_when_finished "rm -f \"$PREMERGE\" actual_hooks" &&
     +	ln -s "success.sample" "$PREMERGE" &&
    -+	touch expected_hooks actual_hooks &&
    ++	git branch -f side side-orig &&
     +	git checkout side &&
     +	git merge --no-verify -m "merge master" master &&
     +	git checkout master &&
    -+	test_cmp expected_hooks actual_hooks
    ++	test_path_is_missing actual_hooks
     +'
     +
      test_expect_success 'with failing hook' '
    @@ t/t7503-pre-commit-and-pre-merge-commit-hooks.sh: test_expect_success 'with fail
      '
      
     +test_expect_success '--no-verify with failing hook (merge)' '
    -+	test_when_finished "rm -f \"$PREMERGE\" expected_hooks actual_hooks" &&
    ++	test_when_finished "rm -f \"$PREMERGE\" actual_hooks" &&
     +	ln -s "fail.sample" "$PREMERGE" &&
    -+	touch expected_hooks actual_hooks &&
    ++	git branch -f side side-orig &&
     +	git checkout side &&
     +	git merge --no-verify -m "merge master" master &&
     +	git checkout master &&
    -+	test_cmp expected_hooks actual_hooks
    ++	test_path_is_missing actual_hooks
     +'
     +
      test_expect_success POSIXPERM 'with non-executable hook' '
    - 	test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks; chmod +x \"$HOOKDIR/fail.sample\"" &&
    - 	ln -s "fail.sample" "$PRECOMMIT" &&
    + 	test_when_finished "rm -f \"$PRECOMMIT\" actual_hooks" &&
    + 	ln -s "non-exec.sample" "$PRECOMMIT" &&
    +@@ t/t7503-pre-commit-and-pre-merge-commit-hooks.sh: test_expect_success POSIXPERM 'with non-executable hook (merge)' '
    + 	test_path_is_missing actual_hooks
    + '
    + 
    ++test_expect_success POSIXPERM '--no-verify with non-executable hook (merge)' '
    ++	test_when_finished "rm -f \"$PREMERGE\" actual_hooks" &&
    ++	ln -s "non-exec.sample" "$PREMERGE" &&
    ++	git branch -f side side-orig &&
    ++	git checkout side &&
    ++	git merge --no-verify -m "merge master" master &&
    ++	git checkout master &&
    ++	test_path_is_missing actual_hooks
    ++'
    ++
    + test_expect_success 'with hook requiring GIT_PREFIX' '
    + 	test_when_finished "rm -rf \"$PRECOMMIT\" expected_hooks actual_hooks success" &&
    + 	ln -s "require-prefix.sample" "$PRECOMMIT" &&
-- 
2.22.0.770.g0f2c4a37fd-goog

[PATCH v4 1/4] t7503: verify proper hook execution

[Josh Steadmon]

t7503 did not verify that the expected hooks actually ran during
testing. Fix that by making the hook scripts write their $0 into a file
so that we can compare actual execution vs. expected execution.

While we're at it, do some test style cleanups, such as using
write_script() and doing setup inside a test_expect_success block.

Improved-by: Martin Ågren <martin.agren@gmail.com>
Signed-off-by: Martin Ågren <martin.agren@gmail.com>
Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 t/t7503-pre-commit-hook.sh | 157 +++++++++++++++++++++----------------
 1 file changed, 89 insertions(+), 68 deletions(-)

diff --git a/t/t7503-pre-commit-hook.sh b/t/t7503-pre-commit-hook.sh
index 984889b39d..a71ec31222 100755
--- a/t/t7503-pre-commit-hook.sh
+++ b/t/t7503-pre-commit-hook.sh
@@ -4,124 +4,144 @@ test_description='pre-commit hook'
 
 . ./test-lib.sh
 
-test_expect_success 'with no hook' '
+HOOKDIR="$(git rev-parse --git-dir)/hooks"
+PRECOMMIT="$HOOKDIR/pre-commit"
+
+# Prepare sample scripts that write their $0 to actual_hooks
+test_expect_success 'sample script setup' '
+	mkdir -p "$HOOKDIR" &&
+	write_script "$HOOKDIR/success.sample" <<-\EOF &&
+	echo $0 >>actual_hooks
+	exit 0
+	EOF
+	write_script "$HOOKDIR/fail.sample" <<-\EOF &&
+	echo $0 >>actual_hooks
+	exit 1
+	EOF
+	write_script "$HOOKDIR/non-exec.sample" <<-\EOF &&
+	echo $0 >>actual_hooks
+	exit 1
+	EOF
+	chmod -x "$HOOKDIR/non-exec.sample" &&
+	write_script "$HOOKDIR/require-prefix.sample" <<-\EOF &&
+	echo $0 >>actual_hooks
+	test $GIT_PREFIX = "success/"
+	EOF
+	write_script "$HOOKDIR/check-author.sample" <<-\EOF
+	echo $0 >>actual_hooks
+	test "$GIT_AUTHOR_NAME" = "New Author" &&
+	test "$GIT_AUTHOR_EMAIL" = "newauthor@example.com"
+	EOF
+'
 
-	echo "foo" > file &&
+test_expect_success 'with no hook' '
+	test_when_finished "rm -f actual_hooks" &&
+	echo "foo" >file &&
 	git add file &&
-	git commit -m "first"
-
+	git commit -m "first" &&
+	test_path_is_missing actual_hooks
 '
 
 test_expect_success '--no-verify with no hook' '
-
-	echo "bar" > file &&
+	test_when_finished "rm -f actual_hooks" &&
+	echo "bar" >file &&
 	git add file &&
-	git commit --no-verify -m "bar"
-
+	git commit --no-verify -m "bar" &&
+	test_path_is_missing actual_hooks
 '
 
-# 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 &&
+	test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks" &&
+	ln -s "success.sample" "$PRECOMMIT" &&
+	echo "$PRECOMMIT" >expected_hooks &&
+	echo "more" >>file &&
 	git add file &&
-	git commit -m "more"
-
+	git commit -m "more" &&
+	test_cmp expected_hooks actual_hooks
 '
 
 test_expect_success '--no-verify with succeeding hook' '
-
-	echo "even more" >> file &&
+	test_when_finished "rm -f \"$PRECOMMIT\" actual_hooks" &&
+	ln -s "success.sample" "$PRECOMMIT" &&
+	echo "even more" >>file &&
 	git add file &&
-	git commit --no-verify -m "even more"
-
+	git commit --no-verify -m "even more" &&
+	test_path_is_missing actual_hooks
 '
 
-# now a hook that fails
-cat > "$HOOK" <<EOF
-#!/bin/sh
-exit 1
-EOF
-
 test_expect_success 'with failing hook' '
-
-	echo "another" >> file &&
+	test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks" &&
+	ln -s "fail.sample" "$PRECOMMIT" &&
+	echo "$PRECOMMIT" >expected_hooks &&
+	echo "another" >>file &&
 	git add file &&
-	test_must_fail git commit -m "another"
-
+	test_must_fail git commit -m "another" &&
+	test_cmp expected_hooks actual_hooks
 '
 
 test_expect_success '--no-verify with failing hook' '
-
-	echo "stuff" >> file &&
+	test_when_finished "rm -f \"$PRECOMMIT\" actual_hooks" &&
+	ln -s "fail.sample" "$PRECOMMIT" &&
+	echo "stuff" >>file &&
 	git add file &&
-	git commit --no-verify -m "stuff"
-
+	git commit --no-verify -m "stuff" &&
+	test_path_is_missing actual_hooks
 '
 
-chmod -x "$HOOK"
 test_expect_success POSIXPERM 'with non-executable hook' '
-
-	echo "content" >> file &&
+	test_when_finished "rm -f \"$PRECOMMIT\" actual_hooks" &&
+	ln -s "non-exec.sample" "$PRECOMMIT" &&
+	echo "content" >>file &&
 	git add file &&
-	git commit -m "content"
-
+	git commit -m "content" &&
+	test_path_is_missing actual_hooks
 '
 
 test_expect_success POSIXPERM '--no-verify with non-executable hook' '
-
-	echo "more content" >> file &&
+	test_when_finished "rm -f \"$PRECOMMIT\" actual_hooks" &&
+	ln -s "non-exec.sample" "$PRECOMMIT" &&
+	echo "more content" >>file &&
 	git add file &&
-	git commit --no-verify -m "more content"
-
+	git commit --no-verify -m "more content" &&
+	test_path_is_missing actual_hooks
 '
-chmod +x "$HOOK"
-
-# a hook that checks $GIT_PREFIX and succeeds inside the
-# success/ subdirectory only
-cat > "$HOOK" <<EOF
-#!/bin/sh
-test \$GIT_PREFIX = success/
-EOF
 
 test_expect_success 'with hook requiring GIT_PREFIX' '
-
-	echo "more content" >> file &&
+	test_when_finished "rm -rf \"$PRECOMMIT\" expected_hooks actual_hooks success" &&
+	ln -s "require-prefix.sample" "$PRECOMMIT" &&
+	echo "$PRECOMMIT" >expected_hooks &&
+	echo "more content" >>file &&
 	git add file &&
 	mkdir success &&
 	(
 		cd success &&
 		git commit -m "hook requires GIT_PREFIX = success/"
 	) &&
-	rmdir success
+	test_cmp expected_hooks actual_hooks
 '
 
 test_expect_success 'with failing hook requiring GIT_PREFIX' '
-
-	echo "more content" >> file &&
+	test_when_finished "rm -rf \"$PRECOMMIT\" expected_hooks actual_hooks fail" &&
+	ln -s "require-prefix.sample" "$PRECOMMIT" &&
+	echo "$PRECOMMIT" >expected_hooks &&
+	echo "more content" >>file &&
 	git add file &&
 	mkdir fail &&
 	(
 		cd fail &&
 		test_must_fail git commit -m "hook must fail"
 	) &&
-	rmdir fail &&
-	git checkout -- file
+	git checkout -- file &&
+	test_cmp expected_hooks actual_hooks
 '
 
 test_expect_success 'check the author in hook' '
-	write_script "$HOOK" <<-\EOF &&
-	test "$GIT_AUTHOR_NAME" = "New Author" &&
-	test "$GIT_AUTHOR_EMAIL" = "newauthor@example.com"
+	test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks" &&
+	ln -s "check-author.sample" "$PRECOMMIT" &&
+	cat >expected_hooks <<-EOF &&
+	$PRECOMMIT
+	$PRECOMMIT
+	$PRECOMMIT
 	EOF
 	test_must_fail git commit --allow-empty -m "by a.u.thor" &&
 	(
@@ -133,7 +153,8 @@ test_expect_success 'check the author in hook' '
 	) &&
 	git commit --author="New Author <newauthor@example.com>" \
 		--allow-empty -m "by new.author via command line" &&
-	git show -s
+	git show -s &&
+	test_cmp expected_hooks actual_hooks
 '
 
 test_done
-- 
2.22.0.770.g0f2c4a37fd-goog

[PATCH v4 2/4] merge: do no-verify like commit

[Josh Steadmon]

From: Michael J Gruber <git@grubix.eu>

f8b863598c ("builtin/merge: honor commit-msg hook for merges", 2017-09-07)
introduced the no-verify flag to merge for bypassing the commit-msg
hook, though in a different way from the implementation in commit.c.

Change the implementation in merge.c to be the same as in commit.c so
that both do the same in the same way. This also changes the output of
"git merge --help" to be more clear that the hook return code is
respected by default.

[js: * reworded commit message
     * squashed documentation changes from original series' patch 3/4
]

Signed-off-by: Michael J Gruber <git@grubix.eu>
Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 Documentation/git-merge.txt     | 2 +-
 Documentation/merge-options.txt | 4 ++++
 builtin/merge.c                 | 6 +++---
 3 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
index 01fd52dc70..092529c619 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>] [-S[<keyid>]]
+	[--no-verify] [-s <strategy>] [-X <strategy-option>] [-S[<keyid>]]
 	[--[no-]allow-unrelated-histories]
 	[--[no-]rerere-autoupdate] [-m <msg>] [-F <file>] [<commit>...]
 'git merge' (--continue | --abort | --quit)
diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
index 79a00d2a4a..d6a9f4b96f 100644
--- a/Documentation/merge-options.txt
+++ b/Documentation/merge-options.txt
@@ -105,6 +105,10 @@ option can be used to override --squash.
 +
 With --squash, --commit is not allowed, and will fail.
 
+--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 e2ccbc44e2..4425a7a12e 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -81,7 +81,7 @@ static int show_progress = -1;
 static int default_to_upstream = 1;
 static int signoff;
 static const char *sign_commit;
-static int verify_msg = 1;
+static int no_verify;
 
 static struct strategy all_strategy[] = {
 	{ "recursive",  DEFAULT_TWOHEAD | NO_TRIVIAL },
@@ -287,7 +287,7 @@ static struct option builtin_merge_options[] = {
 	  N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" },
 	OPT_BOOL(0, "overwrite-ignore", &overwrite_ignore, N_("update ignored files (default)")),
 	OPT_BOOL(0, "signoff", &signoff, N_("add Signed-off-by:")),
-	OPT_BOOL(0, "verify", &verify_msg, N_("verify commit-msg hook")),
+	OPT_BOOL(0, "no-verify", &no_verify, N_("bypass commit-msg hook")),
 	OPT_END()
 };
 
@@ -842,7 +842,7 @@ static void prepare_to_commit(struct commit_list *remoteheads)
 			abort_commit(remoteheads, NULL);
 	}
 
-	if (verify_msg && run_commit_hook(0 < option_edit, get_index_file(),
+	if (!no_verify && run_commit_hook(0 < option_edit, get_index_file(),
 					  "commit-msg",
 					  git_path_merge_msg(the_repository), NULL))
 		abort_commit(remoteheads, NULL);
-- 
2.22.0.770.g0f2c4a37fd-goog

[PATCH v4 3/4] git-merge: honor pre-merge-commit hook

[Josh Steadmon]

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

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-commit 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).

[js: * renamed hook from "pre-merge" to "pre-merge-commit"
     * only discard the index if the hook is actually present
     * expanded githooks documentation entry
     * clarified that hook should write messages to stderr
     * squashed test changes from the original series' patch 4/4
     * modified tests to follow new pattern from this series' patch 1/4
     * added a test case for non-executable merge hooks
     * added a test case for failed merges
     * when testing that the merge hook did not run, make sure we
       actually have a merge to perform (by resetting the "side" branch
       to its original state).
     * reworded commit message
]

Improved-by: Martin Ågren <martin.agren@gmail.com>
Signed-off-by: Michael J Gruber <git@grubix.eu>
Signed-off-by: Martin Ågren <martin.agren@gmail.com>
Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 Documentation/githooks.txt                    | 21 +++++
 builtin/merge.c                               | 12 +++
 ...-pre-commit-and-pre-merge-commit-hooks.sh} | 84 ++++++++++++++++++-
 templates/hooks--pre-merge-commit.sample      | 13 +++
 4 files changed, 129 insertions(+), 1 deletion(-)
 rename t/{t7503-pre-commit-hook.sh => t7503-pre-commit-and-pre-merge-commit-hooks.sh} (63%)
 create mode 100755 templates/hooks--pre-merge-commit.sample

diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index 82cd573776..d9da474fb0 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -103,6 +103,27 @@ The default 'pre-commit' hook, when enabled--and with the
 `hooks.allownonascii` config option unset or set to false--prevents
 the use of non-ASCII filenames.
 
+pre-merge-commit
+~~~~~~~~~~~~~~~~
+
+This hook is invoked by linkgit:git-merge[1]. It takes no parameters, and is
+invoked after the merge has been carried out successfully and before
+obtaining the proposed commit log message to
+make a commit.  Exiting with a non-zero status from this script
+causes the `git merge` command to abort before creating a commit.
+
+The default 'pre-merge-commit' hook, when enabled, runs the
+'pre-commit' hook, if the latter is enabled.
+
+This hook is invoked with the environment variable
+`GIT_EDITOR=:` if the command will not bring up an editor
+to modify the commit message.
+
+If the merge cannot be carried out automatically, the conflicts
+need to be resolved and the result committed separately (see
+linkgit:git-merge[1]). At that point, this hook will not be executed,
+but the 'pre-commit' hook will, if it is enabled.
+
 prepare-commit-msg
 ~~~~~~~~~~~~~~~~~~
 
diff --git a/builtin/merge.c b/builtin/merge.c
index 4425a7a12e..bf0ae68c40 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -816,6 +816,18 @@ static void write_merge_heads(struct commit_list *);
 static void prepare_to_commit(struct commit_list *remoteheads)
 {
 	struct strbuf msg = STRBUF_INIT;
+	const char *index_file = get_index_file();
+
+	if (run_commit_hook(0 < option_edit, index_file, "pre-merge-commit", NULL))
+		abort_commit(remoteheads, NULL);
+	/*
+	 * Re-read the index as pre-merge-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.
+	 */
+	if (find_hook("pre-merge-commit"))
+		discard_cache();
+	read_cache_from(index_file);
 	strbuf_addbuf(&msg, &merge_msg);
 	if (squash)
 		BUG("the control must not reach here under --squash");
diff --git a/t/t7503-pre-commit-hook.sh b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
similarity index 63%
rename from t/t7503-pre-commit-hook.sh
rename to t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
index a71ec31222..0700604f03 100755
--- a/t/t7503-pre-commit-hook.sh
+++ b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
@@ -1,11 +1,12 @@
 #!/bin/sh
 
-test_description='pre-commit hook'
+test_description='pre-commit and pre-merge-commit hooks'
 
 . ./test-lib.sh
 
 HOOKDIR="$(git rev-parse --git-dir)/hooks"
 PRECOMMIT="$HOOKDIR/pre-commit"
+PREMERGE="$HOOKDIR/pre-merge-commit"
 
 # Prepare sample scripts that write their $0 to actual_hooks
 test_expect_success 'sample script setup' '
@@ -34,6 +35,30 @@ test_expect_success 'sample script setup' '
 	EOF
 '
 
+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 branch side-orig side &&
+	git checkout master
+'
+
+test_expect_success 'setup conflicting branches' '
+	test_when_finished "git checkout master" &&
+	git checkout -b conflicting-a master &&
+	echo a >conflicting &&
+	git add conflicting &&
+	git commit -m conflicting-a &&
+	git checkout -b conflicting-b master &&
+	echo b >conflicting &&
+	git add conflicting &&
+	git commit -m conflicting-b
+'
+
 test_expect_success 'with no hook' '
 	test_when_finished "rm -f actual_hooks" &&
 	echo "foo" >file &&
@@ -42,6 +67,15 @@ test_expect_success 'with no hook' '
 	test_path_is_missing actual_hooks
 '
 
+test_expect_success 'with no hook (merge)' '
+	test_when_finished "rm -f actual_hooks" &&
+	git branch -f side side-orig &&
+	git checkout side &&
+	git merge -m "merge master" master &&
+	git checkout master &&
+	test_path_is_missing actual_hooks
+'
+
 test_expect_success '--no-verify with no hook' '
 	test_when_finished "rm -f actual_hooks" &&
 	echo "bar" >file &&
@@ -60,6 +94,34 @@ test_expect_success 'with succeeding hook' '
 	test_cmp expected_hooks actual_hooks
 '
 
+test_expect_success 'with succeeding hook (merge)' '
+	test_when_finished "rm -f \"$PREMERGE\" expected_hooks actual_hooks" &&
+	ln -s "success.sample" "$PREMERGE" &&
+	echo "$PREMERGE" >expected_hooks &&
+	git checkout side &&
+	git merge -m "merge master" master &&
+	git checkout master &&
+	test_cmp expected_hooks actual_hooks
+'
+
+test_expect_success 'automatic merge fails; both hooks are available' '
+	test_when_finished "rm -f \"$PREMERGE\" \"$PRECOMMIT\"" &&
+	test_when_finished "rm -f expected_hooks actual_hooks" &&
+	test_when_finished "git checkout master" &&
+	ln -s "success.sample" "$PREMERGE" &&
+	ln -s "success.sample" "$PRECOMMIT" &&
+
+	git checkout conflicting-a &&
+	test_must_fail git merge -m "merge conflicting-b" conflicting-b &&
+	test_path_is_missing actual_hooks &&
+
+	echo "$PRECOMMIT" >expected_hooks &&
+	echo a+b >conflicting &&
+	git add conflicting &&
+	git commit -m "resolve conflict" &&
+	test_cmp expected_hooks actual_hooks
+'
+
 test_expect_success '--no-verify with succeeding hook' '
 	test_when_finished "rm -f \"$PRECOMMIT\" actual_hooks" &&
 	ln -s "success.sample" "$PRECOMMIT" &&
@@ -88,6 +150,16 @@ test_expect_success '--no-verify with failing hook' '
 	test_path_is_missing actual_hooks
 '
 
+test_expect_success 'with failing hook (merge)' '
+	test_when_finished "rm -f \"$PREMERGE\" expected_hooks actual_hooks" &&
+	ln -s "fail.sample" "$PREMERGE" &&
+	echo "$PREMERGE" >expected_hooks &&
+	git checkout side &&
+	test_must_fail git merge -m "merge master" master &&
+	git checkout master &&
+	test_cmp expected_hooks actual_hooks
+'
+
 test_expect_success POSIXPERM 'with non-executable hook' '
 	test_when_finished "rm -f \"$PRECOMMIT\" actual_hooks" &&
 	ln -s "non-exec.sample" "$PRECOMMIT" &&
@@ -106,6 +178,16 @@ test_expect_success POSIXPERM '--no-verify with non-executable hook' '
 	test_path_is_missing actual_hooks
 '
 
+test_expect_success POSIXPERM 'with non-executable hook (merge)' '
+	test_when_finished "rm -f \"$PREMERGE\" actual_hooks" &&
+	ln -s "non-exec.sample" "$PREMERGE" &&
+	git branch -f side side-orig &&
+	git checkout side &&
+	git merge -m "merge master" master &&
+	git checkout master &&
+	test_path_is_missing actual_hooks
+'
+
 test_expect_success 'with hook requiring GIT_PREFIX' '
 	test_when_finished "rm -rf \"$PRECOMMIT\" expected_hooks actual_hooks success" &&
 	ln -s "require-prefix.sample" "$PRECOMMIT" &&
diff --git a/templates/hooks--pre-merge-commit.sample b/templates/hooks--pre-merge-commit.sample
new file mode 100755
index 0000000000..399eab1924
--- /dev/null
+++ b/templates/hooks--pre-merge-commit.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 to
+# stderr if it wants to stop the merge commit.
+#
+# To enable this hook, rename this file to "pre-merge-commit".
+
+. git-sh-setup
+test -x "$GIT_DIR/hooks/pre-commit" &&
+        exec "$GIT_DIR/hooks/pre-commit"
+:
-- 
2.22.0.770.g0f2c4a37fd-goog

[PATCH v4 4/4] merge: --no-verify to bypass pre-merge-commit hook

[Josh Steadmon]

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

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

[js: * reworded commit message to reflect current state of --no-stat flag
       and new hook name
     * fixed flag documentation to reflect new hook name
     * cleaned up trailing whitespace
     * squashed test changes from the original series' patch 4/4
     * modified tests to follow pattern from this series' patch 1/4
     * added a test case for --no-verify with non-executable hook
     * when testing that the merge hook did not run, make sure we
       actually have a merge to perform (by resetting the "side" branch
       to its original state).

]

Improved-by: Martin Ågren <martin.agren@gmail.com>
Signed-off-by: Michael J Gruber <git@grubix.eu>
Signed-off-by: Martin Ågren <martin.agren@gmail.com>
Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 Documentation/githooks.txt                    |  3 +-
 builtin/merge.c                               |  4 +-
 ...3-pre-commit-and-pre-merge-commit-hooks.sh | 39 +++++++++++++++++++
 3 files changed, 43 insertions(+), 3 deletions(-)

diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index d9da474fb0..57d6e2b98d 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -106,7 +106,8 @@ the use of non-ASCII filenames.
 pre-merge-commit
 ~~~~~~~~~~~~~~~~
 
-This hook is invoked by linkgit:git-merge[1]. It takes no parameters, and is
+This hook is invoked by linkgit:git-merge[1], and can be bypassed
+with the `--no-verify` option.  It takes no parameters, and is
 invoked after the merge has been carried out successfully and before
 obtaining the proposed commit log message to
 make a commit.  Exiting with a non-zero status from this script
diff --git a/builtin/merge.c b/builtin/merge.c
index bf0ae68c40..c9746e37b8 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -287,7 +287,7 @@ static struct option builtin_merge_options[] = {
 	  N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" },
 	OPT_BOOL(0, "overwrite-ignore", &overwrite_ignore, N_("update ignored files (default)")),
 	OPT_BOOL(0, "signoff", &signoff, N_("add Signed-off-by:")),
-	OPT_BOOL(0, "no-verify", &no_verify, N_("bypass commit-msg hook")),
+	OPT_BOOL(0, "no-verify", &no_verify, N_("bypass pre-merge-commit and commit-msg hooks")),
 	OPT_END()
 };
 
@@ -818,7 +818,7 @@ static void prepare_to_commit(struct commit_list *remoteheads)
 	struct strbuf msg = STRBUF_INIT;
 	const char *index_file = get_index_file();
 
-	if (run_commit_hook(0 < option_edit, index_file, "pre-merge-commit", NULL))
+	if (!no_verify && run_commit_hook(0 < option_edit, index_file, "pre-merge-commit", NULL))
 		abort_commit(remoteheads, NULL);
 	/*
 	 * Re-read the index as pre-merge-commit hook could have updated it,
diff --git a/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
index 0700604f03..d900c3a696 100755
--- a/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
+++ b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
@@ -84,6 +84,15 @@ test_expect_success '--no-verify with no hook' '
 	test_path_is_missing actual_hooks
 '
 
+test_expect_success '--no-verify with no hook (merge)' '
+	test_when_finished "rm -f actual_hooks" &&
+	git branch -f side side-orig &&
+	git checkout side &&
+	git merge --no-verify -m "merge master" master &&
+	git checkout master &&
+	test_path_is_missing actual_hooks
+'
+
 test_expect_success 'with succeeding hook' '
 	test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks" &&
 	ln -s "success.sample" "$PRECOMMIT" &&
@@ -131,6 +140,16 @@ test_expect_success '--no-verify with succeeding hook' '
 	test_path_is_missing actual_hooks
 '
 
+test_expect_success '--no-verify with succeeding hook (merge)' '
+	test_when_finished "rm -f \"$PREMERGE\" actual_hooks" &&
+	ln -s "success.sample" "$PREMERGE" &&
+	git branch -f side side-orig &&
+	git checkout side &&
+	git merge --no-verify -m "merge master" master &&
+	git checkout master &&
+	test_path_is_missing actual_hooks
+'
+
 test_expect_success 'with failing hook' '
 	test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks" &&
 	ln -s "fail.sample" "$PRECOMMIT" &&
@@ -160,6 +179,16 @@ test_expect_success 'with failing hook (merge)' '
 	test_cmp expected_hooks actual_hooks
 '
 
+test_expect_success '--no-verify with failing hook (merge)' '
+	test_when_finished "rm -f \"$PREMERGE\" actual_hooks" &&
+	ln -s "fail.sample" "$PREMERGE" &&
+	git branch -f side side-orig &&
+	git checkout side &&
+	git merge --no-verify -m "merge master" master &&
+	git checkout master &&
+	test_path_is_missing actual_hooks
+'
+
 test_expect_success POSIXPERM 'with non-executable hook' '
 	test_when_finished "rm -f \"$PRECOMMIT\" actual_hooks" &&
 	ln -s "non-exec.sample" "$PRECOMMIT" &&
@@ -188,6 +217,16 @@ test_expect_success POSIXPERM 'with non-executable hook (merge)' '
 	test_path_is_missing actual_hooks
 '
 
+test_expect_success POSIXPERM '--no-verify with non-executable hook (merge)' '
+	test_when_finished "rm -f \"$PREMERGE\" actual_hooks" &&
+	ln -s "non-exec.sample" "$PREMERGE" &&
+	git branch -f side side-orig &&
+	git checkout side &&
+	git merge --no-verify -m "merge master" master &&
+	git checkout master &&
+	test_path_is_missing actual_hooks
+'
+
 test_expect_success 'with hook requiring GIT_PREFIX' '
 	test_when_finished "rm -rf \"$PRECOMMIT\" expected_hooks actual_hooks success" &&
 	ln -s "require-prefix.sample" "$PRECOMMIT" &&
-- 
2.22.0.770.g0f2c4a37fd-goog

Re: [PATCH v4 1/4] t7503: verify proper hook execution

[Junio C Hamano]

Josh Steadmon <steadmon@google.com> writes:

> t7503 did not verify that the expected hooks actually ran during
> testing. Fix that by making the hook scripts write their $0 into a file
> so that we can compare actual execution vs. expected execution.
>
> While we're at it, do some test style cleanups, such as using
> write_script() and doing setup inside a test_expect_success block.
> ...

The result mostly looks more pleasant compared to the original,
modulo a few nits.

>  test_expect_success 'with succeeding hook' '
> -
> -	echo "more" >> file &&
> +	test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks" &&
> +	ln -s "success.sample" "$PRECOMMIT" &&

Not all systems that want to run Git are capable of symbolic links.
We should be able to do a copy here, perhaps?  Just the most basic
form of "cp" without even "-p" would be sufficient as the target is
expected to be missing (i.e. "cp" or "ln -s" would be creating the
"$PRECOMMIT"), right?

Re: [PATCH v4 1/4] t7503: verify proper hook execution

[Josh Steadmon]

On 2019.08.06 11:14, Junio C Hamano wrote:
> Josh Steadmon <steadmon@google.com> writes:
> 
> > t7503 did not verify that the expected hooks actually ran during
> > testing. Fix that by making the hook scripts write their $0 into a file
> > so that we can compare actual execution vs. expected execution.
> >
> > While we're at it, do some test style cleanups, such as using
> > write_script() and doing setup inside a test_expect_success block.
> > ...
> 
> The result mostly looks more pleasant compared to the original,
> modulo a few nits.
> 
> >  test_expect_success 'with succeeding hook' '
> > -
> > -	echo "more" >> file &&
> > +	test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks" &&
> > +	ln -s "success.sample" "$PRECOMMIT" &&
> 
> Not all systems that want to run Git are capable of symbolic links.
> We should be able to do a copy here, perhaps?  Just the most basic
> form of "cp" without even "-p" would be sufficient as the target is
> expected to be missing (i.e. "cp" or "ln -s" would be creating the
> "$PRECOMMIT"), right?

Yeah, "cp" works here. Fixed in V5.

[PATCH v5 0/4] pre-merge-commit hook

[Josh Steadmon]

This series adds a new pre-merge-commit hook, similar in usage to
pre-commit. It also improves hook testing in t7503, by verifying that
the correct hooks are run or bypassed as expected.

The original series was done by Michael J Gruber <git@grubix.eu>. I have
addressed the outstanding review comments, and noted my changes in the
commit messages in "[js: ...]" blocks.

Changes since V4:
* Used "cp" instead of "ln -s" in test hook setup to make tests more
  portable.

Changes since V3:
* Applied several test style fixes suggested by Martin (thanks!).
* Clarified the documentation for pre-merge-commit hook.
* Fixed a few cases where testing that the merge hook did not run might
  erroneously succeed if we don't have any merge to actually perform.
* Simplified test cleanup by adding a new non-executable sample hook
  script.
* Added test cases for non-executable pre-merge-commit hooks.

Changes since V2:
* Renamed the hook from "pre-merge" to "pre-merge-commit".
* Added a new patch (1/4) to improve t7503 by verifying that the
  expected hooks are (or are not) run.
* Squashed test changes (from V2's patch 4/4) into patch 3/4.
  Modified the tests to follow the example set in patch 1/4.
* Reworded commit messages to match with the current state of certain
  flags, which changed in between V1 and V2 of this series.

Josh Steadmon (1):
  t7503: verify proper hook execution

Michael J Gruber (3):
  merge: do no-verify like commit
  git-merge: honor pre-merge-commit hook
  merge: --no-verify to bypass pre-merge-commit hook

 Documentation/git-merge.txt                   |   2 +-
 Documentation/githooks.txt                    |  22 ++
 Documentation/merge-options.txt               |   4 +
 builtin/merge.c                               |  18 +-
 ...3-pre-commit-and-pre-merge-commit-hooks.sh | 281 ++++++++++++++++++
 t/t7503-pre-commit-hook.sh                    | 139 ---------
 templates/hooks--pre-merge-commit.sample      |  13 +
 7 files changed, 336 insertions(+), 143 deletions(-)
 create mode 100755 t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
 delete mode 100755 t/t7503-pre-commit-hook.sh
 create mode 100755 templates/hooks--pre-merge-commit.sample

Range-diff against v4:
1:  5085729095 ! 1:  60bbbbf9e0 t7503: verify proper hook execution
    @@ t/t7503-pre-commit-hook.sh: test_description='pre-commit hook'
     -
     -	echo "more" >> file &&
     +	test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks" &&
    -+	ln -s "success.sample" "$PRECOMMIT" &&
    ++	cp "$HOOKDIR/success.sample" "$PRECOMMIT" &&
     +	echo "$PRECOMMIT" >expected_hooks &&
     +	echo "more" >>file &&
      	git add file &&
    @@ t/t7503-pre-commit-hook.sh: test_description='pre-commit hook'
     -
     -	echo "even more" >> file &&
     +	test_when_finished "rm -f \"$PRECOMMIT\" actual_hooks" &&
    -+	ln -s "success.sample" "$PRECOMMIT" &&
    ++	cp "$HOOKDIR/success.sample" "$PRECOMMIT" &&
     +	echo "even more" >>file &&
      	git add file &&
     -	git commit --no-verify -m "even more"
    @@ t/t7503-pre-commit-hook.sh: test_description='pre-commit hook'
     -
     -	echo "another" >> file &&
     +	test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks" &&
    -+	ln -s "fail.sample" "$PRECOMMIT" &&
    ++	cp "$HOOKDIR/fail.sample" "$PRECOMMIT" &&
     +	echo "$PRECOMMIT" >expected_hooks &&
     +	echo "another" >>file &&
      	git add file &&
    @@ t/t7503-pre-commit-hook.sh: test_description='pre-commit hook'
     -
     -	echo "stuff" >> file &&
     +	test_when_finished "rm -f \"$PRECOMMIT\" actual_hooks" &&
    -+	ln -s "fail.sample" "$PRECOMMIT" &&
    ++	cp "$HOOKDIR/fail.sample" "$PRECOMMIT" &&
     +	echo "stuff" >>file &&
      	git add file &&
     -	git commit --no-verify -m "stuff"
    @@ t/t7503-pre-commit-hook.sh: test_description='pre-commit hook'
     -
     -	echo "content" >> file &&
     +	test_when_finished "rm -f \"$PRECOMMIT\" actual_hooks" &&
    -+	ln -s "non-exec.sample" "$PRECOMMIT" &&
    ++	cp "$HOOKDIR/non-exec.sample" "$PRECOMMIT" &&
     +	echo "content" >>file &&
      	git add file &&
     -	git commit -m "content"
    @@ t/t7503-pre-commit-hook.sh: test_description='pre-commit hook'
     -
     -	echo "more content" >> file &&
     +	test_when_finished "rm -f \"$PRECOMMIT\" actual_hooks" &&
    -+	ln -s "non-exec.sample" "$PRECOMMIT" &&
    ++	cp "$HOOKDIR/non-exec.sample" "$PRECOMMIT" &&
     +	echo "more content" >>file &&
      	git add file &&
     -	git commit --no-verify -m "more content"
    @@ t/t7503-pre-commit-hook.sh: test_description='pre-commit hook'
     -
     -	echo "more content" >> file &&
     +	test_when_finished "rm -rf \"$PRECOMMIT\" expected_hooks actual_hooks success" &&
    -+	ln -s "require-prefix.sample" "$PRECOMMIT" &&
    ++	cp "$HOOKDIR/require-prefix.sample" "$PRECOMMIT" &&
     +	echo "$PRECOMMIT" >expected_hooks &&
     +	echo "more content" >>file &&
      	git add file &&
    @@ t/t7503-pre-commit-hook.sh: test_description='pre-commit hook'
     -
     -	echo "more content" >> file &&
     +	test_when_finished "rm -rf \"$PRECOMMIT\" expected_hooks actual_hooks fail" &&
    -+	ln -s "require-prefix.sample" "$PRECOMMIT" &&
    ++	cp "$HOOKDIR/require-prefix.sample" "$PRECOMMIT" &&
     +	echo "$PRECOMMIT" >expected_hooks &&
     +	echo "more content" >>file &&
      	git add file &&
    @@ t/t7503-pre-commit-hook.sh: test_description='pre-commit hook'
     -	test "$GIT_AUTHOR_NAME" = "New Author" &&
     -	test "$GIT_AUTHOR_EMAIL" = "newauthor@example.com"
     +	test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks" &&
    -+	ln -s "check-author.sample" "$PRECOMMIT" &&
    ++	cp "$HOOKDIR/check-author.sample" "$PRECOMMIT" &&
     +	cat >expected_hooks <<-EOF &&
     +	$PRECOMMIT
     +	$PRECOMMIT
2:  3b701a5c41 = 2:  02a97eb369 merge: do no-verify like commit
3:  9210421fbb ! 3:  2d41e0ff79 git-merge: honor pre-merge-commit hook
    @@ t/t7503-pre-commit-and-pre-merge-commit-hooks.sh: test_expect_success 'with succ
      
     +test_expect_success 'with succeeding hook (merge)' '
     +	test_when_finished "rm -f \"$PREMERGE\" expected_hooks actual_hooks" &&
    -+	ln -s "success.sample" "$PREMERGE" &&
    ++	cp "$HOOKDIR/success.sample" "$PREMERGE" &&
     +	echo "$PREMERGE" >expected_hooks &&
     +	git checkout side &&
     +	git merge -m "merge master" master &&
    @@ t/t7503-pre-commit-and-pre-merge-commit-hooks.sh: test_expect_success 'with succ
     +	test_when_finished "rm -f \"$PREMERGE\" \"$PRECOMMIT\"" &&
     +	test_when_finished "rm -f expected_hooks actual_hooks" &&
     +	test_when_finished "git checkout master" &&
    -+	ln -s "success.sample" "$PREMERGE" &&
    -+	ln -s "success.sample" "$PRECOMMIT" &&
    ++	cp "$HOOKDIR/success.sample" "$PREMERGE" &&
    ++	cp "$HOOKDIR/success.sample" "$PRECOMMIT" &&
     +
     +	git checkout conflicting-a &&
     +	test_must_fail git merge -m "merge conflicting-b" conflicting-b &&
    @@ t/t7503-pre-commit-and-pre-merge-commit-hooks.sh: test_expect_success 'with succ
     +
      test_expect_success '--no-verify with succeeding hook' '
      	test_when_finished "rm -f \"$PRECOMMIT\" actual_hooks" &&
    - 	ln -s "success.sample" "$PRECOMMIT" &&
    + 	cp "$HOOKDIR/success.sample" "$PRECOMMIT" &&
     @@ t/t7503-pre-commit-and-pre-merge-commit-hooks.sh: test_expect_success '--no-verify with failing hook' '
      	test_path_is_missing actual_hooks
      '
      
     +test_expect_success 'with failing hook (merge)' '
     +	test_when_finished "rm -f \"$PREMERGE\" expected_hooks actual_hooks" &&
    -+	ln -s "fail.sample" "$PREMERGE" &&
    ++	cp "$HOOKDIR/fail.sample" "$PREMERGE" &&
     +	echo "$PREMERGE" >expected_hooks &&
     +	git checkout side &&
     +	test_must_fail git merge -m "merge master" master &&
    @@ t/t7503-pre-commit-and-pre-merge-commit-hooks.sh: test_expect_success '--no-veri
     +
      test_expect_success POSIXPERM 'with non-executable hook' '
      	test_when_finished "rm -f \"$PRECOMMIT\" actual_hooks" &&
    - 	ln -s "non-exec.sample" "$PRECOMMIT" &&
    + 	cp "$HOOKDIR/non-exec.sample" "$PRECOMMIT" &&
     @@ t/t7503-pre-commit-and-pre-merge-commit-hooks.sh: test_expect_success POSIXPERM '--no-verify with non-executable hook' '
      	test_path_is_missing actual_hooks
      '
      
     +test_expect_success POSIXPERM 'with non-executable hook (merge)' '
     +	test_when_finished "rm -f \"$PREMERGE\" actual_hooks" &&
    -+	ln -s "non-exec.sample" "$PREMERGE" &&
    ++	cp "$HOOKDIR/non-exec.sample" "$PREMERGE" &&
     +	git branch -f side side-orig &&
     +	git checkout side &&
     +	git merge -m "merge master" master &&
    @@ t/t7503-pre-commit-and-pre-merge-commit-hooks.sh: test_expect_success POSIXPERM
     +
      test_expect_success 'with hook requiring GIT_PREFIX' '
      	test_when_finished "rm -rf \"$PRECOMMIT\" expected_hooks actual_hooks success" &&
    - 	ln -s "require-prefix.sample" "$PRECOMMIT" &&
    + 	cp "$HOOKDIR/require-prefix.sample" "$PRECOMMIT" &&
     
      ## templates/hooks--pre-merge-commit.sample (new) ##
     @@
4:  96c54883d3 ! 4:  7b81e2717f merge: --no-verify to bypass pre-merge-commit hook
    @@ t/t7503-pre-commit-and-pre-merge-commit-hooks.sh: test_expect_success '--no-veri
     +
      test_expect_success 'with succeeding hook' '
      	test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks" &&
    - 	ln -s "success.sample" "$PRECOMMIT" &&
    + 	cp "$HOOKDIR/success.sample" "$PRECOMMIT" &&
     @@ t/t7503-pre-commit-and-pre-merge-commit-hooks.sh: test_expect_success '--no-verify with succeeding hook' '
      	test_path_is_missing actual_hooks
      '
      
     +test_expect_success '--no-verify with succeeding hook (merge)' '
     +	test_when_finished "rm -f \"$PREMERGE\" actual_hooks" &&
    -+	ln -s "success.sample" "$PREMERGE" &&
    ++	cp "$HOOKDIR/success.sample" "$PREMERGE" &&
     +	git branch -f side side-orig &&
     +	git checkout side &&
     +	git merge --no-verify -m "merge master" master &&
    @@ t/t7503-pre-commit-and-pre-merge-commit-hooks.sh: test_expect_success '--no-veri
     +
      test_expect_success 'with failing hook' '
      	test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks" &&
    - 	ln -s "fail.sample" "$PRECOMMIT" &&
    + 	cp "$HOOKDIR/fail.sample" "$PRECOMMIT" &&
     @@ t/t7503-pre-commit-and-pre-merge-commit-hooks.sh: test_expect_success 'with failing hook (merge)' '
      	test_cmp expected_hooks actual_hooks
      '
      
     +test_expect_success '--no-verify with failing hook (merge)' '
     +	test_when_finished "rm -f \"$PREMERGE\" actual_hooks" &&
    -+	ln -s "fail.sample" "$PREMERGE" &&
    ++	cp "$HOOKDIR/fail.sample" "$PREMERGE" &&
     +	git branch -f side side-orig &&
     +	git checkout side &&
     +	git merge --no-verify -m "merge master" master &&
    @@ t/t7503-pre-commit-and-pre-merge-commit-hooks.sh: test_expect_success 'with fail
     +
      test_expect_success POSIXPERM 'with non-executable hook' '
      	test_when_finished "rm -f \"$PRECOMMIT\" actual_hooks" &&
    - 	ln -s "non-exec.sample" "$PRECOMMIT" &&
    + 	cp "$HOOKDIR/non-exec.sample" "$PRECOMMIT" &&
     @@ t/t7503-pre-commit-and-pre-merge-commit-hooks.sh: test_expect_success POSIXPERM 'with non-executable hook (merge)' '
      	test_path_is_missing actual_hooks
      '
      
     +test_expect_success POSIXPERM '--no-verify with non-executable hook (merge)' '
     +	test_when_finished "rm -f \"$PREMERGE\" actual_hooks" &&
    -+	ln -s "non-exec.sample" "$PREMERGE" &&
    ++	cp "$HOOKDIR/non-exec.sample" "$PREMERGE" &&
     +	git branch -f side side-orig &&
     +	git checkout side &&
     +	git merge --no-verify -m "merge master" master &&
    @@ t/t7503-pre-commit-and-pre-merge-commit-hooks.sh: test_expect_success POSIXPERM
     +
      test_expect_success 'with hook requiring GIT_PREFIX' '
      	test_when_finished "rm -rf \"$PRECOMMIT\" expected_hooks actual_hooks success" &&
    - 	ln -s "require-prefix.sample" "$PRECOMMIT" &&
    + 	cp "$HOOKDIR/require-prefix.sample" "$PRECOMMIT" &&
-- 
2.22.0.770.g0f2c4a37fd-goog

[PATCH v5 1/4] t7503: verify proper hook execution

[Josh Steadmon]

t7503 did not verify that the expected hooks actually ran during
testing. Fix that by making the hook scripts write their $0 into a file
so that we can compare actual execution vs. expected execution.

While we're at it, do some test style cleanups, such as using
write_script() and doing setup inside a test_expect_success block.

Improved-by: Martin Ågren <martin.agren@gmail.com>
Signed-off-by: Martin Ågren <martin.agren@gmail.com>
Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 t/t7503-pre-commit-hook.sh | 157 +++++++++++++++++++++----------------
 1 file changed, 89 insertions(+), 68 deletions(-)

diff --git a/t/t7503-pre-commit-hook.sh b/t/t7503-pre-commit-hook.sh
index 984889b39d..6aa83204c2 100755
--- a/t/t7503-pre-commit-hook.sh
+++ b/t/t7503-pre-commit-hook.sh
@@ -4,124 +4,144 @@ test_description='pre-commit hook'
 
 . ./test-lib.sh
 
-test_expect_success 'with no hook' '
+HOOKDIR="$(git rev-parse --git-dir)/hooks"
+PRECOMMIT="$HOOKDIR/pre-commit"
+
+# Prepare sample scripts that write their $0 to actual_hooks
+test_expect_success 'sample script setup' '
+	mkdir -p "$HOOKDIR" &&
+	write_script "$HOOKDIR/success.sample" <<-\EOF &&
+	echo $0 >>actual_hooks
+	exit 0
+	EOF
+	write_script "$HOOKDIR/fail.sample" <<-\EOF &&
+	echo $0 >>actual_hooks
+	exit 1
+	EOF
+	write_script "$HOOKDIR/non-exec.sample" <<-\EOF &&
+	echo $0 >>actual_hooks
+	exit 1
+	EOF
+	chmod -x "$HOOKDIR/non-exec.sample" &&
+	write_script "$HOOKDIR/require-prefix.sample" <<-\EOF &&
+	echo $0 >>actual_hooks
+	test $GIT_PREFIX = "success/"
+	EOF
+	write_script "$HOOKDIR/check-author.sample" <<-\EOF
+	echo $0 >>actual_hooks
+	test "$GIT_AUTHOR_NAME" = "New Author" &&
+	test "$GIT_AUTHOR_EMAIL" = "newauthor@example.com"
+	EOF
+'
 
-	echo "foo" > file &&
+test_expect_success 'with no hook' '
+	test_when_finished "rm -f actual_hooks" &&
+	echo "foo" >file &&
 	git add file &&
-	git commit -m "first"
-
+	git commit -m "first" &&
+	test_path_is_missing actual_hooks
 '
 
 test_expect_success '--no-verify with no hook' '
-
-	echo "bar" > file &&
+	test_when_finished "rm -f actual_hooks" &&
+	echo "bar" >file &&
 	git add file &&
-	git commit --no-verify -m "bar"
-
+	git commit --no-verify -m "bar" &&
+	test_path_is_missing actual_hooks
 '
 
-# 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 &&
+	test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks" &&
+	cp "$HOOKDIR/success.sample" "$PRECOMMIT" &&
+	echo "$PRECOMMIT" >expected_hooks &&
+	echo "more" >>file &&
 	git add file &&
-	git commit -m "more"
-
+	git commit -m "more" &&
+	test_cmp expected_hooks actual_hooks
 '
 
 test_expect_success '--no-verify with succeeding hook' '
-
-	echo "even more" >> file &&
+	test_when_finished "rm -f \"$PRECOMMIT\" actual_hooks" &&
+	cp "$HOOKDIR/success.sample" "$PRECOMMIT" &&
+	echo "even more" >>file &&
 	git add file &&
-	git commit --no-verify -m "even more"
-
+	git commit --no-verify -m "even more" &&
+	test_path_is_missing actual_hooks
 '
 
-# now a hook that fails
-cat > "$HOOK" <<EOF
-#!/bin/sh
-exit 1
-EOF
-
 test_expect_success 'with failing hook' '
-
-	echo "another" >> file &&
+	test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks" &&
+	cp "$HOOKDIR/fail.sample" "$PRECOMMIT" &&
+	echo "$PRECOMMIT" >expected_hooks &&
+	echo "another" >>file &&
 	git add file &&
-	test_must_fail git commit -m "another"
-
+	test_must_fail git commit -m "another" &&
+	test_cmp expected_hooks actual_hooks
 '
 
 test_expect_success '--no-verify with failing hook' '
-
-	echo "stuff" >> file &&
+	test_when_finished "rm -f \"$PRECOMMIT\" actual_hooks" &&
+	cp "$HOOKDIR/fail.sample" "$PRECOMMIT" &&
+	echo "stuff" >>file &&
 	git add file &&
-	git commit --no-verify -m "stuff"
-
+	git commit --no-verify -m "stuff" &&
+	test_path_is_missing actual_hooks
 '
 
-chmod -x "$HOOK"
 test_expect_success POSIXPERM 'with non-executable hook' '
-
-	echo "content" >> file &&
+	test_when_finished "rm -f \"$PRECOMMIT\" actual_hooks" &&
+	cp "$HOOKDIR/non-exec.sample" "$PRECOMMIT" &&
+	echo "content" >>file &&
 	git add file &&
-	git commit -m "content"
-
+	git commit -m "content" &&
+	test_path_is_missing actual_hooks
 '
 
 test_expect_success POSIXPERM '--no-verify with non-executable hook' '
-
-	echo "more content" >> file &&
+	test_when_finished "rm -f \"$PRECOMMIT\" actual_hooks" &&
+	cp "$HOOKDIR/non-exec.sample" "$PRECOMMIT" &&
+	echo "more content" >>file &&
 	git add file &&
-	git commit --no-verify -m "more content"
-
+	git commit --no-verify -m "more content" &&
+	test_path_is_missing actual_hooks
 '
-chmod +x "$HOOK"
-
-# a hook that checks $GIT_PREFIX and succeeds inside the
-# success/ subdirectory only
-cat > "$HOOK" <<EOF
-#!/bin/sh
-test \$GIT_PREFIX = success/
-EOF
 
 test_expect_success 'with hook requiring GIT_PREFIX' '
-
-	echo "more content" >> file &&
+	test_when_finished "rm -rf \"$PRECOMMIT\" expected_hooks actual_hooks success" &&
+	cp "$HOOKDIR/require-prefix.sample" "$PRECOMMIT" &&
+	echo "$PRECOMMIT" >expected_hooks &&
+	echo "more content" >>file &&
 	git add file &&
 	mkdir success &&
 	(
 		cd success &&
 		git commit -m "hook requires GIT_PREFIX = success/"
 	) &&
-	rmdir success
+	test_cmp expected_hooks actual_hooks
 '
 
 test_expect_success 'with failing hook requiring GIT_PREFIX' '
-
-	echo "more content" >> file &&
+	test_when_finished "rm -rf \"$PRECOMMIT\" expected_hooks actual_hooks fail" &&
+	cp "$HOOKDIR/require-prefix.sample" "$PRECOMMIT" &&
+	echo "$PRECOMMIT" >expected_hooks &&
+	echo "more content" >>file &&
 	git add file &&
 	mkdir fail &&
 	(
 		cd fail &&
 		test_must_fail git commit -m "hook must fail"
 	) &&
-	rmdir fail &&
-	git checkout -- file
+	git checkout -- file &&
+	test_cmp expected_hooks actual_hooks
 '
 
 test_expect_success 'check the author in hook' '
-	write_script "$HOOK" <<-\EOF &&
-	test "$GIT_AUTHOR_NAME" = "New Author" &&
-	test "$GIT_AUTHOR_EMAIL" = "newauthor@example.com"
+	test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks" &&
+	cp "$HOOKDIR/check-author.sample" "$PRECOMMIT" &&
+	cat >expected_hooks <<-EOF &&
+	$PRECOMMIT
+	$PRECOMMIT
+	$PRECOMMIT
 	EOF
 	test_must_fail git commit --allow-empty -m "by a.u.thor" &&
 	(
@@ -133,7 +153,8 @@ test_expect_success 'check the author in hook' '
 	) &&
 	git commit --author="New Author <newauthor@example.com>" \
 		--allow-empty -m "by new.author via command line" &&
-	git show -s
+	git show -s &&
+	test_cmp expected_hooks actual_hooks
 '
 
 test_done
-- 
2.22.0.770.g0f2c4a37fd-goog

[PATCH v5 2/4] merge: do no-verify like commit

[Josh Steadmon]

From: Michael J Gruber <git@grubix.eu>

f8b863598c ("builtin/merge: honor commit-msg hook for merges", 2017-09-07)
introduced the no-verify flag to merge for bypassing the commit-msg
hook, though in a different way from the implementation in commit.c.

Change the implementation in merge.c to be the same as in commit.c so
that both do the same in the same way. This also changes the output of
"git merge --help" to be more clear that the hook return code is
respected by default.

[js: * reworded commit message
     * squashed documentation changes from original series' patch 3/4
]

Signed-off-by: Michael J Gruber <git@grubix.eu>
Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 Documentation/git-merge.txt     | 2 +-
 Documentation/merge-options.txt | 4 ++++
 builtin/merge.c                 | 6 +++---
 3 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
index 01fd52dc70..092529c619 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>] [-S[<keyid>]]
+	[--no-verify] [-s <strategy>] [-X <strategy-option>] [-S[<keyid>]]
 	[--[no-]allow-unrelated-histories]
 	[--[no-]rerere-autoupdate] [-m <msg>] [-F <file>] [<commit>...]
 'git merge' (--continue | --abort | --quit)
diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
index 79a00d2a4a..d6a9f4b96f 100644
--- a/Documentation/merge-options.txt
+++ b/Documentation/merge-options.txt
@@ -105,6 +105,10 @@ option can be used to override --squash.
 +
 With --squash, --commit is not allowed, and will fail.
 
+--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 e2ccbc44e2..4425a7a12e 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -81,7 +81,7 @@ static int show_progress = -1;
 static int default_to_upstream = 1;
 static int signoff;
 static const char *sign_commit;
-static int verify_msg = 1;
+static int no_verify;
 
 static struct strategy all_strategy[] = {
 	{ "recursive",  DEFAULT_TWOHEAD | NO_TRIVIAL },
@@ -287,7 +287,7 @@ static struct option builtin_merge_options[] = {
 	  N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" },
 	OPT_BOOL(0, "overwrite-ignore", &overwrite_ignore, N_("update ignored files (default)")),
 	OPT_BOOL(0, "signoff", &signoff, N_("add Signed-off-by:")),
-	OPT_BOOL(0, "verify", &verify_msg, N_("verify commit-msg hook")),
+	OPT_BOOL(0, "no-verify", &no_verify, N_("bypass commit-msg hook")),
 	OPT_END()
 };
 
@@ -842,7 +842,7 @@ static void prepare_to_commit(struct commit_list *remoteheads)
 			abort_commit(remoteheads, NULL);
 	}
 
-	if (verify_msg && run_commit_hook(0 < option_edit, get_index_file(),
+	if (!no_verify && run_commit_hook(0 < option_edit, get_index_file(),
 					  "commit-msg",
 					  git_path_merge_msg(the_repository), NULL))
 		abort_commit(remoteheads, NULL);
-- 
2.22.0.770.g0f2c4a37fd-goog

[PATCH v5 3/4] git-merge: honor pre-merge-commit hook

[Josh Steadmon]

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

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-commit 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).

[js: * renamed hook from "pre-merge" to "pre-merge-commit"
     * only discard the index if the hook is actually present
     * expanded githooks documentation entry
     * clarified that hook should write messages to stderr
     * squashed test changes from the original series' patch 4/4
     * modified tests to follow new pattern from this series' patch 1/4
     * added a test case for non-executable merge hooks
     * added a test case for failed merges
     * when testing that the merge hook did not run, make sure we
       actually have a merge to perform (by resetting the "side" branch
       to its original state).
     * reworded commit message
]

Improved-by: Martin Ågren <martin.agren@gmail.com>
Signed-off-by: Michael J Gruber <git@grubix.eu>
Signed-off-by: Martin Ågren <martin.agren@gmail.com>
Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 Documentation/githooks.txt                    | 21 +++++
 builtin/merge.c                               | 12 +++
 ...-pre-commit-and-pre-merge-commit-hooks.sh} | 84 ++++++++++++++++++-
 templates/hooks--pre-merge-commit.sample      | 13 +++
 4 files changed, 129 insertions(+), 1 deletion(-)
 rename t/{t7503-pre-commit-hook.sh => t7503-pre-commit-and-pre-merge-commit-hooks.sh} (63%)
 create mode 100755 templates/hooks--pre-merge-commit.sample

diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index 82cd573776..d9da474fb0 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -103,6 +103,27 @@ The default 'pre-commit' hook, when enabled--and with the
 `hooks.allownonascii` config option unset or set to false--prevents
 the use of non-ASCII filenames.
 
+pre-merge-commit
+~~~~~~~~~~~~~~~~
+
+This hook is invoked by linkgit:git-merge[1]. It takes no parameters, and is
+invoked after the merge has been carried out successfully and before
+obtaining the proposed commit log message to
+make a commit.  Exiting with a non-zero status from this script
+causes the `git merge` command to abort before creating a commit.
+
+The default 'pre-merge-commit' hook, when enabled, runs the
+'pre-commit' hook, if the latter is enabled.
+
+This hook is invoked with the environment variable
+`GIT_EDITOR=:` if the command will not bring up an editor
+to modify the commit message.
+
+If the merge cannot be carried out automatically, the conflicts
+need to be resolved and the result committed separately (see
+linkgit:git-merge[1]). At that point, this hook will not be executed,
+but the 'pre-commit' hook will, if it is enabled.
+
 prepare-commit-msg
 ~~~~~~~~~~~~~~~~~~
 
diff --git a/builtin/merge.c b/builtin/merge.c
index 4425a7a12e..bf0ae68c40 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -816,6 +816,18 @@ static void write_merge_heads(struct commit_list *);
 static void prepare_to_commit(struct commit_list *remoteheads)
 {
 	struct strbuf msg = STRBUF_INIT;
+	const char *index_file = get_index_file();
+
+	if (run_commit_hook(0 < option_edit, index_file, "pre-merge-commit", NULL))
+		abort_commit(remoteheads, NULL);
+	/*
+	 * Re-read the index as pre-merge-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.
+	 */
+	if (find_hook("pre-merge-commit"))
+		discard_cache();
+	read_cache_from(index_file);
 	strbuf_addbuf(&msg, &merge_msg);
 	if (squash)
 		BUG("the control must not reach here under --squash");
diff --git a/t/t7503-pre-commit-hook.sh b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
similarity index 63%
rename from t/t7503-pre-commit-hook.sh
rename to t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
index 6aa83204c2..7a5434c7ab 100755
--- a/t/t7503-pre-commit-hook.sh
+++ b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
@@ -1,11 +1,12 @@
 #!/bin/sh
 
-test_description='pre-commit hook'
+test_description='pre-commit and pre-merge-commit hooks'
 
 . ./test-lib.sh
 
 HOOKDIR="$(git rev-parse --git-dir)/hooks"
 PRECOMMIT="$HOOKDIR/pre-commit"
+PREMERGE="$HOOKDIR/pre-merge-commit"
 
 # Prepare sample scripts that write their $0 to actual_hooks
 test_expect_success 'sample script setup' '
@@ -34,6 +35,30 @@ test_expect_success 'sample script setup' '
 	EOF
 '
 
+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 branch side-orig side &&
+	git checkout master
+'
+
+test_expect_success 'setup conflicting branches' '
+	test_when_finished "git checkout master" &&
+	git checkout -b conflicting-a master &&
+	echo a >conflicting &&
+	git add conflicting &&
+	git commit -m conflicting-a &&
+	git checkout -b conflicting-b master &&
+	echo b >conflicting &&
+	git add conflicting &&
+	git commit -m conflicting-b
+'
+
 test_expect_success 'with no hook' '
 	test_when_finished "rm -f actual_hooks" &&
 	echo "foo" >file &&
@@ -42,6 +67,15 @@ test_expect_success 'with no hook' '
 	test_path_is_missing actual_hooks
 '
 
+test_expect_success 'with no hook (merge)' '
+	test_when_finished "rm -f actual_hooks" &&
+	git branch -f side side-orig &&
+	git checkout side &&
+	git merge -m "merge master" master &&
+	git checkout master &&
+	test_path_is_missing actual_hooks
+'
+
 test_expect_success '--no-verify with no hook' '
 	test_when_finished "rm -f actual_hooks" &&
 	echo "bar" >file &&
@@ -60,6 +94,34 @@ test_expect_success 'with succeeding hook' '
 	test_cmp expected_hooks actual_hooks
 '
 
+test_expect_success 'with succeeding hook (merge)' '
+	test_when_finished "rm -f \"$PREMERGE\" expected_hooks actual_hooks" &&
+	cp "$HOOKDIR/success.sample" "$PREMERGE" &&
+	echo "$PREMERGE" >expected_hooks &&
+	git checkout side &&
+	git merge -m "merge master" master &&
+	git checkout master &&
+	test_cmp expected_hooks actual_hooks
+'
+
+test_expect_success 'automatic merge fails; both hooks are available' '
+	test_when_finished "rm -f \"$PREMERGE\" \"$PRECOMMIT\"" &&
+	test_when_finished "rm -f expected_hooks actual_hooks" &&
+	test_when_finished "git checkout master" &&
+	cp "$HOOKDIR/success.sample" "$PREMERGE" &&
+	cp "$HOOKDIR/success.sample" "$PRECOMMIT" &&
+
+	git checkout conflicting-a &&
+	test_must_fail git merge -m "merge conflicting-b" conflicting-b &&
+	test_path_is_missing actual_hooks &&
+
+	echo "$PRECOMMIT" >expected_hooks &&
+	echo a+b >conflicting &&
+	git add conflicting &&
+	git commit -m "resolve conflict" &&
+	test_cmp expected_hooks actual_hooks
+'
+
 test_expect_success '--no-verify with succeeding hook' '
 	test_when_finished "rm -f \"$PRECOMMIT\" actual_hooks" &&
 	cp "$HOOKDIR/success.sample" "$PRECOMMIT" &&
@@ -88,6 +150,16 @@ test_expect_success '--no-verify with failing hook' '
 	test_path_is_missing actual_hooks
 '
 
+test_expect_success 'with failing hook (merge)' '
+	test_when_finished "rm -f \"$PREMERGE\" expected_hooks actual_hooks" &&
+	cp "$HOOKDIR/fail.sample" "$PREMERGE" &&
+	echo "$PREMERGE" >expected_hooks &&
+	git checkout side &&
+	test_must_fail git merge -m "merge master" master &&
+	git checkout master &&
+	test_cmp expected_hooks actual_hooks
+'
+
 test_expect_success POSIXPERM 'with non-executable hook' '
 	test_when_finished "rm -f \"$PRECOMMIT\" actual_hooks" &&
 	cp "$HOOKDIR/non-exec.sample" "$PRECOMMIT" &&
@@ -106,6 +178,16 @@ test_expect_success POSIXPERM '--no-verify with non-executable hook' '
 	test_path_is_missing actual_hooks
 '
 
+test_expect_success POSIXPERM 'with non-executable hook (merge)' '
+	test_when_finished "rm -f \"$PREMERGE\" actual_hooks" &&
+	cp "$HOOKDIR/non-exec.sample" "$PREMERGE" &&
+	git branch -f side side-orig &&
+	git checkout side &&
+	git merge -m "merge master" master &&
+	git checkout master &&
+	test_path_is_missing actual_hooks
+'
+
 test_expect_success 'with hook requiring GIT_PREFIX' '
 	test_when_finished "rm -rf \"$PRECOMMIT\" expected_hooks actual_hooks success" &&
 	cp "$HOOKDIR/require-prefix.sample" "$PRECOMMIT" &&
diff --git a/templates/hooks--pre-merge-commit.sample b/templates/hooks--pre-merge-commit.sample
new file mode 100755
index 0000000000..399eab1924
--- /dev/null
+++ b/templates/hooks--pre-merge-commit.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 to
+# stderr if it wants to stop the merge commit.
+#
+# To enable this hook, rename this file to "pre-merge-commit".
+
+. git-sh-setup
+test -x "$GIT_DIR/hooks/pre-commit" &&
+        exec "$GIT_DIR/hooks/pre-commit"
+:
-- 
2.22.0.770.g0f2c4a37fd-goog

[PATCH v5 4/4] merge: --no-verify to bypass pre-merge-commit hook

[Josh Steadmon]

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

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

[js: * reworded commit message to reflect current state of --no-stat flag
       and new hook name
     * fixed flag documentation to reflect new hook name
     * cleaned up trailing whitespace
     * squashed test changes from the original series' patch 4/4
     * modified tests to follow pattern from this series' patch 1/4
     * added a test case for --no-verify with non-executable hook
     * when testing that the merge hook did not run, make sure we
       actually have a merge to perform (by resetting the "side" branch
       to its original state).

]

Improved-by: Martin Ågren <martin.agren@gmail.com>
Signed-off-by: Michael J Gruber <git@grubix.eu>
Signed-off-by: Martin Ågren <martin.agren@gmail.com>
Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 Documentation/githooks.txt                    |  3 +-
 builtin/merge.c                               |  4 +-
 ...3-pre-commit-and-pre-merge-commit-hooks.sh | 39 +++++++++++++++++++
 3 files changed, 43 insertions(+), 3 deletions(-)

diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index d9da474fb0..57d6e2b98d 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -106,7 +106,8 @@ the use of non-ASCII filenames.
 pre-merge-commit
 ~~~~~~~~~~~~~~~~
 
-This hook is invoked by linkgit:git-merge[1]. It takes no parameters, and is
+This hook is invoked by linkgit:git-merge[1], and can be bypassed
+with the `--no-verify` option.  It takes no parameters, and is
 invoked after the merge has been carried out successfully and before
 obtaining the proposed commit log message to
 make a commit.  Exiting with a non-zero status from this script
diff --git a/builtin/merge.c b/builtin/merge.c
index bf0ae68c40..c9746e37b8 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -287,7 +287,7 @@ static struct option builtin_merge_options[] = {
 	  N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" },
 	OPT_BOOL(0, "overwrite-ignore", &overwrite_ignore, N_("update ignored files (default)")),
 	OPT_BOOL(0, "signoff", &signoff, N_("add Signed-off-by:")),
-	OPT_BOOL(0, "no-verify", &no_verify, N_("bypass commit-msg hook")),
+	OPT_BOOL(0, "no-verify", &no_verify, N_("bypass pre-merge-commit and commit-msg hooks")),
 	OPT_END()
 };
 
@@ -818,7 +818,7 @@ static void prepare_to_commit(struct commit_list *remoteheads)
 	struct strbuf msg = STRBUF_INIT;
 	const char *index_file = get_index_file();
 
-	if (run_commit_hook(0 < option_edit, index_file, "pre-merge-commit", NULL))
+	if (!no_verify && run_commit_hook(0 < option_edit, index_file, "pre-merge-commit", NULL))
 		abort_commit(remoteheads, NULL);
 	/*
 	 * Re-read the index as pre-merge-commit hook could have updated it,
diff --git a/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
index 7a5434c7ab..b3485450a2 100755
--- a/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
+++ b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
@@ -84,6 +84,15 @@ test_expect_success '--no-verify with no hook' '
 	test_path_is_missing actual_hooks
 '
 
+test_expect_success '--no-verify with no hook (merge)' '
+	test_when_finished "rm -f actual_hooks" &&
+	git branch -f side side-orig &&
+	git checkout side &&
+	git merge --no-verify -m "merge master" master &&
+	git checkout master &&
+	test_path_is_missing actual_hooks
+'
+
 test_expect_success 'with succeeding hook' '
 	test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks" &&
 	cp "$HOOKDIR/success.sample" "$PRECOMMIT" &&
@@ -131,6 +140,16 @@ test_expect_success '--no-verify with succeeding hook' '
 	test_path_is_missing actual_hooks
 '
 
+test_expect_success '--no-verify with succeeding hook (merge)' '
+	test_when_finished "rm -f \"$PREMERGE\" actual_hooks" &&
+	cp "$HOOKDIR/success.sample" "$PREMERGE" &&
+	git branch -f side side-orig &&
+	git checkout side &&
+	git merge --no-verify -m "merge master" master &&
+	git checkout master &&
+	test_path_is_missing actual_hooks
+'
+
 test_expect_success 'with failing hook' '
 	test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks" &&
 	cp "$HOOKDIR/fail.sample" "$PRECOMMIT" &&
@@ -160,6 +179,16 @@ test_expect_success 'with failing hook (merge)' '
 	test_cmp expected_hooks actual_hooks
 '
 
+test_expect_success '--no-verify with failing hook (merge)' '
+	test_when_finished "rm -f \"$PREMERGE\" actual_hooks" &&
+	cp "$HOOKDIR/fail.sample" "$PREMERGE" &&
+	git branch -f side side-orig &&
+	git checkout side &&
+	git merge --no-verify -m "merge master" master &&
+	git checkout master &&
+	test_path_is_missing actual_hooks
+'
+
 test_expect_success POSIXPERM 'with non-executable hook' '
 	test_when_finished "rm -f \"$PRECOMMIT\" actual_hooks" &&
 	cp "$HOOKDIR/non-exec.sample" "$PRECOMMIT" &&
@@ -188,6 +217,16 @@ test_expect_success POSIXPERM 'with non-executable hook (merge)' '
 	test_path_is_missing actual_hooks
 '
 
+test_expect_success POSIXPERM '--no-verify with non-executable hook (merge)' '
+	test_when_finished "rm -f \"$PREMERGE\" actual_hooks" &&
+	cp "$HOOKDIR/non-exec.sample" "$PREMERGE" &&
+	git branch -f side side-orig &&
+	git checkout side &&
+	git merge --no-verify -m "merge master" master &&
+	git checkout master &&
+	test_path_is_missing actual_hooks
+'
+
 test_expect_success 'with hook requiring GIT_PREFIX' '
 	test_when_finished "rm -rf \"$PRECOMMIT\" expected_hooks actual_hooks success" &&
 	cp "$HOOKDIR/require-prefix.sample" "$PRECOMMIT" &&
-- 
2.22.0.770.g0f2c4a37fd-goog

Re: [PATCH v5 0/4] pre-merge-commit hook

[Martin Ågren]

On Wed, 7 Aug 2019 at 20:57, Josh Steadmon <steadmon@google.com> wrote:
>
> This series adds a new pre-merge-commit hook, similar in usage to
> pre-commit. It also improves hook testing in t7503, by verifying that
> the correct hooks are run or bypassed as expected.
>
> The original series was done by Michael J Gruber <git@grubix.eu>. I have
> addressed the outstanding review comments, and noted my changes in the
> commit messages in "[js: ...]" blocks.
>
> Changes since V4:

I don't have any more nits to pick ;-) so this looks good to me, FWIW.

Martin