gnodet-bot commented on code in PR #13136:
URL: https://github.com/apache/maven/pull/13136#discussion_r4017537345
##########
compat/maven-embedder/src/main/java/org/apache/maven/cli/event/ExecutionEventLogger.java:
##########
@@ -247,20 +257,26 @@ private void logReactorSummary(MavenSession session) {
buffer.append(' ');
}
- buffer.append(statusMessage);
- if (buildSummary != null) {
- formatBuildTime(buffer, buildSummary);
+ buffer.append(entry.statusMessage());
+ if (entry.buildSummary() != null) {
+ formatBuildTime(buffer, entry.buildSummary());
}
- logger.info(buffer.toString());
+ if (entry.buildSummary() instanceof BuildFailure) {
+ logger.error(buffer.toString());
+ } else {
+ logger.info(buffer.toString());
+ }
buffer.setLength(0);
}
-
- if (lastWasSkipped) {
- logger.info("...");
- }
}
+ private record ReactorSummaryRequest(
+ List<ReactorSummaryEntry> entries, StringBuilder buffer, boolean
isSingleVersion) {}
Review Comment:
🔧 **Mutable `StringBuilder` in `record` — still present, checkstyle argument
doesn't hold**
The author previously responded that the `StringBuilder` is in the record as
a checkstyle workaround. But checkstyle method-length rules apply to the method
body where the code lives — in this case the relevant restriction would be on
`logReactorSummary`, which is already short. The `StringBuilder` allocation
belongs in `logReactorSummaryGroup`, which is a newly added, short method — no
checkstyle constraint applies there.
The fix:
```suggestion
private record ReactorSummaryRequest(
List<ReactorSummaryEntry> entries, boolean isSingleVersion) {}
```
Then in `logReactorSummaryGroup`, change `StringBuilder buffer =
request.buffer();` to `StringBuilder buffer = new StringBuilder(128);`, and
update the call site in `logReactorSummary`:
```java
ReactorSummaryRequest request = new ReactorSummaryRequest(entries,
isSingleVersion);
```
A `record` carrying mutable shared state that is mutated by three
consecutive callers is a correctness trap — any future refactor that calls
`logReactorSummaryGroup` twice in parallel or reorders the calls will silently
corrupt the buffer. Please fix.
##########
impl/maven-cli/src/main/java/org/apache/maven/cling/event/ExecutionEventLogger.java:
##########
@@ -252,20 +262,26 @@ private void logReactorSummary(MavenSession session) {
buffer.append(' ');
}
- buffer.append(statusMessage);
- if (buildSummary != null) {
- formatBuildTime(buffer, buildSummary);
+ buffer.append(entry.statusMessage());
+ if (entry.buildSummary() != null) {
+ formatBuildTime(buffer, entry.buildSummary());
}
- logger.info(buffer.toString());
+ if (entry.buildSummary() instanceof BuildFailure) {
+ logger.error(buffer.toString());
+ } else {
+ logger.info(buffer.toString());
+ }
buffer.setLength(0);
}
-
- if (lastWasSkipped) {
- logger.info("...");
- }
}
+ private record ReactorSummaryRequest(
+ List<ReactorSummaryEntry> entries, StringBuilder buffer, boolean
isSingleVersion) {}
Review Comment:
🔧 Same mutable-`StringBuilder`-in-record issue as the `compat` copy. Apply
the same fix: drop `StringBuilder buffer` from `ReactorSummaryRequest`,
allocate locally at the top of `logReactorSummaryGroup`.
--
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]
