From 03620e388cbb61272f37940716acf6d1b2f5b87e Mon Sep 17 00:00:00 2001 From: Ivan Kuchin Date: Tue, 25 Aug 2020 00:34:59 +0200 Subject: [PATCH 1/3] don't create a temporary file in destination directory for atomic replacement if temporary directory is on same device as destination, part of solution for #178 --- CHANGELOG.markdown | 2 + lib/image_optim/cache_path.rb | 7 ++- lib/image_optim/path.rb | 21 ++++++--- spec/image_optim/cache_path_spec.rb | 68 ++++++++++++++++++----------- spec/image_optim/path_spec.rb | 68 ++++++++++++++++++----------- 5 files changed, 109 insertions(+), 57 deletions(-) diff --git a/CHANGELOG.markdown b/CHANGELOG.markdown index cf84ca2c..a2379b11 100644 --- a/CHANGELOG.markdown +++ b/CHANGELOG.markdown @@ -2,6 +2,8 @@ ## unreleased +* Don't create a temporary file in destination directory for atomic replacement if temporary directory is on same device as destination [#178](https://github.com/toy/image_optim/issues/178) [@toy](https://github.com/toy) + ## v0.26.5 (2019-07-14) * Remove deprecated `rubyforge_project` attribute from gemspec [rubygems/rubygems#2436](https://github.com/rubygems/rubygems/pull/2436) [@toy](https://github.com/toy) diff --git a/lib/image_optim/cache_path.rb b/lib/image_optim/cache_path.rb index 0752b2c4..522ed9b6 100644 --- a/lib/image_optim/cache_path.rb +++ b/lib/image_optim/cache_path.rb @@ -7,8 +7,11 @@ class ImageOptim class CachePath < Path # Atomic replace dst with self def replace(dst) - dst = self.class.new(dst) - dst.temp_path(dst.dirname) do |temp| + dst = self.class.convert(dst) + tmpdir = [dirname, Path.new(Dir.tmpdir)].find do |dir| + dir.same_dev?(dst.dirname) + end + dst.temp_path(tmpdir || dst.dirname) do |temp| copy(temp) dst.copy_metadata(temp) temp.rename(dst.to_s) diff --git a/lib/image_optim/path.rb b/lib/image_optim/path.rb index e4f8dcf0..56705df4 100644 --- a/lib/image_optim/path.rb +++ b/lib/image_optim/path.rb @@ -50,11 +50,16 @@ def copy_metadata(dst, time = false) # Atomic replace dst with self def replace(dst) - dst = self.class.new(dst) - dst.temp_path(dst.dirname) do |temp| - move(temp) - dst.copy_metadata(temp) - temp.rename(dst.to_s) + dst = self.class.convert(dst) + if same_dev?(dst.dirname) + dst.copy_metadata(self) + rename(dst.to_s) + else + dst.temp_path(dst.dirname) do |temp| + move(temp) + dst.copy_metadata(temp) + temp.rename(dst.to_s) + end end end @@ -68,5 +73,11 @@ def image_format def self.convert(path) path.is_a?(self) ? path : new(path) end + + protected + + def same_dev?(other) + stat.dev == other.stat.dev + end end end diff --git a/spec/image_optim/cache_path_spec.rb b/spec/image_optim/cache_path_spec.rb index e12b4bd9..ab7ed243 100644 --- a/spec/image_optim/cache_path_spec.rb +++ b/spec/image_optim/cache_path_spec.rb @@ -15,45 +15,63 @@ let(:src){ CachePath.temp_file_path } let(:dst){ CachePath.temp_file_path } - it 'moves data to destination' do - src.write('src') + shared_examples 'replaces file' do + it 'moves data to destination' do + src.write('src') - src.replace(dst) + src.replace(dst) - expect(dst.read).to eq('src') - end + expect(dst.read).to eq('src') + end - it 'does not remove original file' do - src.replace(dst) + it 'does not remove original file' do + src.replace(dst) - expect(src).to exist - end + expect(src).to exist + end - it 'preserves attributes of destination file' do - skip 'full file modes are not support' unless any_file_modes_allowed? - mode = 0o666 + it 'preserves attributes of destination file' do + skip 'full file modes are not support' unless any_file_modes_allowed? + mode = 0o666 - dst.chmod(mode) + dst.chmod(mode) - src.replace(dst) + src.replace(dst) - got = dst.stat.mode & 0o777 - expect(got).to eq(mode), format('expected %04o, got %04o', mode, got) - end + got = dst.stat.mode & 0o777 + expect(got).to eq(mode), format('expected %04o, got %04o', mode, got) + end + + it 'does not preserve mtime of destination file' do + time = src.mtime - it 'does not preserve mtime of destination file' do - time = src.mtime + dst.utime(time - 1000, time - 1000) - dst.utime(time - 1000, time - 1000) + src.replace(dst) - src.replace(dst) + expect(dst.mtime).to be >= time + end - expect(dst.mtime).to be >= time + it 'changes inode of destination' do + skip 'inodes are not supported' unless inodes_supported? + expect{ src.replace(dst) }.to change{ dst.stat.ino } + end end - it 'changes inode of destination' do - skip 'inodes are not supported' unless inodes_supported? - expect{ src.replace(dst) }.to change{ dst.stat.ino } + context 'when src and dst are on same device' do + before do + allow_any_instance_of(File::Stat).to receive(:dev).and_return(0) + end + + include_examples 'replaces file' + end + + context 'when src and dst are on different devices' do + before do + allow_any_instance_of(File::Stat).to receive(:dev, &:__id__) + end + + include_examples 'replaces file' end end end diff --git a/spec/image_optim/path_spec.rb b/spec/image_optim/path_spec.rb index 7e5dfe16..b3410332 100644 --- a/spec/image_optim/path_spec.rb +++ b/spec/image_optim/path_spec.rb @@ -61,45 +61,63 @@ let(:src){ Path.temp_file_path } let(:dst){ Path.temp_file_path } - it 'moves data to destination' do - src.write('src') + shared_examples 'replaces file' do + it 'moves data to destination' do + src.write('src') - src.replace(dst) + src.replace(dst) - expect(dst.read).to eq('src') - end + expect(dst.read).to eq('src') + end - it 'removes original file' do - src.replace(dst) + it 'removes original file' do + src.replace(dst) - expect(src).to_not exist - end + expect(src).to_not exist + end - it 'preserves attributes of destination file' do - skip 'full file modes are not support' unless any_file_modes_allowed? - mode = 0o666 + it 'preserves attributes of destination file' do + skip 'full file modes are not support' unless any_file_modes_allowed? + mode = 0o666 - dst.chmod(mode) + dst.chmod(mode) - src.replace(dst) + src.replace(dst) - got = dst.stat.mode & 0o777 - expect(got).to eq(mode), format('expected %04o, got %04o', mode, got) - end + got = dst.stat.mode & 0o777 + expect(got).to eq(mode), format('expected %04o, got %04o', mode, got) + end - it 'does not preserve mtime of destination file' do - time = src.mtime + it 'does not preserve mtime of destination file' do + time = src.mtime - dst.utime(time - 1000, time - 1000) + dst.utime(time - 1000, time - 1000) - src.replace(dst) + src.replace(dst) + + expect(dst.mtime).to be >= time + end - expect(dst.mtime).to be >= time + it 'changes inode of destination' do + skip 'inodes are not supported' unless inodes_supported? + expect{ src.replace(dst) }.to change{ dst.stat.ino } + end end - it 'changes inode of destination' do - skip 'inodes are not supported' unless inodes_supported? - expect{ src.replace(dst) }.to change{ dst.stat.ino } + context 'when src and dst are on same device' do + before do + allow_any_instance_of(File::Stat).to receive(:dev).and_return(0) + end + + include_examples 'replaces file' + end + + context 'when src and dst are on different devices' do + before do + allow_any_instance_of(File::Stat).to receive(:dev, &:__id__) + end + + include_examples 'replaces file' end end end From 3d5e234b8dfa2a683d2464f489a32a8caedb1e53 Mon Sep 17 00:00:00 2001 From: Ivan Kuchin Date: Tue, 25 Aug 2020 00:42:57 +0200 Subject: [PATCH 2/3] use .tmp as the extension for temporary file if it needs to be created for atomic replacement, part of solution for #178 --- CHANGELOG.markdown | 1 + lib/image_optim/cache_path.rb | 2 +- lib/image_optim/path.rb | 6 +++++- spec/image_optim/cache_path_spec.rb | 6 ++++++ spec/image_optim/path_spec.rb | 6 ++++++ 5 files changed, 19 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.markdown b/CHANGELOG.markdown index a2379b11..0a398a5b 100644 --- a/CHANGELOG.markdown +++ b/CHANGELOG.markdown @@ -2,6 +2,7 @@ ## unreleased +* Use `.tmp` as the extension for temporary file if it needs to be created for atomic replacement [#178](https://github.com/toy/image_optim/issues/178) [@toy](https://github.com/toy) * Don't create a temporary file in destination directory for atomic replacement if temporary directory is on same device as destination [#178](https://github.com/toy/image_optim/issues/178) [@toy](https://github.com/toy) ## v0.26.5 (2019-07-14) diff --git a/lib/image_optim/cache_path.rb b/lib/image_optim/cache_path.rb index 522ed9b6..5b545d88 100644 --- a/lib/image_optim/cache_path.rb +++ b/lib/image_optim/cache_path.rb @@ -11,7 +11,7 @@ def replace(dst) tmpdir = [dirname, Path.new(Dir.tmpdir)].find do |dir| dir.same_dev?(dst.dirname) end - dst.temp_path(tmpdir || dst.dirname) do |temp| + dst.temp_path_with_tmp_ext(tmpdir || dst.dirname) do |temp| copy(temp) dst.copy_metadata(temp) temp.rename(dst.to_s) diff --git a/lib/image_optim/path.rb b/lib/image_optim/path.rb index 56705df4..d62b1320 100644 --- a/lib/image_optim/path.rb +++ b/lib/image_optim/path.rb @@ -55,7 +55,7 @@ def replace(dst) dst.copy_metadata(self) rename(dst.to_s) else - dst.temp_path(dst.dirname) do |temp| + dst.temp_path_with_tmp_ext(dst.dirname) do |temp| move(temp) dst.copy_metadata(temp) temp.rename(dst.to_s) @@ -79,5 +79,9 @@ def self.convert(path) def same_dev?(other) stat.dev == other.stat.dev end + + def temp_path_with_tmp_ext(*args, &block) + self.class.temp_file_path([basename.to_s, '.tmp'], *args, &block) + end end end diff --git a/spec/image_optim/cache_path_spec.rb b/spec/image_optim/cache_path_spec.rb index ab7ed243..9745539f 100644 --- a/spec/image_optim/cache_path_spec.rb +++ b/spec/image_optim/cache_path_spec.rb @@ -56,6 +56,12 @@ skip 'inodes are not supported' unless inodes_supported? expect{ src.replace(dst) }.to change{ dst.stat.ino } end + + it 'is using temporary file with .tmp extension' do + expect(src).to receive(:copy).with(having_attributes(:extname => '.tmp')) + + src.replace(dst) + end end context 'when src and dst are on same device' do diff --git a/spec/image_optim/path_spec.rb b/spec/image_optim/path_spec.rb index b3410332..9efe4c6c 100644 --- a/spec/image_optim/path_spec.rb +++ b/spec/image_optim/path_spec.rb @@ -118,6 +118,12 @@ end include_examples 'replaces file' + + it 'is using temporary file with .tmp extension' do + expect(src).to receive(:move).with(having_attributes(:extname => '.tmp')) + + src.replace(dst) + end end end end From 42ba891918601628cac43c37bb1c469d8b497b92 Mon Sep 17 00:00:00 2001 From: Ivan Kuchin Date: Tue, 25 Aug 2020 00:44:20 +0200 Subject: [PATCH 3/3] add a note to readme about changing temporary directory using environment variables --- README.markdown | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/README.markdown b/README.markdown index 755f7f1f..cdea4376 100644 --- a/README.markdown +++ b/README.markdown @@ -277,6 +277,10 @@ optipng: level: 5 ``` +### Temporary directory + +`image_optim` uses standard ruby library for creating temporary files. Temporary directory can be changed using one of `TMPDIR`, `TMP` or `TEMP` environment variables. + ## Options * `:nice` — Nice level, priority of all used tools with higher value meaning lower priority, in range `-20..19`, negative values can be set only if run by root user *(defaults to `10`)*