Feature #17296: Feature: Pathname#chmod use FileUtils.chmod instead of File

• Status: Feedback
• Assignee: akr (Akira Tanaka)
• Target version: -

[schneems (Richard Schneeman)]

The FileUtils.chmod provides the same numerical interface as File.chmod and it also includes a "symbolic mode" interface. With this patch you'll be able to run this code:

Pathname.new("bin/compile").chmod("+x")

I believe that this is backwards compatible with the existing implementation and all changes are an extension. The only difference between File.chmod and FileUtils.chmod I could find is they have different return values and the previous implementation of Pathname#chmod returned the result of the File.chmod call. From the docs File.chmod returns the number of files modified, since we're only ever able to pass in a maximum of one file through this interface, the return value will always be a 1 or an exception if the file does not exist.

I checked and the exceptions when the file does not exist match:

irb(main):004:0> File.chmod(0444, "doesnotexist.txt")
Traceback (most recent call last):
        6: from /Users/rschneeman/.rubies/ruby-2.7.2/bin/irb:23:in `<main>'
        5: from /Users/rschneeman/.rubies/ruby-2.7.2/bin/irb:23:in `load'
        4: from /Users/rschneeman/.rubies/ruby-2.7.2/lib/ruby/gems/2.7.0/gems/irb-1.2.6/exe/irb:11:in `<top (required)>'
        3: from (irb):3
        2: from (irb):4:in `rescue in irb_binding'
        1: from (irb):4:in `chmod'
Errno::ENOENT (No such file or directory @ apply2files - doesnotexist.txt)
irb(main):005:0> FileUtils.chmod(0444, "doesnotexist.txt")
Traceback (most recent call last):
       10: from /Users/rschneeman/.rubies/ruby-2.7.2/bin/irb:23:in `<main>'
        9: from /Users/rschneeman/.rubies/ruby-2.7.2/bin/irb:23:in `load'
        8: from /Users/rschneeman/.rubies/ruby-2.7.2/lib/ruby/gems/2.7.0/gems/irb-1.2.6/exe/irb:11:in `<top (required)>'
        7: from (irb):4
        6: from (irb):5:in `rescue in irb_binding'
        5: from /Users/rschneeman/.rubies/ruby-2.7.2/lib/ruby/2.7.0/fileutils.rb:1016:in `chmod'
        4: from /Users/rschneeman/.rubies/ruby-2.7.2/lib/ruby/2.7.0/fileutils.rb:1016:in `each'
        3: from /Users/rschneeman/.rubies/ruby-2.7.2/lib/ruby/2.7.0/fileutils.rb:1017:in `block in chmod'
        2: from /Users/rschneeman/.rubies/ruby-2.7.2/lib/ruby/2.7.0/fileutils.rb:1346:in `chmod'
        1: from /Users/rschneeman/.rubies/ruby-2.7.2/lib/ruby/2.7.0/fileutils.rb:1346:in `chmod'
Errno::ENOENT (No such file or directory @ apply2files - doesnotexist.txt)

If you're open to changing the interface of the return value my preference would be to return self from this method so that it can be chained. Otherwise this current patch is a smaller change.

Diff:

$ git diff master
diff --git a/ext/pathname/lib/pathname.rb b/ext/pathname/lib/pathname.rb
index e6fb90277d..cb6e32d9ac 100644
--- a/ext/pathname/lib/pathname.rb
+++ b/ext/pathname/lib/pathname.rb
@@ -585,6 +585,15 @@ def mkpath
     nil
   end

+  # Changes file permissions.
+  #
+  # See FileUtils.chmod
+  def chmod(mode)
+    require 'fileutils'
+    FileUtils.chmod(mode, self)
+    return 1
+  end
+
   # Recursively deletes a directory, including all directories beneath it.
   #
   # See FileUtils.rm_r
diff --git a/ext/pathname/pathname.c b/ext/pathname/pathname.c
index f71cec1b25..6778d4f102 100644
--- a/ext/pathname/pathname.c
+++ b/ext/pathname/pathname.c
@@ -12,7 +12,6 @@ static ID id_binwrite;
 static ID id_birthtime;
 static ID id_blockdev_p;
 static ID id_chardev_p;
-static ID id_chmod;
 static ID id_chown;
 static ID id_ctime;
 static ID id_directory_p;
@@ -552,20 +551,6 @@ path_mtime(VALUE self)
     return rb_funcall(rb_cFile, id_mtime, 1, get_strpath(self));
 }

-/*
- * call-seq:
- *   pathname.chmod(mode_int)  -> integer
- *
- * Changes file permissions.
- *
- * See File.chmod.
- */
-static VALUE
-path_chmod(VALUE self, VALUE mode)
-{
-    return rb_funcall(rb_cFile, id_chmod, 2, mode, get_strpath(self));
-}
-
 /*
  * call-seq:
  *   pathname.lchmod(mode_int) -> integer
@@ -1448,7 +1433,6 @@ path_f_pathname(VALUE self, VALUE str)
  * - #birthtime
  * - #ctime
  * - #mtime
- * - #chmod(mode)
  * - #lchmod(mode)
  * - #chown(owner, group)
  * - #lchown(owner, group)
@@ -1495,6 +1479,7 @@ path_f_pathname(VALUE self, VALUE str)
  * === Utilities
  *
  * These methods are a mixture of Find, FileUtils, and others:
+ * - #chmod(mode)
  * - #find(&block)
  * - #mkpath
  * - #rmtree
@@ -1542,7 +1527,6 @@ Init_pathname(void)
     rb_define_method(rb_cPathname, "birthtime", path_birthtime, 0);
     rb_define_method(rb_cPathname, "ctime", path_ctime, 0);
     rb_define_method(rb_cPathname, "mtime", path_mtime, 0);
-    rb_define_method(rb_cPathname, "chmod", path_chmod, 1);
     rb_define_method(rb_cPathname, "lchmod", path_lchmod, 1);
     rb_define_method(rb_cPathname, "chown", path_chown, 2);
     rb_define_method(rb_cPathname, "lchown", path_lchown, 2);
@@ -1618,7 +1602,6 @@ InitVM_pathname(void)
     id_birthtime = rb_intern("birthtime");
     id_blockdev_p = rb_intern("blockdev?");
     id_chardev_p = rb_intern("chardev?");
-    id_chmod = rb_intern("chmod");
     id_chown = rb_intern("chown");
     id_ctime = rb_intern("ctime");
     id_directory_p = rb_intern("directory?");
diff --git a/test/pathname/test_pathname.rb b/test/pathname/test_pathname.rb
index 43cef4849f..5673691231 100644
--- a/test/pathname/test_pathname.rb
+++ b/test/pathname/test_pathname.rb
@@ -823,6 +823,11 @@ def test_chmod
       path.chmod(0444)
       assert_equal(0444, path.stat.mode & 0777)
       path.chmod(old)
+
+      skip "Windows has different symbolic mode" if /mswin|mingw/ =~ RUBY_PLATFORM
+      path.chmod("u=wrx,g=rx,o=x")
+      assert_equal(0751, path.stat.mode & 0777)
+      path.chmod(old)
     }
   end

Github link: https://github.com/ruby/ruby/pull/3708

[Eregon (Benoit Daloze)]

• Assignee set to akr (Akira Tanaka)

Sounds good to me.
@akr (Akira Tanaka) Could you approve?

[hsbt (Hiroshi SHIBATA)]

• Status changed from Open to Assigned

[mame (Yusuke Endoh)]

• Status changed from Assigned to Feedback

It was discussed at the dev meeting.

File.chmod and FileUtils.chmod have an incompatibility in their handling of symbolic links: File.chmod changes the permissions on the target of the symbolic link, while FileUtils.chmod changes the permissions on the symbolic link itself in environments with lchmod itself, and no-op in environments without lchmod.

Therefore, it seems better to have File.chmod support "+x", etc., rather than using FileUtils.chmod.