diff --git a/cli/tests/test_operations.rs b/cli/tests/test_operations.rs index 710b6e40ef..4e2e11e370 100644 --- a/cli/tests/test_operations.rs +++ b/cli/tests/test_operations.rs @@ -478,10 +478,10 @@ fn test_op_abandon_ancestors() { Abandoned 2 operations and reparented 1 descendant operations. "#); insta::assert_snapshot!( - test_env.jj_cmd_success(&repo_path, &["debug", "local-working-copy", "--ignore-working-copy"]), @r###" + test_env.jj_cmd_success(&repo_path, &["debug", "local-working-copy", "--ignore-working-copy"]), @r#" Current operation: OperationId("8545e013752445fd845c84eb961dbfbce47e1deb628e4ef20df10f6dc9aae2ef9e47200b0fcc70ca51f050aede05d0fa6dd1db40e20ae740876775738a07d02e") - Current tree: Merge(Resolved(TreeId("4b825dc642cb6eb9a060e54bf8d69288fbee4904"))) - "###); + Current tree: Resolved(TreeId("4b825dc642cb6eb9a060e54bf8d69288fbee4904")) + "#); insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["op", "log"]), @r###" @ 8545e0137524 test-username@host.example.com 2001-02-03 04:05:09.000 +07:00 - 2001-02-03 04:05:09.000 +07:00 │ commit 81a4ef3dd421f3184289df1c58bd3a16ea1e3d8e @@ -527,10 +527,10 @@ fn test_op_abandon_ancestors() { Abandoned 1 operations and reparented 1 descendant operations. "###); insta::assert_snapshot!( - test_env.jj_cmd_success(&repo_path, &["debug", "local-working-copy", "--ignore-working-copy"]), @r###" + test_env.jj_cmd_success(&repo_path, &["debug", "local-working-copy", "--ignore-working-copy"]), @r#" Current operation: OperationId("0699d720d0cecd80fb7d765c45955708c61b12feb1d7ed9ff2777ae719471f04ffed3c1dc24efdbf94bdb74426065d6fa9a4f0862a89db2c8c8e359eefc45462") - Current tree: Merge(Resolved(TreeId("4b825dc642cb6eb9a060e54bf8d69288fbee4904"))) - "###); + Current tree: Resolved(TreeId("4b825dc642cb6eb9a060e54bf8d69288fbee4904")) + "#); insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["op", "log"]), @r###" @ 0699d720d0ce test-username@host.example.com 2001-02-03 04:05:21.000 +07:00 - 2001-02-03 04:05:21.000 +07:00 │ undo operation d92d0753399f732e438bdd88fa7e5214cba2a310d120ec1714028a514c7116bcf04b4a0b26c04dbecf0a917f1d4c8eb05571b8816dd98b0502aaf321e92500b3 @@ -574,7 +574,7 @@ fn test_op_abandon_without_updating_working_copy() { insta::assert_snapshot!( test_env.jj_cmd_success(&repo_path, &["debug", "local-working-copy", "--ignore-working-copy"]), @r#" Current operation: OperationId("b0711a8ac91f5ac088cff9b57c9daf29dc61b1b4fedcbb9a07fe4c7f7da1e60e333c787eacf73d1e0544db048a4fe9c6c089991b4a67e25365c4f411fa8b489f") - Current tree: Merge(Resolved(TreeId("4b825dc642cb6eb9a060e54bf8d69288fbee4904"))) + Current tree: Resolved(TreeId("4b825dc642cb6eb9a060e54bf8d69288fbee4904")) "#); insta::assert_snapshot!( test_env.jj_cmd_success(&repo_path, &["op", "log", "-n1", "--ignore-working-copy"]), @r#" @@ -594,7 +594,7 @@ fn test_op_abandon_without_updating_working_copy() { insta::assert_snapshot!( test_env.jj_cmd_success(&repo_path, &["debug", "local-working-copy", "--ignore-working-copy"]), @r#" Current operation: OperationId("b0711a8ac91f5ac088cff9b57c9daf29dc61b1b4fedcbb9a07fe4c7f7da1e60e333c787eacf73d1e0544db048a4fe9c6c089991b4a67e25365c4f411fa8b489f") - Current tree: Merge(Resolved(TreeId("4b825dc642cb6eb9a060e54bf8d69288fbee4904"))) + Current tree: Resolved(TreeId("4b825dc642cb6eb9a060e54bf8d69288fbee4904")) "#); insta::assert_snapshot!( test_env.jj_cmd_success(&repo_path, &["op", "log", "-n1", "--ignore-working-copy"]), @r#" diff --git a/cli/tests/test_util_command.rs b/cli/tests/test_util_command.rs index bd019b2f21..10d1056768 100644 --- a/cli/tests/test_util_command.rs +++ b/cli/tests/test_util_command.rs @@ -91,7 +91,7 @@ fn test_gc_operation_log() { // Now this doesn't work. let stderr = test_env.jj_cmd_failure(&repo_path, &["debug", "operation", &op_to_remove]); insta::assert_snapshot!(stderr, @r#" - Error: No operation ID matching "8382f401329617b0c91a63354b86ca48fc28dee8d7a916fdad5310030f9a1260e969c43ed2b13d1d48eaf38f6f45541ecf593bcb6105495d514d21b3b6a98846" + Error: No operation ID matching "528205ec2c5cf94319e7c30e0b42f841812ff66c19e6d46f4099208a940e6758a9bd32b8f26bb5514a2e141b23fc02b24306fa58ba4f2ff7c14f753af0e69604" "#); } diff --git a/cli/tests/test_workspaces.rs b/cli/tests/test_workspaces.rs index db6f07e513..38ec02cc12 100644 --- a/cli/tests/test_workspaces.rs +++ b/cli/tests/test_workspaces.rs @@ -658,14 +658,14 @@ fn test_workspaces_current_op_discarded_by_other() { ], ); insta::assert_snapshot!(stdout, @r#" - @ 757bc1140b abandon commit 20dd439c4bd12c6ad56c187ac490bd0141804618f638dc5c4dc92ff9aecba20f152b23160db9dcf61beb31a5cb14091d9def5a36d11c9599cc4d2e5689236af1 - ○ 8d4abed655 create initial working-copy commit in workspace secondary - ○ 3de27432e5 add workspace 'secondary' - ○ bcf69de808 new empty commit - ○ a36b99a15c snapshot working copy - ○ ddf023d319 new empty commit - ○ 829c93f6a3 snapshot working copy - ○ 2557266dd2 add workspace 'default' + @ 28cd5cdd88 abandon commit 5eaf6e87919b87ce8078cecd6a6216606601e7372eebc1c9a791595ce8fe7963a9bdd8c56640f2e14baac9e62efb4fccc0c5b3d8e21a95e877b20bc8c20e73d5 + ○ 44fbe961e3 create initial working-copy commit in workspace secondary + ○ 114173706e add workspace 'secondary' + ○ 1ad55160ae new empty commit + ○ c9efb83077 snapshot working copy + ○ 243778e06d new empty commit + ○ 0b530a766d snapshot working copy + ○ ffbb23b00f add workspace 'default' ○ 0000000000 "#); @@ -673,13 +673,13 @@ fn test_workspaces_current_op_discarded_by_other() { test_env.jj_cmd_ok(&main_path, &["operation", "abandon", "..@-"]); test_env.jj_cmd_ok(&main_path, &["util", "gc", "--expire=now"]); - insta::assert_snapshot!(get_log_output(&test_env, &main_path), @r###" - ○ 96b31dafdc41 secondary@ - │ @ 6c051bd1ccd5 default@ + insta::assert_snapshot!(get_log_output(&test_env, &main_path), @r#" + @ 214eef6a63cb default@ + │ ○ 1451cf1f1398 secondary@ ├─╯ - ○ 7c5b25a4fc8f + ○ b431c7611a06 ◆ 000000000000 - "###); + "#); let stderr = test_env.jj_cmd_failure(&secondary_path, &["st"]); insta::assert_snapshot!(stderr, @r###" @@ -690,19 +690,19 @@ fn test_workspaces_current_op_discarded_by_other() { let (stdout, stderr) = test_env.jj_cmd_ok(&secondary_path, &["workspace", "update-stale"]); insta::assert_snapshot!(stderr, @r#" - Failed to read working copy's current operation; attempting recovery. Error message from read attempt: Object 8d4abed655badb70b1bab62aa87136619dbc3c8015a8ce8dfb7abfeca4e2f36c713d8f84e070a0613907a6cee7e1cc05323fe1205a319b93fe978f11a060c33c of type operation not found - Created and checked out recovery commit 62f70695e3b0 + Failed to read working copy's current operation; attempting recovery. Error message from read attempt: Object 44fbe961e37a84af0908d31684571660b28fbe0db71d39abd16a8385200cd048b81a529a5e3988a64dad7fb5a8223545d330316e4f629cf6453b937ed2a17355 of type operation not found + Created and checked out recovery commit 44b50c0d749c "#); insta::assert_snapshot!(stdout, @""); - insta::assert_snapshot!(get_log_output(&test_env, &main_path), @r###" - ○ b0b400439a82 secondary@ - ○ 96b31dafdc41 - │ @ 6c051bd1ccd5 default@ + insta::assert_snapshot!(get_log_output(&test_env, &main_path), @r#" + ○ 547a8f368e77 secondary@ + ○ 1451cf1f1398 + │ @ 214eef6a63cb default@ ├─╯ - ○ 7c5b25a4fc8f + ○ b431c7611a06 ◆ 000000000000 - "###); + "#); // The sparse patterns should remain let stdout = test_env.jj_cmd_success(&secondary_path, &["sparse", "list"]); @@ -713,14 +713,14 @@ fn test_workspaces_current_op_discarded_by_other() { "###); let (stdout, stderr) = test_env.jj_cmd_ok(&secondary_path, &["st"]); insta::assert_snapshot!(stderr, @""); - insta::assert_snapshot!(stdout, @r###" + insta::assert_snapshot!(stdout, @r#" Working copy changes: A added D deleted M modified - Working copy : kmkuslsw b0b40043 (no description set) - Parent commit: rzvqmyuk 96b31daf (empty) (no description set) - "###); + Working copy : kmkuslsw 547a8f36 (no description set) + Parent commit: rzvqmyuk 1451cf1f (empty) (no description set) + "#); // The modified file should have the same contents it had before (not reset to // the base contents) insta::assert_snapshot!(std::fs::read_to_string(secondary_path.join("modified")).unwrap(), @r###" @@ -729,12 +729,12 @@ fn test_workspaces_current_op_discarded_by_other() { let (stdout, stderr) = test_env.jj_cmd_ok(&secondary_path, &["evolog"]); insta::assert_snapshot!(stderr, @""); - insta::assert_snapshot!(stdout, @r###" - @ kmkuslsw test.user@example.com 2001-02-03 08:05:18 secondary@ b0b40043 + insta::assert_snapshot!(stdout, @r#" + @ kmkuslsw test.user@example.com 2001-02-03 08:05:18 secondary@ 547a8f36 │ (no description set) - ○ kmkuslsw hidden test.user@example.com 2001-02-03 08:05:18 62f70695 + ○ kmkuslsw hidden test.user@example.com 2001-02-03 08:05:18 44b50c0d (empty) (no description set) - "###); + "#); } #[test] diff --git a/lib/src/backend.rs b/lib/src/backend.rs index 285cdd1295..e7924a787a 100644 --- a/lib/src/backend.rs +++ b/lib/src/backend.rs @@ -103,41 +103,7 @@ pub type SigningFn<'a> = dyn FnMut(&[u8]) -> SignResult> + Send + 'a; /// Identifies a single legacy tree, which may have path-level conflicts, or a /// merge of multiple trees, where the individual trees do not have conflicts. -// TODO(#1624): Delete this type at some point in the future, when we decide to drop -// support for conflicts in older repos, or maybe after we have provided an upgrade -// mechanism. -#[derive(ContentHash, Debug, Clone)] -pub enum MergedTreeId { - /// The tree id of a legacy tree - Legacy(TreeId), - /// The tree id(s) of a merge tree - Merge(Merge), -} - -impl PartialEq for MergedTreeId { - /// Overridden to make conflict-free trees be considered equal even if their - /// `MergedTreeId` variant is different. - fn eq(&self, other: &Self) -> bool { - self.to_merge() == other.to_merge() - } -} - -impl Eq for MergedTreeId {} - -impl MergedTreeId { - /// Create a resolved `MergedTreeId` from a single regular tree. - pub fn resolved(tree_id: TreeId) -> Self { - MergedTreeId::Merge(Merge::resolved(tree_id)) - } - - /// Return this id as `Merge` - pub fn to_merge(&self) -> Merge { - match self { - MergedTreeId::Legacy(tree_id) => Merge::resolved(tree_id.clone()), - MergedTreeId::Merge(tree_ids) => tree_ids.clone(), - } - } -} +pub type MergedTreeId = Merge; #[derive(ContentHash, Debug, PartialEq, Eq, Clone)] pub struct Commit { diff --git a/lib/src/commit.rs b/lib/src/commit.rs index 9d5f210b3c..4de7925322 100644 --- a/lib/src/commit.rs +++ b/lib/src/commit.rs @@ -135,11 +135,7 @@ impl Commit { } pub fn has_conflict(&self) -> BackendResult { - if let MergedTreeId::Merge(tree_ids) = self.tree_id() { - Ok(!tree_ids.is_resolved()) - } else { - Ok(self.tree()?.has_conflict()) - } + Ok(!self.tree_id().is_resolved()) } pub fn change_id(&self) -> &ChangeId { diff --git a/lib/src/git_backend.rs b/lib/src/git_backend.rs index 1f4e2e9154..86a488d922 100644 --- a/lib/src/git_backend.rs +++ b/lib/src/git_backend.rs @@ -368,13 +368,12 @@ impl GitBackend { &self, head_ids: impl IntoIterator, ) -> BackendResult<()> { - self.import_head_commits_with_tree_conflicts(head_ids, true) + self.import_head_commits_with_tree_conflicts(head_ids) } fn import_head_commits_with_tree_conflicts<'a>( &self, head_ids: impl IntoIterator, - uses_tree_conflict_format: bool, ) -> BackendResult<()> { let head_ids: HashSet<&CommitId> = head_ids .into_iter() @@ -404,7 +403,6 @@ impl GitBackend { &mut mut_table, &table_lock, &head_ids, - uses_tree_conflict_format, )?; self.save_extra_metadata_table(mut_table, &table_lock) } @@ -452,7 +450,7 @@ impl GitBackend { repo: &'repo gix::Repository, id: &CommitId, ) -> BackendResult> { - let tree = self.read_commit(id).block_on()?.root_tree.to_merge(); + let tree = self.read_commit(id).block_on()?.root_tree; // TODO(kfm): probably want to do something here if it is a merge let tree_id = tree.first().clone(); let gix_id = validate_git_object_id(&tree_id)?; @@ -509,7 +507,7 @@ fn root_tree_from_header(git_commit: &CommitRef) -> Result, if tree_ids.len() % 2 == 0 { return Err(()); } - return Ok(Some(MergedTreeId::Merge(Merge::from_vec(tree_ids)))); + return Ok(Some(Merge::from_vec(tree_ids))); } } Ok(None) @@ -518,7 +516,6 @@ fn root_tree_from_header(git_commit: &CommitRef) -> Result, fn commit_from_git_without_root_parent( id: &CommitId, git_object: &gix::Object, - uses_tree_conflict_format: bool, is_shallow: bool, ) -> BackendResult { let commit = git_object @@ -554,13 +551,7 @@ fn commit_from_git_without_root_parent( // the extra metadata. let root_tree = root_tree_from_header(&commit) .map_err(|()| to_read_object_err("Invalid jj:trees header", id))?; - let root_tree = root_tree.unwrap_or_else(|| { - if uses_tree_conflict_format { - MergedTreeId::resolved(tree_id) - } else { - MergedTreeId::Legacy(tree_id) - } - }); + let root_tree = root_tree.unwrap_or_else(|| MergedTreeId::resolved(tree_id)); // Use lossy conversion as commit message with "mojibake" is still better than // nothing. // TODO: what should we do with commit.encoding? @@ -656,11 +647,9 @@ fn serialize_extras(commit: &Commit) -> Vec { change_id: commit.change_id.to_bytes(), ..Default::default() }; - if let MergedTreeId::Merge(tree_ids) = &commit.root_tree { - proto.uses_tree_conflict_format = true; - if !tree_ids.is_resolved() { - proto.root_tree = tree_ids.iter().map(|r| r.to_bytes()).collect(); - } + proto.uses_tree_conflict_format = true; + if !commit.root_tree.is_resolved() { + proto.root_tree = commit.root_tree.iter().map(|r| r.to_bytes()).collect(); } for predecessor in &commit.predecessors { proto.predecessors.push(predecessor.to_bytes()); @@ -671,29 +660,20 @@ fn serialize_extras(commit: &Commit) -> Vec { fn deserialize_extras(commit: &mut Commit, bytes: &[u8]) { let proto = crate::protos::git_store::Commit::decode(bytes).unwrap(); commit.change_id = ChangeId::new(proto.change_id); - if proto.uses_tree_conflict_format { - if !proto.root_tree.is_empty() { - let merge_builder: MergeBuilder<_> = proto - .root_tree - .iter() - .map(|id_bytes| TreeId::from_bytes(id_bytes)) - .collect(); - let merge = merge_builder.build(); - // Check that the trees from the extras match the one we found in the jj:trees - // header - if let MergedTreeId::Merge(existing_merge) = &commit.root_tree { - assert!(existing_merge.is_resolved() || *existing_merge == merge); - } - commit.root_tree = MergedTreeId::Merge(merge); - } else { - // uses_tree_conflict_format was set but there was no root_tree override in the - // proto, which means we should just promote the tree id from the - // git commit to be a known-conflict-free tree - let MergedTreeId::Legacy(legacy_tree_id) = &commit.root_tree else { - panic!("root tree should have been initialized to a legacy id"); - }; - commit.root_tree = MergedTreeId::resolved(legacy_tree_id.clone()); - } + if !proto.uses_tree_conflict_format { + panic!("root tree should have been initialized to a legacy id"); + } + if !proto.root_tree.is_empty() { + let merge_builder: MergeBuilder<_> = proto + .root_tree + .iter() + .map(|id_bytes| TreeId::from_bytes(id_bytes)) + .collect(); + let merge = merge_builder.build(); + // Check that the trees from the extras match the one we found in the jj:trees + // header + assert!(commit.root_tree.is_resolved() || commit.root_tree == merge); + commit.root_tree = merge; } for predecessor in &proto.predecessors { commit.predecessors.push(CommitId::from_bytes(predecessor)); @@ -865,7 +845,6 @@ fn import_extra_metadata_entries_from_heads( mut_table: &mut MutableTable, _table_lock: &FileLock, head_ids: &HashSet<&CommitId>, - uses_tree_conflict_format: bool, ) -> BackendResult<()> { let shallow_commits = git_repo .shallow_commits() @@ -886,12 +865,7 @@ fn import_extra_metadata_entries_from_heads( // TODO(#1624): Should we read the root tree here and check if it has a // `.jjconflict-...` entries? That could happen if the user used `git` to e.g. // change the description of a commit with tree-level conflicts. - let commit = commit_from_git_without_root_parent( - &id, - &git_object, - uses_tree_conflict_format, - is_shallow, - )?; + let commit = commit_from_git_without_root_parent(&id, &git_object, is_shallow)?; mut_table.add_entry(id.to_bytes(), serialize_extras(&commit)); work_ids.extend( commit @@ -1165,7 +1139,7 @@ impl Backend for GitBackend { .ok() .flatten() .is_some_and(|shallow| shallow.contains(&git_object.id)); - commit_from_git_without_root_parent(id, &git_object, false, is_shallow)? + commit_from_git_without_root_parent(id, &git_object, is_shallow)? }; if commit.parents.is_empty() { commit.parents.push(self.root_commit_id.clone()); @@ -1196,12 +1170,10 @@ impl Backend for GitBackend { assert!(contents.secure_sig.is_none(), "commit.secure_sig was set"); let locked_repo = self.lock_git_repo(); - let git_tree_id = match &contents.root_tree { - MergedTreeId::Legacy(tree_id) => validate_git_object_id(tree_id)?, - MergedTreeId::Merge(tree_ids) => match tree_ids.as_resolved() { - Some(tree_id) => validate_git_object_id(tree_id)?, - None => write_tree_conflict(&locked_repo, tree_ids)?, - }, + let tree_ids = &contents.root_tree; + let git_tree_id = match tree_ids.as_resolved() { + Some(tree_id) => validate_git_object_id(tree_id)?, + None => write_tree_conflict(&locked_repo, tree_ids)?, }; let author = signature_to_git(&contents.author); let mut committer = signature_to_git(&contents.committer); @@ -1230,14 +1202,12 @@ impl Backend for GitBackend { } } let mut extra_headers = vec![]; - if let MergedTreeId::Merge(tree_ids) = &contents.root_tree { - if !tree_ids.is_resolved() { - let value = tree_ids.iter().map(|id| id.hex()).join(" ").into_bytes(); - extra_headers.push(( - BString::new(JJ_TREES_COMMIT_HEADER.to_vec()), - BString::new(value), - )); - } + if !tree_ids.is_resolved() { + let value = tree_ids.iter().map(|id| id.hex()).join(" ").into_bytes(); + extra_headers.push(( + BString::new(JJ_TREES_COMMIT_HEADER.to_vec()), + BString::new(value), + )); } let extras = serialize_extras(&contents); @@ -1537,14 +1507,12 @@ mod tests { use git2::Oid; use hex::ToHex; use pollster::FutureExt; - use test_case::test_case; use super::*; use crate::content_hash::blake2b_hash; - #[test_case(false; "legacy tree format")] - #[test_case(true; "tree-level conflict format")] - fn read_plain_git_commit(uses_tree_conflict_format: bool) { + #[test] + fn read_plain_git_commit() { let settings = user_settings(); let temp_dir = testutils::new_temp_dir(); let store_path = temp_dir.path(); @@ -1609,7 +1577,7 @@ mod tests { // Import the head commit and its ancestors backend - .import_head_commits_with_tree_conflicts([&commit_id2], uses_tree_conflict_format) + .import_head_commits_with_tree_conflicts([&commit_id2]) .unwrap(); // Ref should be created only for the head commit let git_refs = backend @@ -1626,14 +1594,9 @@ mod tests { assert_eq!(commit.parents, vec![CommitId::from_bytes(&[0; 20])]); assert_eq!(commit.predecessors, vec![]); assert_eq!( - commit.root_tree.to_merge(), + commit.root_tree, Merge::resolved(TreeId::from_bytes(root_tree_id.as_bytes())) ); - if uses_tree_conflict_format { - assert_matches!(commit.root_tree, MergedTreeId::Merge(_)); - } else { - assert_matches!(commit.root_tree, MergedTreeId::Legacy(_)); - } assert_eq!(commit.description, "git commit message"); assert_eq!(commit.author.name, "git author"); assert_eq!(commit.author.email, "git.author@example.com"); @@ -1695,14 +1658,9 @@ mod tests { assert_eq!(commit2.parents, vec![commit_id.clone()]); assert_eq!(commit.predecessors, vec![]); assert_eq!( - commit.root_tree.to_merge(), + commit.root_tree, Merge::resolved(TreeId::from_bytes(root_tree_id.as_bytes())) ); - if uses_tree_conflict_format { - assert_matches!(commit.root_tree, MergedTreeId::Merge(_)); - } else { - assert_matches!(commit.root_tree, MergedTreeId::Legacy(_)); - } } #[test] @@ -1849,7 +1807,7 @@ mod tests { let mut commit = Commit { parents: vec![], predecessors: vec![], - root_tree: MergedTreeId::Legacy(backend.empty_tree_id().clone()), + root_tree: MergedTreeId::resolved(backend.empty_tree_id().clone()), change_id: ChangeId::from_hex("abc123"), description: "".to_string(), author: create_signature(), @@ -1931,7 +1889,7 @@ mod tests { let mut commit = Commit { parents: vec![backend.root_commit_id().clone()], predecessors: vec![], - root_tree: MergedTreeId::Merge(root_tree.clone()), + root_tree: root_tree.clone(), change_id: ChangeId::from_hex("abc123"), description: "".to_string(), author: create_signature(), @@ -2024,7 +1982,7 @@ mod tests { let commit = Commit { parents: vec![backend.root_commit_id().clone()], predecessors: vec![], - root_tree: MergedTreeId::Legacy(backend.empty_tree_id().clone()), + root_tree: MergedTreeId::resolved(backend.empty_tree_id().clone()), change_id: ChangeId::new(vec![]), description: "initial".to_string(), author: signature.clone(), @@ -2101,7 +2059,7 @@ mod tests { let mut commit1 = Commit { parents: vec![backend.root_commit_id().clone()], predecessors: vec![], - root_tree: MergedTreeId::Legacy(backend.empty_tree_id().clone()), + root_tree: MergedTreeId::resolved(backend.empty_tree_id().clone()), change_id: ChangeId::new(vec![]), description: "initial".to_string(), author: create_signature(), @@ -2147,7 +2105,7 @@ mod tests { let commit = Commit { parents: vec![backend.root_commit_id().clone()], predecessors: vec![], - root_tree: MergedTreeId::Legacy(backend.empty_tree_id().clone()), + root_tree: MergedTreeId::resolved(backend.empty_tree_id().clone()), change_id: ChangeId::new(vec![]), description: "initial".to_string(), author: create_signature(), diff --git a/lib/src/local_backend.rs b/lib/src/local_backend.rs index c0255bb2de..9d5192e852 100644 --- a/lib/src/local_backend.rs +++ b/lib/src/local_backend.rs @@ -45,7 +45,6 @@ use crate::backend::ConflictId; use crate::backend::ConflictTerm; use crate::backend::CopyRecord; use crate::backend::FileId; -use crate::backend::MergedTreeId; use crate::backend::MillisSinceEpoch; use crate::backend::SecureSig; use crate::backend::Signature; @@ -357,15 +356,8 @@ pub fn commit_to_proto(commit: &Commit) -> crate::protos::local_store::Commit { for predecessor in &commit.predecessors { proto.predecessors.push(predecessor.to_bytes()); } - match &commit.root_tree { - MergedTreeId::Legacy(tree_id) => { - proto.root_tree = vec![tree_id.to_bytes()]; - } - MergedTreeId::Merge(tree_ids) => { - proto.uses_tree_conflict_format = true; - proto.root_tree = tree_ids.iter().map(|id| id.to_bytes()).collect(); - } - } + proto.uses_tree_conflict_format = true; + proto.root_tree = commit.root_tree.iter().map(|id| id.to_bytes()).collect(); proto.change_id = commit.change_id.to_bytes(); proto.description = commit.description.clone(); proto.author = Some(signature_to_proto(&commit.author)); @@ -383,12 +375,9 @@ fn commit_from_proto(mut proto: crate::protos::local_store::Commit) -> Commit { let parents = proto.parents.into_iter().map(CommitId::new).collect(); let predecessors = proto.predecessors.into_iter().map(CommitId::new).collect(); - let root_tree = if proto.uses_tree_conflict_format { + let root_tree = { let merge_builder: MergeBuilder<_> = proto.root_tree.into_iter().map(TreeId::new).collect(); - MergedTreeId::Merge(merge_builder.build()) - } else { - assert_eq!(proto.root_tree.len(), 1); - MergedTreeId::Legacy(TreeId::new(proto.root_tree[0].clone())) + merge_builder.build() }; let change_id = ChangeId::new(proto.change_id); Commit { @@ -539,6 +528,7 @@ mod tests { use pollster::FutureExt; use super::*; + use crate::backend::MergedTreeId; /// Test that parents get written correctly #[test] diff --git a/lib/src/local_working_copy.rs b/lib/src/local_working_copy.rs index da2f550814..4d6b0aa4f1 100644 --- a/lib/src/local_working_copy.rs +++ b/lib/src/local_working_copy.rs @@ -638,16 +638,13 @@ impl TreeState { source: err, } })?; - if proto.tree_ids.is_empty() { - self.tree_id = MergedTreeId::Legacy(TreeId::new(proto.legacy_tree_id.clone())); - } else { - let tree_ids_builder: MergeBuilder = proto - .tree_ids - .iter() - .map(|id| TreeId::new(id.clone())) - .collect(); - self.tree_id = MergedTreeId::Merge(tree_ids_builder.build()); - } + assert!(!proto.tree_ids.is_empty()); // legacy format incompatibility + let tree_ids_builder: MergeBuilder = proto + .tree_ids + .iter() + .map(|id| TreeId::new(id.clone())) + .collect(); + self.tree_id = tree_ids_builder.build(); self.file_states = FileStatesMap::from_proto(proto.file_states, proto.is_file_states_sorted); self.sparse_patterns = sparse_patterns_from_proto(proto.sparse_patterns.as_ref()); @@ -659,14 +656,7 @@ impl TreeState { #[allow(clippy::assigning_clones)] fn save(&mut self) -> Result<(), TreeStateError> { let mut proto: crate::protos::working_copy::TreeState = Default::default(); - match &self.tree_id { - MergedTreeId::Legacy(tree_id) => { - proto.legacy_tree_id = tree_id.to_bytes(); - } - MergedTreeId::Merge(tree_ids) => { - proto.tree_ids = tree_ids.iter().map(|id| id.to_bytes()).collect(); - } - } + proto.tree_ids = self.tree_id.iter().map(|id| id.to_bytes()).collect(); proto.file_states = self.file_states.data.clone(); // `FileStatesMap` is guaranteed to be sorted. diff --git a/lib/src/merged_tree.rs b/lib/src/merged_tree.rs index 4ce2abe2ea..1c6625a548 100644 --- a/lib/src/merged_tree.rs +++ b/lib/src/merged_tree.rs @@ -237,7 +237,7 @@ impl MergedTree { /// The tree's id pub fn id(&self) -> MergedTreeId { - MergedTreeId::Merge(self.trees.map(|tree| tree.id().clone())) + self.trees.map(|tree| tree.id().clone()) } /// Look up the tree at the given path. @@ -1129,30 +1129,21 @@ impl MergedTreeBuilder { /// a legacy tree, conflicts can be written either as a multi-way `Merge` /// value or as a resolved `Merge` value using `TreeValue::Conflict`. pub fn set_or_remove(&mut self, path: RepoPathBuf, values: MergedTreeValue) { - if let MergedTreeId::Merge(_) = &self.base_tree_id { - assert!(!values - .iter() - .flatten() - .any(|value| matches!(value, TreeValue::Conflict(_)))); - } + assert!(!values + .iter() + .flatten() + .any(|value| matches!(value, TreeValue::Conflict(_)))); self.overrides.insert(path, values); } /// Create new tree(s) from the base tree(s) and overrides. pub fn write_tree(self, store: &Arc) -> BackendResult { - let base_tree_ids = match self.base_tree_id.clone() { - MergedTreeId::Legacy(base_tree_id) => { - let legacy_base_tree = store.get_tree(RepoPath::root(), &base_tree_id)?; - let base_tree = MergedTree::from_legacy_tree(legacy_base_tree)?; - base_tree.id().to_merge() - } - MergedTreeId::Merge(base_tree_ids) => base_tree_ids, - }; + let base_tree_ids = self.base_tree_id.clone(); // XXX (aseipp): rename this field to self.base_tree_ids? let new_tree_ids = self.write_merged_trees(base_tree_ids, store)?; match new_tree_ids.simplify().into_resolved() { Ok(single_tree_id) => Ok(MergedTreeId::resolved(single_tree_id)), Err(tree_id) => { - let tree = store.get_root_tree(&MergedTreeId::Merge(tree_id))?; + let tree = store.get_root_tree(&tree_id)?; let resolved = tree.resolve()?; Ok(resolved.id()) } diff --git a/lib/src/store.rs b/lib/src/store.rs index 6d64e66596..446711ce9c 100644 --- a/lib/src/store.rs +++ b/lib/src/store.rs @@ -202,17 +202,9 @@ impl Store { Ok(data) } - pub fn get_root_tree(self: &Arc, id: &MergedTreeId) -> BackendResult { - match &id { - MergedTreeId::Legacy(id) => { - let tree = self.get_tree(RepoPath::root(), id)?; - MergedTree::from_legacy_tree(tree) - } - MergedTreeId::Merge(ids) => { - let trees = ids.try_map(|id| self.get_tree(RepoPath::root(), id))?; - Ok(MergedTree::new(trees)) - } - } + pub fn get_root_tree(self: &Arc, ids: &MergedTreeId) -> BackendResult { + let trees = ids.try_map(|id| self.get_tree(RepoPath::root(), id))?; + Ok(MergedTree::new(trees)) } pub async fn write_tree( diff --git a/lib/tests/test_local_working_copy.rs b/lib/tests/test_local_working_copy.rs index d6fae2b4ca..3225fa03d6 100644 --- a/lib/tests/test_local_working_copy.rs +++ b/lib/tests/test_local_working_copy.rs @@ -417,7 +417,7 @@ fn test_tree_builder_file_directory_transition() { let workspace_root = ws.workspace_root().to_owned(); let mut check_out_tree = |tree_id: &TreeId| { let tree = repo.store().get_tree(RepoPath::root(), tree_id).unwrap(); - let commit = commit_with_tree(repo.store(), MergedTreeId::Legacy(tree.id().clone())); + let commit = commit_with_tree(repo.store(), MergedTreeId::resolved(tree.id().clone())); ws.check_out(repo.op_id().clone(), None, &commit).unwrap(); }; @@ -1121,7 +1121,7 @@ fn test_gitsubmodule() { TreeValue::GitSubmodule(submodule_id), ); - let tree_id = MergedTreeId::Legacy(tree_builder.write_tree().unwrap()); + let tree_id = MergedTreeId::resolved(tree_builder.write_tree().unwrap()); let tree = store.get_root_tree(&tree_id).unwrap(); let commit = commit_with_tree(repo.store(), tree.id()); let ws = &mut test_workspace.workspace; diff --git a/lib/tests/test_merged_tree.rs b/lib/tests/test_merged_tree.rs index 8ef86e88b7..808f031880 100644 --- a/lib/tests/test_merged_tree.rs +++ b/lib/tests/test_merged_tree.rs @@ -248,7 +248,7 @@ fn test_from_legacy_tree() { .take(5) .cloned() .collect(); - let empty_merged_id = MergedTreeId::Merge(empty_merged_id_builder.build()); + let empty_merged_id = empty_merged_id_builder.build(); let mut tree_builder = MergedTreeBuilder::new(empty_merged_id); for (path, value) in merged_tree.entries() { tree_builder.set_or_remove(path, value.unwrap()); @@ -258,9 +258,7 @@ fn test_from_legacy_tree() { // Create the merged tree by adding the same (variable-arity) entries as we // added to the single-tree TreeBuilder. - let mut tree_builder = MergedTreeBuilder::new(MergedTreeId::Merge(Merge::resolved( - store.empty_tree_id().clone(), - ))); + let mut tree_builder = MergedTreeBuilder::new(Merge::resolved(store.empty_tree_id().clone())); // Add the entries out of order, so we test both increasing and reducing the // arity (going up from 1-way to 3-way to 5-way, then to 3-way again) tree_builder.set_or_remove(file1_path.to_owned(), Merge::normal(file_value(&file1_id))); @@ -289,10 +287,10 @@ fn test_merged_tree_builder_resolves_conflict() { let tree2 = create_single_tree(repo, &[(&path1, "bar")]); let tree3 = create_single_tree(repo, &[(&path1, "bar")]); - let base_tree_id = MergedTreeId::Merge(Merge::from_removes_adds( + let base_tree_id = Merge::from_removes_adds( [tree1.id().clone()], [tree2.id().clone(), tree3.id().clone()], - )); + ); let tree_builder = MergedTreeBuilder::new(base_tree_id); let tree_id = tree_builder.write_tree(store).unwrap(); assert_eq!(tree_id, MergedTreeId::resolved(tree2.id().clone())); diff --git a/lib/testutils/src/lib.rs b/lib/testutils/src/lib.rs index c9fe5f9964..789eb9e798 100644 --- a/lib/testutils/src/lib.rs +++ b/lib/testutils/src/lib.rs @@ -417,11 +417,7 @@ pub fn dump_tree(store: &Arc, tree_id: &MergedTreeId) -> String { writeln!( &mut buf, "tree {}", - tree_id - .to_merge() - .iter() - .map(|tree_id| tree_id.hex()) - .join("&") + tree_id.iter().map(|tree_id| tree_id.hex()).join("&") ) .unwrap(); let tree = store.get_root_tree(tree_id).unwrap();