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

[Stefan Beller <sbeller@google.com>]

On Tue, Sep 5, 2017 at 6:57 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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 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?