code review 6635064: cmd/go: don't ignore error when 'go clean'

cmd/go: don't ignore error when 'go clean'
        Fixes issue 4208.

[minux1, 2012-10-10 16:21:44]

Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go/

[iant2, 2012-10-10 17:30:49]

LGTM

On Wed, Oct 10, 2012 at 9:21 AM,  <minux.ma@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 to
> https://code.google.com/p/go/
>
>
> Description:
> cmd/go: don't ignore error when 'go clean'
>         Fixes issue 4208.
>
> Please review this at http://codereview.appspot.com/6635064/
>
> Affected files:
>   M src/cmd/go/clean.go
>
>
> Index: src/cmd/go/clean.go
> ===================================================================
> --- a/src/cmd/go/clean.go
> +++ b/src/cmd/go/clean.go
> @@ -170,7 +170,10 @@
>                                                 continue
>                                         }
>                                 }
> -                               os.RemoveAll(filepath.Join(p.Dir, name))
> +                               f := filepath.Join(p.Dir, name)
> +                               if err := os.RemoveAll(f); err != nil {
> +                                       errorf("go clean: %v", err)
> +                               }
>                         }
>                         continue
>                 }
> @@ -180,7 +183,7 @@
>                 }
>
>                 if cleanFile[name] || cleanExt[filepath.Ext(name)] ||
> toRemove[name] {
> -                       os.Remove(filepath.Join(p.Dir, name))
> +                       removeFile(filepath.Join(p.Dir, name))
>                 }
>         }
>
> @@ -189,7 +192,7 @@
>                         b.showcmd("", "rm -f %s", p.target)
>                 }
>                 if !cleanN {
> -                       os.Remove(p.target)
> +                       removeFile(p.target)
>                 }
>         }
>
> @@ -202,7 +205,7 @@
>                                 b.showcmd("", "rm -f %s", target)
>                         }
>                         if !cleanN {
> -                               os.Remove(target)
> +                               removeFile(target)
>                         }
>                 }
>         }
> @@ -213,3 +216,11 @@
>                 }
>         }
>  }
> +
> +// removeFile tries to remove file f, if error other than file doesn't
> exist
> +// occurs, it will report the error.
> +func removeFile(f string) {
> +       if err := os.Remove(f); err != nil && !os.IsNotExist(err) {
> +               errorf("go clean: %v", err)
> +       }
> +}
>
>

[minux1, 2012-10-10 17:34:38]

*** Submitted as http://code.google.com/p/go/source/detail?r=bc50fc903d6e ***

cmd/go: don't ignore error when 'go clean'
        Fixes issue 4208.

R=golang-dev, iant
CC=golang-dev
http://codereview.appspot.com/6635064

[anacrolix, 2012-10-10 18:49:16]

On 2012/10/10 16:21:44, minux wrote:
> Hello mailto:golang-dev@googlegroups.com (cc:
mailto:golang-dev@googlegroups.com),
> 
> I'd like you to review this change to
> https://code.google.com/p/go/

ENOENT will be ignored when removing files, but now all errors will be shown for
failure to remove directories. Have I read this wrong? Is there no way or need
to discriminate this error when using RemoveAll?

[iant2, 2012-10-10 18:53:54]

On Wed, Oct 10, 2012 at 11:49 AM,  <anacrolix@gmail.com> wrote:
>
> ENOENT will be ignored when removing files, but now all errors will be
> shown for failure to remove directories. Have I read this wrong? Is
> there no way or need to discriminate this error when using RemoveAll?
>
> https://codereview.appspot.com/6635064/

In the directory case the code has already verified that the name
exists and is a directory, so there should never be an ENOENT error in
that case.

Ian

[minux1, 2012-10-11 15:22:51]

On 2012/10/10 18:53:54, iant2 wrote:
> On Wed, Oct 10, 2012 at 11:49 AM,  <mailto:anacrolix@gmail.com> wrote:
> > ENOENT will be ignored when removing files, but now all errors will be
> > shown for failure to remove directories. Have I read this wrong? Is
> > there no way or need to discriminate this error when using RemoveAll? 
> In the directory case the code has already verified that the name
> exists and is a directory, so there should never be an ENOENT error in
> that case.
Also note that according godoc, os.RemoveAll doesn't return an error
when the path can't be found.