Buglet in i18n?

Buglet in i18n?

[Johannes Sixt]

I just noticed these message after a 'git am' invocation:

When you have resolved this problem run gitam--resolved.
If you would prefer to skip this patch, instead run gitam--skip.
To restore the original branch and stop patching run gitam--abort.

Notice the missing blanks in the suggested commands.

This is on Windows. I have ab/i18n (a102b434c) merged, but compiled with
NO_GETTEXT.

-- Hannes

Re: Buglet in i18n?

[Ævar Arnfjörð Bjarmason]

On Fri, Oct 22, 2010 at 09:18, Johannes Sixt <j.sixt@viscovery.net> wrote:
> I just noticed these message after a 'git am' invocation:
>
> When you have resolved this problem run gitam--resolved.
> If you would prefer to skip this patch, instead run gitam--skip.
> To restore the original branch and stop patching run gitam--abort.
>
> Notice the missing blanks in the suggested commands.
>
> This is on Windows. I have ab/i18n (a102b434c) merged, but compiled with
> NO_GETTEXT.

This is the message in the code:

    eval_gettext "When you have resolved this problem run \"\$cmdline
--resolved\".
If you would prefer to skip this patch, instead run \"\$cmdline
--skip\".
To restore the original branch and stop patching run \"\$cmdline
--abort\"."; echo

And presumably you're using these functions from git-sh-i18n.sh:

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

        eval_gettext () {
            gettext_eval="printf '%s' \"$1\""
            printf "%s" "`eval \"$gettext_eval\"`"
        }

So maybe the shell on Windows doesn't behave the same way wih regards
to eval_gettext()?

Re: Buglet in i18n?

[Jonathan Nieder]

Ævar Arnfjörð Bjarmason wrote:

>         gettext () {
>             printf "%s" "$1"
>         }
> 
>         eval_gettext () {
>             gettext_eval="printf '%s' \"$1\""
>             printf "%s" "`eval \"$gettext_eval\"`"
>         }

This looks wrong.  Consider a simplified example:

	eval_gettext 'foo "bar baz"'

Now eval_gettext is supposed to just interpolate $variable
substitutions, right?  In particular, the quotation marks
ought to be preserved.

But instead, what gets evaluated is:

	printf '%s' "foo "bar baz""

which splits as

	printf '%s' 'foo bar' 'baz'

which is equivalent to

	printf '%s' 'foo bar'
	printf '%s' 'baz'

with output

	foo barbaz

Maybe something like this would do it?

	gettext_eval=$(
		printf '%s\n' "$1" |
		sed '
			s/[`\\"]/\\&/g
			1 s/^/printf "%s" "/
			$ s/$/"/
		'
	) &&
	eval "$gettext_eval"

Re: Buglet in i18n?

[Johannes Sixt]

Am 10/22/2010 10:20, schrieb Ævar Arnfjörð Bjarmason:
> So maybe the shell on Windows doesn't behave the same way wih regards
> to eval_gettext()?

It's the gettext fallbacks that do not work. Try this on Linux:

  GIT_INTERNAL_GETTEXT_TEST_FALLBACKS=t ./git-am -3 some-mbox

(with some-mbox that has a conflicting patch).

-- Hannes

Re: Buglet in i18n?

[Ævar Arnfjörð Bjarmason]

On Fri, Oct 22, 2010 at 08:34, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Ævar Arnfjörð Bjarmason wrote:
>
>>         gettext () {
>>             printf "%s" "$1"
>>         }
>>
>>         eval_gettext () {
>>             gettext_eval="printf '%s' \"$1\""
>>             printf "%s" "`eval \"$gettext_eval\"`"
>>         }
>
> This looks wrong.  Consider a simplified example:
>
>        eval_gettext 'foo "bar baz"'
>
> Now eval_gettext is supposed to just interpolate $variable
> substitutions, right?  In particular, the quotation marks
> ought to be preserved.
>
> But instead, what gets evaluated is:
>
>        printf '%s' "foo "bar baz""
>
> which splits as
>
>        printf '%s' 'foo bar' 'baz'
>
> which is equivalent to
>
>        printf '%s' 'foo bar'
>        printf '%s' 'baz'
>
> with output
>
>        foo barbaz

Indeed. It's a bug.

> Maybe something like this would do it?
>
>        gettext_eval=$(
>                printf '%s\n' "$1" |
>                sed '
>                        s/[`\\"]/\\&/g
>                        1 s/^/printf "%s" "/
>                        $ s/$/"/
>                '
>        ) &&
>        eval "$gettext_eval"

That prints:

    foo "bar baz"

(with double quotes)

But what we want is:

    foo bar baz

But what we're getting is:

    foo barbaz

Have I got that right, sorry, not thinking clearly right now :)

Re: Buglet in i18n?

[Ævar Arnfjörð Bjarmason]

On Sat, Oct 23, 2010 at 18:29, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Ævar Arnfjörð Bjarmason wrote:
>> On Fri, Oct 22, 2010 at 08:34, Jonathan Nieder <jrnieder@gmail.com> wrote:
>
>>> Now eval_gettext is supposed to just interpolate $variable
>>> substitutions, right?  In particular, the quotation marks
>>> ought to be preserved.
> [...]
>> That prints:
>>
>>     foo "bar baz"
>>
>> (with double quotes)
>>
>> But what we want is:
>>
>>     foo bar baz
> [...]
>> Have I got that right
>
> No, I don't think so.  Checking /usr/bin/gettext.sh, I see that it
> uses envsubst:
>
>        # Note: This use of envsubst is much safer than using the shell built-in 'eval'
>        # would be.
>        # 1) The security problem with Chinese translations that happen to use a
>        #    character such as \xe0\x60 is avoided.
>        # 2) The security problem with malevolent translators who put in command lists
>        #    like "$(...)" or "`...`" is avoided.
>        # 3) The translations can only refer to shell variables that are already
>        #    mentioned in MSGID or MSGID-PLURAL.
>
> And:
>
>        ; echo '"foo"' | envsubst
>        "foo"
>
> envsubst(1) has more details.
>
> The idea: translators do not have to worry about quoting at all.
> $var is presumably rare enough in messages as to not matter.
>
> One problem with my mockup: it makes it hard to talk about $5.00
> solutions, unlike envsubst:
>
>        ; echo '$3.00' | envsubst
>        $3.00

Sorry for the late reply. Yes, envsubst is the way to go. What I'm
going to do when I get around to it is to pull (the GPLv2 version of)
envsubst out of gettext.git and modify it to be a minimal
git-sh-i18n--helper command.

Then just do:

    diff --git a/git-sh-i18n.sh b/git-sh-i18n.sh
    index f8dd43a..c65f9ec 100644
    --- a/git-sh-i18n.sh
    +++ b/git-sh-i18n.sh
    @@ -55,8 +55,7 @@ then
                    }

                    eval_gettext () {
    -                       gettext_eval="printf '%s' \"$1\""
    -                       printf "%s" "`eval \"$gettext_eval\"`"
    +                       printf "%s" "$1" | git-sh-i18n--helper envsubst
                    }
            fi
     else

Along with this test:

    commit 42f2eabad4434875f3dd123844461ccfc4ad220b
    Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
    Date:   Sat Oct 30 08:59:51 2010 +0000

        t/t0201-gettext-fallbacks.sh: test for broken eval_gettext

        Add a test for the broken eval_gettext() variable interpolation
        behavior.

        Reported-by: Johannes Sixt <j.sixt@viscovery.net>
        Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>

    diff --git a/t/t0201-gettext-fallbacks.sh b/t/t0201-gettext-fallbacks.sh
    index 7a85d9b..682c602 100755
    --- a/t/t0201-gettext-fallbacks.sh
    +++ b/t/t0201-gettext-fallbacks.sh
    @@ -46,4 +46,27 @@ test_expect_success NO_GETTEXT_POISON
'eval_gettext: our eval_gettext() fallback
         test_cmp expect actual
     '

    +test_expect_success NO_GETTEXT_POISON 'eval_gettext: our
eval_gettext() fallback can interpolate whitespace variables' '
    +    git_am_cmdline="git am" &&
    +    export git_am_cmdline &&
    +    printf "test git am" >expect &&
    +    eval_gettext "test \$git_am_cmdline" >actual &&
    +    test_cmp expect actual
    +'
    +
    +test_expect_success NO_GETTEXT_POISON 'eval_gettext: git am $cmdline bug' '
    +    cmdline="git am -3" &&
    +    export cmdline &&
    +    cat >expect <<EOF &&
    +When you have resolved this problem run "git am -3 --resolved".
    +If you would prefer to skip this patch, instead run "git am -3 --skip".
    +To restore the original branch and stop patching run "git am -3 --abort".
    +EOF
    +    eval_gettext "When you have resolved this problem run
\"\$cmdline --resolved\".
    +If you would prefer to skip this patch, instead run \"\$cmdline --skip\".
    +To restore the original branch and stop patching run \"\$cmdline
--abort\"." >actual &&
    +    echo >>actual &&
    +    test_cmp expect actual
    +'
    +
     test_done

The latter of which starts passing with envsubst.

[RFC/PATCH 0/5] ab/i18n: Things I'll add in the next iteration

[Ævar Arnfjörð Bjarmason]

This is an RFC for things I'll add in the next iteration of ab/i18n:

Ævar Arnfjörð Bjarmason (5):
  gettext: fix bug in git-sh-i18n's eval_gettext() by using envsubst(1)

This solves the "Buglet in i18n?" issue reported by Johannes Sixt.

  gettextize: git-clone: !fixup "basic messages"
  gettextize: git-init: !fixup "basic messages"
  gettextize: git-revert: !fixup "Your local changes"
  gettextize: git-merge: !fixup "basic messages"

These are all minor fixups to the gettextize portion of the series
that I'll squash into exisitng patches.

 .gitignore                   |    1 +
 Makefile                     |    1 +
 builtin/clone.c              |    2 +-
 builtin/init-db.c            |    2 +-
 builtin/merge.c              |    2 +-
 builtin/revert.c             |    4 +-
 git-sh-i18n.sh               |    7 +-
 sh-i18n--envsubst.c          |  303 ++++++++++++++++++++++++++++++++++++++++++
 t/lib-gettext.sh             |   25 ++++
 t/t0200-gettext-basic.sh     |    3 +
 t/t0201-gettext-fallbacks.sh |    3 +
 11 files changed, 343 insertions(+), 10 deletions(-)
 create mode 100644 sh-i18n--envsubst.c

-- 
1.7.3.2.312.ge13a7

[RFC/PATCH 1/5] gettext: fix bug in git-sh-i18n's eval_gettext() by using envsubst(1)

[Ævar Arnfjörð Bjarmason]

Change eval_gettext(1) in git-sh-i18n.sh to use a modified version of
gettext's envsubst(1) program, instead of using a clever (but broken)
printf + eval + printf trick.

Our previous fallback would incorrectly handle cases where the
variable being interpolated contained spaces. E.g.:

    cmd="git foo"; eval_gettext "command: \$cmd"

Would emit "command: gitfoo", instead of the correct "command: git
foo". This happened with a message in git-am.sh that used the $cmdline
variable.

To work around this, and to improve our variable expansion behavior
(eval has security issues) I've imported a stripped-down version of
gettext's envsubst(1) program.

Using it we pass the latter of the two tests added along with this
patch (the first one was just added for completeness).

Since we want to test both our fallback eval_gettext() and the one
we'll end up using (i.e. on Solaris) the new tests are executed in
both t0200-gettext-basic.sh and t0201-gettext-fallbacks.sh.

These are the modifications I made to envsubst.c as I turned it into
sh-i18n--envsubst.c:

 * Added our git-compat-util.h header for xrealloc() and friends.

 * Removed inclusion of gettext-specific headers.

 * Removed most of main() and replaced it with my own. The modified
   version doesn't do option parsing at all, because it doesn't need
   to.

 * Modified error() invocations to use our error() instead of
   error(3).

 * Replaced the gettext XNMALLOC(n, size) macro with just
   xmalloc(n). Since XNMALLOC() only allocated char's.

 * Removed functions made redundant since I deleted some code paths
   from main(). These were:

    * print_variables
    * note_variables
    * print_variable
    * string_list_init
    * cmp_string
    * string_list_sort
    * string_list_destroy
    * find_variables
    * note_variable
    * note_variables

 * Replaced the use of stdbool.h (a C99 header) by doing the following
   replacements on the code:

    * s/bool/unsigned short int/g
    * s/true/1/g
    * s/false/0/g

Reported-by: Johannes Sixt <j.sixt@viscovery.net>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 .gitignore                   |    1 +
 Makefile                     |    1 +
 git-sh-i18n.sh               |    7 +-
 sh-i18n--envsubst.c          |  303 ++++++++++++++++++++++++++++++++++++++++++
 t/lib-gettext.sh             |   25 ++++
 t/t0200-gettext-basic.sh     |    3 +
 t/t0201-gettext-fallbacks.sh |    3 +
 7 files changed, 338 insertions(+), 5 deletions(-)
 create mode 100644 sh-i18n--envsubst.c

diff --git a/.gitignore b/.gitignore
index 80ca718..d2d9ec2 100644
--- a/.gitignore
+++ b/.gitignore
@@ -126,6 +126,7 @@
 /git-send-email
 /git-send-pack
 /git-sh-i18n
+/git-sh-i18n--envsubst
 /git-sh-setup
 /git-shell
 /git-shortlog
diff --git a/Makefile b/Makefile
index e1650ba..c55baa6 100644
--- a/Makefile
+++ b/Makefile
@@ -435,6 +435,7 @@ PROGRAM_OBJS += shell.o
 PROGRAM_OBJS += show-index.o
 PROGRAM_OBJS += upload-pack.o
 PROGRAM_OBJS += http-backend.o
+PROGRAM_OBJS += sh-i18n--envsubst.o
 
 PROGRAMS += $(patsubst %.o,git-%$X,$(PROGRAM_OBJS))
 
diff --git a/git-sh-i18n.sh b/git-sh-i18n.sh
index f8dd43a..41580a8 100644
--- a/git-sh-i18n.sh
+++ b/git-sh-i18n.sh
@@ -38,9 +38,7 @@ then
 
 		# Solaris has a gettext(1) but no eval_gettext(1)
 		eval_gettext () {
-			gettext_out=$(gettext "$1")
-			gettext_eval="printf '%s' \"$gettext_out\""
-			printf "%s" "`eval \"$gettext_eval\"`"
+			gettext "$1" | git sh-i18n--envsubst
 		}
 	else
 		# Since gettext.sh isn't available we'll have to define our own
@@ -55,8 +53,7 @@ then
 		}
 
 		eval_gettext () {
-			gettext_eval="printf '%s' \"$1\""
-			printf "%s" "`eval \"$gettext_eval\"`"
+			printf "%s" "$1" | git sh-i18n--envsubst
 		}
 	fi
 else
diff --git a/sh-i18n--envsubst.c b/sh-i18n--envsubst.c
new file mode 100644
index 0000000..d0e0cc9
--- /dev/null
+++ b/sh-i18n--envsubst.c
@@ -0,0 +1,303 @@
+/*
+ * sh-i18n--envsubst.c - a stripped-down version of gettext's envsubst(1)
+ *
+ * Copyright (C) 2010 Ævar Arnfjörð Bjarmason
+ *
+ * This is a modified version of
+ * 67d0871a8c:gettext-runtime/src/envsubst.c from the gettext.git
+ * repository. It has been stripped down to only implement the
+ * envsubst(1) features that we need in the git-sh-i18n fallbacks.
+ *
+ * The "Close standard error" part in main() is from
+ * 8dac033df0:gnulib-local/lib/closeout.c. The copyright notices for
+ * both files are reproduced immediately below.
+ */
+
+#include "git-compat-util.h"
+
+/* Substitution of environment variables in shell format strings.
+   Copyright (C) 2003-2007 Free Software Foundation, Inc.
+   Written by Bruno Haible <bruno@clisp.org>, 2003.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 2, or (at your option)
+   any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program; if not, write to the Free Software Foundation,
+   Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.  */
+
+/* closeout.c - close standard output and standard error
+   Copyright (C) 1998-2007 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 2, or (at your option)
+   any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program; if not, write to the Free Software Foundation,
+   Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.  */
+
+#include <errno.h>
+#include <getopt.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+
+/* If true, substitution shall be performed on all variables.  */
+static unsigned short int all_variables;
+
+static void subst_from_stdin (void);
+
+int
+main (int argc, char *argv[])
+{
+  all_variables = 1;
+
+  if (argc > 1)
+	{
+	  error ("too many arguments");
+	  exit (EXIT_FAILURE);
+	}
+
+  subst_from_stdin ();
+
+  /* Close standard error.  This is simpler than fwriteerror_no_ebadf, because
+     upon failure we don't need an errno - all we can do at this point is to
+     set an exit status.  */
+  errno = 0;
+  if (ferror (stderr) || fflush (stderr))
+    { 
+      fclose (stderr);
+      exit (EXIT_FAILURE);
+    }
+  if (fclose (stderr) && errno != EBADF)
+    exit (EXIT_FAILURE);
+
+  exit (EXIT_SUCCESS);
+}
+
+/* Type describing list of immutable strings,
+   implemented using a dynamic array.  */
+typedef struct string_list_ty string_list_ty;
+struct string_list_ty
+{
+  const char **item;
+  size_t nitems;
+  size_t nitems_max;
+};
+
+/* Append a single string to the end of a list of strings.  */
+static inline void
+string_list_append (string_list_ty *slp, const char *s)
+{
+  /* Grow the list.  */
+  if (slp->nitems >= slp->nitems_max)
+    {
+      size_t nbytes;
+
+      slp->nitems_max = slp->nitems_max * 2 + 4;
+      nbytes = slp->nitems_max * sizeof (slp->item[0]);
+      slp->item = (const char **) xrealloc (slp->item, nbytes);
+    }
+
+  /* Add the string to the end of the list.  */
+  slp->item[slp->nitems++] = s;
+}
+
+/* Test whether a string list contains a given string.  */
+static inline int
+string_list_member (const string_list_ty *slp, const char *s)
+{
+  size_t j;
+
+  for (j = 0; j < slp->nitems; ++j)
+    if (strcmp (slp->item[j], s) == 0)
+      return 1;
+  return 0;
+}
+
+/* Test whether a sorted string list contains a given string.  */
+static int
+sorted_string_list_member (const string_list_ty *slp, const char *s)
+{
+  size_t j1, j2;
+
+  j1 = 0;
+  j2 = slp->nitems;
+  if (j2 > 0)
+    {
+      /* Binary search.  */
+      while (j2 - j1 > 1)
+	{
+	  /* Here we know that if s is in the list, it is at an index j
+	     with j1 <= j < j2.  */
+	  size_t j = (j1 + j2) >> 1;
+	  int result = strcmp (slp->item[j], s);
+
+	  if (result > 0)
+	    j2 = j;
+	  else if (result == 0)
+	    return 1;
+	  else
+	    j1 = j + 1;
+	}
+      if (j2 > j1)
+	if (strcmp (slp->item[j1], s) == 0)
+	  return 1;
+    }
+  return 0;
+}
+
+
+/* Set of variables on which to perform substitution.
+   Used only if !all_variables.  */
+static string_list_ty variables_set;
+
+static int
+do_getc ()
+{
+  int c = getc (stdin);
+
+  if (c == EOF)
+    {
+      if (ferror (stdin))
+	error ("error while reading standard input");
+    }
+
+  return c;
+}
+
+static inline void
+do_ungetc (int c)
+{
+  if (c != EOF)
+    ungetc (c, stdin);
+}
+
+/* Copies stdin to stdout, performing substitutions.  */
+static void
+subst_from_stdin ()
+{
+  static char *buffer;
+  static size_t bufmax;
+  static size_t buflen;
+  int c;
+
+  for (;;)
+    {
+      c = do_getc ();
+      if (c == EOF)
+	break;
+      /* Look for $VARIABLE or ${VARIABLE}.  */
+      if (c == '$')
+	{
+	  unsigned short int opening_brace = 0;
+	  unsigned short int closing_brace = 0;
+
+	  c = do_getc ();
+	  if (c == '{')
+	    {
+	      opening_brace = 1;
+	      c = do_getc ();
+	    }
+	  if ((c >= 'A' && c <= 'Z') || (c >= 'a' && c <= 'z') || c == '_')
+	    {
+	      unsigned short int valid;
+
+	      /* Accumulate the VARIABLE in buffer.  */
+	      buflen = 0;
+	      do
+		{
+		  if (buflen >= bufmax)
+		    {
+		      bufmax = 2 * bufmax + 10;
+		      buffer = xrealloc (buffer, bufmax);
+		    }
+		  buffer[buflen++] = c;
+
+		  c = do_getc ();
+		}
+	      while ((c >= 'A' && c <= 'Z') || (c >= 'a' && c <= 'z')
+		     || (c >= '0' && c <= '9') || c == '_');
+
+	      if (opening_brace)
+		{
+		  if (c == '}')
+		    {
+		      closing_brace = 1;
+		      valid = 1;
+		    }
+		  else
+		    {
+		      valid = 0;
+		      do_ungetc (c);
+		    }
+		}
+	      else
+		{
+		  valid = 1;
+		  do_ungetc (c);
+		}
+
+	      if (valid)
+		{
+		  /* Terminate the variable in the buffer.  */
+		  if (buflen >= bufmax)
+		    {
+		      bufmax = 2 * bufmax + 10;
+		      buffer = xrealloc (buffer, bufmax);
+		    }
+		  buffer[buflen] = '\0';
+
+		  /* Test whether the variable shall be substituted.  */
+		  if (!all_variables
+		      && !sorted_string_list_member (&variables_set, buffer))
+		    valid = 0;
+		}
+
+	      if (valid)
+		{
+		  /* Substitute the variable's value from the environment.  */
+		  const char *env_value = getenv (buffer);
+
+		  if (env_value != NULL)
+		    fputs (env_value, stdout);
+		}
+	      else
+		{
+		  /* Perform no substitution at all.  Since the buffered input
+		     contains no other '$' than at the start, we can just
+		     output all the buffered contents.  */
+		  putchar ('$');
+		  if (opening_brace)
+		    putchar ('{');
+		  fwrite (buffer, buflen, 1, stdout);
+		  if (closing_brace)
+		    putchar ('}');
+		}
+	    }
+	  else
+	    {
+	      do_ungetc (c);
+	      putchar ('$');
+	      if (opening_brace)
+		putchar ('{');
+	    }
+	}
+      else
+	putchar (c);
+    }
+}
diff --git a/t/lib-gettext.sh b/t/lib-gettext.sh
index c9a079e..436192a 100644
--- a/t/lib-gettext.sh
+++ b/t/lib-gettext.sh
@@ -66,3 +66,28 @@ else
 		say "# lib-gettext: No GETTEXT support available"
 	fi
 fi
+
+test_eval_gettext_interpolation() {
+	test_expect_success NO_GETTEXT_POISON 'eval_gettext: our eval_gettext() fallback can interpolate whitespace variables' '
+	    git_am_cmdline="git am" &&
+	    export git_am_cmdline &&
+	    printf "test git am" >expect &&
+	    eval_gettext "test \$git_am_cmdline" >actual &&
+	    test_cmp expect actual
+	'
+
+	test_expect_success NO_GETTEXT_POISON 'eval_gettext: git am $cmdline bug' '
+	    cmdline="git am -3" &&
+	    export cmdline &&
+	    cat >expect <<EOF &&
+When you have resolved this problem run "git am -3 --resolved".
+If you would prefer to skip this patch, instead run "git am -3 --skip".
+To restore the original branch and stop patching run "git am -3 --abort".
+EOF
+	    eval_gettext "When you have resolved this problem run \"\$cmdline --resolved\".
+If you would prefer to skip this patch, instead run \"\$cmdline --skip\".
+To restore the original branch and stop patching run \"\$cmdline --abort\"." >actual &&
+	    echo >>actual &&
+	    test_cmp expect actual
+	'
+}
diff --git a/t/t0200-gettext-basic.sh b/t/t0200-gettext-basic.sh
index 8853d8a..58948fa 100755
--- a/t/t0200-gettext-basic.sh
+++ b/t/t0200-gettext-basic.sh
@@ -105,4 +105,7 @@ test_expect_success GETTEXT_LOCALE 'sanity: Some gettext("") data for real local
     test -s real-locale
 '
 
+# Test eval_gettext() interpolation with the actual eval_gettext function
+test_eval_gettext_interpolation
+
 test_done
diff --git a/t/t0201-gettext-fallbacks.sh b/t/t0201-gettext-fallbacks.sh
index 7a85d9b..6d18e21 100755
--- a/t/t0201-gettext-fallbacks.sh
+++ b/t/t0201-gettext-fallbacks.sh
@@ -46,4 +46,7 @@ test_expect_success NO_GETTEXT_POISON 'eval_gettext: our eval_gettext() fallback
     test_cmp expect actual
 '
 
+# Test eval_gettext() interpolation with the fallback eval_gettext function
+test_eval_gettext_interpolation
+
 test_done
-- 
1.7.3.2.312.ge13a7

[RFC/PATCH 2/5] gettextize: git-clone: !fixup "basic messages"

[Ævar Arnfjörð Bjarmason]

Gettextize the "--depth is ignored in local clones" message added in
v1.7.3-rc0~8^2 by Nguyễn Thái Ngọc Duy.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/clone.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index de23ba3..2c5d492 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -415,7 +415,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 		repo = repo_name;
 	is_local = path && !is_bundle;
 	if (is_local && option_depth)
-		warning("--depth is ignored in local clones; use file:// instead.");
+		warning(_("--depth is ignored in local clones; use file:// instead."));
 
 	if (argc == 2)
 		dir = xstrdup(argv[1]);
-- 
1.7.3.2.312.ge13a7

[RFC/PATCH 3/5] gettextize: git-init: !fixup "basic messages"

[Ævar Arnfjörð Bjarmason]

Gettextize the "oops" die message in builtin/init-db.c

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/init-db.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/builtin/init-db.c b/builtin/init-db.c
index 28e20f9..2d6132a 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -346,7 +346,7 @@ int init_db(const char *template_dir, unsigned int flags)
 		else if (shared_repository == PERM_EVERYBODY)
 			sprintf(buf, "%d", OLD_PERM_EVERYBODY);
 		else
-			die("oops");
+			die(_("oops"));
 		git_config_set("core.sharedrepository", buf);
 		git_config_set("receive.denyNonFastforwards", "true");
 	}
-- 
1.7.3.2.312.ge13a7

[RFC/PATCH 4/5] gettextize: git-revert: !fixup "Your local changes"

[Ævar Arnfjörð Bjarmason]

Gettextize two messages that I missed in 'gettextize: git-revert "Your
local changes" message'.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/revert.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index c8463d2..178b8a0 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -294,9 +294,9 @@ static NORETURN void die_dirty_index(const char *me)
 					  "Please, commit your changes or stash them to proceed."));
 		} else {
 			if (action == REVERT)
-				die("Your local changes would be overwritten by revert.\n");
+				die(_("Your local changes would be overwritten by revert.\n"));
 			else
-				die("Your local changes would be overwritten by cherry-pick.\n");
+				die(_("Your local changes would be overwritten by cherry-pick.\n"));
 		}
 	}
 }
-- 
1.7.3.2.312.ge13a7

[RFC/PATCH 5/5] gettextize: git-merge: !fixup "basic messages"

[Ævar Arnfjörð Bjarmason]

Gettextize a "%s: negative length %s" error message in builtin/merge.c
that I missed the first time around.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/merge.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index 62bdd6c..524ef22 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -511,7 +511,7 @@ static int git_merge_config(const char *k, const char *v, void *cb)
 		int is_bool;
 		shortlog_len = git_config_bool_or_int(k, v, &is_bool);
 		if (!is_bool && shortlog_len < 0)
-			return error("%s: negative length %s", k, v);
+			return error(_("%s: negative length %s"), k, v);
 		if (is_bool && shortlog_len)
 			shortlog_len = DEFAULT_MERGE_LOG_LEN;
 		return 0;
-- 
1.7.3.2.312.ge13a7

Re: [RFC/PATCH 1/5] gettext: fix bug in git-sh-i18n's eval_gettext() by using envsubst(1)

[Johannes Sixt]

Am 10/31/2010 12:34, schrieb Ævar Arnfjörð Bjarmason:
> Change eval_gettext(1) in git-sh-i18n.sh to use a modified version of
> gettext's envsubst(1) program, instead of using a clever (but broken)
> printf + eval + printf trick.
> 
> Our previous fallback would incorrectly handle cases where the
> variable being interpolated contained spaces. E.g.:
> 
>     cmd="git foo"; eval_gettext "command: \$cmd"
> 
> Would emit "command: gitfoo", instead of the correct "command: git
> foo". This happened with a message in git-am.sh that used the $cmdline
> variable.
> 
> To work around this, and to improve our variable expansion behavior
> (eval has security issues) I've imported a stripped-down version of
> gettext's envsubst(1) program.

Would this help the case mentioned above at all? To pass the value of
'cmd' to envsubst, you have to export it. But the code snippet above
doesn't do that.

Wouldn't it be much simpler to dodge variable substitutions in the
translated string entirely by rewriting such texts as (e.g.)

	gettext_printf "command: %s\n" "$cmd"

and use printf in the implementation. I don't know how compatible you can
make this with existing gettext implementations, though.

-- Hannes

Re: [RFC/PATCH 1/5] gettext: fix bug in git-sh-i18n's eval_gettext() by using envsubst(1)

[Ævar Arnfjörð Bjarmason]

On Tue, Nov 2, 2010 at 09:33, Johannes Sixt <j.sixt@viscovery.net> wrote:
> Am 10/31/2010 12:34, schrieb Ævar Arnfjörð Bjarmason:
>> Change eval_gettext(1) in git-sh-i18n.sh to use a modified version of
>> gettext's envsubst(1) program, instead of using a clever (but broken)
>> printf + eval + printf trick.
>>
>> Our previous fallback would incorrectly handle cases where the
>> variable being interpolated contained spaces. E.g.:
>>
>>     cmd="git foo"; eval_gettext "command: \$cmd"
>>
>> Would emit "command: gitfoo", instead of the correct "command: git
>> foo". This happened with a message in git-am.sh that used the $cmdline
>> variable.
>>
>> To work around this, and to improve our variable expansion behavior
>> (eval has security issues) I've imported a stripped-down version of
>> gettext's envsubst(1) program.
>
> Would this help the case mentioned above at all? To pass the value of
> 'cmd' to envsubst, you have to export it. But the code snippet above
> doesn't do that.

Right, I had a major brainfart. GNU gettext.sh actually works like:

    eval_gettext () {
      gettext "$1" | (export PATH `envsubst --variables "$1"`; envsubst "$1")
    }

So I need to re-make this patch to support --variables.

> Wouldn't it be much simpler to dodge variable substitutions in the
> translated string entirely by rewriting such texts as (e.g.)
>
>        gettext_printf "command: %s\n" "$cmd"
>
> and use printf in the implementation. I don't know how compatible you can
> make this with existing gettext implementations, though.

Using named variables is easier to use, and works drop-in with the GNU gettext
way of doing things. Once I implement --variables (or rather,
un-un-implement them)
it'll just work.

Thanks for the review, and for spotting this sillyness.

Re: [RFC/PATCH 1/5] gettext: fix bug in git-sh-i18n's eval_gettext() by using envsubst(1)

[Johannes Sixt]

Am 11/8/2010 23:39, schrieb Ævar Arnfjörð Bjarmason:
>     eval_gettext () {
>       gettext "$1" | (export PATH `envsubst --variables "$1"`; envsubst "$1")
>     }

So, for every message printed, you have at least 3 fork()s (usually even
more)! I'm not happy about that. You *must* avoid this at least for
NO_GETTEXT builds, but if you can reduce them even for no-NO_GETTEXT
builds, it would be great.

-- Hannes

Re: [RFC/PATCH 1/5] gettext: fix bug in git-sh-i18n's eval_gettext() by using envsubst(1)

[Ævar Arnfjörð Bjarmason]

On Tue, Nov 9, 2010 at 08:33, Johannes Sixt <j.sixt@viscovery.net> wrote:
> Am 11/8/2010 23:39, schrieb Ævar Arnfjörð Bjarmason:
>>     eval_gettext () {
>>       gettext "$1" | (export PATH `envsubst --variables "$1"`; envsubst "$1")
>>     }
>
> So, for every message printed, you have at least 3 fork()s (usually even
> more)! I'm not happy about that. You *must* avoid this at least for
> NO_GETTEXT builds, but if you can reduce them even for no-NO_GETTEXT
> builds, it would be great.

Why is that a "*must*"? For the GNU gettext versions (our will be
faster, if anything):

    $ time (for i in {1..1000}; do gettext "foobar"; done) >/dev/null
    real    0m3.219s
    user    0m0.253s
    sys     0m2.570s

    $ time (for i in {1..1000}; do eval_gettext "foobar"; done) >/dev/null
    real    0m12.615s
    user    0m1.264s
    sys     0m12.384s

So that's around 0.003 seconds and 0.01 seconds per message for
gettext() and eval_gettext() respectively.

I'm not indifferent to that slight cost, but (almost?) all of the
eval_gettext messages we have are just printing out an error message
before we die. None of them are inside a tight loop. This is the
typical use case:

    git-am.sh:    eval_gettext "When you have resolved this problem
run \"\$cmdline --resolved\".
    git-am.sh:                      clean_abort "$(eval_gettext "Patch
format \$patch_format is not supported.")"
    git-am.sh:      die "$(eval_gettext "previous rebase directory
\$dotest still exists but mbox given.")"
    git-am.sh:              die "$(eval_gettext "Dirty index: cannot
apply patches (dirty: \$files)")"
    git-am.sh:                      eval_gettext "Patch is empty.  Was
it split wrong?
    git-am.sh:      say "$(eval_gettext "Applying: \$FIRSTLINE")"
    git-am.sh:              eval_gettext 'Patch failed at $msgnum
$FIRSTLINE'; echo
    git-bisect.sh:                  die "$(eval_gettext "'\$arg' does
not appear to be a valid revision")"
    git-bisect.sh:          *)              die "$(eval_gettext "Bad
bisect_write argument: \$state")" ;;
    git-bisect.sh:                revs=$(git rev-list "$arg") || die
"$(eval_gettext "Bad rev input: \$arg")" ;;
    git-bisect.sh:                          die "$(eval_gettext "Bad
rev input: \$rev")"
    git-bisect.sh:         die "$(eval_gettext "'\$invalid' is not a
valid commit")"
    git-bisect.sh:  test -r "$file" || die "$(eval_gettext "cannot
read \$file for replaying")"
    git-bisect.sh:      eval_gettext "running \$command"; echo
    git-bisect.sh:    echo >&2 "$(eval_gettext "bisect run failed:
    git-bisect.sh:    echo >&2 "$(eval_gettext "bisect run failed:

The cost for that is going to be much less than the time we spend on
forking out to sed, grep and other similar utilities inside our shell
scripts. If eval_gettext() is slowing things down noticeably that's
probably a sign that we need to rewrite the script in C, not
micro-optimize the eval_gettext() implementation.

But maybe you have reason to think otherwise? I haven't noticed any
noticable slowdowns from doing it this way, but maybe I've been
looking at the wrong thing.

Re: [RFC/PATCH 1/5] gettext: fix bug in git-sh-i18n's eval_gettext() by using envsubst(1)

[Johannes Sixt]

Am 11/9/2010 10:35, schrieb Ævar Arnfjörð Bjarmason:
> On Tue, Nov 9, 2010 at 08:33, Johannes Sixt <j.sixt@viscovery.net> wrote:
>> Am 11/8/2010 23:39, schrieb Ævar Arnfjörð Bjarmason:
>>>     eval_gettext () {
>>>       gettext "$1" | (export PATH `envsubst --variables "$1"`; envsubst "$1")
>>>     }
>>
>> So, for every message printed, you have at least 3 fork()s (usually even
>> more)! I'm not happy about that. You *must* avoid this at least for
>> NO_GETTEXT builds, but if you can reduce them even for no-NO_GETTEXT
>> builds, it would be great.
> 
> Why is that a "*must*"?
...
> But maybe you have reason to think otherwise? I haven't noticed any
> noticable slowdowns from doing it this way, but maybe I've been
> looking at the wrong thing.

You didn't do your timings in Windows, did you? Every fork() that you can
avoid is a win.

-- Hannes

Re: [RFC/PATCH 1/5] gettext: fix bug in git-sh-i18n's eval_gettext() by using envsubst(1)

[Ævar Arnfjörð Bjarmason]

On Tue, Nov 9, 2010 at 10:47, Johannes Sixt <j.sixt@viscovery.net> wrote:
> Am 11/9/2010 10:35, schrieb Ævar Arnfjörð Bjarmason:
>> Why is that a "*must*"?
> ...
>> But maybe you have reason to think otherwise? I haven't noticed any
>> noticable slowdowns from doing it this way, but maybe I've been
>> looking at the wrong thing.
>
> You didn't do your timings in Windows, did you? Every fork() that you can
> avoid is a win.

What's the result of timing it on Windows?

Re: [RFC/PATCH 1/5] gettext: fix bug in git-sh-i18n's eval_gettext() by using envsubst(1)

[Johannes Sixt]

Am 11/9/2010 10:49, schrieb Ævar Arnfjörð Bjarmason:
> On Tue, Nov 9, 2010 at 10:47, Johannes Sixt <j.sixt@viscovery.net> wrote:
>> Am 11/9/2010 10:35, schrieb Ævar Arnfjörð Bjarmason:
>>> Why is that a "*must*"?
>> ...
>>> But maybe you have reason to think otherwise? I haven't noticed any
>>> noticable slowdowns from doing it this way, but maybe I've been
>>> looking at the wrong thing.
>>
>> You didn't do your timings in Windows, did you? Every fork() that you can
>> avoid is a win.
> 
> What's the result of timing it on Windows?

I do not have gettext, hence, I test 'git version' as a reference:

$ time (for i in {1..100}; do git version; done) > /dev/null

real    0m5.610s
user    0m1.707s
sys     0m0.712s

Then I tested this function. It is not exactly the same that you tested,
but it has the same number of subshells and builtin and external command
invocations:

eval_gettext ()
{
    gettext "$1" |
    ( : `git-sh-i18n--envsubst <<< "$1"`
      git-sh-i18n--envsubst <<< "$1"
    )
}

$ time (for i in {1..100}; do eval_gettext foobar; done) > /dev/null

real    0m20.578s
user    0m8.457s
sys     0m3.915s

Note that there are only 100 iterations, so we are talking about 0.2
seconds per eval_gettext call! That's an awful lot of time even for a
single error message.

-- Hannes

Re: [RFC/PATCH 1/5] gettext: fix bug in git-sh-i18n's eval_gettext() by using envsubst(1)

[Erik Faye-Lund]

On Tue, Nov 9, 2010 at 11:36 AM, Johannes Sixt <j.sixt@viscovery.net> wrote:
> Am 11/9/2010 10:49, schrieb Ævar Arnfjörð Bjarmason:
>> On Tue, Nov 9, 2010 at 10:47, Johannes Sixt <j.sixt@viscovery.net> wrote:
>>> Am 11/9/2010 10:35, schrieb Ævar Arnfjörð Bjarmason:
>>>> Why is that a "*must*"?
>>> ...
>>>> But maybe you have reason to think otherwise? I haven't noticed any
>>>> noticable slowdowns from doing it this way, but maybe I've been
>>>> looking at the wrong thing.
>>>
>>> You didn't do your timings in Windows, did you? Every fork() that you can
>>> avoid is a win.
>>
>> What's the result of timing it on Windows?
>
> I do not have gettext, hence, I test 'git version' as a reference:
>

Try this branch if you're interested:

git://repo.or.cz/msysgit/kusma.git work/gettext

Re: [RFC/PATCH 1/5] gettext: fix bug in git-sh-i18n's eval_gettext() by using envsubst(1)

[Ævar Arnfjörð Bjarmason]

On Tue, Nov 9, 2010 at 11:36, Johannes Sixt <j.sixt@viscovery.net> wrote:
> Am 11/9/2010 10:49, schrieb Ævar Arnfjörð Bjarmason:
>> On Tue, Nov 9, 2010 at 10:47, Johannes Sixt <j.sixt@viscovery.net> wrote:
>>> Am 11/9/2010 10:35, schrieb Ævar Arnfjörð Bjarmason:
>>>> Why is that a "*must*"?
>>> ...
>>>> But maybe you have reason to think otherwise? I haven't noticed any
>>>> noticable slowdowns from doing it this way, but maybe I've been
>>>> looking at the wrong thing.
>>>
>>> You didn't do your timings in Windows, did you? Every fork() that you can
>>> avoid is a win.
>>
>> What's the result of timing it on Windows?
>
> I do not have gettext, hence, I test 'git version' as a reference:
>
> $ time (for i in {1..100}; do git version; done) > /dev/null
>
> real    0m5.610s
> user    0m1.707s
> sys     0m0.712s
>
> Then I tested this function. It is not exactly the same that you tested,
> but it has the same number of subshells and builtin and external command
> invocations:
>
> eval_gettext ()
> {
>    gettext "$1" |
>    ( : `git-sh-i18n--envsubst <<< "$1"`
>      git-sh-i18n--envsubst <<< "$1"
>    )
> }
>
> $ time (for i in {1..100}; do eval_gettext foobar; done) > /dev/null
>
> real    0m20.578s
> user    0m8.457s
> sys     0m3.915s
>
> Note that there are only 100 iterations, so we are talking about 0.2
> seconds per eval_gettext call! That's an awful lot of time even for a
> single error message.

Thanks for elaborating. But just so I understand you correctly it's a
cost of invoking *any* program in shellscripts on Windows? So e.g. 10
sed calls would cost the same as 10 git-sh-i18n--envsubst calls (but
of course 5 eval_gettext() calls, since it calls git-sh-i18n--envsubst
twice).

So e.g. using eval_gettext once isn't a bigger problem than calling
some trivial sed substitution twice?

Re: [RFC/PATCH 1/5] gettext: fix bug in git-sh-i18n's eval_gettext() by using envsubst(1)

[Johannes Sixt]

Am 11/9/2010 11:52, schrieb Ævar Arnfjörð Bjarmason:
> On Tue, Nov 9, 2010 at 11:36, Johannes Sixt <j.sixt@viscovery.net> wrote:
>> $ time (for i in {1..100}; do git version; done) > /dev/null
>>
>> real    0m5.610s
...
>> $ time (for i in {1..100}; do eval_gettext foobar; done) > /dev/null
>>
>> real    0m20.578s
...
>> Note that there are only 100 iterations, so we are talking about 0.2
>> seconds per eval_gettext call! That's an awful lot of time even for a
>> single error message.
> 
> Thanks for elaborating. But just so I understand you correctly it's a
> cost of invoking *any* program in shellscripts on Windows? So e.g. 10
> sed calls would cost the same as 10 git-sh-i18n--envsubst calls (but
> of course 5 eval_gettext() calls, since it calls git-sh-i18n--envsubst
> twice).

An invocation of sed, cat, etc. (POSIX/MSYS tools) is cheaper by a factor
of 2 than a git invocation for a reason that I do not understand. (Perhaps
it has to do with the number of DLLs that are linked; git has 9 static
dependencies, MSYS tools only 3.)

eval_gettext involves 2 git invocation (git-sh-i18n--envsubst counts as
much as git) and 1 subshell in practice (some subshells are optimized away).

> So e.g. using eval_gettext once isn't a bigger problem than calling
> some trivial sed substitution twice?

Look at the timings: In my book, eval_gettext counts like about 8 sed
substitutions.

-- Hannes

Re: [RFC/PATCH 1/5] gettext: fix bug in git-sh-i18n's eval_gettext() by using envsubst(1)

[Ævar Arnfjörð Bjarmason]

On Tue, Nov 9, 2010 at 12:42, Johannes Sixt <j.sixt@viscovery.net> wrote:
> Am 11/9/2010 11:52, schrieb Ævar Arnfjörð Bjarmason:
>> On Tue, Nov 9, 2010 at 11:36, Johannes Sixt <j.sixt@viscovery.net> wrote:
>>> $ time (for i in {1..100}; do git version; done) > /dev/null
>>>
>>> real    0m5.610s
> ...
>>> $ time (for i in {1..100}; do eval_gettext foobar; done) > /dev/null
>>>
>>> real    0m20.578s
> ...
>>> Note that there are only 100 iterations, so we are talking about 0.2
>>> seconds per eval_gettext call! That's an awful lot of time even for a
>>> single error message.
>>
>> Thanks for elaborating. But just so I understand you correctly it's a
>> cost of invoking *any* program in shellscripts on Windows? So e.g. 10
>> sed calls would cost the same as 10 git-sh-i18n--envsubst calls (but
>> of course 5 eval_gettext() calls, since it calls git-sh-i18n--envsubst
>> twice).
>
> An invocation of sed, cat, etc. (POSIX/MSYS tools) is cheaper by a factor
> of 2 than a git invocation for a reason that I do not understand. (Perhaps
> it has to do with the number of DLLs that are linked; git has 9 static
> dependencies, MSYS tools only 3.)

git-sh-i18n--envsubst doesn't need to be linked to anything but the C
library.

If it is that's because I fat-fingered the Makefile part of the
patch. So it should be cheaper than cat/sed etc. if anything.

    $ ldd -r git-sh-i18n--envsubst
        linux-vdso.so.1 =>  (0x00007fff163c4000)
        libz.so.1 => /usr/lib/libz.so.1 (0x00007f742dea6000)
        libcrypto.so.0.9.8 => /usr/lib/libcrypto.so.0.9.8 (0x00007f742db05000)
        libpthread.so.0 => /lib/libpthread.so.0 (0x00007f742d8e8000)
        libc.so.6 => /lib/libc.so.6 (0x00007f742d587000)
        libdl.so.2 => /lib/libdl.so.2 (0x00007f742d383000)
        /lib64/ld-linux-x86-64.so.2 (0x00007f742e0cc000)

That's on Linux, it doesn't need libz, libcrypto etc. It just needs
standard C library functions along with its own main().

> eval_gettext involves 2 git invocation (git-sh-i18n--envsubst counts as
> much as git) and 1 subshell in practice (some subshells are optimized away).
>
>> So e.g. using eval_gettext once isn't a bigger problem than calling
>> some trivial sed substitution twice?
>
> Look at the timings: In my book, eval_gettext counts like about 8 sed
> substitutions.

How about if you just replace your tests with "cat". That should give
a more accurate indication of what speed it *should* be operating at,
once I fix those Makefile issues.

Anyway, if it's no more expensive than cat(1) (which it shouldn't be)
it probably won't be a problem to use git-sh-i18n--envsubst.

Actually I'd rather keep the current calling semantics (using
variables in the strings) than improve performance slightly on Windows
for what are mostly non-critical error messages. But that's just my
opinion.

Re: [RFC/PATCH 1/5] gettext: fix bug in git-sh-i18n's eval_gettext() by using envsubst(1)

[Johannes Sixt]

Am 11/9/2010 12:57, schrieb Ævar Arnfjörð Bjarmason:
> How about if you just replace your tests with "cat". That should give
> a more accurate indication of what speed it *should* be operating at,
> once I fix those Makefile issues.

Just FYI:

$ time (for i in {1..100}; do eval_gettext2 foobar; done) > /dev/null

real    0m14.844s
user    0m11.635s
sys     0m4.372s

Please understand that on Windows there is a difference between cat, sed,
etc. and git. There is also a speed difference, which is annoying, but it
is fact and *not* a bug. You cannot argue with how the timings "should be".

> Anyway, if it's no more expensive than cat(1) (which it shouldn't be)
> it probably won't be a problem to use git-sh-i18n--envsubst.

It *is* more expensive.

BTW, current ab/i18n fails to compile when NO_GETTEXT is specified in
config.mak and libintl.h is not available. I suggest the fix below.

-- Hannes

diff --git a/Makefile b/Makefile
index c55baa6..e9ee142 100644
--- a/Makefile
+++ b/Makefile
@@ -619,9 +619,6 @@ LIB_OBJS += entry.o
 LIB_OBJS += environment.o
 LIB_OBJS += exec_cmd.o
 LIB_OBJS += fsck.o
-ifndef NO_GETTEXT
-LIB_OBJS += gettext.o
-endif
 LIB_OBJS += graph.o
 LIB_OBJS += grep.o
 LIB_OBJS += hash.o
@@ -1539,7 +1536,8 @@ endif

 ifdef NO_GETTEXT
 	COMPAT_CFLAGS += -DNO_GETTEXT
-endif
+else
+	LIB_OBJS += gettext.o

 ifdef NEEDS_LIBINTL
 	EXTLIBS += -lintl
@@ -1552,6 +1550,7 @@ endif
 ifdef GETTEXT_POISON
 	COMPAT_CFLAGS += -DGETTEXT_POISON
 endif
+endif

 ifeq ($(TCLTK_PATH),)
 NO_TCLTK=NoThanks

Re: [RFC/PATCH 1/5] gettext: fix bug in git-sh-i18n's eval_gettext() by using envsubst(1)

[Ævar Arnfjörð Bjarmason]

On Tue, Nov 9, 2010 at 13:22, Johannes Sixt <j.sixt@viscovery.net> wrote:
> Am 11/9/2010 12:57, schrieb Ævar Arnfjörð Bjarmason:
>> How about if you just replace your tests with "cat". That should give
>> a more accurate indication of what speed it *should* be operating at,
>> once I fix those Makefile issues.
>
> Just FYI:
>
> $ time (for i in {1..100}; do eval_gettext2 foobar; done) > /dev/null
>
> real    0m14.844s
> user    0m11.635s
> sys     0m4.372s
>
> Please understand that on Windows there is a difference between cat, sed,
> etc. and git. There is also a speed difference, which is annoying, but it
> is fact and *not* a bug. You cannot argue with how the timings "should be".

I don't know/use Windows, or Git on Windows. So forgive my ignorance.

I understood your previous comments to mean that the invocation time
of git-* on one hand and cat(1) on the other hand had to do with how
many DLL's the former needed.

Since git-sh-i18n--envsubst needs the same DLL's (i.e. the libc) as
cat(1) and *nothing else* it should be as fast as cat(1) and not as
slow as git-*(1) once I fix that unfortunate Makefile bug, right?

Or is it something to do with the built-in MinGW tools being special
in some way (e.g. preloaded?)? In that case benchmarking a
hello-world.c for a git-sh-i18n--envsubst (or fixing the linking of
git-sh-i18n--envsubst) would make for a better comparison.

>> Anyway, if it's no more expensive than cat(1) (which it shouldn't be)
>> it probably won't be a problem to use git-sh-i18n--envsubst.
>
> It *is* more expensive.
>
> BTW, current ab/i18n fails to compile when NO_GETTEXT is specified in
> config.mak and libintl.h is not available. I suggest the fix below.

We already discussed this back in August. I came up with what I
thought was a more elegant solution, but didn't implement it yet:
http://www.spinics.net/lists/git/msg137783.html

I can try to come up with one (next weekend or something) and CC it to
you for testing.

> diff --git a/Makefile b/Makefile
> index c55baa6..e9ee142 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -619,9 +619,6 @@ LIB_OBJS += entry.o
>  LIB_OBJS += environment.o
>  LIB_OBJS += exec_cmd.o
>  LIB_OBJS += fsck.o
> -ifndef NO_GETTEXT
> -LIB_OBJS += gettext.o
> -endif
>  LIB_OBJS += graph.o
>  LIB_OBJS += grep.o
>  LIB_OBJS += hash.o
> @@ -1539,7 +1536,8 @@ endif
>
>  ifdef NO_GETTEXT
>        COMPAT_CFLAGS += -DNO_GETTEXT
> -endif
> +else
> +       LIB_OBJS += gettext.o
>
>  ifdef NEEDS_LIBINTL
>        EXTLIBS += -lintl
> @@ -1552,6 +1550,7 @@ endif
>  ifdef GETTEXT_POISON
>        COMPAT_CFLAGS += -DGETTEXT_POISON
>  endif
> +endif
>
>  ifeq ($(TCLTK_PATH),)
>  NO_TCLTK=NoThanks
>

Re: [RFC/PATCH 1/5] gettext: fix bug in git-sh-i18n's eval_gettext() by using envsubst(1)

[Johannes Sixt]

Am 11/9/2010 13:38, schrieb Ævar Arnfjörð Bjarmason:
> I understood your previous comments to mean that the invocation time
> of git-* on one hand and cat(1) on the other hand had to do with how
> many DLL's the former needed.

I was only guessing; I can't explain the timing difference.

> Since git-sh-i18n--envsubst needs the same DLL's (i.e. the libc) as
> cat(1) and *nothing else* it should be as fast as cat(1) and not as
> slow as git-*(1) once I fix that unfortunate Makefile bug, right?

Wrong. Please accept it as a fact (and I'll forgive your ignorance ;) I
would like to spend the time explaining the reasons only if you want to
compile git on Windows yourself.

-- Hannes

Re: [RFC/PATCH 1/5] gettext: fix bug in git-sh-i18n's eval_gettext() by using envsubst(1)

[Ævar Arnfjörð Bjarmason]

On Tue, Nov 9, 2010 at 13:53, Johannes Sixt <j.sixt@viscovery.net> wrote:
> Am 11/9/2010 13:38, schrieb Ævar Arnfjörð Bjarmason:
>> I understood your previous comments to mean that the invocation time
>> of git-* on one hand and cat(1) on the other hand had to do with how
>> many DLL's the former needed.
>
> I was only guessing; I can't explain the timing difference.
>
>> Since git-sh-i18n--envsubst needs the same DLL's (i.e. the libc) as
>> cat(1) and *nothing else* it should be as fast as cat(1) and not as
>> slow as git-*(1) once I fix that unfortunate Makefile bug, right?
>
> Wrong. Please accept it as a fact (and I'll forgive your ignorance ;) I
> would like to spend the time explaining the reasons only if you want to
> compile git on Windows yourself.

I'd usually check out things like these myself in a virtual machine,
but in this case I'd have to fork over a significant amount of cash
just to get a test environment on a system I'm not going to use.

I'd have the same issue if someone pointed out an issue with the
series on e.g. AIX, HP/UX etc.

Anyway, I don't see any sensible reason for why a Unix utility like
cat(1) would be fast but a utility of ours that does similar things /
links to the same libraries (and no more) wouldn't be.