Patchwork [07,of,13] pull: move transaction logic into the pull object

login
register
mail settings
Submitter Pierre-Yves David
Date Feb. 12, 2014, 1:34 a.m.
Message ID <db3446f5f515408abe47.1392168861@marginatus.alto.octopoid.net>
Download mbox | patch
Permalink /patch/3607/
State Accepted
Commit 2607a21bb40b251fa317b4821d9451534a871038
Headers show

Comments

Pierre-Yves David - Feb. 12, 2014, 1:34 a.m.
# HG changeset patch
# User Pierre-Yves David <pierre-yves.david@logilab.fr>
# Date 1391159045 28800
#      Fri Jan 31 01:04:05 2014 -0800
# Node ID db3446f5f515408abe47e20ee4e1dbf0d4d66b7d
# Parent  fa738c733ab74929eb898d46fc27fc62a4806024
pull: move transaction logic into the pull object

Most local change that occurs during a pull are doing within a `transaction`.
Currently this mean (1) adding new changeset (2) adding obsolescence markers. We
want the two operations to be done in the same transaction. However we do not
want to create a transaction if nothing is added to the repo. Creating an empty
transaction would drop the previous transaction data and confuse tool and people
who are still using rollback.

So the current pull code has some logic to create and handle this transaction on
demand. We are moving this logic in to the `pulloperation` object itself to
simplify this lazy creation logic through all different par of the push.

Note that, in the future, other part of pull (phases, bookmark) will probably
want to be part of the transaction too.

Patch

diff --git a/mercurial/exchange.py b/mercurial/exchange.py
--- a/mercurial/exchange.py
+++ b/mercurial/exchange.py
@@ -330,10 +330,30 @@  class pulloperation(object):
         self.remote = remote
         # revision we try to pull (None is "all")
         self.heads = heads
         # do we force pull?
         self.force = force
+        # the name the pull transaction
+        self._trname = 'pull\n' + util.hidepassword(remote.url())
+        # hold the transaction once created
+        self._tr = None
+
+    def gettransaction(self):
+        """get appropriate pull transaction, creating it if needed"""
+        if self._tr is None:
+            self._tr = self.repo.transaction(self._trname)
+        return self._tr
+
+    def closetransaction(self):
+        """close transaction if created"""
+        if self._tr is not None:
+            self._tr.close()
+
+    def releasetransaction(self):
+        """release transaction if created"""
+        if self._tr is not None:
+            self._tr.release()
 
 def pull(repo, remote, heads=None, force=False):
     pullop = pulloperation(repo, remote, heads, force)
     if pullop.remote.local():
         missing = set(pullop.remote.requirements) - pullop.repo.supported
@@ -341,14 +361,10 @@  def pull(repo, remote, heads=None, force
             msg = _("required features are not"
                     " supported in the destination:"
                     " %s") % (', '.join(sorted(missing)))
             raise util.Abort(msg)
 
-    # don't open transaction for nothing or you break future useful
-    # rollback call
-    tr = None
-    trname = 'pull\n' + util.hidepassword(pullop.remote.url())
     lock = pullop.repo.lock()
     try:
         tmp = discovery.findcommonincoming(pullop.repo.unfiltered(),
                                            pullop.remote,
                                            heads=pullop.heads,
@@ -356,11 +372,14 @@  def pull(repo, remote, heads=None, force
         common, fetch, rheads = tmp
         if not fetch:
             pullop.repo.ui.status(_("no changes found\n"))
             result = 0
         else:
-            tr = pullop.repo.transaction(trname)
+            # We delay the open of the transaction as late as possible so we
+            # don't open transaction for nothing or you break future useful
+            # rollback call
+            pullop.gettransaction()
             if pullop.heads is None and list(common) == [nullid]:
                 pullop.repo.ui.status(_("requesting all changes\n"))
             elif (pullop.heads is None
                   and pullop.remote.capable('changegroupsubset')):
                 # issue1320, avoid a race if remote changed after discovery
@@ -404,24 +423,15 @@  def pull(repo, remote, heads=None, force
         else:
             # Remote is old or publishing all common changesets
             # should be seen as public
             phases.advanceboundary(pullop.repo, phases.public, subset)
 
-        def gettransaction():
-            if tr is None:
-                return pullop.repo.transaction(trname)
-            return tr
-
-        obstr = _pullobsolete(pullop.repo, pullop.remote, gettransaction)
-        if obstr is not None:
-            tr = obstr
-
-        if tr is not None:
-            tr.close()
+        _pullobsolete(pullop.repo, pullop.remote,
+                      pullop.gettransaction)
+        pullop.closetransaction()
     finally:
-        if tr is not None:
-            tr.release()
+        pullop.releasetransaction()
         lock.release()
 
     return result
 
 def _pullobsolete(repo, remote, gettransaction):