Thanks! Most of your comments were straightforward, and I've implemented them.
On Mon, Feb 16, 2009 at 7:26 PM, Giovanni Bajo <[email protected]> wrote: >> - for L in launchsvn('info "%s"' % target): >> + lines = launchsvn('info "%s"' % target) >> + if not lines: lines = [ "x: (Not a valid URL)" ] > > Why you need this special case? It was a backward-compatibility adjustment that had something to do with older versions of 'svn info'. I don't remember now (it's probably been nine months now), but it had something to do with older versions of 'svn info' not returning any results on an invali URL, while newer versions return the string used above. So this special-case basically makes the older versions of svn act the same as the newer versions. > If I understand correctly your whole patch, you're removing this code > because, after your patch, svnmerge.py will be again able to cope with > this old format. Is this correct? Correct. >> + OptionArg("-L", "--location-type", >> + dest="location-type", >> + default="path", >> + help="Use this type of location identifier in the new " + >> + "Subversion properties; 'uuid', 'url', or 'path' " + >> + "(default)"), > > Why "path" is the default? Is this only some kind of backward > compatibility default? I would say uuid is the best choice here, isn't > it? Yes, it's exactly for backward compatibility. In the absense of backward-compatibility constraints, I would probably prefer uuid, too, but we are not experiencing such an absence. > The patch looks great altogether. It's a great cleanup and also > introduces a valuable feature for cross-repo merges. Thanks! Cool. I'll post a new patch to the bug in a moment. It fails the same three tests that trunk fails, and seems to work for me. Will you take another look? Dustin -- Storage Software Engineer http://www.zmanda.com _______________________________________________ Svnmerge mailing list [email protected] http://www.orcaware.com/mailman/listinfo/svnmerge
