From ccf0c58db89542d48dd8c5c7d55433709c10cfce Mon Sep 17 00:00:00 2001 From: Billy Keyes Date: Sun, 24 Mar 2019 22:03:53 -0700 Subject: [PATCH] Change parser interface to be more iterator-like Call Next() to advance the parser state until it returns a non-nil error, then check if the error is io.EOF. This makes EOF handling easier and also means that Line() and PeekLine() can be called multiple times without changing state. The next step is to update parse functions to return an internal marker error if they are called on an invalid line. This should improve correctness and remove the duplication of testing a condition and then calling a parse function, which checks the same condition. --- gitdiff/file_header.go | 14 ++---- gitdiff/file_header_test.go | 4 +- gitdiff/parser.go | 98 +++++++++++++++++++++---------------- gitdiff/parser_test.go | 41 +++++++--------- 4 files changed, 80 insertions(+), 77 deletions(-) diff --git a/gitdiff/file_header.go b/gitdiff/file_header.go index e6c850a..d848e5c 100644 --- a/gitdiff/file_header.go +++ b/gitdiff/file_header.go @@ -16,15 +16,8 @@ func (p *parser) ParseGitFileHeader(f *File, header string) error { return p.Errorf(0, "git file header: %v", err) } - for { - line, err := p.PeekLine() - if err == io.EOF { - break - } else if err != nil { - return err - } - - end, err := parseGitHeaderData(f, line, defaultName) + for err = p.Next(); err == nil; err = p.Next() { + end, err := parseGitHeaderData(f, p.Line(), defaultName) if err != nil { return p.Errorf(1, "git file header: %v", err) } @@ -33,6 +26,9 @@ func (p *parser) ParseGitFileHeader(f *File, header string) error { } p.Line() } + if err != nil && err != io.EOF { + return err + } if f.OldName == "" && f.NewName == "" { if defaultName == "" { diff --git a/gitdiff/file_header_test.go b/gitdiff/file_header_test.go index 199de29..5587cbb 100644 --- a/gitdiff/file_header_test.go +++ b/gitdiff/file_header_test.go @@ -144,10 +144,10 @@ index deadbeef for name, test := range tests { t.Run(name, func(t *testing.T) { p := &parser{r: bufio.NewReader(strings.NewReader(test.Input))} - header, _ := p.Line() + p.Next() var f File - err := p.ParseGitFileHeader(&f, header) + err := p.ParseGitFileHeader(&f, p.Line()) if test.Err { if err == nil { t.Fatalf("expected error parsing git file header, got nil") diff --git a/gitdiff/parser.go b/gitdiff/parser.go index 7fea17a..ebef5b5 100644 --- a/gitdiff/parser.go +++ b/gitdiff/parser.go @@ -40,9 +40,11 @@ func Parse(r io.Reader) (files []*File, err error) { // by allowing users to set or override defaults type parser struct { - r *bufio.Reader - lineno int64 - nextLine string + r *bufio.Reader + + eof bool + lineno int64 + lines [2]string } const ( @@ -60,17 +62,8 @@ const ( func (p *parser) ParseNextFileHeader() (file *File, err error) { // based on find_header() in git/apply.c - defer func() { - if err == io.EOF && file == nil { - err = nil - } - }() - - for { - line, err := p.Line() - if err != io.EOF { - return nil, err - } + for err = p.Next(); err == nil; err = p.Next() { + line := p.Line() // check for disconnected fragment headers (corrupt patch) if isMaybeFragmentHeader(line) { @@ -91,23 +84,17 @@ func (p *parser) ParseNextFileHeader() (file *File, err error) { return file, nil } - next, err := p.PeekLine() - if err != nil { - return nil, err - } - // check for a "traditional" patch - if strings.HasPrefix(line, oldFilePrefix) && strings.HasPrefix(next, newFilePrefix) { - oldFileLine := line - newFileLine, _ := p.Line() - - next, err := p.PeekLine() - if err != nil { + if strings.HasPrefix(line, oldFilePrefix) && strings.HasPrefix(p.PeekLine(), newFilePrefix) { + if err = p.Next(); err != nil { return nil, err } + oldFileLine := line + newFileLine := p.Line() + // only a file header if followed by a (probable) unified fragment header - if !isMaybeFragmentHeader(next) { + if !isMaybeFragmentHeader(p.PeekLine()) { continue } @@ -118,6 +105,11 @@ func (p *parser) ParseNextFileHeader() (file *File, err error) { return file, nil } } + + if err != nil && err != io.EOF { + return nil, err + } + return file, nil } // ParseFileChanges parses file changes until the next file header or the end @@ -126,28 +118,48 @@ func (p *parser) ParseFileChanges(f *File) error { panic("TODO(bkeyes): unimplemented") } -// Line reads and returns the next line. The first call to Line after a call to -// PeekLine will never retrun an error. -func (p *parser) Line() (line string, err error) { - if p.nextLine != "" { - line = p.nextLine - p.nextLine = "" - } else { - line, err = p.r.ReadString('\n') +// Next advances the parser by one line. It returns any error encountered while +// reading the line, including io.EOF when the end of stream is reached. +func (p *parser) Next() error { + if p.eof { + p.lines[0] = "" + return io.EOF } + + if p.lineno == 0 { + // on the first call, need extra shift to initialize both slots + if err := p.shiftLines(); err != nil && err != io.EOF { + return err + } + } + + err := p.shiftLines() + if err == io.EOF { + p.eof = p.lines[1] == "" + } else if err != nil { + return err + } + p.lineno++ + return nil +} + +func (p *parser) shiftLines() (err error) { + p.lines[0] = p.lines[1] + p.lines[1], err = p.r.ReadString('\n') return } -// PeekLine reads and returns the next line without advancing the parser. -func (p *parser) PeekLine() (line string, err error) { - if p.nextLine != "" { - line = p.nextLine - } else { - line, err = p.r.ReadString('\n') - } - p.nextLine = line - return +// Line returns the current line or an empty string if Next has returned io.EOF. +func (p *parser) Line() string { + return p.lines[0] +} + +// PeekLine returns the line following the current line or an empty string if +// the current line is the final line. If PeekLine returns an empty string, +// Next will return io.EOF on the next call. +func (p *parser) PeekLine() string { + return p.lines[1] } // Errorf generates an error and appends the current line information. diff --git a/gitdiff/parser_test.go b/gitdiff/parser_test.go index 807d10b..6aef8e3 100644 --- a/gitdiff/parser_test.go +++ b/gitdiff/parser_test.go @@ -17,19 +17,21 @@ func TestLineOperations(t *testing.T) { t.Run("readLine", func(t *testing.T) { p := newParser() - line, err := p.Line() - if err != nil { - t.Fatalf("error reading first line: %v", err) + if err := p.Next(); err != nil { + t.Fatalf("error advancing parser: %v", err) } + + line := p.Line() if line != "the first line\n" { t.Fatalf("incorrect first line: %s", line) } - line, err = p.Line() - if err != nil { - t.Fatalf("error reading second line: %v", err) + if err := p.Next(); err != nil { + t.Fatalf("error advancing parser: %v", err) } - if line != "the second line\n" { + + line = p.Line() + if p.Line() != "the second line\n" { t.Fatalf("incorrect second line: %s", line) } }) @@ -37,29 +39,22 @@ func TestLineOperations(t *testing.T) { t.Run("peekLine", func(t *testing.T) { p := newParser() - line, err := p.PeekLine() - if err != nil { - t.Fatalf("error peeking line: %v", err) - } - if line != "the first line\n" { - t.Fatalf("incorrect peek line: %s", line) + if err := p.Next(); err != nil { + t.Fatalf("error advancing parser: %v", err) } - // test that a second peek returns the same value - line, err = p.PeekLine() - if err != nil { - t.Fatalf("error peeking line: %v", err) - } - if line != "the first line\n" { + line := p.PeekLine() + if line != "the second line\n" { t.Fatalf("incorrect peek line: %s", line) } // test that reading the line returns the same value - line, err = p.Line() - if err != nil { - t.Fatalf("error reading line: %v", err) + if err := p.Next(); err != nil { + t.Fatalf("error advancing parser: %v", err) } - if line != "the first line\n" { + + line = p.Line() + if line != "the second line\n" { t.Fatalf("incorrect line: %s", line) } })