Patchwork [2,of,2] obsolete: extract obsolescence marker pulling in a dedicated function

login
register
mail settings
Submitter Pierre-Yves David
Date April 18, 2013, 5:16 p.m.
Message ID <6079d531bcb5542b24e4.1366305382@crater2.logilab.fr>
Download mbox | patch
Permalink /patch/1426/
State Deferred, archived
Headers show

Comments

Pierre-Yves David - April 18, 2013, 5:16 p.m.
# HG changeset patch
# User Pierre-Yves David <pierre-yves.david@logilab.fr>
# Date 1366192069 -7200
#      Wed Apr 17 11:47:49 2013 +0200
# Node ID 6079d531bcb5542b24e4dc192d21eb926b8d223c
# Parent  368a3c420ecf9712b2efa3caf894acf447814b3a
obsolete: extract obsolescence marker pulling in a dedicated function

Having a dedicated function will allows use to experiment with other exchange
strategy in extension. Has we have not solid clues about how to do it right
being able to experiment is vital.

Some transaction trick are necessary for pull. But nothing to scary.
Pierre-Yves David - April 18, 2013, 9:48 p.m.
On 18 avr. 2013, at 23:12, Kevin Bullock wrote:

> (sorry for previous empty reply)
> 
> On 18 Apr 2013, at 12:16 PM, pierre-yves.david@logilab.fr wrote:
> 
>> # HG changeset patch
>> # User Pierre-Yves David <pierre-yves.david@logilab.fr>
>> # Date 1366192069 -7200
>> #      Wed Apr 17 11:47:49 2013 +0200
>> # Node ID 6079d531bcb5542b24e4dc192d21eb926b8d223c
>> # Parent  368a3c420ecf9712b2efa3caf894acf447814b3a
>> obsolete: extract obsolescence marker pulling in a dedicated function
>> [...]
>> diff --git a/mercurial/obsolete.py b/mercurial/obsolete.py
>> --- a/mercurial/obsolete.py
>> +++ b/mercurial/obsolete.py
>> @@ -371,11 +371,11 @@ def pushmarker(repo, key, old, new):
>>        lock.release()
>> 
>> def syncpush(repo, remote):
>>    """utility function to push bookmark to a remote
>> 
>> -    exist mostly to allow overridding for experimentation purpose"""
>> +    Exist mostly to allow overridding for experimentation purpose"""
> 
> Unrelated case change. Should've been rolled into previous patch.

yeah right.

> Otherwise looks okay, but would it be possible to set tr in gettransaction() rather than making syncpull() return it? Maybe the fact that I have to ask that means 'no'.

That would mean trapping a reference to a something mutable (list, fict?) from localrepo.pull in the closure gettransaction before passing it to syncpush and put the tr in there when creating it.

I dediced that was a level of magnitude than the current implementation.

Another approach would be to turn push into an object and have it handle the tr… ho wait.

Patch

diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
--- a/mercurial/localrepo.py
+++ b/mercurial/localrepo.py
@@ -6,11 +6,11 @@ 
 # GNU General Public License version 2 or any later version.
 from node import hex, nullid, short
 from i18n import _
 import peer, changegroup, subrepo, discovery, pushkey, obsolete, repoview
 import changelog, dirstate, filelog, manifest, context, bookmarks, phases
-import lock, transaction, store, encoding, base85
+import lock, transaction, store, encoding
 import scmutil, util, extensions, hook, error, revset
 import match as matchmod
 import merge as mergemod
 import tags as tagsmod
 from lock import release
@@ -1715,21 +1715,19 @@  class localrepository(object):
             else:
                 # Remote is old or publishing all common changesets
                 # should be seen as public
                 phases.advanceboundary(self, phases.public, subset)
 
-            if obsolete._enabled:
-                self.ui.debug('fetching remote obsolete markers\n')
-                remoteobs = remote.listkeys('obsolete')
-                if 'dump0' in remoteobs:
-                    if tr is None:
-                        tr = self.transaction(trname)
-                    for key in sorted(remoteobs, reverse=True):
-                        if key.startswith('dump'):
-                            data = base85.b85decode(remoteobs[key])
-                            self.obsstore.mergemarkers(tr, data)
-                    self.invalidatevolatilesets()
+            def gettransaction():
+                if tr is None:
+                    return self.transaction(trname)
+                return tr
+
+            obstr = obsolete.syncpull(self, remote, gettransaction)
+            if obstr is not None:
+                tr = obstr
+
             if tr is not None:
                 tr.close()
         finally:
             if tr is not None:
                 tr.release()
diff --git a/mercurial/obsolete.py b/mercurial/obsolete.py
--- a/mercurial/obsolete.py
+++ b/mercurial/obsolete.py
@@ -371,11 +371,11 @@  def pushmarker(repo, key, old, new):
         lock.release()
 
 def syncpush(repo, remote):
     """utility function to push bookmark to a remote
 
-    exist mostly to allow overridding for experimentation purpose"""
+    Exist mostly to allow overridding for experimentation purpose"""
     if (_enabled and repo.obsstore and
         'obsolete' in remote.listkeys('namespaces')):
         rslts = []
         remotedata = repo.listkeys('obsolete')
         for key in sorted(remotedata, reverse=True):
@@ -384,10 +384,31 @@  def syncpush(repo, remote):
             rslts.append(remote.pushkey('obsolete', key, '', data))
         if [r for r in rslts if not r]:
             msg = _('failed to push some obsolete markers!\n')
             repo.ui.warn(msg)
 
+def syncpull(repo, remote, gettransaction):
+    """utility function to pull bookmark to a remote
+
+    The `gettransaction` is function that return the pull transaction, creating
+    one if necessary. We return the transaction to inform the calling code that
+    a new transaction have been created (when applicable).
+
+    Exists mostly to allow overridding for experimentation purpose"""
+    tr = None
+    if _enabled:
+        repo.ui.debug('fetching remote obsolete markers\n')
+        remoteobs = remote.listkeys('obsolete')
+        if 'dump0' in remoteobs:
+            tr = gettransaction()
+            for key in sorted(remoteobs, reverse=True):
+                if key.startswith('dump'):
+                    data = base85.b85decode(remoteobs[key])
+                    repo.obsstore.mergemarkers(tr, data)
+            repo.invalidatevolatilesets()
+    return tr
+
 
 
 def allmarkers(repo):
     """all obsolete markers known in a repository"""
     for markerdata in repo.obsstore: