Fix spurious merges

Signed-off-by: Vadim Markovtsev <vadim@sourced.tech>

core.go

@@ -95,11 +95,6 @@ func ForkCopyPipelineItem(origin PipelineItem, n int) []PipelineItem {
 	return core.ForkCopyPipelineItem(origin ,n)
 }
 
-// IsMergeCommit indicates whether the commit is a merge or not.
-func IsMergeCommit(deps map[string]interface{}) bool {
-	return core.IsMergeCommit(deps)
-}
-
 // PipelineItemRegistry contains all the known PipelineItem-s.
 type PipelineItemRegistry = core.PipelineItemRegistry
 
@@ -107,6 +102,16 @@ type PipelineItemRegistry = core.PipelineItemRegistry
 var Registry = core.Registry
 
 const (
+	// DependencyCommit is the name of one of the three items in `deps` supplied to PipelineItem.Consume()
+	// which always exists. It corresponds to the currently analyzed commit.
+	DependencyCommit = core.DependencyCommit
+	// DependencyIndex is the name of one of the three items in `deps` supplied to PipelineItem.Consume()
+	// which always exists. It corresponds to the currently analyzed commit's index.
+	DependencyIndex = core.DependencyIndex
+	// DependencyIsMerge is the name of one of the three items in `deps` supplied to PipelineItem.Consume()
+	// which always exists. It indicates whether the analyzed commit is a merge commit.
+	// Checking the number of parents is not correct - we remove the back edges during the DAG simplification.
+	DependencyIsMerge = core.DependencyIsMerge
 	// DependencyAuthor is the name of the dependency provided by identity.Detector.
 	DependencyAuthor = identity.DependencyAuthor
 	// DependencyBlobCache identifies the dependency provided by BlobCache.

internal/burndown/file.go

@@ -36,13 +36,16 @@ const TreeMaxBinPower = 14
 const TreeMergeMark = (1 << TreeMaxBinPower) - 1
 
 func (file *File) updateTime(currentTime, previousTime, delta int) {
+	if previousTime & TreeMergeMark == TreeMergeMark {
+		if currentTime == previousTime {
+			return
+		}
+		panic("previousTime cannot be TreeMergeMark")
+	}
 	if currentTime & TreeMergeMark == TreeMergeMark {
-		// merge mode
+		// merge mode - `delta` is negative and we have already applied it in a branch
 		return
 	}
-	if previousTime & TreeMergeMark == TreeMergeMark {
-		previousTime = currentTime
-	}
 	for _, update := range file.updaters {
 		update(currentTime, previousTime, delta)
 	}
@@ -304,16 +307,19 @@ func (file *File) Dump() string {
 // 3. Node keys must monotonically increase and never duplicate.
 func (file *File) Validate() {
 	if file.tree.Min().Item().Key != 0 {
-		panic("the tree must start with key 0")
+		log.Panic("the tree must start with key 0")
 	}
 	if file.tree.Max().Item().Value != TreeEnd {
-		panic(fmt.Sprintf("the last value in the tree must be %d", TreeEnd))
+		log.Panicf("the last value in the tree must be %d", TreeEnd)
 	}
 	prevKey := -1
 	for iter := file.tree.Min(); !iter.Limit(); iter = iter.Next() {
 		node := iter.Item()
 		if node.Key == prevKey {
-			panic(fmt.Sprintf("duplicate tree key: %d", node.Key))
+			log.Panicf("duplicate tree key: %d", node.Key)
+		}
+		if node.Value == TreeMergeMark {
+			log.Panicf("unmerged lines left: %d", node.Key)
 		}
 		prevKey = node.Key
 	}

internal/core/forks.go

@@ -34,11 +34,6 @@ func (proc *OneShotMergeProcessor) ShouldConsumeCommit(deps map[string]interface
 	return false
 }
 
-// IsMergeCommit indicates whether the commit is a merge or not.
-func IsMergeCommit(deps map[string]interface{}) bool {
-	return deps[DependencyCommit].(*object.Commit).NumParents() > 1
-}
-
 // NoopMerger provides an empty Merge() method suitable for PipelineItem.
 type NoopMerger struct {
 }

internal/core/pipeline.go

@@ -52,7 +52,8 @@ func (opt ConfigurationOptionType) String() string {
 	case StringsConfigurationOption:
 		return "string"
 	}
-	panic(fmt.Sprintf("Invalid ConfigurationOptionType value %d", opt))
+	log.Panicf("Invalid ConfigurationOptionType value %d", opt)
+	return ""
 }
 
 // ConfigurationOption allows for the unified, retrospective way to setup PipelineItem-s.
@@ -237,12 +238,16 @@ const (
 	// ConfigPipelineCommits is the name of the Pipeline configuration option (Pipeline.Initialize())
 	// which allows to specify the custom commit sequence. By default, Pipeline.Commits() is used.
 	ConfigPipelineCommits = "commits"
-	// DependencyCommit is the name of one of the two items in `deps` supplied to PipelineItem.Consume()
+	// DependencyCommit is the name of one of the three items in `deps` supplied to PipelineItem.Consume()
 	// which always exists. It corresponds to the currently analyzed commit.
 	DependencyCommit = "commit"
-	// DependencyIndex is the name of one of the two items in `deps` supplied to PipelineItem.Consume()
+	// DependencyIndex is the name of one of the three items in `deps` supplied to PipelineItem.Consume()
 	// which always exists. It corresponds to the currently analyzed commit's index.
 	DependencyIndex = "index"
+	// DependencyIsMerge is the name of one of the three items in `deps` supplied to PipelineItem.Consume()
+	// which always exists. It indicates whether the analyzed commit is a merge commit.
+	// Checking the number of parents is not correct - we remove the back edges during the DAG simplification.
+	DependencyIsMerge = "is_merge"
 )
 
 // NewPipeline initializes a new instance of Pipeline struct.
@@ -488,13 +493,13 @@ func (pipeline *Pipeline) resolve(dumpPath string) {
 		for _, key := range item.Requires() {
 			key = "[" + key + "]"
 			if graph.AddEdge(key, name) == 0 {
-				panic(fmt.Sprintf("Unsatisfied dependency: %s -> %s", key, item.Name()))
+				log.Panicf("Unsatisfied dependency: %s -> %s", key, item.Name())
 			}
 		}
 	}
 	// Try to break the cycles in some known scenarios.
 	if len(ambiguousMap) > 0 {
-		ambiguous := []string{}
+		var ambiguous []string
 		for key := range ambiguousMap {
 			ambiguous = append(ambiguous, key)
 		}
@@ -613,6 +618,13 @@ func (pipeline *Pipeline) Run(commits []*object.Commit) (map[LeafPipelineItem]in
 			state := map[string]interface{}{
 				DependencyCommit: step.Commit,
 				DependencyIndex: commitIndex,
+				DependencyIsMerge:
+					(index > 0 &&
+					plan[index-1].Action == runActionCommit &&
+					plan[index-1].Commit.Hash == step.Commit.Hash) ||
+					(index < (len(plan)-1) &&
+					plan[index+1].Action == runActionCommit &&
+					plan[index+1].Commit.Hash == step.Commit.Hash),
 			}
 			for _, item := range branches[firstItem] {
 				update, err := item.Consume(state)
@@ -624,7 +636,7 @@ func (pipeline *Pipeline) Run(commits []*object.Commit) (map[LeafPipelineItem]in
 				for _, key := range item.Provides() {
 					val, ok := update[key]
 					if !ok {
-						panic(fmt.Sprintf("%s: Consume() did not return %s", item.Name(), key))
+						log.Panicf("%s: Consume() did not return %s", item.Name(), key)
 					}
 					state[key] = val
 				}

internal/core/pipeline_test.go

@@ -24,6 +24,7 @@ type testPipelineItem struct {
 	Merged        *bool
 	CommitMatches bool
 	IndexMatches  bool
+	MergeState    *int
 	TestError     bool
 }
 
@@ -65,6 +66,8 @@ func (item *testPipelineItem) Features() []string {
 
 func (item *testPipelineItem) Initialize(repository *git.Repository) {
 	item.Initialized = repository != nil
+	item.Merged = new(bool)
+	item.MergeState = new(int)
 }
 
 func (item *testPipelineItem) Consume(deps map[string]interface{}) (map[string]interface{}, error) {
@@ -82,13 +85,20 @@ func (item *testPipelineItem) Consume(deps map[string]interface{}) (map[string]i
 			item.IndexMatches = obj.(int) == 0
 		}
 	}
+	obj, exists = deps[DependencyIsMerge]
+	if exists {
+		*item.MergeState++
+		if obj.(bool) {
+			*item.MergeState++
+		}
+	}
 	return map[string]interface{}{"test": item}, nil
 }
 
 func (item *testPipelineItem) Fork(n int) []PipelineItem {
 	result := make([]PipelineItem, n)
 	for i := 0; i < n; i++ {
-		result[i] = &testPipelineItem{Merged: item.Merged}
+		result[i] = &testPipelineItem{Merged: item.Merged, MergeState: item.MergeState}
 	}
 	item.Forked = true
 	return result
@@ -198,7 +208,7 @@ func TestPipelineFeatures(t *testing.T) {
 
 func TestPipelineRun(t *testing.T) {
 	pipeline := NewPipeline(test.Repository)
-	item := &testPipelineItem{Merged: new(bool)}
+	item := &testPipelineItem{}
 	pipeline.AddItem(item)
 	pipeline.Initialize(map[string]interface{}{})
 	assert.True(t, item.Initialized)
@@ -217,6 +227,7 @@ func TestPipelineRun(t *testing.T) {
 	assert.True(t, item.DepsConsumed)
 	assert.True(t, item.CommitMatches)
 	assert.True(t, item.IndexMatches)
+	assert.Equal(t, 1, *item.MergeState)
 	assert.True(t, item.Forked)
 	assert.False(t, *item.Merged)
 	pipeline.RemoveItem(item)
@@ -227,7 +238,7 @@ func TestPipelineRun(t *testing.T) {
 
 func TestPipelineRunBranches(t *testing.T) {
 	pipeline := NewPipeline(test.Repository)
-	item := &testPipelineItem{Merged: new(bool)}
+	item := &testPipelineItem{}
 	pipeline.AddItem(item)
 	pipeline.Initialize(map[string]interface{}{})
 	assert.True(t, item.Initialized)
@@ -254,6 +265,7 @@ func TestPipelineRunBranches(t *testing.T) {
 	assert.Equal(t, item, result[item].(*testPipelineItem))
 	common := result[nil].(*CommonAnalysisResult)
 	assert.Equal(t, common.CommitsNumber, 5)
+	assert.Equal(t, *item.MergeState, 8)
 }
 
 func TestPipelineOnProgress(t *testing.T) {

leaves/burndown.go

@@ -68,7 +68,7 @@ type BurndownAnalysis struct {
 	files map[string]*burndown.File
 	// mergedFiles is used during merges to record the real file hashes
 	mergedFiles map[string]bool
-	// author of the processed merge commit
+	// mergedAuthor of the processed merge commit
 	mergedAuthor int
 	// renames is a quick and dirty solution for the "future branch renames" problem.
 	renames map[string]string
@@ -260,7 +260,7 @@ func (analyser *BurndownAnalysis) Initialize(repository *git.Repository) {
 func (analyser *BurndownAnalysis) Consume(deps map[string]interface{}) (map[string]interface{}, error) {
 	author := deps[identity.DependencyAuthor].(int)
 	day := deps[items.DependencyDay].(int)
-	if !core.IsMergeCommit(deps) {
+	if !deps[core.DependencyIsMerge].(bool) {
 		analyser.day = day
 		analyser.onNewDay()
 	} else {
@@ -655,12 +655,12 @@ func addBurndownMatrix(matrix DenseHistory, granularity, sampling int, daily [][
 	}
 	neededRows := len(matrix)*sampling + offset
 	if len(daily) < neededRows {
-		panic(fmt.Sprintf("merge bug: too few daily rows: required %d, have %d",
-			neededRows, len(daily)))
+		log.Panicf("merge bug: too few daily rows: required %d, have %d",
+			neededRows, len(daily))
 	}
 	if len(daily[0]) < maxCols {
-		panic(fmt.Sprintf("merge bug: too few daily cols: required %d, have %d",
-			maxCols, len(daily[0])))
+		log.Panicf("merge bug: too few daily cols: required %d, have %d",
+			maxCols, len(daily[0]))
 	}
 	for x := 0; x < maxCols; x++ {
 		for y := 0; y < len(matrix); y++ {

leaves/burndown_test.go

@@ -16,10 +16,9 @@ import (
 	"gopkg.in/src-d/go-git.v4/plumbing/object"
 	"gopkg.in/src-d/hercules.v4/internal/pb"
 	items "gopkg.in/src-d/hercules.v4/internal/plumbing"
-	file "gopkg.in/src-d/hercules.v4/internal/burndown"
 	"gopkg.in/src-d/hercules.v4/internal/plumbing/identity"
 	"gopkg.in/src-d/hercules.v4/internal/test"
-	)
+)
 
 func TestBurndownMeta(t *testing.T) {
 	burndown := BurndownAnalysis{}
@@ -181,6 +180,7 @@ func TestBurndownConsumeFinalize(t *testing.T) {
 	deps[items.DependencyFileDiff] = result[items.DependencyFileDiff]
 	deps[core.DependencyCommit], _ = test.Repository.CommitObject(plumbing.NewHash(
 		"cce947b98a050c6d356bc6ba95030254914027b1"))
+	deps[core.DependencyIsMerge] = false
 	result, err = burndown.Consume(deps)
 	assert.Nil(t, result)
 	assert.Nil(t, err)
@@ -207,16 +207,18 @@ func TestBurndownConsumeFinalize(t *testing.T) {
 	// check merge hashes
 	burndown3 := BurndownAnalysis{}
 	burndown3.Initialize(test.Repository)
-	deps[items.DependencyDay] = file.TreeMergeMark
+	deps[identity.DependencyAuthor] = 1
+	deps[core.DependencyIsMerge] = true
 	_, err = burndown3.Consume(deps)
 	assert.Nil(t, err)
+	assert.Equal(t, 1, burndown3.mergedAuthor)
 	assert.True(t, burndown3.mergedFiles["cmd/hercules/main.go"])
 	assert.True(t, burndown3.mergedFiles["analyser.go"], plumbing.ZeroHash)
 	assert.True(t, burndown3.mergedFiles[".travis.yml"], plumbing.ZeroHash)
 
 	// stage 2
 	// 2b1ed978194a94edeabbca6de7ff3b5771d4d665
-	deps[identity.DependencyAuthor] = 1
+	deps[core.DependencyIsMerge] = false
 	deps[items.DependencyDay] = 30
 	cache = map[plumbing.Hash]*object.Blob{}
 	hash = plumbing.NewHash("291286b4ac41952cbd1389fda66420ec03c1a9fe")
@@ -406,6 +408,7 @@ func TestBurndownSerialize(t *testing.T) {
 	deps[items.DependencyTreeChanges] = changes
 	deps[core.DependencyCommit], _ = test.Repository.CommitObject(plumbing.NewHash(
 		"cce947b98a050c6d356bc6ba95030254914027b1"))
+	deps[core.DependencyIsMerge] = false
 	fd := fixtures.FileDiff()
 	result, _ := fd.Consume(deps)
 	deps[items.DependencyFileDiff] = result[items.DependencyFileDiff]

leaves/couples.go

@@ -114,7 +114,7 @@ func (couples *CouplesAnalysis) Initialize(repository *git.Repository) {
 // in Provides(). If there was an error, nil is returned.
 func (couples *CouplesAnalysis) Consume(deps map[string]interface{}) (map[string]interface{}, error) {
 	firstMerge := couples.ShouldConsumeCommit(deps)
-	mergeMode := core.IsMergeCommit(deps)
+	mergeMode := deps[core.DependencyIsMerge].(bool)
 	couples.lastCommit = deps[core.DependencyCommit].(*object.Commit)
 	author := deps[identity.DependencyAuthor].(int)
 	if author == identity.AuthorMissing {

leaves/couples_test.go

@@ -92,6 +92,7 @@ func TestCouplesConsumeFinalize(t *testing.T) {
 	deps[identity.DependencyAuthor] = 0
 	deps[core.DependencyCommit], _ = test.Repository.CommitObject(gitplumbing.NewHash(
 		"a3ee37f91f0d705ec9c41ae88426f0ae44b2fbc3"))
+	deps[core.DependencyIsMerge] = false
 	deps[plumbing.DependencyTreeChanges] = generateChanges("+LICENSE2", "+file2.go", "+rbtree2.go")
 	c.Consume(deps)
 	deps[plumbing.DependencyTreeChanges] = generateChanges("+README.md", "-LICENSE2", "=analyser.go", ">file2.go>file_test.go")