From a3dc077738fc55629f22e4effd389385a49f68cc Mon Sep 17 00:00:00 2001 From: Máximo Cuadros Date: Sat, 8 Jul 2017 01:59:48 +0200 Subject: plumbing: protocol, fix handling multiple ACK on upload-pack --- plumbing/protocol/packp/srvresp.go | 49 ++++++++++++++++++++++++++++-- plumbing/protocol/packp/srvresp_test.go | 20 +++++++++--- plumbing/protocol/packp/uppackresp.go | 10 ++++-- plumbing/protocol/packp/uppackresp_test.go | 14 ++++----- 4 files changed, 76 insertions(+), 17 deletions(-) diff --git a/plumbing/protocol/packp/srvresp.go b/plumbing/protocol/packp/srvresp.go index 0c89e47..a5bec9c 100644 --- a/plumbing/protocol/packp/srvresp.go +++ b/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,7 +35,15 @@ 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 } } @@ -42,6 +51,40 @@ func (r *ServerResponse) Decode(reader io.Reader, isMultiACK bool) error { 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") diff --git a/plumbing/protocol/packp/srvresp_test.go b/plumbing/protocol/packp/srvresp_test.go index 6078855..a873b45 100644 --- a/plumbing/protocol/packp/srvresp_test.go +++ b/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) } diff --git a/plumbing/protocol/packp/uppackresp.go b/plumbing/protocol/packp/uppackresp.go index ac456f3..c18e159 100644 --- a/plumbing/protocol/packp/uppackresp.go +++ b/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 } diff --git a/plumbing/protocol/packp/uppackresp_test.go b/plumbing/protocol/packp/uppackresp_test.go index c27fdda..789444d 100644 --- a/plumbing/protocol/packp/uppackresp_test.go +++ b/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) } -- cgit