threads / patch / 32630

patcham: invoke perl's strftime in C locale

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

## tl;dr

13 messages between Jan 14, 2013 and Jan 20, 2013. Diffs are folded; open one to read it.

replies: 12people: 4as markdown or json

Dmitry V. Levin· Jan 14, 2013, 20:59 UTC · lore

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(-)
Show changes to git-am.sh +1 −1
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
Junio C Hamano· Jan 14, 2013, 21:49 UTC · re: Dmitry V. Levin · lore

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

"Dmitry V. Levin" <ldv@altlinux.org> writes:
Show 5 quoted lines
> 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.
Show 16 quoted lines
>  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 /) {
Dmitry V. Levin· Jan 14, 2013, 22:36 UTC · re: Junio C Hamano · lore

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

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(-)
Show changes to git-am.sh +1 −1
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
Jeff King· Jan 15, 2013, 15:59 UTC · re: Dmitry V. Levin · lore

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

On Tue, Jan 15, 2013 at 12:59:33AM +0400, Dmitry V. Levin wrote:
Show 13 quoted lines
> 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
Antoine Pelisse· Jan 15, 2013, 16:42 UTC · re: Jeff King · lore

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

Show 5 quoted lines
> 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.
Jeff King· Jan 15, 2013, 16:50 UTC · re: Antoine Pelisse · lore

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

On Tue, Jan 15, 2013 at 05:42:12PM +0100, Antoine Pelisse wrote:
Show 7 quoted lines
> > 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
Dmitry V. Levin· Jan 15, 2013, 17:40 UTC · re: Jeff King · lore

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

On Tue, Jan 15, 2013 at 08:50:59AM -0800, Jeff King wrote:
Show 12 quoted lines
> 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
Dmitry V. Levin· Jan 15, 2013, 19:05 UTC · re: Dmitry V. Levin · lore

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

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(-)
Show changes to git-am.sh +2 −1
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
Junio C Hamano· Jan 18, 2013, 20:36 UTC · re: Dmitry V. Levin · lore

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

"Dmitry V. Levin" <ldv@altlinux.org> writes:
Show 26 quoted lines
> 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 /) {
Jeff King· Jan 19, 2013, 16:39 UTC · re: Junio C Hamano · lore

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

On Fri, Jan 18, 2013 at 12:36:46PM -0800, Junio C Hamano wrote:
Show 14 quoted lines
> > 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
Dmitry V. Levin· Jan 19, 2013, 20:28 UTC · re: Junio C Hamano · lore

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

On Fri, Jan 18, 2013 at 12:36:46PM -0800, Junio C Hamano wrote:
Show 31 quoted lines
> "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
Junio C Hamano· Jan 20, 2013, 17:47 UTC · re: Dmitry V. Levin · lore

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

"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...

Junio C Hamano· Jan 15, 2013, 19:14 UTC · re: Dmitry V. Levin · lore

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

"Dmitry V. Levin" <ldv@altlinux.org> writes:
Show 24 quoted lines
> 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.

← back to recent threads