diff --git a/CHANGELOG.md b/CHANGELOG.md index 96f5bdf0b..e57feb752 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 * `jj git push` will search `@-` for branches to push if `@` has none. +### Fixed bugs + +* `jj edit root` now fails gracefully. + ## [0.5.1] - 2022-10-17 No changes (just trying to get automated GitHub release to work). diff --git a/lib/src/repo.rs b/lib/src/repo.rs index f39baa37f..0a4c17c15 100644 --- a/lib/src/repo.rs +++ b/lib/src/repo.rs @@ -606,8 +606,16 @@ impl MutableRepo { Ok(rebaser.rebased().len()) } - pub fn set_wc_commit(&mut self, workspace_id: WorkspaceId, commit_id: CommitId) { + pub fn set_wc_commit( + &mut self, + workspace_id: WorkspaceId, + commit_id: CommitId, + ) -> Result<(), RewriteRootCommit> { + if &commit_id == self.store().root_commit_id() { + return Err(RewriteRootCommit); + } self.view_mut().set_wc_commit(workspace_id, commit_id); + Ok(()) } pub fn remove_wc_commit(&mut self, workspace_id: &WorkspaceId) { @@ -624,13 +632,18 @@ impl MutableRepo { let open_commit = CommitBuilder::for_open_commit(settings, commit.id().clone(), commit.tree_id().clone()) .write_to_repo(self); - self.set_wc_commit(workspace_id, open_commit.id().clone()); + self.set_wc_commit(workspace_id, open_commit.id().clone()) + .unwrap(); open_commit } - pub fn edit(&mut self, workspace_id: WorkspaceId, commit: &Commit) { + pub fn edit( + &mut self, + workspace_id: WorkspaceId, + commit: &Commit, + ) -> Result<(), RewriteRootCommit> { self.leave_commit(&workspace_id); - self.set_wc_commit(workspace_id, commit.id().clone()); + self.set_wc_commit(workspace_id, commit.id().clone()) } fn leave_commit(&mut self, workspace_id: &WorkspaceId) { @@ -965,3 +978,8 @@ impl MutableRepo { ); } } + +/// Error from attempts to check out the root commit for editing +#[derive(Debug, Copy, Clone, Error)] +#[error("Cannot rewrite the root commit")] +pub struct RewriteRootCommit; diff --git a/lib/src/rewrite.rs b/lib/src/rewrite.rs index c7a82a051..a40e38eea 100644 --- a/lib/src/rewrite.rs +++ b/lib/src/rewrite.rs @@ -356,7 +356,7 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> { .write_to_repo(self.mut_repo) }; for workspace_id in workspaces_to_update.into_iter() { - self.mut_repo.edit(workspace_id, &new_wc_commit); + self.mut_repo.edit(workspace_id, &new_wc_commit).unwrap(); } Ok(()) } diff --git a/lib/tests/test_mut_repo.rs b/lib/tests/test_mut_repo.rs index 043977959..5a281cf40 100644 --- a/lib/tests/test_mut_repo.rs +++ b/lib/tests/test_mut_repo.rs @@ -38,7 +38,7 @@ fn test_edit(use_git: bool) { let mut tx = repo.start_transaction("test"); let ws_id = WorkspaceId::default(); - tx.mut_repo().edit(ws_id.clone(), &wc_commit); + tx.mut_repo().edit(ws_id.clone(), &wc_commit).unwrap(); let repo = tx.commit(); assert_eq!(repo.view().get_wc_commit_id(&ws_id), Some(wc_commit.id())); } @@ -88,7 +88,7 @@ fn test_checkout_previous_not_empty(use_git: bool) { .set_open(true) .write_to_repo(mut_repo); let ws_id = WorkspaceId::default(); - mut_repo.edit(ws_id.clone(), &old_checkout); + mut_repo.edit(ws_id.clone(), &old_checkout).unwrap(); let repo = tx.commit(); let mut tx = repo.start_transaction("test"); @@ -96,7 +96,7 @@ fn test_checkout_previous_not_empty(use_git: bool) { let new_checkout = testutils::create_random_commit(&settings, &repo) .set_open(true) .write_to_repo(mut_repo); - mut_repo.edit(ws_id, &new_checkout); + mut_repo.edit(ws_id, &new_checkout).unwrap(); mut_repo.rebase_descendants(&settings).unwrap(); assert!(mut_repo.view().heads().contains(old_checkout.id())); } @@ -119,7 +119,7 @@ fn test_checkout_previous_empty(use_git: bool) { ) .write_to_repo(mut_repo); let ws_id = WorkspaceId::default(); - mut_repo.edit(ws_id.clone(), &old_checkout); + mut_repo.edit(ws_id.clone(), &old_checkout).unwrap(); let repo = tx.commit(); let mut tx = repo.start_transaction("test"); @@ -127,7 +127,7 @@ fn test_checkout_previous_empty(use_git: bool) { let new_wc_commit = testutils::create_random_commit(&settings, &repo) .set_open(true) .write_to_repo(mut_repo); - mut_repo.edit(ws_id, &new_wc_commit); + mut_repo.edit(ws_id, &new_wc_commit).unwrap(); mut_repo.rebase_descendants(&settings).unwrap(); assert!(!mut_repo.view().heads().contains(old_checkout.id())); } @@ -151,7 +151,7 @@ fn test_checkout_previous_empty_with_description(use_git: bool) { .set_description("not empty".to_string()) .write_to_repo(mut_repo); let ws_id = WorkspaceId::default(); - mut_repo.edit(ws_id.clone(), &old_checkout); + mut_repo.edit(ws_id.clone(), &old_checkout).unwrap(); let repo = tx.commit(); let mut tx = repo.start_transaction("test"); @@ -159,7 +159,7 @@ fn test_checkout_previous_empty_with_description(use_git: bool) { let new_checkout = testutils::create_random_commit(&settings, &repo) .set_open(true) .write_to_repo(mut_repo); - mut_repo.edit(ws_id, &new_checkout); + mut_repo.edit(ws_id, &new_checkout).unwrap(); mut_repo.rebase_descendants(&settings).unwrap(); assert!(mut_repo.view().heads().contains(old_checkout.id())); } @@ -188,7 +188,7 @@ fn test_checkout_previous_empty_non_head(use_git: bool) { ) .write_to_repo(mut_repo); let ws_id = WorkspaceId::default(); - mut_repo.edit(ws_id.clone(), &old_checkout); + mut_repo.edit(ws_id.clone(), &old_checkout).unwrap(); let repo = tx.commit(); let mut tx = repo.start_transaction("test"); @@ -196,7 +196,7 @@ fn test_checkout_previous_empty_non_head(use_git: bool) { let new_checkout = testutils::create_random_commit(&settings, &repo) .set_open(true) .write_to_repo(mut_repo); - mut_repo.edit(ws_id, &new_checkout); + mut_repo.edit(ws_id, &new_checkout).unwrap(); mut_repo.rebase_descendants(&settings).unwrap(); assert_eq!( *mut_repo.view().heads(), @@ -221,7 +221,7 @@ fn test_edit_initial(use_git: bool) { let mut tx = repo.start_transaction("test"); let workspace_id = WorkspaceId::new("new-workspace".to_string()); - tx.mut_repo().edit(workspace_id.clone(), &checkout); + tx.mut_repo().edit(workspace_id.clone(), &checkout).unwrap(); let repo = tx.commit(); assert_eq!( repo.view().get_wc_commit_id(&workspace_id), @@ -468,7 +468,9 @@ fn test_has_changed(use_git: bool) { mut_repo.remove_head(commit2.id()); mut_repo.add_public_head(&commit1); let ws_id = WorkspaceId::default(); - mut_repo.set_wc_commit(ws_id.clone(), commit1.id().clone()); + mut_repo + .set_wc_commit(ws_id.clone(), commit1.id().clone()) + .unwrap(); mut_repo.set_local_branch("main".to_string(), RefTarget::Normal(commit1.id().clone())); mut_repo.set_remote_branch( "main".to_string(), @@ -485,7 +487,9 @@ fn test_has_changed(use_git: bool) { mut_repo.add_public_head(&commit1); mut_repo.add_head(&commit1); - mut_repo.set_wc_commit(ws_id.clone(), commit1.id().clone()); + mut_repo + .set_wc_commit(ws_id.clone(), commit1.id().clone()) + .unwrap(); mut_repo.set_local_branch("main".to_string(), RefTarget::Normal(commit1.id().clone())); mut_repo.set_remote_branch( "main".to_string(), @@ -515,9 +519,11 @@ fn test_has_changed(use_git: bool) { mut_repo.remove_head(commit2.id()); assert!(!mut_repo.has_changes()); - mut_repo.set_wc_commit(ws_id.clone(), commit2.id().clone()); + mut_repo + .set_wc_commit(ws_id.clone(), commit2.id().clone()) + .unwrap(); assert!(mut_repo.has_changes()); - mut_repo.set_wc_commit(ws_id, commit1.id().clone()); + mut_repo.set_wc_commit(ws_id, commit1.id().clone()).unwrap(); assert!(!mut_repo.has_changes()); mut_repo.set_local_branch("main".to_string(), RefTarget::Normal(commit2.id().clone())); diff --git a/lib/tests/test_revset.rs b/lib/tests/test_revset.rs index 1cc76a63f..2dc190924 100644 --- a/lib/tests/test_revset.rs +++ b/lib/tests/test_revset.rs @@ -266,8 +266,10 @@ fn test_resolve_symbol_checkout(use_git: bool) { ); // Add some workspaces - mut_repo.set_wc_commit(ws1.clone(), commit1.id().clone()); - mut_repo.set_wc_commit(ws2, commit2.id().clone()); + mut_repo + .set_wc_commit(ws1.clone(), commit1.id().clone()) + .unwrap(); + mut_repo.set_wc_commit(ws2, commit2.id().clone()).unwrap(); // @ cannot be resolved without a default workspace ID assert_eq!( resolve_symbol(mut_repo.as_repo_ref(), "@", None), @@ -390,7 +392,9 @@ fn test_resolve_symbol_git_refs() { // Cannot shadow checkout ("@") or root symbols let ws_id = WorkspaceId::default(); - mut_repo.set_wc_commit(ws_id.clone(), commit1.id().clone()); + mut_repo + .set_wc_commit(ws_id.clone(), commit1.id().clone()) + .unwrap(); mut_repo.set_git_ref("@".to_string(), RefTarget::Normal(commit2.id().clone())); mut_repo.set_git_ref("root".to_string(), RefTarget::Normal(commit3.id().clone())); assert_eq!( @@ -457,7 +461,9 @@ fn test_evaluate_expression_root_and_checkout(use_git: bool) { ); // Can find the current checkout - mut_repo.set_wc_commit(WorkspaceId::default(), commit1.id().clone()); + mut_repo + .set_wc_commit(WorkspaceId::default(), commit1.id().clone()) + .unwrap(); assert_eq!( resolve_commit_ids_in_workspace(mut_repo.as_repo_ref(), "@", &WorkspaceId::default()), vec![commit1.id().clone()] @@ -609,7 +615,9 @@ fn test_evaluate_expression_parents(use_git: bool) { assert_eq!(resolve_commit_ids(mut_repo.as_repo_ref(), "root-"), vec![]); // Can find parents of the current checkout - mut_repo.set_wc_commit(WorkspaceId::default(), commit2.id().clone()); + mut_repo + .set_wc_commit(WorkspaceId::default(), commit2.id().clone()) + .unwrap(); assert_eq!( resolve_commit_ids_in_workspace(mut_repo.as_repo_ref(), "@-", &WorkspaceId::default()), vec![commit1.id().clone()] diff --git a/lib/tests/test_rewrite.rs b/lib/tests/test_rewrite.rs index ae7426850..8bd8df14f 100644 --- a/lib/tests/test_rewrite.rs +++ b/lib/tests/test_rewrite.rs @@ -1281,11 +1281,14 @@ fn test_rebase_descendants_update_checkout(use_git: bool) { let ws2_id = WorkspaceId::new("ws2".to_string()); let ws3_id = WorkspaceId::new("ws3".to_string()); tx.mut_repo() - .set_wc_commit(ws1_id.clone(), commit_b.id().clone()); + .set_wc_commit(ws1_id.clone(), commit_b.id().clone()) + .unwrap(); tx.mut_repo() - .set_wc_commit(ws2_id.clone(), commit_b.id().clone()); + .set_wc_commit(ws2_id.clone(), commit_b.id().clone()) + .unwrap(); tx.mut_repo() - .set_wc_commit(ws3_id.clone(), commit_a.id().clone()); + .set_wc_commit(ws3_id.clone(), commit_a.id().clone()) + .unwrap(); let repo = tx.commit(); let mut tx = repo.start_transaction("test"); @@ -1325,11 +1328,14 @@ fn test_rebase_descendants_update_checkout_abandoned(use_git: bool) { let ws2_id = WorkspaceId::new("ws2".to_string()); let ws3_id = WorkspaceId::new("ws3".to_string()); tx.mut_repo() - .set_wc_commit(ws1_id.clone(), commit_b.id().clone()); + .set_wc_commit(ws1_id.clone(), commit_b.id().clone()) + .unwrap(); tx.mut_repo() - .set_wc_commit(ws2_id.clone(), commit_b.id().clone()); + .set_wc_commit(ws2_id.clone(), commit_b.id().clone()) + .unwrap(); tx.mut_repo() - .set_wc_commit(ws3_id.clone(), commit_a.id().clone()); + .set_wc_commit(ws3_id.clone(), commit_a.id().clone()) + .unwrap(); let repo = tx.commit(); let mut tx = repo.start_transaction("test"); @@ -1381,7 +1387,8 @@ fn test_rebase_descendants_update_checkout_abandoned_merge(use_git: bool) { .write_to_repo(tx.mut_repo()); let workspace_id = WorkspaceId::default(); tx.mut_repo() - .set_wc_commit(workspace_id.clone(), commit_d.id().clone()); + .set_wc_commit(workspace_id.clone(), commit_d.id().clone()) + .unwrap(); let repo = tx.commit(); let mut tx = repo.start_transaction("test"); diff --git a/lib/tests/test_view.rs b/lib/tests/test_view.rs index dd3390214..4900df228 100644 --- a/lib/tests/test_view.rs +++ b/lib/tests/test_view.rs @@ -178,43 +178,56 @@ fn test_merge_views_checkout() { let ws7_id = WorkspaceId::new("ws7".to_string()); initial_tx .mut_repo() - .set_wc_commit(ws1_id.clone(), commit1.id().clone()); + .set_wc_commit(ws1_id.clone(), commit1.id().clone()) + .unwrap(); initial_tx .mut_repo() - .set_wc_commit(ws2_id.clone(), commit1.id().clone()); + .set_wc_commit(ws2_id.clone(), commit1.id().clone()) + .unwrap(); initial_tx .mut_repo() - .set_wc_commit(ws3_id.clone(), commit1.id().clone()); + .set_wc_commit(ws3_id.clone(), commit1.id().clone()) + .unwrap(); initial_tx .mut_repo() - .set_wc_commit(ws4_id.clone(), commit1.id().clone()); + .set_wc_commit(ws4_id.clone(), commit1.id().clone()) + .unwrap(); initial_tx .mut_repo() - .set_wc_commit(ws5_id.clone(), commit1.id().clone()); + .set_wc_commit(ws5_id.clone(), commit1.id().clone()) + .unwrap(); let repo = initial_tx.commit(); let mut tx1 = repo.start_transaction("test"); tx1.mut_repo() - .set_wc_commit(ws1_id.clone(), commit2.id().clone()); + .set_wc_commit(ws1_id.clone(), commit2.id().clone()) + .unwrap(); tx1.mut_repo() - .set_wc_commit(ws2_id.clone(), commit2.id().clone()); + .set_wc_commit(ws2_id.clone(), commit2.id().clone()) + .unwrap(); tx1.mut_repo().remove_wc_commit(&ws4_id); tx1.mut_repo() - .set_wc_commit(ws5_id.clone(), commit2.id().clone()); + .set_wc_commit(ws5_id.clone(), commit2.id().clone()) + .unwrap(); tx1.mut_repo() - .set_wc_commit(ws6_id.clone(), commit2.id().clone()); + .set_wc_commit(ws6_id.clone(), commit2.id().clone()) + .unwrap(); tx1.commit(); let mut tx2 = repo.start_transaction("test"); tx2.mut_repo() - .set_wc_commit(ws1_id.clone(), commit3.id().clone()); + .set_wc_commit(ws1_id.clone(), commit3.id().clone()) + .unwrap(); tx2.mut_repo() - .set_wc_commit(ws3_id.clone(), commit3.id().clone()); + .set_wc_commit(ws3_id.clone(), commit3.id().clone()) + .unwrap(); tx2.mut_repo() - .set_wc_commit(ws4_id.clone(), commit3.id().clone()); + .set_wc_commit(ws4_id.clone(), commit3.id().clone()) + .unwrap(); tx2.mut_repo().remove_wc_commit(&ws5_id); tx2.mut_repo() - .set_wc_commit(ws7_id.clone(), commit3.id().clone()); + .set_wc_commit(ws7_id.clone(), commit3.id().clone()) + .unwrap(); // Make sure the end time different, assuming the clock has sub-millisecond // precision. std::thread::sleep(std::time::Duration::from_millis(1)); diff --git a/src/cli_util.rs b/src/cli_util.rs index 5b3e4d129..73bc27a0c 100644 --- a/src/cli_util.rs +++ b/src/cli_util.rs @@ -31,7 +31,7 @@ use jujutsu_lib::matchers::{EverythingMatcher, Matcher, PrefixMatcher, Visit}; use jujutsu_lib::op_heads_store::{OpHeadResolutionError, OpHeads, OpHeadsStore}; use jujutsu_lib::op_store::{OpStore, OpStoreError, OperationId, WorkspaceId}; use jujutsu_lib::operation::Operation; -use jujutsu_lib::repo::{BackendFactories, MutableRepo, ReadonlyRepo, RepoRef}; +use jujutsu_lib::repo::{BackendFactories, MutableRepo, ReadonlyRepo, RepoRef, RewriteRootCommit}; use jujutsu_lib::repo_path::RepoPath; use jujutsu_lib::revset::{RevsetError, RevsetParseError}; use jujutsu_lib::settings::UserSettings; @@ -76,6 +76,12 @@ impl From for CommandError { } } +impl From for CommandError { + fn from(err: RewriteRootCommit) -> Self { + CommandError::UserError(err.to_string()) + } +} + impl From for CommandError { fn from(err: BackendError) -> Self { CommandError::UserError(format!("Unexpected error from store: {err}")) @@ -665,7 +671,9 @@ impl WorkspaceCommandHelper { let commit = CommitBuilder::for_rewrite_from(&self.settings, &wc_commit) .set_tree(new_tree_id) .write_to_repo(mut_repo); - mut_repo.set_wc_commit(workspace_id, commit.id().clone()); + mut_repo + .set_wc_commit(workspace_id, commit.id().clone()) + .unwrap(); // Rebase descendants let num_rebased = mut_repo.rebase_descendants(&self.settings)?; diff --git a/src/commands.rs b/src/commands.rs index 5e3e264ec..bfa225738 100644 --- a/src/commands.rs +++ b/src/commands.rs @@ -1147,7 +1147,8 @@ fn cmd_checkout( let mut tx = workspace_command .start_transaction(&format!("check out commit {}", target.id().hex())); if target.is_open() { - tx.mut_repo().edit(workspace_id, &target); + // Root is never open + tx.mut_repo().edit(workspace_id, &target).unwrap(); } else { let commit_builder = CommitBuilder::for_open_commit( ui.settings(), @@ -1156,7 +1157,7 @@ fn cmd_checkout( ) .set_description(args.message.clone()); let new_commit = commit_builder.write_to_repo(tx.mut_repo()); - tx.mut_repo().edit(workspace_id, &new_commit); + tx.mut_repo().edit(workspace_id, &new_commit).unwrap(); } workspace_command.finish_transaction(ui, tx)?; } @@ -1170,7 +1171,7 @@ fn cmd_checkout( ) .set_description(args.message.clone()); let new_commit = commit_builder.write_to_repo(tx.mut_repo()); - tx.mut_repo().edit(workspace_id, &new_commit); + tx.mut_repo().edit(workspace_id, &new_commit).unwrap(); workspace_command.finish_transaction(ui, tx)?; } Ok(()) @@ -2479,7 +2480,7 @@ fn cmd_close(ui: &mut Ui, command: &CommandHelper, args: &CloseArgs) -> Result<( ) .write_to_repo(tx.mut_repo()); for workspace_id in workspace_ids { - tx.mut_repo().edit(workspace_id, &new_checkout); + tx.mut_repo().edit(workspace_id, &new_checkout).unwrap(); } } workspace_command.finish_transaction(ui, tx)?; @@ -2569,7 +2570,7 @@ fn cmd_edit(ui: &mut Ui, command: &CommandHelper, args: &EditArgs) -> Result<(), } else { let mut tx = workspace_command.start_transaction(&format!("edit commit {}", new_commit.id().hex())); - tx.mut_repo().edit(workspace_id, &new_commit); + tx.mut_repo().edit(workspace_id, &new_commit)?; workspace_command.finish_transaction(ui, tx)?; } Ok(()) @@ -2591,7 +2592,7 @@ fn cmd_new(ui: &mut Ui, command: &CommandHelper, args: &NewArgs) -> Result<(), C .set_open(true) .write_to_repo(tx.mut_repo()); let workspace_id = workspace_command.workspace_id(); - tx.mut_repo().edit(workspace_id, &new_commit); + tx.mut_repo().edit(workspace_id, &new_commit).unwrap(); workspace_command.finish_transaction(ui, tx)?; Ok(()) } diff --git a/tests/test_edit_command.rs b/tests/test_edit_command.rs index dc06640d3..15cc4bad0 100644 --- a/tests/test_edit_command.rs +++ b/tests/test_edit_command.rs @@ -68,3 +68,12 @@ fn get_log_output(test_env: &TestEnvironment, cwd: &Path) -> String { ], ) } + +#[test] +fn test_edit_root() { + let test_env = TestEnvironment::default(); + test_env.jj_cmd_success(test_env.env_root(), &["init", "repo", "--git"]); + let repo_path = test_env.env_root().join("repo"); + let stderr = test_env.jj_cmd_failure(&repo_path, &["edit", "root"]); + insta::assert_snapshot!(stderr, @"Error: Cannot rewrite the root commit"); +}