[Svnmerge] request for review: #2863 - analyze_source_revs fix

[Giovanni Bajo]

On 8/9/2007 5:41 PM, Dustin J. Mitchell wrote:

> I've attached a patch to do just that.  This patch also fixes a typo in
> the RevisionSet constructor which triggered when end_rev was None,
> although that code remains unused.  I did remove the support for a
> nonexistent end_rev from analyze_revs.

Yes, thanks for that. I think it used to be used :)

> I don't have the birds-eye understanding to know if the
> repositories-equal check
>   get_repo_root(branch_target) == get_repo_root(source_url)
> is going to cause an extra round-trip to the server.  I don't *think* it
> will, assuming that branch_target is a working copy.  Can anyone
> confirm?

It will still cause an extra roudtrip because at this point we should 
have ever executed "svn info source_url".

I debugged this further and noticed that the extra roundtrip is already 
present in the trunk version if you use -S, and it's caused by this code 
in main():

         source_pathid = target_to_pathid(source)
         if str(cmd) == "init" and \
                source_pathid == target_to_pathid("."):
             error("cannot init integration source path '%s'\nIts 
repository-relative path must "
                   "differ from the repository-relative path of the 
current directory." % source_pathid)


The target_to_pathid() call requires to get the reporoot.

I think that this shows how hard it is to notice this kind of 
performance issue now that svnmerge.py code has grown in complexity and 
features (and abstraction layers). Thus, I believe that an approach like 
that in your patch, that tries to avoid a server roundtrip explicitally 
within an unrelated function (which is just trying to get its job done) 
does not scale quite well.

I think we should try to fix things at the *bottom* of the chain, that 
is in the guts of the functions that physically execute svn calls.

The attached patch does just that. It adds an internal cache in 
get_reporoot, so that the function becomes smarter and you can now use 
it everywhere (get_reporoot() itself or any abstraction layer built upon 
it) without having to code defensively. I tested this patch and it looks 
  OK. I also verified that it avoids the already existing 
extra-roundtrip when using "-S" on the command line.

I plan to commit this, but I would like your review on it. With this 
patch in, your original one-liner to just use source_url instead of 
branch_url is OK because it does not cause performance regressions anymore.

(also please double check the log commit format, thanks!)
-- 
Giovanni Bajo

-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: svnmerge_reporoot_cache.patch
URL: </pipermail/svnmerge/attachments/20070810/f15eda61/attachment.ksh>