Re: [PATCH v5 3/3] pull: display default warning only when non-ff

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 12, 2020, 02:11 UTC
Message-ID: <xmqq1rfvgtvx.fsf@gitster.c.googlers.com>
In-Reply-To: <xmqqa6ujj3s4.fsf@gitster.c.googlers.com>

Junio C Hamano <gitster@pobox.com> writes:

Show 13 quoted lines

>>  test_expect_success 'pull.rebase not set and --rebase given' '
>> -	git reset --hard c0 &&
>> +	git reset --hard c2 &&
>>  	git pull --rebase . c1 2>err &&
>>  	test_i18ngrep ! "Pulling without specifying how to reconcile" err
>>  '
>
> This used to make sure an attempt to rebase c1 onto c0, which can be
> fast-forwarded, would work fine, even though it used to give
> warning.  We should keep testing the same condition.  The
> expectation of seeing the warning is what must be changed, not the
> test condition (i.e. rebasing c1 onto c2 instead of c0)---you are no
> longer making sure that c1 can be rebased onto c0 cleanly.

Let's try to explain it in a different way.

The original author of this test cared that pulling c1 with --rebase into c0 succeeds, and that it does not give the error message. We have no right [*1*] to say that scenario (i.e. "pull --rebase" c1 into c0) no longer matters without a good justification. And it is not a good justification to say that the current code happens to behave identically whether running "pull --rebase" of c1 into c0 or c2 so it is sufficient to test the operation into c2. The test is *not* about how the current code happens to work. It is to make sure the scenarios test authors care about will keep behaving the same way.

Some tests may be expecting that pulling c1 into c0 would issue the message, and that the command succeeds, and with the patch 3/3 the outcome may become different, i.e. the command succeeds without annoying message. That would break the expectation of the original test authors, and it is a good thing. By recording a change to the expectation, we can document how the new behaviour works better under the same scenario.

[Footnote]

*1* That does not mean we must not care about other scenarios that are different from what have been tested with existing tests. If there is new behaviour introduced by patch 3/3, it is prudent to protect it from future breakage by adding a test that pulls c1 into c2, if that case is not already tested with existing tests.

I suspect we already make sure a non-ff merge gives the annoying message while going ahead, so there may be no new additional test required, though.