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

From: Felipe Contreras <felipe.contreras@gmail.com>
To: Junio C Hamano <gitster@pobox.com>,
	Felipe Contreras <felipe.contreras@gmail.com>
Cc: Git <git@vger.kernel.org>, "Elijah Newren" <newren@gmail.com>,
	"Jeff King" <peff@peff.net>, "Vít Ondruch" <vondruch@redhat.com>
Subject: Re: [PATCH v5 3/3] pull: display default warning only when non-ff
Date: Sat, 12 Dec 2020 10:01:07 -0600	[thread overview]
Message-ID: <5fd4e94317d67_bc1eb208da@natae.notmuch> (raw)
In-Reply-To: <xmqq1rfvgtvx.fsf@gitster.c.googlers.com>

Junio C Hamano wrote:
> 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.

I prefer not to attempt to read minds (plus, the author might have not
cared that the pulling succeeds), and anyway; that's not what matters.

What matters is the current situation, and the desired situation.

> We have no right [*1*] to say that scenario (i.e. "pull --rebase" c1
> into c0) no longer matters without a good justification.

But nobody is saying that. What I said is that in *this* particular
test-case that's not the objective of the test. And if you consider
hypothetical secondary objectives of the test, those are being tested
elsewhere.

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

Again, I don't particularly care to mindread what the test authors might
have cared about.

It's clear from the tests themselves what they are trying to do: check
if the warning exists, or doesn't.

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

No. All the tests (sans 1) check that the warning is *not* present.

If you do a fast-forward pull, the warning will not be present
(regardless of options), and the test would pass, but for the wrong
reasons.

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

No, the expectation has not changed one iota; it's exactly same.

It's the reason for the same output that changed.

If I take the current tests, and I remove the thing that makes them
special (arguments and/or configuration), essentially making them all
"git pull":

@@ -35,52 +35,49 @@ test_expect_success 'pull.rebase not set' '
 
 test_expect_success 'pull.rebase not set and pull.ff=true' '
 	git reset --hard c0 &&
-	test_config pull.ff true &&
 	git pull . c1 2>err &&
 	test_i18ngrep ! "Pulling without specifying how to reconcile" err
 '
 
 test_expect_success 'pull.rebase not set and pull.ff=false' '
 	git reset --hard c0 &&
-	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 &&
-	test_config pull.ff only &&
 	git pull . c1 2>err &&
 	test_i18ngrep ! "Pulling without specifying how to reconcile" err
 '
 
 test_expect_success 'pull.rebase not set and --rebase given' '
 	git reset --hard c0 &&
-	git pull --rebase . c1 2>err &&
+	git pull . c1 2>err &&
 	test_i18ngrep ! "Pulling without specifying how to reconcile" err
 '
 
 test_expect_success 'pull.rebase not set and --no-rebase given' '
 	git reset --hard c0 &&
-	git pull --no-rebase . c1 2>err &&
+	git pull . c1 2>err &&
 	test_i18ngrep ! "Pulling without specifying how to reconcile" err
 '
 
 test_expect_success 'pull.rebase not set and --ff given' '
 	git reset --hard c0 &&
-	git pull --ff . c1 2>err &&
+	git pull . c1 2>err &&
 	test_i18ngrep ! "Pulling without specifying how to reconcile" err
 '
 
 test_expect_success 'pull.rebase not set and --no-ff given' '
 	git reset --hard c0 &&
-	git pull --no-ff . c1 2>err &&
+	git pull . c1 2>err &&
 	test_i18ngrep ! "Pulling without specifying how to reconcile" err
 '
 
 test_expect_success 'pull.rebase not set and --ff-only given' '
 	git reset --hard c0 &&
-	git pull --ff-only . c1 2>err &&
+	git pull . c1 2>err &&
 	test_i18ngrep ! "Pulling without specifying how to reconcile" err
 '
 
What happens?

ok 3 - pull.rebase not set and pull.ff=true
ok 4 - pull.rebase not set and pull.ff=false
ok 5 - pull.rebase not set and pull.ff=only
ok 6 - pull.rebase not set and --rebase given
ok 7 - pull.rebase not set and --no-rebase given
ok 8 - pull.rebase not set and --ff given
ok 9 - pull.rebase not set and --no-ff given
ok 10 - pull.rebase not set and --ff-only given

They all pass. What are they supposed to be testing now? I don't know.

In my opinion they are no-ops now, but fine; I'll leave them as is.

Cheers.

-- 
Felipe Contreras
