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

"Dmitry V. Levin" [off-list ref] writes:

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 /) {

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

quoted

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

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

"Dmitry V. Levin" [off-list ref] writes:

quoted

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 <redacted>
Signed-off-by: Dmitry V. Levin <redacted>
---

 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