[Svnmerge] KeyError on svnmerge.py

[Raman Gupta]

Giovanni Bajo wrote:
> On Sat, 2008-03-08 at 10:10 -0500, Raman Gupta wrote:
> 
>>> which removes the LC_ALL=C from the environment. This is needed to force
>>> SVN to produce non localized strings, as explained by the comment. Why
>>> has it been removed?
>> Ok, so it *was* the removal of the LC_ALL=C that caused this -- I
>> suspected as much when I asked Hernan to test with rev 29665 yesterday.
>>
>> The reason it was removed was discussed on the mailing list. I also
>> asked explicitly on the list about the ramifications related to
>> removing LC_ALL=C, and no one replied.
> 
> Sorry, I had missed that. Please, feel free to CC: me on any questions
> which you feel I might have insights on :)

No problem and will do.

> 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: .

>> 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.

>> In UNIX, one is able to set a per-command environment variable by
>> prefixing the command with the environment setting e.g. specifying
>> "LC_ALL=C svn ...". Will that work on Windows also? 
> 
> No: this is not even a POSIX feature, but a shell feature.

Right. At least suprocesss supports setting environment variables as
an argument. For popen, we could fall back to using os.environ if
LC_MESSAGES does not work.

>> 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.

Cheers,
Raman