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

Give ZipOutputStream an INameTransform property, and use it to transform the names of newly added entries #497

Merged
merged 1 commit into from
Aug 9, 2020

Conversation

Numpsy
Copy link
Contributor

@Numpsy Numpsy commented Aug 5, 2020

Ref: the discussions about name transforming/cleaning when creating zips:

An experiment into giving ZipOutputStream an INameTransform to (optionally) use to transform file paths, to see what it might look like.
A bit more heavyweight that just having a clean function to call, but more flexible (if that's useful), and fits in more with ZipFile and FastZip?

Defaults to a minimized transform that just strips path roots and puts slashes in the appropriate format (copied from the existing versions and trimmed a bit basically).

ZipEntry.Name is read-only after creation, so it ends up transforming the name twice when doing the local/central headers, rather than just doing it once.

I certify that I own, and have sufficient rights to contribute, all source code and related material intended to be compiled or integrated with the source code for the SharpZipLib open source product (the "Contribution"). My Contribution is licensed under the MIT License.

@Numpsy
Copy link
Contributor Author

Numpsy commented Aug 6, 2020

refs #495

@piksel
Copy link
Member

piksel commented Aug 6, 2020

Yeah, I really like this solution, as it gives a good default with a transparent implementation (PathTransform) that still leaves the consumer with the flexibility to customize the behaviour if they so choose.

I'm not sure I get why it has to do the transform twice though? Is that not possible to avoid?

@Numpsy
Copy link
Contributor Author

Numpsy commented Aug 6, 2020

ZipEntry doesn't expose a means to set the name after construction, so ZipOutputStream couldn't just mutate it in PutNextEntry.

Could maybe give ZipEntry.Name an internal setter to use to update it?

@piksel
Copy link
Member

piksel commented Aug 7, 2020

Although, if we allow the null transform, then we might as well allow changing the Name property (for non-commited ZipEntries of course, since changing it for one that is already written to a zip file won't have the intended effect and would break the lookup).

@Numpsy Numpsy force-pushed the rw/zos/name_transform branch from ab9fbc7 to 7babc29 Compare August 8, 2020 16:27
@Numpsy
Copy link
Contributor Author

Numpsy commented Aug 8, 2020

Updated to rebase on top of the other changes, and to try the alternate approach of an internal property setter for ZipEntry.Name and only doing the transform once in ZipOutputStream.

@Numpsy Numpsy changed the title [experiment] Give ZipOutputStream an INameTransform property, and use it to transform the names of newly added entries Give ZipOutputStream an INameTransform property, and use it to transform the names of newly added entries Aug 8, 2020
@piksel
Copy link
Member

piksel commented Aug 9, 2020

Yeah, this is probably the easiest to do right now. Allowing rename on uncommited entires introduces a lot of complexity for no real gain. LGTM!

@piksel piksel merged commit 1a47326 into icsharpcode:master Aug 9, 2020
@piksel piksel added this to the v1.3 milestone Aug 9, 2020
@Numpsy Numpsy deleted the rw/zos/name_transform branch August 9, 2020 19:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
zip Related to ZIP file format
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants