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

@@ -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 == "" {

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")

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.

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)
 		}
 	})