Patchwork [4,of,5] push: send highest changegroup format supported by both side

login
register
mail settings
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

Pierre-Yves David - Nov. 4, 2014, 2:20 p.m.
# 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.
Gregory Szorc - Nov. 4, 2014, 4:55 p.m.
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.
Pierre-Yves David - Nov. 4, 2014, 6:14 p.m.
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?
Pierre-Yves David - Nov. 4, 2014, 9:54 p.m.
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']