Patchwork [1,of,4] pull: extract transaction logic into separate object

login
register
mail settings
Submitter Eric Sumner
Date Nov. 25, 2014, 6:26 p.m.
Message ID <0b5ec0d98ac188df2afa.1416939986@dev911.prn1.facebook.com>
Download mbox | patch
Permalink /patch/6854/
State Accepted
Delegated to: Pierre-Yves David
Headers show

Comments

Eric Sumner - Nov. 25, 2014, 6:26 p.m.
# HG changeset patch
# User Eric Sumner <ericsumner@fb.com>
# Date 1416609177 28800
#      Fri Nov 21 14:32:57 2014 -0800
# Node ID 0b5ec0d98ac188df2afa03457e3f551e28e1eb28
# Parent  a81c76106d9036060b3371f36b00ceefa5d60898
pull: extract transaction logic into separate object

This patch series is intended to allow bundle2 push reply part handlers to
make changes to the local repository; it has been developed in parallel with
an extension that allows the server to rebase incoming changesets while applying
them.

Aside from the transaction logic, the pulloperation class is used primarily as
a logic-free data structure for storing state information.  This diff extracts
the transaction logic into its own class that can be shared with push
operations.
Augie Fackler - Dec. 2, 2014, 3:09 p.m.
On Tue, Nov 25, 2014 at 10:26:26AM -0800, Eric Sumner wrote:
> # HG changeset patch
> # User Eric Sumner <ericsumner@fb.com>
> # Date 1416609177 28800
> #      Fri Nov 21 14:32:57 2014 -0800
> # Node ID 0b5ec0d98ac188df2afa03457e3f551e28e1eb28
> # Parent  a81c76106d9036060b3371f36b00ceefa5d60898
> pull: extract transaction logic into separate object
>
> This patch series is intended to allow bundle2 push reply part handlers to
> make changes to the local repository; it has been developed in parallel with
> an extension that allows the server to rebase incoming changesets while applying
> them.

I'd very much like a look at that extension. Any plans to release it as OSS?

(series so far looks fine to me, was just curious about details)

>
> Aside from the transaction logic, the pulloperation class is used primarily as
> a logic-free data structure for storing state information.  This diff extracts
> the transaction logic into its own class that can be shared with push
> operations.
>
> diff --git a/mercurial/exchange.py b/mercurial/exchange.py
> --- a/mercurial/exchange.py
> +++ b/mercurial/exchange.py
> @@ -771,10 +771,8 @@
>          self.explicitbookmarks = bookmarks
>          # 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
> +        # transaction manager
> +        self.trmanager = None
>          # set of common changeset between local and remote before pull
>          self.common = None
>          # set of pulled head
> @@ -807,14 +805,30 @@
>              return self.heads
>
>      def gettransaction(self):
> -        """get appropriate pull transaction, creating it if needed"""
> -        if self._tr is None:
> -            self._tr = self.repo.transaction(self._trname)
> -            self._tr.hookargs['source'] = 'pull'
> -            self._tr.hookargs['url'] = self.remote.url()
> +        # deprecated; talk to trmanager directly
> +        return self.trmanager.transaction()
> +
> +class transactionmanager(object):
> +    """An object to manages the lifecycle of a transaction
> +
> +    It creates the transaction on demand and calls the appropriate hooks when
> +    closing the transaction."""
> +    def __init__(self, repo, source, url):
> +        self.repo = repo
> +        self.source = source
> +        self.url = url
> +        self._tr = None
> +
> +    def transaction(self):
> +        """Return an open transaction object, constructing if necessary"""
> +        if not self._tr:
> +            trname = '%s\n%s' % (self.source, util.hidepassword(self.url))
> +            self._tr = self.repo.transaction(trname)
> +            self._tr.hookargs['source'] = self.source
> +            self._tr.hookargs['url'] = self.url
>          return self._tr
>
> -    def closetransaction(self):
> +    def close(self):
>          """close transaction if created"""
>          if self._tr is not None:
>              repo = self.repo
> @@ -828,7 +842,7 @@
>                                    lambda tr: repo._afterlock(runhooks))
>              self._tr.close()
>
> -    def releasetransaction(self):
> +    def release(self):
>          """release transaction if created"""
>          if self._tr is not None:
>              self._tr.release()
> @@ -846,6 +860,7 @@
>      pullop.remotebookmarks = remote.listkeys('bookmarks')
>      lock = pullop.repo.lock()
>      try:
> +        pullop.trmanager = transactionmanager(repo, 'pull', remote.url())
>          _pulldiscovery(pullop)
>          if (pullop.repo.ui.configbool('experimental', 'bundle2-exp', False)
>              and pullop.remote.capable('bundle2-exp')):
> @@ -854,9 +869,9 @@
>          _pullphase(pullop)
>          _pullbookmarks(pullop)
>          _pullobsolete(pullop)
> -        pullop.closetransaction()
> +        pullop.trmanager.close()
>      finally:
> -        pullop.releasetransaction()
> +        pullop.trmanager.release()
>          lock.release()
>
>      return pullop
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel

Patch

diff --git a/mercurial/exchange.py b/mercurial/exchange.py
--- a/mercurial/exchange.py
+++ b/mercurial/exchange.py
@@ -771,10 +771,8 @@ 
         self.explicitbookmarks = bookmarks
         # 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
+        # transaction manager
+        self.trmanager = None
         # set of common changeset between local and remote before pull
         self.common = None
         # set of pulled head
@@ -807,14 +805,30 @@ 
             return self.heads
 
     def gettransaction(self):
-        """get appropriate pull transaction, creating it if needed"""
-        if self._tr is None:
-            self._tr = self.repo.transaction(self._trname)
-            self._tr.hookargs['source'] = 'pull'
-            self._tr.hookargs['url'] = self.remote.url()
+        # deprecated; talk to trmanager directly
+        return self.trmanager.transaction()
+
+class transactionmanager(object):
+    """An object to manages the lifecycle of a transaction
+
+    It creates the transaction on demand and calls the appropriate hooks when
+    closing the transaction."""
+    def __init__(self, repo, source, url):
+        self.repo = repo
+        self.source = source
+        self.url = url
+        self._tr = None
+
+    def transaction(self):
+        """Return an open transaction object, constructing if necessary"""
+        if not self._tr:
+            trname = '%s\n%s' % (self.source, util.hidepassword(self.url))
+            self._tr = self.repo.transaction(trname)
+            self._tr.hookargs['source'] = self.source
+            self._tr.hookargs['url'] = self.url
         return self._tr
 
-    def closetransaction(self):
+    def close(self):
         """close transaction if created"""
         if self._tr is not None:
             repo = self.repo
@@ -828,7 +842,7 @@ 
                                   lambda tr: repo._afterlock(runhooks))
             self._tr.close()
 
-    def releasetransaction(self):
+    def release(self):
         """release transaction if created"""
         if self._tr is not None:
             self._tr.release()
@@ -846,6 +860,7 @@ 
     pullop.remotebookmarks = remote.listkeys('bookmarks')
     lock = pullop.repo.lock()
     try:
+        pullop.trmanager = transactionmanager(repo, 'pull', remote.url())
         _pulldiscovery(pullop)
         if (pullop.repo.ui.configbool('experimental', 'bundle2-exp', False)
             and pullop.remote.capable('bundle2-exp')):
@@ -854,9 +869,9 @@ 
         _pullphase(pullop)
         _pullbookmarks(pullop)
         _pullobsolete(pullop)
-        pullop.closetransaction()
+        pullop.trmanager.close()
     finally:
-        pullop.releasetransaction()
+        pullop.trmanager.release()
         lock.release()
 
     return pullop