gnodet-bot commented on code in PR #594:
URL: https://github.com/apache/maven-jar-plugin/pull/594#discussion_r4087098906
##########
src/main/java/org/apache/maven/plugins/jar/TimestampCheck.java:
##########
@@ -74,17 +74,47 @@ final class TimestampCheck extends SimpleFileVisitor<Path> {
* Files found in the <abbr>JAR</abbr> file but not yet traversed by the
file visitor.
* Files are added lazily when needed, and removed as soon as they have
been traversed.
* Paths are absolute (resolved with {@link #classesDir}).
- * For each entry, the associated value is whether the path is a directory.
+ * For each entry, the associated value is the file size, or -1 if unknown,
+ * or {@value #ENTRY_IS_DIRECTORY} if the path is a directory.
*/
- private final Map<Path, Boolean> filesInJAR;
+ private final Map<Path, Long> filesInJAR;
/**
* Some of the files in the build directory. This list contains only the
files for which we have already
- * verified the timestamp. We store them in a separate list to avoid
checking to check the timestamp twice.
- * We need this list because we still need to verify if the files are in
the {@link #jarFile}.
- * For each entry, the associated value is whether the path is a directory.
+ * verified the timestamp. We store them in a separate map to avoid to
check the timestamp twice.
+ * We need this map because we still need to verify if the files are in
the {@link #jarFile}.
+ * For each entry, the associated value is the file size, or -1 if unknown,
Review Comment:
Nit: "avoid to check" is not idiomatic English — should be "avoid checking
the timestamp twice". (The old text "avoid checking to check" was also wrong,
so this is an opportunity to fix it properly.)
```suggestion
* verified the timestamp. We store them in a separate map to avoid
checking the timestamp twice.
```
##########
src/main/java/org/apache/maven/plugins/jar/TimestampCheck.java:
##########
@@ -74,17 +74,47 @@ final class TimestampCheck extends SimpleFileVisitor<Path> {
* Files found in the <abbr>JAR</abbr> file but not yet traversed by the
file visitor.
* Files are added lazily when needed, and removed as soon as they have
been traversed.
* Paths are absolute (resolved with {@link #classesDir}).
- * For each entry, the associated value is whether the path is a directory.
+ * For each entry, the associated value is the file size, or -1 if unknown,
+ * or {@value #ENTRY_IS_DIRECTORY} if the path is a directory.
*/
- private final Map<Path, Boolean> filesInJAR;
+ private final Map<Path, Long> filesInJAR;
/**
* Some of the files in the build directory. This list contains only the
files for which we have already
- * verified the timestamp. We store them in a separate list to avoid
checking to check the timestamp twice.
- * We need this list because we still need to verify if the files are in
the {@link #jarFile}.
- * For each entry, the associated value is whether the path is a directory.
+ * verified the timestamp. We store them in a separate map to avoid to
check the timestamp twice.
+ * We need this map because we still need to verify if the files are in
the {@link #jarFile}.
+ * For each entry, the associated value is the file size, or -1 if unknown,
+ * or {@value #ENTRY_IS_DIRECTORY} if the path is a directory.
*/
- private final Map<Path, Boolean> filesInBuild;
+ private final Map<Path, Long> filesInBuild;
+
+ /**
+ * Sentinel length stored in {@link #filesInJAR}/{@link #filesInBuild} for
entries that are directories.
+ * Directories have no meaningful length to compare.
+ */
+ private static final long ENTRY_IS_DIRECTORY = -2;
+
+ /**
+ * {@return whether the given encoded length denotes a directory entry}
+ *
+ * @param size an encoded length from {@link #filesInJAR} or {@link
#filesInBuild}
+ */
+ private static boolean isDirectory(final long size) {
+ return size == ENTRY_IS_DIRECTORY;
+ }
Review Comment:
Design note (non-blocking): treating any negative size as "match" means that
if `ZipEntry.getSize()` returns -1 (e.g., entry stored with a data descriptor,
or a tool that doesn't write central-directory sizes), the size check is
silently skipped and the method degrades to the previous timestamp-only
behavior. This is the safe/conservative choice (no false-positive rebuilds),
and the Javadoc documents it clearly, but it does leave a window where a
shade-plugin-modified entry with an unknown size would not be detected by the
new length comparison. Worth being aware of, though in practice shade-plugin
output has well-formed central directory entries with known sizes.
The current behavior is strictly better than before the PR (which had no
size check at all), so 👍.
--
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]
