Project

General

Profile

Bug #8034

File.expand_path('something', '~') do not include home path

Added by rap-kasta (Pavel Manylov) almost 5 years ago. Updated almost 5 years ago.

Status:
Closed
Priority:
Normal
Target version:
-
ruby -v:
1.9.3p362
[ruby-core:53168]

Description

=begin
Next code works correctly only on ruby <= 1.9.3p362.
Tested on Windows XP, Windows 2008 with ruby 1.9.3p194 (works properly), ruby 1.9.3p362 (bug), ruby 2.0.0p0 (bug).

(({File.expand_path('something', '~') #=> "C:/path/to/current/dir/~/something"}))

However, next code works as it should in all tested rubies:

(({File.expand_path '~' # => "C:/Documents and Settings/Jack"
Dir.home # => "C:/Documents and Settings/Jack"}))
=end

Associated revisions

Revision 39697
Added by luislavena (Luis Lavena) almost 5 years ago

Expand home directory when used in dir_string

  • win32/file.c (rb_file_expand_path_internal): Expand home directory when used as second parameter (dir_string). [Bug #8034]
  • test/ruby/test_file_exhaustive.rb: add test to verify.

Revision 39697
Added by luislavena (Luis Lavena) almost 5 years ago

Expand home directory when used in dir_string

  • win32/file.c (rb_file_expand_path_internal): Expand home directory when used as second parameter (dir_string). [Bug #8034]
  • test/ruby/test_file_exhaustive.rb: add test to verify.

Revision 39697
Added by luislavena (Luis Lavena) almost 5 years ago

Expand home directory when used in dir_string

  • win32/file.c (rb_file_expand_path_internal): Expand home directory when used as second parameter (dir_string). [Bug #8034]
  • test/ruby/test_file_exhaustive.rb: add test to verify.

Revision 39697
Added by luislavena (Luis Lavena) almost 5 years ago

Expand home directory when used in dir_string

  • win32/file.c (rb_file_expand_path_internal): Expand home directory when used as second parameter (dir_string). [Bug #8034]
  • test/ruby/test_file_exhaustive.rb: add test to verify.

Revision 39751
Added by luislavena (Luis Lavena) almost 5 years ago

Refactor rb_file_expand_path_internal for dir_string corner cases

  • win32/file.c (get_user_from_path): add internal function that retrieves username from supplied path (refactored).
  • win32/file.c (rb_file_expand_path_internal): refactor expansion of user home to use get_user_from_path and cover dir_string corner cases. [Bug #8034]

Revision 39751
Added by luislavena (Luis Lavena) almost 5 years ago

Refactor rb_file_expand_path_internal for dir_string corner cases

  • win32/file.c (get_user_from_path): add internal function that retrieves username from supplied path (refactored).
  • win32/file.c (rb_file_expand_path_internal): refactor expansion of user home to use get_user_from_path and cover dir_string corner cases. [Bug #8034]

Revision 39751
Added by luislavena (Luis Lavena) almost 5 years ago

Refactor rb_file_expand_path_internal for dir_string corner cases

  • win32/file.c (get_user_from_path): add internal function that retrieves username from supplied path (refactored).
  • win32/file.c (rb_file_expand_path_internal): refactor expansion of user home to use get_user_from_path and cover dir_string corner cases. [Bug #8034]

Revision 39751
Added by luislavena (Luis Lavena) almost 5 years ago

Refactor rb_file_expand_path_internal for dir_string corner cases

  • win32/file.c (get_user_from_path): add internal function that retrieves username from supplied path (refactored).
  • win32/file.c (rb_file_expand_path_internal): refactor expansion of user home to use get_user_from_path and cover dir_string corner cases. [Bug #8034]

History

#1 [ruby-core:53170] Updated by luislavena (Luis Lavena) almost 5 years ago

  • Assignee changed from cruby-windows to luislavena (Luis Lavena)

#2 Updated by luislavena (Luis Lavena) almost 5 years ago

  • Status changed from Open to Closed
  • % Done changed from 0 to 100

This issue was solved with changeset r39697.
Pavel, thank you for reporting this issue.
Your contribution to Ruby is greatly appreciated.
May Ruby be with you.


Expand home directory when used in dir_string

  • win32/file.c (rb_file_expand_path_internal): Expand home directory when used as second parameter (dir_string). [Bug #8034]
  • test/ruby/test_file_exhaustive.rb: add test to verify.

#3 [ruby-core:53285] Updated by luislavena (Luis Lavena) almost 5 years ago

Hello,

This has been fixed in trunk and backports for both 2.0.0 and 1.9.3 have been requested.

Thank you for your report.

#4 [ruby-core:53287] Updated by nobu (Nobuyoshi Nakada) almost 5 years ago

=begin
Why is (({File.expand_path('something', '~whoever')})) ignored?
=end

#5 [ruby-core:53289] Updated by luislavena (Luis Lavena) almost 5 years ago

=begin
nobu (Nobuyoshi Nakada) wrote:

=begin
Why is (({File.expand_path('something', '~whoever')})) ignored?

There is no test for that, I didn't see a test that verifies that behavior so I didn't add code for it.

Also, (({~user})) doesn't work as you expect on Windows.
=end

=end

#6 [ruby-core:53290] Updated by luislavena (Luis Lavena) almost 5 years ago

  • % Done changed from 100 to 50
  • Status changed from Closed to Assigned

=begin
Nobu,

I'll add test for (({File.expand_path('something', '~whoever')})) and add support for it in ((|dir_string|)).

=end

#7 [ruby-core:53294] Updated by luislavena (Luis Lavena) almost 5 years ago

  • Assignee changed from luislavena (Luis Lavena) to h.shirosaki (Hiroshi Shirosaki)

Following patch solves the issue:

https://gist.github.com/luislavena/5130078

However there is lot of duplication between wpath and wdir checks, which is crying for a refactoring.

Shirosaki-san, what do you think?

#8 [ruby-core:53297] Updated by h.shirosaki (Hiroshi Shirosaki) almost 5 years ago

  • Assignee changed from h.shirosaki (Hiroshi Shirosaki) to luislavena (Luis Lavena)

Luis, thank you for your work.
xfree(wpath); would be needed before rb_raise(rb_eArgError, "can't find user %s"...
Indeed refactoring is better if possible.

#9 [ruby-core:53354] Updated by luislavena (Luis Lavena) almost 5 years ago

  • Assignee changed from luislavena (Luis Lavena) to h.shirosaki (Hiroshi Shirosaki)
  • % Done changed from 50 to 70

=begin
Shirosaki-san,

I've refactored the code, would you mind take a look?

https://gist.github.com/luislavena/5148562

After applying the patch, it fixes the original issue, however now nobu-san added another test:

assert_nothing_raised(ArgumentError) { File.expand_path("/", UnknownUserHome) }

Which requires more conditions if (({fname})) is '/' and set (({ignore_dir})) accordingly, but I'm confused about the scenario.

I'm not sure if that scenario is portable and correct since 1.8.7 and 1.9.2 return ArgumentError:

C:\Users\Luis>ruby -ve "p File.expand_path('/', '~foo')"
ruby 1.8.7 (2012-10-12 patchlevel 371) [i386-mingw32]
-e:1:in `expand_path': can't find user ~foo (ArgumentError)
from -e:1

C:\Users\Luis>ruby -ve "p File.expand_path('/', '~foo')"
ruby 1.9.2p290 (2011-07-09) [i386-mingw32]
-e:1:in expand_path': can't find user foo (ArgumentError)
from -e:1:in
'

Thoughts?

=end

#10 [ruby-core:53362] Updated by nobu (Nobuyoshi Nakada) almost 5 years ago

luislavena (Luis Lavena) wrote:

After applying the patch, it fixes the original issue, however now nobu-san added another test:

assert_nothing_raised(ArgumentError) { File.expand_path("/", UnknownUserHome) }

Which requires more conditions if (({fname})) is '/' and set (({ignore_dir})) accordingly, but I'm confused about the scenario.

Sorry, fixed the test, it should raise an exception on Windows, but the
latest test should not.

#11 [ruby-core:53373] Updated by h.shirosaki (Hiroshi Shirosaki) almost 5 years ago

  • Assignee changed from h.shirosaki (Hiroshi Shirosaki) to luislavena (Luis Lavena)

luislavena (Luis Lavena) wrote:

I've refactored the code, would you mind take a look?

https://gist.github.com/luislavena/5148562

Coding style is inconsistent at if/else. Otherwise, looks good to me. Thank you.

diff --git a/win32/file.c b/win32/file.c
index 53cf085..350f8da 100644
--- a/win32/file.c
+++ b/win32/file.c
@@ -333,14 +333,12 @@ get_user_from_path(wchar_t **wpath, int offset, UINT cp, UINT path_cp, rb_encodi
convert_wchar_to_mb(wuser, &user, &size, cp);

 /* convert to VALUE and set the path encoding */
  • if (path_cp == INVALID_CODE_PAGE)
  • {
  • if (path_cp == INVALID_CODE_PAGE) { tmp = rb_enc_str_new(user, size, rb_utf8_encoding()); result = rb_str_encode(tmp, rb_enc_from_encoding(path_encoding), 0, Qnil); rb_str_resize(tmp, 0); }
  • else
  • {
  • else { result = rb_enc_str_new(user, size, path_encoding); }

#12 Updated by luislavena (Luis Lavena) almost 5 years ago

  • Status changed from Assigned to Closed
  • % Done changed from 70 to 100

This issue was solved with changeset r39751.
Pavel, thank you for reporting this issue.
Your contribution to Ruby is greatly appreciated.
May Ruby be with you.


Refactor rb_file_expand_path_internal for dir_string corner cases

  • win32/file.c (get_user_from_path): add internal function that retrieves username from supplied path (refactored).
  • win32/file.c (rb_file_expand_path_internal): refactor expansion of user home to use get_user_from_path and cover dir_string corner cases. [Bug #8034]

#13 [ruby-core:53413] Updated by luislavena (Luis Lavena) almost 5 years ago

h.shirosaki (Hiroshi Shirosaki) wrote:

Luis, thank you for your work.
xfree(wpath); would be needed before rb_raise(rb_eArgError, "can't find user %s"...
Indeed refactoring is better if possible.

Hiroshi,

I've pushed the refactored code as r39751 and corrected the style issue you mentioned. Thank you.

I noticed getting the user home directory when detected "~" in either wpath or wdir can be refactored too.

But that is for another ticket :)

Thank you.

Also available in: Atom PDF