garydgregory commented on code in PR #627:
URL: https://github.com/apache/commons-compress/pull/627#discussion_r1894644528
##########
src/main/java/org/apache/commons/compress/compressors/gzip/GzipCompressorOutputStream.java:
##########
@@ -185,14 +196,26 @@ private void writeHeader(final GzipParameters parameters)
throws IOException {
buffer.put((byte) 0);
}
buffer.put((byte) parameters.getOperatingSystem());
+
out.write(buffer.array());
+ crc.update(buffer.array());
if (extra != null) {
out.write(extra.length & 0xff); // little endian
out.write(extra.length >>> 8 & 0xff);
out.write(extra);
+ crc.update(extra.length & 0xff);
+ crc.update(extra.length >>> 8 & 0xff);
+ crc.update(extra);
}
writeC(fileName, parameters.getFileNameCharset());
writeC(comment, parameters.getFileNameCharset());
+
Review Comment:
Remove extra whitespace, if you need to point something out, use an `//
inline comment`.
##########
src/main/java/org/apache/commons/compress/compressors/gzip/GzipCompressorOutputStream.java:
##########
@@ -185,14 +196,26 @@ private void writeHeader(final GzipParameters parameters)
throws IOException {
buffer.put((byte) 0);
}
buffer.put((byte) parameters.getOperatingSystem());
+
Review Comment:
Remove extra whitespace, if you need to point something out, use an `//
inline comment`.
##########
src/test/java/org/apache/commons/compress/compressors/gzip/GzipCompressorOutputStreamTest.java:
##########
@@ -214,4 +220,57 @@ public void testFileNameChinesePercentEncoded() throws
IOException {
testFileName("??????.xml", EXPECTED_FILE_NAME);
}
+
+ /**
+ * Tests the gzip header CRC.
+ *
+ * @throws IOException When the test has issues with the underlying file
system or unexpected gzip operations.
+ */
+ @Test
+ public void testHcrc() throws IOException, DecoderException {
+ final GzipParameters parameters = new GzipParameters();
+ parameters.setHcrc(true);
+ parameters.setModificationTime(0x66554433); // avoid changing time
+ parameters.setFileName("AAAA");
+ parameters.setComment("ZZZZ");
+ parameters.setOS(OS.UNIX);
+ final ExtraField extra = new ExtraField();
+ extra.addSubField("BB", "CCCC".getBytes(StandardCharsets.ISO_8859_1));
+ parameters.setExtraField(extra);
+ ByteArrayOutputStream baos = new ByteArrayOutputStream();
+ try (GzipCompressorOutputStream gos = new
GzipCompressorOutputStream(baos, parameters)) {
+ // no thing to write for this test.
Review Comment:
"no thing" -> "nothing"
##########
src/test/java/org/apache/commons/compress/compressors/gzip/GzipCompressorOutputStreamTest.java:
##########
@@ -214,4 +220,57 @@ public void testFileNameChinesePercentEncoded() throws
IOException {
testFileName("??????.xml", EXPECTED_FILE_NAME);
}
+
+ /**
+ * Tests the gzip header CRC.
+ *
+ * @throws IOException When the test has issues with the underlying file
system or unexpected gzip operations.
+ */
+ @Test
+ public void testHcrc() throws IOException, DecoderException {
+ final GzipParameters parameters = new GzipParameters();
+ parameters.setHcrc(true);
+ parameters.setModificationTime(0x66554433); // avoid changing time
+ parameters.setFileName("AAAA");
+ parameters.setComment("ZZZZ");
+ parameters.setOS(OS.UNIX);
+ final ExtraField extra = new ExtraField();
+ extra.addSubField("BB", "CCCC".getBytes(StandardCharsets.ISO_8859_1));
+ parameters.setExtraField(extra);
+ ByteArrayOutputStream baos = new ByteArrayOutputStream();
+ try (GzipCompressorOutputStream gos = new
GzipCompressorOutputStream(baos, parameters)) {
+ // no thing to write for this test.
+ }
+ byte[] result = baos.toByteArray();
+ byte[] expected = Hex.decodeHex("1f8b" // id1 id2
+ + "08" //cm
+ + "1e" //flg(FEXTRA|FNAME|FCOMMENT|FHCRC)
+ + "33445566" // mtime little endian
+ + "00" + "03" // xfl os
+ + "0800" + "4242" + "0400" + "43434343" //xlen sfid sflen
"CCCC"
+ + "4141414100" // "AAAA" with \0
+ + "5a5a5a5a00" // "ZZZZ" with \0
+ + "d842" //crc32 = 839242d8
+ + "0300" // empty deflate stream
+ + "00000000" // crs32
+ + "00000000" // isize
+ );
+ assertArrayEquals(expected, result);
+ try (GZIPInputStream gis = new GZIPInputStream(new
ByteArrayInputStream(result))) {
+ //if it does not fail, the hcrc is good.
+ }
+ try (GzipCompressorInputStream gis = new GzipCompressorInputStream(new
ByteArrayInputStream(result))) {
+ GzipParameters md = gis.getMetaData();
Review Comment:
"md" is not a good name for maintenance IMO -> "metaData"
##########
src/test/java/org/apache/commons/compress/compressors/gzip/GzipCompressorOutputStreamTest.java:
##########
@@ -214,4 +220,57 @@ public void testFileNameChinesePercentEncoded() throws
IOException {
testFileName("??????.xml", EXPECTED_FILE_NAME);
}
+
+ /**
+ * Tests the gzip header CRC.
+ *
+ * @throws IOException When the test has issues with the underlying file
system or unexpected gzip operations.
+ */
+ @Test
+ public void testHcrc() throws IOException, DecoderException {
+ final GzipParameters parameters = new GzipParameters();
+ parameters.setHcrc(true);
+ parameters.setModificationTime(0x66554433); // avoid changing time
+ parameters.setFileName("AAAA");
+ parameters.setComment("ZZZZ");
+ parameters.setOS(OS.UNIX);
+ final ExtraField extra = new ExtraField();
+ extra.addSubField("BB", "CCCC".getBytes(StandardCharsets.ISO_8859_1));
+ parameters.setExtraField(extra);
+ ByteArrayOutputStream baos = new ByteArrayOutputStream();
+ try (GzipCompressorOutputStream gos = new
GzipCompressorOutputStream(baos, parameters)) {
+ // no thing to write for this test.
+ }
+ byte[] result = baos.toByteArray();
+ byte[] expected = Hex.decodeHex("1f8b" // id1 id2
+ + "08" //cm
+ + "1e" //flg(FEXTRA|FNAME|FCOMMENT|FHCRC)
+ + "33445566" // mtime little endian
+ + "00" + "03" // xfl os
+ + "0800" + "4242" + "0400" + "43434343" //xlen sfid sflen
"CCCC"
+ + "4141414100" // "AAAA" with \0
+ + "5a5a5a5a00" // "ZZZZ" with \0
+ + "d842" //crc32 = 839242d8
+ + "0300" // empty deflate stream
+ + "00000000" // crs32
+ + "00000000" // isize
+ );
+ assertArrayEquals(expected, result);
+ try (GZIPInputStream gis = new GZIPInputStream(new
ByteArrayInputStream(result))) {
+ //if it does not fail, the hcrc is good.
+ }
+ try (GzipCompressorInputStream gis = new GzipCompressorInputStream(new
ByteArrayInputStream(result))) {
+ GzipParameters md = gis.getMetaData();
+ assertTrue(md.hasHcrc());
+ assertEquals(0x66554433, md.getModificationTime());
+ assertEquals(1, md.getExtraField().size());
+ SubField sf = md.getExtraField().iterator().next();
Review Comment:
Use `final` where you can.
##########
src/main/java/org/apache/commons/compress/compressors/gzip/GzipParameters.java:
##########
@@ -539,6 +550,15 @@ public void setFileNameCharset(final Charset charset) {
this.fileNameCharset = Charsets.toCharset(charset,
GzipUtils.GZIP_ENCODING);
}
+ /**
+ * Establishes the presence of the header flag FLG.FHCRC and its headers
crc16 value.
+ *
+ * @param hcrc when true, the header crc16 is calculated and inserted in
the gzip header on write; on read it means it was present.
+ */
+ public void setHcrc(boolean hcrc) {
Review Comment:
- This is not a good public API name IMO. "Hcrc" -> "HeaderCrc()".
- New public and protected elements need Javadoc `@since` tags.
##########
src/main/java/org/apache/commons/compress/compressors/gzip/GzipParameters.java:
##########
@@ -432,6 +433,15 @@ public OS getOS() {
return operatingSystem;
}
+ /**
+ * Returns if the header crc is to be added (when writing) or was present
(when reading).
+ *
+ * @return true is header crc si to be added (on write) or was found
(after read).
+ */
Review Comment:
- This is not a good public API name IMO. "Hcrc" -> "HeaderCrc()".
- New public and protected elements need Javadoc `@since` tags.
##########
src/main/java/org/apache/commons/compress/compressors/gzip/GzipCompressorOutputStream.java:
##########
@@ -47,6 +47,9 @@ public class GzipCompressorOutputStream extends
CompressorOutputStream<OutputStr
/** Header flag indicating a comment follows the header */
private static final int FCOMMENT = 1 << 4;
+ /** Header flag indicating a header crc follows the header */
Review Comment:
Javadoc "crc" -> "CRC" because CRC is an acronym.
--
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]
