[Svnmerge] Fwd: [Issue 2869] multiple pathid formats

[Dustin J. Mitchell]

Thanks!  Most of your comments were straightforward, and I've implemented them.

On Mon, Feb 16, 2009 at 7:26 PM, Giovanni Bajo <rasky at develer.com> 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