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

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 18, 2013, 20:36 UTC
Message-ID: <7vehhiqlcx.fsf@alter.siamese.dyndns.org>
In-Reply-To: <20130115190517.GB7963@altlinux.org>

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