clients: capabilities improvement

7 files changed, 95 insertions, 63 deletions

diff --git a/clients/common/common.go b/clients/common/common.go
index 2c9a37e..9abb8eb 100644
--- a/clients/common/common.go
+++ b/clients/common/common.go
@@ -4,7 +4,6 @@ import (
 	"errors"
 	"fmt"
 	"io/ioutil"
-	"net/url"
 	"strings"
 
 	"gopkg.in/src-d/go-git.v2/core"
@@ -42,15 +41,26 @@ func (e Endpoint) Service(name string) string {
 
 // Capabilities contains all the server capabilities
 // https://github.com/git/git/blob/master/Documentation/technical/protocol-capabilities.txt
-type Capabilities map[string][]string
+type Capabilities struct {
+	m map[string]*Capability
+	o []string
+}
 
-func parseCapabilities(line string) Capabilities {
-	values, _ := url.ParseQuery(strings.Replace(line, " ", "&", -1))
+// Capability represents a server capability
+type Capability struct {
+	Name   string
+	Values []string
+}
 
-	return Capabilities(values)
+// NewCapabilities returns a new Capabilities struct
+func NewCapabilities() *Capabilities {
+	return &Capabilities{
+		m: make(map[string]*Capability, 0),
+	}
 }
 
-func (c Capabilities) Decode(raw string) {
+// Decode decodes a string
+func (c *Capabilities) Decode(raw string) {
 	parts := strings.SplitN(raw, "HEAD", 2)
 	if len(parts) == 2 {
 		raw = parts[1]
@@ -65,43 +75,42 @@ func (c Capabilities) Decode(raw string) {
 			value = s[1]
 		}
 
-		c[s[0]] = append(c[s[0]], value)
+		c.Add(s[0], value)
 	}
 }
 
-func (c Capabilities) String() string {
-	var o string
-	for key, values := range c {
-		if len(values) == 0 {
-			o += key + " "
-		}
+// 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)
+	}
 
-		for _, value := range values {
-			o += fmt.Sprintf("%s=%s ", key, value)
-		}
+	if len(values) == 0 {
+		return
 	}
 
-	return o[:len(o)-1]
+	c.m[capability].Values = append(c.m[capability].Values, values...)
 }
 
-// Supports returns true if capability is preent
-func (r Capabilities) Supports(capability string) bool {
-	_, ok := r[capability]
+// Supports returns true if capability is present
+func (c *Capabilities) Supports(capability string) bool {
+	_, ok := c.m[capability]
 	return ok
 }
 
 // Get returns the values for a capability
-func (r Capabilities) Get(capability string) []string {
-	return r[capability]
+func (c *Capabilities) Get(capability string) *Capability {
+	return c.m[capability]
 }
 
 // SymbolicReference returns the reference for a given symbolic reference
-func (r Capabilities) SymbolicReference(sym string) string {
-	if !r.Supports("symref") {
+func (c *Capabilities) SymbolicReference(sym string) string {
+	if !c.Supports("symref") {
 		return ""
 	}
 
-	for _, symref := range r.Get("symref") {
+	for _, symref := range c.Get("symref").Values {
 		parts := strings.Split(symref, ":")
 		if len(parts) != 2 {
 			continue
@@ -115,23 +124,46 @@ func (r Capabilities) SymbolicReference(sym string) string {
 	return ""
 }
 
+func (c *Capabilities) String() string {
+	if len(c.o) == 0 {
+		return ""
+	}
+
+	var o string
+	for _, key := range c.o {
+		cap := c.m[key]
+		if len(cap.Values) == 0 {
+			o += key + " "
+		}
+
+		for _, value := range cap.Values {
+			o += fmt.Sprintf("%s=%s ", key, value)
+		}
+	}
+
+	return o[:len(o)-1]
+}
+
 type GitUploadPackInfo struct {
-	Capabilities Capabilities
+	Capabilities *Capabilities
 	Head         string
 	Refs         map[string]core.Hash
 }
 
-func NewGitUploadPackInfo(d *pktline.Decoder) (*GitUploadPackInfo, error) {
-	info := &GitUploadPackInfo{}
-	if err := info.read(d); err != nil {
+func NewGitUploadPackInfo() *GitUploadPackInfo {
+	return &GitUploadPackInfo{Capabilities: NewCapabilities()}
+}
+
+func (r *GitUploadPackInfo) Decode(d *pktline.Decoder) error {
+	if err := r.read(d); err != nil {
 		if err == EmptyGitUploadPackErr {
-			return nil, core.NewPermanentError(err)
+			return core.NewPermanentError(err)
 		}
 
-		return nil, core.NewUnexpectedError(err)
+		return core.NewUnexpectedError(err)
 	}
 
-	return info, nil
+	return nil
 }
 
 func (r *GitUploadPackInfo) read(d *pktline.Decoder) error {
@@ -147,8 +179,7 @@ func (r *GitUploadPackInfo) read(d *pktline.Decoder) error {
 			continue
 		}
 
-		if r.Capabilities == nil {
-			r.Capabilities = Capabilities{}
+		if len(r.Capabilities.o) == 0 {
 			r.Capabilities.Decode(line)
 			continue
 		}
@@ -189,7 +220,7 @@ func (r *GitUploadPackInfo) Bytes() []byte {
 	e := pktline.NewEncoder()
 	e.AddLine("# service=git-upload-pack")
 	e.AddFlush()
-	e.AddLine(fmt.Sprintf("%s HEAD%s", r.Refs[r.Head], r.Capabilities.String()))
+	e.AddLine(fmt.Sprintf("%s HEAD\x00%s", r.Refs[r.Head].String(), r.Capabilities.String()))
 
 	for name, id := range r.Refs {
 		e.AddLine(fmt.Sprintf("%s %s", id, name))
diff --git a/clients/common/common_test.go b/clients/common/common_test.go
index 494a03a..7215c17 100644
--- a/clients/common/common_test.go
+++ b/clients/common/common_test.go
@@ -36,7 +36,8 @@ func (s *SuiteCommon) TestEndpointService(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 := parseCapabilities(CapabilitiesFixture)
+	cap := NewCapabilities()
+	cap.Decode(CapabilitiesFixture)
 	c.Assert(cap.SymbolicReference("HEAD"), Equals, "refs/heads/master")
 }
 
@@ -44,42 +45,44 @@ const GitUploadPackInfoFixture = "MDAxZSMgc2VydmljZT1naXQtdXBsb2FkLXBhY2sKMDAwMD
 
 func (s *SuiteCommon) TestGitUploadPackInfo(c *C) {
 	b, _ := base64.StdEncoding.DecodeString(GitUploadPackInfoFixture)
-	info, err := NewGitUploadPackInfo(pktline.NewDecoder(bytes.NewBuffer(b)))
+
+	i := NewGitUploadPackInfo()
+	err := i.Decode(pktline.NewDecoder(bytes.NewBuffer(b)))
 	c.Assert(err, IsNil)
 
-	ref := info.Capabilities.SymbolicReference("HEAD")
+	ref := i.Capabilities.SymbolicReference("HEAD")
 	c.Assert(ref, Equals, "refs/heads/master")
-	c.Assert(info.Refs[ref].String(), Equals, "6ecf0ef2c2dffb796033e5a02219af86ec6584e5")
+	c.Assert(i.Refs[ref].String(), Equals, "6ecf0ef2c2dffb796033e5a02219af86ec6584e5")
 }
 
 func (s *SuiteCommon) TestGitUploadPackInfoEmpty(c *C) {
 	b := bytes.NewBuffer(nil)
-	_, err := NewGitUploadPackInfo(pktline.NewDecoder(b))
+
+	i := NewGitUploadPackInfo()
+	err := i.Decode(pktline.NewDecoder(b))
 	c.Assert(err, ErrorMatches, "permanent.*empty.*")
 }
 
 func (s *SuiteCommon) TestCapabilitiesDecode(c *C) {
-	cap := Capabilities{}
+	cap := NewCapabilities()
 	cap.Decode("symref=foo symref=qux thin-pack")
 
-	c.Assert(cap, HasLen, 2)
-	c.Assert(cap["symref"], DeepEquals, []string{"foo", "qux"})
-	c.Assert(cap["thin-pack"], DeepEquals, []string{""})
+	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) TestCapabilitiesString(c *C) {
-	cap := Capabilities{
-		"symref": []string{"foo", "qux"},
-	}
+	cap := NewCapabilities()
+	cap.Add("symref", "foo", "qux")
+	cap.Add("thin-pack")
 
-	c.Assert(cap.String(), Equals, "symref=foo symref=qux")
+	c.Assert(cap.String(), Equals, "symref=foo symref=qux thin-pack")
 }
 
 func (s *SuiteCommon) TestGitUploadPackEncode(c *C) {
-	info := &GitUploadPackInfo{}
-	info.Capabilities = map[string][]string{
-		"symref": []string{"HEAD:refs/heads/master"},
-	}
+	info := NewGitUploadPackInfo()
+	info.Capabilities.Add("symref", "HEAD:refs/heads/master")
 
 	info.Head = "refs/heads/master"
 	info.Refs = map[string]core.Hash{
@@ -88,7 +91,7 @@ func (s *SuiteCommon) TestGitUploadPackEncode(c *C) {
 
 	c.Assert(info.String(), Equals,
 		"001e# service=git-upload-pack\n"+
-			"0000004f6ecf0ef2c2dffb796033e5a02219af86ec6584e5 HEADsymref=HEAD:refs/heads/master\n"+
+			"000000506ecf0ef2c2dffb796033e5a02219af86ec6584e5 HEAD\x00symref=HEAD:refs/heads/master\n"+
 			"003f6ecf0ef2c2dffb796033e5a02219af86ec6584e5 refs/heads/master\n"+
 			"0000",
 	)
diff --git a/clients/http/git_upload_pack.go b/clients/http/git_upload_pack.go
index bc99fd8..6960be1 100644
--- a/clients/http/git_upload_pack.go
+++ b/clients/http/git_upload_pack.go
@@ -38,8 +38,8 @@ func (s *GitUploadPackService) Info() (*common.GitUploadPackInfo, error) {
 
 	defer res.Body.Close()
 
-	dec := pktline.NewDecoder(res.Body)
-	return common.NewGitUploadPackInfo(dec)
+	i := common.NewGitUploadPackInfo()
+	return i, i.Decode(pktline.NewDecoder(res.Body))
 }
 
 func (s *GitUploadPackService) Fetch(r *common.GitUploadPackRequest) (io.ReadCloser, error) {
diff --git a/clients/http/git_upload_pack_test.go b/clients/http/git_upload_pack_test.go
index c90a259..b870259 100644
--- a/clients/http/git_upload_pack_test.go
+++ b/clients/http/git_upload_pack_test.go
@@ -34,7 +34,7 @@ func (s *SuiteRemote) TestCapabilities(c *C) {
 
 	info, err := r.Info()
 	c.Assert(err, IsNil)
-	c.Assert(info.Capabilities.Get("agent"), HasLen, 1)
+	c.Assert(info.Capabilities.Get("agent").Values, HasLen, 1)
 }
 
 func (s *SuiteRemote) TestFetch(c *C) {
diff --git a/common_test.go b/common_test.go
index d8647ff..1716813 100644
--- a/common_test.go
+++ b/common_test.go
@@ -2,9 +2,7 @@ package git
 
 import (
 	"io"
-	"net/url"
 	"os"
-	"strings"
 	"testing"
 
 	. "gopkg.in/check.v1"
@@ -23,11 +21,11 @@ func (s *MockGitUploadPackService) Connect(url common.Endpoint) error {
 func (s *MockGitUploadPackService) Info() (*common.GitUploadPackInfo, error) {
 	hash := core.NewHash("6ecf0ef2c2dffb796033e5a02219af86ec6584e5")
 
-	line := "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"
-	values, _ := url.ParseQuery(strings.Replace(line, " ", "&", -1))
+	cap := common.NewCapabilities()
+	cap.Decode("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")
 
 	return &common.GitUploadPackInfo{
-		Capabilities: common.Capabilities(values),
+		Capabilities: cap,
 		Head:         "refs/heads/master",
 		Refs:         map[string]core.Hash{"refs/heads/master": hash},
 	}, nil
diff --git a/remote.go b/remote.go
index 1d92ac6..1914e67 100644
--- a/remote.go
+++ b/remote.go
@@ -44,7 +44,7 @@ func (r *Remote) Connect() error {
 }
 
 // Capabilities returns the remote capabilities
-func (r *Remote) Capabilities() common.Capabilities {
+func (r *Remote) Capabilities() *common.Capabilities {
 	return r.upInfo.Capabilities
 }
 
diff --git a/remote_test.go b/remote_test.go
index 6bfa139..c222e7d 100644
--- a/remote_test.go
+++ b/remote_test.go
@@ -34,7 +34,7 @@ func (s *SuiteRemote) TestCapabilities(c *C) {
 
 	c.Assert(err, IsNil)
 	c.Assert(r.Connect(), IsNil)
-	c.Assert(r.Capabilities().Get("agent"), HasLen, 1)
+	c.Assert(r.Capabilities().Get("agent").Values, HasLen, 1)
 }
 
 func (s *SuiteRemote) TestFetchDefaultBranch(c *C) {