Copilot commented on code in PR #208:
URL:
https://github.com/apache/maven-script-interpreter/pull/208#discussion_r4056807813
##########
src/test/java/org/apache/maven/shared/scriptinterpreter/FileLoggerTest.java:
##########
@@ -81,7 +85,8 @@ void outputFileNoMirror(@TempDir File tempDir) throws
Exception {
try (FileLogger fileLogger = new FileLogger(outputFile)) {
fileLogger.consumeLine("Test1");
- fileLogger.getPrintStream().println("Test2");
+ fileLogger.getPrintStream().print("Test2");
+ fileLogger.getPrintStream().print('\n');
fileLogger.getPrintStream().flush();
Review Comment:
This test reads the log file using the platform default charset. Since
FileLogger now writes UTF-8 unconditionally, decode as UTF-8 here too to keep
the test platform-independent.
This issue also appears in the following locations of the same file:
- line 116
- line 167
##########
src/main/java/org/apache/maven/shared/scriptinterpreter/FileLogger.java:
##########
@@ -133,60 +139,140 @@ public void close() {
}
}
+ /**
+ * A byte stream that normalizes line terminators to {@code \n}, forwards
every normalized byte
+ * to the underlying output stream, and mirrors each flushed chunk to the
+ * {@link FileLoggerMirrorHandler} as a UTF-8 decoded string. Because this
wrapper normalizes
+ * line terminators before forwarding, both the underlying file and the
mirror always use the
+ * same {@code \n} separators, independent of the platform.
+ */
private static class MirrorStreamWrapper extends OutputStream {
- private OutputStream out;
+ private final OutputStream out;
private final FileLoggerMirrorHandler mirrorHandler;
- private StringBuilder lineBuffer;
+ private final ByteArrayOutputStream lineBuffer = new
ByteArrayOutputStream();
+
+ private boolean lastWasCR;
- MirrorStreamWrapper(OutputStream outputStream, FileLoggerMirrorHandler
mirrorHandler) {
- this.out = outputStream;
+ MirrorStreamWrapper(OutputStream out, FileLoggerMirrorHandler
mirrorHandler) {
+ this.out = out;
this.mirrorHandler = mirrorHandler;
- this.lineBuffer = new StringBuilder();
}
@Override
public void write(int b) throws IOException {
- out.write(b);
- lineBuffer.append((char) (b));
+ writeByte(b);
}
@Override
public void write(byte[] b, int off, int len) throws IOException {
- out.write(b, off, len);
- lineBuffer.append(new String(b, off, len));
+ for (int i = off; i < off + len; i++) {
+ writeByte(b[i] & 0xFF);
+ }
+ }
+
+ private void writeByte(int b) throws IOException {
+ if (b == '\r') {
+ lastWasCR = true;
+ return;
+ }
+ if (lastWasCR) {
+ lastWasCR = false;
+ emit('\n');
+ if (b == '\n') {
+ return;
+ }
+ }
+ emit(b);
+ }
+
+ private void emit(int b) throws IOException {
+ out.write(b);
+ lineBuffer.write(b);
}
@Override
public void flush() throws IOException {
+ if (lastWasCR) {
+ lastWasCR = false;
+ emit('\n');
+ }
out.flush();
- int len = lineBuffer.length();
+ byte[] bytes = lineBuffer.toByteArray();
+ int len = bytes.length;
if (len == 0) {
// nothing to log
return;
}
- // remove line end for log
- while (len > 0 && (lineBuffer.charAt(len - 1) == '\n' ||
lineBuffer.charAt(len - 1) == '\r')) {
+ // remove the trailing line end, so each flushed chunk is logged
as a single line
Review Comment:
This comment is inaccurate: the code only trims a single trailing LF, so a
flushed chunk may still contain embedded (or even trailing) newlines if
multiple are written before flush. Consider rewording to describe the actual
behavior (trim one trailing LF before mirroring).
##########
src/test/java/org/apache/maven/shared/scriptinterpreter/FileLoggerTest.java:
##########
@@ -81,7 +85,8 @@ void outputFileNoMirror(@TempDir File tempDir) throws
Exception {
Review Comment:
`outputFile` is created with a child path that starts with '/', so `new
File(tempDir, "/target/test.log")` ignores `tempDir` and writes to an absolute
`/target/test.log` path instead. This makes the test non-portable and may fail
due to permissions or pollute the filesystem.
##########
src/main/java/org/apache/maven/shared/scriptinterpreter/FileLogger.java:
##########
@@ -133,60 +139,140 @@ public void close() {
}
}
+ /**
+ * A byte stream that normalizes line terminators to {@code \n}, forwards
every normalized byte
+ * to the underlying output stream, and mirrors each flushed chunk to the
+ * {@link FileLoggerMirrorHandler} as a UTF-8 decoded string. Because this
wrapper normalizes
+ * line terminators before forwarding, both the underlying file and the
mirror always use the
+ * same {@code \n} separators, independent of the platform.
+ */
private static class MirrorStreamWrapper extends OutputStream {
- private OutputStream out;
+ private final OutputStream out;
private final FileLoggerMirrorHandler mirrorHandler;
- private StringBuilder lineBuffer;
+ private final ByteArrayOutputStream lineBuffer = new
ByteArrayOutputStream();
+
+ private boolean lastWasCR;
- MirrorStreamWrapper(OutputStream outputStream, FileLoggerMirrorHandler
mirrorHandler) {
- this.out = outputStream;
+ MirrorStreamWrapper(OutputStream out, FileLoggerMirrorHandler
mirrorHandler) {
+ this.out = out;
this.mirrorHandler = mirrorHandler;
- this.lineBuffer = new StringBuilder();
}
@Override
public void write(int b) throws IOException {
- out.write(b);
- lineBuffer.append((char) (b));
+ writeByte(b);
}
@Override
public void write(byte[] b, int off, int len) throws IOException {
- out.write(b, off, len);
- lineBuffer.append(new String(b, off, len));
+ for (int i = off; i < off + len; i++) {
+ writeByte(b[i] & 0xFF);
+ }
+ }
+
+ private void writeByte(int b) throws IOException {
+ if (b == '\r') {
+ lastWasCR = true;
+ return;
+ }
+ if (lastWasCR) {
+ lastWasCR = false;
+ emit('\n');
+ if (b == '\n') {
+ return;
+ }
+ }
+ emit(b);
+ }
+
+ private void emit(int b) throws IOException {
+ out.write(b);
+ lineBuffer.write(b);
}
@Override
public void flush() throws IOException {
+ if (lastWasCR) {
+ lastWasCR = false;
+ emit('\n');
+ }
out.flush();
- int len = lineBuffer.length();
+ byte[] bytes = lineBuffer.toByteArray();
+ int len = bytes.length;
if (len == 0) {
// nothing to log
return;
}
- // remove line end for log
- while (len > 0 && (lineBuffer.charAt(len - 1) == '\n' ||
lineBuffer.charAt(len - 1) == '\r')) {
+ // remove the trailing line end, so each flushed chunk is logged
as a single line
+ if (bytes[len - 1] == '\n') {
len--;
}
- lineBuffer.setLength(len);
- mirrorHandler.consumeOutput(lineBuffer.toString());
+ mirrorHandler.consumeOutput(new String(bytes, 0, len,
StandardCharsets.UTF_8));
// clear buffer
- lineBuffer = new StringBuilder();
+ lineBuffer.reset();
}
@Override
public void close() throws IOException {
flush();
- if (out != null) {
- out.close();
- out = null;
+ out.close();
+ }
+ }
+
+ /**
+ * An output stream that normalizes line terminators to the Unix LF
({@code \n}) character,
+ * so the written content is independent of the platform on which it runs.
A CRLF sequence
+ * ({@code \r\n}) or a lone carriage return ({@code \r}) is converted to a
single LF.
+ */
+ private static class LineFeedNormalizer extends OutputStream {
+ private final OutputStream out;
+
+ private boolean lastWasCR;
+
+ LineFeedNormalizer(OutputStream out) {
+ this.out = out;
+ }
+
+ @Override
+ public void write(int b) throws IOException {
+ writeByte(b);
+ }
+
+ @Override
+ public void write(byte[] b, int off, int len) throws IOException {
+ for (int i = off; i < off + len; i++) {
+ writeByte(b[i] & 0xFF);
+ }
+ }
+
+ private void writeByte(int b) throws IOException {
+ if (b == '\r') {
+ lastWasCR = true;
+ return;
+ }
+ if (lastWasCR) {
+ lastWasCR = false;
+ out.write('\n');
+ if (b == '\n') {
+ return;
+ }
+ }
+ out.write(b);
+ }
+
+ @Override
+ public void flush() throws IOException {
+ if (lastWasCR) {
+ lastWasCR = false;
+ out.write('\n');
}
+ out.flush();
}
Review Comment:
LineFeedNormalizer wraps the underlying OutputStream but does not override
close(), so closing the PrintStream only closes the wrapper and leaves the real
file stream open. This can leak file descriptors and can break cleanup on
Windows (e.g., delete/rename of the log file after closing).
--
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]
