Repository: bluekeyes/go-gitdiff (go)

Issue:
Flush() is going to endless loop
Hi.

Thanks for this library. It saved me lots of time. However, I'm currently facing a problem.
The following code run indefinitely and eats RAM:

```
package main

import (
	"bytes"

	"github.com/bluekeyes/go-gitdiff/gitdiff"
)

const (
	diff = `
diff --git a/app/controllers/seances_controller.rb b/app/controllers/seances_controller.rb
index 743d0ad..4f4d4e8 100644
--- a/app/controllers/seances_controller.rb
+++ b/app/controllers/seances_controller.rb
@@ -5,8 +5,6 @@ class SeancesController < ApplicationController
     if authorization_result.code != 200
       return render_by_status_code(code: authorization_result.code, data: authorization_result.data)
     end
-
-    render_by_status_code(code: 200, data: json)
   end

   def create
`
)

var (
	body = `class SeancesController < ApplicationController
  def index
    set_auth_operation_id_header

    if authorization_result.code != 200
      return render_by_status_code(code: authorization_result.code, data: authorization_result.data)
    end

    render_by_status_code(code: 200, data: json)
  end

  def create
    seance = Seance.new(
      movie_id: params[:movie_id],
      price: params[:price],
      datetime: params[:datetime]
    )

    if seance.save!
      render json: {
        data: {
          id: seance.id,
          type: 'seances',
          attributes: { datetime: seance.datetime, price: seance.price },
          seats: Seat.pluck(:id).map do |seat_id|
            { id: seat_id, vacant: true }
          end
        }
      }
    end
  rescue ActiveRecord::RecordInvalid => e
    render_invalid_record(message: e.message)
  end

  def destroy
    ActiveRecord::Base.transaction do
      Booking.where(seance: params[:id]).destroy_all
      Seance.find(params[:id]).destroy
    end

    render json: { data: [{ id: params[:id], type: 'seances' }] }
  end

  def json
    seats_ids = Seat.pluck(:id)

    Seance.includes(:bookings).where(movie: params[:movie_id]).order('datetime').limit(params[:max_results] || 50).map do |seance|
      booking_seats_ids = seance.bookings.pluck(:seat_id)

      {
        id: seance.id,
        price: seance.price,
        datetime: seance.datetime,
        seats: seats_ids.map do |seat_id|
          { id: seat_id, vacant: !(booking_seats_ids.include?(seat_id)) }
        end
      }
    end
  end
end
`
)

func main() {
	files, _, err := gitdiff.Parse(bytes.NewBufferString(diff))
	if err != nil {
		panic(err)
	}

	for _, file := range files {
		writer := bytes.NewBuffer(nil)
		reader := bytes.NewReader([]byte(body))
		appl := gitdiff.NewApplier(reader)
		if err := appl.ApplyFile(writer, file); err != nil {
			panic(err)
		}
	}
}
```

I debugged it to somewhere in Flush() operation.  The internals keeps copying same lines over and over.

I would appreciate your help in debugging this.
Ivan

Pull request:
Fix flushing when data is larger than the buffer
`copyFrom` and `copyLinesFrom` did not increment the start offset after reading, so the same lines were copied over and over again.

Fixes #1.

Patch:
diff --git a/gitdiff/io.go b/gitdiff/io.go
index 515fb10..af5c847 100644
--- a/gitdiff/io.go
+++ b/gitdiff/io.go
@@ -147,11 +147,16 @@ func isLen(r io.ReaderAt, n int64) (bool, error) {
 	return false, err
 }
 
+const (
+	byteBufferSize = 32 * 1024 // from io.Copy
+	lineBufferSize = 32
+)
+
 // 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
+	buf := make([]byte, byteBufferSize)
 	for {
 		nr, rerr := src.ReadAt(buf, off)
 		if nr > 0 {
@@ -167,6 +172,7 @@ func copyFrom(dst io.Writer, src io.ReaderAt, off int64) (written int64, err err
 				err = io.ErrShortWrite
 				break
 			}
+			off += int64(nr)
 		}
 		if rerr != nil {
 			if rerr != io.EOF {
@@ -182,7 +188,7 @@ func copyFrom(dst io.Writer, src io.ReaderAt, off int64) (written int64, err err
 // 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)
+	buf := make([][]byte, lineBufferSize)
 ReadLoop:
 	for {
 		nr, rerr := src.ReadLinesAt(buf, off)
@@ -201,6 +207,7 @@ ReadLoop:
 					break ReadLoop
 				}
 			}
+			off += int64(nr)
 		}
 		if rerr != nil {
 			if rerr != io.EOF {


Test patch:
diff --git a/gitdiff/io_test.go b/gitdiff/io_test.go
index 1d1050d..8d8a18b 100644
--- a/gitdiff/io_test.go
+++ b/gitdiff/io_test.go
@@ -4,6 +4,7 @@ import (
 	"bytes"
 	"fmt"
 	"io"
+	"math/rand"
 	"testing"
 )
 
@@ -114,3 +115,88 @@ func TestLineReaderAt(t *testing.T) {
 		})
 	}
 }
+
+func TestCopyFrom(t *testing.T) {
+	tests := map[string]struct {
+		Bytes  int64
+		Offset int64
+	}{
+		"copyAll": {
+			Bytes: byteBufferSize / 2,
+		},
+		"copyPartial": {
+			Bytes:  byteBufferSize / 2,
+			Offset: byteBufferSize / 4,
+		},
+		"copyLarge": {
+			Bytes: 8 * byteBufferSize,
+		},
+	}
+
+	for name, test := range tests {
+		t.Run(name, func(t *testing.T) {
+			data := make([]byte, test.Bytes)
+			rand.Read(data)
+
+			var dst bytes.Buffer
+			n, err := copyFrom(&dst, bytes.NewReader(data), test.Offset)
+			if err != nil {
+				t.Fatalf("unexpected error copying data: %v", err)
+			}
+			if n != test.Bytes-test.Offset {
+				t.Fatalf("incorrect number of bytes copied: expected %d, actual %d", test.Bytes-test.Offset, n)
+			}
+
+			expected := data[test.Offset:]
+			if !bytes.Equal(expected, dst.Bytes()) {
+				t.Fatalf("incorrect data copied:\nexpected: %v\nactual: %v", expected, dst.Bytes())
+			}
+		})
+	}
+}
+
+func TestCopyLinesFrom(t *testing.T) {
+	tests := map[string]struct {
+		Lines  int64
+		Offset int64
+	}{
+		"copyAll": {
+			Lines: lineBufferSize / 2,
+		},
+		"copyPartial": {
+			Lines:  lineBufferSize / 2,
+			Offset: lineBufferSize / 4,
+		},
+		"copyLarge": {
+			Lines: 8 * lineBufferSize,
+		},
+	}
+
+	const lineLength = 128
+
+	for name, test := range tests {
+		t.Run(name, func(t *testing.T) {
+			data := make([]byte, test.Lines*lineLength)
+			for i := range data {
+				data[i] = byte(32 + rand.Intn(95)) // ascii letters, numbers, symbols
+				if i%lineLength == lineLength-1 {
+					data[i] = '\n'
+				}
+			}
+
+			var dst bytes.Buffer
+			n, err := copyLinesFrom(&dst, &lineReaderAt{r: bytes.NewReader(data)}, test.Offset)
+			if err != nil {
+				t.Fatalf("unexpected error copying data: %v", err)
+			}
+			if n != test.Lines-test.Offset {
+				t.Fatalf("incorrect number of lines copied: expected %d, actual %d", test.Lines-test.Offset, n)
+			}
+
+			expected := data[test.Offset*lineLength:]
+			if !bytes.Equal(expected, dst.Bytes()) {
+				t.Fatalf("incorrect data copied:\nexpected: %v\nactual: %v", expected, dst.Bytes())
+			}
+		})
+	}
+}