[Svnmerge] KeyError on svnmerge.py

[Giovanni Bajo]

On Sat, 2008-03-08 at 11:04 -0500, Raman Gupta wrote:

> > One question: is using LC_MESSAGES=C enough to fix the regression? In
> > that case, I would assume that LC_MESSAGES would only affect SVN's own
> > strings and not log messages. It would be a win-win.
> 
> Good idea, but svn doesn't seem to respect LC_MESSAGES for the output
> of svn info:
> 
> # export LC_ALL=de_DE
> # svn info | head -1
> Pfad: .
> 
> # LC_MESSAGES=C svn info | head -1
> Pfad: .
> 
> # LC_ALL=C svn info | head -1
> Path: .

I see. I guess this changed in later SVN versions (given that
svnmerge.sh used to use LC_MESSAGES), but never mind.

> >> Regardless, since it is now clear what the purpose of the setting was,
> >> my suggestion would be to add the LC_ALL=C prefix explicitly to calls
> >> that require the svn output to be parseable by svnmerge.py, which
> >> leaves calls to svn log and the output of svn merge using the
> >> appropriate user locale. I think this could be done with a new
> >> parameter on the call to launchsvn called "localized" which would
> >> default to true.
> > 
> > Looks like a sensible design. I won't argue on the default even if I'd
> > think the opposite is more safe (given that it's the way svnmerge.py has
> > always been run).
> 
> FYI, the reason I suggested the default to be localized is that it is
> easy and quick to specify an environment variable with fixed value "C"
> for cases where we know we need to parse the output, as opposed to the
>  local value which must be retrieved from a function.

Ah right, it makes sense.

> >> If not, we could
> >> set os.environ temporarily 
> > 
> > launch() has two implementations nowadays: subprocess and popen. With
> > subprocess, there is an explicit parameter called "environ" where you
> > can pass in the environment in which the subprocess will be run (and
> > it's of course portable among platforms). 
> > 
> > For popen(), setting and restoring the environ would seem the only
> > solution.
> > 
> > Anyway, let's first see if LC_MESSAGES=C is enough...
> > 
> >> but in that case I suppose we need to ensure that launchsvn is not
> >> called simultaneously by multiple threads.
> > 
> > Which threads? svnmerge.py does not use threads at all.
> 
> Not yet anyway :-) I seem to recall you suggesting the use of threads
> for getting log messages or some such -- though that particular use
> does not seem like it would be an issue, I thought it best to be
> future-proof.
> However its not a big deal and os.environ seems fine for now, unless
> you can propose a cleaner solution for popen.

No, I think that setting and restoring os.environ is the best solution
for now.

Do we have a volunteer for coding this patch, then? I recap:

1) Add a parameter to launchsvn() called "localized", default True.
2) When False, launchsvn()/launch() shall disable localization this way:
  * The subprocess implementation shall modify the environment of the
child process through an environ argument; that argument must be set to
a copy of the current environment (os.environ), plus LC_ALL=C.
  * The popen implementation shall change os.environ adding LC_ALL=C
before running popen() and restoring it back (in a finally: clause).
3) All calls to launchsvn() should be revised checking if they depend or
not on the English messages. If so (such in the case at hand,
get_svninfo()), localized=False shall be added to the call.
4) A test shall be added to the testsuite that tries to run a simple
svnmerge operation after having forced a localized output (eg:
os.environ["LC_ALL"] = "de_DE"). This makes sure we don't regress
anymore.

-- 
Giovanni Bajo