From 5d596bfd55d5d768b4cfd6abe4acf398ac2e5ba0 Mon Sep 17 00:00:00 2001 From: sneak Date: Sun, 4 Oct 2026 14:09:59 +0000 Subject: [PATCH] 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 searched is likewise only a warning unless that flag is given. The manifest itself is left out by file identity, as gen and freshen do, so no name or spelling of its path or of the base gets it reported. Model: opus-5-5 --- README.md | 5 +- internal/cli/check.go | 26 ++++--- internal/cli/entry_test.go | 152 +++++++++++++++++++++++++++++++++---- internal/cli/mfer.go | 2 +- mfer/checker.go | 51 +++++-------- mfer/checker_test.go | 125 ++++++------------------------ 6 files changed, 199 insertions(+), 162 deletions(-) diff --git a/README.md b/README.md index 64c2c6e..5bee2b8 100644 --- a/README.md +++ b/README.md @@ -40,7 +40,7 @@ tree by URL: bin/mfer gen . # Verify the files on disk against the manifest. Exits nonzero if any file -# is missing or corrupted. +# it lists is missing or corrupted; warns about files it does not list. bin/mfer check index.mf # Download and cryptographically verify a tree published over HTTP: mfer @@ -251,6 +251,9 @@ are now tracked only in the [issues](https://git.eeqj.de/sneak/mfer/issues). - `mfer check` / `mfer check .` - verifies checksums of all files in manifest, displaying error and exiting nonzero if any files are missing or corrupted + - warns about each file under the base directory that the manifest does not + list, hidden files included; with `--no-extra-files` each one is a failure + instead - `mfer fetch https://example.com/stuff/` - fetches `/stuff/index.mf` and downloads all files listed in manifest, optionally resuming any that already exist locally, and assures diff --git a/internal/cli/check.go b/internal/cli/check.go index 520c043..3c333dd 100644 --- a/internal/cli/check.go +++ b/internal/cli/check.go @@ -206,28 +206,33 @@ func countCheckFailures( } // findExtraFiles reports files present on disk but absent from the -// manifest, counting each as a failure. +// manifest: each is a failure under --no-extra-files, otherwise a warning. func findExtraFiles(ctx *cli.Context, chk *mfer.Checker, failures *int64) error { extraResults := make(chan mfer.Result, 1) extraDone := make(chan struct{}) go func() { for result := range extraResults { - *failures++ + if ctx.Bool("no-extra-files") { + *failures++ - log.Infof("%s: %s (%s)", result.Status, result.Path, result.Message) + log.Infof("%s: %s (%s)", result.Status, result.Path, result.Message) + } else { + log.Warnf("%s: %s (%s)", result.Status, result.Path, result.Message) + } } close(extraDone) }() err := chk.FindExtraFiles(ctx.Context, extraResults) + + <-extraDone + if err != nil { return fmt.Errorf("failed to check for extra files: %w", err) } - <-extraDone - return nil } @@ -270,12 +275,15 @@ func runCheck(ctx *cli.Context, chk *mfer.Checker, showProgress bool) (int64, er // Wait for results processing to complete <-done - // Check for extra files if requested - if ctx.Bool("no-extra-files") { - err = findExtraFiles(ctx, chk, &failures) - if err != nil { + // Without --no-extra-files the result depends only on the files the + // manifest lists, so failing to look for others is only a warning. + err = findExtraFiles(ctx, chk, &failures) + if err != nil { + if ctx.Bool("no-extra-files") { return 0, err } + + log.Warn(err.Error()) } return failures, nil diff --git a/internal/cli/entry_test.go b/internal/cli/entry_test.go index 8fd7472..54f7a9a 100644 --- a/internal/cli/entry_test.go +++ b/internal/cli/entry_test.go @@ -698,30 +698,150 @@ func TestNoExtraFilesWithSubdirectory(t *testing.T) { "check should fail when extra files exist in subdirectory") } -func TestCheckWithoutNoExtraFilesIgnoresExtra(t *testing.T) { +// TestCheckWarnsAboutUnlistedFiles adds one file the manifest does not list +// to a tree that had none: it gets one warning and the check passes, unless +// --no-extra-files makes it a failure. --quiet hides the warning, not the +// failure. +func TestCheckWarnsAboutUnlistedFiles(t *testing.T) { t.Parallel() - fs := afero.NewMemMapFs() + for name, unlisted := range map[string]string{ + "regular file": "extra.txt", + "dotfile": ".hidden", + "file in hidden directory": ".git/config", + } { + t.Run(name, func(t *testing.T) { + t.Parallel() - // Create test file - require.NoError(t, fs.MkdirAll(testDir, 0o755)) - writeTestFile(t, fs, testFile1, "hello") + fs := afero.NewMemMapFs() + require.NoError(t, fs.MkdirAll(testDir, 0o755)) + writeTestFile(t, fs, testFile1, "hello") - // Generate manifest - opts := testOpts([]string{testApp, cmdGenerate, "-q", "-o", testManifest, testDir}, fs) - exitCode := runCLI(opts) - require.Equal(t, 0, exitCode) + opts := testOpts([]string{ + testApp, cmdGenerate, "-q", "-o", testManifest, testDir, + }, fs) + require.Equal(t, 0, runCLI(opts), "stderr: %s", testStderr(t, opts)) - // Add extra file - writeTestFile(t, fs, "/testdir/extra.txt", "extra") + check := func(flags ...string) (int, string) { + args := append([]string{testApp, cmdCheck, testFlagBase, testDir}, flags...) + opts := testOpts(append(args, testManifest), fs) + + return runCLI(opts), testStderr(t, opts) + } + + exitCode, stderr := check() + assert.Equal(t, 0, exitCode, "stderr: %s", stderr) + assert.NotContains(t, stderr, "not in manifest") + + writeTestFile(t, fs, filepath.Join(testDir, unlisted), "unlisted") + + exitCode, stderr = check() + assert.Equal(t, 0, exitCode, "stderr: %s", stderr) + assert.Equal(t, 1, strings.Count(stderr, "not in manifest"), stderr) + assert.Contains(t, stderr, unlisted) + + exitCode, stderr = check("-q") + assert.Equal(t, 0, exitCode, "stderr: %s", stderr) + assert.NotContains(t, stderr, "not in manifest") + + exitCode, stderr = check(testFlagNoExtra) + assert.Equal(t, 1, exitCode, "stderr: %s", stderr) + assert.Contains(t, stderr, unlisted) + + exitCode, _ = check("-q", testFlagNoExtra) + assert.Equal(t, 1, exitCode) + }) + } +} + +// TestCheckNeverReportsManifest keeps the manifest inside the checked tree, +// under several names and path spellings, and names the tree both directly +// and through a symlink: the manifest is never reported, even under +// --no-extra-files. The manifest is recognized by file identity, which needs +// the real filesystem. +func TestCheckNeverReportsManifest(t *testing.T) { + t.Parallel() + + // Manifest paths are relative to the checked tree. + for name, manifest := range map[string]string{ + "default name": defaultManifestName, + "hidden name": ".index.mf", + "other name in a subdirectory": "sub/listing.mf", + "path spelled through ..": "sub/../index.mf", + } { + t.Run(name, func(t *testing.T) { + t.Parallel() + + // A temp dir holding data/tree and link, a symlink to data. + root := t.TempDir() + tree := filepath.Join(root, "data", "tree") + // Not filepath.Join, which would clean away a "..". + manifestPath := tree + "/" + manifest + + fs := afero.NewOsFs() + require.NoError(t, fs.MkdirAll(filepath.Join(tree, "sub"), 0o750)) + require.NoError(t, + os.Symlink(filepath.Join(root, "data"), filepath.Join(root, "link"))) + writeTestFile(t, fs, filepath.Join(tree, testFileTxt), "hello") + + opts := testOpts([]string{ + testApp, cmdGenerate, "-q", "-o", manifestPath, tree, + }, fs) + require.Equal(t, 0, runCLI(opts), "stderr: %s", testStderr(t, opts)) + + for _, base := range []string{tree, filepath.Join(root, "link", "tree")} { + opts = testOpts([]string{ + testApp, cmdCheck, testFlagNoExtra, testFlagBase, base, manifestPath, + }, fs) + assert.Equal(t, 0, runCLI(opts), + "base %s, stderr: %s", base, testStderr(t, opts)) + assert.NotContains(t, testStderr(t, opts), "not in manifest") + } + }) + } +} + +// unlistableDirFs is a filesystem on which one directory cannot be listed. +type unlistableDirFs struct { + afero.Fs + + dir string +} + +//nolint:ireturn // Open must return afero.File to satisfy afero.Fs. +func (f unlistableDirFs) Open(name string) (afero.File, error) { + if name == f.dir { + return nil, os.ErrPermission + } + + return f.Fs.Open(name) +} + +// TestCheckWithUnlistableDirectory has a directory under the base that +// cannot be listed: the check still passes, with a warning, unless +// --no-extra-files is given. +func TestCheckWithUnlistableDirectory(t *testing.T) { + t.Parallel() + + mem := afero.NewMemMapFs() + require.NoError(t, mem.MkdirAll("/testdir/locked", 0o755)) + writeTestFile(t, mem, testFile1, "hello") + + opts := testOpts([]string{ + testApp, cmdGenerate, "-q", "-o", testManifest, testDir, + }, mem) + require.Equal(t, 0, runCLI(opts), "stderr: %s", testStderr(t, opts)) + + fs := unlistableDirFs{Fs: mem, dir: "/testdir/locked"} + + opts = testOpts([]string{testApp, cmdCheck, testFlagBase, testDir, testManifest}, fs) + assert.Equal(t, 0, runCLI(opts), "stderr: %s", testStderr(t, opts)) + assert.Contains(t, testStderr(t, opts), os.ErrPermission.Error()) - // Check WITHOUT --no-extra-files (should pass - extra files ignored) opts = testOpts([]string{ - testApp, cmdCheck, "-q", testFlagBase, testDir, testManifest, + testApp, cmdCheck, testFlagNoExtra, testFlagBase, testDir, testManifest, }, fs) - exitCode = runCLI(opts) - assert.Equal(t, 0, exitCode, - "check without --no-extra-files should ignore extra files") + assert.Equal(t, 1, runCLI(opts), "stderr: %s", testStderr(t, opts)) } func TestGenerateAtomicWriteNoTempFileOnSuccess(t *testing.T) { diff --git a/internal/cli/mfer.go b/internal/cli/mfer.go index 0e12299..fe848b4 100644 --- a/internal/cli/mfer.go +++ b/internal/cli/mfer.go @@ -227,7 +227,7 @@ func (mfa *CLIApp) checkCommand() *cli.Command { }, &cli.BoolFlag{ Name: "no-extra-files", - Usage: "Fail if files exist in base directory that are not in manifest", + Usage: "Fail, instead of warning, if files in base directory are not in manifest", }, &cli.StringFlag{ Name: "require-signature", diff --git a/mfer/checker.go b/mfer/checker.go index 0b4fd37..a6c9010 100644 --- a/mfer/checker.go +++ b/mfer/checker.go @@ -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,10 +267,10 @@ 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. 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) @@ -299,15 +293,6 @@ func (c *Checker) FindExtraFiles(ctx context.Context, results chan<- Result) err return err } - // Skip hidden files and directories (dotfiles) - if IsHiddenPath(filepath.ToSlash(rel)) { - if info.IsDir() { - return filepath.SkipDir - } - - return nil - } - // Skip directories if info.IsDir() { return nil @@ -315,8 +300,8 @@ func (c *Checker) FindExtraFiles(ctx context.Context, results chan<- Result) err relPath := RelFilePath(rel) - // 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 } diff --git a/mfer/checker_test.go b/mfer/checker_test.go index d00350d..4699ead 100644 --- a/mfer/checker_test.go +++ b/mfer/checker_test.go @@ -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,44 @@ 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, err) + require.NoError(t, chk.FindExtraFiles(context.Background(), results)) - var extras []Result + 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.Len(t, extras, 1) - - if len(extras) > 0 { - assert.Equal(t, RelFilePath("extra.txt"), extras[0].Path) - } + assert.ElementsMatch(t, unlisted, extras) } func TestFindExtraFilesContextCancellation(t *testing.T) { @@ -649,77 +641,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()