From 1ce6aa8db51fa0c3b76be2cff88252a28c5284b5 Mon Sep 17 00:00:00 2001 From: Aleksa Sarai Date: Tue, 16 Sep 2025 22:27:20 +1000 Subject: [PATCH] compare: move FreeBSD loose keyword comparisons to gomtree command FreeBSD has quite unfortunate behaviour when dealing with keywords that are missing in one of the manifests being compared -- namely, they ignore these instances. Commit 21723a3974bc ("*: fix comparison of missing keywords") re-added this behaviour after the introduction of the Compare API, but unfortunately it was implemented in the Compare API itself -- meaning that library users (which didn't want this behaviour) were silently opted into it. This patch moves the behaviour to the command-line, where it belongs (a future patch in this series will allow users to opt-out of this unfortunate behaviour, as well as some other unfortunate FreeBSD compatibility behaviours). Fixes: 21723a3974bc ("*: fix comparison of missing keywords") Signed-off-by: Aleksa Sarai --- cmd/gomtree/cmd/validate.go | 20 +++++++++++ compare.go | 26 -------------- compare_test.go | 72 +++++++++++++++++++++++++++++++++++++ 3 files changed, 92 insertions(+), 26 deletions(-) diff --git a/cmd/gomtree/cmd/validate.go b/cmd/gomtree/cmd/validate.go index 7186be3..bbff281 100644 --- a/cmd/gomtree/cmd/validate.go +++ b/cmd/gomtree/cmd/validate.go @@ -368,6 +368,7 @@ func validateAction(c *cli.Context) error { if isTarSpec(specDh) || c.String("tar") != "" { filters = append(filters, tarKeywordFilter) } + filters = append(filters, freebsdCompatKeywordFilter) res = filterDeltas(res, filters...) if len(res) > 0 { @@ -458,6 +459,25 @@ func tarKeywordFilter(delta *mtree.InodeDelta) bool { return true } +// freebsdCompatKeywordFilter removes any deltas where a key is not present in +// both manifests being compared. This is necessary for compatibility with +// FreeBSD's mtree(8) but is generally undesireable for most users. +func freebsdCompatKeywordFilter(delta *mtree.InodeDelta) bool { + if delta.Type() != mtree.Modified { + return true + } + keys := delta.DiffPtr() + *keys = slices.DeleteFunc(*keys, func(kd mtree.KeyDelta) bool { + if kd.Name().Prefix() == "xattr" { + // Even in FreeBSD compatibility mode, any xattr changes should + // still be treated as a proper change and not filtered out. + return false + } + return kd.Type() != mtree.Modified + }) + return true +} + type deltaFilterFn func(*mtree.InodeDelta) bool // filterDeltas takes the set of deltas generated by mtree and applies the diff --git a/compare.go b/compare.go index 67d92fc..90916dd 100644 --- a/compare.go +++ b/compare.go @@ -4,7 +4,6 @@ import ( "encoding/json" "fmt" "iter" - "maps" "slices" "github.com/sirupsen/logrus" @@ -247,31 +246,6 @@ func compareEntry(oldEntry, newEntry Entry) ([]KeyDelta, error) { newKeys = newEntry.allKeysMap() ) - // Delete any keys which are not present in both maps. - keyFilterFn := func(otherMap map[Keyword]KeyVal) func(Keyword, KeyVal) bool { - return func(k Keyword, _ KeyVal) bool { - switch { - case k.Prefix() == "xattr": - // xattrs are presented as one keyword to users but are actually - // implemented as separate keywords and so we should always include - // them (even if the same xattr is not present in both sides). - // TODO: I actually think this is not inline with the original - // purpose of this code, but I'm leaving it as-is to not not - // introduce bugs. - return false - case k == "time" || k == "tar_time": - // These have special handling later. - return false - default: - // Drop keys which do not exist in the other entry. - _, ok := otherMap[k] - return !ok - } - } - } - maps.DeleteFunc(oldKeys, keyFilterFn(newKeys)) - maps.DeleteFunc(newKeys, keyFilterFn(oldKeys)) - // If both tar_time and time were specified in the set of keys, we have to // convert the "time" entries to "tar_time" to allow for tar archive // manifests to be compared with proper filesystem manifests. diff --git a/compare_test.go b/compare_test.go index d8e0710..19539aa 100644 --- a/compare_test.go +++ b/compare_test.go @@ -7,6 +7,7 @@ import ( "io" "os" "path/filepath" + "slices" "testing" "time" @@ -230,6 +231,77 @@ func TestCompareKeySubset(t *testing.T) { } } +func TestCompareKeyDelta(t *testing.T) { + dir := t.TempDir() + + // Create a bunch of objects. + tmpfile := filepath.Join(dir, "tmpfile") + require.NoError(t, os.WriteFile(tmpfile, []byte("some content here"), 0666)) + + tmpdir := filepath.Join(dir, "testdir") + require.NoError(t, os.Mkdir(tmpdir, 0755)) + + tmpsubfile := filepath.Join(tmpdir, "anotherfile") + require.NoError(t, os.WriteFile(tmpsubfile, []byte("aaa"), 0666)) + + // Walk the current state. + manifestKeywords := append(DefaultKeywords[:], "sha1digest") + old, err := Walk(dir, nil, manifestKeywords, nil) + require.NoErrorf(t, err, "walk %s", dir) + + t.Run("Extra-Key", func(t *testing.T) { + extraKeyword := Keyword("sha256digest") + newManifestKeywords := append(manifestKeywords[:], extraKeyword) + + new, err := Walk(dir, nil, newManifestKeywords, nil) + require.NoErrorf(t, err, "walk %s", dir) + + diffs, err := Compare(old, new, nil) + require.NoError(t, err, "compare") + + assert.NotEmpty(t, diffs, "extra keys in manifest should result in deltas") + for _, diff := range diffs { + if assert.Equal(t, Modified, diff.Type(), "extra keyword diff element should be 'modified'") { + kds := diff.Diff() + if assert.Len(t, kds, 1, "should only get a single key delta") { + kd := kds[0] + assert.Equalf(t, Extra, kd.Type(), "key %q", kd.Name()) + assert.Equal(t, extraKeyword, kd.Name()) + assert.Nil(t, kd.Old(), "Old for extra keyword delta") + assert.NotNil(t, kd.New(), "New for extra keyword delta") + } + } + } + }) + + t.Run("Missing-Key", func(t *testing.T) { + missingKeyword := Keyword("sha1digest") + newManifestKeywords := slices.DeleteFunc(manifestKeywords[:], func(kw Keyword) bool { + return kw == missingKeyword + }) + + new, err := Walk(dir, nil, newManifestKeywords, nil) + require.NoErrorf(t, err, "walk %s", dir) + + diffs, err := Compare(old, new, nil) + require.NoError(t, err, "compare") + + assert.NotEmpty(t, diffs, "missing keys in manifest should result in deltas") + for _, diff := range diffs { + if assert.Equal(t, Modified, diff.Type(), "missing keyword diff element should be 'modified'") { + kds := diff.Diff() + if assert.Len(t, kds, 1, "should only get a single key delta") { + kd := kds[0] + assert.Equalf(t, Missing, kd.Type(), "key %q", kd.Name()) + assert.Equal(t, missingKeyword, kd.Name()) + assert.NotNil(t, kd.Old(), "Old for missing keyword delta") + assert.Nil(t, kd.New(), "New for missing keyword delta") + } + } + } + }) +} + //gocyclo:ignore func TestTarCompare(t *testing.T) { dir := t.TempDir()