Code review comment for lp:~craighewetson-deactivatedaccount/bzr/update_with_local_commit

In the end, I like this. I think this is something that would be good to land
in 'lp:bzr' so that it gets some time to cook in the 2.1 series and we
can see if it is really what we want.

To address what I think is happening. (Sorry if this is a bit technical Craig,
if you don't understand I can try to explain more). As an example:

  Master
     Local
  A
  |\
  B C
    |
    D
    |
    uncommitted

What happens when you do 'bzr update' in local, is that we first "merge D..B"
to revert the working tree to the state of B (leaving the uncommitted changes
alone). This brings the local tree to point at "B".
We then merge "D" *back into* the new state (which is B + uncommitted changes).

This will probably create bogus conflicts more often than not. Specifically,
imagine the case where 'local' is only modifying 1 file, and those contents are
not related to the changes in B. In the most obivous case, imagine that "C"
adds a new file which is then updated in D, and has some uncommitted changes.

When we 'pivot' to B, we will actually try to delete the new file. But that
will fail because there are uncommitted changes. And then when we merge w/ D,
we will get another conflict because you have this unversioned file in the same
location that you have a newly introduced versioned file.

I think the real answer for 'update' is that it should actually just do:

 "merge D..B" and then "set_parent_ids([B, D])"

In other words, don't do 2 merges. Instead do 1 merge, and then swap the parent
ordering. This will give slightly different results, if only because "THIS" and
"OTHER" are probably going to be swapped. it may be more intuitive, though...

Anyway, I'm willing to skip fixing "update" and just using a bandaid like this,
with some tweaks:

1) I think it would probably be better to use "Graph.find_unique_ancestors()"
rather than 'find_unmerged'. The latter is just a wrapper around the latter
anyway.

 local_last_rev = local_branch.last_revision()
 remote_last_rev = master.last_revision()
 g = local_branch.repository.get_graph(master.repository)
 local_extra_revs = g.find_unique_ancestors(local_last_rev, [remote_last_rev])

Maybe I'm wrong, as that is a bit more code than what you've written. Think
about it, though, as "find_unmerged" is more meant for displaying the revs to
the user.

2)

25 + tree.lock_read()
26 + try:
27 + changes = tree.changes_from(tree.basis_tree())
28 + finally:
29 + tree.unlock()

I may be wrong, but it would seem the tree should have been locked
significantly before this. (tree.lock_tree_write()?). Maybe not, and we will be
locking later.

Regardless, a better call is probably "tree.has_changes()", which once
Vincent's patch lands will do all the things you need, and not require passing
in a parameter.

3) I don't think the error message is the best we could do. It is probably
   good enough for now. But if you want to think about it a bit, and see if you
   can think of anything clearer, I would appreciate it.

4) Tests. You at least need to add a new test in

   "bzrlib/tests/blackbox/test_update.py" that checks the return code and
   side-effects of running 'bzr update' in this condition.

5) '--force'. If we are going to block update, I'd like there to be two options
   bzr commit --local
   bzr update --force