[PATCH 3 of 3 V2] bundle2: handle long error params on the unbundling side
Siddharth Agarwal
sid0 at fb.com
Fri Apr 7 19:55:43 UTC 2017
On 4/6/17 05:50, Pierre-Yves David wrote:
>
>
> On 04/05/2017 05:49 AM, Siddharth Agarwal wrote:
>> # HG changeset patch
>> # User Siddharth Agarwal <sid0 at fb.com>
>> # Date 1491349317 25200
>> # Tue Apr 04 16:41:57 2017 -0700
>> # Node ID 52572916e2ae57e92d22d718e469a7c0928e8f5e
>> # Parent 21c811d141254489398a83affa46e066bf2f6b94
>> bundle2: handle long error params on the unbundling side
>>
>> As the tests establish, the unbundling side can now present full
>> parameters.
>>
>> We retain tests to simulate an old client, and add tests to simulate
>> an old
>> server talking to a new client.
>>
>> diff --git a/mercurial/bundle2.py b/mercurial/bundle2.py
>> --- a/mercurial/bundle2.py
>> +++ b/mercurial/bundle2.py
>> @@ -1556,11 +1556,37 @@ def handlereplycaps(op, inpart):
>> class AbortFromPart(error.Abort):
>> """Sub-class of Abort that denotes an error from a bundle2 part."""
>>
>> +def _errorparams(inpart):
>> + """Read error parameters from the payload if so specified.
>> +
>> + This should only be used for errors generated via
>> bundleerrorpart."""
>> + params = inpart.params
>> + if params.get('longparams') != 'payload':
>> + return params
>> + # avoid modifying the params dict in inpart
>> + params = params.copy()
>> + # don't use inpart._unpack/_readexact because they read the
>> underlying
>> + # stream (with payload chunk sizes etc) -- instead read from the
>> + # higher-level stream
>
> I double back this comment. They are internal method for the bundle2
> machinery. Maybe we should update their docstring to make this
> clearer? maybe even doublescore (__ prefix) them.
Could you clarify what you mean by "double back"? I would be in favor of
__ prefixing them. Only consideration would be that they're on
unpackermixin so everything that uses it would want to switch to the __
methods.
>
>> + lensize = struct.calcsize(_ferrorlongparamsize)
>> + def readsize():
>> + return struct.unpack(_ferrorlongparamsize,
>> + changegroup.readexactly(inpart,
>> lensize))[0]
>> +
>> + numparams = readsize()
>> + for _param in xrange(numparams):
>> + namesize = readsize()
>> + name = changegroup.readexactly(inpart, namesize)
>> + valuesize = readsize()
>> + value = changegroup.readexactly(inpart, valuesize)
>> + params[name] = value
>> + return params
>> +
>> @parthandler('error:abort', ('message', 'hint'))
>> def handleerrorabort(op, inpart):
>> """Used to transmit abort error over the wire"""
>> - raise AbortFromPart(inpart.params['message'],
>> - hint=inpart.params.get('hint'))
>> + params = _errorparams(inpart)
>> + raise AbortFromPart(params['message'], hint=params.get('hint'))
>>
>> @parthandler('error:pushkey', ('namespace', 'key', 'new', 'old', 'ret',
>> 'in-reply-to'))
>> @@ -1576,11 +1602,12 @@ def handleerrorpushkey(op, inpart):
>> @parthandler('error:unsupportedcontent', ('parttype', 'params'))
>> def handleerrorunsupportedcontent(op, inpart):
>> """Used to transmit unknown content error over the wire"""
>> + params = _errorparams(inpart)
>> kwargs = {}
>> - parttype = inpart.params.get('parttype')
>> + parttype = params.get('parttype')
>> if parttype is not None:
>> kwargs['parttype'] = parttype
>> - params = inpart.params.get('params')
>> + params = params.get('params')
>> if params is not None:
>> kwargs['params'] = params.split('\0')
>>
>> @@ -1589,7 +1616,8 @@ def handleerrorunsupportedcontent(op, in
>> @parthandler('error:pushraced', ('message',))
>> def handleerrorpushraced(op, inpart):
>> """Used to transmit push race error over the wire"""
>> - raise error.ResponseError(_('push failed:'),
>> inpart.params['message'])
>> + params = _errorparams(inpart)
>> + raise error.ResponseError(_('push failed:'), params['message'])
>>
>> @parthandler('listkeys', ('namespace',))
>> def handlelistkeys(op, inpart):
>> diff --git a/tests/test-bundle2-exchange.t
>> b/tests/test-bundle2-exchange.t
>> --- a/tests/test-bundle2-exchange.t
>> +++ b/tests/test-bundle2-exchange.t
>> @@ -464,6 +464,10 @@ Setting up
>> > bundler.newpart('test:abort')
>> > if reason == 'abort-long':
>> > bundler.newpart('test:abort-long')
>> + > if reason == 'abort-legacy':
>> + > # Use bundler.newpart rather than bundler.newerrorpart
>> to create this part.
>> + > # This simulates a Mercurial server < 4.2.
>> + > bundler.newpart('error:abort', [('message', 'Abandon
>> ship too!')])
>> > if reason == 'unknown':
>> > bundler.newpart('test:unknown')
>> > if reason == 'race':
>> @@ -480,9 +484,16 @@ Setting up
>> > # across payload chunks
>> > raise error.Abort('a' * 8192, hint="don't panic")
>> >
>> + > def overrideerrorparams(orig, inpart):
>> + > # simulate Mercurial < 4.2 if this config is set
>> + > if inpart.ui.configbool('failpush', 'legacyerrorparams'):
>> + > return inpart.params
>> + > return orig(inpart)
>
> note: they are a devel.legacy group of configuration intended at
> providing "downgrade" swith for testing. Maybe you should add
> something there for this case.
I don't know, I'm a bit torn. Doing it this way keeps core Mercurial
simpler. I don't really know of a use case where we'd want to fall back
to old Mercurial behavior here.
More information about the Mercurial-devel
mailing list