| Submitter | Pierre-Yves David |
|---|---|
| Date | Nov. 4, 2014, 2:20 p.m. |
| Message ID | <7407e3ea154921922956.1415110847@marginatus.alto.octopoid.net> |
| Download | mbox | patch |
| Permalink | /patch/6561/ |
| State | Accepted |
| Headers | show |
Comments
On 11/4/14 6:20 AM, Pierre-Yves David wrote: > # HG changeset patch > # User Pierre-Yves David <pierre-yves.david@fb.com> > # Date 1413573564 25200 > # Fri Oct 17 12:19:24 2014 -0700 > # Node ID 7407e3ea154921922956cbaf2e7ff0d89d48634c > # Parent 514f8b2f466377bca8eea987c67e33eb415586e9 > push: send highest changegroup format supported by both side > > When using bundle2, we find the common subset of supported changegroup-packers > and we pick the max of them. This allow to use generaldelta aware changegroups through > bundle2. I like what this series (and the already queued one before it) are trying to do (protocol negotiation). But I'm a bit concerned about using plain integers for protocol negotiation. Is this integer just encompassing the wire/bundle protocol? Or does it extend to more, such as encoding formats (classic vs generaldelta) and compression formats (raw, gzip, bz2, lz4, etc)? If it is just the former, +1. But if version N also implies things beyond the wire protocol (e.g. use of generaldelta), then I have strong objections to using simple integer comparison for ranking preferences, as I think there are too many combinations to express preferences in a shared numeric list. My apologies for not fully grasping the full plan here.
On 11/04/2014 04:55 PM, Gregory Szorc wrote: > On 11/4/14 6:20 AM, Pierre-Yves David wrote: >> # HG changeset patch >> # User Pierre-Yves David <pierre-yves.david@fb.com> >> # Date 1413573564 25200 >> # Fri Oct 17 12:19:24 2014 -0700 >> # Node ID 7407e3ea154921922956cbaf2e7ff0d89d48634c >> # Parent 514f8b2f466377bca8eea987c67e33eb415586e9 >> push: send highest changegroup format supported by both side >> >> When using bundle2, we find the common subset of supported >> changegroup-packers >> and we pick the max of them. This allow to use generaldelta aware >> changegroups through >> bundle2. > > I like what this series (and the already queued one before it) are > trying to do (protocol negotiation). But I'm a bit concerned about using > plain integers for protocol negotiation. > > Is this integer just encompassing the wire/bundle protocol? Or does it > extend to more, such as encoding formats (classic vs generaldelta) and > compression formats (raw, gzip, bz2, lz4, etc)? If it is just the > former, +1. But if version N also implies things beyond the wire > protocol (e.g. use of generaldelta), then I have strong objections to > using simple integer comparison for ranking preferences, as I think > there are too many combinations to express preferences in a shared > numeric list. It control the changegroup format, higher should be better ("01" is old plain changegroup. "02" support general delta (but do not necessarily do any reordering to take advantage of it. So "02" is trickly more powerfull than "01". The bundlecaps are also passed along for more sophisticated negociation. This version thing could maybe be handled by bundlecaps, but It feel like it is a major step that deserve its own things (basically correspond to a HG11 bundle format. (will think about it a bit, thanks for pointing) The compression is should be handled at the bundle2 level. Does this address your concern?
On 11/04/2014 06:14 PM, Pierre-Yves David wrote:
> Does this address your concern?
After an IRC discussion with Greg, I addressed his concern.
The new format "02" is just a super set of "01" is allow to convey the
same information that "01" but also support delta against other changesets.
This does not implies any extra general delta work from the server.
Server will just get data from disk as they are.
- If the server is not using general delta, all delta used in the bundle
will still be "classic" (against the previous chunk) but a field in the
changegroup will make it explicit.
- If the server is using general delta. The such delta will be directly
reused from disk to the "02" changegroup (except when the target is not
accessible from the generated changegroup)
In the same way, "03" is expected to be a superset of "02" that also
support both 20 bytes and 32 bytes hashes (for sha1 vs sha256).
Patch
diff --git a/mercurial/exchange.py b/mercurial/exchange.py --- a/mercurial/exchange.py +++ b/mercurial/exchange.py @@ -443,13 +443,27 @@ def _pushb2ctx(pushop, bundler): pushop.repo.prepushoutgoinghooks(pushop.repo, pushop.remote, pushop.outgoing) if not pushop.force: bundler.newpart('B2X:CHECK:HEADS', data=iter(pushop.remoteheads)) - cg = changegroup.getlocalchangegroupraw(pushop.repo, 'push', - pushop.outgoing) + b2caps = bundle2.bundle2caps(pushop.remote) + version = None + cgversions = b2caps.get('b2x:changegroup') + if cgversions is None: + cg = changegroup.getlocalchangegroupraw(pushop.repo, 'push', + pushop.outgoing) + else: + cgversions = [v for v in cgversions if v in changegroup.packermap] + if not cgversions: + raise ValueError(_('no common changegroup version')) + version = max(cgversions) + cg = changegroup.getlocalchangegroupraw(pushop.repo, 'push', + pushop.outgoing, + version=version) cgpart = bundler.newpart('B2X:CHANGEGROUP', data=cg) + if version is not None: + cgpart.addparam('version', version) def handlereply(op): """extract addchangegroup returns from server reply""" cgreplies = op.records.getreplies(cgpart.id) assert len(cgreplies['changegroup']) == 1 pushop.cgresult = cgreplies['changegroup'][0]['return']