Re: Git Test Coverage Report (v2.20.0-rc0)

From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
To: Derrick Stolee <stolee@gmail.com>
Cc: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
	"git@vger.kernel.org" <git@vger.kernel.org>
Subject: Re: Git Test Coverage Report (v2.20.0-rc0)
Date: Tue, 20 Nov 2018 21:43:18 +0100 (STD)	[thread overview]
Message-ID: <nycvar.QRO.7.76.6.1811202142440.41@tvgsbejvaqbjf.bet> (raw)
In-Reply-To: <6f0ff2e3-4019-1dcc-f61a-cd0919b9a247@gmail.com>

[-- Attachment #1: Type: text/plain, Size: 1769 bytes --]

Hi Stolee,

On Mon, 19 Nov 2018, Derrick Stolee wrote:

> On 11/19/2018 1:33 PM, Ævar Arnfjörð Bjarmason wrote:
> > On Mon, Nov 19 2018, Derrick Stolee wrote:
> >
> > > [...]
> > > builtin/rebase.c
> > > 62c23938fa 55) return env;
> > > [...]
> > > Ævar Arnfjörð Bjarmason 62c23938f: tests: add a special setup
> > > where rebase.useBuiltin is off
> > This one would be covered with
> > GIT_TEST_REBASE_USE_BUILTIN=false. Obviously trivial, but I wonder if
> > the rest of the coverage would look different when passed through the
> > various GIT_TEST_* options.
> >
> 
> Thanks for pointing out this GIT_TEST_* variable to me. I had been running
> builds with some of them enabled, but didn't know about this one.
> 
> Unfortunately, t3406-rebase-message.sh fails with
> GIT_TEST_REBASE_USE_BUILTIN=false and it bisects to 4520c2337: Merge branch
> 'ab/rebase-in-c-escape-hatch'.
> 
> The issue is that the commit 04519d72 "rebase: validate -C<n> and
> --whitespace=<mode> parameters early" introduced the following test that cares
> about error messages:
> 
> +test_expect_success 'error out early upon -C<n> or --whitespace=<bad>' '
> +       test_must_fail git rebase -Cnot-a-number HEAD 2>err &&
> +       test_i18ngrep "numerical value" err &&
> +       test_must_fail git rebase --whitespace=bad HEAD 2>err &&
> +       test_i18ngrep "Invalid whitespace option" err
> +'
> 
> The merge commit then was the first place where this test could run with that
> variable.
> 
> What's the correct fix here? Force the builtin rebase in this test? Unify the
> error message in the non-builtin case?

I am obviously biased, but my take is that the correct fix is in
https://public-inbox.org/git/pull.86.git.gitgitgadget@gmail.com

Ciao,
Dscho