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

Matt Mackall mpm at selenic.com
Fri Sep 30 15:49:43 CDT 2011


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)
> 
> You can get into trouble if you commit, update back to an older
> changeset, and then rollback. The update removes your valuable changes
> from the working dir, then rollback removes them history. Oops: you've
> just irretrievably lost data running nothing but core Mercurial
> commands.  (Or, more subtly, rollback from a shared clone that was
> already at an older changeset -- no update required, just rollback
> from the wrong directory.)
> 
> The fix assumes that rollbacks can be considered "safe" or "unsafe". A
> safe rollback is one that aborts a transaction whose data already
> exists somewhere else: pull, unbundle, or push. Don't be so picky
> about rolling back those transactions, since the repo or bundle from
> which they came presumably still exists. Every other rollback, most
> typically of a commit transaction, is considered unsafe. Finally, you
> can get back the existing dangerous behaviour with --force.
> 
> This is PRELIMINARY for review only. I still have some broken tests to
> deal with (looks like some tests rely on the existing dangerous
> behaviour). More importantly, it's an open issue whether we should do
> anything similar with strip. In this case it seems unnecessary, since
> strip by default backs up the changesets that it destroys. But I want
> to send another "make rollback safer" change -- prevent rollback from
> the wrong shared clone -- and that probably should affect strip.
> 
> So: should we factor out a pre-destroy sanity check that can be shared
> by strip and rollback? Same sort of idea as repo.destroyed(), except
> it would be called before rollback or strip do anything you might
> regret.
> 
> 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.

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

> +            raise util.Abort(_('attempt to rollback a commit when not at tip '

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.

All in all, I think this idea is ok though.

-- 
Mathematics is the supreme nostalgia of our time.




More information about the Mercurial-devel mailing list