Patchwork bundle2: refactor getbundle a bit

login
register
mail settings
Submitter Sune Foldager
Date Sept. 8, 2014, 7:22 p.m.
Message ID <a2e8d7f1e5a565ca30de.1410204141@Cuivienen.local>
Download mbox | patch
Permalink /patch/5726/
State Changes Requested
Headers show

Comments

Sune Foldager - Sept. 8, 2014, 7:22 p.m.
# HG changeset patch
# User Sune Foldager <cryo@cyanite.org>
# Date 1410203835 -7200
#      Mon Sep 08 21:17:15 2014 +0200
# Node ID a2e8d7f1e5a565ca30deb0cf22946d9a1251fdb2
# Parent  1f177bca1fd1149f1a61ccbe1478892b5c6703b3
bundle2: refactor getbundle a bit

This separates the bundle10 and bundle20 parts. Some code is currently
duplicated, but the paths will diverge more in the future.
Sune Foldager - Sept. 8, 2014, 7:24 p.m.
This applies to tip of clowncopter, not main.

-Sune
Pierre-Yves David - Sept. 11, 2014, 11:54 a.m.
On 09/08/2014 09:22 PM, Sune Foldager wrote:
> # HG changeset patch
> # User Sune Foldager <cryo@cyanite.org>
> # Date 1410203835 -7200
> #      Mon Sep 08 21:17:15 2014 +0200
> # Node ID a2e8d7f1e5a565ca30deb0cf22946d9a1251fdb2
> # Parent  1f177bca1fd1149f1a61ccbe1478892b5c6703b3
> bundle2: refactor getbundle a bit
>
> This separates the bundle10 and bundle20 parts. Some code is currently
> duplicated, but the paths will diverge more in the future.

Hum, why not. I think we should push the logic a bit further and have a 
subfunction for each type of bundle. If we do not share any logic, we 
better make is clear a gain a level of indentation back.

I also thing we should move the changegroup extract in its own 
subfunction as we do for the obssmarkers. This will ease wrapping of 
this logic by extensions (Mozilla have such extension planned)

Regarding your question on IRC, we could had a version parameters only 
for client that supports changegroup2. The absence of paramenters means 
cg1. Will would preserve compatibility for new client // old server and 
old client // new server. Preserving this compatibility is currently 
important as there are people beta testing bundle2 (eg: if you break 
bundle2 I cannot push your patches to the clowncopter as I'm using 
bundle2 for doing so). We'll drop this kind of BC logic when moving from 
HG2X to HG20.

Finally, here is a reminder that the most important part I would like 
extracted from your head is a patch introducing a cg2packer and a 
cg2unpacker. Can you prioritize the creation of such patch?


> diff -r 1f177bca1fd1 -r a2e8d7f1e5a5 mercurial/exchange.py
> --- a/mercurial/exchange.py	Fri Sep 05 19:54:26 2014 +0200
> +++ b/mercurial/exchange.py	Mon Sep 08 21:17:15 2014 +0200
> @@ -976,28 +976,27 @@
>       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'))
>       if bundlecaps is None or 'HG2X' not in bundlecaps:
>           if kwargs:
>               raise ValueError(_('unsupported getbundle arguments: %s')
>                                % ', '.join(sorted(kwargs.keys())))
> -        return cg
> +        return changegroup.getchangegroup(repo, source, heads=heads,
> +                                          common=common, bundlecaps=bundlecaps)
> +
>       # very crude first implementation,
>       # the bundle API will change and the generation will be done lazily.
> +    cg = None

This line seems useless.

Patch

diff -r 1f177bca1fd1 -r a2e8d7f1e5a5 mercurial/exchange.py
--- a/mercurial/exchange.py	Fri Sep 05 19:54:26 2014 +0200
+++ b/mercurial/exchange.py	Mon Sep 08 21:17:15 2014 +0200
@@ -976,28 +976,27 @@ 
     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'))
     if bundlecaps is None or 'HG2X' not in bundlecaps:
         if kwargs:
             raise ValueError(_('unsupported getbundle arguments: %s')
                              % ', '.join(sorted(kwargs.keys())))
-        return cg
+        return changegroup.getchangegroup(repo, source, heads=heads,
+                                          common=common, bundlecaps=bundlecaps)
+
     # very crude first implementation,
     # the bundle API will change and the generation will be done lazily.
+    cg = None
     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)
-    if cg:
-        bundler.newpart('b2x:changegroup', data=cg.getchunks())
+    if kwargs.get('cg', True):
+        cg = changegroup.getchangegroup(repo, source, heads=heads,
+                                        common=common, bundlecaps=bundlecaps)
+        if cg:
+            bundler.newpart('b2x:changegroup', data=cg.getchunks())
     listkeys = kwargs.get('listkeys', ())
     for namespace in listkeys:
         part = bundler.newpart('b2x:listkeys')