add new property on UZKFileInfo#88
add new property on UZKFileInfo#88abbeycode merged 15 commits intoabbeycode:v2.0from MartinLau7:v1.9
Conversation
abbeycode
left a comment
There was a problem hiding this comment.
Thanks for this contribution! I'm ultimately going to want this in the 2.0 version, but we'll worry about moving it over once I've gotten an Xcode 10 build of the new v2.0 branch going. Don't bother rebasing at this time.
Most of my comments involve replacing inline comments and constants with enums. But, I'd also like to see more testing. One of my inline comments mentioned the isDirectory flag on an MS-DOS format archive, but I'd like to have tests for all of these attributes. Look at the PermissionsTests.swift file as an example of how to write it and what types of cases we should handle.
We'll need to be able to write these new attributes also, but for now leave those test cases empty. I'll handle writing these out and filling in those test cases once I make some structural changes for v2.0, which I'll do after we merge this PR. I'm basically going to remove most of the overloaded method calls in favor of having the API consumer send a UZKFileInfo instance, and that can have some public initializers for the various cases handled now by the overloads.
|
I've retargeted this PR to the v2.0 branch, which didn't change any of the main classes, only project configuration and unit tests, so it should be a conflict-free rebase if you'd like to do that now. |
|
Let's take a new PR review and discussion in the 2.0 branch. |
|
We can keep this PR open. I already targeted it to 2.0 and you should be able to rebase without any changes. |
|
@abbeycode Sorry, since I started to travel, the new code submission is temporarily unable to provide a test example, I need your help to provide a test example.
|
|
I’ll take a look at your changes and questions when I can, probably Monday. Please don’t feel any pressure from me to work on this while you’re on vacation — work on it whenever you’re able to. One issue, though: I think when you use the word “judgment”, it might not be a precise translation. Are you able to use a different word, or help me figure out what you mean? I can’t tell from the context. |
|
Thanks, judgment is wrong translation, the correct should be Read.
取得 iOS 版 Outlook<https://aka.ms/o0ukef>
…________________________________
寄件者: Dov Frankel <notifications@github.com>
寄件日期: Sunday, June 30, 2019 11:33:53 AM
收件者: abbeycode/UnzipKit
副本: MartinLau; State change
主旨: Re: [abbeycode/UnzipKit] add new property on UZKFileInfo (#88)
I’ll take a look at your changes and questions when I can, probably Monday. Please don’t feel any pressure from me to work on this while you’re on vacation — work on it whenever you’re able to.
One issue, though: I think when you use the word “judgment”, it might not be a precise translation. Are you able to use a different word, or help me figure out what you mean? I can’t tell from the context.
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub<#88?email_source=notifications&email_token=ABJKPFB5GM2OHH3H2ONZT2TP5ASSDA5CNFSM4H4DRJ32YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODY4EMCA#issuecomment-507004424>, or mute the thread<https://github.com/notifications/unsubscribe-auth/ABJKPFABJE5O5T2VHT2TBF3P5ASSDANCNFSM4H4DRJ3Q>.
|
abbeycode
left a comment
There was a problem hiding this comment.
Don't be discouraged - I'm good at finding small errors. Take a look at my comments when you return from your trip, and in the meantime, let me know what specific items you'd like me to help you with. I've made tasks on both code reviews that I'd like to see addressed before a merge.
|
The mistakes you refer to can help me a lot, because this is the code contribution of my first open source project, I need these errors to point out. However, due to google translation problems, I may not be able to understand what you mean in the first place, so I am very grateful that you can patiently point out the mistakes and help me through the process. I will check back and make changes to the new issue tomorrow.
I judged whether the folder I referred to https://github.com/kolpanic/ZipKit, but because of his judgment, I was confused, so I modified the part in the last submitted code.
Also, since I am not familiar with the swift language, I need you to help me with the completion of Tests.
|
|
No problem, I'm happy to help you through this. Also, I wouldn't call most of what I point out "mistakes", implying you did something wrong. Most of the items I point out are stylistic choices, and I'm helping your code stay consistent with the way I do things in the rest of the project. I'm glad to have you helping out, though. You're helping motivate me to do more with this project than I have recently! To make sure I'm understanding correctly, the one thing you need help from me on right now is writing the unit tests for this PR? Also, before I write any sym-link tests, please take a look at my question above about the symlinks. I wanted to know, since it looks like documentation is a little slim on them, what your reason for implementing the |
|
Yes, I currently need the unit test of the new attribute and the unification of the coding style, as you said, this will keep the style of this project.
In addition, the purpose of implementing "isSymLink" is to decide whether to decompress this file when unzipping the zip.
The zip command includes the ability to compress symlinks (but I don't have more actual use cases):
```
$ zip --symlinks -r foo.zip foo/
```
|
|
Thanks for making some of the changes discussed. But looks like you merged in the v1.9 branch for some reason, and the PR is showing a bunch of changes you didn't make, and made it look like some of your changes hadn't been made. Once you rebase and address all open comments, I'll make a branch off of yours and implement some unit tests. The new class methods look pretty much like what I was expecting. Good job! Also, I'm not sure if this was something you did intentionally or if GitHub did it automatically as a result of your pushes, but most (all?) of the open conversations were marked as resolved. If GitHub did it, then you can ignore this, but please don't resolve conversations that I started. I'll resolve once I've verified any changes have been made or questions have been answered. Please don't insert blank lines at the beginning of methods, which is inconsistent with the style of the rest of the code.
|
|
Yes, the current work has not been completed, I will remind you to check when I am finished. The merge for V2.0 is Tower automatically merged with V2.0 changes during Pull, maybe I am not familiar with git operations. |
|
I can't be sure exactly what happened, but when you pull, it shouldn't get any of my changes, only yours from your remote branch on GitHub. You should do a "rebase" off of my latest v2.0 branch, and then force-push that to your remote. I don't use Tower, so you may have a to look up how to do that, but your PR should only show commits with your name on them, not my own, which is what's happening right now (there are 3 of my commits from July 2nd). |
|
@abbeycode The new commit has rolled back the 2.0 submission and resolved the public issue. |
|
If you look at the "Files changed" on this PR, it's showing 16 files now (when I'm expecting maybe 2), and you rolled back all of the v2.0 changes. Please make sure you're rebasing off of abbeycode/v2.0. |
|
Yes, I was unable to submit the rollback correctly because Tower merged with the 2.0 change. Currently I used the file change to exit the 2.0 change. This may cause the submission version to be advanced. |
|
How's it going? I haven't heard from you on this PR in a little while. |
|
Due to the busy schedule, I have not recently reminded you that the currently submitted code has processed most of the problems, I think you can make changes. |
|
Unfortunately, it's going to be difficult for me to assist you with this. All I can suggest is rebasing your branch off of my latest v2.0 commit. I'm not familiar with Tower, or the steps you took to get to where you are. In the worst case, check out the v2.0 branch into a separate directory, paste your changes over that, and then force push to your PR remote's branch. It'll squash your commits, but I think that's fine. |
|
Once I modify the path, the FileInfoTests.swift file will generate compilation errors. |
|
Thanks for catching that! It should be fixed (I added it using the Xcode 11 beta originally, but added just now using Xcode 10). I didn't pay attention when the build failed because I knew a unit test is failing. |
#if os(OSX)
func testIsSymbolicLink_ContainsSymLinks() {
let textFileURL = self.emptyTextFile(ofLength: 20)!
let symLinkURL = textFileURL.deletingLastPathComponent()
.appendingPathComponent(textFileURL
.lastPathComponent
.replacingOccurrences(of: ".txt", with: "-Link.txt"))
try! FileManager.default.createSymbolicLink(at: symLinkURL, withDestinationURL: textFileURL)
let archiveURL = self.archive(withFiles: [textFileURL, symLinkURL], zipOptions: ["--symlinks"])!
let archive = try! UZKArchive(url: archiveURL)
let fileInfo = try! archive.listFileInfo()
let expected = [
textFileURL.lastPathComponent: false,
symLinkURL.lastPathComponent: true,
]
let actual = fileInfo.reduce(into: Dictionary<String, Bool>()) {
$0[$1.filename] = $1.isSymbolicLink
}
XCTAssertEqual(actual, expected)
}
#endifThe code above is from your unit test. let actual = fileInfo.reduce(into: Dictionary<String, Bool>()) {
$0[$1.filename] = $1.isDirectory
}Testing SymbolicLink should be changed to : let actual = fileInfo.reduce(into: Dictionary<String, Bool>()) {
$0[$1.filename] = $1.isSymbolicLink
} |
|
In the new commit code, I have moved "Private Class Methods" after "Properties" and left the first 2 blank lines, ending 3 blank lines. |
There was a problem hiding this comment.
I'm very happy with where we are now - unit tests are passing, and it's a matter of refactoring to make the code as clear as possible. The only big question I have is still regarding symlinked directories. I couldn't produce an archive with a symlinked directory that actually stayed a symlink in the zip. If you're not able to either, let's remove the checks for whether a directory is a symlink from the +itemIsDirectory: method. I've listed the changes I'd like to see in one place below.
Also, the build will probably fail, but as long as the first phase (unit tests and static analysis) succeeds, that's fine. Looks like Twitter updated something with how their profile URLs work and it broke CocoaPods validation.
- Pull in my latest fixes
- Rename zipOSClassMapping variable
- Add
+itemOS: - Encapsulate the
external_fachecks in class methods that describe the role of the fields being read - Either add a test case for a symlinked directory, or remove the
itemIsSymbolicLinkchecks fromitemIsDirectory - Does it make sense to use the
S_IFDIR,S_IFLNKandS_IFMTconstants? - Surround bitwise
&with a space on both sides on line 113
Source/UZKFileInfo.m
Outdated
|
|
||
|
|
||
| + (BOOL)itemIsDirectory:(unz_file_info64 *)fileInfo { | ||
| UZKZipOS zipOSClassMapping = fileInfo->version >> 8; |
There was a problem hiding this comment.
Let's rename this variable zipOS, and maybe also encapsulate each dereference and bit manipulation operation for fileInfo into a separate private class method. In this case, add:
+ (UZKZipOS) itemOS:(unz_file_info64 *)fileInfo { ... }
Source/UZKFileInfo.m
Outdated
| + (BOOL)itemIsDirectory:(unz_file_info64 *)fileInfo { | ||
| UZKZipOS zipOSClassMapping = fileInfo->version >> 8; | ||
| if (zipOSClassMapping == UZKZipOSMSDOS || zipOSClassMapping == UZKZipOSWindowsNT) { | ||
| return 0x01 == (fileInfo->external_fa >> 4) && ![UZKFileInfo itemIsSymbolicLink:fileInfo]; |
There was a problem hiding this comment.
Let's add another wrapper for the fileInfo->external_fa >> 4 field extraction - what's the name of that field in the spec? Also, what's the significance of the 0x01 value? We should have another enum like we did for UZKZipOS to give names to the possible values and make this more readable.
Also, when I was writing the test cases, I couldn't find a way (using the zip command) to get a symlink to a directory in a zip file. Are you able to do that? If you can't figure out how to either, let's remove the itemIsSymbolicLink checks here and on line 109.
Source/UZKFileInfo.m
Outdated
| return 0x01 == (fileInfo->external_fa >> 4) && ![UZKFileInfo itemIsSymbolicLink:fileInfo]; | ||
| } | ||
|
|
||
| return S_IFDIR == (S_IFMT & fileInfo->external_fa >> 16) && ![UZKFileInfo itemIsSymbolicLink:fileInfo]; |
There was a problem hiding this comment.
- Why use the
S_IFDIRandS_IFMTconstants? The names don't make it very clear what we're checking against.S_IFDIRis probably "is dir" andS_IFLNK(below) is probably "is link", but what'sS_IFMT, and how did you even come across these constants? I'm curious - Like I suggested above, let's encapsulate the
S_IFMT & fileInfo->external_fa >> 16expression into a class method explaining the field it's retrieving, and comparing to constants - unless it's boolean, which it looks like it may be. Then just place the boolean call inline where the expression is - Also as mentioned above, either we write a test case that hits the
YES && NOcondition here, or we drop the&&clause
There was a problem hiding this comment.
About using S_IFDIR, S_IFLNK and S_IFMT constants is to get the type of zip entry more accurately, get Unix Filemodel by (external_fa >> 16).
You can get more information here:
Https://unix.stackexchange.com/questions/14705/the-zip-formats-external-file-attribute/14727#14727
About using 0x01 == (fileInfo->external_fa >> 4)
0x01 represents a folder in WinZip, the specific value is from Google chromium project line 4000
zip has an 'attribute' 32bit value.
Its lower half is windows stuff, its upper half is standard unix attr.
About using S_IFDIR, S_IFLNK and S_IFMT constants is to get the type of zip entry more accurately, get Unix Filemodel by About using |
Unzip -Z -l foo.zipUsing the unzip command to list attributes, currently only known from Use |
2. rename zipOSClassMapping variable(itemOS) 3. add `+itemOS:` method 4. add `itemFileMode:` method
|
I like the way this is shaping up. I'm going to merge your changes into Regarding your last comment on checking for symbolic links when listing directories, can you give me a way to create a specific archive that will demonstrate this (instead of foo.zip)? Again, I'm looking for an example archive that would return an incorrect value for the |
|
I pushed an update to |
|
Currently I can only create a zip from the terminal command to test specific code examples.
Now check the list with Archive: test.zip
Zip file size: 850 bytes, number of entries: 5
rwxr-xr-x 3.0 unx 0 bx 0 stor 19-Jul-29 22:32 ../test/
lrwxr-xr-x 3.0 unx 7 bx 7 stor 19-Jul-29 22:29 ../test/testDirLink
drwxr-xr-x 3.0 unx 0 bx 0 stor 19-Jul-29 22:28 ../test/testDir/
-rw-r--r-- 3.0 unx 0 bx 0 stor 19-Jul-29 22:30 ../test/testFile.md
lrwxr-xr-x 3.0 unx 11 bx 11 stor 19-Jul-29 22:31 ../test/testFileLink.md
5 files, 18 bytes uncompressed, 18 bytes compressed: 0.0%The attribute contains This is the file(test.zip) I used for testing. |
|
I am also very satisfied with the current update, I have merged PR88 into my branch. But I still hope that you will perform the final test on the one I suggested above, and keep the judgment of |
|
I used the same commands you specified, and came up with an archive that looks like this when extracted:
I wrote this test case, which passes, showing that the symlink directory shows up in the Zip metadata as a link, not a directory. I'm going to clean up this directory and include it in one last update.
It does make sense, if you look at where the Zip metadata is coming from - there's a single field that can only indicate whether it's a link or a directory. For links to directories, it's more important that it's a symlink. |
…the Zip metadata as symlinks, and not directories
|
Yes, for a link to a directory, Zip metadata can only express that it is a symbolic link, and it cannot be specified whether it is a file or a directory, but it still makes sense. |
|
I'll merge this after I get the CocoaPods social media validation fixed. I don't want to break the v2.0 branch's build. I'm working on a CocoaPods PR (two, actually) to address this. |
|
I merge PR88 in the new commit and use the system function and remove the newly defined UZKFileType. Do you think it is feasible? |
|
Please put |
|
I have rolled back this submission and it is currently consistent with PR88. |
abbeycode
left a comment
There was a problem hiding this comment.
Ready to merge, pending the CocoaPods validation getting fixed (this does not need to be addressed in this PR, but rather in CocoaPods)
@abbeycode Since "external_fa" contains other meanings, the new commit changes and adds the following attributes: