Ævar Arnfjörð Bjarmason [off-list ref] 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.
On Thu, Feb 11 2021, Junio C Hamano wrote:
Ævar Arnfjörð Bjarmason [off-list ref] writes:
quoted
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/
Junio C Hamano wrote:
Ævar Arnfjörð Bjarmason [off-list ref] writes:
quoted
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.
I agree: in particular, I would value the ability to run tests with,
say, LANG=fr_CH.UTF-8 and catch localization-specific issues that way.
Removing the test_i18ncmp helper removes that ability.
That said:
[...]
So, perhaps we should bite the bullet and just take this step.
If we don't have a convenient way to keep "testing the testsuite"
(i.e., some kind of CI run that uses a locale other than C), then I
agree that explicitly removing the test_i18ncmp / test_cmp distinction
is better than letting it bitrot.
Thanks,
Jonathan