[PATCH 0/4] pre-merge hook

STALE3242d

9 messages, 3 authors, 2017-09-23 · open the first message on its own page

[PATCH 0/4] pre-merge hook

From: Michael J Gruber <hidden>
Date: 2017-09-22 12:04:38

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

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

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

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

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

-- 
2.14.1.909.g0fa57a0913

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

From: Michael J Gruber <hidden>
Date: 2017-09-22 12:04:27

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

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

From: Michael J Gruber <hidden>
Date: 2017-09-22 12:04:29

From: Michael J Gruber <redacted>

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 <redacted>
---
 Documentation/githooks.txt        |  7 +++++++
 builtin/merge.c                   | 11 +++++++++++
 templates/hooks--pre-merge.sample | 13 +++++++++++++
 3 files changed, 31 insertions(+)
 create mode 100755 templates/hooks--pre-merge.sample
diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index 1bb4f92d4d..85bedd208c 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -99,6 +99,13 @@ All the 'git commit' hooks are invoked with the environment
 variable `GIT_EDITOR=:` if the command will not bring up an editor
 to modify the commit message.
 
+pre-merge
+~~~~~~~~~
+
+This hook is invoked by 'git merge' when doing an automatic merge
+commit; it is equivalent to 'pre-commit' for a non-automatic commit
+for a merge.
+
 prepare-commit-msg
 ~~~~~~~~~~~~~~~~~~
 
diff --git a/builtin/merge.c b/builtin/merge.c
index ab5ffe85e8..de254d466b 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -769,6 +769,17 @@ static void write_merge_heads(struct commit_list *);
 static void prepare_to_commit(struct commit_list *remoteheads)
 {
 	struct strbuf msg = STRBUF_INIT;
+	const char *index_file = get_index_file();
+
+	if (run_commit_hook(0 < option_edit, index_file, "pre-merge", NULL))
+		abort_commit(remoteheads, NULL);
+	/*
+	 * Re-read the index as pre-merge hook could have updated it,
+	 * and write it out as a tree.  We must do this before we invoke
+	 * the editor and after we invoke run_status above.
+	 */
+	discard_cache();
+	read_cache_from(index_file);
 	strbuf_addbuf(&msg, &merge_msg);
 	strbuf_addch(&msg, '\n');
 	if (squash)
diff --git a/templates/hooks--pre-merge.sample b/templates/hooks--pre-merge.sample
new file mode 100755
index 0000000000..a6313e6d5c
--- /dev/null
+++ b/templates/hooks--pre-merge.sample
@@ -0,0 +1,13 @@
+#!/bin/sh
+#
+# An example hook script to verify what is about to be committed.
+# Called by "git merge" with no arguments.  The hook should
+# exit with non-zero status after issuing an appropriate message if
+# it wants to stop the commit.
+#
+# To enable this hook, rename this file to "pre-merge".
+
+. git-sh-setup
+test -x "$GIT_DIR/hooks/pre-commit" &&
+        exec "$GIT_DIR/hooks/pre-commit"
+:
-- 
2.14.1.909.g0fa57a0913

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

From: Michael J Gruber <hidden>
Date: 2017-09-22 12:04:33

From: Michael J Gruber <redacted>

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 <redacted>
---
 Documentation/git-merge.txt     | 2 +-
 Documentation/githooks.txt      | 2 +-
 Documentation/merge-options.txt | 4 ++++
 builtin/merge.c                 | 4 ++--
 4 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
index 4df6431c34..5f0e1768e1 100644
--- a/Documentation/git-merge.txt
+++ b/Documentation/git-merge.txt
@@ -10,7 +10,7 @@ SYNOPSIS
 --------
 [verse]
 'git merge' [-n] [--stat] [--no-commit] [--squash] [--[no-]edit]
-	[-s <strategy>] [-X <strategy-option>] [-S[<keyid>]]
+	[--no-verify] [-s <strategy>] [-X <strategy-option>] [-S[<keyid>]]
 	[--[no-]allow-unrelated-histories]
 	[--[no-]rerere-autoupdate] [-m <msg>] [<commit>...]
 'git merge' --abort
diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index 85bedd208c..969441d7a2 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -104,7 +104,7 @@ pre-merge
 
 This hook is invoked by 'git merge' when doing an automatic merge
 commit; it is equivalent to 'pre-commit' for a non-automatic commit
-for a merge.
+for a merge, and can be bypassed with the `--no-verify` option. 
 
 prepare-commit-msg
 ~~~~~~~~~~~~~~~~~~
diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
index 4e32304301..75019d5919 100644
--- a/Documentation/merge-options.txt
+++ b/Documentation/merge-options.txt
@@ -74,6 +74,10 @@ merge.
 With --no-squash perform the merge and commit the result. This
 option can be used to override --squash.
 
+--no-verify::
+	This option bypasses the pre-merge and commit-msg hooks.
+	See also linkgit:githooks[5].
+
 -s <strategy>::
 --strategy=<strategy>::
 	Use the given merge strategy; can be supplied more than
diff --git a/builtin/merge.c b/builtin/merge.c
index 7ba094ee87..c63510c199 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -237,7 +237,7 @@ static struct option builtin_merge_options[] = {
 	  N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" },
 	OPT_BOOL(0, "overwrite-ignore", &overwrite_ignore, N_("update ignored files (default)")),
 	OPT_BOOL(0, "signoff", &signoff, N_("add Signed-off-by:")),
-	OPT_BOOL(0, "no-verify", &no_verify, N_("bypass commit-msg hook")),
+	OPT_BOOL(0, "no-verify", &no_verify, N_("bypass pre-merge and commit-msg hooks")),
 	OPT_END()
 };
 
@@ -771,7 +771,7 @@ static void prepare_to_commit(struct commit_list *remoteheads)
 	struct strbuf msg = STRBUF_INIT;
 	const char *index_file = get_index_file();
 
-	if (run_commit_hook(0 < option_edit, index_file, "pre-merge", NULL))
+	if (!no_verify && run_commit_hook(0 < option_edit, index_file, "pre-merge", NULL))
 		abort_commit(remoteheads, NULL);
 	/*
 	 * Re-read the index as pre-merge hook could have updated it,
-- 
2.14.1.909.g0fa57a0913

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

From: Michael J Gruber <hidden>
Date: 2017-09-22 12:04:37

From: Michael J Gruber <redacted>

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

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

From: Stefan Beller <hidden>
Date: 2017-09-22 19:52:06

On Fri, Sep 22, 2017 at 5:04 AM, Michael J Gruber [off-list ref] wrote:
quoted hunk
From: Michael J Gruber <redacted>

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 <redacted>
---
 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.
quoted hunk
        strbuf_addbuf(&msg, &merge_msg);
        strbuf_addch(&msg, '\n');
        if (squash)
diff --git a/templates/hooks--pre-merge.sample b/templates/hooks--pre-merge.sample
new file mode 100755
index 0000000000..a6313e6d5c
--- /dev/null
+++ b/templates/hooks--pre-merge.sample
@@ -0,0 +1,13 @@
+#!/bin/sh
+#
+# An example hook script to verify what is about to be committed.
+# Called by "git merge" with no arguments.  The hook should
+# exit with non-zero status after issuing an appropriate message if
The message goes to stdout or sterr or both?
+# it wants to stop the commit.
nit: s/commit/merge commit/ maybe?

Thanks,
Stefan

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

From: Stefan Beller <hidden>
Date: 2017-09-22 19:55:20

On Fri, Sep 22, 2017 at 5:04 AM, Michael J Gruber [off-list ref] wrote:
f8b863598c ("builtin/merge: honor commit-msg hook for merges", 2017-09-07)
introduced the no-verify to merge for bypassing the commit-msg hook,
though in a different way from the implementation in commit.c.

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

Signed-off-by: Michael J Gruber <redacted>
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
quoted hunk
---
 builtin/merge.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/builtin/merge.c b/builtin/merge.c
index de254d466b..7ba094ee87 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -73,7 +73,7 @@ static int show_progress = -1;
 static int default_to_upstream = 1;
 static int signoff;
 static const char *sign_commit;
-static int verify_msg = 1;
+static int no_verify;

 static struct strategy all_strategy[] = {
        { "recursive",  DEFAULT_TWOHEAD | NO_TRIVIAL },
@@ -237,7 +237,7 @@ static struct option builtin_merge_options[] = {
          N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" },
        OPT_BOOL(0, "overwrite-ignore", &overwrite_ignore, N_("update ignored files (default)")),
        OPT_BOOL(0, "signoff", &signoff, N_("add Signed-off-by:")),
-       OPT_BOOL(0, "verify", &verify_msg, N_("verify commit-msg hook")),
+       OPT_BOOL(0, "no-verify", &no_verify, N_("bypass commit-msg hook")),
        OPT_END()
 };
@@ -798,7 +798,7 @@ static void prepare_to_commit(struct commit_list *remoteheads)
                        abort_commit(remoteheads, NULL);
        }

-       if (verify_msg && run_commit_hook(0 < option_edit, get_index_file(),
+       if (!no_verify && run_commit_hook(0 < option_edit, get_index_file(),
                                          "commit-msg",
                                          git_path_merge_msg(), NULL))
                abort_commit(remoteheads, NULL);
--
2.14.1.909.g0fa57a0913

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

From: Stefan Beller <hidden>
Date: 2017-09-22 20:01:40

On Fri, Sep 22, 2017 at 5:04 AM, Michael J Gruber [off-list ref] wrote:
quoted hunk
--- a/t/t7503-pre-commit-hook.sh
+++ b/t/t7503-pre-commit-hook.sh
-test_description='pre-commit hook'
+test_description='pre-commit and pre-merge hooks'
Tangent: do we also want to rename the file?
+test_expect_success '--no-verify with succeeding hook (merge)' '
The return value of the hook ought to not matter, so we'd rather want
to test for 'no side effect by hook on --no-verify' ?

+test_expect_success '--no-verify with failing hook (merge)' '
+
+       git checkout side &&
+       git merge --no-verify -m "merge master" master &&
+       git checkout master
+
+'
I assume the same (unsolved) issues apply to this hook as
to the commit-msg hook with 'git merge --continue'
Do we want a test for that (I am torn on that) ?

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

From: Martin Ågren <hidden>
Date: 2017-09-23 00:04:59

On 22 September 2017 at 14:04, Michael J Gruber [off-list ref] wrote:
From: Michael J Gruber <redacted>

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 <redacted>
[...]
quoted hunk
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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help