[MERGE][RFC] switch for heavyweight checkouts

John Arbash Meinel john at arbash-meinel.com
Fri Dec 7 02:45:22 GMT 2007


-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Ian Clatworthy wrote:
> The attached patch extends switch to support heavyweight checkouts. I've
> marked this as RFC because I'm happy with it semantically but I
> appreciate that others might not be.
> 
> Here's the NEWS item as it currently stands:
> 
>    * ``switch`` can now be used on heavyweight checkouts as well as
>      lightweight ones. After switching a heavyweight checkout, the
>      local branch is a mirror/cache of the new bound branch and
>      uncommitted changes in the working tree are merged. As a safety
>      check, if there are local commits in a checkout which have not
>      been committed to the previously bound branch, then ``switch``
>      fails unless the ``--force`` option is given.
> 
> So the contentious issue is whether the local branch ought to be truly a
> mirror of the new bound branch or a superset of some kind. In order to
> behave the same for light vs heavy, I think it needs to be a mirror. (My
> only concern is that Robert indicated on IRC that "pull --overwrite" was
> not the right thing for switch. I'm struggling to understand why but I'm
> happy to learn.)
> 
> In case it helps add some context, Matthew Fuller explained in a recent
> email thread that bound branches and checkouts can be intentionally
> different depending on your point of view ...
> 
>> Well, they're very similar concepts, but there are subtle differences.
>> When you use a 'bound branch', what you mean is 'keep this branch in
>> sync with that other one'.  But when you use a checkout, heavy or
>> light, what you mean is 'give me a working tree to work on this
>> branch'; in the heavy case, you mean 'and cache everything locally too
>> so I don't have to hit the network except to commit/update'.
> 
> The difference indeed becomes important when switching a heavyweight
> checkout. My view is that, excluding uncommitted changes in the working
> tree which ought to be merged in, switching a checkout should have the
> same effect as removing the current checkout and creating a new one. In
> other words, the local branch ought to be made a mirror of the branch
> you're switching to. The risk with this is that local commits could be
> lost. As explained in the NEWS item, the code explicitly checks for
> those and fails if any are detected, so I feel it is safe. You can
> override this using --force if required.
> 
> I'd also like feedback on behaviour when the existing reference branch
> cannot be reached. The existing switch code silently handles this for
> checkouts so users can switch to a branch that is accessible instead.
> Now that we have a --force option, my thinking is that silently handling
> this is wrong and that --force ought to be required in this scenario.
> Does anyone have an opinion on this?
> 
> I'm happy to add more tests if required as well. With or without these,
> the patch ought to be good enough for users keen for this functionality
> to give it a test and to confirm it helps their workflow.
> 
> Ian C.
> 

I'm happy to see switch for heavy checkouts implemented (as then I might
actually start using it, we shall see, but then again, maybe I should keep
paying the build_tree price so that I'm motivated to fix it).

Having thought about it a bit, I think I agree that to switch from a missing
branch you should supply '--force'.

There is a subtle but important difference from "the branch moved, point at
this new location" versus "I'm done with this, and want to work on something
else." By silently ignoring a missing branch, the "I'm done with this" may not
really mean that.


+    @classmethod
+    def set_reference(self, a_bzrdir, to_branch):
+        """Set the target reference of the branch in a_bzrdir.
+
+        format probing must have been completed before calling

^- I'm very happy to see set_reference be a method/class method rather than
having it in switch.py.

I didn't see any direct tests of it, though. A simple smoke test is enough
(just so that if someone changes the api, deletes it, etc, it tells us).

+            if not force and _any_local_commits(b, bound_branch):

^- I would tend to not pass the url, and just have the helper use
"b.get_master_branch()" instead.

...

+    if last_rev != revision.NULL_REVISION:
+        a_bzrdir, relpath = BzrDir.open_containing(other_branch_url)
+        other_branch = a_bzrdir.open_branch()
+        other_branch.lock_read()
+        try:

^- I really don't think we want to be doing this as open_containing() as the
target of a bind should be exactly the other branch. Otherwise if you nest your
branches, it starts doing weird things. You can use BzrDir.open().

But why are you using BzrDir rather than Branch.open()? (Or as I mentioned,
this_branch.get_master_branch()).

Also, should we be passing around possible transports?

+            remote_graph = other_branch.repository.get_revision_graph(
+                other_last_rev)
+            if last_rev not in remote_graph:
+                return True

^- Using get_revision_graph() is slow. You should instead do:

graph = this_branch.repository.get_graph(other_branch.repository)
if graph.is_ancestor(last_rev, other_last_rev):
  return True

This can look at just the tip of the graph. And using "this.get_graph(other)"
will allow us to use a StackedParentsProvider that allows us to pull revisions
we have from the local side, rather than downloading all of the revision graph
from the remote side.


Otherwise it looks decent:

BB:tweak

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.7 (Darwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFHWLPCJdeBCYSNAAMRApLvAKCg9Yo1By1l//5OVmIYGhE+kVP/YQCggGYD
XRzvPfgNBD++3orH0UrZSys=
=eIFm
-----END PGP SIGNATURE-----



More information about the bazaar mailing list