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

Keep original entry name (if open in reading mode) #341

Merged
merged 1 commit into from
Mar 4, 2024
Merged

Conversation

kuba--
Copy link
Owner

@kuba-- kuba-- commented Mar 1, 2024

No description provided.

@kuba--
Copy link
Owner Author

kuba-- commented Mar 1, 2024

@404neko, @HunterZ

Hello, I've submitted this PR with some changes which can help you work on windows.
I'm pinging you directly because these changes may have some backward compatible issues, e.g. I decided to remove ZIP_RAW_ENTRYNAME macro (described here: https://github.com/kuba--/zip/issues/233)

Long story short, this PR does not rewrite entry names if archive is opened for reading (keep original names). In other words, if you have incompatible zip archive entry names (e.g. this\filename\is\not\compatible\with\zipspec) it will be kept if opened in 'r' mode, so you can refer to this name.
For writing mode, the \ will be replaced by /.

That's why, I think there is no reason to have ZIP_RAW_ENTRYNAME macro.

Anyway, PTAL, if it works for you.

Btw. This PR should address https://github.com/kuba--/zip/issues/335 issue, as well.

@HunterZ
Copy link

HunterZ commented Mar 1, 2024

I took a look, but I'm not really familiar enough with your codebase to judge the effects of these changes... Does this mean that if a .zip contains \ as a path separator, that I will have to now parse that out myself - unless maybe I open the .zip as read+write instead of just read-only?

@kuba--
Copy link
Owner Author

kuba-- commented Mar 3, 2024

I took a look, but I'm not really familiar enough with your codebase to judge the effects of these changes... Does this mean that if a .zip contains \ as a path separator, that I will have to now parse that out myself - unless maybe I open the .zip as read+write instead of just read-only?

Generally speaking, existing names will be not changed, so if you open for reading and walk through entries you will see original names (like zipinfo does).
Also if you open for appending, original names will not be changed, but the new ones created by this library will have only slashes. In other words, if you want to append an entry my\new\entry, the zip library will create my/new/entry.

@HunterZ
Copy link

HunterZ commented Mar 3, 2024

I guess my question then is what specifically does this change in regards to the libraries behavior that would be relevant to #335, and are there any potential side-effects of that?

Would it make sense to have an optional configuration option to tell a precompiled kubazip lib whether to apply various path normalization heuristics, or have the archive-walking API provide access to raw filename entries so that the application can sort things out as needed (if it doesn't already).

Also wondering if there's anything in the central directory that might help in determining which path normalization heuristics may be reasonable to apply, or if all that metadata is too outdated to be useful?

@kuba--
Copy link
Owner Author

kuba-- commented Mar 3, 2024

I guess my question then is what specifically does this change in regards to the libraries behavior that would be relevant to #335, and are there any potential side-effects of that?

First of all, the method isdir has been change (we check if the last char is either / or \).
The second change (and probably more important) is, we don't cache possible modified entry name comparing what is written in directory.
That's why for invalid zip archives, you could see different entry names if you iterate through entries than names required when you try to open entry for reading.

Last but not least, we've already had ZIP_RAW_ENTRYNAME macro which did slightly similar thing (keep originals instead of following spec.), but in this PR I removed it because there is no more need for that. It's more about conception - do not change anything if it's not your, but if you write stuff by this library it should follow spec.

@kuba-- kuba-- merged commit acb3fac into master Mar 4, 2024
10 checks passed
@kuba-- kuba-- deleted the win-name branch March 4, 2024 23:33
@404neko
Copy link

404neko commented Mar 6, 2024

Thanks for the heads up, I'll keep it in mind for the next zip-library upgrade.

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

Successfully merging this pull request may close these issues.

3 participants