ctubbsii commented on code in PR #2071:
URL: https://github.com/apache/zookeeper/pull/2071#discussion_r1346765979
##########
zookeeper-server/src/main/java/org/apache/zookeeper/cli/GetCommand.java:
##########
@@ -93,10 +95,28 @@ public boolean exec() throws CliException {
throw new CliWrapperException(ex);
}
data = (data == null) ? "null".getBytes() : data;
- out.println(new String(data, UTF_8));
+
+ PrintStream output = out;
+
+ // Write to the file if specified
+ if (cl.hasOption("f")) {
+ String fileName = cl.getOptionValue("f");
+ try {
+ output = new PrintStream(fileName);
+ } catch (Exception ex) {
+ throw new CliException(ex);
+ }
+ }
+
+ output.println(new String(data, UTF_8));
Review Comment:
This prints a newline after the bytes, and also tries to interpret the bytes
as UTF-8. If you're writing to a file, I don't think that's what it should be
doing. It should just put the exact bytes it found, without trying to decode
them as UTF-8. Also, this does not match what the set command is doing, which
seems to be taking the file's exact bytes, and returning them directly, without
any encoding or decoding, and without stripping out any trailing newline that
this get command might have added.
Ultimately, I think it comes down to the fact that it really shouldn't be
using a PrintStream at all, but should instead just do `Files.write(path,
data)` (maybe opened with the `CREATE_NEW` option so it doesn't clobber an
existing file accidentally). `Files.write` is the opposite of the
`Files.readAllBytes` that is used in the corresponding set command in this PR.
`println(new String())` is definitely not.
--
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: notifications-unsubscr...@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
