gnodet-bot commented on code in PR #13173:
URL: https://github.com/apache/maven/pull/13173#discussion_r4041555970
##########
maven-embedder/src/main/java/org/apache/maven/cli/event/ExecutionEventLogger.java:
##########
@@ -213,16 +223,71 @@ private void logReactorSummary(MavenSession session) {
buffer.append(' ');
}
- buffer.append(statusMessage);
- if (buildSummary != null) {
- formatBuildTime(buffer, buildSummary);
+ buffer.append(entry.getStatusMessage());
+ if (entry.getBuildSummary() != null) {
+ formatBuildTime(buffer, entry.getBuildSummary());
}
- logger.info(buffer.toString());
+ if (entry.getBuildSummary() instanceof BuildFailure) {
+ logger.error(buffer.toString());
+ } else {
+ logger.info(buffer.toString());
+ }
+ buffer.setLength(0);
+ }
+ }
+
+ private static final class ReactorSummaryRequest {
+ private final List<ReactorSummaryEntry> entries;
+ private final StringBuilder buffer;
+ private final boolean singleVersion;
+
+ private ReactorSummaryRequest(List<ReactorSummaryEntry> entries,
StringBuilder buffer, boolean singleVersion) {
Review Comment:
🔧 **`ReactorSummaryRequest` carries mutable state — allocate `StringBuilder`
locally instead**
`ReactorSummaryRequest` is a private data-holder class but it carries a
`StringBuilder` that is mutated across all three `logReactorSummaryGroup` calls
(`append` + `setLength(0)`). This is a correctness trap: any future refactor
that calls `logReactorSummaryGroup` in a different order, twice for the same
group, or in parallel will silently corrupt the buffer with no compiler warning.
The fix is trivial and has no checkstyle impact — allocate the
`StringBuilder` locally at the top of `logReactorSummaryGroup` and drop it from
the class:
```suggestion
private final List<ReactorSummaryEntry> entries;
private final boolean singleVersion;
```
Then update the constructor accordingly, remove `getBuffer()`, change the
call site to:
```java
ReactorSummaryRequest request = new ReactorSummaryRequest(entries,
isSingleVersion);
```
And in `logReactorSummaryGroup`, replace `StringBuilder buffer =
request.getBuffer();` with:
```java
StringBuilder buffer = new StringBuilder(128);
```
Note: the merged master implementation (`#13167`) already uses this pattern
— the local `StringBuilder` approach. This 3.10.x backport should match.
##########
maven-embedder/src/main/java/org/apache/maven/cli/event/ExecutionEventLogger.java:
##########
@@ -170,39 +171,48 @@ private void logReactorSummary(MavenSession session) {
String failureMessage = builder().failure("FAILURE").build();
String unknownMessage = builder().warning("UNKNOWN").build();
- boolean lastWasSkipped = false;
+ List<ReactorSummaryEntry> entries = new ArrayList<>(projects.size());
for (MavenProject project : projects) {
BuildSummary buildSummary = result.getBuildSummary(project);
String statusMessage;
- boolean shouldSkip = result.hasExceptions();
- if (buildSummary == null) {
- statusMessage = skippedMessage;
- } else if (buildSummary instanceof BuildSuccess) {
+ int group;
+ if (buildSummary instanceof BuildSuccess) {
statusMessage = successMessage;
+ group = 1;
} else if (buildSummary instanceof BuildFailure) {
statusMessage = failureMessage;
- shouldSkip = false;
+ group = 2;
+ } else if (buildSummary == null) {
+ statusMessage = skippedMessage;
+ group = 0;
Review Comment:
ℹ️ **`UNKNOWN` buildSummary silently grouped with SKIPPED — consider `group
= 2` for safety**
`BuildSummary` is currently abstract with only `BuildSuccess` and
`BuildFailure` as concrete subclasses, so this branch is dead code in practice.
But if a third subclass (e.g. `BuildCancelled`) is ever added, the result will
land in group 0 alongside SKIPPED modules and be logged at `info` level, with
no visual distinction from a module that was never scheduled to run.
Assigning `group = 2` makes this branch safe by default at zero cost:
```suggestion
statusMessage = unknownMessage;
group = 2;
```
Same concern applies to the second `else` branch at line 191.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
