Fix #12749: Do not report a zero exit code from mvnsh as an error - #12750
Fix #12749: Do not report a zero exit code from mvnsh as an error#12750HDPark95 wants to merge 1 commit into
Conversation
Commands invoked from mvnsh terminate by throwing InvokerException.ExitException, and they do so on success too: helpOrVersionAndMayExit() throws ExitException(0) for --help and --version. The mvn, mvnenc and mvnup handlers caught that exception and logged an error without looking at the code, so a plain "mvn --version" inside the shell printed [ERROR] mvn command exited with exit code 0 right after the version banner. Route the three handlers through a shared reportExitCode() that only logs when the code is non-zero. Fixes apache#12749
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
APPROVE — Clean, correct fix that stops the mvnsh shell from reporting exit code 0 as an error.
The root cause is well-identified: helpOrVersionAndMayExit() throws ExitException(0) for --help and --version, and the old catch blocks unconditionally logged an error regardless of exit code. The shell() method (external process execution) already had the correct if (exitCode != 0) guard, confirming this is the expected pattern.
Strengths:
- The refactoring into
reportExitCode()as a shared helper is well-scoped and reduces duplication. - Package-private static placement makes it accessible from the inner class and tests without exposing it publicly.
- The
RecordingLoggertest double cleanly implements theLoggerinterface and verifies both the error path (non-zero exit code) and the no-error path (zero exit code).
Minor observation (non-blocking):
- The existing
shell()method still implements its own inlineif (exitCode != 0)check with a slightly different message format ("exited with code" vs "exited with exit code"). Pre-existing inconsistency, not a blocker — a future cleanup could unify both paths throughreportExitCode.
This review was generated by an AI agent (Claude Code) and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of Guillaume Nodet
| } | ||
|
|
||
| /** | ||
| * Reports the exit code of a command invoked from the shell. Commands terminate by throwing |
There was a problem hiding this comment.
This doc comment is at least confusing and perhaps wrong. Rewrite.
| * {@code --version} exit with code 0. Only a non-zero code is an error worth reporting. | ||
| */ | ||
| static void reportExitCode(Logger logger, String commandName, int exitCode) { | ||
| if (exitCode != 0) { |
There was a problem hiding this comment.
The method name says it reports an exit code, but it doesn't always do that. Rename the method of perhaps move the if block to where this method is invoked.
…, no new commits)
3 participants
helpOrVersionAndMayExit()throwsInvokerException.ExitException(0)for--helpand--version, so a successful command reaches the shell's catch block exactly like a failing one. Themvn,mvnencandmvnuphandlers logged an error without looking at the code:The three handlers now share
reportExitCode(), which logs only a non-zero code.Two unit tests cover both branches. With the guard removed,
zeroExitCodeIsNotReportedfails on the exact message from the report;impl/maven-cliis green at 637 tests with checkstyle and spotless enabled.Fixes #12749