Merge branch 'toon-gitattr-sentinel' into 'master'

gitattributes: Get rid of endOfAttributes

See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/6490

Merged-by: Sami Hiltunen <shiltunen@gitlab.com>
Approved-by: Sami Hiltunen <shiltunen@gitlab.com>
Reviewed-by: Sami Hiltunen <shiltunen@gitlab.com>
Reviewed-by: Justin Tobler <jtobler@gitlab.com>
Co-authored-by: Toon Claes <toon@gitlab.com>

2 files changed, 21 insertions, 11 deletions

diff --git a/internal/git/gitattributes/check_attr.go b/internal/git/gitattributes/check_attr.go
index 35b9c9538..8596efaa3 100644
--- a/internal/git/gitattributes/check_attr.go
+++ b/internal/git/gitattributes/check_attr.go
@@ -10,14 +10,9 @@ import (
 
 	"gitlab.com/gitlab-org/gitaly/v16/internal/command"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/git"
+	"gitlab.com/gitlab-org/gitaly/v16/internal/structerr"
 )
 
-// endOfAttributes is a magic attribute name we use to detect all attributes
-// have been processed. It is added to the end of the list of attributes the
-// caller requests, when this attribute is seen we know it's the last one and we
-// can break the loop.
-const endOfAttributes = "end-of-attributes-----"
-
 // CheckAttrCmd can be used to get the gitattributes(5) for a set of files in a
 // repo.
 type CheckAttrCmd struct {
@@ -25,11 +20,17 @@ type CheckAttrCmd struct {
 	stdout *bufio.Reader
 	stdin  *bufio.Writer
 
+	count int
+
 	m *sync.Mutex
 }
 
 // CheckAttr creates a CheckAttrCmd that checks the given list of attribute names.
 func CheckAttr(ctx context.Context, repo git.RepositoryExecutor, revision git.Revision, names []string) (*CheckAttrCmd, func(), error) {
+	if len(names) == 0 {
+		return nil, nil, structerr.NewInvalidArgument("empty list of attribute names")
+	}
+
 	cmd, err := repo.Exec(ctx, git.Command{
 		Name: "check-attr",
 		Flags: []git.Option{
@@ -37,7 +38,7 @@ func CheckAttr(ctx context.Context, repo git.RepositoryExecutor, revision git.Re
 			git.Flag{Name: "-z"},
 			git.ValueFlag{Name: "--source", Value: revision.String()},
 		},
-		Args: append(names, endOfAttributes),
+		Args: names,
 	},
 		git.WithSetupStdin(),
 		git.WithSetupStdout(),
@@ -50,6 +51,7 @@ func CheckAttr(ctx context.Context, repo git.RepositoryExecutor, revision git.Re
 		cmd:    cmd,
 		stdout: bufio.NewReader(cmd),
 		stdin:  bufio.NewWriter(cmd),
+		count:  len(names),
 		m:      &sync.Mutex{},
 	}
 
@@ -77,7 +79,7 @@ func (c CheckAttrCmd) Check(path string) (Attributes, error) {
 
 	// Using git-check-attr(1) with -z will return data in the format:
 	// <path> NUL <attribute> NUL <info> NUL ...
-	for {
+	for i := 0; i < c.count; {
 		word, err := c.stdout.ReadBytes('\000')
 		if err != nil {
 			return nil, fmt.Errorf("read line: %w", err)
@@ -92,13 +94,11 @@ func (c CheckAttrCmd) Check(path string) (Attributes, error) {
 		if buf[0] != path {
 			return nil, fmt.Errorf("wrong path name detected, expected %q, got %q", path, buf[0])
 		}
-		if buf[1] == endOfAttributes {
-			break
-		}
 		if buf[2] != Unspecified {
 			attrs = append(attrs, Attribute{Name: buf[1], State: buf[2]})
 		}
 
+		i++
 		buf = buf[:0]
 	}
 
diff --git a/internal/git/gitattributes/check_attr_test.go b/internal/git/gitattributes/check_attr_test.go
index dcd7e4e0a..1e65b17f9 100644
--- a/internal/git/gitattributes/check_attr_test.go
+++ b/internal/git/gitattributes/check_attr_test.go
@@ -7,10 +7,20 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v16/internal/git"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/git/gittest"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/git/localrepo"
+	"gitlab.com/gitlab-org/gitaly/v16/internal/structerr"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/testhelper/testcfg"
 )
 
+func TestCheckAttr(t *testing.T) {
+	t.Parallel()
+
+	ctx := testhelper.Context(t)
+	_, finish, err := CheckAttr(ctx, nil, "HEAD", nil)
+	require.Equal(t, structerr.NewInvalidArgument("empty list of attribute names"), err)
+	require.Nil(t, finish)
+}
+
 func TestCheckAttrCmd_Check(t *testing.T) {
 	ctx := testhelper.Context(t)
 	cfg := testcfg.Build(t)