Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

modified File.dirname to accept an optional number of levels to strip of the path, Ruby 3.1 support. #2907

Merged
merged 2 commits into from
Mar 15, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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:

Expand Down
39 changes: 27 additions & 12 deletions spec/ruby/core/file/dirname_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,18 +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

it "raises ArgumentError if the level is negative" do
-> {File.dirname('/home/jason', -1)}.should raise_error(ArgumentError)
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 "returns the same String if the level is 0" do
File.dirname('poot.txt', 0).should == 'poot.txt'
File.dirname('/', 0).should == '/'
end

it "raises ArgumentError if the level is negative" do
-> {
File.dirname('/home/jason', -1)
}.should raise_error(ArgumentError, "negative level: -1")
end

it "returns '/' when level exceeds the number of segments in the path" do
File.dirname("/home/jason", 100).should == '/'
end

it "calls #to_int if passed not numeric value" do
object = Object.new
def object.to_int; 2; end

File.dirname("/a/b/c/d", object).should == '/a/b'
end
end
end

Expand Down
2 changes: 0 additions & 2 deletions spec/tags/core/file/dirname_tags.txt

This file was deleted.

51 changes: 13 additions & 38 deletions src/main/ruby/truffleruby/core/file.rb
Original file line number Diff line number Diff line change
Expand Up @@ -395,57 +395,32 @@ 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)
def self.dirname(path, level = 1)
path = Truffle::Type.coerce_to_path(path)
level = Primitive.rb_num2int(level)

# edge case
return +'.' if path.empty?

slash = '/'

# pull off any /'s at the end to ignore
chunk_size = last_nonslash(path)
return +'/' unless chunk_size
raise ArgumentError, "negative level: #{level}" if level < 0
return path if level == 0

if pos = Primitive.find_string_reverse(path, slash, chunk_size)
return +'/' if pos == 0

path = path.byteslice(0, pos)

return +'/' if path == '/'

return path unless path.end_with? slash

# prune any trailing /'s
idx = last_nonslash(path, pos)

# edge case, only /'s, return /
return +'/' unless idx
# fast path
if level == 1
return +'.' if path.empty?
Copy link
Member

@andrykonchin andrykonchin Mar 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it make sense to check path.empty? on each iteration but not only once at the beginning?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess so, since I'm modifying path every iteration, it can potentially become empty every iteration.

This check is actually the first check in the original File.dirname, I just left it in its place here instead of moving it with the rest into FileOperations.dirname_once, because it seems intuitive that if a path ever becomes empty that we early return right away instead of continuing to call dirname_once uselessly.

Copy link
Member

@andrykonchin andrykonchin Mar 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a big deal actually, but the helper method returns either:

  • "."
  • "/"
  • path.byteslice(0, pos)
  • path.byteslice(0, idx - 1)

So I assume it shouldn't return empty String. Or it can?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested it just now in ruby 3.1.0, it can :

irb(main):004:0> "hello".byteslice(0,0)
=> ""

so if idx is 1 or pos is 0 it will return an empty string.

return Truffle::FileOperations.dirname(path)
end

return path.byteslice(0, idx - 1)
level.times do
return +'.' if path.empty?
path = Truffle::FileOperations.dirname(path)
end

+'.'
path
end

##
Expand Down
42 changes: 42 additions & 0 deletions src/main/ruby/truffleruby/core/truffle/file_operations.rb
Original file line number Diff line number Diff line change
Expand Up @@ -78,5 +78,47 @@ def self.expand_path(path, dir, expand_tilde)
def self.exist?(path)
Truffle::POSIX.truffleposix_stat_mode(path) > 0
end

def self.dirname(path)
slash = '/'

# pull off any /'s at the end to ignore
chunk_size = last_nonslash(path)
return +'/' unless chunk_size

if pos = Primitive.find_string_reverse(path, slash, chunk_size)
return +'/' if pos == 0

path = path.byteslice(0, pos)

return +'/' if path == '/'

return path unless path.end_with? slash

# prune any trailing /'s
idx = last_nonslash(path, pos)

# edge case, only /'s, return /
return +'/' unless idx

return path.byteslice(0, idx - 1)
end

+'.'
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
1 change: 0 additions & 1 deletion test/mri/excludes/TestFileExhaustive.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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"