# [PATCH] am: invoke perl's strftime in C locale 13 messages from 2013-01-14 to 2013-01-20. Participants: Dmitry V. Levin, Junio C Hamano, Jeff King, Antoine Pelisse. Thread: https://gitlist.dev/t/32630 ## Dmitry V. Levin, 2013-01-14 20:59 Subject: [PATCH] am: invoke perl's strftime in C locale Message-ID: <20130114205933.GA25947@altlinux.org> URL: https://gitlist.dev/e/20130114205933.GA25947%40altlinux.org ``` 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 --- 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 ``` ## Junio C Hamano, 2013-01-14 21:49 Subject: Re: [PATCH] am: invoke perl's strftime in C locale Message-ID: <7vr4ln8mgd.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vr4ln8mgd.fsf%40alter.siamese.dyndns.org In-Reply-To: <20130114205933.GA25947@altlinux.org> ``` "Dmitry V. Levin" 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 > --- 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 /) { ``` ## Dmitry V. Levin, 2013-01-14 22:36 Subject: [PATCH v2] am: invoke perl's strftime in C locale Message-ID: <20130114223651.GA26443@altlinux.org> URL: https://gitlist.dev/e/20130114223651.GA26443%40altlinux.org In-Reply-To: <7vr4ln8mgd.fsf@alter.siamese.dyndns.org> ``` 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 Signed-off-by: Dmitry V. Levin --- 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 ``` ## Jeff King, 2013-01-15 15:59 Subject: Re: [PATCH] am: invoke perl's strftime in C locale Message-ID: <20130115155953.GB21815@sigill.intra.peff.net> URL: https://gitlist.dev/e/20130115155953.GB21815%40sigill.intra.peff.net In-Reply-To: <20130114205933.GA25947@altlinux.org> ``` 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 ``` ## Antoine Pelisse, 2013-01-15 16:42 Subject: Re: [PATCH] am: invoke perl's strftime in C locale Message-ID: URL: https://gitlist.dev/e/CALWbr2w%2Bq5%3DZ8__g%2BJ_s2NtTMgziHrntFqsi8vCJyvfO2qi81A%40mail.gmail.com In-Reply-To: <20130115155953.GB21815@sigill.intra.peff.net> ``` > 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, 2013-01-15 16:50 Subject: Re: [PATCH] am: invoke perl's strftime in C locale Message-ID: <20130115165058.GA29301@sigill.intra.peff.net> URL: https://gitlist.dev/e/20130115165058.GA29301%40sigill.intra.peff.net In-Reply-To: ``` 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 ``` ## Dmitry V. Levin, 2013-01-15 17:40 Subject: Re: [PATCH] am: invoke perl's strftime in C locale Message-ID: <20130115174015.GA7471@altlinux.org> URL: https://gitlist.dev/e/20130115174015.GA7471%40altlinux.org In-Reply-To: <20130115165058.GA29301@sigill.intra.peff.net> ``` 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 ``` ## Dmitry V. Levin, 2013-01-15 19:05 Subject: [PATCH v3] am: invoke perl's strftime in C locale Message-ID: <20130115190517.GB7963@altlinux.org> URL: https://gitlist.dev/e/20130115190517.GB7963%40altlinux.org In-Reply-To: <20130115174015.GA7471@altlinux.org> ``` 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 Signed-off-by: Dmitry V. Levin --- 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 ``` ## Junio C Hamano, 2013-01-15 19:14 Subject: Re: [PATCH] am: invoke perl's strftime in C locale Message-ID: <7v1udm45st.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7v1udm45st.fsf%40alter.siamese.dyndns.org In-Reply-To: <20130115174015.GA7471@altlinux.org> ``` "Dmitry V. Levin" 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. ``` ## Junio C Hamano, 2013-01-18 20:36 Subject: Re: [PATCH v3] am: invoke perl's strftime in C locale Message-ID: <7vehhiqlcx.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vehhiqlcx.fsf%40alter.siamese.dyndns.org In-Reply-To: <20130115190517.GB7963@altlinux.org> ``` "Dmitry V. Levin" 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 > Signed-off-by: Dmitry V. Levin > --- > > 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, 2013-01-19 16:39 Subject: Re: [PATCH v3] am: invoke perl's strftime in C locale Message-ID: <20130119163940.GA12307@sigill.intra.peff.net> URL: https://gitlist.dev/e/20130119163940.GA12307%40sigill.intra.peff.net In-Reply-To: <7vehhiqlcx.fsf@alter.siamese.dyndns.org> ``` 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 ``` ## Dmitry V. Levin, 2013-01-19 20:28 Subject: Re: [PATCH v3] am: invoke perl's strftime in C locale Message-ID: <20130119202853.GD1652@altlinux.org> URL: https://gitlist.dev/e/20130119202853.GD1652%40altlinux.org In-Reply-To: <7vehhiqlcx.fsf@alter.siamese.dyndns.org> ``` On Fri, Jan 18, 2013 at 12:36:46PM -0800, Junio C Hamano wrote: > "Dmitry V. Levin" 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 > > Signed-off-by: Dmitry V. Levin > > --- > > > > 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, 2013-01-20 17:47 Subject: Re: [PATCH v3] am: invoke perl's strftime in C locale Message-ID: <7vvcarn3v9.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vvcarn3v9.fsf%40alter.siamese.dyndns.org In-Reply-To: <20130119202853.GD1652@altlinux.org> ``` "Dmitry V. Levin" 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... ```