builtin/merge: honor commit-msg hook for merges

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 <[email protected]>

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

builtin/merge.c +1 -0

index 7df3fe3927..087efd560d 100644
@@ -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 },

This patch disallows all no- options, but we could be more open and allow --no-options that have the NO_NEG bit set.

Quoted 2 lines · Stefan Beller

This patch disallows all no- options, but we could be more open and allow --no-options that have the NO_NEG bit set.

"--no-foo" that does not take "--foo" is perhaps OK so should not trigger an error.

A ("--no-foo", "--foo") pair is better spelled as ("--foo", "--no-foo") pair whose default is "--foo", but making it an error is probably a bit too much.

Compared to that, ("--no-foo", "--no-no-foo") pair feels nonsense.

Having said that, because the existing parse_options_check() is all about catching the programming mistake (the end user cannot fix an error from it by tweaking the command line option s/he gives to the program), I do not think a conditional compilation like you added mixes well. Either make the whole thing, not just your new test, conditional to -DDEVELOPER (which would make it possible for you to build and ship a binary with broken options[] array to the end-users that does not die in this function), which is undesirable, or add a new test that catches a definite error unconditionally.

Quoted 5 lines · Junio C Hamano

Quoted 2 lines · Stefan Beller

This patch disallows all no- options, but we could be more open and allow --no-options that have the NO_NEG bit set.

Quoted 8 lines

"--no-foo" that does not take "--foo" is perhaps OK so should not trigger an error.

A ("--no-foo", "--foo") pair is better spelled as ("--foo", "--no-foo") pair whose default is "--foo", but making it an error is probably a bit too much.

Compared to that, ("--no-foo", "--no-no-foo") pair feels nonsense.

Ahh, I was an idiot (call it vacation-induced-brain-disfunction). I forgot about 0f1930c5 ("parse-options: allow positivation of options starting, with no-", 2012-02-25), which may have already made your new use of "--no-verify" in builtin/merge.c and existing one in commit.c OK long time ago. A quick check to see how your version of

        git merge --verify
        git merge --no-verify

behaves with respect to the commit-msg hook is veriy much appreciated, as my tree is in no shape to apply and try a patch while trying to absorb the patches sent to the list the past week.

Thanks, and sorry for a possible false alarm.

Quoted 9 lines

Having said that, because the existing parse_options_check() is all about catching the programming mistake (the end user cannot fix an error from it by tweaking the command line option s/he gives to the program), I do not think a conditional compilation like you added mixes well. Either make the whole thing, not just your new test, conditional to -DDEVELOPER (which would make it possible for you to build and ship a binary with broken options[] array to the end-users that does not die in this function), which is undesirable, or add a new test that catches a definite error unconditionally.

This part still is valid. If René's work 2 years ago is sufficient to address "--no-foo" thing, then there is nothing we need to add to this test, but if we later need to add new sanity check, we should add it without -DDEVELOPER, or we should make the whole thing inside it.

Thanks.

Quoted 25 lines

Ahh, I was an idiot (call it vacation-induced-brain-disfunction). I forgot about 0f1930c5 ("parse-options: allow positivation of options starting, with no-", 2012-02-25), which may have already made your new use of "--no-verify" in builtin/merge.c and existing one in commit.c OK long time ago. A quick check to see how your version of

        git merge --verify
        git merge --no-verify

behaves with respect to the commit-msg hook is veriy much appreciated, as my tree is in no shape to apply and try a patch while trying to absorb the patches sent to the list the past week.

Thanks, and sorry for a possible false alarm.

Quoted 9 lines

Having said that, because the existing parse_options_check() is all about catching the programming mistake (the end user cannot fix an error from it by tweaking the command line option s/he gives to the program), I do not think a conditional compilation like you added mixes well. Either make the whole thing, not just your new test, conditional to -DDEVELOPER (which would make it possible for you to build and ship a binary with broken options[] array to the end-users that does not die in this function), which is undesirable, or add a new test that catches a definite error unconditionally.

Quoted 5 lines

This part still is valid. If René's work 2 years ago is sufficient to address "--no-foo" thing, then there is nothing we need to add to this test, but if we later need to add new sanity check, we should add it without -DDEVELOPER, or we should make the whole thing inside it.

As far as the code is concerned it is only inside the -DDEVELOPER ? The intent of this patch is to have a developers aid to remind them that too many negations might be a sign of trouble.

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.

Quoted 4 lines · Junio

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", &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;
	}

Quoted 27 lines · Stefan Beller

Quoted 4 lines · Junio

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", &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);

Quoted 4 lines

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

Quoted 3 lines

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?)

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.

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

t/t7504-commit-msg-hook.sh +8 -0

+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^ &&

Quoted 5 lines · Stefan Beller

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

Quoted 4 lines

+static int verify_msg = 1;

builtin/merge.c static struct option builtin_merge_options[] = { · line 242

Quoted 9 lines

static struct strategy all_strategy[] = {
{ "recursive", DEFAULT_TWOHEAD | NO_TRIVIAL },

builtin/merge.c +1 -0

@@ -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", &overwrite_ignore, N_("update ignored files (default)")),
      OPT_BOOL(0, "signoff", &signoff, N_("add Signed-off-by:")),
+     OPT_BOOL(0, "verify", &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.

Quoted 8 lines

t/t7504-commit-msg-hook.sh +8 -0

+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^ &&

t/t7504-commit-msg-hook.sh test_expect_success "hook doesn't edit commit message (editor)" ' · line 276

Quoted 10 lines

t/t7504-commit-msg-hook.sh +9 -0

+     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?

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

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?)

The commit-msg hook is invoked by both commit and merge now.

Reported-by Kaartic Sivaraam <[email protected]>

Signed-off-by Stefan Beller <[email protected]>

---

 Documentation/githooks.txt | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

Documentation/githooks.txt +4 -5

index 623ed1a138..f4e75b9c90 100644
@@ -127,11 +127,10 @@ help message found in the commented portion of the commit template.
 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.
+This hook is invoked by 'git commit' and 'git merge', 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 command to abort.
 
 The hook is allowed to edit the message file in place, and can be used
 to normalize the message into some project standard format. It