Fixes for the UniversalTime extra field #421
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Sorry to bump this, but would you mind having a look @jdleesmiller? |
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.
ee0a718 to
ee028d2
Compare
|
Thanks for the review. I have made the above fixes and have rebased onto current |
jdleesmiller
added a commit
that referenced
this pull request
Dec 27, 2019
This was referenced Mar 11, 2021
This was referenced Mar 15, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
UniversalTimeclass 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.