From cf89d542121912a23e5c1ab830f7b7d838cd1161 Mon Sep 17 00:00:00 2001 From: Matthew Rothenberg Date: Thu, 23 May 2019 16:42:30 -0400 Subject: [PATCH 1/2] 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 | 110 ++++++++++++++++++-------------- commands/status/process_test.go | 16 ++++- commands/status/status.go | 5 +- 3 files changed, 80 insertions(+), 51 deletions(-) diff --git a/commands/status/process.go b/commands/status/process.go index d98d675..6b2e7cf 100644 --- a/commands/status/process.go +++ b/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,7 +213,7 @@ 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) @@ -205,7 +221,7 @@ func extractFile(chunk []byte, root, wd string) (absPath, relPath string) { 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 diff --git a/commands/status/process_test.go b/commands/status/process_test.go index 012f481..353d8ec 100644 --- a/commands/status/process_test.go +++ b/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) diff --git a/commands/status/status.go b/commands/status/status.go index ce34b57..df90c07 100644 --- a/commands/status/status.go +++ b/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) }, } From 8c55c0d16aeb743550522098ebf36846c89ee384 Mon Sep 17 00:00:00 2001 From: Matthew Rothenberg Date: Thu, 23 May 2019 18:19:14 -0400 Subject: [PATCH 2/2] fix(status): underlying slice corruption issue I believe this should fix issues #26 #34 and #35. --- commands/status/process.go | 14 +++++++++++--- commands/status/process_test.go | 25 +++++++++++++++++++++++++ 2 files changed, 36 insertions(+), 3 deletions(-) diff --git a/commands/status/process.go b/commands/status/process.go index 6b2e7cf..cb4163b 100644 --- a/commands/status/process.go +++ b/commands/status/process.go @@ -153,9 +153,17 @@ func ProcessChanges(s *bufio.Scanner, root string) ([]*StatusItem, error) { // ...if chunk represents a rename or copy op, need to append another chunk // to get the full change item, with NUL manually reinserted because scanner // will extract past it. - if (chunk[0] == 'R' || chunk[0] == 'C') && s.Scan() { - chunk = append(chunk, '\x00') - chunk = append(chunk, s.Bytes()...) + // + // Note that the underlying slice from previous scanner.Bytes() MAY be + // overridden by subsequent scans, so need to copy it to a new slice + // first before scanning to get the next token. + if chunk[0] == 'R' || chunk[0] == 'C' { + composite := make([]byte, len(chunk)) + copy(composite, chunk) + s.Scan() + composite = append(composite, '\x00') + composite = append(composite, s.Bytes()...) + chunk = composite } statuses, err := processChange(chunk, wd, root) if err != nil { diff --git a/commands/status/process_test.go b/commands/status/process_test.go index 353d8ec..7b76bdc 100644 --- a/commands/status/process_test.go +++ b/commands/status/process_test.go @@ -277,3 +277,28 @@ func TestExtractBranch(t *testing.T) { }) } } + +/* +// Test to verify https://github.com/mroth/scmpuff/issues/26. +// +// Leaving commented out since unlikely to encounter this exact issue again in +// future, and I'm not sure about importing the user's datafile into this repo. +// +// If needed again, the data file is attached to the issue as `output.txt`. + +func TestBrokenProcessChanges(t *testing.T) { + dat, err := ioutil.ReadFile("testdata/cjfuller_sample.dat") + if err != nil { + t.Fatal(err) + } + s := bufio.NewScanner(bytes.NewReader(dat)) + s.Split(nulSplitFunc) + actual, err := ProcessChanges(s, "") + if err != nil { + t.Fatal(err) + } + if len(actual) != 270 { // `git status -s | wc -l` in replicated repo + t.Errorf("expected %v changes, got %v", 270, len(actual)) + } +} +*/