Use advrefs in gituploadpackinfo (#92)

* add advrefs encoder and parser

* modify advrefs encoder to resemble json encoder

* turn advrefs parser into a decoder

* clean code

* improve documentation

* improve documentation

* clean code

* upgrade to new pktline.Add and add Flush const to easy integration

* gometalinter

* Use packp/advrefs for GitUploadPackInfo parsing

- GitUploadPackInfo now uses packp/advrefs instead of parsing the
  message by itself.

- Capabilities has been moved from clients/common to packp to avoid a
  circular import.

- Cleaning of advrefs_test code.

- Add support for prefix encoding and decoding in advrefs.

* clean advrefs test code

* clean advrefs test code

* clean advrefs test code

* gometalinter

* add pktline encoder

* change pktline.EncodeFlush to pktline.Flush

* make scanner tests use the encoder instead of Pktlines

* check errors on flush and clean constants

* ubstitute the PktLines type with a pktline.Encoder

* use pktline.Encoder in all go-git

* add example of pktline.Encodef()

* add package overview

* documentation

* support symbolic links other than HEAD

* simplify decoding of shallows

* packp: fix mcuadros comments

- all abbreviates removed (by visual inspection, some may remain)
- all empty maps are initialized using make
- simplify readRef with a switch
- make decodeShallow malformed error more verbose
- add pktline.Encoder.encodeLine
- remove infamous panic in checkPayloadLength by refactoring out
  the whole function

4 files changed, 69 insertions, 247 deletions

diff --git a/clients/common/common.go b/clients/common/common.go
index 0179bfd..c723cdd 100644
--- a/clients/common/common.go
+++ b/clients/common/common.go
@@ -2,6 +2,7 @@
 package common
 
 import (
+	"bytes"
 	"errors"
 	"fmt"
 	"io"
@@ -11,6 +12,8 @@ import (
 	"strings"
 
 	"gopkg.in/src-d/go-git.v4/core"
+	"gopkg.in/src-d/go-git.v4/formats/packp"
+	"gopkg.in/src-d/go-git.v4/formats/packp/advrefs"
 	"gopkg.in/src-d/go-git.v4/formats/packp/pktline"
 	"gopkg.in/src-d/go-git.v4/storage/memory"
 )
@@ -75,246 +78,100 @@ func (e *Endpoint) String() string {
 	return u.String()
 }
 
-// Capabilities contains all the server capabilities
-// https://github.com/git/git/blob/master/Documentation/technical/protocol-capabilities.txt
-type Capabilities struct {
-	m map[string]*Capability
-	o []string
-}
-
-// Capability represents a server capability
-type Capability struct {
-	Name   string
-	Values []string
+type GitUploadPackInfo struct {
+	Capabilities *packp.Capabilities
+	Refs         memory.ReferenceStorage
 }
 
-// NewCapabilities returns a new Capabilities struct
-func NewCapabilities() *Capabilities {
-	return &Capabilities{
-		m: make(map[string]*Capability, 0),
-	}
+func NewGitUploadPackInfo() *GitUploadPackInfo {
+	return &GitUploadPackInfo{Capabilities: packp.NewCapabilities()}
 }
 
-// Decode decodes a string
-func (c *Capabilities) Decode(raw string) {
-	params := strings.Split(raw, " ")
-	for _, p := range params {
-		s := strings.SplitN(p, "=", 2)
-
-		var value string
-		if len(s) == 2 {
-			value = s[1]
+func (i *GitUploadPackInfo) Decode(r io.Reader) error {
+	d := advrefs.NewDecoder(r)
+	ar := advrefs.New()
+	if err := d.Decode(ar); err != nil {
+		if err == advrefs.ErrEmpty {
+			return core.NewPermanentError(err)
 		}
-
-		c.Add(s[0], value)
-	}
-}
-
-// Get returns the values for a capability
-func (c *Capabilities) Get(capability string) *Capability {
-	return c.m[capability]
-}
-
-// Set sets a capability removing the values
-func (c *Capabilities) Set(capability string, values ...string) {
-	if _, ok := c.m[capability]; ok {
-		delete(c.m, capability)
+		return core.NewUnexpectedError(err)
 	}
 
-	c.Add(capability, values...)
-}
-
-// Add adds a capability, values are optional
-func (c *Capabilities) Add(capability string, values ...string) {
-	if !c.Supports(capability) {
-		c.m[capability] = &Capability{Name: capability}
-		c.o = append(c.o, capability)
-	}
+	i.Capabilities = ar.Capabilities
 
-	if len(values) == 0 {
-		return
+	if err := i.addRefs(ar); err != nil {
+		return core.NewUnexpectedError(err)
 	}
 
-	c.m[capability].Values = append(c.m[capability].Values, values...)
-}
-
-// Supports returns true if capability is present
-func (c *Capabilities) Supports(capability string) bool {
-	_, ok := c.m[capability]
-	return ok
+	return nil
 }
 
-// SymbolicReference returns the reference for a given symbolic reference
-func (c *Capabilities) SymbolicReference(sym string) string {
-	if !c.Supports("symref") {
-		return ""
-	}
-
-	for _, symref := range c.Get("symref").Values {
-		parts := strings.Split(symref, ":")
-		if len(parts) != 2 {
-			continue
-		}
-
-		if parts[0] == sym {
-			return parts[1]
-		}
+func (i *GitUploadPackInfo) addRefs(ar *advrefs.AdvRefs) error {
+	i.Refs = make(memory.ReferenceStorage, 0)
+	for name, hash := range ar.References {
+		ref := core.NewReferenceFromStrings(name, hash.String())
+		i.Refs.Set(ref)
 	}
 
-	return ""
+	return i.addSymbolicRefs(ar)
 }
 
-func (c *Capabilities) String() string {
-	if len(c.o) == 0 {
-		return ""
-	}
-
-	var o string
-	for _, key := range c.o {
-		cap := c.m[key]
-
-		added := false
-		for _, value := range cap.Values {
-			if value == "" {
-				continue
-			}
-
-			added = true
-			o += fmt.Sprintf("%s=%s ", key, value)
-		}
-
-		if len(cap.Values) == 0 || !added {
-			o += key + " "
-		}
-	}
-
-	if len(o) == 0 {
-		return o
+func (i *GitUploadPackInfo) addSymbolicRefs(ar *advrefs.AdvRefs) error {
+	if !hasSymrefs(ar) {
+		return nil
 	}
 
-	return o[:len(o)-1]
-}
-
-type GitUploadPackInfo struct {
-	Capabilities *Capabilities
-	Refs         memory.ReferenceStorage
-}
-
-func NewGitUploadPackInfo() *GitUploadPackInfo {
-	return &GitUploadPackInfo{Capabilities: NewCapabilities()}
-}
-
-func (r *GitUploadPackInfo) Decode(s *pktline.Scanner) error {
-	if err := r.read(s); err != nil {
-		if err == ErrEmptyGitUploadPack {
-			return core.NewPermanentError(err)
+	for _, symref := range ar.Capabilities.Get("symref").Values {
+		chunks := strings.Split(symref, ":")
+		if len(chunks) != 2 {
+			err := fmt.Errorf("bad number of `:` in symref value (%q)", symref)
+			return core.NewUnexpectedError(err)
 		}
-
-		return core.NewUnexpectedError(err)
+		name := core.ReferenceName(chunks[0])
+		target := core.ReferenceName(chunks[1])
+		ref := core.NewSymbolicReference(name, target)
+		i.Refs.Set(ref)
 	}
 
 	return nil
 }
 
-func (r *GitUploadPackInfo) read(s *pktline.Scanner) error {
-	isEmpty := true
-	r.Refs = make(memory.ReferenceStorage, 0)
-	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
-		}
-
-		if err := r.readLine(line); err != nil {
-			return err
-		}
-
-		isEmpty = false
-	}
-
-	if isEmpty {
-		return ErrEmptyGitUploadPack
-	}
-
-	return s.Err()
+func hasSymrefs(ar *advrefs.AdvRefs) bool {
+	return ar.Capabilities.Supports("symref")
 }
 
-func isSmartHttpComment(line string) bool {
-	return line[0] == '#'
+func (i *GitUploadPackInfo) Head() *core.Reference {
+	ref, _ := core.ResolveReference(i.Refs, core.HEAD)
+	return ref
 }
 
-func (r *GitUploadPackInfo) readLine(line string) error {
-	hashEnd := strings.Index(line, " ")
-	hash := line[:hashEnd]
-
-	zeroID := strings.Index(line, string([]byte{0}))
-	if zeroID == -1 {
-		name := line[hashEnd+1 : len(line)-1]
-		ref := core.NewReferenceFromStrings(name, hash)
-		return r.Refs.Set(ref)
-	}
-
-	name := line[hashEnd+1 : zeroID]
-	r.Capabilities.Decode(line[zeroID+1 : len(line)-1])
-	if !r.Capabilities.Supports("symref") {
-		ref := core.NewReferenceFromStrings(name, hash)
-		return r.Refs.Set(ref)
-	}
-
-	target := r.Capabilities.SymbolicReference(name)
-	ref := core.NewSymbolicReference(core.ReferenceName(name), core.ReferenceName(target))
-	return r.Refs.Set(ref)
+func (i *GitUploadPackInfo) String() string {
+	return string(i.Bytes())
 }
 
-func (r *GitUploadPackInfo) Head() *core.Reference {
-	ref, _ := core.ResolveReference(r.Refs, core.HEAD)
-	return ref
-}
+func (i *GitUploadPackInfo) Bytes() []byte {
+	var buf bytes.Buffer
+	e := pktline.NewEncoder(&buf)
 
-func (r *GitUploadPackInfo) String() string {
-	return string(r.Bytes())
-}
+	_ = e.EncodeString("# service=git-upload-pack\n")
 
-func (r *GitUploadPackInfo) Bytes() []byte {
-	p := pktline.New()
-	_ = p.AddString("# service=git-upload-pack\n")
 	// inserting a flush-pkt here violates the protocol spec, but some
 	// servers do it, like Github.com
-	p.AddFlush()
+	e.Flush()
 
-	firstLine := fmt.Sprintf("%s HEAD\x00%s\n", r.Head().Hash(), r.Capabilities.String())
-	_ = p.AddString(firstLine)
+	_ = e.Encodef("%s HEAD\x00%s\n", i.Head().Hash(), i.Capabilities.String())
 
-	for _, ref := range r.Refs {
+	for _, ref := range i.Refs {
 		if ref.Type() != core.HashReference {
 			continue
 		}
 
-		ref := fmt.Sprintf("%s %s\n", ref.Hash(), ref.Name())
-		_ = p.AddString(ref)
+		_ = e.Encodef("%s %s\n", ref.Hash(), ref.Name())
 	}
 
-	p.AddFlush()
-	b, _ := ioutil.ReadAll(p)
+	e.Flush()
 
-	return b
+	return buf.Bytes()
 }
 
 type GitUploadPackRequest struct {
@@ -337,24 +194,23 @@ func (r *GitUploadPackRequest) String() string {
 }
 
 func (r *GitUploadPackRequest) Reader() *strings.Reader {
-	p := pktline.New()
+	var buf bytes.Buffer
+	e := pktline.NewEncoder(&buf)
 
 	for _, want := range r.Wants {
-		_ = p.AddString(fmt.Sprintf("want %s\n", want))
+		_ = e.Encodef("want %s\n", want)
 	}
 
 	for _, have := range r.Haves {
-		_ = p.AddString(fmt.Sprintf("have %s\n", have))
+		_ = e.Encodef("have %s\n", have)
 	}
 
 	if r.Depth != 0 {
-		_ = p.AddString(fmt.Sprintf("deepen %d\n", r.Depth))
+		_ = e.Encodef("deepen %d\n", r.Depth)
 	}
 
-	p.AddFlush()
-	_ = p.AddString("done\n")
-
-	b, _ := ioutil.ReadAll(p)
+	_ = e.Flush()
+	_ = e.EncodeString("done\n")
 
-	return strings.NewReader(string(b))
+	return strings.NewReader(buf.String())
 }
diff --git a/clients/common/common_test.go b/clients/common/common_test.go
index 66a49e1..5809584 100644
--- a/clients/common/common_test.go
+++ b/clients/common/common_test.go
@@ -6,7 +6,7 @@ import (
 	"testing"
 
 	"gopkg.in/src-d/go-git.v4/core"
-	"gopkg.in/src-d/go-git.v4/formats/packp/pktline"
+	"gopkg.in/src-d/go-git.v4/formats/packp"
 
 	. "gopkg.in/check.v1"
 )
@@ -38,7 +38,7 @@ func (s *SuiteCommon) TestNewEndpointWrongForgat(c *C) {
 const CapabilitiesFixture = "6ecf0ef2c2dffb796033e5a02219af86ec6584e5 HEADmulti_ack thin-pack side-band side-band-64k ofs-delta shallow no-progress include-tag multi_ack_detailed no-done symref=HEAD:refs/heads/master agent=git/2:2.4.8~dbussink-fix-enterprise-tokens-compilation-1167-gc7006cf"
 
 func (s *SuiteCommon) TestCapabilitiesSymbolicReference(c *C) {
-	cap := NewCapabilities()
+	cap := packp.NewCapabilities()
 	cap.Decode(CapabilitiesFixture)
 	c.Assert(cap.SymbolicReference("HEAD"), Equals, "refs/heads/master")
 }
@@ -49,7 +49,7 @@ func (s *SuiteCommon) TestGitUploadPackInfo(c *C) {
 	b, _ := base64.StdEncoding.DecodeString(GitUploadPackInfoFixture)
 
 	i := NewGitUploadPackInfo()
-	err := i.Decode(pktline.NewScanner(bytes.NewBuffer(b)))
+	err := i.Decode(bytes.NewBuffer(b))
 	c.Assert(err, IsNil)
 
 	name := i.Capabilities.SymbolicReference("HEAD")
@@ -71,7 +71,7 @@ func (s *SuiteCommon) TestGitUploadPackInfoNoHEAD(c *C) {
 	b, _ := base64.StdEncoding.DecodeString(GitUploadPackInfoNoHEADFixture)
 
 	i := NewGitUploadPackInfo()
-	err := i.Decode(pktline.NewScanner(bytes.NewBuffer(b)))
+	err := i.Decode(bytes.NewBuffer(b))
 	c.Assert(err, IsNil)
 
 	name := i.Capabilities.SymbolicReference("HEAD")
@@ -87,43 +87,10 @@ func (s *SuiteCommon) TestGitUploadPackInfoEmpty(c *C) {
 	b := bytes.NewBuffer(nil)
 
 	i := NewGitUploadPackInfo()
-	err := i.Decode(pktline.NewScanner(b))
+	err := i.Decode(b)
 	c.Assert(err, ErrorMatches, "permanent.*empty.*")
 }
 
-func (s *SuiteCommon) TestCapabilitiesDecode(c *C) {
-	cap := NewCapabilities()
-	cap.Decode("symref=foo symref=qux thin-pack")
-
-	c.Assert(cap.m, HasLen, 2)
-	c.Assert(cap.Get("symref").Values, DeepEquals, []string{"foo", "qux"})
-	c.Assert(cap.Get("thin-pack").Values, DeepEquals, []string{""})
-}
-
-func (s *SuiteCommon) TestCapabilitiesSet(c *C) {
-	cap := NewCapabilities()
-	cap.Add("symref", "foo", "qux")
-	cap.Set("symref", "bar")
-
-	c.Assert(cap.m, HasLen, 1)
-	c.Assert(cap.Get("symref").Values, DeepEquals, []string{"bar"})
-}
-
-func (s *SuiteCommon) TestCapabilitiesSetEmpty(c *C) {
-	cap := NewCapabilities()
-	cap.Set("foo", "bar")
-
-	c.Assert(cap.Get("foo").Values, HasLen, 1)
-}
-
-func (s *SuiteCommon) TestCapabilitiesAdd(c *C) {
-	cap := NewCapabilities()
-	cap.Add("symref", "foo", "qux")
-	cap.Add("thin-pack")
-
-	c.Assert(cap.String(), Equals, "symref=foo symref=qux thin-pack")
-}
-
 func (s *SuiteCommon) TestGitUploadPackEncode(c *C) {
 	info := NewGitUploadPackInfo()
 	info.Capabilities.Add("symref", "HEAD:refs/heads/master")
diff --git a/clients/http/git_upload_pack.go b/clients/http/git_upload_pack.go
index eb8db0b..f652150 100644
--- a/clients/http/git_upload_pack.go
+++ b/clients/http/git_upload_pack.go
@@ -78,7 +78,7 @@ func (s *GitUploadPackService) Info() (*common.GitUploadPackInfo, error) {
 	defer res.Body.Close()
 
 	i := common.NewGitUploadPackInfo()
-	return i, i.Decode(pktline.NewScanner(res.Body))
+	return i, i.Decode(res.Body)
 }
 
 // Fetch request and returns a reader to a packfile
diff --git a/clients/ssh/git_upload_pack.go b/clients/ssh/git_upload_pack.go
index 513e528..3bdb64f 100644
--- a/clients/ssh/git_upload_pack.go
+++ b/clients/ssh/git_upload_pack.go
@@ -11,7 +11,6 @@ import (
 	"strings"
 
 	"gopkg.in/src-d/go-git.v4/clients/common"
-	"gopkg.in/src-d/go-git.v4/formats/packp/pktline"
 
 	"golang.org/x/crypto/ssh"
 )
@@ -123,7 +122,7 @@ func (s *GitUploadPackService) Info() (i *common.GitUploadPackInfo, err error) {
 	}
 
 	i = common.NewGitUploadPackInfo()
-	return i, i.Decode(pktline.NewScanner(bytes.NewReader(out)))
+	return i, i.Decode(bytes.NewReader(out))
 }
 
 // Disconnect the SSH client.