[MDEP-651] Warn on duplicate archive entries #128
Conversation
If there's an file system entry for the archive entry, a warning is logged. If overwrite is false, extraction is aborted.
src/main/java/org/codehaus/plexus/archiver/AbstractUnArchiver.java
Outdated
Show resolved
Hide resolved
src/main/java/org/codehaus/plexus/archiver/AbstractUnArchiver.java
Outdated
Show resolved
Hide resolved
| String message = String.format( "Archive entry %s and existing file %s names differ only by case." | ||
| + " This may cause issues on case insensitive file systems.", entryName, canonicalDestPath ); | ||
| getLogger().warn( message ); | ||
| if ( !isOverwrite() ) { |
There was a problem hiding this comment.
I would expect ( f.lastModified() >= entryDate.getTime() ) here to stay.
entryname | entrydate | filename | filedate | behavior
readme.txt | 1970 | readme.txt | 2020 | never overwrite
readme.txt | 2020 | readme.txt | 1970 | only overwrite when isOverWrite()
README.txt | 1970 | readme.txt | 2020 | warn + never overwrite
README.txt | 2020 | readme.txt | 1970 | warn + only overwrite when isOverWrite()
I can extend this table when required
There was a problem hiding this comment.
Done. Also added tests that verify the warning is logged.
There was a problem hiding this comment.
You mentioned #3 is incorrect: why warn if there is no override? Did you change that?
There was a problem hiding this comment.
No, I did not change that - I strictly followed the example table.
There was a problem hiding this comment.
@plamentotev and/or @michael-o, any thoughts on rule number 3? Would it make sense to log the warning when we're not going to overwrite anyway?
There was a problem hiding this comment.
In the end it looks like we have 2 tastes:
- make case-sensitive OS users aware the jar will overwrite a file on case-insensitive OS systems.
- warn case-insensitive OS users that is overwriting a file with a different name.
I can imagine that if develop teams are all using case-sensitive OS, the warning doesn't add value.
For that reason I agree that we could go for the second taste.
And if it is an issue, we could always add a parameter to enforce equal OS independent results, but I consider that issue too rare for now.
There was a problem hiding this comment.
@rfscholte How do you want to guarantee the second taste? There are a number of factors to determine case-sensitivity.
There was a problem hiding this comment.
AFAIK the JRE takes care of that: new File("readme.txt").exists() should return true even it is actually "README.txt"
There was a problem hiding this comment.
Yes, you are right, of course. Complete forgot that.
There was a problem hiding this comment.
AFAIK the JRE takes care of that: new File("readme.txt").exists() should return true even it is actually "README.txt"
Double-checked that last Friday, can confirm for Windows, macOS and Linux.
Also verify warning log message.
src/main/java/org/codehaus/plexus/archiver/AbstractUnArchiver.java
Outdated
Show resolved
Hide resolved
I can imagine! In the tests, there are no "real" ZIP files - the tests just invoke this |
…x detecting of casing conflicts
|
If I understand correctly, we have reached consensus on what should in case of (possible) casing conflicts, and on the implementation. If you agree, I'd appreciate if this pull request could be merged - unless some action is required from my side. |
Dummy pom.xml file is needed for invoker to start but it overrides it with content of zip file. Due to codehaus-plexus/plexus-archiver#128 archiver never overrides it as it has newer mdate than what's in archive. Delete manually before extracting.
Dummy pom.xml file is needed for invoker to start but it overrides it with content of zip file. Due to codehaus-plexus/plexus-archiver#128 archiver never overrides it as it has newer mdate than what's in archive. Delete manually before extracting.
If there's an file system entry for the archive entry currently being extracted, a warning is
logged. If overwrite is false, extraction is aborted.
@rfscholte checked with @viqueen, as this PR superseedes #112, but it's less strict - this one will not fail the build.