Re: [PATCH/RFCv2 2/2] git rebase -i: warn about removed commits

From: Eric Sunshine <sunshine@sunshineco.com>
To: Remi Galan Alfonso <remi.galan-alfonso@ensimag.grenoble-inp.fr>
Cc: Git List <git@vger.kernel.org>,
	Remi Lespinet <remi.lespinet@ensimag.grenoble-inp.fr>,
	Guillaume Pages <guillaume.pages@ensimag.grenoble-inp.fr>,
	Louis-Alexandre Stuber 
	<louis--alexandre.stuber@ensimag.grenoble-inp.fr>,
	Antoine Delaite <antoine.delaite@ensimag.grenoble-inp.fr>,
	Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>,
	Junio C Hamano <gitster@pobox.com>,
	Johannes Schindelin <johannes.schindelin@gmx.de>,
	Stefan Beller <sbeller@google.com>,
	Philip Oakley <philipoakley@iee.org>
Subject: Re: [PATCH/RFCv2 2/2] git rebase -i: warn about removed commits
Date: Tue, 2 Jun 2015 09:41:18 -0400	[thread overview]
Message-ID: <CAPig+cQF6irJqYFZ=2YSevjF+ySUn25udYMKSg+VzXP0p0-eAw@mail.gmail.com> (raw)
In-Reply-To: <53678876.5326.1433230965582.JavaMail.zimbra@ensimag.grenoble-inp.fr>

On Tue, Jun 2, 2015 at 3:42 AM, Remi Galan Alfonso
<remi.galan-alfonso@ensimag.grenoble-inp.fr> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
>> > +test_expect_success 'rebase -i respects rebase.checkLevel' '
>> > +       test_config rebase.checkLevel error &&
>> > +       test_when_finished "git checkout master" &&
>> > +       git checkout -b tmp2 master &&
>> > +       set_fake_editor &&
>> > +       test_must_fail env FAKE_LINES="1 2 3 4" git rebase -i --root &&
>> > +       test E = $(git cat-file commit HEAD | sed -ne \$p)
>> > +'
>>
>> Shouldn't you also explicitly test "warn" and "ignore" modes?
>
> I don't think testing "ignore" is really necessary since it
> corresponds to the default behaviour, it is thus silently tested by
> the other tests.

Although the underlying behavior of "ignore" may be the same as the
default, you also want to check that higher-level machinery for
recognizing the "ignore" option is working correctly, which is why
checking "ignore" explicitly is a reasonable thing to do.

> Either way, I will add a test for "warn".