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

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)
 	}