From fe30e4da7f21f307ae127f6ebcad643fab8dabdc Mon Sep 17 00:00:00 2001 From: Bobby Date: Sat, 6 Jun 2026 23:55:43 +0700 Subject: [PATCH] [fix] Serialize gitignore updates --- cmd/init.go | 4 +- cmd/init_personal_test.go | 2 +- cmd/init_test.go | 6 +- internal/fileutil/gitignore.go | 110 +++++++++++++++---- internal/fileutil/gitignore_test.go | 162 ++++++++++++++++++++++++++++ tests/e2e/init_test.go | 12 +-- tests/e2e/trust_e2e_test.go | 2 +- 7 files changed, 263 insertions(+), 35 deletions(-) diff --git a/cmd/init.go b/cmd/init.go index 7953049..017b8c1 100644 --- a/cmd/init.go +++ b/cmd/init.go @@ -82,7 +82,7 @@ register MCP — it only updates agent files. Registration is idempotent.`, dirPath := filepath.Join(repoRoot, config.DirName) legacyPath := filepath.Join(repoRoot, config.FileName) - globEntry := filepath.Join(config.DirName, config.LocalGlob) + globEntry := config.DirName + "/" + config.LocalGlob dirEntry := config.DirName + "/" gitignoreEntry := globEntry @@ -307,7 +307,7 @@ func reconcileExistingIgnore(cmd *cobra.Command, repoRoot string, personal bool) return errhint.WithFix(fmt.Errorf("failed to update .gitignore: %w", err), gitignoreHint) } if added { - fmt.Fprintf(cmd.OutOrStdout(), " Gitignore: %s added to .gitignore\n", filepath.Join(config.DirName, config.LocalGlob)) + fmt.Fprintf(cmd.OutOrStdout(), " Gitignore: %s added to .gitignore\n", config.DirName+"/"+config.LocalGlob) } return nil } diff --git a/cmd/init_personal_test.go b/cmd/init_personal_test.go index b777683..dc3e112 100644 --- a/cmd/init_personal_test.go +++ b/cmd/init_personal_test.go @@ -59,7 +59,7 @@ func TestInitPersonalFreshInit(t *testing.T) { if strings.Contains(content, localEntry) { t.Errorf(".gitignore should not contain %q in personal mode, got:\n%s", localEntry, content) } - globEntry := filepath.Join(config.DirName, config.LocalGlob) + globEntry := config.DirName + "/" + config.LocalGlob if strings.Contains(content, globEntry) { t.Errorf(".gitignore should not contain glob %q in personal mode, got:\n%s", globEntry, content) } diff --git a/cmd/init_test.go b/cmd/init_test.go index e6ad6c7..a7a92ea 100644 --- a/cmd/init_test.go +++ b/cmd/init_test.go @@ -112,7 +112,7 @@ func TestInitMigrationFromLegacy(t *testing.T) { if strings.Contains(content, config.FileName) { t.Error(".gitignore should not contain legacy entry after migration") } - globEntry := filepath.Join(config.DirName, config.LocalGlob) + globEntry := config.DirName + "/" + config.LocalGlob if !strings.Contains(content, globEntry) { t.Errorf(".gitignore should contain %q, got:\n%s", globEntry, content) } @@ -344,7 +344,7 @@ func TestInitFreshGitignoreAlreadyPresent(t *testing.T) { repoDir := t.TempDir() // Pre-seed .gitignore with the glob entry that rimba now writes - gitignoreEntry := filepath.Join(config.DirName, config.LocalGlob) + gitignoreEntry := config.DirName + "/" + config.LocalGlob if err := os.WriteFile(filepath.Join(repoDir, ".gitignore"), []byte(gitignoreEntry+"\n"), 0644); err != nil { t.Fatal(err) } @@ -402,7 +402,7 @@ func TestInitReInitMigratesPerFileEntries(t *testing.T) { t.Fatal(err) } content := string(data) - globEntry := filepath.Join(config.DirName, config.LocalGlob) + globEntry := config.DirName + "/" + config.LocalGlob if !strings.Contains(content, globEntry) { t.Errorf(".gitignore should contain %q after re-init, got:\n%s", globEntry, content) } diff --git a/internal/fileutil/gitignore.go b/internal/fileutil/gitignore.go index f1b7922..fc59101 100644 --- a/internal/fileutil/gitignore.go +++ b/internal/fileutil/gitignore.go @@ -1,17 +1,59 @@ package fileutil import ( + "errors" "os" "path/filepath" "strings" + "time" "github.com/lugassawan/rimba/internal/config" ) +var gitignoreLockTimeout = 2 * time.Second + // EnsureGitignore ensures that entry is present as a line in the .gitignore // file at repoRoot. If the file does not exist it is created. Returns true // if the entry was added, false if it was already present. func EnsureGitignore(repoRoot string, entry string) (added bool, retErr error) { + return withGitignoreLock(repoRoot, func() (bool, error) { + return ensureGitignoreLocked(repoRoot, entry) + }) +} + +// HasGitignoreEntry reports whether entry is present as a trimmed line in the +// .gitignore file at repoRoot. Returns false (not error) when the file is absent. +func HasGitignoreEntry(repoRoot, entry string) (bool, error) { + return hasGitignoreEntry(repoRoot, entry) +} + +// EnsureLocalGlobIgnored consolidates *.local.toml overrides under a single +// .rimba/*.local.toml gitignore glob, removing any pre-existing per-file entries. +// No-op when .rimba/ is already ignored (--personal repos). +// Returns whether the glob line was newly added. +func EnsureLocalGlobIgnored(repoRoot string) (added bool, err error) { + return withGitignoreLock(repoRoot, func() (bool, error) { + hasDir, err := hasGitignoreEntry(repoRoot, config.DirName+"/") + if err != nil || hasDir { + return false, err + } + // Best-effort cleanup: the glob below covers both files even if removal fails. + removeGitignoreEntryVariantsLocked(repoRoot, config.DirName, config.LocalFile) + removeGitignoreEntryVariantsLocked(repoRoot, config.DirName, config.TrustFile) + return ensureGitignoreLocked(repoRoot, gitignorePattern(config.DirName, config.LocalGlob)) + }) +} + +// RemoveGitignoreEntry removes entry from the .gitignore file at repoRoot. +// Returns true if the entry was removed, false if the file doesn't exist or +// the entry was not present. +func RemoveGitignoreEntry(repoRoot string, entry string) (bool, error) { + return withGitignoreLock(repoRoot, func() (bool, error) { + return removeGitignoreEntryLocked(repoRoot, entry) + }) +} + +func ensureGitignoreLocked(repoRoot string, entry string) (added bool, retErr error) { path := filepath.Join(repoRoot, ".gitignore") data, err := os.ReadFile(filepath.Clean(path)) @@ -53,9 +95,7 @@ func EnsureGitignore(repoRoot string, entry string) (added bool, retErr error) { return true, nil } -// HasGitignoreEntry reports whether entry is present as a trimmed line in the -// .gitignore file at repoRoot. Returns false (not error) when the file is absent. -func HasGitignoreEntry(repoRoot, entry string) (bool, error) { +func hasGitignoreEntry(repoRoot, entry string) (bool, error) { path := filepath.Join(repoRoot, ".gitignore") data, err := os.ReadFile(filepath.Clean(path)) if err != nil { @@ -72,25 +112,7 @@ func HasGitignoreEntry(repoRoot, entry string) (bool, error) { return false, nil } -// EnsureLocalGlobIgnored consolidates *.local.toml overrides under a single -// .rimba/*.local.toml gitignore glob, removing any pre-existing per-file entries. -// No-op when .rimba/ is already ignored (--personal repos). -// Returns whether the glob line was newly added. -func EnsureLocalGlobIgnored(repoRoot string) (added bool, err error) { - hasDir, err := HasGitignoreEntry(repoRoot, config.DirName+"/") - if err != nil || hasDir { - return false, err - } - // Best-effort cleanup: the glob below covers both files even if removal fails. - _, _ = RemoveGitignoreEntry(repoRoot, filepath.Join(config.DirName, config.LocalFile)) - _, _ = RemoveGitignoreEntry(repoRoot, filepath.Join(config.DirName, config.TrustFile)) - return EnsureGitignore(repoRoot, filepath.Join(config.DirName, config.LocalGlob)) -} - -// RemoveGitignoreEntry removes entry from the .gitignore file at repoRoot. -// Returns true if the entry was removed, false if the file doesn't exist or -// the entry was not present. -func RemoveGitignoreEntry(repoRoot string, entry string) (bool, error) { +func removeGitignoreEntryLocked(repoRoot string, entry string) (bool, error) { path := filepath.Join(repoRoot, ".gitignore") data, err := os.ReadFile(filepath.Clean(path)) @@ -118,3 +140,47 @@ func RemoveGitignoreEntry(repoRoot string, entry string) (bool, error) { return true, os.WriteFile(path, []byte(strings.Join(filtered, "\n")), 0644) //nolint:gosec // .gitignore must be world-readable for git } + +func withGitignoreLock(repoRoot string, fn func() (bool, error)) (retAdded bool, retErr error) { + lockPath := filepath.Join(repoRoot, ".gitignore.lock") + unlock, err := acquireGitignoreLock(lockPath) + if err != nil { + return false, err + } + defer func() { + if err := unlock(); retErr == nil && err != nil && !os.IsNotExist(err) { + retErr = err + } + }() + + return fn() +} + +func acquireGitignoreLock(lockPath string) (func() error, error) { + deadline := time.Now().Add(gitignoreLockTimeout) + for { + if err := os.Mkdir(lockPath, 0700); err == nil { + return func() error { + return os.Remove(lockPath) + }, nil + } else if !os.IsExist(err) { + return nil, err + } + if time.Now().After(deadline) { + return nil, errors.New("timed out waiting for .gitignore lock") + } + time.Sleep(10 * time.Millisecond) + } +} + +func gitignorePattern(dir, file string) string { + return dir + "/" + file +} + +func removeGitignoreEntryVariantsLocked(repoRoot, dir, file string) { + entry := gitignorePattern(dir, file) + _, _ = removeGitignoreEntryLocked(repoRoot, entry) + if legacyEntry := dir + "\\" + file; legacyEntry != entry { + _, _ = removeGitignoreEntryLocked(repoRoot, legacyEntry) + } +} diff --git a/internal/fileutil/gitignore_test.go b/internal/fileutil/gitignore_test.go index f21cb1c..816ddbc 100644 --- a/internal/fileutil/gitignore_test.go +++ b/internal/fileutil/gitignore_test.go @@ -4,7 +4,9 @@ import ( "os" "path/filepath" "strings" + "sync" "testing" + "time" "github.com/lugassawan/rimba/internal/config" ) @@ -116,6 +118,117 @@ func TestEnsureGitignoreIdempotent(t *testing.T) { } } +func TestEnsureGitignoreConcurrent(t *testing.T) { + dir := t.TempDir() + + const workers = 8 + previousTimeout := gitignoreLockTimeout + gitignoreLockTimeout = 10 * time.Second + t.Cleanup(func() { + gitignoreLockTimeout = previousTimeout + }) + + start := make(chan struct{}) + addedResults := make(chan bool, workers) + errs := make(chan error, workers) + + var wg sync.WaitGroup + for range workers { + wg.Go(func() { + <-start + added, err := EnsureGitignore(dir, testEntry) + if err != nil { + errs <- err + return + } + addedResults <- added + }) + } + + close(start) + wg.Wait() + close(errs) + close(addedResults) + + for err := range errs { + if err != nil { + t.Fatalf(errUnexpected, err) + } + } + + addedCount := 0 + for added := range addedResults { + if added { + addedCount++ + } + } + if addedCount != 1 { + t.Fatalf("expected exactly one caller to add the entry, got %d", addedCount) + } + + got := readFile(t, filepath.Join(dir, gitignoreFile)) + if strings.Count(got, testEntry) != 1 { + t.Errorf("expected exactly one occurrence, got %q", got) + } +} + +func TestEnsureGitignoreLockParentMissing(t *testing.T) { + repoRoot := filepath.Join(t.TempDir(), "missing") + + added, err := EnsureGitignore(repoRoot, testEntry) + if err == nil { + t.Fatal("expected error when lock parent is missing") + } + if added { + t.Fatal("expected added=false when lock cannot be acquired") + } +} + +func TestEnsureGitignoreLockTimeout(t *testing.T) { + dir := t.TempDir() + lockPath := filepath.Join(dir, ".gitignore.lock") + if err := os.Mkdir(lockPath, 0700); err != nil { + t.Fatal(err) + } + + previousTimeout := gitignoreLockTimeout + gitignoreLockTimeout = 20 * time.Millisecond + t.Cleanup(func() { + gitignoreLockTimeout = previousTimeout + _ = os.Remove(lockPath) + }) + + added, err := EnsureGitignore(dir, testEntry) + if err == nil { + t.Fatal("expected error when .gitignore lock is held") + } + if !strings.Contains(err.Error(), "timed out") { + t.Fatalf("expected timeout error, got %v", err) + } + if added { + t.Fatal("expected added=false when lock cannot be acquired") + } +} + +func TestWithGitignoreLockUnlockError(t *testing.T) { + dir := t.TempDir() + lockPath := filepath.Join(dir, ".gitignore.lock") + t.Cleanup(func() { _ = os.RemoveAll(lockPath) }) + + added, err := withGitignoreLock(dir, func() (bool, error) { + if err := os.WriteFile(filepath.Join(lockPath, "child"), []byte("held"), 0644); err != nil { + return false, err + } + return true, nil + }) + if err == nil { + t.Fatal("expected error when lock directory cannot be removed") + } + if !added { + t.Fatal("expected added result from callback to be preserved") + } +} + func TestEnsureGitignoreReadError(t *testing.T) { dir := t.TempDir() // Create .gitignore as a directory so ReadFile returns non-IsNotExist error @@ -137,6 +250,11 @@ func TestEnsureGitignoreOpenError(t *testing.T) { t.Fatal(err) } t.Cleanup(func() { _ = os.Chmod(dir, 0755) }) + probePath := filepath.Join(dir, ".write-probe") + if err := os.WriteFile(probePath, []byte("probe"), 0644); err == nil { + _ = os.Remove(probePath) + t.Skip("directory permissions do not block writes on this platform") + } _, err := EnsureGitignore(dir, testEntry) if err == nil { @@ -258,6 +376,21 @@ func TestHasGitignoreEntryNotPresent(t *testing.T) { } } +func TestHasGitignoreEntryReadError(t *testing.T) { + dir := t.TempDir() + if err := os.Mkdir(filepath.Join(dir, gitignoreFile), 0755); err != nil { + t.Fatal(err) + } + + present, err := HasGitignoreEntry(dir, testEntry) + if err == nil { + t.Fatal("expected error when .gitignore is a directory") + } + if present { + t.Fatal("expected present=false on read error") + } +} + func TestEnsureLocalGlobIgnoredPersonalMode(t *testing.T) { dir := t.TempDir() original := ".rimba/\n" @@ -304,6 +437,35 @@ func TestEnsureLocalGlobIgnoredMigration(t *testing.T) { } } +func TestEnsureLocalGlobIgnoredMigratesLegacyBackslashEntries(t *testing.T) { + dir := t.TempDir() + writeFile(t, filepath.Join(dir, gitignoreFile), + "node_modules\n.rimba\\settings.local.toml\n.rimba\\trust.local.toml\n") + + added, err := EnsureLocalGlobIgnored(dir) + if err != nil { + t.Fatalf(errUnexpected, err) + } + if !added { + t.Error("expected added=true when glob was not yet present") + } + + content := readFile(t, filepath.Join(dir, gitignoreFile)) + glob := config.DirName + "/" + config.LocalGlob + if !strings.Contains(content, glob) { + t.Errorf(".gitignore should contain %q, got:\n%s", glob, content) + } + if strings.Contains(content, ".rimba\\settings.local.toml") { + t.Errorf(".gitignore should not contain legacy settings entry, got:\n%s", content) + } + if strings.Contains(content, ".rimba\\trust.local.toml") { + t.Errorf(".gitignore should not contain legacy trust entry, got:\n%s", content) + } + if !strings.Contains(content, "node_modules") { + t.Errorf(".gitignore should preserve other entries, got:\n%s", content) + } +} + func TestEnsureLocalGlobIgnoredNoGitignore(t *testing.T) { dir := t.TempDir() diff --git a/tests/e2e/init_test.go b/tests/e2e/init_test.go index a2d93b9..0637f12 100644 --- a/tests/e2e/init_test.go +++ b/tests/e2e/init_test.go @@ -28,7 +28,7 @@ func TestInitCreatesConfigAndDir(t *testing.T) { // .gitignore is created with .rimba/*.local.toml assertFileExists(t, filepath.Join(repo, gitignoreFile)) - globEntry := filepath.Join(configDir, localGlob) + globEntry := configDir + "/" + localGlob assertGitignoreContains(t, repo, globEntry) } @@ -91,7 +91,7 @@ func TestInitAddsToGitignore(t *testing.T) { t.Fatalf("failed to write %s: %v", gitignoreFile, err) } - globEntry := filepath.Join(configDir, localGlob) + globEntry := configDir + "/" + localGlob r := rimbaSuccess(t, repo, "init") assertContains(t, r.Stdout, globEntry+" added to .gitignore") assertGitignoreContains(t, repo, globEntry) @@ -112,7 +112,7 @@ func TestInitGitignoreIdempotent(t *testing.T) { } repo := setupRepo(t) - globEntry := filepath.Join(configDir, localGlob) + globEntry := configDir + "/" + localGlob // Pre-create .gitignore already containing the glob entry if err := os.WriteFile(filepath.Join(repo, gitignoreFile), []byte(globEntry+"\n"), 0644); err != nil { t.Fatalf("failed to write %s: %v", gitignoreFile, err) @@ -177,7 +177,7 @@ func TestInitMigratesLegacyConfig(t *testing.T) { if strings.Contains(content, configFile) { t.Error(".gitignore should not contain legacy entry after migration") } - globEntry := filepath.Join(configDir, localGlob) + globEntry := configDir + "/" + localGlob assertGitignoreContains(t, repo, globEntry) } @@ -329,7 +329,7 @@ func TestInitPersonalFreshInit(t *testing.T) { assertGitignoreContains(t, repo, dirEntry) localEntry := filepath.Join(configDir, localFile) assertGitignoreNotContains(t, repo, localEntry) - globEntry := filepath.Join(configDir, localGlob) + globEntry := configDir + "/" + localGlob assertGitignoreNotContains(t, repo, globEntry) } @@ -382,7 +382,7 @@ func TestInitPersonalMigration(t *testing.T) { assertGitignoreContains(t, repo, dirEntry) localEntry := filepath.Join(configDir, localFile) assertGitignoreNotContains(t, repo, localEntry) - globEntry := filepath.Join(configDir, localGlob) + globEntry := configDir + "/" + localGlob assertGitignoreNotContains(t, repo, globEntry) } diff --git a/tests/e2e/trust_e2e_test.go b/tests/e2e/trust_e2e_test.go index 9c13580..db6477c 100644 --- a/tests/e2e/trust_e2e_test.go +++ b/tests/e2e/trust_e2e_test.go @@ -169,7 +169,7 @@ func TestTrustInitAddsGitignoreEntry(t *testing.T) { repo := setupRepo(t) rimbaSuccess(t, repo, "init") - globEntry := filepath.Join(configDir, localGlob) + globEntry := configDir + "/" + localGlob assertGitignoreContains(t, repo, globEntry) // Specific trust.local.toml entry must NOT be added (the glob covers it). assertGitignoreNotContains(t, repo, filepath.Join(configDir, trust.FileName))