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

os: Remove documentation does not explain how it differs from RemoveAll #26507

Closed
seebs opened this issue Jul 20, 2018 · 3 comments
Closed

os: Remove documentation does not explain how it differs from RemoveAll #26507

seebs opened this issue Jul 20, 2018 · 3 comments

Comments

@seebs
Copy link
Contributor

@seebs seebs commented Jul 20, 2018

What version of Go are you using (go version)?

1.10

Does this issue reproduce with the latest release?

yes

What operating system and processor architecture are you using (go env)?

N/A (documentation)

What did you do?

Read documentation.

What did you expect to see?

A description of what os.Remove does that might differ from os.RemoveAll. :)

What did you see instead?

The blank assertion that os.Remove "removes a file".

ISO C's remove will remove a file or directory, but fail if the directory is empty. On the other hand, rm will remove only files, if not given the -r flag, and you need rmdir to remove a directory. I have heard horror stories about filesystems where you could remove a directory that wasn't empty, and nightmarish things occurred. (I hope this isn't real.)

My point is: I think it would be good for the docs for os.Remove to explicitly state behavior for likely edge cases, such as "can remove both files and directories" or "will not remove directories that have other contents." A quick straw poll revealed at least one person who assumed it would definitely not work on empty directories, and I suspect there are people out there assuming it will remove directories even if they are empty (probably recursively). A sentence outlining the intended behavior would reduce confusion.

@robpike

This comment has been minimized.

Copy link
Contributor

@robpike robpike commented Jul 20, 2018

Not a helpful comment: I wish that "rm" just removed an empty directory, that I didn't need to use "rmdir".

More helpful: "rm" and "rmdir" both use the underlying "unlink" system call on Unix; the different experiences they provide is part of their design. The OS just has one system call, which we wrap with os.Remove. We can always add documentation but I put up mild resistance about documenting all the nuances of the OSes, which are expansive and can vary.

Consider permissions: Do we need to add a paragraph to every os function explaining how the permissions of the operating system can affect the result? Because that's pretty much what's happening here: You are permitted to remove an empty directory, but not a non-empty one.

I have no idea what Windows does here, but I know that os.Remove on Windows behaves the same as elsewhere. I like that.

It might be OK to say,

Remove removes the named file or (empty) directory.

adding the parenthetical. I would protest at doing more than that.

@seebs

This comment has been minimized.

Copy link
Contributor Author

@seebs seebs commented Jul 20, 2018

I think the distinction I'd draw with the permissions case is that the permissions behavior is fairly consistent in general. Directory removal has been fairly inconsistent bbetween programming languages. Users who aren't used to thinking at the syscall level are quite possibly used to "drag file to trash" or something similar, and may not expect os.Remove() to fail even on non-empty directories; users who are used to thinking in syscalls may think "directories need a different remove operation than files". My first thought was that if a thing removes a file-or-directory, it presumably removes directories recursively; I only guessed otherwise because the adjacent RemoveAll implied a distinction.

I agree that the (empty) qualifier is enough to make the intent unambiguous. (But in fact, I wasn't even 100% sure that this was the intent, as opposed to how it happened to work on existing targets.)

(And I agree about rm/rmdir, but I think we may be >45 years late to win that one.)

@ianlancetaylor ianlancetaylor changed the title os.Remove documentation does not specify a thing that is probably intended os: Remove documentation does not explain how it differs from RemoveAll Aug 3, 2018
@ianlancetaylor ianlancetaylor added this to the Go1.12 milestone Aug 3, 2018
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Aug 3, 2018

Change https://golang.org/cl/127835 mentions this issue: os: document that Remove removes only empty directories

@gopherbot gopherbot closed this in 0bad634 Aug 3, 2018
@golang golang locked and limited conversation to collaborators Aug 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.