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

[Junio C Hamano <gitster@pobox.com>]

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

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