diff --git a/go.mod b/go.mod index 0d843e9..13a8833 100644 --- a/go.mod +++ b/go.mod @@ -6,6 +6,7 @@ require ( github.com/fatih/color v1.18.0 github.com/olekukonko/tablewriter v1.1.3 github.com/spf13/cobra v1.10.2 + github.com/stretchr/testify v1.10.0 go.uber.org/zap v1.27.1 gopkg.in/yaml.v3 v3.0.1 ) @@ -14,6 +15,7 @@ require ( github.com/clipperhouse/displaywidth v0.7.0 // indirect github.com/clipperhouse/stringish v0.1.1 // indirect github.com/clipperhouse/uax29/v2 v2.3.1 // indirect + github.com/davecgh/go-spew v1.1.1 // indirect github.com/inconshreveable/mousetrap v1.1.0 // indirect github.com/mattn/go-colorable v0.1.14 // indirect github.com/mattn/go-isatty v0.0.20 // indirect @@ -22,8 +24,8 @@ require ( github.com/olekukonko/cat v0.0.0-20250911104152-50322a0618f6 // indirect github.com/olekukonko/errors v1.2.0 // indirect github.com/olekukonko/ll v0.1.4-0.20260115111900-9e59c2286df0 // indirect + github.com/pmezard/go-difflib v1.0.0 // indirect github.com/spf13/pflag v1.0.10 // indirect - github.com/stretchr/testify v1.10.0 // indirect go.uber.org/multierr v1.11.0 // indirect golang.org/x/sys v0.40.0 // indirect gopkg.in/check.v1 v1.0.0-20200227125254-8fa46927fb4f // indirect diff --git a/internal/brew/unified_cache.go b/internal/brew/unified_cache.go index 5846865..6768264 100644 --- a/internal/brew/unified_cache.go +++ b/internal/brew/unified_cache.go @@ -6,6 +6,7 @@ import ( "fmt" "os" "path/filepath" + "strings" "sync" "time" @@ -102,13 +103,8 @@ func ResetCache() { // IsInstalled checks if a package is installed. func (c *UnifiedCache) IsInstalled(name string) bool { - c.mu.RLock() - needsRefresh := c.needsRefreshLocked() - c.mu.RUnlock() - - if needsRefresh { - _ = c.Refresh(context.Background(), false) - } + // Refresh if needed (internally checks TTL and holds appropriate locks) + _ = c.Refresh(context.Background(), false) c.mu.RLock() defer c.mu.RUnlock() @@ -174,6 +170,7 @@ func (c *UnifiedCache) Refresh(ctx context.Context, force bool) error { // Step 1: Get installed packages (brew list) installed, err := c.fetchInstalledPackages() if err != nil { + // Keep existing cache on error - better stale than empty return fmt.Errorf("failed to fetch installed packages: %w", err) } @@ -342,6 +339,7 @@ func (c *UnifiedCache) MarkUpgraded(name, newVersion string) error { defer c.mu.Unlock() state := c.Packages[name] + state.Installed = true state.InstalledVersion = newVersion state.LatestVersion = newVersion state.Outdated = false @@ -472,22 +470,32 @@ func (c *UnifiedCache) saveToDiskLocked() error { // Helper functions func splitLines(s string) []string { + if s == "" { + return []string{} + } + result := make([]string, 0) - current := "" + var line strings.Builder + line.Grow(64) // Pre-allocate reasonable line size + for _, r := range s { if r == '\n' { - trimmed := trimSpace(current) + trimmed := trimSpace(line.String()) if trimmed != "" { result = append(result, trimmed) } - current = "" + line.Reset() } else { - current += string(r) + line.WriteRune(r) } } - trimmed := trimSpace(current) - if trimmed != "" { - result = append(result, trimmed) + + // Handle last line if no trailing newline + if line.Len() > 0 { + trimmed := trimSpace(line.String()) + if trimmed != "" { + result = append(result, trimmed) + } } return result diff --git a/internal/brew/unified_cache_test.go b/internal/brew/unified_cache_test.go new file mode 100644 index 0000000..904fd8f --- /dev/null +++ b/internal/brew/unified_cache_test.go @@ -0,0 +1,237 @@ +package brew + +import ( + "context" + "path/filepath" + "testing" + "time" + + "github.com/MrSnakeDoc/keg/internal/runner" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// testRunner implements runner.CommandRunner for testing +type testRunner struct { + listOutput string + listErr error + outdatedOutput string + outdatedErr error + infoOutput string + infoErr error +} + +func (m *testRunner) Run(ctx context.Context, timeout time.Duration, mode runner.Mode, cmd string, args ...string) ([]byte, error) { + if cmd == "brew" && len(args) > 0 { + switch args[0] { + case "list": + return []byte(m.listOutput), m.listErr + case "outdated": + return []byte(m.outdatedOutput), m.outdatedErr + case "info": + return []byte(m.infoOutput), m.infoErr + } + } + return []byte{}, nil +} + +func TestUnifiedCache_IsInstalled(t *testing.T) { + r := &testRunner{ + listOutput: "wget\ncurl\njq\n", + } + + cache := &UnifiedCache{ + Packages: map[string]PackageState{ + "wget": {Installed: true, FetchedAt: time.Now()}, + "curl": {Installed: true, FetchedAt: time.Now()}, + }, + LastUpdated: time.Now(), + TTL: 1 * time.Hour, + runner: r, + } + + assert.True(t, cache.IsInstalled("wget")) + assert.True(t, cache.IsInstalled("curl")) + assert.False(t, cache.IsInstalled("nonexistent")) +} + +func TestUnifiedCache_GetInstalledSet(t *testing.T) { + cache := &UnifiedCache{ + Packages: map[string]PackageState{ + "wget": {Installed: true, FetchedAt: time.Now()}, + "curl": {Installed: true, FetchedAt: time.Now()}, + "notins": {Installed: false, FetchedAt: time.Now()}, + }, + } + + installed := cache.GetInstalledSet() + assert.Len(t, installed, 2) + assert.Contains(t, installed, "wget") + assert.Contains(t, installed, "curl") + assert.NotContains(t, installed, "notins") +} + +func TestUnifiedCache_GetOutdatedMap(t *testing.T) { + cache := &UnifiedCache{ + Packages: map[string]PackageState{ + "wget": { + Installed: true, + Outdated: true, + InstalledVersion: "1.0.0", + LatestVersion: "1.1.0", + FetchedAt: time.Now(), + }, + "curl": { + Installed: true, + Outdated: false, + FetchedAt: time.Now(), + }, + "jq": { + Installed: false, + Outdated: true, + InstalledVersion: "1.5.0", + LatestVersion: "1.6.0", + FetchedAt: time.Now(), + }, + }, + } + + outdated := cache.GetOutdatedMap() + assert.Len(t, outdated, 1, "only installed+outdated packages should be returned") + assert.Contains(t, outdated, "wget") + assert.Equal(t, "1.0.0", outdated["wget"].InstalledVersion) + assert.Equal(t, "1.1.0", outdated["wget"].LatestVersion) +} + +func TestUnifiedCache_MarkInstalled(t *testing.T) { + cache := &UnifiedCache{ + Packages: make(map[string]PackageState), + cachePath: filepath.Join(t.TempDir(), "cache.json"), + } + + err := cache.MarkInstalled("wget", "1.0.0") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + assert.True(t, cache.Packages["wget"].Installed) + assert.Equal(t, "1.0.0", cache.Packages["wget"].InstalledVersion) + assert.Equal(t, "1.0.0", cache.Packages["wget"].LatestVersion) + assert.False(t, cache.Packages["wget"].Outdated) +} + +func TestUnifiedCache_MarkUpgraded(t *testing.T) { + cache := &UnifiedCache{ + Packages: map[string]PackageState{ + "wget": { + Installed: true, + Outdated: true, + InstalledVersion: "1.0.0", + LatestVersion: "1.1.0", + }, + }, + cachePath: filepath.Join(t.TempDir(), "cache.json"), + } + + err := cache.MarkUpgraded("wget", "1.1.0") + require.NoError(t, err) + + assert.True(t, cache.Packages["wget"].Installed) + assert.Equal(t, "1.1.0", cache.Packages["wget"].InstalledVersion) + assert.Equal(t, "1.1.0", cache.Packages["wget"].LatestVersion) + assert.False(t, cache.Packages["wget"].Outdated) +} + +func TestUnifiedCache_MarkUpgraded_SetsInstalledFlag(t *testing.T) { + cache := &UnifiedCache{ + Packages: make(map[string]PackageState), + cachePath: filepath.Join(t.TempDir(), "cache.json"), + } + + // Upgrade a package that doesn't exist in cache yet + err := cache.MarkUpgraded("newpkg", "2.0.0") + require.NoError(t, err) + + // Should be marked as installed + assert.True(t, cache.Packages["newpkg"].Installed, "upgraded package should be marked as installed") + assert.Equal(t, "2.0.0", cache.Packages["newpkg"].InstalledVersion) +} + +func TestUnifiedCache_MarkUninstalled(t *testing.T) { + cache := &UnifiedCache{ + Packages: map[string]PackageState{ + "wget": { + Installed: true, + InstalledVersion: "1.0.0", + }, + "curl": { + Installed: true, + }, + }, + cachePath: filepath.Join(t.TempDir(), "cache.json"), + } + + err := cache.MarkUninstalled("wget") + require.NoError(t, err) + + // Package should be completely removed from cache + assert.NotContains(t, cache.Packages, "wget", "uninstalled package should be deleted from cache") + // Other packages should remain + assert.Contains(t, cache.Packages, "curl") +} + +func TestUnifiedCache_RefreshVersions_EmptyList(t *testing.T) { + cache := &UnifiedCache{ + Packages: make(map[string]PackageState), + runner: &testRunner{}, + } + + err := cache.RefreshVersions(context.Background(), []string{}) + require.NoError(t, err) +} + +func TestSplitLines_HandlesVariousInputs(t *testing.T) { + tests := []struct { + name string + input string + expected []string + }{ + { + name: "empty string", + input: "", + expected: []string{}, + }, + { + name: "single line", + input: "wget", + expected: []string{"wget"}, + }, + { + name: "multiple lines", + input: "wget\ncurl\njq", + expected: []string{"wget", "curl", "jq"}, + }, + { + name: "lines with spaces", + input: "wget\n curl \njq", + expected: []string{"wget", "curl", "jq"}, + }, + { + name: "empty lines", + input: "wget\n\ncurl\n\n", + expected: []string{"wget", "curl"}, + }, + { + name: "windows line endings", + input: "wget\r\ncurl\r\njq", + expected: []string{"wget", "curl", "jq"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := splitLines(tt.input) + assert.Equal(t, tt.expected, result) + }) + } +} diff --git a/internal/core/core.go b/internal/core/core.go index b674d01..148ff92 100644 --- a/internal/core/core.go +++ b/internal/core/core.go @@ -103,9 +103,7 @@ func NewBase(config *models.Config, r runner.CommandRunner) *Base { cache, err := brew.GetCache(r) if err != nil { - logger.Debug("failed to initialize unified cache: %v", err) - // Fallback to empty cache - cache = &brew.UnifiedCache{} + logger.Fatal("failed to initialize unified cache: %v", err) } return &Base{ diff --git a/internal/logger/logger.go b/internal/logger/logger.go index d1565c6..51bfcf0 100644 --- a/internal/logger/logger.go +++ b/internal/logger/logger.go @@ -120,12 +120,15 @@ func Info(msg string, args ...interface{}) { if !ensureReady() { return } - // Format OUTSIDE the lock (80% of the work) + // Format message outside the lock formatted := formatMessage("✨ ", msg, args...) // Fast path: direct write for non-JSON mode if !useJSON.Load() { - writeDirect(formatted) + mu.RLock() + colorized := p.Info(formatted) + mu.RUnlock() + writeDirect(colorized) return } @@ -142,7 +145,10 @@ func Success(msg string, args ...interface{}) { formatted := formatMessage("✅ ", msg, args...) if !useJSON.Load() { - writeDirect(p.Success(formatted)) + mu.RLock() + colorized := p.Success(formatted) + mu.RUnlock() + writeDirect(colorized) return } @@ -158,7 +164,10 @@ func LogError(msg string, args ...interface{}) { formatted := formatMessage("❌ ", msg, args...) if !useJSON.Load() { - writeDirect(p.Error(formatted)) + mu.RLock() + colorized := p.Error(formatted) + mu.RUnlock() + writeDirect(colorized) return } @@ -174,7 +183,10 @@ func Warn(msg string, args ...interface{}) { formatted := formatMessage("⚠️ ", msg, args...) if !useJSON.Load() { - writeDirect(p.Warning(formatted)) + mu.RLock() + colorized := p.Warning(formatted) + mu.RUnlock() + writeDirect(colorized) return } @@ -199,7 +211,10 @@ func WarnInline(msg string, args ...interface{}) { return } formatted := formatMessage("⚠️ ", msg, args...) - writeDirect(p.Warning(formatted)) + mu.RLock() + colorized := p.Warning(formatted) + mu.RUnlock() + writeDirect(colorized) } func Debug(msg string, args ...interface{}) { @@ -218,7 +233,10 @@ func Debug(msg string, args ...interface{}) { formatted := formatMessage("🛠️ ", msg, args...) if !useJSON.Load() { - writeDirect(p.Debug(formatted)) + mu.RLock() + colorized := p.Debug(formatted) + mu.RUnlock() + writeDirect(colorized) return } @@ -285,6 +303,7 @@ func formatMessage(prefix, msg string, args ...interface{}) string { // writeDirect writes directly to output with minimal locking. // Used for non-JSON mode to bypass zap overhead. +// Expects msg to be already fully formatted (emoji + color). func writeDirect(msg string) { mu.RLock() w := out @@ -295,5 +314,5 @@ func writeDirect(msg string) { } // Write directly without holding lock - _, _ = fmt.Fprintln(w, p.Info(msg)) + _, _ = fmt.Fprintln(w, msg) } diff --git a/internal/middleware/middleware.go b/internal/middleware/middleware.go index d23f507..696d7ec 100644 --- a/internal/middleware/middleware.go +++ b/internal/middleware/middleware.go @@ -43,21 +43,20 @@ func UseMiddlewareChain(middlewares ...MiddlewareFunc) MiddlewareChain { } // Execute middleware chain - // We still need closures here (it's the nature of middleware pattern) - // but we minimize allocations by reusing the pre-stored slice - var chain func(int) error - chain = func(i int) error { + // Chain now properly propagates modified cmd/args through the chain + var chain func(*cobra.Command, []string, int) error + chain = func(currentCmd *cobra.Command, currentArgs []string, i int) error { if i >= mwLen { if orig != nil { - return orig(c, a) + return orig(currentCmd, currentArgs) } return nil } - return mwCopy[i](c, a, func(nc *cobra.Command, na []string) error { - return chain(i + 1) + return mwCopy[i](currentCmd, currentArgs, func(nc *cobra.Command, na []string) error { + return chain(nc, na, i+1) }) } - return chain(0) + return chain(c, a, 0) } return cmd }