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

io/ioutil: Add CopyFile #8868

Closed
kelseyhightower opened this issue Oct 4, 2014 · 21 comments
Closed

io/ioutil: Add CopyFile #8868

kelseyhightower opened this issue Oct 4, 2014 · 21 comments

Comments

@kelseyhightower
Copy link
Contributor

Currently the standard library has the ioutil.ReadFile and ioutil.WriteFile functions,
but no ioutil.CopyFile function. As previouslly discussed in CL 1758041 -
https://golang.org/cl/1758041, the ioutil.CopyFile function should be simple.
@kelseyhightower
Copy link
Contributor Author

Comment 1:

I've proposed the following CL for this issue:
https://golang.org/cl/152180043

@gopherbot
Copy link

Comment 2:

CL https://golang.org/cl/152180043 mentions this issue.

@minux
Copy link
Member

minux commented Oct 4, 2014

Comment 3:

i don't think this need to be in the standard packages (no matter how easy it is to
implement it).
Besides, we are already in code freeze for 1.4.

Labels changed: added release-none, repo-main.

@kelseyhightower
Copy link
Contributor Author

Comment 4:

Thanks for the feedback, but I think a CopyFile function adds value considering we have
ReadFile and WriteFile already.

@kelseyhightower
Copy link
Contributor Author

Comment 5:

This request seemed to have the right amount of support in the past, but the previous CL
was never completed. The current CL at https://golang.org/cl/152180043
incorporates all of the feedback from the previous one at
https://golang.org/cl/1758041, but simplifies things a bit.

@ianlancetaylor
Copy link
Contributor

Comment 6:

We should make a decision one way or another for 1.5.

Labels changed: added release-go1.5, removed release-none.

@adg
Copy link
Contributor

adg commented Oct 6, 2014

Comment 7:

+1 for inclusion in 1.5.
I've implemented it a few times in various places. Its presence in ioutil would have
saved me some trouble. We can't write a version that will suit everyone, but we can
write one that is "correct". I don't see a downside.

@bradfitz bradfitz modified the milestone: Go1.5 Dec 16, 2014
@kelseyhightower
Copy link
Contributor Author

New CL posted here: https://go-review.googlesource.com/#/c/1591

@bradfitz bradfitz removed the new label Dec 18, 2014
@bradfitz
Copy link
Contributor

Like @adg, I would also like this function and find myself often re-implementing it, but there wasn't general agreement before the CL was proposed. (the code looks fine)

Assigning to @robpike for a decision.

/cc @rsc

@bradfitz
Copy link
Contributor

(That is, assigning to @robpike to own finding a decision, not making it)

@minux
Copy link
Member

minux commented Dec 18, 2014

What should be the correct order for the arguments? dst first or src first?
Normally, for Go APIs, the convention seems to be dst first, however, for
filename arguments, os.Rename, os.Symlink and os.Link uses the other
order. CopyFile is similar to those, so should it use the src then dst
order?

On a related note, do we need io/ioutil.MoveFile?
(it should first try os.Rename(src, dst), and if that fails, calls
CopyFile(dst, src),
and then os.Remove(src))

@mattn
Copy link
Member

mattn commented Dec 18, 2014

I hope ioutil.MoveFile use sys.MoveFile on windows.
And about order of arguments. We know memcpy(dst, src, size); so that we may think ioutil.CopyFile(dst, src). But src, dst seems general?

python

shutil.copyfile(src, dst)

java

Path.copy(src, dst, options)

perl

use File::Copy;
copy(src, dst);

ruby

File.copy(src, dst)

etc

sendfile(output_fd, input_fd, &offset, size)

shell

$ cp src.txt dst.txt

@minux
Copy link
Member

minux commented Dec 18, 2014

Other issues worth considering for this API:
Some filesystems (HFS+ on OS X, NTFS) support additional data streams (resource forks and
alternative data streams, respectively).

File systems on Linux has the SELinux context, POSIX ACL and extended attributes.
I'm sure other systems have other additional attributes that ideally a file copying program should
try to preserve (or document that it doesn't care about those).

I don't mean to propose that ioutil.CopyFile should handle all those complexities, but we need to
determine the proper semantics of the API and document it.

@rjeczalik
Copy link

I'd also consider simplifying API by removing perm os.FileMode argument. Currently it's:

func CopyFile(src, dst string, perm os.FileMode) error

The rationale is that dst file eventually created by CopyFile should either inherit permissions from src file (the cp -a behaviour) or use default ones (0777&^umask). If user wants to have different perm, he/she needs to use os.Chmod explicitely. We'd have then one less thing that CopyFile could fail on.

@davecheney
Copy link
Contributor

The rationale is that file created by CopyFile should either inherit
permissions from src file (the cp -a behaviour) or use default ones (0777 &
^umask). If user wants to have different perm, he/she needs to use os.Chmod
explicitely. We'd have then one less thing that CopyFile could fail on.

I agree


Reply to this email directly or view it on GitHub.

@kelseyhightower
Copy link
Contributor Author

@rjeczalik @davecheney The CopyFile function sets the perm to be consistent with WriteFile.

@kelseyhightower
Copy link
Contributor Author

@mattn @minux The order of the args is consistent with io.Copy and io.CopyN

@rjeczalik
Copy link

@kelseyhightower

My take on that is the WriteFile requires perm, as it has not enough information to create a file for user. On the other hand CopyFile has already access to dst file's permissions, so it can proceed without requiring this information from the user. I'd rather not make those two consistent, in my opinion it feels artificial.

@kelseyhightower
Copy link
Contributor Author

@rjeczalik Ok, I don't feel strongly about CopyFile setting permissions. So we can just remove it.

@kelseyhightower
Copy link
Contributor Author

Based on the feedback from @robpike in https://go-review.googlesource.com/#/c/1591, it seems that we should not do this in the ioutil package. It's easy enough to write this bit of code when I need it so I'm happy to close this issue.

//cc @bradfitz @adg

@kelseyhightower
Copy link
Contributor Author

Ok, I've abandoned this change here: https://go-review.googlesource.com/#/c/1591. Please feel free to close.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests