Merge branch 'pks-find-commits-skip-offset-error' into 'master'

commit: Handle real errors when skipping commits failed

See merge request gitlab-org/gitaly!3392

4 files changed, 41 insertions, 6 deletions

diff --git a/.golangci.yml b/.golangci.yml
index bbcc14a28..ef59d1c85 100644
--- a/.golangci.yml
+++ b/.golangci.yml
@@ -793,10 +793,6 @@ issues:
       text: "Error return value of `(cmd.Process.Kill)?` is not checked"
     - linters:
         - errcheck
-      path: "internal/gitaly/service/commit/find_commits.go"
-      text: "Error return value of `getCommits.Offset` is not checked"
-    - linters:
-        - errcheck
       path: "cmd/gitaly-git2go/main.go"
       text: "Error return value of `flags.Parse` is not checked"
     - linters:
diff --git a/changelogs/unreleased/pks-find-commits-skip-offset-error.yml b/changelogs/unreleased/pks-find-commits-skip-offset-error.yml
new file mode 100644
index 000000000..776fa6f98
--- /dev/null
+++ b/changelogs/unreleased/pks-find-commits-skip-offset-error.yml
@@ -0,0 +1,5 @@
+---
+title: 'commit: Handle real errors when skipping commits failed'
+merge_request: 3392
+author:
+type: fixed
diff --git a/internal/gitaly/service/commit/find_commits.go b/internal/gitaly/service/commit/find_commits.go
index db8228388..8662ab593 100644
--- a/internal/gitaly/service/commit/find_commits.go
+++ b/internal/gitaly/service/commit/find_commits.go
@@ -5,6 +5,7 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"io"
 	"strings"
 
 	"github.com/golang/protobuf/proto"
@@ -63,7 +64,16 @@ func (s *server) findCommits(ctx context.Context, req *gitalypb.FindCommitsReque
 	getCommits := NewGetCommits(logCmd, batch)
 
 	if calculateOffsetManually(req) {
-		getCommits.Offset(int(req.GetOffset()))
+		if err := getCommits.Offset(int(req.GetOffset())); err != nil {
+			// If we're at EOF, then it means that the offset has been greater than the
+			// number of available commits. We do not treat this as an error, but
+			// instead just return EOF ourselves.
+			if errors.Is(err, io.EOF) {
+				return nil
+			}
+
+			return fmt.Errorf("skipping to offset %d: %w", req.GetOffset(), err)
+		}
 	}
 
 	if err := streamCommits(getCommits, stream, req.GetTrailers()); err != nil {
@@ -104,7 +114,12 @@ func (g *GetCommits) Err() error {
 func (g *GetCommits) Offset(offset int) error {
 	for i := 0; i < offset; i++ {
 		if !g.Scan() {
-			return fmt.Errorf("offset %d is invalid: %v", offset, g.scanner.Err())
+			err := g.Err()
+			if err == nil {
+				err = io.EOF
+			}
+
+			return fmt.Errorf("skipping commit: %w", err)
 		}
 	}
 	return nil
diff --git a/internal/gitaly/service/commit/find_commits_test.go b/internal/gitaly/service/commit/find_commits_test.go
index 1795a88b8..1ab4f293a 100644
--- a/internal/gitaly/service/commit/find_commits_test.go
+++ b/internal/gitaly/service/commit/find_commits_test.go
@@ -584,6 +584,25 @@ func TestFindCommitsRequestWithFollowAndOffset(t *testing.T) {
 	}
 }
 
+func TestFindCommitsWithExceedingOffset(t *testing.T) {
+	_, repo, _, client := setupCommitServiceWithRepo(t, true)
+
+	ctx, cancel := testhelper.Context()
+	defer cancel()
+
+	stream, err := client.FindCommits(ctx, &gitalypb.FindCommitsRequest{
+		Repository: repo,
+		Follow:     true,
+		Paths:      [][]byte{[]byte("CHANGELOG")},
+		Offset:     9000,
+	})
+	require.NoError(t, err)
+
+	response, err := stream.Recv()
+	require.Nil(t, response)
+	require.EqualError(t, err, "EOF")
+}
+
 func getCommits(ctx context.Context, t *testing.T, request *gitalypb.FindCommitsRequest, client gitalypb.CommitServiceClient) []*gitalypb.GitCommit {
 	t.Helper()