merge changes to not create extra temporary files when possible and i…

…mprove behaviour otherwise, resolves #178

CHANGELOG.markdown

@@ -2,6 +2,9 @@
 
 ## 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)
 
 * Remove deprecated `rubyforge_project` attribute from gemspec [rubygems/rubygems#2436](https://github.com/rubygems/rubygems/pull/2436) [@toy](https://github.com/toy)

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`)*

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_with_tmp_ext(tmpdir || dst.dirname) do |temp|
         copy(temp)
         dst.copy_metadata(temp)
         temp.rename(dst.to_s)

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_with_tmp_ext(dst.dirname) do |temp|
+          move(temp)
+          dst.copy_metadata(temp)
+          temp.rename(dst.to_s)
+        end
       end
     end
 
@@ -68,5 +73,15 @@ 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
+
+    def temp_path_with_tmp_ext(*args, &block)
+      self.class.temp_file_path([basename.to_s, '.tmp'], *args, &block)
+    end
   end
 end

spec/image_optim/cache_path_spec.rb

@@ -15,45 +15,69 @@
     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
+
+        dst.utime(time - 1000, time - 1000)
 
-    it 'does not preserve mtime of destination file' do
-      time = src.mtime
+        src.replace(dst)
 
-      dst.utime(time - 1000, time - 1000)
+        expect(dst.mtime).to be >= time
+      end
 
-      src.replace(dst)
+      it 'changes inode of destination' do
+        skip 'inodes are not supported' unless inodes_supported?
+        expect{ src.replace(dst) }.to change{ dst.stat.ino }
+      end
 
-      expect(dst.mtime).to be >= time
+      it 'is using temporary file with .tmp extension' do
+        expect(src).to receive(:copy).with(having_attributes(:extname => '.tmp'))
+
+        src.replace(dst)
+      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

spec/image_optim/path_spec.rb

@@ -61,45 +61,69 @@
     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
+
+        dst.utime(time - 1000, time - 1000)
+
+        src.replace(dst)
 
-    it 'does not preserve mtime of destination file' do
-      time = src.mtime
+        expect(dst.mtime).to be >= time
+      end
 
-      dst.utime(time - 1000, time - 1000)
+      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
 
-      src.replace(dst)
+    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
 
-      expect(dst.mtime).to be >= time
+      include_examples 'replaces file'
     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 different devices' do
+      before do
+        allow_any_instance_of(File::Stat).to receive(:dev, &:__id__)
+      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