From 283e97111b0b39a4e8f7717234f0bfbbb4f481af Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michael=20Mur=C3=A9?= Date: Tue, 27 Aug 2019 12:11:51 +0200 Subject: [PATCH] bug: make sure there is no Operation's hash collision --- Gopkg.lock | 1 + bug/bug.go | 6 +++++ bug/bug_test.go | 3 --- bug/op_set_metadata.go | 2 ++ bug/operation_iterator_test.go | 34 ++++++++++++++------------ misc/random_bugs/create_random_bugs.go | 31 ++++++++++++++--------- 6 files changed, 47 insertions(+), 30 deletions(-) diff --git a/Gopkg.lock b/Gopkg.lock index e4ecfcb9b1aed465d6b82666b4b9b5906e97d269..fdaa5493ea2c19ef4ba0b04eb0907c01906fc5fb 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 ca817dc1311bdb5eb15654891d8a81b4b147ce91..ae662ef1647b282ef1612f9b413220f79d4629c5 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 35e8a39597936f43945259ecc990dc96a1687c60..480d312ed6e866fe833b559b2ebe768d6f0dceea 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 f99f836bc2f588c0e34c729d5c7f376ec65d6640..67f7e00936af371851a095ad6e7a8df34d2d9a22 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 b922bec1691762d9ab4c5158965cff8147fe0427..fcc61d0f447b5bfab42bab2b34fef4ef6bcebfd6 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 ea8833ee89c222b6e217f4d751089599f557187e..b5c19e639b4724e0fa9c8be663a569d39c61c1d7 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) }