Patchwork bundle2: separate bundle10 and bundle2 cases in getbundle()

login
register
mail settings
Submitter Mike Hommey
Date Sept. 24, 2014, 8:15 a.m.
Message ID <07e31801a4b3fb762bf3.1411546555@zenigata.glandium.org>
Download mbox | patch
Permalink /patch/5956/
State Superseded
Commit 6b180a0c703ea12f27be2f855ea1d6965a63a1ed
Headers show

Comments

Mike Hommey - Sept. 24, 2014, 8:15 a.m.
# HG changeset patch
# User Mike Hommey <mh@glandium.org>
# Date 1411539963 -32400
#      Wed Sep 24 15:26:03 2014 +0900
# Node ID 07e31801a4b3fb762bf3cecf9875186cc182dcd0
# Parent  bc01442c6e030eee52c5f5524a28a50b1c25a4a6
bundle2: separate bundle10 and bundle2 cases in getbundle()

The primary goal is to make it easier for extensions to alter how bundle2
parts are laid out. While I don't expect this to be the final hookable form,
this hookability is necessary for upcoming tests for new bundle2 features.

Note the 'request for bundle10 must include changegroup' error was kept
under the same conditions as before, although the logic changes don't make
it obvious.
Augie Fackler - Sept. 24, 2014, 2:09 p.m.
On Wed, Sep 24, 2014 at 05:15:55PM +0900, Mike Hommey wrote:
> # HG changeset patch
> # User Mike Hommey <mh@glandium.org>
> # Date 1411539963 -32400
> #      Wed Sep 24 15:26:03 2014 +0900
> # Node ID 07e31801a4b3fb762bf3cecf9875186cc182dcd0
> # Parent  bc01442c6e030eee52c5f5524a28a50b1c25a4a6
> bundle2: separate bundle10 and bundle2 cases in getbundle()

Looks reasonable, but I'm going to defer to Pierre-Yves for a final review.

>
> The primary goal is to make it easier for extensions to alter how bundle2
> parts are laid out. While I don't expect this to be the final hookable form,
> this hookability is necessary for upcoming tests for new bundle2 features.
>
> Note the 'request for bundle10 must include changegroup' error was kept
> under the same conditions as before, although the logic changes don't make
> it obvious.
>
> diff --git a/mercurial/exchange.py b/mercurial/exchange.py
> --- a/mercurial/exchange.py
> +++ b/mercurial/exchange.py
> @@ -971,61 +971,85 @@ def getbundle(repo, source, heads=None,
>
>      This is different from changegroup.getchangegroup that only returns an HG10
>      changegroup bundle. They may eventually get reunited in the future when we
>      have a clearer idea of the API we what to query different data.
>
>      The implementation is at a very early stage and will get massive rework
>      when the API of bundle is refined.
>      """
> -    cg = None
> -    if kwargs.get('cg', True):
> -        # build changegroup bundle here.
> -        cg = changegroup.getchangegroup(repo, source, heads=heads,
> -                                         common=common, bundlecaps=bundlecaps)
> -    elif 'HG2X' not in bundlecaps:
> -        raise ValueError(_('request for bundle10 must include changegroup'))
> +    # bundle10 case
>      if bundlecaps is None or 'HG2X' not in bundlecaps:
> +        if bundlecaps and not kwargs.get('cg', True):
> +            raise ValueError(_('request for bundle10 must include changegroup'))
> +
>          if kwargs:
>              raise ValueError(_('unsupported getbundle arguments: %s')
>                               % ', '.join(sorted(kwargs.keys())))
> -        return cg
> -    # very crude first implementation,
> +        return changegroup.getchangegroup(repo, source, heads=heads,
> +                                          common=common, bundlecaps=bundlecaps)
> +
> +    # bundle20 case ; very crude first implementation,
>      # the bundle API will change and the generation will be done lazily.
>      b2caps = {}
>      for bcaps in bundlecaps:
>          if bcaps.startswith('bundle2='):
>              blob = urllib.unquote(bcaps[len('bundle2='):])
>              b2caps.update(bundle2.decodecaps(blob))
>      bundler = bundle2.bundle20(repo.ui, b2caps)
> +
> +    # Use a separate function to ease hooking the whole bundle content.
> +    _getbundle2parts(bundler, repo, source, heads=heads, common=common,
> +                     bundlecaps=bundlecaps, b2caps=b2caps, **kwargs)
> +
> +    return util.chunkbuffer(bundler.getchunks())
> +
> +def _getbundle2parts(bundler, repo, source, heads=None, common=None,
> +                     bundlecaps=None, b2caps=None, **kwargs):
> +    partfuncs = (
> +        _getbundlechangegrouppart,
> +        _getbundlelistkeyspart,
> +        _getbundleobsmarkerpart,
> +        _getbundleextrapart,
> +    )
> +    for func in partfuncs:
> +        func(bundler, repo, source, heads=heads, common=common,
> +             bundlecaps=bundlecaps, b2caps=b2caps, **kwargs)
> +
> +def _getbundlechangegrouppart(bundler, repo, source, heads=None, common=None,
> +                              bundlecaps=None, b2caps=None, **kwargs):
> +    cg = None
> +    if kwargs.get('cg', True):
> +        # build changegroup bundle here.
> +        cg = changegroup.getchangegroup(repo, source, heads=heads,
> +                                        common=common, bundlecaps=bundlecaps)
> +
>      if cg:
>          bundler.newpart('b2x:changegroup', data=cg.getchunks())
> +
> +def _getbundlelistkeyspart(bundler, repo, source, heads=None, common=None,
> +                           bundlecaps=None, b2caps=None, **kwargs):
>      listkeys = kwargs.get('listkeys', ())
>      for namespace in listkeys:
>          part = bundler.newpart('b2x:listkeys')
>          part.addparam('namespace', namespace)
>          keys = repo.listkeys(namespace).items()
>          part.data = pushkey.encodekeys(keys)
> -    _getbundleobsmarkerpart(bundler, repo, source, heads=heads, common=common,
> -                            bundlecaps=bundlecaps, **kwargs)
> -    _getbundleextrapart(bundler, repo, source, heads=heads, common=common,
> -                        bundlecaps=bundlecaps, **kwargs)
> -    return util.chunkbuffer(bundler.getchunks())
>
>  def _getbundleobsmarkerpart(bundler, repo, source, heads=None, common=None,
> -                            bundlecaps=None, **kwargs):
> +                            bundlecaps=None, b2caps=None, **kwargs):
>      if kwargs.get('obsmarkers', False):
>          if heads is None:
>              heads = repo.heads()
>          subset = [c.node() for c in repo.set('::%ln', heads)]
>          markers = repo.obsstore.relevantmarkers(subset)
>          buildobsmarkerspart(bundler, markers)
>
>  def _getbundleextrapart(bundler, repo, source, heads=None, common=None,
> -                        bundlecaps=None, **kwargs):
> +                        bundlecaps=None, b2caps=None, **kwargs):
>      """hook function to let extensions add parts to the requested bundle"""
>      pass
>
>  def check_heads(repo, their_heads, context):
>      """check if the heads of a repo have been modified
>
>      Used by peer for unbundling.
>      """
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
Pierre-Yves David - Sept. 25, 2014, 12:59 a.m.
On 09/24/2014 01:15 AM, Mike Hommey wrote:
> # HG changeset patch
> # User Mike Hommey <mh@glandium.org>
> # Date 1411539963 -32400
> #      Wed Sep 24 15:26:03 2014 +0900
> # Node ID 07e31801a4b3fb762bf3cecf9875186cc182dcd0
> # Parent  bc01442c6e030eee52c5f5524a28a50b1c25a4a6
> bundle2: separate bundle10 and bundle2 cases in getbundle()

\o/, however I've a couple of feedback on the implementation.

>
> The primary goal is to make it easier for extensions to alter how bundle2
> parts are laid out. While I don't expect this to be the final hookable form,
> this hookability is necessary for upcoming tests for new bundle2 features.
>
> Note the 'request for bundle10 must include changegroup' error was kept
> under the same conditions as before, although the logic changes don't make
> it obvious.
>
> diff --git a/mercurial/exchange.py b/mercurial/exchange.py
> --- a/mercurial/exchange.py
> +++ b/mercurial/exchange.py
> @@ -971,61 +971,85 @@ def getbundle(repo, source, heads=None,
>
>       This is different from changegroup.getchangegroup that only returns an HG10
>       changegroup bundle. They may eventually get reunited in the future when we
>       have a clearer idea of the API we what to query different data.
>
>       The implementation is at a very early stage and will get massive rework
>       when the API of bundle is refined.
>       """
> -    cg = None
> -    if kwargs.get('cg', True):
> -        # build changegroup bundle here.
> -        cg = changegroup.getchangegroup(repo, source, heads=heads,
> -                                         common=common, bundlecaps=bundlecaps)
> -    elif 'HG2X' not in bundlecaps:
> -        raise ValueError(_('request for bundle10 must include changegroup'))
> +    # bundle10 case
>       if bundlecaps is None or 'HG2X' not in bundlecaps:
> +        if bundlecaps and not kwargs.get('cg', True):
> +            raise ValueError(_('request for bundle10 must include changegroup'))
> +
>           if kwargs:
>               raise ValueError(_('unsupported getbundle arguments: %s')
>                                % ', '.join(sorted(kwargs.keys())))
> -        return cg
> -    # very crude first implementation,
> +        return changegroup.getchangegroup(repo, source, heads=heads,
> +                                          common=common, bundlecaps=bundlecaps)
> +
> +    # bundle20 case ; very crude first implementation,
>       # the bundle API will change and the generation will be done lazily.

Note that we can probably drop that two lines comment (ha, comments…)

>       b2caps = {}
>       for bcaps in bundlecaps:
>           if bcaps.startswith('bundle2='):
>               blob = urllib.unquote(bcaps[len('bundle2='):])
>               b2caps.update(bundle2.decodecaps(blob))
>       bundler = bundle2.bundle20(repo.ui, b2caps)

I love this idea of extracting the bundle2cas once and for all. But can 
this get in its own patch to clarify the actuall change made here?

> +
> +    # Use a separate function to ease hooking the whole bundle content.
> +    _getbundle2parts(bundler, repo, source, heads=heads, common=common,
> +                     bundlecaps=bundlecaps, b2caps=b2caps, **kwargs)
> +
> +    return util.chunkbuffer(bundler.getchunks())
> +
> +def _getbundle2parts(bundler, repo, source, heads=None, common=None,
> +                     bundlecaps=None, b2caps=None, **kwargs):

Bonus point if you add some documentation.

We should also be able to move heads and common back in kwargs for all 
bundle2 related function multiple of them are not going to use it. 
(deserve its own patches)



> +    partfuncs = (
> +        _getbundlechangegrouppart,
> +        _getbundlelistkeyspart,
> +        _getbundleobsmarkerpart,
> +        _getbundleextrapart,
> +    )

This should be a combination of list and dict outside of the function.

The list will contains name of step to be performed (as string). The 
dict will maps "step" to <function>. This way:

  - extension can add their own step anywhere in the flow.
  - extension can overwrite a give step easily (I wonder who could make 
use of this…)

(see how during the push is handled the same way sooner in the same file)

> +    for func in partfuncs:
> +        func(bundler, repo, source, heads=heads, common=common,
> +             bundlecaps=bundlecaps, b2caps=b2caps, **kwargs)
> +
> +def _getbundlechangegrouppart(bundler, repo, source, heads=None, common=None,
> +                              bundlecaps=None, b2caps=None, **kwargs):

Bonus point if you add some documentation

> +    cg = None
> +    if kwargs.get('cg', True):
> +        # build changegroup bundle here.
> +        cg = changegroup.getchangegroup(repo, source, heads=heads,
> +                                        common=common, bundlecaps=bundlecaps)
> +
>       if cg:
>           bundler.newpart('b2x:changegroup', data=cg.getchunks())
> +
> +def _getbundlelistkeyspart(bundler, repo, source, heads=None, common=None,
> +                           bundlecaps=None, b2caps=None, **kwargs):
>       listkeys = kwargs.get('listkeys', ())
>       for namespace in listkeys:
>           part = bundler.newpart('b2x:listkeys')
>           part.addparam('namespace', namespace)
>           keys = repo.listkeys(namespace).items()
>           part.data = pushkey.encodekeys(keys)
> -    _getbundleobsmarkerpart(bundler, repo, source, heads=heads, common=common,
> -                            bundlecaps=bundlecaps, **kwargs)
> -    _getbundleextrapart(bundler, repo, source, heads=heads, common=common,
> -                        bundlecaps=bundlecaps, **kwargs)
> -    return util.chunkbuffer(bundler.getchunks())
>
>   def _getbundleobsmarkerpart(bundler, repo, source, heads=None, common=None,
> -                            bundlecaps=None, **kwargs):
> +                            bundlecaps=None, b2caps=None, **kwargs):

Bonus point if you add some documentation

>       if kwargs.get('obsmarkers', False):
>           if heads is None:
>               heads = repo.heads()
>           subset = [c.node() for c in repo.set('::%ln', heads)]
>           markers = repo.obsstore.relevantmarkers(subset)
>           buildobsmarkerspart(bundler, markers)
>
>   def _getbundleextrapart(bundler, repo, source, heads=None, common=None,
> -                        bundlecaps=None, **kwargs):
> +                        bundlecaps=None, b2caps=None, **kwargs):

side note: this function will eventually die if we get the extensible 
list of step in place (but this is for the future).

>       """hook function to let extensions add parts to the requested bundle"""
>       pass
>
>   def check_heads(repo, their_heads, context):
>       """check if the heads of a repo have been modified
>
>       Used by peer for unbundling.
>       """
>
Mike Hommey - Sept. 25, 2014, 1:14 a.m.
On Wed, Sep 24, 2014 at 05:59:30PM -0700, Pierre-Yves David wrote:
> This should be a combination of list and dict outside of the function.
>
> The list will contains name of step to be performed (as string). The dict
> will maps "step" to <function>. This way:
> 
>  - extension can add their own step anywhere in the flow.
>  - extension can overwrite a give step easily (I wonder who could make use
> of this…)

Well, I do ;) (I want to replace the changegroup part)

Mike

Patch

diff --git a/mercurial/exchange.py b/mercurial/exchange.py
--- a/mercurial/exchange.py
+++ b/mercurial/exchange.py
@@ -971,61 +971,85 @@  def getbundle(repo, source, heads=None, 
 
     This is different from changegroup.getchangegroup that only returns an HG10
     changegroup bundle. They may eventually get reunited in the future when we
     have a clearer idea of the API we what to query different data.
 
     The implementation is at a very early stage and will get massive rework
     when the API of bundle is refined.
     """
-    cg = None
-    if kwargs.get('cg', True):
-        # build changegroup bundle here.
-        cg = changegroup.getchangegroup(repo, source, heads=heads,
-                                         common=common, bundlecaps=bundlecaps)
-    elif 'HG2X' not in bundlecaps:
-        raise ValueError(_('request for bundle10 must include changegroup'))
+    # bundle10 case
     if bundlecaps is None or 'HG2X' not in bundlecaps:
+        if bundlecaps and not kwargs.get('cg', True):
+            raise ValueError(_('request for bundle10 must include changegroup'))
+
         if kwargs:
             raise ValueError(_('unsupported getbundle arguments: %s')
                              % ', '.join(sorted(kwargs.keys())))
-        return cg
-    # very crude first implementation,
+        return changegroup.getchangegroup(repo, source, heads=heads,
+                                          common=common, bundlecaps=bundlecaps)
+
+    # bundle20 case ; very crude first implementation,
     # the bundle API will change and the generation will be done lazily.
     b2caps = {}
     for bcaps in bundlecaps:
         if bcaps.startswith('bundle2='):
             blob = urllib.unquote(bcaps[len('bundle2='):])
             b2caps.update(bundle2.decodecaps(blob))
     bundler = bundle2.bundle20(repo.ui, b2caps)
+
+    # Use a separate function to ease hooking the whole bundle content.
+    _getbundle2parts(bundler, repo, source, heads=heads, common=common,
+                     bundlecaps=bundlecaps, b2caps=b2caps, **kwargs)
+
+    return util.chunkbuffer(bundler.getchunks())
+
+def _getbundle2parts(bundler, repo, source, heads=None, common=None,
+                     bundlecaps=None, b2caps=None, **kwargs):
+    partfuncs = (
+        _getbundlechangegrouppart,
+        _getbundlelistkeyspart,
+        _getbundleobsmarkerpart,
+        _getbundleextrapart,
+    )
+    for func in partfuncs:
+        func(bundler, repo, source, heads=heads, common=common,
+             bundlecaps=bundlecaps, b2caps=b2caps, **kwargs)
+
+def _getbundlechangegrouppart(bundler, repo, source, heads=None, common=None,
+                              bundlecaps=None, b2caps=None, **kwargs):
+    cg = None
+    if kwargs.get('cg', True):
+        # build changegroup bundle here.
+        cg = changegroup.getchangegroup(repo, source, heads=heads,
+                                        common=common, bundlecaps=bundlecaps)
+
     if cg:
         bundler.newpart('b2x:changegroup', data=cg.getchunks())
+
+def _getbundlelistkeyspart(bundler, repo, source, heads=None, common=None,
+                           bundlecaps=None, b2caps=None, **kwargs):
     listkeys = kwargs.get('listkeys', ())
     for namespace in listkeys:
         part = bundler.newpart('b2x:listkeys')
         part.addparam('namespace', namespace)
         keys = repo.listkeys(namespace).items()
         part.data = pushkey.encodekeys(keys)
-    _getbundleobsmarkerpart(bundler, repo, source, heads=heads, common=common,
-                            bundlecaps=bundlecaps, **kwargs)
-    _getbundleextrapart(bundler, repo, source, heads=heads, common=common,
-                        bundlecaps=bundlecaps, **kwargs)
-    return util.chunkbuffer(bundler.getchunks())
 
 def _getbundleobsmarkerpart(bundler, repo, source, heads=None, common=None,
-                            bundlecaps=None, **kwargs):
+                            bundlecaps=None, b2caps=None, **kwargs):
     if kwargs.get('obsmarkers', False):
         if heads is None:
             heads = repo.heads()
         subset = [c.node() for c in repo.set('::%ln', heads)]
         markers = repo.obsstore.relevantmarkers(subset)
         buildobsmarkerspart(bundler, markers)
 
 def _getbundleextrapart(bundler, repo, source, heads=None, common=None,
-                        bundlecaps=None, **kwargs):
+                        bundlecaps=None, b2caps=None, **kwargs):
     """hook function to let extensions add parts to the requested bundle"""
     pass
 
 def check_heads(repo, their_heads, context):
     """check if the heads of a repo have been modified
 
     Used by peer for unbundling.
     """