vcs_info and locales

vcs_info and locales

[François Gannaz]

Hi

As I installed my zsh prompt on another computer, vcs_info stopped working for
svn repositories, it didn't print the branch nor the revision. It was just
because I had a french locale, and so vcs_info could not parse the output of
"svn info". The locale had no effect on hg/git, but I didn't fully test them.

So I suggest either to fix the documentation or (better) fix the code.

In the documentation, for the example in the quickstart:
- precmd () { vcs_info }
+ precmd () { LC_ALL=C vcs_info }

In the code:

--- VCS_Info/Backends/VCS_INFO_get_data_svn	2010-02-21 20:48:23.000000000 +0100
+++ VCS_INFO_get_data_svn	2010-04-24 23:28:03.000000000 +0200
@@ -10,10 +10,10 @@
 
 svnbase=".";
 svninfo=()
-${vcs_comm[cmd]} info --non-interactive | while IFS=: read a b; do svninfo[${a// /_}]="${b## #}"; done
+LC_ALL=C ${vcs_comm[cmd]} info --non-interactive | while IFS=: read a b; do svninfo[${a// /_}]="${b## #}"; done
 while [[ -d "${svnbase}/../.svn" ]]; do
     parentinfo=()
-    ${vcs_comm[cmd]} info --non-interactive "${svnbase}/.." | while IFS=: read a b; do parentinfo[${a// /_}]="${b## #}"; done
+    LC_ALL=C ${vcs_comm[cmd]} info --non-interactive "${svnbase}/.." | while IFS=: read a b; do parentinfo[${a// /_}]="${b## #}"; done
     [[ ${parentinfo[Repository_UUID]} != ${svninfo[Repository_UUID]} ]] && break
     svninfo=(${(kv)parentinfo})
     svnbase="${svnbase}/.."

BTW, changing the command with:
	zstyle ':vcs_info:svn:*:-all-' command "LC_ALL=C svn"
didn't work at all, and I don't understand why.

Regards
--
François

Re: vcs_info and locales

[Frank Terbeck]

François Gannaz wrote:
> As I installed my zsh prompt on another computer, vcs_info stopped working for
> svn repositories, it didn't print the branch nor the revision. It was just
> because I had a french locale, and so vcs_info could not parse the output of
> "svn info". The locale had no effect on hg/git, but I didn't fully test them.
>
> So I suggest either to fix the documentation or (better) fix the code.

Yes, this could happen. Thanks for reporting. Obviously, fixing the code
is the superior solution. However, I'd like to do it universally for all
of `vcs_info' and not only for the invocation of "svn info".

[...]
> BTW, changing the command with:
> 	zstyle ':vcs_info:svn:*:-all-' command "LC_ALL=C svn"
> didn't work at all, and I don't understand why.

The reason for that is because we're using the command roughly like
this:  ${command} ...

And when sh_split_word isn't set (which it isn't by default and vcs_info
uses mostly zsh defaults), the parameter isn't split, like it would in
other bourne-like shells.

I think it would be best to leave it as it is; but I don't feel strongly
about it. If a lot be people think we should split that parameter, we
could do that (even though it might break backwards compatibility for
some people).


Anyway, could you try the following patch for the locale problem? I
think it should solve the issue once and for all.

---------- >8 ----------

>From 66be07ae53e9af6e386ade123cc9a1450695c7bc Mon Sep 17 00:00:00 2001
From: Frank Terbeck <ft@bewatermyfriend.org>
Date: Sun, 25 Apr 2010 10:20:30 +0200
Subject: PATCH: vcs_info: Set LC_ALL locally
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This is to avoid problems with output from external commands with non-C
locales as reported by François Gannaz in workers-27900.
---
 Functions/VCS_Info/vcs_info |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/Functions/VCS_Info/vcs_info b/Functions/VCS_Info/vcs_info
index a9e65e1..e9f782a 100644
--- a/Functions/VCS_Info/vcs_info
+++ b/Functions/VCS_Info/vcs_info
@@ -47,11 +47,12 @@ vcs_info () {
     local pat
     local -i found
     local -a enabled disabled dps
-    local -x usercontext vcs rrn
+    local -x usercontext vcs rrn LC_ALL
     local -ix maxexports
     local -ax msgs
     local -Ax vcs_comm
 
+    LC_ALL=C
     vcs='-init-'; rrn='-all-'
     usercontext=${1:-default}
 
-- 
1.7.0

Re: vcs_info and locales

[François Gannaz]

Le 2010-04-25, Frank Terbeck <ft@bewatermyfriend.org> a écrit :
> François Gannaz wrote:
> [...]
> > BTW, changing the command with:
> > 	zstyle ':vcs_info:svn:*:-all-' command "LC_ALL=C svn"
> > didn't work at all, and I don't understand why.
> 
> The reason for that is because we're using the command roughly like
> this:  ${command} ...
> 
> And when sh_split_word isn't set (which it isn't by default and vcs_info
> uses mostly zsh defaults), the parameter isn't split, like it would in
> other bourne-like shells.
> 
> I think it would be best to leave it as it is; but I don't feel strongly
> about it. If a lot be people think we should split that parameter, we
> could do that (even though it might break backwards compatibility for
> some people).

I don't really want a change here. I was just to tired yesterday evening to
remember about sh_split_word (or to read the FAQ). Indeed, I feel ashamed for
such a question.

> Anyway, could you try the following patch for the locale problem? I
> think it should solve the issue once and for all.

Yes, it fixed it.

Thanks for your very complete reply
--
François

Re: vcs_info and locales

[Phil Pennock]

On 2010-04-25 at 10:38 +0200, Frank Terbeck wrote:
> Anyway, could you try the following patch for the locale problem? I
> think it should solve the issue once and for all.

I have one concern, which leads to the question: is it really necessary
to set LC_ALL instead of LC_MESSAGES?

The main problem is that when you override LC_CTYPE to C, you lose any
potential UTF-8 support, unless the tool just passes through the binary
data.

I think the safest algorithm is not to set LC_ALL but instead:
 * set LC_MESSAGES=C
 * if LC_ALL is set and is not C, set LANG=$LC_ALL, unset LC_ALL

Make sense?

Rest of this email is just some exploration and skippable.

I don't have NLS support on my main box, or I could do more testing
myself; with { svn log }, where most of my UTF-8 shows, LC_CTYPE=C leads
to expressing the content with escapes instead of cleanly.  I know
VCS_Info doesn't use that, I mention it by way of example.  { svn info }
by contrast always percent-encodes those characters; this works anyway,
because VCS_Info walks back up the dir-tree to find the svn co dir, so
has the relative info by comparing the FS realpath'd root of the repo to
the current dir.

URL: https://svn.spodhuis.org/ksvn/scratch/Fran%C3%A7ois
 -> VCS_Info %S == François
 (yes, I picked the OP's name as example testdata)

For experimentation, I created a repo with a UTF-8 character in its
name.  Apache/mod_dav_svn won't serve it:
(20014)Internal error: Can't convert string from 'UTF-8' to native encoding: [...]

but I can use file:/// access instead.  A repo named foo-☺ appears in my
prompt as <foo-%E2%98%BA:0> (<name:version>).

And still VCS_Info works:
 URL: file:///home/pdp/tmp/T/ROOT/foo-%E2%98%BA/fred
 Repository Root: file:///home/pdp/tmp/T/ROOT/foo-%E2%98%BA
 -> VCS_Info %S == fred
    pwd -> ..../T/foo-☺/fred

 URL: file:///home/pdp/tmp/T/ROOT/foo-%E2%98%BA/%E2%99%A1
 Repository Root: file:///home/pdp/tmp/T/ROOT/foo-%E2%98%BA
 -> VCS_Info %S == ♡
    pwd -> ..../T/foo-☺/♡

I ♡  VCS_Info for just working, but it's still juju.  It also works as
an accidental artifact of the VCS_INFO_get_data_svn implementation.  I
get to say "accidental" because apparently I wrote that code.
*scratches head*

(Through all this, cd gets interesting when xtitle updates to iTerm
 silently drop the UTF-8 characters through to the display)

Re: vcs_info and locales

[Frank Terbeck]

Phil Pennock wrote:
> On 2010-04-25 at 10:38 +0200, Frank Terbeck wrote:
>> Anyway, could you try the following patch for the locale problem? I
>> think it should solve the issue once and for all.
>
> I have one concern, which leads to the question: is it really necessary
> to set LC_ALL instead of LC_MESSAGES?
>
> The main problem is that when you override LC_CTYPE to C, you lose any
> potential UTF-8 support, unless the tool just passes through the binary
> data.
>
> I think the safest algorithm is not to set LC_ALL but instead:
>  * set LC_MESSAGES=C
>  * if LC_ALL is set and is not C, set LANG=$LC_ALL, unset LC_ALL
>
> Make sense?

Probably true. Although, I'd expect programs you can use for scripting
to use untempered byte sequences. But then, where is the boundary for
programs that are used for scripting and pure user-interface programs...
`svn info' is probably a pretty blurry example already.

I'm not an expert but I think your proposal is sound.  Here's a patch
that does exactly that.

It would be good if François could test whether this works for him.

Regards, Frank

---------- >8 ----------
>From 95f005d1d8a5791eb8937b745adbd65ac62828f4 Mon Sep 17 00:00:00 2001
From: Frank Terbeck <ft@bewatermyfriend.org>
Date: Sun, 25 Apr 2010 16:00:09 +0200
Subject: PATCH: vcs_info: Avoid locale related problems
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This is to avoid problems with output from external commands with non-C
locales as reported by François Gannaz in workers-27900.
---
 Functions/VCS_Info/vcs_info |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/Functions/VCS_Info/vcs_info b/Functions/VCS_Info/vcs_info
index a9e65e1..6585e7a 100644
--- a/Functions/VCS_Info/vcs_info
+++ b/Functions/VCS_Info/vcs_info
@@ -47,11 +47,16 @@ vcs_info () {
     local pat
     local -i found
     local -a enabled disabled dps
-    local -x usercontext vcs rrn
+    local -x usercontext vcs rrn LC_MESSAGES
     local -ix maxexports
     local -ax msgs
     local -Ax vcs_comm
 
+    LC_MESSAGES=C
+    if [[ -n ${LC_ALL} ]]; then
+        local -x LANG LC_ALL
+        LANG=${LC_ALL}
+    fi
     vcs='-init-'; rrn='-all-'
     usercontext=${1:-default}
 
-- 
1.7.0

Re: vcs_info and locales

[Frank Terbeck]

Frank Terbeck wrote:
[...]
>  Here's a patch that does exactly that.

Well, not *exactly*... Mikael just told me on IRC, that I'm un-setting
`LC_ALL' before assigning its old value to `LANG'.

Here's another try.

---------- >8 ----------


>From 07cabca511cbfdec4401ff660e078f75f3f6a954 Mon Sep 17 00:00:00 2001
From: Frank Terbeck <ft@bewatermyfriend.org>
Date: Sun, 25 Apr 2010 16:00:09 +0200
Subject: PATCH: vcs_info: Avoid locale related problems

This is to avoid problems with output from external commands with non-C
locales as reported by François Gannaz in workers-27900.
---
 Functions/VCS_Info/vcs_info |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/Functions/VCS_Info/vcs_info b/Functions/VCS_Info/vcs_info
index a9e65e1..7ae1123 100644
--- a/Functions/VCS_Info/vcs_info
+++ b/Functions/VCS_Info/vcs_info
@@ -47,11 +47,17 @@ vcs_info () {
     local pat
     local -i found
     local -a enabled disabled dps
-    local -x usercontext vcs rrn
+    local -x usercontext vcs rrn LC_MESSAGES
     local -ix maxexports
     local -ax msgs
     local -Ax vcs_comm
 
+    LC_MESSAGES=C
+    if [[ -n ${LC_ALL} ]]; then
+        local -x LANG
+        LANG=${LC_ALL}
+        local -x LC_ALL
+    fi
     vcs='-init-'; rrn='-all-'
     usercontext=${1:-default}
 
-- 
1.7.0

Re: vcs_info and locales

[Richard Hartmann]

On Sun, Apr 25, 2010 at 16:09, Frank Terbeck <ft@bewatermyfriend.org> wrote:

> Probably true. Although, I'd expect programs you can use for scripting
> to use untempered byte sequences. But then, where is the boundary for
> programs that are used for scripting and pure user-interface programs...
> `svn info' is probably a pretty blurry example already.

While fixing locales is a viable workaround, the root cause is that svn
is not able to present its information in a machine-readable way.

Would it be worthwhile to try and lobby svn etc to support something
like --output text; --output xml (yah, yah, I know) etc?

Everything else feels like a kludge.


Richard

PS: I seem to remember that one or two VCS have something like
this, but that is _very_ dim.

Re: vcs_info and locales

[François Gannaz]

Le 2010-05-19, Richard Hartmann <richih.mailinglist@gmail.com> a écrit :
> On Sun, Apr 25, 2010 at 16:09, Frank Terbeck <ft@bewatermyfriend.org> wrote:
> 
> > Probably true. Although, I'd expect programs you can use for scripting
> > to use untempered byte sequences. But then, where is the boundary for
> > programs that are used for scripting and pure user-interface programs...
> > `svn info' is probably a pretty blurry example already.
> 
> While fixing locales is a viable workaround, the root cause is that svn
> is not able to present its information in a machine-readable way.
> 
> Would it be worthwhile to try and lobby svn etc to support something
> like --output text; --output xml (yah, yah, I know) etc?

`svn info --xml` is already there. Several other commands allow `--xml`,
especially `log`.

It might be irrelevant, but parsing XML could be quite slower, and vcs_info is
already quite slow with svn (no induced blame).

Regards
--
François

Re: vcs_info and locales

[Richard Hartmann]

2010/5/19 François Gannaz <francois.gannaz@free.fr>:

> `svn info --xml` is already there. Several other commands allow `--xml`,
> especially `log`.

Interesting, thanks.


> It might be irrelevant, but parsing XML could be quite slower, and vcs_info is
> already quite slow with svn (no induced blame).

That is what I implied by listing text first. XML has its uses, shell scripting
is not one of them.


Richard

Re: vcs_info and locales

[François Gannaz]

Le 2010-04-25, Phil Pennock <zsh-workers+phil.pennock@spodhuis.org> a écrit :
> On 2010-04-25 at 10:38 +0200, Frank Terbeck wrote:
> > Anyway, could you try the following patch for the locale problem? I
> > think it should solve the issue once and for all.
> 
> I have one concern, which leads to the question: is it really necessary
> to set LC_ALL instead of LC_MESSAGES?

Yes it is. I used LC_ALL here because that was the only one that made "svn
info" switch to English when default locale is set to "fr_FR.utf8".

% LC_ALL=C svn info     
Path: .
[...]
% LC_MESSAGES=C LANG=C LC_CTYPE=C svn info
Chemin : .
[...]
 
> The main problem is that when you override LC_CTYPE to C, you lose any
> potential UTF-8 support, unless the tool just passes through the binary
> data.
> 
> I think the safest algorithm is not to set LC_ALL but instead:
>  * set LC_MESSAGES=C
>  * if LC_ALL is set and is not C, set LANG=$LC_ALL, unset LC_ALL
> 
> Make sense?

I had not tested what non-ASCII encoding became, and you're right that the
shell can't handle utf-8 anymore.

% LC_ALL=C svn log  
------------------------------------------------------------------------
r1 | zamansky | 2010-04-25 20:46:26 +0200 (Sun, 25 Apr 2010) | 2 lines
Accentu?\195?\169 l?\195?\160 aussi. ?\195?\128 tester.

So setting LC_ALL=C globally is indeed a bad idea. But why not just locally in
VCS_INFO_get_data_svn? I don't think that behavior would hurt much vcs_info.
The only problem I can think of is when the repository name is not un ASCII,
and doing this really calls for troubles ;-)
With a "françois" repository and LC_ALL=C:
% vcs_info command ; vcs_info_lastmsg                          
$vcs_info_msg_0_: "svn:(franfrançois37ois:1) "

The other solution would be to use the line numbers to fetch the data from "svn
info".
--
François

Re: vcs_info and locales

[Phil Pennock]

On 2010-04-25 at 21:09 +0200, François Gannaz wrote:
> Le 2010-04-25, Phil Pennock <zsh-workers+phil.pennock@spodhuis.org> a écrit :
> > On 2010-04-25 at 10:38 +0200, Frank Terbeck wrote:
> > > Anyway, could you try the following patch for the locale problem? I
> > > think it should solve the issue once and for all.
> > 
> > I have one concern, which leads to the question: is it really necessary
> > to set LC_ALL instead of LC_MESSAGES?
> 
> Yes it is. I used LC_ALL here because that was the only one that made "svn
> info" switch to English when default locale is set to "fr_FR.utf8".

In that case, you already had LC_ALL set, which is why part of my
proposed solution was to unset it.

I've just tested, and svn behaves as I expect for a locale-aware app:
LANG is a default, which provides implicit values for LC_<foo> where foo
is not ALL.  It's overriden by each LC_<foo> if set.  All of those are
overriden by LC_ALL.

-Phil

% locale
LANG=
LC_CTYPE=en_US.UTF-8
LC_COLLATE="C"
LC_TIME="C"
LC_NUMERIC="C"
LC_MONETARY="C"
LC_MESSAGES="C"
LC_ALL=
% svn info | head -3
Path: .
URL: https://svn.spodhuis.org/ksvn/pdp-dotconf/etc
Repository Root: https://svn.spodhuis.org/ksvn/pdp-dotconf
svn: Write error: Broken pipe
% export LANG=fr_FR.UTF-8
% svn info | head -3
Chemin : .
URL : https://svn.spodhuis.org/ksvn/pdp-dotconf/etc
Racine du dépôt : https://svn.spodhuis.org/ksvn/pdp-dotconf
svn: Erreur d'écriture: Broken pipe
% export LC_MESSAGES=C
% locale
LANG=fr_FR.UTF-8
LC_CTYPE=en_US.UTF-8
LC_COLLATE="fr_FR.UTF-8"
LC_TIME="fr_FR.UTF-8"
LC_NUMERIC="fr_FR.UTF-8"
LC_MONETARY="fr_FR.UTF-8"
LC_MESSAGES=C
LC_ALL=
% svn info | head -3
Path: .
URL: https://svn.spodhuis.org/ksvn/pdp-dotconf/etc
Repository Root: https://svn.spodhuis.org/ksvn/pdp-dotconf
svn: Write error: Broken pipe

Re: vcs_info and locales

[Frank Terbeck]

Phil Pennock wrote:
> On 2010-04-25 at 21:09 +0200, François Gannaz wrote:
>> Le 2010-04-25, Phil Pennock <zsh-workers+phil.pennock@spodhuis.org> a écrit :
>> > On 2010-04-25 at 10:38 +0200, Frank Terbeck wrote:
>> > > Anyway, could you try the following patch for the locale problem? I
>> > > think it should solve the issue once and for all.
>> > 
>> > I have one concern, which leads to the question: is it really necessary
>> > to set LC_ALL instead of LC_MESSAGES?
>> 
>> Yes it is. I used LC_ALL here because that was the only one that made "svn
>> info" switch to English when default locale is set to "fr_FR.utf8".
>
> In that case, you already had LC_ALL set, which is why part of my
> proposed solution was to unset it.
>
> I've just tested, and svn behaves as I expect for a locale-aware app:
> LANG is a default, which provides implicit values for LC_<foo> where foo
> is not ALL.  It's overriden by each LC_<foo> if set.  All of those are
> overriden by LC_ALL.

Indeed, LANG is a fallback value used for everything that's
unset. LC_ALL forces every value to its value.

My patch from workers-27908¹ should do what Phil proposed. Could you try
if that works for you, François?  This would give all backends C-locale
messages, which would avoid bugs like this for all of them now and in
the future.

Regards, Frank

 ¹ - <http://www.zsh.org/mla/workers/2010/msg00360.html>

Re: vcs_info and locales

[François Gannaz]

Le 2010-04-26, Frank Terbeck <ft@bewatermyfriend.org> a écrit :
> Phil Pennock wrote:
> > On 2010-04-25 at 21:09 +0200, François Gannaz wrote:
> >> Le 2010-04-25, Phil Pennock <zsh-workers+phil.pennock@spodhuis.org> a
> >> écrit :
> >> > On 2010-04-25 at 10:38 +0200, Frank Terbeck wrote:
> >> > > Anyway, could you try the following patch for the locale problem? I
> >> > > think it should solve the issue once and for all.
> >> > 
> >> > I have one concern, which leads to the question: is it really necessary
> >> > to set LC_ALL instead of LC_MESSAGES?
> >> 
> >> Yes it is. I used LC_ALL here because that was the only one that made "svn
> >> info" switch to English when default locale is set to "fr_FR.utf8".
> >
> > In that case, you already had LC_ALL set, which is why part of my
> > proposed solution was to unset it.
> >
> > I've just tested, and svn behaves as I expect for a locale-aware app:
> > LANG is a default, which provides implicit values for LC_<foo> where foo
> > is not ALL.  It's overriden by each LC_<foo> if set.  All of those are
> > overriden by LC_ALL.
> 
> Indeed, LANG is a fallback value used for everything that's
> unset. LC_ALL forces every value to its value.

My mistake, I misread Phil and I didn't understand LC_ALL's role (it's not
documented on locale's man page).

> My patch from workers-27908¹ should do what Phil proposed. Could you try
> if that works for you, François?  This would give all backends C-locale
> messages, which would avoid bugs like this for all of them now and in
> the future.

Works for me.

Regards
--
François

Re: vcs_info and locales

[Frank Terbeck]

François Gannaz wrote:
> Le 2010-04-26, Frank Terbeck <ft@bewatermyfriend.org> a écrit :
[...]
>> Indeed, LANG is a fallback value used for everything that's
>> unset. LC_ALL forces every value to its value.
>
> My mistake, I misread Phil and I didn't understand LC_ALL's role (it's not
> documented on locale's man page).

Probably depends on who you're getting your online manual from.  On this
Linux system, locale(7) describes the effects of the variables are
described quite accurately.

>> My patch from workers-27908¹ should do what Phil proposed. Could you try
>> if that works for you, François?  This would give all backends C-locale
>> messages, which would avoid bugs like this for all of them now and in
>> the future.
>
> Works for me.

Thanks, I've committed that patch.

Regards, Frank

-- 
In protocol design, perfection has been reached not when there is
nothing left to add, but when there is nothing left to take away.
                                                  -- RFC 1925