Skip to content

More robust change detection - #594

Open
desruisseaux wants to merge 2 commits into
apache:masterfrom
Geomatys:more-robust-change-detection
Open

More robust change detection#594
desruisseaux wants to merge 2 commits into
apache:masterfrom
Geomatys:more-robust-change-detection

Conversation

@desruisseaux

Copy link
Copy Markdown
Contributor

Modify the documentation about forceCreation and skipIfEmpty for taking in account the fact that the new JAR plugin implementation also verify if the JAR files has more files than expected, which cover the Shape plugin case documented as a reason for the forceCreation option.

Also add file length comparaison when detecting changes in order to increase the validity of the new forceCreation documentation, which said that while supported, this option is often not necessary.

Edit the documentation of skipIfEmpty because this option skips the JAR file creation not only if the directory does not exist, but also if no input files are matched for inclusion.

This pull request complement #585.

Also add file length comparaisons when detecting changes in order
to increase the validity of the new `forceCreation` documentation.

`forceCreation` is less necessary than it was in 3.x because the new
plugin detects when the JAR file contains more files than expected.
Therefore, the example given in documentation (shade plugin) should
not be true anymore.

`skipIfEmpty` skips the JAR file creation not only if the directory
does not exist, but also if no input files are matched for inclusion.
@ascheman

Copy link
Copy Markdown
Contributor

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): aschemaven@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.

@desruisseaux desruisseaux self-assigned this Sep 10, 2026
@desruisseaux desruisseaux added enhancement New feature or request documentation Improvements or additions to documentation labels Sep 10, 2026
TimestampCheck.removeFromFilesInJAR compared the entry length only when
the entry was already buffered in filesInJAR; on the forward-scan match
it returned true without checking the length. With build files requested
in roughly ZIP order that path is the common one, so the length check
rarely ran and a same-timestamp/different-size file (the shade-plugin
case) could still be treated as up to date.

Extract a sizesMatch(a, b) helper and apply it on both paths, and add an
isDirectory(long) predicate for the sentinel checks.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants