|
|
Created:
13 years, 8 months ago by Kyle Lemons Modified:
2 years, 4 months ago CC:
golang-dev Visibility:
Public. |
Descriptionio/ioutil: add CopyFile
Patch Set 1 #Patch Set 2 : code review 1758041: Added CopyFile(from, to string) os.Error #
Total comments: 7
Patch Set 3 : code review 1758041: Added CopyFile(from, to string) os.Error #Patch Set 4 : code review 1758041: Added CopyFile(from, to string) os.Error #
Total comments: 3
Patch Set 5 : code review 1758041: Added CopyFile(from, to string) os.Error #
Total comments: 1
Patch Set 6 : code review 1758041: Added CopyFile(from, to string) os.Error #Patch Set 7 : code review 1758041: Added CopyFile(from, to string) os.Error #Patch Set 8 : code review 1758041: Added CopyFile(from, to string) os.Error #
Total comments: 2
Patch Set 9 : code review 1758041: Added CopyFile(from, to string) os.Error #
Total comments: 10
MessagesTotal messages: 28
Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com), I'd like you to review this change.
Sign in to reply to this message.
http://codereview.appspot.com/1758041/diff/2001/3001 File src/pkg/io/ioutil/ioutil.go (right): http://codereview.appspot.com/1758041/diff/2001/3001#newcode50 src/pkg/io/ioutil/ioutil.go:50: func CopyFile(fromFile, toFile string) os.Error { s/fromFile/inFilename/ s/toFile/outFilename/ http://codereview.appspot.com/1758041/diff/2001/3001#newcode58 src/pkg/io/ioutil/ioutil.go:58: fOut, err := os.Open(toFile, os.O_WRONLY|os.O_TRUNC|os.O_CREAT, int(stat.Mode)) s/fOut/out/ http://codereview.appspot.com/1758041/diff/2001/3001#newcode61 src/pkg/io/ioutil/ioutil.go:61: } After each Open, put the Close in a defer. Eg, here: defer out.Close() http://codereview.appspot.com/1758041/diff/2001/3001#newcode64 src/pkg/io/ioutil/ioutil.go:64: fIn, err := os.Open(fromFile, os.O_RDONLY, 0) s/fIn/in/ http://codereview.appspot.com/1758041/diff/2001/3001#newcode70 src/pkg/io/ioutil/ioutil.go:70: _, err = io.Copy(fOut, fIn) I wonder if it's worth returning the number of bytes written. http://codereview.appspot.com/1758041/diff/2001/3001#newcode76 src/pkg/io/ioutil/ioutil.go:76: err = fIn.Close() Use the defer blocks instead. The error return value of a Close is irrelevant. http://codereview.appspot.com/1758041/diff/2001/3001#newcode88 src/pkg/io/ioutil/ioutil.go:88: stat, err = os.Stat(fromFile) Why perform Stat twice?
Sign in to reply to this message.
On Thu, Jul 8, 2010 at 4:52 AM, <etherealflaim@gmail.com> wrote: > Reviewers: golang-dev_googlegroups.com, > > Message: > Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com), > > I'd like you to review this change. > > > Description: > Added CopyFile(from, to string) os.Error > > This can't really be in syscall because some $GOOS don't support > SYS_COPYFILE or similar, and > this can't really be in os because the code for it isn't using anything > OS dependent. It seems > similar enough to things like ReadFile and WriteFile that I think > ioutil.CopyFile is appropriate. > > Please review this at http://codereview.appspot.com/1758041/show > > Affected files: > M src/pkg/io/ioutil/ioutil.go > M src/pkg/io/ioutil/ioutil_test.go > > > While I think this function is a great addition, I'm not convinced that it shouldn't be in os. Here's why: 1) I'd expect it to be in os if I didn't know where it was. 2) Most (if not all) OSes provide a way of copying one file to another that is probably more efficient than this method. If they don't, they can fall back to the read/write syscalls. It's already established that syscall is going to be different on different OSes, so it's okay if not all of them have file copying system calls. The os package can have different implementations for different OSes, as it does with many other functions. It just needs a consistent interface. On Windows, you can use the CopyFile Windows API function. Darwin has copyfile. Linux can use mmap, though I'm not positive this will be better than read/write. - Evan
Sign in to reply to this message.
> While I think this function is a great addition, I'm not convinced > that it shouldn't be in os. Here's why: > 1) I'd expect it to be in os if I didn't know where it was. > This I would agree with completely. I might also expect it to be called Copy(), but would that be too close to io.Copy and cause confusion? > It's already established that syscall is going to be different on > different OSes, so it's okay if not all of them have file copying > system calls. The os package can have different implementations for > different OSes, as it does with many other functions. It just needs a > consistent interface. > > On Windows, you can use the CopyFile Windows API function. Darwin has > copyfile. Linux can use mmap, though I'm not positive this will be > better than read/write. > Unfortunately, my darwin box is currently unavailable for development, and I don't have any live windows installs at present. If the consensus is that it should be in os, should I put it in os and let darwin/windows developers add in the appropriate hooks? ~Kyle
Sign in to reply to this message.
Hello golang-dev@googlegroups.com, adg (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
http://codereview.appspot.com/1758041/diff/13001/14001 File src/pkg/io/ioutil/ioutil.go (right): http://codereview.appspot.com/1758041/diff/13001/14001#newcode50 src/pkg/io/ioutil/ioutil.go:50: func CopyFile(inFilename, outFilename string) (numbytes int64, err os.Error) { s/numbytes/written/ http://codereview.appspot.com/1758041/diff/13001/14001#newcode51 src/pkg/io/ioutil/ioutil.go:51: // Initialize the return values We don't need to initialise the return values, as the default zero-values (0 and nil) are perfectly reasonable as-is. (-1 is not idiomatic) http://codereview.appspot.com/1758041/diff/13001/14002 File src/pkg/io/ioutil/ioutil_test.go (right): http://codereview.appspot.com/1758041/diff/13001/14002#newcode96 src/pkg/io/ioutil/ioutil_test.go:96: filename := "/etc/fstab" Instead of /etc/fstab (which won't exist on some systems), just read ioutil_test.go (which will always exist).
Sign in to reply to this message.
PTAL. > s/numbytes/written/ Fixed. > We don't need to initialise the return values, as the default zero-values (0 and > nil) are perfectly reasonable as-is. (-1 is not idiomatic) Not sure what I was thinking... fixed. > Instead of /etc/fstab (which won't exist on some systems), just read > ioutil_test.go (which will always exist). So simple! Fixed.
Sign in to reply to this message.
http://codereview.appspot.com/1758041/diff/21001/22001 File src/pkg/io/ioutil/ioutil.go (right): http://codereview.appspot.com/1758041/diff/21001/22001#newcode49 src/pkg/io/ioutil/ioutil.go:49: // CopyFile copies the named file's contents, m/atime, and permissions to the named file This function should not be OS dependent; it is.
Sign in to reply to this message.
> This function should not be OS dependent; it is. Are you refering to the fact that it uses *nix semantics for copying the mode and m/atime? All of the functions used within CopyFile are in the os package, and thus should map the *nix concepts to the host OS as appropriate. For instance, the mode passed to os.Open is the same mode that comes from os.Stat, as are the arguments to os.Chtimes. As long as an OS interprets the mode in Open the same way it generates the mode for Stat, it should behave as expected. In the test case, only the mode (as reported by os), the contents (as reported by ReadFile) and the mtime (as reported by os) are compared, and only against the values returned by the os package functions, and thus should be self-consistent. This function is completely isolated from the syscall package, where all OS dependent functionality should be implemented. Or so was my understanding. ~Kyle
Sign in to reply to this message.
LGTM Passing it on to Rob and Russ to see what they think.
Sign in to reply to this message.
this seemingly trivial operation is actually pretty complex. doing it right will require operating-system dependent code. there will also be pressure to enhance it or add variants with features to maintain permissions, control the open bits, and so on. i therefore vote against having this in the standard library. its existence presumes thoroughness, correctness, and portability we cannot provide.
Sign in to reply to this message.
On 2010/07/12 19:35:42, r wrote: > this seemingly trivial operation is actually pretty complex. doing it right will > require operating-system dependent code. there will also be pressure to enhance > it or add variants with features to maintain permissions, control the open bits, > and so on. > > i therefore vote against having this in the standard library. its existence > presumes thoroughness, correctness, and portability we cannot provide. i'm pretty scared by all the suggestions about mmap and Windows CopyFile API and sys_copyfile and such. i agree that the really complex one doesn't belong here. given that we already have ReadFile and WriteFile though, i think a CopyFile that just did copying - no messing with mtimes or permissions or special system calls or atomic rename or anything else - would be okay, but only if it stays that way. i can't tel whether it would.
Sign in to reply to this message.
> i'm pretty scared by all the suggestions about mmap > and Windows CopyFile API and sys_copyfile and such. I would personally agree... And this is one reason why I chose to put it in ioutil instead of os. If I saw an os.Copy, I would assume that that copy operation was suited to my os, whereas if I see ioutil,CopyFile, I would be more likely to assume it's simply a helper and not necessarily an optimal or even marginal solution, simply a convenient one. > i agree that the really complex one doesn't belong here. > given that we already have ReadFile and WriteFile though, > i think a CopyFile that just did copyingno messing with > mtimes or permissions or special system calls or atomic > rename or anything else - would be okay So, strip out the mtime and permission copies? That probably makes sense; the os functions are there if the caller wants to make those changes as well. > but only if it stays that way. i can't tel whether it would. I think the community thus far has, by and large, remained pretty diligent on the simplicity front, so I am not too worried about that. As long as the people who review code and those who can commit it are committed to keeping the language lean have that goal in mind, I think we're good. That's also why I won't feel heartbroken if this CL is rejected... while it might simplify some people's code, it doesn't simplify the language, and we all know that perfection is when there is nothing left to remove ;). ~Kyle
Sign in to reply to this message.
Sorry for the delay here, work's been getting in the way. PTAL. As was suggested, the CopyFile has been stripped down so it merely copies from one file to another, with a minimum of fluff. I can also close the CL if we'd rather let application designers do this entirely on their own, implementing OS-specific optimizations as necessary.
Sign in to reply to this message.
The comment needs to be updated. -rob
Sign in to reply to this message.
Hello r, rsc, rsc1 (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
Fix comment http://codereview.appspot.com/1758041/diff/40001/35002 File src/pkg/io/ioutil/ioutil.go (right): http://codereview.appspot.com/1758041/diff/40001/35002#newcode49 src/pkg/io/ioutil/ioutil.go:49: // CopyFile copies the named file's contents CopyFile reads data from a file named by inFilename, and writes the data to a file named by outFilename (which, if it exists, will be truncated).
Sign in to reply to this message.
http://codereview.appspot.com/1758041/diff/40001/35002 File src/pkg/io/ioutil/ioutil.go (right): http://codereview.appspot.com/1758041/diff/40001/35002#newcode49 src/pkg/io/ioutil/ioutil.go:49: // CopyFile copies the named file's contents If there is an error during the CopyFile function, what happens and why? If CopyFile copies a file to itself, what happens and why?
Sign in to reply to this message.
Hello r, rsc, rsc1, adg, PeterGo (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
I remain skeptical this function is worth its weight. Deferring to rsc. http://codereview.appspot.com/1758041/diff/46001/47001 File src/pkg/io/ioutil/ioutil.go (right): http://codereview.appspot.com/1758041/diff/46001/47001#newcode54 src/pkg/io/ioutil/ioutil.go:54: // The file creation mode will be the os (package) defined interpretation of 0644, a the grammar of this sentence could be simplified. The file creation mode, as defined by package os, is 0644: read/write by creator, read-only for others.
Sign in to reply to this message.
This function seems a bit out of place. Reading and Writing files is something people do all the time, it makes sense to have those in the base packages. Copying files seems much more niche. Certainly there are interfaces to remotely copy files, just as there are interfaces to have a file system to a shallow (CoW) of a file; those seem highly specific and I think belong outside of the base packages.
Sign in to reply to this message.
I think this is fine as long as it is simple. There's still some work to make it simple. http://codereview.appspot.com/1758041/diff/46001/47001 File src/pkg/io/ioutil/ioutil.go (right): http://codereview.appspot.com/1758041/diff/46001/47001#newcode49 src/pkg/io/ioutil/ioutil.go:49: // CopyFile copies the contents of the first named file into the second named file way too much detail here // CopyFile copies data from the file named by src to the file named by dst. // If the destination file does not exist, CopyFile creates it with permissions 0644; // otherwise CopyFile truncates it before copying. is all that's necessary. http://codereview.appspot.com/1758041/diff/46001/47001#newcode57 src/pkg/io/ioutil/ioutil.go:57: func CopyFile(inFilename, outFilename string) (written int64, err os.Error) { please name the parameters dst, src. Note that dst should come first, just like io.Copy. http://codereview.appspot.com/1758041/diff/46001/47001#newcode58 src/pkg/io/ioutil/ioutil.go:58: // Open the output file (mode depends on os/syscall's interpretation) these comments aren't really useful. it's obvious that the next line opens the output file. i'd delete all the comments in this function. http://codereview.appspot.com/1758041/diff/46001/47001#newcode73 src/pkg/io/ioutil/ioutil.go:73: written, err = io.Copy(out, in) return io.Copy(out, in) http://codereview.appspot.com/1758041/diff/46001/47002 File src/pkg/io/ioutil/ioutil_test.go (right): http://codereview.appspot.com/1758041/diff/46001/47002#newcode95 src/pkg/io/ioutil/ioutil_test.go:95: //func CopyFile(fromFile, toFile string) os.Error comment is wrong. delete. http://codereview.appspot.com/1758041/diff/46001/47002#newcode99 src/pkg/io/ioutil/ioutil_test.go:99: // Try the file copy comment can go. the next line calls CopyFile so that should be pretty clear. http://codereview.appspot.com/1758041/diff/46001/47002#newcode100 src/pkg/io/ioutil/ioutil_test.go:100: if _, err := CopyFile(filename, targetname); err != nil { n, err := CopyFile... and then check n below.
Sign in to reply to this message.
Also please update the CL description to be io/ioutil: add CopyFile there's no need for more detail there.
Sign in to reply to this message.
Alo
Sign in to reply to this message.
離婚,不敢………夠賤、夠爛,身障 https://codereview.appspot.com/1758041/diff/46001/src/pkg/io/ioutil/ioutil.go File src/pkg/io/ioutil/ioutil.go (right): https://codereview.appspot.com/1758041/diff/46001/src/pkg/io/ioutil/ioutil.go... src/pkg/io/ioutil/ioutil.go:58: // Open the output file (mode depends on os/syscall's interpretation) 看三小,離婚……笑你不敢 https://codereview.appspot.com/1758041/diff/46001/src/pkg/io/ioutil/ioutil.go... src/pkg/io/ioutil/ioutil.go:73: written, err = io.Copy(out, in) 去死
Sign in to reply to this message.
|