This is an archive of the discontinued Mercurial Phabricator instance.

Differential D9238

errors: add config that lets user get more detailed exit codes
ClosedPublic

Authored by martinvonz on Oct 22 2020, 12:07 PM.

Details

Reviewers
durin42
Group Reviewers
hg-reviewers
Commits
rHG21733e8c924f: errors: add config that lets user get more detailed exit codes
Summary

This adds an experimental config that lets the user get more detailed
exit codes. For example, there will be a specific error code for
input/user errors. This is part of
https://www.mercurial-scm.org/wiki/ErrorCategoriesPlan. I've made the
config part of tweakdefaults.

I've made the config enabled by default in tests. My reasoning is that
we want to see that each specific error case gives the right exit code
and we don't want to duplicate all error cases in the entire test
suite. It also makes it easy to grep the .t files for [255] to
find which cases we have left to fix. The logic for the current exit
codes is quite simple, so I'm not too worried about regressions
there. I've added a test case specifically for the "legacy" exit
codes.

I've set the detailed exit status only for the case of
InterventionRequired and SystemExit for now (the cases where we
currently return something other than 255), just to show that it
works.

Diff Detail

Repository
rHG Mercurial
Branch
default
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

martinvonz created this revision.Oct 22 2020, 12:07 PM
martinvonz edited the summary of this revision. (Show Details)Oct 22 2020, 2:21 PM
martinvonz edited the summary of this revision. (Show Details)Oct 22 2020, 4:08 PM
martinvonz updated this revision to Diff 23287.
martinvonz updated this revision to Diff 23369.Oct 29 2020, 4:39 PM
mharbison72 added a subscriber: mharbison72.Nov 3 2020, 6:05 PM
mharbison72 added inline comments.
tests/test-ssh-proto.t
101 ↗(On Diff #23369)

The other non zero -> non zero changes make sense, but was it intended for this to go from zero to non zero exit status? The plan page says this code is for "unexpected", but it seems like it's just an exit? Or was this missing from before?

martinvonz planned changes to this revision.Nov 3 2020, 6:54 PM
martinvonz added inline comments.
tests/test-ssh-proto.t
101 ↗(On Diff #23369)

Thanks for reviewing carefully! I'm pretty sure that's because sshserver.serve_forever() calls sys.exit(0). I'll fix that in some way. I had seen that comment in scmutil.callcatch() about handling SystemExit but I was lazy and just assumed that it wasn't something that we actually do. It seems that the right fix is to search the code for sys.exit() calls and clean those up before this patch. Then SystemExit will be a real programming error and we can correctly use exit 254.

martinvonz added a comment.Nov 3 2020, 11:49 PM

test-ssh-proto.t is no longer affected by this patch, thanks to a few other patches insert ahead of this one. Thanks again for your careful review!

mharbison72 added inline comments.Nov 4 2020, 7:39 PM
tests/test-fix.t
661 ↗(On Diff #23416)

Sorry I missed this before. This appears to be just an error.Abort, so I can't figure out how this gets converted to 254 in the SystemExit(?) handler. Was that intentional? I don't care too much, and was kind of wondering if this is (conceptually) an InterventionRequired case.

I don't want to stall this if you have a plan or are doing more refinement in this area.

tests/test-largefiles-update.t
765 ↗(On Diff #23416)

I assume we don't care about no longer propagating the extension's exit status because it's a python extension, so no API guarantees? Just pointing it out in case this was overlooked in the sea of changes.

tests/test-worker.t
85 ↗(On Diff #23416)

It seems strange that the exit code differs with --traceback (below) vs not (here).

The "known exception" here initially threw me for a bit, but that's from the extension raising the Abort, and not really contra the 254 description of "unknown" from the plan page. So I guess I can see 254 for both of these.

martinvonz marked an inline comment as done.Nov 9 2020, 2:40 PM
martinvonz added inline comments.
tests/test-fix.t
661 ↗(On Diff #23416)

Ah, that happens because worker.py calls sys.exit(). I'll insert a patch before this one to fix that. Thanks again for pointing out these things! I clearly should have reviewed my patch much more carefully myself.

tests/test-largefiles-update.t
765 ↗(On Diff #23416)

Hmm, I had not noticed this one either. It seems better to consider it an internal error if an extension raises SystemExit, so I think this looks like an improvement.

tests/test-worker.t
85 ↗(On Diff #23416)

Same problem as the one above.

martinvonz added a commit: rHG21733e8c924f: errors: add config that lets user get more detailed exit codes.Nov 9 2020, 7:37 PM
This revision was not accepted when it landed; it landed in state Needs Review.
Closed by commit rHG21733e8c924f: errors: add config that lets user get more detailed exit codes (authored by martinvonz). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 23279

mercurial/configitems.py

) )
coreconfigitem( coreconfigitem(
b'ui', b'debugger', default=None, b'ui', b'debugger', default=None,
) )
coreconfigitem( coreconfigitem(
b'ui', b'editor', default=dynamicdefault, b'ui', b'editor', default=dynamicdefault,
) )
coreconfigitem( coreconfigitem(
b'ui', b'detailed-exit-code', default=False, experimental=True,
)
coreconfigitem(
b'ui', b'fallbackencoding', default=None, b'ui', b'fallbackencoding', default=None,
) )
coreconfigitem( coreconfigitem(
b'ui', b'forcecwd', default=None, b'ui', b'forcecwd', default=None,
) )
coreconfigitem( coreconfigitem(
b'ui', b'forcemerge', default=None, b'ui', b'forcemerge', default=None,
) )

mercurial/scmutil.py

def callcatch(ui, func): def callcatch(ui, func):
"""call func() with global exception handling """call func() with global exception handling
return func() if no exception happens. otherwise do some error handling return func() if no exception happens. otherwise do some error handling
and return an exit code accordingly. does not handle all exceptions. and return an exit code accordingly. does not handle all exceptions.
""" """
want_detailed_exit_code = ui.configbool(b'ui', b'detailed-exit-code')
exit_code = -1
try: try:
try: try:
return func() return func()
except: # re-raises except: # re-raises
ui.traceback() ui.traceback()
raise raise
# Global exception handling, alphabetically # Global exception handling, alphabetically
# Mercurial-specific first, followed by built-in and library exceptions # Mercurial-specific first, followed by built-in and library exceptions
except error.InterventionRequired as inst: except error.InterventionRequired as inst:
ui.error(b"%s\n" % inst) ui.error(b"%s\n" % inst)
if inst.hint: if inst.hint:
ui.error(_(b"(%s)\n") % inst.hint) ui.error(_(b"(%s)\n") % inst.hint)
return 1 return 1
except error.WdirUnsupported: except error.WdirUnsupported:
ui.error(_(b"abort: working directory revision cannot be specified\n")) ui.error(_(b"abort: working directory revision cannot be specified\n"))
except error.Abort as inst: except error.Abort as inst:
if want_detailed_exit_code:
# TODO: set exit_code for different errors here
pass
ui.error(_(b"abort: %s\n") % inst.message) ui.error(_(b"abort: %s\n") % inst.message)
if inst.hint: if inst.hint:
ui.error(_(b"(%s)\n") % inst.hint) ui.error(_(b"(%s)\n") % inst.hint)
except ImportError as inst: except ImportError as inst:
ui.error(_(b"abort: %s!\n") % stringutil.forcebytestr(inst)) ui.error(_(b"abort: %s!\n") % stringutil.forcebytestr(inst))
m = stringutil.forcebytestr(inst).split()[-1] m = stringutil.forcebytestr(inst).split()[-1]
if m in b"mpatch bdiff".split(): if m in b"mpatch bdiff".split():
ui.error(_(b"(did you forget to compile extensions?)\n")) ui.error(_(b"(did you forget to compile extensions?)\n"))
raise raise
except MemoryError: except MemoryError:
ui.error(_(b"abort: out of memory\n")) ui.error(_(b"abort: out of memory\n"))
except SystemExit as inst: except SystemExit as inst:
# Commands shouldn't sys.exit directly, but give a return code. # Commands shouldn't sys.exit directly, but give a return code.
# Just in case catch this and and pass exit code to caller. # Just in case catch this and and pass exit code to caller.
return inst.code return inst.code
return -1 return exit_code
def checknewlabel(repo, lbl, kind): def checknewlabel(repo, lbl, kind):
# Do not use the "kind" parameter in ui output. # Do not use the "kind" parameter in ui output.
# It makes strings difficult to translate. # It makes strings difficult to translate.
if lbl in [b'tip', b'.', b'null']: if lbl in [b'tip', b'.', b'null']:
raise error.Abort(_(b"the name '%s' is reserved") % lbl) raise error.Abort(_(b"the name '%s' is reserved") % lbl)
for c in (b':', b'\0', b'\n', b'\r'): for c in (b':', b'\0', b'\n', b'\r'):

mercurial/ui.py

# for use with str.translate(None, _keepalnum), to keep just alphanumerics # for use with str.translate(None, _keepalnum), to keep just alphanumerics
_keepalnum = b''.join( _keepalnum = b''.join(
c for c in map(pycompat.bytechr, range(256)) if not c.isalnum() c for c in map(pycompat.bytechr, range(256)) if not c.isalnum()
) )
# The config knobs that will be altered (if unset) by ui.tweakdefaults. # The config knobs that will be altered (if unset) by ui.tweakdefaults.
tweakrc = b""" tweakrc = b"""
[ui] [ui]
# Gives detailed exit codes for input/user errors, config errors, etc.
detailed-exit-code = True
# The rollback command is dangerous. As a rule, don't use it. # The rollback command is dangerous. As a rule, don't use it.
rollback = False rollback = False
# Make `hg status` report copy information # Make `hg status` report copy information
statuscopies = yes statuscopies = yes
# Prefer curses UIs when available. Revert to plain-text with `text`. # Prefer curses UIs when available. Revert to plain-text with `text`.
interface = curses interface = curses
# Make compatible commands emit cwd-relative paths by default. # Make compatible commands emit cwd-relative paths by default.
relative-paths = yes relative-paths = yes

tests/run-tests.py

return env return env
def _createhgrc(self, path): def _createhgrc(self, path):
"""Create an hgrc file for this test.""" """Create an hgrc file for this test."""
with open(path, 'wb') as hgrc: with open(path, 'wb') as hgrc:
hgrc.write(b'[ui]\n') hgrc.write(b'[ui]\n')
hgrc.write(b'slash = True\n') hgrc.write(b'slash = True\n')
hgrc.write(b'interactive = False\n') hgrc.write(b'interactive = False\n')
hgrc.write(b'detailed-exit-code = True\n')
hgrc.write(b'merge = internal:merge\n') hgrc.write(b'merge = internal:merge\n')
hgrc.write(b'mergemarkers = detailed\n') hgrc.write(b'mergemarkers = detailed\n')
hgrc.write(b'promptecho = True\n') hgrc.write(b'promptecho = True\n')
hgrc.write(b'[defaults]\n') hgrc.write(b'[defaults]\n')
hgrc.write(b'[devel]\n') hgrc.write(b'[devel]\n')
hgrc.write(b'all-warnings = true\n') hgrc.write(b'all-warnings = true\n')
hgrc.write(b'default-date = 0 0\n') hgrc.write(b'default-date = 0 0\n')
hgrc.write(b'[largefiles]\n') hgrc.write(b'[largefiles]\n')

tests/test-basic.t

Create a repository: Create a repository:
#if no-extraextensions #if no-extraextensions
$ hg config $ hg config
devel.all-warnings=true devel.all-warnings=true
devel.default-date=0 0 devel.default-date=0 0
extensions.fsmonitor= (fsmonitor !) extensions.fsmonitor= (fsmonitor !)
largefiles.usercache=$TESTTMP/.cache/largefiles largefiles.usercache=$TESTTMP/.cache/largefiles
lfs.usercache=$TESTTMP/.cache/lfs lfs.usercache=$TESTTMP/.cache/lfs
ui.slash=True ui.slash=True
ui.interactive=False ui.interactive=False
ui.detailed-exit-code=True
ui.merge=internal:merge ui.merge=internal:merge
ui.mergemarkers=detailed ui.mergemarkers=detailed
ui.promptecho=True ui.promptecho=True
web.address=localhost web.address=localhost
web\.ipv6=(?:True|False) (re) web\.ipv6=(?:True|False) (re)
web.server-header=testing stub value web.server-header=testing stub value
#endif #endif

tests/test-commandserver.t

bundle.mainreporoot=$TESTTMP/repo bundle.mainreporoot=$TESTTMP/repo
devel.all-warnings=true devel.all-warnings=true
devel.default-date=0 0 devel.default-date=0 0
extensions.fsmonitor= (fsmonitor !) extensions.fsmonitor= (fsmonitor !)
largefiles.usercache=$TESTTMP/.cache/largefiles largefiles.usercache=$TESTTMP/.cache/largefiles
lfs.usercache=$TESTTMP/.cache/lfs lfs.usercache=$TESTTMP/.cache/lfs
ui.slash=True ui.slash=True
ui.interactive=False ui.interactive=False
ui.detailed-exit-code=True
ui.merge=internal:merge ui.merge=internal:merge
ui.mergemarkers=detailed ui.mergemarkers=detailed
ui.foo=bar ui.foo=bar
ui.nontty=true ui.nontty=true
web.address=localhost web.address=localhost
web\.ipv6=(?:True|False) (re) web\.ipv6=(?:True|False) (re)
web.server-header=testing stub value web.server-header=testing stub value
*** runcommand init foo *** runcommand init foo
*** runcommand -R foo showconfig ui defaults *** runcommand -R foo showconfig ui defaults
ui.slash=True ui.slash=True
ui.interactive=False ui.interactive=False
ui.detailed-exit-code=True
ui.merge=internal:merge ui.merge=internal:merge
ui.mergemarkers=detailed ui.mergemarkers=detailed
ui.nontty=true ui.nontty=true
#endif #endif
$ rm -R foo $ rm -R foo
#if windows #if windows

tests/test-legacy-exit-code.t

  • This file was added.
Tests that the exit code is as expected when ui.detailed-exit-code is *not*
enabled.
$ hg init
$ echo a > a
Expect exit code 0 on success
$ hg ci -Aqm initial
$ hg co nonexistent
abort: unknown revision 'nonexistent'!
[255]
$ hg co 'none()'
abort: empty revision set
[255]
$ hg co 'invalid('
hg: parse error at 8: not a prefix: end
(invalid(
^ here)
[255]
$ hg co 'invalid('
hg: parse error at 8: not a prefix: end
(invalid(
^ here)
[255]
$ hg continue
abort: no operation in progress
[255]
$ hg st --config a=b
abort: malformed --config option: 'a=b' (use --config section.name=value)
[255]