Patchwork D135: rebase: use one dirstateguard for when using rebase.singletransaction

login
register
mail settings
Submitter phabricator
Date July 18, 2017, 3:09 p.m.
Message ID <differential-rev-PHID-DREV-zj2jgnd6ddtducs5kxau-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/22491/
State Superseded, archived
Headers show

Comments

phabricator - July 18, 2017, 3:09 p.m.
durham created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  This was previously landed as https://phab.mercurial-scm.org/rHG2519994d25ca405edee20b816e70f2289fee553d but backed out in https://phab.mercurial-scm.org/rHGb63351f6a246cd8d444ed38641054fefc78bff93 because
  it broke hooks mid-rebase and caused conflict resolution data loss in the event
  of unexpected exceptions. This new version adds the behavior back but behind a
  config flag, since the performance improvement is notable in large repositories.
  
  The old commit message was:
  
  Recently we switched rebases to run the entire rebase inside a single
  transaction, which dramatically improved the speed of rebases in repos with
  large working copies. Let's also move the dirstate into a single dirstateguard
  to get the same benefits. This let's us avoid serializing the dirstate after
  each commit.
  
  In a large repo, rebasing 27 commits is sped up by about 20%.
  
  I believe the test changes are because us touching the dirstate gave the
  transaction something to actually rollback.
  (grafted from 9e3dc3a1638b9754b58a0cb26aaa75d868058109)
  (grafted from 7d38b41d2266d9a02a15c64229fae0da5738dcec)

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D135

AFFECTED FILES
  hgext/rebase.py
  mercurial/dirstateguard.py
  tests/test-rebase-base.t

CHANGE DETAILS




EMAIL PREFERENCES
  https://phab.mercurial-scm.org/settings/panel/emailpreferences/

To: durham, #hg-reviewers
Cc: mercurial-devel
phabricator - July 18, 2017, 3:31 p.m.
martinvonz added inline comments.

INLINE COMMENTS

> rebase.py:483
> +
> +            dsguard = util.nullcontextmanager()
> +            if ui.configbool('rebase', 'singletransaction'):

As in the previous patch, this can be None here and in the cases below

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D135

EMAIL PREFERENCES
  https://phab.mercurial-scm.org/settings/panel/emailpreferences/

To: durham, #hg-reviewers
Cc: martinvonz, mercurial-devel
phabricator - July 18, 2017, 8:42 p.m.
martinvonz added inline comments.

INLINE COMMENTS

> util.py:612-617
> +    def close(self):
> +        pass
> +    def release(self):
> +        pass
> +    def abort(self):
> +        pass

Are these necessary? Looks like the nullcontextmanager() is now only used where a plain contextmanager is expected. That should also mean that you should be able to replace this class by:

  @contextlib.contextmanager
  def nullcontextmanager():
      yield

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D135

EMAIL PREFERENCES
  https://phab.mercurial-scm.org/settings/panel/emailpreferences/

To: durham, #hg-reviewers
Cc: martinvonz, mercurial-devel
phabricator - July 20, 2017, 3:54 p.m.
martinvonz accepted this revision.
martinvonz added a comment.
This revision is now accepted and ready to land.


  Looks good, but we're in code freeze, so it will have to wait until after.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D135

EMAIL PREFERENCES
  https://phab.mercurial-scm.org/settings/panel/emailpreferences/

To: durham, #hg-reviewers, martinvonz
Cc: martinvonz, mercurial-devel

Patch

diff --git a/tests/test-rebase-base.t b/tests/test-rebase-base.t
--- a/tests/test-rebase-base.t
+++ b/tests/test-rebase-base.t
@@ -379,3 +379,40 @@ 
    /
   o  0: A
   
+Rebasing using a single transaction
+
+  $ hg init singletr && cd singletr
+  $ cat >> .hg/hgrc <<EOF
+  > [rebase]
+  > singletransaction=True
+  > EOF
+  $ hg debugdrawdag <<'EOF'
+  >   Z
+  >   |
+  >   | D
+  >   | |
+  >   | C
+  >   | |
+  >   Y B
+  >   |/
+  >   A
+  > EOF
+- We should only see two status stored messages. One from the start, one from
+- the end.
+  $ hg rebase --debug -b D -d Z | grep 'status stored'
+  rebase status stored
+  rebase status stored
+  $ hg tglog
+  o  5: D
+  |
+  o  4: C
+  |
+  o  3: B
+  |
+  o  2: Z
+  |
+  o  1: Y
+  |
+  o  0: A
+  
+  $ cd ..
diff --git a/mercurial/dirstateguard.py b/mercurial/dirstateguard.py
--- a/mercurial/dirstateguard.py
+++ b/mercurial/dirstateguard.py
@@ -43,6 +43,16 @@ 
             # ``release(tr, ....)``.
             self._abort()
 
+    def __enter__(self):
+        return self
+
+    def __exit__(self, exc_type, exc_val, exc_tb):
+        try:
+            if exc_type is None:
+                self.close()
+        finally:
+            self.release()
+
     def close(self):
         if not self._active: # already inactivated
             msg = (_("can't close already inactivated backup: %s")
diff --git a/hgext/rebase.py b/hgext/rebase.py
--- a/hgext/rebase.py
+++ b/hgext/rebase.py
@@ -479,12 +479,17 @@ 
                 editopt = True
             editor = cmdutil.getcommiteditor(edit=editopt, editform=editform)
             revtoreuse = max(self.state)
-            newnode = concludenode(repo, revtoreuse, p1, self.external,
-                                   commitmsg=commitmsg,
-                                   extrafn=_makeextrafn(self.extrafns),
-                                   editor=editor,
-                                   keepbranches=self.keepbranchesf,
-                                   date=self.date)
+
+            dsguard = util.nullcontextmanager()
+            if ui.configbool('rebase', 'singletransaction'):
+                dsguard = dirstateguard.dirstateguard(repo, 'rebase')
+            with util.acceptintervention(dsguard):
+                newnode = concludenode(repo, revtoreuse, p1, self.external,
+                                       commitmsg=commitmsg,
+                                       extrafn=_makeextrafn(self.extrafns),
+                                       editor=editor,
+                                       keepbranches=self.keepbranchesf,
+                                       date=self.date)
             if newnode is None:
                 newrev = self.dest
             else:
@@ -709,13 +714,17 @@ 
             if retcode is not None:
                 return retcode
 
+        tr = util.nullcontextmanager()
+        dsguard = util.nullcontextmanager()
+
         singletr = ui.configbool('rebase', 'singletransaction')
         if singletr:
             tr = repo.transaction('rebase')
-        else:
-            tr = util.nullcontextmanager()
         with util.acceptintervention(tr):
-            rbsrt._performrebase(tr if singletr else None)
+            if singletr:
+                dsguard = dirstateguard.dirstateguard(repo, 'rebase')
+            with util.acceptintervention(dsguard):
+                rbsrt._performrebase(tr if singletr else None)
 
         rbsrt._finishrebase()
 
@@ -842,8 +851,10 @@ 
     '''Commit the wd changes with parents p1 and p2. Reuse commit info from rev
     but also store useful information in extra.
     Return node of committed revision.'''
-    dsguard = dirstateguard.dirstateguard(repo, 'rebase')
-    try:
+    dsguard = util.nullcontextmanager()
+    if not repo.ui.configbool('rebase', 'singletransaction'):
+        dsguard = dirstateguard.dirstateguard(repo, 'rebase')
+    with dsguard:
         repo.setparents(repo[p1].node(), repo[p2].node())
         ctx = repo[rev]
         if commitmsg is None:
@@ -865,10 +876,7 @@ 
                                   date=date, extra=extra, editor=editor)
 
         repo.dirstate.setbranch(repo[newnode].branch())
-        dsguard.close()
         return newnode
-    finally:
-        release(dsguard)
 
 def rebasenode(repo, rev, p1, base, state, collapse, dest):
     'Rebase a single revision rev on top of p1 using base as merge ancestor'