From 9b0fc30459badd7f947fb97fa4c1d27c8d2c1631 Mon Sep 17 00:00:00 2001 From: Billy Keyes Date: Wed, 27 Mar 2019 22:41:37 -0700 Subject: [PATCH] Document and enforce parser invariants After considering fragment parsing, it made sense to change the invariant estabilished in the previous commit. Specifically, parser functions now assume they are call on the first line of their object and return with the parser on the first line after their object. This means code can call parse function immediately after each other without advancing the parser in between. It's possible this will change back later on... there seem to be annoying edge cases with either choice, but I think making the functions consistent is important. --- gitdiff/file_header.go | 12 +++++--- gitdiff/file_header_test.go | 54 --------------------------------- gitdiff/gitdiff.go | 6 ++++ gitdiff/parser.go | 56 +++++++++++++++++++++------------- gitdiff/parser_test.go | 60 +++++++++++++++++++++++++++++++++++++ 5 files changed, 110 insertions(+), 78 deletions(-) diff --git a/gitdiff/file_header.go b/gitdiff/file_header.go index eb8f65a..4ff5ff5 100644 --- a/gitdiff/file_header.go +++ b/gitdiff/file_header.go @@ -32,9 +32,6 @@ func (p *parser) ParseGitFileHeader() (*File, error) { if err != nil { return nil, p.Errorf(1, "git file header: %v", err) } - if end { - break - } if err := p.Next(); err != nil { if err == io.EOF { @@ -42,6 +39,10 @@ func (p *parser) ParseGitFileHeader() (*File, error) { } return nil, err } + + if end { + break + } } if f.OldName == "" && f.NewName == "" { @@ -76,11 +77,14 @@ func (p *parser) ParseTraditionalFileHeader() (*File, error) { return nil, nil } - // advance past the first line so parser is at end of header + // advance past the first two lines so parser is after the header // no EOF check needed because we know there are >=3 valid lines if err := p.Next(); err != nil { return nil, err } + if err := p.Next(); err != nil { + return nil, err + } oldName, _, err := parseName(oldLine[len(oldPrefix):], '\t', 0) if err != nil { diff --git a/gitdiff/file_header_test.go b/gitdiff/file_header_test.go index 62f2cde..46581c3 100644 --- a/gitdiff/file_header_test.go +++ b/gitdiff/file_header_test.go @@ -277,60 +277,6 @@ context line } } -func TestParserAdvancment(t *testing.T) { - tests := map[string]struct { - Input string - Parse func(p *parser) error - NextLine string - }{ - "ParseGitFileHeader": { - Input: `diff --git a/dir/file.txt b/dir/file.txt -index 9540595..30e6333 100644 ---- a/dir/file.txt -+++ b/dir/file.txt -@@ -1,2 +1,3 @@ -context line -`, - Parse: func(p *parser) error { - _, err := p.ParseGitFileHeader() - return err - }, - NextLine: "@@ -1,2 +1,3 @@\n", - }, - "ParseTraditionalFileHeader": { - Input: `--- dir/file.txt -+++ dir/file.txt -@@ -1,2 +1,3 @@ -context line -`, - Parse: func(p *parser) error { - _, err := p.ParseTraditionalFileHeader() - return err - }, - NextLine: "@@ -1,2 +1,3 @@\n", - }, - } - - for name, test := range tests { - t.Run(name, func(t *testing.T) { - p := &parser{r: bufio.NewReader(strings.NewReader(test.Input))} - p.Next() - - if err := test.Parse(p); err != nil { - t.Fatalf("unexpected error while parsing: %v", err) - } - - if err := p.Next(); err != nil { - t.Fatalf("advancing the parser after parsing returned an error: %v", err) - } - - if test.NextLine != p.Line(0) { - t.Errorf("incorrect next line after parsing\nexpected: %q\nactual: %q", test.NextLine, p.Line(0)) - } - }) - } -} - func TestCleanName(t *testing.T) { tests := map[string]struct { Input string diff --git a/gitdiff/gitdiff.go b/gitdiff/gitdiff.go index 8c749a9..3327194 100644 --- a/gitdiff/gitdiff.go +++ b/gitdiff/gitdiff.go @@ -1,6 +1,7 @@ package gitdiff import ( + "fmt" "os" ) @@ -35,3 +36,8 @@ type Fragment struct { NewPosition int64 NewLines int64 } + +// Header returns the cannonical header of this fragment. +func (f *Fragment) Header() string { + return fmt.Sprintf("@@ -%d,%d +%d,%d @@ %s", f.OldPosition, f.OldLines, f.NewPosition, f.NewLines, f.Comment) +} diff --git a/gitdiff/parser.go b/gitdiff/parser.go index 6eaaf9e..bc0751c 100644 --- a/gitdiff/parser.go +++ b/gitdiff/parser.go @@ -11,22 +11,27 @@ import ( // Parse parses a patch with changes for one or more files. Any content // preceding the first file header is ignored. If an error occurs while // parsing, files will contain all files parsed before the error. -func Parse(r io.Reader) (files []*File, err error) { +func Parse(r io.Reader) ([]*File, error) { p := &parser{r: bufio.NewReader(r)} + if err := p.Next(); err != nil { + if err == io.EOF { + return nil, nil + } + return nil, err + } - var file *File + var files []*File for { - file, err = p.ParseNextFileHeader() + file, err := p.ParseNextFileHeader() if err != nil { - return + return files, err } if file == nil { break } - err = p.ParseFileChanges(file) - if err != nil { - return + if err = p.ParseFragments(file); err != nil { + return files, err } files = append(files, file) @@ -39,6 +44,12 @@ func Parse(r io.Reader) (files []*File, err error) { // this would enable OID validation, p-value guessing, and prefix stripping // by allowing users to set or override defaults +// parser invariants: +// - methods that parse objects start on the first line of the first object +// - methods that parse objects return on the first line after the last object +// - if a parse method returns a nil object, the parser was not advanced +// - any exported parsing methods must initialize the parser by calling Next() + type parser struct { r *bufio.Reader @@ -50,26 +61,20 @@ type parser struct { // ParseNextFileHeader finds and parses the next file header in the stream. It // returns nil if no headers are found before the end of the stream. func (p *parser) ParseNextFileHeader() (*File, error) { + var file *File for { - if err := p.Next(); err != nil { - if err == io.EOF { - break - } - return nil, err - } - // check for disconnected fragment headers (corrupt patch) frag, err := p.ParseFragmentHeader() if err != nil { // not a valid header, nothing to worry about - continue + goto NextLine } if frag != nil { - return nil, p.Errorf(0, "patch fragment without header: %s", p.Line(0)) + return nil, p.Errorf(-1, "patch fragment without file header: %s", frag.Header()) } // check for a git-generated patch - file, err := p.ParseGitFileHeader() + file, err = p.ParseGitFileHeader() if err != nil { return nil, err } @@ -85,13 +90,21 @@ func (p *parser) ParseNextFileHeader() (*File, error) { if file != nil { return file, nil } + + NextLine: + if err := p.Next(); err != nil { + if err == io.EOF { + break + } + return nil, err + } } return nil, nil } -// ParseFileChanges parses file changes until the next file header or the end -// of the stream and attaches them to the given file. -func (p *parser) ParseFileChanges(f *File) error { +// ParseFragments parses fragments until the next file header or the end of the +// stream and attaches them to the given file. +func (p *parser) ParseFragments(f *File) error { panic("TODO(bkeyes): unimplemented") } @@ -177,6 +190,9 @@ func (p *parser) ParseFragmentHeader() (*Fragment, error) { return nil, fmt.Errorf("invalid fragment header: %v", err) } + if err := p.Next(); err != nil && err != io.EOF { + return nil, err + } return f, nil } diff --git a/gitdiff/parser_test.go b/gitdiff/parser_test.go index 49f9327..d665c85 100644 --- a/gitdiff/parser_test.go +++ b/gitdiff/parser_test.go @@ -73,6 +73,66 @@ func TestLineOperations(t *testing.T) { }) } +func TestParserAdvancment(t *testing.T) { + tests := map[string]struct { + Input string + Parse func(p *parser) error + EndLine string + }{ + "ParseGitFileHeader": { + Input: `diff --git a/dir/file.txt b/dir/file.txt +index 9540595..30e6333 100644 +--- a/dir/file.txt ++++ b/dir/file.txt +@@ -1,2 +1,3 @@ +context line +`, + Parse: func(p *parser) error { + _, err := p.ParseGitFileHeader() + return err + }, + EndLine: "@@ -1,2 +1,3 @@\n", + }, + "ParseTraditionalFileHeader": { + Input: `--- dir/file.txt ++++ dir/file.txt +@@ -1,2 +1,3 @@ +context line +`, + Parse: func(p *parser) error { + _, err := p.ParseTraditionalFileHeader() + return err + }, + EndLine: "@@ -1,2 +1,3 @@\n", + }, + "ParseFragmentHeader": { + Input: `@@ -1,2 +1,3 @@ +context line +`, + Parse: func(p *parser) error { + _, err := p.ParseFragmentHeader() + return err + }, + EndLine: "context line\n", + }, + } + + for name, test := range tests { + t.Run(name, func(t *testing.T) { + p := &parser{r: bufio.NewReader(strings.NewReader(test.Input))} + p.Next() + + if err := test.Parse(p); err != nil { + t.Fatalf("unexpected error while parsing: %v", err) + } + + if test.EndLine != p.Line(0) { + t.Errorf("incorrect position after parsing\nexpected: %q\nactual: %q", test.EndLine, p.Line(0)) + } + }) + } +} + func TestParseFragmentHeader(t *testing.T) { tests := map[string]struct { Input string