[PATCH 1 of 3 V2] bundle2: add separate handling for error part creation

[Pierre-Yves David]

On 04/05/2017 05:49 AM, Siddharth Agarwal wrote:
> # HG changeset patch
> # User Siddharth Agarwal <sid0 at fb.com>
> # Date 1491348154 25200
> #      Tue Apr 04 16:22:34 2017 -0700
> # Node ID ad6196e3370572b9d0b436ab9d901f26884633f4
> # Parent  04ec317b81280c189fcea33a05c8cbbac3c186b1
> bundle2: add separate handling for error part creation
>
> This will be used in upcoming diffs to handle error parts.
>
> See the included docstrings for why this is necessary.

> […]

> @@ -1026,6 +1041,54 @@ class bundlepart(object):
>          elif len(self.data):
>              yield self.data
>
> +class bundleerrorpart(bundlepart):
> +    """
> +    This is only required for the following error parts that are recognized
> +    by Mercurial versions prior to 4.2.
> +    - error:abort
> +    - error:pushraced
> +    - error:unsupportedcontent

While I understand the need for unbounded generic error:Abort, I not 
sure we should extend this to pushraced and unsupported content one. 
They have defined enough use case where limiting the message and hint to 
255 char seems reasonable (in the exception themselves).
Limiting the scope of this more complex creation and handling to 
error:Abort.

What do you think ?

> +
> +    In Mercurial prior to 4.2, error messages for these parts were stored as
> +    part parameters. Since parameter sizes can be 255 at most, this meant that
> +    error messages longer than 255 bytes would crash the Mercurial server.
> +
> +    To avoid this issue while still retaining backwards compatibility with older
> +    versions of Mercurial, this class of parts:
> +    * truncates long params to 255 bytes
> +    * adds such params to the payload instead.

I think we could get away with a simpler way to add the long message to 
the payload.

You current approach implies packing the parameters name and length 
using our own binary encoding into the payload. This imply tedious 
binary manipulation on both end.
Instead we could leverage more of the bundle2 format. In case of too 
long message or hint, we add a corresponding 'message_long' or 
'hint_long' advisory parameter to the part. This parameter contains a 
'start:end' range of bytes in the paylog containing the message. That 
way most of the binary encoding is handled by the native bundle2 
machinery and less work is needed on the receiving side.

What do you think ?

> +
> +    Any newer error parts should instead:
> +    - use newpart
> +    - take care not to set up any params with potentially unbounded length
> +    - send the unbounded param(s) (e.g. error message) as part of the
> +      payload
> +    """
> +    def __init__(self, *args, **kwargs):
> +        super(bundleerrorpart, self).__init__(*args, **kwargs)
> +        self._errordata = []
> +        self.addparam('longparams', 'payload', mandatory=False)
> +
> +    def addlongparam(self, name, value='', mandatory=True):
> +        vparam = value
> +        if len(value) > 255:
> +            # truncate value so that it doesn't crash
> +            vparam = value[:252] + '...'
> +        self.addparam(name, vparam, mandatory=mandatory)
> +        # encode the full error message as
> +        # (len(name), name, len(value), value)
> +        # mandatory is enforced by the param
> +        data = [struct.pack(_ferrorlongparamsize, len(name)), name,
> +                struct.pack(_ferrorlongparamsize, len(value)), value]
> +        self._errordata.append(''.join(data))
> +
> +    def getchunks(self, ui):
> +        # data = number of long params + all the params
> +        numparams = len(self._errordata)
> +        errordata = ([struct.pack(_ferrorlongparamsize, numparams)] +
> +                     self._errordata)
> +        self._data = ''.join(errordata)

Small nits: _data can be an iterator. Using iter(errordata) would skip 
the join.

> +        return super(bundleerrorpart, self).getchunks(ui)
>
>  flaginterrupt = -1

-- 
Pierre-Yves David