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

login
register
mail settings
Submitter Siddharth Agarwal
Date April 5, 2017, 3:49 a.m.
Message ID <52572916e2ae57e92d22.1491364177@devvm028.frc2.facebook.com>
Download mbox | patch
Permalink /patch/19968/
State Changes Requested
Headers show

Comments

Siddharth Agarwal - April 5, 2017, 3:49 a.m.
# HG changeset patch
# User Siddharth Agarwal <sid0@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.
Pierre-Yves David - April 6, 2017, 12:50 p.m.
On 04/05/2017 05:49 AM, Siddharth Agarwal wrote:
> # HG changeset patch
> # User Siddharth Agarwal <sid0@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.

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

> +  >
>    > def uisetup(ui):
>    >     exchange.b2partsgenmapping['failpart'] = _pushbundle2failpart
>    >     exchange.b2partsgenorder.insert(0, 'failpart')
> +  >     extensions.wrapfunction(bundle2, '_errorparams', overrideerrorparams)
>    >
>    > EOF
>
> @@ -543,6 +554,22 @@ Doing the actual push: Abort error (mess
>    $ cat << EOF >> $HGRCPATH
>    > [failpush]
>    > reason = abort-long
> +  > legacyerrorparams = false
> +  > EOF
> +
> +  $ hg -R main push ssh://user@dummy/other -r e7ec4e813ba6
> +  pushing to ssh://user@dummy/other
> +  searching for changes
> +  remote: (a){8192} (re)
> +  remote: (don't panic)
> +  abort: push failed on remote
> +  [255]
> +
> +Abort error (simulate client Mercurial < 4.2 -- truncated message)
> +
> +  $ cat << EOF >> $HGRCPATH
> +  > [failpush]
> +  > legacyerrorparams = true
>    > EOF
>
>    $ hg -R main push ssh://user@dummy/other -r e7ec4e813ba6
> @@ -553,6 +580,25 @@ Doing the actual push: Abort error (mess
>    abort: push failed on remote
>    [255]
>
> +  $ cat << EOF >> $HGRCPATH
> +  > [failpush]
> +  > legacyerrorparams = false
> +  > EOF
> +
> +Abort error (simulate server Mercurial < 4.2)
> +
> +  $ cat << EOF >> $HGRCPATH
> +  > [failpush]
> +  > reason = abort-legacy
> +  > EOF
> +
> +  $ hg -R main push ssh://user@dummy/other -r e7ec4e813ba6
> +  pushing to ssh://user@dummy/other
> +  searching for changes
> +  remote: Abandon ship too!
> +  abort: push failed on remote
> +  [255]
> +
>  Doing the actual push: unknown mandatory parts
>
>    $ cat << EOF >> $HGRCPATH
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
Siddharth Agarwal - April 7, 2017, 7:55 p.m.
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@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.
Pierre-Yves David - April 7, 2017, 10:32 p.m.
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@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

Patch

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
+    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)
+  > 
   > def uisetup(ui):
   >     exchange.b2partsgenmapping['failpart'] = _pushbundle2failpart
   >     exchange.b2partsgenorder.insert(0, 'failpart')
+  >     extensions.wrapfunction(bundle2, '_errorparams', overrideerrorparams)
   > 
   > EOF
 
@@ -543,6 +554,22 @@  Doing the actual push: Abort error (mess
   $ cat << EOF >> $HGRCPATH
   > [failpush]
   > reason = abort-long
+  > legacyerrorparams = false
+  > EOF
+
+  $ hg -R main push ssh://user@dummy/other -r e7ec4e813ba6
+  pushing to ssh://user@dummy/other
+  searching for changes
+  remote: (a){8192} (re)
+  remote: (don't panic)
+  abort: push failed on remote
+  [255]
+
+Abort error (simulate client Mercurial < 4.2 -- truncated message)
+
+  $ cat << EOF >> $HGRCPATH
+  > [failpush]
+  > legacyerrorparams = true
   > EOF
 
   $ hg -R main push ssh://user@dummy/other -r e7ec4e813ba6
@@ -553,6 +580,25 @@  Doing the actual push: Abort error (mess
   abort: push failed on remote
   [255]
 
+  $ cat << EOF >> $HGRCPATH
+  > [failpush]
+  > legacyerrorparams = false
+  > EOF
+
+Abort error (simulate server Mercurial < 4.2)
+
+  $ cat << EOF >> $HGRCPATH
+  > [failpush]
+  > reason = abort-legacy
+  > EOF
+
+  $ hg -R main push ssh://user@dummy/other -r e7ec4e813ba6
+  pushing to ssh://user@dummy/other
+  searching for changes
+  remote: Abandon ship too!
+  abort: push failed on remote
+  [255]
+
 Doing the actual push: unknown mandatory parts
 
   $ cat << EOF >> $HGRCPATH