From: Michael J Gruber <hidden> Date: 2016-06-15 22:54:38
The pre-commit hook is often used to ensure certain properties of each
comitted tree like formatting or coding standards, validity (lint/make)
or code quality (make test). But merges introduce new commits unless
they are fast forwards, and therefore they can break these properties
because the pre-commit hook is not run by "git merge".
Introduce a pre-merge hook which works for (non ff, automatic) merges
like pre-commit does for commits. Typically this will just call the
pre-commit hook (like in the sample hook), but it does not need to.
Michael J Gruber (3):
git-merge: Honor pre-merge hook
merge: --no-verify to bypass pre-merge hook
t7503: add tests for pre-merge-hook
Documentation/git-merge.txt | 2 +-
Documentation/githooks.txt | 7 +++++
Documentation/merge-options.txt | 4 +++
builtin/merge.c | 15 ++++++++-
t/t7503-pre-commit-hook.sh | 66 ++++++++++++++++++++++++++++++++++++++-
templates/hooks--pre-merge.sample | 13 ++++++++
6 files changed, 104 insertions(+), 3 deletions(-)
create mode 100755 templates/hooks--pre-merge.sample
--
1.7.12.406.gafd3f81
From: Michael J Gruber <hidden> Date: 2016-06-15 22:54:38
Add tests which make sure that the pre-merge-hook is called when
present, allows/disallows merge commits depending on its return value
and is suppressed by "--no-verify".
Signed-off-by: Michael J Gruber <redacted>
---
t/t7503-pre-commit-hook.sh | 66 +++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 65 insertions(+), 1 deletion(-)
@@ -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&&+gitaddfile&&+gitcommit-m"zeroth"&&+gitcheckout-bside&&+echo"foo">foo&&+gitaddfoo&&+gitcommit-m"make it non-ff"&&+gitcheckoutmaster++'+ 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)''++gitcheckoutside&&+gitmerge-m"merge master"master&&+gitcheckoutmaster++'+ 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)''++gitcheckoutside&&+gitmerge--no-verify-m"merge master"master&&+gitcheckoutmaster++'+# now install hook that always succeedsHOOKDIR="$(gitrev-parse--git-dir)/hooks"HOOK="$HOOKDIR/pre-commit"+MERGEHOOK="$HOOKDIR/pre-merge" mkdir-p"$HOOKDIR" cat>"$HOOK"<<EOF#!/bin/shexit0 EOF chmod+x"$HOOK"+cp-p"$HOOK""$MERGEHOOK" test_expect_success'with succeeding hook''
@@ -46,11 +85,20 @@ test_expect_success '--no-verify with succeeding hook' ''+test_expect_success'--no-verify with succeeding hook (merge)''++gitcheckoutside&&+gitmerge--no-verify-m"merge master"master&&+gitcheckoutmaster++'+# now a hook that fails cat>"$HOOK"<<EOF#!/bin/shexit1 EOF+cp-p"$HOOK""$MERGEHOOK" test_expect_success'with failing hook''
From: Michael J Gruber <hidden> Date: 2016-06-15 22:54:38
git-merge does not honor the pre-commit hook when doing automatic merge
commits, and for compatibility reasons this is going to stay.
Introduce a pre-merge hook which is called for an automatic merge commit
just like pre-commit is called for a non-automatic merge commit (or any
other commit).
Signed-off-by: Michael J Gruber <redacted>
---
Documentation/githooks.txt | 7 +++++++
builtin/merge.c | 13 ++++++++++++-
templates/hooks--pre-merge.sample | 13 +++++++++++++
3 files changed, 32 insertions(+), 1 deletion(-)
create mode 100755 templates/hooks--pre-merge.sample
@@ -86,6 +86,13 @@ All the 'git commit' hooks are invoked with the environment variable `GIT_EDITOR=:` if the command will not bring up an editor to modify the commit message.+pre-merge+~~~~~~~~~++This hook is invoked by 'git merge' when doing an automatic merge+commit; it is equivalent to 'pre-commit' for a non-automatic commit+for a merge.+ prepare-commit-msg ~~~~~~~~~~~~~~~~~~
@@ -0,0 +1,13 @@+#!/bin/sh+#+# An example hook script to verify what is about to be committed.+# Called by "git merge" with no arguments. The hook should+# exit with non-zero status after issuing an appropriate message if+# it wants to stop the commit.+#+# To enable this hook, rename this file to "pre-merge".++. git-sh-setup+test -x "$GIT_DIR/hooks/pre-commit" &&+ exec "$GIT_DIR/hooks/pre-commit"+:
@@ -91,7 +91,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 ~~~~~~~~~~~~~~~~~~
@@ -70,6 +70,10 @@ merge. With --no-squash perform the merge and commit the result. This option can be used to override --squash.+--no-verify::+ This option bypasses the pre-merge and commit-msg hooks.+ See also linkgit:githooks[5].+ -s <strategy>:: --strategy=<strategy>:: Use the given merge strategy; can be supplied more than
From: Michael Haggerty <hidden> Date: 2016-06-15 22:54:38
On 09/05/2012 03:39 PM, Michael J Gruber wrote:
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).
What exactly is an "automatic merge commit"? Is it any merge that
doesn't have a conflict? A merge that doesn't invoke the editor? A
merge done as part of another operation (e.g., pull)? I don't see the
term mentioned in the git-merge or githooks man pages.
I think it would be good if you would define this term in the
documentation files that your patch touched, and perhaps in the githooks
section about "pre-commit" as well.
Secondly, though it is impossible (for backwards compatibility reasons)
for the pre-commit hook to be invoked for automatic merges, no such
considerations prohibit the pre-merge commit from being invoked for
non-automatic merges. In other words, both hooks, pre-commit *and*
pre-merge, could be invoked for non-automatic merges. Would this be
preferable?
It depends on what pre-merge scripts are likely to be used for. If they
will tend to be used for merge-specific actions, then it might be more
convenient for *all* merges to be vetted by them. On the other hand, if
they tend to do the same actions as pre-commit hooks, then having
non-automatic merge commits go through both hooks would tend to be more
annoying than helpful. Specifically, one of the scripts would probably
have to check whether the merge is a non-automatic merge, and if so do
nothing (i.e., letting the other script take care of it). This would
also require an easy way for a script to determine whether a commit is a
non-automatic merge commit.
Have you considered this?
Michael
--
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
From: Michael J Gruber <hidden> Date: 2016-06-15 22:54:38
Michael Haggerty venit, vidit, dixit 05.09.2012 17:30:
On 09/05/2012 03:39 PM, Michael J Gruber wrote:
quoted
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).
What exactly is an "automatic merge commit"? Is it any merge that
doesn't have a conflict? A merge that doesn't invoke the editor? A
merge done as part of another operation (e.g., pull)? I don't see the
term mentioned in the git-merge or githooks man pages.
I think it would be good if you would define this term in the
documentation files that your patch touched, and perhaps in the githooks
section about "pre-commit" as well.
"git merge" can go three ways:
F: fast forward: no commit is created, only a ref is changed
A: automatic: true merge (non-ff) without conflicts (i.e. chosen
strategy can perform the merge); a new commit is created
C: merge with conflicts: no commit is created but the index is prepared
(partially) for a merge commit
In case F, no commit hook is run (talking only about pre-commit/pre-merge).
In case A, no commit is run so far but my patch proposes pre-merge to be
run.
In case C, pre-commit (!) is run so far and after my patch.
Secondly, though it is impossible (for backwards compatibility reasons)
for the pre-commit hook to be invoked for automatic merges, no such
considerations prohibit the pre-merge commit from being invoked for
non-automatic merges. In other words, both hooks, pre-commit *and*
pre-merge, could be invoked for non-automatic merges. Would this be
preferable?
It depends on what pre-merge scripts are likely to be used for. If they
will tend to be used for merge-specific actions, then it might be more
convenient for *all* merges to be vetted by them. On the other hand, if
they tend to do the same actions as pre-commit hooks, then having
non-automatic merge commits go through both hooks would tend to be more
annoying than helpful. Specifically, one of the scripts would probably
have to check whether the merge is a non-automatic merge, and if so do
nothing (i.e., letting the other script take care of it). This would
also require an easy way for a script to determine whether a commit is a
non-automatic merge commit.
Have you considered this?
Your second paragraph explains why I did it the way I did. One can
easily have pre-merge call pre-commit, or have them be different. One
can not easily have only pre-merge called for a non-automatic merge
commit, but that is because of backward compatibility. The way *I* would
like it is:
- call pre-merge for any non-ff merge commit (automatic or not)
- call pre-commit for any non-merge commit (#parents <=1)
But that would break compatibility.
So I hope my patch is the best approximation to the above which keeps
compatibility and is simple to handle in most situations.
Cheers
Michael
From: Michael Haggerty <hidden> Date: 2016-06-15 22:54:38
On 09/06/2012 10:16 AM, Michael J Gruber wrote:
Michael Haggerty venit, vidit, dixit 05.09.2012 17:30:
quoted
On 09/05/2012 03:39 PM, Michael J Gruber wrote:
quoted
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).
What exactly is an "automatic merge commit"? Is it any merge that
doesn't have a conflict? A merge that doesn't invoke the editor? A
merge done as part of another operation (e.g., pull)? I don't see the
term mentioned in the git-merge or githooks man pages.
I think it would be good if you would define this term in the
documentation files that your patch touched, and perhaps in the githooks
section about "pre-commit" as well.
"git merge" can go three ways:
F: fast forward: no commit is created, only a ref is changed
A: automatic: true merge (non-ff) without conflicts (i.e. chosen
strategy can perform the merge); a new commit is created
C: merge with conflicts: no commit is created but the index is prepared
(partially) for a merge commit
In case F, no commit hook is run (talking only about pre-commit/pre-merge).
In case A, no commit is run so far but my patch proposes pre-merge to be
run.
In case C, pre-commit (!) is run so far and after my patch.
Thanks for the explanation. I hope you will explain this briefly in the
patch to the docs.
quoted
Secondly, though it is impossible (for backwards compatibility reasons)
for the pre-commit hook to be invoked for automatic merges, no such
considerations prohibit the pre-merge commit from being invoked for
non-automatic merges. In other words, both hooks, pre-commit *and*
pre-merge, could be invoked for non-automatic merges. Would this be
preferable?
It depends on what pre-merge scripts are likely to be used for. If they
will tend to be used for merge-specific actions, then it might be more
convenient for *all* merges to be vetted by them. On the other hand, if
they tend to do the same actions as pre-commit hooks, then having
non-automatic merge commits go through both hooks would tend to be more
annoying than helpful. Specifically, one of the scripts would probably
have to check whether the merge is a non-automatic merge, and if so do
nothing (i.e., letting the other script take care of it). This would
also require an easy way for a script to determine whether a commit is a
non-automatic merge commit.
Have you considered this?
Your second paragraph explains why I did it the way I did. One can
easily have pre-merge call pre-commit, or have them be different. One
can not easily have only pre-merge called for a non-automatic merge
commit, but that is because of backward compatibility. The way *I* would
like it is:
- call pre-merge for any non-ff merge commit (automatic or not)
- call pre-commit for any non-merge commit (#parents <=1)
But that would break compatibility.
So I hope my patch is the best approximation to the above which keeps
compatibility and is simple to handle in most situations.
I can understand your reasoning and won't object. But before I shut up,
I will point out a third alternative that is arguably closer to your
"ideal":
- For non-merge commits, call pre-commit
- For automatic merge commits, call pre-merge
- For non-automatic merge commits:
if pre-merge exists, call pre-merge (only)
else if pre-commit exists, call pre-commit (for backwards comptibility)
Michael
--
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
From: Michael J Gruber <hidden> Date: 2016-06-15 22:54:38
In this second iteration I implement Junio's suggestion: 'git merge' invokes
the pre-commit hook when merge.usePreCommitHook is set to true.
1/4 documents that 'git merge' invokes the prepare-commit-msg hook
unconditionally already.
2-4 are v2 of the previous 1-3.
This leaves aside the issue of commit-msg and post-commit hooks. If we can live
with a bit of incompatibility I would rename the config to merge.useCommitHooks
and call all of them based on that. This would change the way prepare-commit-msg is
treated now.
We could also introduce 4 config variables, 3 defaulting to false, 1 to true,
of course...
[I had messed up my alias file when adding mhagger, and it seems that tripped up
vger; resending, sorry.]
Michael J Gruber (4):
merge: document prepare-commit-msg hook usage
git-merge: Honor pre-commit hook based on config
merge: --no-verify to bypass pre-commit hook
t7503: add tests for pre-commit hook (merge)
Documentation/git-merge.txt | 7 ++++-
Documentation/githooks.txt | 6 ++++
Documentation/merge-config.txt | 5 ++++
Documentation/merge-options.txt | 4 +++
builtin/merge.c | 19 ++++++++++++-
t/t7503-pre-commit-hook.sh | 62 +++++++++++++++++++++++++++++++++++++++++
6 files changed, 101 insertions(+), 2 deletions(-)
--
1.7.12.406.gafd3f81
@@ -308,6 +308,11 @@ branch.<name>.mergeoptions:: supported options are the same as those of 'git merge', but option values containing whitespace characters are currently not supported.+HOOKS+-----+This command can run the `prepare-commit-msg` hook.+See linkgit:githooks[5] for more information.+ SEE ALSO -------- linkgit:git-fmt-merge-msg[1], linkgit:git-pull[1],
@@ -108,6 +108,8 @@ it is not suppressed by the `--no-verify` option. A non-zero exit means a failure of the hook and aborts the commit. It should not be used as replacement for pre-commit hook.+This hook is also called by 'git merge' when it creates a new commit.+ The sample `prepare-commit-msg` hook that comes with git comments out the `Conflicts:` part of a merge's commit message.
From: Michael J Gruber <hidden> Date: 2016-06-15 22:54:38
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 merge.usePreCommitHook which controls whether an automatic
merge commit invokes pre-commit.
Signed-off-by: Michael J Gruber <redacted>
---
Documentation/git-merge.txt | 2 +-
Documentation/githooks.txt | 3 +++
Documentation/merge-config.txt | 5 +++++
builtin/merge.c | 17 ++++++++++++++++-
4 files changed, 25 insertions(+), 2 deletions(-)
@@ -310,7 +310,7 @@ branch.<name>.mergeoptions:: HOOKS ------This command can run the `prepare-commit-msg` hook.+This command can run the `prepare-commit-msg` and `pre-commit` hooks. See linkgit:githooks[5] for more information. SEE ALSO
@@ -86,6 +86,9 @@ All the 'git commit' hooks are invoked with the environment variable `GIT_EDITOR=:` if the command will not bring up an editor to modify the commit message.+If the configuration option `merge.usePreCommitHook` is set to `true`+then 'git merge' invokes this hook whenever it creates a new commit.+ prepare-commit-msg ~~~~~~~~~~~~~~~~~~
@@ -59,6 +59,11 @@ merge.tool:: and "xxdiff". Any other value is treated is custom merge tool and there must be a corresponding mergetool.<tool>.cmd option.+merge.usePreCommitHook::+ Controls whether the pre-commit hook is invoked when 'git merge'+ creates a new commit (true merge without conflicts). False by+ default.+ merge.verbosity:: Controls the amount of output shown by the recursive merge strategy. Level 0 outputs nothing except a final error
@@ -88,6 +88,7 @@ to modify the commit message. If the configuration option `merge.usePreCommitHook` is set to `true` then 'git merge' invokes this hook whenever it creates a new commit.+It can be bypassed with the `\--no-verify` option. prepare-commit-msg ~~~~~~~~~~~~~~~~~~
@@ -70,6 +70,10 @@ merge. With --no-squash perform the merge and commit the result. This option can be used to override --squash.+--no-verify::+ This option bypasses the pre-commit hook.+ See also linkgit:githooks[5].+ -s <strategy>:: --strategy=<strategy>:: Use the given merge strategy; can be supplied more than
From: Michael J Gruber <hidden> Date: 2016-06-15 22:54:38
Add tests which make sure that the pre-commit hook is called by 'git
merge' when merge.usePreCommitHook is set, allows/disallows merge
commits depending on its return value and is suppressed by
"--no-verify".
Signed-off-by: Michael J Gruber <redacted>
---
t/t7503-pre-commit-hook.sh | 62 ++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 62 insertions(+)
@@ -4,6 +4,19 @@ test_description='pre-commit hook' ../test-lib.sh+test_expect_success'root commit''++echo"root">file&&+gitaddfile&&+gitcommit-m"zeroth"&&+gitcheckout-bside&&+echo"foo">foo&&+gitaddfoo&&+gitcommit-m"make it non-ff"&&+gitcheckoutmaster++'+ 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)''++gitcheckoutside&&+gitmerge-m"merge master"master&&+gitcheckoutmaster++'+ test_expect_success'--no-verify with no hook''echo"bar">file&&
@@ -20,6 +41,14 @@ test_expect_success '--no-verify with no hook' ''+test_expect_success'--no-verify with no hook (merge)''++gitcheckoutside&&+gitmerge--no-verify-m"merge master"master&&+gitcheckoutmaster++'+# now install hook that always succeedsHOOKDIR="$(gitrev-parse--git-dir)/hooks"HOOK="$HOOKDIR/pre-commit"
@@ -46,6 +84,14 @@ test_expect_success '--no-verify with succeeding hook' ''+test_expect_success'--no-verify with succeeding hook (merge)''++gitcheckoutside&&+gitmerge--no-verify-m"merge master"master&&+gitcheckoutmaster++'+# now a hook that fails cat>"$HOOK"<<EOF#!/bin/sh