Skip to content
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

Fix UnArchiver#isOverwrite not working as expected #229

Merged
merged 1 commit into from
Sep 6, 2022

Conversation

plamentotev
Copy link
Member

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

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
Copy link
Member Author

@mthmulders do you recall if there was some specific use case that caused the change in behavior, or maybe it was caused by confusion because of this comment - #128 (comment)

@plamentotev plamentotev added the bug label Sep 3, 2022
@mthmulders
Copy link
Contributor

@mthmulders do you recall if there was some specific use case that caused the change in behavior, or maybe it was caused by confusion because of this comment - #128 (comment)

It's been a while, @plamentotev... I don't recall exactly - that particular PR had been open for almost a year when the comment was added, and I do recall that I was confused by it. I am afraid the confusion may have let to an unintended change of behaviour, but I cannot say for sure.

It might be good to check if this (partial) has any effect on the Maven Dependency Plugin, in particular the situation described in MDEP-651.

@plamentotev
Copy link
Member Author

@mthmulders, thanks for the reply.

It might be good to check if this (partial) has any effect on the Maven Dependency Plugin, in particular the situation described in MDEP-651.

I'm not sure I follow you on this one. It would not affect the warning - it would always show it if there is existing file on the file system that is with different casing (e.g readme and README). But the content of the file after this change may be different as the override rules are changed.

My understanding is that MDEP-651 is about showing the warning and the content does not matter because in both cases (file is overridden or not) the content is corrupted, right? Or I'm missing something and for example README should never override readme?

@plamentotev plamentotev merged commit 8656ee0 into master Sep 6, 2022
@plamentotev plamentotev deleted the fix-isOverwrite branch September 6, 2022 06:12
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 this pull request may close these issues.

UnArchiver inconsisteny regarding overwrite
2 participants