ascheman commented on PR #594: URL: https://github.com/apache/maven-jar-plugin/pull/594#issuecomment-5615553863
Thanks @desruisseaux — great to have change detection robust enough that `forceCreation` is usually unnecessary. I think there's one correctness gap in the length comparison; I've pushed a candidate fix you can cherry-pick. **The length comparison is skipped on the forward-scan match.** In `removeFromFilesInJAR(file, size)` the length is only compared when the entry was already buffered in `filesInJAR`; on the forward-scan match (`p.equals(file)`) it returns `true` without checking the length — though `entry.getSize()` is right there. Since build files are usually requested in roughly ZIP order, that's the common path, so the new length check rarely runs, and a same-timestamp/different-size file (the shade-plugin case this PR targets) can still be treated as up to date. I pushed a small fix that extracts a `sizesMatch(a, b)` helper and applies it on **both** paths (plus an `isDirectory(long)` predicate for the sentinel checks): https://github.com/aschemaven/maven-jar-plugin/commit/e218fac (branch `bugfix/594-size-check-both-paths`) — feel free to cherry-pick. Two things it doesn't include yet — happy to add if useful: - **Tests:** there's no `TimestampCheck` test today. I'd suggest a size-change / same-timestamp case *with build order == JAR order* (exercises the forward-scan path — fails without this fix) and an extra-entry-in-the-JAR case (the shade scenario). I can prepare those. - A note documenting the `null`/absent state of the maps (relied on by `isUpToDateJAR`). Let me know if you'd like the tests too, or prefer to take it from here. -- 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]
