cache: properly push/pull identities and bugs

Michael Muré created 6 years ago

Change summary

bug/bug_actions.go           | 92 +++++-------------------------------
cache/repo_cache.go          | 60 ++++++++++++++++++-----
commands/pull.go             | 11 ++--
entity/doc.go                |  8 +++
entity/interface.go          |  8 +++
entity/merge.go              | 68 +++++++++++++++++++++++++++
identity/identity_actions.go | 93 +++++--------------------------------
termui/bug_table.go          | 12 ++--
8 files changed, 169 insertions(+), 183 deletions(-)

Detailed changes

bug/bug_actions.go

@@ -4,6 +4,7 @@ import (
 	"fmt"
 	"strings"
 
+	"github.com/MichaelMure/git-bug/entity"
 	"github.com/MichaelMure/git-bug/repository"
 	"github.com/pkg/errors"
 )
@@ -34,7 +35,7 @@ func Pull(repo repository.ClockedRepo, remote string) error {
 		if merge.Err != nil {
 			return merge.Err
 		}
-		if merge.Status == MergeStatusInvalid {
+		if merge.Status == entity.MergeStatusInvalid {
 			return errors.Errorf("merge failure: %s", merge.Reason)
 		}
 	}
@@ -49,8 +50,8 @@ func Pull(repo repository.ClockedRepo, remote string) error {
 // - if the local bug has new commits but the remote don't, nothing is changed
 // - if both local and remote bug have new commits (that is, we have a concurrent edition),
 //   new local commits are rewritten at the head of the remote history (that is, a rebase)
-func MergeAll(repo repository.ClockedRepo, remote string) <-chan MergeResult {
-	out := make(chan MergeResult)
+func MergeAll(repo repository.ClockedRepo, remote string) <-chan entity.MergeResult {
+	out := make(chan entity.MergeResult)
 
 	go func() {
 		defer close(out)
@@ -59,7 +60,7 @@ func MergeAll(repo repository.ClockedRepo, remote string) <-chan MergeResult {
 		remoteRefs, err := repo.ListRefs(remoteRefSpec)
 
 		if err != nil {
-			out <- MergeResult{Err: err}
+			out <- entity.MergeResult{Err: err}
 			return
 		}
 
@@ -70,13 +71,13 @@ func MergeAll(repo repository.ClockedRepo, remote string) <-chan MergeResult {
 			remoteBug, err := readBug(repo, remoteRef)
 
 			if err != nil {
-				out <- newMergeInvalidStatus(id, errors.Wrap(err, "remote bug is not readable").Error())
+				out <- entity.NewMergeInvalidStatus(id, errors.Wrap(err, "remote bug is not readable").Error())
 				continue
 			}
 
 			// Check for error in remote data
 			if err := remoteBug.Validate(); err != nil {
-				out <- newMergeInvalidStatus(id, errors.Wrap(err, "remote bug is invalid").Error())
+				out <- entity.NewMergeInvalidStatus(id, errors.Wrap(err, "remote bug is invalid").Error())
 				continue
 			}
 
@@ -84,7 +85,7 @@ func MergeAll(repo repository.ClockedRepo, remote string) <-chan MergeResult {
 			localExist, err := repo.RefExist(localRef)
 
 			if err != nil {
-				out <- newMergeError(err, id)
+				out <- entity.NewMergeError(err, id)
 				continue
 			}
 
@@ -93,100 +94,35 @@ func MergeAll(repo repository.ClockedRepo, remote string) <-chan MergeResult {
 				err := repo.CopyRef(remoteRef, localRef)
 
 				if err != nil {
-					out <- newMergeError(err, id)
+					out <- entity.NewMergeError(err, id)
 					return
 				}
 
-				out <- newMergeStatus(MergeStatusNew, id, remoteBug)
+				out <- entity.NewMergeStatus(entity.MergeStatusNew, id, remoteBug)
 				continue
 			}
 
 			localBug, err := readBug(repo, localRef)
 
 			if err != nil {
-				out <- newMergeError(errors.Wrap(err, "local bug is not readable"), id)
+				out <- entity.NewMergeError(errors.Wrap(err, "local bug is not readable"), id)
 				return
 			}
 
 			updated, err := localBug.Merge(repo, remoteBug)
 
 			if err != nil {
-				out <- newMergeInvalidStatus(id, errors.Wrap(err, "merge failed").Error())
+				out <- entity.NewMergeInvalidStatus(id, errors.Wrap(err, "merge failed").Error())
 				return
 			}
 
 			if updated {
-				out <- newMergeStatus(MergeStatusUpdated, id, localBug)
+				out <- entity.NewMergeStatus(entity.MergeStatusUpdated, id, localBug)
 			} else {
-				out <- newMergeStatus(MergeStatusNothing, id, localBug)
+				out <- entity.NewMergeStatus(entity.MergeStatusNothing, id, localBug)
 			}
 		}
 	}()
 
 	return out
 }
-
-// MergeStatus represent the result of a merge operation of a bug
-type MergeStatus int
-
-const (
-	_ MergeStatus = iota
-	MergeStatusNew
-	MergeStatusInvalid
-	MergeStatusUpdated
-	MergeStatusNothing
-)
-
-type MergeResult struct {
-	// Err is set when a terminal error occur in the process
-	Err error
-
-	Id     string
-	Status MergeStatus
-
-	// Only set for invalid status
-	Reason string
-
-	// Not set for invalid status
-	Bug *Bug
-}
-
-func (mr MergeResult) String() string {
-	switch mr.Status {
-	case MergeStatusNew:
-		return "new"
-	case MergeStatusInvalid:
-		return fmt.Sprintf("invalid data: %s", mr.Reason)
-	case MergeStatusUpdated:
-		return "updated"
-	case MergeStatusNothing:
-		return "nothing to do"
-	default:
-		panic("unknown merge status")
-	}
-}
-
-func newMergeError(err error, id string) MergeResult {
-	return MergeResult{
-		Err: err,
-		Id:  id,
-	}
-}
-
-func newMergeStatus(status MergeStatus, id string, bug *Bug) MergeResult {
-	return MergeResult{
-		Id:     id,
-		Status: status,
-
-		// Bug is not set for an invalid merge result
-		Bug: bug,
-	}
-}
-
-func newMergeInvalidStatus(id string, reason string) MergeResult {
-	return MergeResult{
-		Id:     id,
-		Status: MergeStatusInvalid,
-		Reason: reason,
-	}
-}

cache/repo_cache.go

@@ -14,6 +14,7 @@ import (
 	"time"
 
 	"github.com/MichaelMure/git-bug/bug"
+	"github.com/MichaelMure/git-bug/entity"
 	"github.com/MichaelMure/git-bug/identity"
 	"github.com/MichaelMure/git-bug/repository"
 	"github.com/MichaelMure/git-bug/util/git"
@@ -593,23 +594,31 @@ func (c *RepoCache) NewBugRaw(author *IdentityCache, unixTime int64, title strin
 	return cached, nil
 }
 
-// Fetch retrieve update from a remote
-// This does not change the local bugs state
+// Fetch retrieve updates from a remote
+// This does not change the local bugs or identities state
 func (c *RepoCache) Fetch(remote string) (string, error) {
-	return bug.Fetch(c.repo, remote)
-}
+	stdout1, err := identity.Fetch(c.repo, remote)
+	if err != nil {
+		return stdout1, err
+	}
+
+	stdout2, err := bug.Fetch(c.repo, remote)
+	if err != nil {
+		return stdout2, err
+	}
 
-// MergeAll will merge all the available remote bug
-func (c *RepoCache) MergeAll(remote string) <-chan bug.MergeResult {
-	// TODO: add identities
+	return stdout1 + stdout2, nil
+}
 
-	out := make(chan bug.MergeResult)
+// MergeAll will merge all the available remote bug and identities
+func (c *RepoCache) MergeAll(remote string) <-chan entity.MergeResult {
+	out := make(chan entity.MergeResult)
 
 	// Intercept merge results to update the cache properly
 	go func() {
 		defer close(out)
 
-		results := bug.MergeAll(c.repo, remote)
+		results := identity.MergeAll(c.repo, remote)
 		for result := range results {
 			out <- result
 
@@ -617,13 +626,26 @@ func (c *RepoCache) MergeAll(remote string) <-chan bug.MergeResult {
 				continue
 			}
 
-			id := result.Id
+			switch result.Status {
+			case entity.MergeStatusNew, entity.MergeStatusUpdated:
+				i := result.Entity.(*identity.Identity)
+				c.identitiesExcerpts[result.Id] = NewIdentityExcerpt(i)
+			}
+		}
+
+		results = bug.MergeAll(c.repo, remote)
+		for result := range results {
+			out <- result
+
+			if result.Err != nil {
+				continue
+			}
 
 			switch result.Status {
-			case bug.MergeStatusNew, bug.MergeStatusUpdated:
-				b := result.Bug
+			case entity.MergeStatusNew, entity.MergeStatusUpdated:
+				b := result.Entity.(*bug.Bug)
 				snap := b.Compile()
-				c.bugExcerpts[id] = NewBugExcerpt(b, &snap)
+				c.bugExcerpts[result.Id] = NewBugExcerpt(b, &snap)
 			}
 		}
 
@@ -640,7 +662,17 @@ func (c *RepoCache) MergeAll(remote string) <-chan bug.MergeResult {
 
 // Push update a remote with the local changes
 func (c *RepoCache) Push(remote string) (string, error) {
-	return bug.Push(c.repo, remote)
+	stdout1, err := identity.Push(c.repo, remote)
+	if err != nil {
+		return stdout1, err
+	}
+
+	stdout2, err := bug.Push(c.repo, remote)
+	if err != nil {
+		return stdout2, err
+	}
+
+	return stdout1 + stdout2, nil
 }
 
 func repoLockFilePath(repo repository.Repo) string {

commands/pull.go

@@ -6,6 +6,7 @@ import (
 
 	"github.com/MichaelMure/git-bug/bug"
 	"github.com/MichaelMure/git-bug/cache"
+	"github.com/MichaelMure/git-bug/entity"
 	"github.com/MichaelMure/git-bug/util/interrupt"
 	"github.com/spf13/cobra"
 )
@@ -38,13 +39,13 @@ func runPull(cmd *cobra.Command, args []string) error {
 
 	fmt.Println("Merging data ...")
 
-	for merge := range backend.MergeAll(remote) {
-		if merge.Err != nil {
-			fmt.Println(merge.Err)
+	for result := range backend.MergeAll(remote) {
+		if result.Err != nil {
+			fmt.Println(result.Err)
 		}
 
-		if merge.Status != bug.MergeStatusNothing {
-			fmt.Printf("%s: %s\n", bug.FormatHumanID(merge.Id), merge)
+		if result.Status != entity.MergeStatusNothing {
+			fmt.Printf("%s: %s\n", bug.FormatHumanID(result.Id), result)
 		}
 	}
 

entity/doc.go

@@ -0,0 +1,8 @@
+// Package entity contains the base common code to define an entity stored
+// in a chain of git objects, supporting actions like Push, Pull and Merge.
+package entity
+
+// TODO: Bug and Identity are very similar, right ? I expect that this package
+// will eventually hold the common code to define an entity and the related
+// helpers, errors and so on. When this work is done, it will become easier
+// to add new entities, for example to support pull requests.

entity/interface.go

@@ -0,0 +1,8 @@
+package entity
+
+type Interface interface {
+	// Id return the Entity identifier
+	Id() string
+	// HumanId return the Entity identifier truncated for human consumption
+	HumanId() string
+}

entity/merge.go

@@ -0,0 +1,68 @@
+package entity
+
+import "fmt"
+
+// MergeStatus represent the result of a merge operation of an entity
+type MergeStatus int
+
+const (
+	_ MergeStatus = iota
+	MergeStatusNew
+	MergeStatusInvalid
+	MergeStatusUpdated
+	MergeStatusNothing
+)
+
+type MergeResult struct {
+	// Err is set when a terminal error occur in the process
+	Err error
+
+	Id     string
+	Status MergeStatus
+
+	// Only set for invalid status
+	Reason string
+
+	// Not set for invalid status
+	Entity Interface
+}
+
+func (mr MergeResult) String() string {
+	switch mr.Status {
+	case MergeStatusNew:
+		return "new"
+	case MergeStatusInvalid:
+		return fmt.Sprintf("invalid data: %s", mr.Reason)
+	case MergeStatusUpdated:
+		return "updated"
+	case MergeStatusNothing:
+		return "nothing to do"
+	default:
+		panic("unknown merge status")
+	}
+}
+
+func NewMergeError(err error, id string) MergeResult {
+	return MergeResult{
+		Err: err,
+		Id:  id,
+	}
+}
+
+func NewMergeStatus(status MergeStatus, id string, entity Interface) MergeResult {
+	return MergeResult{
+		Id:     id,
+		Status: status,
+
+		// Entity is not set for an invalid merge result
+		Entity: entity,
+	}
+}
+
+func NewMergeInvalidStatus(id string, reason string) MergeResult {
+	return MergeResult{
+		Id:     id,
+		Status: MergeStatusInvalid,
+		Reason: reason,
+	}
+}

identity/identity_actions.go

@@ -4,6 +4,7 @@ import (
 	"fmt"
 	"strings"
 
+	"github.com/MichaelMure/git-bug/entity"
 	"github.com/MichaelMure/git-bug/repository"
 	"github.com/pkg/errors"
 )
@@ -34,7 +35,7 @@ func Pull(repo repository.ClockedRepo, remote string) error {
 		if merge.Err != nil {
 			return merge.Err
 		}
-		if merge.Status == MergeStatusInvalid {
+		if merge.Status == entity.MergeStatusInvalid {
 			return errors.Errorf("merge failure: %s", merge.Reason)
 		}
 	}
@@ -43,8 +44,8 @@ func Pull(repo repository.ClockedRepo, remote string) error {
 }
 
 // MergeAll will merge all the available remote identity
-func MergeAll(repo repository.ClockedRepo, remote string) <-chan MergeResult {
-	out := make(chan MergeResult)
+func MergeAll(repo repository.ClockedRepo, remote string) <-chan entity.MergeResult {
+	out := make(chan entity.MergeResult)
 
 	go func() {
 		defer close(out)
@@ -53,7 +54,7 @@ func MergeAll(repo repository.ClockedRepo, remote string) <-chan MergeResult {
 		remoteRefs, err := repo.ListRefs(remoteRefSpec)
 
 		if err != nil {
-			out <- MergeResult{Err: err}
+			out <- entity.MergeResult{Err: err}
 			return
 		}
 
@@ -64,13 +65,13 @@ func MergeAll(repo repository.ClockedRepo, remote string) <-chan MergeResult {
 			remoteIdentity, err := read(repo, remoteRef)
 
 			if err != nil {
-				out <- newMergeInvalidStatus(id, errors.Wrap(err, "remote identity is not readable").Error())
+				out <- entity.NewMergeInvalidStatus(id, errors.Wrap(err, "remote identity is not readable").Error())
 				continue
 			}
 
 			// Check for error in remote data
 			if err := remoteIdentity.Validate(); err != nil {
-				out <- newMergeInvalidStatus(id, errors.Wrap(err, "remote identity is invalid").Error())
+				out <- entity.NewMergeInvalidStatus(id, errors.Wrap(err, "remote identity is invalid").Error())
 				continue
 			}
 
@@ -78,7 +79,7 @@ func MergeAll(repo repository.ClockedRepo, remote string) <-chan MergeResult {
 			localExist, err := repo.RefExist(localRef)
 
 			if err != nil {
-				out <- newMergeError(err, id)
+				out <- entity.NewMergeError(err, id)
 				continue
 			}
 
@@ -87,101 +88,35 @@ func MergeAll(repo repository.ClockedRepo, remote string) <-chan MergeResult {
 				err := repo.CopyRef(remoteRef, localRef)
 
 				if err != nil {
-					out <- newMergeError(err, id)
+					out <- entity.NewMergeError(err, id)
 					return
 				}
 
-				out <- newMergeStatus(MergeStatusNew, id, remoteIdentity)
+				out <- entity.NewMergeStatus(entity.MergeStatusNew, id, remoteIdentity)
 				continue
 			}
 
 			localIdentity, err := read(repo, localRef)
 
 			if err != nil {
-				out <- newMergeError(errors.Wrap(err, "local identity is not readable"), id)
+				out <- entity.NewMergeError(errors.Wrap(err, "local identity is not readable"), id)
 				return
 			}
 
 			updated, err := localIdentity.Merge(repo, remoteIdentity)
 
 			if err != nil {
-				out <- newMergeInvalidStatus(id, errors.Wrap(err, "merge failed").Error())
+				out <- entity.NewMergeInvalidStatus(id, errors.Wrap(err, "merge failed").Error())
 				return
 			}
 
 			if updated {
-				out <- newMergeStatus(MergeStatusUpdated, id, localIdentity)
+				out <- entity.NewMergeStatus(entity.MergeStatusUpdated, id, localIdentity)
 			} else {
-				out <- newMergeStatus(MergeStatusNothing, id, localIdentity)
+				out <- entity.NewMergeStatus(entity.MergeStatusNothing, id, localIdentity)
 			}
 		}
 	}()
 
 	return out
 }
-
-// MergeStatus represent the result of a merge operation of a bug
-type MergeStatus int
-
-const (
-	_ MergeStatus = iota
-	MergeStatusNew
-	MergeStatusInvalid
-	MergeStatusUpdated
-	MergeStatusNothing
-)
-
-// Todo: share a generalized MergeResult with the bug package ?
-type MergeResult struct {
-	// Err is set when a terminal error occur in the process
-	Err error
-
-	Id     string
-	Status MergeStatus
-
-	// Only set for invalid status
-	Reason string
-
-	// Not set for invalid status
-	Identity *Identity
-}
-
-func (mr MergeResult) String() string {
-	switch mr.Status {
-	case MergeStatusNew:
-		return "new"
-	case MergeStatusInvalid:
-		return fmt.Sprintf("invalid data: %s", mr.Reason)
-	case MergeStatusUpdated:
-		return "updated"
-	case MergeStatusNothing:
-		return "nothing to do"
-	default:
-		panic("unknown merge status")
-	}
-}
-
-func newMergeError(err error, id string) MergeResult {
-	return MergeResult{
-		Err: err,
-		Id:  id,
-	}
-}
-
-func newMergeStatus(status MergeStatus, id string, identity *Identity) MergeResult {
-	return MergeResult{
-		Id:     id,
-		Status: status,
-
-		// Identity is not set for an invalid merge result
-		Identity: identity,
-	}
-}
-
-func newMergeInvalidStatus(id string, reason string) MergeResult {
-	return MergeResult{
-		Id:     id,
-		Status: MergeStatusInvalid,
-		Reason: reason,
-	}
-}

termui/bug_table.go

@@ -4,8 +4,8 @@ import (
 	"bytes"
 	"fmt"
 
-	"github.com/MichaelMure/git-bug/bug"
 	"github.com/MichaelMure/git-bug/cache"
+	"github.com/MichaelMure/git-bug/entity"
 	"github.com/MichaelMure/git-bug/util/colors"
 	"github.com/MichaelMure/git-bug/util/text"
 	"github.com/MichaelMure/gocui"
@@ -435,8 +435,6 @@ func (bt *bugTable) openBug(g *gocui.Gui, v *gocui.View) error {
 }
 
 func (bt *bugTable) pull(g *gocui.Gui, v *gocui.View) error {
-	// Note: this is very hacky
-
 	ui.msgPopup.Activate("Pull from remote "+defaultRemote, "...")
 
 	go func() {
@@ -457,19 +455,19 @@ func (bt *bugTable) pull(g *gocui.Gui, v *gocui.View) error {
 		var buffer bytes.Buffer
 		beginLine := ""
 
-		for merge := range bt.repo.MergeAll(defaultRemote) {
-			if merge.Status == bug.MergeStatusNothing {
+		for result := range bt.repo.MergeAll(defaultRemote) {
+			if result.Status == entity.MergeStatusNothing {
 				continue
 			}
 
-			if merge.Err != nil {
+			if result.Err != nil {
 				g.Update(func(gui *gocui.Gui) error {
 					ui.msgPopup.Activate(msgPopupErrorTitle, err.Error())
 					return nil
 				})
 			} else {
 				_, _ = fmt.Fprintf(&buffer, "%s%s: %s",
-					beginLine, colors.Cyan(merge.Bug.HumanId()), merge,
+					beginLine, colors.Cyan(result.Entity.HumanId()), result,
 				)
 
 				beginLine = "\n"