Patchwork [3,of,4] shelve: use rollback instead of aborting a current transaction for shelve

login
register
mail settings
Submitter Katsunori FUJIWARA
Date Oct. 4, 2015, 12:44 p.m.
Message ID <e265a8c85c3873815730.1443962659@feefifofum>
Download mbox | patch
Permalink /patch/10772/
State Changes Requested
Delegated to: Pierre-Yves David
Headers show

Comments

Katsunori FUJIWARA - Oct. 4, 2015, 12:44 p.m.
# HG changeset patch
# User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
# Date 1443962009 -32400
#      Sun Oct 04 21:33:29 2015 +0900
# Node ID e265a8c85c3873815730921ae2d8ca3c6df3b48a
# Parent  ce4ea67c82972f2988e42372cd6443b6c288e1c8
shelve: use rollback instead of aborting a current transaction for shelve

Before this patch, "hg shelve" uses aborting a current transaction to
discard temporary changes while shelving.

This assumes that dirstate changes in a transaction scope are kept
even after aborting it. But this assumption will be broken by
"transactional dirstate". See the wiki page below for detail about it.

    https://mercurial.selenic.com/wiki/DirstateTransactionPlan

This patch uses 'repo.rollback()' instead of aborting current
transaction to remove the temporary revision for "hg shelve"

'dirstate.write()' just before closing a transaction will be removed
soon by subsequent patch, which writes or discards in-memory dirstate
changes at releasing transaction according to the result of it.
Pierre-Yves David - Oct. 4, 2015, 9:47 p.m.
On 10/04/2015 05:44 AM, FUJIWARA Katsunori wrote:
> # HG changeset patch
> # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
> # Date 1443962009 -32400
> #      Sun Oct 04 21:33:29 2015 +0900
> # Node ID e265a8c85c3873815730921ae2d8ca3c6df3b48a
> # Parent  ce4ea67c82972f2988e42372cd6443b6c288e1c8
> shelve: use rollback instead of aborting a current transaction for shelve

I'm not sure that I like these one (and the next).

Using rollback means that the transaction is actually commited at some 
point. Rolling it back will likely cause cache invalidation issue and 
race condition with other reader.

However:

- the current shelve implementation is highly debatable by itself. (we 
should probably use a combination of fully in memory commit (not 
supported yet) and bundlerepo + merge so I'm not sure your proposal is 
notably worse (we are a bit between Scylla and Charybdis here)

- If this is the last blocking bits to get a transactional dirstate, the 
drawback at certainly worth it. The current bug associated with the lack 
of it have much more user impact.

I need to think about it.
Katsunori FUJIWARA - Oct. 5, 2015, 2:42 p.m.
At Sun, 04 Oct 2015 14:47:18 -0700,
Pierre-Yves David wrote:
> 
> On 10/04/2015 05:44 AM, FUJIWARA Katsunori wrote:
> > # HG changeset patch
> > # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
> > # Date 1443962009 -32400
> > #      Sun Oct 04 21:33:29 2015 +0900
> > # Node ID e265a8c85c3873815730921ae2d8ca3c6df3b48a
> > # Parent  ce4ea67c82972f2988e42372cd6443b6c288e1c8
> > shelve: use rollback instead of aborting a current transaction for shelve
> 
> I'm not sure that I like these one (and the next).
> 
> Using rollback means that the transaction is actually commited at some 
> point. Rolling it back will likely cause cache invalidation issue and 
> race condition with other reader.

Oops, I forgot that incorrect visibility for a moment at closing
transaction :-<

I'll post revised ones using another way (maybe enclosing such scope
by dirstateguard).


> However:
> 
> - the current shelve implementation is highly debatable by itself. (we 
> should probably use a combination of fully in memory commit (not 
> supported yet) and bundlerepo + merge so I'm not sure your proposal is 
> notably worse (we are a bit between Scylla and Charybdis here)
> 
> - If this is the last blocking bits to get a transactional dirstate, the 
> drawback at certainly worth it. The current bug associated with the lack 
> of it have much more user impact.
> 
> I need to think about it.
> 
> -- 
> Pierre-Yves David
> 

----------------------------------------------------------------------
[FUJIWARA Katsunori]                             foozy@lares.dti.ne.jp
Pierre-Yves David - Oct. 5, 2015, 6:24 p.m.
On 10/05/2015 07:42 AM, FUJIWARA Katsunori wrote:
> At Sun, 04 Oct 2015 14:47:18 -0700,
> Pierre-Yves David wrote:
>>
>> On 10/04/2015 05:44 AM, FUJIWARA Katsunori wrote:
>>> # HG changeset patch
>>> # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
>>> # Date 1443962009 -32400
>>> #      Sun Oct 04 21:33:29 2015 +0900
>>> # Node ID e265a8c85c3873815730921ae2d8ca3c6df3b48a
>>> # Parent  ce4ea67c82972f2988e42372cd6443b6c288e1c8
>>> shelve: use rollback instead of aborting a current transaction for shelve
>>
>> I'm not sure that I like these one (and the next).
>>
>> Using rollback means that the transaction is actually commited at some
>> point. Rolling it back will likely cause cache invalidation issue and
>> race condition with other reader.
>
> Oops, I forgot that incorrect visibility for a moment at closing
> transaction :-<
>
> I'll post revised ones using another way (maybe enclosing such scope
> by dirstateguard).

I'm open to any kind of awful hack on the shelve side if it helps us to 
move forward on the big picture.
Katsunori FUJIWARA - Oct. 7, 2015, 4:59 p.m.
At Mon, 05 Oct 2015 11:24:51 -0700,
Pierre-Yves David wrote:
> 
> 
> 
> On 10/05/2015 07:42 AM, FUJIWARA Katsunori wrote:
> > At Sun, 04 Oct 2015 14:47:18 -0700,
> > Pierre-Yves David wrote:
> >>
> >> On 10/04/2015 05:44 AM, FUJIWARA Katsunori wrote:
> >>> # HG changeset patch
> >>> # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
> >>> # Date 1443962009 -32400
> >>> #      Sun Oct 04 21:33:29 2015 +0900
> >>> # Node ID e265a8c85c3873815730921ae2d8ca3c6df3b48a
> >>> # Parent  ce4ea67c82972f2988e42372cd6443b6c288e1c8
> >>> shelve: use rollback instead of aborting a current transaction for shelve
> >>
> >> I'm not sure that I like these one (and the next).
> >>
> >> Using rollback means that the transaction is actually commited at some
> >> point. Rolling it back will likely cause cache invalidation issue and
> >> race condition with other reader.
> >
> > Oops, I forgot that incorrect visibility for a moment at closing
> > transaction :-<
> >
> > I'll post revised ones using another way (maybe enclosing such scope
> > by dirstateguard).
> 
> I'm open to any kind of awful hack on the shelve side if it helps us to 
> move forward on the big picture.

I just posted V2 series, but sorry, I forgot adding V2 flag :-<

V2 series does:

  - add mention about removing 'bmstore.write()' to description
    (changes in the patch are equal to one in V1)

  - fully re-work about shelve/unshelve


> 
> -- 
> Pierre-Yves David
> 

----------------------------------------------------------------------
[FUJIWARA Katsunori]                             foozy@lares.dti.ne.jp

Patch

diff --git a/hgext/shelve.py b/hgext/shelve.py
--- a/hgext/shelve.py
+++ b/hgext/shelve.py
@@ -301,6 +301,19 @@ 
             desc = util.ellipsis(desc, ui.termwidth())
         ui.status(_('shelved as %s\n') % name)
         hg.update(repo, parent.node())
+
+        repo.dirstate.write() # certainly writes all changes while shelving
+        tr.close()
+        tr.release() # is ensured to run successfully by previous 'close()'
+        tr = None
+
+        # this rollbacking discards changes while shelving by
+        # restoring from undo files, but keeps dirstate as it is,
+        # because 'hg.update()' already updates the working directory
+        # to original parent (= not "parent gone")
+        repo.ui.pushbuffer()
+        repo.rollback(force=True)
+        repo.ui.popbuffer()
     finally:
         if tr:
             tr.abort()