libimage: fix computing history

Computing the history did not walk the layers correctly.  Fix that and
try to improve the code to make it easier to read and maintain for
future pairs of eyes.

A regression will also be added to the Podman PR vendoring this change.

Fixes: containers/podman/issues/20375
Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>

libimage/history.go

@@ -2,9 +2,8 @@ package libimage
 
 import (
 	"context"
+	"fmt"
 	"time"
-
-	"github.com/containers/storage"
 )
 
 // ImageHistory contains the history information of an image.
@@ -29,17 +28,19 @@ func (i *Image) History(ctx context.Context) ([]ImageHistory, error) {
 		return nil, err
 	}
 
-	var allHistory []ImageHistory
-	var layer *storage.Layer
+	var nextNode *layerNode
 	if i.TopLayer() != "" {
-		layer, err = i.runtime.store.Layer(i.TopLayer())
+		layer, err := i.runtime.store.Layer(i.TopLayer())
 		if err != nil {
 			return nil, err
 		}
+		nextNode = layerTree.node(layer.ID)
 	}
 
 	// Iterate in reverse order over the history entries, and lookup the
-	// corresponding image ID, size and get the next later if needed.
+	// corresponding image ID, size.  If it's a non-empty history entry,
+	// pick the next "storage" layer by walking the layer tree.
+	var allHistory []ImageHistory
 	numHistories := len(ociImage.History) - 1
 	usedIDs := make(map[string]bool) // prevents assigning images IDs more than once
 	for x := numHistories; x >= 0; x-- {
@@ -50,31 +51,25 @@ func (i *Image) History(ctx context.Context) ([]ImageHistory, error) {
 			Comment:   ociImage.History[x].Comment,
 		}
 
-		if layer != nil {
-			if !ociImage.History[x].EmptyLayer {
-				history.Size = layer.UncompressedSize
+		if len(nextNode.images) > 0 {
+			id := nextNode.images[0].ID() // always use the first one
+			if _, used := usedIDs[id]; !used {
+				history.ID = id
+				usedIDs[id] = true
 			}
-			// Query the layer tree if it's the top layer of an
-			// image.
-			node := layerTree.node(layer.ID)
-			if len(node.images) > 0 {
-				id := node.images[0].ID() // always use the first one
-				if _, used := usedIDs[id]; !used {
-					history.ID = id
-					usedIDs[id] = true
-				}
-				for i := range node.images {
-					history.Tags = append(history.Tags, node.images[i].Names()...)
-				}
+			for i := range nextNode.images {
+				history.Tags = append(history.Tags, nextNode.images[i].Names()...)
 			}
-			if layer.Parent == "" {
-				layer = nil
-			} else if !ociImage.History[x].EmptyLayer {
-				layer, err = i.runtime.store.Layer(layer.Parent)
-				if err != nil {
-					return nil, err
-				}
+		}
+
+		if !ociImage.History[x].EmptyLayer {
+			if nextNode == nil { // If no layer's left, something's wrong.
+				return nil, fmt.Errorf("no layer left for non-empty history entry: %v", history)
 			}
+
+			history.Size = nextNode.layer.UncompressedSize
+
+			nextNode = nextNode.parent
 		}
 
 		allHistory = append(allHistory, history)

libimage/history_test.go

@@ -24,5 +24,7 @@ func TestHistory(t *testing.T) {
 	require.Len(t, history, 2)
 
 	require.Equal(t, []string{name}, history[0].Tags)
-	require.Len(t, history[1].Tags, 0)
+	require.Equal(t, history[1].Tags, []string{name})
+	require.NotEqual(t, history[0].Size, 0)
+	require.NotEqual(t, history[1].Size, 0)
 }