From 5be0c0000c77bc2de7c96269d3c62b637f0f8316 Mon Sep 17 00:00:00 2001 From: sam boyer Date: Fri, 13 Oct 2017 04:16:12 -0400 Subject: [PATCH] gps: Fully refactor pkgtree to use IgnoredRuleset --- internal/gps/pkgtree/pkgtree.go | 87 +++++++------ internal/gps/pkgtree/pkgtree_test.go | 175 +++++++++++++++------------ 2 files changed, 149 insertions(+), 113 deletions(-) diff --git a/internal/gps/pkgtree/pkgtree.go b/internal/gps/pkgtree/pkgtree.go index 4d8bb149..3028d2b6 100644 --- a/internal/gps/pkgtree/pkgtree.go +++ b/internal/gps/pkgtree/pkgtree.go @@ -550,14 +550,7 @@ type PackageTree struct { // "A": []string{}, // "A/bar": []string{"B/baz"}, // } -func (t PackageTree) ToReachMap(main, tests, backprop bool, ignore map[string]bool) (ReachMap, map[string]*ProblemImportError) { - if ignore == nil { - ignore = make(map[string]bool) - } - - // Radix tree for ignore prefixes. - xt := CreateIgnorePrefixTree(ignore) - +func (t PackageTree) ToReachMap(main, tests, backprop bool, ignore *IgnoredRuleset) (ReachMap, map[string]*ProblemImportError) { // world's simplest adjacency list workmap := make(map[string]wm) @@ -576,18 +569,10 @@ func (t PackageTree) ToReachMap(main, tests, backprop bool, ignore map[string]bo continue } // Skip ignored packages - if ignore[ip] { + if ignore.IsIgnored(ip) { continue } - if xt != nil { - // Skip ignored packages prefix matches - _, _, ok := xt.LongestPrefix(ip) - if ok { - continue - } - } - // TODO (kris-nova) Disable to get staticcheck passing //imps = imps[:0] @@ -605,7 +590,7 @@ func (t PackageTree) ToReachMap(main, tests, backprop bool, ignore map[string]bo // For each import, decide whether it should be ignored, or if it // belongs in the external or internal imports list. for _, imp := range imps { - if ignore[imp] || imp == "." { + if ignore.IsIgnored(imp) || imp == "." { continue } @@ -1093,28 +1078,23 @@ type IgnoredRuleset struct { // matching prefix will be ignored. IgnoredRulesets are immutable once created. // // Duplicate and redundant (i.e. a literal path that has a prefix of a wildcard -// path) declarations are discarded, resulting in the most efficient -// representation. -func NewIgnoredRuleset(ig map[string]struct{}) IgnoredRuleset { +// path) declarations are discarded. Consequently, it is possible that the +// returned IgnoredRuleset may have a smaller Len() than the input slice. +func NewIgnoredRuleset(ig []string) *IgnoredRuleset { if len(ig) == 0 { - return IgnoredRuleset{} + return &IgnoredRuleset{} } - ir := IgnoredRuleset{ + ir := &IgnoredRuleset{ t: radix.New(), } - // Create a sorted list of all the ignores in order to ensure that wildcard + // Sort the list of all the ignores in order to ensure that wildcard // precedence is recorded correctly in the trie. - sortedIgnores := make([]string, len(ig)) - for k := range ig { - sortedIgnores = append(sortedIgnores, k) - } - sort.Strings(sortedIgnores) - - for _, i := range sortedIgnores { - // Skip global ignore. - if i == wcIgnoreSuffix { + sort.Strings(ig) + for _, i := range ig { + // Skip global ignore and empty string. + if i == wcIgnoreSuffix || i == "" { continue } @@ -1135,9 +1115,7 @@ func NewIgnoredRuleset(ig map[string]struct{}) IgnoredRuleset { } } - // The radix tree implementation is initialized with a single element - // representing the empty string. - if ir.t.Len() == 1 { + if ir.t.Len() == 0 { ir.t = nil } @@ -1146,11 +1124,44 @@ func NewIgnoredRuleset(ig map[string]struct{}) IgnoredRuleset { // IsIgnored indicates whether the provided path should be ignored, according to // the ruleset. -func (ir IgnoredRuleset) IsIgnored(path string) bool { - if ir.t == nil { +func (ir *IgnoredRuleset) IsIgnored(path string) bool { + if path == "" || ir == nil || ir.t == nil { return false } prefix, wildi, has := ir.t.LongestPrefix(path) return has && (wildi.(bool) || path == prefix) } + +// Len indicates the number of rules in the ruleset. +func (ir *IgnoredRuleset) Len() int { + if ir == nil || ir.t == nil { + return 0 + } + + return ir.t.Len() +} + +// ToSlice converts the contents of the IgnoredRuleset to a string slice. +// +// This operation is symmetrically dual to NewIgnoredRuleset. +func (ir *IgnoredRuleset) ToSlice() []string { + irlen := ir.Len() + if irlen == 0 { + return nil + } + + items := make([]string, 0, irlen) + ir.t.Walk(func(s string, v interface{}) bool { + if s != "" { + if v.(bool) { + items = append(items, s+wcIgnoreSuffix) + } else { + items = append(items, s) + } + } + return false + }) + + return items +} diff --git a/internal/gps/pkgtree/pkgtree_test.go b/internal/gps/pkgtree/pkgtree_test.go index 474ba483..0a05b09a 100644 --- a/internal/gps/pkgtree/pkgtree_test.go +++ b/internal/gps/pkgtree/pkgtree_test.go @@ -1524,10 +1524,10 @@ func TestToReachMap(t *testing.T) { var want ReachMap var name string var main, tests bool - var ignore map[string]bool + var ignore []string validate := func() { - got, em := vptree.ToReachMap(main, tests, true, ignore) + got, em := vptree.ToReachMap(main, tests, true, NewIgnoredRuleset(ignore)) if len(em) != 0 { t.Errorf("Should not have any error packages from ToReachMap, got %s", em) } @@ -1687,9 +1687,7 @@ func TestToReachMap(t *testing.T) { // ignoring the "varied" pkg has same effect as disabling main pkgs name = "ignore root" - ignore = map[string]bool{ - b(""): true, - } + ignore = []string{b("")} main = true validate() @@ -1723,9 +1721,7 @@ func TestToReachMap(t *testing.T) { // now, the fun stuff. punch a hole in the middle by cutting out // varied/simple name = "ignore varied/simple" - ignore = map[string]bool{ - b("simple"): true, - } + ignore = []string{b("simple")} except( // root pkg loses on everything in varied/simple/another // FIXME this is a bit odd, but should probably exclude m1p as well, @@ -1738,9 +1734,9 @@ func TestToReachMap(t *testing.T) { // widen the hole by excluding otherpath name = "ignore varied/{otherpath,simple}" - ignore = map[string]bool{ - b("otherpath"): true, - b("simple"): true, + ignore = []string{ + b("otherpath"), + b("simple"), } except( // root pkg loses on everything in varied/simple/another and varied/m1p @@ -1752,7 +1748,7 @@ func TestToReachMap(t *testing.T) { // remove namemismatch, though we're mostly beating a dead horse now name = "ignore varied/{otherpath,simple,namemismatch}" - ignore[b("namemismatch")] = true + ignore = append(ignore, b("namemismatch")) except( // root pkg loses on everything in varied/simple/another and varied/m1p bl("", "simple", "simple/another", "m1p", "otherpath", "namemismatch")+" hash encoding/binary go/parser github.com/golang/dep/internal/gps sort os github.com/Masterminds/semver", @@ -1851,9 +1847,9 @@ func TestFlattenReachMap(t *testing.T) { isStdLibFn: nil, main: true, tests: true, - ignore: map[string]bool{ - "nomatch": true, - }, + ignore: NewIgnoredRuleset([]string{ + "nomatch", + }), }, // should have the same effect as ignoring main { @@ -1862,9 +1858,9 @@ func TestFlattenReachMap(t *testing.T) { isStdLibFn: nil, main: true, tests: true, - ignore: map[string]bool{ - "github.com/example/varied": true, - }, + ignore: NewIgnoredRuleset([]string{ + "github.com/example/varied", + }), }, // now drop a more interesting one // we get github.com/golang/dep/internal/gps from m1p, too, so it should still be there @@ -1874,9 +1870,9 @@ func TestFlattenReachMap(t *testing.T) { isStdLibFn: nil, main: true, tests: true, - ignore: map[string]bool{ - "github.com/example/varied/simple": true, - }, + ignore: NewIgnoredRuleset([]string{ + "github.com/example/varied/simple", + }), }, // now drop two { @@ -1885,10 +1881,10 @@ func TestFlattenReachMap(t *testing.T) { isStdLibFn: nil, main: true, tests: true, - ignore: map[string]bool{ - "github.com/example/varied/simple": true, - "github.com/example/varied/namemismatch": true, - }, + ignore: NewIgnoredRuleset([]string{ + "github.com/example/varied/simple", + "github.com/example/varied/namemismatch", + }), }, // make sure tests and main play nice with ignore { @@ -1897,10 +1893,10 @@ func TestFlattenReachMap(t *testing.T) { isStdLibFn: nil, main: true, tests: false, - ignore: map[string]bool{ - "github.com/example/varied/simple": true, - "github.com/example/varied/namemismatch": true, - }, + ignore: NewIgnoredRuleset([]string{ + "github.com/example/varied/simple", + "github.com/example/varied/namemismatch", + }), }, { name: "ignore simple and namemismatch, and no main", @@ -1908,10 +1904,10 @@ func TestFlattenReachMap(t *testing.T) { isStdLibFn: nil, main: false, tests: true, - ignore: map[string]bool{ - "github.com/example/varied/simple": true, - "github.com/example/varied/namemismatch": true, - }, + ignore: NewIgnoredRuleset([]string{ + "github.com/example/varied/simple", + "github.com/example/varied/namemismatch", + }), }, { name: "ignore simple and namemismatch, and no main or tests", @@ -1919,10 +1915,10 @@ func TestFlattenReachMap(t *testing.T) { isStdLibFn: nil, main: false, tests: false, - ignore: map[string]bool{ - "github.com/example/varied/simple": true, - "github.com/example/varied/namemismatch": true, - }, + ignore: NewIgnoredRuleset([]string{ + "github.com/example/varied/simple", + "github.com/example/varied/namemismatch", + }), }, // ignore two that should knock out gps { @@ -1931,10 +1927,10 @@ func TestFlattenReachMap(t *testing.T) { isStdLibFn: nil, main: true, tests: true, - ignore: map[string]bool{ - "github.com/example/varied/simple": true, - "github.com/example/varied/m1p": true, - }, + ignore: NewIgnoredRuleset([]string{ + "github.com/example/varied/simple", + "github.com/example/varied/m1p", + }), }, // finally, directly ignore some external packages { @@ -1943,11 +1939,11 @@ func TestFlattenReachMap(t *testing.T) { isStdLibFn: nil, main: true, tests: true, - ignore: map[string]bool{ - "github.com/golang/dep/internal/gps": true, - "go/parser": true, - "sort": true, - }, + ignore: NewIgnoredRuleset([]string{ + "github.com/golang/dep/internal/gps", + "go/parser", + "sort", + }), }, } { t.Run(testCase.name, testFlattenReachMap(&vptree, testCase)) @@ -1971,7 +1967,7 @@ func TestFlattenReachMap(t *testing.T) { type flattenReachMapCase struct { expect []string name string - ignore map[string]bool + ignore *IgnoredRuleset main, tests bool isStdLibFn func(string) bool } @@ -2135,7 +2131,7 @@ func TestIgnoredRuleset(t *testing.T) { } cases := []struct { name string - inputs map[string]struct{} + inputs []string wantInTree tfixm wantEmptyTree bool shouldIgnore []string @@ -2143,15 +2139,15 @@ func TestIgnoredRuleset(t *testing.T) { }{ { name: "only skip global ignore", - inputs: map[string]struct{}{wcIgnoreSuffix: struct{}{}}, + inputs: []string{wcIgnoreSuffix}, wantEmptyTree: true, }, { name: "ignores without ignore suffix", - inputs: map[string]struct{}{ - "x/y/z": struct{}{}, - "*a/b/c": struct{}{}, - "gophers": struct{}{}, + inputs: []string{ + "x/y/z", + "*a/b/c", + "gophers", }, wantInTree: tfixm{ {path: "x/y/z", wild: false}, @@ -2171,10 +2167,10 @@ func TestIgnoredRuleset(t *testing.T) { }, { name: "ignores with ignore suffix", - inputs: map[string]struct{}{ - "x/y/z*": struct{}{}, - "a/b/c": struct{}{}, - "gophers": struct{}{}, + inputs: []string{ + "x/y/z*", + "a/b/c", + "gophers", }, wantInTree: tfixm{ {path: "x/y/z", wild: true}, @@ -2194,11 +2190,11 @@ func TestIgnoredRuleset(t *testing.T) { }, { name: "global ignore with other strings", - inputs: map[string]struct{}{ - "*": struct{}{}, - "gophers*": struct{}{}, - "x/y/z*": struct{}{}, - "a/b/c": struct{}{}, + inputs: []string{ + "*", + "gophers*", + "x/y/z*", + "a/b/c", }, wantInTree: tfixm{ {path: "x/y/z", wild: true}, @@ -2220,11 +2216,11 @@ func TestIgnoredRuleset(t *testing.T) { }, { name: "ineffectual ignore with wildcard", - inputs: map[string]struct{}{ - "a/b*": struct{}{}, - "a/b/c*": struct{}{}, - "a/b/x/y": struct{}{}, - "a/c*": struct{}{}, + inputs: []string{ + "a/b*", + "a/b/c*", + "a/b/x/y", + "a/c*", }, wantInTree: tfixm{ {path: "a/c", wild: true}, @@ -2240,9 +2236,9 @@ func TestIgnoredRuleset(t *testing.T) { }, { name: "same path with and without wildcard", - inputs: map[string]struct{}{ - "a/b*": struct{}{}, - "a/b": struct{}{}, + inputs: []string{ + "a/b*", + "a/b", }, wantInTree: tfixm{ {path: "a/b", wild: true}, @@ -2256,15 +2252,40 @@ func TestIgnoredRuleset(t *testing.T) { "a/d", }, }, + { + name: "empty paths", + inputs: []string{ + "", + "a/b*", + }, + wantInTree: tfixm{ + {path: "a/b", wild: true}, + }, + shouldNotIgnore: []string{ + "", + }, + }, + { + name: "single wildcard", + inputs: []string{ + "a/b*", + }, + wantInTree: tfixm{ + {path: "a/b", wild: true}, + }, + shouldIgnore: []string{ + "a/b/c", + }, + }, } for _, c := range cases { - t.Run(c.name, func(t *testing.T) { - igm := NewIgnoredRuleset(c.inputs) + igm := NewIgnoredRuleset(c.inputs) + f := func(t *testing.T) { if c.wantEmptyTree { - if igm != nil && igm.t != nil { - t.Fatalf("wanted empty tree, got non-nil tree") + if igm.Len() != 0 { + t.Fatalf("wanted empty tree, but had %v elements", igm.Len()) } } @@ -2292,6 +2313,10 @@ func TestIgnoredRuleset(t *testing.T) { t.Fatalf("%q should not be ignored, but it was", p) } } - }) + } + t.Run(c.name, f) + + igm = NewIgnoredRuleset(igm.ToSlice()) + t.Run(c.name+"/inandout", f) } }