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

Fix consumer is never called when extract uncompressed 0 byte entry #190

Merged
merged 7 commits into from
Dec 2, 2020

Conversation

mtgto
Copy link
Contributor

@mtgto mtgto commented Oct 23, 2020

Fixes #

Changes proposed in this PR

  • Fixes bug which closure consumer is never called while extracting 0 byte entry from uncompressed archive.

Tests performed

  • Add testExtractUncompressedEmptyFile
    • testExtractUncompressedEmptyFile.zip is copy from testExtractUncompressedDataDescriptorArchive.zip

Further info for the reviewer

  • Using XCTestExpectation to verify consumer closure surely called at once.

Open Issues

N/A

do {
_ = try archive.extract(entry, progress: progress) { (data) in
// Progress does not finished because Progress assumes totalUnitCount > 0
// XCTAssertFalse(progress.isFinished)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When extract 0byte entry, Progress#isFinished returns false.
I don't know it is good way or not...
https://developer.apple.com/documentation/foundation/progress

IMO, It is intuitive that Progress has 1/1 progress for extracting 0byte entry.

Copy link
Owner

@weichsel weichsel Dec 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good point - we currently don't handle this edge case properly.
We are using a 1/1 progress for entries of type .directory already and could also use the same approach for zero-byte files.

Since this PR handles the bug where the completion handler is not called for zero-byte files, I removed the progress related code from the test case you provided. I think the progress mis-reporting should be handled in a dedicated PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are using a 1/1 progress for entries of type .directory already and could also use the same approach for zero-byte files.

It looks good to me. thanks!

@weichsel
Copy link
Owner

weichsel commented Dec 2, 2020

Thanks for providing this PR - good catch!

I saw that you also filed a similar issue for compressed entries: #191
I haven't been able to reproduce it yet, but from reading your description it doesn't look like it can be closed with this PR?

@weichsel weichsel merged commit 8b20f45 into weichsel:development Dec 2, 2020
@mtgto
Copy link
Contributor Author

mtgto commented Dec 2, 2020

I saw that you also filed a similar issue for compressed entries: #191
I haven't been able to reproduce it yet, but from reading your description it doesn't look like it can be closed with this PR?

Hmm. In my memory, method is different betweeon compressed and uncompressed entry... (It's the reason I created an another issue)
I'll check again and report to #191 .

@mtgto mtgto deleted the uncompressed_zerobyte branch December 2, 2020 11:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants