Re: [PATCH] rebase: fix regression in rebase.useBuiltin=false test mode

From: Phillip Wood <phillip.wood@talktalk.net>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>, git@vger.kernel.org
Cc: Junio C Hamano <gitster@pobox.com>,
	Phillip Wood <phillip.wood@dunelm.org.uk>,
	Johannes Schindelin <Johannes.Schindelin@gmx.de>
Subject: Re: [PATCH] rebase: fix regression in rebase.useBuiltin=false test mode
Date: Thu, 14 Feb 2019 16:12:17 +0000	[thread overview]
Message-ID: <af4ce5d4-a788-25a6-0372-13206b161101@talktalk.net> (raw)
In-Reply-To: <20190213214908.9459-1-avarab@gmail.com>

Hi Ævar

On 13/02/2019 21:49, Ævar Arnfjörð Bjarmason wrote:
> Fix a recently introduced regression in c762aada1a ("rebase -x: sanity
> check command", 2019-01-29) triggered when running the tests with
> GIT_TEST_REBASE_USE_BUILTIN=false. See 62c23938fa ("tests: add a
> special setup where rebase.useBuiltin is off", 2018-11-14) for how
> that test mode works.
> 
> As discussed on-list[1] it's not worth it to implement the sanity
> check in the legacy rebase code, we plan to remove it after the 2.21
> release. So let's do the bare minimum to make the tests pass under the
> GIT_TEST_REBASE_USE_BUILTIN=false special setup.
> 
> 1. https://public-inbox.org/git/xmqqva1nbeno.fsf@gitster-ct.c.googlers.com/
> 

Thanks for doing this, when I wrote the original patch I decided it wasn't
worth updating the legacy rebase but forgot about running the tests with
rebase.useBuiltin off.

Best Wishes

Phillip

> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
> 
> On Wed, Feb 13 2019, Junio C Hamano wrote:
>    
>> [...]
>> It would however be long enough to declare victory _immediately
>> after_ the upcoming release and start the next cycle without the
>> escape hatch.  At that point we'd be committed to maintain only the
>> built-in version.
>>
>> The more important every-day features should still be covered by
>> tests, if the scripted ones are to be kept as escape hatch.  So to
>> me #1 looks like the only sensible choice at this point, if you want
>> to see a test cycle with USE_BUILTIN=false to fully pass (i.e. by
>> skipping the ones that are known not to pass).
> 
> Let's do this for 2.21 then and I'll re-submit the legacy removal
> patch after we start 2.22.
> 
>   t/t3404-rebase-interactive.sh | 9 ++++++---
>   1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index 52fa41c707..b60b11f9f2 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -149,10 +149,12 @@ test_expect_success 'rebase -i with the exec command checks tree cleanness' '
>   
>   test_expect_success 'rebase -x with empty command fails' '
>   	test_when_finished "git rebase --abort ||:" &&
> -	test_must_fail git rebase -x "" @ 2>actual &&
> +	test_must_fail env GIT_TEST_REBASE_USE_BUILTIN=true \
> +		git rebase -x "" @ 2>actual &&
>   	test_write_lines "error: empty exec command" >expected &&
>   	test_i18ncmp expected actual &&
> -	test_must_fail git rebase -x " " @ 2>actual &&
> +	test_must_fail env GIT_TEST_REBASE_USE_BUILTIN=true \
> +		git rebase -x " " @ 2>actual &&
>   	test_i18ncmp expected actual
>   '
>   
> @@ -160,7 +162,8 @@ LF='
>   '
>   test_expect_success 'rebase -x with newline in command fails' '
>   	test_when_finished "git rebase --abort ||:" &&
> -	test_must_fail git rebase -x "a${LF}b" @ 2>actual &&
> +	test_must_fail env GIT_TEST_REBASE_USE_BUILTIN=true \
> +		git rebase -x "a${LF}b" @ 2>actual &&
>   	test_write_lines "error: exec commands cannot contain newlines" \
>   			 >expected &&
>   	test_i18ncmp expected actual
> 