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

From: Stefan Beller <sbeller@google.com>
Date: Sep 11, 2017, 17:12 UTC
Message-ID: <CAGZ79kZCQq8ts6RAN2Qz5vWtUh7swu0UWO_6LTaRhNF5sDbnDg@mail.gmail.com>
In-Reply-To: <xmqqr2vi0z7q.fsf@gitster.mtv.corp.google.com>

On Thu, Sep 7, 2017 at 6:13 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 10 quoted lines

> Stefan Beller <sbeller@google.com> writes:
>
>> .... 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 did not draw a conclusion, I was just saying, that this has to be thought about. It looks to me as if --continue currently wants to take no extra arguments, but to remember all flags from the previous invocation of merge.

But that only looks this way as all the command line flags are related for tree manipulation, not (indirect) commit message manipulation.

So I think having --no-verify be respected (either by restating, or by remembering, or both) by the `git merge --continue` call would be a reasonable thing to want.

So that new patch just added the test as a #needswork for the future, not actually making a decision how to approach it.

Show 9 quoted lines

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

Interestingly the documentation that I read to approach the problem is already in shape as it would not specify the specific command for the '--no-verify' option. I missed that we need to add documentation for the continued merge case.

Show 6 quoted lines

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

I would expect that, too.

Show 20 quoted lines

>
>
>> +static int verify_msg = 1;
>>
>>  static struct strategy all_strategy[] = {
>>       { "recursive",  DEFAULT_TWOHEAD | NO_TRIVIAL },
>> @@ -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.

ok, will revert to that in a resend.

Show 21 quoted lines

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

Currently not possible, due to --continue requiring that it is the only argument. In the future where --continue works well with other arguments, we should override the original.

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

Note that this is already test_expect_failure, which I used to mark that this particular flag is broken across a --continue invocation of git-merge, so I would not add yet another test that describes the future yet to be implemented?

Thanks.