-
Notifications
You must be signed in to change notification settings - Fork 5
[fix] Serialize gitignore updates #290
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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") | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. High — The lock directory is created at Suggested fix: Move the lock path inside |
||
| 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) { | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. High — Stale lock on process crash
This is a strict reliability regression: the original bug produced a cosmetic duplicate line; this introduces a failure mode where all gitignore operations hard-fail after a crash. Suggested fix: Write the current PID into a file inside the lock dir at creation time. On each failed |
||
| 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 { | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Low — func gitignorePattern(dir, file string) string {
return dir + "/" + file
}This function is called in exactly two places and its body is a single string concatenation — the same expression already appears inline everywhere else in this file. It adds a layer of naming indirection without capturing any non-obvious invariant. Suggested fix: Inline |
||
| return dir + "/" + file | ||
| } | ||
|
|
||
| func removeGitignoreEntryVariantsLocked(repoRoot, dir, file string) { | ||
| entry := gitignorePattern(dir, file) | ||
| _, _ = removeGitignoreEntryLocked(repoRoot, entry) | ||
| if legacyEntry := dir + "\\" + file; legacyEntry != entry { | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Low — Legacy backslash guard is vacuously true if legacyEntry := dir + "\\\\"; legacyEntry != entry {On every platform Suggested fix: Remove the guard and call |
||
| _, _ = removeGitignoreEntryLocked(repoRoot, legacyEntry) | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Medium — Package-level
Suggested fix: Replace |
||
| 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() | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Medium —
HasGitignoreEntryis exported but deliberately unguarded: hidden check-then-act trapThe public
HasGitignoreEntrydelegates straight tohasGitignoreEntrywithout acquiring the lock. Any caller that usesHasGitignoreEntryand then conditionally callsEnsureGitignorere-introduces the original TOCTOU. The contract is implicit: this function is safe only for read-only status checks, never in check-then-act patterns.Suggested fix: Either make
HasGitignoreEntrypackage-private (no external callers are visible in this diff), or add a doc comment:// HasGitignoreEntry does not acquire the .gitignore lock. Never use it in a check-then-act pattern; use EnsureGitignore or EnsureLocalGlobIgnored for atomic read-modify-write.