Patchwork [4,of,4] bundle2: Remove unused heads and common arguments to getbundle2partsgenerator functions

login
register
mail settings
Submitter Mike Hommey
Date Sept. 25, 2014, 2:56 a.m.
Message ID <e36dcfe3f313c0183524.1411613761@zenigata.glandium.org>
Download mbox | patch
Permalink /patch/5974/
State Changes Requested
Headers show

Comments

Mike Hommey - Sept. 25, 2014, 2:56 a.m.
# HG changeset patch
# User Mike Hommey <mh@glandium.org>
# Date 1411613656 -32400
#      Thu Sep 25 11:54:16 2014 +0900
# Node ID e36dcfe3f313c0183524050f32b74fc040182995
# Parent  17baf7a6335c720094b1e4d1b2b2c9d9911b5ba5
bundle2: Remove unused heads and common arguments to getbundle2partsgenerator functions
Pierre-Yves David - Sept. 25, 2014, 3:59 a.m.
On 09/24/2014 07:56 PM, Mike Hommey wrote:
> # HG changeset patch
> # User Mike Hommey <mh@glandium.org>
> # Date 1411613656 -32400
> #      Thu Sep 25 11:54:16 2014 +0900
> # Node ID e36dcfe3f313c0183524050f32b74fc040182995
> # Parent  17baf7a6335c720094b1e4d1b2b2c9d9911b5ba5
> bundle2: Remove unused heads and common arguments to getbundle2partsgenerator functions

I had something a bit more radical in mind, heads and common should stop 
being passed explicitly at any of the bundle2 function. The first 
arguments would be bundler, repo, source, bundlecaps, and bundlecaps2. 
And the heads + common one would be passed through kwargs in all cases.
(so nothing in wrong in the patch, but the callling should change too as 
well as arguments order in getchangegrouppart)


Also, test-check-commit-hg says hi:

++ /home/marmoute/mercurial-testing/tests/test-check-commit-hg.t.err
@@ -22,5 +22,10 @@
    >        echo
    >   fi
    > done
+  Revision 3f3fcaa5cf78 does not comply to commit message rules
+  ------------------------------------------------------
+  6: summary line too long
+   bundle2: Remove unused heads and common arguments to 
getbundle2partsgenerator functions
+



ERROR: test-check-commit-hg.t output changed


> diff --git a/mercurial/exchange.py b/mercurial/exchange.py
> --- a/mercurial/exchange.py
> +++ b/mercurial/exchange.py
> @@ -1035,28 +1035,28 @@ def _getbundlechangegrouppart(bundler, r
>           # build changegroup bundle here.
>           cg = changegroup.getchangegroup(repo, source, heads=heads,
>                                           common=common, bundlecaps=bundlecaps)
>
>       if cg:
>           bundler.newpart('b2x:changegroup', data=cg.getchunks())
>
>   @getbundle2partsgenerator('listkeys')
> -def _getbundlelistkeysparts(bundler, repo, source, heads=None, common=None,
> -                           bundlecaps=None, b2caps=None, **kwargs):
> +def _getbundlelistkeysparts(bundler, repo, source, bundlecaps=None,
> +                            b2caps=None, **kwargs):
>       """add parts containing listkeys namespaces to the requested bundle"""
>       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)
>
>   @getbundle2partsgenerator('obsmarkers')
> -def _getbundleobsmarkerpart(bundler, repo, source, heads=None, common=None,
> +def _getbundleobsmarkerpart(bundler, repo, source, heads=None,
>                               bundlecaps=None, b2caps=None, **kwargs):
>       """add an obsolescence markers part to the requested bundle"""
>       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)
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
>
Mike Hommey - Sept. 25, 2014, 4:16 a.m.
On Wed, Sep 24, 2014 at 08:59:10PM -0700, Pierre-Yves David wrote:
> 
> 
> On 09/24/2014 07:56 PM, Mike Hommey wrote:
> ># HG changeset patch
> ># User Mike Hommey <mh@glandium.org>
> ># Date 1411613656 -32400
> >#      Thu Sep 25 11:54:16 2014 +0900
> ># Node ID e36dcfe3f313c0183524050f32b74fc040182995
> ># Parent  17baf7a6335c720094b1e4d1b2b2c9d9911b5ba5
> >bundle2: Remove unused heads and common arguments to getbundle2partsgenerator functions
> 
> I had something a bit more radical in mind, heads and common should stop
> being passed explicitly at any of the bundle2 function. The first arguments
> would be bundler, repo, source, bundlecaps, and bundlecaps2. And the heads +
> common one would be passed through kwargs in all cases.
> (so nothing in wrong in the patch, but the callling should change too as
> well as arguments order in getchangegrouppart)

I'm not sure what there is to gain from changing:
          func(bundler, repo, source, heads=heads, common=common,
                       bundlecaps=bundlecaps, b2caps=b2caps, **kwargs)

to
          kwargs['heads'] = heads
          kwargs['common'] = common
          func(bundler, repo, source, bundlecaps=bundlecaps, b2caps=b2caps,
               **kwargs)

when from the callee's perspective there's no difference. There would be
a difference if kwargs was passed as a plain argument, not as **kwargs.

Mike
Pierre-Yves David - Sept. 25, 2014, 4:21 a.m.
On 09/24/2014 09:16 PM, Mike Hommey wrote:
> On Wed, Sep 24, 2014 at 08:59:10PM -0700, Pierre-Yves David wrote:
>>
>>
>> On 09/24/2014 07:56 PM, Mike Hommey wrote:
>>> # HG changeset patch
>>> # User Mike Hommey <mh@glandium.org>
>>> # Date 1411613656 -32400
>>> #      Thu Sep 25 11:54:16 2014 +0900
>>> # Node ID e36dcfe3f313c0183524050f32b74fc040182995
>>> # Parent  17baf7a6335c720094b1e4d1b2b2c9d9911b5ba5
>>> bundle2: Remove unused heads and common arguments to getbundle2partsgenerator functions
>>
>> I had something a bit more radical in mind, heads and common should stop
>> being passed explicitly at any of the bundle2 function. The first arguments
>> would be bundler, repo, source, bundlecaps, and bundlecaps2. And the heads +
>> common one would be passed through kwargs in all cases.
>> (so nothing in wrong in the patch, but the callling should change too as
>> well as arguments order in getchangegrouppart)
>
> I'm not sure what there is to gain from changing:
>            func(bundler, repo, source, heads=heads, common=common,
>                         bundlecaps=bundlecaps, b2caps=b2caps, **kwargs)
>
> to
>            kwargs['heads'] = heads
>            kwargs['common'] = common
>            func(bundler, repo, source, bundlecaps=bundlecaps, b2caps=b2caps,
>                 **kwargs)
>
> when from the callee's perspective there's no difference. There would be
> a difference if kwargs was passed as a plain argument, not as **kwargs.

The difference is only semantical. it highly the fact heads and common 
should not be seen as special.

It will also help people that want to drop the keyword argument in front 
on bundlecaps and b2caps (such brave people)

Patch

diff --git a/mercurial/exchange.py b/mercurial/exchange.py
--- a/mercurial/exchange.py
+++ b/mercurial/exchange.py
@@ -1035,28 +1035,28 @@  def _getbundlechangegrouppart(bundler, r
         # build changegroup bundle here.
         cg = changegroup.getchangegroup(repo, source, heads=heads,
                                         common=common, bundlecaps=bundlecaps)
 
     if cg:
         bundler.newpart('b2x:changegroup', data=cg.getchunks())
 
 @getbundle2partsgenerator('listkeys')
-def _getbundlelistkeysparts(bundler, repo, source, heads=None, common=None,
-                           bundlecaps=None, b2caps=None, **kwargs):
+def _getbundlelistkeysparts(bundler, repo, source, bundlecaps=None,
+                            b2caps=None, **kwargs):
     """add parts containing listkeys namespaces to the requested bundle"""
     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)
 
 @getbundle2partsgenerator('obsmarkers')
-def _getbundleobsmarkerpart(bundler, repo, source, heads=None, common=None,
+def _getbundleobsmarkerpart(bundler, repo, source, heads=None,
                             bundlecaps=None, b2caps=None, **kwargs):
     """add an obsolescence markers part to the requested bundle"""
     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)