Merge branch 'jc/unify-tree-object' into 'master'

lstree: Use localrepo.TreeEntry instead of lstree.Entry

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

Merged-by: John Cai <jcai@gitlab.com>
Approved-by: Will Chandler <wchandler@gitlab.com>
Reviewed-by: Will Chandler <wchandler@gitlab.com>
Reviewed-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>

13 files changed, 147 insertions, 137 deletions

diff --git a/cmd/gitaly-git2go/submodule_test.go b/cmd/gitaly-git2go/submodule_test.go
index 0edb9cfe0..69b278495 100644
--- a/cmd/gitaly-git2go/submodule_test.go
+++ b/cmd/gitaly-git2go/submodule_test.go
@@ -129,7 +129,7 @@ func TestSubmodule(t *testing.T) {
 			parsedEntry, err := parser.NextEntry()
 			require.NoError(t, err)
 			require.Equal(t, tc.command.Submodule, parsedEntry.Path)
-			require.Equal(t, tc.command.CommitSHA, parsedEntry.ObjectID.String())
+			require.Equal(t, tc.command.CommitSHA, parsedEntry.OID.String())
 		})
 	}
 }
diff --git a/internal/git/gitpipe/ls_tree.go b/internal/git/gitpipe/ls_tree.go
index 98d8a35b6..eb1535c95 100644
--- a/internal/git/gitpipe/ls_tree.go
+++ b/internal/git/gitpipe/ls_tree.go
@@ -15,7 +15,7 @@ import (
 // lsTreeConfig is configuration for the LsTree pipeline step.
 type lsTreeConfig struct {
 	recursive  bool
-	typeFilter func(*lstree.Entry) bool
+	typeFilter func(*localrepo.TreeEntry) bool
 	skipResult func(*RevisionResult) (bool, error)
 }
 
@@ -32,7 +32,7 @@ func LsTreeWithRecursive() LsTreeOption {
 // LsTreeWithBlobFilter configures LsTree to only pass through blob objects.
 func LsTreeWithBlobFilter() LsTreeOption {
 	return func(cfg *lsTreeConfig) {
-		cfg.typeFilter = func(e *lstree.Entry) bool { return e.Type == lstree.Blob }
+		cfg.typeFilter = func(e *localrepo.TreeEntry) bool { return e.IsBlob() }
 	}
 }
 
@@ -115,7 +115,7 @@ func LsTree(
 			}
 
 			result := RevisionResult{
-				OID:        entry.ObjectID,
+				OID:        entry.OID,
 				ObjectName: []byte(entry.Path),
 			}
 
diff --git a/internal/git/localrepo/tree.go b/internal/git/localrepo/tree.go
index 42772d77d..cfb1e722d 100644
--- a/internal/git/localrepo/tree.go
+++ b/internal/git/localrepo/tree.go
@@ -9,6 +9,62 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v15/internal/helper/text"
 )
 
+// ObjectType is an Enum for the type of object of
+// the ls-tree entry, which can be can be tree, blob or commit
+type ObjectType int
+
+// Entries holds every ls-tree Entry
+type Entries []TreeEntry
+
+// Enum values for ObjectType
+const (
+	Unknown ObjectType = iota
+	Tree
+	Blob
+	Submodule
+)
+
+func (e Entries) Len() int {
+	return len(e)
+}
+
+func (e Entries) Swap(i, j int) {
+	e[i], e[j] = e[j], e[i]
+}
+
+// Less sorts entries by type in the order [*tree *blobs *submodules]
+func (e Entries) Less(i, j int) bool {
+	return e[i].Type < e[j].Type
+}
+
+// ToEnum translates a string representation of the object type into an
+// ObjectType enum.
+func ToEnum(s string) ObjectType {
+	switch s {
+	case "tree":
+		return Tree
+	case "blob":
+		return Blob
+	case "commit":
+		return Submodule
+	default:
+		return Unknown
+	}
+}
+
+func fromEnum(t ObjectType) string {
+	switch t {
+	case Tree:
+		return "tree"
+	case Blob:
+		return "blob"
+	case Submodule:
+		return "commit"
+	default:
+		return "unknown"
+	}
+}
+
 // TreeEntry represents an entry of a git tree object.
 type TreeEntry struct {
 	// OID is the object ID the tree entry refers to.
@@ -17,8 +73,13 @@ type TreeEntry struct {
 	Mode string
 	// Path is the full path of the tree entry.
 	Path string
-	// Content is the content of the tree entry.
-	Content string
+	// Type is the type of the tree entry.
+	Type ObjectType
+}
+
+// IsBlob returns whether or not the TreeEntry is a blob.
+func (t *TreeEntry) IsBlob() bool {
+	return t.Type == Blob
 }
 
 // WriteTree writes a new tree object to the given path. This function does not verify whether OIDs
@@ -26,24 +87,26 @@ type TreeEntry struct {
 func (repo *Repo) WriteTree(ctx context.Context, entries []TreeEntry) (git.ObjectID, error) {
 	var tree bytes.Buffer
 	for _, entry := range entries {
-		var entryType string
-
-		switch entry.Mode {
-		case "100644":
-			fallthrough
-		case "100755":
-			fallthrough
-		case "120000":
-			entryType = "blob"
-		case "040000":
-			entryType = "tree"
-		case "160000":
-			entryType = "commit"
+		entryType := entry.Type
+
+		if entryType == Unknown {
+			switch entry.Mode {
+			case "100644":
+				fallthrough
+			case "100755":
+				fallthrough
+			case "120000":
+				entryType = Blob
+			case "040000":
+				entryType = Tree
+			case "160000":
+				entryType = Submodule
+			}
 		}
 
 		oid := entry.OID
 
-		formattedEntry := fmt.Sprintf("%s %s %s\t%s\000", entry.Mode, entryType, oid.String(), entry.Path)
+		formattedEntry := fmt.Sprintf("%s %s %s\t%s\000", entry.Mode, fromEnum(entryType), oid.String(), entry.Path)
 		if _, err := tree.WriteString(formattedEntry); err != nil {
 			return "", err
 		}
diff --git a/internal/git/lstree/entries.go b/internal/git/lstree/entries.go
deleted file mode 100644
index 89167d9b4..000000000
--- a/internal/git/lstree/entries.go
+++ /dev/null
@@ -1,38 +0,0 @@
-package lstree
-
-import "gitlab.com/gitlab-org/gitaly/v15/internal/git"
-
-// ObjectType is an Enum for the type of object of
-// the ls-tree entry, which can be can be tree, blob or commit
-type ObjectType int
-
-// Entry represents a single ls-tree entry
-type Entry struct {
-	Mode     []byte
-	Type     ObjectType
-	ObjectID git.ObjectID
-	Path     string
-}
-
-// Entries holds every ls-tree Entry
-type Entries []Entry
-
-// Enum values for ObjectType
-const (
-	Tree ObjectType = iota
-	Blob
-	Submodule
-)
-
-func (e Entries) Len() int {
-	return len(e)
-}
-
-func (e Entries) Swap(i, j int) {
-	e[i], e[j] = e[j], e[i]
-}
-
-// Less sorts entries by type in the order [*tree *blobs *submodules]
-func (e Entries) Less(i, j int) bool {
-	return e[i].Type < e[j].Type
-}
diff --git a/internal/git/lstree/list_entries.go b/internal/git/lstree/list_entries.go
index fa40720fe..d8cb5acc5 100644
--- a/internal/git/lstree/list_entries.go
+++ b/internal/git/lstree/list_entries.go
@@ -37,7 +37,7 @@ func ListEntries(
 	repo *localrepo.Repo,
 	treeish git.Revision,
 	cfg *ListEntriesConfig,
-) ([]*Entry, error) {
+) ([]*localrepo.TreeEntry, error) {
 	if cfg == nil {
 		cfg = &ListEntriesConfig{}
 	}
@@ -66,7 +66,7 @@ func ListEntries(
 	}
 
 	parser := NewParser(cmd, objectHash)
-	var entries []*Entry
+	var entries []*localrepo.TreeEntry
 	for {
 		entry, err := parser.NextEntry()
 		if err != nil {
diff --git a/internal/git/lstree/list_entries_test.go b/internal/git/lstree/list_entries_test.go
index 2c3ef8256..a1b9a58f9 100644
--- a/internal/git/lstree/list_entries_test.go
+++ b/internal/git/lstree/list_entries_test.go
@@ -46,7 +46,7 @@ func TestListEntries(t *testing.T) {
 		desc            string
 		treeish         git.Revision
 		cfg             *ListEntriesConfig
-		expectedResults []*Entry
+		expectedResults []*localrepo.TreeEntry
 		expectedErr     error
 	}{
 		{
@@ -56,23 +56,23 @@ func TestListEntries(t *testing.T) {
 		{
 			desc:    "tree with blob",
 			treeish: treeWithBlob.Revision(),
-			expectedResults: []*Entry{
-				{Mode: []byte("100755"), Type: Blob, ObjectID: blobID, Path: "executable"},
-				{Mode: []byte("100644"), Type: Blob, ObjectID: blobID, Path: "nonexecutable"},
+			expectedResults: []*localrepo.TreeEntry{
+				{Mode: "100755", Type: localrepo.Blob, OID: blobID, Path: "executable"},
+				{Mode: "100644", Type: localrepo.Blob, OID: blobID, Path: "nonexecutable"},
 			},
 		},
 		{
 			desc:    "tree with subtree",
 			treeish: treeWithSubtree.Revision(),
-			expectedResults: []*Entry{
-				{Mode: []byte("040000"), Type: Tree, ObjectID: emptyTreeID, Path: "subdir"},
+			expectedResults: []*localrepo.TreeEntry{
+				{Mode: "040000", Type: localrepo.Tree, OID: emptyTreeID, Path: "subdir"},
 			},
 		},
 		{
 			desc:    "nested trees",
 			treeish: treeWithNestedSubtrees.Revision(),
-			expectedResults: []*Entry{
-				{Mode: []byte("040000"), Type: Tree, ObjectID: treeWithSubtree, Path: "nested-subdir"},
+			expectedResults: []*localrepo.TreeEntry{
+				{Mode: "040000", Type: localrepo.Tree, OID: treeWithSubtree, Path: "nested-subdir"},
 			},
 		},
 		{
@@ -81,9 +81,9 @@ func TestListEntries(t *testing.T) {
 			cfg: &ListEntriesConfig{
 				Recursive: true,
 			},
-			expectedResults: []*Entry{
-				{Mode: []byte("040000"), Type: Tree, ObjectID: treeWithSubtree, Path: "nested-subdir"},
-				{Mode: []byte("040000"), Type: Tree, ObjectID: emptyTreeID, Path: "nested-subdir/subdir"},
+			expectedResults: []*localrepo.TreeEntry{
+				{Mode: "040000", Type: localrepo.Tree, OID: treeWithSubtree, Path: "nested-subdir"},
+				{Mode: "040000", Type: localrepo.Tree, OID: emptyTreeID, Path: "nested-subdir/subdir"},
 			},
 		},
 		{
@@ -92,8 +92,8 @@ func TestListEntries(t *testing.T) {
 			cfg: &ListEntriesConfig{
 				RelativePath: "nested-subdir",
 			},
-			expectedResults: []*Entry{
-				{Mode: []byte("040000"), Type: Tree, ObjectID: emptyTreeID, Path: "subdir"},
+			expectedResults: []*localrepo.TreeEntry{
+				{Mode: "040000", Type: localrepo.Tree, OID: emptyTreeID, Path: "subdir"},
 			},
 		},
 		{
@@ -103,10 +103,10 @@ func TestListEntries(t *testing.T) {
 				RelativePath: "subdir",
 				Recursive:    true,
 			},
-			expectedResults: []*Entry{
-				{Mode: []byte("100644"), Type: Blob, ObjectID: blobID, Path: "blob"},
-				{Mode: []byte("040000"), Type: Tree, ObjectID: treeWithSubtree, Path: "subdir"},
-				{Mode: []byte("040000"), Type: Tree, ObjectID: emptyTreeID, Path: "subdir/subdir"},
+			expectedResults: []*localrepo.TreeEntry{
+				{Mode: "100644", Type: localrepo.Blob, OID: blobID, Path: "blob"},
+				{Mode: "040000", Type: localrepo.Tree, OID: treeWithSubtree, Path: "subdir"},
+				{Mode: "040000", Type: localrepo.Tree, OID: emptyTreeID, Path: "subdir/subdir"},
 			},
 		},
 		{
@@ -115,10 +115,10 @@ func TestListEntries(t *testing.T) {
 			cfg: &ListEntriesConfig{
 				Recursive: true,
 			},
-			expectedResults: []*Entry{
-				{Mode: []byte("100644"), Type: Blob, ObjectID: blobID, Path: "blob"},
-				{Mode: []byte("040000"), Type: Tree, ObjectID: treeWithSubtree, Path: "subdir"},
-				{Mode: []byte("040000"), Type: Tree, ObjectID: emptyTreeID, Path: "subdir/subdir"},
+			expectedResults: []*localrepo.TreeEntry{
+				{Mode: "100644", Type: localrepo.Blob, OID: blobID, Path: "blob"},
+				{Mode: "040000", Type: localrepo.Tree, OID: treeWithSubtree, Path: "subdir"},
+				{Mode: "040000", Type: localrepo.Tree, OID: emptyTreeID, Path: "subdir/subdir"},
 			},
 		},
 		{
diff --git a/internal/git/lstree/parser.go b/internal/git/lstree/parser.go
index a8618288c..85d73da6a 100644
--- a/internal/git/lstree/parser.go
+++ b/internal/git/lstree/parser.go
@@ -7,6 +7,7 @@ import (
 	"io"
 
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git"
+	"gitlab.com/gitlab-org/gitaly/v15/internal/git/localrepo"
 )
 
 // ErrParse is returned when the parse of an entry was unsuccessful
@@ -27,9 +28,8 @@ func NewParser(src io.Reader, objectHash git.ObjectHash) *Parser {
 }
 
 // NextEntry reads a tree entry as it would be written by `git ls-tree -z`.
-func (p *Parser) NextEntry() (*Entry, error) {
+func (p *Parser) NextEntry() (*localrepo.TreeEntry, error) {
 	// Each tree entry is expected to have a format of `<mode> SP <type> SP <objectid> TAB <path> NUL`.
-
 	treeEntryMode, err := p.reader.ReadBytes(' ')
 	if err != nil {
 		if errors.Is(err, io.EOF) {
@@ -58,21 +58,16 @@ func (p *Parser) NextEntry() (*Entry, error) {
 	}
 	treeEntryPath = treeEntryPath[:len(treeEntryPath)-1]
 
-	objectType, err := toEnum(string(treeEntryType))
-	if err != nil {
-		return nil, err
-	}
-
 	objectID, err := p.objectHash.FromHex(string(treeEntryID))
 	if err != nil {
 		return nil, err
 	}
 
-	return &Entry{
-		Mode:     treeEntryMode,
-		Type:     objectType,
-		ObjectID: objectID,
-		Path:     string(treeEntryPath),
+	return &localrepo.TreeEntry{
+		Mode: string(treeEntryMode),
+		OID:  objectID,
+		Path: string(treeEntryPath),
+		Type: localrepo.ToEnum(string(treeEntryType)),
 	}, nil
 }
 
@@ -88,16 +83,3 @@ func (p *Parser) NextEntryPath() ([]byte, error) {
 	}
 	return treeEntryPath[:len(treeEntryPath)-1], nil
 }
-
-func toEnum(s string) (ObjectType, error) {
-	switch s {
-	case "tree":
-		return Tree, nil
-	case "blob":
-		return Blob, nil
-	case "commit":
-		return Submodule, nil
-	default:
-		return -1, ErrParse
-	}
-}
diff --git a/internal/git/lstree/parser_test.go b/internal/git/lstree/parser_test.go
index af4d540d7..277f42f83 100644
--- a/internal/git/lstree/parser_test.go
+++ b/internal/git/lstree/parser_test.go
@@ -8,6 +8,7 @@ import (
 	"github.com/stretchr/testify/require"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/gittest"
+	"gitlab.com/gitlab-org/gitaly/v15/internal/git/localrepo"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper/testcfg"
 )
@@ -35,35 +36,35 @@ func TestParser(t *testing.T) {
 	for _, tc := range []struct {
 		desc            string
 		treeID          git.ObjectID
-		expectedEntries Entries
+		expectedEntries localrepo.Entries
 	}{
 		{
 			desc:   "regular entries",
 			treeID: regularEntriesTreeID,
-			expectedEntries: Entries{
+			expectedEntries: localrepo.Entries{
 				{
-					Mode:     []byte("100644"),
-					Type:     Blob,
-					ObjectID: gitignoreBlobID,
-					Path:     ".gitignore",
+					Mode: "100644",
+					Type: localrepo.Blob,
+					OID:  gitignoreBlobID,
+					Path: ".gitignore",
 				},
 				{
-					Mode:     []byte("100644"),
-					Type:     Blob,
-					ObjectID: gitmodulesBlobID,
-					Path:     ".gitmodules",
+					Mode: "100644",
+					Type: localrepo.Blob,
+					OID:  gitmodulesBlobID,
+					Path: ".gitmodules",
 				},
 				{
-					Mode:     []byte("040000"),
-					Type:     Tree,
-					ObjectID: gittest.DefaultObjectHash.EmptyTreeOID,
-					Path:     "entry with space",
+					Mode: "040000",
+					Type: localrepo.Tree,
+					OID:  gittest.DefaultObjectHash.EmptyTreeOID,
+					Path: "entry with space",
 				},
 				{
-					Mode:     []byte("160000"),
-					Type:     Submodule,
-					ObjectID: submoduleCommitID,
-					Path:     "gitlab-shell",
+					Mode: "160000",
+					Type: localrepo.Submodule,
+					OID:  submoduleCommitID,
+					Path: "gitlab-shell",
 				},
 			},
 		},
@@ -72,7 +73,7 @@ func TestParser(t *testing.T) {
 			treeData := gittest.Exec(t, cfg, "-C", repoPath, "ls-tree", "-z", tc.treeID.String())
 
 			parser := NewParser(bytes.NewReader(treeData), gittest.DefaultObjectHash)
-			parsedEntries := Entries{}
+			parsedEntries := localrepo.Entries{}
 			for {
 				entry, err := parser.NextEntry()
 				if err == io.EOF {
diff --git a/internal/gitaly/service/commit/list_files.go b/internal/gitaly/service/commit/list_files.go
index 39e79710d..6956e5dfe 100644
--- a/internal/gitaly/service/commit/list_files.go
+++ b/internal/gitaly/service/commit/list_files.go
@@ -6,6 +6,7 @@ import (
 	"github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus/ctxlogrus"
 	log "github.com/sirupsen/logrus"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git"
+	"gitlab.com/gitlab-org/gitaly/v15/internal/git/localrepo"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/lstree"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/service"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/helper/chunk"
@@ -94,7 +95,7 @@ func (s *server) listFiles(repo git.RepositoryExecutor, revision string, stream
 			return err
 		}
 
-		if entry.Type != lstree.Blob {
+		if entry.Type != localrepo.Blob {
 			continue
 		}
 
diff --git a/internal/gitaly/service/commit/list_last_commits_for_tree.go b/internal/gitaly/service/commit/list_last_commits_for_tree.go
index 20409fb93..84a9bab61 100644
--- a/internal/gitaly/service/commit/list_last_commits_for_tree.go
+++ b/internal/gitaly/service/commit/list_last_commits_for_tree.go
@@ -7,6 +7,7 @@ import (
 
 	"gitlab.com/gitlab-org/gitaly/v15/internal/command"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git"
+	"gitlab.com/gitlab-org/gitaly/v15/internal/git/localrepo"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/log"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/lstree"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/service"
@@ -51,7 +52,7 @@ func (s *server) listLastCommitsForTree(in *gitalypb.ListLastCommitsForTreeReque
 	offset := int(in.GetOffset())
 	if offset >= len(entries) {
 		offset = 0
-		entries = lstree.Entries{}
+		entries = localrepo.Entries{}
 	}
 
 	limit := offset + int(in.GetLimit())
@@ -87,8 +88,8 @@ func (s *server) listLastCommitsForTree(in *gitalypb.ListLastCommitsForTreeReque
 	return sendCommitsForTree(batch, stream)
 }
 
-func getLSTreeEntries(parser *lstree.Parser) (lstree.Entries, error) {
-	entries := lstree.Entries{}
+func getLSTreeEntries(parser *lstree.Parser) (localrepo.Entries, error) {
+	entries := localrepo.Entries{}
 
 	for {
 		entry, err := parser.NextEntry()
diff --git a/internal/gitaly/service/commit/tree_entries.go b/internal/gitaly/service/commit/tree_entries.go
index 46bea9d5b..b1aa9e111 100644
--- a/internal/gitaly/service/commit/tree_entries.go
+++ b/internal/gitaly/service/commit/tree_entries.go
@@ -132,7 +132,7 @@ func (s *server) sendTreeEntries(
 
 		entries = make([]*gitalypb.TreeEntry, 0, len(treeEntries))
 		for _, entry := range treeEntries {
-			objectID, err := entry.ObjectID.Bytes()
+			objectID, err := entry.OID.Bytes()
 			if err != nil {
 				return fmt.Errorf("converting tree entry OID: %w", err)
 			}
@@ -143,7 +143,7 @@ func (s *server) sendTreeEntries(
 				path,
 				[]byte(entry.Path),
 				objectID,
-				entry.Mode,
+				[]byte(entry.Mode),
 			)
 			if err != nil {
 				return fmt.Errorf("converting tree entry: %w", err)
@@ -246,14 +246,14 @@ func sortTrees(entries []*gitalypb.TreeEntry, sortBy gitalypb.GetTreeEntriesRequ
 }
 
 // This is used to match the sorting order given by getLSTreeEntries
-func toLsTreeEnum(input gitalypb.TreeEntry_EntryType) (lstree.ObjectType, error) {
+func toLsTreeEnum(input gitalypb.TreeEntry_EntryType) (localrepo.ObjectType, error) {
 	switch input {
 	case gitalypb.TreeEntry_TREE:
-		return lstree.Tree, nil
+		return localrepo.Tree, nil
 	case gitalypb.TreeEntry_COMMIT:
-		return lstree.Submodule, nil
+		return localrepo.Submodule, nil
 	case gitalypb.TreeEntry_BLOB:
-		return lstree.Blob, nil
+		return localrepo.Blob, nil
 	default:
 		return -1, lstree.ErrParse
 	}
diff --git a/internal/gitaly/service/operations/submodules_test.go b/internal/gitaly/service/operations/submodules_test.go
index 02f35d8c9..8047932fc 100644
--- a/internal/gitaly/service/operations/submodules_test.go
+++ b/internal/gitaly/service/operations/submodules_test.go
@@ -669,7 +669,7 @@ func TestUserUpdateSubmodule(t *testing.T) {
 				parsedEntry, err := parser.NextEntry()
 				require.NoError(t, err)
 				require.Equal(t, tc.subPath, parsedEntry.Path)
-				require.Equal(t, setupData.commitID, parsedEntry.ObjectID.String())
+				require.Equal(t, setupData.commitID, parsedEntry.OID.String())
 			} else {
 				setupData.verify(t)
 			}
diff --git a/internal/gitaly/service/repository/license.go b/internal/gitaly/service/repository/license.go
index 42accce07..df90cebd7 100644
--- a/internal/gitaly/service/repository/license.go
+++ b/internal/gitaly/service/repository/license.go
@@ -236,7 +236,7 @@ func (f *gitFiler) ReadDir(string) ([]filer.File, error) {
 			return nil, err
 		}
 
-		if entry.Type != lstree.Blob {
+		if !entry.IsBlob() {
 			continue
 		}