From b8421709946b05b3bc056014df338f600f421c5a Mon Sep 17 00:00:00 2001
From: Valentin Rothberg <vrothberg@redhat.com>
Date: Tue, 17 Oct 2023 13:40:34 +0200
Subject: [PATCH] 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      | 51 ++++++++++++++++++----------------------
 libimage/history_test.go |  4 +++-
 2 files changed, 26 insertions(+), 29 deletions(-)

diff --git a/libimage/history.go b/libimage/history.go
index ad989b528a..9064b2b455 100644
--- a/libimage/history.go
+++ b/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)
diff --git a/libimage/history_test.go b/libimage/history_test.go
index 97484acfde..0102a09207 100644
--- a/libimage/history_test.go
+++ b/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)
 }