[PATCH] i18n: disable i18n for shell scripts if NO_GETTEXT defined

Otherwise the i18n is used in the scripts even with NO_GETTEXT set.
It is very unexpected.

---

I usually disable i18n on my working systems as they are generally very
out-of-date and not supported by any sane developer. In particular the
gettext provided with this (very old) Cygwin distribution is fubar and
never produces any output.

 Makefile       |    1 +
 git-sh-i18n.sh |    4 ++--
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index a782409..d82ea6a 100644
--- a/Makefile
+++ b/Makefile

@@ -1887,6 +1887,7 @@ sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
     -e 's/@@GIT_VERSION@@/$(GIT_VERSION)/g' \
     -e 's|@@LOCALEDIR@@|$(localedir_SQ)|g' \
     -e 's/@@NO_CURL@@/$(NO_CURL)/g' \
+    -e 's/@@NO_GETTEXT@@/$(NO_GETTEXT)/g' \
     -e $(BROKEN_PATH_FIX) \
     $@.sh >$@+
 endef

diff --git a/git-sh-i18n.sh b/git-sh-i18n.sh
index b4575fb..7f7e32b 100644
--- a/git-sh-i18n.sh
+++ b/git-sh-i18n.sh

@@ -18,7 +18,7 @@ export TEXTDOMAINDIR

 if test -z "$GIT_GETTEXT_POISON"
 then
-	if test -z "$GIT_INTERNAL_GETTEXT_TEST_FALLBACKS" && type gettext.sh

/dev/null 2>&1

+	if test -z "@@NO_GETTEXT@@" && test -z
"$GIT_INTERNAL_GETTEXT_TEST_FALLBACKS" && type gettext.sh >/dev/null
2>&1
 	then
 		# This is GNU libintl's gettext.sh, we don't need to do anything
 		# else than setting up the environment and loading gettext.sh

@@ -29,7 +29,7 @@ then
 		# can't.
 		. gettext.sh

-	elif test -z "$GIT_INTERNAL_GETTEXT_TEST_FALLBACKS" && test

"$(gettext -h 2>&1)" = "-h"
+	elif test -z "@@NO_GETTEXT@@" && test -z
"$GIT_INTERNAL_GETTEXT_TEST_FALLBACKS" && test "$(gettext -h 2>&1)" =
"-h"
 	then
 		# We don't have gettext.sh, but there's a gettext binary in our
 		# path. This is probably Solaris or something like it which has a
-- 
1.7.8.2.388.ge40c2

On Tue, Jan 17, 2012 at 14:42, Alex Riesen [off-list ref] wrote:

Otherwise the i18n is used in the scripts even with NO_GETTEXT set.
It is very unexpected.

So the reason it's like that is that I was assuming that gettext.sh
wouldn't be FUBAR anywhere, but the translations shouldn't kick in
since we haven't installed them during "make install".

But I wonder if this negatively affects some systems, now we now:

 * Don't use gettext.sh, which means that we're using our fallback
   shell function instead of the binary gettext(1), which is probably
   faster.

 * Use our own eval_gettext() instead of using the system one, which
   uses the GNU binary which is more likely to be in the FS cache
   already since other programs are probably using it.

Which is why I didn't do something like this to begin with.

On Wed, Jan 18, 2012 at 16:22, Ævar Arnfjörð Bjarmason [off-list ref] wrote:

On Tue, Jan 17, 2012 at 14:42, Alex Riesen [off-list ref] wrote:

quoted

Otherwise the i18n is used in the scripts even with NO_GETTEXT set.
It is very unexpected.

So the reason it's like that is that I was assuming that gettext.sh
wouldn't be FUBAR anywhere, but the translations shouldn't kick in
since we haven't installed them during "make install".

But I wonder if this negatively affects some systems, now we now:

 * Don't use gettext.sh, which means that we're using our fallback
  shell function instead of the binary gettext(1), which is probably
  faster.

 * Use our own eval_gettext() instead of using the system one, which
  uses the GNU binary which is more likely to be in the FS cache
  already since other programs are probably using it.

Which is why I didn't do something like this to begin with.

Well, if I say NO_GETTEXT, I kind of want none of local gettext,
whether it works, or not.

On Wed, Jan 18, 2012 at 19:57, Alex Riesen [off-list ref] wrote:

On Wed, Jan 18, 2012 at 16:22, Ævar Arnfjörð Bjarmason [off-list ref] wrote:

quoted

On Tue, Jan 17, 2012 at 14:42, Alex Riesen [off-list ref] wrote:

quoted

Otherwise the i18n is used in the scripts even with NO_GETTEXT set.
It is very unexpected.

So the reason it's like that is that I was assuming that gettext.sh
wouldn't be FUBAR anywhere, but the translations shouldn't kick in
since we haven't installed them during "make install".

But I wonder if this negatively affects some systems, now we now:

 * Don't use gettext.sh, which means that we're using our fallback
  shell function instead of the binary gettext(1), which is probably
  faster.

 * Use our own eval_gettext() instead of using the system one, which
  uses the GNU binary which is more likely to be in the FS cache
  already since other programs are probably using it.

Which is why I didn't do something like this to begin with.

Well, if I say NO_GETTEXT, I kind of want none of local gettext,
whether it works, or not.

That's not what NO_GETTEXT means, and not what it *should* mean. It
means that your output won't be translated, but we might still make
use of a locally installed library to provide the gettext() and
eval_gettext() functions.

This approach has worked everywhere so far (Linux, OSX, *BSD etc.),
and you want to change *everywhere* because you have some completely
broken Cygwin install.

How did you even get that install? Is it a known issue? Some ancient
now-fixed bug? What version of Cygwin / gettext etc.

Now I'm not saying that we shouldn't fix this, I just don't think that
this is the right way to go about it.

Now I haven't done exhaustive tests but this is the sort of slowdown
we might be looking at on Linux for output, both with warm cache:

    $ cat our-eval_gettext.sh
    #!/bin/bash

    eval_gettext () {
            printf "%s" "$1" | (
                    export PATH $(git sh-i18n--envsubst --variables "$1");
                    git sh-i18n--envsubst "$1"
            )
    }
    for i in {1..1000}
    do
        some_variable="for speed"
        eval_gettext "benchmark this \$some_variable"
    done
    $ time bash our-eval_gettext.sh >/dev/null

    real    0m3.336s
    user    0m0.052s
    sys     0m0.128s

Compared to using the system eval_gettext, which for me is much
faster:

    $ cat system-eval_gettext.sh
    #!/bin/bash

    . gettext.sh
    for i in {1..1000}
    do
        some_variable="for speed"
        eval_gettext "benchmark this \$some_variable"
    done
    $ time bash system-eval_gettext.sh >/dev/null

    real    0m1.671s
    user    0m0.048s
    sys     0m0.140s

And then we have the gettext() function itself:

    $ cat our-gettext.sh
    #!/bin/bash

    gettext () {
            printf "%s" "$1"
    }

    for i in {1..1000}
    do
        gettext "benchmark this"
    done
    $ cat system-gettext.sh
    #!/bin/bash

    for i in {1..1000}
    do
        gettext "benchmark this"
    done

Where our fallback is faster, because printf() is a bash built-in:

    $ time bash system-gettext.sh >/dev/null

    real    0m0.534s
    user    0m0.016s
    sys     0m0.084s
    $ time bash our-gettext.sh >/dev/null

    real    0m0.018s
    user    0m0.016s
    sys     0m0.000s

Anyway speed is the least of the issues here, it's not like we're very
constrained by spewing out gettext output.

I just think we should consider portability more carefully than "it
doesn't work on one obscure setup, let's change it everywhere", when
actually it's working just fine in most places.

I think a better fix would be to add probes for whether the system
functions actually work in the autoconf script.

I'd also love to be able to use C macros in the git-*.sh scripts, it
would make the code in git-sh-i18n.sh much nicer since we can
determine what functions we want at compile time.

Another option would be to pipe our shellscripts through some
pre-processor that would completely remove the gettext and
eval_gettext calls. Then we'd be doing the same thing we're doing on
the C-Level, and we wouldn't have the previously cited command-call
overhead or Win32.

But in summary: We shouldn't be *always* using fallback functions
whether they're the C stuff in compat/* or the gettext fallbacks in
git-sh-i18n.sh just because there's some version out there of the
system-supplied functions that's broken.

It makes sense to prefer the system functions by default in both
cases, but when the OS one can be broken or lacking we can just add
probes or Makefile options like we do for fnmatch() with the
NO_FNMATCH_CASEFOLD switch.

Ævar Arnfjörð Bjarmason wrote:

That's not what NO_GETTEXT means, and not what it *should* mean. It
means that your output won't be translated, but we might still make
use of a locally installed library to provide the gettext() and
eval_gettext() functions.

This approach has worked everywhere so far (Linux, OSX, *BSD etc.),
and you want to change *everywhere* because you have some completely
broken Cygwin install.

I thought NO_GETTEXT meant either "I'm aware that there is this new
translation feature, and it may or may not be useful to me some day,
but no thanks for now, since I cannot tolerate the possibility of
regressions" (i.e., opting out of a new feature) or "my platform does
not have suitable gettext infrastructure so please do not use it"
(i.e., reducing build-time dependencies by making some optional).

"I don't want localized messages" is spelled as "LC_MESSAGES=C; export
LC_MESSAGES", not as "make NO_GETTEXT=YesPlease".

I guess I am wondering, does the approach in Alex's patch have the
potential to cause actual problems?  If it doesn't, I don't see what
there is to complain about.

Hope that helps,
Jonathan

On Thu, Jan 19, 2012 at 00:18, Ævar Arnfjörð Bjarmason [off-list ref] wrote:

On Wed, Jan 18, 2012 at 19:57, Alex Riesen [off-list ref] wrote:

quoted

Well, if I say NO_GETTEXT, I kind of want none of local gettext,
whether it works, or not.

That's not what NO_GETTEXT means, and not what it *should* mean. It
means that your output won't be translated, but we might still make
use of a locally installed library to provide the gettext() and
eval_gettext() functions.

I would never guess all that by its name: NO_GETTEXT. I wanted to
say: there is no gettext in this installation, don't even try it.

This approach has worked everywhere so far (Linux, OSX, *BSD etc.),
and you want to change *everywhere* because you have some completely
broken Cygwin install.

Just as I said.

How did you even get that install? Is it a known issue? Some ancient
now-fixed bug?

It is very likely. Or probably just a one installation problem: the
problem is not consistently everywhere here. Some installations
work (if slow).

What version of Cygwin / gettext etc.

No idea. The person or persons who did this to me have no idea either.

Now I'm not saying that we shouldn't fix this, I just don't think that
this is the right way to go about it.

And I agree.

But in summary: We shouldn't be *always* using fallback functions
whether they're the C stuff in compat/* or the gettext fallbacks in
git-sh-i18n.sh just because there's some version out there of the
system-supplied functions that's broken.

It makes sense to prefer the system functions by default in both
cases, but when the OS one can be broken or lacking we can just add
probes or Makefile options like we do for fnmatch() with the
NO_FNMATCH_CASEFOLD switch.

Yes, and I personally shall welcome a chance to insult the local IT
by suggesting BROKEN_SH_GETTEXT. Not that they get the point...

Some systems have gettext.sh (GNU gettext) installed, but it is either broken
or misconfigured in such a way so its output is not usable.
For instance, on this particular system, a Cygwin installations gettext
produces no output whatsoever.

In case the users of these systems are unable or not interested in fixing
them, setting the new Makefile switch should help:

    USE_FALLTHROUGH_GETTEXT_SCHEME=yes

This will replace the translation routines with fallthrough versions, which
currently used only for regression testing.

Signed-off-by: Alex Riesen <redacted>
---

Alex Riesen, Thu, Jan 19, 2012 10:13:20 +0100:

On Thu, Jan 19, 2012 at 00:18, Ævar Arnfjörð Bjarmason [off-list ref] wrote:

quoted

It makes sense to prefer the system functions by default in both
cases, but when the OS one can be broken or lacking we can just add
probes or Makefile options like we do for fnmatch() with the
NO_FNMATCH_CASEFOLD switch.

Yes, and I personally shall welcome a chance to insult the local IT
by suggesting BROKEN_SH_GETTEXT. Not that they get the point...

I believe this patch does just that. It is certainly enough for my purposes.
The copy-paste error noticed by Jonathan is also fixed, thanks!
I didn't add the tracking of the switch in GIT-BUILD-OPTIONS: didn't found
how to do it quickly enough in this time of evening, and gave up, thinking
that no one sane would need to set the option anyway. So at the moment a
"make clean" needed when changing it.

 Makefile       |    4 ++
 git-sh-i18n.sh |  102 +++++++++++++++++++++++++++-----------------------------
 2 files changed, 53 insertions(+), 53 deletions(-)

diff --git a/Makefile b/Makefile
index a782409..806d126 100644
--- a/Makefile
+++ b/Makefile

@@ -47,6 +47,9 @@ all::
 # A translated Git requires GNU libintl or another gettext implementation,
 # plus libintl-perl at runtime.
 #
+# Define USE_FALLTHROUGH_GETTEXT_SCHEME, if you don't want to trust the
+# installed gettext translation of the shell scripts output.
+#
 # Define HAVE_LIBCHARSET_H if you haven't set NO_GETTEXT and you can't
 # trust the langinfo.h's nl_langinfo(CODESET) function to return the
 # current character set. GNU and Solaris have a nl_langinfo(CODESET),

@@ -1887,6 +1890,7 @@ sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
     -e 's/@@GIT_VERSION@@/$(GIT_VERSION)/g' \
     -e 's|@@LOCALEDIR@@|$(localedir_SQ)|g' \
     -e 's/@@NO_CURL@@/$(NO_CURL)/g' \
+    -e 's/@@USE_FALLTHROUGH_GETTEXT_SCHEME@@/$(USE_FALLTHROUGH_GETTEXT_SCHEME)/g' \
     -e $(BROKEN_PATH_FIX) \
     $@.sh >$@+
 endef

diff --git a/git-sh-i18n.sh b/git-sh-i18n.sh
index b4575fb..da8b214 100644
--- a/git-sh-i18n.sh
+++ b/git-sh-i18n.sh

@@ -16,61 +16,44 @@ else
 fi
 export TEXTDOMAINDIR
 
-if test -z "$GIT_GETTEXT_POISON"
+GIT_INTERNAL_GETTEXT_SH_SCHEME=fallthrough
+if test -n "@@USE_FALLTHROUGH_GETTEXT_SCHEME@@$GIT_INTERNAL_GETTEXT_TEST_FALLBACKS"
+then
+	: no probing necessary
+elif test -n "$GIT_GETTEXT_POISON"
 then
-	if test -z "$GIT_INTERNAL_GETTEXT_TEST_FALLBACKS" && type gettext.sh >/dev/null 2>&1
-	then
-		# This is GNU libintl's gettext.sh, we don't need to do anything
-		# else than setting up the environment and loading gettext.sh
-		GIT_INTERNAL_GETTEXT_SH_SCHEME=gnu
-		export GIT_INTERNAL_GETTEXT_SH_SCHEME
-
-		# Try to use libintl's gettext.sh, or fall back to English if we
-		# can't.
-		. gettext.sh
-
-	elif test -z "$GIT_INTERNAL_GETTEXT_TEST_FALLBACKS" && test "$(gettext -h 2>&1)" = "-h"
-	then
-		# We don't have gettext.sh, but there's a gettext binary in our
-		# path. This is probably Solaris or something like it which has a
-		# gettext implementation that isn't GNU libintl.
-		GIT_INTERNAL_GETTEXT_SH_SCHEME=solaris
-		export GIT_INTERNAL_GETTEXT_SH_SCHEME
-
-		# Solaris has a gettext(1) but no eval_gettext(1)
-		eval_gettext () {
-			gettext "$1" | (
-				export PATH $(git sh-i18n--envsubst --variables "$1");
-				git sh-i18n--envsubst "$1"
-			)
-		}
-
-	else
-		# Since gettext.sh isn't available we'll have to define our own
-		# dummy pass-through functions.
-
-		# Tell our tests that we don't have the real gettext.sh
-		GIT_INTERNAL_GETTEXT_SH_SCHEME=fallthrough
-		export GIT_INTERNAL_GETTEXT_SH_SCHEME
-
-		gettext () {
-			printf "%s" "$1"
-		}
-
-		eval_gettext () {
-			printf "%s" "$1" | (
-				export PATH $(git sh-i18n--envsubst --variables "$1");
-				git sh-i18n--envsubst "$1"
-			)
-		}
-	fi
-else
-	# Emit garbage under GETTEXT_POISON=YesPlease. Unlike the C tests
-	# this relies on an environment variable
-
 	GIT_INTERNAL_GETTEXT_SH_SCHEME=poison
-	export GIT_INTERNAL_GETTEXT_SH_SCHEME
+elif type gettext.sh >/dev/null 2>&1
+then
+	# This is GNU libintl's gettext.sh, we don't need to do anything
+	# else than setting up the environment and loading gettext.sh
+	GIT_INTERNAL_GETTEXT_SH_SCHEME=gnu
+elif test "$(gettext -h 2>&1)" = "-h"
+then
+	# We don't have gettext.sh, but there's a gettext binary in our
+	# path. This is probably Solaris or something like it which has a
+	# gettext implementation that isn't GNU libintl.
+	GIT_INTERNAL_GETTEXT_SH_SCHEME=solaris
+fi
+export GIT_INTERNAL_GETTEXT_SH_SCHEME
 
+case "$GIT_INTERNAL_GETTEXT_SH_SCHEME" in
+gnu)
+	# Try to use libintl's gettext.sh, or fall back to English if we
+	# can't.
+	. gettext.sh
+	;;
+solaris)
+	# Solaris has a gettext(1) but no eval_gettext(1)
+	eval_gettext () {
+		gettext "$1" | (
+			export PATH $(git sh-i18n--envsubst --variables "$1");
+			git sh-i18n--envsubst "$1"
+		)
+	}
+	;;
+poison)
+	# Used in tests
 	gettext () {
 		printf "%s" "# GETTEXT POISON #"
 	}

@@ -78,7 +61,20 @@ else
 	eval_gettext () {
 		printf "%s" "# GETTEXT POISON #"
 	}
-fi
+	;;
+*)
+	gettext () {
+		printf "%s" "$1"
+	}
+
+	eval_gettext () {
+		printf "%s" "$1" | (
+			export PATH $(git sh-i18n--envsubst --variables "$1");
+			git sh-i18n--envsubst "$1"
+		)
+	}
+	;;
+esac
 
 # Git-specific wrapper functions
 gettextln () {

-- 
1.7.9.rc1.92.ga90a1