[PATCH 2 of 2] rollback: avoid unsafe rollback when not at tip (issue2998)

[Greg Ward]

On Fri, Sep 30, 2011 at 4:49 PM, Matt Mackall <mpm at selenic.com> wrote:
> On Thu, 2011-09-29 at 22:55 -0400, Greg Ward wrote:
>> # HG changeset patch
>> # User Greg Ward <greg at gerg.ca>
>> # Date 1317350673 14400
>> # Node ID c10af212a55cbfa84b5d5182ee41028c4baa33c6
>> # Parent  f295542f482d4b567fa6229b93c1854aca78792c
>> rollback: avoid unsafe rollback when not at tip (issue2998)
>> diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
>> --- a/mercurial/localrepo.py
>> +++ b/mercurial/localrepo.py
>> +        safe = desc in ('pull', 'push', 'unbundle')
>
> Here you have a whitelist..
>
>> +        if not force and not safe and nottip:
>> +            raise util.Abort(_('attempt to rollback a commit when not at tip '
>
> ..but the message implies a blacklist. We've probably got no other types
> of transaction out there, but it might be better to simply use a
> blacklist.

Yeah, good point. For the record, my rationale was:

  * we know changesets in a pull/push/unbundle transaction
    are not irreplaceable
  * we know changesets in a commit transaction are irreplaceable
  * we don't know what other kinds of transactions there might be
    out there in the world; to be safe, we should assume they are
    irreplaceable

Consider two examples: transplant and automerge.

1) What if transplant did all its work under one transaction, so you
could transplant multiple changesets and then rollback the lot? Then
desc would be 'transplant', and the changesets in that transaction are
valuable, since they might contain the result of resolving rejected
patches.

2) automerge is a script I wrote at work to automate our rather
complex workflow; it takes care of merging across named branches (4.1
-> 4.2 -> 4.3 -> trunk) and doing whatever in-branch merges are needed
along the way. And it does all that as one big transaction (desc =
'automerge') so that Ctrl-C does the right thing. Again, that
transaction might contain the result of resolved merge conflicts, so
it has valuable changesets.

That said, the valuable data in my hypothetical 'transplant'
transaction or my real 'automerge' transaction is *not* as valuable as
what's in a commit. Losing the result of a merge just isn't as big a
deal as losing an entire commit.

In a nutshell: I have no serious objection to treating 'commit'
transactions specially and assuming all other transactions are not
valuable. Just thought I should explain the reasoning for the patch I
sent.

>
>> +        nottip = self.dirstate.parents()[0] != self.changelog.tip()
>
> This is the pre-1.0 style. I'd much rather see something like:
>
> self['.'] != self['tip']
>
> The performance hit is negligible. This whole thing might be more
> natural as:
>
> if not force and self['.'] != self['tip'] and desc == 'commit':
>    raise util.Abort(...)
>
> This has the downside that the existence of the desc file is actually
> optional, so either version now erroneously aborts perfectly safe
> commits made by old clients (ie for single users sharing repos between
> operating systems). That's probably not cool.

Sure, sounds fine. I'll add a test for the "old client" case.

> I'm trying to remove 'tip' from our jargon as it's confusing/irrelevant
> to named branch users. I propose:
>
>  abort: rollback of last commit while not checked out may lose data
>
> ..which I think is a pretty good summary of the whole situation.
>
>> +                               '(use -f to force and lose data)'))
>
> The data loss is technically only potential.

Yeah, I like that. I just need to tell the user about -f without
scaring them *too* much.

Will resend in a bit.

Greg