Substitute old pktline encoder/decoder with new pktline scanner (#84)

* replace old pktline package with new pktline scanner

* remove error checks on pktline.NewFromString

* fix deppend bug

* reduce memory garbage when pktline.NewFromStrings

* improve int to hex conversion to help gc

* make intToHex func private

* clean function names

Author: alcortesm

clients/common/common.go

@@ -11,7 +11,7 @@ import (
 	"strings"
 
 	"gopkg.in/src-d/go-git.v4/core"
-	"gopkg.in/src-d/go-git.v4/formats/pktline"
+	"gopkg.in/src-d/go-git.v4/formats/packp/pktline"
 	"gopkg.in/src-d/go-git.v4/storage/memory"
 )
 
@@ -204,8 +204,8 @@ func NewGitUploadPackInfo() *GitUploadPackInfo {
 	return &GitUploadPackInfo{Capabilities: NewCapabilities()}
 }
 
-func (r *GitUploadPackInfo) Decode(d *pktline.Decoder) error {
-	if err := r.read(d); err != nil {
+func (r *GitUploadPackInfo) Decode(s *pktline.Scanner) error {
+	if err := r.read(s); err != nil {
 		if err == ErrEmptyGitUploadPack {
 			return core.NewPermanentError(err)
 		}
@@ -216,16 +216,29 @@ func (r *GitUploadPackInfo) Decode(d *pktline.Decoder) error {
 	return nil
 }
 
-func (r *GitUploadPackInfo) read(d *pktline.Decoder) error {
-	lines, err := d.ReadAll()
-	if err != nil {
-		return err
-	}
-
+func (r *GitUploadPackInfo) read(s *pktline.Scanner) error {
 	isEmpty := true
 	r.Refs = make(memory.ReferenceStorage, 0)
-	for _, line := range lines {
-		if !r.isValidLine(line) {
+	smartCommentIgnore := false
+	for s.Scan() {
+		line := string(s.Bytes())
+
+		if smartCommentIgnore {
+			// some servers like Github add a flush-pkt after the smart http comment
+			// that we must ignore to prevent a premature termination of the read.
+			if len(line) == 0 {
+				continue
+			}
+			smartCommentIgnore = false
+		}
+
+		// exit on first flush-pkt
+		if len(line) == 0 {
+			break
+		}
+
+		if isSmartHttpComment(line) {
+			smartCommentIgnore = true
 			continue
 		}
 
@@ -240,11 +253,11 @@ func (r *GitUploadPackInfo) read(d *pktline.Decoder) error {
 		return ErrEmptyGitUploadPack
 	}
 
-	return nil
+	return s.Err()
 }
 
-func (r *GitUploadPackInfo) isValidLine(line string) bool {
-	return line[0] != '#'
+func isSmartHttpComment(line string) bool {
+	return line[0] == '#'
 }
 
 func (r *GitUploadPackInfo) readLine(line string) error {
@@ -280,21 +293,28 @@ func (r *GitUploadPackInfo) String() string {
 }
 
 func (r *GitUploadPackInfo) Bytes() []byte {
-	e := pktline.NewEncoder()
-	e.AddLine("# service=git-upload-pack")
-	e.AddFlush()
-	e.AddLine(fmt.Sprintf("%s HEAD\x00%s", r.Head().Hash(), r.Capabilities.String()))
+	payloads := []string{}
+	payloads = append(payloads, "# service=git-upload-pack\n")
+	// inserting a flush-pkt here violates the protocol spec, but some
+	// servers do it, like Github.com
+	payloads = append(payloads, "")
+
+	firstLine := fmt.Sprintf("%s HEAD\x00%s\n", r.Head().Hash(), r.Capabilities.String())
+	payloads = append(payloads, firstLine)
 
 	for _, ref := range r.Refs {
 		if ref.Type() != core.HashReference {
 			continue
 		}
 
-		e.AddLine(fmt.Sprintf("%s %s", ref.Hash(), ref.Name()))
+		ref := fmt.Sprintf("%s %s\n", ref.Hash(), ref.Name())
+		payloads = append(payloads, ref)
 	}
 
-	e.AddFlush()
-	b, _ := ioutil.ReadAll(e.Reader())
+	payloads = append(payloads, "")
+	pktlines, _ := pktline.NewFromStrings(payloads...)
+	b, _ := ioutil.ReadAll(pktlines)
+
 	return b
 }
 
@@ -318,21 +338,25 @@ func (r *GitUploadPackRequest) String() string {
 }
 
 func (r *GitUploadPackRequest) Reader() *strings.Reader {
-	e := pktline.NewEncoder()
+	payloads := []string{}
+
 	for _, want := range r.Wants {
-		e.AddLine(fmt.Sprintf("want %s", want))
+		payloads = append(payloads, fmt.Sprintf("want %s\n", want))
 	}
 
 	for _, have := range r.Haves {
-		e.AddLine(fmt.Sprintf("have %s", have))
+		payloads = append(payloads, fmt.Sprintf("have %s\n", have))
 	}
 
 	if r.Depth != 0 {
-		e.AddLine(fmt.Sprintf("deepen %d", r.Depth))
+		payloads = append(payloads, fmt.Sprintf("deepen %d\n", r.Depth))
 	}
 
-	e.AddFlush()
-	e.AddLine("done")
+	payloads = append(payloads, "")
+	payloads = append(payloads, "done\n")
+
+	pktlines, _ := pktline.NewFromStrings(payloads...)
+	b, _ := ioutil.ReadAll(pktlines)
 
-	return e.Reader()
+	return strings.NewReader(string(b))
 }

clients/common/common_test.go

@@ -5,9 +5,10 @@ import (
 	"encoding/base64"
 	"testing"
 
-	. "gopkg.in/check.v1"
 	"gopkg.in/src-d/go-git.v4/core"
-	"gopkg.in/src-d/go-git.v4/formats/pktline"
+	"gopkg.in/src-d/go-git.v4/formats/packp/pktline"
+
+	. "gopkg.in/check.v1"
 )
 
 func Test(t *testing.T) { TestingT(t) }
@@ -48,7 +49,7 @@ func (s *SuiteCommon) TestGitUploadPackInfo(c *C) {
 	b, _ := base64.StdEncoding.DecodeString(GitUploadPackInfoFixture)
 
 	i := NewGitUploadPackInfo()
-	err := i.Decode(pktline.NewDecoder(bytes.NewBuffer(b)))
+	err := i.Decode(pktline.NewScanner(bytes.NewBuffer(b)))
 	c.Assert(err, IsNil)
 
 	name := i.Capabilities.SymbolicReference("HEAD")
@@ -70,7 +71,7 @@ func (s *SuiteCommon) TestGitUploadPackInfoNoHEAD(c *C) {
 	b, _ := base64.StdEncoding.DecodeString(GitUploadPackInfoNoHEADFixture)
 
 	i := NewGitUploadPackInfo()
-	err := i.Decode(pktline.NewDecoder(bytes.NewBuffer(b)))
+	err := i.Decode(pktline.NewScanner(bytes.NewBuffer(b)))
 	c.Assert(err, IsNil)
 
 	name := i.Capabilities.SymbolicReference("HEAD")
@@ -86,7 +87,7 @@ func (s *SuiteCommon) TestGitUploadPackInfoEmpty(c *C) {
 	b := bytes.NewBuffer(nil)
 
 	i := NewGitUploadPackInfo()
-	err := i.Decode(pktline.NewDecoder(b))
+	err := i.Decode(pktline.NewScanner(b))
 	c.Assert(err, ErrorMatches, "permanent.*empty.*")
 }
 

clients/http/git_upload_pack.go

@@ -2,14 +2,15 @@ package http
 
 import (
 	"bufio"
+	"bytes"
 	"fmt"
 	"io"
 	"net/http"
 	"strings"
 
 	"gopkg.in/src-d/go-git.v4/clients/common"
 	"gopkg.in/src-d/go-git.v4/core"
-	"gopkg.in/src-d/go-git.v4/formats/pktline"
+	"gopkg.in/src-d/go-git.v4/formats/packp/pktline"
 )
 
 // GitUploadPackService git-upoad-pack service over HTTP
@@ -77,7 +78,7 @@ func (s *GitUploadPackService) Info() (*common.GitUploadPackInfo, error) {
 	defer res.Body.Close()
 
 	i := common.NewGitUploadPackInfo()
-	return i, i.Decode(pktline.NewDecoder(res.Body))
+	return i, i.Decode(pktline.NewScanner(res.Body))
 }
 
 // Fetch request and returns a reader to a packfile
@@ -101,27 +102,22 @@ func (s *GitUploadPackService) Fetch(r *common.GitUploadPackRequest) (io.ReadClo
 		return nil, err
 	}
 
-	if err := s.discardResponseInfo(reader); err != nil {
+	if err := discardResponseInfo(reader); err != nil {
 		return nil, err
 	}
 
 	return reader, nil
 }
 
-func (s *GitUploadPackService) discardResponseInfo(r io.Reader) error {
-	decoder := pktline.NewDecoder(r)
-	for {
-		line, err := decoder.ReadLine()
-		if err != nil {
-			break
-		}
-
-		if line == "NAK\n" {
+func discardResponseInfo(r io.Reader) error {
+	s := pktline.NewScanner(r)
+	for s.Scan() {
+		if bytes.Equal(s.Bytes(), []byte{'N', 'A', 'K', '\n'}) {
 			break
 		}
 	}
 
-	return nil
+	return s.Err()
 }
 
 func (s *GitUploadPackService) doRequest(method, url string, content *strings.Reader) (*http.Response, error) {

clients/ssh/git_upload_pack.go

@@ -11,7 +11,7 @@ import (
 	"strings"
 
 	"gopkg.in/src-d/go-git.v4/clients/common"
-	"gopkg.in/src-d/go-git.v4/formats/pktline"
+	"gopkg.in/src-d/go-git.v4/formats/packp/pktline"
 
 	"golang.org/x/crypto/ssh"
 )
@@ -123,7 +123,7 @@ func (s *GitUploadPackService) Info() (i *common.GitUploadPackInfo, err error) {
 	}
 
 	i = common.NewGitUploadPackInfo()
-	return i, i.Decode(pktline.NewDecoder(bytes.NewReader(out)))
+	return i, i.Decode(pktline.NewScanner(bytes.NewReader(out)))
 }
 
 // Disconnect the SSH client.

formats/packp/pktline/pktline.go

@@ -0,0 +1,114 @@
+// Package pktline implements reading and creating pkt-lines as per
+// https://github.com/git/git/blob/master/Documentation/technical/protocol-common.txt.
+package pktline
+
+import (
+	"bytes"
+	"errors"
+	"io"
+	"strings"
+)
+
+const (
+	// MaxPayloadSize is the maximum payload size of a pkt-line in bytes.
+	MaxPayloadSize = 65516
+)
+
+var (
+	flush = []byte{'0', '0', '0', '0'}
+)
+
+// PktLine values represent a succession of pkt-lines.
+// Values from this type are not zero-value safe, see the functions New
+// and NewFromString below.
+type PktLine struct {
+	io.Reader
+}
+
+// ErrPayloadTooLong is returned by New and NewFromString when any of
+// the provided payloads is bigger than MaxPayloadSize.
+var ErrPayloadTooLong = errors.New("payload is too long")
+
+// New returns the concatenation of several pkt-lines, each of them with
+// the payload specified by the contents of each input byte slice.  An
+// empty payload byte slice will produce a flush-pkt.
+func New(payloads ...[]byte) (PktLine, error) {
+	ret := []io.Reader{}
+	for _, p := range payloads {
+		if err := add(&ret, p); err != nil {
+			return PktLine{}, err
+		}
+	}
+
+	return PktLine{io.MultiReader(ret...)}, nil
+}
+
+func add(dst *[]io.Reader, e []byte) error {
+	if len(e) > MaxPayloadSize {
+		return ErrPayloadTooLong
+	}
+
+	if len(e) == 0 {
+		*dst = append(*dst, bytes.NewReader(flush))
+		return nil
+	}
+
+	n := len(e) + 4
+	*dst = append(*dst, bytes.NewReader(int16ToHex(n)))
+	*dst = append(*dst, bytes.NewReader(e))
+
+	return nil
+}
+
+// susbtitutes fmt.Sprintf("%04x", n) to avoid memory garbage
+// generation.
+func int16ToHex(n int) []byte {
+	var ret [4]byte
+	ret[0] = byteToAsciiHex(byte(n & 0xf000 >> 12))
+	ret[1] = byteToAsciiHex(byte(n & 0x0f00 >> 8))
+	ret[2] = byteToAsciiHex(byte(n & 0x00f0 >> 4))
+	ret[3] = byteToAsciiHex(byte(n & 0x000f))
+
+	return ret[:]
+}
+
+// turns a byte into its hexadecimal ascii representation.  Example:
+// from 11 (0xb) into 'b'.
+func byteToAsciiHex(n byte) byte {
+	if n < 10 {
+		return byte('0' + n)
+	}
+
+	return byte('a' - 10 + n)
+}
+
+// NewFromStrings returns the concatenation of several pkt-lines, each
+// of them with the payload specified by the contents of each input
+// string.  An empty payload string will produce a flush-pkt.
+func NewFromStrings(payloads ...string) (PktLine, error) {
+	ret := []io.Reader{}
+	for _, p := range payloads {
+		if err := addString(&ret, p); err != nil {
+			return PktLine{}, err
+		}
+	}
+
+	return PktLine{io.MultiReader(ret...)}, nil
+}
+
+func addString(dst *[]io.Reader, s string) error {
+	if len(s) > MaxPayloadSize {
+		return ErrPayloadTooLong
+	}
+
+	if len(s) == 0 {
+		*dst = append(*dst, bytes.NewReader(flush))
+		return nil
+	}
+
+	n := len(s) + 4
+	*dst = append(*dst, bytes.NewReader(int16ToHex(n)))
+	*dst = append(*dst, strings.NewReader(s))
+
+	return nil
+}