Fix history saving when directory writability is unreliable (#1220)

* Fix history saving when directory writability is unreliable

* Use Windows history regression test

* Share history test setup

Author: st0012

lib/irb/history.rb

@@ -64,13 +64,6 @@ def load_history
     def save_history
       return unless History.save_history?
       return unless (history_file = History.history_file)
-      unless ensure_history_file_writable(history_file)
-        warn <<~WARN
-          Can't write history to #{History.history_file.inspect} due to insufficient permissions.
-          Please verify the value of `IRB.conf[:HISTORY_FILE]`. Ensure the folder exists and that both the folder and file (if it exists) are writable.
-        WARN
-        return
-      end
 
       history = self.class::HISTORY.to_a
 
@@ -80,36 +73,43 @@ def save_history
         append_history = true
       end
 
-      File.open(history_file, (append_history ? "a" : "w"), 0o600, encoding: IRB.conf[:LC_MESSAGES]&.encoding) do |f|
-        hist = history.map { |l| l.scrub.split("\n").join("\\\n") }
+      begin
+        ensure_history_file_permissions(history_file)
 
-        unless append_history || History.infinite?
-          # Check size before slicing because array.last(huge_number) raises RangeError.
-          hist = hist.last(History.save_history) if hist.size > History.save_history
-        end
+        File.open(history_file, (append_history ? "a" : "w"), 0o600, encoding: IRB.conf[:LC_MESSAGES]&.encoding) do |f|
+          hist = history.map { |l| l.scrub.split("\n").join("\\\n") }
+
+          unless append_history || History.infinite?
+            # Check size before slicing because array.last(huge_number) raises RangeError.
+            hist = hist.last(History.save_history) if hist.size > History.save_history
+          end
 
-        f.puts(hist)
+          f.puts(hist)
+        end
+      rescue Errno::ENOENT
+        warn <<~WARN
+          Can't write history to #{History.history_file.inspect} because the folder does not exist.
+          Please verify the value of `IRB.conf[:HISTORY_FILE]`. Ensure the folder exists.
+        WARN
+      rescue Errno::EACCES, Errno::EPERM
+        warn <<~WARN
+          Can't write history to #{History.history_file.inspect} due to insufficient permissions.
+          Please verify the value of `IRB.conf[:HISTORY_FILE]`. Ensure the folder exists and that both the folder and file (if it exists) are writable.
+        WARN
       end
     end
 
     private
 
-    # Returns boolean whether writing to +history_file+ will be possible.
     # Permissions of already existing +history_file+ are changed to
     # owner-only-readable if necessary [BUG #7694].
-    def ensure_history_file_writable(history_file)
+    def ensure_history_file_permissions(history_file)
       history_file = Pathname.new(history_file)
 
-      return false unless history_file.dirname.writable?
-      return true unless history_file.exist?
+      return unless history_file.exist?
 
-      begin
-        if history_file.stat.mode & 0o66 != 0
-          history_file.chmod 0o600
-        end
-        true
-      rescue Errno::EPERM # no permissions
-        false
+      if history_file.stat.mode & 0o66 != 0
+        history_file.chmod 0o600
       end
     end
   end

test/irb/history_test_case.rb

@@ -0,0 +1,29 @@
+# frozen_string_literal: false
+require "irb"
+require "fileutils"
+require "tmpdir"
+
+require_relative "helper"
+
+module TestIRB
+  class HistoryTestCase < TestCase
+    def setup
+      @conf_backup = IRB.conf.dup
+      @original_verbose, $VERBOSE = $VERBOSE, nil
+      @tmpdir = Dir.mktmpdir("test_irb_history_")
+      setup_envs(home: @tmpdir)
+      IRB.conf[:LC_MESSAGES] = IRB::Locale.new
+      save_encodings
+      IRB.instance_variable_set(:@existing_rc_name_generators, nil)
+    end
+
+    def teardown
+      IRB.conf.replace(@conf_backup)
+      IRB.instance_variable_set(:@existing_rc_name_generators, nil)
+      teardown_envs
+      restore_encodings
+      $VERBOSE = @original_verbose
+      FileUtils.rm_rf(@tmpdir)
+    end
+  end
+end

test/irb/test_history.rb

@@ -1,8 +1,7 @@
 # frozen_string_literal: false
-require 'irb'
 require "tempfile"
 
-require_relative "helper"
+require_relative "history_test_case"
 
 return if RUBY_PLATFORM.match?(/solaris|mswin|mingw/i)
 
@@ -14,26 +13,7 @@ module TestIRB
     Readline = ::Reline
   end
 
-  class HistoryTest < TestCase
-    def setup
-      @conf_backup = IRB.conf.dup
-      @original_verbose, $VERBOSE = $VERBOSE, nil
-      @tmpdir = Dir.mktmpdir("test_irb_history_")
-      setup_envs(home: @tmpdir)
-      IRB.conf[:LC_MESSAGES] = IRB::Locale.new
-      save_encodings
-      IRB.instance_variable_set(:@existing_rc_name_generators, nil)
-    end
-
-    def teardown
-      IRB.conf.replace(@conf_backup)
-      IRB.instance_variable_set(:@existing_rc_name_generators, nil)
-      teardown_envs
-      restore_encodings
-      $VERBOSE = @original_verbose
-      FileUtils.rm_rf(@tmpdir)
-    end
-
+  class HistoryTest < HistoryTestCase
     class TestInputMethodWithRelineHistory < TestInputMethod
       # When IRB.conf[:USE_MULTILINE] is true, IRB::RelineInputMethod uses Reline::History
       HISTORY = Reline::History.new(Reline.core.config)

test/irb/test_history_windows.rb

@@ -0,0 +1,73 @@
+# frozen_string_literal: false
+require_relative "history_test_case"
+
+module TestIRB
+  class HistoryWindowsTest < HistoryTestCase
+    class TestInputMethodWithHistory < TestInputMethod
+      HISTORY = []
+
+      include IRB::HistorySavingAbility
+
+      def gets
+        super&.tap do |line|
+          HISTORY << line unless line.empty?
+        end
+      end
+    end
+
+    def test_history_is_saved_in_readonly_attributed_directory
+      omit "Windows-only" unless windows?
+
+      history_dir = File.join(@tmpdir, "history")
+      history_file = File.join(history_dir, "irb_history")
+      FileUtils.mkdir_p(history_dir)
+
+      with_readonly_attribute(history_dir) do
+        assert_nothing_raised do
+          File.write(File.join(history_dir, "manual_write"), "ok")
+        end
+
+        _output, warning = capture_output do
+          run_irb_with_history(history_file)
+        end
+
+        assert_equal("", warning)
+        assert_equal(<<~HISTORY, File.read(history_file))
+          puts 'history_entry'
+          exit
+        HISTORY
+      end
+    end
+
+    private
+
+    def with_readonly_attribute(path)
+      assert(system("attrib", "+R", windows_path(path)), "failed to set read-only attribute on #{path}")
+      yield
+    ensure
+      system("attrib", "-R", windows_path(path)) if path && File.directory?(path)
+    end
+
+    def windows_path(path)
+      return path unless File::ALT_SEPARATOR
+
+      path.tr(File::SEPARATOR, File::ALT_SEPARATOR)
+    end
+
+    def run_irb_with_history(history_file)
+      IRB.init_config(nil)
+      IRB.init_error
+      IRB.conf[:PROMPT_MODE] = :SIMPLE
+      IRB.conf[:SAVE_HISTORY] = 100
+      IRB.conf[:HISTORY_FILE] = history_file
+      IRB.conf[:USE_PAGER] = false
+
+      input = TestInputMethodWithHistory.new(["puts 'history_entry'", "exit"])
+      irb = IRB::Irb.new(IRB::WorkSpace.new(Object.new), input)
+      irb.context.return_format = "=> %s\n"
+      irb.run(IRB.conf)
+    ensure
+      TestInputMethodWithHistory::HISTORY.clear
+    end
+  end
+end