Repository: rubyzip/rubyzip (ruby)

Issue:
Options not working in File class
This code
https://github.com/rubyzip/rubyzip/blob/249775f5637e6d65112574b3ac1763dc2393c7f6/lib/zip/file.rb#L88-L89
doesn't do what it is intended to do, because when you pass in `restore_permissions: false` or `restore_times: false`, it will be evaluating to `false || true`, which will always be `true`


Discussion:
Interestingly the options code doesn't agree with the comments about defaults above either:

https://github.com/rubyzip/rubyzip/blob/94b7fa276992933592d69eb6bb17fc09105f8395/lib/zip/file.rb#L56-L61

vs

https://github.com/rubyzip/rubyzip/blob/94b7fa276992933592d69eb6bb17fc09105f8395/lib/zip/file.rb#L101-L103

I'll cook up a PR that fixes this issue and works as expected to keep the test passing - adjusting the comments as necessary.
Also, depressingly, it doesn't matter what those options are set to; the tests always all pass. I'll add some tests.
So, on further investigation, I don't think that `restore_times: true` can ever work, because I don't think that when you extract a file it ever even attempts to set the timestamp.

As far as I can tell, this is the code which does the actual extraction to a file:
https://github.com/rubyzip/rubyzip/blob/34d2074ecbf6ef8557c29efc03a585c309be5c4b/lib/zip/entry.rb#L597-L624
And there's nothing there that would restore times...

I'm looking at this in more depth.
Sorry, overzealous auto-closing. This is still a problem.

Patch:
diff --git a/lib/zip/extra_field/universal_time.rb b/lib/zip/extra_field/universal_time.rb
--- a/lib/zip/extra_field/universal_time.rb
+++ b/lib/zip/extra_field/universal_time.rb
@@ -4,24 +4,51 @@ class ExtraField::UniversalTime < ExtraField::Generic
     HEADER_ID = 'UT'
     register_map
 
+    ATIME_MASK = 0b010
+    CTIME_MASK = 0b100
+    MTIME_MASK = 0b001
+
     def initialize(binstr = nil)
       @ctime = nil
       @mtime = nil
       @atime = nil
-      @flag  = nil
-      binstr && merge(binstr)
+      @flag  = 0
+
+      merge(binstr) unless binstr.nil?
+    end
+
+    attr_reader :atime, :ctime, :mtime, :flag
+
+    def atime=(time)
+      @flag = time.nil? ? @flag & ~ATIME_MASK : @flag | ATIME_MASK
+      @atime = time
     end
 
-    attr_accessor :atime, :ctime, :mtime, :flag
+    def ctime=(time)
+      @flag = time.nil? ? @flag & ~CTIME_MASK : @flag | CTIME_MASK
+      @ctime = time
+    end
+
+    def mtime=(time)
+      @flag = time.nil? ? @flag & ~MTIME_MASK : @flag | MTIME_MASK
+      @mtime = time
+    end
 
     def merge(binstr)
       return if binstr.empty?
+
       size, content = initial_parse(binstr)
-      size || return
-      @flag, mtime, atime, ctime = content.unpack('CVVV')
-      mtime && @mtime ||= ::Zip::DOSTime.at(mtime)
-      atime && @atime ||= ::Zip::DOSTime.at(atime)
-      ctime && @ctime ||= ::Zip::DOSTime.at(ctime)
+      return if !size || size <= 0
+
+      @flag, *times = content.unpack('Cl<l<l<')
+
+      # Parse the timestamps, in order, based on which flags are set.
+      return if times[0].nil?
+      @mtime ||= ::Zip::DOSTime.at(times.shift) unless @flag & MTIME_MASK == 0
+      return if times[0].nil?
+      @atime ||= ::Zip::DOSTime.at(times.shift) unless @flag & ATIME_MASK == 0
+      return if times[0].nil?
+      @ctime ||= ::Zip::DOSTime.at(times.shift) unless @flag & CTIME_MASK == 0
     end
 
     def ==(other)
@@ -32,15 +59,15 @@ def ==(other)
 
     def pack_for_local
       s = [@flag].pack('C')
-      @flag & 1 != 0 && s << [@mtime.to_i].pack('V')
-      @flag & 2 != 0 && s << [@atime.to_i].pack('V')
-      @flag & 4 != 0 && s << [@ctime.to_i].pack('V')
+      s << [@mtime.to_i].pack('l<') unless @flag & MTIME_MASK == 0
+      s << [@atime.to_i].pack('l<') unless @flag & ATIME_MASK == 0
+      s << [@ctime.to_i].pack('l<') unless @flag & CTIME_MASK == 0
       s
     end
 
     def pack_for_c_dir
       s = [@flag].pack('C')
-      @flag & 1 == 1 && s << [@mtime.to_i].pack('V')
+      s << [@mtime.to_i].pack('l<') unless @flag & MTIME_MASK == 0
       s
     end
   end


Test patch:
diff --git a/test/extra_field_ut_test.rb b/test/extra_field_ut_test.rb
new file mode 100644
--- /dev/null
+++ b/test/extra_field_ut_test.rb
@@ -0,0 +1,97 @@
+require 'test_helper'
+
+class ZipExtraFieldUTTest < MiniTest::Test
+
+  PARSE_TESTS = [
+    ["UT\x05\x00\x01PS>A", 0b001, true, true, false],
+    ["UT\x05\x00\x02PS>A", 0b010, false, true, true],
+    ["UT\x05\x00\x04PS>A", 0b100, true, false, true],
+    ["UT\x09\x00\x03PS>APS>A", 0b011, false, true, false],
+    ["UT\x09\x00\x05PS>APS>A", 0b101, true, false, false],
+    ["UT\x09\x00\x06PS>APS>A", 0b110, false, false, true],
+    ["UT\x13\x00\x07PS>APS>APS>A", 0b111, false, false, false]
+  ]
+
+  def test_parse
+    PARSE_TESTS.each do |bin, flags, a, c, m|
+      ut = ::Zip::ExtraField::UniversalTime.new(bin)
+      assert_equal(flags, ut.flag)
+      assert(ut.atime.nil? == a)
+      assert(ut.ctime.nil? == c)
+      assert(ut.mtime.nil? == m)
+    end
+  end
+
+  def test_parse_size_zero
+    ut = ::Zip::ExtraField::UniversalTime.new("UT\x00")
+    assert_equal(0b000, ut.flag)
+    assert_nil(ut.atime)
+    assert_nil(ut.ctime)
+    assert_nil(ut.mtime)
+  end
+
+  def test_parse_size_nil
+    ut = ::Zip::ExtraField::UniversalTime.new('UT')
+    assert_equal(0b000, ut.flag)
+    assert_nil(ut.atime)
+    assert_nil(ut.ctime)
+    assert_nil(ut.mtime)
+  end
+
+  def test_parse_nil
+    ut = ::Zip::ExtraField::UniversalTime.new
+    assert_equal(0b000, ut.flag)
+    assert_nil(ut.atime)
+    assert_nil(ut.ctime)
+    assert_nil(ut.mtime)
+  end
+
+  def test_set_clear_times
+    time = ::Zip::DOSTime.now
+    ut = ::Zip::ExtraField::UniversalTime.new
+    assert_equal(0b000, ut.flag)
+
+    ut.mtime = time
+    assert_equal(0b001, ut.flag)
+    assert_equal(time, ut.mtime)
+
+    ut.ctime = time
+    assert_equal(0b101, ut.flag)
+    assert_equal(time, ut.ctime)
+
+    ut.atime = time
+    assert_equal(0b111, ut.flag)
+    assert_equal(time, ut.atime)
+
+    ut.ctime = nil
+    assert_equal(0b011, ut.flag)
+    assert_nil ut.ctime
+
+    ut.mtime = nil
+    assert_equal(0b010, ut.flag)
+    assert_nil ut.mtime
+
+    ut.atime = nil
+    assert_equal(0b000, ut.flag)
+    assert_nil ut.atime
+  end
+
+  def test_pack
+    time = ::Zip::DOSTime.at('PS>A'.unpack1('l<'))
+    ut = ::Zip::ExtraField::UniversalTime.new
+    assert_equal("\x00", ut.pack_for_local)
+    assert_equal("\x00", ut.pack_for_c_dir)
+
+    ut.mtime = time
+    assert_equal("\x01PS>A", ut.pack_for_local)
+    assert_equal("\x01PS>A", ut.pack_for_c_dir)
+
+    ut.atime = time
+    assert_equal("\x03PS>APS>A", ut.pack_for_local)
+    assert_equal("\x03PS>A", ut.pack_for_c_dir)
+
+    ut.ctime = time
+    assert_equal("\x07PS>APS>APS>A", ut.pack_for_local)
+    assert_equal("\x07PS>A", ut.pack_for_c_dir)
+  end
+end