bug: refactor the Pull code to have the message formating in the upper layers

Michael Muré created 7 years ago

Change summary

bug/bug_actions.go             | 26 +++++---------------------
bug/operations/label_change.go |  1 +
cache/repo_cache.go            |  6 ------
commands/pull.go               | 26 ++++++++++++++++++++++++--
tests/bug_actions_test.go      | 18 +++++++++---------
5 files changed, 39 insertions(+), 38 deletions(-)

Detailed changes

bug/bug_actions.go

@@ -2,8 +2,6 @@ package bug
 
 import (
 	"fmt"
-	"io"
-	"io/ioutil"
 	"strings"
 
 	"github.com/MichaelMure/git-bug/repository"
@@ -28,33 +26,19 @@ func Push(repo repository.Repo, remote string) (string, error) {
 	return repo.PushRefs(remote, bugsRefPattern+"*")
 }
 
-// Pull does a Fetch and merge the updates into the local bug states
-func Pull(repo repository.Repo, out io.Writer, remote string) error {
-	// TODO: return a chan of changes for the cache to be updated properly
-
-	if out == nil {
-		out = ioutil.Discard
-	}
-
-	fmt.Fprintf(out, "Fetching remote ...\n")
-
-	stdout, err := Fetch(repo, remote)
+// Pull will do a Fetch + MergeAll
+// This function won't give details on the underlying process. If you need more
+// use Fetch and MergeAll separately.
+func Pull(repo repository.Repo, remote string) error {
+	_, err := Fetch(repo, remote)
 	if err != nil {
 		return err
 	}
 
-	out.Write([]byte(stdout))
-
-	fmt.Fprintf(out, "Merging data ...\n")
-
 	for merge := range MergeAll(repo, remote) {
 		if merge.Err != nil {
 			return merge.Err
 		}
-
-		if merge.Status != MsgMergeNothing {
-			fmt.Fprintf(out, "%s: %s\n", merge.HumanId, merge.Status)
-		}
 	}
 
 	return nil

bug/operations/label_change.go

@@ -61,6 +61,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.Interface, author bug.Person, add, remove []string) error {
+	// TODO: return a channel of result (like MergeAll) instead of formatting the result for the upper layers
 	var added, removed []bug.Label
 
 	if out == nil {

cache/repo_cache.go

@@ -266,12 +266,6 @@ func (c *RepoCache) MergeAll(remote string) <-chan bug.MergeResult {
 	return bug.MergeAll(c.repo, remote)
 }
 
-// Pull does a Fetch and merge the updates into the local bug states
-func (c *RepoCache) Pull(remote string, out io.Writer) error {
-	// Todo: update the cache properly
-	return bug.Pull(c.repo, out, remote)
-}
-
 // Push update a remote with the local changes
 func (c *RepoCache) Push(remote string) (string, error) {
 	return bug.Push(c.repo, remote)

commands/pull.go

@@ -2,8 +2,9 @@ package commands
 
 import (
 	"errors"
-	"os"
+	"fmt"
 
+	"github.com/MichaelMure/git-bug/bug"
 	"github.com/MichaelMure/git-bug/cache"
 	"github.com/spf13/cobra"
 )
@@ -24,7 +25,28 @@ func runPull(cmd *cobra.Command, args []string) error {
 	}
 	defer backend.Close()
 
-	return backend.Pull(remote, os.Stdout)
+	fmt.Println("Fetching remote ...")
+
+	stdout, err := backend.Fetch(remote)
+	if err != nil {
+		return err
+	}
+
+	fmt.Println(stdout)
+
+	fmt.Println("Merging data ...")
+
+	for merge := range backend.MergeAll(remote) {
+		if merge.Err != nil {
+			return merge.Err
+		}
+
+		if merge.Status != bug.MsgMergeNothing {
+			fmt.Printf("%s: %s\n", merge.HumanId, merge.Status)
+		}
+	}
+
+	return nil
 }
 
 // showCmd defines the "push" subcommand.

tests/bug_actions_test.go

@@ -80,7 +80,7 @@ func TestPushPull(t *testing.T) {
 	_, err = bug.Push(repoA, "origin")
 	checkErr(t, err)
 
-	err = bug.Pull(repoB, nil, "origin")
+	err = bug.Pull(repoB, "origin")
 	checkErr(t, err)
 
 	bugs := allBugs(t, bug.ReadAllLocalBugs(repoB))
@@ -98,7 +98,7 @@ func TestPushPull(t *testing.T) {
 	_, err = bug.Push(repoB, "origin")
 	checkErr(t, err)
 
-	err = bug.Pull(repoA, nil, "origin")
+	err = bug.Pull(repoA, "origin")
 	checkErr(t, err)
 
 	bugs = allBugs(t, bug.ReadAllLocalBugs(repoA))
@@ -149,7 +149,7 @@ func _RebaseTheirs(t testing.TB) {
 	checkErr(t, err)
 
 	// remote --> B
-	err = bug.Pull(repoB, nil, "origin")
+	err = bug.Pull(repoB, "origin")
 	checkErr(t, err)
 
 	bug2, err := bug.ReadLocalBug(repoB, bug1.Id())
@@ -166,7 +166,7 @@ func _RebaseTheirs(t testing.TB) {
 	checkErr(t, err)
 
 	// remote --> A
-	err = bug.Pull(repoA, nil, "origin")
+	err = bug.Pull(repoA, "origin")
 	checkErr(t, err)
 
 	bugs := allBugs(t, bug.ReadAllLocalBugs(repoB))
@@ -207,7 +207,7 @@ func _RebaseOurs(t testing.TB) {
 	checkErr(t, err)
 
 	// remote --> B
-	err = bug.Pull(repoB, nil, "origin")
+	err = bug.Pull(repoB, "origin")
 	checkErr(t, err)
 
 	operations.Comment(bug1, rene, "message2")
@@ -229,7 +229,7 @@ func _RebaseOurs(t testing.TB) {
 	checkErr(t, err)
 
 	// remote --> A
-	err = bug.Pull(repoA, nil, "origin")
+	err = bug.Pull(repoA, "origin")
 	checkErr(t, err)
 
 	bugs := allBugs(t, bug.ReadAllLocalBugs(repoA))
@@ -279,7 +279,7 @@ func _RebaseConflict(t testing.TB) {
 	checkErr(t, err)
 
 	// remote --> B
-	err = bug.Pull(repoB, nil, "origin")
+	err = bug.Pull(repoB, "origin")
 	checkErr(t, err)
 
 	operations.Comment(bug1, rene, "message2")
@@ -326,7 +326,7 @@ func _RebaseConflict(t testing.TB) {
 	checkErr(t, err)
 
 	// remote --> B
-	err = bug.Pull(repoB, nil, "origin")
+	err = bug.Pull(repoB, "origin")
 	checkErr(t, err)
 
 	bugs := allBugs(t, bug.ReadAllLocalBugs(repoB))
@@ -347,7 +347,7 @@ func _RebaseConflict(t testing.TB) {
 	checkErr(t, err)
 
 	// remote --> A
-	err = bug.Pull(repoA, nil, "origin")
+	err = bug.Pull(repoA, "origin")
 	checkErr(t, err)
 
 	bugs = allBugs(t, bug.ReadAllLocalBugs(repoA))