cleanup(librarian): avoid traversing directories twice (#4544)

It just bothers me to see code that does the same job twice. Or three
times, as we were checking the existence of the top-level directory to
clean up at least 3 times.

Author: coryan

internal/librarian/clean.go

@@ -26,49 +26,11 @@ import (
 // should contain paths relative to dir. It returns an error if any file
 // in keep does not exist.
 func checkAndClean(dir string, keep []string) error {
-	keepSet, err := check(dir, keep)
-	if err != nil {
-		return err
-	}
-	return clean(dir, keepSet)
-}
-
-// check validates the given directory and returns a set of files to keep.
-// It ensures that the provided directory exists and is a directory.
-// It also verifies that all files specified in 'keep' exist within 'dir'.
-func check(dir string, keep []string) (map[string]bool, error) {
-	info, err := os.Stat(dir)
-	if err != nil {
-		if errors.Is(err, fs.ErrNotExist) {
-			return nil, nil
-		}
-		return nil, fmt.Errorf("cannot access output directory %q: %w", dir, err)
-	}
-	if !info.IsDir() {
-		return nil, fmt.Errorf("%q is not a directory", dir)
-	}
 	keepSet := make(map[string]bool)
 	for _, k := range keep {
-		path := filepath.Join(dir, k)
-		if _, err := os.Stat(path); errors.Is(err, fs.ErrNotExist) {
-			return nil, fmt.Errorf("keep file %q does not exist", k)
-		}
-		// Effectively get a canonical relative path. While in most cases
-		// this will be equal to k, it might not be - in particular,
-		// on Windows the directory separator in paths returned by Rel
-		// will be a backslash.
-		rel, err := filepath.Rel(dir, path)
-		if err != nil {
-			return nil, err
-		}
-		keepSet[rel] = true
+		keepSet[filepath.Clean(k)] = true
 	}
-	return keepSet, nil
-}
-
-// clean removes files from dir that are not in keepSet.
-func clean(dir string, keepSet map[string]bool) error {
-	return filepath.WalkDir(dir, func(path string, d fs.DirEntry, err error) error {
+	err := filepath.WalkDir(dir, func(path string, d fs.DirEntry, err error) error {
 		if err != nil {
 			return err
 		}
@@ -80,8 +42,28 @@ func clean(dir string, keepSet map[string]bool) error {
 			return err
 		}
 		if keepSet[rel] {
+			keepSet[rel] = false
 			return nil
 		}
 		return os.Remove(path)
 	})
+	if err != nil {
+		if errors.Is(err, fs.ErrNotExist) {
+			// The top-level directory was not found. This happens when
+			// calling `librarian generate` on new libraries and it is not
+			// an error.
+			return nil
+		}
+		return err
+	}
+	var missing []string
+	for relative, v := range keepSet {
+		if v {
+			missing = append(missing, relative)
+		}
+	}
+	if len(missing) != 0 {
+		return fmt.Errorf("some keep files %q do not exist", keep)
+	}
+	return nil
 }

internal/librarian/clean_test.go

@@ -117,3 +117,29 @@ func TestCleanOutput(t *testing.T) {
 		})
 	}
 }
+
+// checkAndClean() needs to work when adding a library. In that case the
+// destination does not exist.
+func TestCheckAndCleanMissingDirectory(t *testing.T) {
+	for _, test := range []struct {
+		name string
+		keep []string
+	}{
+		{
+			name: "no keep files",
+			keep: []string{},
+		},
+		{
+			name: "with keep files",
+			keep: []string{"README.md", "src/something-else-to-keep.md"},
+		},
+	} {
+		t.Run(test.name, func(t *testing.T) {
+			dir := t.TempDir()
+			path := filepath.Join(dir, "does-not-exist")
+			if err := checkAndClean(path, test.keep); err != nil {
+				t.Fatal(err)
+			}
+		})
+	}
+}

internal/librarian/fake.go

@@ -97,6 +97,13 @@ func fakeDefaultLibraryName(api string) string {
 }
 
 func fakeClean(library *config.Library) error {
+	// When calling `librarian generate` on new libraries the top-level directory
+	// does not exist. Clean should not treat that as an error. Otherwise we
+	// need to check the directory existence before calling clean, only to check
+	// it again because `clean()` typically walks the directory.
+	if _, err := os.Stat(library.Output); os.IsNotExist(err) {
+		return nil
+	}
 	// We always generate a README.md file, so it's fine to delete that.
 	// This function shouldn't be called if the output directory doesn't exist.
 	return os.Remove(filepath.Join(library.Output, "README.md"))

internal/librarian/fake_test.go

@@ -94,6 +94,9 @@ func TestFakeClean_Error(t *testing.T) {
 	}
 	tmpDir := t.TempDir()
 	t.Chdir(tmpDir)
+	if err := os.MkdirAll(library.Output, 0755); err != nil {
+		t.Fatal(err)
+	}
 	err := fakeClean(library)
 	wantErr := os.ErrNotExist
 	if !errors.Is(err, wantErr) {

internal/librarian/generate.go

@@ -18,7 +18,6 @@ import (
 	"context"
 	"errors"
 	"fmt"
-	"os"
 	"strings"
 
 	"github.com/googleapis/librarian/internal/config"
@@ -118,9 +117,6 @@ func runGenerate(ctx context.Context, cfg *config.Config, all bool, libraryName
 // delegating to language-specific code to clean each library.
 func cleanLibraries(language string, libraries []*config.Library) error {
 	for _, library := range libraries {
-		if _, err := os.Stat(library.Output); os.IsNotExist(err) {
-			continue
-		}
 		switch language {
 		case config.LanguageDart:
 			if err := checkAndClean(library.Output, library.Keep); err != nil {