fix(diff): honor .gitignore negation patterns

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,54 +186,106 @@ 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) {
 			return true
 		}
 	}
 
-	// .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
 	}
 

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) {