Merge branch 'zj-ff-upload-pack-sha1' into 'master'

upload-pack: Remove feature gate for partial clone

Closes gitlab#221311

See merge request gitlab-org/gitaly!2300

8 files changed, 19 insertions, 49 deletions

diff --git a/changelogs/unreleased/zj-ff-upload-pack-sha1.yml b/changelogs/unreleased/zj-ff-upload-pack-sha1.yml
new file mode 100644
index 000000000..c41443036
--- /dev/null
+++ b/changelogs/unreleased/zj-ff-upload-pack-sha1.yml
@@ -0,0 +1,5 @@
+---
+title: Remove upload-pack feature flags to enable partial clone by default
+merge_request:
+author:
+type: added
diff --git a/internal/metadata/featureflag/feature_flags.go b/internal/metadata/featureflag/feature_flags.go
index 40440b0f9..2c39e63c9 100644
--- a/internal/metadata/featureflag/feature_flags.go
+++ b/internal/metadata/featureflag/feature_flags.go
@@ -1,9 +1,6 @@
 package featureflag
 
 const (
-	// UploadPackFilter enables partial clones by sending uploadpack.allowFilter and uploadpack.allowAnySHA1InWant
-	// to upload-pack
-	UploadPackFilter = "upload_pack_filter"
 	// LinguistFileCountStats will invoke an additional git-linguist command to get the number of files per language
 	LinguistFileCountStats = "linguist_file_count_stats"
 	// GoUpdateHook will bypass the ruby update hook and use the go implementation of custom hooks
diff --git a/internal/service/smarthttp/inforefs.go b/internal/service/smarthttp/inforefs.go
index 21f0394ef..55cbd8f1c 100644
--- a/internal/service/smarthttp/inforefs.go
+++ b/internal/service/smarthttp/inforefs.go
@@ -10,7 +10,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/internal/git"
 	"gitlab.com/gitlab-org/gitaly/internal/git/pktline"
 	"gitlab.com/gitlab-org/gitaly/internal/helper"
-	"gitlab.com/gitlab-org/gitaly/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/proto/go/gitalypb"
 	"gitlab.com/gitlab-org/gitaly/streamio"
 	"google.golang.org/grpc/codes"
@@ -56,7 +55,7 @@ func handleInfoRefs(ctx context.Context, service string, req *gitalypb.InfoRefsR
 		globalOpts = append(globalOpts, git.ReceivePackConfig()...)
 	}
 
-	if service == "upload-pack" && featureflag.IsEnabled(ctx, featureflag.UploadPackFilter) {
+	if service == "upload-pack" {
 		globalOpts = append(globalOpts, git.UploadPackFilterConfig()...)
 	}
 
diff --git a/internal/service/smarthttp/inforefs_test.go b/internal/service/smarthttp/inforefs_test.go
index 05bbc8649..9fafe0c95 100644
--- a/internal/service/smarthttp/inforefs_test.go
+++ b/internal/service/smarthttp/inforefs_test.go
@@ -18,7 +18,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/internal/git/objectpool"
 	"gitlab.com/gitlab-org/gitaly/internal/git/stats"
 	"gitlab.com/gitlab-org/gitaly/internal/helper"
-	"gitlab.com/gitlab-org/gitaly/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/internal/tempdir"
 	"gitlab.com/gitlab-org/gitaly/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/proto/go/gitalypb"
@@ -56,25 +55,14 @@ func TestSuccessfulInfoRefsUploadWithPartialClone(t *testing.T) {
 		Repository: testRepo,
 	}
 
-	fullResponse, err := makeInfoRefsUploadPackRequest(ctx, t, serverSocketPath, request)
-	require.NoError(t, err)
-	fullRefs := stats.Get{}
-	err = fullRefs.Parse(bytes.NewReader(fullResponse))
-	require.NoError(t, err)
-
-	ctx = featureflag.OutgoingCtxWithFeatureFlag(ctx, featureflag.UploadPackFilter)
-
 	partialResponse, err := makeInfoRefsUploadPackRequest(ctx, t, serverSocketPath, request)
 	require.NoError(t, err)
 	partialRefs := stats.Get{}
 	err = partialRefs.Parse(bytes.NewReader(partialResponse))
 	require.NoError(t, err)
 
-	require.Equal(t, fullRefs.Refs, partialRefs.Refs)
-
 	for _, c := range []string{"allow-tip-sha1-in-want", "allow-reachable-sha1-in-want", "filter"} {
 		require.Contains(t, partialRefs.Caps, c)
-		require.NotContains(t, fullRefs.Caps, c)
 	}
 }
 
diff --git a/internal/service/smarthttp/upload_pack.go b/internal/service/smarthttp/upload_pack.go
index 69350abbb..eb3bdac87 100644
--- a/internal/service/smarthttp/upload_pack.go
+++ b/internal/service/smarthttp/upload_pack.go
@@ -10,7 +10,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/internal/git"
 	"gitlab.com/gitlab-org/gitaly/internal/git/stats"
 	"gitlab.com/gitlab-org/gitaly/internal/helper"
-	"gitlab.com/gitlab-org/gitaly/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/internal/service/inspect"
 	"gitlab.com/gitlab-org/gitaly/proto/go/gitalypb"
 	"gitlab.com/gitlab-org/gitaly/streamio"
@@ -75,11 +74,7 @@ func (s *server) PostUploadPack(stream gitalypb.SmartHTTPService_PostUploadPackS
 	}
 
 	git.WarnIfTooManyBitmaps(ctx, req.GetRepository())
-
-	var globalOpts []git.Option
-	if featureflag.IsEnabled(ctx, featureflag.UploadPackFilter) {
-		globalOpts = append(globalOpts, git.UploadPackFilterConfig()...)
-	}
+	globalOpts := git.UploadPackFilterConfig()
 
 	for _, o := range req.GitConfigOptions {
 		globalOpts = append(globalOpts, git.ValueFlag{"-c", o})
diff --git a/internal/service/smarthttp/upload_pack_test.go b/internal/service/smarthttp/upload_pack_test.go
index 780039a0e..7b6b6e631 100644
--- a/internal/service/smarthttp/upload_pack_test.go
+++ b/internal/service/smarthttp/upload_pack_test.go
@@ -19,7 +19,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/internal/git"
 	"gitlab.com/gitlab-org/gitaly/internal/git/pktline"
 	"gitlab.com/gitlab-org/gitaly/internal/helper"
-	"gitlab.com/gitlab-org/gitaly/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/proto/go/gitalypb"
 	"gitlab.com/gitlab-org/gitaly/streamio"
@@ -369,7 +368,7 @@ func TestUploadPackRequestForPartialCloneSuccess(t *testing.T) {
 	// UploadPack request is a "want" packet line followed by a packet flush, then many "have" packets followed by a packet flush.
 	// This is explained a bit in https://git-scm.com/book/en/v2/Git-Internals-Transfer-Protocols#_downloading_data
 
-	var requestBuffer, requestBufferForFailed bytes.Buffer
+	var requestBuffer bytes.Buffer
 	pktline.WriteString(&requestBuffer, fmt.Sprintf("want %s %s\n", newHead, clientCapabilities))
 	pktline.WriteString(&requestBuffer, fmt.Sprintf("filter %s\n", "blob:limit=200"))
 	pktline.WriteFlush(&requestBuffer)
@@ -386,13 +385,6 @@ func TestUploadPackRequestForPartialCloneSuccess(t *testing.T) {
 	ctx, cancel := testhelper.Context()
 	defer cancel()
 
-	requestBufferForFailed = requestBuffer
-
-	_, err = makePostUploadPackRequest(ctx, t, serverSocketPath, req, &requestBufferForFailed)
-	require.Error(t, err, "trying to use filters without the feature flag should result in an error")
-
-	ctx = featureflag.OutgoingCtxWithFeatureFlag(ctx, featureflag.UploadPackFilter)
-
 	responseBuffer, err := makePostUploadPackRequest(ctx, t, serverSocketPath, req, &requestBuffer)
 	require.NoError(t, err)
 
@@ -430,5 +422,5 @@ func TestUploadPackRequestForPartialCloneSuccess(t *testing.T) {
 
 	metric, err := negotiationMetrics.GetMetricWithLabelValues("filter")
 	require.NoError(t, err)
-	require.Equal(t, 2.0, promtest.ToFloat64(metric))
+	require.Equal(t, 1.0, promtest.ToFloat64(metric))
 }
diff --git a/internal/service/ssh/upload_pack.go b/internal/service/ssh/upload_pack.go
index cf1a4c527..0c9e60342 100644
--- a/internal/service/ssh/upload_pack.go
+++ b/internal/service/ssh/upload_pack.go
@@ -13,7 +13,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/internal/git/pktline"
 	"gitlab.com/gitlab-org/gitaly/internal/git/stats"
 	"gitlab.com/gitlab-org/gitaly/internal/helper"
-	"gitlab.com/gitlab-org/gitaly/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/internal/service/inspect"
 	"gitlab.com/gitlab-org/gitaly/proto/go/gitalypb"
 	"gitlab.com/gitlab-org/gitaly/streamio"
@@ -77,11 +76,7 @@ func (s *server) sshUploadPack(stream gitalypb.SSHService_SSHUploadPackServer, r
 
 	git.WarnIfTooManyBitmaps(ctx, req.GetRepository())
 
-	var globalOpts []git.Option
-	if featureflag.IsEnabled(ctx, featureflag.UploadPackFilter) {
-		globalOpts = append(globalOpts, git.UploadPackFilterConfig()...)
-	}
-
+	globalOpts := git.UploadPackFilterConfig()
 	for _, o := range req.GitConfigOptions {
 		globalOpts = append(globalOpts, git.ValueFlag{"-c", o})
 	}
diff --git a/internal/service/ssh/upload_pack_test.go b/internal/service/ssh/upload_pack_test.go
index a7307fd84..96158c668 100644
--- a/internal/service/ssh/upload_pack_test.go
+++ b/internal/service/ssh/upload_pack_test.go
@@ -17,7 +17,6 @@ import (
 	"github.com/stretchr/testify/require"
 	"gitlab.com/gitlab-org/gitaly/internal/git"
 	"gitlab.com/gitlab-org/gitaly/internal/helper/text"
-	"gitlab.com/gitlab-org/gitaly/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/proto/go/gitalypb"
 	"google.golang.org/grpc/codes"
@@ -240,22 +239,23 @@ func TestUploadPackCloneWithPartialCloneFilter(t *testing.T) {
 	blobGreaterThanLimit := "18079e308ff9b3a5e304941020747e5c39b46c88"
 
 	tests := []struct {
-		desc     string
-		flags    []string
-		repoTest func(t *testing.T, repoPath string)
+		desc      string
+		repoTest  func(t *testing.T, repoPath string)
+		cloneArgs []string
 	}{
 		{
 			desc: "full_clone",
 			repoTest: func(t *testing.T, repoPath string) {
 				testhelper.GitObjectMustExist(t, repoPath, blobGreaterThanLimit)
 			},
+			cloneArgs: []string{"clone", "git@localhost:test/test.git"},
 		},
 		{
-			desc:  "partial_clone",
-			flags: []string{featureflag.UploadPackFilter},
+			desc: "partial_clone",
 			repoTest: func(t *testing.T, repoPath string) {
 				testhelper.GitObjectMustNotExist(t, repoPath, blobGreaterThanLimit)
 			},
+			cloneArgs: []string{"clone", "--filter=blob:limit=2048", "git@localhost:test/test.git"},
 		},
 	}
 
@@ -266,10 +266,9 @@ func TestUploadPackCloneWithPartialCloneFilter(t *testing.T) {
 			// UploadPackFilter flag disabled.
 			localPath := path.Join(testRepoRoot, fmt.Sprintf("gitlab-test-upload-pack-local-%s", tc.desc))
 			cmd := cloneCommand{
-				repository:   testRepo,
-				command:      exec.Command("git", "clone", "--filter=blob:limit=2048", "git@localhost:test/test.git", localPath),
-				server:       serverSocketPath,
-				featureFlags: tc.flags,
+				repository: testRepo,
+				command:    exec.Command("git", append(tc.cloneArgs, localPath)...),
+				server:     serverSocketPath,
 			}
 			err := cmd.execute(t)
 			defer os.RemoveAll(localPath)