From 4fe90cabd8e8533c13fa6ce99cff030db0f125bd Mon Sep 17 00:00:00 2001 From: Martin Vogel Date: Thu, 12 Mar 2026 13:47:51 +0100 Subject: [PATCH] Fix file handle leak in gitignore loading for Windows compatibility MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace the go-gitignore library's NewRepository (which opens .gitignore files via os.Open but never closes the handles) with a custom repoMatcher that reads files via os.ReadFile and passes strings.NewReader to the library's New() function. This ensures all file handles are closed immediately after reading. The library's ignore.Relative() pattern matching is preserved — only the hierarchy walker and file loading are reimplemented. The matching algorithm follows standard git precedence: child .gitignore overrides parent, negation patterns in child override matching patterns in parent, and .git/info/exclude is checked last. Fixes Windows CI where leaked handles prevented t.TempDir() cleanup. --- internal/discover/gitignore.go | 164 +++++++++++++++++++++++++++++---- 1 file changed, 146 insertions(+), 18 deletions(-) diff --git a/internal/discover/gitignore.go b/internal/discover/gitignore.go index 15f55aae9..87297cbf0 100644 --- a/internal/discover/gitignore.go +++ b/internal/discover/gitignore.go @@ -1,9 +1,10 @@ package discover import ( - "log/slog" "os" "path/filepath" + "strings" + "sync" gitignore "github.com/boyter/gocodewalker/go-gitignore" ) @@ -11,37 +12,27 @@ import ( // ignoreMatchers holds the layered ignore matchers for a repository. // Evaluation order: hardcoded patterns (fastest) → gitignore → cbmignore. type ignoreMatchers struct { - gitignore gitignore.GitIgnore // .gitignore hierarchy + .git/info/exclude (nil for non-git repos) + gitignore *repoMatcher // .gitignore hierarchy + .git/info/exclude (nil for non-git repos) cbmignore gitignore.GitIgnore // .cbmignore at repo root (nil if absent) } // loadIgnoreMatchers loads all gitignore-style matchers for the repository. // .gitignore is loaded only for git repos (presence of .git dir). // .cbmignore stacks on top — patterns there additionally exclude from indexing. +// +// All files are read and closed immediately — no file handle leaks. +// This is critical for Windows where open handles prevent t.TempDir() cleanup. func loadIgnoreMatchers(repoPath string) ignoreMatchers { var m ignoreMatchers // .gitignore — only for git repos gitDir := filepath.Join(repoPath, ".git") if info, err := os.Stat(gitDir); err == nil && info.IsDir() { - gi, err := gitignore.NewRepository(repoPath) - if err != nil { - slog.Warn("discover.gitignore_load_err", "err", err) - } else { - m.gitignore = gi - } + m.gitignore = newRepoMatcher(repoPath) } // .cbmignore — repo-root file with gitignore-style patterns - cbmPath := filepath.Join(repoPath, ".cbmignore") - if _, err := os.Stat(cbmPath); err == nil { - ci, err := gitignore.NewFromFile(cbmPath) - if err != nil { - slog.Warn("discover.cbmignore_load_err", "err", err) - } else { - m.cbmignore = ci - } - } + m.cbmignore = safeLoadGitignore(filepath.Join(repoPath, ".cbmignore")) return m } @@ -50,7 +41,7 @@ func loadIgnoreMatchers(repoPath string) ignoreMatchers { // absPath must be absolute; isDir indicates whether the path is a directory. func (m *ignoreMatchers) shouldIgnore(absPath string, isDir bool) bool { if m.gitignore != nil { - if match := m.gitignore.Absolute(absPath, isDir); match != nil && match.Ignore() { + if m.gitignore.shouldIgnore(absPath, isDir) { return true } } @@ -61,3 +52,140 @@ func (m *ignoreMatchers) shouldIgnore(absPath string, isDir bool) bool { } return false } + +// safeLoadGitignore reads a gitignore-style file and returns a GitIgnore matcher. +// The file is read completely and closed immediately — no handle leak. +// Returns nil if the file doesn't exist or can't be read. +func safeLoadGitignore(path string) gitignore.GitIgnore { + content, err := os.ReadFile(path) + if err != nil { + return nil + } + base := filepath.Dir(path) + return gitignore.New(strings.NewReader(string(content)), base, nil) +} + +// repoMatcher implements gitignore-style matching across a repository's +// .gitignore hierarchy. It replaces the library's repository type to avoid +// file handle leaks (the library's NewWithErrors opens files but never closes them). +// +// Pattern matching uses the library's ignore.Relative() — only the hierarchy +// walker and file loading are reimplemented. +type repoMatcher struct { + base string + exclude gitignore.GitIgnore // .git/info/exclude patterns + mu sync.Mutex // protects cache + cache map[string]gitignore.GitIgnore // dir path → loaded .gitignore (nil = no file) +} + +func newRepoMatcher(base string) *repoMatcher { + m := &repoMatcher{ + base: base, + cache: make(map[string]gitignore.GitIgnore), + } + m.exclude = safeLoadGitignore(filepath.Join(base, ".git", "info", "exclude")) + return m +} + +// loadDir loads the .gitignore for a directory, caching the result. +func (m *repoMatcher) loadDir(dir string) gitignore.GitIgnore { + m.mu.Lock() + defer m.mu.Unlock() + if gi, ok := m.cache[dir]; ok { + return gi // may be nil (cached miss) + } + gi := safeLoadGitignore(filepath.Join(dir, ".gitignore")) + m.cache[dir] = gi + return gi +} + +// shouldIgnore checks if an absolute path should be ignored according to +// the repository's .gitignore hierarchy, following standard git precedence: +// - child .gitignore overrides parent .gitignore +// - negation patterns (!) in child override matching patterns in parent +// - .git/info/exclude is checked last (lowest priority) +func (m *repoMatcher) shouldIgnore(absPath string, isDir bool) bool { + if !strings.HasPrefix(absPath, m.base) { + return false + } + if absPath == m.base { + return false + } + + rel, err := filepath.Rel(m.base, absPath) + if err != nil { + return false + } + rel = filepath.ToSlash(rel) + if rel == "." { + return false + } + + return m.matchRel(rel, isDir) +} + +// matchRel implements the core gitignore hierarchy matching algorithm. +// Mirrors the logic in the library's repository.Relative(). +func (m *repoMatcher) matchRel(rel string, isDir bool) bool { + rel = filepath.ToSlash(filepath.Clean(rel)) + if rel == "." { + return false + } + + // First: is the parent directory ignored? + // An ignored parent means the child is also ignored. + parent, local := splitPath(rel) + if parent != "" { + if m.matchRel(parent, true) { + return true + } + } + + // Walk from the file's directory up to the repo root, + // checking .gitignore at each level. First match wins. + dir := parent + curLocal := local + for { + absDir := filepath.Join(m.base, filepath.FromSlash(dir)) + if dir == "" { + absDir = m.base + } + gi := m.loadDir(absDir) + if gi != nil { + if match := gi.Relative(curLocal, isDir); match != nil { + return match.Ignore() + } + } + + if dir == "" { + break + } + + // Walk up: prepend the current directory component to local + var last string + dir, last = splitPath(dir) + curLocal = last + "/" + curLocal + } + + // Finally check .git/info/exclude (lowest priority) + if m.exclude != nil { + if match := m.exclude.Relative(rel, isDir); match != nil { + return match.Ignore() + } + } + + return false +} + +// splitPath splits a forward-slash path into parent and last component. +// "a/b/c" → ("a/b", "c"), "a" → ("", "a"), "" → ("", "") +func splitPath(p string) (parent, last string) { + if p == "" { + return "", "" + } + i := strings.LastIndex(p, "/") + if i < 0 { + return "", p + } + return p[:i], p[i+1:] +}