* [PATCH v2 1/4] git-merge: Honor pre-merge hook
2019-07-18 23:56 ` Josh Steadmon
@ 2019-07-18 22:57 ` Josh Steadmon
2019-07-29 20:00 ` Martin Ågren
2019-07-18 22:57 ` [PATCH v2 2/4] merge: do no-verify like commit Josh Steadmon
` (5 subsequent siblings)
6 siblings, 1 reply; 59+ messages in thread
From: Josh Steadmon @ 2019-07-18 22:57 UTC (permalink / raw)
To: git; +Cc: gitster, git, stefanbeller, martin.agren
From: Michael J Gruber <git@grubix.eu>
git-merge does not honor the pre-commit hook when doing automatic merge
commits, and for compatibility reasons this is going to stay.
Introduce a pre-merge hook which is called for an automatic merge commit
just like pre-commit is called for a non-automatic merge commit (or any
other commit).
[js: addressed review comments:
* clarified that hook should write messages to stderr
* only discard the index if the hook is actually present
]
Signed-off-by: Michael J Gruber <git@grubix.eu>
Signed-off-by: Josh Steadmon <steadmon@google.com>
---
Documentation/githooks.txt | 7 +++++++
builtin/merge.c | 12 ++++++++++++
templates/hooks--pre-merge.sample | 13 +++++++++++++
3 files changed, 32 insertions(+)
create mode 100755 templates/hooks--pre-merge.sample
diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index 786e778ab8..dcc6606d44 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -103,6 +103,13 @@ The default 'pre-commit' hook, when enabled--and with the
`hooks.allownonascii` config option unset or set to false--prevents
the use of non-ASCII filenames.
+pre-merge
+~~~~~~~~~
+
+This hook is invoked by 'git merge' when doing an automatic merge
+commit; it is equivalent to 'pre-commit' for a non-automatic commit
+for a merge.
+
prepare-commit-msg
~~~~~~~~~~~~~~~~~~
diff --git a/builtin/merge.c b/builtin/merge.c
index 6e99aead46..5cd7752191 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -816,6 +816,18 @@ static void write_merge_heads(struct commit_list *);
static void prepare_to_commit(struct commit_list *remoteheads)
{
struct strbuf msg = STRBUF_INIT;
+ const char *index_file = get_index_file();
+
+ if (run_commit_hook(0 < option_edit, index_file, "pre-merge", NULL))
+ abort_commit(remoteheads, NULL);
+ /*
+ * Re-read the index as pre-merge hook could have updated it,
+ * and write it out as a tree. We must do this before we invoke
+ * the editor and after we invoke run_status above.
+ */
+ if (find_hook("pre-merge"))
+ discard_cache();
+ read_cache_from(index_file);
strbuf_addbuf(&msg, &merge_msg);
if (squash)
BUG("the control must not reach here under --squash");
diff --git a/templates/hooks--pre-merge.sample b/templates/hooks--pre-merge.sample
new file mode 100755
index 0000000000..f459b3db44
--- /dev/null
+++ b/templates/hooks--pre-merge.sample
@@ -0,0 +1,13 @@
+#!/bin/sh
+#
+# An example hook script to verify what is about to be committed.
+# Called by "git merge" with no arguments. The hook should
+# exit with non-zero status after issuing an appropriate message to
+# stderr if it wants to stop the merge commit.
+#
+# To enable this hook, rename this file to "pre-merge".
+
+. git-sh-setup
+test -x "$GIT_DIR/hooks/pre-commit" &&
+ exec "$GIT_DIR/hooks/pre-commit"
+:
--
2.22.0.657.g960e92d24f-goog
^ permalink raw reply related [flat|nested] 59+ messages in thread* Re: [PATCH v2 1/4] git-merge: Honor pre-merge hook
2019-07-18 22:57 ` [PATCH v2 1/4] git-merge: Honor " Josh Steadmon
@ 2019-07-29 20:00 ` Martin Ågren
0 siblings, 0 replies; 59+ messages in thread
From: Martin Ågren @ 2019-07-29 20:00 UTC (permalink / raw)
To: Josh Steadmon
Cc: Git Mailing List, Junio C Hamano, Michael J Gruber, stefanbeller
On Fri, 19 Jul 2019 at 00:57, Josh Steadmon <steadmon@google.com> wrote:
> --- /dev/null
> +++ b/templates/hooks--pre-merge.sample
> @@ -0,0 +1,13 @@
> +#!/bin/sh
> +#
> +# An example hook script to verify what is about to be committed.
> +# Called by "git merge" with no arguments. The hook should
> +# exit with non-zero status after issuing an appropriate message to
> +# stderr if it wants to stop the merge commit.
> +#
> +# To enable this hook, rename this file to "pre-merge".
> +
> +. git-sh-setup
> +test -x "$GIT_DIR/hooks/pre-commit" &&
> + exec "$GIT_DIR/hooks/pre-commit"
> +:
I'm pretty certain that my comment here was with respect to the ":",
which made me think "hmmm, won't : always exit successfully?". My
slightly less ignorant self of today answers "sure, but we won't always
get that far" -- the "exec" is pretty important, and it will cause the
current shell to be replaced with the hook invocation. So however the
pre-commit hook decides to exit, that'll be our exit status.
I retract my comment from back then.
Martin
^ permalink raw reply [flat|nested] 59+ messages in thread
* [PATCH v2 2/4] merge: do no-verify like commit
2019-07-18 23:56 ` Josh Steadmon
2019-07-18 22:57 ` [PATCH v2 1/4] git-merge: Honor " Josh Steadmon
@ 2019-07-18 22:57 ` Josh Steadmon
2019-07-18 22:57 ` [PATCH v2 3/4] merge: --no-verify to bypass pre-merge hook Josh Steadmon
` (4 subsequent siblings)
6 siblings, 0 replies; 59+ messages in thread
From: Josh Steadmon @ 2019-07-18 22:57 UTC (permalink / raw)
To: git; +Cc: gitster, git, stefanbeller, martin.agren
From: Michael J Gruber <git@grubix.eu>
f8b863598c ("builtin/merge: honor commit-msg hook for merges", 2017-09-07)
introduced the no-verify flag to merge for bypassing the commit-msg
hook, though in a different way from the implementation in commit.c.
Change the implementation in merge.c to be the same as in commit.c so
that both do the same in the same way. This also changes the output of
"git merge --help" to be more clear that the hook return code is
respected by default.
[js: reworded commit message, and moved documentation changes from patch
3/4 to this commit.]
Signed-off-by: Michael J Gruber <git@grubix.eu>
Signed-off-by: Josh Steadmon <steadmon@google.com>
---
Documentation/git-merge.txt | 2 +-
Documentation/merge-options.txt | 4 ++++
builtin/merge.c | 6 +++---
3 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
index c01cfa6595..f71ed1d0f9 100644
--- a/Documentation/git-merge.txt
+++ b/Documentation/git-merge.txt
@@ -10,7 +10,7 @@ SYNOPSIS
--------
[verse]
'git merge' [-n] [--stat] [--no-commit] [--squash] [--[no-]edit]
- [-s <strategy>] [-X <strategy-option>] [-S[<keyid>]]
+ [--no-verify] [-s <strategy>] [-X <strategy-option>] [-S[<keyid>]]
[--[no-]allow-unrelated-histories]
[--[no-]rerere-autoupdate] [-m <msg>] [-F <file>] [<commit>...]
'git merge' --abort
diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
index 79a00d2a4a..d6a9f4b96f 100644
--- a/Documentation/merge-options.txt
+++ b/Documentation/merge-options.txt
@@ -105,6 +105,10 @@ option can be used to override --squash.
+
With --squash, --commit is not allowed, and will fail.
+--no-verify::
+ This option bypasses the pre-merge and commit-msg hooks.
+ See also linkgit:githooks[5].
+
-s <strategy>::
--strategy=<strategy>::
Use the given merge strategy; can be supplied more than
diff --git a/builtin/merge.c b/builtin/merge.c
index 5cd7752191..5bbef203f3 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -81,7 +81,7 @@ static int show_progress = -1;
static int default_to_upstream = 1;
static int signoff;
static const char *sign_commit;
-static int verify_msg = 1;
+static int no_verify;
static struct strategy all_strategy[] = {
{ "recursive", DEFAULT_TWOHEAD | NO_TRIVIAL },
@@ -287,7 +287,7 @@ static struct option builtin_merge_options[] = {
N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" },
OPT_BOOL(0, "overwrite-ignore", &overwrite_ignore, N_("update ignored files (default)")),
OPT_BOOL(0, "signoff", &signoff, N_("add Signed-off-by:")),
- OPT_BOOL(0, "verify", &verify_msg, N_("verify commit-msg hook")),
+ OPT_BOOL(0, "no-verify", &no_verify, N_("bypass commit-msg hook")),
OPT_END()
};
@@ -854,7 +854,7 @@ static void prepare_to_commit(struct commit_list *remoteheads)
abort_commit(remoteheads, NULL);
}
- if (verify_msg && run_commit_hook(0 < option_edit, get_index_file(),
+ if (!no_verify && run_commit_hook(0 < option_edit, get_index_file(),
"commit-msg",
git_path_merge_msg(the_repository), NULL))
abort_commit(remoteheads, NULL);
--
2.22.0.657.g960e92d24f-goog
^ permalink raw reply related [flat|nested] 59+ messages in thread* [PATCH v2 3/4] merge: --no-verify to bypass pre-merge hook
2019-07-18 23:56 ` Josh Steadmon
2019-07-18 22:57 ` [PATCH v2 1/4] git-merge: Honor " Josh Steadmon
2019-07-18 22:57 ` [PATCH v2 2/4] merge: do no-verify like commit Josh Steadmon
@ 2019-07-18 22:57 ` Josh Steadmon
2019-07-29 20:02 ` Martin Ågren
2019-07-18 22:57 ` [PATCH v2 4/4] t7503: add tests for pre-merge-hook Josh Steadmon
` (3 subsequent siblings)
6 siblings, 1 reply; 59+ messages in thread
From: Josh Steadmon @ 2019-07-18 22:57 UTC (permalink / raw)
To: git; +Cc: gitster, git, stefanbeller, martin.agren
From: Michael J Gruber <git@grubix.eu>
Analogous to commit, introduce a '--no-verify' option which bypasses the
pre-merge hook. The shorthand '-n' is taken by the (non-existing)
'--no-stat' already.
[js: cleaned up trailing whitespace, moved some documentation changes
from this commit to 2/4.]
Signed-off-by: Michael J Gruber <git@grubix.eu>
Signed-off-by: Josh Steadmon <steadmon@google.com>
---
Documentation/githooks.txt | 2 +-
builtin/merge.c | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index dcc6606d44..6a93478dcf 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -108,7 +108,7 @@ pre-merge
This hook is invoked by 'git merge' when doing an automatic merge
commit; it is equivalent to 'pre-commit' for a non-automatic commit
-for a merge.
+for a merge, and can be bypassed with the `--no-verify` option.
prepare-commit-msg
~~~~~~~~~~~~~~~~~~
diff --git a/builtin/merge.c b/builtin/merge.c
index 5bbef203f3..5ed3472c84 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -287,7 +287,7 @@ static struct option builtin_merge_options[] = {
N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" },
OPT_BOOL(0, "overwrite-ignore", &overwrite_ignore, N_("update ignored files (default)")),
OPT_BOOL(0, "signoff", &signoff, N_("add Signed-off-by:")),
- OPT_BOOL(0, "no-verify", &no_verify, N_("bypass commit-msg hook")),
+ OPT_BOOL(0, "no-verify", &no_verify, N_("bypass pre-merge and commit-msg hooks")),
OPT_END()
};
@@ -818,7 +818,7 @@ static void prepare_to_commit(struct commit_list *remoteheads)
struct strbuf msg = STRBUF_INIT;
const char *index_file = get_index_file();
- if (run_commit_hook(0 < option_edit, index_file, "pre-merge", NULL))
+ if (!no_verify && run_commit_hook(0 < option_edit, index_file, "pre-merge", NULL))
abort_commit(remoteheads, NULL);
/*
* Re-read the index as pre-merge hook could have updated it,
--
2.22.0.657.g960e92d24f-goog
^ permalink raw reply related [flat|nested] 59+ messages in thread* Re: [PATCH v2 3/4] merge: --no-verify to bypass pre-merge hook
2019-07-18 22:57 ` [PATCH v2 3/4] merge: --no-verify to bypass pre-merge hook Josh Steadmon
@ 2019-07-29 20:02 ` Martin Ågren
2019-07-29 23:33 ` Josh Steadmon
0 siblings, 1 reply; 59+ messages in thread
From: Martin Ågren @ 2019-07-29 20:02 UTC (permalink / raw)
To: Josh Steadmon
Cc: Git Mailing List, Junio C Hamano, Michael J Gruber, stefanbeller
On Fri, 19 Jul 2019 at 00:57, Josh Steadmon <steadmon@google.com> wrote:
> From: Michael J Gruber <git@grubix.eu>
>
> Analogous to commit, introduce a '--no-verify' option which bypasses the
> pre-merge hook. The shorthand '-n' is taken by the (non-existing)
> '--no-stat' already.
I don't understand this "(non-existing)". I realize this message is two
years old, maybe lots older still, and I haven't dug. But at least
*now*, "-n" seems to behave like "--no-stat", suppressing "--stat", from
my simple testing.
Martin
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH v2 3/4] merge: --no-verify to bypass pre-merge hook
2019-07-29 20:02 ` Martin Ågren
@ 2019-07-29 23:33 ` Josh Steadmon
0 siblings, 0 replies; 59+ messages in thread
From: Josh Steadmon @ 2019-07-29 23:33 UTC (permalink / raw)
To: Martin Ågren; +Cc: Git Mailing List, Junio C Hamano, stefanbeller
On 2019.07.29 22:02, Martin Ågren wrote:
> On Fri, 19 Jul 2019 at 00:57, Josh Steadmon <steadmon@google.com> wrote:
> > From: Michael J Gruber <git@grubix.eu>
> >
> > Analogous to commit, introduce a '--no-verify' option which bypasses the
> > pre-merge hook. The shorthand '-n' is taken by the (non-existing)
> > '--no-stat' already.
>
> I don't understand this "(non-existing)". I realize this message is two
> years old, maybe lots older still, and I haven't dug. But at least
> *now*, "-n" seems to behave like "--no-stat", suppressing "--stat", from
> my simple testing.
Fixed in V3.
^ permalink raw reply [flat|nested] 59+ messages in thread
* [PATCH v2 4/4] t7503: add tests for pre-merge-hook
2019-07-18 23:56 ` Josh Steadmon
` (2 preceding siblings ...)
2019-07-18 22:57 ` [PATCH v2 3/4] merge: --no-verify to bypass pre-merge hook Josh Steadmon
@ 2019-07-18 22:57 ` Josh Steadmon
2019-07-29 20:04 ` Martin Ågren
2019-07-29 20:09 ` [PATCH v2 0/4] pre-merge hook Martin Ågren
` (2 subsequent siblings)
6 siblings, 1 reply; 59+ messages in thread
From: Josh Steadmon @ 2019-07-18 22:57 UTC (permalink / raw)
To: git; +Cc: gitster, git, stefanbeller, martin.agren
From: Michael J Gruber <git@grubix.eu>
Add tests which make sure that the pre-merge-hook is called when
present, allows/disallows merge commits depending on its return value
and is suppressed by "--no-verify".
[js: renamed test as suggested in review comments]
Signed-off-by: Michael J Gruber <git@grubix.eu>
Signed-off-by: Josh Steadmon <steadmon@google.com>
---
...> t7503-pre-commit-and-pre-merge-hooks.sh} | 66 ++++++++++++++++++-
1 file changed, 65 insertions(+), 1 deletion(-)
rename t/{t7503-pre-commit-hook.sh => t7503-pre-commit-and-pre-merge-hooks.sh} (67%)
diff --git a/t/t7503-pre-commit-hook.sh b/t/t7503-pre-commit-and-pre-merge-hooks.sh
similarity index 67%
rename from t/t7503-pre-commit-hook.sh
rename to t/t7503-pre-commit-and-pre-merge-hooks.sh
index 984889b39d..36ae87f7ef 100755
--- a/t/t7503-pre-commit-hook.sh
+++ b/t/t7503-pre-commit-and-pre-merge-hooks.sh
@@ -1,9 +1,22 @@
#!/bin/sh
-test_description='pre-commit hook'
+test_description='pre-commit and pre-merge hooks'
. ./test-lib.sh
+test_expect_success 'root commit' '
+
+ echo "root" > file &&
+ git add file &&
+ git commit -m "zeroth" &&
+ git checkout -b side &&
+ echo "foo" > foo &&
+ git add foo &&
+ git commit -m "make it non-ff" &&
+ git checkout master
+
+'
+
test_expect_success 'with no hook' '
echo "foo" > file &&
@@ -12,6 +25,14 @@ test_expect_success 'with no hook' '
'
+test_expect_success 'with no hook (merge)' '
+
+ git checkout side &&
+ git merge -m "merge master" master &&
+ git checkout master
+
+'
+
test_expect_success '--no-verify with no hook' '
echo "bar" > file &&
@@ -20,15 +41,25 @@ test_expect_success '--no-verify with no hook' '
'
+test_expect_success '--no-verify with no hook (merge)' '
+
+ git checkout side &&
+ git merge --no-verify -m "merge master" master &&
+ git checkout master
+
+'
+
# now install hook that always succeeds
HOOKDIR="$(git rev-parse --git-dir)/hooks"
HOOK="$HOOKDIR/pre-commit"
+MERGEHOOK="$HOOKDIR/pre-merge"
mkdir -p "$HOOKDIR"
cat > "$HOOK" <<EOF
#!/bin/sh
exit 0
EOF
chmod +x "$HOOK"
+cp -p "$HOOK" "$MERGEHOOK"
test_expect_success 'with succeeding hook' '
@@ -38,6 +69,14 @@ test_expect_success 'with succeeding hook' '
'
+test_expect_success 'with succeeding hook (merge)' '
+
+ git checkout side &&
+ git merge -m "merge master" master &&
+ git checkout master
+
+'
+
test_expect_success '--no-verify with succeeding hook' '
echo "even more" >> file &&
@@ -46,11 +85,20 @@ test_expect_success '--no-verify with succeeding hook' '
'
+test_expect_success '--no-verify with succeeding hook (merge)' '
+
+ git checkout side &&
+ git merge --no-verify -m "merge master" master &&
+ git checkout master
+
+'
+
# now a hook that fails
cat > "$HOOK" <<EOF
#!/bin/sh
exit 1
EOF
+cp -p "$HOOK" "$MERGEHOOK"
test_expect_success 'with failing hook' '
@@ -68,6 +116,22 @@ test_expect_success '--no-verify with failing hook' '
'
+test_expect_success 'with failing hook (merge)' '
+
+ git checkout side &&
+ test_must_fail git merge -m "merge master" master &&
+ git checkout master
+
+'
+
+test_expect_success '--no-verify with failing hook (merge)' '
+
+ git checkout side &&
+ git merge --no-verify -m "merge master" master &&
+ git checkout master
+
+'
+
chmod -x "$HOOK"
test_expect_success POSIXPERM 'with non-executable hook' '
--
2.22.0.657.g960e92d24f-goog
^ permalink raw reply related [flat|nested] 59+ messages in thread* Re: [PATCH v2 4/4] t7503: add tests for pre-merge-hook
2019-07-18 22:57 ` [PATCH v2 4/4] t7503: add tests for pre-merge-hook Josh Steadmon
@ 2019-07-29 20:04 ` Martin Ågren
2019-07-29 23:43 ` Josh Steadmon
0 siblings, 1 reply; 59+ messages in thread
From: Martin Ågren @ 2019-07-29 20:04 UTC (permalink / raw)
To: Josh Steadmon
Cc: Git Mailing List, Junio C Hamano, Michael J Gruber, stefanbeller
On Fri, 19 Jul 2019 at 00:57, Josh Steadmon <steadmon@google.com> wrote:
> +test_expect_success '--no-verify with succeeding hook (merge)' '
> +
> + git checkout side &&
> + git merge --no-verify -m "merge master" master &&
> + git checkout master
> +
> +'
This test doesn't even try to verify that the hook was actually ignored.
That left me puzzled for a while...
> +test_expect_success '--no-verify with failing hook (merge)' '
> +
> + git checkout side &&
> + git merge --no-verify -m "merge master" master &&
> + git checkout master
> +
> +'
... but this would then (most likely) fail, so we would notice
something's wrong.
This script seems to me like if it passes 100%, we can be fairly sure
we're ok, but if some individual test fails, we shouldn't be surprised
if its oneline description is a bit off compared to the bug. Similarly,
quite a few tests could pass, despite their oneline description inducing
the thought of "but surely, if /that/ were the problem, /those/ tests
would fail".
Anyway, I realize this is just following the existing approach. I guess
you could argue it has served us well for a long time.
I would probably prefer seeing the various hunks in this patch being
squashed into the relevant commits (1/4 vs 3/4) to make those patches
more self-describing.
Martin
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH v2 4/4] t7503: add tests for pre-merge-hook
2019-07-29 20:04 ` Martin Ågren
@ 2019-07-29 23:43 ` Josh Steadmon
2019-07-30 7:13 ` Martin Ågren
0 siblings, 1 reply; 59+ messages in thread
From: Josh Steadmon @ 2019-07-29 23:43 UTC (permalink / raw)
To: Martin Ågren; +Cc: Git Mailing List, Junio C Hamano, stefanbeller
On 2019.07.29 22:04, Martin Ågren wrote:
> On Fri, 19 Jul 2019 at 00:57, Josh Steadmon <steadmon@google.com> wrote:
> > +test_expect_success '--no-verify with succeeding hook (merge)' '
> > +
> > + git checkout side &&
> > + git merge --no-verify -m "merge master" master &&
> > + git checkout master
> > +
> > +'
>
> This test doesn't even try to verify that the hook was actually ignored.
> That left me puzzled for a while...
>
> > +test_expect_success '--no-verify with failing hook (merge)' '
> > +
> > + git checkout side &&
> > + git merge --no-verify -m "merge master" master &&
> > + git checkout master
> > +
> > +'
>
> ... but this would then (most likely) fail, so we would notice
> something's wrong.
>
> This script seems to me like if it passes 100%, we can be fairly sure
> we're ok, but if some individual test fails, we shouldn't be surprised
> if its oneline description is a bit off compared to the bug. Similarly,
> quite a few tests could pass, despite their oneline description inducing
> the thought of "but surely, if /that/ were the problem, /those/ tests
> would fail".
>
> Anyway, I realize this is just following the existing approach. I guess
> you could argue it has served us well for a long time.
>
> I would probably prefer seeing the various hunks in this patch being
> squashed into the relevant commits (1/4 vs 3/4) to make those patches
> more self-describing.
Will squash these as you said in V3. Will also think about whether
another test approach would make more sense here.
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH v2 4/4] t7503: add tests for pre-merge-hook
2019-07-29 23:43 ` Josh Steadmon
@ 2019-07-30 7:13 ` Martin Ågren
2019-07-31 18:34 ` Josh Steadmon
0 siblings, 1 reply; 59+ messages in thread
From: Martin Ågren @ 2019-07-30 7:13 UTC (permalink / raw)
To: Josh Steadmon, Martin Ågren, Git Mailing List,
Junio C Hamano, stefanbeller
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
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH v2 4/4] t7503: add tests for pre-merge-hook
2019-07-30 7:13 ` Martin Ågren
@ 2019-07-31 18:34 ` Josh Steadmon
0 siblings, 0 replies; 59+ messages in thread
From: Josh Steadmon @ 2019-07-31 18:34 UTC (permalink / raw)
To: Martin Ågren; +Cc: Git Mailing List, Junio C Hamano, stefanbeller
On 2019.07.30 09:13, Martin Ågren wrote:
> On Tue, 30 Jul 2019 at 01:43, Josh Steadmon <steadmon@google.com> wrote:
> >
> > On 2019.07.29 22:04, Martin Ågren wrote:
> > > This script seems to me like if it passes 100%, we can be fairly sure
> > > we're ok, but [...]
>
> > Will squash these as you said in V3. Will also think about whether
> > another test approach would make more sense here.
>
> Thinking a bit more about this, this test uses two identical hooks, runs
> some commands and verifies that "the" hook was run (or not, with
> --no-verify). If the implementation started calling the wrong hook
> (pre-commit / pre-merge) or both hooks, we wouldn't notice.
>
> Please forgive my braindump below, I'm on the run so I'm just throwing
> this out there:
>
> Perhaps (first do some modernizing of this script, to protect various
> setup steps, use "write_script", etc, then) make the existing hook a
> tiny bit pre-commit-specific, e.g., by doing something like "echo
> pre-commit >>executed-hooks", then at select places check "test_cmp
> executed-hooks pre-commit" (against "echo pre-commit >pre-commit"),
> "test_path_is_missing executed-hooks", and so on, coupled with some
> "test_when_finished 'rm -f executed_hooks'". Then the tests added for
> this series would use a very similar hook, appending and checking for
> "pre-merge[-commit]", That should make us fairly certain that we're
> running precisely the wanted hook, I think.
>
> Martin
That sounds like a reasonable approach, thank you for the suggestions. I
will work on this for V3.
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH v2 0/4] pre-merge hook
2019-07-18 23:56 ` Josh Steadmon
` (3 preceding siblings ...)
2019-07-18 22:57 ` [PATCH v2 4/4] t7503: add tests for pre-merge-hook Josh Steadmon
@ 2019-07-29 20:09 ` Martin Ågren
2019-07-29 23:29 ` Josh Steadmon
2019-07-29 20:29 ` Martin Ågren
2019-08-01 22:20 ` [PATCH v3 0/4] pre-merge-commit hook Josh Steadmon
6 siblings, 1 reply; 59+ messages in thread
From: Martin Ågren @ 2019-07-29 20:09 UTC (permalink / raw)
To: Josh Steadmon
Cc: Git Mailing List, Junio C Hamano, Michael J Gruber, stefanbeller
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
^ permalink raw reply [flat|nested] 59+ messages in thread* Re: [PATCH v2 0/4] pre-merge hook
2019-07-29 20:09 ` [PATCH v2 0/4] pre-merge hook Martin Ågren
@ 2019-07-29 23:29 ` Josh Steadmon
0 siblings, 0 replies; 59+ messages in thread
From: Josh Steadmon @ 2019-07-29 23:29 UTC (permalink / raw)
To: Martin Ågren
Cc: Git Mailing List, Junio C Hamano, Michael J Gruber, stefanbeller
On 2019.07.29 22:09, Martin Ågren wrote:
> On Fri, 19 Jul 2019 at 01:56, Josh Steadmon <steadmon@google.com> wrote:
> > * Martin's objection on 1/4 that the sample hook would always exit
> > successfully is (I believe) incorrect. To test this, I ran
> > "/bin/true && exec test 0 == 1" in a /bin/sh subshell, and it
> > correctly had a non-zero exit status.
>
> I retract my comment on this. It was incorrect, indeed.
>
> > git-merge: Honor pre-merge hook
>
> Nit: s/H/h/
Fixed in V3.
> > merge: do no-verify like commit
>
> Nit (maybe just me): this could be patch 1/N, before getting started
> with the real focus of this series.
Agreed, I found the ordering strange as well, but wanted to keep it the
same as V1 to start with. Fixed in V3.
>
>
> Martin
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH v2 0/4] pre-merge hook
2019-07-18 23:56 ` Josh Steadmon
` (4 preceding siblings ...)
2019-07-29 20:09 ` [PATCH v2 0/4] pre-merge hook Martin Ågren
@ 2019-07-29 20:29 ` Martin Ågren
2019-07-29 23:39 ` Josh Steadmon
2019-08-01 22:20 ` [PATCH v3 0/4] pre-merge-commit hook Josh Steadmon
6 siblings, 1 reply; 59+ messages in thread
From: Martin Ågren @ 2019-07-29 20:29 UTC (permalink / raw)
To: Josh Steadmon
Cc: Git Mailing List, Junio C Hamano, Michael J Gruber, stefanbeller
On Fri, 19 Jul 2019 at 01:56, Josh Steadmon <steadmon@google.com> wrote:
> This series revives an old suggestion [...] to make merge honor
> pre-commit hook or a separate pre-merge hook. Since pre-commit does not
> receive any arguments (which the hook could use tell between commit and
> merge) and in order to keep existing behaviour (as opposed to the other
> patch) this series introduces a pre-merge hook, with a sample hook that
> simply calls the pre-commit hook.
Argh, I wanted to comment on this in the cover letter, but forgot and
just left a bunch of nits.
I wonder if we might ever regret naming this "pre-merge" and not, e.g.,
"pre-merge-commit". There's the pre-rebase hook which runs before
git-rebase even starts actually rebasing, so I could well imagine a
"pre-merge" hook which would get called before merging starts.
I'm also pondering whether there should be an "automatic" in there, but
"pre-automatic-merge-commit" looks a bit awkward. Anyway, I'm not even
sure an "automatic merge commit" is a well-defined thing. I can figure
out what it pretty certainly means, but it's not crystal clear. There
might be a need for some more discussion in the added docs for what this
new hook is for and how it compares to pre-commit. Right now, the
proposed docs suggests they're equivalent in a way, but I think that's a
bit confusing -- they are certainly not synonyms, and they'd never both
be called, for example. They can be used for the same purpose in
different scenarios, sure.
I tried coming up with some proposed docs for githooks.txt, but didn't
feel I achieved much of value. :-/
Martin
^ permalink raw reply [flat|nested] 59+ messages in thread* Re: [PATCH v2 0/4] pre-merge hook
2019-07-29 20:29 ` Martin Ågren
@ 2019-07-29 23:39 ` Josh Steadmon
0 siblings, 0 replies; 59+ messages in thread
From: Josh Steadmon @ 2019-07-29 23:39 UTC (permalink / raw)
To: Martin Ågren; +Cc: Git Mailing List, Junio C Hamano, stefanbeller
On 2019.07.29 22:29, Martin Ågren wrote:
> On Fri, 19 Jul 2019 at 01:56, Josh Steadmon <steadmon@google.com> wrote:
> > This series revives an old suggestion [...] to make merge honor
> > pre-commit hook or a separate pre-merge hook. Since pre-commit does not
> > receive any arguments (which the hook could use tell between commit and
> > merge) and in order to keep existing behaviour (as opposed to the other
> > patch) this series introduces a pre-merge hook, with a sample hook that
> > simply calls the pre-commit hook.
>
> Argh, I wanted to comment on this in the cover letter, but forgot and
> just left a bunch of nits.
>
> I wonder if we might ever regret naming this "pre-merge" and not, e.g.,
> "pre-merge-commit". There's the pre-rebase hook which runs before
> git-rebase even starts actually rebasing, so I could well imagine a
> "pre-merge" hook which would get called before merging starts.
"pre-merge-commit" sounds reasonable to me. I'll wait for a bit before
sending out V3 in case anyone else wants to chime in here though.
> I'm also pondering whether there should be an "automatic" in there, but
> "pre-automatic-merge-commit" looks a bit awkward. Anyway, I'm not even
> sure an "automatic merge commit" is a well-defined thing. I can figure
> out what it pretty certainly means, but it's not crystal clear. There
> might be a need for some more discussion in the added docs for what this
> new hook is for and how it compares to pre-commit. Right now, the
> proposed docs suggests they're equivalent in a way, but I think that's a
> bit confusing -- they are certainly not synonyms, and they'd never both
> be called, for example. They can be used for the same purpose in
> different scenarios, sure.
>
> I tried coming up with some proposed docs for githooks.txt, but didn't
> feel I achieved much of value. :-/
>
> Martin
^ permalink raw reply [flat|nested] 59+ messages in thread
* [PATCH v3 0/4] pre-merge-commit hook
2019-07-18 23:56 ` Josh Steadmon
` (5 preceding siblings ...)
2019-07-29 20:29 ` Martin Ågren
@ 2019-08-01 22:20 ` Josh Steadmon
2019-08-01 22:20 ` [PATCH v3 1/4] t7503: verify proper hook execution Josh Steadmon
` (5 more replies)
6 siblings, 6 replies; 59+ messages in thread
From: Josh Steadmon @ 2019-08-01 22:20 UTC (permalink / raw)
To: git; +Cc: gitster, git, martin.agren
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
^ permalink raw reply [flat|nested] 59+ messages in thread* [PATCH v3 1/4] t7503: verify proper hook execution
2019-08-01 22:20 ` [PATCH v3 0/4] pre-merge-commit hook Josh Steadmon
@ 2019-08-01 22:20 ` Josh Steadmon
2019-08-02 9:43 ` Martin Ågren
2019-08-01 22:20 ` [PATCH v3 2/4] merge: do no-verify like commit Josh Steadmon
` (4 subsequent siblings)
5 siblings, 1 reply; 59+ messages in thread
From: Josh Steadmon @ 2019-08-01 22:20 UTC (permalink / raw)
To: git; +Cc: gitster, git, martin.agren
t7503 did not verify that the expected hooks actually ran during
testing. Fix that by making the hook scripts write their $0 into a file
so that we can compare actual execution vs. expected execution.
While we're at it, do some test style cleanups, such as using
write_script() and doing setup inside a test_expect_success block.
Signed-off-by: Josh Steadmon <steadmon@google.com>
---
t/t7503-pre-commit-hook.sh | 162 +++++++++++++++++++++----------------
1 file changed, 94 insertions(+), 68 deletions(-)
diff --git a/t/t7503-pre-commit-hook.sh b/t/t7503-pre-commit-hook.sh
index 984889b39d..500bdd97c2 100755
--- a/t/t7503-pre-commit-hook.sh
+++ b/t/t7503-pre-commit-hook.sh
@@ -4,124 +4,149 @@ test_description='pre-commit hook'
. ./test-lib.sh
-test_expect_success 'with no hook' '
+HOOKDIR="$(git rev-parse --git-dir)/hooks"
+PRECOMMIT="$HOOKDIR/pre-commit"
+
+# Prepare sample scripts that write their $0 to actual_hooks
+test_expect_success 'sample script setup' '
+ mkdir -p "$HOOKDIR" &&
+ write_script "$HOOKDIR/success.sample" <<-\EOF &&
+ echo $0 >>actual_hooks
+ exit 0
+ EOF
+ write_script "$HOOKDIR/fail.sample" <<-\EOF &&
+ echo $0 >>actual_hooks
+ exit 1
+ EOF
+ write_script "$HOOKDIR/require-prefix.sample" <<-\EOF &&
+ echo $0 >>actual_hooks
+ test $GIT_PREFIX = "success/"
+ EOF
+ write_script "$HOOKDIR/check-author.sample" <<-\EOF
+ echo $0 >>actual_hooks
+ test "$GIT_AUTHOR_NAME" = "New Author" &&
+ test "$GIT_AUTHOR_EMAIL" = "newauthor@example.com"
+ EOF
+'
- echo "foo" > file &&
+test_expect_success 'with no hook' '
+ test_when_finished "rm -f expected_hooks actual_hooks" &&
+ touch expected_hooks actual_hooks &&
+ echo "foo" >file &&
git add file &&
- git commit -m "first"
-
+ git commit -m "first" &&
+ test_cmp expected_hooks actual_hooks
'
test_expect_success '--no-verify with no hook' '
-
- echo "bar" > file &&
+ test_when_finished "rm -f expected_hooks actual_hooks" &&
+ touch expected_hooks actual_hooks &&
+ echo "bar" >file &&
git add file &&
- git commit --no-verify -m "bar"
-
+ git commit --no-verify -m "bar" &&
+ test_cmp expected_hooks actual_hooks
'
-# now install hook that always succeeds
-HOOKDIR="$(git rev-parse --git-dir)/hooks"
-HOOK="$HOOKDIR/pre-commit"
-mkdir -p "$HOOKDIR"
-cat > "$HOOK" <<EOF
-#!/bin/sh
-exit 0
-EOF
-chmod +x "$HOOK"
-
test_expect_success 'with succeeding hook' '
-
- echo "more" >> file &&
+ test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks" &&
+ ln -s "success.sample" "$PRECOMMIT" &&
+ touch actual_hooks &&
+ echo "$PRECOMMIT" >expected_hooks &&
+ echo "more" >>file &&
git add file &&
- git commit -m "more"
-
+ git commit -m "more" &&
+ test_cmp expected_hooks actual_hooks
'
test_expect_success '--no-verify with succeeding hook' '
-
- echo "even more" >> file &&
+ test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks" &&
+ ln -s "success.sample" "$PRECOMMIT" &&
+ touch expected_hooks actual_hooks &&
+ echo "even more" >>file &&
git add file &&
- git commit --no-verify -m "even more"
-
+ git commit --no-verify -m "even more" &&
+ test_cmp expected_hooks actual_hooks
'
-# now a hook that fails
-cat > "$HOOK" <<EOF
-#!/bin/sh
-exit 1
-EOF
-
test_expect_success 'with failing hook' '
-
- echo "another" >> file &&
+ test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks" &&
+ ln -s "fail.sample" "$PRECOMMIT" &&
+ touch actual_hooks &&
+ echo "$PRECOMMIT" >expected_hooks &&
+ echo "another" >>file &&
git add file &&
- test_must_fail git commit -m "another"
-
+ test_must_fail git commit -m "another" &&
+ test_cmp expected_hooks actual_hooks
'
test_expect_success '--no-verify with failing hook' '
-
- echo "stuff" >> file &&
+ test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks" &&
+ ln -s "fail.sample" "$PRECOMMIT" &&
+ touch expected_hooks actual_hooks &&
+ echo "stuff" >>file &&
git add file &&
- git commit --no-verify -m "stuff"
-
+ git commit --no-verify -m "stuff" &&
+ test_cmp expected_hooks actual_hooks
'
-chmod -x "$HOOK"
test_expect_success POSIXPERM 'with non-executable hook' '
-
- echo "content" >> file &&
+ test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks; chmod +x \"$HOOKDIR/fail.sample\"" &&
+ ln -s "fail.sample" "$PRECOMMIT" &&
+ chmod -x "$HOOKDIR/fail.sample" &&
+ touch expected_hooks actual_hooks &&
+ echo "content" >>file &&
git add file &&
- git commit -m "content"
-
+ git commit -m "content" &&
+ test_cmp expected_hooks actual_hooks
'
test_expect_success POSIXPERM '--no-verify with non-executable hook' '
-
- echo "more content" >> file &&
+ test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks; chmod +x \"$HOOKDIR/fail.sample\"" &&
+ ln -s "fail.sample" "$PRECOMMIT" &&
+ chmod -x "$HOOKDIR/fail.sample" &&
+ touch expected_hooks actual_hooks &&
+ echo "more content" >>file &&
git add file &&
- git commit --no-verify -m "more content"
-
+ git commit --no-verify -m "more content" &&
+ test_cmp expected_hooks actual_hooks
'
-chmod +x "$HOOK"
-
-# a hook that checks $GIT_PREFIX and succeeds inside the
-# success/ subdirectory only
-cat > "$HOOK" <<EOF
-#!/bin/sh
-test \$GIT_PREFIX = success/
-EOF
test_expect_success 'with hook requiring GIT_PREFIX' '
-
- echo "more content" >> file &&
+ test_when_finished "rm -rf \"$PRECOMMIT\" expected_hooks actual_hooks success" &&
+ ln -s "require-prefix.sample" "$PRECOMMIT" &&
+ echo "$PRECOMMIT" >expected_hooks &&
+ echo "more content" >>file &&
git add file &&
mkdir success &&
(
cd success &&
git commit -m "hook requires GIT_PREFIX = success/"
) &&
- rmdir success
+ test_cmp expected_hooks actual_hooks
'
test_expect_success 'with failing hook requiring GIT_PREFIX' '
-
- echo "more content" >> file &&
+ test_when_finished "rm -rf \"$PRECOMMIT\" expected_hooks actual_hooks fail" &&
+ ln -s "require-prefix.sample" "$PRECOMMIT" &&
+ echo "$PRECOMMIT" >expected_hooks &&
+ echo "more content" >>file &&
git add file &&
mkdir fail &&
(
cd fail &&
test_must_fail git commit -m "hook must fail"
) &&
- rmdir fail &&
- git checkout -- file
+ git checkout -- file &&
+ test_cmp expected_hooks actual_hooks
'
test_expect_success 'check the author in hook' '
- write_script "$HOOK" <<-\EOF &&
- test "$GIT_AUTHOR_NAME" = "New Author" &&
- test "$GIT_AUTHOR_EMAIL" = "newauthor@example.com"
+ test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks" &&
+ ln -s "check-author.sample" "$PRECOMMIT" &&
+ cat >expected_hooks <<-EOF &&
+ $PRECOMMIT
+ $PRECOMMIT
+ $PRECOMMIT
EOF
test_must_fail git commit --allow-empty -m "by a.u.thor" &&
(
@@ -133,7 +158,8 @@ test_expect_success 'check the author in hook' '
) &&
git commit --author="New Author <newauthor@example.com>" \
--allow-empty -m "by new.author via command line" &&
- git show -s
+ git show -s &&
+ test_cmp expected_hooks actual_hooks
'
test_done
--
2.22.0.770.g0f2c4a37fd-goog
^ permalink raw reply related [flat|nested] 59+ messages in thread* Re: [PATCH v3 1/4] t7503: verify proper hook execution
2019-08-01 22:20 ` [PATCH v3 1/4] t7503: verify proper hook execution Josh Steadmon
@ 2019-08-02 9:43 ` Martin Ågren
0 siblings, 0 replies; 59+ messages in thread
From: Martin Ågren @ 2019-08-02 9:43 UTC (permalink / raw)
To: Josh Steadmon; +Cc: Git Mailing List, Junio C Hamano, Michael J Gruber
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
^ permalink raw reply [flat|nested] 59+ messages in thread
* [PATCH v3 2/4] merge: do no-verify like commit
2019-08-01 22:20 ` [PATCH v3 0/4] pre-merge-commit hook Josh Steadmon
2019-08-01 22:20 ` [PATCH v3 1/4] t7503: verify proper hook execution Josh Steadmon
@ 2019-08-01 22:20 ` Josh Steadmon
2019-08-01 22:20 ` [PATCH v3 3/4] git-merge: honor pre-merge-commit hook Josh Steadmon
` (3 subsequent siblings)
5 siblings, 0 replies; 59+ messages in thread
From: Josh Steadmon @ 2019-08-01 22:20 UTC (permalink / raw)
To: git; +Cc: gitster, git, martin.agren
f8b863598c ("builtin/merge: honor commit-msg hook for merges", 2017-09-07)
introduced the no-verify flag to merge for bypassing the commit-msg
hook, though in a different way from the implementation in commit.c.
Change the implementation in merge.c to be the same as in commit.c so
that both do the same in the same way. This also changes the output of
"git merge --help" to be more clear that the hook return code is
respected by default.
[js: * reworded commit message
* squashed documentation changes from original series' patch 3/4
]
Signed-off-by: Michael J Gruber <git@grubix.eu>
Signed-off-by: Josh Steadmon <steadmon@google.com>
---
Documentation/git-merge.txt | 2 +-
Documentation/merge-options.txt | 4 ++++
builtin/merge.c | 6 +++---
3 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
index 01fd52dc70..092529c619 100644
--- a/Documentation/git-merge.txt
+++ b/Documentation/git-merge.txt
@@ -10,7 +10,7 @@ SYNOPSIS
--------
[verse]
'git merge' [-n] [--stat] [--no-commit] [--squash] [--[no-]edit]
- [-s <strategy>] [-X <strategy-option>] [-S[<keyid>]]
+ [--no-verify] [-s <strategy>] [-X <strategy-option>] [-S[<keyid>]]
[--[no-]allow-unrelated-histories]
[--[no-]rerere-autoupdate] [-m <msg>] [-F <file>] [<commit>...]
'git merge' (--continue | --abort | --quit)
diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
index 79a00d2a4a..d6a9f4b96f 100644
--- a/Documentation/merge-options.txt
+++ b/Documentation/merge-options.txt
@@ -105,6 +105,10 @@ option can be used to override --squash.
+
With --squash, --commit is not allowed, and will fail.
+--no-verify::
+ This option bypasses the pre-merge and commit-msg hooks.
+ See also linkgit:githooks[5].
+
-s <strategy>::
--strategy=<strategy>::
Use the given merge strategy; can be supplied more than
diff --git a/builtin/merge.c b/builtin/merge.c
index e2ccbc44e2..4425a7a12e 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -81,7 +81,7 @@ static int show_progress = -1;
static int default_to_upstream = 1;
static int signoff;
static const char *sign_commit;
-static int verify_msg = 1;
+static int no_verify;
static struct strategy all_strategy[] = {
{ "recursive", DEFAULT_TWOHEAD | NO_TRIVIAL },
@@ -287,7 +287,7 @@ static struct option builtin_merge_options[] = {
N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" },
OPT_BOOL(0, "overwrite-ignore", &overwrite_ignore, N_("update ignored files (default)")),
OPT_BOOL(0, "signoff", &signoff, N_("add Signed-off-by:")),
- OPT_BOOL(0, "verify", &verify_msg, N_("verify commit-msg hook")),
+ OPT_BOOL(0, "no-verify", &no_verify, N_("bypass commit-msg hook")),
OPT_END()
};
@@ -842,7 +842,7 @@ static void prepare_to_commit(struct commit_list *remoteheads)
abort_commit(remoteheads, NULL);
}
- if (verify_msg && run_commit_hook(0 < option_edit, get_index_file(),
+ if (!no_verify && run_commit_hook(0 < option_edit, get_index_file(),
"commit-msg",
git_path_merge_msg(the_repository), NULL))
abort_commit(remoteheads, NULL);
--
2.22.0.770.g0f2c4a37fd-goog
^ permalink raw reply related [flat|nested] 59+ messages in thread* [PATCH v3 3/4] git-merge: honor pre-merge-commit hook
2019-08-01 22:20 ` [PATCH v3 0/4] pre-merge-commit hook Josh Steadmon
2019-08-01 22:20 ` [PATCH v3 1/4] t7503: verify proper hook execution Josh Steadmon
2019-08-01 22:20 ` [PATCH v3 2/4] merge: do no-verify like commit Josh Steadmon
@ 2019-08-01 22:20 ` Josh Steadmon
2019-08-02 9:45 ` Martin Ågren
2019-08-01 22:20 ` [PATCH v3 4/4] merge: --no-verify to bypass " Josh Steadmon
` (2 subsequent siblings)
5 siblings, 1 reply; 59+ messages in thread
From: Josh Steadmon @ 2019-08-01 22:20 UTC (permalink / raw)
To: git; +Cc: gitster, git, martin.agren
git-merge does not honor the pre-commit hook when doing automatic merge
commits, and for compatibility reasons this is going to stay.
Introduce a pre-merge-commit hook which is called for an automatic merge
commit just like pre-commit is called for a non-automatic merge commit
(or any other commit).
[js: * renamed hook from "pre-merge" to "pre-merge-commit"
* only discard the index if the hook is actually present
* clarified that hook should write messages to stderr
* squashed test changes from the original series' patch 4/4
* modified tests to follow new pattern from this series' patch 1/4
* reworded commit message
]
Signed-off-by: Michael J Gruber <git@grubix.eu>
Signed-off-by: Josh Steadmon <steadmon@google.com>
---
Documentation/githooks.txt | 7 +++
builtin/merge.c | 12 +++++
...-pre-commit-and-pre-merge-commit-hooks.sh} | 45 ++++++++++++++++++-
templates/hooks--pre-merge-commit.sample | 13 ++++++
4 files changed, 76 insertions(+), 1 deletion(-)
rename t/{t7503-pre-commit-hook.sh => t7503-pre-commit-and-pre-merge-commit-hooks.sh} (78%)
create mode 100755 templates/hooks--pre-merge-commit.sample
diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index 82cd573776..7c4c994858 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -103,6 +103,13 @@ The default 'pre-commit' hook, when enabled--and with the
`hooks.allownonascii` config option unset or set to false--prevents
the use of non-ASCII filenames.
+pre-merge-commit
+~~~~~~~~~~~~~~~~
+
+This hook is invoked by 'git merge' when doing an automatic merge
+commit; it is equivalent to 'pre-commit' for a non-automatic commit
+for a merge.
+
prepare-commit-msg
~~~~~~~~~~~~~~~~~~
diff --git a/builtin/merge.c b/builtin/merge.c
index 4425a7a12e..bf0ae68c40 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -816,6 +816,18 @@ static void write_merge_heads(struct commit_list *);
static void prepare_to_commit(struct commit_list *remoteheads)
{
struct strbuf msg = STRBUF_INIT;
+ const char *index_file = get_index_file();
+
+ if (run_commit_hook(0 < option_edit, index_file, "pre-merge-commit", NULL))
+ abort_commit(remoteheads, NULL);
+ /*
+ * Re-read the index as pre-merge-commit hook could have updated it,
+ * and write it out as a tree. We must do this before we invoke
+ * the editor and after we invoke run_status above.
+ */
+ if (find_hook("pre-merge-commit"))
+ discard_cache();
+ read_cache_from(index_file);
strbuf_addbuf(&msg, &merge_msg);
if (squash)
BUG("the control must not reach here under --squash");
diff --git a/t/t7503-pre-commit-hook.sh b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
similarity index 78%
rename from t/t7503-pre-commit-hook.sh
rename to t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
index 500bdd97c2..040dfa0175 100755
--- a/t/t7503-pre-commit-hook.sh
+++ b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
@@ -1,11 +1,12 @@
#!/bin/sh
-test_description='pre-commit hook'
+test_description='pre-commit and pre-merge-commit hooks'
. ./test-lib.sh
HOOKDIR="$(git rev-parse --git-dir)/hooks"
PRECOMMIT="$HOOKDIR/pre-commit"
+PREMERGE="$HOOKDIR/pre-merge-commit"
# Prepare sample scripts that write their $0 to actual_hooks
test_expect_success 'sample script setup' '
@@ -29,6 +30,17 @@ test_expect_success 'sample script setup' '
EOF
'
+test_expect_success 'root commit' '
+ echo "root" > file &&
+ git add file &&
+ git commit -m "zeroth" &&
+ git checkout -b side &&
+ echo "foo" > foo &&
+ git add foo &&
+ git commit -m "make it non-ff" &&
+ git checkout master
+'
+
test_expect_success 'with no hook' '
test_when_finished "rm -f expected_hooks actual_hooks" &&
touch expected_hooks actual_hooks &&
@@ -38,6 +50,15 @@ test_expect_success 'with no hook' '
test_cmp expected_hooks actual_hooks
'
+test_expect_success 'with no hook (merge)' '
+ test_when_finished "rm -f expected_hooks actual_hooks" &&
+ touch expected_hooks actual_hooks &&
+ git checkout side &&
+ git merge -m "merge master" master &&
+ git checkout master &&
+ test_cmp expected_hooks actual_hooks
+'
+
test_expect_success '--no-verify with no hook' '
test_when_finished "rm -f expected_hooks actual_hooks" &&
touch expected_hooks actual_hooks &&
@@ -58,6 +79,17 @@ test_expect_success 'with succeeding hook' '
test_cmp expected_hooks actual_hooks
'
+test_expect_success 'with succeeding hook (merge)' '
+ test_when_finished "rm -f \"$PREMERGE\" expected_hooks actual_hooks" &&
+ ln -s "success.sample" "$PREMERGE" &&
+ touch actual_hooks &&
+ echo "$PREMERGE" >expected_hooks &&
+ git checkout side &&
+ git merge -m "merge master" master &&
+ git checkout master &&
+ test_cmp expected_hooks actual_hooks
+'
+
test_expect_success '--no-verify with succeeding hook' '
test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks" &&
ln -s "success.sample" "$PRECOMMIT" &&
@@ -89,6 +121,17 @@ test_expect_success '--no-verify with failing hook' '
test_cmp expected_hooks actual_hooks
'
+test_expect_success 'with failing hook (merge)' '
+ test_when_finished "rm -f \"$PREMERGE\" expected_hooks actual_hooks" &&
+ ln -s "fail.sample" "$PREMERGE" &&
+ touch actual_hooks &&
+ echo "$PREMERGE" >expected_hooks &&
+ git checkout side &&
+ test_must_fail git merge -m "merge master" master &&
+ git checkout master &&
+ test_cmp expected_hooks actual_hooks
+'
+
test_expect_success POSIXPERM 'with non-executable hook' '
test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks; chmod +x \"$HOOKDIR/fail.sample\"" &&
ln -s "fail.sample" "$PRECOMMIT" &&
diff --git a/templates/hooks--pre-merge-commit.sample b/templates/hooks--pre-merge-commit.sample
new file mode 100755
index 0000000000..399eab1924
--- /dev/null
+++ b/templates/hooks--pre-merge-commit.sample
@@ -0,0 +1,13 @@
+#!/bin/sh
+#
+# An example hook script to verify what is about to be committed.
+# Called by "git merge" with no arguments. The hook should
+# exit with non-zero status after issuing an appropriate message to
+# stderr if it wants to stop the merge commit.
+#
+# To enable this hook, rename this file to "pre-merge-commit".
+
+. git-sh-setup
+test -x "$GIT_DIR/hooks/pre-commit" &&
+ exec "$GIT_DIR/hooks/pre-commit"
+:
--
2.22.0.770.g0f2c4a37fd-goog
^ permalink raw reply related [flat|nested] 59+ messages in thread* Re: [PATCH v3 3/4] git-merge: honor pre-merge-commit hook
2019-08-01 22:20 ` [PATCH v3 3/4] git-merge: honor pre-merge-commit hook Josh Steadmon
@ 2019-08-02 9:45 ` Martin Ågren
2019-08-02 22:20 ` Josh Steadmon
0 siblings, 1 reply; 59+ messages in thread
From: Martin Ågren @ 2019-08-02 9:45 UTC (permalink / raw)
To: Josh Steadmon; +Cc: Git Mailing List, Junio C Hamano, Michael J Gruber
On Fri, 2 Aug 2019 at 00:20, Josh Steadmon <steadmon@google.com> wrote:
> diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
> index 82cd573776..7c4c994858 100644
> --- a/Documentation/githooks.txt
> +++ b/Documentation/githooks.txt
> @@ -103,6 +103,13 @@ The default 'pre-commit' hook, when enabled--and with the
> `hooks.allownonascii` config option unset or set to false--prevents
> the use of non-ASCII filenames.
>
> +pre-merge-commit
> +~~~~~~~~~~~~~~~~
> +
> +This hook is invoked by 'git merge' when doing an automatic merge
> +commit; it is equivalent to 'pre-commit' for a non-automatic commit
> +for a merge.
> +
I'm not sure everyone understands what an "automatic merge commit" is.
(Is it an automatic "merge commit", or an "automatic merge" commit? Or
sort of both?) And I'm not sure exactly what to infer from the
"equivalence". I happen to know that the statement about the default
hook can only be half-carried over. And I'm not sure what to infer from
"All the git commit hooks are invoked with the environment variable
...".
Is the below suggestion 1) correct, 2) readable?
This hook is invoked by linkgit:git-merge[1], and can be bypassed
with the `--no-verify` option. It takes no parameters, and is
invoked after the merge has been carried out successfully and before
obtaining the proposed commit log message to
make a commit. Exiting with a non-zero status from this script
causes the `git merge` command to abort before creating a commit.
The default 'pre-merge-commit' hook, when enabled, runs the
'pre-commit' hook, if the latter is enabled.
This hook is invoked with the environment variable
`GIT_EDITOR=:` if the command will not bring up an editor
to modify the commit message.
If the merge cannot be carried out automatically, the conflicts
need to be resolved and the result committed separately (see
linkgit:git-merge[1]). At that point, this hook will not be executed,
but the 'pre-commit' hook will, if it is enabled.
(If you use this or something like it, notice how this already mentions
`--no-verify`...)
> +test_expect_success 'root commit' '
> + echo "root" > file &&
> + git add file &&
> + git commit -m "zeroth" &&
> + git checkout -b side &&
> + echo "foo" > foo &&
> + git add foo &&
> + git commit -m "make it non-ff" &&
> + git checkout master
> +'
You got rid of loads of "> file" in patch 1/4, so it seems unfortunate
to introduce a few here. ;-)
Martin
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH v3 3/4] git-merge: honor pre-merge-commit hook
2019-08-02 9:45 ` Martin Ågren
@ 2019-08-02 22:20 ` Josh Steadmon
0 siblings, 0 replies; 59+ messages in thread
From: Josh Steadmon @ 2019-08-02 22:20 UTC (permalink / raw)
To: Martin Ågren; +Cc: Git Mailing List, Junio C Hamano, Michael J Gruber
On 2019.08.02 11:45, Martin Ågren wrote:
> On Fri, 2 Aug 2019 at 00:20, Josh Steadmon <steadmon@google.com> wrote:
>
> > diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
> > index 82cd573776..7c4c994858 100644
> > --- a/Documentation/githooks.txt
> > +++ b/Documentation/githooks.txt
> > @@ -103,6 +103,13 @@ The default 'pre-commit' hook, when enabled--and with the
> > `hooks.allownonascii` config option unset or set to false--prevents
> > the use of non-ASCII filenames.
> >
> > +pre-merge-commit
> > +~~~~~~~~~~~~~~~~
> > +
> > +This hook is invoked by 'git merge' when doing an automatic merge
> > +commit; it is equivalent to 'pre-commit' for a non-automatic commit
> > +for a merge.
> > +
>
> I'm not sure everyone understands what an "automatic merge commit" is.
> (Is it an automatic "merge commit", or an "automatic merge" commit? Or
> sort of both?) And I'm not sure exactly what to infer from the
> "equivalence". I happen to know that the statement about the default
> hook can only be half-carried over. And I'm not sure what to infer from
> "All the git commit hooks are invoked with the environment variable
> ...".
>
> Is the below suggestion 1) correct, 2) readable?
>
> This hook is invoked by linkgit:git-merge[1], and can be bypassed
> with the `--no-verify` option. It takes no parameters, and is
> invoked after the merge has been carried out successfully and before
> obtaining the proposed commit log message to
> make a commit. Exiting with a non-zero status from this script
> causes the `git merge` command to abort before creating a commit.
>
> The default 'pre-merge-commit' hook, when enabled, runs the
> 'pre-commit' hook, if the latter is enabled.
>
> This hook is invoked with the environment variable
> `GIT_EDITOR=:` if the command will not bring up an editor
> to modify the commit message.
>
> If the merge cannot be carried out automatically, the conflicts
> need to be resolved and the result committed separately (see
> linkgit:git-merge[1]). At that point, this hook will not be executed,
> but the 'pre-commit' hook will, if it is enabled.
>
> (If you use this or something like it, notice how this already mentions
> `--no-verify`...)
This looks good to me at first glance. I may reword a bit in V4, but
probably not much.
> > +test_expect_success 'root commit' '
> > + echo "root" > file &&
> > + git add file &&
> > + git commit -m "zeroth" &&
> > + git checkout -b side &&
> > + echo "foo" > foo &&
> > + git add foo &&
> > + git commit -m "make it non-ff" &&
> > + git checkout master
> > +'
>
> You got rid of loads of "> file" in patch 1/4, so it seems unfortunate
> to introduce a few here. ;-)
Thanks for the catch, will fix in V4.
^ permalink raw reply [flat|nested] 59+ messages in thread
* [PATCH v3 4/4] merge: --no-verify to bypass pre-merge-commit hook
2019-08-01 22:20 ` [PATCH v3 0/4] pre-merge-commit hook Josh Steadmon
` (2 preceding siblings ...)
2019-08-01 22:20 ` [PATCH v3 3/4] git-merge: honor pre-merge-commit hook Josh Steadmon
@ 2019-08-01 22:20 ` Josh Steadmon
2019-08-02 9:56 ` [PATCH v3 0/4] " Martin Ågren
2019-08-05 22:43 ` [PATCH v4 " Josh Steadmon
5 siblings, 0 replies; 59+ messages in thread
From: Josh Steadmon @ 2019-08-01 22:20 UTC (permalink / raw)
To: git; +Cc: gitster, git, martin.agren
Analogous to commit, introduce a '--no-verify' option which bypasses the
pre-merge-commit hook. The shorthand '-n' is taken by '--no-stat'
already.
[js: * reworded commit message to reflect current state of --no-stat flag
and new hook name
* fixed flag documentation to reflect new hook name
* cleaned up trailing whitespace
* squashed test changes from the original series' patch 4/4
* modified tests to follow pattern from this series' patch 1/4
]
Signed-off-by: Michael J Gruber <git@grubix.eu>
Signed-off-by: Josh Steadmon <steadmon@google.com>
---
Documentation/githooks.txt | 2 +-
builtin/merge.c | 4 +--
...3-pre-commit-and-pre-merge-commit-hooks.sh | 29 +++++++++++++++++++
3 files changed, 32 insertions(+), 3 deletions(-)
diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index 7c4c994858..a934553509 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -108,7 +108,7 @@ pre-merge-commit
This hook is invoked by 'git merge' when doing an automatic merge
commit; it is equivalent to 'pre-commit' for a non-automatic commit
-for a merge.
+for a merge, and can be bypassed with the `--no-verify` option.
prepare-commit-msg
~~~~~~~~~~~~~~~~~~
diff --git a/builtin/merge.c b/builtin/merge.c
index bf0ae68c40..c9746e37b8 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -287,7 +287,7 @@ static struct option builtin_merge_options[] = {
N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" },
OPT_BOOL(0, "overwrite-ignore", &overwrite_ignore, N_("update ignored files (default)")),
OPT_BOOL(0, "signoff", &signoff, N_("add Signed-off-by:")),
- OPT_BOOL(0, "no-verify", &no_verify, N_("bypass commit-msg hook")),
+ OPT_BOOL(0, "no-verify", &no_verify, N_("bypass pre-merge-commit and commit-msg hooks")),
OPT_END()
};
@@ -818,7 +818,7 @@ static void prepare_to_commit(struct commit_list *remoteheads)
struct strbuf msg = STRBUF_INIT;
const char *index_file = get_index_file();
- if (run_commit_hook(0 < option_edit, index_file, "pre-merge-commit", NULL))
+ if (!no_verify && run_commit_hook(0 < option_edit, index_file, "pre-merge-commit", NULL))
abort_commit(remoteheads, NULL);
/*
* Re-read the index as pre-merge-commit hook could have updated it,
diff --git a/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
index 040dfa0175..86a375ab3e 100755
--- a/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
+++ b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
@@ -68,6 +68,15 @@ test_expect_success '--no-verify with no hook' '
test_cmp expected_hooks actual_hooks
'
+test_expect_success '--no-verify with no hook (merge)' '
+ test_when_finished "rm -f expected_hooks actual_hooks" &&
+ touch expected_hooks actual_hooks &&
+ git checkout side &&
+ git merge --no-verify -m "merge master" master &&
+ git checkout master &&
+ test_cmp expected_hooks actual_hooks
+'
+
test_expect_success 'with succeeding hook' '
test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks" &&
ln -s "success.sample" "$PRECOMMIT" &&
@@ -100,6 +109,16 @@ test_expect_success '--no-verify with succeeding hook' '
test_cmp expected_hooks actual_hooks
'
+test_expect_success '--no-verify with succeeding hook (merge)' '
+ test_when_finished "rm -f \"$PREMERGE\" expected_hooks actual_hooks" &&
+ ln -s "success.sample" "$PREMERGE" &&
+ touch expected_hooks actual_hooks &&
+ git checkout side &&
+ git merge --no-verify -m "merge master" master &&
+ git checkout master &&
+ test_cmp expected_hooks actual_hooks
+'
+
test_expect_success 'with failing hook' '
test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks" &&
ln -s "fail.sample" "$PRECOMMIT" &&
@@ -132,6 +151,16 @@ test_expect_success 'with failing hook (merge)' '
test_cmp expected_hooks actual_hooks
'
+test_expect_success '--no-verify with failing hook (merge)' '
+ test_when_finished "rm -f \"$PREMERGE\" expected_hooks actual_hooks" &&
+ ln -s "fail.sample" "$PREMERGE" &&
+ touch expected_hooks actual_hooks &&
+ git checkout side &&
+ git merge --no-verify -m "merge master" master &&
+ git checkout master &&
+ test_cmp expected_hooks actual_hooks
+'
+
test_expect_success POSIXPERM 'with non-executable hook' '
test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks; chmod +x \"$HOOKDIR/fail.sample\"" &&
ln -s "fail.sample" "$PRECOMMIT" &&
--
2.22.0.770.g0f2c4a37fd-goog
^ permalink raw reply related [flat|nested] 59+ messages in thread* Re: [PATCH v3 0/4] pre-merge-commit hook
2019-08-01 22:20 ` [PATCH v3 0/4] pre-merge-commit hook Josh Steadmon
` (3 preceding siblings ...)
2019-08-01 22:20 ` [PATCH v3 4/4] merge: --no-verify to bypass " Josh Steadmon
@ 2019-08-02 9:56 ` Martin Ågren
2019-08-02 9:56 ` [PATCH 1/5] t7503: use "&&" in "test_when_finished" rather than ";" Martin Ågren
` (5 more replies)
2019-08-05 22:43 ` [PATCH v4 " Josh Steadmon
5 siblings, 6 replies; 59+ messages in thread
From: Martin Ågren @ 2019-08-02 9:56 UTC (permalink / raw)
To: Josh Steadmon; +Cc: git, Junio C Hamano, Michael J Gruber
[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
^ permalink raw reply [flat|nested] 59+ messages in thread* [PATCH 1/5] t7503: use "&&" in "test_when_finished" rather than ";"
2019-08-02 9:56 ` [PATCH v3 0/4] " Martin Ågren
@ 2019-08-02 9:56 ` Martin Ågren
2019-08-02 9:56 ` [PATCH 2/5] t7503: avoid touch when mtime doesn't matter Martin Ågren
` (4 subsequent siblings)
5 siblings, 0 replies; 59+ messages in thread
From: Martin Ågren @ 2019-08-02 9:56 UTC (permalink / raw)
To: Josh Steadmon; +Cc: git, Junio C Hamano, Michael J Gruber
Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
t/t7503-pre-commit-and-pre-merge-commit-hooks.sh | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
index 86a375ab3e..5cc6c98375 100755
--- a/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
+++ b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
@@ -162,7 +162,7 @@ test_expect_success '--no-verify with failing hook (merge)' '
'
test_expect_success POSIXPERM 'with non-executable hook' '
- test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks; chmod +x \"$HOOKDIR/fail.sample\"" &&
+ test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks && chmod +x \"$HOOKDIR/fail.sample\"" &&
ln -s "fail.sample" "$PRECOMMIT" &&
chmod -x "$HOOKDIR/fail.sample" &&
touch expected_hooks actual_hooks &&
@@ -173,7 +173,7 @@ test_expect_success POSIXPERM 'with non-executable hook' '
'
test_expect_success POSIXPERM '--no-verify with non-executable hook' '
- test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks; chmod +x \"$HOOKDIR/fail.sample\"" &&
+ test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks && chmod +x \"$HOOKDIR/fail.sample\"" &&
ln -s "fail.sample" "$PRECOMMIT" &&
chmod -x "$HOOKDIR/fail.sample" &&
touch expected_hooks actual_hooks &&
--
2.23.0.rc0.30.g51cf315870
^ permalink raw reply related [flat|nested] 59+ messages in thread* [PATCH 2/5] t7503: avoid touch when mtime doesn't matter
2019-08-02 9:56 ` [PATCH v3 0/4] " Martin Ågren
2019-08-02 9:56 ` [PATCH 1/5] t7503: use "&&" in "test_when_finished" rather than ";" Martin Ågren
@ 2019-08-02 9:56 ` Martin Ågren
2019-08-02 9:56 ` [PATCH 3/5] t7503: simplify file-juggling Martin Ågren
` (3 subsequent siblings)
5 siblings, 0 replies; 59+ messages in thread
From: Martin Ågren @ 2019-08-02 9:56 UTC (permalink / raw)
To: Josh Steadmon; +Cc: git, Junio C Hamano, Michael J Gruber
AFAIU, we avoid using `touch` unless we really do want to update the
mtime. Here, we just want to create empty files, so write `>foo`
instead.
Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
...3-pre-commit-and-pre-merge-commit-hooks.sh | 38 ++++++++++++-------
1 file changed, 24 insertions(+), 14 deletions(-)
diff --git a/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
index 5cc6c98375..a6f1240aa2 100755
--- a/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
+++ b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
@@ -43,7 +43,8 @@ test_expect_success 'root commit' '
test_expect_success 'with no hook' '
test_when_finished "rm -f expected_hooks actual_hooks" &&
- touch expected_hooks actual_hooks &&
+ >expected_hooks &&
+ >actual_hooks &&
echo "foo" >file &&
git add file &&
git commit -m "first" &&
@@ -52,7 +53,8 @@ test_expect_success 'with no hook' '
test_expect_success 'with no hook (merge)' '
test_when_finished "rm -f expected_hooks actual_hooks" &&
- touch expected_hooks actual_hooks &&
+ >expected_hooks &&
+ >actual_hooks &&
git checkout side &&
git merge -m "merge master" master &&
git checkout master &&
@@ -61,7 +63,8 @@ test_expect_success 'with no hook (merge)' '
test_expect_success '--no-verify with no hook' '
test_when_finished "rm -f expected_hooks actual_hooks" &&
- touch expected_hooks actual_hooks &&
+ >expected_hooks &&
+ >actual_hooks &&
echo "bar" >file &&
git add file &&
git commit --no-verify -m "bar" &&
@@ -70,7 +73,8 @@ test_expect_success '--no-verify with no hook' '
test_expect_success '--no-verify with no hook (merge)' '
test_when_finished "rm -f expected_hooks actual_hooks" &&
- touch expected_hooks actual_hooks &&
+ >expected_hooks &&
+ >actual_hooks &&
git checkout side &&
git merge --no-verify -m "merge master" master &&
git checkout master &&
@@ -80,7 +84,7 @@ test_expect_success '--no-verify with no hook (merge)' '
test_expect_success 'with succeeding hook' '
test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks" &&
ln -s "success.sample" "$PRECOMMIT" &&
- touch actual_hooks &&
+ >actual_hooks &&
echo "$PRECOMMIT" >expected_hooks &&
echo "more" >>file &&
git add file &&
@@ -91,7 +95,7 @@ test_expect_success 'with succeeding hook' '
test_expect_success 'with succeeding hook (merge)' '
test_when_finished "rm -f \"$PREMERGE\" expected_hooks actual_hooks" &&
ln -s "success.sample" "$PREMERGE" &&
- touch actual_hooks &&
+ >actual_hooks &&
echo "$PREMERGE" >expected_hooks &&
git checkout side &&
git merge -m "merge master" master &&
@@ -102,7 +106,8 @@ test_expect_success 'with succeeding hook (merge)' '
test_expect_success '--no-verify with succeeding hook' '
test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks" &&
ln -s "success.sample" "$PRECOMMIT" &&
- touch expected_hooks actual_hooks &&
+ >expected_hooks &&
+ >actual_hooks &&
echo "even more" >>file &&
git add file &&
git commit --no-verify -m "even more" &&
@@ -112,7 +117,8 @@ test_expect_success '--no-verify with succeeding hook' '
test_expect_success '--no-verify with succeeding hook (merge)' '
test_when_finished "rm -f \"$PREMERGE\" expected_hooks actual_hooks" &&
ln -s "success.sample" "$PREMERGE" &&
- touch expected_hooks actual_hooks &&
+ >expected_hooks &&
+ >actual_hooks &&
git checkout side &&
git merge --no-verify -m "merge master" master &&
git checkout master &&
@@ -122,7 +128,7 @@ test_expect_success '--no-verify with succeeding hook (merge)' '
test_expect_success 'with failing hook' '
test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks" &&
ln -s "fail.sample" "$PRECOMMIT" &&
- touch actual_hooks &&
+ >actual_hooks &&
echo "$PRECOMMIT" >expected_hooks &&
echo "another" >>file &&
git add file &&
@@ -133,7 +139,8 @@ test_expect_success 'with failing hook' '
test_expect_success '--no-verify with failing hook' '
test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks" &&
ln -s "fail.sample" "$PRECOMMIT" &&
- touch expected_hooks actual_hooks &&
+ >expected_hooks &&
+ >actual_hooks &&
echo "stuff" >>file &&
git add file &&
git commit --no-verify -m "stuff" &&
@@ -143,7 +150,7 @@ test_expect_success '--no-verify with failing hook' '
test_expect_success 'with failing hook (merge)' '
test_when_finished "rm -f \"$PREMERGE\" expected_hooks actual_hooks" &&
ln -s "fail.sample" "$PREMERGE" &&
- touch actual_hooks &&
+ >actual_hooks &&
echo "$PREMERGE" >expected_hooks &&
git checkout side &&
test_must_fail git merge -m "merge master" master &&
@@ -154,7 +161,8 @@ test_expect_success 'with failing hook (merge)' '
test_expect_success '--no-verify with failing hook (merge)' '
test_when_finished "rm -f \"$PREMERGE\" expected_hooks actual_hooks" &&
ln -s "fail.sample" "$PREMERGE" &&
- touch expected_hooks actual_hooks &&
+ >expected_hooks &&
+ >actual_hooks &&
git checkout side &&
git merge --no-verify -m "merge master" master &&
git checkout master &&
@@ -165,7 +173,8 @@ test_expect_success POSIXPERM 'with non-executable hook' '
test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks && chmod +x \"$HOOKDIR/fail.sample\"" &&
ln -s "fail.sample" "$PRECOMMIT" &&
chmod -x "$HOOKDIR/fail.sample" &&
- touch expected_hooks actual_hooks &&
+ >expected_hooks &&
+ >actual_hooks &&
echo "content" >>file &&
git add file &&
git commit -m "content" &&
@@ -176,7 +185,8 @@ test_expect_success POSIXPERM '--no-verify with non-executable hook' '
test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks && chmod +x \"$HOOKDIR/fail.sample\"" &&
ln -s "fail.sample" "$PRECOMMIT" &&
chmod -x "$HOOKDIR/fail.sample" &&
- touch expected_hooks actual_hooks &&
+ >expected_hooks &&
+ >actual_hooks &&
echo "more content" >>file &&
git add file &&
git commit --no-verify -m "more content" &&
--
2.23.0.rc0.30.g51cf315870
^ permalink raw reply related [flat|nested] 59+ messages in thread* [PATCH 3/5] t7503: simplify file-juggling
2019-08-02 9:56 ` [PATCH v3 0/4] " Martin Ågren
2019-08-02 9:56 ` [PATCH 1/5] t7503: use "&&" in "test_when_finished" rather than ";" Martin Ågren
2019-08-02 9:56 ` [PATCH 2/5] t7503: avoid touch when mtime doesn't matter Martin Ågren
@ 2019-08-02 9:56 ` Martin Ågren
2019-08-02 9:56 ` [PATCH 4/5] t7503: don't create "actual_hooks" for later appending Martin Ågren
` (2 subsequent siblings)
5 siblings, 0 replies; 59+ messages in thread
From: Martin Ågren @ 2019-08-02 9:56 UTC (permalink / raw)
To: Josh Steadmon; +Cc: git, Junio C Hamano, Michael J Gruber
Rather than creating an empty "expected_hooks" file and another empty
file "actual_hooks" (which we want to not be appended to), just verify
that "actual_hooks" isn't created. Do still clean up the "actual_hooks"
file, so that a failing test won't start affecting later tests too.
Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
...3-pre-commit-and-pre-merge-commit-hooks.sh | 60 +++++++------------
1 file changed, 20 insertions(+), 40 deletions(-)
diff --git a/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
index a6f1240aa2..477207cb5c 100755
--- a/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
+++ b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
@@ -42,43 +42,35 @@ test_expect_success 'root commit' '
'
test_expect_success 'with no hook' '
- test_when_finished "rm -f expected_hooks actual_hooks" &&
- >expected_hooks &&
- >actual_hooks &&
+ test_when_finished "rm -f actual_hooks" &&
echo "foo" >file &&
git add file &&
git commit -m "first" &&
- test_cmp expected_hooks actual_hooks
+ test_path_is_missing actual_hooks
'
test_expect_success 'with no hook (merge)' '
- test_when_finished "rm -f expected_hooks actual_hooks" &&
- >expected_hooks &&
- >actual_hooks &&
+ test_when_finished "rm -f actual_hooks" &&
git checkout side &&
git merge -m "merge master" master &&
git checkout master &&
- test_cmp expected_hooks actual_hooks
+ test_path_is_missing actual_hooks
'
test_expect_success '--no-verify with no hook' '
- test_when_finished "rm -f expected_hooks actual_hooks" &&
- >expected_hooks &&
- >actual_hooks &&
+ test_when_finished "rm -f actual_hooks" &&
echo "bar" >file &&
git add file &&
git commit --no-verify -m "bar" &&
- test_cmp expected_hooks actual_hooks
+ test_path_is_missing actual_hooks
'
test_expect_success '--no-verify with no hook (merge)' '
- test_when_finished "rm -f expected_hooks actual_hooks" &&
- >expected_hooks &&
- >actual_hooks &&
+ test_when_finished "rm -f actual_hooks" &&
git checkout side &&
git merge --no-verify -m "merge master" master &&
git checkout master &&
- test_cmp expected_hooks actual_hooks
+ test_path_is_missing actual_hooks
'
test_expect_success 'with succeeding hook' '
@@ -104,25 +96,21 @@ test_expect_success 'with succeeding hook (merge)' '
'
test_expect_success '--no-verify with succeeding hook' '
- test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks" &&
+ test_when_finished "rm -f \"$PRECOMMIT\" actual_hooks" &&
ln -s "success.sample" "$PRECOMMIT" &&
- >expected_hooks &&
- >actual_hooks &&
echo "even more" >>file &&
git add file &&
git commit --no-verify -m "even more" &&
- test_cmp expected_hooks actual_hooks
+ test_path_is_missing actual_hooks
'
test_expect_success '--no-verify with succeeding hook (merge)' '
- test_when_finished "rm -f \"$PREMERGE\" expected_hooks actual_hooks" &&
+ test_when_finished "rm -f \"$PREMERGE\" actual_hooks" &&
ln -s "success.sample" "$PREMERGE" &&
- >expected_hooks &&
- >actual_hooks &&
git checkout side &&
git merge --no-verify -m "merge master" master &&
git checkout master &&
- test_cmp expected_hooks actual_hooks
+ test_path_is_missing actual_hooks
'
test_expect_success 'with failing hook' '
@@ -137,14 +125,12 @@ test_expect_success 'with failing hook' '
'
test_expect_success '--no-verify with failing hook' '
- test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks" &&
+ test_when_finished "rm -f \"$PRECOMMIT\" actual_hooks" &&
ln -s "fail.sample" "$PRECOMMIT" &&
- >expected_hooks &&
- >actual_hooks &&
echo "stuff" >>file &&
git add file &&
git commit --no-verify -m "stuff" &&
- test_cmp expected_hooks actual_hooks
+ test_path_is_missing actual_hooks
'
test_expect_success 'with failing hook (merge)' '
@@ -159,38 +145,32 @@ test_expect_success 'with failing hook (merge)' '
'
test_expect_success '--no-verify with failing hook (merge)' '
- test_when_finished "rm -f \"$PREMERGE\" expected_hooks actual_hooks" &&
+ test_when_finished "rm -f \"$PREMERGE\" actual_hooks" &&
ln -s "fail.sample" "$PREMERGE" &&
- >expected_hooks &&
- >actual_hooks &&
git checkout side &&
git merge --no-verify -m "merge master" master &&
git checkout master &&
- test_cmp expected_hooks actual_hooks
+ test_path_is_missing actual_hooks
'
test_expect_success POSIXPERM 'with non-executable hook' '
- test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks && chmod +x \"$HOOKDIR/fail.sample\"" &&
+ test_when_finished "rm -f \"$PRECOMMIT\" actual_hooks && chmod +x \"$HOOKDIR/fail.sample\"" &&
ln -s "fail.sample" "$PRECOMMIT" &&
chmod -x "$HOOKDIR/fail.sample" &&
- >expected_hooks &&
- >actual_hooks &&
echo "content" >>file &&
git add file &&
git commit -m "content" &&
- test_cmp expected_hooks actual_hooks
+ test_path_is_missing actual_hooks
'
test_expect_success POSIXPERM '--no-verify with non-executable hook' '
- test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks && chmod +x \"$HOOKDIR/fail.sample\"" &&
+ test_when_finished "rm -f \"$PRECOMMIT\" actual_hooks && chmod +x \"$HOOKDIR/fail.sample\"" &&
ln -s "fail.sample" "$PRECOMMIT" &&
chmod -x "$HOOKDIR/fail.sample" &&
- >expected_hooks &&
- >actual_hooks &&
echo "more content" >>file &&
git add file &&
git commit --no-verify -m "more content" &&
- test_cmp expected_hooks actual_hooks
+ test_path_is_missing actual_hooks
'
test_expect_success 'with hook requiring GIT_PREFIX' '
--
2.23.0.rc0.30.g51cf315870
^ permalink raw reply related [flat|nested] 59+ messages in thread* [PATCH 4/5] t7503: don't create "actual_hooks" for later appending
2019-08-02 9:56 ` [PATCH v3 0/4] " Martin Ågren
` (2 preceding siblings ...)
2019-08-02 9:56 ` [PATCH 3/5] t7503: simplify file-juggling Martin Ågren
@ 2019-08-02 9:56 ` Martin Ågren
2019-08-02 9:56 ` [PATCH 5/5] t7503: test failing merge with both hooks available Martin Ågren
2019-08-02 22:18 ` [PATCH v3 0/4] pre-merge-commit hook Josh Steadmon
5 siblings, 0 replies; 59+ messages in thread
From: Martin Ågren @ 2019-08-02 9:56 UTC (permalink / raw)
To: Josh Steadmon; +Cc: git, Junio C Hamano, Michael J Gruber
If we fail to call the hook, we won't append to "actual_hooks".
"test_cmp" is able to handle a missing file just fine, so these
"pre-creations" are mostly confusing. Let's drop them.
Signed-off-by: Martin Ågren <martin.agren@gmail.com>
--
This "pre-creation" does protect against the file already
existing, i.e., it could be seen as a "clearing". But since
we use "test_when_finished" in this script... Also, these
were "touch" before another of my suggestions, so these
really were "pre-creating" these files if needed.
t/t7503-pre-commit-and-pre-merge-commit-hooks.sh | 4 ----
1 file changed, 4 deletions(-)
diff --git a/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
index 477207cb5c..f0c73fd58d 100755
--- a/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
+++ b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
@@ -76,7 +76,6 @@ test_expect_success '--no-verify with no hook (merge)' '
test_expect_success 'with succeeding hook' '
test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks" &&
ln -s "success.sample" "$PRECOMMIT" &&
- >actual_hooks &&
echo "$PRECOMMIT" >expected_hooks &&
echo "more" >>file &&
git add file &&
@@ -87,7 +86,6 @@ test_expect_success 'with succeeding hook' '
test_expect_success 'with succeeding hook (merge)' '
test_when_finished "rm -f \"$PREMERGE\" expected_hooks actual_hooks" &&
ln -s "success.sample" "$PREMERGE" &&
- >actual_hooks &&
echo "$PREMERGE" >expected_hooks &&
git checkout side &&
git merge -m "merge master" master &&
@@ -116,7 +114,6 @@ test_expect_success '--no-verify with succeeding hook (merge)' '
test_expect_success 'with failing hook' '
test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks" &&
ln -s "fail.sample" "$PRECOMMIT" &&
- >actual_hooks &&
echo "$PRECOMMIT" >expected_hooks &&
echo "another" >>file &&
git add file &&
@@ -136,7 +133,6 @@ test_expect_success '--no-verify with failing hook' '
test_expect_success 'with failing hook (merge)' '
test_when_finished "rm -f \"$PREMERGE\" expected_hooks actual_hooks" &&
ln -s "fail.sample" "$PREMERGE" &&
- >actual_hooks &&
echo "$PREMERGE" >expected_hooks &&
git checkout side &&
test_must_fail git merge -m "merge master" master &&
--
2.23.0.rc0.30.g51cf315870
^ permalink raw reply related [flat|nested] 59+ messages in thread* [PATCH 5/5] t7503: test failing merge with both hooks available
2019-08-02 9:56 ` [PATCH v3 0/4] " Martin Ågren
` (3 preceding siblings ...)
2019-08-02 9:56 ` [PATCH 4/5] t7503: don't create "actual_hooks" for later appending Martin Ågren
@ 2019-08-02 9:56 ` Martin Ågren
2019-08-02 22:18 ` [PATCH v3 0/4] pre-merge-commit hook Josh Steadmon
5 siblings, 0 replies; 59+ messages in thread
From: Martin Ågren @ 2019-08-02 9:56 UTC (permalink / raw)
To: Josh Steadmon; +Cc: git, Junio C Hamano, Michael J Gruber
With a failing automatic merge, we want to make sure that we *don't*
call the pre-merge-commit hook and that we (eventually) *do* call the
pre-commit hook.
Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
...3-pre-commit-and-pre-merge-commit-hooks.sh | 30 +++++++++++++++++++
1 file changed, 30 insertions(+)
diff --git a/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
index f0c73fd58d..4bf359c097 100755
--- a/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
+++ b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
@@ -41,6 +41,18 @@ test_expect_success 'root commit' '
git checkout master
'
+test_expect_success 'setup conflicting branches' '
+ test_when_finished "git checkout master" &&
+ git checkout -b conflicting-a master &&
+ echo a >conflicting &&
+ git add conflicting &&
+ git commit -m conflicting-a &&
+ git checkout -b conflicting-b master &&
+ echo b >conflicting &&
+ git add conflicting &&
+ git commit -m conflicting-b
+'
+
test_expect_success 'with no hook' '
test_when_finished "rm -f actual_hooks" &&
echo "foo" >file &&
@@ -93,6 +105,24 @@ test_expect_success 'with succeeding hook (merge)' '
test_cmp expected_hooks actual_hooks
'
+test_expect_success 'automatic merge fails; both hooks are available' '
+ test_when_finished "rm -f \"$PREMERGE\" \"$PRECOMMIT\"" &&
+ test_when_finished "rm -f expected_hooks actual_hooks" &&
+ test_when_finished "git checkout master" &&
+ ln -s "success.sample" "$PREMERGE" &&
+ ln -s "success.sample" "$PRECOMMIT" &&
+
+ git checkout conflicting-a &&
+ test_must_fail git merge -m "merge conflicting-b" conflicting-b &&
+ test_path_is_missing actual_hooks &&
+
+ echo "$PRECOMMIT" >expected_hooks &&
+ echo a+b >conflicting &&
+ git add conflicting &&
+ git commit -m "resolve conflict" &&
+ test_cmp expected_hooks actual_hooks
+'
+
test_expect_success '--no-verify with succeeding hook' '
test_when_finished "rm -f \"$PRECOMMIT\" actual_hooks" &&
ln -s "success.sample" "$PRECOMMIT" &&
--
2.23.0.rc0.30.g51cf315870
^ permalink raw reply related [flat|nested] 59+ messages in thread* Re: [PATCH v3 0/4] pre-merge-commit hook
2019-08-02 9:56 ` [PATCH v3 0/4] " Martin Ågren
` (4 preceding siblings ...)
2019-08-02 9:56 ` [PATCH 5/5] t7503: test failing merge with both hooks available Martin Ågren
@ 2019-08-02 22:18 ` Josh Steadmon
5 siblings, 0 replies; 59+ messages in thread
From: Josh Steadmon @ 2019-08-02 22:18 UTC (permalink / raw)
To: Martin Ågren; +Cc: git, Junio C Hamano, Michael J Gruber
On 2019.08.02 11:56, Martin Ågren wrote:
> [Dropped cc-list the first time around. Apologies to those who receive
> this twice...]
>
> On Fri, 2 Aug 2019 at 00:20, Josh Steadmon <steadmon@google.com> wrote:
> >
> > This series adds a new pre-merge-commit hook, similar in usage to
> > pre-commit. It also improves hook testing in t7503, by verifying that
> > the correct hooks are run or bypassed as expected.
>
> I really like those test improvements. Now it should be harder to mess
> up a future refactoring and run the wrong hook. These hooks are "very
> related" so I think this is important.
>
> I've messed with the test a bit and offer these potential improvements
> for your consideration. I was lazy and just built this on top of your
> series -- if you agree to some or all of these, you'll probably need to
> squash them into a few individual patches.
>
> The first four are perhaps more or less a matter of opinion, although I
> do think that patch 2/5 is based on an opinion shared by others. ;-)
> Patch 5/5 or something like it seems pretty important to me to make
> sure that of these two "similar"/"related" hooks with some
> "backwards-compatibility-and-code-copying-history" around them, we'd
> better pick the right one when they're both available.
>
> Feel free to pick and squash as you see fit. (I don't think it makes
> sense to have these go in as-are. They really are meant for squashing.)
>
> Martin
>
> Martin Ågren (5):
> t7503: use "&&" in "test_when_finished" rather than ";"
> t7503: avoid touch when mtime doesn't matter
> t7503: simplify file-juggling
> t7503: don't create "actual_hooks" for later appending
> t7503: test failing merge with both hooks available
>
> ...3-pre-commit-and-pre-merge-commit-hooks.sh | 84 +++++++++++--------
> 1 file changed, 50 insertions(+), 34 deletions(-)
>
> --
> 2.23.0.rc0.30.g51cf315870
These all look good to me, thank you for the suggestions! I'll include
them in V4.
^ permalink raw reply [flat|nested] 59+ messages in thread
* [PATCH v4 0/4] pre-merge-commit hook
2019-08-01 22:20 ` [PATCH v3 0/4] pre-merge-commit hook Josh Steadmon
` (4 preceding siblings ...)
2019-08-02 9:56 ` [PATCH v3 0/4] " Martin Ågren
@ 2019-08-05 22:43 ` Josh Steadmon
2019-08-05 22:43 ` [PATCH v4 1/4] t7503: verify proper hook execution Josh Steadmon
` (4 more replies)
5 siblings, 5 replies; 59+ messages in thread
From: Josh Steadmon @ 2019-08-05 22:43 UTC (permalink / raw)
To: git; +Cc: gitster, git, martin.agren
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
^ permalink raw reply [flat|nested] 59+ messages in thread* [PATCH v4 1/4] t7503: verify proper hook execution
2019-08-05 22:43 ` [PATCH v4 " Josh Steadmon
@ 2019-08-05 22:43 ` Josh Steadmon
2019-08-06 18:14 ` Junio C Hamano
2019-08-05 22:43 ` [PATCH v4 2/4] merge: do no-verify like commit Josh Steadmon
` (3 subsequent siblings)
4 siblings, 1 reply; 59+ messages in thread
From: Josh Steadmon @ 2019-08-05 22:43 UTC (permalink / raw)
To: git; +Cc: gitster, git, martin.agren
t7503 did not verify that the expected hooks actually ran during
testing. Fix that by making the hook scripts write their $0 into a file
so that we can compare actual execution vs. expected execution.
While we're at it, do some test style cleanups, such as using
write_script() and doing setup inside a test_expect_success block.
Improved-by: Martin Ågren <martin.agren@gmail.com>
Signed-off-by: Martin Ågren <martin.agren@gmail.com>
Signed-off-by: Josh Steadmon <steadmon@google.com>
---
t/t7503-pre-commit-hook.sh | 157 +++++++++++++++++++++----------------
1 file changed, 89 insertions(+), 68 deletions(-)
diff --git a/t/t7503-pre-commit-hook.sh b/t/t7503-pre-commit-hook.sh
index 984889b39d..a71ec31222 100755
--- a/t/t7503-pre-commit-hook.sh
+++ b/t/t7503-pre-commit-hook.sh
@@ -4,124 +4,144 @@ test_description='pre-commit hook'
. ./test-lib.sh
-test_expect_success 'with no hook' '
+HOOKDIR="$(git rev-parse --git-dir)/hooks"
+PRECOMMIT="$HOOKDIR/pre-commit"
+
+# Prepare sample scripts that write their $0 to actual_hooks
+test_expect_success 'sample script setup' '
+ mkdir -p "$HOOKDIR" &&
+ write_script "$HOOKDIR/success.sample" <<-\EOF &&
+ echo $0 >>actual_hooks
+ exit 0
+ EOF
+ write_script "$HOOKDIR/fail.sample" <<-\EOF &&
+ echo $0 >>actual_hooks
+ exit 1
+ EOF
+ write_script "$HOOKDIR/non-exec.sample" <<-\EOF &&
+ echo $0 >>actual_hooks
+ exit 1
+ EOF
+ chmod -x "$HOOKDIR/non-exec.sample" &&
+ write_script "$HOOKDIR/require-prefix.sample" <<-\EOF &&
+ echo $0 >>actual_hooks
+ test $GIT_PREFIX = "success/"
+ EOF
+ write_script "$HOOKDIR/check-author.sample" <<-\EOF
+ echo $0 >>actual_hooks
+ test "$GIT_AUTHOR_NAME" = "New Author" &&
+ test "$GIT_AUTHOR_EMAIL" = "newauthor@example.com"
+ EOF
+'
- echo "foo" > file &&
+test_expect_success 'with no hook' '
+ test_when_finished "rm -f actual_hooks" &&
+ echo "foo" >file &&
git add file &&
- git commit -m "first"
-
+ git commit -m "first" &&
+ test_path_is_missing actual_hooks
'
test_expect_success '--no-verify with no hook' '
-
- echo "bar" > file &&
+ test_when_finished "rm -f actual_hooks" &&
+ echo "bar" >file &&
git add file &&
- git commit --no-verify -m "bar"
-
+ git commit --no-verify -m "bar" &&
+ test_path_is_missing actual_hooks
'
-# now install hook that always succeeds
-HOOKDIR="$(git rev-parse --git-dir)/hooks"
-HOOK="$HOOKDIR/pre-commit"
-mkdir -p "$HOOKDIR"
-cat > "$HOOK" <<EOF
-#!/bin/sh
-exit 0
-EOF
-chmod +x "$HOOK"
-
test_expect_success 'with succeeding hook' '
-
- echo "more" >> file &&
+ test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks" &&
+ ln -s "success.sample" "$PRECOMMIT" &&
+ echo "$PRECOMMIT" >expected_hooks &&
+ echo "more" >>file &&
git add file &&
- git commit -m "more"
-
+ git commit -m "more" &&
+ test_cmp expected_hooks actual_hooks
'
test_expect_success '--no-verify with succeeding hook' '
-
- echo "even more" >> file &&
+ test_when_finished "rm -f \"$PRECOMMIT\" actual_hooks" &&
+ ln -s "success.sample" "$PRECOMMIT" &&
+ echo "even more" >>file &&
git add file &&
- git commit --no-verify -m "even more"
-
+ git commit --no-verify -m "even more" &&
+ test_path_is_missing actual_hooks
'
-# now a hook that fails
-cat > "$HOOK" <<EOF
-#!/bin/sh
-exit 1
-EOF
-
test_expect_success 'with failing hook' '
-
- echo "another" >> file &&
+ test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks" &&
+ ln -s "fail.sample" "$PRECOMMIT" &&
+ echo "$PRECOMMIT" >expected_hooks &&
+ echo "another" >>file &&
git add file &&
- test_must_fail git commit -m "another"
-
+ test_must_fail git commit -m "another" &&
+ test_cmp expected_hooks actual_hooks
'
test_expect_success '--no-verify with failing hook' '
-
- echo "stuff" >> file &&
+ test_when_finished "rm -f \"$PRECOMMIT\" actual_hooks" &&
+ ln -s "fail.sample" "$PRECOMMIT" &&
+ echo "stuff" >>file &&
git add file &&
- git commit --no-verify -m "stuff"
-
+ git commit --no-verify -m "stuff" &&
+ test_path_is_missing actual_hooks
'
-chmod -x "$HOOK"
test_expect_success POSIXPERM 'with non-executable hook' '
-
- echo "content" >> file &&
+ test_when_finished "rm -f \"$PRECOMMIT\" actual_hooks" &&
+ ln -s "non-exec.sample" "$PRECOMMIT" &&
+ echo "content" >>file &&
git add file &&
- git commit -m "content"
-
+ git commit -m "content" &&
+ test_path_is_missing actual_hooks
'
test_expect_success POSIXPERM '--no-verify with non-executable hook' '
-
- echo "more content" >> file &&
+ test_when_finished "rm -f \"$PRECOMMIT\" actual_hooks" &&
+ ln -s "non-exec.sample" "$PRECOMMIT" &&
+ echo "more content" >>file &&
git add file &&
- git commit --no-verify -m "more content"
-
+ git commit --no-verify -m "more content" &&
+ test_path_is_missing actual_hooks
'
-chmod +x "$HOOK"
-
-# a hook that checks $GIT_PREFIX and succeeds inside the
-# success/ subdirectory only
-cat > "$HOOK" <<EOF
-#!/bin/sh
-test \$GIT_PREFIX = success/
-EOF
test_expect_success 'with hook requiring GIT_PREFIX' '
-
- echo "more content" >> file &&
+ test_when_finished "rm -rf \"$PRECOMMIT\" expected_hooks actual_hooks success" &&
+ ln -s "require-prefix.sample" "$PRECOMMIT" &&
+ echo "$PRECOMMIT" >expected_hooks &&
+ echo "more content" >>file &&
git add file &&
mkdir success &&
(
cd success &&
git commit -m "hook requires GIT_PREFIX = success/"
) &&
- rmdir success
+ test_cmp expected_hooks actual_hooks
'
test_expect_success 'with failing hook requiring GIT_PREFIX' '
-
- echo "more content" >> file &&
+ test_when_finished "rm -rf \"$PRECOMMIT\" expected_hooks actual_hooks fail" &&
+ ln -s "require-prefix.sample" "$PRECOMMIT" &&
+ echo "$PRECOMMIT" >expected_hooks &&
+ echo "more content" >>file &&
git add file &&
mkdir fail &&
(
cd fail &&
test_must_fail git commit -m "hook must fail"
) &&
- rmdir fail &&
- git checkout -- file
+ git checkout -- file &&
+ test_cmp expected_hooks actual_hooks
'
test_expect_success 'check the author in hook' '
- write_script "$HOOK" <<-\EOF &&
- test "$GIT_AUTHOR_NAME" = "New Author" &&
- test "$GIT_AUTHOR_EMAIL" = "newauthor@example.com"
+ test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks" &&
+ ln -s "check-author.sample" "$PRECOMMIT" &&
+ cat >expected_hooks <<-EOF &&
+ $PRECOMMIT
+ $PRECOMMIT
+ $PRECOMMIT
EOF
test_must_fail git commit --allow-empty -m "by a.u.thor" &&
(
@@ -133,7 +153,8 @@ test_expect_success 'check the author in hook' '
) &&
git commit --author="New Author <newauthor@example.com>" \
--allow-empty -m "by new.author via command line" &&
- git show -s
+ git show -s &&
+ test_cmp expected_hooks actual_hooks
'
test_done
--
2.22.0.770.g0f2c4a37fd-goog
^ permalink raw reply related [flat|nested] 59+ messages in thread* Re: [PATCH v4 1/4] t7503: verify proper hook execution
2019-08-05 22:43 ` [PATCH v4 1/4] t7503: verify proper hook execution Josh Steadmon
@ 2019-08-06 18:14 ` Junio C Hamano
2019-08-07 18:11 ` Josh Steadmon
0 siblings, 1 reply; 59+ messages in thread
From: Junio C Hamano @ 2019-08-06 18:14 UTC (permalink / raw)
To: Josh Steadmon; +Cc: git, git, martin.agren
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?
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH v4 1/4] t7503: verify proper hook execution
2019-08-06 18:14 ` Junio C Hamano
@ 2019-08-07 18:11 ` Josh Steadmon
0 siblings, 0 replies; 59+ messages in thread
From: Josh Steadmon @ 2019-08-07 18:11 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, git, martin.agren
On 2019.08.06 11:14, Junio C Hamano wrote:
> Josh Steadmon <steadmon@google.com> writes:
>
> > t7503 did not verify that the expected hooks actually ran during
> > testing. Fix that by making the hook scripts write their $0 into a file
> > so that we can compare actual execution vs. expected execution.
> >
> > While we're at it, do some test style cleanups, such as using
> > write_script() and doing setup inside a test_expect_success block.
> > ...
>
> The result mostly looks more pleasant compared to the original,
> modulo a few nits.
>
> > test_expect_success 'with succeeding hook' '
> > -
> > - echo "more" >> file &&
> > + test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks" &&
> > + ln -s "success.sample" "$PRECOMMIT" &&
>
> Not all systems that want to run Git are capable of symbolic links.
> We should be able to do a copy here, perhaps? Just the most basic
> form of "cp" without even "-p" would be sufficient as the target is
> expected to be missing (i.e. "cp" or "ln -s" would be creating the
> "$PRECOMMIT"), right?
Yeah, "cp" works here. Fixed in V5.
^ permalink raw reply [flat|nested] 59+ messages in thread
* [PATCH v4 2/4] merge: do no-verify like commit
2019-08-05 22:43 ` [PATCH v4 " Josh Steadmon
2019-08-05 22:43 ` [PATCH v4 1/4] t7503: verify proper hook execution Josh Steadmon
@ 2019-08-05 22:43 ` Josh Steadmon
2019-08-05 22:43 ` [PATCH v4 3/4] git-merge: honor pre-merge-commit hook Josh Steadmon
` (2 subsequent siblings)
4 siblings, 0 replies; 59+ messages in thread
From: Josh Steadmon @ 2019-08-05 22:43 UTC (permalink / raw)
To: git; +Cc: gitster, git, martin.agren
From: Michael J Gruber <git@grubix.eu>
f8b863598c ("builtin/merge: honor commit-msg hook for merges", 2017-09-07)
introduced the no-verify flag to merge for bypassing the commit-msg
hook, though in a different way from the implementation in commit.c.
Change the implementation in merge.c to be the same as in commit.c so
that both do the same in the same way. This also changes the output of
"git merge --help" to be more clear that the hook return code is
respected by default.
[js: * reworded commit message
* squashed documentation changes from original series' patch 3/4
]
Signed-off-by: Michael J Gruber <git@grubix.eu>
Signed-off-by: Josh Steadmon <steadmon@google.com>
---
Documentation/git-merge.txt | 2 +-
Documentation/merge-options.txt | 4 ++++
builtin/merge.c | 6 +++---
3 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
index 01fd52dc70..092529c619 100644
--- a/Documentation/git-merge.txt
+++ b/Documentation/git-merge.txt
@@ -10,7 +10,7 @@ SYNOPSIS
--------
[verse]
'git merge' [-n] [--stat] [--no-commit] [--squash] [--[no-]edit]
- [-s <strategy>] [-X <strategy-option>] [-S[<keyid>]]
+ [--no-verify] [-s <strategy>] [-X <strategy-option>] [-S[<keyid>]]
[--[no-]allow-unrelated-histories]
[--[no-]rerere-autoupdate] [-m <msg>] [-F <file>] [<commit>...]
'git merge' (--continue | --abort | --quit)
diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
index 79a00d2a4a..d6a9f4b96f 100644
--- a/Documentation/merge-options.txt
+++ b/Documentation/merge-options.txt
@@ -105,6 +105,10 @@ option can be used to override --squash.
+
With --squash, --commit is not allowed, and will fail.
+--no-verify::
+ This option bypasses the pre-merge and commit-msg hooks.
+ See also linkgit:githooks[5].
+
-s <strategy>::
--strategy=<strategy>::
Use the given merge strategy; can be supplied more than
diff --git a/builtin/merge.c b/builtin/merge.c
index e2ccbc44e2..4425a7a12e 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -81,7 +81,7 @@ static int show_progress = -1;
static int default_to_upstream = 1;
static int signoff;
static const char *sign_commit;
-static int verify_msg = 1;
+static int no_verify;
static struct strategy all_strategy[] = {
{ "recursive", DEFAULT_TWOHEAD | NO_TRIVIAL },
@@ -287,7 +287,7 @@ static struct option builtin_merge_options[] = {
N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" },
OPT_BOOL(0, "overwrite-ignore", &overwrite_ignore, N_("update ignored files (default)")),
OPT_BOOL(0, "signoff", &signoff, N_("add Signed-off-by:")),
- OPT_BOOL(0, "verify", &verify_msg, N_("verify commit-msg hook")),
+ OPT_BOOL(0, "no-verify", &no_verify, N_("bypass commit-msg hook")),
OPT_END()
};
@@ -842,7 +842,7 @@ static void prepare_to_commit(struct commit_list *remoteheads)
abort_commit(remoteheads, NULL);
}
- if (verify_msg && run_commit_hook(0 < option_edit, get_index_file(),
+ if (!no_verify && run_commit_hook(0 < option_edit, get_index_file(),
"commit-msg",
git_path_merge_msg(the_repository), NULL))
abort_commit(remoteheads, NULL);
--
2.22.0.770.g0f2c4a37fd-goog
^ permalink raw reply related [flat|nested] 59+ messages in thread* [PATCH v4 3/4] git-merge: honor pre-merge-commit hook
2019-08-05 22:43 ` [PATCH v4 " Josh Steadmon
2019-08-05 22:43 ` [PATCH v4 1/4] t7503: verify proper hook execution Josh Steadmon
2019-08-05 22:43 ` [PATCH v4 2/4] merge: do no-verify like commit Josh Steadmon
@ 2019-08-05 22:43 ` Josh Steadmon
2019-08-05 22:43 ` [PATCH v4 4/4] merge: --no-verify to bypass " Josh Steadmon
2019-08-07 18:57 ` [PATCH v5 0/4] " Josh Steadmon
4 siblings, 0 replies; 59+ messages in thread
From: Josh Steadmon @ 2019-08-05 22:43 UTC (permalink / raw)
To: git; +Cc: gitster, git, martin.agren
From: Michael J Gruber <git@drmicha.warpmail.net>
git-merge does not honor the pre-commit hook when doing automatic merge
commits, and for compatibility reasons this is going to stay.
Introduce a pre-merge-commit hook which is called for an automatic merge
commit just like pre-commit is called for a non-automatic merge commit
(or any other commit).
[js: * renamed hook from "pre-merge" to "pre-merge-commit"
* only discard the index if the hook is actually present
* expanded githooks documentation entry
* clarified that hook should write messages to stderr
* squashed test changes from the original series' patch 4/4
* modified tests to follow new pattern from this series' patch 1/4
* added a test case for non-executable merge hooks
* added a test case for failed merges
* when testing that the merge hook did not run, make sure we
actually have a merge to perform (by resetting the "side" branch
to its original state).
* reworded commit message
]
Improved-by: Martin Ågren <martin.agren@gmail.com>
Signed-off-by: Michael J Gruber <git@grubix.eu>
Signed-off-by: Martin Ågren <martin.agren@gmail.com>
Signed-off-by: Josh Steadmon <steadmon@google.com>
---
Documentation/githooks.txt | 21 +++++
builtin/merge.c | 12 +++
...-pre-commit-and-pre-merge-commit-hooks.sh} | 84 ++++++++++++++++++-
templates/hooks--pre-merge-commit.sample | 13 +++
4 files changed, 129 insertions(+), 1 deletion(-)
rename t/{t7503-pre-commit-hook.sh => t7503-pre-commit-and-pre-merge-commit-hooks.sh} (63%)
create mode 100755 templates/hooks--pre-merge-commit.sample
diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index 82cd573776..d9da474fb0 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -103,6 +103,27 @@ The default 'pre-commit' hook, when enabled--and with the
`hooks.allownonascii` config option unset or set to false--prevents
the use of non-ASCII filenames.
+pre-merge-commit
+~~~~~~~~~~~~~~~~
+
+This hook is invoked by linkgit:git-merge[1]. It takes no parameters, and is
+invoked after the merge has been carried out successfully and before
+obtaining the proposed commit log message to
+make a commit. Exiting with a non-zero status from this script
+causes the `git merge` command to abort before creating a commit.
+
+The default 'pre-merge-commit' hook, when enabled, runs the
+'pre-commit' hook, if the latter is enabled.
+
+This hook is invoked with the environment variable
+`GIT_EDITOR=:` if the command will not bring up an editor
+to modify the commit message.
+
+If the merge cannot be carried out automatically, the conflicts
+need to be resolved and the result committed separately (see
+linkgit:git-merge[1]). At that point, this hook will not be executed,
+but the 'pre-commit' hook will, if it is enabled.
+
prepare-commit-msg
~~~~~~~~~~~~~~~~~~
diff --git a/builtin/merge.c b/builtin/merge.c
index 4425a7a12e..bf0ae68c40 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -816,6 +816,18 @@ static void write_merge_heads(struct commit_list *);
static void prepare_to_commit(struct commit_list *remoteheads)
{
struct strbuf msg = STRBUF_INIT;
+ const char *index_file = get_index_file();
+
+ if (run_commit_hook(0 < option_edit, index_file, "pre-merge-commit", NULL))
+ abort_commit(remoteheads, NULL);
+ /*
+ * Re-read the index as pre-merge-commit hook could have updated it,
+ * and write it out as a tree. We must do this before we invoke
+ * the editor and after we invoke run_status above.
+ */
+ if (find_hook("pre-merge-commit"))
+ discard_cache();
+ read_cache_from(index_file);
strbuf_addbuf(&msg, &merge_msg);
if (squash)
BUG("the control must not reach here under --squash");
diff --git a/t/t7503-pre-commit-hook.sh b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
similarity index 63%
rename from t/t7503-pre-commit-hook.sh
rename to t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
index a71ec31222..0700604f03 100755
--- a/t/t7503-pre-commit-hook.sh
+++ b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
@@ -1,11 +1,12 @@
#!/bin/sh
-test_description='pre-commit hook'
+test_description='pre-commit and pre-merge-commit hooks'
. ./test-lib.sh
HOOKDIR="$(git rev-parse --git-dir)/hooks"
PRECOMMIT="$HOOKDIR/pre-commit"
+PREMERGE="$HOOKDIR/pre-merge-commit"
# Prepare sample scripts that write their $0 to actual_hooks
test_expect_success 'sample script setup' '
@@ -34,6 +35,30 @@ test_expect_success 'sample script setup' '
EOF
'
+test_expect_success 'root commit' '
+ echo "root" >file &&
+ git add file &&
+ git commit -m "zeroth" &&
+ git checkout -b side &&
+ echo "foo" >foo &&
+ git add foo &&
+ git commit -m "make it non-ff" &&
+ git branch side-orig side &&
+ git checkout master
+'
+
+test_expect_success 'setup conflicting branches' '
+ test_when_finished "git checkout master" &&
+ git checkout -b conflicting-a master &&
+ echo a >conflicting &&
+ git add conflicting &&
+ git commit -m conflicting-a &&
+ git checkout -b conflicting-b master &&
+ echo b >conflicting &&
+ git add conflicting &&
+ git commit -m conflicting-b
+'
+
test_expect_success 'with no hook' '
test_when_finished "rm -f actual_hooks" &&
echo "foo" >file &&
@@ -42,6 +67,15 @@ test_expect_success 'with no hook' '
test_path_is_missing actual_hooks
'
+test_expect_success 'with no hook (merge)' '
+ test_when_finished "rm -f actual_hooks" &&
+ git branch -f side side-orig &&
+ git checkout side &&
+ git merge -m "merge master" master &&
+ git checkout master &&
+ test_path_is_missing actual_hooks
+'
+
test_expect_success '--no-verify with no hook' '
test_when_finished "rm -f actual_hooks" &&
echo "bar" >file &&
@@ -60,6 +94,34 @@ test_expect_success 'with succeeding hook' '
test_cmp expected_hooks actual_hooks
'
+test_expect_success 'with succeeding hook (merge)' '
+ test_when_finished "rm -f \"$PREMERGE\" expected_hooks actual_hooks" &&
+ ln -s "success.sample" "$PREMERGE" &&
+ echo "$PREMERGE" >expected_hooks &&
+ git checkout side &&
+ git merge -m "merge master" master &&
+ git checkout master &&
+ test_cmp expected_hooks actual_hooks
+'
+
+test_expect_success 'automatic merge fails; both hooks are available' '
+ test_when_finished "rm -f \"$PREMERGE\" \"$PRECOMMIT\"" &&
+ test_when_finished "rm -f expected_hooks actual_hooks" &&
+ test_when_finished "git checkout master" &&
+ ln -s "success.sample" "$PREMERGE" &&
+ ln -s "success.sample" "$PRECOMMIT" &&
+
+ git checkout conflicting-a &&
+ test_must_fail git merge -m "merge conflicting-b" conflicting-b &&
+ test_path_is_missing actual_hooks &&
+
+ echo "$PRECOMMIT" >expected_hooks &&
+ echo a+b >conflicting &&
+ git add conflicting &&
+ git commit -m "resolve conflict" &&
+ test_cmp expected_hooks actual_hooks
+'
+
test_expect_success '--no-verify with succeeding hook' '
test_when_finished "rm -f \"$PRECOMMIT\" actual_hooks" &&
ln -s "success.sample" "$PRECOMMIT" &&
@@ -88,6 +150,16 @@ test_expect_success '--no-verify with failing hook' '
test_path_is_missing actual_hooks
'
+test_expect_success 'with failing hook (merge)' '
+ test_when_finished "rm -f \"$PREMERGE\" expected_hooks actual_hooks" &&
+ ln -s "fail.sample" "$PREMERGE" &&
+ echo "$PREMERGE" >expected_hooks &&
+ git checkout side &&
+ test_must_fail git merge -m "merge master" master &&
+ git checkout master &&
+ test_cmp expected_hooks actual_hooks
+'
+
test_expect_success POSIXPERM 'with non-executable hook' '
test_when_finished "rm -f \"$PRECOMMIT\" actual_hooks" &&
ln -s "non-exec.sample" "$PRECOMMIT" &&
@@ -106,6 +178,16 @@ test_expect_success POSIXPERM '--no-verify with non-executable hook' '
test_path_is_missing actual_hooks
'
+test_expect_success POSIXPERM 'with non-executable hook (merge)' '
+ test_when_finished "rm -f \"$PREMERGE\" actual_hooks" &&
+ ln -s "non-exec.sample" "$PREMERGE" &&
+ git branch -f side side-orig &&
+ git checkout side &&
+ git merge -m "merge master" master &&
+ git checkout master &&
+ test_path_is_missing actual_hooks
+'
+
test_expect_success 'with hook requiring GIT_PREFIX' '
test_when_finished "rm -rf \"$PRECOMMIT\" expected_hooks actual_hooks success" &&
ln -s "require-prefix.sample" "$PRECOMMIT" &&
diff --git a/templates/hooks--pre-merge-commit.sample b/templates/hooks--pre-merge-commit.sample
new file mode 100755
index 0000000000..399eab1924
--- /dev/null
+++ b/templates/hooks--pre-merge-commit.sample
@@ -0,0 +1,13 @@
+#!/bin/sh
+#
+# An example hook script to verify what is about to be committed.
+# Called by "git merge" with no arguments. The hook should
+# exit with non-zero status after issuing an appropriate message to
+# stderr if it wants to stop the merge commit.
+#
+# To enable this hook, rename this file to "pre-merge-commit".
+
+. git-sh-setup
+test -x "$GIT_DIR/hooks/pre-commit" &&
+ exec "$GIT_DIR/hooks/pre-commit"
+:
--
2.22.0.770.g0f2c4a37fd-goog
^ permalink raw reply related [flat|nested] 59+ messages in thread* [PATCH v4 4/4] merge: --no-verify to bypass pre-merge-commit hook
2019-08-05 22:43 ` [PATCH v4 " Josh Steadmon
` (2 preceding siblings ...)
2019-08-05 22:43 ` [PATCH v4 3/4] git-merge: honor pre-merge-commit hook Josh Steadmon
@ 2019-08-05 22:43 ` Josh Steadmon
2019-08-07 18:57 ` [PATCH v5 0/4] " Josh Steadmon
4 siblings, 0 replies; 59+ messages in thread
From: Josh Steadmon @ 2019-08-05 22:43 UTC (permalink / raw)
To: git; +Cc: gitster, git, martin.agren
From: Michael J Gruber <git@drmicha.warpmail.net>
Analogous to commit, introduce a '--no-verify' option which bypasses the
pre-merge-commit hook. The shorthand '-n' is taken by '--no-stat'
already.
[js: * reworded commit message to reflect current state of --no-stat flag
and new hook name
* fixed flag documentation to reflect new hook name
* cleaned up trailing whitespace
* squashed test changes from the original series' patch 4/4
* modified tests to follow pattern from this series' patch 1/4
* added a test case for --no-verify with non-executable hook
* when testing that the merge hook did not run, make sure we
actually have a merge to perform (by resetting the "side" branch
to its original state).
]
Improved-by: Martin Ågren <martin.agren@gmail.com>
Signed-off-by: Michael J Gruber <git@grubix.eu>
Signed-off-by: Martin Ågren <martin.agren@gmail.com>
Signed-off-by: Josh Steadmon <steadmon@google.com>
---
Documentation/githooks.txt | 3 +-
builtin/merge.c | 4 +-
...3-pre-commit-and-pre-merge-commit-hooks.sh | 39 +++++++++++++++++++
3 files changed, 43 insertions(+), 3 deletions(-)
diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index d9da474fb0..57d6e2b98d 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -106,7 +106,8 @@ the use of non-ASCII filenames.
pre-merge-commit
~~~~~~~~~~~~~~~~
-This hook is invoked by linkgit:git-merge[1]. It takes no parameters, and is
+This hook is invoked by linkgit:git-merge[1], and can be bypassed
+with the `--no-verify` option. It takes no parameters, and is
invoked after the merge has been carried out successfully and before
obtaining the proposed commit log message to
make a commit. Exiting with a non-zero status from this script
diff --git a/builtin/merge.c b/builtin/merge.c
index bf0ae68c40..c9746e37b8 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -287,7 +287,7 @@ static struct option builtin_merge_options[] = {
N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" },
OPT_BOOL(0, "overwrite-ignore", &overwrite_ignore, N_("update ignored files (default)")),
OPT_BOOL(0, "signoff", &signoff, N_("add Signed-off-by:")),
- OPT_BOOL(0, "no-verify", &no_verify, N_("bypass commit-msg hook")),
+ OPT_BOOL(0, "no-verify", &no_verify, N_("bypass pre-merge-commit and commit-msg hooks")),
OPT_END()
};
@@ -818,7 +818,7 @@ static void prepare_to_commit(struct commit_list *remoteheads)
struct strbuf msg = STRBUF_INIT;
const char *index_file = get_index_file();
- if (run_commit_hook(0 < option_edit, index_file, "pre-merge-commit", NULL))
+ if (!no_verify && run_commit_hook(0 < option_edit, index_file, "pre-merge-commit", NULL))
abort_commit(remoteheads, NULL);
/*
* Re-read the index as pre-merge-commit hook could have updated it,
diff --git a/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
index 0700604f03..d900c3a696 100755
--- a/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
+++ b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
@@ -84,6 +84,15 @@ test_expect_success '--no-verify with no hook' '
test_path_is_missing actual_hooks
'
+test_expect_success '--no-verify with no hook (merge)' '
+ test_when_finished "rm -f actual_hooks" &&
+ git branch -f side side-orig &&
+ git checkout side &&
+ git merge --no-verify -m "merge master" master &&
+ git checkout master &&
+ test_path_is_missing actual_hooks
+'
+
test_expect_success 'with succeeding hook' '
test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks" &&
ln -s "success.sample" "$PRECOMMIT" &&
@@ -131,6 +140,16 @@ test_expect_success '--no-verify with succeeding hook' '
test_path_is_missing actual_hooks
'
+test_expect_success '--no-verify with succeeding hook (merge)' '
+ test_when_finished "rm -f \"$PREMERGE\" actual_hooks" &&
+ ln -s "success.sample" "$PREMERGE" &&
+ git branch -f side side-orig &&
+ git checkout side &&
+ git merge --no-verify -m "merge master" master &&
+ git checkout master &&
+ test_path_is_missing actual_hooks
+'
+
test_expect_success 'with failing hook' '
test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks" &&
ln -s "fail.sample" "$PRECOMMIT" &&
@@ -160,6 +179,16 @@ test_expect_success 'with failing hook (merge)' '
test_cmp expected_hooks actual_hooks
'
+test_expect_success '--no-verify with failing hook (merge)' '
+ test_when_finished "rm -f \"$PREMERGE\" actual_hooks" &&
+ ln -s "fail.sample" "$PREMERGE" &&
+ git branch -f side side-orig &&
+ git checkout side &&
+ git merge --no-verify -m "merge master" master &&
+ git checkout master &&
+ test_path_is_missing actual_hooks
+'
+
test_expect_success POSIXPERM 'with non-executable hook' '
test_when_finished "rm -f \"$PRECOMMIT\" actual_hooks" &&
ln -s "non-exec.sample" "$PRECOMMIT" &&
@@ -188,6 +217,16 @@ test_expect_success POSIXPERM 'with non-executable hook (merge)' '
test_path_is_missing actual_hooks
'
+test_expect_success POSIXPERM '--no-verify with non-executable hook (merge)' '
+ test_when_finished "rm -f \"$PREMERGE\" actual_hooks" &&
+ ln -s "non-exec.sample" "$PREMERGE" &&
+ git branch -f side side-orig &&
+ git checkout side &&
+ git merge --no-verify -m "merge master" master &&
+ git checkout master &&
+ test_path_is_missing actual_hooks
+'
+
test_expect_success 'with hook requiring GIT_PREFIX' '
test_when_finished "rm -rf \"$PRECOMMIT\" expected_hooks actual_hooks success" &&
ln -s "require-prefix.sample" "$PRECOMMIT" &&
--
2.22.0.770.g0f2c4a37fd-goog
^ permalink raw reply related [flat|nested] 59+ messages in thread* [PATCH v5 0/4] pre-merge-commit hook
2019-08-05 22:43 ` [PATCH v4 " Josh Steadmon
` (3 preceding siblings ...)
2019-08-05 22:43 ` [PATCH v4 4/4] merge: --no-verify to bypass " Josh Steadmon
@ 2019-08-07 18:57 ` Josh Steadmon
2019-08-07 18:57 ` [PATCH v5 1/4] t7503: verify proper hook execution Josh Steadmon
` (4 more replies)
4 siblings, 5 replies; 59+ messages in thread
From: Josh Steadmon @ 2019-08-07 18:57 UTC (permalink / raw)
To: git; +Cc: gitster, git, martin.agren
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
^ permalink raw reply [flat|nested] 59+ messages in thread* [PATCH v5 1/4] t7503: verify proper hook execution
2019-08-07 18:57 ` [PATCH v5 0/4] " Josh Steadmon
@ 2019-08-07 18:57 ` Josh Steadmon
2019-08-07 18:57 ` [PATCH v5 2/4] merge: do no-verify like commit Josh Steadmon
` (3 subsequent siblings)
4 siblings, 0 replies; 59+ messages in thread
From: Josh Steadmon @ 2019-08-07 18:57 UTC (permalink / raw)
To: git; +Cc: gitster, git, martin.agren
t7503 did not verify that the expected hooks actually ran during
testing. Fix that by making the hook scripts write their $0 into a file
so that we can compare actual execution vs. expected execution.
While we're at it, do some test style cleanups, such as using
write_script() and doing setup inside a test_expect_success block.
Improved-by: Martin Ågren <martin.agren@gmail.com>
Signed-off-by: Martin Ågren <martin.agren@gmail.com>
Signed-off-by: Josh Steadmon <steadmon@google.com>
---
t/t7503-pre-commit-hook.sh | 157 +++++++++++++++++++++----------------
1 file changed, 89 insertions(+), 68 deletions(-)
diff --git a/t/t7503-pre-commit-hook.sh b/t/t7503-pre-commit-hook.sh
index 984889b39d..6aa83204c2 100755
--- a/t/t7503-pre-commit-hook.sh
+++ b/t/t7503-pre-commit-hook.sh
@@ -4,124 +4,144 @@ test_description='pre-commit hook'
. ./test-lib.sh
-test_expect_success 'with no hook' '
+HOOKDIR="$(git rev-parse --git-dir)/hooks"
+PRECOMMIT="$HOOKDIR/pre-commit"
+
+# Prepare sample scripts that write their $0 to actual_hooks
+test_expect_success 'sample script setup' '
+ mkdir -p "$HOOKDIR" &&
+ write_script "$HOOKDIR/success.sample" <<-\EOF &&
+ echo $0 >>actual_hooks
+ exit 0
+ EOF
+ write_script "$HOOKDIR/fail.sample" <<-\EOF &&
+ echo $0 >>actual_hooks
+ exit 1
+ EOF
+ write_script "$HOOKDIR/non-exec.sample" <<-\EOF &&
+ echo $0 >>actual_hooks
+ exit 1
+ EOF
+ chmod -x "$HOOKDIR/non-exec.sample" &&
+ write_script "$HOOKDIR/require-prefix.sample" <<-\EOF &&
+ echo $0 >>actual_hooks
+ test $GIT_PREFIX = "success/"
+ EOF
+ write_script "$HOOKDIR/check-author.sample" <<-\EOF
+ echo $0 >>actual_hooks
+ test "$GIT_AUTHOR_NAME" = "New Author" &&
+ test "$GIT_AUTHOR_EMAIL" = "newauthor@example.com"
+ EOF
+'
- echo "foo" > file &&
+test_expect_success 'with no hook' '
+ test_when_finished "rm -f actual_hooks" &&
+ echo "foo" >file &&
git add file &&
- git commit -m "first"
-
+ git commit -m "first" &&
+ test_path_is_missing actual_hooks
'
test_expect_success '--no-verify with no hook' '
-
- echo "bar" > file &&
+ test_when_finished "rm -f actual_hooks" &&
+ echo "bar" >file &&
git add file &&
- git commit --no-verify -m "bar"
-
+ git commit --no-verify -m "bar" &&
+ test_path_is_missing actual_hooks
'
-# now install hook that always succeeds
-HOOKDIR="$(git rev-parse --git-dir)/hooks"
-HOOK="$HOOKDIR/pre-commit"
-mkdir -p "$HOOKDIR"
-cat > "$HOOK" <<EOF
-#!/bin/sh
-exit 0
-EOF
-chmod +x "$HOOK"
-
test_expect_success 'with succeeding hook' '
-
- echo "more" >> file &&
+ test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks" &&
+ cp "$HOOKDIR/success.sample" "$PRECOMMIT" &&
+ echo "$PRECOMMIT" >expected_hooks &&
+ echo "more" >>file &&
git add file &&
- git commit -m "more"
-
+ git commit -m "more" &&
+ test_cmp expected_hooks actual_hooks
'
test_expect_success '--no-verify with succeeding hook' '
-
- echo "even more" >> file &&
+ test_when_finished "rm -f \"$PRECOMMIT\" actual_hooks" &&
+ cp "$HOOKDIR/success.sample" "$PRECOMMIT" &&
+ echo "even more" >>file &&
git add file &&
- git commit --no-verify -m "even more"
-
+ git commit --no-verify -m "even more" &&
+ test_path_is_missing actual_hooks
'
-# now a hook that fails
-cat > "$HOOK" <<EOF
-#!/bin/sh
-exit 1
-EOF
-
test_expect_success 'with failing hook' '
-
- echo "another" >> file &&
+ test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks" &&
+ cp "$HOOKDIR/fail.sample" "$PRECOMMIT" &&
+ echo "$PRECOMMIT" >expected_hooks &&
+ echo "another" >>file &&
git add file &&
- test_must_fail git commit -m "another"
-
+ test_must_fail git commit -m "another" &&
+ test_cmp expected_hooks actual_hooks
'
test_expect_success '--no-verify with failing hook' '
-
- echo "stuff" >> file &&
+ test_when_finished "rm -f \"$PRECOMMIT\" actual_hooks" &&
+ cp "$HOOKDIR/fail.sample" "$PRECOMMIT" &&
+ echo "stuff" >>file &&
git add file &&
- git commit --no-verify -m "stuff"
-
+ git commit --no-verify -m "stuff" &&
+ test_path_is_missing actual_hooks
'
-chmod -x "$HOOK"
test_expect_success POSIXPERM 'with non-executable hook' '
-
- echo "content" >> file &&
+ test_when_finished "rm -f \"$PRECOMMIT\" actual_hooks" &&
+ cp "$HOOKDIR/non-exec.sample" "$PRECOMMIT" &&
+ echo "content" >>file &&
git add file &&
- git commit -m "content"
-
+ git commit -m "content" &&
+ test_path_is_missing actual_hooks
'
test_expect_success POSIXPERM '--no-verify with non-executable hook' '
-
- echo "more content" >> file &&
+ test_when_finished "rm -f \"$PRECOMMIT\" actual_hooks" &&
+ cp "$HOOKDIR/non-exec.sample" "$PRECOMMIT" &&
+ echo "more content" >>file &&
git add file &&
- git commit --no-verify -m "more content"
-
+ git commit --no-verify -m "more content" &&
+ test_path_is_missing actual_hooks
'
-chmod +x "$HOOK"
-
-# a hook that checks $GIT_PREFIX and succeeds inside the
-# success/ subdirectory only
-cat > "$HOOK" <<EOF
-#!/bin/sh
-test \$GIT_PREFIX = success/
-EOF
test_expect_success 'with hook requiring GIT_PREFIX' '
-
- echo "more content" >> file &&
+ test_when_finished "rm -rf \"$PRECOMMIT\" expected_hooks actual_hooks success" &&
+ cp "$HOOKDIR/require-prefix.sample" "$PRECOMMIT" &&
+ echo "$PRECOMMIT" >expected_hooks &&
+ echo "more content" >>file &&
git add file &&
mkdir success &&
(
cd success &&
git commit -m "hook requires GIT_PREFIX = success/"
) &&
- rmdir success
+ test_cmp expected_hooks actual_hooks
'
test_expect_success 'with failing hook requiring GIT_PREFIX' '
-
- echo "more content" >> file &&
+ test_when_finished "rm -rf \"$PRECOMMIT\" expected_hooks actual_hooks fail" &&
+ cp "$HOOKDIR/require-prefix.sample" "$PRECOMMIT" &&
+ echo "$PRECOMMIT" >expected_hooks &&
+ echo "more content" >>file &&
git add file &&
mkdir fail &&
(
cd fail &&
test_must_fail git commit -m "hook must fail"
) &&
- rmdir fail &&
- git checkout -- file
+ git checkout -- file &&
+ test_cmp expected_hooks actual_hooks
'
test_expect_success 'check the author in hook' '
- write_script "$HOOK" <<-\EOF &&
- test "$GIT_AUTHOR_NAME" = "New Author" &&
- test "$GIT_AUTHOR_EMAIL" = "newauthor@example.com"
+ test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks" &&
+ cp "$HOOKDIR/check-author.sample" "$PRECOMMIT" &&
+ cat >expected_hooks <<-EOF &&
+ $PRECOMMIT
+ $PRECOMMIT
+ $PRECOMMIT
EOF
test_must_fail git commit --allow-empty -m "by a.u.thor" &&
(
@@ -133,7 +153,8 @@ test_expect_success 'check the author in hook' '
) &&
git commit --author="New Author <newauthor@example.com>" \
--allow-empty -m "by new.author via command line" &&
- git show -s
+ git show -s &&
+ test_cmp expected_hooks actual_hooks
'
test_done
--
2.22.0.770.g0f2c4a37fd-goog
^ permalink raw reply related [flat|nested] 59+ messages in thread* [PATCH v5 2/4] merge: do no-verify like commit
2019-08-07 18:57 ` [PATCH v5 0/4] " Josh Steadmon
2019-08-07 18:57 ` [PATCH v5 1/4] t7503: verify proper hook execution Josh Steadmon
@ 2019-08-07 18:57 ` Josh Steadmon
2019-08-07 18:57 ` [PATCH v5 3/4] git-merge: honor pre-merge-commit hook Josh Steadmon
` (2 subsequent siblings)
4 siblings, 0 replies; 59+ messages in thread
From: Josh Steadmon @ 2019-08-07 18:57 UTC (permalink / raw)
To: git; +Cc: gitster, git, martin.agren
From: Michael J Gruber <git@grubix.eu>
f8b863598c ("builtin/merge: honor commit-msg hook for merges", 2017-09-07)
introduced the no-verify flag to merge for bypassing the commit-msg
hook, though in a different way from the implementation in commit.c.
Change the implementation in merge.c to be the same as in commit.c so
that both do the same in the same way. This also changes the output of
"git merge --help" to be more clear that the hook return code is
respected by default.
[js: * reworded commit message
* squashed documentation changes from original series' patch 3/4
]
Signed-off-by: Michael J Gruber <git@grubix.eu>
Signed-off-by: Josh Steadmon <steadmon@google.com>
---
Documentation/git-merge.txt | 2 +-
Documentation/merge-options.txt | 4 ++++
builtin/merge.c | 6 +++---
3 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
index 01fd52dc70..092529c619 100644
--- a/Documentation/git-merge.txt
+++ b/Documentation/git-merge.txt
@@ -10,7 +10,7 @@ SYNOPSIS
--------
[verse]
'git merge' [-n] [--stat] [--no-commit] [--squash] [--[no-]edit]
- [-s <strategy>] [-X <strategy-option>] [-S[<keyid>]]
+ [--no-verify] [-s <strategy>] [-X <strategy-option>] [-S[<keyid>]]
[--[no-]allow-unrelated-histories]
[--[no-]rerere-autoupdate] [-m <msg>] [-F <file>] [<commit>...]
'git merge' (--continue | --abort | --quit)
diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
index 79a00d2a4a..d6a9f4b96f 100644
--- a/Documentation/merge-options.txt
+++ b/Documentation/merge-options.txt
@@ -105,6 +105,10 @@ option can be used to override --squash.
+
With --squash, --commit is not allowed, and will fail.
+--no-verify::
+ This option bypasses the pre-merge and commit-msg hooks.
+ See also linkgit:githooks[5].
+
-s <strategy>::
--strategy=<strategy>::
Use the given merge strategy; can be supplied more than
diff --git a/builtin/merge.c b/builtin/merge.c
index e2ccbc44e2..4425a7a12e 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -81,7 +81,7 @@ static int show_progress = -1;
static int default_to_upstream = 1;
static int signoff;
static const char *sign_commit;
-static int verify_msg = 1;
+static int no_verify;
static struct strategy all_strategy[] = {
{ "recursive", DEFAULT_TWOHEAD | NO_TRIVIAL },
@@ -287,7 +287,7 @@ static struct option builtin_merge_options[] = {
N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" },
OPT_BOOL(0, "overwrite-ignore", &overwrite_ignore, N_("update ignored files (default)")),
OPT_BOOL(0, "signoff", &signoff, N_("add Signed-off-by:")),
- OPT_BOOL(0, "verify", &verify_msg, N_("verify commit-msg hook")),
+ OPT_BOOL(0, "no-verify", &no_verify, N_("bypass commit-msg hook")),
OPT_END()
};
@@ -842,7 +842,7 @@ static void prepare_to_commit(struct commit_list *remoteheads)
abort_commit(remoteheads, NULL);
}
- if (verify_msg && run_commit_hook(0 < option_edit, get_index_file(),
+ if (!no_verify && run_commit_hook(0 < option_edit, get_index_file(),
"commit-msg",
git_path_merge_msg(the_repository), NULL))
abort_commit(remoteheads, NULL);
--
2.22.0.770.g0f2c4a37fd-goog
^ permalink raw reply related [flat|nested] 59+ messages in thread* [PATCH v5 3/4] git-merge: honor pre-merge-commit hook
2019-08-07 18:57 ` [PATCH v5 0/4] " Josh Steadmon
2019-08-07 18:57 ` [PATCH v5 1/4] t7503: verify proper hook execution Josh Steadmon
2019-08-07 18:57 ` [PATCH v5 2/4] merge: do no-verify like commit Josh Steadmon
@ 2019-08-07 18:57 ` Josh Steadmon
2019-08-07 18:57 ` [PATCH v5 4/4] merge: --no-verify to bypass " Josh Steadmon
2019-08-08 13:08 ` [PATCH v5 0/4] " Martin Ågren
4 siblings, 0 replies; 59+ messages in thread
From: Josh Steadmon @ 2019-08-07 18:57 UTC (permalink / raw)
To: git; +Cc: gitster, git, martin.agren
From: Michael J Gruber <git@drmicha.warpmail.net>
git-merge does not honor the pre-commit hook when doing automatic merge
commits, and for compatibility reasons this is going to stay.
Introduce a pre-merge-commit hook which is called for an automatic merge
commit just like pre-commit is called for a non-automatic merge commit
(or any other commit).
[js: * renamed hook from "pre-merge" to "pre-merge-commit"
* only discard the index if the hook is actually present
* expanded githooks documentation entry
* clarified that hook should write messages to stderr
* squashed test changes from the original series' patch 4/4
* modified tests to follow new pattern from this series' patch 1/4
* added a test case for non-executable merge hooks
* added a test case for failed merges
* when testing that the merge hook did not run, make sure we
actually have a merge to perform (by resetting the "side" branch
to its original state).
* reworded commit message
]
Improved-by: Martin Ågren <martin.agren@gmail.com>
Signed-off-by: Michael J Gruber <git@grubix.eu>
Signed-off-by: Martin Ågren <martin.agren@gmail.com>
Signed-off-by: Josh Steadmon <steadmon@google.com>
---
Documentation/githooks.txt | 21 +++++
builtin/merge.c | 12 +++
...-pre-commit-and-pre-merge-commit-hooks.sh} | 84 ++++++++++++++++++-
templates/hooks--pre-merge-commit.sample | 13 +++
4 files changed, 129 insertions(+), 1 deletion(-)
rename t/{t7503-pre-commit-hook.sh => t7503-pre-commit-and-pre-merge-commit-hooks.sh} (63%)
create mode 100755 templates/hooks--pre-merge-commit.sample
diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index 82cd573776..d9da474fb0 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -103,6 +103,27 @@ The default 'pre-commit' hook, when enabled--and with the
`hooks.allownonascii` config option unset or set to false--prevents
the use of non-ASCII filenames.
+pre-merge-commit
+~~~~~~~~~~~~~~~~
+
+This hook is invoked by linkgit:git-merge[1]. It takes no parameters, and is
+invoked after the merge has been carried out successfully and before
+obtaining the proposed commit log message to
+make a commit. Exiting with a non-zero status from this script
+causes the `git merge` command to abort before creating a commit.
+
+The default 'pre-merge-commit' hook, when enabled, runs the
+'pre-commit' hook, if the latter is enabled.
+
+This hook is invoked with the environment variable
+`GIT_EDITOR=:` if the command will not bring up an editor
+to modify the commit message.
+
+If the merge cannot be carried out automatically, the conflicts
+need to be resolved and the result committed separately (see
+linkgit:git-merge[1]). At that point, this hook will not be executed,
+but the 'pre-commit' hook will, if it is enabled.
+
prepare-commit-msg
~~~~~~~~~~~~~~~~~~
diff --git a/builtin/merge.c b/builtin/merge.c
index 4425a7a12e..bf0ae68c40 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -816,6 +816,18 @@ static void write_merge_heads(struct commit_list *);
static void prepare_to_commit(struct commit_list *remoteheads)
{
struct strbuf msg = STRBUF_INIT;
+ const char *index_file = get_index_file();
+
+ if (run_commit_hook(0 < option_edit, index_file, "pre-merge-commit", NULL))
+ abort_commit(remoteheads, NULL);
+ /*
+ * Re-read the index as pre-merge-commit hook could have updated it,
+ * and write it out as a tree. We must do this before we invoke
+ * the editor and after we invoke run_status above.
+ */
+ if (find_hook("pre-merge-commit"))
+ discard_cache();
+ read_cache_from(index_file);
strbuf_addbuf(&msg, &merge_msg);
if (squash)
BUG("the control must not reach here under --squash");
diff --git a/t/t7503-pre-commit-hook.sh b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
similarity index 63%
rename from t/t7503-pre-commit-hook.sh
rename to t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
index 6aa83204c2..7a5434c7ab 100755
--- a/t/t7503-pre-commit-hook.sh
+++ b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
@@ -1,11 +1,12 @@
#!/bin/sh
-test_description='pre-commit hook'
+test_description='pre-commit and pre-merge-commit hooks'
. ./test-lib.sh
HOOKDIR="$(git rev-parse --git-dir)/hooks"
PRECOMMIT="$HOOKDIR/pre-commit"
+PREMERGE="$HOOKDIR/pre-merge-commit"
# Prepare sample scripts that write their $0 to actual_hooks
test_expect_success 'sample script setup' '
@@ -34,6 +35,30 @@ test_expect_success 'sample script setup' '
EOF
'
+test_expect_success 'root commit' '
+ echo "root" >file &&
+ git add file &&
+ git commit -m "zeroth" &&
+ git checkout -b side &&
+ echo "foo" >foo &&
+ git add foo &&
+ git commit -m "make it non-ff" &&
+ git branch side-orig side &&
+ git checkout master
+'
+
+test_expect_success 'setup conflicting branches' '
+ test_when_finished "git checkout master" &&
+ git checkout -b conflicting-a master &&
+ echo a >conflicting &&
+ git add conflicting &&
+ git commit -m conflicting-a &&
+ git checkout -b conflicting-b master &&
+ echo b >conflicting &&
+ git add conflicting &&
+ git commit -m conflicting-b
+'
+
test_expect_success 'with no hook' '
test_when_finished "rm -f actual_hooks" &&
echo "foo" >file &&
@@ -42,6 +67,15 @@ test_expect_success 'with no hook' '
test_path_is_missing actual_hooks
'
+test_expect_success 'with no hook (merge)' '
+ test_when_finished "rm -f actual_hooks" &&
+ git branch -f side side-orig &&
+ git checkout side &&
+ git merge -m "merge master" master &&
+ git checkout master &&
+ test_path_is_missing actual_hooks
+'
+
test_expect_success '--no-verify with no hook' '
test_when_finished "rm -f actual_hooks" &&
echo "bar" >file &&
@@ -60,6 +94,34 @@ test_expect_success 'with succeeding hook' '
test_cmp expected_hooks actual_hooks
'
+test_expect_success 'with succeeding hook (merge)' '
+ test_when_finished "rm -f \"$PREMERGE\" expected_hooks actual_hooks" &&
+ cp "$HOOKDIR/success.sample" "$PREMERGE" &&
+ echo "$PREMERGE" >expected_hooks &&
+ git checkout side &&
+ git merge -m "merge master" master &&
+ git checkout master &&
+ test_cmp expected_hooks actual_hooks
+'
+
+test_expect_success 'automatic merge fails; both hooks are available' '
+ test_when_finished "rm -f \"$PREMERGE\" \"$PRECOMMIT\"" &&
+ test_when_finished "rm -f expected_hooks actual_hooks" &&
+ test_when_finished "git checkout master" &&
+ cp "$HOOKDIR/success.sample" "$PREMERGE" &&
+ cp "$HOOKDIR/success.sample" "$PRECOMMIT" &&
+
+ git checkout conflicting-a &&
+ test_must_fail git merge -m "merge conflicting-b" conflicting-b &&
+ test_path_is_missing actual_hooks &&
+
+ echo "$PRECOMMIT" >expected_hooks &&
+ echo a+b >conflicting &&
+ git add conflicting &&
+ git commit -m "resolve conflict" &&
+ test_cmp expected_hooks actual_hooks
+'
+
test_expect_success '--no-verify with succeeding hook' '
test_when_finished "rm -f \"$PRECOMMIT\" actual_hooks" &&
cp "$HOOKDIR/success.sample" "$PRECOMMIT" &&
@@ -88,6 +150,16 @@ test_expect_success '--no-verify with failing hook' '
test_path_is_missing actual_hooks
'
+test_expect_success 'with failing hook (merge)' '
+ test_when_finished "rm -f \"$PREMERGE\" expected_hooks actual_hooks" &&
+ cp "$HOOKDIR/fail.sample" "$PREMERGE" &&
+ echo "$PREMERGE" >expected_hooks &&
+ git checkout side &&
+ test_must_fail git merge -m "merge master" master &&
+ git checkout master &&
+ test_cmp expected_hooks actual_hooks
+'
+
test_expect_success POSIXPERM 'with non-executable hook' '
test_when_finished "rm -f \"$PRECOMMIT\" actual_hooks" &&
cp "$HOOKDIR/non-exec.sample" "$PRECOMMIT" &&
@@ -106,6 +178,16 @@ test_expect_success POSIXPERM '--no-verify with non-executable hook' '
test_path_is_missing actual_hooks
'
+test_expect_success POSIXPERM 'with non-executable hook (merge)' '
+ test_when_finished "rm -f \"$PREMERGE\" actual_hooks" &&
+ cp "$HOOKDIR/non-exec.sample" "$PREMERGE" &&
+ git branch -f side side-orig &&
+ git checkout side &&
+ git merge -m "merge master" master &&
+ git checkout master &&
+ test_path_is_missing actual_hooks
+'
+
test_expect_success 'with hook requiring GIT_PREFIX' '
test_when_finished "rm -rf \"$PRECOMMIT\" expected_hooks actual_hooks success" &&
cp "$HOOKDIR/require-prefix.sample" "$PRECOMMIT" &&
diff --git a/templates/hooks--pre-merge-commit.sample b/templates/hooks--pre-merge-commit.sample
new file mode 100755
index 0000000000..399eab1924
--- /dev/null
+++ b/templates/hooks--pre-merge-commit.sample
@@ -0,0 +1,13 @@
+#!/bin/sh
+#
+# An example hook script to verify what is about to be committed.
+# Called by "git merge" with no arguments. The hook should
+# exit with non-zero status after issuing an appropriate message to
+# stderr if it wants to stop the merge commit.
+#
+# To enable this hook, rename this file to "pre-merge-commit".
+
+. git-sh-setup
+test -x "$GIT_DIR/hooks/pre-commit" &&
+ exec "$GIT_DIR/hooks/pre-commit"
+:
--
2.22.0.770.g0f2c4a37fd-goog
^ permalink raw reply related [flat|nested] 59+ messages in thread* [PATCH v5 4/4] merge: --no-verify to bypass pre-merge-commit hook
2019-08-07 18:57 ` [PATCH v5 0/4] " Josh Steadmon
` (2 preceding siblings ...)
2019-08-07 18:57 ` [PATCH v5 3/4] git-merge: honor pre-merge-commit hook Josh Steadmon
@ 2019-08-07 18:57 ` Josh Steadmon
2019-08-08 13:08 ` [PATCH v5 0/4] " Martin Ågren
4 siblings, 0 replies; 59+ messages in thread
From: Josh Steadmon @ 2019-08-07 18:57 UTC (permalink / raw)
To: git; +Cc: gitster, git, martin.agren
From: Michael J Gruber <git@drmicha.warpmail.net>
Analogous to commit, introduce a '--no-verify' option which bypasses the
pre-merge-commit hook. The shorthand '-n' is taken by '--no-stat'
already.
[js: * reworded commit message to reflect current state of --no-stat flag
and new hook name
* fixed flag documentation to reflect new hook name
* cleaned up trailing whitespace
* squashed test changes from the original series' patch 4/4
* modified tests to follow pattern from this series' patch 1/4
* added a test case for --no-verify with non-executable hook
* when testing that the merge hook did not run, make sure we
actually have a merge to perform (by resetting the "side" branch
to its original state).
]
Improved-by: Martin Ågren <martin.agren@gmail.com>
Signed-off-by: Michael J Gruber <git@grubix.eu>
Signed-off-by: Martin Ågren <martin.agren@gmail.com>
Signed-off-by: Josh Steadmon <steadmon@google.com>
---
Documentation/githooks.txt | 3 +-
builtin/merge.c | 4 +-
...3-pre-commit-and-pre-merge-commit-hooks.sh | 39 +++++++++++++++++++
3 files changed, 43 insertions(+), 3 deletions(-)
diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index d9da474fb0..57d6e2b98d 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -106,7 +106,8 @@ the use of non-ASCII filenames.
pre-merge-commit
~~~~~~~~~~~~~~~~
-This hook is invoked by linkgit:git-merge[1]. It takes no parameters, and is
+This hook is invoked by linkgit:git-merge[1], and can be bypassed
+with the `--no-verify` option. It takes no parameters, and is
invoked after the merge has been carried out successfully and before
obtaining the proposed commit log message to
make a commit. Exiting with a non-zero status from this script
diff --git a/builtin/merge.c b/builtin/merge.c
index bf0ae68c40..c9746e37b8 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -287,7 +287,7 @@ static struct option builtin_merge_options[] = {
N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" },
OPT_BOOL(0, "overwrite-ignore", &overwrite_ignore, N_("update ignored files (default)")),
OPT_BOOL(0, "signoff", &signoff, N_("add Signed-off-by:")),
- OPT_BOOL(0, "no-verify", &no_verify, N_("bypass commit-msg hook")),
+ OPT_BOOL(0, "no-verify", &no_verify, N_("bypass pre-merge-commit and commit-msg hooks")),
OPT_END()
};
@@ -818,7 +818,7 @@ static void prepare_to_commit(struct commit_list *remoteheads)
struct strbuf msg = STRBUF_INIT;
const char *index_file = get_index_file();
- if (run_commit_hook(0 < option_edit, index_file, "pre-merge-commit", NULL))
+ if (!no_verify && run_commit_hook(0 < option_edit, index_file, "pre-merge-commit", NULL))
abort_commit(remoteheads, NULL);
/*
* Re-read the index as pre-merge-commit hook could have updated it,
diff --git a/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
index 7a5434c7ab..b3485450a2 100755
--- a/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
+++ b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
@@ -84,6 +84,15 @@ test_expect_success '--no-verify with no hook' '
test_path_is_missing actual_hooks
'
+test_expect_success '--no-verify with no hook (merge)' '
+ test_when_finished "rm -f actual_hooks" &&
+ git branch -f side side-orig &&
+ git checkout side &&
+ git merge --no-verify -m "merge master" master &&
+ git checkout master &&
+ test_path_is_missing actual_hooks
+'
+
test_expect_success 'with succeeding hook' '
test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks" &&
cp "$HOOKDIR/success.sample" "$PRECOMMIT" &&
@@ -131,6 +140,16 @@ test_expect_success '--no-verify with succeeding hook' '
test_path_is_missing actual_hooks
'
+test_expect_success '--no-verify with succeeding hook (merge)' '
+ test_when_finished "rm -f \"$PREMERGE\" actual_hooks" &&
+ cp "$HOOKDIR/success.sample" "$PREMERGE" &&
+ git branch -f side side-orig &&
+ git checkout side &&
+ git merge --no-verify -m "merge master" master &&
+ git checkout master &&
+ test_path_is_missing actual_hooks
+'
+
test_expect_success 'with failing hook' '
test_when_finished "rm -f \"$PRECOMMIT\" expected_hooks actual_hooks" &&
cp "$HOOKDIR/fail.sample" "$PRECOMMIT" &&
@@ -160,6 +179,16 @@ test_expect_success 'with failing hook (merge)' '
test_cmp expected_hooks actual_hooks
'
+test_expect_success '--no-verify with failing hook (merge)' '
+ test_when_finished "rm -f \"$PREMERGE\" actual_hooks" &&
+ cp "$HOOKDIR/fail.sample" "$PREMERGE" &&
+ git branch -f side side-orig &&
+ git checkout side &&
+ git merge --no-verify -m "merge master" master &&
+ git checkout master &&
+ test_path_is_missing actual_hooks
+'
+
test_expect_success POSIXPERM 'with non-executable hook' '
test_when_finished "rm -f \"$PRECOMMIT\" actual_hooks" &&
cp "$HOOKDIR/non-exec.sample" "$PRECOMMIT" &&
@@ -188,6 +217,16 @@ test_expect_success POSIXPERM 'with non-executable hook (merge)' '
test_path_is_missing actual_hooks
'
+test_expect_success POSIXPERM '--no-verify with non-executable hook (merge)' '
+ test_when_finished "rm -f \"$PREMERGE\" actual_hooks" &&
+ cp "$HOOKDIR/non-exec.sample" "$PREMERGE" &&
+ git branch -f side side-orig &&
+ git checkout side &&
+ git merge --no-verify -m "merge master" master &&
+ git checkout master &&
+ test_path_is_missing actual_hooks
+'
+
test_expect_success 'with hook requiring GIT_PREFIX' '
test_when_finished "rm -rf \"$PRECOMMIT\" expected_hooks actual_hooks success" &&
cp "$HOOKDIR/require-prefix.sample" "$PRECOMMIT" &&
--
2.22.0.770.g0f2c4a37fd-goog
^ permalink raw reply related [flat|nested] 59+ messages in thread* Re: [PATCH v5 0/4] pre-merge-commit hook
2019-08-07 18:57 ` [PATCH v5 0/4] " Josh Steadmon
` (3 preceding siblings ...)
2019-08-07 18:57 ` [PATCH v5 4/4] merge: --no-verify to bypass " Josh Steadmon
@ 2019-08-08 13:08 ` Martin Ågren
4 siblings, 0 replies; 59+ messages in thread
From: Martin Ågren @ 2019-08-08 13:08 UTC (permalink / raw)
To: Josh Steadmon; +Cc: Git Mailing List, Junio C Hamano, Michael J Gruber
On Wed, 7 Aug 2019 at 20:57, Josh Steadmon <steadmon@google.com> wrote:
>
> This series adds a new pre-merge-commit hook, similar in usage to
> pre-commit. It also improves hook testing in t7503, by verifying that
> the correct hooks are run or bypassed as expected.
>
> The original series was done by Michael J Gruber <git@grubix.eu>. I have
> addressed the outstanding review comments, and noted my changes in the
> commit messages in "[js: ...]" blocks.
>
> Changes since V4:
I don't have any more nits to pick ;-) so this looks good to me, FWIW.
Martin
^ permalink raw reply [flat|nested] 59+ messages in thread