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 _______________________________________________ Svnmerge mailing list [email protected] http://www.orcaware.com/mailman/listinfo/svnmerge
