Merge branch 'kn-4520-fetch-source-branch-quarantine-2' into 'master'

Make use of a quarantine repo in FetchSourceBranch

Closes #4520

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

Merged-by: John Cai <jcai@gitlab.com>
Approved-by: John Cai <jcai@gitlab.com>
Reviewed-by: Pavlo Strokov <pstrokov@gitlab.com>
Reviewed-by: John Cai <jcai@gitlab.com>
Reviewed-by: karthik nayak <knayak@gitlab.com>
Co-authored-by: Karthik Nayak <knayak@gitlab.com>

5 files changed, 64 insertions, 10 deletions

diff --git a/internal/git/quarantine/quarantine.go b/internal/git/quarantine/quarantine.go
index fa8d7e617..41afb8e1f 100644
--- a/internal/git/quarantine/quarantine.go
+++ b/internal/git/quarantine/quarantine.go
@@ -29,8 +29,8 @@ type Dir struct {
 	locator         storage.Locator
 }
 
-// New creates a new quarantine directory and returns both the directory as well as its cleanup
-// function.
+// New creates a new quarantine directory and returns the directory. The repository is cleaned
+// up when the user invokes the Migrate() functionality on the Dir.
 func New(ctx context.Context, repo *gitalypb.Repository, locator storage.Locator) (*Dir, error) {
 	repoPath, err := locator.GetPath(repo)
 	if err != nil {
diff --git a/internal/gitaly/service/repository/fetch.go b/internal/gitaly/service/repository/fetch.go
index afbfaf3ed..a411fff8d 100644
--- a/internal/gitaly/service/repository/fetch.go
+++ b/internal/gitaly/service/repository/fetch.go
@@ -7,9 +7,11 @@ import (
 	"github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus/ctxlogrus"
 	"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/quarantine"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/remoterepo"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/service"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/helper"
+	"gitlab.com/gitlab-org/gitaly/v15/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/structerr"
 	"gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb"
 )
@@ -32,7 +34,18 @@ func (s *server) FetchSourceBranch(ctx context.Context, req *gitalypb.FetchSourc
 		return nil, structerr.NewInvalidArgument("%w", err)
 	}
 
-	targetRepo := s.localrepo(req.GetRepository())
+	origTargetRepo := s.localrepo(req.GetRepository())
+	targetRepo := origTargetRepo
+
+	var quarantineDir *quarantine.Dir
+	if featureflag.FetchSourceBranchQuarantined.IsEnabled(ctx) {
+		var err error
+
+		quarantineDir, targetRepo, err = s.quarantinedRepo(ctx, req.GetRepository())
+		if err != nil {
+			return nil, err
+		}
+	}
 
 	sourceRepo, err := remoterepo.New(ctx, req.GetSourceRepository(), s.conns)
 	if err != nil {
@@ -99,7 +112,13 @@ func (s *server) FetchSourceBranch(ctx context.Context, req *gitalypb.FetchSourc
 		}
 	}
 
-	if err := targetRepo.UpdateRef(ctx, git.ReferenceName(req.GetTargetRef()), sourceOid, ""); err != nil {
+	if quarantineDir != nil {
+		if err := quarantineDir.Migrate(); err != nil {
+			return nil, structerr.NewInternal("migrating quarantined objects: %w", err)
+		}
+	}
+
+	if err := origTargetRepo.UpdateRef(ctx, git.ReferenceName(req.GetTargetRef()), sourceOid, ""); err != nil {
 		return nil, err
 	}
 
diff --git a/internal/gitaly/service/repository/fetch_test.go b/internal/gitaly/service/repository/fetch_test.go
index 0091da3b9..f976036d7 100644
--- a/internal/gitaly/service/repository/fetch_test.go
+++ b/internal/gitaly/service/repository/fetch_test.go
@@ -3,6 +3,7 @@
 package repository
 
 import (
+	"context"
 	"fmt"
 	"testing"
 
@@ -11,6 +12,7 @@ import (
 	"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/gitaly/config"
+	"gitlab.com/gitlab-org/gitaly/v15/internal/metadata/featureflag"
 	"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"
@@ -21,7 +23,11 @@ import (
 func TestFetchSourceBranch(t *testing.T) {
 	t.Parallel()
 
-	ctx := testhelper.Context(t)
+	testhelper.NewFeatureSets(featureflag.FetchSourceBranchQuarantined).Run(t, testFetchSourceBranch)
+}
+
+func testFetchSourceBranch(t *testing.T, ctx context.Context) {
+	t.Parallel()
 
 	type setupData struct {
 		cfg     config.Cfg
@@ -55,7 +61,7 @@ func TestFetchSourceBranch(t *testing.T) {
 						TargetRef:        []byte("refs/tmp/fetch-source-branch-test"),
 					},
 					verify: func() {
-						actualCommitID := gittest.ResolveRevision(t, cfg, repoPath, "refs/tmp/fetch-source-branch-test")
+						actualCommitID := gittest.ResolveRevision(t, cfg, repoPath, "refs/tmp/fetch-source-branch-test^{commit}")
 						require.Equal(t, commitID, actualCommitID)
 					},
 				}
@@ -80,7 +86,7 @@ func TestFetchSourceBranch(t *testing.T) {
 						TargetRef:        []byte("refs/tmp/fetch-source-branch-test"),
 					},
 					verify: func() {
-						actualCommitID := gittest.ResolveRevision(t, cfg, sourceRepoPath, "refs/tmp/fetch-source-branch-test")
+						actualCommitID := gittest.ResolveRevision(t, cfg, sourceRepoPath, "refs/tmp/fetch-source-branch-test^{commit}")
 						require.Equal(t, commitID, actualCommitID)
 					},
 				}
@@ -405,9 +411,11 @@ func TestFetchSourceBranch(t *testing.T) {
 						repo := localrepo.NewTestRepo(t, cfg, repoProto)
 						exists, err := repo.HasRevision(ctx, commitID.Revision()+"^{commit}")
 						require.NoError(t, err)
-						// TODO: This should be fixed in:
-						// https://gitlab.com/gitlab-org/gitaly/-/issues/4520
-						require.True(t, exists, "fetched commit isn't discarded")
+						if featureflag.FetchSourceBranchQuarantined.IsEnabled(ctx) {
+							require.False(t, exists, "fetched commit isn't discarded")
+						} else {
+							require.True(t, exists, "fetched commit is discarded")
+						}
 					},
 				}
 			},
diff --git a/internal/gitaly/service/repository/server.go b/internal/gitaly/service/repository/server.go
index d63bfade7..b6924a0a2 100644
--- a/internal/gitaly/service/repository/server.go
+++ b/internal/gitaly/service/repository/server.go
@@ -1,17 +1,21 @@
 package repository
 
 import (
+	"context"
+
 	"gitlab.com/gitlab-org/gitaly/v15/client"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/catfile"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/housekeeping"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/localrepo"
+	"gitlab.com/gitlab-org/gitaly/v15/internal/git/quarantine"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/repository"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git2go"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/config"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/rubyserver"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/storage"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/transaction"
+	"gitlab.com/gitlab-org/gitaly/v15/internal/structerr"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/unarycache"
 	"gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb"
 )
@@ -63,3 +67,16 @@ func NewServer(
 func (s *server) localrepo(repo repository.GitRepo) *localrepo.Repo {
 	return localrepo.New(s.locator, s.gitCmdFactory, s.catfileCache, repo)
 }
+
+func (s *server) quarantinedRepo(
+	ctx context.Context, repo *gitalypb.Repository,
+) (*quarantine.Dir, *localrepo.Repo, error) {
+	quarantineDir, err := quarantine.New(ctx, repo, s.locator)
+	if err != nil {
+		return nil, nil, structerr.NewInternal("creating object quarantine: %w", err)
+	}
+
+	quarantineRepo := s.localrepo(quarantineDir.QuarantinedRepo())
+
+	return quarantineDir, quarantineRepo, nil
+}
diff --git a/internal/metadata/featureflag/ff_fetch_source_quarantine.go b/internal/metadata/featureflag/ff_fetch_source_quarantine.go
new file mode 100644
index 000000000..6dec73c89
--- /dev/null
+++ b/internal/metadata/featureflag/ff_fetch_source_quarantine.go
@@ -0,0 +1,10 @@
+package featureflag
+
+// FetchSourceBranchQuarantined enables use of quarantined repository for
+// `FetchSourceBranch`.
+var FetchSourceBranchQuarantined = NewFeatureFlag(
+	"fetch_source_branch_quarantined",
+	"v15.8.0",
+	"https://gitlab.com/gitlab-org/gitaly/-/issues/4698",
+	false,
+)