Benson Margulies wrote: > See patch attached to issue 3500. Benson, first of all, thank you for the patch. Couple minor points: please post patches to the list rather than the tracker so that it is easier to comment on them. Also, even though svnmerge has its own list, we follow the svn development guidelines -- please review and submit accordingly:
http://subversion.tigris.org/hacking.html#patches I'll post some comments to your patch here this time (please reply to these items inline to keep context rather than top posting -- it'll be easier to discuss them individually): 1) Hunk -167,7 +167,7: don't submit unrelated items as part of the patch. Please remove. 2) Hunk -222,11 +222,8: Why change the encoding used by the decode? AFAIK, there is no problem with the assumption that svn is outputting the log in the encoding returned by sys.stdout.encoding, so this shouldn't need to change. 3) General comment: I don't really like making the commit file encoding a user option, since I don't think the user should need to worry about this. However, since svnmerge.py appears to be doing the wrong thing in certain environments (like Benson's) and so far no one seems to know what python code will consistently return the same encoding that svn defaults to using when reading commit log messages, I am +0 on this to provide a workaround for people in Benson's situation. Benson, a question for you that I should have asked before: on your machine, if you manually i.e. not via svnmerge, create a commit log file with Arabic characters (presumably in utf-8) do you need to explicitly specify the "--encoding utf-8" option when doing svn commit? Or does svn use utf-8 when reading the commit log file by default? If the former, than the current python code is actually "correct" and the user option is definitely required. I would then be +1 on this change -- its not unreasonable to ask the user to specify the option to svnmerge.py if they are already doing so for svn. 4) Hunk -521,7 +518,7 and hunk -1073,7 +1070,7 and -1121,7 +1118,7: Unnecessary, sys.stdout.encoding works for svn log output. As far as I know this option does not affect svn log anyway -- it's for svn commit. Cheers, Raman _______________________________________________ Svnmerge mailing list [email protected] http://www.orcaware.com/mailman/listinfo/svnmerge
