[Svnmerge] request for review: #2862 - cleanup pathid abstraction

[Dustin J. Mitchell]

On Sat, Aug 04, 2007 at 01:25:13PM +0200, Giovanni Bajo wrote:
> Full support for merges across multiple repositories?

Yep -- even when the repos-relative paths are the same.

> Your patch looks OK. There's only a couple of nits:
> 
> - This change:
> 
> > @@ -806,10 +809,14 @@
> >      # Try using "svn info URL". This works only on SVN clients >= 1.2
> >      try:
> >          info = get_svninfo(url)
> > -        return info["Repository Root"]
> >      except LaunchError:
> >          pass
> >  
> > +    try:
> > +        return info["Repository Root"]
> > +    except KeyError:
> > +        pass
> > +
> 
> is unrelated, AFAICT. Also I don't understand it: when is it possible
> that info does not contain the Repository Root?

You're right -- I made that in response to some bugs discovred while
researching Juan's bug report.  I'll factor it into a separate patch,
possibly with test cases (though IIRC the bugs only showed themselves
with http).

> - If you're going to totally abstract away pathid, why not making it a
> little (immutable) class? It might end up being a little more readabale
> (pathid.to_url(), pathid.path, ecc.).

Don't get ahead of me now!

I'd post the last patch, but it will change pretty radically as this one
is revised.

> - I don't undesrtand the *targets disambiguation arguments, but I guess
> those will be clearer in the next patch :)

The idea is that, when only a UUID is supplied, it's helpful to have a
collection of svn working copies or URLs against which to search for
that UUID.

Now that I look at the patch, I think I did a bad job of merging the
recent commits -- it has two copies of SvnLogParser, for one thing.

Thanks for the review -- I'll post an updated version soon.

Dustin

-- 
        Dustin J. Mitchell
        Storage Software Engineer, Zmanda, Inc.
        http://www.zmanda.com/