Add tests for ParseTextFragments

Covers that multiple fragments actually works and the unique error
contidions, but otherwise relies on the lower level tests for
correctness. Also update some comments and the README based on
observations from writing/debugging these tests.

Author: bluekeyes

README.md

@@ -34,22 +34,25 @@ In development, most functionality is currently missing, incomplete, or broken.
 
 [apply.c]: https://github.com/git/git/blob/master/apply.c
 
-## Known Issues and Differences From Git
+## Differences From Git
 
-1. Certain types of invalid input that I believe are accepted by `git apply`
-   generate errors. These include:
+1. Certain types of invalid input that are accepted by `git apply` generate
+   errors. These include:
 
    - Numbers immediately followed by non-numeric characters
    - Trailing characters on a line after valid or expected content
 
-2. The translation from C to Go may have introduced inconsistencies in the way
+2. Errors for invalid input are generally more verbose and specific than those
+   from `git apply`.
+
+3. The translation from C to Go may have introduced inconsistencies in the way
    Unicode file names are handled; these are bugs, so please report any issues
    of this type.
 
-3. When reading headers, there is no validation that OIDs present on an `index`
+4. When reading headers, there is no validation that OIDs present on an `index`
    line are shorter than or equal to the maximum hash length, as this requires
    knowing if the repository used SHA1 or SHA256 hashes.
 
-4. When reading "traditional" patches (those not produced by `git`), prefixes
-   are not stripped from file names (`git apply` attempts to remove prefixes
-   that match the current repository directory/prefix.)
+5. When reading "traditional" patches (those not produced by `git`), prefixes
+   are not stripped from file names; `git apply` attempts to remove prefixes
+   that match the current repository directory/prefix.

gitdiff/parser.go

@@ -134,7 +134,7 @@ func (p *parser) ParseTextFragments(f *File) (n int, err error) {
 		}
 
 		if err := p.ParseTextChunk(frag); err != nil {
-			return n, nil
+			return n, err
 		}
 
 		f.Fragments = append(f.Fragments, frag)
@@ -277,6 +277,11 @@ func (p *parser) ParseTextChunk(frag *Fragment) error {
 				last.Line = strings.TrimSuffix(last.Line, "\n")
 				break
 			}
+			// TODO(bkeyes): if this is because we hit the next header, it
+			// would be helpful to return the miscounts line error. We could
+			// either test for the common headers ("@@ -", "diff --git") or
+			// assume any invalid op ends the fragment; git returns the same
+			// generic error in all cases so either is compatible
 			return p.Errorf(0, "invalid line operation: %q", op)
 		}
 

gitdiff/parser_text_test.go

@@ -1,6 +1,7 @@
 package gitdiff
 
 import (
+	"io"
 	"reflect"
 	"testing"
 )
@@ -55,8 +56,8 @@ func TestParseTextFragmentHeader(t *testing.T) {
 
 			frag, err := p.ParseTextFragmentHeader()
 			if test.Err {
-				if err == nil {
-					t.Fatalf("expected error parsing header, but got nil")
+				if err == nil || err == io.EOF {
+					t.Fatalf("expected error parsing header, but got %v", err)
 				}
 				return
 			}
@@ -152,6 +153,33 @@ func TestParseTextChunk(t *testing.T) {
 				TrailingContext: 1,
 			},
 		},
+		"middleContext": {
+			Input: ` context line
+-old line 1
+ context line
++new line 1
+ context line
+`,
+			Fragment: Fragment{
+				OldLines: 4,
+				NewLines: 4,
+			},
+			Output: &Fragment{
+				OldLines: 4,
+				NewLines: 4,
+				Lines: []FragmentLine{
+					{OpContext, "context line\n"},
+					{OpDelete, "old line 1\n"},
+					{OpContext, "context line\n"},
+					{OpAdd, "new line 1\n"},
+					{OpContext, "context line\n"},
+				},
+				LinesDeleted:    1,
+				LinesAdded:      1,
+				LeadingContext:  1,
+				TrailingContext: 1,
+			},
+		},
 		"deleteFinalNewline": {
 			Input: ` context line
 -old line 1
@@ -298,8 +326,8 @@ func TestParseTextChunk(t *testing.T) {
 			frag := test.Fragment
 			err := p.ParseTextChunk(&frag)
 			if test.Err {
-				if err == nil {
-					t.Fatalf("expected error parsing text chunk, but got nil")
+				if err == nil || err == io.EOF {
+					t.Fatalf("expected error parsing text chunk, but got %v", err)
 				}
 				return
 			}
@@ -313,3 +341,130 @@ func TestParseTextChunk(t *testing.T) {
 		})
 	}
 }
+
+func TestParseTextFragments(t *testing.T) {
+	tests := map[string]struct {
+		Input string
+		File  File
+
+		Fragments []*Fragment
+		Err       bool
+	}{
+		"multipleChanges": {
+			Input: `@@ -1,3 +1,2 @@
+ context line
+-old line 1
+ context line
+@@ -8,3 +7,3 @@
+ context line
+-old line 2
++new line 1
+ context line
+@@ -15,3 +14,4 @@
+ context line
+-old line 3
++new line 2
++new line 3
+ context line
+`,
+			Fragments: []*Fragment{
+				{
+					OldPosition: 1,
+					OldLines:    3,
+					NewPosition: 1,
+					NewLines:    2,
+					Lines: []FragmentLine{
+						{OpContext, "context line\n"},
+						{OpDelete, "old line 1\n"},
+						{OpContext, "context line\n"},
+					},
+					LinesDeleted:    1,
+					LeadingContext:  1,
+					TrailingContext: 1,
+				},
+				{
+					OldPosition: 8,
+					OldLines:    3,
+					NewPosition: 7,
+					NewLines:    3,
+					Lines: []FragmentLine{
+						{OpContext, "context line\n"},
+						{OpDelete, "old line 2\n"},
+						{OpAdd, "new line 1\n"},
+						{OpContext, "context line\n"},
+					},
+					LinesDeleted:    1,
+					LinesAdded:      1,
+					LeadingContext:  1,
+					TrailingContext: 1,
+				},
+				{
+					OldPosition: 15,
+					OldLines:    3,
+					NewPosition: 14,
+					NewLines:    4,
+					Lines: []FragmentLine{
+						{OpContext, "context line\n"},
+						{OpDelete, "old line 3\n"},
+						{OpAdd, "new line 2\n"},
+						{OpAdd, "new line 3\n"},
+						{OpContext, "context line\n"},
+					},
+					LinesDeleted:    1,
+					LinesAdded:      2,
+					LeadingContext:  1,
+					TrailingContext: 1,
+				},
+			},
+		},
+		"badNewFile": {
+			Input: `@@ -1 +1,2 @@
+-old line 1
++new line 1
++new line 2
+`,
+			File: File{
+				IsNew: true,
+			},
+			Err: true,
+		},
+		"badDeletedFile": {
+			Input: `@@ -1,2 +1 @@
+-old line 1
+ context line
+`,
+			File: File{
+				IsDelete: true,
+			},
+			Err: true,
+		},
+	}
+
+	for name, test := range tests {
+		t.Run(name, func(t *testing.T) {
+			p := newTestParser(test.Input, true)
+
+			file := test.File
+			n, err := p.ParseTextFragments(&file)
+			if test.Err {
+				if err == nil || err == io.EOF {
+					t.Fatalf("expected error parsing text fragments, but got %v", err)
+				}
+				return
+			}
+			if err != nil {
+				t.Fatalf("error parsing text fragments: %v", err)
+			}
+
+			if len(test.Fragments) != n {
+				t.Fatalf("incorrect number of added fragments: expected %d, actual %d", len(test.Fragments), n)
+			}
+
+			for i, frag := range test.Fragments {
+				if !reflect.DeepEqual(frag, file.Fragments[i]) {
+					t.Errorf("incorrect fragment at position %d\nexpected: %+v\nactual: %+v", i, frag, file.Fragments[i])
+				}
+			}
+		})
+	}
+}