Merge pull request #12 from grdl/error_handling

Improve error messages

go.mod

@@ -10,7 +10,7 @@ require (
 	github.com/pkg/errors v0.9.1
 	github.com/spf13/cobra v1.0.0
 	github.com/spf13/viper v1.7.0
-	github.com/stretchr/testify v1.4.0 // indirect
+	github.com/stretchr/testify v1.4.0
 	github.com/xlab/treeprint v1.0.0
 	golang.org/x/sys v0.0.0-20200302150141-5c8b2ff67527 // indirect
 	gopkg.in/check.v1 v1.0.0-20200227125254-8fa46927fb4f // indirect

pkg/git/finder.go

@@ -16,8 +16,8 @@ import (
 // It's handled by ErrorsCallback to tell the WalkCallback to skip this dir.
 var errSkipNode = errors.New(".git directory found, skipping this node")
 
-// errDirectoryAccess indicates a directory doesn't exists or can't be accessed
+var errDirNoAccess = errors.New("directory can't be accessed")
-var errDirectoryAccess = errors.New("directory doesn't exist or can't be accessed")
+var errDirNotExist = errors.New("directory doesn't exist")
 
 // Exists returns true if a directory exists. If it doesn't or the directory can't be accessed it returns an error.
 func Exists(path string) (bool, error) {
@@ -29,19 +29,18 @@ func Exists(path string) (bool, error) {
 
 	if err != nil {
 		if os.IsNotExist(err) {
-			return false, errDirectoryAccess
+			return false, errors.Wrapf(errDirNotExist, "can't access %s", path)
 		}
 	}
 
 	// Directory exists but can't be accessed
-	return true, errDirectoryAccess
+	return true, errors.Wrapf(errDirNoAccess, "can't access %s", path)
 }
 
 // RepoFinder finds git repositories inside a given path.
 type RepoFinder struct {
-	root   string
+	root  string
-	repos  []*Repo
+	repos []*Repo
-	errors []error
 }
 
 // NewRepoFinder returns a RepoFinder pointed at given root path.
@@ -128,15 +127,15 @@ func (f *RepoFinder) walkCb(path string, ent *godirwalk.Dirent) error {
 }
 
 // addIfOk adds the found repo to the repos slice if it can be opened.
-// If repo path can't be accessed it will add an error to the errors slice.
 func (f *RepoFinder) addIfOk(path string) {
-	repo, err := Open(strings.TrimSuffix(path, dotgit))
+	// TODO: is the case below really correct? What if there's a race condition and the dir becomes unaccessible between finding it and opening?
-	if err != nil {
-		f.errors = append(f.errors, err)
-		return
-	}
 
-	f.repos = append(f.repos, repo)
+	// Open() should never return an error here. If a finder found a .git inside this dir, it means it could open and access it.
+	// If the dir was unaccessible, then it would have been skipped by the check in errorCb().
+	repo, err := Open(strings.TrimSuffix(path, dotgit))
+	if err == nil {
+		f.repos = append(f.repos, repo)
+	}
 }
 
 func (f *RepoFinder) errorCb(_ string, err error) godirwalk.ErrorAction {

pkg/git/finder_test.go

@@ -1,8 +1,11 @@
 package git
 
 import (
+	"errors"
 	"git-get/pkg/git/test"
 	"testing"
+
+	"github.com/stretchr/testify/assert"
 )
 
 func TestFinder(t *testing.T) {
@@ -15,18 +18,15 @@ func TestFinder(t *testing.T) {
 			name:       "no repos",
 			reposMaker: makeNoRepos,
 			want:       0,
-		},
+		}, {
-		{
 			name:       "single repos",
 			reposMaker: makeSingleRepo,
 			want:       1,
-		},
+		}, {
-		{
 			name:       "single nested repo",
 			reposMaker: makeNestedRepo,
 			want:       1,
-		},
+		}, {
-		{
 			name:       "multiple nested repo",
 			reposMaker: makeMultipleNestedRepos,
 			want:       2,
@@ -40,9 +40,38 @@ func TestFinder(t *testing.T) {
 			finder := NewRepoFinder(root)
 			finder.Find()
 
-			if len(finder.repos) != test.want {
+			assert.Len(t, finder.repos, test.want)
-				t.Errorf("expected %d; got %d", test.want, len(finder.repos))
+		})
-			}
+	}
+}
+
+// TODO: this test will only work on Linux
+func TestExists(t *testing.T) {
+	tests := []struct {
+		name string
+		path string
+		want error
+	}{
+		{
+			name: "dir does not exist",
+			path: "/this/directory/does/not/exist",
+			want: errDirNotExist,
+		}, {
+			name: "dir cant be accessed",
+			path: "/root/some/directory",
+			want: errDirNoAccess,
+		}, {
+			name: "dir exists",
+			path: "/tmp/",
+			want: nil,
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			_, err := Exists(test.path)
+
+			assert.True(t, errors.Is(err, test.want))
 		})
 	}
 }

pkg/git/repo_test.go

@@ -6,14 +6,6 @@ import (
 	"testing"
 )
 
-func TestOpen(t *testing.T) {
-	_, err := Open("/paththatdoesnotexist/repo")
-
-	if err != errDirectoryAccess {
-		t.Errorf("Opening a repo in non existing path should throw an error")
-	}
-}
-
 func TestUncommitted(t *testing.T) {
 	tests := []struct {
 		name      string

pkg/list.go

@@ -30,11 +30,11 @@ func List(c *ListCfg) error {
 
 	switch c.Output {
 	case cfg.OutFlat:
-		fmt.Println(print.NewFlatPrinter().Print(printables))
+		fmt.Print(print.NewFlatPrinter().Print(printables))
 	case cfg.OutTree:
-		fmt.Println(print.NewTreePrinter().Print(c.Root, printables))
+		fmt.Print(print.NewTreePrinter().Print(c.Root, printables))
 	case cfg.OutDump:
-		fmt.Println(print.NewDumpPrinter().Print(printables))
+		fmt.Print(print.NewDumpPrinter().Print(printables))
 	default:
 		return fmt.Errorf("invalid --out flag; allowed values: [%s]", strings.Join(cfg.AllowedOut, ", "))
 	}

pkg/print/dump.go

@@ -17,7 +17,7 @@ func NewDumpPrinter() *DumpPrinter {
 func (p *DumpPrinter) Print(repos []Printable) string {
 	var str strings.Builder
 
-	for i, r := range repos {
+	for _, r := range repos {
 		str.WriteString(r.Remote())
 
 		// TODO: if head is detached maybe we should get the revision it points to in case it's a tag
@@ -25,9 +25,7 @@ func (p *DumpPrinter) Print(repos []Printable) string {
 			str.WriteString(" " + current)
 		}
 
-		if i < len(repos)-1 {
+		str.WriteString("\n")
-			str.WriteString("\n")
-		}
 	}
 
 	return str.String()

pkg/print/flat.go

@@ -18,8 +18,14 @@ func NewFlatPrinter() *FlatPrinter {
 func (p *FlatPrinter) Print(repos []Printable) string {
 	var str strings.Builder
 
-	for i, r := range repos {
+	for _, r := range repos {
 		str.WriteString(strings.TrimSuffix(r.Path(), string(os.PathSeparator)))
+
+		if len(r.Errors()) > 0 {
+			str.WriteString(" " + red("error") + "\n")
+			continue
+		}
+
 		str.WriteString(" " + blue(r.Current()))
 
 		current := r.BranchStatus(r.Current())
@@ -45,10 +51,8 @@ func (p *FlatPrinter) Print(repos []Printable) string {
 			str.WriteString(fmt.Sprintf("\n%s %s %s", indent, blue(branch), yellow(status)))
 		}
 
-		if i < len(repos)-1 {
+		str.WriteString("\n")
-			str.WriteString("\n")
-		}
 	}
 
-	return str.String()
+	return str.String() + Errors(repos)
 }

pkg/print/print.go

@@ -1,6 +1,9 @@
 package print
 
-import "fmt"
+import (
+	"fmt"
+	"strings"
+)
 
 const (
 	head = "HEAD"
@@ -17,6 +20,28 @@ type Printable interface {
 	Errors() []string
 }
 
+// Errors returns a printable list of errors from the slice of Printables or an empty string if there are no errors.
+// It's meant to be appended at the end of Print() result.
+func Errors(repos []Printable) string {
+	errors := []string{}
+
+	for _, repo := range repos {
+		for _, err := range repo.Errors() {
+			errors = append(errors, err)
+		}
+	}
+
+	if len(errors) == 0 {
+		return ""
+	}
+
+	var str strings.Builder
+	str.WriteString(red("\nOops, errors happened when loading repository status:\n"))
+	str.WriteString(strings.Join(errors, "\n"))
+
+	return str.String()
+}
+
 // TODO: not sure if this works on windows. See https://github.com/mattn/go-colorable
 func red(str string) string {
 	return fmt.Sprintf("\033[1;31m%s\033[0m", str)

pkg/print/tree.go

@@ -29,7 +29,7 @@ func (p *TreePrinter) Print(root string, repos []Printable) string {
 
 	p.printTree(tree, tp)
 
-	return tp.String()
+	return tp.String() + Errors(repos)
 }
 
 // Node represents a path fragment in repos tree.
@@ -114,34 +114,11 @@ func buildTree(root string, repos []Printable) *Node {
 	return tree
 }
 
+// printTree renders the repo tree by recursively traversing the tree nodes.
+// If a node doesn't have any children, it's a leaf node containing the repo status.
 func (p *TreePrinter) printTree(node *Node, tp treeprint.Tree) {
 	if node.children == nil {
-		r := node.repo
+		tp.SetValue(printLeaf(node))
-		current := r.BranchStatus(r.Current())
-		worktree := r.WorkTreeStatus()
-
-		if worktree != "" {
-			worktree = fmt.Sprintf("[ %s ]", worktree)
-		}
-
-		var str strings.Builder
-
-		if worktree == "" && current == "" {
-			str.WriteString(fmt.Sprintf("%s %s %s", node.val, blue(r.Current()), green("ok")))
-		} else {
-			str.WriteString(fmt.Sprintf("%s %s %s", node.val, blue(r.Current()), strings.Join([]string{yellow(current), red(worktree)}, " ")))
-		}
-
-		for _, branch := range r.Branches() {
-			status := r.BranchStatus(branch)
-			if status == "" {
-				status = green("ok")
-			}
-
-			str.WriteString(fmt.Sprintf("\n%s%s %s", indentation(node), blue(branch), yellow(status)))
-		}
-
-		tp.SetValue(str.String())
 	}
 
 	for _, child := range node.children {
@@ -150,6 +127,42 @@ func (p *TreePrinter) printTree(node *Node, tp treeprint.Tree) {
 	}
 }
 
+func printLeaf(node *Node) string {
+	r := node.repo
+
+	// If any errors happened during status loading, don't print the status but "error" instead.
+	// Actual error messages are printed in bulk below the tree.
+	if len(r.Errors()) > 0 {
+		return fmt.Sprintf("%s %s", node.val, red("error"))
+	}
+
+	current := r.BranchStatus(r.Current())
+	worktree := r.WorkTreeStatus()
+
+	if worktree != "" {
+		worktree = fmt.Sprintf("[ %s ]", worktree)
+	}
+
+	var str strings.Builder
+
+	if worktree == "" && current == "" {
+		str.WriteString(fmt.Sprintf("%s %s %s", node.val, blue(r.Current()), green("ok")))
+	} else {
+		str.WriteString(fmt.Sprintf("%s %s %s", node.val, blue(r.Current()), strings.Join([]string{yellow(current), red(worktree)}, " ")))
+	}
+
+	for _, branch := range r.Branches() {
+		status := r.BranchStatus(branch)
+		if status == "" {
+			status = green("ok")
+		}
+
+		str.WriteString(fmt.Sprintf("\n%s%s %s", indentation(node), blue(branch), yellow(status)))
+	}
+
+	return str.String()
+}
+
 // indentation generates a correct indentation for the branches row to match the links to lower rows.
 // It traverses the tree "upwards" and checks if a parent node is the youngest one (ie, there are no more sibling at the same level).
 // If it is, it means that level should be indented with empty spaces because there is nothing to link to anymore.

pkg/run/run.go

@@ -25,13 +25,16 @@ import (
 // - run.Git("pull").OnRepo(<REPO>).AndShutUp()
 //   means running "git pull" inside <REPO> and not printing any output
 type Cmd struct {
-	cmd *exec.Cmd
+	cmd  *exec.Cmd
+	args string
+	path string
 }
 
 // Git creates a git command with given arguments.
 func Git(args ...string) *Cmd {
 	return &Cmd{
-		cmd: exec.Command("git", args...),
+		cmd:  exec.Command("git", args...),
+		args: strings.Join(args, " "),
 	}
 }
 
@@ -46,6 +49,8 @@ func (c *Cmd) OnRepo(path string) *Cmd {
 	// Insert into the args slice after the 1st element (https://github.com/golang/go/wiki/SliceTricks#insert)
 	c.cmd.Args = append(c.cmd.Args[:1], append(insert, c.cmd.Args[1:]...)...)
 
+	c.path = path
+
 	return c
 }
 
@@ -56,7 +61,7 @@ func (c *Cmd) AndCaptureLines() ([]string, error) {
 
 	out, err := c.cmd.Output()
 	if err != nil {
-		return nil, &GitError{errStream, c.cmd.Args, err}
+		return nil, &GitError{errStream, c.args, c.path, err}
 	}
 
 	lines := lines(out)
@@ -83,7 +88,7 @@ func (c *Cmd) AndShow() error {
 
 	err := c.cmd.Run()
 	if err != nil {
-		return &GitError{&bytes.Buffer{}, c.cmd.Args, err}
+		return &GitError{&bytes.Buffer{}, c.args, c.path, err}
 	}
 	return nil
 }
@@ -97,7 +102,7 @@ func (c *Cmd) AndShutUp() error {
 
 	err := c.cmd.Run()
 	if err != nil {
-		return &GitError{errStream, c.cmd.Args, err}
+		return &GitError{errStream, c.args, c.path, err}
 	}
 	return nil
 }
@@ -105,16 +110,20 @@ func (c *Cmd) AndShutUp() error {
 // GitError provides more visibility into why an git command had failed.
 type GitError struct {
 	Stderr *bytes.Buffer
-	Args   []string
+	Args   string
+	Path   string
 	Err    error
 }
 
 func (e GitError) Error() string {
 	msg := e.Stderr.String()
-	if msg != "" && !strings.HasSuffix(msg, "\n") {
+
-		msg += "\n"
+	if e.Path == "" {
+		return fmt.Sprintf("git %s failed: %s", e.Args, msg)
 	}
-	return fmt.Sprintf("%s%q: %s", msg, strings.Join(e.Args, " "), e.Err)
+
+	return fmt.Sprintf("git %s failed on %s: %s", e.Args, e.Path, msg)
+
 }
 
 func lines(output []byte) []string {