[MERGE] Fix for bug 285055

[Justyn Butler]

Hi,

2008/10/22 Andrew Bennetts <andrew.bennetts at canonical.com>:
> 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.

I'm not sure about this - because the test will have to checkout from
a branch on the local machine and will therefore not be affected by
the bug, or the patch.

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

Thanks, I understand now.

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

In this case presumably it would still fail without --force, but with
a sensible error, just as if it could not find the branch on the local
file system.

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

Personally I feel that --force should simply skip any "uncommitted
change" checks (as appears to be incorrectly suggested in the doc
string) regardless of the location. But this would change existing
behaviour of course.

My worry about connecting to remote branches is that it can take a
long time to fail if there is a problem with the server/connection.
Occasionally the login for a remote branch can take an absurd amount
of time to complete due to non-bzr problems. There has to be a way to
avoid any connection being attempted when you are trying to switch the
branch away from the problem.

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

I'll hold off trying to write tests until we've decided what needs to be done.

Justyn.