-
-
Notifications
You must be signed in to change notification settings - Fork 925
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
expand_path should use the original encoding #3866
Conversation
It appears one of the tests in the JRuby suite failed, but I need to confirm if it's an appropriate test. |
Oh, silly me...it's the test you added. Testing on MRI now. |
@headius should be fixed now |
@ahorek Ok that matches a patch I put together here that I think fits a little better. I'll merge your PR to get the test but I'll go with my patch and you can let me know if it fixes your local issues too. |
Looks like with either of our patches I get a couple failures in MRI's suite:
Here's the code for that test: def test_expand_path_encoding_filesystem
home = ENV["HOME"]
ENV["HOME"] = "#{DRIVE}/UserHome"
path = "~".encode("US-ASCII")
dir = "C:/".encode("IBM437")
fs = Encoding.find("filesystem")
assert_equal fs, File.expand_path(path).encoding
assert_equal fs, File.expand_path(path, dir).encoding
ensure
ENV["HOME"] = home
end And here's the patch I made (applies over your PR): diff --git a/core/src/main/java/org/jruby/RubyFile.java b/core/src/main/java/org/jruby/RubyFile.java
index 23689f1..237fd90 100644
--- a/core/src/main/java/org/jruby/RubyFile.java
+++ b/core/src/main/java/org/jruby/RubyFile.java
@@ -785,13 +785,9 @@ public class RubyFile extends RubyIO implements EncodingCapable {
@JRubyMethod(name = "expand_path", required = 1, optional = 1, meta = true)
public static IRubyObject expand_path19(ThreadContext context, IRubyObject recv, IRubyObject[] args) {
- RubyString path = (RubyString) expandPathInternal(context, recv, args, true, false);
- path.force_encoding(context, context.runtime.getEncodingService().convertEncodingToRubyEncoding(get_path(context, args[0]).getEncoding()));
-
- return path;
+ return expandPathInternal(context, recv, args, true, false);
}
-
/**
* ---------------------------------------------------- File::absolute_path
* File.absolute_path(file_name [, dir_string] ) -> abs_file_name
@@ -1560,6 +1556,7 @@ public class RubyFile extends RubyIO implements EncodingCapable {
RubyString origPath = StringSupport.checkEmbeddedNulls(runtime, get_path(context, args[0]));
String relativePath = origPath.getUnicodeValue();
+ Encoding enc = origPath.getEncoding();
// Special /dev/null of windows
if (Platform.IS_WINDOWS && ("NUL:".equalsIgnoreCase(relativePath) || "NUL".equalsIgnoreCase(relativePath))) {
@@ -1613,7 +1610,7 @@ public class RubyFile extends RubyIO implements EncodingCapable {
// this is basically for classpath:/ and uri:classloader:/
relativePath = relativePath.substring(2).replace('\\', '/');
}
- return concatStrings(runtime, preFix, extra, relativePath);
+ return concatStrings(runtime, preFix, extra, relativePath, enc);
}
String[] uriParts = splitURI(relativePath);
@@ -1627,7 +1624,7 @@ public class RubyFile extends RubyIO implements EncodingCapable {
if (uriParts != null) {
//If the path was an absolute classpath path, return it as-is.
if (uriParts[0].equals("classpath:")) {
- return concatStrings(runtime, preFix, relativePath, postFix);
+ return concatStrings(runtime, preFix, relativePath, postFix, enc);
}
relativePath = uriParts[1];
}
@@ -1745,13 +1742,14 @@ public class RubyFile extends RubyIO implements EncodingCapable {
}
}
}
- return concatStrings(runtime, preFix, realPath, postFix);
+ return concatStrings(runtime, preFix, realPath, postFix, enc);
}
- private static RubyString concatStrings(final Ruby runtime, String s1, String s2, String s3) {
+ private static RubyString concatStrings(final Ruby runtime, String s1, String s2, String s3, Encoding enc) {
return RubyString.newString(runtime,
new StringBuilder(s1.length() + s2.length() + s3.length()).
- append(s1).append(s2).append(s3)
+ append(s1).append(s2).append(s3).toString(),
+ enc
);
}
Unfortunately this is where it gets a bit tricky. Looking at MRI's C code for |
Oh, line 660 is the first |
@headius Tested, thanks for getting this done! |
fixes #3849