[PATCH 3 of 3 V2] bundle2: handle long error params on the unbundling side

[Siddharth Agarwal]

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.