threads / patch / 47768

patch, 2 partsminor GETTEXT_POISON fixes

Subject: [PATCH 0/2] minor GETTEXT_POISON fixes

## tl;dr

5 messages between Feb 6, 2018 and Feb 6, 2018. Diffs are folded; open one to read it.

replies: 4people: 2as markdown or json

Jeff King· Feb 6, 2018, 08:42 UTC · lore

I set NO_GETTEXT=1 in my config.mak, and happened to notice that running the tests with GETTEXT_POISON fails. I think this has been broken for years, but I don't generally play with GETTEXT_POISON. ;)

  [1/2]: t0205: drop redundant test
  [2/2]: git-sh-i18n: check GETTEXT_POISON before USE_GETTEXT_SCHEME
 git-sh-i18n.sh            | 8 ++++----
 t/t0205-gettext-poison.sh | 4 ----
 2 files changed, 4 insertions(+), 8 deletions(-)
-Peff
Jeff King· Feb 6, 2018, 08:43 UTC · re: Jeff King · lore

[PATCH 1/2] t0205: drop redundant test

We check that a shell variable is non-empty, and then we check that it's equal to a particular value. Just checking the latter covers both cases.

I suspect the original was trying to give better output when the test fails, but using "-x" covers that these days.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t0205-gettext-poison.sh | 4 ----
 1 file changed, 4 deletions(-)
Show changes to t/t0205-gettext-poison.sh +0 −4
diff --git a/t/t0205-gettext-poison.sh b/t/t0205-gettext-poison.sh
index 2361590d54..438e778d6a 100755
--- a/t/t0205-gettext-poison.sh
+++ b/t/t0205-gettext-poison.sh
@@ -7,10 +7,6 @@ test_description='Gettext Shell poison'
 
 . ./lib-gettext.sh
 
-test_expect_success GETTEXT_POISON "sanity: \$GIT_INTERNAL_GETTEXT_SH_SCHEME is set (to $GIT_INTERNAL_GETTEXT_SH_SCHEME)" '
-    test -n "$GIT_INTERNAL_GETTEXT_SH_SCHEME"
-'
-
 test_expect_success GETTEXT_POISON 'sanity: $GIT_INTERNAL_GETTEXT_SH_SCHEME" is poison' '
     test "$GIT_INTERNAL_GETTEXT_SH_SCHEME" = "poison"
 '
-- 
2.16.1.273.gc07cfcd8c9
Jeff King· Feb 6, 2018, 08:44 UTC · re: Jeff King · lore

[PATCH 2/2] git-sh-i18n: check GETTEXT_POISON before USE_GETTEXT_SCHEME

Running "make NO_GETTEXT=1 GETTEXT_POISON=1" currently fails t0205.

While it might seem nonsensical at first glance to both poison and disable gettext, it's useful to be able to do a poison test-run on a system that doesn't have gettext at all. And it works fine for C programs; the problem is only with the shell code.

The issue is that we check the baked-in USE_GETTEXT_SCHEME value before GETTEXT_POISON. And when NO_GETTEXT is set, the Makefile sets USE_GETTEXT_SCHEME to "fallthrough".

So one fix would be to have the Makefile just set USE_GETTEXT_SCHEME to "poison" if GETTEXT_POISON is set. But there are two problems with that:

  1. USE_GETTEXT_SCHEME is actually a user-facing knob, so
     conceivably somebody could override it with:
       make USE_GETTEXT_SCHEME=gnu GETTEXT_POISON=1
     which would do the wrong thing (though that's much less
     likely than them having the variable set in their
     config.mak and just overriding GETTEXT_POISON on the
     command-line for a one-off test).
  2. We don't actually bake GETTEXT_POISON in to the shell
     library like we do for the C code. It checks
     $GIT_GETTEXT_POISON at runtime, which is set up by the
     test suite. So it makes sense to put the fix in the
     runtime code, too, which would cover something like:
       GIT_GETTEXT_POISON=foo git foo
     It's not likely that people use the poison code outside
     of running the test suite, but it's easy enough to make
     this case work.
Signed-off-by: Jeff King <peff@peff.net>
---
 git-sh-i18n.sh | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
Show changes to git-sh-i18n.sh +4 −4
diff --git a/git-sh-i18n.sh b/git-sh-i18n.sh
index 1ef1889dbd..9d065fb4bf 100644
--- a/git-sh-i18n.sh
+++ b/git-sh-i18n.sh
@@ -17,15 +17,15 @@ export TEXTDOMAINDIR
 
 # First decide what scheme to use...
 GIT_INTERNAL_GETTEXT_SH_SCHEME=fallthrough
-if test -n "@@USE_GETTEXT_SCHEME@@"
+if test -n "$GIT_GETTEXT_POISON"
+then
+	GIT_INTERNAL_GETTEXT_SH_SCHEME=poison
+elif test -n "@@USE_GETTEXT_SCHEME@@"
 then
 	GIT_INTERNAL_GETTEXT_SH_SCHEME="@@USE_GETTEXT_SCHEME@@"
 elif test -n "$GIT_INTERNAL_GETTEXT_TEST_FALLBACKS"
 then
 	: no probing necessary
-elif test -n "$GIT_GETTEXT_POISON"
-then
-	GIT_INTERNAL_GETTEXT_SH_SCHEME=poison
 elif type gettext.sh >/dev/null 2>&1
 then
 	# GNU libintl's gettext.sh
-- 
2.16.1.273.gc07cfcd8c9
Lars Schneider· Feb 6, 2018, 09:06 UTC · re: Jeff King · lore

Re: [PATCH 0/2] minor GETTEXT_POISON fixes

Show 5 quoted lines
> On 06 Feb 2018, at 09:42, Jeff King <peff@peff.net> wrote:
> 
> I set NO_GETTEXT=1 in my config.mak, and happened to notice that running
> the tests with GETTEXT_POISON fails. I think this has been broken for
> years, but I don't generally play with GETTEXT_POISON. ;)

On Travis we run GETTEXT_POISON with gettext installed. What do you think about an additional job with GETTEXT_POISON and NO_GETTEXT=1?

- Lars
Jeff King· Feb 6, 2018, 11:22 UTC · re: Lars Schneider · lore

Re: [PATCH 0/2] minor GETTEXT_POISON fixes

On Tue, Feb 06, 2018 at 10:06:53AM +0100, Lars Schneider wrote:
Show 9 quoted lines
> 
> > On 06 Feb 2018, at 09:42, Jeff King <peff@peff.net> wrote:
> > 
> > I set NO_GETTEXT=1 in my config.mak, and happened to notice that running
> > the tests with GETTEXT_POISON fails. I think this has been broken for
> > years, but I don't generally play with GETTEXT_POISON. ;)
> 
> On Travis we run GETTEXT_POISON with gettext installed.
> What do you think about an additional job with GETTEXT_POISON and NO_GETTEXT=1?
I think it's probably a waste of CPU. ;)

It would be running the whole test suite to fix this one particular bug, which I don't think is pretty likely to regress (at least not more than any other bug). Whereas running with GETTEXT_POISON is meant to shake out strings that maybe shouldn't be translated, which is a very likely bug to get introduced.

-Peff

← back to recent threads