Re: [PATCH v2] add -i tests: mark "TODO" depending on GIT_TEST_ADD_I_USE_BUILTIN

From: Todd Zullinger <tmz@pobox.com>
Date: Jun 15, 2022, 02:47 UTC
Message-ID: <YqlIRveupj6tOO4P@pobox.com>
In-Reply-To: <patch-v2-1.1-13c26e546f6-20220614T153746Z-avarab@gmail.com>

Ævar Arnfjörð Bjarmason wrote:

> Fix an issue that existed before 0527ccb1b55 (add -i: default to the
> built-in implementation, 2021-11-30), but which became the default
> with that change, we should not be marking tests that are known to
> pass as "TODO" tests.

[...]

Show 5 quoted lines

> ---
> Just converting it to "test_expect_success" will break CI and other
> setups that are testing with GIT_TEST_ADD_I_USE_BUILTIN=false.
> 
> The below fixes it, however.

Nice catch. FWIW, I tested w/GIT_TEST_ADD_I_USE_BUILTIN=0 and without.

Show 12 quoted lines

> diff --git a/t/t2016-checkout-patch.sh b/t/t2016-checkout-patch.sh
> index bc3f69b4b1d..a5822e41af2 100755
> --- a/t/t2016-checkout-patch.sh
> +++ b/t/t2016-checkout-patch.sh
> @@ -4,7 +4,7 @@ test_description='git checkout --patch'
>  
>  . ./lib-patch-mode.sh
>  
> -if ! test_bool_env GIT_TEST_ADD_I_USE_BUILTIN true && ! test_have_prereq PERL
> +if ! test_have_prereq ADD_I_USE_BUILTIN && ! test_have_prereq PERL
>  then
>  	skip_all='skipping interactive add tests, PERL not set'

It's not the fault of this patch, but it makes it obvious that the `skip_all` message is no longer accurate. Perhaps somethine like this?

    skip_all='skipping interactive add tests, missing ADD_I_USE_BUILTIN or PERL'

Maybe a separate `ADD_I` prereq would be better? Though without looking closer, I don't know if that would end up being clearer to anyone running the tests without either PERL or the add -i builtin enabled.

Thanks for the keen eye and attention to detail, Ævar,

-- 
Todd