Merge pull request #469 from mcuadros/fix-multiple-ack

plumbing: protocol, fix handling multiple ACK on upload-pack

Author: mcuadros

plumbing/protocol/packp/srvresp.go

@@ -1,6 +1,7 @@
 package packp
 
 import (
+	"bufio"
 	"bytes"
 	"errors"
 	"fmt"
@@ -18,8 +19,8 @@ type ServerResponse struct {
 }
 
 // Decode decodes the response into the struct, isMultiACK should be true, if
-// the request was done with multi_ack or multi_ack_detailed capabilities
-func (r *ServerResponse) Decode(reader io.Reader, isMultiACK bool) error {
+// the request was done with multi_ack or multi_ack_detailed capabilities.
+func (r *ServerResponse) Decode(reader *bufio.Reader, isMultiACK bool) error {
 	// TODO: implement support for multi_ack or multi_ack_detailed responses
 	if isMultiACK {
 		return errors.New("multi_ack and multi_ack_detailed are not supported")
@@ -34,14 +35,56 @@ func (r *ServerResponse) Decode(reader io.Reader, isMultiACK bool) error {
 			return err
 		}
 
-		if !isMultiACK {
+		// we need to detect when the end of a response header and the begining
+		// of a packfile header happend, some requests to the git daemon
+		// produces a duplicate ACK header even when multi_ack is not supported.
+		isEnd, err := r.endReached(reader)
+		if err != nil {
+			return err
+		}
+
+		if isEnd {
 			break
 		}
 	}
 
 	return s.Err()
 }
 
+func (r *ServerResponse) endReached(reader *bufio.Reader) (bool, error) {
+	isPack, err := r.isPACKHeader(reader)
+	if err == io.EOF {
+		return true, nil
+	}
+
+	return isPack, err
+
+}
+
+// isPACKHeader detects when a header of a packfile is found, with this goal
+// the function is reading from the reader without moving the read pointer.
+func (r *ServerResponse) isPACKHeader(reader *bufio.Reader) (bool, error) {
+	ahead, err := reader.Peek(9)
+	if err != nil {
+		return false, err
+	}
+
+	if len(ahead) == 0 {
+		return true, nil
+	}
+
+	if len(ahead) > 4 && string(ahead[0:4]) == "PACK" {
+		return true, nil
+	}
+
+	if len(ahead) == 9 && string(ahead[5:]) == "PACK" {
+		return true, nil
+	}
+
+	return true, nil
+
+}
+
 func (r *ServerResponse) decodeLine(line []byte) error {
 	if len(line) == 0 {
 		return fmt.Errorf("unexpected flush")

plumbing/protocol/packp/srvresp_test.go

@@ -1,6 +1,7 @@
 package packp
 
 import (
+	"bufio"
 	"bytes"
 
 	"gopkg.in/src-d/go-git.v4/plumbing"
@@ -16,7 +17,7 @@ func (s *ServerResponseSuite) TestDecodeNAK(c *C) {
 	raw := "0008NAK\n"
 
 	sr := &ServerResponse{}
-	err := sr.Decode(bytes.NewBufferString(raw), false)
+	err := sr.Decode(bufio.NewReader(bytes.NewBufferString(raw)), false)
 	c.Assert(err, IsNil)
 
 	c.Assert(sr.ACKs, HasLen, 0)
@@ -26,7 +27,18 @@ func (s *ServerResponseSuite) TestDecodeACK(c *C) {
 	raw := "0031ACK 6ecf0ef2c2dffb796033e5a02219af86ec6584e5\n"
 
 	sr := &ServerResponse{}
-	err := sr.Decode(bytes.NewBufferString(raw), false)
+	err := sr.Decode(bufio.NewReader(bytes.NewBufferString(raw)), false)
+	c.Assert(err, IsNil)
+
+	c.Assert(sr.ACKs, HasLen, 1)
+	c.Assert(sr.ACKs[0], Equals, plumbing.NewHash("6ecf0ef2c2dffb796033e5a02219af86ec6584e5"))
+}
+
+func (s *ServerResponseSuite) TestDecodeMultipleACK(c *C) {
+	raw := "0031ACK 6ecf0ef2c2dffb796033e5a02219af86ec6584e5\n0031ACK 6ecf0ef2c2dffb796033e5a02219af86ec6584e5\n"
+
+	sr := &ServerResponse{}
+	err := sr.Decode(bufio.NewReader(bytes.NewBufferString(raw)), false)
 	c.Assert(err, IsNil)
 
 	c.Assert(sr.ACKs, HasLen, 1)
@@ -37,12 +49,12 @@ func (s *ServerResponseSuite) TestDecodeMalformed(c *C) {
 	raw := "0029ACK 6ecf0ef2c2dffb796033e5a02219af86ec6584e\n"
 
 	sr := &ServerResponse{}
-	err := sr.Decode(bytes.NewBufferString(raw), false)
+	err := sr.Decode(bufio.NewReader(bytes.NewBufferString(raw)), false)
 	c.Assert(err, NotNil)
 }
 
 func (s *ServerResponseSuite) TestDecodeMultiACK(c *C) {
 	sr := &ServerResponse{}
-	err := sr.Decode(bytes.NewBuffer(nil), true)
+	err := sr.Decode(bufio.NewReader(bytes.NewBuffer(nil)), true)
 	c.Assert(err, NotNil)
 }

plumbing/protocol/packp/uppackresp.go

@@ -4,6 +4,8 @@ import (
 	"errors"
 	"io"
 
+	"bufio"
+
 	"gopkg.in/src-d/go-git.v4/plumbing/protocol/packp/capability"
 	"gopkg.in/src-d/go-git.v4/utils/ioutil"
 )
@@ -51,18 +53,20 @@ func NewUploadPackResponseWithPackfile(req *UploadPackRequest,
 // Decode decodes all the responses sent by upload-pack service into the struct
 // and prepares it to read the packfile using the Read method
 func (r *UploadPackResponse) Decode(reader io.ReadCloser) error {
+	buf := bufio.NewReader(reader)
+
 	if r.isShallow {
-		if err := r.ShallowUpdate.Decode(reader); err != nil {
+		if err := r.ShallowUpdate.Decode(buf); err != nil {
 			return err
 		}
 	}
 
-	if err := r.ServerResponse.Decode(reader, r.isMultiACK); err != nil {
+	if err := r.ServerResponse.Decode(buf, r.isMultiACK); err != nil {
 		return err
 	}
 
 	// now the reader is ready to read the packfile content
-	r.r = reader
+	r.r = ioutil.NewReadCloser(buf, reader)
 
 	return nil
 }

plumbing/protocol/packp/uppackresp_test.go

@@ -15,7 +15,7 @@ type UploadPackResponseSuite struct{}
 var _ = Suite(&UploadPackResponseSuite{})
 
 func (s *UploadPackResponseSuite) TestDecodeNAK(c *C) {
-	raw := "0008NAK\n[PACK]"
+	raw := "0008NAK\nPACK"
 
 	req := NewUploadPackRequest()
 	res := NewUploadPackResponse(req)
@@ -26,11 +26,11 @@ func (s *UploadPackResponseSuite) TestDecodeNAK(c *C) {
 
 	pack, err := ioutil.ReadAll(res)
 	c.Assert(err, IsNil)
-	c.Assert(pack, DeepEquals, []byte("[PACK]"))
+	c.Assert(pack, DeepEquals, []byte("PACK"))
 }
 
 func (s *UploadPackResponseSuite) TestDecodeDepth(c *C) {
-	raw := "00000008NAK\n[PACK]"
+	raw := "00000008NAK\nPACK"
 
 	req := NewUploadPackRequest()
 	req.Depth = DepthCommits(1)
@@ -43,11 +43,11 @@ func (s *UploadPackResponseSuite) TestDecodeDepth(c *C) {
 
 	pack, err := ioutil.ReadAll(res)
 	c.Assert(err, IsNil)
-	c.Assert(pack, DeepEquals, []byte("[PACK]"))
+	c.Assert(pack, DeepEquals, []byte("PACK"))
 }
 
 func (s *UploadPackResponseSuite) TestDecodeMalformed(c *C) {
-	raw := "00000008ACK\n[PACK]"
+	raw := "00000008ACK\nPACK"
 
 	req := NewUploadPackRequest()
 	req.Depth = DepthCommits(1)
@@ -96,7 +96,7 @@ func (s *UploadPackResponseSuite) TestEncodeNAK(c *C) {
 }
 
 func (s *UploadPackResponseSuite) TestEncodeDepth(c *C) {
-	pf := ioutil.NopCloser(bytes.NewBuffer([]byte("[PACK]")))
+	pf := ioutil.NopCloser(bytes.NewBuffer([]byte("PACK")))
 	req := NewUploadPackRequest()
 	req.Depth = DepthCommits(1)
 
@@ -106,7 +106,7 @@ func (s *UploadPackResponseSuite) TestEncodeDepth(c *C) {
 	b := bytes.NewBuffer(nil)
 	c.Assert(res.Encode(b), IsNil)
 
-	expected := "00000008NAK\n[PACK]"
+	expected := "00000008NAK\nPACK"
 	c.Assert(string(b.Bytes()), Equals, expected)
 }