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

>
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
> > There's no need to display the annoying warning on every pull... only
> > the ones that are not fast-forward.
>
> Yes!  And thanks to the previous two steps, the change to the code
> is quite obvious.  I don't have to give any further comment on the
> part that changes code---it is well done, period.
>
> > This requires the tests to pick another base, so the merge is not
> > fast-forward. And in the cases where --ff-only is specified add
> > test_must_fail (since now they are non-fast-forward).
>
> I am not sure what this means.

> Existing tests pull histories that may or may not be descendants of
> the HEAD.  If we do not change the pair of commits involved in the
> tests, i.e. if we do not apply many s/c0/c2/ replacement I see in
> the patch, some of these tests would change their behaviour with
> respect to their advice output (but not the outcome).

> The actual changes to some of the tests do look sensible, testing
> both sides of the coin.  I've looked at the changes to the tests,
> but cannot convince me that we are not making irrelevant changes
> to the tests, or losing coverage needlessly because of s/c0/c2/
> (i.e. turning tests that used to check fast-forward situations
> into tests that check non-ff situations).

> > diff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh
> > index 6774e9d86f..6b4adab8b1 100755
> > --- a/t/t7601-merge-pull-config.sh
> > +++ b/t/t7601-merge-pull-config.sh
> > @@ -28,7 +28,7 @@ test_expect_success 'setup' '
> >  '
> >
> >  test_expect_success 'pull.rebase not set' '
> > -     git reset --hard c0 &&
> > +     git reset --hard c2 &&
> >       git -c color.advice=always pull . c1 2>err &&
> >       test_decode_color <err >decoded &&
> >       test_i18ngrep "<YELLOW>hint: " decoded &&
>
> This is not "keeping what the original test does and adjusting the
> expectation, to demonstrate how behaviour changed"; instead, we make
> sure that the message we originally gave and we intend to keep
> giving is shown in non-ff situation by choosing the current commit
> that won't allow a ff merge.  This is OK if we did not lose test
> coverage---as long as we test that we no longer give the message in
> ff situation somewhere else, And that happens later, I think.
>
> > @@ -36,54 +36,60 @@ test_expect_success 'pull.rebase not set' '
> >
> >  '
> >
> > -test_expect_success 'pull.rebase not set and pull.ff=true' '
> > +test_expect_success 'pull.rebase not set (fast-forward)' '
> >       git reset --hard c0 &&
> > +     git pull . c1 2>err &&
> > +     test_i18ngrep ! "Pulling without specifying how to reconcile" err
> > +'
>
> This is the new test to check the other side of the coin.  It sees
> how the original test to merge c1 into c0 would behave with the new
> code.  We make sure we do not give the advice because it is
> irrelevant in this situation.
>
> So the above two are good, even though the way this patch updates
> tests is probably a bit more error prone than necessary.

> Since we have checked how the new code behave for fast-forward with
> this new test, the remainder of the entire test script can be
> modified to test only non-ff situation without losing test coverage?
>
> I am not sure if that is the case.

> > +test_expect_success 'pull.rebase not set and pull.ff=true' '
> > +     git reset --hard c2 &&
> >       test_config pull.ff true &&
> >       git pull . c1 2>err &&
> >       test_i18ngrep ! "Pulling without specifying how to reconcile" err
> >  '
>
> We are merely allowing fast-forward merges without unnecessary merge
> commits, but we are faced to merge c1 into c2, which is not ff.  The
> command goes ahead and merges anyway, but why shouldn't we be seeing
> the message?  I am puzzled.

> >  test_expect_success 'pull.rebase not set and pull.ff=false' '
> > -     git reset --hard c0 &&
> > +     git reset --hard c2 &&
> >       test_config pull.ff false &&
> >       git pull . c1 2>err &&
> >       test_i18ngrep ! "Pulling without specifying how to reconcile" err
> >  '
> >
> >  test_expect_success 'pull.rebase not set and pull.ff=only' '
> > -     git reset --hard c0 &&
> > +     git reset --hard c2 &&
> >       test_config pull.ff only &&
> > -     git pull . c1 2>err &&
> > +     test_must_fail git pull . c1 2>err &&
> >       test_i18ngrep ! "Pulling without specifying how to reconcile" err
> >  '
>
> This used to test that fast-forwarding the HEAD from c0 to c2 would
> be done successfully without issuing the message.  Shouldn't that
> still be true in the improved "do not complain on fast-forward" code?

> I'd stop my review on the tests here, but I generally think s/c0/c2/
> done in this patch is a wrong thing to do.  We are changing the
> condition under which the messages is given (we are narrowing it to
> avoid giving it when it is irrelevant), without changing the final
> outcome (even though we changed the condition to give the message,
> we didn't change what the final outcome of the pull command would
> be), so I'd strongly prefer testing the same set of scenarios and
> update the expectation to an improved reality.