Skip to content

File.dirname: add a spec for Shift JIS handling #1330

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.

Sign up for GitHub

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

Merged
eregon merged 2 commits into from
Jan 20, 2026
+29 −0
Merged

File.dirname: add a spec for Shift JIS handling #1330

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5c63521
File.dirname: add a spec for Shift JIS handling
byroot Jan 19, 2026
1724265
Update core/file/dirname_spec.rb
eregon Jan 20, 2026
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
29 changes: 29 additions & 0 deletions core/file/dirname_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,28 @@ def object.to_int; 2; end
File.dirname("foo/../").should == "foo"
end

it "rejects strings encoded with non ASCII-compatible encodings" do
Encoding.list.reject(&:ascii_compatible?).reject(&:dummy?).each do |enc|
path = "/foo/bar".encode(enc)
-> {
File.dirname(path)
}.should raise_error(Encoding::CompatibilityError)
end
end
Comment on lines +81 to +88
Copy link
Contributor

@trinistr trinistr Jan 28, 2026

Choose a reason for hiding this comment

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

Just had this test fail in CI on macOS with Ruby 3.2.9:

File.dirname rejects strings encoded with non ASCII-compatible encodings ERROR
Encoding::ConverterNotFoundError: code converter not found (UTF-8 to RS3UTF-16-BE)
/Users/runner/work/spec/spec/core/file/dirname_spec.rb:83:in `encode'

What even is this encoding? It's not listed on my system and clearly wasn't a problem when this PR was made 🤔

Copy link
Contributor

@trinistr trinistr Jan 28, 2026

Choose a reason for hiding this comment

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

Actually, this makes no sense, that branch does not include this test at all!

Copy link
Member Author

@byroot byroot Jan 28, 2026

Choose a reason for hiding this comment

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

I had to handle that too with File.dirname: https://github.com/ruby/ruby/pull/15919/changes#diff-f9288a658b2b8283fa9576185f8e17b99ade402e97f2ca60b368188234f917f1R167

I suspect we should do the same here, but maybe @eregon has a different opinion?

Copy link
Member

@herwinw herwinw Jan 28, 2026

Choose a reason for hiding this comment

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

I've pushed #1334 as a simple workaround by not running these specs on Ruby 3.2. The error is not related to the spec, and Ruby 3.2 is nearing EOL.

Copy link
Member

@eregon eregon Jan 28, 2026

Choose a reason for hiding this comment

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

Fixed now on master.
From #1334 (comment)

I had a similar issue when adding more encoding-related specs in TruffleRuby (not sync'd yet) [...]

Maybe we should drop 3.2 now?
I'm happy I managed to convince CRuby to drop replicate encodings, these replicates clearly caused extra edge cases like this one for little value.

Copy link
Member

@eregon eregon Jan 28, 2026

Choose a reason for hiding this comment

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

I'll drop Ruby 3.2 on the next sync from ruby/spec. It's also annoying to have 4 versions supported due to slower CI.

Copy link
Contributor

@trinistr trinistr Jan 28, 2026

Choose a reason for hiding this comment

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

I still don't understand how this test failed on a branch that doesn't have the test (here). This implies that wrong code is running, somehow.

Copy link
Member

@eregon eregon Jan 28, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

That's just because CI runs on a merge commit between your branch and master:
This run ran on 1c21abb

And this issue is transient because of spec ordering and we're running specs in parallel in CI so the ordering is not always the same.

Copy link
Contributor

@trinistr trinistr Jan 28, 2026

Choose a reason for hiding this comment

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

Huh, didn't realise it could do that. Thank you for the explanation!


it "works with all ASCII-compatible encodings" do
Encoding.list.select(&:ascii_compatible?).each do |enc|
File.dirname("/foo/bar".encode(enc)).should == "/foo".encode(enc)
end
end

it "handles Shift JIS 0x5C (\\) as second byte of a multi-byte sequence" do
# dir/fileソname.txt
path = "dir/file\x83\x5cname.txt".b.force_encoding(Encoding::SHIFT_JIS)
path.valid_encoding?.should be_true
File.dirname(path).should == "dir"
end

platform_is_not :windows do
it "ignores repeated leading / (edge cases on non-windows)" do
File.dirname("/////foo/bar/").should == "/foo"
Expand All @@ -98,6 +120,13 @@ def object.to_int; 2; end
File.dirname("//foo//").should == "//foo"
File.dirname('/////').should == '//'
end

it "handles Shift JIS 0x5C (\\) as second byte of a multi-byte sequence (windows)" do
# dir\fileソname.txt
path = "dir\\file\x83\x5cname.txt".b.force_encoding(Encoding::SHIFT_JIS)
path.valid_encoding?.should be_true
File.dirname(path).should == "dir"
end
end

it "accepts an object that has a #to_path method" do
Expand Down