From 7cd263e6a0d53f793ba41b249f4a78f84c4dc016 Mon Sep 17 00:00:00 2001 From: Robert Haines Date: Tue, 3 Apr 2018 23:45:04 +0100 Subject: [PATCH 1/5] Clean up use of file_name in Zip::File::initialize. --- lib/zip/file.rb | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/lib/zip/file.rb b/lib/zip/file.rb index 6952ba99..6ff3463d 100644 --- a/lib/zip/file.rb +++ b/lib/zip/file.rb @@ -69,18 +69,18 @@ def initialize(file_name, create = false, buffer = false, options = {}) @name = file_name @comment = '' @create = create ? true : false # allow any truthy value to mean true - if !buffer && ::File.size?(file_name) + if !buffer && ::File.size?(@name) @create = false - @file_permissions = ::File.stat(file_name).mode - ::File.open(name, 'rb') do |f| + @file_permissions = ::File.stat(@name).mode + ::File.open(@name, 'rb') do |f| read_from_stream(f) end elsif @create @entry_set = EntrySet.new - elsif ::File.zero?(file_name) - raise Error, "File #{file_name} has zero size. Did you mean to pass the create flag?" + elsif ::File.zero?(@name) + raise Error, "File #{@name} has zero size. Did you mean to pass the create flag?" else - raise Error, "File #{file_name} not found" + raise Error, "File #{@name} not found" end @stored_entries = @entry_set.dup @stored_comment = @comment From cfa9441914d56bb866dd70c29fd5ec33bd2a8fc6 Mon Sep 17 00:00:00 2001 From: Robert Haines Date: Tue, 3 Apr 2018 23:48:54 +0100 Subject: [PATCH 2/5] Handle passing an IO to Zip::File.new better. This now actually extracts the path from the IO if one is passed in. --- lib/zip/file.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/zip/file.rb b/lib/zip/file.rb index 6ff3463d..8cb5f03a 100644 --- a/lib/zip/file.rb +++ b/lib/zip/file.rb @@ -64,9 +64,9 @@ class File < CentralDirectory # Opens a zip archive. Pass true as the second parameter to create # a new archive if it doesn't exist already. - def initialize(file_name, create = false, buffer = false, options = {}) + def initialize(path_or_io, create = false, buffer = false, options = {}) super() - @name = file_name + @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 if !buffer && ::File.size?(@name) From 03633933ebb714cdff1e3f5604434f35bad0e0cf Mon Sep 17 00:00:00 2001 From: Robert Haines Date: Wed, 4 Apr 2018 14:31:34 +0100 Subject: [PATCH 3/5] No need to require stringio in Zip::File.open_buffer. It's already required in zip.rb. --- lib/zip/file.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/zip/file.rb b/lib/zip/file.rb index 8cb5f03a..dc86bb72 100644 --- a/lib/zip/file.rb +++ b/lib/zip/file.rb @@ -120,7 +120,6 @@ def open_buffer(io, options = {}) raise "Zip::File.open_buffer expects a String or IO-like argument (responds to #{IO_METHODS.join(', ')}). Found: #{io.class}" end if io.is_a?(::String) - require 'stringio' io = ::StringIO.new(io) elsif io.respond_to?(:binmode) # https://github.com/rubyzip/rubyzip/issues/119 From 15ccc25da1755200f6d2cb29fde49c303a236073 Mon Sep 17 00:00:00 2001 From: Robert Haines Date: Wed, 4 Apr 2018 15:12:22 +0100 Subject: [PATCH 4/5] Fix File.open_buffer when no changes are made. Things are now more carefully set up, and if a buffer is passed in which represents a file that already exists then this is taken into account. All initialization is now done in File.new, rather than being split between there and File.open_buffer. This has also needed a bit of a re-write of Zip::File.initialize. I've tried to bring some logic to it as a result, and have added comments to explain what is now happening. --- lib/zip/file.rb | 24 ++++++++++++++++++++---- test/file_test.rb | 5 +++-- 2 files changed, 23 insertions(+), 6 deletions(-) diff --git a/lib/zip/file.rb b/lib/zip/file.rb index dc86bb72..c0a44207 100644 --- a/lib/zip/file.rb +++ b/lib/zip/file.rb @@ -69,19 +69,33 @@ def initialize(path_or_io, create = false, buffer = false, 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 - if !buffer && ::File.size?(@name) + + if ::File.size?(@name.to_s) + # There is a file, which exists, that is associated with this zip. @create = false @file_permissions = ::File.stat(@name).mode - ::File.open(@name, 'rb') do |f| - read_from_stream(f) + + if buffer + read_from_stream(path_or_io) + else + ::File.open(@name, 'rb') do |f| + read_from_stream(f) + end end + elsif buffer && path_or_io.size > 0 + # This zip is probably a non-empty StringIO. + read_from_stream(path_or_io) elsif @create + # This zip is completely new/empty and is to be created. @entry_set = EntrySet.new elsif ::File.zero?(@name) + # A file exists, but it is empty. raise Error, "File #{@name} has zero size. Did you mean to pass the create flag?" else + # Everything is wrong. raise Error, "File #{@name} not found" end + @stored_entries = @entry_set.dup @stored_comment = @comment @restore_ownership = options[:restore_ownership] || false @@ -119,16 +133,18 @@ def open_buffer(io, options = {}) unless IO_METHODS.map { |method| io.respond_to?(method) }.all? || io.is_a?(String) raise "Zip::File.open_buffer expects a String or IO-like argument (responds to #{IO_METHODS.join(', ')}). Found: #{io.class}" end + if io.is_a?(::String) io = ::StringIO.new(io) elsif io.respond_to?(:binmode) # https://github.com/rubyzip/rubyzip/issues/119 io.binmode end + zf = ::Zip::File.new(io, true, true, options) - zf.read_from_stream(io) return zf unless block_given? yield zf + begin zf.write_buffer(io) rescue IOError => e diff --git a/test/file_test.rb b/test/file_test.rb index 32e21e33..53124bea 100644 --- a/test/file_test.rb +++ b/test/file_test.rb @@ -107,13 +107,14 @@ def test_open_buffer_with_stringio def test_close_buffer_with_stringio string_io = StringIO.new File.read('test/data/rubycode.zip') zf = ::Zip::File.open_buffer string_io - assert(zf.close || true) # Poor man's refute_raises + assert_nil zf.close end def test_close_buffer_with_io f = File.open('test/data/rubycode.zip') zf = ::Zip::File.open_buffer f - assert zf.close + refute zf.commit_required? + assert_nil zf.close f.close end From 84c208982f717f10f7133cdfc1f016607398858d Mon Sep 17 00:00:00 2001 From: Robert Haines Date: Wed, 4 Apr 2018 15:40:38 +0100 Subject: [PATCH 5/5] Switch newly created StringIOs to binmode. StringIO objects created within File.open_buffer were not being switched into binmode, but those passed in were. Fix this inconsistency and add a test. --- lib/zip/file.rb | 10 ++++------ test/file_test.rb | 7 +++++++ 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/lib/zip/file.rb b/lib/zip/file.rb index c0a44207..b5b85eea 100644 --- a/lib/zip/file.rb +++ b/lib/zip/file.rb @@ -134,12 +134,10 @@ def open_buffer(io, options = {}) raise "Zip::File.open_buffer expects a String or IO-like argument (responds to #{IO_METHODS.join(', ')}). Found: #{io.class}" end - if io.is_a?(::String) - io = ::StringIO.new(io) - elsif io.respond_to?(:binmode) - # https://github.com/rubyzip/rubyzip/issues/119 - io.binmode - end + io = ::StringIO.new(io) if io.is_a?(::String) + + # https://github.com/rubyzip/rubyzip/issues/119 + io.binmode if io.respond_to?(:binmode) zf = ::Zip::File.new(io, true, true, options) return zf unless block_given? diff --git a/test/file_test.rb b/test/file_test.rb index 53124bea..8bbf7cf8 100644 --- a/test/file_test.rb +++ b/test/file_test.rb @@ -97,6 +97,13 @@ def test_get_output_stream end end + def test_open_buffer_with_string + string = File.read('test/data/rubycode.zip') + ::Zip::File.open_buffer string do |zf| + assert zf.entries.map { |e| e.name }.include?('zippedruby1.rb') + end + end + def test_open_buffer_with_stringio string_io = StringIO.new File.read('test/data/rubycode.zip') ::Zip::File.open_buffer string_io do |zf|