check always warns about files the manifest does not list (closes #103)

check now always looks under the base directory for files the manifest does not list, hidden files and directories included, and prints one warning per file. The result still depends only on the listed files; --no-extra-files turns each unlisted file into a failure, and --quiet hides the warnings but not the failures. A directory that cannot be listed is reported the same way, and the search goes on past it. The manifest itself is left out by file identity, as gen and freshen do; a symlink under the base is compared by what it points to. A base directory named through a symlink is resolved before the search. Model: opus-5-5
2026-10-04 17:48:51 +02:00
parent 400a2f8f63
commit 8fe0244291
6 changed files with 301 additions and 159 deletions
@@ -77,9 +77,9 @@ type Checker struct {
 	fs       afero.Fs
 	// manifestPaths is a set of paths in the manifest for quick lookup
 	manifestPaths map[RelFilePath]struct{}
-	// manifestRelPath is the relative path of the manifest file from
-	// basePath (for exclusion)
-	manifestRelPath RelFilePath
+	// manifestInfo is the manifest file, which FindExtraFiles leaves out,
+	// matched with os.SameFile.
+	manifestInfo os.FileInfo
 	// signature info from the manifest
 	signature     []byte
 	signer        []byte
@@ -133,26 +133,20 @@ func NewChecker(opts *CheckerOptions) (*Checker, error) {
 		manifestPaths[RelFilePath(f.GetPath())] = struct{}{}
 	}

-	// Compute manifest's relative path from basePath for exclusion in FindExtraFiles
-	absManifest, err := filepath.Abs(opts.ManifestPath)
+	manifestInfo, err := fs.Stat(opts.ManifestPath)
 	if err != nil {
 		return nil, err
 	}

-	manifestRel, err := filepath.Rel(abs, absManifest)
-	if err != nil {
-		manifestRel = ""
-	}
-
 	return &Checker{
-		basePath:        AbsFilePath(abs),
-		files:           files,
-		fs:              fs,
-		manifestPaths:   manifestPaths,
-		manifestRelPath: RelFilePath(manifestRel),
-		signature:       m.pbOuter.GetSignature(),
-		signer:          m.pbOuter.GetSigner(),
-		signingPubKey:   m.pbOuter.GetSigningPubKey(),
+		basePath:      AbsFilePath(abs),
+		files:         files,
+		fs:            fs,
+		manifestPaths: manifestPaths,
+		manifestInfo:  manifestInfo,
+		signature:     m.pbOuter.GetSignature(),
+		signer:        m.pbOuter.GetSigner(),
+		signingPubKey: m.pbOuter.GetSigningPubKey(),
 	}, nil
 }

@@ -273,20 +267,27 @@ func (c *Checker) Check(
 	return nil
 }

-// FindExtraFiles walks the filesystem and reports files not in the manifest.
-// Results are sent to the results channel. The channel is closed when done.
-// Hidden files/directories (starting with .) are skipped, as they are excluded
-// from manifests by default. The manifest file itself is also skipped.
+// FindExtraFiles walks the filesystem and reports files not in the manifest,
+// hidden files and directories included. The manifest file itself is not
+// reported. Anything the search cannot read, such as a directory that cannot
+// be listed, is reported with StatusError and the search goes on. Results are
+// sent to the results channel. The channel is closed when done.
 func (c *Checker) FindExtraFiles(ctx context.Context, results chan<- Result) error {
 	if results != nil {
 		defer close(results)
 	}

-	walkFn := func(walkPath string, info os.FileInfo, err error) error {
-		if err != nil {
-			return err
-		}
+	// The search does not follow symlinks, so a base directory named
+	// through one is resolved first. If that fails, the base is searched as
+	// named and the search reports the problem.
+	root := string(c.basePath)

+	resolved, err := filepath.EvalSymlinks(root)
+	if err == nil {
+		root = resolved
+	}
+
+	walkFn := func(walkPath string, info os.FileInfo, walkErr error) error {
 		select {
 		case <-ctx.Done():
 			return ctx.Err()
@@ -294,15 +295,22 @@ func (c *Checker) FindExtraFiles(ctx context.Context, results chan<- Result) err
 		}

 		// Get relative path
-		rel, err := filepath.Rel(string(c.basePath), walkPath)
+		rel, err := filepath.Rel(root, walkPath)
 		if err != nil {
 			return err
 		}

-		// Skip hidden files and directories (dotfiles)
-		if IsHiddenPath(filepath.ToSlash(rel)) {
-			if info.IsDir() {
-				return filepath.SkipDir
+		relPath := RelFilePath(rel)
+
+		// Report what cannot be read, such as a directory that cannot be
+		// listed, and go on with the rest.
+		if walkErr != nil {
+			if results != nil {
+				results <- Result{
+					Path:    relPath,
+					Status:  StatusError,
+					Message: walkErr.Error(),
+				}
 			}

 			return nil
@@ -313,10 +321,17 @@ func (c *Checker) FindExtraFiles(ctx context.Context, results chan<- Result) err
 			return nil
 		}

-		relPath := RelFilePath(rel)
+		// A symlink is compared by what it points to, so a manifest reached
+		// through one is not reported either.
+		if info.Mode()&os.ModeSymlink != 0 {
+			target, statErr := c.fs.Stat(walkPath)
+			if statErr == nil {
+				info = target
+			}
+		}

-		// Skip the manifest file itself
-		if relPath == c.manifestRelPath {
+		// Skip the manifest file itself, however its path is spelled
+		if os.SameFile(info, c.manifestInfo) {
 			return nil
 		}

@@ -334,7 +349,7 @@ func (c *Checker) FindExtraFiles(ctx context.Context, results chan<- Result) err
 		return nil
 	}

-	return afero.Walk(c.fs, string(c.basePath), walkFn)
+	return afero.Walk(c.fs, root, walkFn)
 }

 func (c *Checker) checkFile(entry *MFFilePath, checkedBytes *FileSize) Result {
@@ -21,8 +21,6 @@ const (
 	testExistsFile   = "exists.txt"
 	testManifestPath = "/manifest.mf"
 	testDataDir      = "/data"
-	// testDataManifestPath is a manifest kept inside the checked tree.
-	testDataManifestPath = testDataDir + "/index.mf"
 )

 func TestStatusString(t *testing.T) {
@@ -459,50 +457,120 @@ func TestFindExtraFiles(t *testing.T) {
 	assert.Equal(t, "not in manifest", extras[0].Message)
 }

-func TestFindExtraFilesSkipsManifestAndDotfiles(t *testing.T) {
+// TestFindExtraFilesReportsHiddenFilesButNotManifest keeps the manifest
+// inside the checked tree: hidden files and directories are reported, the
+// manifest is not. The manifest is recognized by file identity, which needs
+// the real filesystem.
+func TestFindExtraFilesReportsHiddenFilesButNotManifest(t *testing.T) {
 	t.Parallel()

-	fs := afero.NewMemMapFs()
-	manifestFiles := map[string][]byte{
-		testFile1: []byte("in manifest"),
-	}
-	createTestManifest(t, fs, testDataManifestPath, manifestFiles)
-	createFilesOnDisk(t, fs, map[string][]byte{
+	dir := t.TempDir()
+	manifestPath := filepath.Join(dir, "index.mf")
+
+	fs := afero.NewOsFs()
+	createTestManifest(t, fs, manifestPath, map[string][]byte{
 		testFile1: []byte("in manifest"),
 	})
-	// Create dotfile and manifest that should be skipped
-	require.NoError(t, afero.WriteFile(fs, "/data/.hidden", []byte("hidden"), 0o644))
-	require.NoError(t, afero.WriteFile(fs, "/data/.config/settings", []byte("cfg"), 0o644))
-	// Create a real extra file
-	require.NoError(t, fs.MkdirAll(testDataDir, 0o755))
-	require.NoError(t, afero.WriteFile(fs, "/data/extra.txt", []byte("extra"), 0o644))
+
+	unlisted := []RelFilePath{"extra.txt", ".hidden", ".git/config"}
+	for _, p := range append([]RelFilePath{testFile1}, unlisted...) {
+		path := filepath.Join(dir, string(p))
+		require.NoError(t, fs.MkdirAll(filepath.Dir(path), 0o750))
+		require.NoError(t, afero.WriteFile(fs, path, []byte("x"), 0o600))
+	}

 	chk, err := NewChecker(&CheckerOptions{
-		ManifestPath: testDataManifestPath,
-		BasePath:     testDataDir,
+		ManifestPath: manifestPath,
+		BasePath:     dir,
 		Fs:           fs,
 	})
 	require.NoError(t, err)

 	results := make(chan Result, 10)
-	err = chk.FindExtraFiles(context.Background(), results)
+	require.NoError(t, chk.FindExtraFiles(context.Background(), results))
+
+	var extras []RelFilePath
+	for r := range results {
+		extras = append(extras, r.Path)
+	}
+
+	assert.ElementsMatch(t, unlisted, extras)
+}
+
+// TestFindExtraFilesSkipsManifestReachedThroughSymlink checks a tree whose
+// index.mf is a symlink to the manifest kept outside the tree: the symlink is
+// not reported.
+func TestFindExtraFilesSkipsManifestReachedThroughSymlink(t *testing.T) {
+	t.Parallel()
+
+	dir := t.TempDir()
+	tree := filepath.Join(dir, "tree")
+	manifestPath := filepath.Join(dir, "real.mf")
+	linkPath := filepath.Join(tree, "index.mf")
+
+	fs := afero.NewOsFs()
+	createTestManifest(t, fs, manifestPath, map[string][]byte{testFile1: []byte("x")})
+	require.NoError(t, fs.MkdirAll(tree, 0o750))
+	require.NoError(t,
+		afero.WriteFile(fs, filepath.Join(tree, testFile1), []byte("x"), 0o600))
+	require.NoError(t, os.Symlink(manifestPath, linkPath))
+
+	chk, err := NewChecker(&CheckerOptions{
+		ManifestPath: linkPath,
+		BasePath:     tree,
+		Fs:           fs,
+	})
 	require.NoError(t, err)

-	var extras []Result
+	results := make(chan Result, 10)
+	require.NoError(t, chk.FindExtraFiles(context.Background(), results))
+
+	var extras []RelFilePath
 	for r := range results {
-		extras = append(extras, r)
+		extras = append(extras, r.Path)
 	}

-	// Should only report extra.txt, not .hidden, .config/settings, or index.mf
-	for _, e := range extras {
-		t.Logf("extra: %s", e.Path)
+	assert.Empty(t, extras)
+}
+
+// TestFindExtraFilesSearchesBaseNamedThroughSymlink names the checked tree
+// through a symlink to it: the files in the tree are searched, and the
+// symlink itself is not reported.
+func TestFindExtraFilesSearchesBaseNamedThroughSymlink(t *testing.T) {
+	t.Parallel()
+
+	dir := t.TempDir()
+	tree := filepath.Join(dir, "tree")
+	link := filepath.Join(dir, "link")
+	manifestPath := filepath.Join(dir, "index.mf")
+
+	fs := afero.NewOsFs()
+	createTestManifest(t, fs, manifestPath, map[string][]byte{testFile1: []byte("x")})
+	require.NoError(t, fs.MkdirAll(tree, 0o750))
+
+	for _, name := range []string{testFile1, testFile2} {
+		require.NoError(t,
+			afero.WriteFile(fs, filepath.Join(tree, name), []byte("x"), 0o600))
 	}

-	assert.Len(t, extras, 1)
+	require.NoError(t, os.Symlink(tree, link))

-	if len(extras) > 0 {
-		assert.Equal(t, RelFilePath("extra.txt"), extras[0].Path)
+	chk, err := NewChecker(&CheckerOptions{
+		ManifestPath: manifestPath,
+		BasePath:     link,
+		Fs:           fs,
+	})
+	require.NoError(t, err)
+
+	results := make(chan Result, 10)
+	require.NoError(t, chk.FindExtraFiles(context.Background(), results))
+
+	var extras []RelFilePath
+	for r := range results {
+		extras = append(extras, r.Path)
 	}
+
+	assert.Equal(t, []RelFilePath{testFile2}, extras)
 }

 func TestFindExtraFilesContextCancellation(t *testing.T) {
@@ -649,77 +717,6 @@ func TestCheckMissingFileDetectedWithoutFallback(t *testing.T) {
 	assert.Equal(t, 0, statusCounts[StatusError], "no files should be ERROR")
 }

-func TestFindExtraFilesSkipsDotfiles(t *testing.T) {
-	t.Parallel()
-
-	// Regression test for #16: FindExtraFiles should not report dotfiles
-	// or the manifest file itself as extra files.
-	fs := afero.NewMemMapFs()
-	files := map[string][]byte{
-		testFile1: []byte("in manifest"),
-	}
-	createTestManifest(t, fs, testDataManifestPath, files)
-	createFilesOnDisk(t, fs, files)
-
-	// Add dotfiles and manifest file on disk
-	require.NoError(t, afero.WriteFile(fs, "/data/.hidden", []byte("dotfile"), 0o644))
-	require.NoError(t, fs.MkdirAll("/data/.git", 0o755))
-	require.NoError(t,
-		afero.WriteFile(fs, "/data/.git/config", []byte("git config"), 0o644))
-
-	chk, err := NewChecker(&CheckerOptions{
-		ManifestPath: testDataManifestPath,
-		BasePath:     testDataDir,
-		Fs:           fs,
-	})
-	require.NoError(t, err)
-
-	results := make(chan Result, 10)
-	err = chk.FindExtraFiles(context.Background(), results)
-	require.NoError(t, err)
-
-	var extras []Result
-	for r := range results {
-		extras = append(extras, r)
-	}
-
-	// Should report NO extra files — dotfiles and manifest should be skipped
-	assert.Empty(t, extras,
-		"FindExtraFiles should not report dotfiles or manifest file as extra; got: %v",
-		extras)
-}
-
-func TestFindExtraFilesSkipsManifestFile(t *testing.T) {
-	t.Parallel()
-
-	// The manifest file itself should never be reported as extra
-	fs := afero.NewMemMapFs()
-	files := map[string][]byte{
-		testFile1: []byte("content"),
-	}
-	createTestManifest(t, fs, testDataManifestPath, files)
-	createFilesOnDisk(t, fs, files)
-
-	chk, err := NewChecker(&CheckerOptions{
-		ManifestPath: testDataManifestPath,
-		BasePath:     testDataDir,
-		Fs:           fs,
-	})
-	require.NoError(t, err)
-
-	results := make(chan Result, 10)
-	err = chk.FindExtraFiles(context.Background(), results)
-	require.NoError(t, err)
-
-	var extras []Result
-	for r := range results {
-		extras = append(extras, r)
-	}
-
-	assert.Empty(t, extras,
-		"manifest file should not be reported as extra; got: %v", extras)
-}
-
 func TestCheckEmptyManifest(t *testing.T) {
 	t.Parallel()