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

login
register
mail settings
Submitter Siddharth Agarwal
Date April 5, 2017, 3:49 a.m.
Message ID <21c811d141254489398a.1491364176@devvm028.frc2.facebook.com>
Download mbox | patch
Permalink /patch/19969/
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 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.
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 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
>
Siddharth Agarwal - April 7, 2017, 7:28 p.m.
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?
Pierre-Yves David - April 7, 2017, 10:20 p.m.
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,

Patch

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")
+  > 
   > 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