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

Comments

Projects
None yet
@kelseyhightower
Contributor

kelseyhightower commented Oct 4, 2014

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

This comment has been minimized.

Contributor

kelseyhightower commented Oct 4, 2014

Comment 1:

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

This comment has been minimized.

gopherbot commented Oct 4, 2014

Comment 2:

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

This comment has been minimized.

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

This comment has been minimized.

Contributor

kelseyhightower commented Oct 4, 2014

Comment 4:

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

This comment has been minimized.

Contributor

kelseyhightower commented Oct 4, 2014

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

This comment has been minimized.

Contributor

ianlancetaylor commented Oct 5, 2014

Comment 6:

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

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

@adg

This comment has been minimized.

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

@bradfitz bradfitz removed the release-go1.5 label Dec 16, 2014

@kelseyhightower

This comment has been minimized.

Contributor

kelseyhightower commented Dec 16, 2014

@bradfitz bradfitz removed the new label Dec 18, 2014

@bradfitz

This comment has been minimized.

Member

bradfitz commented Dec 18, 2014

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

This comment has been minimized.

Member

bradfitz commented Dec 18, 2014

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

@minux

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

rjeczalik commented Dec 18, 2014

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

This comment has been minimized.

Contributor

davecheney commented Dec 18, 2014

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

This comment has been minimized.

Contributor

kelseyhightower commented Dec 18, 2014

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

@kelseyhightower

This comment has been minimized.

Contributor

kelseyhightower commented Dec 18, 2014

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

@rjeczalik

This comment has been minimized.

rjeczalik commented Dec 18, 2014

@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

This comment has been minimized.

Contributor

kelseyhightower commented Dec 18, 2014

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

@kelseyhightower

This comment has been minimized.

Contributor

kelseyhightower commented Dec 26, 2014

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

This comment has been minimized.

Contributor

kelseyhightower commented Dec 26, 2014

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

@mikioh mikioh removed this from the Go1.5 milestone Mar 10, 2015

@golang golang locked and limited conversation to collaborators Jun 25, 2016

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