You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by GitBox <gi...@apache.org> on 2022/12/29 21:11:05 UTC
[GitHub] [commons-compress] garydgregory commented on a diff in pull request #345: COMPRESS-613: Support for extra time data in Zip archives
garydgregory commented on code in PR #345:
URL: https://github.com/apache/commons-compress/pull/345#discussion_r1059143999
##########
src/main/java/org/apache/commons/compress/archivers/zip/X000A_NTFS.java:
##########
@@ -68,7 +68,14 @@
* @NotThreadSafe
*/
public class X000A_NTFS implements ZipExtraField {
- private static final ZipShort HEADER_ID = new ZipShort(0x000a);
+
+ /**
+ * The header ID for this extra field.
+ *
+ * @since 1.23
+ */
+ public static final ZipShort HEADER_ID = new ZipShort(0x000a);
Review Comment:
Why is this public now? Would package-private suffice? Let's make sure to avoid increasing the API surface unless we must.
##########
src/main/java/org/apache/commons/compress/utils/TimeUtils.java:
##########
@@ -46,6 +46,62 @@ public final class TimeUtils {
*/
static final long WINDOWS_EPOCH_OFFSET = -116444736000000000L;
+ /**
+ * Converts "standard UNIX time" (in seconds, UTC/GMT) to {@link FileTime}.
Review Comment:
Why is "standard UNIX time" in quotes?
##########
src/main/java/org/apache/commons/compress/utils/TimeUtils.java:
##########
@@ -46,6 +46,62 @@ public final class TimeUtils {
*/
static final long WINDOWS_EPOCH_OFFSET = -116444736000000000L;
+ /**
+ * Converts "standard UNIX time" (in seconds, UTC/GMT) to {@link FileTime}.
+ *
+ * @param time UNIX timestamp
+ * @return the corresponding FileTime
+ */
+ public static FileTime unixTimeToFileTime(final long time) {
+ return FileTime.from(time, TimeUnit.SECONDS);
+ }
+
+ /**
+ * Converts {@link FileTime} to "standard UNIX time".
+ *
+ * @param time the original FileTime
+ * @return the UNIX timestamp
+ */
+ public static long fileTimeToUnixTime(final FileTime time) {
+ return time.to(TimeUnit.SECONDS);
+ }
+
+ /**
+ * Converts Java time (milliseconds since Epoch) to "standard UNIX time".
+ *
+ * @param time the original Java time
+ * @return the UNIX timestamp
+ */
+ public static long javaTimeToUnixTime(final long time) {
Review Comment:
The method name is good here because the parameter and return type are long.
##########
src/main/java/org/apache/commons/compress/archivers/zip/X5455_ExtendedTimestamp.java:
##########
@@ -130,16 +155,20 @@ private static Date zipLongToDate(final ZipLong unixTime) {
return unixTime != null ? new Date(unixTime.getIntValue() * 1000L) : null;
}
+ private static FileTime zipLongToFileTime(final ZipLong unixTime) {
Review Comment:
The "zipLong" method name prefix is redundant with the argument type `zipLongToFileTime` -> `toFileTime` or probably better `zipLongToFileTime` -> `unixToFileTime`
##########
src/main/java/org/apache/commons/compress/archivers/zip/X000A_NTFS.java:
##########
@@ -97,13 +105,13 @@ private static FileTime zipToFileTime(final ZipEightByteInteger z) {
}
return TimeUtils.ntfsTimeToFileTime(z.getLongValue());
}
-
private ZipEightByteInteger modifyTime = ZipEightByteInteger.ZERO;
private ZipEightByteInteger accessTime = ZipEightByteInteger.ZERO;
private ZipEightByteInteger createTime = ZipEightByteInteger.ZERO;
+
Review Comment:
Remove extra empty line.
##########
src/main/java/org/apache/commons/compress/archivers/zip/X5455_ExtendedTimestamp.java:
##########
@@ -242,6 +285,20 @@ public Date getCreateJavaTime() {
return zipLongToDate(createTime);
}
+ /**
+ * Returns the create time as a {@link FileTime}
+ * of this zip entry, or null if no such timestamp exists in the zip entry.
+ * The milliseconds are always zeroed out, since the underlying data
+ * offers only per-second precision.
+ *
+ * @return modify time as {@link FileTime} or null.
+ *
Review Comment:
Remove extra line b/w Javadoc tags return and since.
##########
src/main/java/org/apache/commons/compress/utils/TimeUtils.java:
##########
@@ -46,6 +46,62 @@ public final class TimeUtils {
*/
static final long WINDOWS_EPOCH_OFFSET = -116444736000000000L;
+ /**
+ * Converts "standard UNIX time" (in seconds, UTC/GMT) to {@link FileTime}.
+ *
+ * @param time UNIX timestamp
+ * @return the corresponding FileTime
+ */
+ public static FileTime unixTimeToFileTime(final long time) {
+ return FileTime.from(time, TimeUnit.SECONDS);
+ }
+
+ /**
+ * Converts {@link FileTime} to "standard UNIX time".
+ *
+ * @param time the original FileTime
+ * @return the UNIX timestamp
+ */
+ public static long fileTimeToUnixTime(final FileTime time) {
+ return time.to(TimeUnit.SECONDS);
+ }
+
+ /**
+ * Converts Java time (milliseconds since Epoch) to "standard UNIX time".
+ *
+ * @param time the original Java time
+ * @return the UNIX timestamp
+ */
+ public static long javaTimeToUnixTime(final long time) {
+ return time / 1000L;
+ }
+
+ /**
+ * Checks whether a FileTime exceeds the minimum or maximum for the "standard UNIX time".
+ * If the FileTime is null, this method always returns false.
+ *
+ * @param time the FileTime to evaluate, can be null
+ * @return true if the time exceeds the minimum or maximum UNIX time, false otherwise
+ */
+ public static boolean exceedsUnixTime(final FileTime time) {
Review Comment:
The word "exceeds" is quite unusual in APIs IMO, also implies a greater than relationship, which is not what happens here. I would just is the more standard "is" prefix for names like "isUnixTime()". For "is", method, I've been using the starting sentence as "Tests...".
##########
src/main/java/org/apache/commons/compress/utils/TimeUtils.java:
##########
@@ -46,6 +46,62 @@ public final class TimeUtils {
*/
static final long WINDOWS_EPOCH_OFFSET = -116444736000000000L;
+ /**
+ * Converts "standard UNIX time" (in seconds, UTC/GMT) to {@link FileTime}.
+ *
+ * @param time UNIX timestamp
+ * @return the corresponding FileTime
+ */
+ public static FileTime unixTimeToFileTime(final long time) {
+ return FileTime.from(time, TimeUnit.SECONDS);
+ }
+
+ /**
+ * Converts {@link FileTime} to "standard UNIX time".
+ *
+ * @param time the original FileTime
+ * @return the UNIX timestamp
+ */
+ public static long fileTimeToUnixTime(final FileTime time) {
Review Comment:
The method prefix "fileTime" is redundant with the argument type, `fileTimeToUnixTime` -> `toUnixTime`. Since Unix time is always in seconds (as a primitive long here), we should not need to include the scale in the name.
##########
src/main/java/org/apache/commons/compress/archivers/zip/X000A_NTFS.java:
##########
@@ -97,13 +105,13 @@ private static FileTime zipToFileTime(final ZipEightByteInteger z) {
}
return TimeUtils.ntfsTimeToFileTime(z.getLongValue());
}
-
Review Comment:
Keep this empty line.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org