Skip to content

[MDEP-651] Warn on duplicate archive entries #128

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Oct 12, 2020
Merged

[MDEP-651] Warn on duplicate archive entries #128

Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
52a9a1f
[MDEP-651] Warn on duplicate archive entries
mthmulders Nov 7, 2019
9b09808
[MDEP-651] Warn on casing conflicts between archive entries and files…
mthmulders Oct 3, 2020
e2f6f4c
[MDEP-651] Tweaks in warning message
mthmulders Oct 4, 2020
e5a465c
[MDEP-651] Restore check on timestamps
mthmulders Oct 4, 2020
6b03a0e
[MDEP-651] Add specification table
mthmulders Oct 5, 2020
b6f5499
[MDEP-651] Rename variables
mthmulders Oct 9, 2020
cf7c6d0
[MDEP-651] Optimise for situations where there is no conflict, and fi…
mthmulders Oct 9, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions src/main/java/org/codehaus/plexus/archiver/AbstractUnArchiver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -343,9 +343,14 @@ protected void extractFile( final File srcF, final File dir, final InputStream c

try
{
if ( !isOverwrite() && f.exists() && ( f.lastModified() >= entryDate.getTime() ) )
if ( f.exists() && !StringUtils.equalsIgnoreCase( entryName, canonicalDestPath ) )
{
return;
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() ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Also added tests that verify the warning is logged.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mentioned #3 is incorrect: why warn if there is no override? Did you change that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I did not change that - I strictly followed the example table.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rfscholte How do you want to guarantee the second taste? There are a number of factors to determine case-sensitivity.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK the JRE takes care of that: new File("readme.txt").exists() should return true even it is actually "README.txt"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you are right, of course. Complete forgot that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return;
}
}

// create intermediary directories - sometimes zip don't add them
Expand Down