Merge lp:~craighewetson-deactivatedaccount/bzr/update_with_local_commit into lp:~bzr/bzr/trunk-old

I've tested this on my ubuntu machine and it works very well. I fails (with a clear error message) to update if the repository has local commits and uncommitted changes. It will inform the user to locally commit them before doing the update.

I suppose there could be a future improvement, and that is to check if the uncommitted changes where files that where locally committed. But I reckon that we might miss certain conditions (renaming etc) and this might cause it not to be full proof.

I suppose in the future Bazaar can handle this in a better way and try to merge the files for the user, but until then lets not allow the user to hang himself.

Please review and let me know if its good enough to go into trunk

Tested in the case where a checkout was unbound (changed to a branch).
Then a normal commit is done on fileA, then an uncommitted change is made to fileA.
The branch is then changed back to a checkout (bzr bind).
When attempting an bzr update the error message is displayed as expected.

Let me know of other corner cases so I can test this fix.

Whats the motivation for this? [what does bzr do wrong at the moment]. I think its important to be able to update with local changes - its the workflow that update is intended for.

> Whats the motivation for this? [what does bzr do wrong at the moment].

The following steps will illustrate the problem:
Modify fileA.
Commit --local fileA.
Modify fileA (without committing)
Then do an update.

Result:
Bazaar creates non intuitive conflicts for work that only he (the user) modified.

Bazaar doesn't do anything wrong, but it produces an output that the user will not make sense of. The user would have to rename fileA.moved.THIS to fileA and then do a bzr resolve fileA.

Its worth doing the above steps just to test it and see what I mean.

Now imagine: I had to somehow explain to my colleagues (being the resident Bazaar promoter) that bzr was doing the correct thing after it created conflicts for many critical changes that only my colleague made.

>I think its important to be able to update with local changes - its the workflow that update is intended for.
It does allow you to do this but with one limitation, first locally commit your changes to proceed with update. By doing this one can eliminate the negative effect of the above problem.

There is one further improvement that can be made:
On doing an update I could check to see if any of the modified files (eg: fileA) is within the local commits. It could then tell the user that the following files: FileA... needs to be locally committed before proceeding etc.

I think that conflict is spurious - we shouldn't be creating it in the
first place.

*if* the update after the commit works, then the update before the
commit should work identically.

-Rob

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

Robert Collins wrote:
> I think that conflict is spurious - we shouldn't be creating it in the
> first place.
>
> *if* the update after the commit works, then the update before the
> commit should work identically.
>
> -Rob
>

So the problem as I understand it is that if you have:

1) Local commits
2) remote commits
3) Local uncommitted changes

things can easily conflict, and then conflict with the conflicts.

We've discussed a few ways around it. The one proposed here, I believe,
is to refuse if you have 1 and 3. Another possibility is to just stop in
the case that you get a conflict while doing one of the updates.

However, because update does the pivot, I think there is a good chance
we don't have an obviously sane state to stop at. At the end of the
first update, I *think* we are supposed to be in a branch with no
'local' commits applied, and remote changes present, and presumably you
have to have the uncommitted changes.

If we stopped at that point, then the next step is actually to merge the
local committed changes, but since those aren't on the master branch, it
isn't trivial to tell 'update' to do that. (So doing 'bzr update', got
some conflicts, fix them, 'bzr update-continue' ?)

However, uncommitted changes are *more* likely to be dependent on local
commits, so they are extra likely to be problematic at that point. Which
is why, IMO, this patch is reasonable. Get them put somewhere that is
easy to recover from, bring in upstream, pivot, and merge them in.
Better than shooting your foot off. (Though personally I also think
"local" commits are a misfeature, versus a clear separation when working
in bound mode versus non-bound.)

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

iEYEARECAAYFAkqSq8cACgkQJdeBCYSNAAOQRwCfaoQim3jbS/CTvS+Vx7SizY+y
kAMAoLQMjN4DpHX0+ShoT9wd8yF8snmw
=sf/j
-----END PGP SIGNATURE-----

I feel that until the update handles the presence of local changes and local commits better; this patch will make sure the user doesn't mess his workspace up.

I've seen this problem happen to collegues that have made changes to dozens of files (i.e by renaming a single method) and then after the update, starting pulling there hair out because they forgot to commit locally. :)

Any chance that this fix can get a yah or nay? If nay let me know so I can make the necessary change. This problem is a ticking time bomb for me :(

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

@Craig: this merge proposal seems empty now but I don't think it was merged correctly.

Did you overwrite your lp branch ?
Are you still working on this patch ?
Do you still have the branch available somewhere ?

I've not had a chance to work on this problem again... I think it was around
the time with launchpad used 2a repository format.
I'll try and work on it in the next few days...