Merge branch 'wc/filter-merge-commits' into 'master'

cleanup: Check parents of internal refs

Closes #5712

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

Merged-by: Toon Claes <toon@gitlab.com>
Reviewed-by: Patrick Steinhardt <psteinhardt@gitlab.com>
Reviewed-by: Will Chandler <wchandler@gitlab.com>
Co-authored-by: Will Chandler <wchandler@gitlab.com>

2 files changed, 32 insertions, 10 deletions

diff --git a/internal/gitaly/service/cleanup/apply_bfg_object_map_stream_test.go b/internal/gitaly/service/cleanup/apply_bfg_object_map_stream_test.go
index 2c37cadfd..e85398082 100644
--- a/internal/gitaly/service/cleanup/apply_bfg_object_map_stream_test.go
+++ b/internal/gitaly/service/cleanup/apply_bfg_object_map_stream_test.go
@@ -35,6 +35,16 @@ func TestApplyBfgObjectMapStreamSuccess(t *testing.T) {
 		Message: "annotated tag",
 	})
 
+	otherBlobID := gittest.WriteBlob(t, cfg, repoPath, []byte("more contents"))
+	otherCommitID := gittest.WriteCommit(t, cfg, repoPath, gittest.WithBranch("feature"), gittest.WithTreeEntries(
+		gittest.TreeEntry{Path: "other", Mode: "100644", OID: otherBlobID},
+	))
+
+	// Create a commit that has a parent commit that will be removed,
+	// but is not itself on the list.
+	mergeID := gittest.WriteCommit(t, cfg, repoPath, gittest.WithBranch("feature"), gittest.WithParents(commitID, otherCommitID))
+	gittest.WriteRef(t, cfg, repoPath, "refs/merge-requests/merge", mergeID)
+
 	// Create some refs pointing to HEAD
 	for _, ref := range []git.ReferenceName{
 		"refs/environments/1", "refs/keep-around/1", "refs/merge-requests/1", "refs/pipelines/1",
@@ -85,6 +95,7 @@ func TestApplyBfgObjectMapStreamSuccess(t *testing.T) {
 	assert.NotContains(t, refNames, "refs/environments/1")
 	assert.NotContains(t, refNames, "refs/keep-around/1")
 	assert.NotContains(t, refNames, "refs/merge-requests/1")
+	assert.NotContains(t, refNames, "refs/merge-requests/merge")
 	assert.NotContains(t, refNames, "refs/pipelines/1")
 	assert.Contains(t, refNames, "refs/heads/_keep")
 	assert.Contains(t, refNames, "refs/tags/_keep")
diff --git a/internal/gitaly/service/cleanup/cleaner.go b/internal/gitaly/service/cleanup/cleaner.go
index 574933f7f..360510f2e 100644
--- a/internal/gitaly/service/cleanup/cleaner.go
+++ b/internal/gitaly/service/cleanup/cleaner.go
@@ -27,8 +27,9 @@ type cleaner struct {
 	logger  log.Logger
 
 	// Map of SHA -> reference names
-	table map[string][]git.ReferenceName
-	repo  git.RepositoryExecutor
+	table       map[string][]git.ReferenceName
+	removedRefs map[git.ReferenceName]struct{}
+	repo        git.RepositoryExecutor
 }
 
 // errInvalidObjectMap is returned with descriptive text if the supplied object
@@ -43,7 +44,8 @@ func newCleaner(ctx context.Context, logger log.Logger, repo git.RepositoryExecu
 		return nil, err
 	}
 
-	return &cleaner{ctx: ctx, logger: logger, table: table, repo: repo, forEach: forEach}, nil
+	removedRefs := make(map[git.ReferenceName]struct{})
+	return &cleaner{ctx: ctx, logger: logger, table: table, removedRefs: removedRefs, repo: repo, forEach: forEach}, nil
 }
 
 // applyObjectMap processes an object map file generated by git filter-repo, or
@@ -127,6 +129,12 @@ func (c *cleaner) processEntry(ctx context.Context, updater *updateref.Updater,
 
 	// Remove the internal refs pointing to oldSHA
 	for _, ref := range refs {
+		// Delete each ref only once.
+		if _, ok := c.removedRefs[ref]; ok {
+			continue
+		}
+		c.removedRefs[ref] = struct{}{}
+
 		if err := updater.Delete(ref); err != nil {
 			return err
 		}
@@ -155,7 +163,7 @@ func buildLookupTable(ctx context.Context, logger log.Logger, repo git.Repositor
 
 	cmd, err := repo.Exec(ctx, git.Command{
 		Name:  "for-each-ref",
-		Flags: []git.Option{git.ValueFlag{Name: "--format", Value: "%(objectname) %(refname)"}},
+		Flags: []git.Option{git.ValueFlag{Name: "--format", Value: "%(objectname) %(parent) %(refname)"}},
 		Args:  internalRefPrefixes,
 	}, git.WithSetupStdout())
 	if err != nil {
@@ -168,17 +176,20 @@ func buildLookupTable(ctx context.Context, logger log.Logger, repo git.Repositor
 	for scanner.Scan() {
 		line := scanner.Text()
 
-		objectName, refName, ok := strings.Cut(line, " ")
-		if !ok {
+		output := strings.Fields(line)
+		if len(output) < 2 {
 			logger.WithFields(log.Fields{"line": line}).WarnContext(ctx, "failed to parse git refs")
 			return nil, fmt.Errorf("failed to parse git refs")
 		}
 
-		if err := objectHash.ValidateHex(objectName); err != nil {
-			return nil, fmt.Errorf("failed to parse object name: %w", err)
-		}
+		refName := output[len(output)-1]
+		for _, objectName := range output[:len(output)-1] {
+			if err := objectHash.ValidateHex(objectName); err != nil {
+				return nil, fmt.Errorf("failed to parse object name: %w", err)
+			}
 
-		out[objectName] = append(out[objectName], git.ReferenceName(refName))
+			out[objectName] = append(out[objectName], git.ReferenceName(refName))
+		}
 	}
 
 	if err := cmd.Wait(); err != nil {