Correctly diff nested data structures

[bhagany]

Summary

I would like to do (boot.core/fileset-diff old-fs new-fs :foo), where the :foo key is a nested data structure set on tmpfiles in the filesets, but this currently only works in some cases. While the current implementation of fileset-diff works well for diffing scalar values, the semantics are not correct for diffing nested structures. This small modification results in correct diffs for nested structures, which is useful when using custom tmpfile metadata.

Explanation

The root of the problem is that, in boot.tmpdir/diff*, only the first two return values of clojure.data/diff are used to calculate changes. For scalars, this is all that's necessary, but nested compound data structures require the use of the third value as well. For example, if you were to diff the following metadata structures, the 3rd value comes into play:

user> (data/diff {"path" {:foo {:stays true :goes false}}}
                 {"path" {:foo {:stays true}}})
({"path" {:foo {:goes false}}} nil {"path" {:foo {:stays true}}})

diff* should return this in the :changed fileset, but it currently registers as :removed. A similar interaction results in nested key additions registering as :added instead of :changed.

If the return value of data/diff is [x y z], then :changed should include all paths that are in both x and y, plus all keys that are in x or y (or both) and in z. Following on from that, :added and :removed should not contain any paths that are in :changed.

[bhagany]

Agreed! I started to add tests, but I'm new to boot, and it wasn't obvious to me how to run them. Any tips?

[bhagany]

I suppose exactly where to add them would be good to know too, my initial stab just picked a random test file.

[martinklepsch]

make test

should run the tests. As for tests file, use whatever seems most appropriate, probably tmpdir_test.clj would make sense...

[bhagany]

I tried make test, but it reports the same number of tests and assertions whether my file exists or not. I must be doing something wrong.

[bhagany]

Okay, I added tests. They pass when running lein test boot.tmpdir-test from the pod directory. However, running all tests in that directory results in unrelated failures, and additionally, the line in the Makefile that would run these tests is commented out for a yet-to-be-determined reason. I feel like that's out of scope for this PR, though. If there's more you'd like to see, let me know.

[arichiardi]

From what I see we actually don't change the behavior for scalar comparison, am I right?

[bhagany]

Yes, that's correct

[arichiardi]

This looks good to me, if you want to be super zealous you could add some edge case like empty maps or nils, in input to diff* but I guess it can be added later (which means never ah ah)

[bhagany]

I'm feeling super zealous, I guess - tests added

[arichiardi]

Awesome, I am not going to "push the button" here but very thorough job!

[bhagany]

Not sure what happened there, but tests are passing again

[arichiardi]

🎉 🍾

[bhagany]

yay!