From 5ea8be708b26e89c8d170e90eca5b7c9747817b7 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= <mitr@redhat.com>
Date: Wed, 27 Nov 2024 23:02:58 +0100
Subject: [PATCH] Improve comments throughout commitLayer
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Should not change behavior.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
---
 storage/storage_dest.go | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/storage/storage_dest.go b/storage/storage_dest.go
index 6b082feca..25701834f 100644
--- a/storage/storage_dest.go
+++ b/storage/storage_dest.go
@@ -884,11 +884,12 @@ func (s *storageImageDestination) commitLayer(index int, info addedLayerInfo, si
 		return false, nil
 	}
 
-	// Start with an empty string or the previous layer ID.  Note that
-	// `s.indexToStorageID` can only be accessed by *one* goroutine at any
-	// given time. Hence, we don't need to lock accesses.
+	// Start with the parent layer, if any.
 	var parentLayer string
 	if index != 0 {
+		// s.indexToStorageID can only be written by this function, and the our caller
+		// is responsible for ensuring it can be only be called by *one* goroutine at any
+		// given time. Hence, we don't need to lock accesses.
 		prev, ok := s.indexToStorageID[index-1]
 		if !ok {
 			return false, fmt.Errorf("Internal error: commitLayer called with previous layer %d not committed yet", index-1)
@@ -896,19 +897,18 @@ func (s *storageImageDestination) commitLayer(index int, info addedLayerInfo, si
 		parentLayer = prev
 	}
 
-	// Carry over the previous ID for empty non-base layers.
+	// Carry over the parent ID for empty non-base layers.
 	if info.emptyLayer {
 		s.indexToStorageID[index] = parentLayer
 		return false, nil
 	}
 
-	// Check if there's already a layer with the ID that we'd give to the result of applying
-	// this layer blob to its parent, if it has one, or the blob's hex value otherwise.
+	// Collect trusted parameters of the layer.
 	s.lock.Lock()
 	trusted, ok := s.trustedLayerIdentityDataLocked(index, info.digest)
 	s.lock.Unlock()
 	if !ok {
-		// Check if it's elsewhere and the caller just forgot to pass it to us in a PutBlob() / TryReusingBlob() / …
+		// Check if the layer exists already and the caller just (incorrectly) forgot to pass it to us in a PutBlob() / TryReusingBlob() / …
 		//
 		// Use none.NoCache to avoid a repeated DiffID lookup in the BlobInfoCache: a caller
 		// that relies on using a blob digest that has never been seen by the store had better call
@@ -939,10 +939,13 @@ 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.
+
+	// Build the single layer’s ID component, and the final “chain” ID for the layer on top of its parent.
 	var layerIDComponent string
 	if trusted.layerIdentifiedByTOC {
-		layerIDComponent = "@TOC=" + trusted.tocDigest.Encoded() // "@" 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.
 	}
@@ -950,6 +953,8 @@ func (s *storageImageDestination) commitLayer(index int, info addedLayerInfo, si
 	if trusted.layerIdentifiedByTOC || parentLayer != "" {
 		id = digest.Canonical.FromString(parentLayer + "+" + layerIDComponent).Encoded()
 	}
+
+	// Check if there's already a layer with the intended ID.
 	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