Bug #8034

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

Added by Pavel Manylov about 1 year ago. Updated about 1 year ago.

[ruby-core:53168]
Status:Closed
Priority:Normal
Assignee:Luis Lavena
Category:platform/windows
Target version:-
ruby -v:1.9.3p362 Backport:

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


Related issues

Related to Backport200 - Backport #8069: Backport r39697 Closed 03/10/2013
Related to Backport93 - Backport #8070: Backport r39697 Closed 03/10/2013

Associated revisions

Revision 39697
Added by Luis Lavena about 1 year ago

Expand home directory when used in dir_string

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

Revision 39751
Added by Luis Lavena about 1 year ago

Refactor rbfileexpandpathinternal for dir_string corner cases

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

History

#1 Updated by Luis Lavena about 1 year ago

  • Assignee changed from cruby-windows to Luis Lavena

#2 Updated by Luis Lavena about 1 year 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 (rbfileexpandpathinternal): Expand home directory when used as second parameter (dir_string). [Bug #8034]
  • test/ruby/testfileexhaustive.rb: add test to verify.

#3 Updated by Luis Lavena about 1 year 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 Updated by Nobuyoshi Nakada about 1 year ago

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

#5 Updated by Luis Lavena about 1 year 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 Updated by Luis Lavena about 1 year ago

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

=begin
Nobu,

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

=end

#7 Updated by Luis Lavena about 1 year ago

  • Assignee changed from Luis Lavena to 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 Updated by Hiroshi Shirosaki about 1 year ago

  • Assignee changed from Hiroshi Shirosaki to Luis Lavena

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

#9 Updated by Luis Lavena about 1 year ago

  • Assignee changed from Luis Lavena to 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:

assertnothingraised(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.expandpath('/', '~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 Updated by Nobuyoshi Nakada about 1 year ago

luislavena (Luis Lavena) wrote:

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

assertnothingraised(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 Updated by Hiroshi Shirosaki about 1 year ago

  • Assignee changed from Hiroshi Shirosaki to 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 @@ getuserfrompath(wchart **wpath, int offset, UINT cp, UINT pathcp, rbencodi
convertwcharto_mb(wuser, &user, &size, cp);

 /* convert to VALUE and set the path encoding */
  • if (pathcp == INVALIDCODE_PAGE)
  • {
  • if (pathcp == INVALIDCODEPAGE) { tmp = rbencstrnew(user, size, rbutf8encoding()); result = rbstrencode(tmp, rbencfromencoding(pathencoding), 0, Qnil); rbstrresize(tmp, 0); }
  • else
  • {
  • else { result = rbencstrnew(user, size, pathencoding); }

#12 Updated by Luis Lavena about 1 year 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 rbfileexpandpathinternal for dir_string corner cases

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

#13 Updated by Luis Lavena about 1 year ago

h.shirosaki (Hiroshi Shirosaki) wrote:

Luis, thank you for your work.
xfree(wpath); would be needed before rbraise(rbeArgError, "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