From e901a4e66b7524cd9be74e499592f2556e9c4cc7 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Wed, 3 Nov 2021 22:54:59 -0700 Subject: [PATCH] repo: don't materialize conflicts on checkout Since the working copy can now handle conflicts, we don't need to materialize conflicts when checking out a commit. Before this patch, we used to create a new commit on top whenever we checked out a commit with conflicts. That new commit was intended just for resolving the conflicts. The typical workflow was the resolve the conflicts and then amend. To use the same workflow after this patch, one needs to explicitly create a new commit on top with `jj new` after checking out a commit with conflict. --- lib/src/repo.rs | 24 ++------ lib/tests/test_mut_repo.rs | 112 ------------------------------------- 2 files changed, 5 insertions(+), 131 deletions(-) diff --git a/lib/src/repo.rs b/lib/src/repo.rs index 0e17ba23d..d2d80242f 100644 --- a/lib/src/repo.rs +++ b/lib/src/repo.rs @@ -38,7 +38,7 @@ use crate::store::Store; use crate::transaction::Transaction; use crate::view::{RefName, View}; use crate::working_copy::WorkingCopy; -use crate::{backend, conflicts, op_store}; +use crate::{backend, op_store}; #[derive(Debug, Error, PartialEq, Eq)] pub enum RepoError { @@ -601,34 +601,20 @@ impl MutableRepo { assert!(current_checkout.is_open(), "current checkout is closed"); if current_checkout.is_empty() { // Abandon the checkout we're leaving if it's empty. - // TODO: Also abandon it if the only changes are conflicts that got - // materialized. self.record_abandoned_commit(current_checkout_id); } - let store = self.store(); - // Create a new tree with any conflicts resolved. - let mut tree_builder = store.tree_builder(commit.tree().id().clone()); - for (path, conflict_id) in commit.tree().conflicts() { - let conflict = store.read_conflict(&conflict_id).unwrap(); - let materialized_value = - conflicts::conflict_to_materialized_value(store, &path, &conflict); - tree_builder.set(path, materialized_value); - } - let tree_id = tree_builder.write_tree(); let open_commit; - if !commit.is_open() || &tree_id != commit.tree().id() { - // If the commit is closed, or if it had conflicts, create a new open commit on - // top + if !commit.is_open() { + // If the commit is closed, create a new open commit on top open_commit = CommitBuilder::for_open_commit( settings, self.store(), commit.id().clone(), - tree_id, + commit.tree().id().clone(), ) .write_to_repo(self); } else { - // Otherwise the commit was open and didn't have any conflicts, so just use - // that commit as is. + // Otherwise the commit was open, so just use that commit as is. open_commit = commit.clone(); } let id = open_commit.id().clone(); diff --git a/lib/tests/test_mut_repo.rs b/lib/tests/test_mut_repo.rs index e6e5615f4..b0cb3e78f 100644 --- a/lib/tests/test_mut_repo.rs +++ b/lib/tests/test_mut_repo.rs @@ -12,12 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -use std::sync::Arc; - -use jujutsu_lib::backend::{Conflict, ConflictId, ConflictPart, TreeValue}; use jujutsu_lib::commit_builder::CommitBuilder; -use jujutsu_lib::repo_path::RepoPath; -use jujutsu_lib::store::Store; use jujutsu_lib::testutils; use jujutsu_lib::testutils::{assert_rebased, CommitGraphBuilder}; use test_case::test_case; @@ -68,113 +63,6 @@ fn test_checkout_closed(use_git: bool) { assert_eq!(repo.view().checkout(), actual_checkout.id()); } -#[test_case(false ; "local backend")] -// #[test_case(true ; "git backend")] -fn test_checkout_open_with_conflict(use_git: bool) { - // Test that MutableRepo::check_out() creates a child if the requested - // commit is open and has conflicts - let settings = testutils::user_settings(); - let (_temp_dir, repo) = testutils::init_repo(&settings, use_git); - let store = repo.store(); - - let file_path = RepoPath::from_internal_string("file"); - let conflict_id = write_conflict(store, &file_path); - let mut tree_builder = repo - .store() - .tree_builder(repo.store().empty_tree_id().clone()); - tree_builder.set(file_path.clone(), TreeValue::Conflict(conflict_id)); - let tree_id = tree_builder.write_tree(); - - let mut tx = repo.start_transaction("test"); - let requested_checkout = CommitBuilder::for_new_commit(&settings, store, tree_id) - .set_open(true) - .write_to_repo(tx.mut_repo()); - let repo = tx.commit(); - - let mut tx = repo.start_transaction("test"); - let actual_checkout = tx.mut_repo().check_out(&settings, &requested_checkout); - let file_value = actual_checkout.tree().path_value(&file_path); - match file_value { - Some(TreeValue::Normal { - id: _, - executable: false, - }) => {} - _ => panic!("unexpected tree value: {:?}", file_value), - } - assert_eq!(actual_checkout.parents().len(), 1); - assert_eq!(actual_checkout.parents()[0].id(), requested_checkout.id()); - let repo = tx.commit(); - assert_eq!(repo.view().checkout(), actual_checkout.id()); -} - -#[test_case(false ; "local backend")] -// #[test_case(true ; "git backend")] -fn test_checkout_closed_with_conflict(use_git: bool) { - // Test that MutableRepo::check_out() creates a child if the requested commit is - // closed and has conflicts - let settings = testutils::user_settings(); - let (_temp_dir, repo) = testutils::init_repo(&settings, use_git); - let store = repo.store(); - - let file_path = RepoPath::from_internal_string("file"); - let conflict_id = write_conflict(store, &file_path); - let mut tree_builder = repo - .store() - .tree_builder(repo.store().empty_tree_id().clone()); - tree_builder.set(file_path.clone(), TreeValue::Conflict(conflict_id)); - let tree_id = tree_builder.write_tree(); - - let mut tx = repo.start_transaction("test"); - let requested_checkout = CommitBuilder::for_new_commit(&settings, store, tree_id) - .set_open(false) - .write_to_repo(tx.mut_repo()); - let repo = tx.commit(); - - let mut tx = repo.start_transaction("test"); - let actual_checkout = tx.mut_repo().check_out(&settings, &requested_checkout); - let file_value = actual_checkout.tree().path_value(&file_path); - match file_value { - Some(TreeValue::Normal { - id: _, - executable: false, - }) => {} - _ => panic!("unexpected tree value: {:?}", file_value), - } - assert_eq!(actual_checkout.parents().len(), 1); - assert_eq!(actual_checkout.parents()[0].id(), requested_checkout.id()); - let repo = tx.commit(); - assert_eq!(repo.view().checkout(), actual_checkout.id()); -} - -fn write_conflict(store: &Arc, file_path: &RepoPath) -> ConflictId { - let file_id1 = testutils::write_file(store, file_path, "a\n"); - let file_id2 = testutils::write_file(store, file_path, "b\n"); - let file_id3 = testutils::write_file(store, file_path, "c\n"); - let conflict = Conflict { - removes: vec![ConflictPart { - value: TreeValue::Normal { - id: file_id1, - executable: false, - }, - }], - adds: vec![ - ConflictPart { - value: TreeValue::Normal { - id: file_id2, - executable: false, - }, - }, - ConflictPart { - value: TreeValue::Normal { - id: file_id3, - executable: false, - }, - }, - ], - }; - store.write_conflict(&conflict).unwrap() -} - #[test_case(false ; "local backend")] // #[test_case(true ; "git backend")] fn test_checkout_previous_not_empty(use_git: bool) {