Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DOC] Enhanced RDoc for FileUtils #78

Merged
merged 9 commits into from
Jun 6, 2022
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 14 additions & 9 deletions lib/fileutils.rb
Original file line number Diff line number Diff line change
Expand Up @@ -951,18 +951,19 @@ def mv(src, dest, force: nil, noop: nil, verbose: nil, secure: nil)
alias move mv
module_function :move

# Removes entries at the paths given in array +list+; returns +list+.
# Removes entries at the paths given in +list+,
# which should be a string path or an array of string paths; returns +list+.
jeremyevans marked this conversation as resolved.
Show resolved Hide resolved
#
# With no keyword arguments, returns files at the paths given in +list+:
# With no keyword arguments, removes files at the paths given in +list+:
#
# FileUtils.touch(['src0.txt', 'src0.dat'])
# FileUtils.rm(['src0.dat', 'src0.txt']) # => ["src0.dat", "src0.txt"]
#
# Keyword arguments:
#
# - <tt>force: true</tt> - attempts to remove files regardless of permissions;
# ignores raised exceptions of StandardError and its descendants:
# - <tt>noop: true</tt> - does not remove files.
# ignores raised exceptions of StandardError and its descendants.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The previous description

All StandardErrors are ignored when the :force option is set.

is actually more descriptive of the current behavior. Currently the only difference between force: true and force: false is that force: true makes any StandardErrors while trying to remove files be ignored. But if a file is impossible to remove due to permissions, the removal will be attempted regardless of force, and the files won't get removed regardless of force.

For what it's worth, I think this behaviour is harmful, I opened a ruby-core ticket at https://bugs.ruby-lang.org/issues/18784. But unless the behavior is changed, the documentation should explain the current behavior.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

# - <tt>noop: true</tt> - does not remove files; returns +nil+.
# - <tt>verbose: true</tt> - prints an equivalent command:
#
# FileUtils.rm(['src0.dat', 'src0.txt'], noop: true, verbose: true)
Expand Down Expand Up @@ -991,7 +992,7 @@ def rm(list, force: nil, noop: nil, verbose: nil)
#
# FileUtils.rm(list, force: true, **kwargs)
#
# See FileUtils.rm for keyword arguments +noop+ and +verbose+.
# See FileUtils.rm for keyword arguments.
#
# FileUtils.safe_unlink is an alias for FileUtils.rm_f.
#
Expand All @@ -1006,6 +1007,9 @@ def rm_f(list, noop: nil, verbose: nil)
# Removes files and directories at the paths given in array +list+;
# returns +list+.
#
# May cause a local vulnerability if not called with keyword argument
# <tt>secure: true</tt>.
jeremyevans marked this conversation as resolved.
Show resolved Hide resolved
#
# For each file path, removes the file at that path:
#
# FileUtils.touch(['src0.txt', 'src0.dat'])
Expand All @@ -1029,7 +1033,7 @@ def rm_f(list, noop: nil, verbose: nil)
# Keyword arguments:
#
# - <tt>force: true</tt> - attempts to remove entries regardless of permissions;
# ignores raised exceptions of StandardError and its descendants:
# ignores raised exceptions of StandardError and its descendants.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto regarding force.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also fixed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't seem fixed, no? The sentence about permissions is still there. You did remove some stuff regarding force in other methods that take care of moving files, but I'm not sure how force behaves there, so I'm not sure if that's correct or not.

# - <tt>noop: true</tt> - does not remove entries.
# - <tt>secure: true</tt> - removes +src+ securely;
# see details at FileUtils.remove_entry_secure.
Expand Down Expand Up @@ -1061,9 +1065,10 @@ def rm_r(list, force: nil, noop: nil, verbose: nil, secure: nil)
#
# FileUtils.rm_r(list, force: true, **kwargs)
#
# See FileUtils.rm_r for keyword arguments +noop+ and +verbose+,
# and especially for keyword argument +secure+,
# which relates to security and vulnerability.
# May cause a local vulnerability if not called with keyword argument
# <tt>secure: true</tt>.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a link to the place that explains this vulnerability?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll be reworking all about secure and vulnerabilities, putting it into the doc for the module, and adding links as needed.

#
# See FileUtils.rm_r for keyword arguments.
#
# FileUtils.rmtree is an alias for FileUtils.rm_rf.
#
Expand Down