fix(diff): honor .gitignore negation patterns

[cru-Luis-Rodriguez]

Description

Fixes #650.

.gitignore negation patterns were discarded outright:

// Negation patterns are not needed for exclusion purposes
if strings.HasPrefix(pat, "!") {
    return false
}

That holds for a blocklist .gitignore, but it inverts the result for the "allow list" idiom — ignore everything with *, then re-include with ! lines — which github/gitignore ships one of per language (Go.AllowList.gitignore, Java.AllowList.gitignore, …). isPathExcluded also resolved first-match-wins, and a bare * has no /, so it goes down the basename branch where filepath.Match("*", base) matches every file. The first pattern excluded everything and the ! lines that would have re-included never got a say.

The result is that every file in such a repository is treated as ignored, and it fails silently: 0 reviewable / 0 total, No files changed. — indistinguishable from a clean diff. See the issue for the full reasoning on why that's the dangerous part.

Scope is wider than diff review: internal/scan/provider.go and internal/tool/file_find.go both filter through diff.IsPathExcluded, so ocr scan and the agent's file_find tool are affected in the same repositories.

Approach

Resolve patterns the way git does — in file order, last match wins, with a leading ! inverting that pattern's verdict — and support the two constructs the allow-list idiom needs:

• a leading /, which anchors a pattern to the repository root (/.golangci.yml names the root file) rather than making it a path pattern;
• **, routed through doublestar (already a direct dependency) since filepath.Match cannot express it.

Three deliberate decisions, each the kind that's easy to get backwards:

1. The hardcoded directory blocklist still short-circuits. .git/, node_modules/, vendor/ and friends are checked before any pattern, so a ! line cannot re-admit them.

2. A negated directory-only pattern is inert. Git uses a trailing !*/ to keep descending into subdirectories, not to re-admit the files inside them. Honouring it against file paths would readmit everything below the root — in the issue's repro, coverage.out would come back reviewable. Directory-only patterns are also now matched against directory components only, so vendor/ no longer matches a file literally named vendor (previously the final path segment was considered).

3. MatchGitignorePattern keeps its current contract. A negated pattern still reports false, so callers testing one pattern in isolation continue to read it as "does this exclude the path". Ordered resolution, where polarity carries meaning, lives in isPathExcluded. The existing TestMatchGitignorePattern case for !important.log is unchanged and still passes.

The matcher body moved into matchGitignoreBody, which takes a pattern with any ! already stripped; matchGitignorePattern is now a thin polarity-preserving wrapper over it.

Type of Change

• Bug fix (non-breaking change that fixes an issue)
• New feature (non-breaking change that adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Refactoring (no functional changes)
• Documentation update
• CI / Build / Tooling

How Has This Been Tested?

• make test passes locally
• Manual testing (describe below)

Unit tests. Two new tables in internal/diff/gitignore_test.go, written before the fix and confirmed failing against main:

• TestIsPathExcluded_AllowListGitignore — 16 cases over a realistic Go.AllowList.gitignore, covering root-anchored dotfiles, ** workflow paths, directory globs, and the four artifacts that must stay excluded.
• TestIsPathExcluded_LastMatchWins — pins ordering directly: negation after exclusion re-includes, exclusion after negation re-excludes, a negation of an unmatched path is inert, and the hardcoded dirs are not negatable.

Verification run (Go 1.25.6):

• go test ./internal/diff/ — ok
• gofmt -l internal/ cmd/ — clean
• go vet ./... — clean
• go test ./... — green

End-to-end, on a real repository using Go.AllowList.gitignore, with the ignore file untouched:

	ocr delegate preview -c <sha>
main	# Files (0 reviewable / 0 total)
this branch	# Files (9 reviewable / 18 total)

and on the minimal repro from #650: 0 reviewable / 0 total → 2 reviewable / 2 total, with coverage.out still correctly excluded.

_{An earlier revision of this description claimed internal/scan TestProvider_Enumerate_FullRepo fails on a clean main. That was wrong and I have retracted it — see the comment below. It was my local git config, not your test.}

Checklist

• My code follows the project's coding style (go fmt, go vet)
• I have performed a self-review of my code
• I have added tests that prove my fix is effective or my feature works
• New and existing unit tests pass locally with my changes
• I have updated the documentation accordingly (if applicable) — no user-facing docs describe the ignore semantics; happy to add a note if you'd like one
• I have signed the CLA — will sign as soon as the bot posts the link

Related Issues

Closes #650

[CLAassistant]

All committers have signed the CLA.

[cru-Luis-Rodriguez]

Retraction: my original description claimed internal/scan TestProvider_Enumerate_FullRepo fails on a clean checkout of main. That was wrong — your test is fine, and I've corrected the description. Sorry for the noise.

The failure was my own git config. I had a stale core.excludesFile pointing at a global ignore file containing the line .gitignore, so git was ignoring every .gitignore file on my machine. In the test fixture that meant git add -A never staged the .gitignore that writeFile had just created, so git ls-files didn't return it and Enumerate never saw it:

$ git check-ignore -v .gitignore
/Users/.../.gitignore_global:3:.gitignore	.gitignore

With that neutralised the test passes, and so does the rest of the suite:

$ GIT_CONFIG_GLOBAL=/dev/null GIT_CONFIG_SYSTEM=/dev/null go test ./internal/scan/
ok  	github.com/alibaba/open-code-review/internal/scan	0.575s

I'd verified the failure reproduced with my changes stashed and concluded "pre-existing on main". That was the wrong conclusion from a correct observation — both runs shared the same broken machine config, so stashing only ruled out my diff as the cause, not my environment. Should have run git check-ignore before saying anything.

One small thing that may still be worth having, entirely your call: initTestRepo inherits the ambient git config, so this test fails for any contributor whose global excludes happen to cover .gitignore — an old .gitignore_global from a dotfiles template is a plausible way to end up there. Setting core.excludesFile to /dev/null (or os.DevNull) alongside the existing user.email / commit.gpgsign config in initTestRepo would pin the fixture to the behaviour the test intends. Happy to send that as a separate one-line PR if you want it; not filing an issue for it since there's no bug in the product.

Nothing here affects the fix in this PR — internal/diff is unaffected either way.

[cru-Luis-Rodriguez]

Closing this for now — I put it up ahead of an internal review on our side, so it's premature. The fix stays on our fork while we evaluate it. Apologies for the noise; happy to reopen if we bring it back.

[cru-Luis-Rodriguez]

Reopened — the close was a miscommunication on our side, not a withdrawal. This fix is still wanted and unchanged; disregard the closing note above. Sorry for the churn in your timeline.

[github-actions]

🔍 OpenCodeReview found 1 issue(s) in this PR.

• ✅ Successfully posted inline: 1 comment(s)

[github-actions]

[bug · medium]
The suffix matching does not enforce a path-component boundary, which can cause false positives. For example, with pattern src/main.go, a file at path othersrc/main.go would incorrectly match because strings.HasSuffix("othersrc/main.go", "src/main.go") is true, even though git would not consider this a match.

The suffix check should verify that the match starts at a / boundary to avoid matching partial directory names.

Suggestion:

Suggested change

	if !anchored && strings.HasSuffix(relPath, body) {
	return true
	}
	if !anchored && strings.HasSuffix(relPath, "/"+body) {
	return true
	}

[cru-Luis-Rodriguez]

Good catch — fixed in 3c6b881, as you recommended.

Worth noting this is pre-existing rather than something this PR introduced: strings.HasSuffix(relPath, pat) is unchanged on main, and reproduces the same false positive there. The line appears in this diff only because of the anchored-pattern guard I added next to it. Since the fix is a one-liner on a line already in the diff, I folded it in rather than deferring — happy to split it into its own PR if you would rather keep this one scoped to negation handling.

Confirmed the behaviour before and after:

	before	after
othersrc/main.go vs src/main.go	true	false
src/main.go vs src/main.go	true	true
src/generated/api.go vs generated/api.go	true	true

The last row is the existing path suffix match case, so the intentionally loose suffix behaviour is preserved — only the partial-component match goes away. Two regression cases added to TestMatchGitignorePattern.