From c67d3ba5454ce7c51549876c89b98d5fb1022a08 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 ad989b528..9064b2b45 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 97484acfd..0102a0920 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)
 }