Implement path:remove

[krader1961]

I considered adhering more closely to the traditional Unix rm and rmdir commands. For example, by implementing each independently and supporting the --force option. However, I concluded there are insufficient reasons to do so. In particular, the POSIX --force (or -f) option is most often used solely for its side-effect of making elimination of a non-existent path a non-error. I also don't like that -f also fixes permission errors to allow allow removing files. If we decide that feature is needed it should be added under a distinct option (e.g. &fix-perms).

Related: #1659

[xiaq]

• I don't see a good reason to deviate from Go, which has os.Remove (non-recursive) and os.RemoveAll (recursive). It's not clear why you reimplemented recursive removal instead of using os.RemoveAll.

• I feel &ignore-missing is better named &missing-ok (there's precedent in *[nomatch-ok]). It is not exactly ambiguous, but it requires a bit of thinking to deduce what "ignore" exactly entails (in this case, ignoring any error from missing files).

I also feel this command shouldn't live in path. It's my fault for letting in is-dir, is-regular, temp-dir and temp-file, but I'd like to make path just about manipulating paths themselves, and create a new os module to mirror Go's os package. But feel free to leave it there and I'll do the cleanup after merging.

[krader1961]

I mostly agree with your feedback, @xiaq. I need to think about whether a new os module is preferable. On the one hand I am inclined to agree given issue #1659 that I opened. I even noted this tension on where such commands should live in that issue. The question is how tightly we want to bind Elvish builtins to the Go package that underlies their functionality? This obviously has implications for the path:stat command I want to see merged. Should that also be named os:stat?

[krader1961]

I don't see a good reason to deviate from Go, which has os.Remove (non-recursive) and os.RemoveAll (recursive). It's not clear why you reimplemented recursive removal instead of using os.RemoveAll.

Because the Go os.RemoveAll documentation says this:

RemoveAll removes path and any children it contains. It removes everything it can but returns the first error it encounters. If the path does not exist, RemoveAll returns nil (no error). If there is an error, it will be of type *PathError.

It is not clear that is the preferable behavior to expose since it elides (hides) some errors. My implementation exposes all errors. I also haven't tested the os.RemoveAll behavior. Does it actually delete all files that can be deleted even if there is an error deleting a particular path? The documentation of os.RemoveAll is ambiguous on that point.

[krader1961]

I don't see a good reason to deviate from Go, which has os.Remove (non-recursive) and os.RemoveAll (recursive). It's not clear why you reimplemented recursive removal instead of using os.RemoveAll.

The main problem with using os.RemoveAll is that it silently ignores a non-existent path. Which we only want to do if the &missing-ok (or &ignore-missing) is true. So using that Go function requires adding an explicit test whether the path exists. It is simpler to just use os.Remove and recurse if the path is a non-empty directory. Not to mention aggregating removal errors which os.RemoveAll makes possible while os.Remove does not.

I am going to close this P.R. since the implementation is moving from the path module to a new os module in light of previous feedback. The new P.R. will include a comment why the Go os.RemoveAll is not used.