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

Fixes for the UniversalTime extra field #421

Merged
merged 6 commits into from Dec 27, 2019

Conversation

hainesr
Copy link
Member

@hainesr hainesr commented Oct 30, 2019

When working on #413 to fix #395 I discovered that there were a few problems with how the timestamps were being stored - particularly with the UniversalTime extra field. On even closer inspection, and after having consulted the UniversalTime specification (ftp://ftp.info-zip.org/pub/infozip/doc/), I found further issues.

This PR fixes those issues and makes the UniversalTime class more user-friendly. It also adds tests.

At a later date I will fold this in so that it is used to store and restore the timestamps of files, but I've left this for now as it has the side-effect of adding data (that might be unexpected/unwanted) between the filename and file data. In short, I wonder if we should allow some control over which extra fields are used, rather than inserting them by default, c.f. #398.

@coveralls
Copy link

coveralls commented Oct 30, 2019

Coverage Status

Coverage increased (+0.03%) to 95.494% when pulling ee028d2 on hainesr:universal-time into d3027ea on rubyzip:master.

@hainesr
Copy link
Member Author

hainesr commented Nov 30, 2019

Sorry to bump this, but would you mind having a look @jdleesmiller?

Copy link
Member

@jdleesmiller jdleesmiller left a comment

Sorry for the slow turnaround. Nice improvements 👍. Just a few comments.

lib/zip/extra_field/universal_time.rb Outdated Show resolved Hide resolved
lib/zip/extra_field/universal_time.rb Outdated Show resolved Hide resolved
lib/zip/extra_field/universal_time.rb Outdated Show resolved Hide resolved
hainesr added 6 commits Dec 15, 2019
When a timestamp is set/unset the flags should reflect this.
From the documentation: "The time values are in standard Unix signed-long
format, indicating the number of seconds since 1 January 1970 00:00:00."

The three time values were being unpacked with 'VVV', which is unsigned
32-bit, but they should be unpacked with 'l<l<l<': signed-long little
endian.
From the documentation: "...times that are present will appear in the
order indicated, but any combination of times may be omitted. (Creation
time may be present without access time, for example.)"

This fixes the parsing so that the times are read into the correct
fields, according to the flags. Before they were simply assumed to be in
order and all present.
@hainesr
Copy link
Member Author

hainesr commented Dec 15, 2019

Thanks for the review. I have made the above fixes and have rebased onto current master.

Copy link
Member

@jdleesmiller jdleesmiller left a comment

Nice 👍 . Looks good.

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.

3 participants