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

Giovanni Bajo rasky at develer.com
Sat Aug 4 04:25:13 PDT 2007


On ven, 2007-08-03 at 16:21 -0500, Dustin J. Mitchell wrote:

> This patch cleans up the pathid abstraction in preparation for adding
> multiple underlying pathid representations.  

I keep forgetting what you're aiming at... :)
Full support for merges across multiple repositories?

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?

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

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





More information about the Svnmerge mailing list