Patchwork [2,of,2] changegroup: delete unused 'bundlecaps' argument

login
register
mail settings
Submitter via Mercurial-devel
Date May 3, 2017, 5:44 p.m.
Message ID <06058cbb9209df54bfa0.1493833489@martinvonz.svl.corp.google.com>
Download mbox | patch
Permalink /patch/20409/
State Accepted
Headers show

Comments

via Mercurial-devel - May 3, 2017, 5:44 p.m.
# HG changeset patch
# User Martin von Zweigbergk <martinvonz@google.com>
# Date 1493794030 25200
#      Tue May 02 23:47:10 2017 -0700
# Node ID 06058cbb9209df54bfa0aaf9f7d10b836a1e3ee9
# Parent  bf2a70b5c015ff0720571c4512d67eb53f52b85e
changegroup: delete unused 'bundlecaps' argument
via Mercurial-devel - May 3, 2017, 6:18 p.m.
On Wed, May 3, 2017 at 10:44 AM, Martin von Zweigbergk
<martinvonz@google.com> wrote:
> # HG changeset patch
> # User Martin von Zweigbergk <martinvonz@google.com>
> # Date 1493794030 25200
> #      Tue May 02 23:47:10 2017 -0700
> # Node ID 06058cbb9209df54bfa0aaf9f7d10b836a1e3ee9
> # Parent  bf2a70b5c015ff0720571c4512d67eb53f52b85e
> changegroup: delete unused 'bundlecaps' argument

This should have been tagged (API), which I hope can be added in flight.
Pierre-Yves David - May 4, 2017, 10:27 a.m.
On 05/03/2017 07:44 PM, Martin von Zweigbergk via Mercurial-devel wrote:
> # HG changeset patch
> # User Martin von Zweigbergk <martinvonz@google.com>
> # Date 1493794030 25200
> #      Tue May 02 23:47:10 2017 -0700
> # Node ID 06058cbb9209df54bfa0aaf9f7d10b836a1e3ee9
> # Parent  bf2a70b5c015ff0720571c4512d67eb53f52b85e
> changegroup: delete unused 'bundlecaps' argument

This series looks good to me.
Yuya Nishihara - May 5, 2017, 2:30 a.m.
On Thu, 4 May 2017 12:27:26 +0200, Pierre-Yves David wrote:
> On 05/03/2017 07:44 PM, Martin von Zweigbergk via Mercurial-devel wrote:
> > # HG changeset patch
> > # User Martin von Zweigbergk <martinvonz@google.com>
> > # Date 1493794030 25200
> > #      Tue May 02 23:47:10 2017 -0700
> > # Node ID 06058cbb9209df54bfa0aaf9f7d10b836a1e3ee9
> > # Parent  bf2a70b5c015ff0720571c4512d67eb53f52b85e
> > changegroup: delete unused 'bundlecaps' argument
> 
> This series looks good to me.

Queued per review, thanks.
Durham Goode - May 9, 2017, 7:30 p.m.
On 5/3/17 10:44 AM, Martin von Zweigbergk via Mercurial-devel wrote:
> # HG changeset patch
> # User Martin von Zweigbergk <martinvonz@google.com>
> # Date 1493794030 25200
> #      Tue May 02 23:47:10 2017 -0700
> # Node ID 06058cbb9209df54bfa0aaf9f7d10b836a1e3ee9
> # Parent  bf2a70b5c015ff0720571c4512d67eb53f52b85e
> changegroup: delete unused 'bundlecaps' argument

This was pretty heavily used by remotefilelog to tell the bundler to not 
include filelogs in the changegroup.  Unfortunately there aren't really 
any other ways of communicating from the exchange layer to the cgpacker 
layer.

If this was just a clean-up commit, could we roll it back? Otherwise I 
have to jump through a lot of hoops to make remotefilelog work again 
(potentially involving adding some sort of options that get passed from 
the exchange layer down through to changegroup creation, just like 
bundlecaps were before).
via Mercurial-devel - May 9, 2017, 7:39 p.m.
On Tue, May 9, 2017, 12:30 Durham Goode <durham@fb.com> wrote:

> On 5/3/17 10:44 AM, Martin von Zweigbergk via Mercurial-devel wrote:
> > # HG changeset patch
> > # User Martin von Zweigbergk <martinvonz@google.com>
> > # Date 1493794030 25200
> > #      Tue May 02 23:47:10 2017 -0700
> > # Node ID 06058cbb9209df54bfa0aaf9f7d10b836a1e3ee9
> > # Parent  bf2a70b5c015ff0720571c4512d67eb53f52b85e
> > changegroup: delete unused 'bundlecaps' argument
>
> This was pretty heavily used by remotefilelog to tell the bundler to not
> include filelogs in the changegroup.  Unfortunately there aren't really
> any other ways of communicating from the exchange layer to the cgpacker
> layer.
>
> If this was just a clean-up commit, could we roll it back? Otherwise I
> have to jump through a lot of hoops to make remotefilelog work again
> (potentially involving adding some sort of options that get passed from
> the exchange layer down through to changegroup creation, just like
> bundlecaps were before).
>

Fine with me. It was just a cleanup. Do you think the bundlecaps parameter
is the right solution for your need long-term?

>
Durham Goode - May 9, 2017, 7:51 p.m.
On 5/9/17 12:39 PM, Martin von Zweigbergk wrote:
>
>
> On Tue, May 9, 2017, 12:30 Durham Goode <durham@fb.com
> <mailto:durham@fb.com>> wrote:
>
>     On 5/3/17 10:44 AM, Martin von Zweigbergk via Mercurial-devel wrote:
>     > # HG changeset patch
>     > # User Martin von Zweigbergk <martinvonz@google.com
>     <mailto:martinvonz@google.com>>
>     > # Date 1493794030 25200
>     > #      Tue May 02 23:47:10 2017 -0700
>     > # Node ID 06058cbb9209df54bfa0aaf9f7d10b836a1e3ee9
>     > # Parent  bf2a70b5c015ff0720571c4512d67eb53f52b85e
>     > changegroup: delete unused 'bundlecaps' argument
>
>     This was pretty heavily used by remotefilelog to tell the bundler to not
>     include filelogs in the changegroup.  Unfortunately there aren't really
>     any other ways of communicating from the exchange layer to the cgpacker
>     layer.
>
>     If this was just a clean-up commit, could we roll it back? Otherwise I
>     have to jump through a lot of hoops to make remotefilelog work again
>     (potentially involving adding some sort of options that get passed from
>     the exchange layer down through to changegroup creation, just like
>     bundlecaps were before).
>
>
> Fine with me. It was just a cleanup. Do you think the bundlecaps
> parameter is the right solution for your need long-term?

Nah, it's not a good long-term solution. The bundlecaps parameter was a 
hacky solution from the beginning.

Ideally we would have changegroup creation happen in it's own function 
at the exchange layer. That way an extension could hook in at the 
exchange layer (where they have access to bundle2 arguments, etc) and 
customize what cgpacker is used. The problem right now is that exchange 
layer never sees the cgpacker.  It calls 
changegroup.getlocalchangegroupraw(...) which returns a generator, at 
which point it's too late to modify the cgpacker.

So if exchange code could be written like:

   cg = createchangegroup(repo,..., bundle2args)
   part = bundler.newpart('changegroup', data=cg.generate()

then I as an extension could wrap the cg appropriately based on the 
bundle2args we see.

But I'd rather not solve that right now while I'm working on 
treemanifest and hidden commit stuff.

Note: the proposal above only works for bundle2. There is no alternative 
solution for bundle1, so if we drop bundlecaps entirely, remotefilelog 
has to drop support for bundle1. We don't use bundle1, but I don't know 
if other consumers might.


I'll send a backout commit for now (it requires some merging with recent 
changes, so I'm happy to do the work there).
via Mercurial-devel - May 9, 2017, 7:56 p.m.
On Tue, May 9, 2017 at 12:51 PM, Durham Goode <durham@fb.com> wrote:
>
>
> On 5/9/17 12:39 PM, Martin von Zweigbergk wrote:
>>
>>
>>
>> On Tue, May 9, 2017, 12:30 Durham Goode <durham@fb.com
>> <mailto:durham@fb.com>> wrote:
>>
>>     On 5/3/17 10:44 AM, Martin von Zweigbergk via Mercurial-devel wrote:
>>     > # HG changeset patch
>>     > # User Martin von Zweigbergk <martinvonz@google.com
>>     <mailto:martinvonz@google.com>>
>>     > # Date 1493794030 25200
>>     > #      Tue May 02 23:47:10 2017 -0700
>>     > # Node ID 06058cbb9209df54bfa0aaf9f7d10b836a1e3ee9
>>     > # Parent  bf2a70b5c015ff0720571c4512d67eb53f52b85e
>>     > changegroup: delete unused 'bundlecaps' argument
>>
>>     This was pretty heavily used by remotefilelog to tell the bundler to
>> not
>>     include filelogs in the changegroup. Unfortunately there aren't
>> really
>>     any other ways of communicating from the exchange layer to the
>> cgpacker
>>     layer.
>>
>>     If this was just a clean-up commit, could we roll it back? Otherwise I
>>     have to jump through a lot of hoops to make remotefilelog work again
>>     (potentially involving adding some sort of options that get passed
>> from
>>     the exchange layer down through to changegroup creation, just like
>>     bundlecaps were before).
>>
>>
>> Fine with me. It was just a cleanup. Do you think the bundlecaps
>> parameter is the right solution for your need long-term?
>
>
> Nah, it's not a good long-term solution. The bundlecaps parameter was a
> hacky solution from the beginning.
>
> Ideally we would have changegroup creation happen in it's own function at
> the exchange layer. That way an extension could hook in at the exchange
> layer (where they have access to bundle2 arguments, etc) and customize what
> cgpacker is used. The problem right now is that exchange layer never sees
> the cgpacker.  It calls changegroup.getlocalchangegroupraw(...) which
> returns a generator, at which point it's too late to modify the cgpacker.
>
> So if exchange code could be written like:
>
>   cg = createchangegroup(repo,..., bundle2args)
>   part = bundler.newpart('changegroup', data=cg.generate()
>
> then I as an extension could wrap the cg appropriately based on the
> bundle2args we see.
>
> But I'd rather not solve that right now while I'm working on treemanifest
> and hidden commit stuff.

Sure, that's fine with me. I was just trying to see if we wanted the
bundlespecs to stay "forever" or if there was a better long-term
solution.

>
> Note: the proposal above only works for bundle2. There is no alternative
> solution for bundle1, so if we drop bundlecaps entirely, remotefilelog has
> to drop support for bundle1. We don't use bundle1, but I don't know if other
> consumers might.
>
>
> I'll send a backout commit for now (it requires some merging with recent
> changes, so I'm happy to do the work there).

Thanks. Just FYI, I'm in meetings much of the day.

Patch

diff --git a/mercurial/changegroup.py b/mercurial/changegroup.py
--- a/mercurial/changegroup.py
+++ b/mercurial/changegroup.py
@@ -502,16 +502,9 @@ 
 class cg1packer(object):
     deltaheader = _CHANGEGROUPV1_DELTA_HEADER
     version = '01'
-    def __init__(self, repo, bundlecaps=None):
+    def __init__(self, repo):
         """Given a source repo, construct a bundler.
-
-        bundlecaps is optional and can be used to specify the set of
-        capabilities which can be used to build the bundle.
         """
-        # Set of capabilities we can use to build the bundle.
-        if bundlecaps is None:
-            bundlecaps = set()
-        self._bundlecaps = bundlecaps
         # experimental config: bundle.reorder
         reorder = repo.ui.config('bundle', 'reorder', 'auto')
         if reorder == 'auto':
@@ -815,8 +808,8 @@ 
     version = '02'
     deltaheader = _CHANGEGROUPV2_DELTA_HEADER
 
-    def __init__(self, repo, bundlecaps=None):
-        super(cg2packer, self).__init__(repo, bundlecaps)
+    def __init__(self, repo):
+        super(cg2packer, self).__init__(repo)
         if self._reorder is None:
             # Since generaldelta is directly supported by cg2, reordering
             # generally doesn't help, so we disable it by default (treating
@@ -910,9 +903,9 @@ 
     assert versions
     return min(versions)
 
-def getbundler(version, repo, bundlecaps=None):
+def getbundler(version, repo):
     assert version in supportedoutgoingversions(repo)
-    return _packermap[version][0](repo, bundlecaps)
+    return _packermap[version][0](repo)
 
 def getunbundler(version, fh, alg, extras=None):
     return _packermap[version][1](fh, alg, extras=extras)
@@ -963,30 +956,27 @@ 
     bundler = getbundler(version, repo)
     return getsubset(repo, outgoing, bundler, source)
 
-def getlocalchangegroupraw(repo, source, outgoing, bundlecaps=None,
-                           version='01'):
+def getlocalchangegroupraw(repo, source, outgoing, version='01'):
     """Like getbundle, but taking a discovery.outgoing as an argument.
 
     This is only implemented for local repos and reuses potentially
     precomputed sets in outgoing. Returns a raw changegroup generator."""
     if not outgoing.missing:
         return None
-    bundler = getbundler(version, repo, bundlecaps)
+    bundler = getbundler(version, repo)
     return getsubsetraw(repo, outgoing, bundler, source)
 
-def getlocalchangegroup(repo, source, outgoing, bundlecaps=None,
-                        version='01'):
+def getlocalchangegroup(repo, source, outgoing, version='01'):
     """Like getbundle, but taking a discovery.outgoing as an argument.
 
     This is only implemented for local repos and reuses potentially
     precomputed sets in outgoing."""
     if not outgoing.missing:
         return None
-    bundler = getbundler(version, repo, bundlecaps)
+    bundler = getbundler(version, repo)
     return getsubset(repo, outgoing, bundler, source)
 
-def getchangegroup(repo, source, outgoing, bundlecaps=None,
-                   version='01'):
+def getchangegroup(repo, source, outgoing, version='01'):
     """Like changegroupsubset, but returns the set difference between the
     ancestors of heads and the ancestors common.
 
@@ -995,8 +985,7 @@ 
     The nodes in common might not all be known locally due to the way the
     current discovery protocol works.
     """
-    return getlocalchangegroup(repo, source, outgoing, bundlecaps=bundlecaps,
-                               version=version)
+    return getlocalchangegroup(repo, source, outgoing, version=version)
 
 def changegroup(repo, basenodes, source):
     # to avoid a race we use changegroupsubset() (issue1320)
diff --git a/mercurial/commands.py b/mercurial/commands.py
--- a/mercurial/commands.py
+++ b/mercurial/commands.py
@@ -1339,8 +1339,6 @@ 
         base = ['null']
     else:
         base = scmutil.revrange(repo, opts.get('base'))
-    # TODO: get desired bundlecaps from command line.
-    bundlecaps = None
     if cgversion not in changegroup.supportedoutgoingversions(repo):
         raise error.Abort(_("repository does not support bundle version %s") %
                           cgversion)
@@ -1353,7 +1351,6 @@ 
         heads = revs and map(repo.lookup, revs) or None
         outgoing = discovery.outgoing(repo, common, heads)
         cg = changegroup.getchangegroup(repo, 'bundle', outgoing,
-                                        bundlecaps=bundlecaps,
                                         version=cgversion)
         outgoing = None
     else:
@@ -1367,7 +1364,7 @@ 
                                                 force=opts.get('force'),
                                                 portable=True)
         cg = changegroup.getlocalchangegroup(repo, 'bundle', outgoing,
-                                                bundlecaps, version=cgversion)
+                                             version=cgversion)
     if not cg:
         scmutil.nochangesfound(ui, repo, outgoing and outgoing.excluded)
         return 1
diff --git a/mercurial/exchange.py b/mercurial/exchange.py
--- a/mercurial/exchange.py
+++ b/mercurial/exchange.py
@@ -938,22 +938,19 @@ 
     pushop.repo.prepushoutgoinghooks(pushop)
     outgoing = pushop.outgoing
     unbundle = pushop.remote.capable('unbundle')
-    # TODO: get bundlecaps from remote
-    bundlecaps = None
     # create a changegroup from local
     if pushop.revs is None and not (outgoing.excluded
                             or pushop.repo.changelog.filteredrevs):
         # push everything,
         # use the fast path, no race possible on push
-        bundler = changegroup.cg1packer(pushop.repo, bundlecaps)
+        bundler = changegroup.cg1packer(pushop.repo)
         cg = changegroup.getsubset(pushop.repo,
                                    outgoing,
                                    bundler,
                                    'push',
                                    fastpath=True)
     else:
-        cg = changegroup.getlocalchangegroup(pushop.repo, 'push', outgoing,
-                                        bundlecaps)
+        cg = changegroup.getlocalchangegroup(pushop.repo, 'push', outgoing)
 
     # apply changegroup to remote
     if unbundle:
@@ -1578,7 +1575,7 @@ 
             raise ValueError(_('unsupported getbundle arguments: %s')
                              % ', '.join(sorted(kwargs.keys())))
         outgoing = _computeoutgoing(repo, heads, common)
-        bundler = changegroup.getbundler('01', repo, bundlecaps)
+        bundler = changegroup.getbundler('01', repo)
         return changegroup.getsubsetraw(repo, outgoing, bundler, source)
 
     # bundle20 case
@@ -1616,7 +1613,6 @@ 
             version = max(cgversions)
         outgoing = _computeoutgoing(repo, heads, common)
         cg = changegroup.getlocalchangegroupraw(repo, source, outgoing,
-                                                bundlecaps=bundlecaps,
                                                 version=version)
 
     if cg:
diff --git a/tests/test-bundle2-format.t b/tests/test-bundle2-format.t
--- a/tests/test-bundle2-format.t
+++ b/tests/test-bundle2-format.t
@@ -113,7 +113,7 @@ 
   >             headmissing = [c.node() for c in repo.set('heads(%ld)', revs)]
   >             headcommon  = [c.node() for c in repo.set('parents(%ld) - %ld', revs, revs)]
   >             outgoing = discovery.outgoing(repo, headcommon, headmissing)
-  >             cg = changegroup.getlocalchangegroup(repo, 'test:bundle2', outgoing, None)
+  >             cg = changegroup.getlocalchangegroup(repo, 'test:bundle2', outgoing)
   >             bundler.newpart('changegroup', data=cg.getchunks(),
   >                             mandatory=False)
   > 
diff --git a/tests/test-bundle2-multiple-changegroups.t b/tests/test-bundle2-multiple-changegroups.t
--- a/tests/test-bundle2-multiple-changegroups.t
+++ b/tests/test-bundle2-multiple-changegroups.t
@@ -13,13 +13,11 @@ 
   >     # in 'heads' as intermediate heads for the first changegroup.
   >     intermediates = [repo[r].p1().node() for r in heads]
   >     outgoing = discovery.outgoing(repo, common, intermediates)
-  >     cg = changegroup.getchangegroup(repo, source, outgoing,
-  >                                     bundlecaps=bundlecaps)
+  >     cg = changegroup.getchangegroup(repo, source, outgoing)
   >     bundler.newpart('output', data='changegroup1')
   >     bundler.newpart('changegroup', data=cg.getchunks())
   >     outgoing = discovery.outgoing(repo, common + intermediates, heads)
-  >     cg = changegroup.getchangegroup(repo, source, outgoing,
-  >                                     bundlecaps=bundlecaps)
+  >     cg = changegroup.getchangegroup(repo, source, outgoing)
   >     bundler.newpart('output', data='changegroup2')
   >     bundler.newpart('changegroup', data=cg.getchunks())
   >