On Tue, Jun 14 2022, Todd Zullinger wrote:
> Æ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.
> [...]
>> ---
>> 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.
My patch landed on "master" as 7ccbea564e8 (add -i tests: mark "TODO"
depending on GIT_TEST_ADD_I_USE_BUILTIN, 2022-06-14) so this is water
under the bridge.
But just to tie this loose knot I think something went wrong in your
testing.
If I:
git checkout v2.37.0-rc0
# Apply your patch from <20220614185218.1091413-1-tmz@xxxxxxxxx>
I'll consistently get a failure from:
GIT_TEST_ADD_I_USE_BUILTIN=false ./t3701-add-interactive.sh
Since we do fail that test with the Perl implementation, and now it's no
longer a TODO test.
Perhaps you used it as a parameter to "make"? I.e.:
make GIT_TEST_ADD_I_USE_BUILTIN=false
make test
Which isn't how it works, just speculating...
>> 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.
Yeah seems like a good idea for a follow-up, but since it's landed I'll
probably forget :)
> Thanks for the keen eye and attention to detail, Ævar,
Happy to have it fixed!
