-
Notifications
You must be signed in to change notification settings - Fork 64
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
Allow empty zip files. Require a line break at the end. #99
base: master
Are you sure you want to change the base?
Conversation
ngx_http_zip_parsers.c
Outdated
@@ -1,4 +1,3 @@ | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@evanmiller: how do you generate this file? No matter which options I pass to ragel, it uses different formatting, and even different logic. Or do you re-format the code manually?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be an older or newer version of Ragel. What version are you using?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have 7.0.0.12 (that's what Fedora has in its repo). What about you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, yeah Ragel 7 uses a completely new architecture based on Colm. I've been using 6.x. I don't mind either way as long as we standardize.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. Then I'll send out a separate PR that just re-generates this file using 7.x - and then this PR will look more reasonable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rebased it.
An empty zip file is completely valid, but the current mod_zip syntax does not support them. This comes up in cases where e.g. a user clicks "Download this folder" - and the backend needs to special-case empty folders and use different code to generate an empty .zip file. This is technically a breaking change - though the documentation does not say anything about trailing line breaks, and it seems unlikely that anyone would actually rely on the last line break being optional.
c7633f6
to
e7966b9
Compare
An empty zip file is completely valid, but the current mod_zip syntax does not support them. This comes up in cases where e.g. a user clicks "Download this folder" - and the backend needs to special-case empty folders and use different code to generate an empty .zip file.
This is technically a breaking change - though the documentation does not say anything about trailing line breaks, and it seems unlikely that anyone would actually rely on the last line break being optional.