Re: [PATCH v5 3/3] pull: display default warning only when non-ff
- From
Felipe Contreras <felipe.contreras@gmail.com>
- Date
- Dec 12, 2020, 16:01 UTC
- Message-ID
- <5fd4e94317d67_bc1eb208da@natae.notmuch>
- In-Reply-To
- <xmqq1rfvgtvx.fsf@gitster.c.googlers.com>
Junio C Hamano wrote:
Show 20 quoted lines
> 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.
Show 6 quoted lines
> 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