From 0beb885cbf699ada428254b61ea2485b8e4d9c7e Mon Sep 17 00:00:00 2001
From: Aleksa Sarai <cyphar@cyphar.com>
Date: Tue, 9 Sep 2025 17:31:57 +1000
Subject: [PATCH] compare: properly compare tar_time and time

In certain circumstances, if a manifest with "time" keywords was
compared to another manifest with "tar_time" keywords, differences were
not actually reported despite there being logic for it.

Unfortunately, even though commit 26ff922da64a ("compare: implement
mtree.DirectoryHierarchy comparisons") explicitly included logic to
handle the necessary "time" -> "tar_time" conversions, the same commit
also made it so that Compare() could be instructed to only consider a
subset of keywords and failed to take into account said "time" ->
"tar_time" remapping.

Most users have likely not noticed this because gomtree will re-use the
keywords from a manifest when doing a straightforward "gomtree validate"
invocation, but if you explicitly requested "time" when validating a
"tar_time" manifest (and *not* the other way around) then any time
changes would be undetected.

Fixes: 26ff922da64a ("compare: implement mtree.DirectoryHierarchy comparisons")
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
 compare.go       | 18 ++++++++------
 test/cli/0012.sh | 64 ++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 74 insertions(+), 8 deletions(-)
 create mode 100644 test/cli/0012.sh

diff --git a/compare.go b/compare.go
index 29da1ab..b73f174 100644
--- a/compare.go
+++ b/compare.go
@@ -3,6 +3,7 @@ package mtree
 import (
 	"encoding/json"
 	"fmt"
+	"slices"
 	"strconv"
 )
 
@@ -405,15 +406,16 @@ func compare(oldDh, newDh *DirectoryHierarchy, keys []Keyword, same bool) ([]Ino
 				return nil, fmt.Errorf("comparison failed %s: %s", path, err)
 			}
 
-			// Now remove "changed" entries that don't match the keys.
+			// Ignore changes to keys not in the requested set.
 			if keys != nil {
-				var filterChanged []KeyDelta
-				for _, keyDiff := range changed {
-					if InKeywordSlice(keyDiff.name.Prefix(), keys) {
-						filterChanged = append(filterChanged, keyDiff)
-					}
-				}
-				changed = filterChanged
+				changed = slices.DeleteFunc(changed, func(delta KeyDelta) bool {
+					name := delta.name.Prefix()
+					return !InKeywordSlice(name, keys) &&
+						// We remap time to tar_time in compareEntry, so we
+						// need to treat them equivalently here.
+						!(name == "time" && InKeywordSlice("tar_time", keys)) &&
+						!(name == "tar_time" && InKeywordSlice("time", keys))
+				})
 			}
 
 			// Check if there were any actual changes.
diff --git a/test/cli/0012.sh b/test/cli/0012.sh
new file mode 100644
index 0000000..cd4960b
--- /dev/null
+++ b/test/cli/0012.sh
@@ -0,0 +1,64 @@
+#!/bin/bash
+set -ex
+
+name=$(basename $0)
+root="$(dirname $(dirname $(dirname $0)))"
+gomtree=$(go run ${root}/test/realpath/main.go ${root}/gomtree)
+t=$(mktemp -d /tmp/go-mtree.XXXXXX)
+
+echo "[${name}] Running in ${t}"
+
+## Make sure that comparing manifests with tar_time and time correctly detect
+## errors (while also *truncating* timestamps for tar_time comparisons).
+
+pushd ${root}
+mkdir -p ${t}/root
+
+date="2025-09-05T13:05:10.12345" # POSIX format for "touch -d".
+
+touch -d "$date" ${t}/root/foo
+
+keywords=type,uid,gid,nlink,link,mode,flags,xattr,size,sha256
+
+# Generate regular manifests with time and tar_time.
+${gomtree} -c -k ${keywords},time -p ${t}/root -f ${t}/time.mtree
+${gomtree} -c -k ${keywords},tar_time -p ${t}/root -f ${t}/tartime.mtree
+
+# Validation with both manifests should still succeed.
+${gomtree} validate -p ${t}/root -f ${t}/time.mtree
+${gomtree} validate -p ${t}/root -f ${t}/tartime.mtree
+# Manifest comparison should also succeed.
+${gomtree} validate -f ${t}/tartime.mtree -f ${t}/time.mtree
+${gomtree} validate -f ${t}/time.mtree -f ${t}/tartime.mtree
+# Forcefully requesting a different time type should also succeed.
+${gomtree} validate -k ${keywords},tar_time -p ${t}/root -f ${t}/time.mtree
+${gomtree} validate -k ${keywords},time -p ${t}/root -f ${t}/tartime.mtree
+
+# Change the timestamp.
+touch -d "1997-03-25T13:40:00.67890" ${t}/root/foo
+
+# Standard validations should fail.
+(! ${gomtree} validate -p ${t}/root -f ${t}/time.mtree)
+(! ${gomtree} validate -p ${t}/root -f ${t}/tartime.mtree)
+
+# Forcefully requesting a different time type should also fail.
+(! ${gomtree} validate -k ${keywords},tar_time -p ${t}/root -f ${t}/time.mtree)
+(! ${gomtree} validate -k ${keywords},time -p ${t}/root -f ${t}/tartime.mtree)
+
+# Ditto if we generate the manifests and compare them.
+${gomtree} -c -k ${keywords},time -p ${t}/root -f ${t}/time-change.mtree
+${gomtree} -c -k ${keywords},tar_time -p ${t}/root -f ${t}/tartime-change.mtree
+
+# Same time types.
+(! ${gomtree} validate -f ${t}/time.mtree -f ${t}/time-change.mtree)
+(! ${gomtree} validate -f ${t}/time-change.mtree -f ${t}/time.mtree)
+(! ${gomtree} validate -f ${t}/tartime.mtree -f ${t}/tartime-change.mtree)
+(! ${gomtree} validate -f ${t}/tartime-change.mtree -f ${t}/tartime.mtree)
+
+# Different time types:
+# (old) time <=> (new) tar_time
+(! ${gomtree} validate -f ${t}/time.mtree -f ${t}/tartime-change.mtree)
+(! ${gomtree} validate -f ${t}/tartime-change.mtree -f ${t}/time.mtree)
+# (old) tar_time <=> (new) time
+(! ${gomtree} validate -f ${t}/tartime.mtree -f ${t}/time-change.mtree)
+(! ${gomtree} validate -f ${t}/time-change.mtree -f ${t}/tartime.mtree)