Page MenuHomePhabricator

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

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



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 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

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

Diff Detail

rHG Mercurial
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

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 inline comments.
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.
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.

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

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.


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.

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.

Ah, that happens because 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.


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.

85 ↗(On Diff #23416)

Same problem as the one above.

This revision was not accepted when it landed; it landed in state Needs Review.
This revision was automatically updated to reflect the committed changes.