[PATCH STABLE] fix bookmarks rollback behavior

[Kevin Bullock]

On May 1, 2011, at 7:00 AM, Alexander Solovyov wrote:

> # HG changeset patch
> # User Alexander Solovyov <alexander at solovyov.net>
> # Date 1304248020 -7200
> # Branch stable
> # Node ID f815ee0e597cd5e8e220f5df9b9a5154a80cf10e
> # Parent  30cb7ece4378a874c0abc8610ebe5af32d3eb0b7
> fix bookmarks rollback behavior

Seems related to this thread: http://markmail.org/message/ibucl5ud3aqsqbs3

I make no claim that the behavior table in that post is correct, but as Matt has repeatedly pointed out, we need to keep all the related behaviors consistent. How does this compare to e.g. strip or qpop?

> Before this patch undo.bookmarks was created on bookmarks write and not with
> other transaction-related files. There were two issues: first is that if you
> have changed bookmarks few times after a transaction happened, rollback will
> give you a state which can point to non-existing revision. Second is that if you
> have not changed bookmarks after a transaction, rollback will touch your state
> anyway.

And what's the behavior with this patch? It looks like bookmarks that point to rolled-back revisions get reset to the null rev. I don't think this patch necessarily needs to do more than that, but I don't like this as the final behavior.

Hopefully I can find some time to do a followup patch (series?) that gets to the behavior I proposed in the aforementioned thread.

> This change also adds `localrepo._writejournal` method, which can be used by
> other extensions to save their transaction-related backup in right time.
> 
> diff --git a/mercurial/bookmarks.py b/mercurial/bookmarks.py
> --- a/mercurial/bookmarks.py
> +++ b/mercurial/bookmarks.py
> @@ -61,12 +61,6 @@ def write(repo):
>     '''
>     refs = repo._bookmarks
> 
> -    try:
> -        bms = repo.opener('bookmarks').read()
> -    except IOError:
> -        bms = ''
> -    repo.opener('undo.bookmarks', 'w').write(bms)
> -
>     if repo._bookmarkcurrent not in refs:
>         setcurrent(repo, None)
>     for mark in refs.keys():
> diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
> --- a/mercurial/localrepo.py
> +++ b/mercurial/localrepo.py
> @@ -669,6 +669,17 @@ class localrepository(repo.repository):
>             raise error.RepoError(
>                 _("abandoned transaction found - run hg recover"))
> 
> +        journalfiles = self._writejournal(desc)
> +        renames = [(x, undoname(x)) for x in journalfiles]
> +
> +        tr = transaction.transaction(self.ui.warn, self.sopener,
> +                                     self.sjoin("journal"),
> +                                     aftertrans(renames),
> +                                     self.store.createmode)
> +        self._transref = weakref.ref(tr)
> +        return tr
> +
> +    def _writejournal(self, desc):
>         # save dirstate for rollback
>         try:
>             ds = self.opener("dirstate").read()
> @@ -679,16 +690,15 @@ class localrepository(repo.repository):
>             encoding.fromlocal(self.dirstate.branch()))
>         self.opener("journal.desc", "w").write("%d\n%s\n" % (len(self), desc))
> 
> -        renames = [(self.sjoin("journal"), self.sjoin("undo")),
> -                   (self.join("journal.dirstate"), self.join("undo.dirstate")),
> -                   (self.join("journal.branch"), self.join("undo.branch")),
> -                   (self.join("journal.desc"), self.join("undo.desc"))]
> -        tr = transaction.transaction(self.ui.warn, self.sopener,
> -                                     self.sjoin("journal"),
> -                                     aftertrans(renames),
> -                                     self.store.createmode)
> -        self._transref = weakref.ref(tr)
> -        return tr
> +        bkname = self.join('bookmarks')
> +        if os.path.exists(bkname):
> +            util.copyfile(bkname, self.join('journal.bookmarks'))
> +        else:
> +            self.opener('journal.bookmarks', 'w').write('')
> +
> +        return (self.sjoin('journal'), self.join('journal.dirstate'),
> +                self.join('journal.branch'), self.join('journal.desc'),
> +                self.join('journal.bookmarks'))
> 
>     def recover(self):
>         lock = self.lock()
> @@ -2027,6 +2037,11 @@ def aftertrans(files):
>             util.rename(src, dest)
>     return a
> 
> +def undoname(fn):
> +    base, name = os.path.split(fn)
> +    assert name.startswith('journal')
> +    return os.path.join(base, name.replace('journal', 'undo', 1))
> +
> def instance(ui, path, create):
>     return localrepository(ui, util.drop_scheme('file', path), create)
> 
> diff --git a/tests/test-bookmarks.t b/tests/test-bookmarks.t
> --- a/tests/test-bookmarks.t
> +++ b/tests/test-bookmarks.t
> @@ -231,6 +231,7 @@ the bookmark extension should be ignored
>      Y                         2:db815d6d32e6
>    * Z                         2:db815d6d32e6
>      x  y                      2:db815d6d32e6
> +
> test summary
> 
>   $ hg summary
> @@ -244,3 +245,16 @@ test id
> 
>   $ hg id
>   db815d6d32e6 tip Y/Z/x  y
> +
> +test rollback
> +
> +  $ hg bookmark -f Y -r 1
> +  $ hg bookmark -f Z -r 1
> +  $ hg rollback
> +  repository tip rolled back to revision 1 (undo commit)
> +  working directory now based on revision 0
> +  $ hg bookmarks
> +     X                         0:f7b1eb17ad24
> +     X2                        1:925d80f479bb
> +     Y                         -1:000000000000
> +   * Z                         0:f7b1eb17ad24
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel at selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel

pacem in terris / mir / shanti / salaam / heiwa
Kevin R. Bullock