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

From: Felipe Contreras <felipe.contreras@gmail.com>
Date: Dec 12, 2020, 01:01 UTC
Message-ID: <CAMP44s256RnOy0pPDCa1M18Ahu30i3NY4S7OZD41U0tfZWYTCQ@mail.gmail.com>
In-Reply-To: <xmqqa6ujj3s4.fsf@gitster.c.googlers.com>

On Fri, Dec 11, 2020 at 5:56 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 25 quoted lines

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

It proves we are testing exactly the thing the test is meant to test.

No test is ever perfect.

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

I appreciate that.

Show 13 quoted lines

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

I disagree. The old condition is perfectly well tested with this test:

  test_expect_success 'pull.rebase not set' '
    git reset --hard c2 &&
    git -c color.advice=always pull . c1 2>err &&
    test_decode_color <err >decoded &&
    test_i18ngrep "<YELLOW>hint: " decoded &&
    test_i18ngrep "Pulling without specifying how to reconcile" decoded
  '

In other words; we have tests going horizontally, and tests going vertically. That should cover all the bases.

But fine, if you want a matrix of tests I can do that.

Show 9 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,

No it didn't. It may very well have done a merge, or done nothing at all.

The tests that actually check that "git pull --rebase" works both in the fast-forward and non-fast-forward are in t/t5520-pull.sh.

The goal of this particular test is to check that the warning is not there.

> even though it used to give
> warning.  We should keep testing the same condition.

We are testing what was originally tested: that the warning is not presented.

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

We disagree on what the "test condition" is supposed to be on this test.

But fine, I'll create the matrix.

Cheers.

-- 
Felipe Contreras