Patchwork [STABLE] amend: prevent loose of bookmark on failed amend

login
register
mail settings
Submitter Pierre-Yves David
Date Dec. 30, 2012, 2:56 a.m.
Message ID <6522e491b723939bcab2.1356836196@yamac.lan>
Download mbox | patch
Permalink /patch/334/
State Accepted
Headers show

Comments

Pierre-Yves David - Dec. 30, 2012, 2:56 a.m.
# HG changeset patch
# User Pierre-Yves David <pierre-yves.david at ens-lyon.org>
# Date 1356835755 -3600
# Branch stable
# Node ID 6522e491b723939bcab26cede89058ed7eece300
# Parent  a51a5199a672e378924066cd5103afef8de26fb8
amend: prevent loose of bookmark on failed amend

The active bookmark were moved to the temporary commit. When the transaction
were rollbacked, the bookmark were lost.

We now temporarly disable the bookmark to prevent this effect.
Idan Kamara - Dec. 30, 2012, 10:54 a.m.
On Sun, Dec 30, 2012 at 4:56 AM, Pierre-Yves David <
pierre-yves.david at ens-lyon.org> wrote:
>
> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david at ens-lyon.org>
> # Date 1356835755 -3600
> # Branch stable
> # Node ID 6522e491b723939bcab26cede89058ed7eece300
> # Parent  a51a5199a672e378924066cd5103afef8de26fb8
> amend: prevent loose of bookmark on failed amend
>
> The active bookmark were moved to the temporary commit. When the
> transaction
> were rollbacked, the bookmark were lost.
>
> We now temporarly disable the bookmark to prevent this effect.

Looks like this fixes the problem, but I'm wondering if it's
the right answer. It seems there are several issues here:

1) files are being written during a transaction (e.g. .hg/bookmarks),
    shouldn't we write those when the locks are unlocked?

2) when a transaction is aborted, only revlogs get rolledback,
    shouldn't other files that aren't revlogs also be taken care of?
    it seems this is currently done in localrepo._rollback by using
    the undo files.

>
> diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
> --- a/mercurial/cmdutil.py
> +++ b/mercurial/cmdutil.py
> @@ -1633,14 +1633,17 @@ def amend(ui, repo, commitfunc, old, ext
>              # `logmessage` anyway.
>              opts.pop('logfile')
>              # First, do a regular commit to record all changes in the
> working
>              # directory (if there are any)
>              ui.callhooks = False
> +            currentbookmark = repo._bookmarkcurrent
>              try:
> +                repo._bookmarkcurrent = None
>                  opts['message'] = 'temporary amend commit for %s' % old
>                  node = commit(ui, repo, commitfunc, pats, opts)
>              finally:
> +                repo._bookmarkcurrent = currentbookmark
>                  ui.callhooks = True
>              ctx = repo[node]
>
>              # Participating changesets:
>              #
> diff --git a/tests/test-commit-amend.t b/tests/test-commit-amend.t
> --- a/tests/test-commit-amend.t
> +++ b/tests/test-commit-amend.t
> @@ -241,10 +241,28 @@ Moving bookmarks, preserve active bookma
>    saved backup bundle to
> $TESTTMP/.hg/strip-backup/6cec5aa930e2-amend-backup.hg (glob)
>    $ hg book
>       book1                     1:48bb6e53a15f
>     * book2                     1:48bb6e53a15f

Can't this test be folded somehow with the one
you added in the previous patch?

>
> +abort does not loose bookmarks
> +
> +  $ cat > editor.sh << '__EOF__'
> +  > #!/bin/sh
> +  > echo "" > "$1"
> +  > __EOF__
> +  $ echo a >> a
> +  $ HGEDITOR="\"sh\" \"`pwd`/editor.sh\"" hg commit --amend

You can use HGEDITOR=false here instead.

> +  transaction abort!
> +  rollback completed
> +  abort: empty commit message
> +  [255]
> +  $ hg book
> +     book1                     1:48bb6e53a15f
> +   * book2                     1:48bb6e53a15f
> +  $ hg revert -Caq
> +  $ rm editor.sh
> +
>    $ echo '[defaults]' >> $HGRCPATH
>    $ echo "commit=-d '0 0'" >> $HGRCPATH
>
>  Moving branches:
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://selenic.com/pipermail/mercurial-devel/attachments/20121230/a93dc23e/attachment.html>
Pierre-Yves David - Dec. 30, 2012, 4:01 p.m.
On 30 d?c. 2012, at 11:54, Idan Kamara wrote:

> On Sun, Dec 30, 2012 at 4:56 AM, Pierre-Yves David <pierre-yves.david at ens-lyon.org> wrote:
> >
> > # HG changeset patch
> > # User Pierre-Yves David <pierre-yves.david at ens-lyon.org>
> > # Date 1356835755 -3600
> > # Branch stable
> > # Node ID 6522e491b723939bcab26cede89058ed7eece300
> > # Parent  a51a5199a672e378924066cd5103afef8de26fb8
> > amend: prevent loose of bookmark on failed amend
> >
> > The active bookmark were moved to the temporary commit. When the
> > transaction
> > were rollbacked, the bookmark were lost.
> >
> > We now temporarly disable the bookmark to prevent this effect.
> 
> Looks like this fixes the problem, but I'm wondering if it's
> the right answer. It seems there are several issues here:

Oh, yeah

> 1) files are being written during a transaction (e.g. .hg/bookmarks),
>     shouldn't we write those when the locks are unlocked?

yes, we should. Augie has moved in this direction on default but such change is too big for stable.

In fact we should totally stop writing stuff "when lock is released" and stick all write cod in transaction. But again, that's far too big for stable


> 2) when a transaction is aborted, only revlogs get rolledback,
>     shouldn't other files that aren't revlogs also be taken care of?
>     it seems this is currently done in localrepo._rollback by using
>     the undo files.

Yes, this seems like a serious issue. But it wont help in several case even if restored by transaction abort some file would be overwritten by lock release :-(

To conclude: This area is a mess, the cleanup will probably be a bit complex and totally unsuitable for stable.

It's on my roadmap for changeset evolution, so I'll eventually hammer in a few month it if nobody else did.

> > diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
> > --- a/mercurial/cmdutil.py
> > +++ b/mercurial/cmdutil.py
> > @@ -1633,14 +1633,17 @@ def amend(ui, repo, commitfunc, old, ext
> >              # `logmessage` anyway.
> >              opts.pop('logfile')
> >              # First, do a regular commit to record all changes in the
> > working
> >              # directory (if there are any)
> >              ui.callhooks = False
> > +            currentbookmark = repo._bookmarkcurrent
> >              try:
> > +                repo._bookmarkcurrent = None
> >                  opts['message'] = 'temporary amend commit for %s' % old
> >                  node = commit(ui, repo, commitfunc, pats, opts)
> >              finally:
> > +                repo._bookmarkcurrent = currentbookmark
> >                  ui.callhooks = True
> >              ctx = repo[node]
> >
> >              # Participating changesets:
> >              #
> > diff --git a/tests/test-commit-amend.t b/tests/test-commit-amend.t
> > --- a/tests/test-commit-amend.t
> > +++ b/tests/test-commit-amend.t
> > @@ -241,10 +241,28 @@ Moving bookmarks, preserve active bookma
> >    saved backup bundle to
> > $TESTTMP/.hg/strip-backup/6cec5aa930e2-amend-backup.hg (glob)
> >    $ hg book
> >       book1                     1:48bb6e53a15f
> >     * book2                     1:48bb6e53a15f
> 
> Can't this test be folded somehow with the one
> you added in the previous patch?

I prefer that bookmark testing stay together.

> > +abort does not loose bookmarks
> > +
> > +  $ cat > editor.sh << '__EOF__'
> > +  > #!/bin/sh
> > +  > echo "" > "$1"
> > +  > __EOF__
> > +  $ echo a >> a
> > +  $ HGEDITOR="\"sh\" \"`pwd`/editor.sh\"" hg commit --amend
> 
> You can use HGEDITOR=false here instead.

The bug report said "empty message" I'm testing empty message. HGEDITOR=false would works too but I prefered to stay clause to the report.
Matt Mackall - Jan. 2, 2013, 1:19 a.m.
On Sun, 2012-12-30 at 03:56 +0100, Pierre-Yves David wrote:
> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david at ens-lyon.org>
> # Date 1356835755 -3600
> # Branch stable
> # Node ID 6522e491b723939bcab26cede89058ed7eece300
> # Parent  a51a5199a672e378924066cd5103afef8de26fb8
> amend: prevent loose of bookmark on failed amend

Queued for stable, thanks.

Patch

diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
--- a/mercurial/cmdutil.py
+++ b/mercurial/cmdutil.py
@@ -1633,14 +1633,17 @@  def amend(ui, repo, commitfunc, old, ext
             # `logmessage` anyway.
             opts.pop('logfile')
             # First, do a regular commit to record all changes in the working
             # directory (if there are any)
             ui.callhooks = False
+            currentbookmark = repo._bookmarkcurrent
             try:
+                repo._bookmarkcurrent = None
                 opts['message'] = 'temporary amend commit for %s' % old
                 node = commit(ui, repo, commitfunc, pats, opts)
             finally:
+                repo._bookmarkcurrent = currentbookmark
                 ui.callhooks = True
             ctx = repo[node]
 
             # Participating changesets:
             #
diff --git a/tests/test-commit-amend.t b/tests/test-commit-amend.t
--- a/tests/test-commit-amend.t
+++ b/tests/test-commit-amend.t
@@ -241,10 +241,28 @@  Moving bookmarks, preserve active bookma
   saved backup bundle to $TESTTMP/.hg/strip-backup/6cec5aa930e2-amend-backup.hg (glob)
   $ hg book
      book1                     1:48bb6e53a15f
    * book2                     1:48bb6e53a15f
 
+abort does not loose bookmarks
+
+  $ cat > editor.sh << '__EOF__'
+  > #!/bin/sh
+  > echo "" > "$1"
+  > __EOF__
+  $ echo a >> a
+  $ HGEDITOR="\"sh\" \"`pwd`/editor.sh\"" hg commit --amend
+  transaction abort!
+  rollback completed
+  abort: empty commit message
+  [255]
+  $ hg book
+     book1                     1:48bb6e53a15f
+   * book2                     1:48bb6e53a15f
+  $ hg revert -Caq
+  $ rm editor.sh
+
   $ echo '[defaults]' >> $HGRCPATH
   $ echo "commit=-d '0 0'" >> $HGRCPATH
 
 Moving branches: