Implement `os: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

[krader1961]

@xiaq: I see you asked on the go-nuts mailing list why os.RemoveAll explicitly returns nil rather than an error when the path does not exist. The answer you received is what I expected. That is, if the path doesn't exist then attempting to "remove all" the files rooted at the path should be considered successful. That is arguably reasonable behavior for a typical Go program. However, I don't think that is reasonable behavior for the os:remove command implemented by this change. To mimic the behavior of the legacy Unix rm command it should be necessary to explicitly ask that a path not exist via the &missing-ok option. It should not be sufficient to simply ask for a recursive removal using the &recursive option to ignore a missing path. Obviously this could also be resolved by explicitly testing whether the path exists if &missing-ok is false and &recursive is true; thus allowing the use of os.RemoveAll. But that introduces a race condition and it is not obvious that solution is preferable. Especially since os.RemoveAll only exposes one path error whereas my existing implementation exposes every path removal error. Which seems more consistent with the non-recursive case.

[xiaq]

@xiaq: I see you asked on the go-nuts mailing list why os.RemoveAll explicitly returns nil rather than an error when the path does not exist. The answer you received is what I expected. That is, if the path doesn't exist then attempting to "remove all" the files rooted at the path should be considered successful. That is arguably reasonable behavior for a typical Go program.

The answer I got was from a community member, so it may or may not be what the original designer of os.RemoveAll had in mind. In particular, it doesn't explain why os.Remove doesn't swallow the same error. I have some theories, but in any case the exact reason is not very important.

Obviously this could also be resolved by explicitly testing whether the path exists if &missing-ok is false and &recursive is true; thus allowing the use of os.RemoveAll. But that introduces a race condition and it is not obvious that solution is preferable. Especially since os.RemoveAll only exposes one path error whereas my existing implementation exposes every path removal error. Which seems more consistent with the non-recursive case.

Race conditions are already unavoidable in anything that involves multiple filesystem operations. In the recursiveRemove function of your implementation, if the named file did not exist before the first os.Remove call but gets created after it, recursiveRemove returns an error for missing file and leaves the file unremoved. Therefore an implementation that checks the existence of the file before calling os.RemovaAll doesn't seem to make things worse.

My preference is to expose thin wrappers of os.Remove and os.RemoveAll. Elvish code that wishes to error on non-existent files can always check itself (assuming a new os:exists command):

var path = ...
if (not (os:exists $path)) {
  fail 'File to remove doesn't exist'
}
os:remove-all $path

However, I don't mean giving up handling race conditions entirely. One particular race condition is worth handling correctly: if the working directory changes while os:remove-all ./relative/path is running, the correct behavior would be to remove files under relative/path relative to the original working directory. This can be fixed by resolving relative paths to absolute paths as the first step.

I also don't feel supporting multiple arguments is that necessary - interactive users will likely continue to use rm anyway, this is more for cross-platform scripting.

Thanks for the contribution, but since my preferred implementation will use very little of this code, I'll close this PR.