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 restore_times option when extracting, and test file options #413

Merged
merged 5 commits into from Oct 27, 2019

Conversation

hainesr
Copy link
Member

@hainesr hainesr commented Oct 7, 2019

This PR was prompted by #395. When investigating that issue it quickly became obvious that there were deeper problems - as can be seen in the notes for that ticket.

This PR:

  • Correctly applies the options specified in ::Zip::File.new, and supplies defaults for any not given.
  • Fixes the restore_times option by actually storing the original timestamp of a file, and then optionally restoring it on extraction. It also adds tests for this functionality.
  • Adds tests for the restore_permissions option.
  • Does not add tests for the restore_ownership option. This needs some thinking about because restoring ownership would require other users to be set up on the test machine, and would also potentially require such tests to be run as root.

They are fairly simple fixes, once one has one's head around what is going on in entry.rb, but it would be good to have another pair of eyes look at this just in case, @jdleesmiller.

@coveralls
Copy link

coveralls commented Oct 7, 2019

Coverage Status

Coverage increased (+0.01%) to 95.455% when pulling 1a21f39 on hainesr:file-options into e43e360 on rubyzip:master.

Copy link
Member

@jdleesmiller jdleesmiller left a comment

Thanks for taking this on! I think the rabbit hole may go deeper still, as described in the review comments.

Most of the complexity here comes from implementing the missing timestamp support. I think just fixing the default check for restore_permissions would be a viable PR in and of itself, if we'd like to take the quicker win first. :)

lib/zip/file.rb Outdated
DEFAULT_OPTIONS = {
restore_ownership: false,
restore_permissions: false,
restore_times: true
Copy link
Member

@jdleesmiller jdleesmiller Oct 19, 2019

Choose a reason for hiding this comment

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

While the default value for restore_times was notionally true in the original code, it didn't actually do anything (as you discovered), so it effectively defaulted to false. For compatibility, perhaps this should default false for now?

Copy link
Member Author

@hainesr hainesr Oct 19, 2019

Choose a reason for hiding this comment

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

I think that restoring the original times probably should be the eventual default - I've seen use-cases where the whole point of zipping a directory up and transferring it was to preserve the timestamps.

But I take your point that it might be better to default to false for now. I'll change that.

@@ -51,21 +51,31 @@ class File < CentralDirectory
DATA_BUFFER_SIZE = 8192
IO_METHODS = [:tell, :seek, :read, :close]

DEFAULT_OPTIONS = {
restore_ownership: false,
restore_permissions: false,
Copy link
Member

@jdleesmiller jdleesmiller Oct 19, 2019

Choose a reason for hiding this comment

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

It looks like this used to default to true (and, unlike restore_times, did actually do something). Is there a particular reason to default it to false now?

Copy link
Member Author

@hainesr hainesr Oct 19, 2019

Choose a reason for hiding this comment

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

It did default to true, but it didn't do anything! I forgot to mention this in the original PR.

In Entry#create_file the call to set the attributes of a file (set_extra_attributes_on_path) was being called too early - before the file was closed - so the permissions weren't being set correctly. In the course of my PR I moved the call to set_extra_attributes_on_path to when the file is closed to fix this. (Entry#create_directory got this right already.)

Then I set this to be be false by default for consistency. I certainly think that permissions should be restored by default though.

I'm happy to change this to true while I'm making the other changes required.

Copy link
Member

@jdleesmiller jdleesmiller Oct 19, 2019

Choose a reason for hiding this comment

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

Aha, I didn't pick up on that. 👍 for false in that case. Edit: I think it makes sense to maintain compatibility initially but then in a future (major?) version bump change the defaults.

Copy link
Member Author

@hainesr hainesr Oct 19, 2019

Choose a reason for hiding this comment

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

OK, I will leave it set to false for now then.

@@ -406,24 +406,28 @@ def get_extra_attributes_from_path(path) # :nodoc:
@unix_uid = stat.uid
@unix_gid = stat.gid
@unix_perms = stat.mode & 0o7777
@time = ::Zip::DOSTime.from_time(stat.mtime)
Copy link
Member

@jdleesmiller jdleesmiller Oct 19, 2019

Choose a reason for hiding this comment

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

  • This is currently guarded by return if Zip::RUNNING_ON_WINDOWS. I am pretty sure that the modification time would also be available on Windows, and from what I can see in the documentation, File::stat should still work. I have been meaning to find a Windows machine to test this on... I guess even if the restore_times only works on *nix, it would still be an improvement.

  • What do you think to using time= rather than assigning the @time instance variable directly? It looks like that would also set some related attributes in @extra, which seems desirable.

Copy link
Member Author

@hainesr hainesr Oct 19, 2019

Choose a reason for hiding this comment

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

Good point, I will try using time=. TBH working through how all this works was horribly convoluted so I must have just missed this subtlety 😬

Copy link
Member Author

@hainesr hainesr Oct 19, 2019

Choose a reason for hiding this comment

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

Urgh. So I don't think the UniversalTime extra field can work as currently implemented. In Entry#time= when it creates a UniversalTime entry in @extra the .flag field is set to nil. But the UniversalTime field can only be packed if @flag is non-nil.

Going through the tests for the extra field code it seems that @flag is always set to 3 for UniversalTime, and indeed forcing this does work. But I need to look and see if this reasonable.

It's late now, so I'll pick this up another time.

Copy link
Member Author

@hainesr hainesr Oct 20, 2019

Choose a reason for hiding this comment

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

Right. I think given the issues with using time= at the moment, I think it's best to keep using @time for now.

I've found documentation for the UniversalTime extra field now, so I think I can fix it in a different PR once I've properly got my head round it.

I also think, now I've seen what adding the extra fields does to the archive, that having a way of not using extra fields by default (as suggested in #398) would be a good idea. So I'll have a think about that too.

I'll finish up making the other changes to this PR we've already discussed this evening and update it.

# Restore the timestamp on a file. This will either have come from the
# original source file that was copied into the archive, or from the
# creation date of the archive if there was no original source file.
::FileUtils.touch(dest_path, mtime: time) if @restore_times
Copy link
Member

@jdleesmiller jdleesmiller Oct 19, 2019

Choose a reason for hiding this comment

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

Possibly one for a future PR... but we could use File::utime here as the original comment hinted, if we used the atime and ctime from the UniversalTime extra field. (And we could store those from File::stat, too.)

Copy link
Member Author

@hainesr hainesr Oct 19, 2019

Choose a reason for hiding this comment

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

I think I used mtime because that is the only one FileUtils.touch supports. But now I look at the code for FileUtils.touch it turns out it uses File.utime internally anyway [1], setting atime and mtime to the same thing.

So, I agree, shall we leave this as is for now and look at it in a new PR?

[1] https://ruby-doc.org/stdlib-2.4.7/libdoc/fileutils/rdoc/FileUtils.html#method-c-touch

Copy link
Member

@jdleesmiller jdleesmiller Oct 19, 2019

Choose a reason for hiding this comment

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

Good points. I am definitely OK with that 👍.

hainesr added 2 commits Oct 19, 2019
Fixes rubyzip#395. Set the options to false for now for consistency.
Note what the default is and that a couple of them will change at some
point soon.
hainesr added 3 commits Oct 20, 2019
There has been an option in `Zip::File` (`:restore_times`) for a long
time, but it seems it has never worked. Firstly the actual timestamp of
an added file wasn't being saved, and secondly an extracted file wasn't
having its timestamp set correctly.

This commit fixes both of those issues, and adds tests to make sure.
DOSTime::from_time creates a DOSTime instance from a vanilla Time
instance.
@hainesr
Copy link
Member Author

hainesr commented Oct 20, 2019

I have made the changes we discussed above - apart from the switch to using time= for now as explained - and rebased everything onto the tip of master.

Copy link
Member

@jdleesmiller jdleesmiller left a comment

Looks good 👍

Thanks for the digging on time=.

@jdleesmiller jdleesmiller merged commit c9b6523 into rubyzip:master Oct 27, 2019
2 checks passed
@hainesr hainesr deleted the file-options branch Oct 27, 2019
jdleesmiller added a commit that referenced this pull request Oct 27, 2019
Copy link

@tronable1 tronable1 left a comment

Changes not made others are not accepted LICENSES

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.

None yet

4 participants