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

Improve support for Unix timestamps in ZIP archives #463

Merged
merged 3 commits into from
Jun 19, 2020
Merged

Improve support for Unix timestamps in ZIP archives #463

merged 3 commits into from
Jun 19, 2020

Conversation

bastianeicher
Copy link
Contributor

I certify that I own, and have sufficient rights to contribute, all source code and related material intended to be compiled or integrated with the source code for the SharpZipLib open source product (the "Contribution"). My Contribution is licensed under the MIT License.

Changes:

  • Store ZipEntry.DateTime in dedicated backing field.
    This allows reading values with a higher resolution than DOS time (2 second accuracy).
  • Fix getting unix modification time in ZIP files.
    I was the contributor that originally added that if-check, but I unfortunately was mistaken. InfoZIP actually does respect a file's modification time, even if the access time and/or creation time are not set.

@bastianeicher bastianeicher changed the title Improve extrac Improve support for Unix timestamps in ZIP archives May 20, 2020
@Numpsy
Copy link
Contributor

Numpsy commented May 20, 2020

Seems like a reasonable simplification (avoids doing all those calculations again on each DataTime property get), and possible helpful for #40 as well (related to the other ifdefed out data time code @

NTTaggedData ntData = extraData.GetData<NTTaggedData>();
)

It does appear to have caused a pair of unit test failures though

ICSharpCode.SharpZipLib.Tests.Zip.ZipEntryHandling.DateAndTime
ICSharpCode.SharpZipLib.Tests.Zip.ZipEntryHandling.Copying

(the test failures don't cause the 'checks' to fail alas).

Looking at the code, I was wondering if there is scope for the DateTime and DosTime members to get out of sync (where setting DateTime updates DosTime, but not vice-versa)?

@Numpsy Numpsy added the zip Related to ZIP file format label May 20, 2020
@bastianeicher
Copy link
Contributor Author

@Numpsy: Thanks for the fast feedback. :)

I've added an additional commit to the PR that removes the backing field for ZipEntry.DosTime. The getters and setters instead convert to and from ZipEntry.DateTime to prevent the two values from becoming potentially inconsistent.

This also fixes the failing unit tests. Sorry I missed them in the original PR.

@piksel piksel self-requested a review May 27, 2020 21:20
This allows reading values with a higher resolution than DOS time (2 second accuracy).
InfoZIP actually does respect a file's modification time, even if the access time and/or creation time are not set.
…ry.DateTime instead

This prevents the two values from becoming potentially inconsistent.
@bastianeicher
Copy link
Contributor Author

Anything else I can do to help this PR get merged?

@piksel
Copy link
Member

piksel commented Jun 7, 2020

Sorry, I have this on my backlog to go through. I hope to get to it soon!

@piksel piksel self-assigned this Jun 7, 2020
Copy link
Member

@piksel piksel left a comment

Choose a reason for hiding this comment

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

LGTM

@piksel piksel merged commit 861a313 into icsharpcode:master Jun 19, 2020
@piksel piksel mentioned this pull request Aug 6, 2020
@piksel
Copy link
Member

piksel commented Aug 6, 2020

@bastianeicher Could you check my fix in #502? I changed the DateTime.Kind to Unspecified instead of UTC to prevent manually set dates from being shifted after "Serializing" them in an archive.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
zip Related to ZIP file format
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants