Repository: rubyzip/rubyzip (ruby)

Issue:
Inconsistent behaviour of `Zip::File#get_entry` and `Zip::File#find_entry` exposes options out of sync.
This is a funny one that has been exposed by fixing #395...

The defaults for `restore_times`, `restore_permissions` and `restore_ownership` are separately specified in both `Zip::File` and `Zip::Entry` and they are out of sync.

Current defaults for each option:

| Option/Class | `Zip::File` | `Zip::Entry` |
|--------:|:------------:|:--------------:|
| `restore_times` | `false` | `true` |
| `restore_permissions` | `false` | `false` |
| `restore_ownership` | `false` | `false` |

This isn't generally an issue as whatever is set in `Zip::File` is percolated through to `Zip::Entry` - except not always :grimacing:

The following example shows how this can get confused:

```ruby
zip = ::Zip::File.open('test/data/globTest.zip')

e1 = zip.find_entry('globTest/food.txt')
e1.extract('f_test.txt')

e2 = zip.get_entry('globTest/food.txt')
e2.extract('g_test.txt')

zip.close
```

`f_test.txt` has a timestamp of 18 May 2012; `g_test.txt` has the current time and date for its timestamp.

This all occurs because the default for the above options are stored twice - and (kind of) need to be due to there not always being a `Zip::File` parent for every `Zip::Entry` - but also because while `Zip::File#get_entry` and `Zip::File#find_entry` are ostensibly doing the same thing, they are implemented very differently.

The quick fix for this is fairly easy, and I'll work up a PR for it later.

The main reason I am documenting this in a detailed issue is to ask: should these options be moved up to the top level, alongside `validate_entry_sizes`, _et al_? Or do we leave them where they are and risk them being out of sync again in the future? @jdleesmiller, @simonoff, I'd appreciate your thoughts, but I reckon moving them up is the right way to go - this would have the happy side-effect of simplifying various interfaces in `Zip::File`. Happy to also work this up as a PR if you agree.
View and/or preserve original date created, date modified?
For files inside the zip file, is there any way in rubyzip to access their original dates created and modified? If I extract them, Windows shows them as having been created and modified at the time of the extraction, which is quite different from the dates showing inside the zip file. I need to be able to list the dates they were originally created and last modified and, ideally, preserve those dates with the extracted files.

I have searched for this answer both here and on the Web, and the only references to solutions I can find use DotNetZip or Python. I would like to use Ruby.

Thanks.



Discussion:
Good find. PR approved.

> should these options be moved up to the top level

I feel like the same (large) application might want to treat zips differently in this regard for one feature vs another, so I am not sure about moving them to top level.

A small improvement might be to move the hash with the defaults to top level and then use it in both `Zip::File` and `Zip::Entry` to look up the defaults. That eliminates one way in which they can get out of sync, but it does still require `File` to remember to always pass them to its `Entry`s.
This maybe another instance of behaviour seen in #465, but also could be something that will be fixed by fixing #422. Either way, I'm looking into it (finally).

Patch:
diff --git a/Changelog.md b/Changelog.md
--- a/Changelog.md
+++ b/Changelog.md
@@ -1,5 +1,7 @@
 # 3.0.0 (Next)
 
+- Fix restore options consistency. [#486](https://github.com/rubyzip/rubyzip/pull/486)
+- View and/or preserve original date created, date modified? (Windows). [#336](https://github.com/rubyzip/rubyzip/issues/336)
 - Fix frozen string literal error. [#475](https://github.com/rubyzip/rubyzip/pull/475)
 - Set the default `Entry` time to the file's mtime on Windows. [#465](https://github.com/rubyzip/rubyzip/issues/465)
 - Ensure that `Entry#time=` sets times as `DOSTime` objects. [#481](https://github.com/rubyzip/rubyzip/issues/481)
diff --git a/lib/zip.rb b/lib/zip.rb
--- a/lib/zip.rb
+++ b/lib/zip.rb
@@ -48,6 +48,12 @@ module Zip
                 :force_entry_names_encoding,
                 :validate_entry_sizes
 
+  DEFAULT_RESTORE_OPTIONS = {
+    restore_ownership:   false,
+    restore_permissions: true,
+    restore_times:       true
+  }.freeze
+
   def reset!
     @_ran_once = false
     @unicode_names = false
diff --git a/lib/zip/entry.rb b/lib/zip/entry.rb
--- a/lib/zip/entry.rb
+++ b/lib/zip/entry.rb
@@ -42,9 +42,9 @@ def set_default_vars_values
       end
       @follow_symlinks = false
 
-      @restore_times       = false
-      @restore_permissions = false
-      @restore_ownership   = false
+      @restore_times       = DEFAULT_RESTORE_OPTIONS[:restore_times]
+      @restore_permissions = DEFAULT_RESTORE_OPTIONS[:restore_permissions]
+      @restore_ownership   = DEFAULT_RESTORE_OPTIONS[:restore_ownership]
       # BUG: need an extra field to support uid/gid's
       @unix_uid            = nil
       @unix_gid            = nil
@@ -462,11 +462,6 @@ def set_unix_attributes_on_path(dest_path)
       unix_perms_mask = 0o7777 if @restore_ownership
       ::FileUtils.chmod(@unix_perms & unix_perms_mask, dest_path) if @restore_permissions && @unix_perms
       ::FileUtils.chown(@unix_uid, @unix_gid, dest_path) if @restore_ownership && @unix_uid && @unix_gid && ::Process.egid == 0
-
-      # 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
     end
 
     def set_extra_attributes_on_path(dest_path) # :nodoc:
@@ -476,6 +471,11 @@ def set_extra_attributes_on_path(dest_path) # :nodoc:
       when ::Zip::FSTYPE_UNIX
         set_unix_attributes_on_path(dest_path)
       end
+
+      # 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
     end
 
     def pack_c_dir_entry
diff --git a/lib/zip/file.rb b/lib/zip/file.rb
--- a/lib/zip/file.rb
+++ b/lib/zip/file.rb
@@ -53,21 +53,15 @@ class File < CentralDirectory
     DATA_BUFFER_SIZE     = 8192
     IO_METHODS           = [:tell, :seek, :read, :eof, :close].freeze
 
-    DEFAULT_OPTIONS = {
-      restore_ownership:   false,
-      restore_permissions: false,
-      restore_times:       false
-    }.freeze
-
     attr_reader :name
 
     # default -> false.
     attr_accessor :restore_ownership
 
-    # default -> false, but will be set to true in a future version.
+    # default -> true.
     attr_accessor :restore_permissions
 
-    # default -> false, but will be set to true in a future version.
+    # default -> true.
     attr_accessor :restore_times
 
     # Returns the zip files comment, if it has one
@@ -77,7 +71,7 @@ class File < CentralDirectory
     # a new archive if it doesn't exist already.
     def initialize(path_or_io, create = false, buffer = false, options = {})
       super()
-      options  = DEFAULT_OPTIONS
+      options  = DEFAULT_RESTORE_OPTIONS
                  .merge(compression_level: ::Zip.default_compression)
                  .merge(options)
       @name    = path_or_io.respond_to?(:path) ? path_or_io.path : path_or_io


Test patch:
diff --git a/test/file_options_test.rb b/test/file_options_test.rb
--- a/test/file_options_test.rb
+++ b/test/file_options_test.rb
@@ -23,7 +23,7 @@ def teardown
     ::File.unlink(TXTPATH_755) if ::File.exist?(TXTPATH_755)
   end
 
-  def test_restore_permissions
+  def test_restore_permissions_true
     # Copy and set up files with different permissions.
     ::FileUtils.cp(TXTPATH, TXTPATH_600)
     ::File.chmod(0o600, TXTPATH_600)
@@ -47,6 +47,55 @@ def test_restore_permissions
     assert_equal(::File.stat(TXTPATH_755).mode, ::File.stat(EXTPATH_3).mode)
   end
 
+  def test_restore_permissions_false
+    # Copy and set up files with different permissions.
+    ::FileUtils.cp(TXTPATH, TXTPATH_600)
+    ::File.chmod(0o600, TXTPATH_600)
+    ::FileUtils.cp(TXTPATH, TXTPATH_755)
+    ::File.chmod(0o755, TXTPATH_755)
+
+    ::Zip::File.open(ZIPPATH, true) do |zip|
+      zip.add(ENTRY_1, TXTPATH)
+      zip.add(ENTRY_2, TXTPATH_600)
+      zip.add(ENTRY_3, TXTPATH_755)
+    end
+
+    ::Zip::File.open(ZIPPATH, false, restore_permissions: false) do |zip|
+      zip.extract(ENTRY_1, EXTPATH_1)
+      zip.extract(ENTRY_2, EXTPATH_2)
+      zip.extract(ENTRY_3, EXTPATH_3)
+    end
+
+    default_perms = (Zip::RUNNING_ON_WINDOWS ? 0o100_644 : 0o100_666) - ::File.umask
+    assert_equal(default_perms, ::File.stat(EXTPATH_1).mode)
+    assert_equal(default_perms, ::File.stat(EXTPATH_2).mode)
+    assert_equal(default_perms, ::File.stat(EXTPATH_3).mode)
+  end
+
+  def test_restore_permissions_as_default
+    # Copy and set up files with different permissions.
+    ::FileUtils.cp(TXTPATH, TXTPATH_600)
+    ::File.chmod(0o600, TXTPATH_600)
+    ::FileUtils.cp(TXTPATH, TXTPATH_755)
+    ::File.chmod(0o755, TXTPATH_755)
+
+    ::Zip::File.open(ZIPPATH, true) do |zip|
+      zip.add(ENTRY_1, TXTPATH)
+      zip.add(ENTRY_2, TXTPATH_600)
+      zip.add(ENTRY_3, TXTPATH_755)
+    end
+
+    ::Zip::File.open(ZIPPATH) do |zip|
+      zip.extract(ENTRY_1, EXTPATH_1)
+      zip.extract(ENTRY_2, EXTPATH_2)
+      zip.extract(ENTRY_3, EXTPATH_3)
+    end
+
+    assert_equal(::File.stat(TXTPATH).mode, ::File.stat(EXTPATH_1).mode)
+    assert_equal(::File.stat(TXTPATH_600).mode, ::File.stat(EXTPATH_2).mode)
+    assert_equal(::File.stat(TXTPATH_755).mode, ::File.stat(EXTPATH_3).mode)
+  end
+
   def test_restore_times_true
     ::Zip::File.open(ZIPPATH, true) do |zip|
       zip.add(ENTRY_1, TXTPATH)
@@ -58,10 +107,6 @@ def test_restore_times_true
       zip.extract(ENTRY_2, EXTPATH_2)
     end
 
-    # this test is disabled on Windows for now, waiting for #486.
-    # please remove this after merging #486.
-    skip if Zip::RUNNING_ON_WINDOWS
-
     assert_time_equal(::File.mtime(TXTPATH), ::File.mtime(EXTPATH_1))
     assert_time_equal(::File.mtime(TXTPATH), ::File.mtime(EXTPATH_2))
   end
@@ -81,6 +126,21 @@ def test_restore_times_false
     assert_time_equal(::Time.now, ::File.mtime(EXTPATH_2))
   end
 
+  def test_restore_times_true_as_default
+    ::Zip::File.open(ZIPPATH, true) do |zip|
+      zip.add(ENTRY_1, TXTPATH)
+      zip.add_stored(ENTRY_2, TXTPATH)
+    end
+
+    ::Zip::File.open(ZIPPATH) do |zip|
+      zip.extract(ENTRY_1, EXTPATH_1)
+      zip.extract(ENTRY_2, EXTPATH_2)
+    end
+
+    assert_time_equal(::File.mtime(TXTPATH), ::File.mtime(EXTPATH_1))
+    assert_time_equal(::File.mtime(TXTPATH), ::File.mtime(EXTPATH_2))
+  end
+
   def test_get_find_consistency
     testzip = ::File.expand_path(::File.join('data', 'globTest.zip'), __dir__)
     file_f = ::File.expand_path('f_test.txt', Dir.tmpdir)