Patchwork dispatch: unify handling of None returned by a command function

login
register
mail settings
Submitter Yuya Nishihara
Date May 12, 2018, 1:09 p.m.
Message ID <f5d41abf680b6830b21d.1526130541@mimosa>
Download mbox | patch
Permalink /patch/31548/
State Accepted
Headers show

Comments

Yuya Nishihara - May 12, 2018, 1:09 p.m.
# HG changeset patch
# User Yuya Nishihara <yuya@tcha.org>
# Date 1526106789 -32400
#      Sat May 12 15:33:09 2018 +0900
# Node ID f5d41abf680b6830b21de4aca1c81532eda36d90
# Parent  e9c5888025293fc06bc7865e54a11de6b17d12bc
dispatch: unify handling of None returned by a command function

A command function may return None in place of 0 just for convenience, but
dispatch() doesn't need to inherit that property. This patch makes it be
friendly to callers.
Gregory Szorc - May 12, 2018, 6:16 p.m.
On Sat, May 12, 2018 at 6:09 AM, Yuya Nishihara <yuya@tcha.org> wrote:

> # HG changeset patch
> # User Yuya Nishihara <yuya@tcha.org>
> # Date 1526106789 -32400
> #      Sat May 12 15:33:09 2018 +0900
> # Node ID f5d41abf680b6830b21de4aca1c81532eda36d90
> # Parent  e9c5888025293fc06bc7865e54a11de6b17d12bc
> dispatch: unify handling of None returned by a command function
>

Queued, thanks. I'll rebase my other series.


>
> A command function may return None in place of 0 just for convenience, but
> dispatch() doesn't need to inherit that property. This patch makes it be
> friendly to callers.
>
> diff --git a/mercurial/commandserver.py b/mercurial/commandserver.py
> --- a/mercurial/commandserver.py
> +++ b/mercurial/commandserver.py
> @@ -256,7 +256,7 @@ class server(object):
>                                 self.cout, self.cerr)
>
>          try:
> -            ret = (dispatch.dispatch(req) or 0) & 255 # might return None
> +            ret = dispatch.dispatch(req) & 255
>              self.cresult.write(struct.pack('>i', int(ret)))
>          finally:
>              # restore old cwd
> diff --git a/mercurial/dispatch.py b/mercurial/dispatch.py
> --- a/mercurial/dispatch.py
> +++ b/mercurial/dispatch.py
> @@ -87,7 +87,7 @@ def run():
>      req = request(pycompat.sysargv[1:])
>      err = None
>      try:
> -        status = dispatch(req) or 0
> +        status = dispatch(req)
>      except error.StdioError as e:
>          err = e
>          status = -1
> @@ -175,7 +175,7 @@ def _formatargs(args):
>      return ' '.join(procutil.shellquote(a) for a in args)
>
>  def dispatch(req):
> -    "run the command specified in req.args"
> +    """run the command specified in req.args; returns an integer status
> code"""
>      if req.ferr:
>          ferr = req.ferr
>      elif req.ui:
> @@ -208,9 +208,9 @@ def dispatch(req):
>
>      msg = _formatargs(req.args)
>      starttime = util.timer()
> -    ret = None
> +    ret = -1
>      try:
> -        ret = _runcatch(req)
> +        ret = _runcatch(req) or 0
>      except error.ProgrammingError as inst:
>          req.ui.warn(_('** ProgrammingError: %s\n') % inst)
>          if inst.hint:
> diff --git a/tests/test-blackbox.t b/tests/test-blackbox.t
> --- a/tests/test-blackbox.t
> +++ b/tests/test-blackbox.t
> @@ -206,7 +206,7 @@ log rotation
>    committing changelog
>    updating the branch cache
>    committed changeset 0:0e46349438790c460c5c9f7546bfcd39b267bbd2
> -  result: None
> +  result: 0
>    running: --debug commit -m commit2 -d 2000-01-02 foo
>    committing files:
>    foo
> @@ -214,7 +214,7 @@ log rotation
>    committing changelog
>    updating the branch cache
>    committed changeset 1:45589e459b2edfbf3dbde7e01f611d2c1e7453d7
> -  result: None
> +  result: 0
>    running: --debug log -r 0
>    changeset:   0:0e46349438790c460c5c9f7546bfcd39b267bbd2
>    phase:       draft
> @@ -229,7 +229,7 @@ log rotation
>    commit1
>
>
> -  result: None
> +  result: 0
>    running: --debug log -r tip
>    changeset:   1:45589e459b2edfbf3dbde7e01f611d2c1e7453d7
>    tag:         tip
> @@ -245,7 +245,7 @@ log rotation
>    commit2
>
>
> -  result: None
> +  result: 0
>    $ hg blackbox
>    1970/01/01 00:00:00 bob @45589e459b2edfbf3dbde7e01f611d2c1e7453d7
> (5000)> updating the branch cache
>    1970/01/01 00:00:00 bob @45589e459b2edfbf3dbde7e01f611d2c1e7453d7
> (5000)> updated served branch cache in * seconds (glob)
> diff --git a/tests/test-dispatch.py.out b/tests/test-dispatch.py.out
> --- a/tests/test-dispatch.py.out
> +++ b/tests/test-dispatch.py.out
> @@ -1,18 +1,18 @@
>  running: init test1
> -result: None
> +result: 0
>  running: add foo
>  result: 0
>  running: commit -m commit1 -d 2000-01-01 foo
> -result: None
> +result: 0
>  running: commit -m commit2 -d 2000-01-02 foo
> -result: None
> +result: 0
>  running: log -r 0
>  changeset:   0:0e4634943879
>  user:        test
>  date:        Sat Jan 01 00:00:00 2000 +0000
>  summary:     commit1
>
> -result: None
> +result: 0
>  running: log -r tip
>  changeset:   1:45589e459b2e
>  tag:         tip
> @@ -20,4 +20,4 @@ user:        test
>  date:        Sun Jan 02 00:00:00 2000 +0000
>  summary:     commit2
>
> -result: None
> +result: 0
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
via Mercurial-devel - May 15, 2018, 4:50 p.m.
On Sat, May 12, 2018 at 6:09 AM Yuya Nishihara <yuya@tcha.org> wrote:

> # HG changeset patch
> # User Yuya Nishihara <yuya@tcha.org>
> # Date 1526106789 -32400
> #      Sat May 12 15:33:09 2018 +0900
> # Node ID f5d41abf680b6830b21de4aca1c81532eda36d90
> # Parent  e9c5888025293fc06bc7865e54a11de6b17d12bc
> dispatch: unify handling of None returned by a command function
>
> A command function may return None in place of 0 just for convenience, but
> dispatch() doesn't need to inherit that property. This patch makes it be
> friendly to callers.
>
> diff --git a/mercurial/dispatch.py b/mercurial/dispatch.py
> --- a/mercurial/dispatch.py
> +++ b/mercurial/dispatch.py
> @@ -208,9 +208,9 @@ def dispatch(req):
>
>      msg = _formatargs(req.args)
>      starttime = util.timer()
> -    ret = None
> +    ret = -1
>

I think this means that the "ret or 0" on line 242 will no longer ever
evaluate to 0. That code is about blackbox logs, so I think the effect is
that if some command raises an unhandled exception (maybe assertions, for
example), it will instead say "<command> exited -1 after <number> seconds".
That seems like an improvement, since the command failed. However, the
actual error code in this case seems to be 1, so we should probably fix
that.
Yuya Nishihara - May 16, 2018, 11:09 a.m.
On Tue, 15 May 2018 09:50:36 -0700, Martin von Zweigbergk wrote:
> On Sat, May 12, 2018 at 6:09 AM Yuya Nishihara <yuya@tcha.org> wrote:
> 
> > # HG changeset patch
> > # User Yuya Nishihara <yuya@tcha.org>
> > # Date 1526106789 -32400
> > #      Sat May 12 15:33:09 2018 +0900
> > # Node ID f5d41abf680b6830b21de4aca1c81532eda36d90
> > # Parent  e9c5888025293fc06bc7865e54a11de6b17d12bc
> > dispatch: unify handling of None returned by a command function
> >
> > A command function may return None in place of 0 just for convenience, but
> > dispatch() doesn't need to inherit that property. This patch makes it be
> > friendly to callers.
> >
> > diff --git a/mercurial/dispatch.py b/mercurial/dispatch.py
> > --- a/mercurial/dispatch.py
> > +++ b/mercurial/dispatch.py
> > @@ -208,9 +208,9 @@ def dispatch(req):
> >
> >      msg = _formatargs(req.args)
> >      starttime = util.timer()
> > -    ret = None
> > +    ret = -1
> >
> 
> I think this means that the "ret or 0" on line 242 will no longer ever
> evaluate to 0. That code is about blackbox logs, so I think the effect is
> that if some command raises an unhandled exception (maybe assertions, for
> example), it will instead say "<command> exited -1 after <number> seconds".
> That seems like an improvement, since the command failed. However, the
> actual error code in this case seems to be 1, so we should probably fix
> that.

Good catch. I'll send a patch shortly.

Patch

diff --git a/mercurial/commandserver.py b/mercurial/commandserver.py
--- a/mercurial/commandserver.py
+++ b/mercurial/commandserver.py
@@ -256,7 +256,7 @@  class server(object):
                                self.cout, self.cerr)
 
         try:
-            ret = (dispatch.dispatch(req) or 0) & 255 # might return None
+            ret = dispatch.dispatch(req) & 255
             self.cresult.write(struct.pack('>i', int(ret)))
         finally:
             # restore old cwd
diff --git a/mercurial/dispatch.py b/mercurial/dispatch.py
--- a/mercurial/dispatch.py
+++ b/mercurial/dispatch.py
@@ -87,7 +87,7 @@  def run():
     req = request(pycompat.sysargv[1:])
     err = None
     try:
-        status = dispatch(req) or 0
+        status = dispatch(req)
     except error.StdioError as e:
         err = e
         status = -1
@@ -175,7 +175,7 @@  def _formatargs(args):
     return ' '.join(procutil.shellquote(a) for a in args)
 
 def dispatch(req):
-    "run the command specified in req.args"
+    """run the command specified in req.args; returns an integer status code"""
     if req.ferr:
         ferr = req.ferr
     elif req.ui:
@@ -208,9 +208,9 @@  def dispatch(req):
 
     msg = _formatargs(req.args)
     starttime = util.timer()
-    ret = None
+    ret = -1
     try:
-        ret = _runcatch(req)
+        ret = _runcatch(req) or 0
     except error.ProgrammingError as inst:
         req.ui.warn(_('** ProgrammingError: %s\n') % inst)
         if inst.hint:
diff --git a/tests/test-blackbox.t b/tests/test-blackbox.t
--- a/tests/test-blackbox.t
+++ b/tests/test-blackbox.t
@@ -206,7 +206,7 @@  log rotation
   committing changelog
   updating the branch cache
   committed changeset 0:0e46349438790c460c5c9f7546bfcd39b267bbd2
-  result: None
+  result: 0
   running: --debug commit -m commit2 -d 2000-01-02 foo
   committing files:
   foo
@@ -214,7 +214,7 @@  log rotation
   committing changelog
   updating the branch cache
   committed changeset 1:45589e459b2edfbf3dbde7e01f611d2c1e7453d7
-  result: None
+  result: 0
   running: --debug log -r 0
   changeset:   0:0e46349438790c460c5c9f7546bfcd39b267bbd2
   phase:       draft
@@ -229,7 +229,7 @@  log rotation
   commit1
   
   
-  result: None
+  result: 0
   running: --debug log -r tip
   changeset:   1:45589e459b2edfbf3dbde7e01f611d2c1e7453d7
   tag:         tip
@@ -245,7 +245,7 @@  log rotation
   commit2
   
   
-  result: None
+  result: 0
   $ hg blackbox
   1970/01/01 00:00:00 bob @45589e459b2edfbf3dbde7e01f611d2c1e7453d7 (5000)> updating the branch cache
   1970/01/01 00:00:00 bob @45589e459b2edfbf3dbde7e01f611d2c1e7453d7 (5000)> updated served branch cache in * seconds (glob)
diff --git a/tests/test-dispatch.py.out b/tests/test-dispatch.py.out
--- a/tests/test-dispatch.py.out
+++ b/tests/test-dispatch.py.out
@@ -1,18 +1,18 @@ 
 running: init test1
-result: None
+result: 0
 running: add foo
 result: 0
 running: commit -m commit1 -d 2000-01-01 foo
-result: None
+result: 0
 running: commit -m commit2 -d 2000-01-02 foo
-result: None
+result: 0
 running: log -r 0
 changeset:   0:0e4634943879
 user:        test
 date:        Sat Jan 01 00:00:00 2000 +0000
 summary:     commit1
 
-result: None
+result: 0
 running: log -r tip
 changeset:   1:45589e459b2e
 tag:         tip
@@ -20,4 +20,4 @@  user:        test
 date:        Sun Jan 02 00:00:00 2000 +0000
 summary:     commit2
 
-result: None
+result: 0