Merge branch 'gitpipe_better_errors' into 'master'

gitpipe: Add stderr to Revlist errors

See merge request gitlab-org/gitaly!4174

4 files changed, 34 insertions, 13 deletions

diff --git a/internal/git/gitpipe/pipeline_test.go b/internal/git/gitpipe/pipeline_test.go
index a80f7e769..6237ba6f5 100644
--- a/internal/git/gitpipe/pipeline_test.go
+++ b/internal/git/gitpipe/pipeline_test.go
@@ -177,7 +177,10 @@ func TestPipeline_revlist(t *testing.T) {
 			revisions: []string{
 				"doesnotexist",
 			},
-			expectedErr: errors.New("rev-list pipeline command: exit status 128"),
+			expectedErr: errors.New("rev-list pipeline command: exit status 128, stderr: " +
+				"\"fatal: ambiguous argument 'doesnotexist': unknown revision or path not in the working tree.\\n" +
+				"Use '--' to separate paths from revisions, like this:\\n" +
+				"'git <command> [<revision>...] -- [<file>...]'\\n\""),
 		},
 		{
 			desc: "mixed valid and invalid revision",
@@ -186,7 +189,10 @@ func TestPipeline_revlist(t *testing.T) {
 				"doesnotexist",
 				lfsPointer2,
 			},
-			expectedErr: errors.New("rev-list pipeline command: exit status 128"),
+			expectedErr: errors.New("rev-list pipeline command: exit status 128, stderr: " +
+				"\"fatal: ambiguous argument 'doesnotexist': unknown revision or path not in the working tree.\\n" +
+				"Use '--' to separate paths from revisions, like this:\\n" +
+				"'git <command> [<revision>...] -- [<file>...]'\\n\""),
 		},
 		{
 			desc: "invalid revision with all filters",
@@ -205,7 +211,10 @@ func TestPipeline_revlist(t *testing.T) {
 					return true
 				}),
 			},
-			expectedErr: errors.New("rev-list pipeline command: exit status 128"),
+			expectedErr: errors.New("rev-list pipeline command: exit status 128, stderr: " +
+				"\"fatal: ambiguous argument 'doesnotexist': unknown revision or path not in the working tree.\\n" +
+				"Use '--' to separate paths from revisions, like this:\\n" +
+				"'git <command> [<revision>...] -- [<file>...]'\\n\""),
 		},
 	} {
 		t.Run(tc.desc, func(t *testing.T) {
diff --git a/internal/git/gitpipe/revision.go b/internal/git/gitpipe/revision.go
index 0f97a9d8a..68fcecbba 100644
--- a/internal/git/gitpipe/revision.go
+++ b/internal/git/gitpipe/revision.go
@@ -257,13 +257,19 @@ func Revlist(
 			})
 		}
 
-		revlist, err := repo.Exec(ctx, git.SubCmd{
-			Name:  "rev-list",
-			Flags: flags,
-			Args:  revisions,
-		})
+		var stderr strings.Builder
+		revlist, err := repo.Exec(ctx,
+			git.SubCmd{
+				Name:  "rev-list",
+				Flags: flags,
+				Args:  revisions,
+			},
+			git.WithStderr(&stderr),
+		)
 		if err != nil {
-			sendRevisionResult(ctx, resultChan, RevisionResult{err: err})
+			sendRevisionResult(ctx, resultChan, RevisionResult{
+				err: fmt.Errorf("rev-list: %w, stderr: %q", err, stderr.String()),
+			})
 			return
 		}
 
@@ -301,7 +307,7 @@ func Revlist(
 
 		if err := revlist.Wait(); err != nil {
 			sendRevisionResult(ctx, resultChan, RevisionResult{
-				err: fmt.Errorf("rev-list pipeline command: %w", err),
+				err: fmt.Errorf("rev-list pipeline command: %w, stderr: %q", err, stderr.String()),
 			})
 			return
 		}
diff --git a/internal/git/gitpipe/revision_test.go b/internal/git/gitpipe/revision_test.go
index 8b0d7bd25..ba06663bc 100644
--- a/internal/git/gitpipe/revision_test.go
+++ b/internal/git/gitpipe/revision_test.go
@@ -419,7 +419,10 @@ func TestRevlist(t *testing.T) {
 			revisions: []string{
 				"refs/heads/does-not-exist",
 			},
-			expectedErr: errors.New("rev-list pipeline command: exit status 128"),
+			expectedErr: errors.New("rev-list pipeline command: exit status 128, stderr: " +
+				"\"fatal: ambiguous argument 'refs/heads/does-not-exist': unknown revision or path not in the working tree.\\n" +
+				"Use '--' to separate paths from revisions, like this:\\n" +
+				"'git <command> [<revision>...] -- [<file>...]'\\n\""),
 		},
 		{
 			desc: "mixed valid and invalid revision",
@@ -427,7 +430,10 @@ func TestRevlist(t *testing.T) {
 				lfsPointer1,
 				"refs/heads/does-not-exist",
 			},
-			expectedErr: errors.New("rev-list pipeline command: exit status 128"),
+			expectedErr: errors.New("rev-list pipeline command: exit status 128, stderr: " +
+				"\"fatal: ambiguous argument 'refs/heads/does-not-exist': unknown revision or path not in the working tree.\\n" +
+				"Use '--' to separate paths from revisions, like this:\\n" +
+				"'git <command> [<revision>...] -- [<file>...]'\\n\""),
 		},
 		{
 			desc: "skip everything",
diff --git a/internal/gitaly/service/ref/tag_signatures_test.go b/internal/gitaly/service/ref/tag_signatures_test.go
index bcf6d6b97..460439ae2 100644
--- a/internal/gitaly/service/ref/tag_signatures_test.go
+++ b/internal/gitaly/service/ref/tag_signatures_test.go
@@ -58,7 +58,7 @@ func TestGetTagSignatures(t *testing.T) {
 			revisions: []string{
 				"b10ff336f3fbfb131431c4959915cdfd1b49c635",
 			},
-			expectedErr: status.Error(codes.Internal, "rev-list pipeline command: exit status 128"),
+			expectedErr: status.Error(codes.Internal, "rev-list pipeline command: exit status 128, stderr: \"fatal: bad object b10ff336f3fbfb131431c4959915cdfd1b49c635\\n\""),
 		},
 		{
 			desc: "commit id",