Merge pull request #936 from aymanbagabas/more-packpv5.10.1

Respect pktline error-line errors

9 files changed, 403 insertions, 112 deletions

diff --git a/plumbing/format/pktline/error.go b/plumbing/format/pktline/error.go
new file mode 100644
index 0000000..2c0e5a7
--- /dev/null
+++ b/plumbing/format/pktline/error.go
@@ -0,0 +1,51 @@
+package pktline
+
+import (
+	"bytes"
+	"errors"
+	"io"
+	"strings"
+)
+
+var (
+	// ErrInvalidErrorLine is returned by Decode when the packet line is not an
+	// error line.
+	ErrInvalidErrorLine = errors.New("expected an error-line")
+
+	errPrefix = []byte("ERR ")
+)
+
+// ErrorLine is a packet line that contains an error message.
+// Once this packet is sent by client or server, the data transfer process is
+// terminated.
+// See https://git-scm.com/docs/pack-protocol#_pkt_line_format
+type ErrorLine struct {
+	Text string
+}
+
+// Error implements the error interface.
+func (e *ErrorLine) Error() string {
+	return e.Text
+}
+
+// Encode encodes the ErrorLine into a packet line.
+func (e *ErrorLine) Encode(w io.Writer) error {
+	p := NewEncoder(w)
+	return p.Encodef("%s%s\n", string(errPrefix), e.Text)
+}
+
+// Decode decodes a packet line into an ErrorLine.
+func (e *ErrorLine) Decode(r io.Reader) error {
+	s := NewScanner(r)
+	if !s.Scan() {
+		return s.Err()
+	}
+
+	line := s.Bytes()
+	if !bytes.HasPrefix(line, errPrefix) {
+		return ErrInvalidErrorLine
+	}
+
+	e.Text = strings.TrimSpace(string(line[4:]))
+	return nil
+}
diff --git a/plumbing/format/pktline/error_test.go b/plumbing/format/pktline/error_test.go
new file mode 100644
index 0000000..3cffd20
--- /dev/null
+++ b/plumbing/format/pktline/error_test.go
@@ -0,0 +1,68 @@
+package pktline
+
+import (
+	"bytes"
+	"errors"
+	"io"
+	"testing"
+)
+
+func TestEncodeEmptyErrorLine(t *testing.T) {
+	e := &ErrorLine{}
+	err := e.Encode(io.Discard)
+	if err != nil {
+		t.Fatal(err)
+	}
+}
+
+func TestEncodeErrorLine(t *testing.T) {
+	e := &ErrorLine{
+		Text: "something",
+	}
+	var buf bytes.Buffer
+	err := e.Encode(&buf)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if buf.String() != "0012ERR something\n" {
+		t.Fatalf("unexpected encoded error line: %q", buf.String())
+	}
+}
+
+func TestDecodeEmptyErrorLine(t *testing.T) {
+	var buf bytes.Buffer
+	e := &ErrorLine{}
+	err := e.Decode(&buf)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if e.Text != "" {
+		t.Fatalf("unexpected error line: %q", e.Text)
+	}
+}
+
+func TestDecodeErrorLine(t *testing.T) {
+	var buf bytes.Buffer
+	buf.WriteString("000eERR foobar")
+	var e *ErrorLine
+	err := e.Decode(&buf)
+	if !errors.As(err, &e) {
+		t.Fatalf("expected error line, got: %T: %v", err, err)
+	}
+	if e.Text != "foobar" {
+		t.Fatalf("unexpected error line: %q", e.Text)
+	}
+}
+
+func TestDecodeErrorLineLn(t *testing.T) {
+	var buf bytes.Buffer
+	buf.WriteString("000fERR foobar\n")
+	var e *ErrorLine
+	err := e.Decode(&buf)
+	if !errors.As(err, &e) {
+		t.Fatalf("expected error line, got: %T: %v", err, err)
+	}
+	if e.Text != "foobar" {
+		t.Fatalf("unexpected error line: %q", e.Text)
+	}
+}
diff --git a/plumbing/format/pktline/scanner.go b/plumbing/format/pktline/scanner.go
index 5e85ed0..fbb137d 100644
--- a/plumbing/format/pktline/scanner.go
+++ b/plumbing/format/pktline/scanner.go
@@ -1,8 +1,10 @@
 package pktline
 
 import (
+	"bytes"
 	"errors"
 	"io"
+	"strings"
 
 	"github.com/go-git/go-git/v5/utils/trace"
 )
@@ -69,6 +71,13 @@ func (s *Scanner) Scan() bool {
 	s.payload = s.payload[:l]
 	trace.Packet.Printf("packet: < %04x %s", l, s.payload)
 
+	if bytes.HasPrefix(s.payload, errPrefix) {
+		s.err = &ErrorLine{
+			Text: strings.TrimSpace(string(s.payload[4:])),
+		}
+		return false
+	}
+
 	return true
 }
 
diff --git a/plumbing/protocol/packp/common.go b/plumbing/protocol/packp/common.go
index fef50a4..a858323 100644
--- a/plumbing/protocol/packp/common.go
+++ b/plumbing/protocol/packp/common.go
@@ -48,6 +48,11 @@ func isFlush(payload []byte) bool {
 	return len(payload) == 0
 }
 
+var (
+	// ErrNilWriter is returned when a nil writer is passed to the encoder.
+	ErrNilWriter = fmt.Errorf("nil writer")
+)
+
 // ErrUnexpectedData represents an unexpected data decoding a message
 type ErrUnexpectedData struct {
 	Msg  string
diff --git a/plumbing/protocol/packp/gitproto.go b/plumbing/protocol/packp/gitproto.go
new file mode 100644
index 0000000..0b7ff8f
--- /dev/null
+++ b/plumbing/protocol/packp/gitproto.go
@@ -0,0 +1,120 @@
+package packp
+
+import (
+	"fmt"
+	"io"
+	"strings"
+
+	"github.com/go-git/go-git/v5/plumbing/format/pktline"
+)
+
+var (
+	// ErrInvalidGitProtoRequest is returned by Decode if the input is not a
+	// valid git protocol request.
+	ErrInvalidGitProtoRequest = fmt.Errorf("invalid git protocol request")
+)
+
+// GitProtoRequest is a command request for the git protocol.
+// It is used to send the command, endpoint, and extra parameters to the
+// remote.
+// See https://git-scm.com/docs/pack-protocol#_git_transport
+type GitProtoRequest struct {
+	RequestCommand string
+	Pathname       string
+
+	// Optional
+	Host string
+
+	// Optional
+	ExtraParams []string
+}
+
+// validate validates the request.
+func (g *GitProtoRequest) validate() error {
+	if g.RequestCommand == "" {
+		return fmt.Errorf("%w: empty request command", ErrInvalidGitProtoRequest)
+	}
+
+	if g.Pathname == "" {
+		return fmt.Errorf("%w: empty pathname", ErrInvalidGitProtoRequest)
+	}
+
+	return nil
+}
+
+// Encode encodes the request into the writer.
+func (g *GitProtoRequest) Encode(w io.Writer) error {
+	if w == nil {
+		return ErrNilWriter
+	}
+
+	if err := g.validate(); err != nil {
+		return err
+	}
+
+	p := pktline.NewEncoder(w)
+	req := fmt.Sprintf("%s %s\x00", g.RequestCommand, g.Pathname)
+	if host := g.Host; host != "" {
+		req += fmt.Sprintf("host=%s\x00", host)
+	}
+
+	if len(g.ExtraParams) > 0 {
+		req += "\x00"
+		for _, param := range g.ExtraParams {
+			req += param + "\x00"
+		}
+	}
+
+	if err := p.Encode([]byte(req)); err != nil {
+		return err
+	}
+
+	return nil
+}
+
+// Decode decodes the request from the reader.
+func (g *GitProtoRequest) Decode(r io.Reader) error {
+	s := pktline.NewScanner(r)
+	if !s.Scan() {
+		err := s.Err()
+		if err == nil {
+			return ErrInvalidGitProtoRequest
+		}
+		return err
+	}
+
+	line := string(s.Bytes())
+	if len(line) == 0 {
+		return io.EOF
+	}
+
+	if line[len(line)-1] != 0 {
+		return fmt.Errorf("%w: missing null terminator", ErrInvalidGitProtoRequest)
+	}
+
+	parts := strings.SplitN(line, " ", 2)
+	if len(parts) != 2 {
+		return fmt.Errorf("%w: short request", ErrInvalidGitProtoRequest)
+	}
+
+	g.RequestCommand = parts[0]
+	params := strings.Split(parts[1], string(null))
+	if len(params) < 1 {
+		return fmt.Errorf("%w: missing pathname", ErrInvalidGitProtoRequest)
+	}
+
+	g.Pathname = params[0]
+	if len(params) > 1 {
+		g.Host = strings.TrimPrefix(params[1], "host=")
+	}
+
+	if len(params) > 2 {
+		for _, param := range params[2:] {
+			if param != "" {
+				g.ExtraParams = append(g.ExtraParams, param)
+			}
+		}
+	}
+
+	return nil
+}
diff --git a/plumbing/protocol/packp/gitproto_test.go b/plumbing/protocol/packp/gitproto_test.go
new file mode 100644
index 0000000..9cf1049
--- /dev/null
+++ b/plumbing/protocol/packp/gitproto_test.go
@@ -0,0 +1,99 @@
+package packp
+
+import (
+	"bytes"
+	"testing"
+)
+
+func TestEncodeEmptyGitProtoRequest(t *testing.T) {
+	var buf bytes.Buffer
+	var p GitProtoRequest
+	err := p.Encode(&buf)
+	if err == nil {
+		t.Fatal("expected error")
+	}
+}
+
+func TestEncodeGitProtoRequest(t *testing.T) {
+	var buf bytes.Buffer
+	p := GitProtoRequest{
+		RequestCommand: "command",
+		Pathname:       "pathname",
+		Host:           "host",
+		ExtraParams:    []string{"param1", "param2"},
+	}
+	err := p.Encode(&buf)
+	if err != nil {
+		t.Fatal(err)
+	}
+	expected := "002ecommand pathname\x00host=host\x00\x00param1\x00param2\x00"
+	if buf.String() != expected {
+		t.Fatalf("expected %q, got %q", expected, buf.String())
+	}
+}
+
+func TestEncodeInvalidGitProtoRequest(t *testing.T) {
+	var buf bytes.Buffer
+	p := GitProtoRequest{
+		RequestCommand: "command",
+	}
+	err := p.Encode(&buf)
+	if err == nil {
+		t.Fatal("expected error")
+	}
+}
+
+func TestDecodeEmptyGitProtoRequest(t *testing.T) {
+	var buf bytes.Buffer
+	var p GitProtoRequest
+	err := p.Decode(&buf)
+	if err == nil {
+		t.Fatal("expected error")
+	}
+}
+
+func TestDecodeGitProtoRequest(t *testing.T) {
+	var buf bytes.Buffer
+	buf.WriteString("002ecommand pathname\x00host=host\x00\x00param1\x00param2\x00")
+	var p GitProtoRequest
+	err := p.Decode(&buf)
+	if err != nil {
+		t.Fatal(err)
+	}
+	expected := GitProtoRequest{
+		RequestCommand: "command",
+		Pathname:       "pathname",
+		Host:           "host",
+		ExtraParams:    []string{"param1", "param2"},
+	}
+	if p.RequestCommand != expected.RequestCommand {
+		t.Fatalf("expected %q, got %q", expected.RequestCommand, p.RequestCommand)
+	}
+	if p.Pathname != expected.Pathname {
+		t.Fatalf("expected %q, got %q", expected.Pathname, p.Pathname)
+	}
+	if p.Host != expected.Host {
+		t.Fatalf("expected %q, got %q", expected.Host, p.Host)
+	}
+	if len(p.ExtraParams) != len(expected.ExtraParams) {
+		t.Fatalf("expected %d, got %d", len(expected.ExtraParams), len(p.ExtraParams))
+	}
+}
+
+func TestDecodeInvalidGitProtoRequest(t *testing.T) {
+	var buf bytes.Buffer
+	buf.WriteString("0026command \x00host=host\x00\x00param1\x00param2")
+	var p GitProtoRequest
+	err := p.Decode(&buf)
+	if err == nil {
+		t.Fatal("expected error")
+	}
+}
+
+func TestValidateEmptyGitProtoRequest(t *testing.T) {
+	var p GitProtoRequest
+	err := p.validate()
+	if err == nil {
+		t.Fatal("expected error")
+	}
+}
diff --git a/plumbing/transport/git/common.go b/plumbing/transport/git/common.go
index 92fc0be..2b878b0 100644
--- a/plumbing/transport/git/common.go
+++ b/plumbing/transport/git/common.go
@@ -2,12 +2,11 @@
 package git
 
 import (
-	"fmt"
 	"io"
 	"net"
 	"strconv"
 
-	"github.com/go-git/go-git/v5/plumbing/format/pktline"
+	"github.com/go-git/go-git/v5/plumbing/protocol/packp"
 	"github.com/go-git/go-git/v5/plumbing/transport"
 	"github.com/go-git/go-git/v5/plumbing/transport/internal/common"
 	"github.com/go-git/go-git/v5/utils/ioutil"
@@ -42,10 +41,18 @@ type command struct {
 
 // Start executes the command sending the required message to the TCP connection
 func (c *command) Start() error {
-	cmd := endpointToCommand(c.command, c.endpoint)
+	req := packp.GitProtoRequest{
+		RequestCommand: c.command,
+		Pathname:       c.endpoint.Path,
+	}
+	host := c.endpoint.Host
+	if c.endpoint.Port != DefaultPort {
+		host = net.JoinHostPort(c.endpoint.Host, strconv.Itoa(c.endpoint.Port))
+	}
+
+	req.Host = host
 
-	e := pktline.NewEncoder(c.conn)
-	return e.Encode([]byte(cmd))
+	return req.Encode(c.conn)
 }
 
 func (c *command) connect() error {
@@ -90,15 +97,6 @@ func (c *command) StdoutPipe() (io.Reader, error) {
 	return c.conn, nil
 }
 
-func endpointToCommand(cmd string, ep *transport.Endpoint) string {
-	host := ep.Host
-	if ep.Port != DefaultPort {
-		host = net.JoinHostPort(ep.Host, strconv.Itoa(ep.Port))
-	}
-
-	return fmt.Sprintf("%s %s%chost=%s%c", cmd, ep.Path, 0, host, 0)
-}
-
 // Close closes the TCP connection and connection.
 func (c *command) Close() error {
 	if !c.connected {
diff --git a/plumbing/transport/internal/common/common.go b/plumbing/transport/internal/common/common.go
index da1c2ac..9e1d023 100644
--- a/plumbing/transport/internal/common/common.go
+++ b/plumbing/transport/internal/common/common.go
@@ -203,9 +203,22 @@ func (s *session) AdvertisedReferencesContext(ctx context.Context) (*packp.AdvRe
 }
 
 func (s *session) handleAdvRefDecodeError(err error) error {
+	var errLine *pktline.ErrorLine
+	if errors.As(err, &errLine) {
+		if isRepoNotFoundError(errLine.Text) {
+			return transport.ErrRepositoryNotFound
+		}
+
+		return errLine
+	}
+
 	// If repository is not found, we get empty stdout and server writes an
 	// error to stderr.
-	if err == packp.ErrEmptyInput {
+	if errors.Is(err, packp.ErrEmptyInput) {
+		// TODO:(v6): handle this error in a better way.
+		// Instead of checking the stderr output for a specific error message,
+		// define an ExitError and embed the stderr output and exit (if one
+		// exists) in the error struct. Just like exec.ExitError.
 		s.finished = true
 		if err := s.checkNotFoundError(); err != nil {
 			return err
@@ -399,59 +412,43 @@ func (s *session) checkNotFoundError() error {
 			return transport.ErrRepositoryNotFound
 		}
 
+		// TODO:(v6): return server error just as it is without a prefix
 		return fmt.Errorf("unknown error: %s", line)
 	}
 }
 
-var (
-	githubRepoNotFoundErr      = "ERROR: Repository not found."
-	bitbucketRepoNotFoundErr   = "conq: repository does not exist."
+const (
+	githubRepoNotFoundErr      = "Repository not found."
+	bitbucketRepoNotFoundErr   = "repository does not exist."
 	localRepoNotFoundErr       = "does not appear to be a git repository"
-	gitProtocolNotFoundErr     = "ERR \n  Repository not found."
-	gitProtocolNoSuchErr       = "ERR no such repository"
-	gitProtocolAccessDeniedErr = "ERR access denied"
-	gogsAccessDeniedErr        = "Gogs: Repository does not exist or you do not have access"
-	gitlabRepoNotFoundErr      = "remote: ERROR: The project you were looking for could not be found"
+	gitProtocolNotFoundErr     = "Repository not found."
+	gitProtocolNoSuchErr       = "no such repository"
+	gitProtocolAccessDeniedErr = "access denied"
+	gogsAccessDeniedErr        = "Repository does not exist or you do not have access"
+	gitlabRepoNotFoundErr      = "The project you were looking for could not be found"
 )
 
 func isRepoNotFoundError(s string) bool {
-	if strings.HasPrefix(s, githubRepoNotFoundErr) {
-		return true
-	}
-
-	if strings.HasPrefix(s, bitbucketRepoNotFoundErr) {
-		return true
-	}
-
-	if strings.HasSuffix(s, localRepoNotFoundErr) {
-		return true
-	}
-
-	if strings.HasPrefix(s, gitProtocolNotFoundErr) {
-		return true
-	}
-
-	if strings.HasPrefix(s, gitProtocolNoSuchErr) {
-		return true
-	}
-
-	if strings.HasPrefix(s, gitProtocolAccessDeniedErr) {
-		return true
-	}
-
-	if strings.HasPrefix(s, gogsAccessDeniedErr) {
-		return true
-	}
-
-	if strings.HasPrefix(s, gitlabRepoNotFoundErr) {
-		return true
+	for _, err := range []string{
+		githubRepoNotFoundErr,
+		bitbucketRepoNotFoundErr,
+		localRepoNotFoundErr,
+		gitProtocolNotFoundErr,
+		gitProtocolNoSuchErr,
+		gitProtocolAccessDeniedErr,
+		gogsAccessDeniedErr,
+		gitlabRepoNotFoundErr,
+	} {
+		if strings.Contains(s, err) {
+			return true
+		}
 	}
 
 	return false
 }
 
 // uploadPack implements the git-upload-pack protocol.
-func uploadPack(w io.WriteCloser, r io.Reader, req *packp.UploadPackRequest) error {
+func uploadPack(w io.WriteCloser, _ io.Reader, req *packp.UploadPackRequest) error {
 	// TODO support multi_ack mode
 	// TODO support multi_ack_detailed mode
 	// TODO support acks for common objects
diff --git a/plumbing/transport/internal/common/common_test.go b/plumbing/transport/internal/common/common_test.go
index f6f2f67..9344bb6 100644
--- a/plumbing/transport/internal/common/common_test.go
+++ b/plumbing/transport/internal/common/common_test.go
@@ -22,64 +22,8 @@ func (s *CommonSuite) TestIsRepoNotFoundErrorForUnknownSource(c *C) {
 	c.Assert(isRepoNotFound, Equals, false)
 }
 
-func (s *CommonSuite) TestIsRepoNotFoundErrorForGithub(c *C) {
-	msg := fmt.Sprintf("%s : some error stuf", githubRepoNotFoundErr)
-
-	isRepoNotFound := isRepoNotFoundError(msg)
-
-	c.Assert(isRepoNotFound, Equals, true)
-}
-
-func (s *CommonSuite) TestIsRepoNotFoundErrorForBitBucket(c *C) {
-	msg := fmt.Sprintf("%s : some error stuf", bitbucketRepoNotFoundErr)
-
-	isRepoNotFound := isRepoNotFoundError(msg)
-
-	c.Assert(isRepoNotFound, Equals, true)
-}
-
-func (s *CommonSuite) TestIsRepoNotFoundErrorForLocal(c *C) {
-	msg := fmt.Sprintf("some error stuf : %s", localRepoNotFoundErr)
-
-	isRepoNotFound := isRepoNotFoundError(msg)
-
-	c.Assert(isRepoNotFound, Equals, true)
-}
-
-func (s *CommonSuite) TestIsRepoNotFoundErrorForGitProtocolNotFound(c *C) {
-	msg := fmt.Sprintf("%s : some error stuf", gitProtocolNotFoundErr)
-
-	isRepoNotFound := isRepoNotFoundError(msg)
-
-	c.Assert(isRepoNotFound, Equals, true)
-}
-
-func (s *CommonSuite) TestIsRepoNotFoundErrorForGitProtocolNoSuch(c *C) {
-	msg := fmt.Sprintf("%s : some error stuf", gitProtocolNoSuchErr)
-
-	isRepoNotFound := isRepoNotFoundError(msg)
-
-	c.Assert(isRepoNotFound, Equals, true)
-}
-
-func (s *CommonSuite) TestIsRepoNotFoundErrorForGitProtocolAccessDenied(c *C) {
-	msg := fmt.Sprintf("%s : some error stuf", gitProtocolAccessDeniedErr)
-
-	isRepoNotFound := isRepoNotFoundError(msg)
-
-	c.Assert(isRepoNotFound, Equals, true)
-}
-
-func (s *CommonSuite) TestIsRepoNotFoundErrorForGogsAccessDenied(c *C) {
-	msg := fmt.Sprintf("%s : some error stuf", gogsAccessDeniedErr)
-
-	isRepoNotFound := isRepoNotFoundError(msg)
-
-	c.Assert(isRepoNotFound, Equals, true)
-}
-
-func (s *CommonSuite) TestIsRepoNotFoundErrorForGitlab(c *C) {
-	msg := fmt.Sprintf("%s : some error stuf", gitlabRepoNotFoundErr)
+func (s *CommonSuite) TestIsRepoNotFoundError(c *C) {
+	msg := "no such repository : some error stuf"
 
 	isRepoNotFound := isRepoNotFoundError(msg)