bug: introduce WithSnapshot to maintain incrementally and effitiently a snapshot

Michael Muré created

Change summary

bug/bug.go                             | 20 +++++---
bug/interface.go                       | 52 +++++++++++++++++++++++++
bug/operation_iterator.go              |  4 
bug/operations/add_comment.go          |  4 
bug/operations/label_change.go         |  2 
bug/operations/set_status.go           |  4 
bug/operations/set_title.go            |  2 
bug/with_snapshot.go                   | 58 +++++++++++++++++++++++++++
cache/bug_cache.go                     | 31 +-------------
misc/random_bugs/create_random_bugs.go |  8 +-
10 files changed, 137 insertions(+), 48 deletions(-)

Detailed changes

bug/bug.go 🔗

@@ -24,6 +24,8 @@ const editClockEntryPattern = "edit-clock-%d"
 const idLength = 40
 const humanIdLength = 7
 
+var _ Interface = &Bug{}
+
 // Bug hold the data of a bug thread, organized in a way close to
 // how it will be persisted inside Git. This is the data structure
 // used to merge two different version of the same Bug.
@@ -468,25 +470,27 @@ func makeMediaTree(pack OperationPack) []repository.TreeEntry {
 // Merge a different version of the same bug by rebasing operations of this bug
 // that are not present in the other on top of the chain of operations of the
 // other version.
-func (bug *Bug) Merge(repo repository.Repo, other *Bug) (bool, error) {
+func (bug *Bug) Merge(repo repository.Repo, other Interface) (bool, error) {
+	var otherBug = bugFromInterface(other)
+
 	// Note: a faster merge should be possible without actually reading and parsing
 	// all operations pack of our side.
 	// Reading the other side is still necessary to validate remote data, at least
 	// for new operations
 
-	if bug.id != other.id {
+	if bug.id != otherBug.id {
 		return false, errors.New("merging unrelated bugs is not supported")
 	}
 
-	if len(other.staging.Operations) > 0 {
+	if len(otherBug.staging.Operations) > 0 {
 		return false, errors.New("merging a bug with a non-empty staging is not supported")
 	}
 
-	if bug.lastCommit == "" || other.lastCommit == "" {
+	if bug.lastCommit == "" || otherBug.lastCommit == "" {
 		return false, errors.New("can't merge a bug that has never been stored")
 	}
 
-	ancestor, err := repo.FindCommonAncestor(bug.lastCommit, other.lastCommit)
+	ancestor, err := repo.FindCommonAncestor(bug.lastCommit, otherBug.lastCommit)
 
 	if err != nil {
 		return false, err
@@ -505,15 +509,15 @@ func (bug *Bug) Merge(repo repository.Repo, other *Bug) (bool, error) {
 		}
 	}
 
-	if len(other.packs) == ancestorIndex+1 {
+	if len(otherBug.packs) == ancestorIndex+1 {
 		// Nothing to rebase, return early
 		return false, nil
 	}
 
 	// get other bug's extra packs
-	for i := ancestorIndex + 1; i < len(other.packs); i++ {
+	for i := ancestorIndex + 1; i < len(otherBug.packs); i++ {
 		// clone is probably not necessary
-		newPack := other.packs[i].Clone()
+		newPack := otherBug.packs[i].Clone()
 
 		newPacks = append(newPacks, newPack)
 		bug.lastCommit = newPack.commitHash

bug/interface.go 🔗

@@ -0,0 +1,52 @@
+package bug
+
+import (
+	"github.com/MichaelMure/git-bug/repository"
+)
+
+type Interface interface {
+	// Id return the Bug identifier
+	Id() string
+
+	// HumanId return the Bug identifier truncated for human consumption
+	HumanId() string
+
+	// IsValid check if the Bug data is valid
+	IsValid() bool
+
+	// Append an operation into the staging area, to be committed later
+	Append(op Operation)
+
+	// Append an operation into the staging area, to be committed later
+	HasPendingOp() bool
+
+	// Commit write the staging area in Git and move the operations to the packs
+	Commit(repo repository.Repo) error
+
+	// Merge a different version of the same bug by rebasing operations of this bug
+	// that are not present in the other on top of the chain of operations of the
+	// other version.
+	Merge(repo repository.Repo, other Interface) (bool, error)
+
+	// Lookup for the very first operation of the bug.
+	// For a valid Bug, this operation should be a CreateOp
+	FirstOp() Operation
+
+	// Lookup for the very last operation of the bug.
+	// For a valid Bug, should never be nil
+	LastOp() Operation
+
+	// Compile a bug in a easily usable snapshot
+	Compile() Snapshot
+}
+
+func bugFromInterface(bug Interface) *Bug {
+	switch bug.(type) {
+	case *Bug:
+		return bug.(*Bug)
+	case *WithSnapshot:
+		return bug.(*WithSnapshot).Bug
+	default:
+		panic("missing type case")
+	}
+}

bug/operation_iterator.go 🔗

@@ -6,9 +6,9 @@ type OperationIterator struct {
 	opIndex   int
 }
 
-func NewOperationIterator(bug *Bug) *OperationIterator {
+func NewOperationIterator(bug Interface) *OperationIterator {
 	return &OperationIterator{
-		bug:       bug,
+		bug:       bugFromInterface(bug),
 		packIndex: 0,
 		opIndex:   -1,
 	}

bug/operations/add_comment.go 🔗

@@ -42,11 +42,11 @@ func NewAddCommentOp(author bug.Person, message string, files []util.Hash) AddCo
 }
 
 // Convenience function to apply the operation
-func Comment(b *bug.Bug, author bug.Person, message string) {
+func Comment(b bug.Interface, author bug.Person, message string) {
 	CommentWithFiles(b, author, message, nil)
 }
 
-func CommentWithFiles(b *bug.Bug, author bug.Person, message string, files []util.Hash) {
+func CommentWithFiles(b bug.Interface, author bug.Person, message string, files []util.Hash) {
 	addCommentOp := NewAddCommentOp(author, message, files)
 	b.Append(addCommentOp)
 }

bug/operations/label_change.go 🔗

@@ -60,7 +60,7 @@ func NewLabelChangeOperation(author bug.Person, added, removed []bug.Label) Labe
 }
 
 // ChangeLabels is a convenience function to apply the operation
-func ChangeLabels(out io.Writer, b *bug.Bug, author bug.Person, add, remove []string) error {
+func ChangeLabels(out io.Writer, b bug.Interface, author bug.Person, add, remove []string) error {
 	var added, removed []bug.Label
 
 	if out == nil {

bug/operations/set_status.go 🔗

@@ -27,13 +27,13 @@ func NewSetStatusOp(author bug.Person, status bug.Status) SetStatusOperation {
 }
 
 // Convenience function to apply the operation
-func Open(b *bug.Bug, author bug.Person) {
+func Open(b bug.Interface, author bug.Person) {
 	op := NewSetStatusOp(author, bug.OpenStatus)
 	b.Append(op)
 }
 
 // Convenience function to apply the operation
-func Close(b *bug.Bug, author bug.Person) {
+func Close(b bug.Interface, author bug.Person) {
 	op := NewSetStatusOp(author, bug.ClosedStatus)
 	b.Append(op)
 }

bug/operations/set_title.go 🔗

@@ -29,7 +29,7 @@ func NewSetTitleOp(author bug.Person, title string, was string) SetTitleOperatio
 }
 
 // Convenience function to apply the operation
-func SetTitle(b *bug.Bug, author bug.Person, title string) {
+func SetTitle(b bug.Interface, author bug.Person, title string) {
 	it := bug.NewOperationIterator(b)
 
 	var lastTitleOp bug.Operation

bug/with_snapshot.go 🔗

@@ -0,0 +1,58 @@
+package bug
+
+import "github.com/MichaelMure/git-bug/repository"
+
+var _ Interface = &WithSnapshot{}
+
+// WithSnapshot encapsulate a Bug and maintain the corresponding Snapshot efficiently
+type WithSnapshot struct {
+	*Bug
+	snap *Snapshot
+}
+
+// Snapshot return the current snapshot
+func (b *WithSnapshot) Snapshot() *Snapshot {
+	if b.snap == nil {
+		snap := b.Bug.Compile()
+		b.snap = &snap
+	}
+	return b.snap
+}
+
+// Append intercept Bug.Append() to update the snapshot efficiently
+func (b *WithSnapshot) Append(op Operation) {
+	b.Bug.Append(op)
+
+	if b.snap == nil {
+		return
+	}
+
+	snap := op.Apply(*b.snap)
+	b.snap = &snap
+}
+
+// Commit intercept Bug.Commit() to update the snapshot efficiently
+func (b *WithSnapshot) Commit(repo repository.Repo) error {
+	err := b.Bug.Commit(repo)
+
+	if err != nil {
+		b.snap = nil
+		return err
+	}
+
+	// Commit() shouldn't change anything of the bug state apart from the
+	// initial ID set
+
+	if b.snap == nil {
+		return nil
+	}
+
+	b.snap.id = b.Bug.id
+	return nil
+}
+
+// Merge intercept Bug.Merge() and clear the snapshot
+func (b *WithSnapshot) Merge(repo repository.Repo, other Interface) (bool, error) {
+	b.snap = nil
+	return b.Bug.Merge(repo, other)
+}

cache/bug_cache.go 🔗

@@ -9,7 +9,6 @@ import (
 
 type BugCacher interface {
 	Snapshot() *bug.Snapshot
-	ClearSnapshot()
 
 	// Mutations
 	AddComment(message string) error
@@ -25,27 +24,18 @@ type BugCacher interface {
 
 type BugCache struct {
 	repo repository.Repo
-	bug  *bug.Bug
-	snap *bug.Snapshot
+	bug  *bug.WithSnapshot
 }
 
 func NewBugCache(repo repository.Repo, b *bug.Bug) BugCacher {
 	return &BugCache{
 		repo: repo,
-		bug:  b,
+		bug:  &bug.WithSnapshot{Bug: b},
 	}
 }
 
 func (c *BugCache) Snapshot() *bug.Snapshot {
-	if c.snap == nil {
-		snap := c.bug.Compile()
-		c.snap = &snap
-	}
-	return c.snap
-}
-
-func (c *BugCache) ClearSnapshot() {
-	c.snap = nil
+	return c.bug.Snapshot()
 }
 
 func (c *BugCache) AddComment(message string) error {
@@ -60,9 +50,6 @@ func (c *BugCache) AddCommentWithFiles(message string, files []util.Hash) error
 
 	operations.CommentWithFiles(c.bug, author, message, files)
 
-	// TODO: perf --> the snapshot could simply be updated with the new op
-	c.ClearSnapshot()
-
 	return nil
 }
 
@@ -77,9 +64,6 @@ func (c *BugCache) ChangeLabels(added []string, removed []string) error {
 		return err
 	}
 
-	// TODO: perf --> the snapshot could simply be updated with the new op
-	c.ClearSnapshot()
-
 	return nil
 }
 
@@ -91,9 +75,6 @@ func (c *BugCache) Open() error {
 
 	operations.Open(c.bug, author)
 
-	// TODO: perf --> the snapshot could simply be updated with the new op
-	c.ClearSnapshot()
-
 	return nil
 }
 
@@ -105,9 +86,6 @@ func (c *BugCache) Close() error {
 
 	operations.Close(c.bug, author)
 
-	// TODO: perf --> the snapshot could simply be updated with the new op
-	c.ClearSnapshot()
-
 	return nil
 }
 
@@ -119,9 +97,6 @@ func (c *BugCache) SetTitle(title string) error {
 
 	operations.SetTitle(c.bug, author, title)
 
-	// TODO: perf --> the snapshot could simply be updated with the new op
-	c.ClearSnapshot()
-
 	return nil
 }
 

misc/random_bugs/create_random_bugs.go 🔗

@@ -11,7 +11,7 @@ import (
 	"github.com/icrowley/fake"
 )
 
-type opsGenerator func(*bug.Bug, bug.Person)
+type opsGenerator func(bug.Interface, bug.Person)
 
 type Options struct {
 	BugNumber    int
@@ -94,17 +94,17 @@ func paragraphs() string {
 	return strings.Replace(p, "\t", "\n\n", -1)
 }
 
-func comment(b *bug.Bug, p bug.Person) {
+func comment(b bug.Interface, p bug.Person) {
 	operations.Comment(b, p, paragraphs())
 }
 
-func title(b *bug.Bug, p bug.Person) {
+func title(b bug.Interface, p bug.Person) {
 	operations.SetTitle(b, p, fake.Sentence())
 }
 
 var addedLabels []string
 
-func labels(b *bug.Bug, p bug.Person) {
+func labels(b bug.Interface, p bug.Person) {
 	var removed []string
 	nbRemoved := rand.Intn(3)
 	for nbRemoved > 0 && len(addedLabels) > 0 {