Re: [PATCH 4/4] tests: remove most uses of test_i18ncmp

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Feb 13, 2021, 14:31 UTC
Message-ID: <8735y0ca4i.fsf@evledraar.gmail.com>
In-Reply-To: <xmqqa6say0h8.fsf@gitster.c.googlers.com>

On Thu, Feb 11 2021, Junio C Hamano wrote:

Show 32 quoted lines

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> As a follow-up to d162b25f956 (tests: remove support for
>> GIT_TEST_GETTEXT_POISON, 2021-01-20) remove most uses of test_i18ncmp
>> via a simple s/test_i18ncmp/test_cmp/g search-replacement.
>
> I am a bit hesitant to take this step, primarily because the
> distinction between test_cmp and test_i18ncmp is serving as a sign
> for tests to identify if they are dealing with translatable messages
> or plumbing/machine-readable messages.
>
> If a totally different approach to test i18n messging part
> (e.g. Dscho's rot13) appears any time soon, it would be helpful to
> leave these distinction in, rather than ripping them out, with two
> caveats.
>
>  - If such a change does happen soon enough, then the codebase this
>    huge patch touches would not be all that different by the time
>    when it happens, so reverting this patch should not be too
>    involved, which means my hesitation is not all that warranted.
>
>  - If such a change does not happen for some time, then it would
>    become impractical to revert this patch when it does happen due
>    to changes in the codebase, if we apply it with the intention of
>    "we can easily revert if we need to".  But even if we drop this
>    step, until such a change appears, we surely will gain more calls
>    to test_cmp (and possibly test_i18ncmp) in the codebase, and at
>    that point, we can trust the distinction between test_cmp and
>    test_i18ncmp even less, which means it does not help all that
>    much if we tried to keep test_i18ncmp.
>
> So, perhaps we should bite the bullet and just take this step.

I think it either makes sense to take this series as a whole, or not take any of it and also revert the already merged 73c01d25fe (tests: remove uses of GIT_TEST_GETTEXT_POISON=false, 2021-01-20).

I.e. if we're going to not remove test_i18ncmp uses then we should leave C_LOCALE_OUTPUT & GIT_TEST_GETTEXT_POISON=false alone as well (or convert them to the test_i18n* forms).

As noted before I'm not invested in the outcome here, I just submitted this to not leave the obvious post-cleanup undone.

My reading of the list discussion/consensus[1] (such as it was) was that even if we had this rot13 mode ready we wouldn't think it's worth the time investment overall, hence the follow-up patches.

Also, if we have it down the line we're probably better off with these patches. The parts that'll revert cleanly can likely be resurrected as-is, but conflicts would be a good thing, as that code needs to be looked at anyway (it will be re-arranged tests that never used GETTEXT_POISON in their current form).

We also won't have cargo-culting in the meantime, with new code using test_i18ncmp or whatever just because the test above them did.

1. https://lore.kernel.org/git/20210120182759.31102-1-avarab@gmail.com/