Re: [PATCHv2] builtin/merge: honor commit-msg hook for merges
- From
Stefan Beller <sbeller@google.com>
- Date
- Sep 6, 2017, 22:11 UTC
- Message-ID
- <CAGZ79kauB+AznEOwmNBQLbuz-szD3kYBHAJDmy0OWWEbxny9xQ@mail.gmail.com>
- In-Reply-To
- <xmqq4lsga8s7.fsf@gitster.mtv.corp.google.com>
On Tue, Sep 5, 2017 at 6:57 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 36 quoted lines
> 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?)
Show 5 quoted lines
> 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?