🐛 Compare a nil-valued attribute and an absent one as equal

A branch copy re-points its local references through the duplication pipeline
and leaves keys like :fill-color-ref-file and :typography-ref-file present with
nil where the source omitted them. Comparing the two forms as different made an
untouched entity look edited on one side, which becomes a modify-modify
conflict as soon as the other side moves the same shape.

The comparison now drops nil-valued keys deeply, for all three sides, in
three-way-entities and so for every pass that goes through it. The new engine
test failed before the change on both orderings and passes after; the three
branch suites report 30 tests, 245 assertions, 0 failures.

backend/test/backend_tests/branch_merge_test.clj

@@ -0,0 +1,75 @@
+;; This Source Code Form is subject to the terms of the Mozilla Public
+;; License, v. 2.0. If a copy of the MPL was not distributed with this
+;; file, You can obtain one at http://mozilla.org/MPL/2.0/.
+;;
+;; Copyright (c) KALEIDOS INC Sucursal en España SL
+
+(ns backend-tests.branch-merge-test
+  "Engine-level tests for the three-way comparison, with data built by hand
+  rather than through a file fixture.
+
+  The case that matters here came from a real update-from-main: the branch
+  copy and its base disagreed only on whether a reference attribute was
+  present with a `nil` value or was absent, and because main had moved the
+  same shapes, the engine called 368 of them `:modify-modify` conflicts."
+  (:require
+   [app.common.files.branch-merge :as bm]
+   [app.common.uuid :as uuid]
+   [clojure.test :refer [deftest is testing]]))
+
+(def page-id (uuid/next))
+(def shape-id (uuid/next))
+
+(defn- file-data
+  "A one-page file whose single text shape carries `extra` inside its
+  `:content` (or omits it, when `extra` is empty) and sits at `y`."
+  [y extra]
+  {:pages [page-id]
+   :pages-index
+   {page-id
+    {:id page-id
+     :objects
+     {shape-id
+      {:id shape-id :name "label" :type :text :x 0 :y y :width 10 :height 10
+       :content (merge {:type :root
+                        :children [{:type :paragraph-set
+                                    :children [{:type :paragraph
+                                                :children [{:type :span :text "hi"}]}]}]}
+                       extra)}}}}})
+
+(def ^:private nil-ref {:typography-ref-file nil})
+
+(deftest nil-ref-and-absent-ref-are-the-same-shape
+  (testing "the branch's copy has the ref with nil, its base without, and main moved the shape"
+    (let [res (bm/compute-merge (file-data 0 {})
+                                (file-data 5 {})
+                                (file-data 0 nil-ref)
+                                :main->branch)]
+      (is (zero? (count (:conflicts res))) "a nil-versus-absent reference is not a branch edit")
+      (is (= 1 (count (:changes res))) "main's move still travels as a change")
+      (is (= :modified (:status (first (:changes res)))))))
+
+  (testing "and the same with the sides the other way round"
+    (let [res (bm/compute-merge (file-data 0 nil-ref)
+                                (file-data 5 nil-ref)
+                                (file-data 0 {})
+                                :main->branch)]
+      (is (zero? (count (:conflicts res))))
+      (is (= 1 (count (:changes res)))))))
+
+(deftest a-real-difference-is-still-a-difference
+  (testing "a value both sides moved off the base is still a conflict"
+    (let [res (bm/compute-merge (file-data 0 {})
+                                (file-data 5 {})
+                                (file-data -5 {})
+                                :main->branch)]
+      (is (= 1 (count (:conflicts res))))
+      (is (= :modify-modify (:reason (first (:conflicts res)))))))
+
+  (testing "a change on one side only still travels"
+    (let [res (bm/compute-merge (file-data 0 {})
+                                (file-data 5 {})
+                                (file-data 0 {})
+                                :main->branch)]
+      (is (zero? (count (:conflicts res))))
+      (is (= 1 (count (:changes res)))))))

common/src/app/common/files/branch_merge.cljc

@@ -123,6 +123,36 @@
               (transient {})
               (or objects {}))))
 
+(defn- strip-nil-attrs
+  "Drop keys whose value is `nil`, deeply, so that a key present with a
+  `nil` value compares equal to an absent key.
+
+  A branch copy re-points its local references through the duplication
+  pipeline, which leaves keys like `:fill-color-ref-file` and
+  `:typography-ref-file` present with `nil` where the source omitted them;
+  the same asymmetry appears inside a text `:content`. Comparing the two
+  forms as different makes an untouched entity look edited on one side, and
+  when the other side really moved it the engine reports a false
+  `:modify-modify` conflict — 368 of them on the design-system file's
+  screenshots of the shape trees, before this existed. Absent and nil mean
+  the same thing for a shape attribute, so the comparison should not see
+  the difference."
+  [v]
+  (cond
+    (map? v)
+    (persistent!
+     (reduce-kv (fn [acc k x]
+                  (let [x' (strip-nil-attrs x)]
+                    (if (nil? x')
+                      acc
+                      (assoc! acc k x'))))
+                (transient {})
+                v))
+
+    (vector? v) (mapv strip-nil-attrs v)
+    (set? v)    (into #{} (map strip-nil-attrs) v)
+    :else       v))
+
 (defn- shape-display-meta
   "Display-only metadata for a shape diff entry — its type and component
   nature — so the compare view can pick a type-accurate icon and label
@@ -162,9 +192,9 @@
               (merge extra extras {:kind kind :status status}))]
     (reduce
      (fn [acc id]
-       (let [b (get base id)
-             t (get theirs id)
-             o (get ours id)
+       (let [b (strip-nil-attrs (get base id))
+             t (strip-nil-attrs (get theirs id))
+             o (strip-nil-attrs (get ours id))
              in-b? (contains? base id)
              in-t? (contains? theirs id)
              in-o? (contains? ours id)]