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

[Pierre-Yves David]

On 04/07/2017 09:55 PM, Siddharth Agarwal wrote:
> 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 just checked the definition and I've way off target here :-)

What I meant was: "I agree with this comment"

> 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.

Ha, I missed the fact it is a mixin. This mean we cannot use the '__' 
mangling[1], since it would makes it "inaccessible" to the class that 
needs it.

I see three option left:

1) clarify their docstring

2) move to the verbose __xx__ (eg: __unpack__)

3) change nothing (meh).

I think I would prefer to go for the doc update (1) first and see if 
this is enough or if the confusion arise again in the future.

[1]https://docs.python.org/2/tutorial/classes.html#private-variables-and-class-local-references

>
>>
>>> +    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.

If people introduces new error class who needs it. They will want to 
introduce the same testing as you do.

It is not too important. Do it the way you feel like doing it.

Cheers

-- 
Pierre-Yves David