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

Siddharth Agarwal sid at less-broken.com
Fri Apr 7 15:52:11 EDT 2017


On 4/6/17 05:50, Pierre-Yves David wrote:
> 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 ?


My view is that if we're doing this for abort anyway, we might as well 
do this for the other part types.

* unsupportedcontent is arbitrary so it can easily go beyond 255 when 
you concatenate the params together. A misbehaving client can easily 
crash the server that way.
* pushraced is less likely but there's still a non-zero chance that a 
confluence of events can cause this. I think the incremental amount of 
work required to support it is minimal, so we might as well do it in my 
opinion.


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


It'll probably lead to simpler code overall. I like it.

I would want to store the full message in the payload though, as we're 
truncating the message in params for older clients in a friendly-ish way.



More information about the Mercurial-devel mailing list