archive mirror
 help / color / mirror / Atom feed
From: Junio C Hamano <>
To: Felipe Contreras <>
Cc: Git <>, "Elijah Newren" <>,
	"Jeff King" <>, "Vít Ondruch" <>
Subject: Re: [PATCH v5 3/3] pull: display default warning only when non-ff
Date: Fri, 11 Dec 2020 18:11:44 -0800	[thread overview]
Message-ID: <> (raw)

Junio C Hamano <> 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.  We
have no right [*1*] to say that scenario (i.e. "pull --rebase" c1
into c0) no longer matters without a good justification.  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.

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


*1* That does not mean we must not care about other scenarios that
are different from what have been tested with existing tests.  If
there is new behaviour introduced by patch 3/3, it is prudent to
protect it from future breakage by adding a test that pulls c1 into
c2, if that case is not already tested with existing tests.  

I suspect we already make sure a non-ff merge gives the annoying
message while going ahead, so there may be no new additional test
required, though.

  parent reply	other threads:[~2020-12-12 11:14 UTC|newest]

Thread overview: 27+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-12-10 10:05 [PATCH v5 0/3] pull: stop warning on every pull Felipe Contreras
2020-12-10 10:05 ` [PATCH v5 1/3] pull: refactor fast-forward check Felipe Contreras
2020-12-11  6:54   ` Junio C Hamano
2020-12-12 15:18     ` Felipe Contreras
2020-12-10 10:05 ` [PATCH v5 2/3] pull: move default warning Felipe Contreras
2020-12-11  6:54   ` Junio C Hamano
2020-12-11  7:55     ` Felipe Contreras
2020-12-12  0:00       ` Junio C Hamano
2020-12-12  1:05         ` Felipe Contreras
2020-12-13 20:58           ` Junio C Hamano
2020-12-14 11:02             ` Felipe Contreras
2020-12-12 16:42       ` Felipe Contreras
2020-12-10 10:05 ` [PATCH v5 3/3] pull: display default warning only when non-ff Felipe Contreras
2020-12-11  7:16   ` Junio C Hamano
2020-12-11 12:48     ` Felipe Contreras
2020-12-11 23:56       ` Junio C Hamano
2020-12-12  1:01         ` Felipe Contreras
2020-12-12  2:11         ` Junio C Hamano [this message]
2020-12-12 16:01           ` Felipe Contreras
2020-12-14 21:04             ` Junio C Hamano
2020-12-14 21:40               ` Felipe Contreras
2020-12-11  7:17 ` [PATCH v5 0/3] pull: stop warning on every pull Junio C Hamano
2020-12-11 13:28   ` Felipe Contreras
2020-12-12  2:50     ` Junio C Hamano
2020-12-12 16:36       ` Felipe Contreras
2020-12-14  0:57         ` Felipe Contreras
2020-12-12 16:52 Felipe Contreras
2020-12-12 16:52 ` [PATCH v5 3/3] pull: display default warning only when non-ff Felipe Contreras

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \ \ \ \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).