Refactor and add changelog entry

Author: andrykonchin

CHANGELOG.md

@@ -93,6 +93,7 @@ Compatibility:
 * Update to JCodings 1.0.58 and Joni 2.1.44 (@eregon).
 * Add `MatchData#match` and `MatchData#match_length` (#2733, @horakivo).
 * Add `StructClass#keyword_init?` method (#2377, @moste00).
+* Support optional `level` argument for `File.dirname` method (#2733, @moste00).
 
 Performance:
 

spec/ruby/core/file/dirname_spec.rb

@@ -12,38 +12,33 @@
   end
 
   ruby_version_is '3.1' do
-    it "returns all the components of filename except the last parts by the level" do
-      File.dirname('/home/jason', 2).should == '/'
-      File.dirname('/home/jason/poot.txt', 2).should == '/home'
-    end
-
-    it "returns the same string if the level is 0" do
-      File.dirname('poot.txt', 0).should == 'poot.txt'
-      File.dirname('/', 0).should == '/'
-    end
+    context "when level is passed" do
+      it "returns all the components of filename except the last parts by the level" do
+        File.dirname('/home/jason', 2).should == '/'
+        File.dirname('/home/jason/poot.txt', 2).should == '/home'
+      end
 
-    it "raises ArgumentError if the level is negative" do
-      -> {File.dirname('/home/jason', -1)}.should raise_error(ArgumentError)
-    end
+      it "returns the same String if the level is 0" do
+        File.dirname('poot.txt', 0).should == 'poot.txt'
+        File.dirname('/', 0).should == '/'
+      end
 
-    it "returns the exact same object when passed 0 as the level" do
-      obj = "path"
-      File.dirname(obj,0).should .equal?(obj)
+      it "raises ArgumentError if the level is negative" do
+        -> {
+          File.dirname('/home/jason', -1)
+        }.should raise_error(ArgumentError, "negative level: -1")
+      end
 
-      obj = mock("to_path")
-      def obj.to_path
-        "path"
+      it "returns '/' when level exceeds the number of segments in the path" do
+        File.dirname("/home/jason", 100).should == '/'
       end
-      File.dirname(obj,0).should .equal?(obj)
 
-      obj = Object.new
-      File.dirname(obj,0).should .equal?(obj)
-    end
+      it "calls #to_int if passed not numeric value" do
+        object = Object.new
+        def object.to_int; 2; end
 
-    it "returns / when passed number of levels exceeds the number of segments in the path" do
-      (3..100).each { |level|
-        File.dirname("/home/jason", level).should == '/'
-      }
+        File.dirname("/a/b/c/d", object).should == '/a/b'
+      end
     end
   end
 

src/main/ruby/truffleruby/core/file.rb

@@ -395,45 +395,31 @@ def self.directory?(io_or_path)
     Truffle::StatOperations.directory?(mode)
   end
 
-  def self.last_nonslash(path, start = nil)
-    # Find the first non-/ from the right
-    data = path.bytes
-    start ||= (path.size - 1)
-
-    start.downto(0) do |i|
-      if data[i] != 47  # ?/
-        return i
-      end
-    end
-    nil
-  end
-
   ##
   # Returns all components of the filename given in
   # file_name except the last one. The filename must be
   # formed using forward slashes ("/") regardless of
   # the separator used on the local file system.
   #
   #  File.dirname("/home/gumby/work/ruby.rb")   #=> "/home/gumby/work"
-  def self.dirname(path,num_levels=1)
-    if num_levels < 0
-      raise ArgumentError, "level can't be negative"
-    end
-    #This must happen before the type coercion to string below, because File.dirname accepts any objects which can respond to a :to_path message
-    #If path was such an object, File.dirname(obj,0) returns it as-is without conversion to string
-    #Tricky, but that's how Matz ruby behave
-    if num_levels == 0
-      return path
-    end
-
+  def self.dirname(path, level = 1)
     path = Truffle::Type.coerce_to_path(path)
+    level = Primitive.rb_num2int(level)
+
+    raise ArgumentError, "negative level: #{level}" if level < 0
+    return path if level == 0
 
-    num_levels.times do
-      # edge case
+    # fast path
+    if level == 1
       return +'.' if path.empty?
+      return Truffle::FileOperations.dirname(path)
+    end
 
-      path = Truffle::FileOperations.remove_last_segment_from_path(path)
+    level.times do
+      return +'.' if path.empty?
+      path = Truffle::FileOperations.dirname(path)
     end
+
     path
   end
 

src/main/ruby/truffleruby/core/truffle/file_operations.rb

@@ -79,12 +79,11 @@ def self.exist?(path)
       Truffle::POSIX.truffleposix_stat_mode(path) > 0
     end
 
-    #Helper to File.dirname, extracts the directory of a path (doesn't need to handle the empty string)
-    #This was the original File.dirname, but ruby 3.1 made File.dirname take an optional number of times to do its logic
-    def self.remove_last_segment_from_path(path)
+    def self.dirname(path)
       slash = '/'
+
       # pull off any /'s at the end to ignore
-      chunk_size = File.last_nonslash(path)
+      chunk_size = last_nonslash(path)
       return +'/' unless chunk_size
 
       if pos = Primitive.find_string_reverse(path, slash, chunk_size)
@@ -97,7 +96,7 @@ def self.remove_last_segment_from_path(path)
         return path unless path.end_with? slash
 
         # prune any trailing /'s
-        idx = File.last_nonslash(path, pos)
+        idx = last_nonslash(path, pos)
 
         # edge case, only /'s, return /
         return +'/' unless idx
@@ -107,5 +106,19 @@ def self.remove_last_segment_from_path(path)
 
       +'.'
     end
+
+    def self.last_nonslash(path, start = nil)
+      # Find the first non-/ from the right
+      data = path.bytes
+      start ||= (path.size - 1)
+
+      start.downto(0) do |i|
+        if data[i] != 47  # ?/
+          return i
+        end
+      end
+
+      nil
+    end
   end
 end

test/mri/excludes/TestFileExhaustive.rb

@@ -15,6 +15,5 @@
 exclude :test_expand_path_for_existent_username, "needs investigation"
 exclude :test_readlink_long_path, "needs investigation"
 exclude :test_utime, "needs investigation"
-exclude :test_dirname, "ArgumentError: wrong number of arguments (given 2, expected 1)"
 exclude :test_flock_shared, "Zlib::InProgressError: zlib stream is in progress"
 exclude :test_flock_exclusive, "Zlib::InProgressError: zlib stream is in progress"