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.