From 4e3b725cf06a3a2a5ddde8585f21c81d36c61c86 Mon Sep 17 00:00:00 2001 From: Rob Pike Date: Tue, 13 Sep 2011 17:47:59 -0700 Subject: [PATCH] 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 #2237. R=golang-dev, gri, rsc, r, cw, n13m3y3r CC=golang-dev https://golang.org/cl/5014043 --- src/cmd/gofix/main.go | 30 +++----- src/cmd/gofmt/gofmt.go | 30 +++----- src/cmd/govet/govet.go | 30 +++----- src/pkg/path/filepath/path.go | 76 +++++++++++--------- src/pkg/path/filepath/path_test.go | 109 +++++++++++++++-------------- 5 files changed, 123 insertions(+), 152 deletions(-) diff --git a/src/cmd/gofix/main.go b/src/cmd/gofix/main.go index e7e7013c56..e0709fc8ba 100644 --- a/src/cmd/gofix/main.go +++ b/src/cmd/gofix/main.go @@ -198,31 +198,17 @@ func report(err os.Error) { } func walkDir(path string) { - v := make(fileVisitor) - go func() { - filepath.Walk(path, v, v) - close(v) - }() - for err := range v { - if err != nil { - report(err) - } - } -} - -type fileVisitor chan os.Error - -func (v fileVisitor) VisitDir(path string, f *os.FileInfo) bool { - return true + filepath.Walk(path, visitFile) } -func (v fileVisitor) VisitFile(path string, f *os.FileInfo) { - if isGoFile(f) { - v <- nil // synchronize error handler - if err := processFile(path, false); err != nil { - v <- err - } +func visitFile(path string, f *os.FileInfo, err os.Error) os.Error { + if err == nil && isGoFile(f) { + err = processFile(path, false) + } + if err != nil { + report(err) } + return nil } func isGoFile(f *os.FileInfo) bool { diff --git a/src/cmd/gofmt/gofmt.go b/src/cmd/gofmt/gofmt.go index 277f743ab4..1c0efb6db7 100644 --- a/src/cmd/gofmt/gofmt.go +++ b/src/cmd/gofmt/gofmt.go @@ -149,32 +149,18 @@ func processFile(filename string, in io.Reader, out io.Writer, stdin bool) os.Er return err } -type fileVisitor chan os.Error - -func (v fileVisitor) VisitDir(path string, f *os.FileInfo) bool { - return true -} - -func (v fileVisitor) VisitFile(path string, f *os.FileInfo) { - if isGoFile(f) { - v <- nil // synchronize error handler - if err := processFile(path, nil, os.Stdout, false); err != nil { - v <- err - } +func visitFile(path string, f *os.FileInfo, err os.Error) os.Error { + if err == nil && isGoFile(f) { + err = processFile(path, nil, os.Stdout, false) + } + if err != nil { + report(err) } + return nil } func walkDir(path string) { - v := make(fileVisitor) - go func() { - filepath.Walk(path, v, v) - close(v) - }() - for err := range v { - if err != nil { - report(err) - } - } + filepath.Walk(path, visitFile) } func main() { diff --git a/src/cmd/govet/govet.go b/src/cmd/govet/govet.go index 98d3d5c17f..9aa97e316f 100644 --- a/src/cmd/govet/govet.go +++ b/src/cmd/govet/govet.go @@ -101,34 +101,20 @@ func doFile(name string, reader io.Reader) { file.checkFile(name, parsedFile) } -// Visitor for filepath.Walk - trivial. Just calls doFile on each file. -// TODO: if govet becomes richer, might want to process -// a directory (package) at a time. -type V struct{} - -func (v V) VisitDir(path string, f *os.FileInfo) bool { - return true -} - -func (v V) VisitFile(path string, f *os.FileInfo) { - if strings.HasSuffix(path, ".go") { +func visit(path string, f *os.FileInfo, err os.Error) os.Error { + if err != nil { + errorf("walk error: %s", err) + return nil + } + if f.IsRegular() && strings.HasSuffix(path, ".go") { doFile(path, nil) } + return nil } // walkDir recursively walks the tree looking for .go files. func walkDir(root string) { - errors := make(chan os.Error) - done := make(chan bool) - go func() { - for e := range errors { - errorf("walk error: %s", e) - } - done <- true - }() - filepath.Walk(root, V{}, errors) - close(errors) - <-done + filepath.Walk(root, visit) } // error formats the error to standard error, adding program diff --git a/src/pkg/path/filepath/path.go b/src/pkg/path/filepath/path.go index a1457e8d22..668d20fa13 100644 --- a/src/pkg/path/filepath/path.go +++ b/src/pkg/path/filepath/path.go @@ -258,37 +258,61 @@ func Abs(path string) (string, os.Error) { return Join(wd, path), nil } -// Visitor methods are invoked for corresponding file tree entries -// visited by Walk. The provided path parameter begins with root. -type Visitor interface { - VisitDir(path string, f *os.FileInfo) bool - VisitFile(path string, f *os.FileInfo) -} +// SkipDir is used as a return value from WalkFuncs to indicate that +// the directory named in the call is to be skipped. It is not returned +// as an error by any function. +var SkipDir = os.NewError("skip this directory") + +// WalkFunc is the type of the function called for each file or directory +// visited by Walk. If there was a problem walking to the file or directory +// named by path, the incoming error will describe the problem and the +// function can decide how to handle that error (and Walk will not descend +// into that directory). If an error is returned, processing stops. The +// sole exception is that if path is a directory and the function returns the +// special value SkipDir, the contents of the directory are skipped +// and processing continues as usual on the next file. +type WalkFunc func(path string, info *os.FileInfo, err os.Error) os.Error -func walk(path string, f *os.FileInfo, v Visitor, errors chan<- os.Error) { - if !f.IsDirectory() { - v.VisitFile(path, f) - return +// walk recursively descends path, calling w. +func walk(path string, info *os.FileInfo, walkFn WalkFunc) os.Error { + err := walkFn(path, info, nil) + if err != nil { + if info.IsDirectory() && err == SkipDir { + return nil + } + return err } - if !v.VisitDir(path, f) { - return // skip directory entries + if !info.IsDirectory() { + return nil } list, err := readDir(path) if err != nil { - if errors != nil { - errors <- err + return walkFn(path, info, err) + } + + for _, fileInfo := range list { + if err = walk(Join(path, fileInfo.Name), fileInfo, walkFn); err != nil { + return err } } + return nil +} - for _, e := range list { - walk(Join(path, e.Name), e, v, errors) +// Walk walks the file tree rooted at root, calling walkFn for each file or +// directory in the tree, including root. All errors that arise visiting files +// and directories are filtered by walkFn. +func Walk(root string, walkFn WalkFunc) os.Error { + info, err := os.Lstat(root) + if err != nil { + return walkFn(root, nil, err) } + return walk(root, info, walkFn) } // readDir reads the directory named by dirname and returns -// a list of sorted directory entries. +// 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) @@ -315,24 +339,6 @@ func (f fileInfoList) Len() int { return len(f) } func (f fileInfoList) Less(i, j int) bool { return f[i].Name < f[j].Name } func (f fileInfoList) Swap(i, j int) { f[i], f[j] = f[j], f[i] } -// Walk walks the file tree rooted at root, calling v.VisitDir or -// v.VisitFile for each directory or file in the tree, including root. -// If v.VisitDir returns false, Walk skips the directory's entries; -// otherwise it invokes itself for each directory entry in sorted order. -// An error reading a directory does not abort the Walk. -// If errors != nil, Walk sends each directory read error -// to the channel. Otherwise Walk discards the error. -func Walk(root string, v Visitor, errors chan<- os.Error) { - f, err := os.Lstat(root) - if err != nil { - if errors != nil { - errors <- err - } - return // can't progress - } - walk(root, f, v, errors) -} - // Base returns the last element of path. // Trailing path separators are removed before extracting the last element. // If the path is empty, Base returns ".". diff --git a/src/pkg/path/filepath/path_test.go b/src/pkg/path/filepath/path_test.go index 9d28992454..850ead8e81 100644 --- a/src/pkg/path/filepath/path_test.go +++ b/src/pkg/path/filepath/path_test.go @@ -306,9 +306,9 @@ func makeTree(t *testing.T) { func markTree(n *Node) { walkTree(n, "", func(path string, n *Node) { n.mark++ }) } -func checkMarks(t *testing.T) { +func checkMarks(t *testing.T, report bool) { walkTree(tree, tree.name, func(path string, n *Node) { - if n.mark != 1 { + if n.mark != 1 && report { t.Errorf("node %s mark = %d; expected 1", path, n.mark) } n.mark = 0 @@ -316,44 +316,41 @@ func checkMarks(t *testing.T) { } // Assumes that each node name is unique. Good enough for a test. -func mark(name string) { - name = filepath.ToSlash(name) +// If clear is true, any incoming error is cleared before return. The errors +// are always accumulated, though. +func mark(path string, info *os.FileInfo, err os.Error, errors *[]os.Error, clear bool) os.Error { + if err != nil { + *errors = append(*errors, err) + if clear { + return nil + } + return err + } walkTree(tree, tree.name, func(path string, n *Node) { - if n.name == name { + if n.name == info.Name { n.mark++ } }) -} - -type TestVisitor struct{} - -func (v *TestVisitor) VisitDir(path string, f *os.FileInfo) bool { - mark(f.Name) - return true -} - -func (v *TestVisitor) VisitFile(path string, f *os.FileInfo) { - mark(f.Name) + return nil } func TestWalk(t *testing.T) { makeTree(t) - - // 1) ignore error handling, expect none - v := &TestVisitor{} - filepath.Walk(tree.name, v, nil) - checkMarks(t) - - // 2) handle errors, expect none - errors := make(chan os.Error, 64) - filepath.Walk(tree.name, v, errors) - select { - case err := <-errors: + errors := make([]os.Error, 0, 10) + clear := true + markFn := func(path string, info *os.FileInfo, err os.Error) os.Error { + return mark(path, info, err, &errors, clear) + } + // Expect no errors. + err := filepath.Walk(tree.name, markFn) + if err != nil { t.Errorf("no error expected, found: %s", err) - default: - // ok } - checkMarks(t) + if len(errors) != 0 { + t.Errorf("unexpected errors: %s", errors) + } + checkMarks(t, true) + errors = errors[0:0] // Test permission errors. Only possible if we're not root // and only on some file systems (AFS, FAT). To avoid errors during @@ -362,40 +359,50 @@ func TestWalk(t *testing.T) { // introduce 2 errors: chmod top-level directories to 0 os.Chmod(filepath.Join(tree.name, tree.entries[1].name), 0) os.Chmod(filepath.Join(tree.name, tree.entries[3].name), 0) + + // 3) capture errors, expect two. // mark respective subtrees manually markTree(tree.entries[1]) markTree(tree.entries[3]) // correct double-marking of directory itself tree.entries[1].mark-- tree.entries[3].mark-- + err := filepath.Walk(tree.name, markFn) + if err != nil { + t.Errorf("expected no error return from Walk, %s", err) + } + if len(errors) != 2 { + t.Errorf("expected 2 errors, got %d: %s", len(errors), errors) + } + // the inaccessible subtrees were marked manually + checkMarks(t, true) + errors = errors[0:0] - // 3) handle errors, expect two - errors = make(chan os.Error, 64) - os.Chmod(filepath.Join(tree.name, tree.entries[1].name), 0) - filepath.Walk(tree.name, v, errors) - Loop: - for i := 1; i <= 2; i++ { - select { - case <-errors: - // ok - default: - t.Errorf("%d. error expected, none found", i) - break Loop - } + // 4) capture errors, stop after first error. + // mark respective subtrees manually + markTree(tree.entries[1]) + markTree(tree.entries[3]) + // correct double-marking of directory itself + tree.entries[1].mark-- + tree.entries[3].mark-- + clear = false // error will stop processing + err = filepath.Walk(tree.name, markFn) + if err == nil { + t.Errorf("expected error return from Walk") } - select { - case err := <-errors: - t.Errorf("only two errors expected, found 3rd: %v", err) - default: - // ok + if len(errors) != 1 { + t.Errorf("expected 1 error, got %d: %s", len(errors), errors) } // the inaccessible subtrees were marked manually - checkMarks(t) + checkMarks(t, false) + errors = errors[0:0] + + // restore permissions + os.Chmod(filepath.Join(tree.name, tree.entries[1].name), 0770) + os.Chmod(filepath.Join(tree.name, tree.entries[3].name), 0770) } // cleanup - os.Chmod(filepath.Join(tree.name, tree.entries[1].name), 0770) - os.Chmod(filepath.Join(tree.name, tree.entries[3].name), 0770) if err := os.RemoveAll(tree.name); err != nil { t.Errorf("removeTree: %v", err) } -- 2.48.1