Merge pull request #213 from MichaelMure/hash-collision

bug: make sure there is no Operation's hash collision

6 files changed, 47 insertions, 30 deletions

diff --git a/Gopkg.lock b/Gopkg.lock
index e4ecfcb9..fdaa5493 100644
--- a/Gopkg.lock
+++ b/Gopkg.lock
@@ -498,6 +498,7 @@
     "github.com/go-errors/errors",
     "github.com/gorilla/mux",
     "github.com/icrowley/fake",
+    "github.com/mattn/go-isatty",
     "github.com/phayes/freeport",
     "github.com/pkg/errors",
     "github.com/shurcooL/githubv4",
diff --git a/bug/bug.go b/bug/bug.go
index ca817dc1..ae662ef1 100644
--- a/bug/bug.go
+++ b/bug/bug.go
@@ -330,12 +330,18 @@ func (bug *Bug) Validate() error {
 	}
 
 	// Check that there is no more CreateOp op
+	// Check that there is no colliding operation's ID
 	it := NewOperationIterator(bug)
 	createCount := 0
+	ids := make(map[entity.Id]struct{})
 	for it.Next() {
 		if it.Value().base().OperationType == CreateOp {
 			createCount++
 		}
+		if _, ok := ids[it.Value().Id()]; ok {
+			return fmt.Errorf("id collision: %s", it.Value().Id())
+		}
+		ids[it.Value().Id()] = struct{}{}
 	}
 
 	if createCount != 1 {
diff --git a/bug/bug_test.go b/bug/bug_test.go
index 35e8a395..480d312e 100644
--- a/bug/bug_test.go
+++ b/bug/bug_test.go
@@ -73,8 +73,6 @@ func TestBugCommitLoad(t *testing.T) {
 
 	bug1.Append(createOp)
 	bug1.Append(setTitleOp)
-	bug1.Append(setTitleOp)
-	bug1.Append(addCommentOp)
 
 	repo := repository.NewMockRepoForTest()
 
@@ -90,7 +88,6 @@ func TestBugCommitLoad(t *testing.T) {
 
 	// add more op
 
-	bug1.Append(setTitleOp)
 	bug1.Append(addCommentOp)
 
 	assert.True(t, bug1.NeedCommit())
diff --git a/bug/op_set_metadata.go b/bug/op_set_metadata.go
index f99f836b..67f7e009 100644
--- a/bug/op_set_metadata.go
+++ b/bug/op_set_metadata.go
@@ -34,6 +34,8 @@ func (op *SetMetadataOperation) Apply(snapshot *Snapshot) {
 				base.extraMetadata = make(map[string]string)
 			}
 
+			// Apply the metadata in an immutable way: if a metadata already
+			// exist, it's not possible to override it.
 			for key, val := range op.NewMetadata {
 				if _, exist := base.extraMetadata[key]; !exist {
 					base.extraMetadata[key] = val
diff --git a/bug/operation_iterator_test.go b/bug/operation_iterator_test.go
index b922bec1..fcc61d0f 100644
--- a/bug/operation_iterator_test.go
+++ b/bug/operation_iterator_test.go
@@ -1,12 +1,14 @@
 package bug
 
 import (
-	"github.com/MichaelMure/git-bug/identity"
-	"github.com/MichaelMure/git-bug/repository"
-	"github.com/stretchr/testify/assert"
-
+	"fmt"
 	"testing"
 	"time"
+
+	"github.com/stretchr/testify/assert"
+
+	"github.com/MichaelMure/git-bug/identity"
+	"github.com/MichaelMure/git-bug/repository"
 )
 
 func ExampleOperationIterator() {
@@ -29,16 +31,20 @@ func TestOpIterator(t *testing.T) {
 	unix := time.Now().Unix()
 
 	createOp := NewCreateOp(rene, unix, "title", "message", nil)
-	setTitleOp := NewSetTitleOp(rene, unix, "title2", "title1")
 	addCommentOp := NewAddCommentOp(rene, unix, "message2", nil)
 	setStatusOp := NewSetStatusOp(rene, unix, ClosedStatus)
 	labelChangeOp := NewLabelChangeOperation(rene, unix, []Label{"added"}, []Label{"removed"})
 
+	var i int
+	genTitleOp := func() Operation {
+		i++
+		return NewSetTitleOp(rene, unix, fmt.Sprintf("title%d", i), "")
+	}
+
 	bug1 := NewBug()
 
 	// first pack
 	bug1.Append(createOp)
-	bug1.Append(setTitleOp)
 	bug1.Append(addCommentOp)
 	bug1.Append(setStatusOp)
 	bug1.Append(labelChangeOp)
@@ -46,16 +52,16 @@ func TestOpIterator(t *testing.T) {
 	assert.NoError(t, err)
 
 	// second pack
-	bug1.Append(setTitleOp)
-	bug1.Append(setTitleOp)
-	bug1.Append(setTitleOp)
+	bug1.Append(genTitleOp())
+	bug1.Append(genTitleOp())
+	bug1.Append(genTitleOp())
 	err = bug1.Commit(mockRepo)
 	assert.NoError(t, err)
 
 	// staging
-	bug1.Append(setTitleOp)
-	bug1.Append(setTitleOp)
-	bug1.Append(setTitleOp)
+	bug1.Append(genTitleOp())
+	bug1.Append(genTitleOp())
+	bug1.Append(genTitleOp())
 
 	it := NewOperationIterator(bug1)
 
@@ -65,7 +71,5 @@ func TestOpIterator(t *testing.T) {
 		counter++
 	}
 
-	if counter != 11 {
-		t.Fatalf("Wrong count of value iterated (%d instead of 8)", counter)
-	}
+	assert.Equal(t, 10, counter)
 }
diff --git a/misc/random_bugs/create_random_bugs.go b/misc/random_bugs/create_random_bugs.go
index ea8833ee..b5c19e63 100644
--- a/misc/random_bugs/create_random_bugs.go
+++ b/misc/random_bugs/create_random_bugs.go
@@ -11,7 +11,7 @@ import (
 	"github.com/icrowley/fake"
 )
 
-type opsGenerator func(bug.Interface, identity.Interface)
+type opsGenerator func(bug.Interface, identity.Interface, int64)
 
 type Options struct {
 	BugNumber    int
@@ -61,6 +61,12 @@ func generateRandomBugsWithSeed(opts Options, seed int64) []*bug.Bug {
 	rand.Seed(seed)
 	fake.Seed(seed)
 
+	// At the moment git-bug has a risk of hash collision is simple
+	// operation (like open/close) are made with the same timestamp.
+	// As a temporary workaround, we use here an strictly increasing
+	// timestamp
+	timestamp := time.Now().Unix()
+
 	opsGenerators := []opsGenerator{
 		comment,
 		comment,
@@ -94,7 +100,8 @@ func generateRandomBugsWithSeed(opts Options, seed int64) []*bug.Bug {
 
 		for j := 0; j < nOps; j++ {
 			index := rand.Intn(len(opsGenerators))
-			opsGenerators[index](b, randomPerson())
+			opsGenerators[index](b, randomPerson(), timestamp)
+			timestamp++
 		}
 
 		result[i] = b
@@ -177,25 +184,25 @@ func paragraphs() string {
 	return strings.Replace(p, "\t", "\n\n", -1)
 }
 
-func comment(b bug.Interface, p identity.Interface) {
-	_, _ = bug.AddComment(b, p, time.Now().Unix(), paragraphs())
+func comment(b bug.Interface, p identity.Interface, timestamp int64) {
+	_, _ = bug.AddComment(b, p, timestamp, paragraphs())
 }
 
-func title(b bug.Interface, p identity.Interface) {
-	_, _ = bug.SetTitle(b, p, time.Now().Unix(), fake.Sentence())
+func title(b bug.Interface, p identity.Interface, timestamp int64) {
+	_, _ = bug.SetTitle(b, p, timestamp, fake.Sentence())
 }
 
-func open(b bug.Interface, p identity.Interface) {
-	_, _ = bug.Open(b, p, time.Now().Unix())
+func open(b bug.Interface, p identity.Interface, timestamp int64) {
+	_, _ = bug.Open(b, p, timestamp)
 }
 
-func close(b bug.Interface, p identity.Interface) {
-	_, _ = bug.Close(b, p, time.Now().Unix())
+func close(b bug.Interface, p identity.Interface, timestamp int64) {
+	_, _ = bug.Close(b, p, timestamp)
 }
 
 var addedLabels []string
 
-func labels(b bug.Interface, p identity.Interface) {
+func labels(b bug.Interface, p identity.Interface, timestamp int64) {
 	var removed []string
 	nbRemoved := rand.Intn(3)
 	for nbRemoved > 0 && len(addedLabels) > 0 {
@@ -217,5 +224,5 @@ func labels(b bug.Interface, p identity.Interface) {
 	// ignore error
 	// if the randomisation produce no changes, no op
 	// is added to the bug
-	_, _, _ = bug.ChangeLabels(b, p, time.Now().Unix(), added, removed)
+	_, _, _ = bug.ChangeLabels(b, p, timestamp, added, removed)
 }