squash! copy: add option to strip sparse manifest lists

copy: implement code review feedback for sparse manifest stripping

Address feedback from mtrmac's review on PR podman-container-tools#655:

1. Simplify deletion logic by combining EditInstances calls
   - Moved deletion tracking into prepareInstanceCopies function
   - Combined update and delete operations into single EditInstances call
   - Removed separate digest-based tracking and redundant loops

2. Use slices.Delete for manifest entry removal
   - Replaced manual slice manipulation with slices.Delete in both
     docker_schema2_list.go and oci_index.go for cleaner code

3. Integrate deletion tests into existing test methods
   - Removed standalone TestListDeleteInstances function
   - Added deletion test cases to TestSchema2ListEditInstances and
     TestOCI1EditInstances for better test organization

4. Move deletion logic into prepareInstanceCopies (most critical fix)
   - Modified prepareInstanceCopies to return ([]instanceCopy, []int, error)
   - Track indices to delete when SparseManifestListAction == StripSparseManifestList
   - Properly handles duplicate digests by using instance indices instead
     of digest-based lookup, fixing edge case where same digest appears
     multiple times in manifest list

5. Use slices.Backward for reverse iteration
   - Simplified deletion loop using slices.Backward iterator
   - Maintains high-to-low deletion order to avoid index shifting

6. Move SparseManifestListAction field for better organization
   - Relocated field from line 150 to line 116 in copy.go
   - Now positioned near related ImageListSelection and Instances fields
   - Improves logical grouping of manifest list options

All tests pass. The implementation now correctly handles:
- Duplicate digests in manifest lists
- Single EditInstances call for efficiency
- Cleaner separation of concerns
- Better code organization

Fixes feedback on podman-container-tools#655

Signed-off-by: Alex Guidi <aguidi@redhat.com>

Author: aguidirh

image/copy/copy.go

@@ -111,6 +111,9 @@ type Options struct {
 	ForceManifestMIMEType string
 	ImageListSelection    ImageListSelection // set to either CopySystemImage (the default), CopyAllImages, or CopySpecificImages to control which instances we copy when the source reference is a list; ignored if the source reference is not a list
 	Instances             []digest.Digest    // if ImageListSelection is CopySpecificImages, copy only these instances and the list itself
+	// When only a subset of images of a list is copied, this action indicates if the manifest should be kept or stripped.
+	// See CopySpecificImages.
+	SparseManifestListAction SparseManifestListAction
 	// Give priority to pulling gzip images if multiple images are present when configured to OptionalBoolTrue,
 	// prefers the best compression if this is configured as OptionalBoolFalse. Choose automatically (and the choice may change over time)
 	// if this is set to OptionalBoolUndefined (which is the default behavior, and recommended for most callers).
@@ -145,10 +148,6 @@ type Options struct {
 	// to not indicate "nondistributable".
 	DownloadForeignLayers bool
 
-	// When only a subset of images of a list is copied, this action indicates if the manifest should be kept or stripped.
-	// See CopySpecificImages.
-	SparseManifestListAction SparseManifestListAction
-
 	// Contains slice of OptionCompressionVariant, where copy will ensure that for each platform
 	// in the manifest list, a variant with the requested compression will exist.
 	// Invalid when copying a non-multi-architecture image. That will probably

image/copy/multiple.go

@@ -100,37 +100,43 @@ func validateCompressionVariantExists(input []OptionCompressionVariant) error {
 }
 
 // prepareInstanceCopies prepares a list of instances which needs to copied to the manifest list.
-func prepareInstanceCopies(list internalManifest.List, instanceDigests []digest.Digest, options *Options) ([]instanceCopy, error) {
+// It returns the list of instances to copy and a list of instance indices to delete (if stripping sparse manifests).
+func prepareInstanceCopies(list internalManifest.List, instanceDigests []digest.Digest, options *Options) ([]instanceCopy, []int, error) {
 	res := []instanceCopy{}
+	indicesToDelete := []int{}
 	if options.ImageListSelection == CopySpecificImages && len(options.EnsureCompressionVariantsExist) > 0 {
 		// List can already contain compressed instance for a compression selected in `EnsureCompressionVariantsExist`
-		// It’s unclear what it means when `CopySpecificImages` includes an instance in options.Instances,
+		// It's unclear what it means when `CopySpecificImages` includes an instance in options.Instances,
 		// EnsureCompressionVariantsExist asks for an instance with some compression,
 		// an instance with that compression already exists, but is not included in options.Instances.
 		// We might define the semantics and implement this in the future.
-		return res, fmt.Errorf("EnsureCompressionVariantsExist is not implemented for CopySpecificImages")
+		return res, indicesToDelete, fmt.Errorf("EnsureCompressionVariantsExist is not implemented for CopySpecificImages")
 	}
 	err := validateCompressionVariantExists(options.EnsureCompressionVariantsExist)
 	if err != nil {
-		return res, err
+		return res, indicesToDelete, err
 	}
 	compressionsByPlatform, err := platformCompressionMap(list, instanceDigests)
 	if err != nil {
-		return nil, err
+		return nil, indicesToDelete, err
 	}
 	for i, instanceDigest := range instanceDigests {
 		if options.ImageListSelection == CopySpecificImages &&
 			!slices.Contains(options.Instances, instanceDigest) {
 			logrus.Debugf("Skipping instance %s (%d/%d)", instanceDigest, i+1, len(instanceDigests))
+			// Track this index for deletion if we're stripping sparse manifests
+			if options.SparseManifestListAction == StripSparseManifestList {
+				indicesToDelete = append(indicesToDelete, i)
+			}
 			continue
 		}
 		instanceDetails, err := list.Instance(instanceDigest)
 		if err != nil {
-			return res, fmt.Errorf("getting details for instance %s: %w", instanceDigest, err)
+			return res, indicesToDelete, fmt.Errorf("getting details for instance %s: %w", instanceDigest, err)
 		}
 		forceCompressionFormat, err := shouldRequireCompressionFormatMatch(options)
 		if err != nil {
-			return nil, err
+			return nil, indicesToDelete, err
 		}
 		res = append(res, instanceCopy{
 			op:                         instanceCopyCopy,
@@ -149,12 +155,12 @@ func prepareInstanceCopies(list internalManifest.List, instanceDigests []digest.
 					clonePlatform:           instanceDetails.ReadOnly.Platform,
 					cloneAnnotations:        maps.Clone(instanceDetails.ReadOnly.Annotations),
 				})
-				// add current compression to the list so that we don’t create duplicate clones
+				// add current compression to the list so that we don't create duplicate clones
 				compressionList.Add(compressionVariant.Algorithm.Name())
 			}
 		}
 	}
-	return res, nil
+	return res, indicesToDelete, nil
 }
 
 // copyMultipleImages copies some or all of an image list's instances, using
@@ -230,7 +236,7 @@ func (c *copier) copyMultipleImages(ctx context.Context) (copiedManifest []byte,
 	// Copy each image, or just the ones we want to copy, in turn.
 	instanceDigests := updatedList.Instances()
 	instanceEdits := []internalManifest.ListEdit{}
-	instanceCopyList, err := prepareInstanceCopies(updatedList, instanceDigests, c.options)
+	instanceCopyList, indicesToDelete, err := prepareInstanceCopies(updatedList, instanceDigests, c.options)
 	if err != nil {
 		return nil, fmt.Errorf("preparing instances for copy: %w", err)
 	}
@@ -284,44 +290,18 @@ func (c *copier) copyMultipleImages(ctx context.Context) (copiedManifest []byte,
 		}
 	}
 
-	// Now reset the digest/size/types of the manifests in the list to account for any conversions that we made.
-	if err = updatedList.EditInstances(instanceEdits, cannotModifyManifestListReason != ""); err != nil {
-		return nil, fmt.Errorf("updating manifest list: %w", err)
+	// Add delete operations for skipped instances if stripping sparse manifests
+	// Delete from highest to lowest index to avoid shifting
+	for i := range slices.Backward(indicesToDelete) {
+		instanceEdits = append(instanceEdits, internalManifest.ListEdit{
+			ListOperation: internalManifest.ListOpDelete,
+			DeleteIndex:   indicesToDelete[i],
+		})
 	}
 
-	// Remove skipped instances from the manifest list if StripSparseManifestList is enabled
-	if c.options.ImageListSelection == CopySpecificImages && c.options.SparseManifestListAction == StripSparseManifestList {
-		// Build a set of digests that were copied
-		copiedDigests := set.New[digest.Digest]()
-		for _, instance := range instanceCopyList {
-			copiedDigests.Add(instance.sourceDigest)
-		}
-
-		// Find which indices were skipped
-		var indicesToDelete []int
-		for i, instanceDigest := range instanceDigests {
-			if !copiedDigests.Contains(instanceDigest) {
-				indicesToDelete = append(indicesToDelete, i)
-			}
-		}
-
-		// Build delete operations for skipped instances
-		// Delete from highest to lowest index to avoid shifting
-		var deleteEdits []internalManifest.ListEdit
-		for i := len(indicesToDelete) - 1; i >= 0; i-- {
-			deleteEdits = append(deleteEdits, internalManifest.ListEdit{
-				ListOperation: internalManifest.ListOpDelete,
-				DeleteIndex:   indicesToDelete[i],
-			})
-		}
-
-		// Remove skipped instances from the manifest list using EditInstances
-		if len(deleteEdits) > 0 {
-			logrus.Debugf("Removing %d instances from manifest list", len(deleteEdits))
-			if err := updatedList.EditInstances(deleteEdits, false); err != nil {
-				return nil, fmt.Errorf("stripping sparse manifest list: %w", err)
-			}
-		}
+	// Now reset the digest/size/types of the manifests in the list and remove deleted instances.
+	if err = updatedList.EditInstances(instanceEdits, cannotModifyManifestListReason != ""); err != nil {
+		return nil, fmt.Errorf("updating manifest list: %w", err)
 	}
 
 	// Iterate through supported list types, preferred format first.

image/copy/multiple_test.go

@@ -26,8 +26,9 @@ func TestPrepareCopyInstancesforInstanceCopyCopy(t *testing.T) {
 		digest.Digest("sha256:cccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccc"),
 	}
 
-	instancesToCopy, err := prepareInstanceCopies(list, sourceInstances, &Options{})
+	instancesToCopy, indicesToDelete, err := prepareInstanceCopies(list, sourceInstances, &Options{})
 	require.NoError(t, err)
+	require.Empty(t, indicesToDelete)
 	compare := []instanceCopy{}
 
 	for _, instance := range sourceInstances {
@@ -39,15 +40,30 @@ func TestPrepareCopyInstancesforInstanceCopyCopy(t *testing.T) {
 	assert.Equal(t, instancesToCopy, compare)
 
 	// Test CopySpecificImages where selected instance is sourceInstances[1]
-	instancesToCopy, err = prepareInstanceCopies(list, sourceInstances, &Options{Instances: []digest.Digest{sourceInstances[1]}, ImageListSelection: CopySpecificImages})
+	instancesToCopy, indicesToDelete, err = prepareInstanceCopies(list, sourceInstances, &Options{Instances: []digest.Digest{sourceInstances[1]}, ImageListSelection: CopySpecificImages})
 	require.NoError(t, err)
+	require.Empty(t, indicesToDelete) // No stripping requested
 	compare = []instanceCopy{{
 		op:           instanceCopyCopy,
 		sourceDigest: sourceInstances[1],
 	}}
 	assert.Equal(t, instancesToCopy, compare)
 
-	_, err = prepareInstanceCopies(list, sourceInstances, &Options{Instances: []digest.Digest{sourceInstances[1]}, ImageListSelection: CopySpecificImages, ForceCompressionFormat: true})
+	// Test CopySpecificImages with StripSparseManifestList where selected instance is sourceInstances[1]
+	instancesToCopy, indicesToDelete, err = prepareInstanceCopies(list, sourceInstances, &Options{
+		Instances:                []digest.Digest{sourceInstances[1]},
+		ImageListSelection:       CopySpecificImages,
+		SparseManifestListAction: StripSparseManifestList,
+	})
+	require.NoError(t, err)
+	assert.Equal(t, []int{0, 2}, indicesToDelete) // Should delete indices 0 and 2
+	compare = []instanceCopy{{
+		op:           instanceCopyCopy,
+		sourceDigest: sourceInstances[1],
+	}}
+	assert.Equal(t, instancesToCopy, compare)
+
+	_, _, err = prepareInstanceCopies(list, sourceInstances, &Options{Instances: []digest.Digest{sourceInstances[1]}, ImageListSelection: CopySpecificImages, ForceCompressionFormat: true})
 	require.EqualError(t, err, "cannot use ForceCompressionFormat with undefined default compression format")
 }
 
@@ -68,16 +84,17 @@ func TestPrepareCopyInstancesforInstanceCopyClone(t *testing.T) {
 	}
 
 	// CopySpecificImage must fail with error
-	_, err = prepareInstanceCopies(list, sourceInstances, &Options{
+	_, _, err = prepareInstanceCopies(list, sourceInstances, &Options{
 		EnsureCompressionVariantsExist: ensureCompressionVariantsExist,
 		Instances:                      []digest.Digest{sourceInstances[1]},
 		ImageListSelection:             CopySpecificImages,
 	})
 	require.EqualError(t, err, "EnsureCompressionVariantsExist is not implemented for CopySpecificImages")
 
 	// Test copying all images with replication
-	instancesToCopy, err := prepareInstanceCopies(list, sourceInstances, &Options{EnsureCompressionVariantsExist: ensureCompressionVariantsExist})
+	instancesToCopy, indicesToDelete, err := prepareInstanceCopies(list, sourceInstances, &Options{EnsureCompressionVariantsExist: ensureCompressionVariantsExist})
 	require.NoError(t, err)
+	require.Empty(t, indicesToDelete)
 
 	// Following test ensures
 	// * Still copy gzip variants if they exist in the original
@@ -104,8 +121,9 @@ func TestPrepareCopyInstancesforInstanceCopyClone(t *testing.T) {
 	// Test option with multiple copy request for same compression format.
 	// The above expectation should stay the same, if ensureCompressionVariantsExist requests zstd twice.
 	ensureCompressionVariantsExist = []OptionCompressionVariant{{Algorithm: compression.Zstd}, {Algorithm: compression.Zstd}}
-	instancesToCopy, err = prepareInstanceCopies(list, sourceInstances, &Options{EnsureCompressionVariantsExist: ensureCompressionVariantsExist})
+	instancesToCopy, indicesToDelete, err = prepareInstanceCopies(list, sourceInstances, &Options{EnsureCompressionVariantsExist: ensureCompressionVariantsExist})
 	require.NoError(t, err)
+	require.Empty(t, indicesToDelete)
 	expectedResponse = []simplerInstanceCopy{}
 	for _, instance := range sourceInstances {
 		expectedResponse = append(expectedResponse, simplerInstanceCopy{
@@ -126,8 +144,9 @@ func TestPrepareCopyInstancesforInstanceCopyClone(t *testing.T) {
 		digest.Digest("sha256:cccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccc"),
 		digest.Digest("sha256:cccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccc"),
 	}
-	instancesToCopy, err = prepareInstanceCopies(list, sourceInstances, &Options{EnsureCompressionVariantsExist: ensureCompressionVariantsExist})
+	instancesToCopy, indicesToDelete, err = prepareInstanceCopies(list, sourceInstances, &Options{EnsureCompressionVariantsExist: ensureCompressionVariantsExist})
 	require.NoError(t, err)
+	require.Empty(t, indicesToDelete)
 	// two copies but clone should happen only once
 	numberOfCopyClone := 0
 	for _, instance := range instancesToCopy {

image/internal/manifest/docker_schema2_list.go

@@ -136,8 +136,7 @@ func (list *Schema2ListPublic) editInstances(editInstances []ListEdit, cannotMod
 			if editInstance.DeleteIndex < 0 || editInstance.DeleteIndex >= len(list.Manifests) {
 				return fmt.Errorf("Schema2List.EditInstances: invalid delete index %d (list has %d instances)", editInstance.DeleteIndex, len(list.Manifests))
 			}
-			// Remove the element by appending slices before and after the target index
-			list.Manifests = append(list.Manifests[:editInstance.DeleteIndex], list.Manifests[editInstance.DeleteIndex+1:]...)
+			list.Manifests = slices.Delete(list.Manifests, editInstance.DeleteIndex, editInstance.DeleteIndex+1)
 		default:
 			return fmt.Errorf("internal error: invalid operation: %d", editInstance.ListOperation)
 		}

image/internal/manifest/docker_schema2_list_test.go

@@ -90,6 +90,29 @@ func TestSchema2ListEditInstances(t *testing.T) {
 		digest.Digest("sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"),
 		digest.Digest("sha256:cccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccc"),
 	), list.Instances())
+
+	// Create a fresh list for delete tests
+	list, err = ListFromBlob(validManifest, GuessMIMEType(validManifest))
+	require.NoError(t, err)
+	originalInstances := list.Instances()
+	require.GreaterOrEqual(t, len(originalInstances), 1)
+
+	// Test successful deletion of first instance
+	err = list.EditInstances([]ListEdit{{
+		ListOperation: ListOpDelete,
+		DeleteIndex:   0,
+	}}, false)
+	require.NoError(t, err)
+	assert.Equal(t, len(originalInstances)-1, len(list.Instances()))
+	assert.Equal(t, originalInstances[1:], list.Instances())
+
+	// Test error on invalid index
+	err = list.EditInstances([]ListEdit{{
+		ListOperation: ListOpDelete,
+		DeleteIndex:   999,
+	}}, false)
+	assert.Error(t, err)
+	assert.Contains(t, err.Error(), "invalid delete index")
 }
 
 func TestSchema2ListFromManifest(t *testing.T) {

image/internal/manifest/list_test.go

@@ -159,40 +159,3 @@ func TestChooseInstance(t *testing.T) {
 		}
 	}
 }
-
-// TestListDeleteInstances tests the ListOpDelete functionality for both OCI and Docker manifest formats.
-func TestListDeleteInstances(t *testing.T) {
-	manifestFiles := []string{
-		"ociv1.image.index.json",
-		"v2list.manifest.json",
-	}
-
-	for _, manifestFile := range manifestFiles {
-		t.Run(manifestFile, func(t *testing.T) {
-			validManifest, err := os.ReadFile(filepath.Join("testdata", manifestFile))
-			require.NoError(t, err)
-			list, err := ListFromBlob(validManifest, GuessMIMEType(validManifest))
-			require.NoError(t, err)
-
-			originalInstances := list.Instances()
-			require.GreaterOrEqual(t, len(originalInstances), 1)
-
-			// Test successful deletion
-			err = list.EditInstances([]ListEdit{{
-				ListOperation: ListOpDelete,
-				DeleteIndex:   0,
-			}}, false)
-			require.NoError(t, err)
-			assert.Equal(t, len(originalInstances)-1, len(list.Instances()))
-			assert.Equal(t, originalInstances[1:], list.Instances())
-
-			// Test error on invalid index
-			err = list.EditInstances([]ListEdit{{
-				ListOperation: ListOpDelete,
-				DeleteIndex:   999,
-			}}, false)
-			assert.Error(t, err)
-			assert.Contains(t, err.Error(), "invalid delete index")
-		})
-	}
-}

image/internal/manifest/oci_index.go

@@ -180,8 +180,7 @@ func (index *OCI1IndexPublic) editInstances(editInstances []ListEdit, cannotModi
 			if editInstance.DeleteIndex < 0 || editInstance.DeleteIndex >= len(index.Manifests) {
 				return fmt.Errorf("OCI1Index.EditInstances: invalid delete index %d (list has %d instances)", editInstance.DeleteIndex, len(index.Manifests))
 			}
-			// Remove the element by appending slices before and after the target index
-			index.Manifests = append(index.Manifests[:editInstance.DeleteIndex], index.Manifests[editInstance.DeleteIndex+1:]...)
+			index.Manifests = slices.Delete(index.Manifests, editInstance.DeleteIndex, editInstance.DeleteIndex+1)
 		default:
 			return fmt.Errorf("internal error: invalid operation: %d", editInstance.ListOperation)
 		}

image/internal/manifest/oci_index_test.go

@@ -228,6 +228,29 @@ func TestOCI1EditInstances(t *testing.T) {
 			assert.Equal(t, originalInstance, instance)
 		}
 	}
+
+	// Create a fresh list for delete tests
+	list, err = ListFromBlob(validManifest, GuessMIMEType(validManifest))
+	require.NoError(t, err)
+	originalInstances := list.Instances()
+	require.GreaterOrEqual(t, len(originalInstances), 1)
+
+	// Test successful deletion of first instance
+	err = list.EditInstances([]ListEdit{{
+		ListOperation: ListOpDelete,
+		DeleteIndex:   0,
+	}}, false)
+	require.NoError(t, err)
+	assert.Equal(t, len(originalInstances)-1, len(list.Instances()))
+	assert.Equal(t, originalInstances[1:], list.Instances())
+
+	// Test error on invalid index
+	err = list.EditInstances([]ListEdit{{
+		ListOperation: ListOpDelete,
+		DeleteIndex:   999,
+	}}, false)
+	assert.Error(t, err)
+	assert.Contains(t, err.Error(), "invalid delete index")
 }
 
 func TestOCI1IndexChooseInstanceByCompression(t *testing.T) {