Fix restore_times option when extracting, and test file options

lib/zip/dos_time.rb

@@ -29,6 +29,11 @@ def dos_equals(other)
       to_i / 2 == other.to_i / 2
     end
 
+    # Create a DOSTime instance from a vanilla Time instance.
+    def self.from_time(time)
+      local(time.year, time.month, time.day, time.hour, time.min, time.sec)
+    end
+
     def self.parse_binary_dos_format(binaryDosDate, binaryDosTime)
       second = 2 * (0b11111 & binaryDosTime)
       minute = (0b11111100000 & binaryDosTime) >> 5

lib/zip/entry.rb

@@ -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)
     end
 
-    def set_unix_permissions_on_path(dest_path)
-      # BUG: does not update timestamps into account
+    def set_unix_attributes_on_path(dest_path)
       # ignore setuid/setgid bits by default.  honor if @restore_ownership
       unix_perms_mask = 0o1777
       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
-      # File::utimes()
+
+      # 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:
       return unless file? || directory?
 
       case @fstype
       when ::Zip::FSTYPE_UNIX
-        set_unix_permissions_on_path(dest_path)
+        set_unix_attributes_on_path(dest_path)
       end
     end
 
@@ -601,8 +605,6 @@ def create_file(dest_path, _continue_on_exists_proc = proc { Zip.continue_on_exi
       end
       ::File.open(dest_path, 'wb') do |os|
         get_input_stream do |is|
-          set_extra_attributes_on_path(dest_path)
-
           bytes_written = 0
           warned = false
           buf = ''.dup
@@ -621,6 +623,8 @@ def create_file(dest_path, _continue_on_exists_proc = proc { Zip.continue_on_exi
           end
         end
       end
+
+      set_extra_attributes_on_path(dest_path)
     end
 
     def create_directory(dest_path)

lib/zip/file.rb

@@ -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,
+      restore_times:       false
+    }.freeze
+
     attr_reader :name
 
-    # default -> false
+    # default -> false.
     attr_accessor :restore_ownership
-    # default -> false
+
+    # default -> false, but will be set to true in a future version.
     attr_accessor :restore_permissions
-    # default -> true
+
+    # default -> false, but will be set to true in a future version.
     attr_accessor :restore_times
+
     # Returns the zip files comment, if it has one
     attr_accessor :comment
 
     # Opens a zip archive. Pass true as the second parameter to create
     # a new archive if it doesn't exist already.
     def initialize(path_or_io, create = false, buffer = false, options = {})
       super()
+      options = DEFAULT_OPTIONS.merge(options)
       @name    = path_or_io.respond_to?(:path) ? path_or_io.path : path_or_io
       @comment = ''
       @create  = create ? true : false # allow any truthy value to mean true
@@ -98,9 +108,9 @@ def initialize(path_or_io, create = false, buffer = false, options = {})
 
       @stored_entries      = @entry_set.dup
       @stored_comment      = @comment
-      @restore_ownership   = options[:restore_ownership]    || false
-      @restore_permissions = options[:restore_permissions]  || true
-      @restore_times       = options[:restore_times]        || true
+      @restore_ownership   = options[:restore_ownership]
+      @restore_permissions = options[:restore_permissions]
+      @restore_times       = options[:restore_times]
     end
 
     class << self

test/file_options_test.rb

@@ -0,0 +1,89 @@
+require 'test_helper'
+
+class FileOptionsTest < MiniTest::Test
+  ZIPPATH = ::File.join(Dir.tmpdir, 'options.zip').freeze
+  TXTPATH = ::File.expand_path(::File.join('data', 'file1.txt'), __dir__).freeze
+  TXTPATH_600 = ::File.join(Dir.tmpdir, 'file1.600.txt').freeze
+  TXTPATH_755 = ::File.join(Dir.tmpdir, 'file1.755.txt').freeze
+  EXTPATH_1 = ::File.join(Dir.tmpdir, 'extracted_1.txt').freeze
+  EXTPATH_2 = ::File.join(Dir.tmpdir, 'extracted_2.txt').freeze
+  EXTPATH_3 = ::File.join(Dir.tmpdir, 'extracted_3.txt').freeze
+  ENTRY_1 = 'entry_1.txt'.freeze
+  ENTRY_2 = 'entry_2.txt'.freeze
+  ENTRY_3 = 'entry_3.txt'.freeze
+
+  def teardown
+    ::File.unlink(ZIPPATH) if ::File.exist?(ZIPPATH)
+    ::File.unlink(EXTPATH_1) if ::File.exist?(EXTPATH_1)
+    ::File.unlink(EXTPATH_2) if ::File.exist?(EXTPATH_2)
+    ::File.unlink(EXTPATH_3) if ::File.exist?(EXTPATH_3)
+    ::File.unlink(TXTPATH_600) if ::File.exist?(TXTPATH_600)
+    ::File.unlink(TXTPATH_755) if ::File.exist?(TXTPATH_755)
+  end
+
+  def test_restore_permissions
+    # Copy and set up files with different permissions.
+    ::FileUtils.cp(TXTPATH, TXTPATH_600)
+    ::File.chmod(0600, TXTPATH_600)
+    ::FileUtils.cp(TXTPATH, TXTPATH_755)
+    ::File.chmod(0755, 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: true) 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)
+      zip.add_stored(ENTRY_2, TXTPATH)
+    end
+
+    ::Zip::File.open(ZIPPATH, false, restore_times: true) 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_restore_times_false
+    ::Zip::File.open(ZIPPATH, true) do |zip|
+      zip.add(ENTRY_1, TXTPATH)
+      zip.add_stored(ENTRY_2, TXTPATH)
+    end
+
+    ::Zip::File.open(ZIPPATH, false, restore_times: false) do |zip|
+      zip.extract(ENTRY_1, EXTPATH_1)
+      zip.extract(ENTRY_2, EXTPATH_2)
+    end
+
+    assert_time_equal(::Time.now, ::File.mtime(EXTPATH_1))
+    assert_time_equal(::Time.now, ::File.mtime(EXTPATH_2))
+  end
+
+  private
+
+  # Method to compare file times. DOS times only have 2 second accuracy.
+  def assert_time_equal(expected, actual)
+    assert_equal(expected.year, actual.year)
+    assert_equal(expected.month, actual.month)
+    assert_equal(expected.day, actual.day)
+    assert_equal(expected.hour, actual.hour)
+    assert_equal(expected.min, actual.min)
+    assert_in_delta(expected.sec, actual.sec, 1)
+  end
+end