address comments

Author: juanvallejo

tools/depcheck/examples/origin_filters_extended.json

@@ -249,11 +249,18 @@
 	"github.com/openshift/origin/vendor/k8s.io/sample-controller",
 	"github.com/openshift/origin/vendor/k8s.io/utils",
 
+	"github.com/openshift/origin/vendor/gopkg.in/asn1-ber.v1",
+	"github.com/openshift/origin/vendor/gopkg.in/gcfg.v1",
+	"github.com/openshift/origin/vendor/gopkg.in/inf.v0",
+	"github.com/openshift/origin/vendor/gopkg.in/ldap.v2",
+	"github.com/openshift/origin/vendor/gopkg.in/natefinch",
+	"github.com/openshift/origin/vendor/gopkg.in/warnings.v0",
+	"github.com/openshift/origin/vendor/gopkg.in/yaml.v2",
+	
 	"github.com/openshift/origin/vendor/bitbucket.org",
 	"github.com/openshift/origin/vendor/cloud.google.com",
 	"github.com/openshift/origin/vendor/go4.org",
 	"github.com/openshift/origin/vendor/golang.org",
 	"github.com/openshift/origin/vendor/google.golang.org",
-	"github.com/openshift/origin/vendor/gopkg.in",
 	"github.com/openshift/origin/vendor/vbom.ml"
-]
+]

tools/depcheck/pkg/cmd/trace/filter.go

@@ -15,7 +15,7 @@ import (
 // packagePrefix subpackages are re-written to originate from the packagePrefix, and edges
 // terminating at packagePrefix subpackages are re-written to terminate at the packagePrefix.
 func FilterPackages(g *depgraph.MutableDirectedGraph, packagePrefixes []string) (*depgraph.MutableDirectedGraph, error) {
-	collapsedGraph := depgraph.NewMutableDirectedGraph(concrete.NewDirectedGraph())
+	collapsedGraph := depgraph.NewMutableDirectedGraph()
 
 	// copy all nodes to new graph
 	for _, n := range g.Nodes() {

tools/depcheck/pkg/cmd/trace/pkg.go

@@ -45,7 +45,7 @@ func (p *PackageList) Add(pkg Package) {
 // Any packages whose import path is contained within a list of "excludes" are not added to the graph.
 // Returns a directed graph and a map of package import paths to node ids, or an error.
 func BuildGraph(packages *PackageList, excludes []string) (*depgraph.MutableDirectedGraph, error) {
-	g := depgraph.NewMutableDirectedGraph(concrete.NewDirectedGraph())
+	g := depgraph.NewMutableDirectedGraph()
 
 	// contains the subset of packages from the set of given packages (and their immediate dependencies)
 	// that will actually be included in our graph - any packages in the excludes slice, or that do not
@@ -111,55 +111,6 @@ func BuildGraph(packages *PackageList, excludes []string) (*depgraph.MutableDire
 	return g, nil
 }
 
-func copyGraph(g *depgraph.MutableDirectedGraph) (*depgraph.MutableDirectedGraph, error) {
-	if g == nil {
-		return nil, fmt.Errorf("cannot copy nil graph")
-	}
-
-	newGraph := depgraph.NewMutableDirectedGraph(concrete.NewDirectedGraph())
-
-	// copy nodes
-	for _, n := range g.Nodes() {
-		newNode, ok := n.(*depgraph.Node)
-		if !ok {
-			continue
-		}
-
-		err := newGraph.AddNode(newNode)
-		if err != nil {
-			return nil, err
-		}
-	}
-
-	// copy edges
-	for _, n := range g.Nodes() {
-		node, ok := n.(*depgraph.Node)
-		if !ok {
-			return nil, fmt.Errorf("expected node to be of type *depgraph.Node")
-		}
-
-		// ensure node exists in new graph
-		if _, exists := newGraph.NodeByName(node.UniqueName); !exists {
-			return nil, fmt.Errorf("expected node %q to exist in new graph", node.UniqueName)
-		}
-
-		from := g.From(n)
-		for _, to := range from {
-			if newGraph.HasEdgeFromTo(n, to) {
-				continue
-			}
-
-			newGraph.SetEdge(concrete.Edge{
-				F: n,
-				T: to,
-			}, 0)
-		}
-	}
-
-
-	return newGraph, nil
-}
-
 func isExcludedPath(path string, excludes []string) bool {
 	for _, exclude := range excludes {
 		if strings.HasPrefix(path, exclude) {

tools/depcheck/pkg/cmd/trace/trace.go

@@ -214,6 +214,10 @@ func (o *TraceImportsOpts) Run() error {
 		}
 	}
 
+	// prune orphans after filtering in order to maximize chance of deleting disconnected
+	// trees from the graph that have been collapsed into their root node by a given filter.
+	g.PruneOrphans()
+
 	if len(o.OutputFormat) > 0 {
 		return o.outputGraph(g)
 	}
@@ -256,173 +260,40 @@ func (o *TraceImportsOpts) analyzeGraph(g *depgraph.MutableDirectedGraph) error
 	fmt.Printf("Analyzing a total of %v packages\n", len(g.Nodes()))
 	fmt.Println()
 
-	// determine existing orphans and roots
-	keep := []graph.Node{}
-	roots := []graph.Node{}
-	for _, n := range g.Nodes() {
-		node, ok := n.(*depgraph.Node)
-		if !ok {
-			continue
-		}
-
-		to := g.To(n)
-		from := g.From(n)
-
-		// found an existing orphaned node to exclude
-		if len(to) == 0 && len(from) == 0 {
-			continue
-		}
-
-		// found a root
-		if len(from) > 0 && len(to) == 0 {
-			roots = append(roots, n)
-			continue
-		}
-
-		// skip package we are analyzing
-		if node.UniqueName == o.Analyze {
-			continue
-		}
-
-		keep = append(keep, n)
-	}
-
-	yoursGraph, err := copyGraph(g)
-	if err != nil {
-		return err
-	}
-	yours, err := printYours(yoursGraph, roots, keep, you)
-	if err != nil {
-		return err
-	}
-	fmt.Println()
-
-	// no need to copy the graph a second time - we can mutate it
-	// since it won't be used anymore
-	mine, err := printMine(g, roots, keep, you)
-	if err != nil {
-		return err
-	}
-
-	ours := []graph.Node{}
-	yoursOrMine := append(yours, mine...)
-	for _, n := range keep {
-		isYoursOrMine := false
-		for _, yom := range yoursOrMine {
-			if n.ID() == yom.ID() {
-				isYoursOrMine = true
-				break
-			}
-		}
-		if !isYoursOrMine {
-			ours = append(ours, n)
-		}
-	}
-	fmt.Println()
-
-	fmt.Printf("\"Ours\": %v packages shared between the main repo and %q\n", len(ours), you.UniqueName)
-	for _, n := range ours {
-		pkg, ok := n.(*depgraph.Node)
-		if !ok {
-			continue
-		}
-
-		fmt.Printf("    - %s\n", pkg.UniqueName)
-	}
-
-	return nil
-}
-
-func printYours(g *depgraph.MutableDirectedGraph, roots, nodes []graph.Node, you *depgraph.Node) ([]graph.Node, error) {
-	g.RemoveNode(you)
-
-	// find nodes not reachable from our given roots
-	yours := findOrphans(g, roots, nodes)
-
+	yours := calculateOrphans(g, []*depgraph.Node{you})
 	fmt.Printf("\"Yours\": %v packages exclusive to %q\n", len(yours), you.UniqueName)
 	for _, n := range yours {
-		pkg, ok := n.(*depgraph.Node)
-		if !ok {
-			continue
-		}
-
-		fmt.Printf("    - %s\n", pkg.UniqueName)
+		fmt.Printf("    - %s\n", n.UniqueName)
 	}
 
-	return yours, nil
+	return nil
 }
 
-func printMine(g *depgraph.MutableDirectedGraph, roots, nodes []graph.Node, you *depgraph.Node) ([]graph.Node, error) {
-	for _, r := range roots {
-		g.RemoveNode(r)
-	}
-
-	// find nodes not reachable from "you"
-	mine := findOrphans(g, []graph.Node{you}, nodes)
-	fmt.Printf("\"Mine\": %v packages exclusive to the main repo\n", len(mine))
-	for _, n := range mine {
-		pkg, ok := n.(*depgraph.Node)
-		if !ok {
-			continue
-		}
+// calculateOrphans receives a graph and a set of targetNodes. The graph is cloned, and all targetNodes
+// are removed from it. Any nodes left orphaned are added to a "removed" set.
+// This operation is done recursively until we no longer cause any nodes to become orphaned after pruning the removed set.
+// The resulting graph is returned along with the removed set.
+func calculateOrphans(g *depgraph.MutableDirectedGraph, targetNodes []*depgraph.Node) []*depgraph.Node {
+	newGraph := g.Copy()
+	orphans := []*depgraph.Node{}
 
-		fmt.Printf("    - %s\n", pkg.UniqueName)
+	for _, target := range targetNodes {
+		newGraph.RemoveNode(target)
 	}
 
-	return mine, nil
-}
-
-// findOrphans receives a set of root nodes and a set of child nodes and
-// returns the set of child nodes not reachable from any root node.
-func findOrphans(g graph.Directed, roots []graph.Node, nodes []graph.Node) []graph.Node {
-	orphans := []graph.Node{}
-
-	for _, n := range nodes {
-		isOrphan := true
-		for _, root := range roots {
-			if hasPathFromTo(g, root, n) {
-				isOrphan = false
-				break
+	for _, n := range newGraph.Nodes() {
+		if len(newGraph.To(n)) == 0 {
+			if _, ok := n.(*depgraph.Node); !ok {
+				continue
 			}
-		}
 
-		if !isOrphan {
-			continue
+			orphans = append(orphans, n.(*depgraph.Node))
 		}
-
-		orphans = append(orphans, n)
 	}
 
-	return orphans
-}
-
-// hasPathFromTo receives a node A and determines
-// if a node B can be reached from it.
-// Returns false if the source and destination are the same.
-func hasPathFromTo(g graph.Directed, A graph.Node, B graph.Node) bool {
-	if A.ID() == B.ID() {
-		return false
-	}
-	if !g.Has(A) || !g.Has(B) {
-		return false
-	}
-
-	seen := map[int]bool{}
-	unseen := []graph.Node{B}
-	for len(unseen) > 0 {
-		n := unseen[0]
-		unseen = unseen[1:]
-
-		if _, isSeen := seen[n.ID()]; isSeen {
-			continue
-		}
-		if n.ID() == A.ID() {
-			return true
-		}
-
-		seen[n.ID()] = true
-		unseen = append(unseen, g.To(n)...)
+	if len(orphans) == 0 {
+		return []*depgraph.Node{}
 	}
 
-	return false
+	return append(orphans, calculateOrphans(newGraph, orphans)...)
 }

tools/depcheck/pkg/graph/graph.go

@@ -32,7 +32,7 @@ func (n Node) DOTAttributes() []dot.Attribute {
 	}
 }
 
-func NewMutableDirectedGraph(g *concrete.DirectedGraph) *MutableDirectedGraph {
+func NewMutableDirectedGraph() *MutableDirectedGraph {
 	return &MutableDirectedGraph{
 		DirectedGraph: concrete.NewDirectedGraph(),
 		nodesByName:   make(map[string]graph.Node),
@@ -55,7 +55,64 @@ func (g *MutableDirectedGraph) AddNode(n *Node) error {
 	return nil
 }
 
+func (g *MutableDirectedGraph) RemoveNode(n *Node) {
+	delete(g.nodesByName, n.UniqueName)
+	g.DirectedGraph.RemoveNode(n)
+}
+
 func (g *MutableDirectedGraph) NodeByName(name string) (graph.Node, bool) {
 	n, exists := g.nodesByName[name]
 	return n, exists && g.DirectedGraph.Has(n)
 }
+
+// remove nodes with no inbound edges
+func (g *MutableDirectedGraph) PruneOrphans() {
+	for _, n := range g.Nodes() {
+		if len(g.To(n)) == 0 {
+			g.RemoveNode(n.(*Node))
+		}
+	}
+}
+
+func (g *MutableDirectedGraph) Copy() *MutableDirectedGraph {
+	newGraph := NewMutableDirectedGraph()
+
+	// copy nodes
+	for _, n := range g.Nodes() {
+		newNode, ok := n.(*Node)
+		if !ok {
+			continue
+		}
+
+		// ignore error: the only error that could happen is if a name collision occurs.
+		// since we are simply moving nodes from an existing graph to a new one,
+		// we are guaranteed to already have unique node names from an existing valid graph.
+		newGraph.AddNode(newNode)
+	}
+
+	// copy edges
+	for _, n := range g.Nodes() {
+		node, ok := n.(*Node)
+		if !ok {
+			continue
+		}
+
+		if _, exists := newGraph.NodeByName(node.UniqueName); !exists {
+			continue
+		}
+
+		from := g.From(n)
+		for _, to := range from {
+			if newGraph.HasEdgeFromTo(n, to) {
+				continue
+			}
+
+			newGraph.SetEdge(concrete.Edge{
+				F: n,
+				T: to,
+			}, 0)
+		}
+	}
+
+	return newGraph
+}