Re: [PATCHv3] builtin/merge: honor commit-msg hook for merges

[Junio C Hamano]

Stefan Beller  writes:

> I'll send a patch fixing the docs, though with this thought, maybe we need
> to fix other commands, that produce commits as well?
> (git revert, others?)

I do not think "commands that create commits" is not a good
criteria.  "git notes" and "git stash" would (internally) create a
commit while recording a new change, but you obvously would not want
these hooks to kick in.

A command that can stop in the middle and to which running "git
commit" is how the end-user concludes the operation would be a
candidate.  "git merge X", "git cherry-pick A", and "git cherry-pick
A..B" may be good candidates.

For "rebase" and others that have the "convenience --continue"
option that make a commit before continuing, I would think that we
should treat these as invoking "git commit".  That is, when these
commands stop and you resolve the conflict in the index and in the
working tree, the next "git $cmd --continue" you type is merely a
way to let you be lazy.  You'd be typing "git commit && git $cmd
--continue" if you were to refuse the lazy convenience option and
want to spell out what you are doing explicitly, and you'd get the
same result as you'd get from just "git $cmd --continue" if you did
so.

On the other hand, "git am" is not a candidate.  You never use "git
commit" to mark that you are done, even if you refuse to use the
lazy convenience option and spell out what you are doing explicitly.

Re: [PATCHv3] builtin/merge: honor commit-msg hook for merges

[Stefan Beller]

On Fri, Sep 15, 2017 at 11:22 PM, Kaartic Sivaraam
 wrote:
> Seems 'Documentation/githooks.txt' needs an update related to this
> change. Previously it said(note the **s) that 'commit-msg' is invoked
> only by 'git commit',
>
> commit-msg
>This hook is invoked by git commit**, and can be bypassed with the
>--no-verify option. It takes a single parameter, the name of the 
> file
>that holds the proposed commit log message. Exiting with a non-zero
>status causes the git commit** to abort.

Yes that needs an update. When writing the patch, it read ambivalently[1], such
that I decided to not include the update to the man page.

[1] at the time I was reading it as "when producing a [git] commit", instead
of "the command `git commit`".

I'll send a patch fixing the docs, though with this thought, maybe we need
to fix other commands, that produce commits as well?
(git revert, others?)


>
> ---
> Kaartic

Re: [PATCHv3] builtin/merge: honor commit-msg hook for merges

[Kaartic Sivaraam]

Seems 'Documentation/githooks.txt' needs an update related to this
change. Previously it said(note the **s) that 'commit-msg' is invoked
only by 'git commit',

commit-msg
   This hook is invoked by git commit**, and can be bypassed with the
   --no-verify option. It takes a single parameter, the name of the file
   that holds the proposed commit log message. Exiting with a non-zero
   status causes the git commit** to abort.

---
Kaartic

Re: [PATCHv3] builtin/merge: honor commit-msg hook for merges

[Stefan Beller]

On Thu, Sep 7, 2017 at 6:13 PM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>>  The --no-verify option however is not remembered across invocations
>> of git-merge. Originally the author assumed an alternative in which the
>> 'git merge --continue' command accepts the --no-verify flag, but that
>> opens up the discussion which flags are allows to the continued merge
>> command and which must be given in the first invocation.
>
> This leaves a reader (me) wondering what the final conclusion was,
> after the author assumed something and thought about alternatives.

I did not draw a conclusion, I was just saying, that this has to be
thought about.
It looks to me as if --continue currently wants to take no extra arguments,
but to remember all flags from the previous invocation of merge.

But that only looks this way as all the command line flags are related
for tree manipulation, not (indirect) commit message manipulation.

So I think having --no-verify be respected (either by restating, or by
remembering, or both) by the `git merge --continue` call would be a
reasonable thing to want.

So that new patch just added the test as a #needswork for the future,
not actually making a decision how to approach it.

> I am guessing that your final decision was not to remember
> "--no-verify" so a user who started "merge --no-verify" that stopped
> in the middle must say "merge --continue --no-verify" or "commit
> --no-verify" to conclude the merge?  Or you added some mechanism to
> remember the fact that no-verify was given so that "merge --continue"
> will read from there, ignoring "merge --continue --verify" from the
> command line?  Not just the above part of the log message confusing,
> but there is no update to the documentation, and we shouldn't expect
> end-users to find out what ought to happen by reading t7504 X-<.

Interestingly the documentation that I read to approach the problem
is already in shape as it would not specify the specific command for the
'--no-verify' option. I missed that we need to add documentation for
the continued merge case.

> The new test in t7504 tells me that you remember --[no-]verify from
> the initial invocation and use the same when --continue is given; it
> is unclear how that remembered one interacts with --[no-]verify that
> is given when --continue is given.  It is not documented, tested and
> explained in the log message.  I would expect that the command line
> trumps what was given in the initial invocation.

I would expect that, too.

>
>
>> +static int verify_msg = 1;
>>
>>  static struct strategy all_strategy[] = {
>>   { "recursive",  DEFAULT_TWOHEAD | NO_TRIVIAL },
>> @@ -236,6 +237,7 @@ static struct option builtin_merge_options[] = {
>> N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" },
>>   OPT_BOOL(0, "overwrite-ignore", _ignore, N_("update ignored 
>> files (default)")),
>>   OPT_BOOL(0, "signoff", , N_("add Signed-off-by:")),
>> + OPT_BOOL(0, "verify", _msg, N_("verify commit-msg hook")),
>>   OPT_END()
>>  };
>
> I suspect that the previous iteration gives a much better end-user
> experience when "git merge -h" is used.  This will give the
> impression that the user MUST say "merge --verify" if the user wants
> to verify commit-msg hook (whatever that means), but because the
> option defaults to true, that is not what happens.  The user instead
> must say "merge --no-verify" if the verification is unwanted.

ok, will revert to that in a resend.

>> +test_expect_failure 'merge --continue remembers --no-verify' '
>> + test_when_finished "git branch -D newbranch" &&
>> + test_when_finished "git checkout -f master" &&
>> + git checkout master &&
>> + echo a >file2 &&
>> + git add file2 &&
>> + git commit --no-verify -m "add file2 to master" &&
>> + git checkout -b newbranch master^ &&
>> + echo b >file2 &&
>> + git add file2 &&
>> + git commit --no-verify file2 -m in-side-branch &&
>> + git merge --no-verify -m not-rewritten-by-hook master &&
>> + # resolve conflict:
>> + echo c >file2 &&
>> + git add file2 &&
>> + git merge --continue &&
>> + commit_msg_is not-rewritten-by-hook
>>  '
>
> OK.  What should happen when the last "merge --continue" was given
> "--verify" at the same time?

Currently not possible, due to --continue requiring that it is the
only argument. In the future where --continue works well with other
arguments, we should override the original.

> A similar test whose title is
> "--no-verify remembered by merge --continue can be overriden" may be
> a good thing to follow this one, perhaps?

Note that this is already test_expect_failure, which I used to mark
that this particular flag is broken across a --continue invocation of git-merge,
so I would not add yet another test that describes the future yet to be
implemented?

Thanks.

Re: [PATCHv3] builtin/merge: honor commit-msg hook for merges

[Junio C Hamano]

Stefan Beller  writes:

>  The --no-verify option however is not remembered across invocations
> of git-merge. Originally the author assumed an alternative in which the
> 'git merge --continue' command accepts the --no-verify flag, but that
> opens up the discussion which flags are allows to the continued merge
> command and which must be given in the first invocation.

This leaves a reader (me) wondering what the final conclusion was,
after the author assumed something and thought about alternatives.
I am guessing that your final decision was not to remember
"--no-verify" so a user who started "merge --no-verify" that stopped
in the middle must say "merge --continue --no-verify" or "commit
--no-verify" to conclude the merge?  Or you added some mechanism to
remember the fact that no-verify was given so that "merge --continue"
will read from there, ignoring "merge --continue --verify" from the
command line?  Not just the above part of the log message confusing,
but there is no update to the documentation, and we shouldn't expect
end-users to find out what ought to happen by reading t7504 X-<.

The new test in t7504 tells me that you remember --[no-]verify from
the initial invocation and use the same when --continue is given; it
is unclear how that remembered one interacts with --[no-]verify that
is given when --continue is given.  It is not documented, tested and
explained in the log message.  I would expect that the command line 
trumps what was given in the initial invocation.


> +static int verify_msg = 1;
>  
>  static struct strategy all_strategy[] = {
>   { "recursive",  DEFAULT_TWOHEAD | NO_TRIVIAL },
> @@ -236,6 +237,7 @@ static struct option builtin_merge_options[] = {
> N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" },
>   OPT_BOOL(0, "overwrite-ignore", _ignore, N_("update ignored 
> files (default)")),
>   OPT_BOOL(0, "signoff", , N_("add Signed-off-by:")),
> + OPT_BOOL(0, "verify", _msg, N_("verify commit-msg hook")),
>   OPT_END()
>  };

I suspect that the previous iteration gives a much better end-user
experience when "git merge -h" is used.  This will give the
impression that the user MUST say "merge --verify" if the user wants
to verify commit-msg hook (whatever that means), but because the
option defaults to true, that is not what happens.  The user instead
must say "merge --no-verify" if the verification is unwanted.

"git commit -h" explains 

--no-verifybypass pre-commit and commit-msg hooks

and I think that is the way how we want to explain this option in
"git merge" too.  Normally it is not bypassed, and the user can ask
with "--no-verify".  Thanks to René's change in 2012, the option
definition you had in the previous one will make --[no-]verify
accepted just fine.

> +test_expect_success 'merge fails with failing hook' '
> + ...
> +'
> +
> +test_expect_success 'merge bypasses failing hook with --no-verify' '
> + ...
> +'

Both look sensible.

> +test_expect_failure 'merge --continue remembers --no-verify' '
> + test_when_finished "git branch -D newbranch" &&
> + test_when_finished "git checkout -f master" &&
> + git checkout master &&
> + echo a >file2 &&
> + git add file2 &&
> + git commit --no-verify -m "add file2 to master" &&
> + git checkout -b newbranch master^ &&
> + echo b >file2 &&
> + git add file2 &&
> + git commit --no-verify file2 -m in-side-branch &&
> + git merge --no-verify -m not-rewritten-by-hook master &&
> + # resolve conflict:
> + echo c >file2 &&
> + git add file2 &&
> + git merge --continue &&
> + commit_msg_is not-rewritten-by-hook
>  '

OK.  What should happen when the last "merge --continue" was given
"--verify" at the same time?  A similar test whose title is
"--no-verify remembered by merge --continue can be overriden" may be
a good thing to follow this one, perhaps?

Thanks.

[PATCHv3] builtin/merge: honor commit-msg hook for merges

[Stefan Beller]

Similar to 65969d43d1 (merge: honor prepare-commit-msg hook, 2011-02-14)
merge should also honor the commit-msg hook: When a merge is stopped due
to conflicts or --no-commit, the subsequent commit calls the commit-msg
hook.  However, it is not called after a clean merge. Fix this
inconsistency by invoking the hook after clean merges as well.

This change is motivated by Gerrit's commit-msg hook to install a ChangeId
trailer into the commit message. Without such a ChangeId, Gerrit refuses
to accept any commit by default, such that the inconsistency of (not)
running the commit-msg hook between commit and merge leads to confusion
and might block people from getting their work done.

As the githooks man page is very vocal about the possibility of skipping
the commit-msg hook via the --no-verify option, implement the option
in merge, too.

'git merge --continue' is currently implemented as calling cmd_commit
with no further arguments. This works for most other merge related options,
such as demonstrated via the --allow-unrelated-histories flag in the
test. The --no-verify option however is not remembered across invocations
of git-merge. Originally the author assumed an alternative in which the
'git merge --continue' command accepts the --no-verify flag, but that
opens up the discussion which flags are allows to the continued merge
command and which must be given in the first invocation.

Signed-off-by: Stefan Beller 
---

> I didn't check how "merge --continue" is implemented, but we need to
> behave exactly the same way over there, too.  Making sure that it is
> a case in t7504 may be a good idea, in addition to the test you
> added.

First I understood this as if we'd want to support
'git merge --continue --no-verify' eventually, but by now I think we want
to 'carry over' the meaning from the first invocation of git-merge.

For that I added a test.

Thanks,
Stefan

 builtin/merge.c|  8 ++
 t/t7504-commit-msg-hook.sh | 64 +++---
 2 files changed, 68 insertions(+), 4 deletions(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index 7df3fe3927..780435d7a1 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -73,6 +73,7 @@ static int show_progress = -1;
 static int default_to_upstream = 1;
 static int signoff;
 static const char *sign_commit;
+static int verify_msg = 1;
 
 static struct strategy all_strategy[] = {
{ "recursive",  DEFAULT_TWOHEAD | NO_TRIVIAL },
@@ -236,6 +237,7 @@ static struct option builtin_merge_options[] = {
  N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" },
OPT_BOOL(0, "overwrite-ignore", _ignore, N_("update ignored 
files (default)")),
OPT_BOOL(0, "signoff", , N_("add Signed-off-by:")),
+   OPT_BOOL(0, "verify", _msg, N_("verify commit-msg hook")),
OPT_END()
 };
 
@@ -780,6 +782,12 @@ static void prepare_to_commit(struct commit_list 
*remoteheads)
if (launch_editor(git_path_merge_msg(), NULL, NULL))
abort_commit(remoteheads, NULL);
}
+
+   if (verify_msg && run_commit_hook(0 < option_edit, get_index_file(),
+ "commit-msg",
+ git_path_merge_msg(), NULL))
+   abort_commit(remoteheads, NULL);
+
read_merge_msg();
strbuf_stripspace(, 0 < option_edit);
if (!msg.len)
diff --git a/t/t7504-commit-msg-hook.sh b/t/t7504-commit-msg-hook.sh
index 88d4cda299..302a3a2082 100755
--- a/t/t7504-commit-msg-hook.sh
+++ b/t/t7504-commit-msg-hook.sh
@@ -101,6 +101,10 @@ cat > "$HOOK" <> file &&
@@ -135,6 +139,32 @@ test_expect_success '--no-verify with failing hook 
(editor)' '
 
 '
 
+test_expect_success 'merge fails with failing hook' '
+
+   test_when_finished "git branch -D newbranch" &&
+   test_when_finished "git checkout -f master" &&
+   git checkout --orphan newbranch &&
+   : >file2 &&
+   git add file2 &&
+   git commit --no-verify file2 -m in-side-branch &&
+   test_must_fail git merge --allow-unrelated-histories master &&
+   commit_msg_is "in-side-branch" # HEAD before merge
+
+'
+
+test_expect_success 'merge bypasses failing hook with --no-verify' '
+
+   test_when_finished "git branch -D newbranch" &&
+   test_when_finished "git checkout -f master" &&
+   git checkout --orphan newbranch &&
+   : >file2 &&
+   git add file2 &&
+   git commit --no-verify file2 -m in-side-branch &&
+   git merge --no-verify --allow-unrelated-histories master &&
+   commit_msg_is "Merge branch '\''master'\'' into newbranch"
+'
+
+
 chmod -x "$HOOK"
 test_expect_success POSIXPERM 'with non-executable hook' '
 
@@ -178,10 +208,6 @@ exit 0
 EOF
 chmod +x "$HOOK"
 
-commit_msg_is () {
-   test "$(git log --pretty=format:%s%b -1)" = "$1"
-}
-
 test_expect_success 'hook edits commit message' '
 
echo "additional" >> file &&
@@ -217,7 +243,36 @@ 

Re: [PATCHv2] builtin/merge: honor commit-msg hook for merges

[Junio C Hamano]

Stefan Beller  writes:

>> I also thought that we were hunting calls of cmd_foo() from outside
>> the git.c command dispatcher as grave errors and want to clean up
>> the codebase to get rid of them.
>
> ... but I did not account for this fact. (I was not aware of these being
> called grave errors, but assumed this is a good state. And why change
> a good state?)

https://public-inbox.org/git/20170830053108.g2xsn43rwulnw...@sigill.intra.peff.net/

gives a good explanation why it is not a good state.

Re: [PATCHv2] builtin/merge: honor commit-msg hook for merges

[Stefan Beller]

On Tue, Sep 5, 2017 at 6:57 PM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> Junio writes:
>>> I didn't check how "merge --continue" is implemented, but we need to
>>> behave exactly the same way over there, too.  Making sure that it is
>>> a case in t7504 may be a good idea, in addition to the test you
>>> added.
>>
>> After inspection of the code I do not think it is a good idea, because
>> (a) it clutters the test suite with something "obvious" for now,
>> the call to cmd_commit will be the same as git-commit on the
>> command line and
>> (b) piping through --[no-]verify would either introduce irregularities
>> ("Why do we pipe through --no-verify, when --sign-off is more 
>> important?")
>> or miss important options to pipe through:
>>
>>   static int continue_current_merge;
>> ...
>>   OPT_BOOL(0, "continue", _current_merge,
>>   N_("continue the current in-progress merge")),
>> ...
>>   if (continue_current_merge) {
>>   int nargc = 1;
>>   const char *nargv[] = {"commit", NULL};
>>
>>   if (orig_argc != 2)
>>   usage_msg_opt(_("--continue expects no arguments"),
>> builtin_merge_usage, builtin_merge_options);
>>
>>   /* Invoke 'git commit' */
>>   ret = cmd_commit(nargc, nargv, prefix);
>>   goto done;
>>   }
>
> That line of thought is backwards.  'something "obvious" for now'
> talks about the present.  tests are all about future-proofing.

I agree, but I did not think a call to cmd_commit would need to
be future-proofed as we already test git-commit, and these
are equal

>
> I also thought that we were hunting calls of cmd_foo() from outside
> the git.c command dispatcher as grave errors and want to clean up
> the codebase to get rid of them.

... but I did not account for this fact. (I was not aware of these being
called grave errors, but assumed this is a good state. And why change
a good state?)

> So the above is the worst example
> to use when you are trying to convince why it needs no test---the
> above is a good example of the code that would need to change soon
> when we have enough volunteers willing to keep the codebase clean
> and healthy, and we would benefit from future-proofing tests.

Given that new fact, I agree with the reasoning to add a new test
for future proofing. In the current form

git merge --continue --no-verify

would trigger to usage_msg_opt(..), so all I'd offer is a test_must_fail
for now?

Re: [PATCHv2] builtin/merge: honor commit-msg hook for merges

[Junio C Hamano]

Stefan Beller  writes:

> Junio writes:
>> I didn't check how "merge --continue" is implemented, but we need to
>> behave exactly the same way over there, too.  Making sure that it is
>> a case in t7504 may be a good idea, in addition to the test you
>> added.
>
> After inspection of the code I do not think it is a good idea, because
> (a) it clutters the test suite with something "obvious" for now,
> the call to cmd_commit will be the same as git-commit on the
> command line and
> (b) piping through --[no-]verify would either introduce irregularities
> ("Why do we pipe through --no-verify, when --sign-off is more important?")
> or miss important options to pipe through: 
>
>   static int continue_current_merge;
> ...
>   OPT_BOOL(0, "continue", _current_merge,
>   N_("continue the current in-progress merge")),
> ...
>   if (continue_current_merge) {
>   int nargc = 1;
>   const char *nargv[] = {"commit", NULL};
>
>   if (orig_argc != 2)
>   usage_msg_opt(_("--continue expects no arguments"),
> builtin_merge_usage, builtin_merge_options);
>
>   /* Invoke 'git commit' */
>   ret = cmd_commit(nargc, nargv, prefix);
>   goto done;
>   }

That line of thought is backwards.  'something "obvious" for now'
talks about the present.  tests are all about future-proofing.

I also thought that we were hunting calls of cmd_foo() from outside
the git.c command dispatcher as grave errors and want to clean up
the codebase to get rid of them.  So the above is the worst example
to use when you are trying to convince why it needs no test---the
above is a good example of the code that would need to change soon
when we have enough volunteers willing to keep the codebase clean
and healthy, and we would benefit from future-proofing tests.

[PATCHv2] builtin/merge: honor commit-msg hook for merges

[Stefan Beller]

Similar to 65969d43d1 (merge: honor prepare-commit-msg hook, 2011-02-14)
merge should also honor the commit-msg hook: When a merge is stopped due
to conflicts or --no-commit, the subsequent commit calls the commit-msg
hook.  However, it is not called after a clean merge. Fix this
inconsistency by invoking the hook after clean merges as well.

This change is motivated by Gerrit's commit-msg hook to install a ChangeId
trailer into the commit message. Without such a ChangeId, Gerrit refuses
to accept any commit by default, such that the inconsistency of (not)
running the commit-msg hook between commit and merge leads to confusion
and might block people from getting their work done.

As the githooks man page is very vocal about the possibility of skipping
the commit-msg hook via the --no-verify option, implement the option
in merge, too.

Signed-off-by: Stefan Beller 
---

addressed all but one issues.

Junio writes:
> I didn't check how "merge --continue" is implemented, but we need to
> behave exactly the same way over there, too.  Making sure that it is
> a case in t7504 may be a good idea, in addition to the test you
> added.

After inspection of the code I do not think it is a good idea, because
(a) it clutters the test suite with something "obvious" for now,
the call to cmd_commit will be the same as git-commit on the
command line and
(b) piping through --[no-]verify would either introduce irregularities
("Why do we pipe through --no-verify, when --sign-off is more important?")
or miss important options to pipe through: 

static int continue_current_merge;
...
OPT_BOOL(0, "continue", _current_merge,
N_("continue the current in-progress merge")),
...
if (continue_current_merge) {
int nargc = 1;
const char *nargv[] = {"commit", NULL};

if (orig_argc != 2)
usage_msg_opt(_("--continue expects no arguments"),
  builtin_merge_usage, builtin_merge_options);

/* Invoke 'git commit' */
ret = cmd_commit(nargc, nargv, prefix);
goto done;
}

Thanks,
Stefan

 builtin/merge.c|  8 
 t/t7504-commit-msg-hook.sh | 45 +
 2 files changed, 49 insertions(+), 4 deletions(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index 7df3fe3927..780435d7a1 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -73,6 +73,7 @@ static int show_progress = -1;
 static int default_to_upstream = 1;
 static int signoff;
 static const char *sign_commit;
+static int verify_msg = 1;
 
 static struct strategy all_strategy[] = {
{ "recursive",  DEFAULT_TWOHEAD | NO_TRIVIAL },
@@ -236,6 +237,7 @@ static struct option builtin_merge_options[] = {
  N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" },
OPT_BOOL(0, "overwrite-ignore", _ignore, N_("update ignored 
files (default)")),
OPT_BOOL(0, "signoff", , N_("add Signed-off-by:")),
+   OPT_BOOL(0, "verify", _msg, N_("verify commit-msg hook")),
OPT_END()
 };
 
@@ -780,6 +782,12 @@ static void prepare_to_commit(struct commit_list 
*remoteheads)
if (launch_editor(git_path_merge_msg(), NULL, NULL))
abort_commit(remoteheads, NULL);
}
+
+   if (verify_msg && run_commit_hook(0 < option_edit, get_index_file(),
+ "commit-msg",
+ git_path_merge_msg(), NULL))
+   abort_commit(remoteheads, NULL);
+
read_merge_msg();
strbuf_stripspace(, 0 < option_edit);
if (!msg.len)
diff --git a/t/t7504-commit-msg-hook.sh b/t/t7504-commit-msg-hook.sh
index 88d4cda299..1cd54af3cc 100755
--- a/t/t7504-commit-msg-hook.sh
+++ b/t/t7504-commit-msg-hook.sh
@@ -101,6 +101,10 @@ cat > "$HOOK" <> file &&
@@ -135,6 +139,32 @@ test_expect_success '--no-verify with failing hook 
(editor)' '
 
 '
 
+test_expect_success 'merge fails with failing hook' '
+
+   test_when_finished "git branch -D newbranch" &&
+   test_when_finished "git checkout -f master" &&
+   git checkout --orphan newbranch &&
+   : >file2 &&
+   git add file2 &&
+   git commit --no-verify file2 -m in-side-branch &&
+   test_must_fail git merge --allow-unrelated-histories master &&
+   commit_msg_is "in-side-branch" # HEAD before merge
+
+'
+
+test_expect_success 'merge bypasses failing hook with --no-verify' '
+
+   test_when_finished "git branch -D newbranch" &&
+   test_when_finished "git checkout -f master" &&
+   git checkout --orphan newbranch &&
+   : >file2 &&
+   git add file2 &&
+   git commit --no-verify file2 -m in-side-branch &&
+   git merge --no-verify --allow-unrelated-histories master &&
+   commit_msg_is "Merge branch '\''master'\'' into newbranch"
+'
+
+
 chmod -x "$HOOK"
 

Re: [PATCH] builtin/merge: honor commit-msg hook for merges

[Junio C Hamano]

Stefan Beller  writes:

> Similar to 65969d43d1 (merge: honor prepare-commit-msg hook, 2011-02-14)
> merge should also honor the commit-msg hook; the reason is the same as
> in that commit: When a merge is stopped due to conflicts or --no-commit,
> the subsequent commit calls the commit-msg hook.  However, it is not
> called after a clean merge. Fix this inconsistency by invoking the hook
> after clean merges as well.

The above reads better without "; the reason is the same as in that
commit"---"Similar to", combined with the clean and concise
explanation after the colon you have, sufficiently justifies why
this is a good change.  

Excellent job spotting the precedent and making it consistent ;-).

> This change is motivated by Gerrits commit-msg hook to install a change-id

s/Gerrits/Gerrit's/ perhaps?

> trailer into the commit message. Without such a change id, Gerrit refuses

I do not live in Gerrit land and I do not know which one is the more
preferred one, but be consistent between "change-id" and "change
id".

> to accept (merge) commits by default, such that the inconsistency of
> (not) running commit-msg hook between commit and merge leads to confusion
> and might block people from getting their work done.

Yup.  Nicely explained.

I didn't check how "merge --continue" is implemented, but we need to
behave exactly the same way over there, too.  Making sure that it is
a case in t7504 may be a good idea, in addition to the test you
added.

> Signed-off-by: Stefan Beller 
> ---
>  builtin/merge.c|  8 
>  t/t7504-commit-msg-hook.sh | 45 +
>  2 files changed, 49 insertions(+), 4 deletions(-)
>
> diff --git a/builtin/merge.c b/builtin/merge.c
> index 7df3fe3927..087efd560d 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -73,6 +73,7 @@ static int show_progress = -1;
>  static int default_to_upstream = 1;
>  static int signoff;
>  static const char *sign_commit;
> +static int no_verify;
>  
>  static struct strategy all_strategy[] = {
>   { "recursive",  DEFAULT_TWOHEAD | NO_TRIVIAL },
> @@ -236,6 +237,7 @@ static struct option builtin_merge_options[] = {
> N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" },
>   OPT_BOOL(0, "overwrite-ignore", _ignore, N_("update ignored 
> files (default)")),
>   OPT_BOOL(0, "signoff", , N_("add Signed-off-by:")),
> + OPT_BOOL(0, "no-verify", _verify, N_("bypass pre-commit and 
> commit-msg hooks")),

This allows "--no-no-verify", which may want to be eventually
addressed (either by changing the code not to accept, or declaring
that it is an intended behaviour); I do not offhand know for sure but I
strong suspect "commit" shares the same issue, in which case this
patch is perfectly fine and addressing "--no-no-verify" should be
done for both of them in a separate follow-up topic.  #leftoverbits

Thanks.  I'll be online starting today, but please expect slow
responses for a few days as there is significant backlog.

[PATCH] builtin/merge: honor commit-msg hook for merges

[Stefan Beller]

Similar to 65969d43d1 (merge: honor prepare-commit-msg hook, 2011-02-14)
merge should also honor the commit-msg hook; the reason is the same as
in that commit: When a merge is stopped due to conflicts or --no-commit,
the subsequent commit calls the commit-msg hook.  However, it is not
called after a clean merge. Fix this inconsistency by invoking the hook
after clean merges as well.

This change is motivated by Gerrits commit-msg hook to install a change-id
trailer into the commit message. Without such a change id, Gerrit refuses
to accept (merge) commits by default, such that the inconsistency of
(not) running commit-msg hook between commit and merge leads to confusion
and might block people from getting their work done.

Signed-off-by: Stefan Beller 
---
 builtin/merge.c|  8 
 t/t7504-commit-msg-hook.sh | 45 +
 2 files changed, 49 insertions(+), 4 deletions(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index 7df3fe3927..087efd560d 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -73,6 +73,7 @@ static int show_progress = -1;
 static int default_to_upstream = 1;
 static int signoff;
 static const char *sign_commit;
+static int no_verify;
 
 static struct strategy all_strategy[] = {
{ "recursive",  DEFAULT_TWOHEAD | NO_TRIVIAL },
@@ -236,6 +237,7 @@ static struct option builtin_merge_options[] = {
  N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" },
OPT_BOOL(0, "overwrite-ignore", _ignore, N_("update ignored 
files (default)")),
OPT_BOOL(0, "signoff", , N_("add Signed-off-by:")),
+   OPT_BOOL(0, "no-verify", _verify, N_("bypass pre-commit and 
commit-msg hooks")),
OPT_END()
 };
 
@@ -780,6 +782,12 @@ static void prepare_to_commit(struct commit_list 
*remoteheads)
if (launch_editor(git_path_merge_msg(), NULL, NULL))
abort_commit(remoteheads, NULL);
}
+
+   if (!no_verify && run_commit_hook(0 < option_edit, get_index_file(),
+ "commit-msg",
+ git_path_merge_msg(), NULL))
+   abort_commit(remoteheads, NULL);
+
read_merge_msg();
strbuf_stripspace(, 0 < option_edit);
if (!msg.len)
diff --git a/t/t7504-commit-msg-hook.sh b/t/t7504-commit-msg-hook.sh
index 88d4cda299..1cd54af3cc 100755
--- a/t/t7504-commit-msg-hook.sh
+++ b/t/t7504-commit-msg-hook.sh
@@ -101,6 +101,10 @@ cat > "$HOOK" <> file &&
@@ -135,6 +139,32 @@ test_expect_success '--no-verify with failing hook 
(editor)' '
 
 '
 
+test_expect_success 'merge fails with failing hook' '
+
+   test_when_finished "git branch -D newbranch" &&
+   test_when_finished "git checkout -f master" &&
+   git checkout --orphan newbranch &&
+   : >file2 &&
+   git add file2 &&
+   git commit --no-verify file2 -m in-side-branch &&
+   test_must_fail git merge --allow-unrelated-histories master &&
+   commit_msg_is "in-side-branch" # HEAD before merge
+
+'
+
+test_expect_success 'merge bypasses failing hook with --no-verify' '
+
+   test_when_finished "git branch -D newbranch" &&
+   test_when_finished "git checkout -f master" &&
+   git checkout --orphan newbranch &&
+   : >file2 &&
+   git add file2 &&
+   git commit --no-verify file2 -m in-side-branch &&
+   git merge --no-verify --allow-unrelated-histories master &&
+   commit_msg_is "Merge branch '\''master'\'' into newbranch"
+'
+
+
 chmod -x "$HOOK"
 test_expect_success POSIXPERM 'with non-executable hook' '
 
@@ -178,10 +208,6 @@ exit 0
 EOF
 chmod +x "$HOOK"
 
-commit_msg_is () {
-   test "$(git log --pretty=format:%s%b -1)" = "$1"
-}
-
 test_expect_success 'hook edits commit message' '
 
echo "additional" >> file &&
@@ -217,7 +243,17 @@ test_expect_success "hook doesn't edit commit message 
(editor)" '
echo "more plus" > FAKE_MSG &&
GIT_EDITOR="\"\$FAKE_EDITOR\"" git commit --no-verify &&
commit_msg_is "more plus"
+'
 
+test_expect_success 'hook called in git-merge picks up commit message' '
+   test_when_finished "git branch -D newbranch" &&
+   test_when_finished "git checkout -f master" &&
+   git checkout --orphan newbranch &&
+   : >file2 &&
+   git add file2 &&
+   git commit --no-verify file2 -m in-side-branch &&
+   git merge --allow-unrelated-histories master &&
+   commit_msg_is "new message"
 '
 
 # set up fake editor to replace `pick` by `reword`
@@ -237,4 +273,5 @@ test_expect_success 'hook is called for reword during 
`rebase -i`' '
 
 '
 
+
 test_done
-- 
2.14.0.rc0.3.g6c2e499285

commit-msg hook and merges

[Søren Holm]

Hi. 

I'm running a speciallized commit-msg hook to help me fill out commit 
messages. This all works nicely for alle commits except for merges.

What I normally do to circumvent this is this :

$ git merge somebranch

here I append to autogenerated message with my own text

$ git commit --ammend

I just save the message without changes and the commit-message is run 
throught my filter


Is that intentional or an error ?

-- 
Søren Holm
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html