Support archival and restoration of POSIX permissions #84

MartinLau7 · 2019-05-31T02:02:18Z

Submit log

1. Unzip to add recovery for macOS file permissions
2. Compress add retention for macOS file permissions
3. UZKFileInfo Add Permission Attributes

Test Code

NSString *zipFilePath = @"/Users/martin/
UZKArchive *archive = [[UZKArchive alloc] initWithPath:zipFilePath error:&archiveError];
if (archiveError) {
        return;
}
NSDictionary *infoDict = [[NSFileManager defaultManager] attributesOfItemAtPath:[@"your file" stringByAppendingPathComponent:obj] error:nil];
NSNumber *mode = [infoDict objectForKey:NSFilePosixPermissions];

NSData *fileData = ...;
if (![archive writeData:fileData filePath:@"zip file path" posixPermissions:[mode unsignedLongValue] error:nil]) {
       NSLog(@"zip file error");
}

abbeycode

Martin, I'd first like to thank you for contributing this PR! It's a really awesome idea for the framework, and I'm glad you worked out all the details for how best to implement it. You even submitted the request to the right branch! There are some changes I'd like to see before we merge, though. I made some comments inline for context, but I've created tasks in this comment for the bigger-picture changes.

I'd like to leave permissions to only be specified in the most detailed overload. All the other overloads can pass 0. This way we don't break any existing code. So in the header, there should only be two changes (a new declaration for writeData and one for writeIntoBuffer, which add the permissions parameter)
Please revert your changes to the project and scheme (as I think they're what's breaking the build)
Make sure the Travis CI build is passing (it makes sure all the code compiles, and tests pass)
Thanks for adding test code in your PR description, but would you be able to add some unit test cases? It would be great if you could add a PermissionsTests class (preferably in Swift), with methods to test the cases below. There are tons of examples for how I'm testing this framework in the other test cases, and I'll be glad to help with any questions you have.
- Using each of the overloads that doesn't specify permissions creates files on disk with default permissions like what currently happens (preserve backwards-compatibility)
- Creating a new archive with specific non-default permissions and then reading it in (same file, different UZKArchive instance) returns the expected values in the UZKFileInfo instances
- Extracting an archive with non-default permissions results in those permissions in the extracted files

Source/UZKArchive.m

Source/UZKFileInfo.h

Source/UZKArchive.m

MartinLau7 · 2019-06-03T01:54:37Z

First of all, I am sorry that I have not made the correct modifications in the submitted code, I will improve these errors in the future.

abbeycode · 2019-06-03T12:46:34Z

Hey Martin, no big deal, this is part of the process for a pull request. Let's work on this together. The PR isn't final - you can push more changes to your branch, and it'll update.

Source/UZKArchive.m

Source/UZKArchive.h

Source/UZKArchive.m

MartinLau7 · 2019-06-04T03:02:39Z

@abbeycode There is currently a change in the package, I seem to be destroying the version of Xcode, it has been upgraded to Xcode 10.2

abbeycode · 2019-06-04T12:23:37Z

Please let me know when you're done with your changes and are ready for me to review again. Also, make sure you review your changes in the "Files changed" tab when you're done, so they only contain changes you intended to make (not making whitespace changes to code you otherwise didn't touch).

Also, if you're having trouble understanding any of my comments, please let me know, and I can try to rephrase, or go into more detail.

abbeycode · 2019-06-04T12:25:43Z

I just saw your Xcode 10.2 comment. The version of Xcode you use shouldn't matter, so long as you don't commit any upgrade checks or use APIs only available in newer versions of the frameworks (which Xcode shouldn't let you do unless you change the deployment target, which you shouldn't change either :)

MartinLau7

Please let me know when you're done with your changes and are ready for me to review again. Also, make sure you review your changes in the "Files changed" tab when you're done, so they only contain changes you intended to make (not making whitespace changes to code you otherwise didn't touch).

Also, if you're having trouble understanding any of my comments, please let me know, and I can try to rephrase, or go into more detail.

<0031d57>
I have already submitted a new code and still need your check. I will continue if I need to modify it.

How can I conduct a new review?

abbeycode · 2019-06-05T14:36:59Z

Did you see the list of requests I made in the initial review comment?

MartinLau7 · 2019-06-06T05:35:35Z

Did you see the list of requests I made in the initial review comment?

Yes, I see this problem now, because I have an exception in the code version of the workbench that I can't resubmit, I need to close the Pull Request later, until I rearrange and create a new Pull Request. What do you think?

abbeycode · 2019-06-06T12:37:20Z

You don't need to close this PR, and you don't need to use a different version of Xcode. Just back out the changes you made to the project.pbxproj and UnzipKit.xcscheme files that aren't directly necessary for your change. You can use Xcode to this, if you view the changes from the v1.9 branch. Once you've backed out those changes, you can push to this branch again, and the PR will update.

2. Remove the permission to set the default 0644U, now the default is 0755U 3. Fix the damage caused by Xcode 4. Add to Test Code and Test File 5. Add the writeFile function

MartinLau7 · 2019-06-13T03:31:20Z

You don't need to close this PR, and you don't need to use a different version of Xcode. Just back out the changes you made to the project.pbxproj and UnzipKit.xcscheme files that aren't directly necessary for your change. You can use Xcode to this, if you view the changes from the v1.9 branch. Once you've backed out those changes, you can push to this branch again, and the PR will update.

@abbeycode Since Xcode breaks git, it is not considered to turn off PR before. Now I have resubmitted and passed the Travis CI build, I will continue to modify if there are new issues.

abbeycode

Thanks for sticking with this process, Martin. It's starting to shape up! It seems like there's a lot of criticism in this review, but there are only a few bigger things. Most of my comments are minor nitpicks.

Source/UZKArchive.h

Source/UZKArchive.m

Source/UZKArchive.h

Source/UZKArchive.m

Tests/TestKeppPrmissions.m

Tests/UZKArchiveTestCase.m

UnzipKit.xcodeproj/project.pbxproj

abbeycode · 2019-06-20T12:50:01Z

I've created a new branch, pr84, to merge your PR into, for me to then work on it further. Can you please rebase your changes on that branch? I merged a different PR just now, that made two single-line fixes. After your rebase I'll merge into that branch and create a new PR for you to review once I've implemented the changes we discussed. You can keep the "v1.9" branch name in your repo. That shouldn't matter.

2. Remove the permission to set the default 0644U, now the default is 0755U 3. Fix the damage caused by Xcode 4. Add to Test Code and Test File 5. Add the writeFile function

MartinLau7 · 2019-06-21T09:33:40Z

I've created a new branch, pr84, to merge your PR into, for me to then work on it further. Can you please rebase your changes on that branch? I merged a different PR just now, that made two single-line fixes. After your rebase I'll merge into that branch and create a new PR for you to review once I've implemented the changes we discussed. You can keep the "v1.9" branch name in your repo. That shouldn't matter.

I have rebase to "PR84", but there are some merge errors, I think I need to modify it in my free time, I will add some new code to optimize the "UZKFileInfo - isDirectory" implementation, then I will commit again.

abbeycode · 2019-06-22T23:18:55Z

What are you changing in the isDirectory implementation?

Also, the changes I'm asking you to rebase into your branch are super simple: just a single commit affecting two lines (link here). Just resolve the rebase conflict using your version of UZKArchive.m, and before you commit make that same change, replacing UZKCompressionMethodDefault with method on those two lines.

2. Add the UZKFileInfo property (isSymLink, isResourceFork) 3. UZKFileInfo isDirectory attribute value takes a new judgment function 4. Rebase branch to "PR84"

MartinLau7 · 2019-06-24T07:30:42Z

I have rebased to "PR84" and modified the previous error.
In the new commit I modified the isDirectory code to make it better for type judgment, and I added "isSymLink" and "isResourceFork".
"isSymLink" can determine if the file is a symbolic link.
"isResourceFork" can help us decide whether to decompress the "__MACOSX" content.

MartinLau7 · 2019-06-24T07:32:59Z

The new commit seems to have changed something in Xcode, and I have avoided submitting Xcode files as much as possible. This makes me very annoyed, I need your help to get it back to the original version.

abbeycode · 2019-06-24T12:09:59Z

Martin, I would like to merge this PR into the pr84 branch I created and make the remaining changes I'd like to see there myself. What would you like to get back to the original version, and what do you mean by the original version? There are ways in Git and in Xcode to checkout an older revision of a file over your working copy (or to compare its contents to the current revision), and then you can use Xcode to select which changes you want to commit (if any).

I'm going to squash and merge your PR and will start making my own commits on top. I'll open up a new PR and we can continue the discussion there.

… getting the build to succeed

Adds support for archival and restoration of POSIX permissions, finishing the process that started across PRs #84 and #86

abbeycode · 2019-06-27T13:33:59Z

PR #87 was merged into the v1.9 branch, including these changes

abbeycode requested changes May 31, 2019

View reviewed changes

abbeycode changed the title ~~add posix permissions support~~ Support reading and writing of POSIX permissions May 31, 2019

abbeycode assigned MartinLau7 May 31, 2019

abbeycode added the enhancement label May 31, 2019

abbeycode added this to the 1.9 milestone May 31, 2019

MartinLau7 closed this Jun 3, 2019

abbeycode reopened this Jun 3, 2019

abbeycode reviewed Jun 3, 2019

View reviewed changes

Source/UZKArchive.m Show resolved Hide resolved

abbeycode changed the title ~~Support reading and writing of POSIX permissions~~ Support archival of POSIX permissions Jun 3, 2019

MartinLau7 commented Jun 4, 2019

View reviewed changes

Source/UZKArchive.h Show resolved Hide resolved

Source/UZKArchive.m Outdated Show resolved Hide resolved

abbeycode changed the title ~~Support archival of POSIX permissions~~ Support archival and restoration of POSIX permissions Jun 4, 2019

MartinLau7 commented Jun 5, 2019

View reviewed changes

MartinLau7 added 2 commits June 13, 2019 10:26

1. Code rollback to update

c2f4fa7

2. Remove the permission to set the default 0644U, now the default is 0755U 3. Fix the damage caused by Xcode 4. Add to Test Code and Test File 5. Add the writeFile function

Update test code

e7ed5a4

abbeycode requested changes Jun 14, 2019

View reviewed changes

abbeycode changed the base branch from v1.9 to pr84 June 20, 2019 12:47

MartinLau7 added 3 commits June 21, 2019 15:06

1. Code rollback to update

42a009c

2. Remove the permission to set the default 0644U, now the default is 0755U 3. Fix the damage caused by Xcode 4. Add to Test Code and Test File 5. Add the writeFile function

Update test code

7a47fd7

Merge branch 'v1.9' of https://github.com/MartinLau7/UnzipKit into v1.9

983a706

MartinLau7 added 2 commits June 24, 2019 15:17

1. Remove writeFile method in this version

6c50353

2. Add the UZKFileInfo property (isSymLink, isResourceFork) 3. UZKFileInfo isDirectory attribute value takes a new judgment function 4. Rebase branch to "PR84"

Roll back the Xcode file

34f4dbc

abbeycode merged commit 8c73785 into abbeycode:pr84 Jun 24, 2019

abbeycode added a commit that referenced this pull request Jun 24, 2019

Began cleaning up and starting implementing my suggestions from PR #84,…

67c87ac

… getting the build to succeed

abbeycode mentioned this pull request Jun 25, 2019

Clean up PR #84 #86

Closed

abbeycode pushed a commit that referenced this pull request Jun 27, 2019

Collaborate on PR #84 (#87)

8542397

Adds support for archival and restoration of POSIX permissions, finishing the process that started across PRs #84 and #86

abbeycode added a commit that referenced this pull request Jun 27, 2019

Updated changelog for PR #84

67f304e

abbeycode added a commit that referenced this pull request Jun 27, 2019

Updated changelog for PR #84

6cdc756

Support archival and restoration of POSIX permissions #84

Support archival and restoration of POSIX permissions #84

Uh oh!

Conversation

MartinLau7 commented May 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Submit log

Test Code

Uh oh!

abbeycode left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MartinLau7 commented Jun 3, 2019

Uh oh!

abbeycode commented Jun 3, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MartinLau7 commented Jun 4, 2019

Uh oh!

abbeycode commented Jun 4, 2019

Uh oh!

abbeycode commented Jun 4, 2019

Uh oh!

MartinLau7 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abbeycode commented Jun 5, 2019

Uh oh!

MartinLau7 commented Jun 6, 2019

Uh oh!

abbeycode commented Jun 6, 2019

Uh oh!

MartinLau7 commented Jun 13, 2019

Uh oh!

abbeycode left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

abbeycode commented Jun 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MartinLau7 commented Jun 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

abbeycode commented Jun 22, 2019

Uh oh!

MartinLau7 commented Jun 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MartinLau7 commented Jun 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

abbeycode commented Jun 24, 2019

Uh oh!

abbeycode commented Jun 27, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

MartinLau7 commented May 31, 2019 •

edited

Loading

abbeycode left a comment •

edited

Loading

MartinLau7 left a comment •

edited

Loading

abbeycode commented Jun 20, 2019 •

edited

Loading

MartinLau7 commented Jun 21, 2019 •

edited

Loading

MartinLau7 commented Jun 24, 2019 •

edited

Loading

MartinLau7 commented Jun 24, 2019 •

edited

Loading