[PATCH 00/21] i18n and tests updates

classic Classic list List threaded Threaded
42 messages Options
123
Reply | Threaded
Open this post in threaded view
|

[PATCH 20/21] t4153: fix negated test_i18ngrep call

Vasco Almeida
The function test_i18ngrep fakes success when run under GETTEXT_POISON.
Hence, running in the following manner will always fail under gettext
poison:

        ! test_i18ngrep expected actual

Use correct syntax: test_i18ngrep ! expected actual

For other instance of this issue see 41ca19b ("tests: fix negated
test_i18ngrep calls", 2014-08-13).

Signed-off-by: Vasco Almeida <[hidden email]>
---
 t/t4153-am-resume-override-opts.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t4153-am-resume-override-opts.sh b/t/t4153-am-resume-override-opts.sh
index 7c013d8..8ea22d1 100755
--- a/t/t4153-am-resume-override-opts.sh
+++ b/t/t4153-am-resume-override-opts.sh
@@ -53,7 +53,7 @@ test_expect_success '--no-quiet overrides --quiet' '
  # Applying side1 will be quiet.
  test_must_fail git am --quiet side[123].eml >out &&
  test_path_is_dir .git/rebase-apply &&
- ! test_i18ngrep "^Applying: " out &&
+ test_i18ngrep ! "^Applying: " out &&
  echo side1 >file &&
  git add file &&
 
--
2.7.3

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

[PATCH 21/21] t5523: use test_i18ngrep for negation

Vasco Almeida
In reply to this post by Vasco Almeida
Replace the first form by the second one:
        ! grep expected actual
        test_i18ngrep ! expected actual

The latter syntax is supported by test_i18ngrep defined in
t/test-lib.sh.

Although the test already passes whether GETTEXT_POSION is enabled, use
the i18n grep variant for the sake of consistency and also to make
obvious that those strings are subject to i18n.

Signed-off-by: Vasco Almeida <[hidden email]>
---
 t/t5523-push-upstream.sh | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/t/t5523-push-upstream.sh b/t/t5523-push-upstream.sh
index 4a7b98b..d6981ba 100755
--- a/t/t5523-push-upstream.sh
+++ b/t/t5523-push-upstream.sh
@@ -83,7 +83,7 @@ test_expect_success 'progress messages do not go to non-tty' '
 
  # skip progress messages, since stderr is non-tty
  git push -u upstream master >out 2>err &&
- ! grep "Writing objects" err
+ test_i18ngrep ! "Writing objects" err
 '
 
 test_expect_success 'progress messages go to non-tty (forced)' '
@@ -98,15 +98,15 @@ test_expect_success TTY 'push -q suppresses progress' '
  ensure_fresh_upstream &&
 
  test_terminal git push -u -q upstream master >out 2>err &&
- ! grep "Writing objects" err
+ test_i18ngrep ! "Writing objects" err
 '
 
 test_expect_success TTY 'push --no-progress suppresses progress' '
  ensure_fresh_upstream &&
 
  test_terminal git push -u --no-progress upstream master >out 2>err &&
- ! grep "Unpacking objects" err &&
- ! grep "Writing objects" err
+ test_i18ngrep ! "Unpacking objects" err &&
+ test_i18ngrep ! "Writing objects" err
 '
 
 test_expect_success TTY 'quiet push' '
--
2.7.3

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 03/21] i18n: advice: internationalize message for conflicts

Eric Sunshine
In reply to this post by Vasco Almeida
On Wed, May 18, 2016 at 11:27 AM, Vasco Almeida <[hidden email]> wrote:

> Mark message for translation telling the user she has conflicts to
> resolve. Expose each particular use case, in order to enable translating
> entire sentences which would facilitate translating into other
> languages.
>
> Change "Pull" to lowercase to match other messages.
>
> Signed-off-by: Vasco Almeida <[hidden email]>
> ---
> diff --git a/advice.c b/advice.c
> @@ -79,7 +79,20 @@ int git_default_advice_config(const char *var, const char *value)
>  int error_resolve_conflict(const char *me)
>  {
> -       error("%s is not possible because you have unmerged files.", me);
> +       if (!strcmp(me, "cherry-pick"))
> +               error(_("cherry-pick is not possible because you have unmerged files."));
> +       else if (!strcmp(me, "commit"))
> +               error(_("commit is not possible because you have unmerged files."));
> +       else if (!strcmp(me, "merge"))
> +               error(_("merge is not possible because you have unmerged files."));
> +       else if (!strcmp(me, "pull"))
> +               error(_("pull is not possible because you have unmerged files."));
> +       else if (!strcmp(me, "revert"))
> +               error(_("revert is not possible because you have unmerged files."));
> +       else
> +               error(_("%s is not possible because you have unmerged files."),
> +                       me);

Despite the commit message, I still don't understand this change. 'me'
is a literal git command which shouldn't be translated, so how is this
helping?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 05/21] i18n: sequencer: mark entire sentences for translation

Eric Sunshine
In reply to this post by Vasco Almeida
On Wed, May 18, 2016 at 11:27 AM, Vasco Almeida <[hidden email]> wrote:

> Mark entire sentences of error message rather than assembling one using
> placeholders (e.g. "Cannot %s during a %s"). That would facilitate
> translation work.
>
> Signed-off-by: Vasco Almeida <[hidden email]>
> ---
> diff --git a/sequencer.c b/sequencer.c
> @@ -697,9 +697,14 @@ static struct commit *parse_insn_line(char *bol, char *eol, struct replay_opts *
>         if (action != opts->action) {
> -               const char *action_str;
> -               action_str = action == REPLAY_REVERT ? "revert" : "cherry-pick";
> -               error(_("Cannot %s during a %s"), action_str, action_name(opts));
> +               if (action == REPLAY_REVERT)
> +                     error((opts->action == REPLAY_REVERT)
> +                           ? _("Cannot revert during a another revert.")
> +                           : _("Cannot revert during a cherry-pick."));
> +               else
> +                     error((opts->action == REPLAY_REVERT)
> +                           ? _("Cannot cherry-pick during a revert.")
> +                           : _("Cannot cherry-pick during another cherry-pick."));
>                 return NULL;
>         }

Similar to my comment on patch 3/21, since the "actions" are literal
git commands, it's not clear why this change is helpful. Perhaps the
commit message needs to do a better job of persuading the reader?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 03/21] i18n: advice: internationalize message for conflicts

Vasco Almeida
In reply to this post by Eric Sunshine
Às 19:24 de 18-05-2016, Eric Sunshine escreveu:

> On Wed, May 18, 2016 at 11:27 AM, Vasco Almeida <[hidden email]> wrote:
>> Mark message for translation telling the user she has conflicts to
>> resolve. Expose each particular use case, in order to enable translating
>> entire sentences which would facilitate translating into other
>> languages.
>>
>> Change "Pull" to lowercase to match other messages.
>>
>> Signed-off-by: Vasco Almeida <[hidden email]>
>> ---
>> diff --git a/advice.c b/advice.c
>> @@ -79,7 +79,20 @@ int git_default_advice_config(const char *var, const char *value)
>>  int error_resolve_conflict(const char *me)
>>  {
>> -       error("%s is not possible because you have unmerged files.", me);
>> +       if (!strcmp(me, "cherry-pick"))
>> +               error(_("cherry-pick is not possible because you have unmerged files."));
>> +       else if (!strcmp(me, "commit"))
>> +               error(_("commit is not possible because you have unmerged files."));
>> +       else if (!strcmp(me, "merge"))
>> +               error(_("merge is not possible because you have unmerged files."));
>> +       else if (!strcmp(me, "pull"))
>> +               error(_("pull is not possible because you have unmerged files."));
>> +       else if (!strcmp(me, "revert"))
>> +               error(_("revert is not possible because you have unmerged files."));
>> +       else
>> +               error(_("%s is not possible because you have unmerged files."),
>> +                       me);
>
> Despite the commit message, I still don't understand this change. 'me'
> is a literal git command which shouldn't be translated, so how is this
> helping?
>
I saw it as an operation that could be translated, not necessary a git
command - just happens to be so. Now that you mention, I can see this
patch doesn't had much value from that point of view.

On the other hand, I can translate the sentence, including the command
name. Something I couldn't before and I would likely do it to be
consistent with other translations I've done and also because, as a
user, I would like to read the entire sentence in my language.

I have few experience in i18n and l10n, but often I realize I'm making
assumptions about other languages that are not true. Some translations,
say Chinese or Bulgarian that have their own writing systems, might
translate that command word to something to their writing. Others might
agglutinate the command word with some suffix.
I can't say for sure because I don't know those languages, but it's
possible.

Maybe the first paragraph can give you a better understanding of the issue:
http://www.gnu.org/software/gettext/manual/html_node/Names.html
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 05/21] i18n: sequencer: mark entire sentences for translation

Vasco Almeida
In reply to this post by Eric Sunshine
Às 19:28 de 18-05-2016, Eric Sunshine escreveu:

> On Wed, May 18, 2016 at 11:27 AM, Vasco Almeida <[hidden email]> wrote:
>> Mark entire sentences of error message rather than assembling one using
>> placeholders (e.g. "Cannot %s during a %s"). That would facilitate
>> translation work.
>>
>> Signed-off-by: Vasco Almeida <[hidden email]>
>> ---
>> diff --git a/sequencer.c b/sequencer.c
>> @@ -697,9 +697,14 @@ static struct commit *parse_insn_line(char *bol, char *eol, struct replay_opts *
>>         if (action != opts->action) {
>> -               const char *action_str;
>> -               action_str = action == REPLAY_REVERT ? "revert" : "cherry-pick";
>> -               error(_("Cannot %s during a %s"), action_str, action_name(opts));
>> +               if (action == REPLAY_REVERT)
>> +                     error((opts->action == REPLAY_REVERT)
>> +                           ? _("Cannot revert during a another revert.")
>> +                           : _("Cannot revert during a cherry-pick."));
>> +               else
>> +                     error((opts->action == REPLAY_REVERT)
>> +                           ? _("Cannot cherry-pick during a revert.")
>> +                           : _("Cannot cherry-pick during another cherry-pick."));
>>                 return NULL;
>>         }
>
> Similar to my comment on patch 3/21, since the "actions" are literal
> git commands, it's not clear why this change is helpful. Perhaps the
> commit message needs to do a better job of persuading the reader?
>
I agree, I should have explained why this way. As I tried to explain on
patch 3/21, it concerns a) we can't assume what and how does the
translator translates into her language, so b) give translations freedom
to choose.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 03/21] i18n: advice: internationalize message for conflicts

Junio C Hamano
In reply to this post by Vasco Almeida
Vasco Almeida <[hidden email]> writes:

>>> +       else
>>> +               error(_("%s is not possible because you have unmerged files."),
>>> +                       me);
>>
>> Despite the commit message, I still don't understand this change. 'me'
>> is a literal git command which shouldn't be translated, so how is this
>> helping?
>>
> I saw it as an operation that could be translated, not necessary a git
> command

The intention of who wrote this message was to tell this to the
user:

    Running the command '$X' is not possible in this situation.

implying "Don't type '$X' again; it is futile.  Correct the unmerged
status first".

Our subcommand names are English verbs, because they are designed to
be easily understandable by English speakers, so we do not exactly
say "Running the command 'commit'" in the message.  Instead we say
"commit is not possible..." for brevity.

Languages, if their grammatical rules do not allow a subcommand name
in the place in a straight-translation of that original sentence,
are allowed to (and encouraged to) translate the original "%s is not
possible" as if it were "Running the command '%s' is not possible".
That is what good translaters would do anyway.

So for this (and 05/21 for the same reasons), I do not think we want
to split and duplicate the messages.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 14/21] i18n: rebase-interactive: mark strings for translation

Eric Sunshine
In reply to this post by Vasco Almeida
On Wed, May 18, 2016 at 11:27 AM, Vasco Almeida <[hidden email]> wrote:
> Mark strings in git-rebase--interactive.sh for translation. There is no
> need to source git-sh-i18n since git-rebase.sh already does so.
>
> Add git-rebase--interactive.sh to LOCALIZED_SH in Makefile in order to
> enabled extracting strings marked for translation by xgettext.

s/enabled/enable/

> Signed-off-by: Vasco Almeida <[hidden email]>
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 19/21] t9003: become resilient to GETTEXT_POISON

Eric Sunshine
In reply to this post by Vasco Almeida
On Wed, May 18, 2016 at 11:27 AM, Vasco Almeida <[hidden email]> wrote:

> The test t9003-help-autocorrect.sh fails when run under GETTEXT_POISON,
> because it's expecting to filter out the original output. Accommodate
> gettext poison case by also filtering out the default simulated output.
>
> Signed-off-by: Vasco Almeida <[hidden email]>
> ---
> diff --git a/t/t9003-help-autocorrect.sh b/t/t9003-help-autocorrect.sh
> @@ -31,10 +31,14 @@ test_expect_success 'autocorrect showing candidates' '
>         git config help.autocorrect 0 &&
>
>         test_must_fail git lfg 2>actual &&
> -       sed -e "1,/^Did you mean this/d" actual | grep lgf &&
> +       sed -e "1,/^Did you mean this/d" actual |
> +       sed -e "/GETTEXT POISON/d" actual |

Why not do so with a single sed invocation?

   sed -e "..." -e "..." |

> +       grep lgf &&
>
>         test_must_fail git distimdist 2>actual &&
> -       sed -e "1,/^Did you mean this/d" actual | grep distimdistim
> +       sed -e "1,/^Did you mean this/d" actual |
> +       sed -e "/GETTEXT POISON/d" actual |

Ditto.

> +       grep distimdistim
>  '
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 19/21] t9003: become resilient to GETTEXT_POISON

Vasco Almeida
Às 18:34 de 19-05-2016, Eric Sunshine escreveu:

> On Wed, May 18, 2016 at 11:27 AM, Vasco Almeida <[hidden email]> wrote:
>> The test t9003-help-autocorrect.sh fails when run under GETTEXT_POISON,
>> because it's expecting to filter out the original output. Accommodate
>> gettext poison case by also filtering out the default simulated output.
>>
>> Signed-off-by: Vasco Almeida <[hidden email]>
>> ---
>> diff --git a/t/t9003-help-autocorrect.sh b/t/t9003-help-autocorrect.sh
>> @@ -31,10 +31,14 @@ test_expect_success 'autocorrect showing candidates' '
>>         git config help.autocorrect 0 &&
>>
>>         test_must_fail git lfg 2>actual &&
>> -       sed -e "1,/^Did you mean this/d" actual | grep lgf &&
>> +       sed -e "1,/^Did you mean this/d" actual |
>> +       sed -e "/GETTEXT POISON/d" actual |
>
> Why not do so with a single sed invocation?
>
>    sed -e "..." -e "..." |
>
>> +       grep lgf &&
>>
>>         test_must_fail git distimdist 2>actual &&
>> -       sed -e "1,/^Did you mean this/d" actual | grep distimdistim
>> +       sed -e "1,/^Did you mean this/d" actual |
>> +       sed -e "/GETTEXT POISON/d" actual |
>
> Ditto.
>
>> +       grep distimdistim
>>  '

I tried but it seems not to work.

Actually I did this wrong, I haven't thought this through.

Under gettext poison:
sed -e "1,/^Did you mean this/d" removes all lines, gives no output.
And then the one second, sed -e "/GETTEXT POISON/d", outputs "lgf" as
expected.

But running normally (without gettext poison):
1st sed outputs "lgf" as expected
And then second one output the entire 'actual' file, like if it were
cat, undoing the first sed.

I think the sed here is superfluous in the first place.
Should we remove it? If it weren't the case of gettext poison it was ok
to have sed there, but it makes the test fail under gettext poison.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 19/21] t9003: become resilient to GETTEXT_POISON

Eric Sunshine
[cc:+junio]

On Thu, May 19, 2016 at 5:31 PM, Vasco Almeida <[hidden email]> wrote:

> Às 18:34 de 19-05-2016, Eric Sunshine escreveu:
>> On Wed, May 18, 2016 at 11:27 AM, Vasco Almeida <[hidden email]> wrote:
>>> -       sed -e "1,/^Did you mean this/d" actual | grep lgf &&
>>> +       sed -e "1,/^Did you mean this/d" actual |
>>> +       sed -e "/GETTEXT POISON/d" actual |
>>> +       grep distimdistim
>>
>> Why not do so with a single sed invocation?
>>
>>    sed -e "..." -e "..." |
>
> I tried but it seems not to work.
>
> Actually I did this wrong, I haven't thought this through.
>
> Under gettext poison:
> sed -e "1,/^Did you mean this/d" removes all lines, gives no output.
> And then the one second, sed -e "/GETTEXT POISON/d", outputs "lgf" as
> expected.
>
> But running normally (without gettext poison):
> 1st sed outputs "lgf" as expected
> And then second one output the entire 'actual' file, like if it were
> cat, undoing the first sed.
>
> I think the sed here is superfluous in the first place.
> Should we remove it? If it weren't the case of gettext poison it was ok
> to have sed there, but it makes the test fail under gettext poison.

Indeed, the sed seems superfluous. The output of the test command is:

    git: 'lfg' is not a git command. See 'git --help'.

    Did you mean this?
        lgf

And, the grep'd string, "lgf" only appears once, so grep alone should
be sufficient to verify expected behavior. Likewise for the other case
of misspelled "distimdist" and grep'd "distimdistim" which appears
only once.

I agree that the simplest fix to make GETTEXT_POISON work is to drop
the sed invocation.

Anyhow, I've cc:'d the author of 9d1d2b7 (git: remove an early return
from save_env_before_alias(), 2016-01-26) which introduced it.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 19/21] t9003: become resilient to GETTEXT_POISON

Junio C Hamano
Eric Sunshine <[hidden email]> writes:

> [cc:+junio]
>
> Indeed, the sed seems superfluous. The output of the test command is:
>
>     git: 'lfg' is not a git command. See 'git --help'.
>
>     Did you mean this?
>         lgf
>
> And, the grep'd string, "lgf" only appears once, so grep alone should
> be sufficient to verify expected behavior.

We want to see the string appear after "Did you mean this?" and we
do not want to be fooled by a future change in the early part of the
message, which may contain a substring l-g-f that does not have
anything to do with the alias we are looking for.

And the way you express "I do not care anything above this line" is
to say "sed -e '1,/^that line/d'".

Of course, if you use this with POISON, you'd need to consider that
"Did you mean this" would not be a good marker to identify where the
introductory text we want to ignore ends.  You'd need to find a
different mechanism to exclude the introductory text if you want to
retain the future-proofing the existing "sed -e" gave us.

Perhaps discarding up to the first blank line (i.e. assuming that we
would not remove that blank, and also assuming that we will not
rephrase "Did you mean this?") may be a good alternative.

Or assuming that the explanatory text would not begin its lines with
a tab, i.e.

        grep '^ lgf$' actual

(the space between '^' and 'l' above is a TAB) without using
test_i18ngrep?

I think I like that the best among what I can think of offhand.


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 19/21] t9003: become resilient to GETTEXT_POISON

Eric Sunshine
On Fri, May 20, 2016 at 12:39 PM, Junio C Hamano <[hidden email]> wrote:

> Eric Sunshine <[hidden email]> writes:
>> Indeed, the sed seems superfluous. The output of the test command is:
>>
>>     git: 'lfg' is not a git command. See 'git --help'.
>>
>>     Did you mean this?
>>         lgf
>>
>> And, the grep'd string, "lgf" only appears once, so grep alone should
>> be sufficient to verify expected behavior.
>
> Perhaps discarding up to the first blank line (i.e. assuming that we
> would not remove that blank, and also assuming that we will not
> rephrase "Did you mean this?") may be a good alternative.
>
> Or assuming that the explanatory text would not begin its lines with
> a tab, i.e.
>
>         grep '^ lgf$' actual
>
> (the space between '^' and 'l' above is a TAB) without using
> test_i18ngrep?
>
> I think I like that the best among what I can think of offhand.

Yep, I also considered both of these approaches and favored the latter, as well.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 19/21] t9003: become resilient to GETTEXT_POISON

Vasco Almeida
In reply to this post by Junio C Hamano
Às 16:39 de 20-05-2016, Junio C Hamano escreveu:

> We want to see the string appear after "Did you mean this?" and we
> do not want to be fooled by a future change in the early part of the
> message, which may contain a substring l-g-f that does not have
> anything to do with the alias we are looking for.
>
> And the way you express "I do not care anything above this line" is
> to say "sed -e '1,/^that line/d'".
>
> Of course, if you use this with POISON, you'd need to consider that
> "Did you mean this" would not be a good marker to identify where the
> introductory text we want to ignore ends.  You'd need to find a
> different mechanism to exclude the introductory text if you want to
> retain the future-proofing the existing "sed -e" gave us.
>
> Perhaps discarding up to the first blank line (i.e. assuming that we
> would not remove that blank, and also assuming that we will not
> rephrase "Did you mean this?") may be a good alternative.
>
> Or assuming that the explanatory text would not begin its lines with
> a tab, i.e.
>
> grep '^ lgf$' actual
>
> (the space between '^' and 'l' above is a TAB) without using
> test_i18ngrep?
>
> I think I like that the best among what I can think of offhand.
>
>
Alternatively, we could leave sed alone as it were before this patch and
use test_i18ngrep instead of grep to fake success under GETTEXT_POISON.
I think I prefer this way. What do you think?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 19/21] t9003: become resilient to GETTEXT_POISON

Junio C Hamano
Vasco Almeida <[hidden email]> writes:

> Alternatively, we could leave sed alone as it were before this patch and
> use test_i18ngrep instead of grep to fake success under GETTEXT_POISON.
> I think I prefer this way. What do you think?

That is equivalent to saying that "we would translate 'lgf' to
end-user's language", which does not make much sense to me.

Wouldn't the introductory explanation, up to "Did you mean this?",
be the only thing that is translated?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 19/21] t9003: become resilient to GETTEXT_POISON

Junio C Hamano
Junio C Hamano <[hidden email]> writes:

> Vasco Almeida <[hidden email]> writes:
>
>> Alternatively, we could leave sed alone as it were before this patch and
>> use test_i18ngrep instead of grep to fake success under GETTEXT_POISON.
>> I think I prefer this way. What do you think?
>
> That is equivalent to saying that "we would translate 'lgf' to
> end-user's language", which does not make much sense to me.
>
> Wouldn't the introductory explanation, up to "Did you mean this?",
> be the only thing that is translated?

I just checked the code ;-)  We do this:

        fprintf_ln(stderr,
                   Q_("\nDid you mean this?",
                      "\nDid you mean one of these?",
                   n));

        for (i = 0; i < n; i++)
                fprintf(stderr, "\t%s\n", main_cmds.names[i]->name);

Using test_i18ngrep would mean we would not be able to catch a
potential future bug that does an equivalent of

        for (i = 0; i < n; i++)
                fprintf(stderr, "\t%s\n", _(main_cmds.names[i]->name));

in the loop, i.e. marking something that is not meant to be
translated as translatable.

As long as translators do not translate "Did you mean..." to begin a
line with a tab (which I do not think there is any reason to), it is
going to work reliably to grep for "^ lgf$" here, and it will catch
such a potential future bug under GETTEXT_POISON build.



--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 19/21] t9003: become resilient to GETTEXT_POISON

Vasco Almeida
Às 17:59 de 20-05-2016, Junio C Hamano escreveu:
> As long as translators do not translate "Did you mean..." to begin a
> line with a tab (which I do not think there is any reason to), it is
> going to work reliably to grep for "^ lgf$" here, and it will catch
> such a potential future bug under GETTEXT_POISON build.

Translations don't affect the tests since they're run with C locale.
I think all tests source test-lib.sh which does:

        # For repeatability, reset the environment to known value.
        # TERM is sanitized below, after saving color control sequences.
        LANG=C
        LC_ALL=C
        PAGER=cat
        TZ=UTC
        export LANG LC_ALL PAGER TZ

So, I'll remove sed command and use grep the way you said it, in the
next re-roll.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 14/21] i18n: rebase-interactive: mark strings for translation

Ævar Arnfjörð Bjarmason
In reply to this post by Vasco Almeida
On Wed, May 18, 2016 at 5:27 PM, Vasco Almeida <[hidden email]> wrote:
> Mark strings in git-rebase--interactive.sh for translation. There is no
> need to source git-sh-i18n since git-rebase.sh already does so.

Cool, thanks for working on this.

> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -128,7 +128,7 @@ mark_action_done () {
>         if test "$last_count" != "$new_count"
>         then
>                 last_count=$new_count
> -               printf "Rebasing (%d/%d)\r" $new_count $total
> +               printf "$(gettext Rebasing) (%d/%d)\r" $new_count $total
>                 test -z "$verbose" || echo
>         fi
>  }

Things like these should be converted into something you can pass to
eval_gettext. I.e. For any message the translator needs to be able to
translate the whole message. Consider e.g. RTL languages where the
(%d/%d) will be on the right-hand-side of the message.

> @@ -201,11 +201,13 @@ exit_with_patch () {
>         make_patch $1
>         git rev-parse --verify HEAD > "$amend"
>         gpg_sign_opt_quoted=${gpg_sign_opt:+$(git rev-parse --sq-quote "$gpg_sign_opt")}
> -       warn "You can amend the commit now, with"
> +       # TRANSLATORS: after this line is a command to be issued by the user
> +       warn "$(gettext "You can amend the commit now, with")"
>         warn
>         warn "  git commit --amend $gpg_sign_opt_quoted"
>         warn
> -       warn "Once you are satisfied with your changes, run"
> +       # TRANSLATORS: after this line is a command to be issued by the user
> +       warn "$(gettext "Once you are satisfied with your changes, run")"
>         warn
>         warn "  git rebase --continue"
>         warn

Stuff like this should be one big call to gettext. I.e. everything
from "You can amend" up to and including "--continue". Even if the
translators probably don't want to change the order here it helps a
lot to have the extra context. I.e. do it like ...

> @@ -536,10 +543,11 @@ do_next () {
>                 mark_action_done
>                 do_pick $sha1 "$rest"
>                 git commit --amend --no-post-rewrite ${gpg_sign_opt:+"$gpg_sign_opt"} || {
> -                       warn "Could not amend commit after successfully picking $sha1... $rest"
> -                       warn "This is most likely due to an empty commit message, or the pre-commit hook"
> -                       warn "failed. If the pre-commit hook failed, you may need to resolve the issue before"
> -                       warn "you are able to reword the commit."
> +                       warn "$(eval_gettext "\
> +Could not amend commit after successfully picking \$sha1... \$rest
> +This is most likely due to an empty commit message, or the pre-commit hook
> +failed. If the pre-commit hook failed, you may need to resolve the issue before
> +you are able to reword the commit.")"
>                         exit_with_patch $sha1 1
>                 }

... this! :)

> @@ -607,7 +615,7 @@ do_next () {
>         x|"exec")
>                 read -r command rest < "$todo"
>                 mark_action_done
> -               printf 'Executing: %s\n' "$rest"
> +               printf "$(gettext Executing:) %s\n" "$rest"
>                 "${SHELL:-@SHELL_PATH@}" -c "$rest" # Actual execution
>                 status=$?
>                 # Run in subshell because require_clean_work_tree can die.

Ditto my first comment about the whole thing needing to be a call to
eval_gettext. I.e. also that " %s" part.

> -                       warn "You can fix the problem, and then run"
> +                       # TRANSLATORS: after this line is a command to be issued by the user
> +                       warn "$(gettext "You can fix the problem, and then run")"
>                         warn
>                         warn "  git rebase --continue"
>                         warn
> @@ -630,9 +639,11 @@ do_next () {
>                         exit "$status"
>                 elif test "$dirty" = t
>                 then
> -                       warn "Execution succeeded: $rest"
> -                       warn "but left changes to the index and/or the working tree"
> -                       warn "Commit or stash your changes, and then run"
> +                       # TRANSLATORS: after these lines is a command to be issued by the user
> +                       warn "$(eval_gettext "\
> +Execution succeeded: \$rest
> +but left changes to the index and/or the working tree
> +Commit or stash your changes, and then run")"
>                         warn
>                         warn "  git rebase --continue"
>                         warn

Both ditto the above. I.e. just include the command to be issued in
the message. Then you can also skip the TRANSLATORS comment since this
won't be confusing to them anymore.

> @@ -991,28 +1002,26 @@ check_todo_list () {
>                 then
>                         test "$check_level" = error && raise_error=t
>
> -                       warn "Warning: some commits may have been dropped" \
> -                               "accidentally."
> -                       warn "Dropped commits (newer to older):"
> +                       warn "$(gettext "\
> +Warning: some commits may have been dropped accidentally.
> +Dropped commits (newer to older):")"
>
>                         # Make the list user-friendly and display
>                         opt="--no-walk=sorted --format=oneline --abbrev-commit --stdin"
>                         git rev-list $opt <"$todo".miss | warn_lines
>
> -                       warn "To avoid this message, use \"drop\" to" \
> -                               "explicitly remove a commit."
> -                       warn
> -                       warn "Use 'git config rebase.missingCommitsCheck' to change" \
> -                               "the level of warnings."
> -                       warn "The possible behaviours are: ignore, warn, error."
> +                       warn "$(gettext "\
> +To avoid this message, use \"drop\" to explicitly remove a commit.
> +
> +Use 'git config rebase.missingCommitsCheck' to change the level of warnings.
> +The possible behaviours are: ignore, warn, error.")"
>                         warn
>                 fi
>                 ;;
>         ignore)
>                 ;;
>         *)

Making this into one big eval_gettext where we stash away those "newer
to older" commits into a variable would be easier on translators, but
maybe there are performance considerations and we could just live with
two messages. Not sure how large this list might get in practice.

I haven't tried to apply & run this patch. But aside from the chunks I
commented on the rest of this looks fine to me.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 05/21] i18n: sequencer: mark entire sentences for translation

Ævar Arnfjörð Bjarmason
In reply to this post by Vasco Almeida
On Wed, May 18, 2016 at 11:02 PM, Vasco Almeida <[hidden email]> wrote:

> Às 19:28 de 18-05-2016, Eric Sunshine escreveu:
>> On Wed, May 18, 2016 at 11:27 AM, Vasco Almeida <[hidden email]> wrote:
>>> Mark entire sentences of error message rather than assembling one using
>>> placeholders (e.g. "Cannot %s during a %s"). That would facilitate
>>> translation work.
>>>
>>> Signed-off-by: Vasco Almeida <[hidden email]>
>>> ---
>>> diff --git a/sequencer.c b/sequencer.c
>>> @@ -697,9 +697,14 @@ static struct commit *parse_insn_line(char *bol, char *eol, struct replay_opts *
>>>         if (action != opts->action) {
>>> -               const char *action_str;
>>> -               action_str = action == REPLAY_REVERT ? "revert" : "cherry-pick";
>>> -               error(_("Cannot %s during a %s"), action_str, action_name(opts));
>>> +               if (action == REPLAY_REVERT)
>>> +                     error((opts->action == REPLAY_REVERT)
>>> +                           ? _("Cannot revert during a another revert.")
>>> +                           : _("Cannot revert during a cherry-pick."));
>>> +               else
>>> +                     error((opts->action == REPLAY_REVERT)
>>> +                           ? _("Cannot cherry-pick during a revert.")
>>> +                           : _("Cannot cherry-pick during another cherry-pick."));
>>>                 return NULL;
>>>         }
>>
>> Similar to my comment on patch 3/21, since the "actions" are literal
>> git commands, it's not clear why this change is helpful. Perhaps the
>> commit message needs to do a better job of persuading the reader?
>>
> I agree, I should have explained why this way. As I tried to explain on
> patch 3/21, it concerns a) we can't assume what and how does the
> translator translates into her language, so b) give translations freedom
> to choose.

For what it's worth I agree with you and disagree with Eric here and
Junio in the "[PATCH 03/21] i18n: advice: internationalize message for
conflicts" thread.

Of course there's a trade-off in source code verbosity when you have
to change every occurance of (pseudocode):

    "our %s failed" # %s can be revert or merge

to:

    if (action == "merge")
        gettext("our merge failed")
    elsif (action == "revert")
        gettext("our revert failed")

But forcing the translator to turn every such occurrence that flows
naturally in English into "the '%s' command failed" leads to a worse
translation.

For example, if I ever get around to doing the Icelandic translation
which I've had on my backlog I might translate something like this:

    "You can merge or revert this commit"
    "To merge run 'git merge $commit', to revert run 'git revert $commit'"
    # Subsequently
    "The %s failed" # for %s = merge || revert

As:

    "Þú getur getur framkvæmt samruna á þessari breytingu, eða afturkallað hana"
    "Til að framkvæma samruna keyrðu 'git merge $commit', til að
afturkalla 'git revert $commit'"
    # Subsequently
    "Samruninn mistókst þegar við keyrðum 'merge' skipunina".
    "Afturköllunnin mistókst þegar við keyrðum 'revert' skipunina"

I.e. even though you might be running "git merge" or "git revert" the
UI is talking about those terms in the translated using native terms
for the action of merging or reverting, but referring to the literal
command names in English.

Not accepting changes like these means you have to translate this sort
of stuff like:

    # Same as the above
    "Þú getur getur framkvæmt samruna á þessari breytingu, eða
afturkallað hana".
    "Til að framkvæma samruna keyrðu 'git merge $commit', til að
afturkalla 'git revert $commit'"
    # Subsequently
    "Okkur tókst ekki að keyra 'git %s' skipunina" # %s = merge || revert

It just doesn't flow as well, and leads to a more verbose translation.
Now instead of referring to the translated verb I'd already
established I have to just refer to literal command names.

Both this change and the change you submitted in
<[hidden email]> and Junio
didn't like in <[hidden email]> are
actual examples of cases where if I was (finally getting of my ass to)
doing the Icelandic translation I'd take advantage of this.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 14/21] i18n: rebase-interactive: mark strings for translation

Vasco Almeida
In reply to this post by Ævar Arnfjörð Bjarmason
Às 12:57 de 21-05-2016, Ævar Arnfjörð Bjarmason escreveu:

> On Wed, May 18, 2016 at 5:27 PM, Vasco Almeida <[hidden email]> wrote:
>> > Mark strings in git-rebase--interactive.sh for translation. There is no
>> > need to source git-sh-i18n since git-rebase.sh already does so.
> Cool, thanks for working on this.
>
>> > --- a/git-rebase--interactive.sh
>> > +++ b/git-rebase--interactive.sh
>> > @@ -128,7 +128,7 @@ mark_action_done () {
>> >         if test "$last_count" != "$new_count"
>> >         then
>> >                 last_count=$new_count
>> > -               printf "Rebasing (%d/%d)\r" $new_count $total
>> > +               printf "$(gettext Rebasing) (%d/%d)\r" $new_count $total
>> >                 test -z "$verbose" || echo
>> >         fi
>> >  }
> Things like these should be converted into something you can pass to
> eval_gettext. I.e. For any message the translator needs to be able to
> translate the whole message. Consider e.g. RTL languages where the
> (%d/%d) will be on the right-hand-side of the message.
>
One more thing I haven't anticipated. Thank you for pointing it out.

I'll take this and the other comments in consideration for the next re-roll.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
123