code review 152180043: io/ioutil: add CopyFile

io/ioutil: add CopyFile Fixes issue 8868.

[hightower, 2014-10-04 19:57:14]

Hello golang-codereviews@googlegroups.com, I'd like you to review this change to https://code.google.com/p/go

[bradfitz, 2014-10-04 22:37:02]

Too late for Go 1.4. We can discuss after. E.g. file permissions etc. But not now. On Oct 4, 2014 12:57 PM, <kelsey.hightower@gmail.com> wrote: > Reviewers: golang-codereviews, > > Message: > Hello golang-codereviews@googlegroups.com, > > I'd like you to review this change to > https://code.google.com/p/go > > > Description: > io/ioutil: add CopyFile > > Fixes issue 8868. > > Please review this at https://codereview.appspot.com/152180043/ > > Affected files (+72, -0 lines): > M src/io/ioutil/ioutil.go > M src/io/ioutil/ioutil_test.go > > > Index: src/io/ioutil/ioutil.go > =================================================================== > --- a/src/io/ioutil/ioutil.go > +++ b/src/io/ioutil/ioutil.go > @@ -70,6 +70,24 @@ > return readAll(f, n+bytes.MinRead) > } > > +// CopyFile copies the contents from src to dst using io.Copy. > +// If dst does not exist, CopyFile creates it with permissions perm; > +// otherwise CopyFile truncates it before writing. > +func CopyFile(dst, src string, perm os.FileMode) error { > + out, err := os.OpenFile(dst, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, > perm) > + if err != nil { > + return err > + } > + defer out.Close() > + in, err := os.Open(src) > + if err != nil { > + return err > + } > + defer in.Close() > + _, err = io.Copy(out, in) > + return err > +} > + > // WriteFile writes data to a file named by filename. > // If the file does not exist, WriteFile creates it with permissions perm; > // otherwise WriteFile truncates it before writing. > Index: src/io/ioutil/ioutil_test.go > =================================================================== > --- a/src/io/ioutil/ioutil_test.go > +++ b/src/io/ioutil/ioutil_test.go > @@ -93,3 +93,57 @@ > t.Fatalf("ReadDir %s: ioutil directory not found", dirname) > } > } > + > +var copyFileTests = []struct { > + src string > + dst string > + expectError bool > +}{ > + {"ioutil_test.go", "temp", false}, > + {"ioutil_test.go", "doesnotexist", false}, > + {"ioutil_test.go", "", true}, > + {"", "temp", true}, > + {"", "", true}, > +} > + > +func TestCopyFile(t *testing.T) { > + for _, tt := range copyFileTests { > + dst := "" > + if tt.dst != "" { > + tempFile, err := TempFile("", tt.dst) > + if err != nil { > + t.Fatalf("TempFile %s", err) > + } > + tempFile.Close() > + dst = tempFile.Name() > + // CopyFile should create the dst file if it does > not exist. > + if tt.dst == "doesnotexist" { > + os.Remove(dst) > + } > + defer os.Remove(dst) > + } > + err := CopyFile(dst, tt.src, 0644) > + switch { > + case tt.expectError: > + if err == nil { > + t.Fatalf("CopyFile %s, %s: error expected, > none found", dst, tt.src) > + } > + continue > + case !tt.expectError: > + if err != nil { > + t.Fatalf("CopyFile %s", err) > + } > + } > + dstContents, err := ReadFile(dst) > + if err != nil { > + t.Fatalf("ReadFile %s", err) > + } > + srcContents, err := ReadFile(tt.src) > + if err != nil { > + t.Fatalf("ReadFile %s", err) > + } > + if string(dstContents) != string(srcContents) { > + t.Errorf("expected: %s, got %s", srcContents, > dstContents) > + } > + } > +} > > > -- > You received this message because you are subscribed to the Google Groups > "golang-codereviews" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to golang-codereviews+unsubscribe@googlegroups.com. > For more options, visit https://groups.google.com/d/optout. > 

[adg, 2014-10-04 22:37:38]

FWIW, this came up before, but the review was never completed: https://codereview.appspot.com/1758041/ This change must wait until after the Go 1.4 freeze (early December). https://codereview.appspot.com/152180043/diff/40001/src/io/ioutil/ioutil.go File src/io/ioutil/ioutil.go (right): https://codereview.appspot.com/152180043/diff/40001/src/io/ioutil/ioutil.go#n... src/io/ioutil/ioutil.go:81: defer out.Close() You *must* check the error on closing the file you're writing to. https://codereview.appspot.com/152180043/diff/40001/src/io/ioutil/ioutil.go#n... src/io/ioutil/ioutil.go:82: in, err := os.Open(src) It's better to open the source before creating the destination file, because why create the destination if you can't read the source?

[hightower, 2014-10-06 14:24:01]

On 2014/10/04 22:37:38, adg wrote: > FWIW, this came up before, but the review was never completed: > https://codereview.appspot.com/1758041/ > > This change must wait until after the Go 1.4 freeze (early December). > > https://codereview.appspot.com/152180043/diff/40001/src/io/ioutil/ioutil.go > File src/io/ioutil/ioutil.go (right): > > https://codereview.appspot.com/152180043/diff/40001/src/io/ioutil/ioutil.go#n... > src/io/ioutil/ioutil.go:81: defer out.Close() > You *must* check the error on closing the file you're writing to. > > https://codereview.appspot.com/152180043/diff/40001/src/io/ioutil/ioutil.go#n... > src/io/ioutil/ioutil.go:82: in, err := os.Open(src) > It's better to open the source before creating the destination file, because why > create the destination if you can't read the source? Thanks for the review and the reminder that the tree is frozen. I've gone ahead and addressed the issues raised since I want to see this happen even if it takes a while.

[gobot, 2014-12-19 05:14:05]

R=close To the author of this CL: The Go project has moved to Gerrit Code Review. If this CL should be continued, please see the latest version of https://golang.org/doc/contribute.html for instructions on how to set up Git and the Go project's Gerrit codereview plugin, and then create a new change with your current code. If there has been discussion on this CL, please give a link to it (golang.org/cl/152180043 is best) in the description in your new CL. Thanks very much.