[MERGE] Fix for bug 285055

[Andrew Bennetts]

Justyn Butler wrote:
> Hi
> 
> Thanks for the tips, my python and bzr noobishness is showing.
> 
> The original patch broke switch for heavyweight checkouts.
> I've attached a new patch that works and passes the existing switch tests.

Thanks!

bb:resubmit

It still needs tests, I've tried to provide some help below.

> 2008/10/21 Andrew Bennetts <andrew.bennetts at canonical.com>:
> > The tests I'd like to see added are:
> >
> >  * that switch with force=True works when the branch reference is absent (but
> >   the revision_id/revision_ids of the checkout's parent trees are all still
> >   accessible, i.e. they are present in the to_branch.repository)
> >  * what happens when switch with force=True is attempted in a lightweight
> >   checkout, but not all the revisions listed in checkout.get_parent_ids() are
> >   present in the to_branch.repository.  I guess the best we can do here is give
> >   an error?
> 
> I'm afraid I don't really understand enough of bzr internals to follow
> what you are saying here.

Fair enough.  Here's more details.

The first test is to make sure that this patch actually works — i.e. that when
the force flag is used that switch in a lightweight checkout will work anyway,
normally.  So, the test would do these steps, roughly:

  * make a branch (with at least one commit in it)
  * make a checkout of the branch
  * rename the branch
  * switch with force=True to the branch's new location

That test should fail without your patch, but pass with it.

The second test is exploring an edge case — just because a user passes --force
doesn't mean that bzr will be able to do this.  A lightweight checkout holds no
revision data itself (it relies on the reference branch), but like any working
tree it references one or more revisions, the parents of that tree.

For a fresh checkout, there will be just one parent, the revision ID that was
checked out.  But if there is an uncommitted merge (e.g. due to "bzr co
--lightweight branch_url checkout_dir; cd checkout_dir; bzr merge
another_branch_url") then that merge is also a parent of the working tree.  In
the bzrlib API, you can get this list of parent revision IDs with the
“get_parent_ids()” method on a workingtree object (such as is returned by
“branch.create_checkout”).

So, even with --force, “bzr switch” to a new branch can fail if the new branch
doesn't have all the revisions listed in get_parent_ids().  Normally switch
refuses to run if there are uncommitted merges, but --force overrides that.  But
even without pending merges, this case can happen.

So, here's the shape of the second test I had in mind:

  * make a branch (with at least one commit in it)
  * make a checkout of the branch
  * make a second, unrelated branch
  * (optional: delete the first branch.  I don't think this is strictly
    necessary for the test, but maybe it makes things clearer.)
  * switch with force=True to the unrelated branch
  * what happens?

It will fail, but the question is in what way?  It should be some sort of error
that gives a sensible message to the user, rather than a traceback.  (i.e.
whatever error it is won't have its .internal_error == True).

Hopefully it should be straightforward to write these tests based on how the
other test_switch tests work.  If not I can take a stab at it.

All that said, looking closer, I'm inclined to this that this ought to Just Work
without --force.  Two ideas spring to mind:

 1. The switch code should be catching and treating connection errors the same
    way it treats NotBranchError.
 2. The switch code arguably shouldn't even be trying to use the source
    location if the target location already has all the required data.

Neither of these trivial to implement, so in the meantime I think your patch is
still a good idea! :)

> === modified file 'bzrlib/switch.py'
> --- bzrlib/switch.py	2007-12-07 05:31:54 +0000
> +++ bzrlib/switch.py	2008-10-21 18:06:41 +0000
> @@ -29,10 +29,17 @@
>      :param to_branch: branch that the checkout is to reference
>      :param force: skip the check for local commits in a heavy checkout
>      """
> -    _check_pending_merges(control_dir, force)
> -    try:
> -        source_repository = control_dir.open_branch().repository
> -    except errors.NotBranchError:
> +    branch_reference = control_dir.get_branch_reference()
> +    if not force or branch_reference is None or branch_reference.startswith('file://'):

This line is still over 80 columns.  If you rename the “branch_reference”
variable to “branch_ref” that's probably the simplest remedy.

There are other ways to wrap long lines (using parentheses around the multiline
expression, or using backslash to do a line continuation), but I find that
multiline if statements are always ugly, so I'd take the solution that keeps it
all on one line :)

-Andrew.