From c218fe56e997d73160ff5fa1acbf0fe39a264f40 Mon Sep 17 00:00:00 2001 From: sneak Date: Wed, 17 Dec 2025 15:57:20 -0800 Subject: [PATCH] Add atomic writes, humanized sizes, debug logging, and -v/-q per-command - Atomic writes for mfer gen: writes to temp file, renames on success, cleans up temp on error/interrupt. Prevents empty manifests on Ctrl-C. - Humanized byte sizes using dustin/go-humanize (e.g., "10 MiB" not "10485760") - Progress lines clear when done (using ANSI escape \r\033[K]) - Debug logging when files are added to manifest (mfer gen -vv) - Move -v/-q flags from global to per-command for better UX - Add tests for atomic write behavior with failing filesystem mock --- CLAUDE.md | 2 + README.md | 1 + go.mod | 1 + go.sum | 2 + internal/cli/check.go | 17 ++-- internal/cli/entry_test.go | 193 ++++++++++++++++++++++++++++++++---- internal/cli/fetch.go | 13 +-- internal/cli/freshen.go | 18 ++-- internal/cli/gen.go | 48 ++++++--- internal/cli/mfer.go | 59 +++++------ internal/log/log.go | 5 +- internal/scanner/scanner.go | 3 + 12 files changed, 276 insertions(+), 86 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 4bbb99b..eb8a9ef 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -11,3 +11,5 @@ * after each change, commit the files you've changed. push after committing. + +* NEVER use `git add -A`. always add only individual files that you've changed. diff --git a/README.md b/README.md index 962be97..c6ca12a 100644 --- a/README.md +++ b/README.md @@ -356,6 +356,7 @@ The manifest file would do several important things: ## Medium Priority +- [x] **Atomic writes for `mfer gen`** - Writes to temp file then atomic rename; cleans up temp file on error/interrupt. - [ ] **Change FileProgress callback to channel** - `mfer/builder.go` uses a callback for progress reporting; should use channels like `EnumerateStatus` and `ScanStatus` for consistency. - [ ] **Consolidate legacy manifest code** - `mfer/manifest.go` has old scanning code (`Scan()`, `addFile()`) that duplicates the new `internal/scanner` + `mfer/builder.go` pattern. - [ ] **Add context cancellation to legacy code** - The old `manifest.Scan()` doesn't support context cancellation; the new scanner does. diff --git a/go.mod b/go.mod index 9271ad8..add7622 100644 --- a/go.mod +++ b/go.mod @@ -5,6 +5,7 @@ go 1.23 require ( github.com/apex/log v1.9.0 github.com/davecgh/go-spew v1.1.1 + github.com/dustin/go-humanize v1.0.1 github.com/klauspost/compress v1.18.2 github.com/multiformats/go-multihash v0.2.3 github.com/pterm/pterm v0.12.35 diff --git a/go.sum b/go.sum index b4c7c10..7d53693 100644 --- a/go.sum +++ b/go.sum @@ -66,6 +66,8 @@ github.com/cpuguy83/go-md2man/v2 v2.0.2/go.mod h1:tgQtvFlXSQOSOSIRvRPT7W67SCa46t github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= +github.com/dustin/go-humanize v1.0.1 h1:GzkhY7T5VNhEkwH0PVJgjz+fX1rhBrR7pRT3mDkpeCY= +github.com/dustin/go-humanize v1.0.1/go.mod h1:Mu1zIs6XwVuF/gI1OepvI0qD18qycQx+mFykh5fBlto= github.com/envoyproxy/go-control-plane v0.9.0/go.mod h1:YTl/9mNaCwkRvm6d1a2C3ymFceY/DCBVvsKhRF0iEA4= github.com/envoyproxy/go-control-plane v0.9.1-0.20191026205805-5f8ba28d4473/go.mod h1:YTl/9mNaCwkRvm6d1a2C3ymFceY/DCBVvsKhRF0iEA4= github.com/envoyproxy/go-control-plane v0.9.4/go.mod h1:6rpuAdCZL397s3pYoYcLgu1mIlRU8Am5FuJP05cCM98= diff --git a/internal/cli/check.go b/internal/cli/check.go index 592869e..39aa325 100644 --- a/internal/cli/check.go +++ b/internal/cli/check.go @@ -5,6 +5,7 @@ import ( "path/filepath" "time" + "github.com/dustin/go-humanize" "github.com/spf13/afero" "github.com/urfave/cli/v2" "sneak.berlin/go/mfer/internal/checker" @@ -67,7 +68,7 @@ func (mfa *CLIApp) checkManifestOperation(ctx *cli.Context) error { return fmt.Errorf("failed to load manifest: %w", err) } - log.Infof("manifest contains %d files, %d bytes", chk.FileCount(), chk.TotalBytes()) + log.Infof("manifest contains %d files, %s", chk.FileCount(), humanize.IBytes(uint64(chk.TotalBytes()))) // Set up results channel results := make(chan checker.Result, 1) @@ -79,17 +80,17 @@ func (mfa *CLIApp) checkManifestOperation(ctx *cli.Context) error { go func() { for status := range progress { if status.ETA > 0 { - log.Progressf("Checking: %d/%d files, %.1f MB/s, ETA %s, %d failures", + log.Progressf("Checking: %d/%d files, %s/s, ETA %s, %d failures", status.CheckedFiles, status.TotalFiles, - status.BytesPerSec/1e6, + humanize.IBytes(uint64(status.BytesPerSec)), status.ETA.Round(time.Second), status.Failures) } else { - log.Progressf("Checking: %d/%d files, %.1f MB/s, %d failures", + log.Progressf("Checking: %d/%d files, %s/s, %d failures", status.CheckedFiles, status.TotalFiles, - status.BytesPerSec/1e6, + humanize.IBytes(uint64(status.BytesPerSec)), status.Failures) } } @@ -141,11 +142,11 @@ func (mfa *CLIApp) checkManifestOperation(ctx *cli.Context) error { } elapsed := time.Since(mfa.startupTime).Seconds() - rate := float64(chk.TotalBytes()) / elapsed / 1e6 + rate := float64(chk.TotalBytes()) / elapsed if failures == 0 { - log.Infof("checked %d files (%.1f MB) in %.1fs (%.1f MB/s): all OK", chk.FileCount(), float64(chk.TotalBytes())/1e6, elapsed, rate) + log.Infof("checked %d files (%s) in %.1fs (%s/s): all OK", chk.FileCount(), humanize.IBytes(uint64(chk.TotalBytes())), elapsed, humanize.IBytes(uint64(rate))) } else { - log.Infof("checked %d files (%.1f MB) in %.1fs (%.1f MB/s): %d failed", chk.FileCount(), float64(chk.TotalBytes())/1e6, elapsed, rate, failures) + log.Infof("checked %d files (%s) in %.1fs (%s/s): %d failed", chk.FileCount(), humanize.IBytes(uint64(chk.TotalBytes())), elapsed, humanize.IBytes(uint64(rate)), failures) } if failures > 0 { diff --git a/internal/cli/entry_test.go b/internal/cli/entry_test.go index 7e59022..3312902 100644 --- a/internal/cli/entry_test.go +++ b/internal/cli/entry_test.go @@ -2,6 +2,7 @@ package cli import ( "bytes" + "fmt" "testing" "github.com/spf13/afero" @@ -67,7 +68,7 @@ func TestGenerateCommand(t *testing.T) { require.NoError(t, afero.WriteFile(fs, "/testdir/file1.txt", []byte("hello world"), 0644)) require.NoError(t, afero.WriteFile(fs, "/testdir/file2.txt", []byte("test content"), 0644)) - opts := testOpts([]string{"mfer", "-q", "generate", "-o", "/testdir/test.mf", "/testdir"}, fs) + opts := testOpts([]string{"mfer", "generate", "-q", "-o", "/testdir/test.mf", "/testdir"}, fs) exitCode := RunWithOptions(opts) @@ -88,12 +89,12 @@ func TestGenerateAndCheckCommand(t *testing.T) { require.NoError(t, afero.WriteFile(fs, "/testdir/subdir/file2.txt", []byte("test content"), 0644)) // Generate manifest - opts := testOpts([]string{"mfer", "-q", "generate", "-o", "/testdir/test.mf", "/testdir"}, fs) + opts := testOpts([]string{"mfer", "generate", "-q", "-o", "/testdir/test.mf", "/testdir"}, fs) exitCode := RunWithOptions(opts) require.Equal(t, 0, exitCode, "generate failed: %s", opts.Stderr.(*bytes.Buffer).String()) // Check manifest - opts = testOpts([]string{"mfer", "-q", "check", "--base", "/testdir", "/testdir/test.mf"}, fs) + opts = testOpts([]string{"mfer", "check", "-q", "--base", "/testdir", "/testdir/test.mf"}, fs) exitCode = RunWithOptions(opts) assert.Equal(t, 0, exitCode, "check failed: %s", opts.Stderr.(*bytes.Buffer).String()) } @@ -106,7 +107,7 @@ func TestCheckCommandWithMissingFile(t *testing.T) { require.NoError(t, afero.WriteFile(fs, "/testdir/file1.txt", []byte("hello world"), 0644)) // Generate manifest - opts := testOpts([]string{"mfer", "-q", "generate", "-o", "/testdir/test.mf", "/testdir"}, fs) + opts := testOpts([]string{"mfer", "generate", "-q", "-o", "/testdir/test.mf", "/testdir"}, fs) exitCode := RunWithOptions(opts) require.Equal(t, 0, exitCode, "generate failed: %s", opts.Stderr.(*bytes.Buffer).String()) @@ -114,7 +115,7 @@ func TestCheckCommandWithMissingFile(t *testing.T) { require.NoError(t, fs.Remove("/testdir/file1.txt")) // Check manifest - should fail - opts = testOpts([]string{"mfer", "-q", "check", "--base", "/testdir", "/testdir/test.mf"}, fs) + opts = testOpts([]string{"mfer", "check", "-q", "--base", "/testdir", "/testdir/test.mf"}, fs) exitCode = RunWithOptions(opts) assert.Equal(t, 1, exitCode, "check should have failed for missing file") } @@ -127,7 +128,7 @@ func TestCheckCommandWithCorruptedFile(t *testing.T) { require.NoError(t, afero.WriteFile(fs, "/testdir/file1.txt", []byte("hello world"), 0644)) // Generate manifest - opts := testOpts([]string{"mfer", "-q", "generate", "-o", "/testdir/test.mf", "/testdir"}, fs) + opts := testOpts([]string{"mfer", "generate", "-q", "-o", "/testdir/test.mf", "/testdir"}, fs) exitCode := RunWithOptions(opts) require.Equal(t, 0, exitCode, "generate failed: %s", opts.Stderr.(*bytes.Buffer).String()) @@ -135,7 +136,7 @@ func TestCheckCommandWithCorruptedFile(t *testing.T) { require.NoError(t, afero.WriteFile(fs, "/testdir/file1.txt", []byte("HELLO WORLD"), 0644)) // Check manifest - should fail with hash mismatch - opts = testOpts([]string{"mfer", "-q", "check", "--base", "/testdir", "/testdir/test.mf"}, fs) + opts = testOpts([]string{"mfer", "check", "-q", "--base", "/testdir", "/testdir/test.mf"}, fs) exitCode = RunWithOptions(opts) assert.Equal(t, 1, exitCode, "check should have failed for corrupted file") } @@ -148,7 +149,7 @@ func TestCheckCommandWithSizeMismatch(t *testing.T) { require.NoError(t, afero.WriteFile(fs, "/testdir/file1.txt", []byte("hello world"), 0644)) // Generate manifest - opts := testOpts([]string{"mfer", "-q", "generate", "-o", "/testdir/test.mf", "/testdir"}, fs) + opts := testOpts([]string{"mfer", "generate", "-q", "-o", "/testdir/test.mf", "/testdir"}, fs) exitCode := RunWithOptions(opts) require.Equal(t, 0, exitCode, "generate failed: %s", opts.Stderr.(*bytes.Buffer).String()) @@ -156,7 +157,7 @@ func TestCheckCommandWithSizeMismatch(t *testing.T) { require.NoError(t, afero.WriteFile(fs, "/testdir/file1.txt", []byte("different size content here"), 0644)) // Check manifest - should fail with size mismatch - opts = testOpts([]string{"mfer", "-q", "check", "--base", "/testdir", "/testdir/test.mf"}, fs) + opts = testOpts([]string{"mfer", "check", "-q", "--base", "/testdir", "/testdir/test.mf"}, fs) exitCode = RunWithOptions(opts) assert.Equal(t, 1, exitCode, "check should have failed for size mismatch") } @@ -196,7 +197,7 @@ func TestGenerateExcludesDotfilesByDefault(t *testing.T) { require.NoError(t, afero.WriteFile(fs, "/testdir/.hidden", []byte("secret"), 0644)) // Generate manifest without --include-dotfiles (default excludes dotfiles) - opts := testOpts([]string{"mfer", "-q", "generate", "-o", "/testdir/test.mf", "/testdir"}, fs) + opts := testOpts([]string{"mfer", "generate", "-q", "-o", "/testdir/test.mf", "/testdir"}, fs) exitCode := RunWithOptions(opts) require.Equal(t, 0, exitCode) @@ -220,7 +221,7 @@ func TestGenerateWithIncludeDotfiles(t *testing.T) { require.NoError(t, afero.WriteFile(fs, "/testdir/.hidden", []byte("secret"), 0644)) // Generate manifest with --include-dotfiles - opts := testOpts([]string{"mfer", "-q", "generate", "--include-dotfiles", "-o", "/testdir/test.mf", "/testdir"}, fs) + opts := testOpts([]string{"mfer", "generate", "-q", "--include-dotfiles", "-o", "/testdir/test.mf", "/testdir"}, fs) exitCode := RunWithOptions(opts) require.Equal(t, 0, exitCode) @@ -240,7 +241,7 @@ func TestMultipleInputPaths(t *testing.T) { require.NoError(t, afero.WriteFile(fs, "/dir2/file2.txt", []byte("content2"), 0644)) // Generate manifest from multiple paths - opts := testOpts([]string{"mfer", "-q", "generate", "-o", "/output.mf", "/dir1", "/dir2"}, fs) + opts := testOpts([]string{"mfer", "generate", "-q", "-o", "/output.mf", "/dir1", "/dir2"}, fs) exitCode := RunWithOptions(opts) assert.Equal(t, 0, exitCode, "stderr: %s", opts.Stderr.(*bytes.Buffer).String()) @@ -257,12 +258,12 @@ func TestNoExtraFilesPass(t *testing.T) { require.NoError(t, afero.WriteFile(fs, "/testdir/file2.txt", []byte("world"), 0644)) // Generate manifest - opts := testOpts([]string{"mfer", "-q", "generate", "-o", "/manifest.mf", "/testdir"}, fs) + opts := testOpts([]string{"mfer", "generate", "-q", "-o", "/manifest.mf", "/testdir"}, fs) exitCode := RunWithOptions(opts) require.Equal(t, 0, exitCode) // Check with --no-extra-files (should pass - no extra files) - opts = testOpts([]string{"mfer", "-q", "check", "--no-extra-files", "--base", "/testdir", "/manifest.mf"}, fs) + opts = testOpts([]string{"mfer", "check", "-q", "--no-extra-files", "--base", "/testdir", "/manifest.mf"}, fs) exitCode = RunWithOptions(opts) assert.Equal(t, 0, exitCode) } @@ -275,7 +276,7 @@ func TestNoExtraFilesFail(t *testing.T) { require.NoError(t, afero.WriteFile(fs, "/testdir/file1.txt", []byte("hello"), 0644)) // Generate manifest - opts := testOpts([]string{"mfer", "-q", "generate", "-o", "/manifest.mf", "/testdir"}, fs) + opts := testOpts([]string{"mfer", "generate", "-q", "-o", "/manifest.mf", "/testdir"}, fs) exitCode := RunWithOptions(opts) require.Equal(t, 0, exitCode) @@ -283,7 +284,7 @@ func TestNoExtraFilesFail(t *testing.T) { require.NoError(t, afero.WriteFile(fs, "/testdir/extra.txt", []byte("extra"), 0644)) // Check with --no-extra-files (should fail - extra file exists) - opts = testOpts([]string{"mfer", "-q", "check", "--no-extra-files", "--base", "/testdir", "/manifest.mf"}, fs) + opts = testOpts([]string{"mfer", "check", "-q", "--no-extra-files", "--base", "/testdir", "/manifest.mf"}, fs) exitCode = RunWithOptions(opts) assert.Equal(t, 1, exitCode, "check should fail when extra files exist") } @@ -297,7 +298,7 @@ func TestNoExtraFilesWithSubdirectory(t *testing.T) { require.NoError(t, afero.WriteFile(fs, "/testdir/subdir/file2.txt", []byte("world"), 0644)) // Generate manifest - opts := testOpts([]string{"mfer", "-q", "generate", "-o", "/manifest.mf", "/testdir"}, fs) + opts := testOpts([]string{"mfer", "generate", "-q", "-o", "/manifest.mf", "/testdir"}, fs) exitCode := RunWithOptions(opts) require.Equal(t, 0, exitCode) @@ -305,7 +306,7 @@ func TestNoExtraFilesWithSubdirectory(t *testing.T) { require.NoError(t, afero.WriteFile(fs, "/testdir/subdir/extra.txt", []byte("extra"), 0644)) // Check with --no-extra-files (should fail) - opts = testOpts([]string{"mfer", "-q", "check", "--no-extra-files", "--base", "/testdir", "/manifest.mf"}, fs) + opts = testOpts([]string{"mfer", "check", "-q", "--no-extra-files", "--base", "/testdir", "/manifest.mf"}, fs) exitCode = RunWithOptions(opts) assert.Equal(t, 1, exitCode, "check should fail when extra files exist in subdirectory") } @@ -318,7 +319,7 @@ func TestCheckWithoutNoExtraFilesIgnoresExtra(t *testing.T) { require.NoError(t, afero.WriteFile(fs, "/testdir/file1.txt", []byte("hello"), 0644)) // Generate manifest - opts := testOpts([]string{"mfer", "-q", "generate", "-o", "/manifest.mf", "/testdir"}, fs) + opts := testOpts([]string{"mfer", "generate", "-q", "-o", "/manifest.mf", "/testdir"}, fs) exitCode := RunWithOptions(opts) require.Equal(t, 0, exitCode) @@ -326,7 +327,159 @@ func TestCheckWithoutNoExtraFilesIgnoresExtra(t *testing.T) { require.NoError(t, afero.WriteFile(fs, "/testdir/extra.txt", []byte("extra"), 0644)) // Check WITHOUT --no-extra-files (should pass - extra files ignored) - opts = testOpts([]string{"mfer", "-q", "check", "--base", "/testdir", "/manifest.mf"}, fs) + opts = testOpts([]string{"mfer", "check", "-q", "--base", "/testdir", "/manifest.mf"}, fs) exitCode = RunWithOptions(opts) assert.Equal(t, 0, exitCode, "check without --no-extra-files should ignore extra files") } + +func TestGenerateAtomicWriteNoTempFileOnSuccess(t *testing.T) { + fs := afero.NewMemMapFs() + + // Create test file + require.NoError(t, fs.MkdirAll("/testdir", 0755)) + require.NoError(t, afero.WriteFile(fs, "/testdir/file1.txt", []byte("hello"), 0644)) + + // Generate manifest + opts := testOpts([]string{"mfer", "generate", "-q", "-o", "/output.mf", "/testdir"}, fs) + exitCode := RunWithOptions(opts) + require.Equal(t, 0, exitCode) + + // Verify output file exists + exists, err := afero.Exists(fs, "/output.mf") + require.NoError(t, err) + assert.True(t, exists, "output file should exist") + + // Verify temp file does NOT exist + tmpExists, err := afero.Exists(fs, "/output.mf.tmp") + require.NoError(t, err) + assert.False(t, tmpExists, "temp file should not exist after successful generation") +} + +func TestGenerateAtomicWriteOverwriteWithForce(t *testing.T) { + fs := afero.NewMemMapFs() + + // Create test file + require.NoError(t, fs.MkdirAll("/testdir", 0755)) + require.NoError(t, afero.WriteFile(fs, "/testdir/file1.txt", []byte("hello"), 0644)) + + // Create existing manifest with different content + require.NoError(t, afero.WriteFile(fs, "/output.mf", []byte("old content"), 0644)) + + // Generate manifest with --force + opts := testOpts([]string{"mfer", "generate", "-q", "-f", "-o", "/output.mf", "/testdir"}, fs) + exitCode := RunWithOptions(opts) + require.Equal(t, 0, exitCode) + + // Verify output file exists and was overwritten + content, err := afero.ReadFile(fs, "/output.mf") + require.NoError(t, err) + assert.NotEqual(t, "old content", string(content), "manifest should be overwritten") + + // Verify temp file does NOT exist + tmpExists, err := afero.Exists(fs, "/output.mf.tmp") + require.NoError(t, err) + assert.False(t, tmpExists, "temp file should not exist after successful generation") +} + +func TestGenerateFailsWithoutForceWhenOutputExists(t *testing.T) { + fs := afero.NewMemMapFs() + + // Create test file + require.NoError(t, fs.MkdirAll("/testdir", 0755)) + require.NoError(t, afero.WriteFile(fs, "/testdir/file1.txt", []byte("hello"), 0644)) + + // Create existing manifest + require.NoError(t, afero.WriteFile(fs, "/output.mf", []byte("existing"), 0644)) + + // Generate manifest WITHOUT --force (should fail) + opts := testOpts([]string{"mfer", "generate", "-q", "-o", "/output.mf", "/testdir"}, fs) + exitCode := RunWithOptions(opts) + assert.Equal(t, 1, exitCode, "should fail when output exists without --force") + + // Verify original content is preserved + content, err := afero.ReadFile(fs, "/output.mf") + require.NoError(t, err) + assert.Equal(t, "existing", string(content), "original file should be preserved") +} + +func TestGenerateAtomicWriteUsesTemp(t *testing.T) { + // This test verifies that generate uses a temp file by checking + // that the output file doesn't exist until generation completes. + // We do this by generating to a path and verifying the temp file + // pattern is used (output.mf.tmp -> output.mf) + fs := afero.NewMemMapFs() + + // Create test file + require.NoError(t, fs.MkdirAll("/testdir", 0755)) + require.NoError(t, afero.WriteFile(fs, "/testdir/file1.txt", []byte("hello"), 0644)) + + // Generate manifest + opts := testOpts([]string{"mfer", "generate", "-q", "-o", "/output.mf", "/testdir"}, fs) + exitCode := RunWithOptions(opts) + require.Equal(t, 0, exitCode) + + // Both output file should exist and temp should not + exists, _ := afero.Exists(fs, "/output.mf") + assert.True(t, exists, "output file should exist") + + tmpExists, _ := afero.Exists(fs, "/output.mf.tmp") + assert.False(t, tmpExists, "temp file should be cleaned up") + + // Verify manifest is valid (not empty) + content, err := afero.ReadFile(fs, "/output.mf") + require.NoError(t, err) + assert.True(t, len(content) > 0, "manifest should not be empty") +} + +// failingWriterFs wraps a filesystem and makes writes fail after N bytes +type failingWriterFs struct { + afero.Fs + failAfter int64 + written int64 +} + +type failingFile struct { + afero.File + fs *failingWriterFs +} + +func (f *failingFile) Write(p []byte) (int, error) { + f.fs.written += int64(len(p)) + if f.fs.written > f.fs.failAfter { + return 0, fmt.Errorf("simulated write failure") + } + return f.File.Write(p) +} + +func (fs *failingWriterFs) Create(name string) (afero.File, error) { + f, err := fs.Fs.Create(name) + if err != nil { + return nil, err + } + return &failingFile{File: f, fs: fs}, nil +} + +func TestGenerateAtomicWriteCleansUpOnError(t *testing.T) { + baseFs := afero.NewMemMapFs() + + // Create test files - need enough content to trigger the write failure + require.NoError(t, baseFs.MkdirAll("/testdir", 0755)) + require.NoError(t, afero.WriteFile(baseFs, "/testdir/file1.txt", []byte("hello world this is a test file"), 0644)) + + // Wrap with failing writer that fails after writing some bytes + fs := &failingWriterFs{Fs: baseFs, failAfter: 10} + + // Generate manifest - should fail during write + opts := testOpts([]string{"mfer", "generate", "-q", "-o", "/output.mf", "/testdir"}, fs) + exitCode := RunWithOptions(opts) + assert.Equal(t, 1, exitCode, "should fail due to write error") + + // With atomic writes: output.mf should NOT exist (temp was cleaned up) + // With non-atomic writes: output.mf WOULD exist (partial/empty) + exists, _ := afero.Exists(baseFs, "/output.mf") + assert.False(t, exists, "output file should not exist after failed generation (atomic write)") + + // Temp file should also not exist + tmpExists, _ := afero.Exists(baseFs, "/output.mf.tmp") + assert.False(t, tmpExists, "temp file should be cleaned up after failed generation") +} diff --git a/internal/cli/fetch.go b/internal/cli/fetch.go index 894c60a..a8a3bff 100644 --- a/internal/cli/fetch.go +++ b/internal/cli/fetch.go @@ -13,6 +13,7 @@ import ( "strings" "time" + "github.com/dustin/go-humanize" "github.com/multiformats/go-multihash" "github.com/urfave/cli/v2" "sneak.berlin/go/mfer/internal/log" @@ -89,12 +90,12 @@ func (mfa *CLIApp) fetchManifestOperation(ctx *cli.Context) error { for p := range progress { rate := formatBitrate(p.BytesPerSec * 8) if p.ETA > 0 { - log.Infof("%s: %d/%d bytes, %s, ETA %s", - p.Path, p.BytesRead, p.TotalBytes, + log.Infof("%s: %s/%s, %s, ETA %s", + p.Path, humanize.IBytes(uint64(p.BytesRead)), humanize.IBytes(uint64(p.TotalBytes)), rate, p.ETA.Round(time.Second)) } else { - log.Infof("%s: %d/%d bytes, %s", - p.Path, p.BytesRead, p.TotalBytes, rate) + log.Infof("%s: %s/%s, %s", + p.Path, humanize.IBytes(uint64(p.BytesRead)), humanize.IBytes(uint64(p.TotalBytes)), rate) } } }() @@ -129,9 +130,9 @@ func (mfa *CLIApp) fetchManifestOperation(ctx *cli.Context) error { elapsed := time.Since(startTime) avgBytesPerSec := float64(totalBytes) / elapsed.Seconds() avgRate := formatBitrate(avgBytesPerSec * 8) - log.Infof("downloaded %d files (%.1f MB) in %.1fs (%s avg)", + log.Infof("downloaded %d files (%s) in %.1fs (%s avg)", len(files), - float64(totalBytes)/1e6, + humanize.IBytes(uint64(totalBytes)), elapsed.Seconds(), avgRate) diff --git a/internal/cli/freshen.go b/internal/cli/freshen.go index 40cc2a4..1de2713 100644 --- a/internal/cli/freshen.go +++ b/internal/cli/freshen.go @@ -8,6 +8,7 @@ import ( "path/filepath" "time" + "github.com/dustin/go-humanize" "github.com/multiformats/go-multihash" "github.com/spf13/afero" "github.com/urfave/cli/v2" @@ -217,7 +218,7 @@ func (mfa *CLIApp) freshenManifestOperation(ctx *cli.Context) error { // Phase 2: Hash changed and new files if filesToHash > 0 { - log.Infof("hashing %d files (%.1f MB)...", filesToHash, float64(totalHashBytes)/1e6) + log.Infof("hashing %d files (%s)...", filesToHash, humanize.IBytes(uint64(totalHashBytes))) } startHash := time.Now() @@ -255,11 +256,11 @@ func (mfa *CLIApp) freshenManifestOperation(ctx *cli.Context) error { } } if eta > 0 { - log.Progressf("Hashing: %d/%d files, %.1f MB/s, ETA %s", - hashedFiles, filesToHash, rate/1e6, eta.Round(time.Second)) + log.Progressf("Hashing: %d/%d files, %s/s, ETA %s", + hashedFiles, filesToHash, humanize.IBytes(uint64(rate)), eta.Round(time.Second)) } else { - log.Progressf("Hashing: %d/%d files, %.1f MB/s", - hashedFiles, filesToHash, rate/1e6) + log.Progressf("Hashing: %d/%d files, %s/s", + hashedFiles, filesToHash, humanize.IBytes(uint64(rate))) } } }) @@ -315,12 +316,11 @@ func (mfa *CLIApp) freshenManifestOperation(ctx *cli.Context) error { } totalDuration := time.Since(mfa.startupTime) - var hashRate float64 if hashedBytes > 0 { hashDuration := time.Since(startHash) - hashRate = float64(hashedBytes) / hashDuration.Seconds() / 1e6 - log.Infof("hashed %.1f MB in %.1fs (%.1f MB/s)", - float64(hashedBytes)/1e6, totalDuration.Seconds(), hashRate) + hashRate := float64(hashedBytes) / hashDuration.Seconds() + log.Infof("hashed %s in %.1fs (%s/s)", + humanize.IBytes(uint64(hashedBytes)), totalDuration.Seconds(), humanize.IBytes(uint64(hashRate))) } log.Infof("wrote %d files to %s", len(entries), manifestPath) diff --git a/internal/cli/gen.go b/internal/cli/gen.go index 67cdf87..47a32cf 100644 --- a/internal/cli/gen.go +++ b/internal/cli/gen.go @@ -6,6 +6,7 @@ import ( "sync" "time" + "github.com/dustin/go-humanize" "github.com/spf13/afero" "github.com/urfave/cli/v2" "sneak.berlin/go/mfer/internal/log" @@ -36,9 +37,9 @@ func (mfa *CLIApp) generateManifestOperation(ctx *cli.Context) error { go func() { defer enumWg.Done() for status := range enumProgress { - log.Progressf("Enumerating: %d files, %.1f MB", + log.Progressf("Enumerating: %d files, %s", status.FilesFound, - float64(status.BytesFound)/1e6) + humanize.IBytes(uint64(status.BytesFound))) } log.ProgressDone() }() @@ -66,7 +67,7 @@ func (mfa *CLIApp) generateManifestOperation(ctx *cli.Context) error { } enumWg.Wait() - log.Infof("enumerated %d files, %d bytes total", s.FileCount(), s.TotalBytes()) + log.Infof("enumerated %d files, %s total", s.FileCount(), humanize.IBytes(uint64(s.TotalBytes()))) // Check if output file exists outputPath := ctx.String("output") @@ -76,12 +77,21 @@ func (mfa *CLIApp) generateManifestOperation(ctx *cli.Context) error { } } - // Open output file - outFile, err := mfa.Fs.Create(outputPath) + // Create temp file for atomic write + tmpPath := outputPath + ".tmp" + outFile, err := mfa.Fs.Create(tmpPath) if err != nil { - return fmt.Errorf("failed to create output file: %w", err) + return fmt.Errorf("failed to create temp file: %w", err) } - defer func() { _ = outFile.Close() }() + + // Clean up temp file on any error or interruption + success := false + defer func() { + _ = outFile.Close() + if !success { + _ = mfa.Fs.Remove(tmpPath) + } + }() // Phase 2: Scan - read file contents and generate manifest var scanProgress chan scanner.ScanStatus @@ -93,16 +103,16 @@ func (mfa *CLIApp) generateManifestOperation(ctx *cli.Context) error { defer scanWg.Done() for status := range scanProgress { if status.ETA > 0 { - log.Progressf("Scanning: %d/%d files, %.1f MB/s, ETA %s", + log.Progressf("Scanning: %d/%d files, %s/s, ETA %s", status.ScannedFiles, status.TotalFiles, - status.BytesPerSec/1e6, + humanize.IBytes(uint64(status.BytesPerSec)), status.ETA.Round(time.Second)) } else { - log.Progressf("Scanning: %d/%d files, %.1f MB/s", + log.Progressf("Scanning: %d/%d files, %s/s", status.ScannedFiles, status.TotalFiles, - status.BytesPerSec/1e6) + humanize.IBytes(uint64(status.BytesPerSec))) } } log.ProgressDone() @@ -115,9 +125,21 @@ func (mfa *CLIApp) generateManifestOperation(ctx *cli.Context) error { return fmt.Errorf("failed to generate manifest: %w", err) } + // Close file before rename to ensure all data is flushed + if err := outFile.Close(); err != nil { + return fmt.Errorf("failed to close temp file: %w", err) + } + + // Atomic rename + if err := mfa.Fs.Rename(tmpPath, outputPath); err != nil { + return fmt.Errorf("failed to rename temp file: %w", err) + } + + success = true + elapsed := time.Since(mfa.startupTime).Seconds() - rate := float64(s.TotalBytes()) / elapsed / 1e6 - log.Infof("wrote %d files (%.1f MB) to %s in %.1fs (%.1f MB/s)", s.FileCount(), float64(s.TotalBytes())/1e6, outputPath, elapsed, rate) + rate := float64(s.TotalBytes()) / elapsed + log.Infof("wrote %d files (%s) to %s in %.1fs (%s/s)", s.FileCount(), humanize.IBytes(uint64(s.TotalBytes())), outputPath, elapsed, humanize.IBytes(uint64(rate))) return nil } diff --git a/internal/cli/mfer.go b/internal/cli/mfer.go index d020279..d72f55d 100644 --- a/internal/cli/mfer.go +++ b/internal/cli/mfer.go @@ -57,14 +57,31 @@ func (mfa *CLIApp) VersionString() string { return mfer.Version } -func (mfa *CLIApp) setVerbosity(quiet bool, v int) { +func (mfa *CLIApp) setVerbosity(c *cli.Context) { _, present := os.LookupEnv("MFER_DEBUG") if present { log.EnableDebugLogging() - } else if quiet { + } else if c.Bool("quiet") { log.SetLevel(log.ErrorLevel) } else { - log.SetLevelFromVerbosity(v) + log.SetLevelFromVerbosity(c.Count("verbose")) + } +} + +// commonFlags returns the flags shared by most commands (-v, -q) +func commonFlags() []cli.Flag { + return []cli.Flag{ + &cli.BoolFlag{ + Name: "verbose", + Aliases: []string{"v"}, + Usage: "Increase verbosity (-v for verbose, -vv for debug)", + Count: new(int), + }, + &cli.BoolFlag{ + Name: "quiet", + Aliases: []string{"q"}, + Usage: "Suppress output except errors", + }, } } @@ -80,8 +97,6 @@ func (mfa *CLIApp) run(args []string) { log.SetOutput(mfa.Stdout, mfa.Stderr) log.Init() - var verbosity int - mfa.app = &cli.App{ Name: mfa.appname, Usage: "Manifest generator", @@ -96,30 +111,17 @@ func (mfa *CLIApp) run(args []string) { mfa.printBanner() return cli.ShowAppHelp(c) }, - Flags: []cli.Flag{ - &cli.BoolFlag{ - Name: "verbose", - Usage: "Verbosity level", - Aliases: []string{"v"}, - Count: &verbosity, - }, - &cli.BoolFlag{ - Name: "quiet", - Usage: "don't produce output except on error", - Aliases: []string{"q"}, - }, - }, Commands: []*cli.Command{ { Name: "generate", Aliases: []string{"gen"}, Usage: "Generate manifest file", Action: func(c *cli.Context) error { - mfa.setVerbosity(c.Bool("quiet"), verbosity) + mfa.setVerbosity(c) mfa.printBanner() return mfa.generateManifestOperation(c) }, - Flags: []cli.Flag{ + Flags: append(commonFlags(), &cli.BoolFlag{ Name: "FollowSymLinks", Aliases: []string{"follow-symlinks"}, @@ -146,18 +148,18 @@ func (mfa *CLIApp) run(args []string) { Aliases: []string{"P"}, Usage: "Show progress during enumeration and scanning", }, - }, + ), }, { Name: "check", Usage: "Validate files using manifest file", ArgsUsage: "[manifest file]", Action: func(c *cli.Context) error { - mfa.setVerbosity(c.Bool("quiet"), verbosity) + mfa.setVerbosity(c) mfa.printBanner() return mfa.checkManifestOperation(c) }, - Flags: []cli.Flag{ + Flags: append(commonFlags(), &cli.StringFlag{ Name: "base", Aliases: []string{"b"}, @@ -173,18 +175,18 @@ func (mfa *CLIApp) run(args []string) { Name: "no-extra-files", Usage: "Fail if files exist in base directory that are not in manifest", }, - }, + ), }, { Name: "freshen", Usage: "Update manifest with changed, new, and removed files", ArgsUsage: "[manifest file]", Action: func(c *cli.Context) error { - mfa.setVerbosity(c.Bool("quiet"), verbosity) + mfa.setVerbosity(c) mfa.printBanner() return mfa.freshenManifestOperation(c) }, - Flags: []cli.Flag{ + Flags: append(commonFlags(), &cli.StringFlag{ Name: "base", Aliases: []string{"b"}, @@ -206,7 +208,7 @@ func (mfa *CLIApp) run(args []string) { Aliases: []string{"P"}, Usage: "Show progress during scanning and hashing", }, - }, + ), }, { Name: "version", @@ -240,10 +242,11 @@ func (mfa *CLIApp) run(args []string) { Name: "fetch", Usage: "fetch manifest and referenced files", Action: func(c *cli.Context) error { - mfa.setVerbosity(c.Bool("quiet"), verbosity) + mfa.setVerbosity(c) mfa.printBanner() return mfa.fetchManifestOperation(c) }, + Flags: commonFlags(), }, }, } diff --git a/internal/log/log.go b/internal/log/log.go index 74bb9b6..7f9364b 100644 --- a/internal/log/log.go +++ b/internal/log/log.go @@ -266,7 +266,8 @@ func Progressf(format string, args ...interface{}) { pterm.Printf("\r"+format, args...) } -// ProgressDone completes a progress line by printing a newline. +// ProgressDone clears the progress line when progress is complete. func ProgressDone() { - pterm.Println() + // Clear the line with spaces and return to beginning + pterm.Print("\r\033[K") } diff --git a/internal/scanner/scanner.go b/internal/scanner/scanner.go index 2c62575..04c6227 100644 --- a/internal/scanner/scanner.go +++ b/internal/scanner/scanner.go @@ -11,6 +11,7 @@ import ( "time" "github.com/spf13/afero" + "sneak.berlin/go/mfer/internal/log" "sneak.berlin/go/mfer/mfer" ) @@ -353,6 +354,8 @@ func (s *Scanner) ToManifest(ctx context.Context, w io.Writer, progress chan<- S return err } + log.Debugf("+ %s (%d bytes)", entry.Path, bytesRead) + scannedFiles++ scannedBytes += bytesRead }