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

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

Felipe Contreras <felipe.contreras@gmail.com> writes:

>> We seem to be losing test coverage by checking how pull.ff=only prevents
>> the command from working in a non-ff merge.
>
> No we don't. Remove the "test_config pull.ff only" and the test fails,
> as expected.

> ...
> What do we get?
>
> not ok 4 - pull.rebase not set and pull.ff=true
> not ok 5 - pull.rebase not set and pull.ff=false
> not ok 6 - pull.rebase not set and pull.ff=only
> not ok 7 - pull.rebase not set and --rebase given
> not ok 8 - pull.rebase not set and --no-rebase given
> not ok 9 - pull.rebase not set and --ff given
> not ok 10 - pull.rebase not set and --no-ff given
> not ok 11 - pull.rebase not set and --ff-only given
>
> All failures. Exactly as expected.

Assuming only one kind of breakage and try to break exactly that
thing does not prove much.

I'll keep this short as I am supposed to be off officially.

With pulling without choosing between rebase/merge, the old code
had one behaviour wrt the message---it always advised, whether
the pull was ff or not.

The new code has two behaviour wrt this aspect.  It behaves
differently when the pull is ff or non-ff.  That would double the
possibility that needs to be tested if we wanted to keep covering
the original set of conditions *and* cover all new possibilities.

I am saying that you should keep the original ones, and add new ones
to cover the new cases if that matters.  Otherwise the conditions
under which the original tests were checking would no longer be
tested.

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