| Message ID | 61b989ff16eadfd0508e10f71c9b318eb15ce2a7.1564695893.git.steadmon@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | pre-merge-commit hook | expand |
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
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.
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" +:
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