Bug #20056: Dir#chdir inconsistency with Dir.chdir

• Status: Closed
• Assignee: jeremyevans0 (Jeremy Evans)
• Target version: -
• Backport: 3.0: UNKNOWN, 3.1: UNKNOWN, 3.2: UNKNOWN

[zverok (Victor Shepelev)]

I am not sure it is important; I just wanted to understand if this is intentional or accidental.

1. There is no block form for Dir#chdir, unlike Dir.chdir (the form that will return to the previous directory when the block is finished)
2. Dir.chdir returns 0, while Dir#chdir returns nil (both seem to be not representing any particular internal value, just a hardcoded return value).

[jeremyevans0 (Jeremy Evans)]

zverok (Victor Shepelev) wrote:

1. There is no block form for Dir#chdir, unlike Dir.chdir (the form that will return to the previous directory when the block is finished)

Dir#chdir implicitly calls Dir.chdir or Dir.fchdir with the passed block, so the block form does work, but it apparently is not documented:

p Dir.pwd
# => '/home/jeremy'

Dir.new('..').chdir{p Dir.pwd}
# => '/home'

p Dir.pwd
# => '/home/jeremy'

2. Dir.chdir returns 0, while Dir#chdir returns nil (both seem to be not representing any particular internal value, just a hardcoded return value).

Return value being nil is expected. I assume the only reason Dir.chdir returns 0 is backwards compatibility, as I highly doubt we would use 0 as the return value for success for new methods.

[zverok (Victor Shepelev)]

• Status changed from Open to Closed

Dir#chdir implicitly calls Dir.chdir or Dir.fchdir with the passed block, so the block form does work, but it apparently is not documented:

Ugh, indeed. My bad. Instead of checking manually, just looked in the code and didn't saw anything related to block passing— but now I understand.

I'll push a small docs adjustment in the evening into https://github.com/ruby/ruby/pull/9183 while fixing the review notes.

[zverok (Victor Shepelev)]

• Status changed from Closed to Open

Actually, I found another inconsistency while writing docs. Not sure it is worth fixing, but still somewhat confusing:

Dir.chdir('doc') do |*args|
  p args #=> ["doc"]
  "res"
end.tap { p _1 } #=> "res"

Dir.new('doc').chdir do |*args|
  p args #=> []
  "res"
end.tap { p _1 } #=> nil

Dir.fchdir(Dir.new('doc').fileno) do |*args|
  p args #=> []
  "res"
end.tap { p _1 } #=> "res"

The difference in args passing into a block might cause only mild confusion, but the fact that the Dir#chdir doesn't return the block result seems problematic.

Unless I am missing something again.

[jeremyevans0 (Jeremy Evans)]

Not passing an argument to the block in the Dir.fchdir or Dir#chdir case makes sense to me. A directory may not even know the path (e.g. Dir.for_fd(dir_fd).chdir), and yielding the file descriptor doesn't seem helpful (plus it isn't portable).

Note that whether the block is passed an argument in the Dir#chdir case depends on the platform. If the platform supports fchdir, then no argument is passed, but if it does not support an argument, then the path is passed. This isn't optimal, ideally no argument should passed in any case. That will take a little refactoring.

[zverok (Victor Shepelev)]

Not passing an argument to the block in the Dir.fchdir or Dir#chdir case makes sense to me.

Yup, I can see the reasoning, that's why I don't think it is a big problem. (I thought that may be passing self, i.e. an instance of Dir inside the block might be a good idea, but the implementation doesn't allow that.)

On the other hand, the return value of the block in #chdir case should be fixed, don't you think? I think this should do, probably...

dir_chdir(VALUE dir)
{
#if defined(HAVE_FCHDIR) && defined(HAVE_DIRFD) && HAVE_FCHDIR && HAVE_DIRFD
-    dir_s_fchdir(rb_cDir, dir_fileno(dir));
+    return dir_s_fchdir(rb_cDir, dir_fileno(dir));
#else
    VALUE path = dir_get(dir)->path;
-    dir_s_chdir(1, &path, rb_cDir);
+    return dir_s_chdir(1, &path, rb_cDir);
#endif
-
-    return Qnil;
}

[jeremyevans0 (Jeremy Evans)]

zverok (Victor Shepelev) wrote in #note-5:

On the other hand, the return value of the block in #chdir case should be fixed, don't you think? I think this should do, probably...

That's more or less the same diff I came up with before I determined we need more refactoring so that Dir#chdir never yields an argument to the block.

[zverok (Victor Shepelev)]

That's more or less the same diff I came up with before I determined we need more refactoring so that Dir#chdir never yields an argument to the block.

I provided the diff to solve return value problem for the block form. Which is not a big problem, but still might be confusing:

Dir.chdir('doc') do 
  # ...some more logic or calling other modules
  # ...which might do something like...
  Dir['*.md'].map { File.read(_1) }.join("\n\n")
end #=> what was calculated in the block

Dir.new('doc').chdir do 
  Dir['*.md'].map { File.read(_1) }.join("\n\n")
end #=> nil

This is simply fixed by returning what the called method have returned, right? Whatever the implementation is chosen.

[jeremyevans0 (Jeremy Evans)]

zverok (Victor Shepelev) wrote in #note-7:

This is simply fixed by returning what the called method have returned, right? Whatever the implementation is chosen.

It's not that simple. It does fix the block return value, but doesn't fix the Dir#chdir block yielding the path if fchdir is not supported. More refactoring is needed for that, you cannot just reuse the Dir.chdir code in that case, since that yields the path.

[zverok (Victor Shepelev)]

It does fix the block return value, but doesn't fix the Dir#chdir block yielding the path if fchdir is not supported.

I understand that. But for now, just fixing the return value (while ignoring args inconsistency) before 3.3 seems more reasonable than not fixing it?

Or do you plan the deeper refactoring soon?

[jeremyevans0 (Jeremy Evans)]

zverok (Victor Shepelev) wrote in #note-9:

Or do you plan the deeper refactoring soon?

Yes. I'll work on a pull request tonight. The refactoring necessary is not extensive.

[jeremyevans0 (Jeremy Evans)]

• Tracker changed from Misc to Bug
• Status changed from Open to Closed
• Backport set to 3.0: UNKNOWN, 3.1: UNKNOWN, 3.2: UNKNOWN

Fixed by f49af3c969eb7ba9937514a229b49e5b7d91f0f1