Michael J Gruber | 5 Sep 15:39 2012
Picon

[PATCH 0/3] pre-merge-hook

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

Michael J Gruber | 5 Sep 15:39 2012
Picon

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

Add tests which make sure that the pre-merge-hook is called when
present, allows/disallows merge commits depending on its return value
and is suppressed by "--no-verify".

Signed-off-by: Michael J Gruber <git <at> drmicha.warpmail.net>
---
 t/t7503-pre-commit-hook.sh | 66 +++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 65 insertions(+), 1 deletion(-)

diff --git a/t/t7503-pre-commit-hook.sh b/t/t7503-pre-commit-hook.sh
index 984889b..36ae87f 100755
--- a/t/t7503-pre-commit-hook.sh
+++ b/t/t7503-pre-commit-hook.sh
 <at>  <at>  -1,9 +1,22  <at>  <at> 
 #!/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" &&
(Continue reading)

Michael J Gruber | 5 Sep 15:39 2012
Picon

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

git-merge does not honor the pre-commit hook when doing automatic merge
commits, and for compatibility reasons this is going to stay.

Introduce a pre-merge hook which is called for an automatic merge commit
just like pre-commit is called for a non-automatic merge commit (or any
other commit).

Signed-off-by: Michael J Gruber <git <at> drmicha.warpmail.net>
---
 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

diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index b9003fe..3fae643 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
 <at>  <at>  -86,6 +86,13  <at>  <at>  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.
+
(Continue reading)

Michael Haggerty | 5 Sep 17:30 2012
Picon

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

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

Michael J Gruber | 6 Sep 10:16 2012
Picon

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

Michael Haggerty venit, vidit, dixit 05.09.2012 17:30:
> 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.

"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.
(Continue reading)

Michael Haggerty | 6 Sep 12:48 2012
Picon

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

On 09/06/2012 10:16 AM, Michael J Gruber wrote:
> Michael Haggerty venit, vidit, dixit 05.09.2012 17:30:
>> 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.
> 
> "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
(Continue reading)

Michael J Gruber | 6 Sep 16:25 2012
Picon

[PATCHv2 0/4] pre-commit hook for merges

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

Michael J Gruber | 6 Sep 16:25 2012
Picon

[PATCHv2 1/4] merge: document prepare-commit-msg hook usage


Signed-off-by: Michael J Gruber <git <at> drmicha.warpmail.net>
---
 Documentation/git-merge.txt | 5 +++++
 Documentation/githooks.txt  | 2 ++
 2 files changed, 7 insertions(+)

diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
index 20f9228..b3ba8a8 100644
--- a/Documentation/git-merge.txt
+++ b/Documentation/git-merge.txt
 <at>  <at>  -308,6 +308,11  <at>  <at>  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],
diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index b9003fe..b32134c 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
 <at>  <at>  -108,6 +108,8  <at>  <at>  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.
(Continue reading)

Michael J Gruber | 6 Sep 16:25 2012
Picon

[PATCHv2 2/4] git-merge: Honor pre-commit hook based on config

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 <git <at> drmicha.warpmail.net>
---
 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(-)

diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
index b3ba8a8..3501ae2 100644
--- a/Documentation/git-merge.txt
+++ b/Documentation/git-merge.txt
 <at>  <at>  -310,7 +310,7  <at>  <at>  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
diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index b32134c..d62e02d 100644
--- a/Documentation/githooks.txt
(Continue reading)

Michael J Gruber | 6 Sep 16:25 2012
Picon

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

Analogous to commit, introduce a '--no-verify' option which bypasses the
pre-commit hook. The shorthand '-n' is taken by the (non-existing)
'--no-stat' already.

Signed-off-by: Michael J Gruber <git <at> drmicha.warpmail.net>
---
 Documentation/git-merge.txt     | 2 +-
 Documentation/githooks.txt      | 1 +
 Documentation/merge-options.txt | 4 ++++
 builtin/merge.c                 | 4 +++-
 4 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
index 3501ae2..363fbea 100644
--- a/Documentation/git-merge.txt
+++ b/Documentation/git-merge.txt
 <at>  <at>  -10,7 +10,7  <at>  <at>  SYNOPSIS
 --------
 [verse]
 'git merge' [-n] [--stat] [--no-commit] [--squash] [--[no-]edit]
-	[-s <strategy>] [-X <strategy-option>]
+	[--no-verify] [-s <strategy>] [-X <strategy-option>]
 	[--[no-]rerere-autoupdate] [-m <msg>] [<commit>...]
 'git merge' <msg> HEAD <commit>...
 'git merge' --abort
diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index d62e02d..c734e2c 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
 <at>  <at>  -88,6 +88,7  <at>  <at>  to modify the commit message.
(Continue reading)

Michael J Gruber | 6 Sep 16:25 2012
Picon

[PATCHv2 4/4] t7503: add tests for pre-commit hook (merge)

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 <git <at> drmicha.warpmail.net>
---
 t/t7503-pre-commit-hook.sh | 62 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 62 insertions(+)

diff --git a/t/t7503-pre-commit-hook.sh b/t/t7503-pre-commit-hook.sh
index 984889b..e4d324d 100755
--- a/t/t7503-pre-commit-hook.sh
+++ b/t/t7503-pre-commit-hook.sh
 <at>  <at>  -4,6 +4,19  <at>  <at>  test_description='pre-commit hook'

 . ./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
+
+'
(Continue reading)

Michael J Gruber | 5 Sep 15:39 2012
Picon

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

Analogous to commit, introduce a '--no-verify' option which bypasses the
pre-merge hook. The shorthand '-n' is taken by the (non-existing)
'--no-stat' already.

Signed-off-by: Michael J Gruber <git <at> drmicha.warpmail.net>
---
 Documentation/git-merge.txt     | 2 +-
 Documentation/githooks.txt      | 2 +-
 Documentation/merge-options.txt | 4 ++++
 builtin/merge.c                 | 4 +++-
 4 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
index 20f9228..8816865 100644
--- a/Documentation/git-merge.txt
+++ b/Documentation/git-merge.txt
 <at>  <at>  -10,7 +10,7  <at>  <at>  SYNOPSIS
 --------
 [verse]
 'git merge' [-n] [--stat] [--no-commit] [--squash] [--[no-]edit]
-	[-s <strategy>] [-X <strategy-option>]
+	[--no-verify] [-s <strategy>] [-X <strategy-option>]
 	[--[no-]rerere-autoupdate] [-m <msg>] [<commit>...]
 'git merge' <msg> HEAD <commit>...
 'git merge' --abort
diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index 3fae643..e20bfe0 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
 <at>  <at>  -91,7 +91,7  <at>  <at>  pre-merge
(Continue reading)

Junio C Hamano | 6 Sep 07:07 2012
Picon
Picon

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

Michael J Gruber <git <at> drmicha.warpmail.net> writes:

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

When your merge asks for a help from you to resolve conflict, you
conclude with "git commit", and at that point, pre-commit hook will
have a chance to reject it, presumably.  That means for any project
that wants to audit a merge via hook, their pre-commit hook MUST be
prepared to look at and judge a merge.  Given that, is a separate
hook that "can just call the pre-commit but does not need to" really
needed and useful?

I admit that I haven't thought things through, but adding a boolean
"merge.usePreCommitHook" smells like a more appropriate approach to
me.

I dunno.
Michael J Gruber | 6 Sep 10:16 2012
Picon

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

Junio C Hamano venit, vidit, dixit 06.09.2012 07:07:
> Michael J Gruber <git <at> drmicha.warpmail.net> writes:
> 
>> 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.
> 
> When your merge asks for a help from you to resolve conflict, you
> conclude with "git commit", and at that point, pre-commit hook will
> have a chance to reject it, presumably.  That means for any project
> that wants to audit a merge via hook, their pre-commit hook MUST be
> prepared to look at and judge a merge.  Given that, is a separate
> hook that "can just call the pre-commit but does not need to" really
> needed and useful?
> 
> I admit that I haven't thought things through, but adding a boolean
> "merge.usePreCommitHook" smells like a more appropriate approach to
> me.
> 
> I dunno.

That would be an option ;)

Seriously, that would make the case where both hooks are the same even
(Continue reading)

Junio C Hamano | 6 Sep 20:34 2012
Picon
Picon

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

Michael J Gruber <git <at> drmicha.warpmail.net> writes:

> Junio C Hamano venit, vidit, dixit 06.09.2012 07:07:
>> Michael J Gruber <git <at> drmicha.warpmail.net> writes:
>> 
>>> 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.
>> 
>> When your merge asks for a help from you to resolve conflict, you
>> conclude with "git commit", and at that point, pre-commit hook will
>> have a chance to reject it, presumably.  That means for any project
>> that wants to audit a merge via hook, their pre-commit hook MUST be
>> prepared to look at and judge a merge.  Given that, is a separate
>> hook that "can just call the pre-commit but does not need to" really
>> needed and useful?
>> 
>> I admit that I haven't thought things through, but adding a boolean
>> "merge.usePreCommitHook" smells like a more appropriate approach to
>> me.
>> 
>> I dunno.
>
> That would be an option ;)
(Continue reading)

Michael J Gruber | 7 Sep 12:00 2012
Picon

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

Junio C Hamano venit, vidit, dixit 06.09.2012 20:34:
> Michael J Gruber <git <at> drmicha.warpmail.net> writes:
> 
>> Junio C Hamano venit, vidit, dixit 06.09.2012 07:07:
>>> Michael J Gruber <git <at> drmicha.warpmail.net> writes:
>>> 
>>>> 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.
>>> 
>>> When your merge asks for a help from you to resolve conflict,
>>> you conclude with "git commit", and at that point, pre-commit
>>> hook will have a chance to reject it, presumably.  That means for
>>> any project that wants to audit a merge via hook, their
>>> pre-commit hook MUST be prepared to look at and judge a merge.
>>> Given that, is a separate hook that "can just call the pre-commit
>>> but does not need to" really needed and useful?
>>> 
>>> I admit that I haven't thought things through, but adding a
>>> boolean "merge.usePreCommitHook" smells like a more appropriate
>>> approach to me.
>>> 
(Continue reading)


Gmane