Split layerID from commitLayer

It's fairly isolated from the rest of the function,
and if split, it can have unit tests. Those tests are valuable
to ensure that layer IDs continue to behave the expected way
and maximize layer reuse (although we are not making an API commitment
to layer ID values).

Should not change behavior.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>

storage/storage_dest.go

@@ -893,12 +893,9 @@ func (s *storageImageDestination) commitLayer(index int, info addedLayerInfo, si
 			return false, fmt.Errorf("we have blob %q, but don't know its layer ID", info.digest.String())
 		}
 	}
-	// The layerID refers either to the DiffID or the digest of the TOC.
-	layerIDComponent, layerIDComponentStandalone := trusted.singleLayerIDComponent()
-	id := layerIDComponent
-	if !layerIDComponentStandalone || parentLayer != "" {
-		id = digest.Canonical.FromString(parentLayer + "+" + layerIDComponent).Encoded()
-	}
+
+	id := layerID(parentLayer, trusted)
+
 	if layer, err2 := s.imageRef.transport.store.Layer(id); layer != nil && err2 == nil {
 		// There's already a layer that should have the right contents, just reuse it.
 		s.indexToStorageID[index] = layer.ID
@@ -916,13 +913,21 @@ func (s *storageImageDestination) commitLayer(index int, info addedLayerInfo, si
 	return false, nil
 }
 
-// singleLayerIDComponent returns a single layer’s the input to computing a layer (chain) ID,
-// and an indication whether the input already has the shape of a layer ID.
-func (trusted trustedLayerIdentityData) singleLayerIDComponent() (string, bool) {
+// layerID computes a layer (“chain”) ID for (a possibly-empty parentLayer, trusted)
+func layerID(parentLayer string, trusted trustedLayerIdentityData) string {
+	var layerIDComponent string
 	if trusted.layerIdentifiedByTOC {
-		return "@TOC=" + trusted.tocDigest.Encoded(), false // "@" is not a valid start of a digest.Digest, so this is unambiguous.
+		// "@" is not a valid start of a digest.Digest.Encoded(), so this is unambiguous with the !layerIdentifiedByTOC case.
+		// But we _must_ hash this below to get a Digest.Encoded()-formatted value.
+		layerIDComponent = "@TOC=" + trusted.tocDigest.Encoded()
+	} else {
+		layerIDComponent = trusted.diffID.Encoded() // This looks like chain IDs, and it uses the traditional value.
+	}
+	id := layerIDComponent
+	if trusted.layerIdentifiedByTOC || parentLayer != "" {
+		id = digest.Canonical.FromString(parentLayer + "+" + layerIDComponent).Encoded()
 	}
-	return trusted.diffID.Encoded(), true // This looks like chain IDs, and it uses the traditional value.
+	return id
 }
 
 // createNewLayer creates a new layer newLayerID for (index, layerDigest) on top of parentLayer (which may be "").

storage/storage_dest_test.go

@@ -0,0 +1,105 @@
+package storage
+
+import (
+	"testing"
+
+	"github.com/opencontainers/go-digest"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestLayerID(t *testing.T) {
+	blobDigest, err := digest.Parse("sha256:0000000000000000000000000000000000000000000000000000000000000000")
+	require.NoError(t, err)
+
+	for _, c := range []struct {
+		parentID        string
+		identifiedByTOC bool
+		diffID          string
+		tocDigest       string
+		expected        string
+	}{
+		{
+			parentID:        "",
+			identifiedByTOC: false,
+			diffID:          "sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa",
+			tocDigest:       "",
+			expected:        "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa",
+		},
+		{
+			parentID:        "",
+			identifiedByTOC: false,
+			diffID:          "sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa",
+			expected:        "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa",
+			tocDigest:       "sha256:bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb",
+		},
+		{
+			parentID:        "",
+			identifiedByTOC: true,
+			diffID:          "",
+			tocDigest:       "sha256:bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb",
+			expected:        "07f60ddaf18a3d1fa18a71bf40f0b9889b473e26555d6fffdfbd72ba6a59469e",
+		},
+		{
+			parentID:        "",
+			identifiedByTOC: true,
+			diffID:          "sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa",
+			tocDigest:       "sha256:bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb",
+			expected:        "07f60ddaf18a3d1fa18a71bf40f0b9889b473e26555d6fffdfbd72ba6a59469e",
+		},
+		{
+			parentID:        "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa",
+			identifiedByTOC: false,
+			diffID:          "sha256:bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb",
+			tocDigest:       "",
+			expected:        "76f79efda453922cda1cecb6ec9e7cf9d86ea968c6dd199d4030dd00078a1686",
+		},
+		{
+			parentID:        "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa",
+			identifiedByTOC: false,
+			diffID:          "sha256:bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb",
+			tocDigest:       "sha256:cccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccc",
+			expected:        "76f79efda453922cda1cecb6ec9e7cf9d86ea968c6dd199d4030dd00078a1686",
+		},
+		{
+			parentID:        "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa",
+			identifiedByTOC: true,
+			diffID:          "",
+			tocDigest:       "sha256:cccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccc",
+			expected:        "468becc3d25ee862f81fd728d229a2b2487cfc9b3e6cf3a4d0af8c3fdde0e7a9",
+		},
+		{
+			parentID:        "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa",
+			identifiedByTOC: true,
+			diffID:          "sha256:bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb",
+			tocDigest:       "sha256:cccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccc",
+			expected:        "468becc3d25ee862f81fd728d229a2b2487cfc9b3e6cf3a4d0af8c3fdde0e7a9",
+		},
+	} {
+		var diffID, tocDigest digest.Digest
+		if c.diffID != "" {
+			diffID, err = digest.Parse(c.diffID)
+			require.NoError(t, err)
+		}
+		if c.tocDigest != "" {
+			tocDigest, err = digest.Parse(c.tocDigest)
+			require.NoError(t, err)
+		}
+
+		res := layerID(c.parentID, trustedLayerIdentityData{
+			layerIdentifiedByTOC: c.identifiedByTOC,
+			diffID:               diffID,
+			tocDigest:            tocDigest,
+			blobDigest:           "",
+		})
+		assert.Equal(t, c.expected, res)
+		// blobDigest does not affect the layer ID
+		res = layerID(c.parentID, trustedLayerIdentityData{
+			layerIdentifiedByTOC: c.identifiedByTOC,
+			diffID:               diffID,
+			tocDigest:            tocDigest,
+			blobDigest:           blobDigest,
+		})
+		assert.Equal(t, c.expected, res)
+	}
+}