Merge pull request #88 from vmarkovtsev/master

Fix the burndown merges

internal/burndown/file.go

@@ -243,7 +243,7 @@ func (file *File) Merge(day int, others... *File) bool {
 		if other == nil {
 			panic("merging File with nil")
 		}
-		if file.Hash != other.Hash {
+		if file.Hash != other.Hash || other.Hash == plumbing.ZeroHash {
 			dirty = true
 			break
 		}

internal/burndown/file_test.go

@@ -625,8 +625,19 @@ func TestFileMergeNoop(t *testing.T) {
 	file1.Update(4, 20, 10, 0)
 	// 0 0 | 20 4 | 30 1 | 50 0 | 130 -1        [0]: 100, [1]: 20, [4]: 10
 	file2 := file1.Clone(false)
+
 	dirty := file1.Merge(7, file2)
+	assert.True(t, dirty)
+	dirty = file1.Merge(7, file2)
+	assert.True(t, dirty)
+	file1.Hash = plumbing.NewHash("ffffffffffffffffffffffffffffffffffffffff")
+	file2.Hash = plumbing.NewHash("ffffffffffffffffffffffffffffffffffffffff")
+	dirty = file1.Merge(7, file2)
 	assert.False(t, dirty)
+	file2.Hash = plumbing.ZeroHash
+	dirty = file1.Merge(7, file2)
+	assert.True(t, dirty)
+
 	dump1 := file1.Dump()
 	dump2 := file2.Dump()
 	assert.Equal(t, dump1, dump2)
@@ -639,8 +650,10 @@ func TestFileMergeNoop(t *testing.T) {
 	file1.Update(TreeMergeMark, 60, 30, 30)
 	// 0 0 | 20 4 | 30 1 | 50 0 | 60 M | 90 0 | 130 -1
 	// [0]: 70, [1]: 20, [4]: 10
+	file1.Hash = plumbing.NewHash("ffffffffffffffffffffffffffffffffffffffff")
+	file2.Hash = plumbing.NewHash("ffffffffffffffffffffffffffffffffffffffff")
 	dirty = file1.Merge(7, file2)
-	// because the hashes are still the same
+	// because the hashes are the same
 	assert.False(t, dirty)
 }
 

leaves/burndown.go

@@ -66,6 +66,8 @@ type BurndownAnalysis struct {
 	peopleHistories []sparseHistory
 	// files is the mapping <file path> -> *File.
 	files map[string]*burndown.File
+	// mergedFiles is used during merges to record the real file hashes
+	mergedFiles map[*burndown.File]plumbing.Hash
 	// renames is a quick and dirty solution for the "future branch renames" problem.
 	renames map[string]string
 	// matrix is the mutual deletions and self insertions.
@@ -240,6 +242,7 @@ func (analyser *BurndownAnalysis) Initialize(repository *git.Repository) {
 	analyser.fileHistories = map[string]sparseHistory{}
 	analyser.peopleHistories = make([]sparseHistory, analyser.PeopleNumber)
 	analyser.files = map[string]*burndown.File{}
+	analyser.mergedFiles = map[*burndown.File]plumbing.Hash{}
 	analyser.renames = map[string]string{}
 	analyser.matrix = make([]map[int]int64, analyser.PeopleNumber)
 	analyser.day = 0
@@ -305,14 +308,30 @@ func (analyser *BurndownAnalysis) Merge(branches []core.PipelineItem) {
 	for key, file := range analyser.files {
 		others := make([]*burndown.File, 0, len(branches))
 		for _, branch := range branches {
-			file := branch.(*BurndownAnalysis).files[key]
-			if file != nil {
-				// file can be nil if it is considered binary in the other branch
-				others = append(others, file)
+			otherFile := branch.(*BurndownAnalysis).files[key]
+			if otherFile != nil {
+				// otherFile can be nil if it is considered binary in the other branch
+				others = append(others, otherFile)
 			}
 		}
 		// don't worry, we compare the hashes first before heavy-lifting
 		if file.Merge(analyser.day, others...) {
+			hash := analyser.mergedFiles[file]
+			if hash != plumbing.ZeroHash {
+				delete(analyser.mergedFiles, file)
+			}
+			for _, branch := range branches {
+				otherAnalyser := branch.(*BurndownAnalysis)
+				otherHash := otherAnalyser.mergedFiles[otherAnalyser.files[key]]
+				if otherHash != plumbing.ZeroHash {
+					delete(otherAnalyser.mergedFiles, otherAnalyser.files[key])
+					if hash != plumbing.ZeroHash && hash != otherHash {
+						log.Panicf("merged hash mismatch: %s", key)
+					}
+					hash = otherHash
+				}
+			}
+			file.Hash = hash
 			for _, branch := range branches {
 				branch.(*BurndownAnalysis).files[key] = file.Clone(false)
 			}
@@ -976,8 +995,15 @@ func (analyser *BurndownAnalysis) handleInsertion(
 	if exists {
 		return fmt.Errorf("file %s already exists", name)
 	}
-	file, err = analyser.newFile(blob.Hash, name, author, analyser.day, lines)
+	var hash plumbing.Hash
+	if analyser.day != burndown.TreeMergeMark {
+		hash = blob.Hash
+	}
+	file, err = analyser.newFile(hash, name, author, analyser.day, lines)
 	analyser.files[name] = file
+	if analyser.day == burndown.TreeMergeMark {
+		analyser.mergedFiles[file] = blob.Hash
+	}
 	return err
 }
 
@@ -995,7 +1021,7 @@ func (analyser *BurndownAnalysis) handleDeletion(
 	name := change.From.Name
 	file := analyser.files[name]
 	file.Update(analyser.packPersonWithDay(author, analyser.day), 0, 0, lines)
-	file.Hash = plumbing.ZeroHash
+	file.Hash = plumbing.NewHash("ffffffffffffffffffffffffffffffffffffffff")
 	delete(analyser.files, name)
 	delete(analyser.fileHistories, name)
 	analyser.renames[name] = ""
@@ -1011,7 +1037,13 @@ func (analyser *BurndownAnalysis) handleModification(
 		// this indeed may happen
 		return analyser.handleInsertion(change, author, cache)
 	}
-	file.Hash = change.To.TreeEntry.Hash
+	var hash plumbing.Hash
+	if analyser.day != burndown.TreeMergeMark {
+		hash = change.To.TreeEntry.Hash
+	} else {
+		analyser.mergedFiles[file] = change.To.TreeEntry.Hash
+	}
+	file.Hash = hash
 
 	// possible rename
 	if change.To.Name != change.From.Name {

leaves/burndown_test.go

@@ -16,6 +16,7 @@ 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"
 	)
@@ -203,6 +204,16 @@ func TestBurndownConsumeFinalize(t *testing.T) {
 	assert.Len(t, burndown2.peopleHistories, 0)
 	assert.Len(t, burndown2.fileHistories, 0)
 
+	// check merge hashes
+	burndown3 := BurndownAnalysis{}
+	burndown3.Initialize(test.Repository)
+	deps[items.DependencyDay] = file.TreeMergeMark
+	_, err = burndown3.Consume(deps)
+	assert.Nil(t, err)
+	assert.Equal(t, burndown3.files["cmd/hercules/main.go"].Hash, plumbing.ZeroHash)
+	assert.Equal(t, burndown3.files["analyser.go"].Hash, plumbing.ZeroHash)
+	assert.Equal(t, burndown3.files[".travis.yml"].Hash, plumbing.ZeroHash)
+
 	// stage 2
 	// 2b1ed978194a94edeabbca6de7ff3b5771d4d665
 	deps[identity.DependencyAuthor] = 1