[PATCH] am: invoke perl's strftime in C locale

[PATCH] am: invoke perl's strftime in C locale

[Dmitry V. Levin]

This fixes "hg" patch format support for locales other than C and en_*,
see https://bugzilla.altlinux.org/show_bug.cgi?id=28248

Signed-off-by: Dmitry V. Levin <ldv@altlinux.org>
---
 git-am.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git-am.sh b/git-am.sh
index c682d34..64b88e4 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -334,7 +334,7 @@ split_patches () {
 			# Since we cannot guarantee that the commit message is in
 			# git-friendly format, we put no Subject: line and just consume
 			# all of the message as the body
-			perl -M'POSIX qw(strftime)' -ne 'BEGIN { $subject = 0 }
+			LC_ALL=C perl -M'POSIX qw(strftime)' -ne 'BEGIN { $subject = 0 }
 				if ($subject) { print ; }
 				elsif (/^\# User /) { s/\# User/From:/ ; print ; }
 				elsif (/^\# Date /) {

-- 
ldv

Re: [PATCH] am: invoke perl's strftime in C locale

[Junio C Hamano]

"Dmitry V. Levin" <ldv@altlinux.org> writes:

> This fixes "hg" patch format support for locales other than C and en_*,
> see https://bugzilla.altlinux.org/show_bug.cgi?id=28248
>
> Signed-off-by: Dmitry V. Levin <ldv@altlinux.org>
> ---

Thanks.

The reference URL is not very friendly, and you should be able to
state it here on a single line in English instead, I think.

The patch looks correct, though.

>  git-am.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/git-am.sh b/git-am.sh
> index c682d34..64b88e4 100755
> --- a/git-am.sh
> +++ b/git-am.sh
> @@ -334,7 +334,7 @@ split_patches () {
>  			# Since we cannot guarantee that the commit message is in
>  			# git-friendly format, we put no Subject: line and just consume
>  			# all of the message as the body
> -			perl -M'POSIX qw(strftime)' -ne 'BEGIN { $subject = 0 }
> +			LC_ALL=C perl -M'POSIX qw(strftime)' -ne 'BEGIN { $subject = 0 }
>  				if ($subject) { print ; }
>  				elsif (/^\# User /) { s/\# User/From:/ ; print ; }
>  				elsif (/^\# Date /) {

[PATCH v2] am: invoke perl's strftime in C locale

[Dmitry V. Levin]

This fixes "hg" patch format support for locales other than C and en_*.
Before the change, git-am was making "Date:" line from hg changeset
metadata according to the current locale, and this line was rejected
later with "invalid date format" diagnostics because localized date
strings are not supported.

Reported-by: Gleb Fotengauer-Malinovskiy <glebfm@altlinux.org>
Signed-off-by: Dmitry V. Levin <ldv@altlinux.org>
---

 v2: replaced "unfriendly" URL with a short description

 git-am.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git-am.sh b/git-am.sh
index c682d34..64b88e4 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -334,7 +334,7 @@ split_patches () {
 			# Since we cannot guarantee that the commit message is in
 			# git-friendly format, we put no Subject: line and just consume
 			# all of the message as the body
-			perl -M'POSIX qw(strftime)' -ne 'BEGIN { $subject = 0 }
+			LC_ALL=C perl -M'POSIX qw(strftime)' -ne 'BEGIN { $subject = 0 }
 				if ($subject) { print ; }
 				elsif (/^\# User /) { s/\# User/From:/ ; print ; }
 				elsif (/^\# Date /) {

-- 
ldv

Re: [PATCH] am: invoke perl's strftime in C locale

[Jeff King]

On Tue, Jan 15, 2013 at 12:59:33AM +0400, Dmitry V. Levin wrote:

> diff --git a/git-am.sh b/git-am.sh
> index c682d34..64b88e4 100755
> --- a/git-am.sh
> +++ b/git-am.sh
> @@ -334,7 +334,7 @@ split_patches () {
>  			# Since we cannot guarantee that the commit message is in
>  			# git-friendly format, we put no Subject: line and just consume
>  			# all of the message as the body
> -			perl -M'POSIX qw(strftime)' -ne 'BEGIN { $subject = 0 }
> +			LC_ALL=C perl -M'POSIX qw(strftime)' -ne 'BEGIN { $subject = 0 }
>  				if ($subject) { print ; }
>  				elsif (/^\# User /) { s/\# User/From:/ ; print ; }
>  				elsif (/^\# Date /) {

This puts all of perl into the C locale, which would mean error messages
from perl would be in English rather than the user's language. It
probably isn't a big deal, because that snippet of perl is short and not
likely to produce problems, but I wonder how hard it would be to set the
locale just for the strftime call.

-Peff

Re: [PATCH] am: invoke perl's strftime in C locale

[Antoine Pelisse]

> This puts all of perl into the C locale, which would mean error messages
> from perl would be in English rather than the user's language. It
> probably isn't a big deal, because that snippet of perl is short and not
> likely to produce problems, but I wonder how hard it would be to set the
> locale just for the strftime call.

Maybe just setting LC_TIME to C would do ...

>From locale(7) man page:

       LC_TIME
              changes  the behavior of the strftime(3) function to
display the current time in a locally acceptable form; for
              example, most of Europe uses a 24-hour clock versus the
12-hour clock used in the United States.

Re: [PATCH] am: invoke perl's strftime in C locale

[Jeff King]

On Tue, Jan 15, 2013 at 05:42:12PM +0100, Antoine Pelisse wrote:

> > This puts all of perl into the C locale, which would mean error messages
> > from perl would be in English rather than the user's language. It
> > probably isn't a big deal, because that snippet of perl is short and not
> > likely to produce problems, but I wonder how hard it would be to set the
> > locale just for the strftime call.
> 
> Maybe just setting LC_TIME to C would do ...

Yeah, that is a nice simple solution. Dmitry, does just setting LC_TIME
fix the problem for you?

-Peff

Re: [PATCH] am: invoke perl's strftime in C locale

[Dmitry V. Levin]

[-- Attachment #1: Type: text/plain, Size: 1150 bytes --]

On Tue, Jan 15, 2013 at 08:50:59AM -0800, Jeff King wrote:
> On Tue, Jan 15, 2013 at 05:42:12PM +0100, Antoine Pelisse wrote:
> 
> > > This puts all of perl into the C locale, which would mean error messages
> > > from perl would be in English rather than the user's language. It
> > > probably isn't a big deal, because that snippet of perl is short and not
> > > likely to produce problems, but I wonder how hard it would be to set the
> > > locale just for the strftime call.
> > 
> > Maybe just setting LC_TIME to C would do ...
> 
> Yeah, that is a nice simple solution. Dmitry, does just setting LC_TIME
> fix the problem for you?

Just setting LC_TIME environment variable instead of LC_ALL would end up
with unreliable solution because LC_ALL has the highest priority.

If keeping error messages from perl has the utmost importance, it could be
achieved by
-			perl -M'POSIX qw(strftime)' -ne 'BEGIN { $subject = 0 }
+			perl -M'POSIX qw(strftime :locale_h)' -ne '
+				BEGIN { setlocale(LC_TIME, "C"); $subject = 0 }
but the little perl helper script we are talking about hardly worths so
much efforts.


-- 
ldv

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

[PATCH v3] am: invoke perl's strftime in C locale

[Dmitry V. Levin]

This fixes "hg" patch format support for locales other than C and en_*.
Before the change, git-am was making "Date:" line from hg changeset
metadata according to the current locale, and this line was rejected
later with "invalid date format" diagnostics because localized date
strings are not supported.

Reported-by: Gleb Fotengauer-Malinovskiy <glebfm@altlinux.org>
Signed-off-by: Dmitry V. Levin <ldv@altlinux.org>
---

 v3: alternative implementation using setlocale(LC_TIME, "C")

 git-am.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/git-am.sh b/git-am.sh
index c682d34..8677d8c 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -334,7 +334,8 @@ split_patches () {
 			# Since we cannot guarantee that the commit message is in
 			# git-friendly format, we put no Subject: line and just consume
 			# all of the message as the body
-			perl -M'POSIX qw(strftime)' -ne 'BEGIN { $subject = 0 }
+			perl -M'POSIX qw(strftime :locale_h)' -ne '
+				BEGIN { setlocale(LC_TIME, "C"); $subject = 0 }
 				if ($subject) { print ; }
 				elsif (/^\# User /) { s/\# User/From:/ ; print ; }
 				elsif (/^\# Date /) {

-- 
ldv

Re: [PATCH v3] am: invoke perl's strftime in C locale

[Junio C Hamano]

"Dmitry V. Levin" <ldv@altlinux.org> writes:

> This fixes "hg" patch format support for locales other than C and en_*.
> Before the change, git-am was making "Date:" line from hg changeset
> metadata according to the current locale, and this line was rejected
> later with "invalid date format" diagnostics because localized date
> strings are not supported.
>
> Reported-by: Gleb Fotengauer-Malinovskiy <glebfm@altlinux.org>
> Signed-off-by: Dmitry V. Levin <ldv@altlinux.org>
> ---
>
>  v3: alternative implementation using setlocale(LC_TIME, "C")
>
>  git-am.sh | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/git-am.sh b/git-am.sh
> index c682d34..8677d8c 100755
> --- a/git-am.sh
> +++ b/git-am.sh
> @@ -334,7 +334,8 @@ split_patches () {
>  			# Since we cannot guarantee that the commit message is in
>  			# git-friendly format, we put no Subject: line and just consume
>  			# all of the message as the body
> -			perl -M'POSIX qw(strftime)' -ne 'BEGIN { $subject = 0 }
> +			perl -M'POSIX qw(strftime :locale_h)' -ne '
> +				BEGIN { setlocale(LC_TIME, "C"); $subject = 0 }

I still haven't convinced myself that this is an improvement over
the simple "LC_ALL=C LANG=C perl ..." approach.

This alternative might be theoretically more correct if we cared
about the error and other messages from this Perl invocation, but it
requires that everybody's Perl implementation correctly supports the
additional -M'POSIX ":locale_h"' and "setlocale(LC_TIME, ...)".

I am tempted to use the previous one that puts the whole process
under LC_ALL=C instead, unless I hear a "we already depend on that
elsewhere, look at $that_code".

Thanks.

>  				if ($subject) { print ; }
>  				elsif (/^\# User /) { s/\# User/From:/ ; print ; }
>  				elsif (/^\# Date /) {

Re: [PATCH v3] am: invoke perl's strftime in C locale

[Jeff King]

On Fri, Jan 18, 2013 at 12:36:46PM -0800, Junio C Hamano wrote:

> > diff --git a/git-am.sh b/git-am.sh
> > index c682d34..8677d8c 100755
> > --- a/git-am.sh
> > +++ b/git-am.sh
> > @@ -334,7 +334,8 @@ split_patches () {
> >  			# Since we cannot guarantee that the commit message is in
> >  			# git-friendly format, we put no Subject: line and just consume
> >  			# all of the message as the body
> > -			perl -M'POSIX qw(strftime)' -ne 'BEGIN { $subject = 0 }
> > +			perl -M'POSIX qw(strftime :locale_h)' -ne '
> > +				BEGIN { setlocale(LC_TIME, "C"); $subject = 0 }
> 
> I still haven't convinced myself that this is an improvement over
> the simple "LC_ALL=C LANG=C perl ..." approach.

Yeah, I was the one who brought it up, but I think I was probably being
too nit-picky. It almost certainly doesn't matter, and the alternatives
are just as likely to cause problems.

> I am tempted to use the previous one that puts the whole process
> under LC_ALL=C instead, unless I hear a "we already depend on that
> elsewhere, look at $that_code".

I'm fine with that.

-Peff

Re: [PATCH v3] am: invoke perl's strftime in C locale

[Dmitry V. Levin]

[-- Attachment #1: Type: text/plain, Size: 1600 bytes --]

On Fri, Jan 18, 2013 at 12:36:46PM -0800, Junio C Hamano wrote:
> "Dmitry V. Levin" <ldv@altlinux.org> writes:
> 
> > This fixes "hg" patch format support for locales other than C and en_*.
> > Before the change, git-am was making "Date:" line from hg changeset
> > metadata according to the current locale, and this line was rejected
> > later with "invalid date format" diagnostics because localized date
> > strings are not supported.
> >
> > Reported-by: Gleb Fotengauer-Malinovskiy <glebfm@altlinux.org>
> > Signed-off-by: Dmitry V. Levin <ldv@altlinux.org>
> > ---
> >
> >  v3: alternative implementation using setlocale(LC_TIME, "C")
> >
> >  git-am.sh | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/git-am.sh b/git-am.sh
> > index c682d34..8677d8c 100755
> > --- a/git-am.sh
> > +++ b/git-am.sh
> > @@ -334,7 +334,8 @@ split_patches () {
> >  			# Since we cannot guarantee that the commit message is in
> >  			# git-friendly format, we put no Subject: line and just consume
> >  			# all of the message as the body
> > -			perl -M'POSIX qw(strftime)' -ne 'BEGIN { $subject = 0 }
> > +			perl -M'POSIX qw(strftime :locale_h)' -ne '
> > +				BEGIN { setlocale(LC_TIME, "C"); $subject = 0 }
> 
> I still haven't convinced myself that this is an improvement over
> the simple "LC_ALL=C LANG=C perl ..." approach.

Personally I prefer 2nd edition that is simpler and does the right thing
(not that LC_ALL=C is necessary and sufficient, you neither need to add
things like LANG=C nor can relax it to LC_TIME=C).


-- 
ldv

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH v3] am: invoke perl's strftime in C locale

[Junio C Hamano]

"Dmitry V. Levin" <ldv@altlinux.org> writes:

> Personally I prefer 2nd edition that is simpler and does the right thing
> (not that LC_ALL=C is necessary and sufficient, you neither need to add
> things like LANG=C nor can relax it to LC_TIME=C).

I guess everybody involved is in agreement, then.

Just FYI, "LC_ALL=C LANG=C" comes from the inertia dating back when
not everybody understood LC_*; I do not personally know of a system
that will be helped by the extra LANG=C these days, but I know it
will not hurt anybody, so...

Re: [PATCH] am: invoke perl's strftime in C locale

[Junio C Hamano]

"Dmitry V. Levin" <ldv@altlinux.org> writes:

> On Tue, Jan 15, 2013 at 08:50:59AM -0800, Jeff King wrote:
>> On Tue, Jan 15, 2013 at 05:42:12PM +0100, Antoine Pelisse wrote:
>> 
>> > > This puts all of perl into the C locale, which would mean error messages
>> > > from perl would be in English rather than the user's language. It
>> > > probably isn't a big deal, because that snippet of perl is short and not
>> > > likely to produce problems, but I wonder how hard it would be to set the
>> > > locale just for the strftime call.
>> > 
>> > Maybe just setting LC_TIME to C would do ...
>> 
>> Yeah, that is a nice simple solution. Dmitry, does just setting LC_TIME
>> fix the problem for you?
>
> Just setting LC_TIME environment variable instead of LC_ALL would end up
> with unreliable solution because LC_ALL has the highest priority.
>
> If keeping error messages from perl has the utmost importance, it could be
> achieved by
> -			perl -M'POSIX qw(strftime)' -ne 'BEGIN { $subject = 0 }
> +			perl -M'POSIX qw(strftime :locale_h)' -ne '
> +				BEGIN { setlocale(LC_TIME, "C"); $subject = 0 }
> but the little perl helper script we are talking about hardly worths so
> much efforts.

Yeah I agree that this is not worth it, I would think.