Merge pull request #189 from vmarkovtsev/master

enry-based vendor filtering

Author: vmarkovtsev

internal/core/pipeline.go

@@ -681,6 +681,15 @@ func (pipeline *Pipeline) Initialize(facts map[string]interface{}) error {
 // There is always a "nil" record with CommonAnalysisResult.
 func (pipeline *Pipeline) Run(commits []*object.Commit) (map[LeafPipelineItem]interface{}, error) {
 	startRunTime := time.Now()
+	cleanReturn := false
+	defer func() {
+		if !cleanReturn {
+			remotes, _ := pipeline.repository.Remotes()
+			if len(remotes) > 0 {
+				log.Printf("Failed to run the pipeline on %s", remotes[0].Config().URLs)
+			}
+		}
+	}()
 	onProgress := pipeline.OnProgress
 	if onProgress == nil {
 		onProgress = func(int, int) {}
@@ -838,6 +847,7 @@ func (pipeline *Pipeline) Run(commits []*object.Commit) (map[LeafPipelineItem]in
 		RunTime:        time.Since(startRunTime),
 		RunTimePerItem: runTimePerItem,
 	}
+	cleanReturn = true
 	return result, nil
 }
 

internal/plumbing/tree_diff.go

@@ -22,7 +22,7 @@ import (
 // TreeDiff is a PipelineItem.
 type TreeDiff struct {
 	core.NoopMerger
-	SkipDirs   []string
+	SkipFiles  []string
 	NameFilter *regexp.Regexp
 	Languages  map[string]bool
 
@@ -58,8 +58,8 @@ const (
 var defaultBlacklistedPrefixes = []string{
 	"vendor/",
 	"vendors/",
-	"node_modules/",
 	"package-lock.json",
+	"Gopkg.lock",
 }
 
 // Name of this PipelineItem. Uniquely identifies the type, used for mapping keys, etc.
@@ -85,11 +85,12 @@ func (treediff *TreeDiff) Requires() []string {
 // ListConfigurationOptions returns the list of changeable public properties of this PipelineItem.
 func (treediff *TreeDiff) ListConfigurationOptions() []core.ConfigurationOption {
 	options := [...]core.ConfigurationOption{{
-		Name:        ConfigTreeDiffEnableBlacklist,
-		Description: "Skip blacklisted directories.",
-		Flag:        "skip-blacklist",
-		Type:        core.BoolConfigurationOption,
-		Default:     false}, {
+		Name: ConfigTreeDiffEnableBlacklist,
+		Description: "Skip blacklisted directories and vendored files (according to " +
+			"src-d/enry.IsVendor).",
+		Flag:    "skip-blacklist",
+		Type:    core.BoolConfigurationOption,
+		Default: false}, {
 
 		Name: ConfigTreeDiffBlacklistedPrefixes,
 		Description: "List of blacklisted path prefixes (e.g. directories or specific files). " +
@@ -120,7 +121,7 @@ func (treediff *TreeDiff) ListConfigurationOptions() []core.ConfigurationOption
 // Configure sets the properties previously published by ListConfigurationOptions().
 func (treediff *TreeDiff) Configure(facts map[string]interface{}) error {
 	if val, exists := facts[ConfigTreeDiffEnableBlacklist].(bool); exists && val {
-		treediff.SkipDirs = facts[ConfigTreeDiffBlacklistedPrefixes].([]string)
+		treediff.SkipFiles = facts[ConfigTreeDiffBlacklistedPrefixes].([]string)
 	}
 	if val, exists := facts[ConfigTreeDiffLanguages].([]string); exists {
 		treediff.Languages = map[string]bool{}
@@ -170,14 +171,14 @@ func (treediff *TreeDiff) Consume(deps map[string]interface{}) (map[string]inter
 	if err != nil {
 		return nil, err
 	}
-	var diff object.Changes
+	var diffs object.Changes
 	if treediff.previousTree != nil {
-		diff, err = object.DiffTree(treediff.previousTree, tree)
+		diffs, err = object.DiffTree(treediff.previousTree, tree)
 		if err != nil {
 			return nil, err
 		}
 	} else {
-		diff = []*object.Change{}
+		diffs = []*object.Change{}
 		err = func() error {
 			fileIter := tree.Files()
 			defer fileIter.Close()
@@ -196,7 +197,7 @@ func (treediff *TreeDiff) Consume(deps map[string]interface{}) (map[string]inter
 				if !pass {
 					continue
 				}
-				diff = append(diff, &object.Change{
+				diffs = append(diffs, &object.Change{
 					To: object.ChangeEntry{Name: file.Name, Tree: tree, TreeEntry: object.TreeEntry{
 						Name: file.Name, Mode: file.Mode, Hash: file.Hash}}})
 			}
@@ -208,12 +209,19 @@ func (treediff *TreeDiff) Consume(deps map[string]interface{}) (map[string]inter
 	}
 	treediff.previousTree = tree
 	treediff.previousCommit = commit.Hash
+	diffs = treediff.filterDiffs(diffs)
+	return map[string]interface{}{DependencyTreeChanges: diffs}, nil
+}
 
+func (treediff *TreeDiff) filterDiffs(diffs object.Changes) object.Changes {
 	// filter without allocation
-	filteredDiff := make([]*object.Change, 0, len(diff))
+	filteredDiffs := make(object.Changes, 0, len(diffs))
 OUTER:
-	for _, change := range diff {
-		for _, dir := range treediff.SkipDirs {
+	for _, change := range diffs {
+		if len(treediff.SkipFiles) > 0 && (enry.IsVendor(change.To.Name) || enry.IsVendor(change.From.Name)) {
+			continue
+		}
+		for _, dir := range treediff.SkipFiles {
 			if strings.HasPrefix(change.To.Name, dir) || strings.HasPrefix(change.From.Name, dir) {
 				continue OUTER
 			}
@@ -223,7 +231,7 @@ OUTER:
 			matchedFrom := treediff.NameFilter.MatchString(change.From.Name)
 
 			if !matchedTo && !matchedFrom {
-				continue OUTER
+				continue
 			}
 		}
 		var changeEntry object.ChangeEntry
@@ -232,15 +240,12 @@ OUTER:
 		} else {
 			changeEntry = change.To
 		}
-		pass, _ := treediff.checkLanguage(changeEntry.Name, changeEntry.TreeEntry.Hash)
-		if !pass {
+		if pass, _ := treediff.checkLanguage(changeEntry.Name, changeEntry.TreeEntry.Hash); !pass {
 			continue
 		}
-		filteredDiff = append(filteredDiff, change)
+		filteredDiffs = append(filteredDiffs, change)
 	}
-
-	diff = filteredDiff
-	return map[string]interface{}{DependencyTreeChanges: diff}, nil
+	return filteredDiffs
 }
 
 // Fork clones this PipelineItem.

internal/plumbing/tree_diff_test.go

@@ -38,16 +38,16 @@ func TestTreeDiffConfigure(t *testing.T) {
 	}
 	assert.Nil(t, td.Configure(facts))
 	assert.Equal(t, td.Languages, map[string]bool{"go": true})
-	assert.Equal(t, td.SkipDirs, []string{"vendor"})
+	assert.Equal(t, td.SkipFiles, []string{"vendor"})
 	assert.Equal(t, td.NameFilter.String(), "_.*")
 	delete(facts, ConfigTreeDiffLanguages)
 	td.Languages = nil
 	assert.Nil(t, td.Configure(facts))
 	assert.Equal(t, td.Languages, map[string]bool{"all": true})
-	td.SkipDirs = []string{"test"}
+	td.SkipFiles = []string{"test"}
 	delete(facts, ConfigTreeDiffEnableBlacklist)
 	assert.Nil(t, td.Configure(facts))
-	assert.Equal(t, td.SkipDirs, []string{"test"})
+	assert.Equal(t, td.SkipFiles, []string{"test"})
 }
 
 func TestTreeDiffRegistration(t *testing.T) {
@@ -135,7 +135,7 @@ func TestTreeDiffBadCommit(t *testing.T) {
 }
 
 func TestTreeDiffConsumeSkip(t *testing.T) {
-	// consume without skiping
+	// consume without skipping
 	td := fixtureTreeDiff()
 	assert.Contains(t, td.Languages, allLanguages)
 	commit, _ := test.Repository.CommitObject(plumbing.NewHash(
@@ -166,7 +166,7 @@ func TestTreeDiffConsumeSkip(t *testing.T) {
 }
 
 func TestTreeDiffConsumeOnlyFilesThatMatchFilter(t *testing.T) {
-	// consume without skiping
+	// consume without skipping
 	td := fixtureTreeDiff()
 	assert.Contains(t, td.Languages, allLanguages)
 	commit, _ := test.Repository.CommitObject(plumbing.NewHash(
@@ -238,12 +238,12 @@ func TestTreeDiffConsumeLanguageFilter(t *testing.T) {
 
 func TestTreeDiffFork(t *testing.T) {
 	td1 := fixtureTreeDiff()
-	td1.SkipDirs = append(td1.SkipDirs, "skip")
+	td1.SkipFiles = append(td1.SkipFiles, "skip")
 	clones := td1.Fork(1)
 	assert.Len(t, clones, 1)
 	td2 := clones[0].(*TreeDiff)
 	assert.False(t, td1 == td2)
-	assert.Equal(t, td1.SkipDirs, td2.SkipDirs)
+	assert.Equal(t, td1.SkipFiles, td2.SkipFiles)
 	assert.Equal(t, td1.previousTree, td2.previousTree)
 	td1.Merge([]core.PipelineItem{td2})
 }
@@ -256,3 +256,23 @@ func TestTreeDiffCheckLanguage(t *testing.T) {
 	assert.Nil(t, err)
 	assert.True(t, lang)
 }
+
+func TestTreeDiffConsumeEnryFilter(t *testing.T) {
+	diffs := object.Changes{&object.Change{
+		From: object.ChangeEntry{Name: ""},
+		To:   object.ChangeEntry{Name: "vendor/test.go"},
+	}, &object.Change{
+		From: object.ChangeEntry{Name: "vendor/test.go"},
+		To:   object.ChangeEntry{Name: ""},
+	}}
+	td := fixtureTreeDiff()
+
+	newDiffs := td.filterDiffs(diffs)
+	assert.Len(t, newDiffs, 2)
+	td.Configure(map[string]interface{}{
+		ConfigTreeDiffEnableBlacklist:     true,
+		ConfigTreeDiffBlacklistedPrefixes: []string{"whatever"},
+	})
+	newDiffs = td.filterDiffs(diffs)
+	assert.Len(t, newDiffs, 0)
+}

leaves/burndown.go

@@ -1132,7 +1132,6 @@ func (analyser *BurndownAnalysis) handleInsertion(
 	name := change.To.Name
 	file, exists := analyser.files[name]
 	if exists {
-		log.Println("\n", analyser, "error")
 		return fmt.Errorf("file %s already exists", name)
 	}
 	var hash plumbing.Hash