Merge branch 'pks-objectpool-strip-remote-config' into 'master'

git/objectpool: Remove remote configuration after creation

Closes #4384

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

Merged-by: Sami Hiltunen <shiltunen@gitlab.com>
Approved-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Co-authored-by: Patrick Steinhardt <psteinhardt@gitlab.com>

4 files changed, 20 insertions, 17 deletions

diff --git a/internal/git/objectpool/create.go b/internal/git/objectpool/create.go
index 1117825f8..eb96458d9 100644
--- a/internal/git/objectpool/create.go
+++ b/internal/git/objectpool/create.go
@@ -6,7 +6,6 @@ import (
 	"errors"
 	"fmt"
 	"os"
-	"path/filepath"
 
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/catfile"
@@ -73,14 +72,26 @@ func Create(
 		return nil, fmt.Errorf("cloning to pool: %w, stderr: %q", err, stderr.String())
 	}
 
-	if err := os.RemoveAll(filepath.Join(objectPoolPath, "hooks")); err != nil {
-		return nil, fmt.Errorf("removing hooks: %v", err)
-	}
-
 	objectPool, err := FromProto(locator, gitCmdFactory, catfileCache, txManager, housekeepingManager, proto)
 	if err != nil {
 		return nil, err
 	}
 
+	// git-clone(1) writes the remote configuration into the object pool. We nowadays don't have
+	// remote configuration in the gitconfig anymore, so let's remove it before returning. Note
+	// that we explicitly don't use git-remote(1) to do this, as this command would also remove
+	// references part of the remote.
+	if err := objectPool.ExecAndWait(ctx, git.Command{
+		Name: "config",
+		Flags: []git.Option{
+			git.Flag{Name: "--remove-section"},
+		},
+		Args: []string{
+			"remote.origin",
+		},
+	}); err != nil {
+		return nil, fmt.Errorf("removing origin remote config: %w", err)
+	}
+
 	return objectPool, nil
 }
diff --git a/internal/git/objectpool/create_test.go b/internal/git/objectpool/create_test.go
index b5c86b40b..0241b360b 100644
--- a/internal/git/objectpool/create_test.go
+++ b/internal/git/objectpool/create_test.go
@@ -18,7 +18,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/localrepo"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/config"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/transaction"
-	"gitlab.com/gitlab-org/gitaly/v15/internal/helper/text"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/structerr"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper/testcfg"
@@ -70,8 +69,8 @@ func TestCreate(t *testing.T) {
 
 		// There should not be a "hooks" directory in the pool.
 		require.NoDirExists(t, filepath.Join(poolPath, "hooks"))
-		// The "origin" remote of the pool points to the pool member.
-		require.Equal(t, repoPath, text.ChompBytes(gittest.Exec(t, cfg, "-C", poolPath, "remote", "get-url", "origin")))
+		// The repository has no remote.
+		require.Empty(t, gittest.Exec(t, cfg, "-C", poolPath, "remote"))
 		// The "master" branch points to the same commit as in the pool member.
 		require.Equal(t, commitID, gittest.ResolveRevision(t, cfg, poolPath, "refs/heads/master"))
 		// Objects exist in the pool repository.
diff --git a/internal/git/objectpool/link_test.go b/internal/git/objectpool/link_test.go
index 12afdf559..5a297ba70 100644
--- a/internal/git/objectpool/link_test.go
+++ b/internal/git/objectpool/link_test.go
@@ -41,7 +41,7 @@ func TestLink(t *testing.T) {
 	newContent := testhelper.MustReadFile(t, altPath)
 	require.Equal(t, content, newContent)
 
-	require.Equal(t, []byte("origin\n"), gittest.Exec(t, cfg, "-C", gittest.RepositoryPath(t, pool), "remote"))
+	require.Empty(t, gittest.Exec(t, cfg, "-C", gittest.RepositoryPath(t, pool), "remote"))
 }
 
 func TestLink_transactional(t *testing.T) {
@@ -130,5 +130,5 @@ func TestLink_absoluteLinkExists(t *testing.T) {
 	repoObjectsPath := filepath.Join(repoPath, "objects")
 	require.Equal(t, poolObjectsPath, filepath.Join(repoObjectsPath, string(content)), "the content of the alternates file should be the relative version of the absolute pat")
 
-	require.Equal(t, []byte("origin\n"), gittest.Exec(t, cfg, "-C", poolPath, "remote"))
+	require.Empty(t, gittest.Exec(t, cfg, "-C", poolPath, "remote"))
 }
diff --git a/internal/gitaly/service/objectpool/fetch_into_object_pool_test.go b/internal/gitaly/service/objectpool/fetch_into_object_pool_test.go
index 748db9899..ba9863dcf 100644
--- a/internal/gitaly/service/objectpool/fetch_into_object_pool_test.go
+++ b/internal/gitaly/service/objectpool/fetch_into_object_pool_test.go
@@ -129,13 +129,6 @@ func testFetchIntoObjectPooltransactional(t *testing.T, ctx context.Context) {
 
 	poolProto, pool, poolPath := createObjectPool(t, ctx, cfg, client, repo)
 
-	// CreateObjectPool has a bug because it will leave the configuration of the origin remote
-	// in the gitconfig. This will get cleaned up at a later point by our housekeeping logic, so
-	// it doesn't hurt much in the first place to have it. But the cleanup logic would trigger
-	// another transactional vote which we want to avoid, so we simply unset the configuration
-	// here.
-	gittest.Exec(t, cfg, "-C", poolPath, "config", "--unset", "remote.origin.url")
-
 	// Inject transaction information so that FetchInotObjectPool knows to perform
 	// transactional voting.
 	ctx, err = txinfo.InjectTransaction(peer.NewContext(ctx, &peer.Peer{}), 1, "node", true)