plumbing: transport/common, Improve handling of remote errors

Max Jonas Werner · Max Jonas Werner · commit 129b709887b4 · 2023-10-12T11:26:36.000+02:00
Instead of simply returning the first line that the remote returned,
go-git now actively searches all of stderr for lines that may contain
a more actionable error message and returns that.

In addition, this change adds a case to map the GitLab-specific error
message to an ErrRepositoryNotFound error.

Signed-off-by: Max Jonas Werner &lt;mail@makk.es&gt;
diff --git a/plumbing/transport/internal/common/common.go b/plumbing/transport/internal/common/common.go
@@ -11,6 +11,7 @@ import (
 	"errors"
 	"fmt"
 	"io"
+	"regexp"
 	"strings"
 	"time"
 
@@ -28,6 +29,10 @@ const (
 
 var (
 	ErrTimeoutExceeded = errors.New("timeout exceeded")
+	// stdErrSkipPattern is used for skipping lines from a command's stderr output.
+	// Any line matching this pattern will be skipped from further
+	// processing and not be returned to calling code.
+	stdErrSkipPattern = regexp.MustCompile("^remote:( =*){0,1}$")
 )
 
 // Commander creates Command instances. This is the main entry point for
@@ -149,10 +154,17 @@ func (c *client) listenFirstError(r io.Reader) chan string {
 	errLine := make(chan string, 1)
 	go func() {
 		s := bufio.NewScanner(r)
-		if s.Scan() {
-			errLine <- s.Text()
-		} else {
-			close(errLine)
+		for {
+			if s.Scan() {
+				line := s.Text()
+				if !stdErrSkipPattern.MatchString(line) {
+					errLine <- line
+					break
+				}
+			} else {
+				close(errLine)
+				break
+			}
 		}
 
 		_, _ = io.Copy(io.Discard, r)
@@ -393,6 +405,7 @@ var (
 	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"
 )
 
 func isRepoNotFoundError(s string) bool {
@@ -424,6 +437,10 @@ func isRepoNotFoundError(s string) bool {
 		return true
 	}
 
+	if strings.HasPrefix(s, gitlabRepoNotFoundErr) {
+		return true
+	}
+
 	return false
 }
 
diff --git a/plumbing/transport/internal/common/common_test.go b/plumbing/transport/internal/common/common_test.go
@@ -4,6 +4,7 @@ import (
 	"fmt"
 	"testing"
 
+	"github.com/go-git/go-git/v5/plumbing/transport"
 	. "gopkg.in/check.v1"
 )
 
@@ -77,6 +78,14 @@ func (s *CommonSuite) TestIsRepoNotFoundErrorForGogsAccessDenied(c *C) {
 	c.Assert(isRepoNotFound, Equals, true)
 }
 
+func (s *CommonSuite) TestIsRepoNotFoundErrorForGitlab(c *C) {
+	msg := fmt.Sprintf("%s : some error stuf", gitlabRepoNotFoundErr)
+
+	isRepoNotFound := isRepoNotFoundError(msg)
+
+	c.Assert(isRepoNotFound, Equals, true)
+}
+
 func (s *CommonSuite) TestCheckNotFoundError(c *C) {
 	firstErrLine := make(chan string, 1)
 
@@ -90,3 +99,51 @@ func (s *CommonSuite) TestCheckNotFoundError(c *C) {
 
 	c.Assert(err, IsNil)
 }
+
+func TestAdvertisedReferencesWithRemoteError(t *testing.T) {
+	tests := []struct {
+		name    string
+		stderr  string
+		wantErr error
+	}{
+		{
+			name:    "unknown error",
+			stderr:  "something",
+			wantErr: fmt.Errorf("unknown error: something"),
+		},
+		{
+			name: "GitLab: repository not found",
+			stderr: `remote:
+remote: ========================================================================
+remote: 
+remote: ERROR: The project you were looking for could not be found or you don't have permission to view it.
+
+remote: 
+remote: ========================================================================
+remote:`,
+			wantErr: transport.ErrRepositoryNotFound,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			client := NewClient(MockCommander{stderr: tt.stderr})
+			sess, err := client.NewUploadPackSession(nil, nil)
+			if err != nil {
+				t.Fatalf("unexpected error: %s", err)
+			}
+
+			_, err = sess.AdvertisedReferences()
+
+			if tt.wantErr != nil {
+				if tt.wantErr != err {
+					if tt.wantErr.Error() != err.Error() {
+						t.Fatalf("expected a different error: got '%s', expected '%s'", err, tt.wantErr)
+					}
+				}
+			} else if err != nil {
+				t.Fatalf("unexpected error: %s", err)
+			}
+		})
+	}
+}
diff --git a/plumbing/transport/internal/common/mocks.go b/plumbing/transport/internal/common/mocks.go
@@ -0,0 +1,46 @@
+package common
+
+import (
+	"bytes"
+	"io"
+
+	gogitioutil "github.com/go-git/go-git/v5/utils/ioutil"
+
+	"github.com/go-git/go-git/v5/plumbing/transport"
+)
+
+type MockCommand struct {
+	stdin  bytes.Buffer
+	stdout bytes.Buffer
+	stderr bytes.Buffer
+}
+
+func (c MockCommand) StderrPipe() (io.Reader, error) {
+	return &c.stderr, nil
+}
+
+func (c MockCommand) StdinPipe() (io.WriteCloser, error) {
+	return gogitioutil.WriteNopCloser(&c.stdin), nil
+}
+
+func (c MockCommand) StdoutPipe() (io.Reader, error) {
+	return &c.stdout, nil
+}
+
+func (c MockCommand) Start() error {
+	return nil
+}
+
+func (c MockCommand) Close() error {
+	panic("not implemented")
+}
+
+type MockCommander struct {
+	stderr string
+}
+
+func (c MockCommander) Command(cmd string, ep *transport.Endpoint, auth transport.AuthMethod) (Command, error) {
+	return &MockCommand{
+		stderr: *bytes.NewBufferString(c.stderr),
+	}, nil
+}
