Junio C Hamano wrote: > Junio C Hamano <gitster@xxxxxxxxx> 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
