Patchwork [2,of,3,V2] bundle2: use bundleerrorparts for error parts with unbounded parameters

# HG changeset patch
# User Siddharth Agarwal <sid0@fb.com>
# Date 1491349293 25200
#      Tue Apr 04 16:41:33 2017 -0700
# Node ID 21c811d141254489398a83affa46e066bf2f6b94
# Parent  ad6196e3370572b9d0b436ab9d901f26884633f4
bundle2: use bundleerrorparts for error parts with unbounded parameters

Clients do not know how to read these error parts yet -- that's why they will
only be able to print out truncated messages. In the next diff we'll be able to
provide the full message.

On 04/05/2017 05:49 AM, Siddharth Agarwal wrote:
> # HG changeset patch
> # User Siddharth Agarwal <sid0@fb.com>
> # Date 1491349293 25200
> #      Tue Apr 04 16:41:33 2017 -0700
> # Node ID 21c811d141254489398a83affa46e066bf2f6b94
> # Parent  ad6196e3370572b9d0b436ab9d901f26884633f4
> bundle2: use bundleerrorparts for error parts with unbounded parameters
>
> Clients do not know how to read these error parts yet -- that's why they will
> only be able to print out truncated messages. In the next diff we'll be able to
> provide the full message.
>
> diff --git a/mercurial/wireproto.py b/mercurial/wireproto.py
> --- a/mercurial/wireproto.py
> +++ b/mercurial/wireproto.py
> @@ -1033,18 +1033,17 @@ def unbundle(repo, proto, heads):
>                  if exc.ret is not None:
>                      part.addparam('ret', exc.ret, mandatory=False)
>          except error.BundleValueError as exc:
> -            errpart = bundler.newpart('error:unsupportedcontent')
> +            errpart = bundler.newerrorpart('error:unsupportedcontent')
>              if exc.parttype is not None:
>                  errpart.addparam('parttype', exc.parttype)
>              if exc.params:
> -                errpart.addparam('params', '\0'.join(exc.params))
> +                errpart.addlongparam('params', '\0'.join(exc.params))
>          except error.Abort as exc:
> -            manargs = [('message', str(exc))]
> -            advargs = []
> +            part = bundler.newerrorpart('error:abort')
> +            part.addlongparam('message', str(exc))
>              if exc.hint is not None:
> -                advargs.append(('hint', exc.hint))
> -            bundler.addpart(bundle2.bundlepart('error:abort',
> -                                               manargs, advargs))
> +                part.addlongparam('hint', exc.hint, mandatory=False)
>          except error.PushRaced as exc:
> -            bundler.newpart('error:pushraced', [('message', str(exc))])
> +            part = bundler.newerrorpart('error:pushraced')
> +            part.addlongparam('message', str(exc))
>          return streamres(gen=bundler.getchunks())
> 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
> @@ -462,6 +462,8 @@ Setting up
>    >     part = None
>    >     if reason == 'abort':
>    >         bundler.newpart('test:abort')
> +  >     if reason == 'abort-long':
> +  >         bundler.newpart('test:abort-long')
>    >     if reason == 'unknown':
>    >         bundler.newpart('test:unknown')
>    >     if reason == 'race':
> @@ -472,6 +474,12 @@ Setting up
>    > def handleabort(op, part):
>    >     raise error.Abort('Abandon ship!', hint="don't panic")
>    >
> +  > @bundle2.parthandler("test:abort-long")
> +  > def handleabortlong(op, part):
> +  >     # Make sure error messages are more than 4k long to ensure they work
> +  >     # across payload chunks
> +  >     raise error.Abort('a' * 8192, hint="don't panic")

small note: This test seems to be checking the bundle2 format and API 
more than the target feature. This seems superfluous or at least wrongly 
located.

> +  >
>    > def uisetup(ui):
>    >     exchange.b2partsgenmapping['failpart'] = _pushbundle2failpart
>    >     exchange.b2partsgenorder.insert(0, 'failpart')
> @@ -530,6 +538,20 @@ Doing the actual push: Abort error
>    abort: push failed on remote
>    [255]
>
> +Doing the actual push: Abort error (message too long to fit in a param)
> +
> +  $ cat << EOF >> $HGRCPATH
> +  > [failpush]
> +  > reason = abort-long
> +  > EOF
> +
> +  $ hg -R main push ssh://user@dummy/other -r e7ec4e813ba6
> +  pushing to ssh://user@dummy/other
> +  searching for changes
> +  remote: (a){252}... (re)
> +  remote: (don't panic)
> +  abort: push failed on remote
> +  [255]
>
>  Doing the actual push: unknown mandatory parts
>
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>

On 4/6/17 05:50, Pierre-Yves David wrote:
> small note: This test seems to be checking the bundle2 format and API 
> more than the target feature. This seems superfluous or at least 
> wrongly located. 


Could you elaborate about what you mean? There's a wire transmission 
going on. This is an example of an abort that would have crashed the 
server earlier. Now it no longer does. How is it superfluous?

On 04/07/2017 09:28 PM, Siddharth Agarwal wrote:
> On 4/6/17 05:50, Pierre-Yves David wrote:
>> small note: This test seems to be checking the bundle2 format and API
>> more than the target feature. This seems superfluous or at least
>> wrongly located.
>
>
> Could you elaborate about what you mean? There's a wire transmission
> going on. This is an example of an abort that would have crashed the
> server earlier. Now it no longer does. How is it superfluous?

I'm referring to this comment:

>>> +  >     # Make sure error messages are more than 4k long to ensure they work
>>> +  >     # across payload chunks

The payload chunks is an internal details of the bundle2 protocol 
itself, it is already abstracted to the code handling a part. So you 
should not have to test it when testing your part handler.

If you need/want to add tests for payload above 4K, that would be a 
lower level tests that belong to `test-bundle2-format.t`

I hope this clarifies my previous comment.

The rest of this patch (testing cases that previous aborted) is fine and 
I agree we want that tested.

Cheers,