Skip to content

UnArchiver inconsisteny regarding overwrite #228

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.

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

Closed
cstamas opened this issue Sep 2, 2022 · 1 comment · Fixed by #229
Closed

UnArchiver inconsisteny regarding overwrite #228

cstamas opened this issue Sep 2, 2022 · 1 comment · Fixed by #229
Labels

Comments

@cstamas
Copy link
Member

cstamas commented Sep 2, 2022

In case of UnArchiver we have this Javadoc:
https://github.com/codehaus-plexus/plexus-archiver/blob/master/src/main/java/org/codehaus/plexus/archiver/UnArchiver.java#L58-L66

The default value of overwrite member is true:
https://github.com/codehaus-plexus/plexus-archiver/blob/master/src/main/java/org/codehaus/plexus/archiver/AbstractUnArchiver.java#L64

But in AbstractUnArchiver, the extractFile method invokes shouldExtractEntry method to check is entry to be extracted, and this method looks like this:
https://github.com/codehaus-plexus/plexus-archiver/blob/master/src/main/java/org/codehaus/plexus/archiver/AbstractUnArchiver.java#L401-L453

Hence, the Javadoc says one, but this method does "extract only if not exists or is newer" (and there is case thing but keep this issue simple). Basically, if file exists, and is newer, it is NOT overwritten despite overwrite member is true.

This is kinda confirmed with UTs as well:
https://github.com/codehaus-plexus/plexus-archiver/blob/master/src/test/java/org/codehaus/plexus/archiver/AbstractUnArchiverTest.java#L127-L180

Do I miss something here?

Reason: using git results that files being used (for example checked out) will always have "now" timestamps, even if project itself is ancient (years old). Hence, this MAY be causing issue like https://issues.apache.org/jira/browse/MWRAPPER-77

@plamentotev
Copy link
Member

plamentotev commented Sep 3, 2022

This is regression introduced with efd980d. The check used to be:

if ( !isOverwrite() && f.exists() && ( f.lastModified() >= entryDate.getTime() ) )
{
    return;
}

plamentotev added a commit that referenced this issue Sep 3, 2022
Regression in efd980d changed the way
UnArchiver#isOverwrite flag work.
It is indented to indicate that UnArchiver should
always override the existing entries.
efd980d changed it to indicate whether existing file
should be overridden if the entry is newer,
while in this case the file should be always overridden.

This commit returns the old behavior: if the entry
is newer, override the existing file; if the entry
is older, override the existing file only if isOverwrite is true.

Fixes: #228
@plamentotev plamentotev added the bug label Sep 3, 2022
plamentotev added a commit that referenced this issue Sep 6, 2022
Regression in efd980d changed the way
UnArchiver#isOverwrite flag work.
It is indented to indicate that UnArchiver should
always override the existing entries.
efd980d changed it to indicate whether existing file
should be overridden if the entry is newer,
while in this case the file should be always overridden.

This commit returns the old behavior: if the entry
is newer, override the existing file; if the entry
is older, override the existing file only if isOverwrite is true.

Fixes: #228
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants