diff --git a/internal/diff/git.go b/internal/diff/git.go index c880453c..61d8928f 100644 --- a/internal/diff/git.go +++ b/internal/diff/git.go @@ -10,6 +10,8 @@ import ( "slices" "strings" + "github.com/bmatcuk/doublestar/v4" + "github.com/alibaba/open-code-review/internal/gitcmd" "github.com/alibaba/open-code-review/internal/model" ) @@ -184,8 +186,16 @@ func (p *Provider) loadGitignorePatterns() []string { // isPathExcluded returns true when the given relative file path should be skipped // based on hardcoded dir rules or .gitignore patterns. +// +// Patterns are resolved the way git resolves them: in file order, with the LAST +// matching pattern deciding, and a leading "!" inverting that pattern's verdict. +// Order matters because the "allow list" idiom (ignore everything with `*`, then +// re-include with `!` lines — github/gitignore ships one per language) is only +// correct under last-match-wins. Treating negations as unmatchable made every +// file in such a repository look excluded, so a review silently covered nothing. func (p *Provider) isPathExcluded(relPath string, gitignorePatterns []string) bool { - // Hardcoded directory prefix checks + // Hardcoded directory prefix checks. These are an unconditional blocklist: + // a .gitignore negation cannot re-admit .git/ or node_modules/. for _, prefix := range providerDirIgnoreDirs { dirPart := strings.TrimSuffix(prefix, "/") if relPath == dirPart || strings.HasPrefix(relPath, prefix) { @@ -193,45 +203,89 @@ func (p *Provider) isPathExcluded(relPath string, gitignorePatterns []string) bo } } - // .gitignore pattern matching + excluded := false for _, pat := range gitignorePatterns { - if matchGitignorePattern(relPath, pat) { - return true + body, negated := strings.CutPrefix(pat, "!") + if body == "" { + continue + } + + // Directory-only patterns (trailing "/") apply to directories, never to + // files. Git uses a negated one such as `!*/` to keep descending into + // subdirectories, not to re-admit the files inside them — honouring it + // here would readmit everything below the root. + if negated && strings.HasSuffix(body, "/") { + continue + } + + if matchGitignoreBody(relPath, body) { + excluded = !negated } } - return false + return excluded } // matchGitignorePattern checks if relPath matches a single .gitignore pattern. +// +// Polarity is not this function's concern: a negated pattern reports false, so +// callers testing one pattern in isolation still read it as "does this exclude +// the path". Ordered resolution across a whole pattern list, where negations do +// carry meaning, lives in isPathExcluded. func matchGitignorePattern(relPath, pat string) bool { + if strings.HasPrefix(pat, "!") { + return false + } + return matchGitignoreBody(relPath, pat) +} + +// matchGitignoreBody reports whether relPath matches a single pattern body — +// the pattern with any leading "!" already stripped. +func matchGitignoreBody(relPath, body string) bool { // Directory-only patterns (trailing /) - if before, ok := strings.CutSuffix(pat, "/"); ok { - dirName := before - // Match if any path segment equals the dir name + if before, ok := strings.CutSuffix(body, "/"); ok { + // Only a real directory component can match, so the final segment (the + // file's own name) is excluded from consideration: `vendor/` must not + // match a *file* named "vendor", and `*/` must not match every path. segments := strings.Split(relPath, "/") - return slices.Contains(segments, dirName) + return slices.Contains(segments[:max(len(segments)-1, 0)], before) } - // Negation patterns are not needed for exclusion purposes - if strings.HasPrefix(pat, "!") { - return false + // A leading "/" anchors the pattern to the repository root rather than + // making it a path pattern; "/.golangci.yml" addresses the root file. + anchored := false + if trimmed, ok := strings.CutPrefix(body, "/"); ok { + body, anchored = trimmed, true } - // Patterns without / match basename - if !strings.Contains(pat, "/") { - base := filepath.Base(relPath) - if matched, _ := filepath.Match(pat, base); matched { - return true + // "**" is not expressible with filepath.Match, so patterns containing it go + // through doublestar, which implements gitignore's globstar semantics. + if strings.Contains(body, "**") { + matched, err := doublestar.Match(body, relPath) + return err == nil && matched + } + + // Patterns without / match basename — unless anchored, where the pattern + // addresses that name at the root only. + if !strings.Contains(body, "/") { + target := filepath.Base(relPath) + if anchored { + target = relPath } - return false + matched, _ := filepath.Match(body, target) + return matched } // Patterns with / match against the full relative path - if matched, _ := filepath.Match(pat, relPath); matched { + if matched, _ := filepath.Match(body, relPath); matched { return true } - // Also try matching against suffix of path - if strings.HasSuffix(relPath, pat) { + // Also try matching against suffix of path, but not for anchored patterns: + // "/docs/api.md" names one file, not any path ending that way. + // + // The leading "/" makes the suffix start on a path component: without it + // "src/main.go" also matches "othersrc/main.go", because the tail of + // "othersrc" completes the pattern. + if !anchored && strings.HasSuffix(relPath, "/"+body) { return true } diff --git a/internal/diff/gitignore_test.go b/internal/diff/gitignore_test.go index ee86a912..82dd463c 100644 --- a/internal/diff/gitignore_test.go +++ b/internal/diff/gitignore_test.go @@ -81,6 +81,89 @@ func TestIsPathExcluded(t *testing.T) { } } +// allowListGitignore is the "ignore everything, then re-include" idiom from +// github/gitignore's Go.AllowList.gitignore. Repositories using it are the +// reason negation patterns cannot be discarded: the leading `*` matches every +// basename, so without honouring the `!` lines every file in the repository +// looks excluded and a review silently covers nothing. +var allowListGitignore = []string{ + "*", + "!/.github/**/*", + "!/.gitignore", + "!/.tool-versions", + "!/.golangci.yml", + "!Taskfile.yml", + "!*.go", + "!go.sum", + "!go.mod", + "!README.md", + "!LICENSE", + "!scripts/*", + "!*/", +} + +func TestIsPathExcluded_AllowListGitignore(t *testing.T) { + tests := []struct { + name string + relPath string + want bool + }{ + {"go file at root", "main.go", false}, + {"go file nested", "internal/diff/git.go", false}, + {"go test file nested", "internal/diff/git_test.go", false}, + {"go.mod", "go.mod", false}, + {"go.sum", "go.sum", false}, + {"readme", "README.md", false}, + {"license", "LICENSE", false}, + {"root-anchored dotfile", ".golangci.yml", false}, + {"root-anchored tool-versions", ".tool-versions", false}, + {"doublestar workflow", ".github/workflows/ci.yml", false}, + {"script by dir glob", "scripts/build.sh", false}, + {"taskfile", "Taskfile.yml", false}, + + // Still excluded: nothing re-includes these. A negated directory-only + // pattern (`!*/`) must not re-include a file, or the trailing entry + // would readmit everything below the root. + {"build artifact at root", "coverage.out", true}, + {"build artifact nested", "internal/diff/coverage.out", true}, + {"binary at root", "ocr", true}, + {"unrelated yaml nested", "internal/testdata/fixture.yaml", true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := IsPathExcluded(".", tt.relPath, allowListGitignore) + if got != tt.want { + t.Errorf("IsPathExcluded(%q) = %v, want %v", tt.relPath, got, tt.want) + } + }) + } +} + +// TestIsPathExcluded_LastMatchWins pins ordering semantics: gitignore resolves +// a path by the LAST pattern that matches it, not the first. +func TestIsPathExcluded_LastMatchWins(t *testing.T) { + tests := []struct { + name string + relPath string + patterns []string + want bool + }{ + {"negation after exclusion re-includes", "important.log", []string{"*.log", "!important.log"}, false}, + {"exclusion after negation re-excludes", "important.log", []string{"!important.log", "*.log"}, true}, + {"negation of unmatched path is inert", "main.go", []string{"!important.log"}, false}, + {"hardcoded dirs are not negatable", ".git/config", []string{"!.git/config"}, true}, + {"blocklist still works", "debug.log", []string{"*.log"}, true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := IsPathExcluded(".", tt.relPath, tt.patterns) + if got != tt.want { + t.Errorf("IsPathExcluded(%q, %v) = %v, want %v", tt.relPath, tt.patterns, got, tt.want) + } + }) + } +} + func TestMatchGitignorePattern(t *testing.T) { tests := []struct { name string @@ -97,6 +180,11 @@ func TestMatchGitignorePattern(t *testing.T) { {"full path no match", "src/api.md", "docs/*.md", false}, {"negation pattern", "important.log", "!important.log", false}, {"path suffix match", "src/generated/api.go", "generated/api.go", true}, + // The suffix has to begin on a path component. "othersrc" ends in + // "src", which would complete the pattern on a plain string suffix + // check and exclude a directory git never matched. + {"path suffix respects component boundary", "othersrc/main.go", "src/main.go", false}, + {"path suffix at root is not a suffix match", "src/main.go", "rc/main.go", false}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) {