how to revert update operation

[Robert Collins]

On Wed, 2006-07-19 at 23:12 -0400, Aaron Bentley wrote:


> Robert Collins wrote:

> >>
> >> If you try to preserve the ancestry for working tree, you're now
> >> starting to treat the working tree like a branch.
> > 
> > Uh, we must be miscommunicating here. I'm not trying to preserve the
> > ancestry of the working tree. But the code in update() is designed to
> > accomodate this state - I dont consider it illegal - though its
> > certainly a corner case. Its true that not all our code handles
> > uncommits gracefully - but that does not mean we should not try.
> 
> You are trying to preserve the text of the working tree commits.  If
> you're not setting wt.last_revision() as a pending merge, I don't see
> why not-- it would be consistent with what you're doing here.

Good point, we should do that, if its not in the ancestry of any of the
other final parents - the new last_revision & the pending merges.

> >>>> Furthermore, it's not at all clear to me that the user wants the
> >>>> branch.last_revision() to be merged with the master.  If they have a
> >>>> bound branch, and their working tree is out of date with the branch,
> >>>> this suggests that someone else has committed or pushed into the branch.
> >>>> They may not be aware of these new revisions, and they certainly
> >>>> wouldn't have reviewed them, since they're not the current tree state.
> >>>
> >>> For shared branches, the 'review before I accept it' workflow is not
> >>> present. If users want to do that they should not be using a shared
> >>> branch.
> >> There is a big difference between applying the changes that are present
> >> in the master branch, and applying the changes that are present in the
> >> local branch.  They are two different locations, and may have different
> >> access controls.
> >>
> >> Users don't mean 'apply the local branch changes' when they update, they
> >> mean 'apply the master branch changes'.  So it is surprising behaviour
> >> to apply the local branch changes.
> > 
> > Uh, the local branch changes are the changes they have done. 
> 
> They may not be changes that they have done, or they may be changes that
> they did not intend to include, as shown in my examples below.
> 
> > They
> > certainly do -not- mean 'discard my work'. So we have to preserve the
> > work.
> 
> They *may* mean 'discard my work'.  This is why I say user intent is
> unclear.

No. I fundamentally disagree. The *statement* of running update is
'preserve my work'. You can certainly design other commands, but the
will not be analagous to 'update' in CVS or SVN.

> >> I can't think of a good reason why both the checkout and the local
> >> branch would be out of date at the same time.  But here are some bad
> >> reasons:
> >>
> >> $ bzr unbind
> >> # --local hasn't been implemented for uncommit, but AIUI, the plan is to
> >> # do so.
> >> $ bzr uncommit --local
> >> $ bzr bind foo
> >> $ bzr update
> >>
> >> It's pretty clear that the user didn't want to commit those changes to
> >> the master branch, so in this scenario, applying the local branch
> >> changes is The Wrong Thing.
> > 
> > UHHHH. --local has no effect after an unbind.
> 
> - --local is not implemented for uncommit.
> 
> Currently, when you have a checkout, uncommit acts on both the
> wt.last_revision and the branch.revision_history.
> 
> When I the change to make uncommit affect wt.last_revision, it was
> suggested that we should also provide uncommit --local, to adjust only
> the wt.
> 
> I suppose update -r -2 (in an unbound branch) would have the same effect.

Right

> >> Here's another:
> >>
> >> $ cp -r foo bar
> >> $ cd bar
> >> $ bzr commit -m 'created new branch'
> >> $ cd ../foo
> >> $ bzr update
> >>
> >> In this scenario, the user intended to create a new branch, but foo is
> >> actually a lightweight checkout, so bar is a checkout of the same branch.
> >>
> >> Again, applying the local changes is The Wrong Thing.
> > 
> > This is what update is *intended* to do. If you have two working
> > instances of the same branch, and commit on one, update is *meant* to
> > bring across the commits from the other. Its what CVS and svn
> > shared-branch workflow delivers. 
> 
> In my experience, this is nothing like CVS.  CVS/SVN do a three-way
> update, not a four-way one.  This may be your extrapolation of CVS/SVN
> to this situation, but that's not the same thing.  CVS never provided a
> way to check a branch out into another branch, only into a checkout.

The fact its done as a 3-way merge is interesting but not the key point.
They could do a knit merge just as well. They have two trees - the
shared branch tip, and the current content. We have three, when you have
a bound branch, and yes, this is my extrapolation, which is AFAICT
completely consistent with CVS and SVN.

> > If you dont want that, dont use update.
> > (seriously!). 
> 
> But then, how do I get changes from the master?  That's all update does
> 99.9% of the time, and it's all I'm expecting update to do.

There are two basic use cases:
 *) Make my checkout be the same as the latest in master. To achieve
this 'bzr update, bzr revert'.
 *) Get the latest stuff from master to be my basis, and my local work
preserved, ready to commit. To achieve this 'bzr update'

> > As for the user not getting a new branch - they had to
> > take an explicit action to get a 'checkout', and for *all* our
> > checkouts, 'cp -r foo bar' will preserve the fact that it is a checkout.
> 
> Yeah, but checkouts look very similar to standalone working trees.  I am
> saying a mistake like this is possible.

This is true. Its also, IMO, a reason we have the differences shown in
'info', and also have 'bzr branch' which will always make a new branch.

> > This is desired - its what users of CVS and svn and arch expect a copy
> > of a checkout to do.
> 
> It is not what a Darcs user would expect cp to do.  Both types of users
> exist.  Hence the possibility for mistakes.

Darcs does not have checkouts, nor AFAIK update. The update and checkout
commands are specifically for the workflow CVS and SVN users are
accustomed to, so ...

> > Well, this is more interesting. It suggests to me that update should be
> > showing much more info about the nature of the local changes - perhaps
> > an automatic status output after the update or something. But this
> > vector exists *without* the push:
> > A cracker does:
> > 'scp hacked-file.c example.com:myproj/'
> 
> True, but that case is much easier to detect.

How? By status? Status would have shown you in your example as well.

> > 
> > An honest user does
> > $ ssh example.com
> > $ cd myproj
> > $ bzr update
> > $ bzr commit -m 'add my latest harmless changes'
> > 
> > And has now committed that malicous code.
> > 
> > So I dont see update making the vector better -or- worse.
> 
> I see it making the attack harder to detect, and therefore worse.
> 
> I suppose an even nastier approach would be to put bogus revisions into
> the user's local repository which appeared to be the revisions that
> update would apply.

True.

> >> On the other hand, we can't just automatically do a double merge,
> >> because update is a destructive operation, and so it's very bad to guess
> >> wrong.
> > 
> > update should never be destructive. 
> 
> Update is destructive, whether or not you think it should be.  It is
> intended to combine your local, uncommitted changes with the mainline
> changes.
> 
> Once the changes have been combined, they cannot reliably be
> disentangled.  You can't get your working tree back to the state it was
> in.  Destructive change, as Alexander discovered.

In that sense, yes it is. However, Alexander had work lost because he
ran 'revert' after 'update', which is the workflow one users to discard
ones work!

> > There is no need to guess user intent: using a
> > shared branch is a deliberate end user choice which brings in the
> > workflow.
> 
> Using a shared branch is fine, but it doesn't naturally result in
> situations where the checkout is out-sync-with the branch AND the branch
> is out of sync with the master branch.

It *can*, but its somewhat pathological for sure. Anyhow, I like the
idea of making update more robust, so lets focus on that - your stop and
have them retry on conflicts is good. So we should open a bug for that.

Rob

-- 
GPG key available at: <http://www.robertcollins.net/keys.txt>.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 191 bytes
Desc: This is a digitally signed message part
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20060720/1bfe1fb7/attachment.pgp