view: add wrapper that will exclude absent RefTarget entries from ContentHash

The next commit will change these maps to store Option<RefTarget> entries, but
None entries will still be omitted from the serialized data. Since ContentHash
should describe the serialized data, relying on the generic ContentHash would
cause future hash conflict where absent RefTarget entries will be preserved.

For example, ([remove], [None, add]) will be serialized as ([remove], [add]),
and deserialized to ([remove], [add, None]). If we add support for lossless
serialization, hash(([remove], [None, add])) should differ from the lossy one.

lib/src/op_store.rs

@@ -14,8 +14,9 @@
 
 #![allow(missing_docs)]
 
-use std::collections::{BTreeMap, HashMap, HashSet};
+use std::collections::{btree_map, BTreeMap, HashMap, HashSet};
 use std::fmt::{Debug, Error, Formatter};
+use std::ops::{Deref, DerefMut};
 use std::slice;
 
 use thiserror::Error;
@@ -272,6 +273,64 @@ impl RefTargetExt for Option<&RefTarget> {
     }
 }
 
+/// Wrapper to exclude absent `RefTarget` entries from `ContentHash`.
+#[derive(Default, PartialEq, Eq, Clone, Debug)]
+pub struct RefTargetMap(pub BTreeMap<String, RefTarget>);
+
+impl RefTargetMap {
+    pub fn new() -> Self {
+        RefTargetMap(BTreeMap::new())
+    }
+}
+
+// TODO: Update serialization code to preserve absent RefTarget entries, and
+// remove this wrapper.
+impl ContentHash for RefTargetMap {
+    fn hash(&self, state: &mut impl digest::Update) {
+        // Derived from content_hash.rs. It's okay for this to produce a different hash
+        // value than the inner map, but the value must not be equal to the map which
+        // preserves absent RefTarget entries.
+        state.update(&(self.0.len() as u64).to_le_bytes());
+        for (k, v) in self.0.iter() {
+            k.hash(state);
+            v.hash(state);
+        }
+    }
+}
+
+// Abuse Deref as this is a temporary workaround. See the comment above.
+impl Deref for RefTargetMap {
+    type Target = BTreeMap<String, RefTarget>;
+
+    fn deref(&self) -> &Self::Target {
+        &self.0
+    }
+}
+
+impl DerefMut for RefTargetMap {
+    fn deref_mut(&mut self) -> &mut Self::Target {
+        &mut self.0
+    }
+}
+
+impl IntoIterator for RefTargetMap {
+    type Item = (String, RefTarget);
+    type IntoIter = btree_map::IntoIter<String, RefTarget>;
+
+    fn into_iter(self) -> Self::IntoIter {
+        self.0.into_iter()
+    }
+}
+
+impl<'a> IntoIterator for &'a RefTargetMap {
+    type Item = (&'a String, &'a RefTarget);
+    type IntoIter = btree_map::Iter<'a, String, RefTarget>;
+
+    fn into_iter(self) -> Self::IntoIter {
+        self.0.iter()
+    }
+}
+
 content_hash! {
     #[derive(Default, PartialEq, Eq, Clone, Debug)]
     pub struct BranchTarget {
@@ -282,7 +341,7 @@ content_hash! {
         // has been deleted locally and you pull from a remote, maybe it should make a difference
         // whether the branch is known to have existed on the remote. We may not want to resurrect
         // the branch if the branch's state on the remote was just not known.
-        pub remote_targets: BTreeMap<String, RefTarget>,
+        pub remote_targets: RefTargetMap,
     }
 }
 
@@ -296,8 +355,8 @@ content_hash! {
         /// Heads of the set of public commits.
         pub public_head_ids: HashSet<CommitId>,
         pub branches: BTreeMap<String, BranchTarget>,
-        pub tags: BTreeMap<String, RefTarget>,
-        pub git_refs: BTreeMap<String, RefTarget>,
+        pub tags: RefTargetMap,
+        pub git_refs: RefTargetMap,
         /// The commit the Git HEAD points to.
         // TODO: Support multiple Git worktrees?
         // TODO: Do we want to store the current branch name too?

lib/src/refs.rs

@@ -151,15 +151,16 @@ mod tests {
 
     use super::*;
     use crate::backend::ObjectId;
+    use crate::op_store::RefTargetMap;
 
     #[test]
     fn test_classify_branch_push_action_unchanged() {
         let commit_id1 = CommitId::from_hex("11");
         let branch = BranchTarget {
             local_target: RefTarget::normal(commit_id1.clone()),
-            remote_targets: btreemap! {
+            remote_targets: RefTargetMap(btreemap! {
                 "origin".to_string() => RefTarget::normal(commit_id1).unwrap(),
-            },
+            }),
         };
         assert_eq!(
             classify_branch_push_action(&branch, "origin"),
@@ -172,7 +173,7 @@ mod tests {
         let commit_id1 = CommitId::from_hex("11");
         let branch = BranchTarget {
             local_target: RefTarget::normal(commit_id1.clone()),
-            remote_targets: btreemap! {},
+            remote_targets: RefTargetMap::new(),
         };
         assert_eq!(
             classify_branch_push_action(&branch, "origin"),
@@ -188,9 +189,9 @@ mod tests {
         let commit_id1 = CommitId::from_hex("11");
         let branch = BranchTarget {
             local_target: RefTarget::absent(),
-            remote_targets: btreemap! {
+            remote_targets: RefTargetMap(btreemap! {
                 "origin".to_string() => RefTarget::normal(commit_id1.clone()).unwrap(),
-            },
+            }),
         };
         assert_eq!(
             classify_branch_push_action(&branch, "origin"),
@@ -207,9 +208,9 @@ mod tests {
         let commit_id2 = CommitId::from_hex("22");
         let branch = BranchTarget {
             local_target: RefTarget::normal(commit_id2.clone()),
-            remote_targets: btreemap! {
+            remote_targets: RefTargetMap(btreemap! {
                 "origin".to_string() => RefTarget::normal(commit_id1.clone()).unwrap(),
-            },
+            }),
         };
         assert_eq!(
             classify_branch_push_action(&branch, "origin"),
@@ -226,9 +227,9 @@ mod tests {
         let commit_id2 = CommitId::from_hex("22");
         let branch = BranchTarget {
             local_target: RefTarget::from_legacy_form([], [commit_id1.clone(), commit_id2]),
-            remote_targets: btreemap! {
+            remote_targets: RefTargetMap(btreemap! {
                 "origin".to_string() => RefTarget::normal(commit_id1).unwrap(),
-            },
+            }),
         };
         assert_eq!(
             classify_branch_push_action(&branch, "origin"),
@@ -242,12 +243,12 @@ mod tests {
         let commit_id2 = CommitId::from_hex("22");
         let branch = BranchTarget {
             local_target: RefTarget::normal(commit_id1.clone()),
-            remote_targets: btreemap! {
+            remote_targets: RefTargetMap(btreemap! {
                 "origin".to_string() => RefTarget::from_legacy_form(
                     [],
                     [commit_id1, commit_id2],
                 ).unwrap(),
-            },
+            }),
         };
         assert_eq!(
             classify_branch_push_action(&branch, "origin"),

lib/src/simple_op_store.rs

@@ -14,7 +14,6 @@
 
 #![allow(missing_docs)]
 
-use std::collections::BTreeMap;
 use std::fmt::Debug;
 use std::fs;
 use std::io::{ErrorKind, Write};
@@ -28,7 +27,7 @@ use crate::content_hash::blake2b_hash;
 use crate::file_util::persist_content_addressed_temp_file;
 use crate::op_store::{
     BranchTarget, OpStore, OpStoreError, OpStoreResult, Operation, OperationId, OperationMetadata,
-    RefTarget, RefTargetExt as _, View, ViewId, WorkspaceId,
+    RefTarget, RefTargetExt as _, RefTargetMap, View, ViewId, WorkspaceId,
 };
 
 impl From<std::io::Error> for OpStoreError {
@@ -274,7 +273,7 @@ fn view_from_proto(proto: crate::protos::op_store::View) -> View {
     for branch_proto in proto.branches {
         let local_target = ref_target_from_proto(branch_proto.local_target);
 
-        let mut remote_targets = BTreeMap::new();
+        let mut remote_targets = RefTargetMap::new();
         for remote_branch in branch_proto.remote_branches {
             remote_targets.insert(
                 remote_branch.remote_name,
@@ -391,24 +390,24 @@ mod tests {
             branches: btreemap! {
                 "main".to_string() => BranchTarget {
                     local_target: branch_main_local_target,
-                    remote_targets: btreemap! {
+                    remote_targets: RefTargetMap(btreemap! {
                         "origin".to_string() => branch_main_origin_target.unwrap(),
-                    }
+                    }),
                 },
                 "deleted".to_string() => BranchTarget {
                     local_target: RefTarget::absent(),
-                    remote_targets: btreemap! {
+                    remote_targets: RefTargetMap(btreemap! {
                         "origin".to_string() => branch_deleted_origin_target.unwrap(),
-                    }
+                    }),
                 },
             },
-            tags: btreemap! {
+            tags: RefTargetMap(btreemap! {
                 "v1.0".to_string() => tag_v1_target.unwrap(),
-            },
-            git_refs: btreemap! {
+            }),
+            git_refs: RefTargetMap(btreemap! {
                 "refs/heads/main".to_string() => git_refs_main_target.unwrap(),
                 "refs/heads/feature".to_string() => git_refs_feature_target.unwrap(),
-            },
+            }),
             git_head: RefTarget::normal(CommitId::from_hex("fff111")),
             wc_commit_ids: hashmap! {
                 WorkspaceId::default() => default_wc_commit_id,

lib/tests/test_git.rs

@@ -27,7 +27,7 @@ use jj_lib::commit_builder::CommitBuilder;
 use jj_lib::git;
 use jj_lib::git::{GitFetchError, GitPushError, GitRefUpdate, SubmoduleConfig};
 use jj_lib::git_backend::GitBackend;
-use jj_lib::op_store::{BranchTarget, RefTarget};
+use jj_lib::op_store::{BranchTarget, RefTarget, RefTargetMap};
 use jj_lib::repo::{MutableRepo, ReadonlyRepo, Repo};
 use jj_lib::settings::{GitSettings, UserSettings};
 use jj_lib::view::RefName;
@@ -110,9 +110,9 @@ fn test_import_refs() {
 
     let expected_main_branch = BranchTarget {
         local_target: RefTarget::normal(jj_id(&commit2)),
-        remote_targets: btreemap! {
+        remote_targets: RefTargetMap(btreemap! {
           "origin".to_string() => RefTarget::normal(jj_id(&commit1)).unwrap(),
-        },
+        }),
     };
     assert_eq!(
         view.branches().get("main"),
@@ -120,7 +120,7 @@ fn test_import_refs() {
     );
     let expected_feature1_branch = BranchTarget {
         local_target: RefTarget::normal(jj_id(&commit3)),
-        remote_targets: btreemap! {},
+        remote_targets: RefTargetMap::new(),
     };
     assert_eq!(
         view.branches().get("feature1"),
@@ -128,7 +128,7 @@ fn test_import_refs() {
     );
     let expected_feature2_branch = BranchTarget {
         local_target: RefTarget::normal(jj_id(&commit4)),
-        remote_targets: btreemap! {},
+        remote_targets: RefTargetMap::new(),
     };
     assert_eq!(
         view.branches().get("feature2"),
@@ -136,9 +136,9 @@ fn test_import_refs() {
     );
     let expected_feature3_branch = BranchTarget {
         local_target: RefTarget::normal(jj_id(&commit6)),
-        remote_targets: btreemap! {
+        remote_targets: RefTargetMap(btreemap! {
           "origin".to_string() => RefTarget::normal(jj_id(&commit6)).unwrap(),
-        },
+        }),
     };
     assert_eq!(
         view.branches().get("feature3"),
@@ -240,9 +240,9 @@ fn test_import_refs_reimport() {
     let commit2_target = RefTarget::normal(jj_id(&commit2));
     let expected_main_branch = BranchTarget {
         local_target: RefTarget::normal(jj_id(&commit2)),
-        remote_targets: btreemap! {
+        remote_targets: RefTargetMap(btreemap! {
           "origin".to_string() => commit1_target.clone().unwrap(),
-        },
+        }),
     };
     assert_eq!(
         view.branches().get("main"),
@@ -253,7 +253,7 @@ fn test_import_refs_reimport() {
             [jj_id(&commit4)],
             [commit6.id().clone(), jj_id(&commit5)],
         ),
-        remote_targets: btreemap! {},
+        remote_targets: RefTargetMap::new(),
     };
     assert_eq!(
         view.branches().get("feature2"),
@@ -438,18 +438,18 @@ fn test_import_refs_reimport_with_deleted_remote_ref() {
             // Even though the git repo does not have a local branch for `feature-remote-only`, jj
             // creates one. This follows the model explained in docs/branches.md.
             local_target: RefTarget::normal(jj_id(&commit_remote_only)),
-            remote_targets: btreemap! {
+            remote_targets: RefTargetMap(btreemap! {
                 "origin".to_string() => RefTarget::normal(jj_id(&commit_remote_only)).unwrap(),
-            },
+            }),
         }),
     );
     assert_eq!(
         view.branches().get("feature-remote-and-local"),
         Some(&BranchTarget {
             local_target: RefTarget::normal(jj_id(&commit_remote_and_local)),
-            remote_targets: btreemap! {
+            remote_targets: RefTargetMap(btreemap! {
                 "origin".to_string() => RefTarget::normal(jj_id(&commit_remote_and_local)).unwrap(),
-            },
+            }),
         }),
     );
     view.branches().get("main").unwrap(); // branch #3 of 3
@@ -527,18 +527,18 @@ fn test_import_refs_reimport_with_moved_remote_ref() {
             // Even though the git repo does not have a local branch for `feature-remote-only`, jj
             // creates one. This follows the model explained in docs/branches.md.
             local_target: RefTarget::normal(jj_id(&commit_remote_only)),
-            remote_targets: btreemap! {
+            remote_targets: RefTargetMap(btreemap! {
                 "origin".to_string() => RefTarget::normal(jj_id(&commit_remote_only)).unwrap(),
-            },
+            }),
         }),
     );
     assert_eq!(
         view.branches().get("feature-remote-and-local"),
         Some(&BranchTarget {
             local_target: RefTarget::normal(jj_id(&commit_remote_and_local)),
-            remote_targets: btreemap! {
+            remote_targets: RefTargetMap(btreemap! {
                 "origin".to_string() => RefTarget::normal(jj_id(&commit_remote_and_local)).unwrap(),
-            },
+            }),
         }),
     );
     view.branches().get("main").unwrap(); // branch #3 of 3
@@ -571,18 +571,18 @@ fn test_import_refs_reimport_with_moved_remote_ref() {
         view.branches().get("feature-remote-only"),
         Some(&BranchTarget {
             local_target: RefTarget::normal(jj_id(&new_commit_remote_only)),
-            remote_targets: btreemap! {
+            remote_targets: RefTargetMap(btreemap! {
                 "origin".to_string() => RefTarget::normal(jj_id(&new_commit_remote_only)).unwrap(),
-            },
+            }),
         }),
     );
     assert_eq!(
         view.branches().get("feature-remote-and-local"),
         Some(&BranchTarget {
             local_target: RefTarget::normal(jj_id(&new_commit_remote_and_local)),
-            remote_targets: btreemap! {
+            remote_targets: RefTargetMap(btreemap! {
                 "origin".to_string() => RefTarget::normal(jj_id(&new_commit_remote_and_local)).unwrap(),
-            },
+            }),
         }),
     );
     view.branches().get("main").unwrap(); // branch #3 of 3
@@ -712,7 +712,9 @@ fn test_import_some_refs() {
     let commit_feat4_target = RefTarget::normal(jj_id(&commit_feat4));
     let expected_feature1_branch = BranchTarget {
         local_target: RefTarget::normal(jj_id(&commit_feat1)),
-        remote_targets: btreemap! { "origin".to_string() => commit_feat1_target.unwrap() },
+        remote_targets: RefTargetMap(btreemap! {
+            "origin".to_string() => commit_feat1_target.unwrap(),
+        }),
     };
     assert_eq!(
         view.branches().get("feature1"),
@@ -720,7 +722,9 @@ fn test_import_some_refs() {
     );
     let expected_feature2_branch = BranchTarget {
         local_target: RefTarget::normal(jj_id(&commit_feat2)),
-        remote_targets: btreemap! { "origin".to_string() => commit_feat2_target.unwrap() },
+        remote_targets: RefTargetMap(btreemap! {
+            "origin".to_string() => commit_feat2_target.unwrap(),
+        }),
     };
     assert_eq!(
         view.branches().get("feature2"),
@@ -728,7 +732,9 @@ fn test_import_some_refs() {
     );
     let expected_feature3_branch = BranchTarget {
         local_target: RefTarget::normal(jj_id(&commit_feat3)),
-        remote_targets: btreemap! { "origin".to_string() => commit_feat3_target.unwrap() },
+        remote_targets: RefTargetMap(btreemap! {
+            "origin".to_string() => commit_feat3_target.unwrap(),
+        }),
     };
     assert_eq!(
         view.branches().get("feature3"),
@@ -736,7 +742,9 @@ fn test_import_some_refs() {
     );
     let expected_feature4_branch = BranchTarget {
         local_target: RefTarget::normal(jj_id(&commit_feat4)),
-        remote_targets: btreemap! { "origin".to_string() => commit_feat4_target.unwrap() },
+        remote_targets: RefTargetMap(btreemap! {
+            "origin".to_string() => commit_feat4_target.unwrap(),
+        }),
     };
     assert_eq!(
         view.branches().get("feature4"),
@@ -1144,9 +1152,9 @@ fn test_import_export_no_auto_local_branch() {
 
     let expected_branch = BranchTarget {
         local_target: RefTarget::absent(),
-        remote_targets: btreemap! {
+        remote_targets: RefTargetMap(btreemap! {
             "origin".to_string() => RefTarget::normal(jj_id(&git_commit)).unwrap(),
-        },
+        }),
     };
     assert_eq!(
         mut_repo.view().branches().get("main"),
@@ -1496,7 +1504,9 @@ fn test_fetch_initial_commit() {
         btreemap! {
             "main".to_string() => BranchTarget {
                 local_target: initial_commit_target.clone(),
-                remote_targets: btreemap! {"origin".to_string() => initial_commit_target.unwrap()}
+                remote_targets: RefTargetMap(btreemap! {
+                    "origin".to_string() => initial_commit_target.unwrap(),
+                })
             },
         }
     );
@@ -1559,7 +1569,9 @@ fn test_fetch_success() {
         btreemap! {
             "main".to_string() => BranchTarget {
                 local_target: new_commit_target.clone(),
-                remote_targets: btreemap! {"origin".to_string() => new_commit_target.unwrap()}
+                remote_targets: RefTargetMap(btreemap! {
+                    "origin".to_string() => new_commit_target.unwrap(),
+                }),
             },
         }
     );

lib/tests/test_view.rs

@@ -14,7 +14,7 @@
 
 use std::sync::Arc;
 
-use jj_lib::op_store::{BranchTarget, RefTarget, WorkspaceId};
+use jj_lib::op_store::{BranchTarget, RefTarget, RefTargetMap, WorkspaceId};
 use jj_lib::repo::{ReadonlyRepo, Repo};
 use jj_lib::settings::UserSettings;
 use jj_lib::transaction::Transaction;
@@ -309,14 +309,14 @@ fn test_merge_views_branches() {
                 main_branch_local_tx2.id().clone(),
             ],
         ),
-        remote_targets: btreemap! {
+        remote_targets: RefTargetMap(btreemap! {
             "origin".to_string() => RefTarget::normal(main_branch_origin_tx1.id().clone()).unwrap(),
             "alternate".to_string() => RefTarget::normal(main_branch_alternate_tx0.id().clone()).unwrap(),
-        },
+        }),
     };
     let expected_feature_branch = BranchTarget {
         local_target: RefTarget::normal(feature_branch_tx1.id().clone()),
-        remote_targets: btreemap! {},
+        remote_targets: RefTargetMap::new(),
     };
     assert_eq!(
         repo.view().branches(),