* [ruby-core:100670] [Ruby master Feature#17296] Feature: Pathname#chmod use FileUtils.chmod instead of File
@ 2020-10-30 15:08 get.codetriage
2020-11-19 19:18 ` [ruby-core:100948] " eregontp
2024-10-03 9:44 ` [ruby-core:119430] " mame (Yusuke Endoh) via ruby-core
0 siblings, 2 replies; 3+ messages in thread
From: get.codetriage @ 2020-10-30 15:08 UTC (permalink / raw)
To: ruby-core
Issue #17296 has been reported by schneems (Richard Schneeman).
----------------------------------------
Feature #17296: Feature: Pathname#chmod use FileUtils.chmod instead of File
https://bugs.ruby-lang.org/issues/17296
* Author: schneems (Richard Schneeman)
* Status: Open
* Priority: Normal
----------------------------------------
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:
```ruby
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
--
https://bugs.ruby-lang.org/
^ permalink raw reply [flat|nested] 3+ messages in thread
* [ruby-core:100948] [Ruby master Feature#17296] Feature: Pathname#chmod use FileUtils.chmod instead of File
2020-10-30 15:08 [ruby-core:100670] [Ruby master Feature#17296] Feature: Pathname#chmod use FileUtils.chmod instead of File get.codetriage
@ 2020-11-19 19:18 ` eregontp
2024-10-03 9:44 ` [ruby-core:119430] " mame (Yusuke Endoh) via ruby-core
1 sibling, 0 replies; 3+ messages in thread
From: eregontp @ 2020-11-19 19:18 UTC (permalink / raw)
To: ruby-core
Issue #17296 has been updated by Eregon (Benoit Daloze).
Assignee set to akr (Akira Tanaka)
Sounds good to me.
@akr Could you approve?
----------------------------------------
Feature #17296: Feature: Pathname#chmod use FileUtils.chmod instead of File
https://bugs.ruby-lang.org/issues/17296#change-88603
* Author: schneems (Richard Schneeman)
* Status: Open
* Priority: Normal
* Assignee: akr (Akira Tanaka)
----------------------------------------
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:
```ruby
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
--
https://bugs.ruby-lang.org/
^ permalink raw reply [flat|nested] 3+ messages in thread
* [ruby-core:119430] [Ruby master Feature#17296] Feature: Pathname#chmod use FileUtils.chmod instead of File
2020-10-30 15:08 [ruby-core:100670] [Ruby master Feature#17296] Feature: Pathname#chmod use FileUtils.chmod instead of File get.codetriage
2020-11-19 19:18 ` [ruby-core:100948] " eregontp
@ 2024-10-03 9:44 ` mame (Yusuke Endoh) via ruby-core
1 sibling, 0 replies; 3+ messages in thread
From: mame (Yusuke Endoh) via ruby-core @ 2024-10-03 9:44 UTC (permalink / raw)
To: ruby-core; +Cc: mame (Yusuke Endoh)
Issue #17296 has been updated by 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`.
----------------------------------------
Feature #17296: Feature: Pathname#chmod use FileUtils.chmod instead of File
https://bugs.ruby-lang.org/issues/17296#change-110045
* Author: schneems (Richard Schneeman)
* Status: Feedback
* Assignee: akr (Akira Tanaka)
----------------------------------------
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:
```ruby
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
--
https://bugs.ruby-lang.org/
______________________________________________
ruby-core mailing list -- ruby-core@ml.ruby-lang.org
To unsubscribe send an email to ruby-core-leave@ml.ruby-lang.org
ruby-core info -- https://ml.ruby-lang.org/mailman3/lists/ruby-core.ml.ruby-lang.org/
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-10-03 9:44 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-30 15:08 [ruby-core:100670] [Ruby master Feature#17296] Feature: Pathname#chmod use FileUtils.chmod instead of File get.codetriage
2020-11-19 19:18 ` [ruby-core:100948] " eregontp
2024-10-03 9:44 ` [ruby-core:119430] " mame (Yusuke Endoh) via ruby-core
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).