fix(git): optimize listFiles() BE-11184 (#12160)

api/git/git.go

@@ -6,6 +6,8 @@ import (
 	"path/filepath"
 	"strings"
 
+	gittypes "github.com/portainer/portainer/api/git/types"
+
 	"github.com/go-git/go-git/v5"
 	"github.com/go-git/go-git/v5/config"
 	"github.com/go-git/go-git/v5/plumbing"
@@ -14,7 +16,6 @@ import (
 	githttp "github.com/go-git/go-git/v5/plumbing/transport/http"
 	"github.com/go-git/go-git/v5/storage/memory"
 	"github.com/pkg/errors"
-	gittypes "github.com/portainer/portainer/api/git/types"
 )
 
 type gitClient struct {
@@ -143,6 +144,7 @@ func (c *gitClient) listFiles(ctx context.Context, opt fetchOption) ([]string, e
 		ReferenceName:   plumbing.ReferenceName(opt.referenceName),
 		Auth:            getAuth(opt.username, opt.password),
 		InsecureSkipTLS: opt.tlsSkipVerify,
+		Tags:            git.NoTags,
 	}
 
 	repo, err := git.Clone(memory.NewStorage(), nil, cloneOption)
@@ -166,7 +168,10 @@ func (c *gitClient) listFiles(ctx context.Context, opt fetchOption) ([]string, e
 	}
 
 	var allPaths []string
+
 	w := object.NewTreeWalker(tree, true, nil)
+	defer w.Close()
+
 	for {
 		name, entry, err := w.Next()
 		if err != nil {

api/git/git_test.go

@@ -91,6 +91,29 @@ func Test_latestCommitID(t *testing.T) {
 	assert.Equal(t, "68dcaa7bd452494043c64252ab90db0f98ecf8d2", id)
 }
 
+func Test_ListRefs(t *testing.T) {
+	service := Service{git: NewGitClient(true)}
+
+	repositoryURL := setup(t)
+
+	fs, err := service.ListRefs(repositoryURL, "", "", false, false)
+
+	assert.NoError(t, err)
+	assert.Equal(t, []string{"refs/heads/main"}, fs)
+}
+
+func Test_ListFiles(t *testing.T) {
+	service := Service{git: NewGitClient(true)}
+
+	repositoryURL := setup(t)
+	referenceName := "refs/heads/main"
+
+	fs, err := service.ListFiles(repositoryURL, referenceName, "", "", false, false, []string{".yml"}, false)
+
+	assert.NoError(t, err)
+	assert.Equal(t, []string{"docker-compose.yml"}, fs)
+}
+
 func getCommitHistoryLength(t *testing.T, err error, dir string) int {
 	repo, err := git.PlainOpen(dir)
 	if err != nil {

api/git/service.go

@@ -9,6 +9,7 @@ import (
 
 	lru "github.com/hashicorp/golang-lru"
 	"github.com/rs/zerolog/log"
+	"golang.org/x/sync/singleflight"
 )
 
 const (
@@ -139,12 +140,18 @@ func (service *Service) CloneRepository(destination, repositoryURL, referenceNam
 	return service.cloneRepository(destination, options)
 }
 
-func (service *Service) cloneRepository(destination string, options cloneOption) error {
+func (service *Service) repoManager(options baseOption) repoManager {
+	repoManager := service.git
+
 	if isAzureUrl(options.repositoryUrl) {
-		return service.azure.download(context.TODO(), destination, options)
+		repoManager = service.azure
 	}
 
-	return service.git.download(context.TODO(), destination, options)
+	return repoManager
+}
+
+func (service *Service) cloneRepository(destination string, options cloneOption) error {
+	return service.repoManager(options.baseOption).download(context.TODO(), destination, options)
 }
 
 // LatestCommitID returns SHA1 of the latest commit of the specified reference
@@ -159,11 +166,7 @@ func (service *Service) LatestCommitID(repositoryURL, referenceName, username, p
 		referenceName: referenceName,
 	}
 
-	if isAzureUrl(options.repositoryUrl) {
+	return service.repoManager(options.baseOption).latestCommitID(context.TODO(), options)
-		return service.azure.latestCommitID(context.TODO(), options)
-	}
-
-	return service.git.latestCommitID(context.TODO(), options)
 }
 
 // ListRefs will list target repository's references without cloning the repository
@@ -174,21 +177,16 @@ func (service *Service) ListRefs(repositoryURL, username, password string, hardR
 		service.repoRefCache.Remove(refCacheKey)
 		// Remove file caches pointed to the same repository
 		for _, fileCacheKey := range service.repoFileCache.Keys() {
-			key, ok := fileCacheKey.(string)
+			if key, ok := fileCacheKey.(string); ok && strings.HasPrefix(key, repositoryURL) {
-			if ok {
+				service.repoFileCache.Remove(key)
-				if strings.HasPrefix(key, repositoryURL) {
-					service.repoFileCache.Remove(key)
-				}
 			}
 		}
 	}
 
 	if service.repoRefCache != nil {
 		// Lookup the refs cache first
-		cache, ok := service.repoRefCache.Get(refCacheKey)
+		if cache, ok := service.repoRefCache.Get(refCacheKey); ok {
-		if ok {
+			if refs, ok := cache.([]string); ok {
-			refs, success := cache.([]string)
-			if success {
 				return refs, nil
 			}
 		}
@@ -201,33 +199,35 @@ func (service *Service) ListRefs(repositoryURL, username, password string, hardR
 		tlsSkipVerify: tlsSkipVerify,
 	}
 
-	var (
+	refs, err := service.repoManager(options).listRefs(context.TODO(), options)
-		refs []string
+	if err != nil {
-		err  error
+		return nil, err
-	)
-	if isAzureUrl(options.repositoryUrl) {
-		refs, err = service.azure.listRefs(context.TODO(), options)
-		if err != nil {
-			return nil, err
-		}
-	} else {
-		refs, err = service.git.listRefs(context.TODO(), options)
-		if err != nil {
-			return nil, err
-		}
 	}
 
 	if service.cacheEnabled && service.repoRefCache != nil {
 		service.repoRefCache.Add(refCacheKey, refs)
 	}
+
 	return refs, nil
 }
 
+var singleflightGroup = &singleflight.Group{}
+
 // ListFiles will list all the files of the target repository with specific extensions.
 // If extension is not provided, it will list all the files under the target repository
 func (service *Service) ListFiles(repositoryURL, referenceName, username, password string, dirOnly, hardRefresh bool, includedExts []string, tlsSkipVerify bool) ([]string, error) {
 	repoKey := generateCacheKey(repositoryURL, referenceName, username, password, strconv.FormatBool(tlsSkipVerify), strconv.FormatBool(dirOnly))
 
+	fs, err, _ := singleflightGroup.Do(repoKey, func() (any, error) {
+		return service.listFiles(repositoryURL, referenceName, username, password, dirOnly, hardRefresh, tlsSkipVerify)
+	})
+
+	return filterFiles(fs.([]string), includedExts), err
+}
+
+func (service *Service) listFiles(repositoryURL, referenceName, username, password string, dirOnly, hardRefresh bool, tlsSkipVerify bool) ([]string, error) {
+	repoKey := generateCacheKey(repositoryURL, referenceName, username, password, strconv.FormatBool(tlsSkipVerify), strconv.FormatBool(dirOnly))
+
 	if service.cacheEnabled && hardRefresh {
 		// Should remove the cache explicitly, so that the following normal list can show the correct result
 		service.repoFileCache.Remove(repoKey)
@@ -235,14 +235,9 @@ func (service *Service) ListFiles(repositoryURL, referenceName, username, passwo
 
 	if service.repoFileCache != nil {
 		// lookup the files cache first
-		cache, ok := service.repoFileCache.Get(repoKey)
+		if cache, ok := service.repoFileCache.Get(repoKey); ok {
-		if ok {
+			if files, ok := cache.([]string); ok {
-			files, success := cache.([]string)
+				return files, nil
-			if success {
-				// For the case while searching files in a repository without include extensions for the first time,
-				// but with include extensions for the second time
-				includedFiles := filterFiles(files, includedExts)
-				return includedFiles, nil
 			}
 		}
 	}
@@ -258,28 +253,16 @@ func (service *Service) ListFiles(repositoryURL, referenceName, username, passwo
 		dirOnly:       dirOnly,
 	}
 
-	var (
+	files, err := service.repoManager(options.baseOption).listFiles(context.TODO(), options)
-		files []string
+	if err != nil {
-		err   error
+		return nil, err
-	)
-	if isAzureUrl(options.repositoryUrl) {
-		files, err = service.azure.listFiles(context.TODO(), options)
-		if err != nil {
-			return nil, err
-		}
-	} else {
-		files, err = service.git.listFiles(context.TODO(), options)
-		if err != nil {
-			return nil, err
-		}
 	}
 
-	includedFiles := filterFiles(files, includedExts)
 	if service.cacheEnabled && service.repoFileCache != nil {
-		service.repoFileCache.Add(repoKey, includedFiles)
+		service.repoFileCache.Add(repoKey, files)
-		return includedFiles, nil
 	}
-	return includedFiles, nil
+
+	return files, nil
 }
 
 func (service *Service) purgeCache() {
@@ -306,6 +289,7 @@ func matchExtensions(target string, exts []string) bool {
 			return true
 		}
 	}
+
 	return false
 }
 
@@ -316,10 +300,11 @@ func filterFiles(paths []string, includedExts []string) []string {
 
 	var includedFiles []string
 	for _, filename := range paths {
-		// filter out the filenames with non-included extension
+		// Filter out the filenames with non-included extension
 		if matchExtensions(filename, includedExts) {
 			includedFiles = append(includedFiles, filename)
 		}
 	}
+
 	return includedFiles
 }