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"