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

[Giovanni Bajo]

On lun, 2009-02-16 at 23:42 -0500, Dustin J. Mitchell wrote:

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

Yes, thanks!

The only one that it seems you missed is this one:

> > 
> > -        info[key] = value.strip()
> > +        value = value.strip()
> > +        if value == '(Not a valid URL)':
> > +            if fail_on_invalid:
> > +                error("Not a valid URL: %s" % target)
> > +            return {} # no info on this URL
> > +        info[key] = value
> 
> Hmm... I don't this this whole modification to get_svninfo() is worth
> it. It looks like your only goal was to implement the URL validity
> check. In that case, it's easy even without all this code:
> 
> def check_url(url):
>     return "URL" in get_svninfo(url)
> 
> Notice that this how this is already done: if you look at other usages
> of get_svninfo(), you will see that missing items from the dictionary
> is sometimes interpreted as a "bad target" signal.

Any specific reason why you skipped it? The idea is that the additional
argument fail_on_invalid shouldn't be necessary, nor playing with the
"Not a valid URL" message. It is probably sufficient to check the return
value.

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

I see. That's fine.
-- 
Giovanni Bajo
Develer S.r.l.
http://www.develer.com