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

Pierre-Yves David pierre-yves.david at ens-lyon.org
Sun Dec 30 10:01:49 CST 2012


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.

-- 
Pierre-Yves


More information about the Mercurial-devel mailing list