Re: [PATCH v2 04/11] i18n: add--interactive: mark plural strings

2016-10-03 Thread Vasco Almeida 
A Sáb, 01-10-2016 às 18:49 +0200, Jakub Narębski escreveu:
> W dniu 26.09.2016 o 20:15, Vasco Almeida pisze:
> > 
> > A Qua, 31-08-2016 às 12:31 +, Vasco Almeida escreveu:
> > > 
> > > 
> > > Mark plural strings for translation.  Unfold each action case in
> > > one
> > > entire sentence.
> > > 
> > > Pass new keyword for xgettext to extract.
> > > 
> > > Update test to include new subrotine Q__() for plural strings
> > > handling.
> 
> Why use Q__() as the name of the subroutine? [looks further]. Oh, I
> see
> that you are following the example of C shortcut functions (_, Q_ and
> N_).
> 
> But this is Perl, not C.  The standard shortcut functions are those
> defined in Locale::TextDomain, even if we can't use this module
> directly.
> Those that deal with plural strings handling are __n and __nx / __xn.
> 
> The Perl equivalent of Q_ shorthand function in C, C++, etc. is __n.
> There is also a function __nx for combining handling plural strings
> together with variable interpolation.

I was trying to follow the same style of C. I will change the name Q__
to __x for the sake of conformance.


> > > diff --git a/git-add--interactive.perl b/git-add
> > > --interactive.perl
> > > index 4e1e857..08badfa 100755
> > > --- a/git-add--interactive.perl
> > > +++ b/git-add--interactive.perl
> > > @@ -666,12 +666,18 @@ sub status_cmd {
> > >  sub say_n_paths {
> > >   my $did = shift @_;
> > >   my $cnt = scalar @_;
> > > - print "$did ";
> > > - if (1 < $cnt) {
> > > - print "$cnt paths\n";
> > > - }
> > > - else {
> > > - print "one path\n";
> > > + if ($did eq 'added') {
> > > + printf(Q__("added one path\n", "added %d
> > > paths\n",
> > > +    $cnt), $cnt);
> > > + } elsif ($did eq 'updated') {
> > > + printf(Q__("updated one path\n", "updated %d
> > > paths\n",
> > > +    $cnt), $cnt);
> > > + } elsif ($did eq 'reverted') {
> > > + printf(Q__("reverted one path\n", "reverted %d
> > > paths\n",
> > > +    $cnt), $cnt);
> > > + } else {
> > > + printf(Q__("touched one path\n", "touched %d
> > > paths\n",
> > > +    $cnt), $cnt);
> > >   }
> > >  }
> 
> One one hand side, it is recommended to avoid lego-like construction
> of sentences.
> 
>   Translatable strings should be entire sentences. It is often not
>   possible to translate single verbs or adjectives in a substitutable
>   way.
> 
> I think however that the action part ($did in original non-i18n code)
> is whole part in any language, so something like the following would
> be enough:
> 
>   # this hash is as much for validation, as for translation
>       my %actions = map { $_ => 1 } (N__"added", N__"updated",
> N__"reverted");
>       if (exists $actions{$did}) {
>       print __nx("{did} one path\n", "{did} {count}
> paths\n", $cnt,
>          did => __($did), count => $cnt);
>   } else {
>       print __nx("touched one path\n", "touched {count}
> paths\n", $cnt,
>          count => $cnt);
>       }
> 
> Please correct me if I am wrong, and you know language where
> "added %d paths", "updated %d paths", "reverted %d paths" etc. must
> have
> different word order.

We may never know. :-) I prefer not to make assumptions about other
languages and I think there is not much to gain from using this
approach instead of marking entire sentence as the patch does. I mean
the code verbosity is almost the same but maybe it gets harder to
translate.
Other thing, we want to avoid marking for translation single words
(when that is avoidable) because those could appear on other sites that
need a different translation according to the context. For example:
'commit' could be a verb or a noun from the context.

> > When $cnt is 1 I get the following warning:
> > Redundant argument in printf at .../libexec/git-core/git-add
> > --interactive line 680.
> 
> I wonder what is the case of C code - is similar warning here, or is
> gettext smarter in that case...

I do not know but I know there is a few cases like this in C code.

> > The singular form does not have a %d to consume $cnt argument to
> > printf(). Either we find a way to suppress that warning or we
> > change
> > the singular form to contain %d.
> 
> Anyway, with __nx there should be no such problem.
> 
> > 
> > 
> > > 
> > > @@ -1508,8 +1514,10 @@ sub patch_update_file {
> > > ...
> > > - print colored
> > > $header_color, "Split into ",
> > > - scalar(@split), "
> > > hunks.\n";
> > > + print colored
> > > $header_color, sprintf(
> > > + Q__("Split into
> > > %d hunk.\n",
> > > + "Split into
> > > %d hunks.\n",
> > > + scalar(@spli
> > > t)), scalar(@split));
> > 
> > Like we do with this.
> 
> Note that it is a bit of

Re: [PATCH v2 04/11] i18n: add--interactive: mark plural strings

2016-10-01 Thread Jakub Narębski 
W dniu 26.09.2016 o 20:15, Vasco Almeida pisze:
> A Qua, 31-08-2016 às 12:31 +, Vasco Almeida escreveu:
>>
>> Mark plural strings for translation.  Unfold each action case in one
>> entire sentence.
>>
>> Pass new keyword for xgettext to extract.
>>
>> Update test to include new subrotine Q__() for plural strings handling.

Why use Q__() as the name of the subroutine? [looks further]. Oh, I see
that you are following the example of C shortcut functions (_, Q_ and N_).

But this is Perl, not C.  The standard shortcut functions are those
defined in Locale::TextDomain, even if we can't use this module directly.
Those that deal with plural strings handling are __n and __nx / __xn.

The Perl equivalent of Q_ shorthand function in C, C++, etc. is __n.
There is also a function __nx for combining handling plural strings
together with variable interpolation.

  __n MSGID, MSGID_PLURAL, COUNT
  ^^

  That is the reason for the existance of the function ngettext(),
  that __n() is a short-cut for:

print __n"One file has been deleted.\n", 
 "All files have been deleted.\n",
 $files_deleted;

  Alternatively:

print __n ("One file has been deleted.\n",
   "All files have been deleted.\n",
   $files_deleted);


  __nx MSGID, MSGID_PLURAL, COUNT, VAR1 => VAL1, VAR2 => VAL2, ...
  

  Bringing it all together:

print __nx ("One file has been deleted.\n",
"{count} files have been deleted.\n",
$num_files,
count => $num_files);

  The function __nx() [and its alias __xn()] picks the correct plural
  form (also for English!) and it is capable of interpolating variables
  into strings.


>>
>> Signed-off-by: Vasco Almeida 
>> ---
>>  Makefile  |  3 ++-
>>  git-add--interactive.perl | 24 
>>  perl/Git/I18N.pm  |  4 +++-
>>  t/t0202/test.pl   | 11 ++-
>>  4 files changed, 31 insertions(+), 11 deletions(-)

>> diff --git a/Makefile b/Makefile
>> index de5a030..eedf1fa 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -2061,7 +2061,8 @@ XGETTEXT_FLAGS_C = $(XGETTEXT_FLAGS) --language=C \
>>  --keyword=_ --keyword=N_ --keyword="Q_:1,2"
>>  XGETTEXT_FLAGS_SH = $(XGETTEXT_FLAGS) --language=Shell \
>>  --keyword=gettextln --keyword=eval_gettextln
>> -XGETTEXT_FLAGS_PERL = $(XGETTEXT_FLAGS) --keyword=__ --language=Perl
>> +XGETTEXT_FLAGS_PERL = $(XGETTEXT_FLAGS) --language=Perl \
>> +--keyword=__ --keyword="Q__:1,2"

So this would be

   +XGETTEXT_FLAGS_PERL = $(XGETTEXT_FLAGS) --language=Perl \
   +--keyword=__ --keyword=__x --keyword=__n:1,2 --keyword=__nx:1,2

(assuming that __x was used for interpolation)
 
>>  LOCALIZED_C = $(C_OBJ:o=c) $(LIB_H) $(GENERATED_H)
>>  LOCALIZED_SH = $(SCRIPT_SH) git-parse-remote.sh
>>  LOCALIZED_PERL = $(SCRIPT_PERL)
[...]

>> diff --git a/git-add--interactive.perl b/git-add--interactive.perl
>> index 4e1e857..08badfa 100755
>> --- a/git-add--interactive.perl
>> +++ b/git-add--interactive.perl
>> @@ -666,12 +666,18 @@ sub status_cmd {
>>  sub say_n_paths {
>>  my $did = shift @_;
>>  my $cnt = scalar @_;
>> -print "$did ";
>> -if (1 < $cnt) {
>> -print "$cnt paths\n";
>> -}
>> -else {
>> -print "one path\n";
>> +if ($did eq 'added') {
>> +printf(Q__("added one path\n", "added %d paths\n",
>> +   $cnt), $cnt);
>> +} elsif ($did eq 'updated') {
>> +printf(Q__("updated one path\n", "updated %d paths\n",
>> +   $cnt), $cnt);
>> +} elsif ($did eq 'reverted') {
>> +printf(Q__("reverted one path\n", "reverted %d paths\n",
>> +   $cnt), $cnt);
>> +} else {
>> +printf(Q__("touched one path\n", "touched %d paths\n",
>> +   $cnt), $cnt);
>>  }
>>  }

One one hand side, it is recommended to avoid lego-like construction
of sentences.

  Translatable strings should be entire sentences. It is often not
  possible to translate single verbs or adjectives in a substitutable
  way.

I think however that the action part ($did in original non-i18n code)
is whole part in any language, so something like the following would
be enough:

# this hash is as much for validation, as for translation
my %actions = map { $_ => 1 } (N__"added", N__"updated", N__"reverted");
if (exists $actions{$did}) {
print __nx("{did} one path\n", "{did} {count} paths\n", $cnt,
   did => __($did), count => $cnt);
} else {
print __nx("touched one path\n", "touched {count} paths\n", 
$cnt,
   count => $cnt);
}

Please correct me if I am wrong, and you know language where
"added %d paths", "updated %d paths", "reverted %d paths" etc. must

Re: [PATCH v2 04/11] i18n: add--interactive: mark plural strings

2016-09-26 Thread Junio C Hamano 
Vasco Almeida  writes:

>> > +  } elsif ($did eq 'reversed') {
>> > +  printf(Q__("reversed one path\n", "reversed %d paths\n",
>
> This should be 'reverted' not 'reversed'.

I'll mark v2 of this topic "not to be merged yet"; please send in a
corrected version 3 after you collect feedbacks from others and
adjusted the patches for them.

Thanks.

Re: [PATCH v2 04/11] i18n: add--interactive: mark plural strings

2016-09-26 Thread Vasco Almeida 
A Qua, 31-08-2016 às 12:31 +, Vasco Almeida escreveu:
> Mark plural strings for translation.  Unfold each action case in one
> entire sentence.
> 
> Pass new keyword for xgettext to extract.
> 
> Update test to include new subrotine Q__() for plural strings handling.
> 
> > Signed-off-by: Vasco Almeida 
> ---
>  Makefile  |  3 ++-
>  git-add--interactive.perl | 24 
>  perl/Git/I18N.pm  |  4 +++-
>  t/t0202/test.pl   | 11 ++-
>  4 files changed, 31 insertions(+), 11 deletions(-)
> 
> diff --git a/git-add--interactive.perl b/git-add--interactive.perl
> index 4e1e857..08badfa 100755
> --- a/git-add--interactive.perl
> +++ b/git-add--interactive.perl
> @@ -666,12 +666,18 @@ sub status_cmd {
>  sub say_n_paths {
> >     my $did = shift @_;
> >     my $cnt = scalar @_;
> > -   print "$did ";
> > -   if (1 < $cnt) {
> > -   print "$cnt paths\n";
> > -   }
> > -   else {
> > -   print "one path\n";
> > +   if ($did eq 'added') {
> > +   printf(Q__("added one path\n", "added %d paths\n",
> > +      $cnt), $cnt);
> > +   } elsif ($did eq 'updated') {
> > +   printf(Q__("updated one path\n", "updated %d paths\n",
> > +      $cnt), $cnt);
> > +   } elsif ($did eq 'reversed') {
> > +   printf(Q__("reversed one path\n", "reversed %d paths\n",

This should be 'reverted' not 'reversed'.

> > +      $cnt), $cnt);
> > +   } else {
> > +   printf(Q__("touched one path\n", "touched %d paths\n",
> > +      $cnt), $cnt);
> >     }
>  }

When $cnt is 1 I get the following warning:
Redundant argument in printf at 
/home/vasco/dev/local/libexec/git-core/git-add--interactive line 680.

The singular form does not have a %d to consume $cnt argument to
printf(). Either we find a way to suppress that warning or we change
the singular form to contain %d.

> @@ -1508,8 +1514,10 @@ sub patch_update_file {
> ...
> > -   print colored $header_color, "Split 
> > into ",
> > -   scalar(@split), " hunks.\n";
> > +   print colored $header_color, sprintf(
> > +   Q__("Split into %d hunk.\n",
> > +   "Split into %d hunks.\n",
> > +   scalar(@split)), 
> > scalar(@split));
Like we do with this.
> > 
> >

[PATCH v2 04/11] i18n: add--interactive: mark plural strings

2016-08-31 Thread Vasco Almeida 
Mark plural strings for translation.  Unfold each action case in one
entire sentence.

Pass new keyword for xgettext to extract.

Update test to include new subrotine Q__() for plural strings handling.

Signed-off-by: Vasco Almeida 
---
 Makefile  |  3 ++-
 git-add--interactive.perl | 24 
 perl/Git/I18N.pm  |  4 +++-
 t/t0202/test.pl   | 11 ++-
 4 files changed, 31 insertions(+), 11 deletions(-)

diff --git a/Makefile b/Makefile
index de5a030..eedf1fa 100644
--- a/Makefile
+++ b/Makefile
@@ -2061,7 +2061,8 @@ XGETTEXT_FLAGS_C = $(XGETTEXT_FLAGS) --language=C \
--keyword=_ --keyword=N_ --keyword="Q_:1,2"
 XGETTEXT_FLAGS_SH = $(XGETTEXT_FLAGS) --language=Shell \
--keyword=gettextln --keyword=eval_gettextln
-XGETTEXT_FLAGS_PERL = $(XGETTEXT_FLAGS) --keyword=__ --language=Perl
+XGETTEXT_FLAGS_PERL = $(XGETTEXT_FLAGS) --language=Perl \
+   --keyword=__ --keyword="Q__:1,2"
 LOCALIZED_C = $(C_OBJ:o=c) $(LIB_H) $(GENERATED_H)
 LOCALIZED_SH = $(SCRIPT_SH) git-parse-remote.sh
 LOCALIZED_PERL = $(SCRIPT_PERL)
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 4e1e857..08badfa 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -666,12 +666,18 @@ sub status_cmd {
 sub say_n_paths {
my $did = shift @_;
my $cnt = scalar @_;
-   print "$did ";
-   if (1 < $cnt) {
-   print "$cnt paths\n";
-   }
-   else {
-   print "one path\n";
+   if ($did eq 'added') {
+   printf(Q__("added one path\n", "added %d paths\n",
+  $cnt), $cnt);
+   } elsif ($did eq 'updated') {
+   printf(Q__("updated one path\n", "updated %d paths\n",
+  $cnt), $cnt);
+   } elsif ($did eq 'reversed') {
+   printf(Q__("reversed one path\n", "reversed %d paths\n",
+  $cnt), $cnt);
+   } else {
+   printf(Q__("touched one path\n", "touched %d paths\n",
+  $cnt), $cnt);
}
 }
 
@@ -1508,8 +1514,10 @@ sub patch_update_file {
elsif ($other =~ /s/ && $line =~ /^s/) {
my @split = split_hunk($hunk[$ix]{TEXT}, 
$hunk[$ix]{DISPLAY});
if (1 < @split) {
-   print colored $header_color, "Split 
into ",
-   scalar(@split), " hunks.\n";
+   print colored $header_color, sprintf(
+   Q__("Split into %d hunk.\n",
+   "Split into %d hunks.\n",
+   scalar(@split)), 
scalar(@split));
}
splice (@hunk, $ix, 1, @split);
$num = scalar @hunk;
diff --git a/perl/Git/I18N.pm b/perl/Git/I18N.pm
index f889fd6..5a75dd5 100644
--- a/perl/Git/I18N.pm
+++ b/perl/Git/I18N.pm
@@ -13,7 +13,7 @@ BEGIN {
}
 }
 
-our @EXPORT = qw(__);
+our @EXPORT = qw(__ Q__);
 our @EXPORT_OK = @EXPORT;
 
 sub __bootstrap_locale_messages {
@@ -44,6 +44,7 @@ BEGIN
eval {
__bootstrap_locale_messages();
*__ = \::Messages::gettext;
+   *Q__ = \::Messages::ngettext;
1;
} or do {
# Tell test.pl that we couldn't load the gettext library.
@@ -51,6 +52,7 @@ BEGIN
 
# Just a fall-through no-op
*__ = sub ($) { $_[0] };
+   *Q__ = sub ($$$) { $_[2] == 1 ? $_[0] : $_[1] };
};
 }
 
diff --git a/t/t0202/test.pl b/t/t0202/test.pl
index 2c10cb4..98aa9a3 100755
--- a/t/t0202/test.pl
+++ b/t/t0202/test.pl
@@ -4,7 +4,7 @@ use lib (split(/:/, $ENV{GITPERLLIB}));
 use strict;
 use warnings;
 use POSIX qw(:locale_h);
-use Test::More tests => 8;
+use Test::More tests => 11;
 use Git::I18N;
 
 my $has_gettext_library = $Git::I18N::__HAS_LIBRARY;
@@ -31,6 +31,7 @@ is_deeply(\@Git::I18N::EXPORT, \@Git::I18N::EXPORT_OK, 
"sanity: Git::I18N export
# more gettext wrapper functions.
my %prototypes = (qw(
__  $
+   Q__ $$$
));
while (my ($sub, $proto) = each %prototypes) {
is(prototype(\&{"Git::I18N::$sub"}), $proto, "sanity: $sub has 
a $proto prototype");
@@ -46,6 +47,14 @@ is_deeply(\@Git::I18N::EXPORT, \@Git::I18N::EXPORT_OK, 
"sanity: Git::I18N export
my ($got, $expect) = (('TEST: A Perl test string') x 2);
 
is(__($got), $expect, "Passing a string through __() in the C locale 
works");
+
+   my ($got_singular, $got_plural, $expect_singular, $expect_plural) =
+   (('TEST: 1 file', 'TEST: n files') x 2);
+
+   is(Q__($got_singular, $got_plural, 1), $expect_singular,
+

[PATCH v2 04/11] i18n: add--interactive: mark plural strings

2016-06-29 Thread Vasco Almeida 
Mark plural strings for translation.  Unfold each action case in one
entire sentence.

Pass new keyword for xgettext to extract.

Update test to include new subrotine Q__() for plural strings handling.

Signed-off-by: Vasco Almeida 
---
 Makefile  |  3 ++-
 git-add--interactive.perl | 24 
 perl/Git/I18N.pm  |  4 +++-
 t/t0202/test.pl   | 11 ++-
 4 files changed, 31 insertions(+), 11 deletions(-)

diff --git a/Makefile b/Makefile
index de5a030..eedf1fa 100644
--- a/Makefile
+++ b/Makefile
@@ -2061,7 +2061,8 @@ XGETTEXT_FLAGS_C = $(XGETTEXT_FLAGS) --language=C \
--keyword=_ --keyword=N_ --keyword="Q_:1,2"
 XGETTEXT_FLAGS_SH = $(XGETTEXT_FLAGS) --language=Shell \
--keyword=gettextln --keyword=eval_gettextln
-XGETTEXT_FLAGS_PERL = $(XGETTEXT_FLAGS) --keyword=__ --language=Perl
+XGETTEXT_FLAGS_PERL = $(XGETTEXT_FLAGS) --language=Perl \
+   --keyword=__ --keyword="Q__:1,2"
 LOCALIZED_C = $(C_OBJ:o=c) $(LIB_H) $(GENERATED_H)
 LOCALIZED_SH = $(SCRIPT_SH) git-parse-remote.sh
 LOCALIZED_PERL = $(SCRIPT_PERL)
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 4e1e857..08badfa 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -666,12 +666,18 @@ sub status_cmd {
 sub say_n_paths {
my $did = shift @_;
my $cnt = scalar @_;
-   print "$did ";
-   if (1 < $cnt) {
-   print "$cnt paths\n";
-   }
-   else {
-   print "one path\n";
+   if ($did eq 'added') {
+   printf(Q__("added one path\n", "added %d paths\n",
+  $cnt), $cnt);
+   } elsif ($did eq 'updated') {
+   printf(Q__("updated one path\n", "updated %d paths\n",
+  $cnt), $cnt);
+   } elsif ($did eq 'reversed') {
+   printf(Q__("reversed one path\n", "reversed %d paths\n",
+  $cnt), $cnt);
+   } else {
+   printf(Q__("touched one path\n", "touched %d paths\n",
+  $cnt), $cnt);
}
 }
 
@@ -1508,8 +1514,10 @@ sub patch_update_file {
elsif ($other =~ /s/ && $line =~ /^s/) {
my @split = split_hunk($hunk[$ix]{TEXT}, 
$hunk[$ix]{DISPLAY});
if (1 < @split) {
-   print colored $header_color, "Split 
into ",
-   scalar(@split), " hunks.\n";
+   print colored $header_color, sprintf(
+   Q__("Split into %d hunk.\n",
+   "Split into %d hunks.\n",
+   scalar(@split)), 
scalar(@split));
}
splice (@hunk, $ix, 1, @split);
$num = scalar @hunk;
diff --git a/perl/Git/I18N.pm b/perl/Git/I18N.pm
index f889fd6..5a75dd5 100644
--- a/perl/Git/I18N.pm
+++ b/perl/Git/I18N.pm
@@ -13,7 +13,7 @@ BEGIN {
}
 }
 
-our @EXPORT = qw(__);
+our @EXPORT = qw(__ Q__);
 our @EXPORT_OK = @EXPORT;
 
 sub __bootstrap_locale_messages {
@@ -44,6 +44,7 @@ BEGIN
eval {
__bootstrap_locale_messages();
*__ = \::Messages::gettext;
+   *Q__ = \::Messages::ngettext;
1;
} or do {
# Tell test.pl that we couldn't load the gettext library.
@@ -51,6 +52,7 @@ BEGIN
 
# Just a fall-through no-op
*__ = sub ($) { $_[0] };
+   *Q__ = sub ($$$) { $_[2] == 1 ? $_[0] : $_[1] };
};
 }
 
diff --git a/t/t0202/test.pl b/t/t0202/test.pl
index 2c10cb4..98aa9a3 100755
--- a/t/t0202/test.pl
+++ b/t/t0202/test.pl
@@ -4,7 +4,7 @@ use lib (split(/:/, $ENV{GITPERLLIB}));
 use strict;
 use warnings;
 use POSIX qw(:locale_h);
-use Test::More tests => 8;
+use Test::More tests => 11;
 use Git::I18N;
 
 my $has_gettext_library = $Git::I18N::__HAS_LIBRARY;
@@ -31,6 +31,7 @@ is_deeply(\@Git::I18N::EXPORT, \@Git::I18N::EXPORT_OK, 
"sanity: Git::I18N export
# more gettext wrapper functions.
my %prototypes = (qw(
__  $
+   Q__ $$$
));
while (my ($sub, $proto) = each %prototypes) {
is(prototype(\&{"Git::I18N::$sub"}), $proto, "sanity: $sub has 
a $proto prototype");
@@ -46,6 +47,14 @@ is_deeply(\@Git::I18N::EXPORT, \@Git::I18N::EXPORT_OK, 
"sanity: Git::I18N export
my ($got, $expect) = (('TEST: A Perl test string') x 2);
 
is(__($got), $expect, "Passing a string through __() in the C locale 
works");
+
+   my ($got_singular, $got_plural, $expect_singular, $expect_plural) =
+   (('TEST: 1 file', 'TEST: n files') x 2);
+
+   is(Q__($got_singular, $got_plural, 1), $expect_singular,
+