plumbing/protocol: paktp avoid duplication of haves, wants and shallow (#158)

4 files changed, 137 insertions, 42 deletions

diff --git a/plumbing/protocol/packp/ulreq_encode.go b/plumbing/protocol/packp/ulreq_encode.go
index b2ca491..4a26e74 100644
--- a/plumbing/protocol/packp/ulreq_encode.go
+++ b/plumbing/protocol/packp/ulreq_encode.go
@@ -1,9 +1,9 @@
 package packp
 
 import (
+	"bytes"
 	"fmt"
 	"io"
-	"sort"
 	"time"
 
 	"gopkg.in/src-d/go-git.v4/plumbing"
@@ -21,10 +21,9 @@ func (u *UploadRequest) Encode(w io.Writer) error {
 }
 
 type ulReqEncoder struct {
-	pe          *pktline.Encoder // where to write the encoded data
-	data        *UploadRequest   // the data to encode
-	sortedWants []string
-	err         error // sticky error
+	pe   *pktline.Encoder // where to write the encoded data
+	data *UploadRequest   // the data to encode
+	err  error            // sticky error
 }
 
 func newUlReqEncoder(w io.Writer) *ulReqEncoder {
@@ -34,13 +33,13 @@ func newUlReqEncoder(w io.Writer) *ulReqEncoder {
 }
 
 func (e *ulReqEncoder) Encode(v *UploadRequest) error {
+	e.data = v
+
 	if len(v.Wants) == 0 {
 		return fmt.Errorf("empty wants provided")
 	}
 
-	e.data = v
-	e.sortedWants = sortHashes(v.Wants)
-
+	plumbing.HashesSort(e.data.Wants)
 	for state := e.encodeFirstWant; state != nil; {
 		state = state()
 	}
@@ -48,27 +47,18 @@ func (e *ulReqEncoder) Encode(v *UploadRequest) error {
 	return e.err
 }
 
-func sortHashes(list []plumbing.Hash) []string {
-	sorted := make([]string, len(list))
-	for i, hash := range list {
-		sorted[i] = hash.String()
-	}
-	sort.Strings(sorted)
-
-	return sorted
-}
-
 func (e *ulReqEncoder) encodeFirstWant() stateFn {
 	var err error
 	if e.data.Capabilities.IsEmpty() {
-		err = e.pe.Encodef("want %s\n", e.sortedWants[0])
+		err = e.pe.Encodef("want %s\n", e.data.Wants[0])
 	} else {
 		err = e.pe.Encodef(
 			"want %s %s\n",
-			e.sortedWants[0],
+			e.data.Wants[0],
 			e.data.Capabilities.String(),
 		)
 	}
+
 	if err != nil {
 		e.err = fmt.Errorf("encoding first want line: %s", err)
 		return nil
@@ -78,23 +68,38 @@ func (e *ulReqEncoder) encodeFirstWant() stateFn {
 }
 
 func (e *ulReqEncoder) encodeAditionalWants() stateFn {
-	for _, w := range e.sortedWants[1:] {
+	last := e.data.Wants[0]
+	for _, w := range e.data.Wants[1:] {
+		if bytes.Compare(last[:], w[:]) == 0 {
+			continue
+		}
+
 		if err := e.pe.Encodef("want %s\n", w); err != nil {
 			e.err = fmt.Errorf("encoding want %q: %s", w, err)
 			return nil
 		}
+
+		last = w
 	}
 
 	return e.encodeShallows
 }
 
 func (e *ulReqEncoder) encodeShallows() stateFn {
-	sorted := sortHashes(e.data.Shallows)
-	for _, s := range sorted {
+	plumbing.HashesSort(e.data.Shallows)
+
+	var last plumbing.Hash
+	for _, s := range e.data.Shallows {
+		if bytes.Compare(last[:], s[:]) == 0 {
+			continue
+		}
+
 		if err := e.pe.Encodef("shallow %s\n", s); err != nil {
 			e.err = fmt.Errorf("encoding shallow %q: %s", s, err)
 			return nil
 		}
+
+		last = s
 	}
 
 	return e.encodeDepth
diff --git a/plumbing/protocol/packp/ulreq_encode_test.go b/plumbing/protocol/packp/ulreq_encode_test.go
index 3b3b6c2..b414a37 100644
--- a/plumbing/protocol/packp/ulreq_encode_test.go
+++ b/plumbing/protocol/packp/ulreq_encode_test.go
@@ -76,11 +76,13 @@ func (s *UlReqEncodeSuite) TestOneWantWithCapabilities(c *C) {
 
 func (s *UlReqEncodeSuite) TestWants(c *C) {
 	ur := NewUploadRequest()
-	ur.Wants = append(ur.Wants, plumbing.NewHash("4444444444444444444444444444444444444444"))
-	ur.Wants = append(ur.Wants, plumbing.NewHash("1111111111111111111111111111111111111111"))
-	ur.Wants = append(ur.Wants, plumbing.NewHash("3333333333333333333333333333333333333333"))
-	ur.Wants = append(ur.Wants, plumbing.NewHash("2222222222222222222222222222222222222222"))
-	ur.Wants = append(ur.Wants, plumbing.NewHash("5555555555555555555555555555555555555555"))
+	ur.Wants = append(ur.Wants,
+		plumbing.NewHash("4444444444444444444444444444444444444444"),
+		plumbing.NewHash("1111111111111111111111111111111111111111"),
+		plumbing.NewHash("3333333333333333333333333333333333333333"),
+		plumbing.NewHash("2222222222222222222222222222222222222222"),
+		plumbing.NewHash("5555555555555555555555555555555555555555"),
+	)
 
 	expected := []string{
 		"want 1111111111111111111111111111111111111111\n",
@@ -94,13 +96,37 @@ func (s *UlReqEncodeSuite) TestWants(c *C) {
 	testUlReqEncode(c, ur, expected)
 }
 
+func (s *UlReqEncodeSuite) TestWantsDuplicates(c *C) {
+	ur := NewUploadRequest()
+	ur.Wants = append(ur.Wants,
+		plumbing.NewHash("4444444444444444444444444444444444444444"),
+		plumbing.NewHash("1111111111111111111111111111111111111111"),
+		plumbing.NewHash("3333333333333333333333333333333333333333"),
+		plumbing.NewHash("1111111111111111111111111111111111111111"),
+		plumbing.NewHash("2222222222222222222222222222222222222222"),
+		plumbing.NewHash("1111111111111111111111111111111111111111"),
+	)
+
+	expected := []string{
+		"want 1111111111111111111111111111111111111111\n",
+		"want 2222222222222222222222222222222222222222\n",
+		"want 3333333333333333333333333333333333333333\n",
+		"want 4444444444444444444444444444444444444444\n",
+		pktline.FlushString,
+	}
+
+	testUlReqEncode(c, ur, expected)
+}
+
 func (s *UlReqEncodeSuite) TestWantsWithCapabilities(c *C) {
 	ur := NewUploadRequest()
-	ur.Wants = append(ur.Wants, plumbing.NewHash("4444444444444444444444444444444444444444"))
-	ur.Wants = append(ur.Wants, plumbing.NewHash("1111111111111111111111111111111111111111"))
-	ur.Wants = append(ur.Wants, plumbing.NewHash("3333333333333333333333333333333333333333"))
-	ur.Wants = append(ur.Wants, plumbing.NewHash("2222222222222222222222222222222222222222"))
-	ur.Wants = append(ur.Wants, plumbing.NewHash("5555555555555555555555555555555555555555"))
+	ur.Wants = append(ur.Wants,
+		plumbing.NewHash("4444444444444444444444444444444444444444"),
+		plumbing.NewHash("1111111111111111111111111111111111111111"),
+		plumbing.NewHash("3333333333333333333333333333333333333333"),
+		plumbing.NewHash("2222222222222222222222222222222222222222"),
+		plumbing.NewHash("5555555555555555555555555555555555555555"),
+	)
 
 	ur.Capabilities.Add(capability.MultiACK)
 	ur.Capabilities.Add(capability.OFSDelta)
@@ -139,10 +165,12 @@ func (s *UlReqEncodeSuite) TestManyShallows(c *C) {
 	ur := NewUploadRequest()
 	ur.Wants = append(ur.Wants, plumbing.NewHash("1111111111111111111111111111111111111111"))
 	ur.Capabilities.Add(capability.MultiACK)
-	ur.Shallows = append(ur.Shallows, plumbing.NewHash("bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb"))
-	ur.Shallows = append(ur.Shallows, plumbing.NewHash("dddddddddddddddddddddddddddddddddddddddd"))
-	ur.Shallows = append(ur.Shallows, plumbing.NewHash("cccccccccccccccccccccccccccccccccccccccc"))
-	ur.Shallows = append(ur.Shallows, plumbing.NewHash("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"))
+	ur.Shallows = append(ur.Shallows,
+		plumbing.NewHash("bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb"),
+		plumbing.NewHash("dddddddddddddddddddddddddddddddddddddddd"),
+		plumbing.NewHash("cccccccccccccccccccccccccccccccccccccccc"),
+		plumbing.NewHash("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"),
+	)
 
 	expected := []string{
 		"want 1111111111111111111111111111111111111111 multi_ack\n",
@@ -156,6 +184,28 @@ func (s *UlReqEncodeSuite) TestManyShallows(c *C) {
 	testUlReqEncode(c, ur, expected)
 }
 
+func (s *UlReqEncodeSuite) TestShallowsDuplicate(c *C) {
+	ur := NewUploadRequest()
+	ur.Wants = append(ur.Wants, plumbing.NewHash("1111111111111111111111111111111111111111"))
+	ur.Capabilities.Add(capability.MultiACK)
+	ur.Shallows = append(ur.Shallows,
+		plumbing.NewHash("bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb"),
+		plumbing.NewHash("cccccccccccccccccccccccccccccccccccccccc"),
+		plumbing.NewHash("cccccccccccccccccccccccccccccccccccccccc"),
+		plumbing.NewHash("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"),
+	)
+
+	expected := []string{
+		"want 1111111111111111111111111111111111111111 multi_ack\n",
+		"shallow aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n",
+		"shallow bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb\n",
+		"shallow cccccccccccccccccccccccccccccccccccccccc\n",
+		pktline.FlushString,
+	}
+
+	testUlReqEncode(c, ur, expected)
+}
+
 func (s *UlReqEncodeSuite) TestDepthCommits(c *C) {
 	ur := NewUploadRequest()
 	ur.Wants = append(ur.Wants, plumbing.NewHash("1111111111111111111111111111111111111111"))
@@ -220,11 +270,13 @@ func (s *UlReqEncodeSuite) TestDepthReference(c *C) {
 
 func (s *UlReqEncodeSuite) TestAll(c *C) {
 	ur := NewUploadRequest()
-	ur.Wants = append(ur.Wants, plumbing.NewHash("4444444444444444444444444444444444444444"))
-	ur.Wants = append(ur.Wants, plumbing.NewHash("1111111111111111111111111111111111111111"))
-	ur.Wants = append(ur.Wants, plumbing.NewHash("3333333333333333333333333333333333333333"))
-	ur.Wants = append(ur.Wants, plumbing.NewHash("2222222222222222222222222222222222222222"))
-	ur.Wants = append(ur.Wants, plumbing.NewHash("5555555555555555555555555555555555555555"))
+	ur.Wants = append(ur.Wants,
+		plumbing.NewHash("4444444444444444444444444444444444444444"),
+		plumbing.NewHash("1111111111111111111111111111111111111111"),
+		plumbing.NewHash("3333333333333333333333333333333333333333"),
+		plumbing.NewHash("2222222222222222222222222222222222222222"),
+		plumbing.NewHash("5555555555555555555555555555555555555555"),
+	)
 
 	ur.Capabilities.Add(capability.MultiACK)
 	ur.Capabilities.Add(capability.OFSDelta)
diff --git a/plumbing/protocol/packp/uppackreq.go b/plumbing/protocol/packp/uppackreq.go
index 2b1cb84..887d27a 100644
--- a/plumbing/protocol/packp/uppackreq.go
+++ b/plumbing/protocol/packp/uppackreq.go
@@ -1,6 +1,7 @@
 package packp
 
 import (
+	"bytes"
 	"fmt"
 	"io"
 
@@ -68,10 +69,20 @@ type UploadHaves struct {
 // Encode encodes the UploadHaves into the Writer.
 func (u *UploadHaves) Encode(w io.Writer) error {
 	e := pktline.NewEncoder(w)
+
+	plumbing.HashesSort(u.Haves)
+
+	var last plumbing.Hash
 	for _, have := range u.Haves {
+		if bytes.Compare(last[:], have[:]) == 0 {
+			continue
+		}
+
 		if err := e.Encodef("have %s\n", have); err != nil {
 			return fmt.Errorf("sending haves for %q: %s", have, err)
 		}
+
+		last = have
 	}
 
 	if len(u.Haves) != 0 {
diff --git a/plumbing/protocol/packp/uppackreq_test.go b/plumbing/protocol/packp/uppackreq_test.go
index 75b75b4..e551f45 100644
--- a/plumbing/protocol/packp/uppackreq_test.go
+++ b/plumbing/protocol/packp/uppackreq_test.go
@@ -4,6 +4,8 @@ import (
 	"gopkg.in/src-d/go-git.v4/plumbing"
 	"gopkg.in/src-d/go-git.v4/plumbing/protocol/packp/capability"
 
+	"bytes"
+
 	. "gopkg.in/check.v1"
 )
 
@@ -40,3 +42,28 @@ func (s *UploadPackRequestSuite) TestIsEmpty(c *C) {
 
 	c.Assert(r.IsEmpty(), Equals, true)
 }
+
+type UploadHavesSuite struct{}
+
+var _ = Suite(&UploadHavesSuite{})
+
+func (s *UploadHavesSuite) TestEncode(c *C) {
+	uh := &UploadHaves{}
+	uh.Haves = append(uh.Haves,
+		plumbing.NewHash("1111111111111111111111111111111111111111"),
+		plumbing.NewHash("3333333333333333333333333333333333333333"),
+		plumbing.NewHash("1111111111111111111111111111111111111111"),
+		plumbing.NewHash("2222222222222222222222222222222222222222"),
+		plumbing.NewHash("1111111111111111111111111111111111111111"),
+	)
+
+	buf := bytes.NewBuffer(nil)
+	err := uh.Encode(buf)
+	c.Assert(err, IsNil)
+	c.Assert(buf.String(), Equals, ""+
+		"0032have 1111111111111111111111111111111111111111\n"+
+		"0032have 2222222222222222222222222222222222222222\n"+
+		"0032have 3333333333333333333333333333333333333333\n"+
+		"0000",
+	)
+}