PATCH: Restore `LC_ALL' when setting `LANG'

PATCH: Restore `LC_ALL' when setting `LANG'

[Frank Terbeck]

(Cc:ing Upstream)

When setting LANG, zsh sets LC_ALL and then goes on to restore all
interesting LC_* values after that. Up to now, it did not restore LC_ALL
itself, though.

That could lead to trouble when setting LC_ALL=something-utf8-ish and
after that LANG=something-non-utf8.

This was reported in Debian bugreport #654225:

  <http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=654225>

A recipe to reproduce the issue is here:

  <http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=654225#65>

I think the patch below fixes the issue correctly, by restoring LC_ALL
from its parameter along the way with all the other LC_* values zsh
cares about.

(FWIW, the test-suite still passes with this change.)

Regards, Frank


---
 Src/params.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/Src/params.c b/Src/params.c
index 446cccc..6d98b8e 100644
--- a/Src/params.c
+++ b/Src/params.c
@@ -3780,6 +3780,9 @@ static struct localename {
 #ifdef LC_TIME
     {"LC_TIME", LC_TIME},
 #endif
+#ifdef LC_ALL
+    {"LC_ALL", LC_ALL},
+#endif
     {NULL, 0}
 };
 
-- 
1.7.8.rc3.17.gf56ef1

Re: PATCH: Restore `LC_ALL' when setting `LANG'

[Vincent Lefevre]

On 2012-01-03 16:26:44 +0100, Frank Terbeck wrote:
> I think the patch below fixes the issue correctly, by restoring LC_ALL
> from its parameter along the way with all the other LC_* values zsh
> cares about.

I don't understand why zsh has anything to do with environment
variables to detect the charset (if I understood correctly).
Why not using nl_langinfo(CODESET)?

-- 
Vincent Lefèvre <vincent@vinc17.net> - Web: <http://www.vinc17.net/>
100% accessible validated (X)HTML - Blog: <http://www.vinc17.net/blog/>
Work: CR INRIA - computer arithmetic / Arénaire project (LIP, ENS-Lyon)

Re: PATCH: Restore `LC_ALL' when setting `LANG'

[Peter Stephenson]

On 2012-01-03 16:26:44 +0100, Frank Terbeck wrote:
> I think the patch below fixes the issue correctly, by restoring LC_ALL
> from its parameter along the way with all the other LC_* values zsh
> cares about.

Looks plausible, without following through all the implications.

On Wed, 4 Jan 2012 03:16:41 +0100
Vincent Lefevre <vincent@vinc17.net> wrote:
> I don't understand why zsh has anything to do with environment
> variables to detect the charset (if I understood correctly).
> Why not using nl_langinfo(CODESET)?

Intercepting the setting of environment variables is mostly to ensure
setlocale() gets run when necessary.

-- 
Peter Stephenson <p.w.stephenson@ntlworld.com>
Web page now at http://homepage.ntlworld.com/p.w.stephenson/

Re: PATCH: Restore `LC_ALL' when setting `LANG'

[Jun T.]

Instead of adding LC_ALL to lc_names[], I think it better
not calling setlocale() at all if LC_ALL is non-empty.

Index: Src/params.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/params.c,v
retrieving revision 1.175
diff -u -r1.175 params.c
--- Src/params.c	11 Aug 2011 19:32:50 -0000	1.175
+++ Src/params.c	6 Jan 2012 05:42:12 -0000
@@ -3788,6 +3788,10 @@
 setlang(char *x)
 {
     struct localename *ln;
+    char *x2;
+
+    if ((x2 = getsparam("LC_ALL")) && *x2)
+	return;
 
     /*
      * Set the global locale to the value passed, but override

Re: PATCH: Restore `LC_ALL' when setting `LANG'

[Peter Stephenson]

On Fri, 6 Jan 2012 15:03:54 +0900
"Jun T." <takimoto-j@kba.biglobe.ne.jp> wrote:
> Instead of adding LC_ALL to lc_names[], I think it better
> not calling setlocale() at all if LC_ALL is non-empty.

I can see that's a reasonably minimal change.  As long as Frank's happy
that fixes the problem, I can apply it.

-- 
Peter Stephenson <p.w.stephenson@ntlworld.com>
Web page now at http://homepage.ntlworld.com/p.w.stephenson/

Re: PATCH: Restore `LC_ALL' when setting `LANG'

[Frank Terbeck]

Peter Stephenson wrote:
> On Fri, 6 Jan 2012 15:03:54 +0900
> "Jun T." <takimoto-j@kba.biglobe.ne.jp> wrote:
>> Instead of adding LC_ALL to lc_names[], I think it better
>> not calling setlocale() at all if LC_ALL is non-empty.
>
> I can see that's a reasonably minimal change.  As long as Frank's happy
> that fixes the problem, I can apply it.

I don't know if it's the better fix, but then I'm pretty ignorant about
matters of `locale'... I can confirm that Jun's patch seems to fix the
bug as well. So I don't mind either way.

Regards, Frank