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

From: Ævar Arnfjörð Bjarmason <[email-masked]>
Date: Mon, 19 Nov 2018 22:37:49 +0100

On Mon, Nov 19 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.

Yup. Sorry should have mentioned that, it's broken in master. Reported
it earlier today:
https://public-inbox.org/git/[email-masked]/

> What's the correct fix here? Force the builtin rebase in this test?
> Unify the error message in the non-builtin case?

Probably to just force the builtin, unless Johannes wants to also fix
the bug for the shellscript version. I don't know if for 2.20 we're
trying to maintain 100% compatibility.