| 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
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.
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.
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.
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).
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? >
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).
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()) >