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