diff --git a/lib/src/op_store.rs b/lib/src/op_store.rs index d99517409..c1b22748c 100644 --- a/lib/src/op_store.rs +++ b/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); + +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; + + 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; + + 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, + pub remote_targets: RefTargetMap, } } @@ -296,8 +355,8 @@ content_hash! { /// Heads of the set of public commits. pub public_head_ids: HashSet, pub branches: BTreeMap, - pub tags: BTreeMap, - pub git_refs: BTreeMap, + 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? diff --git a/lib/src/refs.rs b/lib/src/refs.rs index 23ef19bfc..793e285c1 100644 --- a/lib/src/refs.rs +++ b/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"), diff --git a/lib/src/simple_op_store.rs b/lib/src/simple_op_store.rs index df118cc76..749cf566e 100644 --- a/lib/src/simple_op_store.rs +++ b/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 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, diff --git a/lib/tests/test_git.rs b/lib/tests/test_git.rs index b5a8d9094..6af944445 100644 --- a/lib/tests/test_git.rs +++ b/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(), + }), }, } ); diff --git a/lib/tests/test_view.rs b/lib/tests/test_view.rs index edb5a64b6..4c5c07569 100644 --- a/lib/tests/test_view.rs +++ b/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(),