refactor(status): move process error handling up

This was originally written early in my Go programming days, and as a
CLI tool there are a lot of log.Fatals deep in the stack showing for
reporting fatal parsing conditions to the user. But, this makes writing
new tests a bit more difficult.

In preparation for starting to debug the process parsing bugs, extract
the error handling to be more library like.

commands/status/process.go

@@ -3,8 +3,8 @@ package status
 import (
 	"bufio"
 	"bytes"
+	"errors"
 	"fmt"
-	"log"
 	"os"
 	"path/filepath"
 	"regexp"
@@ -13,39 +13,48 @@ import (
 
 // Process takes the raw output of `git status --porcelain -b -z` and turns it
 // into a structured data type.
-func Process(gitStatusOutput []byte, root string) *StatusList {
+func Process(gitStatusOutput []byte, root string) (*StatusList, error) {
 	// initialize a statuslist to hold the results
 	results := NewStatusList()
 
-	// put the output into a bufferreader+scanner so we can consume it iteratively
+	// put output into bufferreader+scanner so we can consume it iteratively
 	scanner := bufio.NewScanner(bytes.NewReader(gitStatusOutput))
-
 	// the scanner needs a custom split function for splitting on NUL
-	scanNul := func(data []byte, atEOF bool) (advance int, token []byte, err error) {
-		for i, b := range data {
-			if b == '\x00' {
-				return i + 1, data[:i], nil
-			}
-		}
-		return 0, nil, nil
-	}
-	scanner.Split(scanNul)
+	scanner.Split(nulSplitFunc)
 
 	// branch output is first line
 	if !scanner.Scan() {
-		log.Println("Failed to read buffer when expecting branch status")
-		log.Fatal(scanner.Err())
+		return nil, fmt.Errorf("Failed to read buffer when expecting branch status: %v", scanner.Err())
 	}
 	branchBytes := scanner.Bytes()
-	results.branch = ExtractBranch(branchBytes)
+	branch, err := ExtractBranch(branchBytes)
+	if err != nil {
+		return nil, err
+	}
+	results.branch = branch
 
 	// give ProcessChanges the scanner and let it handle the rest
 	// (it does complicated stuff so it needs the entire scanner)
-	for _, r := range ProcessChanges(scanner, root) {
+	statuses, err := ProcessChanges(scanner, root)
+	if err != nil {
+		return nil, err
+	}
+	// put the results in the proper group
+	for _, r := range statuses {
 		results.groups[r.group].items = append(results.groups[r.group].items, r)
 	}
 
-	return results
+	return results, nil
+}
+
+// custom split function for splitting on NUL
+func nulSplitFunc(data []byte, atEOF bool) (advance int, token []byte, err error) {
+	for i, b := range data {
+		if b == '\x00' {
+			return i + 1, data[:i], nil
+		}
+	}
+	return 0, nil, nil
 }
 
 // ExtractBranch handles parsing the branch status from `status --porcelain -b`.
@@ -57,34 +66,34 @@ func Process(gitStatusOutput []byte, root string) *StatusList {
 // 		## master...origin/master
 // 		## master...origin/master [ahead 1]
 //
-func ExtractBranch(bs []byte) *BranchInfo {
-	b := BranchInfo{}
-
-	b.name = decodeBranchName(bs)
-	b.ahead, b.behind = decodeBranchPosition(bs)
+func ExtractBranch(bs []byte) (*BranchInfo, error) {
+	name, err := decodeBranchName(bs)
+	if err != nil {
+		return nil, err
+	}
+	a, b := decodeBranchPosition(bs)
 
-	return &b
+	return &BranchInfo{
+		name:   name,
+		ahead:  a,
+		behind: b,
+	}, nil
 }
 
-func decodeBranchName(bs []byte) (branch string) {
+func decodeBranchName(bs []byte) (string, error) {
 	branchRegex := regexp.MustCompile(`^## (?:Initial commit on )?(?:No commits yet on )?(\S+?)(?:\.{3}|$)`)
-	headRegex := regexp.MustCompile(`^## (HEAD \(no branch\))`)
-
 	branchMatch := branchRegex.FindSubmatch(bs)
 	if branchMatch != nil {
-		branch = string(branchMatch[1])
+		return string(branchMatch[1]), nil
 	}
 
+	headRegex := regexp.MustCompile(`^## (HEAD \(no branch\))`)
 	headMatch := headRegex.FindSubmatch(bs)
 	if headMatch != nil {
-		branch = string(headMatch[1])
+		return string(headMatch[1]), nil
 	}
 
-	if headMatch == nil && branchMatch == nil {
-		log.Fatalf("Failed to parse branch name for output: [%s]", bs)
-	}
-
-	return
+	return "", fmt.Errorf("Failed to parse branch name for output: [%s]", bs)
 }
 
 func decodeBranchPosition(bs []byte) (ahead, behind int) {
@@ -127,7 +136,7 @@ until we have consumed a full entry.
 We put up with this because it means no shell escaping, which should mean better
 cross-platform support. Better hope some Windows people end up using it someday!
 */
-func ProcessChanges(s *bufio.Scanner, root string) (results []*StatusItem) {
+func ProcessChanges(s *bufio.Scanner, root string) ([]*StatusItem, error) {
 
 	// Before we process any changes, get the Current Working Directory.
 	// We're going to need use to calculate absolute and relative filepaths for
@@ -138,6 +147,7 @@ func ProcessChanges(s *bufio.Scanner, root string) (results []*StatusItem) {
 		wd = root
 	}
 
+	var results []*StatusItem
 	for s.Scan() {
 		chunk := s.Bytes()
 		// ...if chunk represents a rename or copy op, need to append another chunk
@@ -147,10 +157,13 @@ func ProcessChanges(s *bufio.Scanner, root string) (results []*StatusItem) {
 			chunk = append(chunk, '\x00')
 			chunk = append(chunk, s.Bytes()...)
 		}
-		results = append(results, processChange(chunk, wd, root)...)
+		statuses, err := processChange(chunk, wd, root)
+		if err != nil {
+			return results, err
+		}
+		results = append(results, statuses...)
 	}
-
-	return
+	return results, nil
 }
 
 // process change for a single item from a `git status -z`.
@@ -162,23 +175,26 @@ func ProcessChanges(s *bufio.Scanner, root string) (results []*StatusItem) {
 // See ProcessChanges (plural) for more details on that process.
 //
 // Note some change items can have multiple statuses, so this returns a slice.
-func processChange(chunk []byte, wd, root string) (results []*StatusItem) {
-
-	absolutePath, relativePath := extractFile(chunk, root, wd)
+func processChange(chunk []byte, wd, root string) ([]*StatusItem, error) {
+	var results []*StatusItem
+	absolutePath, relativePath, err := extractFile(chunk, root, wd)
+	if err != nil {
+		return nil, err
+	}
 
 	for _, c := range extractChangeCodes(chunk) {
-		result := &StatusItem{
+		r := &StatusItem{
 			msg:         c.msg,
 			col:         c.col,
 			group:       c.group,
 			fileAbsPath: absolutePath,
 			fileRelPath: relativePath,
 		}
-		results = append(results, result)
+		results = append(results, r)
 	}
 
 	if len(results) < 1 {
-		log.Fatalf(`
+		return nil, fmt.Errorf(`
 Failed to decode git status change code for chunk: [%s]
 Please file a bug including this error message as well as the output of:
 
@@ -187,7 +203,7 @@ git status --porcelain
 You can file the bug at: https://github.com/mroth/scmpuff/issues/
 		`, chunk)
 	}
-	return results
+	return results, nil
 }
 
 /*
@@ -197,15 +213,15 @@ absolute and display paths.
  - root: the absolute path to the git working tree
  - wd: current working directory path
 */
-func extractFile(chunk []byte, root, wd string) (absPath, relPath string) {
+func extractFile(chunk []byte, root, wd string) (absPath, relPath string, err error) {
 	// file identifier starts at pos4 and continues to EOL
 	filePortion := chunk[3:]
 	files := bytes.SplitN(filePortion, []byte{'\x00'}, 2)
 
 	n := len(files)
 	switch {
 	case n < 1:
-		log.Fatalf("tried to process a change chunk with no file")
+		err = errors.New("tried to process a change chunk with no file")
 	case n > 1:
 		toFile, fromFile := files[0], files[1]
 		var toRelPath, fromRelPath string

commands/status/process_test.go

@@ -11,7 +11,11 @@ import (
 // actual cases in more specific methods
 func TestProcessChange(t *testing.T) {
 	chunk := []byte("A  HELLO.md")
-	actual := processChange(chunk, "/tmp", "/tmp")[0]
+	res, err := processChange(chunk, "/tmp", "/tmp")
+	actual := res[0]
+	if err != nil {
+		t.Fatal(err)
+	}
 
 	expectedChange := &change{
 		msg:   "  new file",
@@ -114,7 +118,10 @@ var testCasesExtractFile = []struct {
 func TestExtractFile(t *testing.T) {
 	for _, tc := range testCasesExtractFile {
 		t.Run(fmt.Sprintf("[root:%s],[wd:%s]", tc.root, tc.wd), func(t *testing.T) {
-			actualAbs, actualRel := extractFile(tc.chunk, tc.root, tc.wd)
+			actualAbs, actualRel, err := extractFile(tc.chunk, tc.root, tc.wd)
+			if err != nil {
+				t.Fatal(err)
+			}
 
 			if actualAbs != tc.expectedAbs {
 				t.Fatalf(
@@ -259,7 +266,10 @@ var testCasesExtractBranch = []struct {
 func TestExtractBranch(t *testing.T) {
 	for _, tc := range testCasesExtractBranch {
 		t.Run(string(tc.chunk[:]), func(t *testing.T) {
-			actual := ExtractBranch(tc.chunk)
+			actual, err := ExtractBranch(tc.chunk)
+			if err != nil {
+				t.Fatal(err)
+			}
 			if !reflect.DeepEqual(actual, tc.expected) {
 				t.Fatalf("processBranch('%s'): expected %v, actual %v",
 					tc.chunk, tc.expected, actual)

commands/status/status.go

@@ -38,7 +38,10 @@ see 'scmpuff init'.)
 			root := gitProjectRoot()
 			status := gitStatusOutput()
 
-			results := Process(status, root)
+			results, err := Process(status, root)
+			if err != nil {
+				log.Fatal(err)
+			}
 			results.printStatus(optsFilelist, optsDisplay)
 		},
 	}