From 18058d1e151e780b7617d574c037b8a4e948816c Mon Sep 17 00:00:00 2001 From: Billy Keyes Date: Thu, 23 Jan 2020 23:02:31 -0800 Subject: [PATCH] Move apply methods to an Applier type This removes the distinction between "strict" and "fuzzy" application by allowing future methods on Applier that control settings. It also avoids state tracking in the text fragment apply signature by moving it into the Applier type. While in practice, an Applier will be used once and discarded, the capability is provided to reset it for multiple uses. --- gitdiff/apply.go | 215 ++++++++++++++++++++++++++---------------- gitdiff/apply_test.go | 26 +++-- gitdiff/gitdiff.go | 7 +- gitdiff/io.go | 84 ++++++++++++++++- 4 files changed, 231 insertions(+), 101 deletions(-) diff --git a/gitdiff/apply.go b/gitdiff/apply.go index c1216c7..3d296e1 100644 --- a/gitdiff/apply.go +++ b/gitdiff/apply.go @@ -1,11 +1,9 @@ package gitdiff import ( - "bytes" "errors" "fmt" "io" - "io/ioutil" ) // Conflict indicates an apply failed due to a conflict between the patch and @@ -85,70 +83,101 @@ func applyError(err error, args ...interface{}) error { return e } -// ApplyStrict writes data from src to dst, modifying it as described by the -// fragments in the file. For text files, each fragment, including all context -// lines, must exactly match src at the expected line number. +var ( + errApplyInProgress = errors.New("gitdiff: incompatible apply in progress") +) + +const ( + applyInitial = iota + applyText + applyBinary +) + +// Applier applies changes described in fragments to source data. If changes +// are described in multiple fragments, those fragments must be applied in +// order, usually by calling ApplyFile. // -// If the apply fails, ApplyStrict returns an *ApplyError wrapping the cause. -// Partial data may be written to dst in this case. -func (f *File) ApplyStrict(dst io.Writer, src io.Reader) error { - // TODO(bkeyes): take an io.ReaderAt and avoid this! - data, err := ioutil.ReadAll(src) - if err != nil { - return applyError(err) - } +// By default, Applier operates in "strict" mode, where fragment content and +// positions must exactly match those of the source. +// +// If an error occurs while applying, methods on Applier return instances of +// *ApplyError that annotate the wrapped error with additional information +// when available. If the error is because of a conflict between a fragment and +// the source, the wrapped error will be a *Conflict. +// +// While an Applier can apply both text and binary fragments, only one fragment +// type can be used without resetting the Applier. The first fragment applied +// sets the type for the Applier. Mixing fragment types or mixing +// fragment-level and file-level applies results in an error. +type Applier struct { + src io.ReaderAt + lineSrc LineReaderAt + nextLine int64 + applyType int +} - if f.IsBinary { - if f.BinaryFragment != nil { - return f.BinaryFragment.Apply(dst, bytes.NewReader(data)) +// NewApplier creates an Applier that reads data from src. If src is a +// LineReaderAt, it is used directly to apply text fragments. +func NewApplier(src io.ReaderAt) *Applier { + a := new(Applier) + a.Reset(src) + return a +} + +// Reset resets the input and internal state of the Applier. If src is nil, the +// existing source is reused. +func (a *Applier) Reset(src io.ReaderAt) { + if src != nil { + a.src = src + if lineSrc, ok := src.(LineReaderAt); ok { + a.lineSrc = lineSrc + } else { + a.lineSrc = &lineReaderAt{r: src} } - _, err = dst.Write(data) - return applyError(err) + } + a.nextLine = 0 + a.applyType = applyInitial +} + +// ApplyFile applies the changes in all of the fragments of f and writes the +// result to dst. +func (a *Applier) ApplyFile(dst io.Writer, f *File) error { + if a.applyType != applyInitial { + return applyError(errApplyInProgress) } - // TODO(bkeyes): check for this conflict case - // &Conflict{"cannot create new file from non-empty src"} + if f.IsBinary && f.BinaryFragment != nil { + return a.ApplyBinaryFragment(dst, f.BinaryFragment) + } - lra := NewLineReaderAt(bytes.NewReader(data)) + // TODO(bkeyes): sort fragments by start position + // TODO(bkeyes): merge overlapping fragments - var next int64 for i, frag := range f.TextFragments { - next, err = frag.ApplyStrict(dst, lra, next) - if err != nil { + if err := a.ApplyTextFragment(dst, frag); err != nil { return applyError(err, fragNum(i)) } } - // TODO(bkeyes): extract this to a utility - buf := make([][]byte, 64) - for { - n, err := lra.ReadLinesAt(buf, next) - if err != nil && err != io.EOF { - return applyError(err, lineNum(next+int64(n))) - } - - for i := 0; i < n; i++ { - if _, err := dst.Write(buf[n]); err != nil { - return applyError(err, lineNum(next+int64(n))) - } - } - - next += int64(n) - if n < len(buf) { - return nil - } - } + return applyError(a.Flush(dst)) } -// ApplyStrict copies from src to dst, from line start through then end of the -// fragment, modifying the data as described by the fragment. The fragment, -// including all context lines, must exactly match src at the expected line -// number. ApplyStrict returns the number of the next unprocessed line in src -// and any error. When the error is not non-nil, partial data may be written. -func (f *TextFragment) ApplyStrict(dst io.Writer, src LineReaderAt, start int64) (next int64, err error) { +// ApplyTextFragment applies the changes in the fragment f and writes unwritten +// data before the start of the fragment and the result to dst. If multiple +// text fragments apply to the same source, ApplyTextFragment must be called in +// order of increasing start position. As a result, each fragment can be +// applied at most once before a call to Reset. +func (a *Applier) ApplyTextFragment(dst io.Writer, f *TextFragment) error { + switch a.applyType { + case applyInitial, applyText: + default: + return applyError(errApplyInProgress) + } + a.applyType = applyText + // application code assumes fragment fields are consistent if err := f.Validate(); err != nil { - return start, applyError(err) + return applyError(err) } // lines are 0-indexed, positions are 1-indexed (but new files have position = 0) @@ -158,24 +187,35 @@ func (f *TextFragment) ApplyStrict(dst io.Writer, src LineReaderAt, start int64) } fragEnd := fragStart + f.OldLines + start := a.nextLine if fragStart < start { - return start, applyError(&Conflict{"fragment overlaps with an applied fragment"}) + return applyError(&Conflict{"fragment overlaps with an applied fragment"}) + } + + if f.OldPosition == 0 { + ok, err := isLen(a.src, 0) + if err != nil { + return applyError(err) + } + if !ok { + return applyError(&Conflict{"cannot create new file from non-empty src"}) + } } preimage := make([][]byte, fragEnd-start) - n, err := src.ReadLinesAt(preimage, start) + n, err := a.lineSrc.ReadLinesAt(preimage, start) switch { case err == nil: case err == io.EOF && n == len(preimage): // last line of frag has no newline character default: - return start, applyError(err, lineNum(start+int64(n))) + return applyError(err, lineNum(start+int64(n))) } // copy leading data before the fragment starts for i, line := range preimage[:fragStart-start] { if _, err := dst.Write(line); err != nil { - next = start + int64(i) - return next, applyError(err, lineNum(next)) + a.nextLine = start + int64(i) + return applyError(err, lineNum(a.nextLine)) } } preimage = preimage[fragStart-start:] @@ -184,14 +224,15 @@ func (f *TextFragment) ApplyStrict(dst io.Writer, src LineReaderAt, start int64) used := int64(0) for i, line := range f.Lines { if err := applyTextLine(dst, line, preimage, used); err != nil { - next = fragStart + used - return next, applyError(err, lineNum(next), fragLineNum(i)) + a.nextLine = fragStart + used + return applyError(err, lineNum(a.nextLine), fragLineNum(i)) } if line.Old() { used++ } } - return fragStart + used, nil + a.nextLine = fragStart + used + return nil } func applyTextLine(dst io.Writer, line Line, preimage [][]byte, i int64) (err error) { @@ -201,34 +242,53 @@ func applyTextLine(dst io.Writer, line Line, preimage [][]byte, i int64) (err er if line.New() { _, err = io.WriteString(dst, line.Line) } - return + return err } -// Apply writes data from src to dst, modifying it as described by the -// fragment. -// -// Unlike text fragments, binary fragments do not distinguish between strict -// and non-strict application. -func (f *BinaryFragment) Apply(dst io.Writer, src io.ReaderAt) error { +// Flush writes any data following the last applied fragment to dst. +func (a *Applier) Flush(dst io.Writer) (err error) { + switch a.applyType { + case applyInitial: + _, err = copyFrom(dst, a.src, 0) + case applyText: + _, err = copyLinesFrom(dst, a.lineSrc, a.nextLine) + case applyBinary: + // nothing to flush, binary apply "consumes" full source + } + return err +} + +// ApplyBinaryFragment applies the changes in the fragment f and writes the +// result to dst. At most one binary fragment can be applied before a call to +// Reset. +func (a *Applier) ApplyBinaryFragment(dst io.Writer, f *BinaryFragment) error { + if a.applyType != applyInitial { + return applyError(errApplyInProgress) + } + a.applyType = applyText + + if f == nil { + return applyError(errors.New("nil fragment")) + } + switch f.Method { case BinaryPatchLiteral: if _, err := dst.Write(f.Data); err != nil { return applyError(err) } case BinaryPatchDelta: - if err := applyBinaryDeltaFragment(dst, src, f.Data); err != nil { + if err := applyBinaryDeltaFragment(dst, a.src, f.Data); err != nil { return applyError(err) } default: return applyError(fmt.Errorf("unsupported binary patch method: %v", f.Method)) } - return nil } func applyBinaryDeltaFragment(dst io.Writer, src io.ReaderAt, frag []byte) error { srcSize, delta := readBinaryDeltaSize(frag) - if err := checkBinarySrcSize(srcSize, src); err != nil { + if err := checkBinarySrcSize(src, srcSize); err != nil { return err } @@ -342,20 +402,15 @@ func applyBinaryDeltaCopy(w io.Writer, op byte, delta []byte, src io.ReaderAt) ( return size, delta, err } -func checkBinarySrcSize(size int64, src io.ReaderAt) error { - start := size - if start > 0 { - start-- - } - var b [2]byte - n, err := src.ReadAt(b[:], start) - if err == io.EOF && (size == 0 && n == 0) || (size > 0 && n == 1) { - return nil - } - if err != nil && err != io.EOF { +func checkBinarySrcSize(r io.ReaderAt, size int64) error { + ok, err := isLen(r, size) + if err != nil { return err } - return &Conflict{"fragment src size does not match actual src size"} + if !ok { + return &Conflict{"fragment src size does not match actual src size"} + } + return nil } func wrapEOF(err error) error { diff --git a/gitdiff/apply_test.go b/gitdiff/apply_test.go index b7cc645..939720b 100644 --- a/gitdiff/apply_test.go +++ b/gitdiff/apply_test.go @@ -57,14 +57,13 @@ func TestTextFragmentApplyStrict(t *testing.T) { }, Err: &Conflict{}, }, - // TODO(bkeyes): this check has moved to the file level (probably) - // "errorNewFile": { - // Files: applyFiles{ - // Src: "text_fragment_error.src", - // Patch: "text_fragment_error_new_file.patch", - // }, - // Err: &Conflict{}, - // }, + "errorNewFile": { + Files: applyFiles{ + Src: "text_fragment_error.src", + Patch: "text_fragment_error_new_file.patch", + }, + Err: &Conflict{}, + }, } for name, test := range tests { @@ -82,10 +81,10 @@ func TestTextFragmentApplyStrict(t *testing.T) { t.Fatalf("patch should contain exactly one fragment, but it has %d", len(files[0].TextFragments)) } - frag := files[0].TextFragments[0] + applier := NewApplier(bytes.NewReader(src)) var dst bytes.Buffer - _, err = frag.ApplyStrict(&dst, NewLineReaderAt(bytes.NewReader(src)), 0) + err = applier.ApplyTextFragment(&dst, files[0].TextFragments[0]) if test.Err != nil { checkApplyError(t, test.Err, err) return @@ -153,13 +152,10 @@ func TestBinaryFragmentApply(t *testing.T) { t.Fatalf("patch should contain exactly one file, but it has %d", len(files)) } - frag := files[0].BinaryFragment - if frag == nil { - t.Fatalf("patch should contain a binary fragment, but it was nil") - } + applier := NewApplier(bytes.NewReader(src)) var dst bytes.Buffer - err = frag.Apply(&dst, bytes.NewReader(src)) + err = applier.ApplyBinaryFragment(&dst, files[0].BinaryFragment) if test.Err != nil { checkApplyError(t, test.Err, err) return diff --git a/gitdiff/gitdiff.go b/gitdiff/gitdiff.go index 9b5457c..18645bd 100644 --- a/gitdiff/gitdiff.go +++ b/gitdiff/gitdiff.go @@ -1,6 +1,7 @@ package gitdiff import ( + "errors" "fmt" "os" ) @@ -64,6 +65,10 @@ func (f *TextFragment) Header() string { // Validate checks that the fragment is self-consistent and appliable. Validate // returns an error if and only if the fragment is invalid. func (f *TextFragment) Validate() error { + if f == nil { + return errors.New("nil fragment") + } + var ( oldLines, newLines int64 leadingContext, trailingContext int64 @@ -117,7 +122,7 @@ func (f *TextFragment) Validate() error { // if a file is being created, it can only contain additions if f.OldPosition == 0 && f.OldLines != 0 { - return fmt.Errorf("file creation fragment contains context or deletion lines") + return errors.New("file creation fragment contains context or deletion lines") } return nil diff --git a/gitdiff/io.go b/gitdiff/io.go index bb3936b..ee9972e 100644 --- a/gitdiff/io.go +++ b/gitdiff/io.go @@ -24,11 +24,6 @@ type LineReaderAt interface { ReadLinesAt(lines [][]byte, offset int64) (n int, err error) } -// NewLineReaderAt creates a LineReaderAt from an io.ReaderAt. -func NewLineReaderAt(r io.ReaderAt) LineReaderAt { - return &lineReaderAt{r: r} -} - type lineReaderAt struct { r io.ReaderAt index []int64 @@ -129,3 +124,82 @@ func lookupLines(index []int64, start, n int64) (size int64, offset int64) { } return } + +func isLen(r io.ReaderAt, n int64) (bool, error) { + off := n - 1 + if off < 0 { + off = 0 + } + + var b [2]byte + nr, err := r.ReadAt(b[:], off) + if err == io.EOF { + return (n == 0 && nr == 0) || (n > 0 && nr == 1), nil + } + return false, err +} + +// copyFrom writes bytes starting from offset off in src to dst stopping at the +// end of src or at the first error. copyFrom returns the number of bytes +// written and any error. +func copyFrom(dst io.Writer, src io.ReaderAt, off int64) (written int64, err error) { + buf := make([]byte, 32*1024) // stolen from io.Copy + for { + nr, rerr := src.ReadAt(buf, off) + if nr > 0 { + nw, werr := dst.Write(buf[0:nr]) + if nw > 0 { + written += int64(nw) + } + if werr != nil { + err = werr + break + } + if nr != nw { + err = io.ErrShortWrite + break + } + } + if rerr != nil { + if rerr != io.EOF { + err = rerr + } + break + } + } + return written, err +} + +// copyLinesFrom writes lines starting from line off in src to dst stopping at +// the end of src or at the first error. copyLinesFrom returns the number of +// lines written and any error. +func copyLinesFrom(dst io.Writer, src LineReaderAt, off int64) (written int64, err error) { + buf := make([][]byte, 32) +ReadLoop: + for { + nr, rerr := src.ReadLinesAt(buf, off) + if nr > 0 { + for _, line := range buf[0:nr] { + nw, werr := dst.Write(line) + if nw > 0 { + written++ + } + if werr != nil { + err = werr + break ReadLoop + } + if len(line) != nw { + err = io.ErrShortWrite + break ReadLoop + } + } + } + if rerr != nil { + if rerr != io.EOF { + err = rerr + } + break + } + } + return written, err +}