don't create a temporary file in destination directory for atomic rep…

…lacement if temporary directory is on same device as destination, part of solution for #178

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)

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)

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

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

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