Patchwork D3444: tests: comprehensively test exit handling

login
register
mail settings
Submitter phabricator
Date May 6, 2018, 4:11 a.m.
Message ID <differential-rev-PHID-DREV-42ugfl44o2z25vpxim5y-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/31278/
State New
Headers show

Comments

phabricator - May 6, 2018, 4:11 a.m.
indygreg created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  Rust `hg` currently has some test failures around exit handling.
  This commit establishes some centralized tests around exit handling so
  that we can unify the behavior of our Rust frontend and `python`.
  
  There are some added tests that use a value other than an integer
  or None for the exit code. The docs for sys.exit() say such a value
  is allowed. However, Mercurial currently crashes in this case. Upcoming
  commits will teach Mercurial to handle these values.
  
  This does introduce a few Python 3 test failures. However, this test
  already has a few failures. And the failures being introduced should
  mostly go away with subsequent commits. So I think this is acceptable.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D3444

AFFECTED FILES
  tests/test-dispatch.t

CHANGE DETAILS




To: indygreg, #hg-reviewers
Cc: mercurial-devel
Yuya Nishihara - May 6, 2018, 12:15 p.m.
I generally like the direction of this series, but I think there's no point
to extend Mercurial's exit code handling to support all weird Python types.

Only ints and (None for 0) are ever valid.
phabricator - May 6, 2018, 12:29 p.m.
yuja added a comment.


  I generally like the direction of this series, but I think there's no point
  to extend Mercurial's exit code handling to support all weird Python types.
  
  Only ints and (None for 0) are ever valid.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D3444

To: indygreg, #hg-reviewers
Cc: yuja, mercurial-devel
phabricator - May 12, 2018, 3:55 a.m.
indygreg added a comment.


  In https://phab.mercurial-scm.org/D3444#54922, @yuja wrote:
  
  > I generally like the direction of this series, but I think there's no point
  >  to extend Mercurial's exit code handling to support all weird Python types.
  >
  > Only ints and (None for 0) are ever valid.
  
  
  The reason I did this is because from the context or `rhg`, we don't have CPython's default exit handling to fall back on. Our choices are:
  
  1. Reimplement CPython's exit/error handling in Rust
  2. Reimplement CPython's exit/error handling in Python in dispatch [so the end state is more well-defined]
  3. Do something crude in `rhg` when we hit special cases that CPython would normally deal with.
  
  Since extensions could do weird things, I figured 2 was the best option.
  
  But I'll take a look at this series again and re-evaluate if we can simplify things. I just thought I'd brain dump on what I'm thinking.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D3444

To: indygreg, #hg-reviewers
Cc: yuja, mercurial-devel
Yuya Nishihara - May 12, 2018, 5:51 a.m.
>   > I generally like the direction of this series, but I think there's no point
>   >  to extend Mercurial's exit code handling to support all weird Python types.
>   >
>   > Only ints and (None for 0) are ever valid.
>   
>   
>   The reason I did this is because from the context or `rhg`, we don't have CPython's default exit handling to fall back on. Our choices are:
>   
>   1. Reimplement CPython's exit/error handling in Rust
>   2. Reimplement CPython's exit/error handling in Python in dispatch [so the end state is more well-defined]
>   3. Do something crude in `rhg` when we hit special cases that CPython would normally deal with.
>   
>   Since extensions could do weird things, I figured 2 was the best option.

I don't fully understand the situation in `rhg`, but maybe we can instead
fix `scmutil.callcatch()` to either translate non-int SystemExit to integer
or raise ProgrammingError as a bad use of sys.exit() in hg. No SystemExit
should be raised outside of the callcatch().

I'm against allowing command functions to return any objects other than
int or None because it's useless.
phabricator - May 12, 2018, 5:52 a.m.
yuja added a comment.


  >   > I generally like the direction of this series, but I think there's no point
  >   >  to extend Mercurial's exit code handling to support all weird Python types.
  >   >
  >   > Only ints and (None for 0) are ever valid.
  >   
  >   
  >   The reason I did this is because from the context or `rhg`, we don't have CPython's default exit handling to fall back on. Our choices are:
  >   
  >   1. Reimplement CPython's exit/error handling in Rust
  >   2. Reimplement CPython's exit/error handling in Python in dispatch [so the end state is more well-defined]
  >   3. Do something crude in `rhg` when we hit special cases that CPython would normally deal with.
  >   
  >   Since extensions could do weird things, I figured 2 was the best option.
  
  I don't fully understand the situation in `rhg`, but maybe we can instead
  fix `scmutil.callcatch()` to either translate non-int SystemExit to integer
  or raise ProgrammingError as a bad use of sys.exit() in hg. No SystemExit
  should be raised outside of the callcatch().
  
  I'm against allowing command functions to return any objects other than
  int or None because it's useless.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D3444

To: indygreg, #hg-reviewers
Cc: yuja, mercurial-devel
phabricator - May 31, 2018, 7:18 p.m.
durin42 accepted this revision.
durin42 added a comment.
This revision is now accepted and ready to land.


  This LG, esp with https://phab.mercurial-scm.org/D3445 which appears to be its child, but https://phab.mercurial-scm.org/D3445 still needs rebased AFAICT

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D3444

To: indygreg, #hg-reviewers, durin42
Cc: durin42, yuja, mercurial-devel

Patch

diff --git a/tests/test-dispatch.t b/tests/test-dispatch.t
--- a/tests/test-dispatch.t
+++ b/tests/test-dispatch.t
@@ -217,3 +217,94 @@ 
   [1]
 
 #endif
+
+Test various exit codes and methods
+
+  $ cd $TESTTMP
+
+  $ cat >> $TESTTMP/exitmodes.py <<'EOF'
+  > from mercurial import pycompat, registrar, commands
+  > cmdtable = {}
+  > command = registrar.command(cmdtable)
+  > @command(b'exit', [], b'[MODE] [VALUE]', norepo=True)
+  > def exitmodes(ui, mode, value=None):
+  >     if mode == b'returnint':
+  >         return int(value)
+  >     elif mode == b'returnstring':
+  >         # sysstr is to make testing easier, since Python 2 and 3 will
+  >         # format bytes and unicode types differently when printed.
+  >         return pycompat.sysstr(value)
+  >     elif mode == b'returnnone':
+  >         return None
+  >     elif mode == b'returnempty':
+  >         return
+  >     elif mode == b'systemexitint':
+  >         raise SystemExit(int(value))
+  >     elif mode == b'systemexitstring':
+  >         raise SystemExit(pycompat.sysstr(value))
+  >     elif mode == b'systemexitnoarg':
+  >         raise SystemExit()
+  >     elif mode == b'systemexitnone':
+  >         raise SystemExit(None)
+  >     elif mode == b'returndict':
+  >         return {'key1': 'value1'}
+  >     elif mode == b'systemexitdict':
+  >         raise SystemExit({'key1': 'value1'})
+  >     else:
+  >         raise Exception('unknown mode: %s' % mode)
+  > EOF
+
+  $ cat >> $HGRCPATH <<'EOF'
+  > [extensions]
+  > exitmodes = $TESTTMP/exitmodes.py
+  > EOF
+
+  $ hg exit returnint 42
+  [42]
+
+  $ hg exit returnstring 'some message'
+  Traceback (most recent call last):
+    File "*/hg", line *, in <module> (glob)
+      dispatch.run()
+    File "*/mercurial/dispatch.py", line *, in run (glob)
+      sys.exit(status & 255)
+  TypeError: unsupported operand type(s) for &: 'str' and 'int'
+  [1]
+
+  $ hg exit returnnone
+
+  $ hg exit returnempty
+
+  $ hg exit returndict
+  Traceback (most recent call last):
+    File "*/hg", line *, in <module> (glob)
+      dispatch.run()
+    File "*/mercurial/dispatch.py", line *, in run (glob)
+      sys.exit(status & 255)
+  TypeError: unsupported operand type(s) for &: 'dict' and 'int'
+  [1]
+
+  $ hg exit systemexitint 42
+  [42]
+
+  $ hg exit systemexitstring 'failure message'
+  Traceback (most recent call last):
+    File "*/hg", line *, in <module> (glob)
+      dispatch.run()
+    File "*/mercurial/dispatch.py", line *, in run (glob)
+      sys.exit(status & 255)
+  TypeError: unsupported operand type(s) for &: 'str' and 'int'
+  [1]
+
+  $ hg exit systemexitnoarg
+
+  $ hg exit systemexitnone
+
+  $ hg exit systemexitdict
+  Traceback (most recent call last):
+    File "*/hg", line *, in <module> (glob)
+      dispatch.run()
+    File "*/mercurial/dispatch.py", line *, in run (glob)
+      sys.exit(status & 255)
+  TypeError: unsupported operand type(s) for &: 'dict' and 'int'
+  [1]