Re: [PATCH v5 3/3] pull: display default warning only when non-ff
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Dec 11, 2020, 23:56 UTC
- Message-ID
- <xmqqa6ujj3s4.fsf@gitster.c.googlers.com>
- In-Reply-To
- <CAMP44s0wjfZ9TeQzpJvVD-OzFA47HFd87TABiJo3Ec9H8j-fjA@mail.gmail.com>
Felipe Contreras <felipe.contreras@gmail.com> writes:
Show 5 quoted lines
>> 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.
Show 13 quoted lines
> ... > 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.
I'll keep this short as I am supposed to be off officially.
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.
Show 6 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, 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.