protocol/packp: UploadPackResponse implementation (#161)

* plumbing/protocol: paktp avoid duplication of haves, wants and shallow

* protocol/pakp: UploadPackResponse implementation

* changes

* changes

* changes

* debug

* changes

3 files changed, 25 insertions, 49 deletions

diff --git a/plumbing/transport/common.go b/plumbing/transport/common.go
index bfc999f..2379422 100644
--- a/plumbing/transport/common.go
+++ b/plumbing/transport/common.go
@@ -69,10 +69,10 @@ type FetchPackSession interface {
 	AdvertisedReferences() (*packp.AdvRefs, error)
 	// FetchPack takes a request and returns a reader for the packfile
 	// received from the server.
-	FetchPack(req *packp.UploadPackRequest) (io.ReadCloser, error)
+	FetchPack(*packp.UploadPackRequest) (*packp.UploadPackResponse, error)
 }
 
-// FetchPackSession represents a git-send-pack session.
+// SendPackSession represents a git-send-pack session.
 // A git-send-pack session has two steps: reference discovery
 // (`AdvertisedReferences` function) and sending pack (`SendPack` function).
 // In that order.
diff --git a/plumbing/transport/http/fetch_pack.go b/plumbing/transport/http/fetch_pack.go
index f250667..20cdb55 100644
--- a/plumbing/transport/http/fetch_pack.go
+++ b/plumbing/transport/http/fetch_pack.go
@@ -11,6 +11,7 @@ import (
 	"gopkg.in/src-d/go-git.v4/plumbing/format/pktline"
 	"gopkg.in/src-d/go-git.v4/plumbing/protocol/packp"
 	"gopkg.in/src-d/go-git.v4/plumbing/transport"
+	"gopkg.in/src-d/go-git.v4/plumbing/transport/internal/common"
 	"gopkg.in/src-d/go-git.v4/utils/ioutil"
 )
 
@@ -74,12 +75,12 @@ func (s *fetchPackSession) AdvertisedReferences() (*packp.AdvRefs, error) {
 	return ar, nil
 }
 
-func (s *fetchPackSession) FetchPack(r *packp.UploadPackRequest) (io.ReadCloser, error) {
-	if r.IsEmpty() {
+func (s *fetchPackSession) FetchPack(req *packp.UploadPackRequest) (*packp.UploadPackResponse, error) {
+	if req.IsEmpty() {
 		return nil, transport.ErrEmptyUploadPackRequest
 	}
 
-	if err := r.Validate(); err != nil {
+	if err := req.Validate(); err != nil {
 		return nil, err
 	}
 
@@ -88,31 +89,27 @@ func (s *fetchPackSession) FetchPack(r *packp.UploadPackRequest) (io.ReadCloser,
 		s.endpoint.String(), transport.UploadPackServiceName,
 	)
 
-	content, err := uploadPackRequestToReader(r)
+	content, err := uploadPackRequestToReader(req)
 	if err != nil {
 		return nil, err
 	}
 
-	res, err := s.doRequest("POST", url, content)
+	res, err := s.doRequest(http.MethodPost, url, content)
 	if err != nil {
 		return nil, err
 	}
 
-	reader, err := ioutil.NonEmptyReader(res.Body)
-	if err == ioutil.ErrEmptyReader || err == io.ErrUnexpectedEOF {
-		return nil, transport.ErrEmptyUploadPackRequest
-	}
-
+	r, err := ioutil.NonEmptyReader(res.Body)
 	if err != nil {
-		return nil, err
-	}
+		if err == ioutil.ErrEmptyReader || err == io.ErrUnexpectedEOF {
+			return nil, transport.ErrEmptyUploadPackRequest
+		}
 
-	rc := ioutil.NewReadCloser(reader, res.Body)
-	if err := discardResponseInfo(rc); err != nil {
 		return nil, err
 	}
 
-	return rc, nil
+	rc := ioutil.NewReadCloser(r, res.Body)
+	return common.DecodeUploadPackResponse(rc, req)
 }
 
 // Close does nothing.
@@ -120,17 +117,6 @@ func (s *fetchPackSession) Close() error {
 	return nil
 }
 
-func discardResponseInfo(r io.Reader) error {
-	s := pktline.NewScanner(r)
-	for s.Scan() {
-		if bytes.Equal(s.Bytes(), []byte{'N', 'A', 'K', '\n'}) {
-			break
-		}
-	}
-
-	return s.Err()
-}
-
 func (s *fetchPackSession) doRequest(method, url string, content *strings.Reader) (*http.Response, error) {
 	var body io.Reader
 	if content != nil {
diff --git a/plumbing/transport/internal/common/common.go b/plumbing/transport/internal/common/common.go
index 8b2f9f3..f6aa204 100644
--- a/plumbing/transport/internal/common/common.go
+++ b/plumbing/transport/internal/common/common.go
@@ -7,7 +7,6 @@ package common
 
 import (
 	"bufio"
-	"bytes"
 	"errors"
 	"fmt"
 	"io"
@@ -193,7 +192,7 @@ func (s *session) AdvertisedReferences() (*packp.AdvRefs, error) {
 
 // FetchPack performs a request to the server to fetch a packfile. A reader is
 // returned with the packfile content. The reader must be closed after reading.
-func (s *session) FetchPack(req *packp.UploadPackRequest) (io.ReadCloser, error) {
+func (s *session) FetchPack(req *packp.UploadPackRequest) (*packp.UploadPackResponse, error) {
 	if req.IsEmpty() {
 		return nil, transport.ErrEmptyUploadPackRequest
 	}
@@ -230,7 +229,7 @@ func (s *session) FetchPack(req *packp.UploadPackRequest) (io.ReadCloser, error)
 	wc := &waitCloser{s.Command}
 	rc := ioutil.NewReadCloser(r, wc)
 
-	return rc, nil
+	return DecodeUploadPackResponse(rc, req)
 }
 
 func (s *session) finish() error {
@@ -314,9 +313,7 @@ var (
 // TODO support multi_ack_detailed mode
 // TODO support acks for common objects
 // TODO build a proper state machine for all these processing options
-func fetchPack(w io.WriteCloser, r io.Reader,
-	req *packp.UploadPackRequest) error {
-
+func fetchPack(w io.WriteCloser, r io.Reader, req *packp.UploadPackRequest) error {
 	if err := req.UploadRequest.Encode(w); err != nil {
 		return fmt.Errorf("sending upload-req message: %s", err)
 	}
@@ -333,10 +330,6 @@ func fetchPack(w io.WriteCloser, r io.Reader,
 		return fmt.Errorf("closing input: %s", err)
 	}
 
-	if err := readNAK(r); err != nil {
-		return fmt.Errorf("reading NAK: %s", err)
-	}
-
 	return nil
 }
 
@@ -346,19 +339,16 @@ func sendDone(w io.Writer) error {
 	return e.Encodef("done\n")
 }
 
-func readNAK(r io.Reader) error {
-	s := pktline.NewScanner(r)
-	if !s.Scan() {
-		return s.Err()
+// DecodeUploadPackResponse decodes r into a new packp.UploadPackResponse
+func DecodeUploadPackResponse(r io.ReadCloser, req *packp.UploadPackRequest) (
+	*packp.UploadPackResponse, error,
+) {
+	res := packp.NewUploadPackResponse(req)
+	if err := res.Decode(r); err != nil {
+		return nil, fmt.Errorf("error decoding upload-pack response: %s", err)
 	}
 
-	b := s.Bytes()
-	b = bytes.TrimSuffix(b, eol)
-	if !bytes.Equal(b, nak) {
-		return fmt.Errorf("expecting NAK, found %q instead", string(b))
-	}
-
-	return nil
+	return res, nil
 }
 
 type waitCloser struct {