Clean up PR #84 #86
Clean up PR #84 #86
Conversation
Merging @MartinLau7's code from PR #84, as a WIP
… idea, @MartinLau7, but it belongs in a separate PR. Let's work on that after we tackle the POSIX permissions changes
… getting the build to succeed
…using a bitmask instead of the original addition method
…(when none are specified) match previous releases. They didn't (were getting written with 0 permissions), and so I fixed with 0644, which is what the old behavior under previous versions did
…-[UZKArchiveTestCase archiveWIthFiles:password:]
…xing it in an NSNumber
…oads instead of unsigned long, moved the assignment of `external_fa` to the same method that writes the file date attribute, and added tests for writing buffered data with and without permissions
|
@MartinLau7 I'm not able to assign this PR to you for review, but please let me know if it works for you, and comment regarding any changes you'd like to see. Please note that I removed the additional properties you added to |
MartinLau7
left a comment
MartinLau7
left a comment
There was a problem hiding this comment.
I have noticed that the new property is removed in this branch, waiting for the work of this branch to complete, and we continue to add new properties.
| * key from the attributes NSFileManager returns. Assign in octal form, like 0777 in Objective-C or | ||
| * 0o777 in Swift | ||
| */ | ||
| @property (nonatomic, readonly) short posixPermissions; |
There was a problem hiding this comment.
Why use "short" instead of "UInt32", I think "UInt32" would be better.
There was a problem hiding this comment.
The permissions NSFileManager returns are of the type short, or Int16 in Swift, as per the documentation.
| _compressionMethod = [self readCompressionMethod:fileInfo->compression_method | ||
| flag:fileInfo->flag]; | ||
|
|
||
| uLong permissions = (fileInfo->external_fa >> 16) & 0777U; |
There was a problem hiding this comment.
I think you should avoid using such high permissions as 0777. It is recommended to use a maximum of 0755 permissions to prevent malicious code from being corrupted in the zip file.
There was a problem hiding this comment.
You're misunderstanding what this line does. It's applying a bit mask to the external_fa field, basically saying get rid of all bits from this field except the permission bits. I use 777 here because that turns on all of the permission bits, and leaves everything else off.
This comment has been minimized.
This comment has been minimized.
|
@MartinLau7 I want to make sure you get the credit for this change (with your name on the commit), but I also want to squash it to a single commit. Can you clone my |
|
@abbeycode Ok, I have cloned your PR84 to my repository and submitted a new Pull Request.(here) |
|
PR #87 was merged into the v1.9 branch, including these changes |
This PR takes @MartinLau7's changes from PR #84 and implements most (all?) of the feedback that came up during review. He should review this PR, and once he's good with it, I'll have him introduce a PR from a copy of this branch that will merge into the v1.9 branch.