code review 5014043: path/filepath: new signature for Walk

path/filepath: new signature for Walk
This one uses a closure than an interface, and is much simpler to use.
It also enables a called function to return an error and (possibly)
halt processing.

Fixes issue 2237.

[r, 2011-09-13 21:48:58]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://go.googlecode.com/hg/

[gri, 2011-09-13 22:12:33]

http://codereview.appspot.com/5014043/diff/1/src/cmd/govet/govet.go
File src/cmd/govet/govet.go (right):

http://codereview.appspot.com/5014043/diff/1/src/cmd/govet/govet.go#newcode105
src/cmd/govet/govet.go:105: if f.IsRegular() && strings.HasSuffix(path, ".go") {
fyi: in gofmt we also check: !strings.HasPrefix(f.Name, ".")

http://codereview.appspot.com/5014043/diff/1/src/pkg/path/filepath/path.go
File src/pkg/path/filepath/path.go (right):

http://codereview.appspot.com/5014043/diff/1/src/pkg/path/filepath/path.go#ne...
src/pkg/path/filepath/path.go:267: var (
()'s not needed here

http://codereview.appspot.com/5014043/diff/1/src/pkg/path/filepath/path.go#ne...
src/pkg/path/filepath/path.go:279: default:
a comment might be appropriate

http://codereview.appspot.com/5014043/diff/1/src/pkg/path/filepath/path.go#ne...
src/pkg/path/filepath/path.go:285: // walk recursively descends path, calling w.
 If cont is false on return, all processing
it's called stop now

http://codereview.appspot.com/5014043/diff/1/src/pkg/path/filepath/path.go#ne...
src/pkg/path/filepath/path.go:288: err = w(path, info)
combine with if?

http://codereview.appspot.com/5014043/diff/1/src/pkg/path/filepath/path.go#ne...
src/pkg/path/filepath/path.go:320: // errors occur.
This is also the case for the "Otherwise..." case (processing continues). I
think this can be better phrased.

http://codereview.appspot.com/5014043/diff/1/src/pkg/path/filepath/path.go#ne...
src/pkg/path/filepath/path.go:329: errors <- err
shouldn't this call handleWalkError?

[rsc, 2011-09-13 22:24:42]

What is the reason to use select?
It looks like it is guarding against broken callers
at the cost of making it impossible to write
correct callers.  I don't see any way to guarantee
that you get all the errors, short of using a very
large channel buffer and hoping.

Russ

[r2, 2011-09-13 22:27:31]

On Sep 13, 2011, at 3:24 PM, Russ Cox wrote:

> What is the reason to use select?
> It looks like it is guarding against broken callers
> at the cost of making it impossible to write
> correct callers.  I don't see any way to guarantee
> that you get all the errors, short of using a very
> large channel buffer and hoping.

they're only errors; so what if one drops on the floor :)

seriously, this whole fiasco started because the old code was leaving orphans.
this code won't leave orphans.

http://code.google.com/p/go/issues/detail?id=2237

i should add a 'fixed 2237' to this CL, but i'll hold off for now.

-rob

[r, 2011-09-13 22:27:53]

http://codereview.appspot.com/5014043/diff/1/src/pkg/path/filepath/path.go
File src/pkg/path/filepath/path.go (right):

http://codereview.appspot.com/5014043/diff/1/src/pkg/path/filepath/path.go#ne...
src/pkg/path/filepath/path.go:267: var (
On 2011/09/13 22:12:33, gri wrote:
> ()'s not needed here

Done.

http://codereview.appspot.com/5014043/diff/1/src/pkg/path/filepath/path.go#ne...
src/pkg/path/filepath/path.go:279: default:
On 2011/09/13 22:12:33, gri wrote:
> a comment might be appropriate

Done.

http://codereview.appspot.com/5014043/diff/1/src/pkg/path/filepath/path.go#ne...
src/pkg/path/filepath/path.go:285: // walk recursively descends path, calling w.
 If cont is false on return, all processing
On 2011/09/13 22:12:33, gri wrote:
> it's called stop now

Done.

http://codereview.appspot.com/5014043/diff/1/src/pkg/path/filepath/path.go#ne...
src/pkg/path/filepath/path.go:288: err = w(path, info)
On 2011/09/13 22:12:33, gri wrote:
> combine with if?

nah, i prefer it this way. w a small name for a big deal, and it parallels the
structure below.

http://codereview.appspot.com/5014043/diff/1/src/pkg/path/filepath/path.go#ne...
src/pkg/path/filepath/path.go:320: // errors occur.
On 2011/09/13 22:12:33, gri wrote:
> This is also the case for the "Otherwise..." case (processing continues). I
> think this can be better phrased.

Done.

http://codereview.appspot.com/5014043/diff/1/src/pkg/path/filepath/path.go#ne...
src/pkg/path/filepath/path.go:329: errors <- err
On 2011/09/13 22:12:33, gri wrote:
> shouldn't this call handleWalkError?

yes, although it won't matter in practice

[r, 2011-09-13 22:28:08]

Hello golang-dev@googlegroups.com, gri@golang.org, rsc@golang.org, r@google.com
(cc: golang-dev@googlegroups.com),

Please take another look.

[r2, 2011-09-13 22:35:00]

never mind, API #4 coming up

[cw, 2011-09-13 22:55:52]

http://codereview.appspot.com/5014043/diff/8002/src/pkg/path/filepath/path.go
File src/pkg/path/filepath/path.go (right):

http://codereview.appspot.com/5014043/diff/8002/src/pkg/path/filepath/path.go...
src/pkg/path/filepath/path.go:274: if errors != nil && errors !=
IgnoreWalkErrors {
can't we just have nil here to avoid need for IgnoreWalkErrors?

[r, 2011-09-13 23:14:16]

Hello golang-dev@googlegroups.com, gri@golang.org, rsc@golang.org, r@google.com,
cw@f00f.org (cc: golang-dev@googlegroups.com),

Please take another look.

[rsc, 2011-09-13 23:27:15]

LGTM

http://codereview.appspot.com/5014043/diff/13001/src/pkg/path/filepath/path.go
File src/pkg/path/filepath/path.go (right):

http://codereview.appspot.com/5014043/diff/13001/src/pkg/path/filepath/path.g...
src/pkg/path/filepath/path.go:263: // WalkFn is the type of the function called
for each file or directory
s/WalkFn/WalkFunc/ (matches whole tree)

http://codereview.appspot.com/5014043/diff/13001/src/pkg/path/filepath/path.g...
src/pkg/path/filepath/path.go:302: // If root cannot be stat'ed, an error is
returned immediately; otherwise
Why the special case?  Because all the other errors are
passed through walkFn, you'd be able to do all the processing
in one place except for this.  return walkFn(root, nil, err)?

[cw, 2011-09-13 23:27:48]

I like the ease of use of the new interface but it's not clear how I can store
per-walker instance state (without a closure).

I use that right now to avoid crossing mount-points.




type FileProcessor struct {
        crossdev bool   // cross mount-points?
        mpdev    uint64 // moint-point device
}

func (w *FileProcessor) VisitDir(path string, d *os.FileInfo) bool {
        if w.crossdev {
                return true
        }
        if w.mpdev == 0 { // record mount-point
                w.mpdev = d.Dev
        }
        if w.mpdev != d.Dev { // crossed mount-point
                return false
        }
        return true
}

[r2, 2011-09-13 23:29:05]

On Sep 13, 2011, at 4:27 PM, cw@f00f.org wrote:

> I like the ease of use of the new interface but it's not clear how I can
> store per-walker instance state (without a closure).
> 
> I use that right now to avoid crossing mount-points.
> 
> 
> 
> 
> type FileProcessor struct {
>        crossdev bool   // cross mount-points?
>        mpdev    uint64 // moint-point device
> }
> 
> func (w *FileProcessor) VisitDir(path string, d *os.FileInfo) bool {
>        if w.crossdev {
>                return true
>        }
>        if w.mpdev == 0 { // record mount-point
>                w.mpdev = d.Dev
>        }
>        if w.mpdev != d.Dev { // crossed mount-point
>                return false
>        }
>        return true
> }
> 
> 
> http://codereview.appspot.com/5014043/

What's wrong with using a closure?  See the tests in filepath for an example.

-rob

[r, 2011-09-13 23:31:28]

http://codereview.appspot.com/5014043/diff/13001/src/pkg/path/filepath/path.go
File src/pkg/path/filepath/path.go (right):

http://codereview.appspot.com/5014043/diff/13001/src/pkg/path/filepath/path.g...
src/pkg/path/filepath/path.go:302: // If root cannot be stat'ed, an error is
returned immediately; otherwise
On 2011/09/13 23:27:15, rsc wrote:
> Why the special case?  Because all the other errors are
> passed through walkFn, you'd be able to do all the processing
> in one place except for this.  return walkFn(root, nil, err)?

because it means that that err!=nil only can occur in walkFn if it's a
directory.
one way or another, it's a special case. it seems if you can't even start the
operation, that's a reasonable place to put the special case.

[rsc, 2011-09-13 23:33:00]

On Tue, Sep 13, 2011 at 19:31,  <r@golang.org> wrote:
> because it means that that err!=nil only can occur in walkFn if it's a
> directory.
> one way or another, it's a special case. it seems if you can't even
> start the operation, that's a reasonable place to put the special case.

Right, but I am worried about code like

filepath.Walk(dir, func(..., err os.Error) os.Error {
    if err != nil {
        print(err)
    }
    return nil
})

missing an error.  Except for that one case, a WalkFunc
like the one above would never have to look at the
return value from filepath.Walk, since it always comes
from the func, and the func returns nil.

[r2, 2011-09-13 23:33:23]

On Sep 13, 2011, at 4:32 PM, Russ Cox wrote:

> On Tue, Sep 13, 2011 at 19:31,  <r@golang.org> wrote:
>> because it means that that err!=nil only can occur in walkFn if it's a
>> directory.
>> one way or another, it's a special case. it seems if you can't even
>> start the operation, that's a reasonable place to put the special case.
> 
> Right, but I am worried about code like
> 
> filepath.Walk(dir, func(..., err os.Error) os.Error {
>    if err != nil {
>        print(err)
>    }
>    return nil
> })
> 
> missing an error.  Except for that one case, a WalkFunc
> like the one above would never have to look at the
> return value from filepath.Walk, since it always comes
> from the func, and the func returns nil.

ok

[gri, 2011-09-13 23:34:56]

http://codereview.appspot.com/5014043/diff/13001/src/cmd/gofix/main.go
File src/cmd/gofix/main.go (right):

http://codereview.appspot.com/5014043/diff/13001/src/cmd/gofix/main.go#newcod...
src/cmd/gofix/main.go:211: }
if err == nil && isGoFile(f) {
    err = processFile(path, false)
}
if err != nil {
   report(err)
}
return nil

?

http://codereview.appspot.com/5014043/diff/13001/src/cmd/gofmt/gofmt.go
File src/cmd/gofmt/gofmt.go (right):

http://codereview.appspot.com/5014043/diff/13001/src/cmd/gofmt/gofmt.go#newco...
src/cmd/gofmt/gofmt.go:153: if err != nil {
if err == nil && isGoFile(f) {
    err = processFile(path, nil, os.Stdout, false)
}
if err != nil {
   report(err)
}
return nil

?

http://codereview.appspot.com/5014043/diff/13001/src/cmd/govet/govet.go
File src/cmd/govet/govet.go (right):

http://codereview.appspot.com/5014043/diff/13001/src/cmd/govet/govet.go#newco...
src/cmd/govet/govet.go:104: var walkError os.Error
is is this ever used?

http://codereview.appspot.com/5014043/diff/13001/src/pkg/path/filepath/path.go
File src/pkg/path/filepath/path.go (right):

http://codereview.appspot.com/5014043/diff/13001/src/pkg/path/filepath/path.g...
src/pkg/path/filepath/path.go:303: // all errors are passed through walkFn.
s/through/to/

(walkFn may decide to not let anything pass "through")

[r, 2011-09-13 23:35:57]

Hello golang-dev@googlegroups.com, gri@golang.org, rsc@golang.org, r@google.com,
cw@f00f.org (cc: golang-dev@googlegroups.com),

Please take another look.

[cw, 2011-09-13 23:39:16]

On Tue, Sep 13, 2011 at 04:29:01PM -0700, Rob 'Commander' Pike wrote:

> What's wrong with using a closure?  See the tests in filepath for an example.

It's not clear to me how I have n of these in parallel using that
model.  Naively I would think the state visible would be from the last
iteraton of the loop, not per-iteration state.


What if Walk took na extra interface{} argument that was passed in for
those who care where they could type-cast it ans deal as needed?

[r, 2011-09-13 23:40:10]

Hello golang-dev@googlegroups.com, gri@golang.org, rsc@golang.org, r@google.com,
cw@f00f.org (cc: golang-dev@googlegroups.com),

Please take another look.

[r2, 2011-09-13 23:40:51]

On Sep 13, 2011, at 4:39 PM, Chris Wedgwood wrote:

> On Tue, Sep 13, 2011 at 04:29:01PM -0700, Rob 'Commander' Pike wrote:
> 
>> What's wrong with using a closure?  See the tests in filepath for an example.
> 
> It's not clear to me how I have n of these in parallel using that
> model.  Naively I would think the state visible would be from the last
> iteraton of the loop, not per-iteration state.
> 
> 
> What if Walk took na extra interface{} argument that was passed in for
> those who care where they could type-cast it ans deal as needed?

that's what closures are for.

-rob

[rsc, 2011-09-13 23:41:37]

var crossdev bool
var mpdev uint64

filepath.Walk(root, func(path string, d *os.FileInfo, err os.Error) os.Error {
	if mpdev == 0 {
		mpdev = d.Dev
	}
	if !crossdev && d.Dev != mpdev {
		return ErrSkipDirectory
	}
	return nil
})

[cw, 2011-09-13 23:44:00]

> var crossdev bool
> var mpdev uint64
> 
> filepath.Walk(root, func(path string, d *os.FileInfo, err os.Error) os.Error {
> 	if mpdev == 0 {
> 		mpdev = d.Dev
> 	}
> 	if !crossdev && d.Dev != mpdev {
> 		return ErrSkipDirectory
> 	}
> 	return nil
> })

Sure.  The issue for me is that I do 10s of these in parallel (disk
slow, cpu fast) on some systems ; where the walker had state I could
use that.

Now that it has none, and I have a common function for the visitor,
it's not clear when it's invoked who it's invoked for and what
parameters it has.

[rsc, 2011-09-13 23:52:28]

If it helps, you can always define your type and use

filepath.Walk(dir, func(path, d *os.FileInfo, err os.Error) os.Error {
return v.VisitFile(path, d, err) })

or move that expression into a helper method:

func (v *Visitor) walker() filepath.WalkFunc {
    return func ...
}

filepath.Walk(dir, v.walker())

[niemeyer, 2011-09-14 00:15:38]

That's a very nice API Rob, thank you.

As a minor suggestion, it'd feel nicer to

    return filepath.SkipDir

Than to

    return filepath.ErrSkipDirectory

[r, 2011-09-14 00:24:34]

Hello golang-dev@googlegroups.com, gri@golang.org, rsc@golang.org, r@google.com,
cw@f00f.org, n13m3y3r@gmail.com (cc: golang-dev@googlegroups.com),

Please take another look.

[gri, 2011-09-14 00:46:38]

LGTM

http://codereview.appspot.com/5014043/diff/15006/src/pkg/path/filepath/path.go
File src/pkg/path/filepath/path.go (right):

http://codereview.appspot.com/5014043/diff/15006/src/pkg/path/filepath/path.g...
src/pkg/path/filepath/path.go:267: // visited by Walk.  If there is a problem
walking to the file or directory
s/is/was/ ?

[r, 2011-09-14 00:48:03]

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

path/filepath: new signature for Walk
This one uses a closure than an interface, and is much simpler to use.
It also enables a called function to return an error and (possibly)
halt processing.

Fixes issue 2237.

R=golang-dev, gri, rsc, r, cw, n13m3y3r
CC=golang-dev
http://codereview.appspot.com/5014043

[rog, 2011-09-14 08:16:10]

the docs don't seem to mention that directory entries are walked
in alphabetical order. perhaps they should - it's a useful
property (e.g. when merging file trees).

On 14 September 2011 01:48,  <r@golang.org> wrote:
> *** Submitted as
> http://code.google.com/p/go/source/detail?r=15a61cdfd2db ***
>
> path/filepath: new signature for Walk
> This one uses a closure than an interface, and is much simpler to use.
> It also enables a called function to return an error and (possibly)
> halt processing.
>
> Fixes issue 2237.
>
> R=golang-dev, gri, rsc, r, cw, n13m3y3r
> CC=golang-dev
> http://codereview.appspot.com/5014043
>
>
> http://codereview.appspot.com/5014043/
>

[cw, 2011-09-14 17:14:27]

> the docs don't seem to mention that directory entries are walked
> in alphabetical order. perhaps they should - it's a useful
> property (e.g. when merging file trees).

i don't know that we should assume that

i'm not even sure i like that


what if i have a directory with 1M files in it (yes, people do this
and it's reasonable) ... we should be able to get work from Walk(...)
before all the dirents are consumed

[r2, 2011-09-14 17:23:06]

On Wed, Sep 14, 2011 at 10:14 AM, Chris Wedgwood <cw@f00f.org> wrote:
>> the docs don't seem to mention that directory entries are walked
>> in alphabetical order. perhaps they should - it's a useful
>> property (e.g. when merging file trees).
>
> i don't know that we should assume that
>
> i'm not even sure i like that
>
>
> what if i have a directory with 1M files in it (yes, people do this
> and it's reasonable) ... we should be able to get work from Walk(...)
> before all the dirents are consumed

i admit to being surprised when i went in there that the files were
sorted. there must be a reason for it (i didn't write the original).

thinking continues.

-rob

[cw, 2011-09-14 17:25:54]

On Wed, Sep 14, 2011 at 10:23:03AM -0700, Rob 'Commander' Pike wrote:

> i admit to being surprised when i went in there that the files were
> sorted. there must be a reason for it (i didn't write the original).
> 
> thinking continues.

  // readDir reads the directory named by dirname and returns
  // a sorted list of directory entries.
  // Copied from io/ioutil to avoid the circular import.
  func readDir(dirname string) ([]*os.FileInfo, os.Error) {
	  f, err := os.Open(dirname)
	  if err != nil {
		  return nil, err
	  }
	  list, err := f.Readdir(-1)
	  f.Close()
	  if err != nil {
		  return nil, err
	  }
	  fi := make(fileInfoList, len(list))
	  for i := range list {
		  fi[i] = &list[i]
	  }
	  sort.Sort(fi)
	  return fi, nil
  }

it comes from there

the longer-term issue is that we have to consume all dirents and sort
them, i think we should be allowed to incrementally process things in
the future so we should NOT put sorting into the API

[r2, 2011-09-14 17:28:04]

I have read the code. My question was not about the implementation but
the reason for the design, and I do understand the tradeoffs.
Sometimes sorting is necessary, sometimes it's not.

-rob

[rsc, 2011-09-14 17:29:45]

I think we should just document that
Walk sorts and be done with it.
It makes the walk's output deterministic,
which is very nice for most programs.
If people are processing such enormous
directories that sorting is prohibitive,
they can just not use Walk.

Russ

[cw, 2011-09-14 17:31:34]

> I have read the code. My question was not about the implementation but
> the reason for the design, and I do understand the tradeoffs.
> Sometimes sorting is necessary, sometimes it's not.

I can't comment on why it's done that way, but having done it other
ways I will point out if you process things incrementally you tend to
keep fd's open for the dirs and you descend down.

So for deep traversal to avoid eating too many fd's you have to close
and recycle them, re-open, and recover --- which isn't guaranteed to
be robust.

What we have now I'm fine with, it clearly works for just about
everything people care about.

I just would prefer when didn't promise to keep things sorted in the
future.

[cw, 2011-09-14 17:32:30]

> If people are processing such enormous
> directories that sorting is prohibitive,
> they can just not use Walk.

ok, that's very reasonable

[r2, 2011-09-14 17:33:10]

On Wed, Sep 14, 2011 at 10:32 AM, Chris Wedgwood <cw@f00f.org> wrote:
>> If people are processing such enormous
>> directories that sorting is prohibitive,
>> they can just not use Walk.
>
> ok, that's very reasonable
>

back to rog's point, though, i agree some documentation is required.  CL coming.

-rob

[borman, 2011-09-14 17:51:12]

Seems to me you should be able to tell Walk if you want the directories
sorted or not.  The traditional fts walker allows you to pass in an optional
compar function (probably should be an interface for Go).  If compar is NULL
files are returned in the order they are read.

There are other handy features of fts as well, such as having directories
reported pre- and post-order (pre-order so you can prepare for the entries
showing up and post-order so you can now process the completed directory).
 These latter could  be done by lexically by watching the path names that
come back.

It might be worth to take a look at the existing technologies for this.

On Wed, Sep 14, 2011 at 10:14 AM, Chris Wedgwood <cw@f00f.org> wrote:

> > the docs don't seem to mention that directory entries are walked
> > in alphabetical order. perhaps they should - it's a useful
> > property (e.g. when merging file trees).
>
> i don't know that we should assume that
>
> i'm not even sure i like that
>
>
> what if i have a directory with 1M files in it (yes, people do this
> and it's reasonable) ... we should be able to get work from Walk(...)
> before all the dirents are consumed
>

[r2, 2011-09-14 18:05:40]

There's always a tradeoff between feature-richness and smoothness in
interface design.

I like the current tradeoff.

-rob