Refactoring: Git interfaces un-exposure, and move of test-server to test tooling (kptdev#503)

* Move testing git server to /test/git/pkg

* Move all package internal types and functions to private

* Prevent test imports during production builds of images

* Remove deadcode

* Found 2 more methods

---------

Co-authored-by: Liam Fallon <35595825+liamfallon@users.noreply.github.com>

Author: nagygergo

pkg/cache/crcache/cache_test.go

@@ -30,9 +30,9 @@ import (
 	"github.com/nephio-project/porch/pkg/cache/repomap"
 	cachetypes "github.com/nephio-project/porch/pkg/cache/types"
 	"github.com/nephio-project/porch/pkg/externalrepo/fake"
-	"github.com/nephio-project/porch/pkg/externalrepo/git"
 	externalrepotypes "github.com/nephio-project/porch/pkg/externalrepo/types"
 	"github.com/nephio-project/porch/pkg/repository"
+	gitserver "github.com/nephio-project/porch/test/git/pkg"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -246,7 +246,7 @@ func openRepositoryFromArchive(t *testing.T, ctx context.Context, testPath, name
 
 	tempdir := t.TempDir()
 	tarfile := filepath.Join(testPath, fmt.Sprintf("%s-repository.tar", name))
-	_, address := git.ServeGitRepository(t, tarfile, tempdir)
+	_, address := gitserver.ServeGitRepository(t, tarfile, tempdir)
 	metadataStore := createMetadataStoreFromArchive(t, fmt.Sprintf("%s-metadata.yaml", name), name)
 
 	apiRepo := &v1alpha1.Repository{

pkg/externalrepo/git/annotation.go

@@ -46,10 +46,10 @@ type gitAnnotation struct {
 	Task *porchapi.Task `json:"task,omitempty"`
 }
 
-// ExtractGitAnnotations reads the gitAnnotations from the given commit.
+// extractGitAnnotations reads the gitAnnotations from the given commit.
 // If no annotation are found, it returns [], nil
 // If an invalid annotation is found, it returns an error.
-func ExtractGitAnnotations(commit *object.Commit) ([]gitAnnotation, error) {
+func extractGitAnnotations(commit *object.Commit) ([]gitAnnotation, error) {
 	annotations := []gitAnnotation{}
 	ec := errors.NewErrorCollector().WithSeparator(";").WithFormat("{%s}")
 
@@ -69,8 +69,8 @@ func ExtractGitAnnotations(commit *object.Commit) ([]gitAnnotation, error) {
 	return annotations, ec.Join()
 }
 
-// AnnotateCommitMessage adds the gitAnnotation to the commit message.
-func AnnotateCommitMessage(message string, annotation *gitAnnotation) (string, error) {
+// annotateCommitMessage adds the gitAnnotation to the commit message.
+func annotateCommitMessage(message string, annotation *gitAnnotation) (string, error) {
 	b, err := json.Marshal(annotation)
 	if err != nil {
 		return "", pkgerrors.Wrap(err, "error marshaling annotation")

pkg/externalrepo/git/branchcommithash_test.go

@@ -104,7 +104,7 @@ func TestBranchCommitHash(t *testing.T) {
 				// Validate commit exists if requested
 				if tc.validateCommit {
 					gitRepo := repo.(*gitRepository)
-					err := gitRepo.sharedDir.WithRLock(func(r *gogit.Repository) error {
+					err := gitRepo.sharedDir.withRLock(func(r *gogit.Repository) error {
 						_, err := r.CommitObject(plumbing.NewHash(hash))
 						return err
 					})

pkg/externalrepo/git/cachedir_pool.go

@@ -24,28 +24,28 @@ import (
 	"k8s.io/klog/v2"
 )
 
-// DirectoryPool manages shared access to cached git directories
-type DirectoryPool struct {
+// directoryPool manages shared access to cached git directories
+type directoryPool struct {
 	directories sync.Map
 	mutex       sync.Mutex
 }
 
-type SharedDirectory struct {
+type sharedDirectory struct {
 	repo     *git.Repository
 	mutex    sync.RWMutex
 	refCount int // number of gitRepository instances using this directory
 }
 
-var globalDirectoryPool = &DirectoryPool{
+var globalDirectoryPool = &directoryPool{
 	directories: sync.Map{},
 }
 
-// GetOrCreateSharedRepository safely initializes or reuses a cached git directory
-func (p *DirectoryPool) GetOrCreateSharedRepository(dir, reponame string) (*SharedDirectory, error) {
+// getOrCreateSharedRepository safely initializes or reuses a cached git directory
+func (p *directoryPool) getOrCreateSharedRepository(dir, reponame string) (*sharedDirectory, error) {
 	// Fast path: check if directory already exists
 	if sharedDir, exists := p.directories.Load(dir); exists {
 		p.mutex.Lock()
-		shared := sharedDir.(*SharedDirectory)
+		shared := sharedDir.(*sharedDirectory)
 		shared.refCount++
 		klog.V(2).Infof("Repo %s is reusing shared directory %s, refCount now: %d", reponame, dir, shared.refCount)
 		p.mutex.Unlock()
@@ -58,7 +58,7 @@ func (p *DirectoryPool) GetOrCreateSharedRepository(dir, reponame string) (*Shar
 
 	// Double-check after acquiring lock
 	if sharedDir, exists := p.directories.Load(dir); exists {
-		shared := sharedDir.(*SharedDirectory)
+		shared := sharedDir.(*sharedDirectory)
 		shared.refCount++
 		klog.V(2).Infof("Repo %s is reusing shared directory %s, refCount now: %d", reponame, dir, shared.refCount)
 		return shared, nil
@@ -93,7 +93,7 @@ func (p *DirectoryPool) GetOrCreateSharedRepository(dir, reponame string) (*Shar
 		}
 	}
 
-	shared := &SharedDirectory{
+	shared := &sharedDirectory{
 		repo:     repo,
 		refCount: 1,
 	}
@@ -102,13 +102,13 @@ func (p *DirectoryPool) GetOrCreateSharedRepository(dir, reponame string) (*Shar
 	return shared, nil
 }
 
-// ReleaseSharedRepository decrements reference count and cleans up cached git directory if needed
-func (p *DirectoryPool) ReleaseSharedRepository(dir, reponame string) {
+// releaseSharedRepository decrements reference count and cleans up cached git directory if needed
+func (p *directoryPool) releaseSharedRepository(dir, reponame string) {
 	p.mutex.Lock()
 	defer p.mutex.Unlock()
 
 	if sharedDir, exists := p.directories.Load(dir); exists {
-		shared := sharedDir.(*SharedDirectory)
+		shared := sharedDir.(*sharedDirectory)
 		shared.refCount--
 		klog.V(2).Infof("Released repo %s from %s, refCount now: %d", reponame, filepath.Base(dir), shared.refCount)
 
@@ -131,15 +131,15 @@ func (p *DirectoryPool) ReleaseSharedRepository(dir, reponame string) {
 	}
 }
 
-// WithLock executes function with exclusive access to the cached git directory
-func (s *SharedDirectory) WithLock(fn func(*git.Repository) error) error {
+// withLock executes function with exclusive access to the cached git directory
+func (s *sharedDirectory) withLock(fn func(*git.Repository) error) error {
 	s.mutex.Lock()
 	defer s.mutex.Unlock()
 	return fn(s.repo)
 }
 
-// WithRLock executes function with read-only access to the cached git directory
-func (s *SharedDirectory) WithRLock(fn func(*git.Repository) error) error {
+// withRLock executes function with read-only access to the cached git directory
+func (s *sharedDirectory) withRLock(fn func(*git.Repository) error) error {
 	s.mutex.RLock()
 	defer s.mutex.RUnlock()
 	return fn(s.repo)

pkg/externalrepo/git/cachedir_pool_test.go

@@ -26,48 +26,48 @@ import (
 )
 
 func TestDirectoryPool_GetOrCreateSharedRepository(t *testing.T) {
-	pool := &DirectoryPool{
+	pool := &directoryPool{
 		directories: sync.Map{},
 	}
 
 	tempDir := t.TempDir()
 	repoDir := filepath.Join(tempDir, "test-repo")
 
 	// First call should create new shared directory
-	shared1, err := pool.GetOrCreateSharedRepository(repoDir, "test-repo")
+	shared1, err := pool.getOrCreateSharedRepository(repoDir, "test-repo")
 	require.NoError(t, err)
 	require.NotNil(t, shared1)
 	assert.Equal(t, 1, shared1.refCount)
 
 	// Second call should reuse existing directory
-	shared2, err := pool.GetOrCreateSharedRepository(repoDir, "test-repo")
+	shared2, err := pool.getOrCreateSharedRepository(repoDir, "test-repo")
 	require.NoError(t, err)
 	assert.Same(t, shared1, shared2)
 	assert.Equal(t, 2, shared2.refCount)
 }
 
 func TestDirectoryPool_ReleaseSharedRepository(t *testing.T) {
-	pool := &DirectoryPool{
+	pool := &directoryPool{
 		directories: sync.Map{},
 	}
 
 	tempDir := t.TempDir()
 	repoDir := filepath.Join(tempDir, "test-repo")
 
 	// Create shared repository
-	shared, err := pool.GetOrCreateSharedRepository(repoDir, "test-repo")
+	shared, err := pool.getOrCreateSharedRepository(repoDir, "test-repo")
 	require.NoError(t, err)
 
 	// Add another reference
-	_, err = pool.GetOrCreateSharedRepository(repoDir, "test-repo")
+	_, err = pool.getOrCreateSharedRepository(repoDir, "test-repo")
 	require.NoError(t, err)
 
 	// First release should decrement refCount
-	pool.ReleaseSharedRepository(repoDir, "test-repo")
+	pool.releaseSharedRepository(repoDir, "test-repo")
 	assert.Equal(t, 1, shared.refCount)
 
 	// Second release should remove from pool and cleanup directory
-	pool.ReleaseSharedRepository(repoDir, "test-repo")
+	pool.releaseSharedRepository(repoDir, "test-repo")
 	_, exists := pool.directories.Load(repoDir)
 	assert.False(t, exists)
 	_, err = os.Stat(repoDir)
@@ -79,13 +79,13 @@ func TestSharedDirectory_WithLock(t *testing.T) {
 	repo, err := initEmptyRepository(tempDir)
 	require.NoError(t, err)
 
-	shared := &SharedDirectory{
+	shared := &sharedDirectory{
 		repo:     repo,
 		refCount: 1,
 	}
 
 	called := false
-	err = shared.WithLock(func(r *git.Repository) error {
+	err = shared.withLock(func(r *git.Repository) error {
 		called = true
 		assert.Same(t, repo, r)
 		return nil
@@ -100,13 +100,13 @@ func TestSharedDirectory_WithRLock(t *testing.T) {
 	repo, err := initEmptyRepository(tempDir)
 	require.NoError(t, err)
 
-	shared := &SharedDirectory{
+	shared := &sharedDirectory{
 		repo:     repo,
 		refCount: 1,
 	}
 
 	called := false
-	err = shared.WithRLock(func(r *git.Repository) error {
+	err = shared.withRLock(func(r *git.Repository) error {
 		called = true
 		assert.Same(t, repo, r)
 		return nil
@@ -117,7 +117,7 @@ func TestSharedDirectory_WithRLock(t *testing.T) {
 }
 
 func TestDirectoryPool_ConcurrentAccess(t *testing.T) {
-	pool := &DirectoryPool{
+	pool := &directoryPool{
 		directories: sync.Map{},
 	}
 
@@ -132,15 +132,15 @@ func TestDirectoryPool_ConcurrentAccess(t *testing.T) {
 		wg.Add(1)
 		go func(id int) {
 			defer wg.Done()
-			_, err := pool.GetOrCreateSharedRepository(repoDir, "concurrent-repo")
+			_, err := pool.getOrCreateSharedRepository(repoDir, "concurrent-repo")
 			assert.NoError(t, err)
 		}(i)
 	}
 	wg.Wait()
 
 	// Check final refCount
 	sharedDir, _ := pool.directories.Load(repoDir)
-	shared := sharedDir.(*SharedDirectory)
+	shared := sharedDir.(*sharedDirectory)
 
 	assert.Equal(t, numGoroutines, shared.refCount)
 
@@ -149,7 +149,7 @@ func TestDirectoryPool_ConcurrentAccess(t *testing.T) {
 		wg.Add(1)
 		go func() {
 			defer wg.Done()
-			pool.ReleaseSharedRepository(repoDir, "concurrent-repo")
+			pool.releaseSharedRepository(repoDir, "concurrent-repo")
 		}()
 	}
 	wg.Wait()
@@ -160,7 +160,7 @@ func TestDirectoryPool_ConcurrentAccess(t *testing.T) {
 }
 
 func TestDirectoryPool_InitEmptyRepositoryFailure(t *testing.T) {
-	pool := &DirectoryPool{
+	pool := &directoryPool{
 		directories: sync.Map{},
 	}
 
@@ -174,7 +174,7 @@ func TestDirectoryPool_InitEmptyRepositoryFailure(t *testing.T) {
 	repoDir := filepath.Join(filePath, "subdir") // This will fail because parent is a file
 
 	// Should fail to create repository
-	_, err = pool.GetOrCreateSharedRepository(repoDir, "test-repo")
+	_, err = pool.getOrCreateSharedRepository(repoDir, "test-repo")
 	assert.Error(t, err)
 
 	// Should not be in pool
@@ -183,12 +183,12 @@ func TestDirectoryPool_InitEmptyRepositoryFailure(t *testing.T) {
 
 	// Retry should work if we fix the issue
 	os.Remove(filePath)
-	_, err = pool.GetOrCreateSharedRepository(repoDir, "test-repo")
+	_, err = pool.getOrCreateSharedRepository(repoDir, "test-repo")
 	assert.NoError(t, err)
 }
 
 func TestDirectoryPool_OpenRepositoryFailure(t *testing.T) {
-	pool := &DirectoryPool{
+	pool := &directoryPool{
 		directories: sync.Map{},
 	}
 
@@ -203,7 +203,7 @@ func TestDirectoryPool_OpenRepositoryFailure(t *testing.T) {
 	require.NoError(t, err)
 
 	// Should fail to open corrupted repository
-	_, err = pool.GetOrCreateSharedRepository(repoDir, "test-repo")
+	_, err = pool.getOrCreateSharedRepository(repoDir, "test-repo")
 	assert.Error(t, err)
 	assert.Contains(t, err.Error(), "corrupted cache was removed")
 
@@ -216,12 +216,12 @@ func TestDirectoryPool_OpenRepositoryFailure(t *testing.T) {
 	assert.True(t, os.IsNotExist(err))
 
 	// Retry should work now that corrupted dir is removed
-	_, err = pool.GetOrCreateSharedRepository(repoDir, "test-repo")
+	_, err = pool.getOrCreateSharedRepository(repoDir, "test-repo")
 	assert.NoError(t, err)
 }
 
 func TestDirectoryPool_OpenRepositoryCleanupFailure(t *testing.T) {
-	pool := &DirectoryPool{
+	pool := &directoryPool{
 		directories: sync.Map{},
 	}
 
@@ -242,7 +242,7 @@ func TestDirectoryPool_OpenRepositoryCleanupFailure(t *testing.T) {
 	defer os.Chmod(repoDir, 0755)
 
 	// Should fail to open and also fail to cleanup
-	_, err = pool.GetOrCreateSharedRepository(repoDir, "test-repo")
+	_, err = pool.getOrCreateSharedRepository(repoDir, "test-repo")
 	assert.Error(t, err)
 	// When cleanup fails, error message should mention checking local cache
 	assert.Contains(t, err.Error(), "check the local git cache")
@@ -257,7 +257,7 @@ func TestDirectoryPool_OpenRepositoryCleanupFailure(t *testing.T) {
 }
 
 func TestDirectoryPool_PathIsFile(t *testing.T) {
-	pool := &DirectoryPool{
+	pool := &directoryPool{
 		directories: sync.Map{},
 	}
 
@@ -269,7 +269,7 @@ func TestDirectoryPool_PathIsFile(t *testing.T) {
 	require.NoError(t, err)
 
 	// Should fail because path is a file, not a directory
-	_, err = pool.GetOrCreateSharedRepository(filePath, "test-repo")
+	_, err = pool.getOrCreateSharedRepository(filePath, "test-repo")
 	assert.Error(t, err)
 	assert.Contains(t, err.Error(), "not a directory")
 

pkg/externalrepo/git/commit_test.go

@@ -92,7 +92,7 @@ func TestPackageCommitToMain(t *testing.T) {
 
 	// Commit `bucket`` package from drafts/bucket/v1 into main
 
-	main := resolveReference(t, gitRepo, DefaultMainReferenceName)
+	main := resolveReference(t, gitRepo, defaultMainReferenceName)
 	packagePath := "bucket"
 
 	// Confirm no 'bucket' package in main
@@ -143,7 +143,7 @@ func TestCommitWithUser(t *testing.T) {
 	gitRepo := OpenGitRepositoryFromArchive(t, filepath.Join("testdata", "trivial-repository.tar"), tempdir)
 
 	ctx := context.Background()
-	main := resolveReference(t, gitRepo, DefaultMainReferenceName)
+	main := resolveReference(t, gitRepo, defaultMainReferenceName)
 
 	{
 		const testEmail = "porch-test@porch-domain.com"