Skip to content

Commit 227b7c6

Browse files
andrewandrew
authored
Replace go-git gitignore matcher with custom implementation (#99)
* Replace go-git gitignore matcher with custom implementation go-git's gitignore.Matcher doesn't handle negation patterns correctly. Repos with deny-by-default .gitignore patterns (/* then !.github/) cause DependenciesInWorkingDir and where to skip entire directory trees that should be visible, breaking diff on clean working trees. Add internal/gitignore package with a matcher that correctly implements gitignore semantics: last-match-wins, directory-only trailing slash, leading/middle slash anchoring, ** in all positions, and pattern scoping for nested .gitignore files. Verified against git check-ignore. Remove LoadIgnoreMatcher/IgnoreMatcher from repository.go. Add GetExcludeDirs for configurable directory skipping via git config. Ref #98 * Skip escaped wildcards test on Windows The "escaped wildcards" test case creates files named hello* and hello? on disk, which Windows does not support. Skip this case on Windows since it's a filesystem limitation, not a matcher bug. * Fix phantom diffs for packages with multiple versions in lockfiles npm lockfiles can contain the same package at multiple versions due to dependency hoisting (e.g. [email protected] and [email protected]). Both computeDiff and AnalyzeCommit used single-value maps keyed by package name, so duplicate versions collapsed to whichever was iterated last. When the database and working tree paths iterated in different orders, this produced phantom "modified" entries on clean workspaces. Replace single-value maps with multi-maps in both locations: - computeDiff: group by manifest:name, compare version sets when a package appears at multiple versions - AnalyzeCommit: replace beforeByName/afterByName with multi-maps so PreviousRequirement reflects the actual replaced version Fixes #53 * Support POSIX character classes in gitignore patterns Git's wildmatch supports POSIX character classes like [[:space:]], [[:alpha:]], [[:digit:]] inside bracket expressions. Our parseBracket found the first ] as the closing bracket, splitting [[:space:]] into [[:space:] plus a literal ]. Fix by skipping past [:...:] sequences when scanning for the closing bracket. Test cases adapted from git/t/t3070-wildmatch.sh, including multiple classes in one bracket and mixing ranges with POSIX classes. * Handle backslash escapes inside bracket expressions In wildmatch, \X inside a bracket expression means literal X. The bracket parser was blindly doubling all backslashes for the regex engine, which broke patterns like [\-_] (literal dash and underscore), [\1-\3] (range 1-3), and [[-\]] (range [ to ]). The fix processes escape sequences during both bracket scanning (so \] does not prematurely close the bracket) and content building (so \X resolves to the literal character). * Use github.com/git-pkgs/gitignore v0.1.0 Replace the internal gitignore package with the standalone module. * Bump gitignore to v1.0.0
1 parent 6a7c099 commit 227b7c6

File tree

11 files changed

+494
-100
lines changed

11 files changed

+494
-100
lines changed

‎cmd/diff.go‎

Lines changed: 84 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -158,53 +158,103 @@ func changesToDeps(changes []analyzer.Change) []database.Dependency {
158158
func computeDiff(fromDeps, toDeps []database.Dependency) *DiffResult {
159159
result := &DiffResult{}
160160

161-
// Build maps keyed by manifest:name to detect added, removed, and modified
162-
fromMap := make(map[string]database.Dependency)
161+
// Build multi-maps keyed by manifest:name, since lockfiles can contain
162+
// the same package at multiple versions (e.g. npm dependency hoisting).
163+
type depKey struct {
164+
ManifestPath string
165+
Name string
166+
}
167+
168+
fromMulti := make(map[depKey][]database.Dependency)
163169
for _, d := range fromDeps {
164-
key := d.ManifestPath + ":" + d.Name
165-
fromMap[key] = d
170+
key := depKey{d.ManifestPath, d.Name}
171+
fromMulti[key] = append(fromMulti[key], d)
166172
}
167173

168-
toMap := make(map[string]database.Dependency)
174+
toMulti := make(map[depKey][]database.Dependency)
169175
for _, d := range toDeps {
170-
key := d.ManifestPath + ":" + d.Name
171-
toMap[key] = d
176+
key := depKey{d.ManifestPath, d.Name}
177+
toMulti[key] = append(toMulti[key], d)
172178
}
173179

174180
// Find added and modified
175-
for key, toDep := range toMap {
176-
if fromDep, exists := fromMap[key]; exists {
177-
if fromDep.Requirement != toDep.Requirement {
181+
for key, toList := range toMulti {
182+
fromList, exists := fromMulti[key]
183+
if !exists {
184+
// Entirely new package
185+
for _, d := range toList {
186+
result.Added = append(result.Added, DiffEntry{
187+
Name: d.Name,
188+
Ecosystem: d.Ecosystem,
189+
ManifestPath: d.ManifestPath,
190+
DependencyType: d.DependencyType,
191+
ToRequirement: d.Requirement,
192+
})
193+
}
194+
continue
195+
}
196+
197+
// Single version on each side: compare directly (shows "modified")
198+
if len(fromList) == 1 && len(toList) == 1 {
199+
if fromList[0].Requirement != toList[0].Requirement {
178200
result.Modified = append(result.Modified, DiffEntry{
179-
Name: toDep.Name,
180-
Ecosystem: toDep.Ecosystem,
181-
ManifestPath: toDep.ManifestPath,
182-
DependencyType: toDep.DependencyType,
183-
FromRequirement: fromDep.Requirement,
184-
ToRequirement: toDep.Requirement,
201+
Name: toList[0].Name,
202+
Ecosystem: toList[0].Ecosystem,
203+
ManifestPath: toList[0].ManifestPath,
204+
DependencyType: toList[0].DependencyType,
205+
FromRequirement: fromList[0].Requirement,
206+
ToRequirement: toList[0].Requirement,
207+
})
208+
}
209+
continue
210+
}
211+
212+
// Multiple versions on at least one side: compare version sets
213+
fromVersions := make(map[string]bool, len(fromList))
214+
for _, d := range fromList {
215+
fromVersions[d.Requirement] = true
216+
}
217+
toVersions := make(map[string]bool, len(toList))
218+
for _, d := range toList {
219+
toVersions[d.Requirement] = true
220+
}
221+
222+
for _, d := range toList {
223+
if !fromVersions[d.Requirement] {
224+
result.Added = append(result.Added, DiffEntry{
225+
Name: d.Name,
226+
Ecosystem: d.Ecosystem,
227+
ManifestPath: d.ManifestPath,
228+
DependencyType: d.DependencyType,
229+
ToRequirement: d.Requirement,
230+
})
231+
}
232+
}
233+
for _, d := range fromList {
234+
if !toVersions[d.Requirement] {
235+
result.Removed = append(result.Removed, DiffEntry{
236+
Name: d.Name,
237+
Ecosystem: d.Ecosystem,
238+
ManifestPath: d.ManifestPath,
239+
DependencyType: d.DependencyType,
240+
FromRequirement: d.Requirement,
185241
})
186242
}
187-
} else {
188-
result.Added = append(result.Added, DiffEntry{
189-
Name: toDep.Name,
190-
Ecosystem: toDep.Ecosystem,
191-
ManifestPath: toDep.ManifestPath,
192-
DependencyType: toDep.DependencyType,
193-
ToRequirement: toDep.Requirement,
194-
})
195243
}
196244
}
197245

198-
// Find removed
199-
for key, fromDep := range fromMap {
200-
if _, exists := toMap[key]; !exists {
201-
result.Removed = append(result.Removed, DiffEntry{
202-
Name: fromDep.Name,
203-
Ecosystem: fromDep.Ecosystem,
204-
ManifestPath: fromDep.ManifestPath,
205-
DependencyType: fromDep.DependencyType,
206-
FromRequirement: fromDep.Requirement,
207-
})
246+
// Find removed (packages not in toMulti at all)
247+
for key, fromList := range fromMulti {
248+
if _, exists := toMulti[key]; !exists {
249+
for _, d := range fromList {
250+
result.Removed = append(result.Removed, DiffEntry{
251+
Name: d.Name,
252+
Ecosystem: d.Ecosystem,
253+
ManifestPath: d.ManifestPath,
254+
DependencyType: d.DependencyType,
255+
FromRequirement: d.Requirement,
256+
})
257+
}
208258
}
209259
}
210260

‎cmd/diff_test.go‎

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -428,6 +428,47 @@ func TestComputeDiff_SamePackageDifferentManifests(t *testing.T) {
428428
}
429429
}
430430

431+
func TestComputeDiff_DuplicatePackageVersionsInLockfile(t *testing.T) {
432+
// npm lockfiles can contain the same package at multiple versions due to
433+
// dependency hoisting. For example, package-lock.json might have:
434+
// node_modules/ini (version 6.0.0)
435+
// node_modules/some-dep/node_modules/ini (version 4.1.1)
436+
// The manifests parser returns both as separate dependencies with the same
437+
// name and manifest path but different versions.
438+
//
439+
// When from and to have identical deps, computeDiff should report no changes,
440+
// regardless of iteration order.
441+
fromDeps := []database.Dependency{
442+
{Name: "ini", Ecosystem: "npm", Requirement: "6.0.0", ManifestPath: "package-lock.json", DependencyType: "development"},
443+
{Name: "ini", Ecosystem: "npm", Requirement: "4.1.1", ManifestPath: "package-lock.json", DependencyType: "development"},
444+
{Name: "minipass", Ecosystem: "npm", Requirement: "7.1.2", ManifestPath: "package-lock.json", DependencyType: "development"},
445+
{Name: "minipass", Ecosystem: "npm", Requirement: "3.3.6", ManifestPath: "package-lock.json", DependencyType: "development"},
446+
{Name: "debug", Ecosystem: "npm", Requirement: "4.4.3", ManifestPath: "package-lock.json", DependencyType: "development"},
447+
{Name: "debug", Ecosystem: "npm", Requirement: "2.6.9", ManifestPath: "package-lock.json", DependencyType: "development"},
448+
}
449+
// Same deps, different order (simulating different iteration paths)
450+
toDeps := []database.Dependency{
451+
{Name: "ini", Ecosystem: "npm", Requirement: "4.1.1", ManifestPath: "package-lock.json", DependencyType: "development"},
452+
{Name: "ini", Ecosystem: "npm", Requirement: "6.0.0", ManifestPath: "package-lock.json", DependencyType: "development"},
453+
{Name: "minipass", Ecosystem: "npm", Requirement: "3.3.6", ManifestPath: "package-lock.json", DependencyType: "development"},
454+
{Name: "minipass", Ecosystem: "npm", Requirement: "7.1.2", ManifestPath: "package-lock.json", DependencyType: "development"},
455+
{Name: "debug", Ecosystem: "npm", Requirement: "2.6.9", ManifestPath: "package-lock.json", DependencyType: "development"},
456+
{Name: "debug", Ecosystem: "npm", Requirement: "4.4.3", ManifestPath: "package-lock.json", DependencyType: "development"},
457+
}
458+
459+
result := computeDiff(fromDeps, toDeps)
460+
461+
if len(result.Added) != 0 {
462+
t.Errorf("expected 0 added, got %d: %+v", len(result.Added), result.Added)
463+
}
464+
if len(result.Modified) != 0 {
465+
t.Errorf("expected 0 modified, got %d: %+v", len(result.Modified), result.Modified)
466+
}
467+
if len(result.Removed) != 0 {
468+
t.Errorf("expected 0 removed, got %d: %+v", len(result.Removed), result.Removed)
469+
}
470+
}
471+
431472
func containsString(s, substr string) bool {
432473
for i := 0; i <= len(s)-len(substr); i++ {
433474
if s[i:i+len(substr)] == substr {

‎cmd/where.go‎

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -9,8 +9,9 @@ import (
99
"regexp"
1010
"strings"
1111

12-
"github.com/git-pkgs/manifests"
1312
"github.com/git-pkgs/git-pkgs/internal/git"
13+
"github.com/git-pkgs/gitignore"
14+
"github.com/git-pkgs/manifests"
1415
"github.com/spf13/cobra"
1516
)
1617

@@ -53,11 +54,7 @@ func runWhere(cmd *cobra.Command, args []string) error {
5354

5455
workDir := repo.WorkDir()
5556

56-
ignoreMatcher, err := repo.LoadIgnoreMatcher()
57-
if err != nil {
58-
// Continue without gitignore support if loading fails
59-
ignoreMatcher = nil
60-
}
57+
matcher := gitignore.New(workDir)
6158

6259
// Load submodule paths only if we need to skip them
6360
var submoduleMap map[string]bool
@@ -92,18 +89,25 @@ func runWhere(cmd *cobra.Command, args []string) error {
9289
return filepath.SkipDir
9390
}
9491
// Skip directories that match gitignore patterns
95-
if ignoreMatcher != nil && ignoreMatcher.IsIgnored(relPath, true) {
92+
if relPath != "." && matcher.Match(relPath+"/") {
9693
return filepath.SkipDir
9794
}
9895
// Skip git submodule directories
9996
if submoduleMap[relPath] {
10097
return filepath.SkipDir
10198
}
99+
// Pick up nested .gitignore files
100+
if relPath != "." {
101+
nestedIgnore := filepath.Join(path, ".gitignore")
102+
if _, err := os.Stat(nestedIgnore); err == nil {
103+
matcher.AddFromFile(nestedIgnore, relPath)
104+
}
105+
}
102106
return nil
103107
}
104108

105109
// Skip files that match gitignore patterns
106-
if ignoreMatcher != nil && ignoreMatcher.IsIgnored(relPath, false) {
110+
if matcher.Match(relPath) {
107111
return nil
108112
}
109113

‎go.mod‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ go 1.25.6
44

55
require (
66
github.com/git-pkgs/enrichment v0.1.0
7+
github.com/git-pkgs/gitignore v1.0.0
78
github.com/git-pkgs/managers v0.5.0
89
github.com/git-pkgs/manifests v0.3.4
910
github.com/git-pkgs/purl v0.1.5

‎go.sum‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -36,8 +36,8 @@ github.com/emirpasic/gods v1.18.1 h1:FXtiHYKDGKCW2KzwZKx0iC0PQmdlorYgdFG9jPXJ1Bc
3636
github.com/emirpasic/gods v1.18.1/go.mod h1:8tpGGwCnJ5H4r6BWwaV6OrWmMoPhUl5jm/FMNAnJvWQ=
3737
github.com/git-pkgs/enrichment v0.1.0 h1:t7Zya45Dkjh8a1ULQ4lEY8UqvKHnM63nrhdIm79smDg=
3838
github.com/git-pkgs/enrichment v0.1.0/go.mod h1:xdy8+jZ5TCbYwG/LOPD1QBkjzpRlE5Wuv4//guXrGjw=
39-
github.com/git-pkgs/managers v0.4.0 h1:ydsy8Hq4UpHYrrr33swkfJiR+0HFsFOdlOktsrsaAbY=
40-
github.com/git-pkgs/managers v0.4.0/go.mod h1:K6B5Z8vd6tfywKzi2JgOB1eDFevmT4F9wxk4Q3VCWXQ=
39+
github.com/git-pkgs/gitignore v1.0.0 h1:bLy6FTho5Sbegh2bNnzxXjA09qKxJ+tNKfb+tnM/7CY=
40+
github.com/git-pkgs/gitignore v1.0.0/go.mod h1:Lr0XwhbvP071rZF/zIIhkY1gEhFDoWHH91lngwLpeUg=
4141
github.com/git-pkgs/managers v0.5.0 h1:hHQoZvLfj2tbXNciv5ltI3zue5cPmMTw3ZxKx80sxiY=
4242
github.com/git-pkgs/managers v0.5.0/go.mod h1:8DR7tIQEEyPyJ7QGzStVbovnGl7tAJcrhQLzjQPzFzc=
4343
github.com/git-pkgs/manifests v0.3.4 h1:274YXdkREKjo36KCWKlIpUKtsw5I2AgjDxu7kiaBAPI=

0 commit comments

Comments
 (0)