threads / patch / 46808

patch, 4 partsmerge: do no-verify like commit

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

## tl;dr

59 messages between Sep 22, 2017 and Aug 8, 2019. Diffs are folded; open one to read it.

replies: 58people: 5as markdown or json

Michael J Gruber· Sep 22, 2017, 12:04 UTC · lore

[PATCH 0/4] pre-merge hook

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

Michael J Gruber· Sep 22, 2017, 12:04 UTC · re: Michael J Gruber · lore

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

Show changes to builtin/merge.c +3 −3

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

Stefan Beller· Sep 22, 2017, 19:55 UTC · re: Michael J Gruber · lore

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

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

Show 8 quoted lines

> 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

Show 38 quoted lines

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

Michael J Gruber· Sep 22, 2017, 12:04 UTC · re: Michael J Gruber · lore

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

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

Show changes to 3 files +31 −0

Documentation/githooks.txt, builtin/merge.c, 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

Stefan Beller· Sep 22, 2017, 19:52 UTC · re: Michael J Gruber · lore

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

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

Show 54 quoted lines

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

Show 14 quoted lines

>         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

Martin Ågren· Sep 23, 2017, 00:04 UTC · re: Michael J Gruber · lore

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

On 22 September 2017 at 14:04, Michael J Gruber <git@grubix.eu> wrote:

Show 10 quoted lines

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

[...]

Show 19 quoted lines

> 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

Michael J Gruber· Sep 22, 2017, 12:04 UTC · re: Michael J Gruber · lore

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

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

Show changes to 4 files +8 −4

Documentation/git-merge.txt, Documentation/githooks.txt, Documentation/merge-options.txt, builtin/merge.c

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

Junio C Hamano· Sep 23, 2017, 23:48 UTC · re: Michael J Gruber · lore

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

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

Show 8 quoted lines

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

Michael J Gruber· Sep 25, 2017, 10:54 UTC · re: Junio C Hamano · lore

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

Junio C Hamano venit, vidit, dixit 24.09.2017 01:48:

Show 16 quoted lines

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

Michael J Gruber· Sep 22, 2017, 12:04 UTC · re: Michael J Gruber · lore

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

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

Show changes to t/t7503-pre-commit-hook.sh +65 −1

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

Stefan Beller· Sep 22, 2017, 20:01 UTC · re: Michael J Gruber · lore

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

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

Show 7 quoted lines

> +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) ?

Junio C Hamano· Oct 2, 2017, 05:54 UTC · re: Michael J Gruber · lore

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

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.

Stefan Beller· Oct 2, 2017, 16:59 UTC · re: Junio C Hamano · lore

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

On Sun, Oct 1, 2017 at 10:54 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 14 quoted lines

> 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

Junio C Hamano· Oct 17, 2017, 04:05 UTC · re: Michael J Gruber · lore

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

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

Josh Steadmon· Jul 18, 2019, 22:57 UTC · re: Junio C Hamano · lore

[PATCH v2 0/4] pre-merge hook

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:

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

Josh Steadmon· Jul 18, 2019, 23:56 UTC · re: Junio C Hamano · lore

[PATCH v2 0/4] pre-merge hook

(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:

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

Josh Steadmon· Jul 18, 2019, 22:57 UTC · re: Josh Steadmon · lore

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

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

Show changes to 3 files +32 −0

Documentation/githooks.txt, builtin/merge.c, 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

Martin Ågren· Jul 29, 2019, 20:00 UTC · re: Josh Steadmon · lore

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

On Fri, 19 Jul 2019 at 00:57, Josh Steadmon <steadmon@google.com> wrote:

Show 16 quoted lines

> --- /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

Josh Steadmon· Jul 18, 2019, 22:57 UTC · re: Josh Steadmon · lore

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

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

Show changes to 3 files +8 −4

Documentation/git-merge.txt, Documentation/merge-options.txt, builtin/merge.c

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

Josh Steadmon· Jul 18, 2019, 22:57 UTC · re: Josh Steadmon · lore

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

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

Show changes to 2 files +3 −3

Documentation/githooks.txt, builtin/merge.c

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

Martin Ågren· Jul 29, 2019, 20:02 UTC · re: Josh Steadmon · lore

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

On Fri, 19 Jul 2019 at 00:57, Josh Steadmon <steadmon@google.com> wrote:

Show 5 quoted lines

> 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

Josh Steadmon· Jul 29, 2019, 23:33 UTC · re: Martin Ågren · lore

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

On 2019.07.29 22:02, Martin Ågren wrote:

Show 11 quoted lines

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

Josh Steadmon· Jul 18, 2019, 22:57 UTC · re: Josh Steadmon · lore

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

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

Show changes to 2 files +65 −1

t/t7503-pre-commit-hook.sh, t/t7503-pre-commit-and-pre-merge-hooks.sh

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

Martin Ågren· Jul 29, 2019, 20:04 UTC · re: Josh Steadmon · lore

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

On Fri, 19 Jul 2019 at 00:57, Josh Steadmon <steadmon@google.com> wrote:

Show 7 quoted lines

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

Show 7 quoted lines

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

Josh Steadmon· Jul 29, 2019, 23:43 UTC · re: Martin Ågren · lore

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

On 2019.07.29 22:04, Martin Ågren wrote:

Show 36 quoted lines

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

Martin Ågren· Jul 30, 2019, 07:13 UTC · re: Josh Steadmon · lore

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

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

Josh Steadmon· Jul 31, 2019, 18:34 UTC · re: Martin Ågren · lore

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

On 2019.07.30 09:13, Martin Ågren wrote:

Show 29 quoted lines

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

Martin Ågren· Jul 29, 2019, 20:09 UTC · re: Josh Steadmon · lore

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

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

Josh Steadmon· Jul 29, 2019, 23:29 UTC · re: Martin Ågren · lore

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

On 2019.07.29 22:09, Martin Ågren wrote:

Show 11 quoted lines

> 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

Martin Ågren· Jul 29, 2019, 20:29 UTC · re: Josh Steadmon · lore

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

On Fri, 19 Jul 2019 at 01:56, Josh Steadmon <steadmon@google.com> wrote:

Show 6 quoted lines

> 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

Josh Steadmon· Jul 29, 2019, 23:39 UTC · re: Martin Ågren · lore

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

On 2019.07.29 22:29, Martin Ågren wrote:

Show 15 quoted lines

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

Show 15 quoted lines

> 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

Josh Steadmon· Aug 1, 2019, 22:20 UTC · re: Josh Steadmon · lore

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

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

Josh Steadmon· Aug 1, 2019, 22:20 UTC · re: Josh Steadmon · lore

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

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

Show changes to t/t7503-pre-commit-hook.sh +94 −68

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

Martin Ågren· Aug 2, 2019, 09:43 UTC · re: Josh Steadmon · lore

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

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

Josh Steadmon· Aug 1, 2019, 22:20 UTC · re: Josh Steadmon · lore

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

[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(-)

Show changes to 3 files +8 −4

Documentation/git-merge.txt, Documentation/merge-options.txt, builtin/merge.c

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

Josh Steadmon· Aug 1, 2019, 22:20 UTC · re: Josh Steadmon · lore

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

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

Show changes to 5 files +76 −1

Documentation/githooks.txt, builtin/merge.c, t/t7503-pre-commit-hook.sh, t/t7503-pre-commit-and-pre-merge-commit-hooks.sh, 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

Martin Ågren· Aug 2, 2019, 09:45 UTC · re: Josh Steadmon · lore

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

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

Show 15 quoted lines

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

Show 10 quoted lines

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

Josh Steadmon· Aug 2, 2019, 22:20 UTC · re: Martin Ågren · lore

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

On 2019.08.02 11:45, Martin Ågren wrote:

Show 49 quoted lines

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

Show 13 quoted lines

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

Josh Steadmon· Aug 1, 2019, 22:20 UTC · re: Josh Steadmon · lore

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

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

Show changes to 3 files +32 −3

Documentation/githooks.txt, builtin/merge.c, t/t7503-pre-commit-and-pre-merge-commit-hooks.sh

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

Martin Ågren· Aug 2, 2019, 09:56 UTC · re: Josh Steadmon · lore

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

[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

Martin Ågren· Aug 2, 2019, 09:56 UTC · re: Martin Ågren · lore

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

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

Show changes to t/t7503-pre-commit-and-pre-merge-commit-hooks.sh +2 −2

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

Martin Ågren· Aug 2, 2019, 09:56 UTC · re: Martin Ågren · lore

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

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

Show changes to t/t7503-pre-commit-and-pre-merge-commit-hooks.sh +24 −14

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

Martin Ågren· Aug 2, 2019, 09:56 UTC · re: Martin Ågren · lore

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

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(+)

Show changes to t/t7503-pre-commit-and-pre-merge-commit-hooks.sh +30 −0

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

Martin Ågren· Aug 2, 2019, 09:56 UTC · re: Martin Ågren · lore

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

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

Show changes to t/t7503-pre-commit-and-pre-merge-commit-hooks.sh +0 −4

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

Martin Ågren· Aug 2, 2019, 09:56 UTC · re: Martin Ågren · lore

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

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

Show changes to t/t7503-pre-commit-and-pre-merge-commit-hooks.sh +20 −40

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

Josh Steadmon· Aug 2, 2019, 22:18 UTC · re: Martin Ågren · lore

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

On 2019.08.02 11:56, Martin Ågren wrote:

Show 42 quoted lines

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

Josh Steadmon· Aug 5, 2019, 22:43 UTC · re: Josh Steadmon · lore

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

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

Josh Steadmon· Aug 5, 2019, 22:43 UTC · re: Josh Steadmon · lore

[PATCH v4 1/4] t7503: verify proper hook 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(-)

Show changes to t/t7503-pre-commit-hook.sh +89 −68

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

Junio C Hamano· Aug 6, 2019, 18:14 UTC · re: Josh Steadmon · lore

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

Josh Steadmon <steadmon@google.com> writes:

Show 7 quoted lines

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

Show 5 quoted lines

>  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?

Josh Steadmon· Aug 7, 2019, 18:11 UTC · re: Junio C Hamano · lore

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

On 2019.08.06 11:14, Junio C Hamano wrote:

Show 24 quoted lines

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

Josh Steadmon· Aug 5, 2019, 22:43 UTC · re: Josh Steadmon · lore

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

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

[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(-)

Show changes to 3 files +8 −4

Documentation/git-merge.txt, Documentation/merge-options.txt, builtin/merge.c

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

Josh Steadmon· Aug 5, 2019, 22:43 UTC · re: Josh Steadmon · lore

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

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

Show changes to 5 files +129 −1

Documentation/githooks.txt, builtin/merge.c, t/t7503-pre-commit-hook.sh, t/t7503-pre-commit-and-pre-merge-commit-hooks.sh, 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

Josh Steadmon· Aug 5, 2019, 22:43 UTC · re: Josh Steadmon · lore

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

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

Show changes to 3 files +43 −3

Documentation/githooks.txt, builtin/merge.c, t/t7503-pre-commit-and-pre-merge-commit-hooks.sh

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

Josh Steadmon· Aug 7, 2019, 18:57 UTC · re: Josh Steadmon · lore

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

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

Josh Steadmon· Aug 7, 2019, 18:57 UTC · re: Josh Steadmon · lore

[PATCH v5 1/4] t7503: verify proper hook 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(-)

Show changes to t/t7503-pre-commit-hook.sh +89 −68

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

Josh Steadmon· Aug 7, 2019, 18:57 UTC · re: Josh Steadmon · lore

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

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

[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(-)

Show changes to 3 files +8 −4

Documentation/git-merge.txt, Documentation/merge-options.txt, builtin/merge.c

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

Josh Steadmon· Aug 7, 2019, 18:57 UTC · re: Josh Steadmon · lore

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

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

Show changes to 5 files +129 −1

Documentation/githooks.txt, builtin/merge.c, t/t7503-pre-commit-hook.sh, t/t7503-pre-commit-and-pre-merge-commit-hooks.sh, 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

Josh Steadmon· Aug 7, 2019, 18:57 UTC · re: Josh Steadmon · lore

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

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

Show changes to 3 files +43 −3

Documentation/githooks.txt, builtin/merge.c, t/t7503-pre-commit-and-pre-merge-commit-hooks.sh

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

Martin Ågren· Aug 8, 2019, 13:08 UTC · re: Josh Steadmon · lore

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

On Wed, 7 Aug 2019 at 20:57, Josh Steadmon <steadmon@google.com> wrote:

Show 10 quoted lines

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