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

[Junio C Hamano <gitster@pobox.com>]

Stefan Beller <sbeller@google.com> 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", &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.