From 034fbb47e3509ae12759156647dd80eac9f8f71b Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Mon, 28 Mar 2022 06:46:21 -0700 Subject: [PATCH] cli: fix crash when initializing workspace colocated with Git repo When initializing a workspace that shares its working copy with a Git repo (i.e. `jj init --git-repo=.`), we import refs and HEAD when creating the `WorkspaceCommandHelper` (as we do for all commands when the working copy is shared). That makes the explicit import we do in `cmd_init()` unnecessary. It also makes the checkout of HEAD I added for the fix of #102 unnecessary. More importantly, as @yuja reported in #177, it makes the command crash (at least if the repo is small enough that the two checkouts happen within a second). I think the problem is that the second checkout tries to create the same commit except that the Change ID is different (the problem is not the predecessors as I speculated in the issue tracker). The fix is to simply avoid doing the redundant work. We still need a proper fix for #27 eventually. Closes #177. --- CHANGELOG.md | 2 ++ src/commands.rs | 27 +++++++-------- tests/test_git_colocated.rs | 1 + tests/test_init_command.rs | 65 ++++++++++++++++++++++--------------- 4 files changed, 56 insertions(+), 39 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5bffba064..5771d75bd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -43,6 +43,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed bugs +* Fixed crash on `jj init --git-repo=.` (it almost always crashed). + * When sharing the working copy with a Git repo, the automatic importing and exporting (sometimes?) didn't happen on Windows. diff --git a/src/commands.rs b/src/commands.rs index 39d27560c..b926fc467 100644 --- a/src/commands.rs +++ b/src/commands.rs @@ -1741,19 +1741,20 @@ fn cmd_init(ui: &mut Ui, command: &CommandHelper, args: &InitArgs) -> Result<(), let mut workspace_command = command.for_loaded_repo(ui, workspace, repo)?; if workspace_command.working_copy_shared_with_git() { add_to_git_exclude(ui, &git_repo)?; - } - let mut tx = workspace_command.start_transaction("import git refs"); - git::import_refs(tx.mut_repo(), &git_repo)?; - if let Some(git_head_id) = tx.mut_repo().view().git_head() { - let git_head_commit = tx.mut_repo().store().get_commit(&git_head_id)?; - tx.mut_repo().check_out( - workspace_command.workspace_id(), - ui.settings(), - &git_head_commit, - ); - } - if tx.mut_repo().has_changes() { - workspace_command.finish_transaction(ui, tx)?; + } else { + let mut tx = workspace_command.start_transaction("import git refs"); + git::import_refs(tx.mut_repo(), &git_repo)?; + if let Some(git_head_id) = tx.mut_repo().view().git_head() { + let git_head_commit = tx.mut_repo().store().get_commit(&git_head_id)?; + tx.mut_repo().check_out( + workspace_command.workspace_id(), + ui.settings(), + &git_head_commit, + ); + } + if tx.mut_repo().has_changes() { + workspace_command.finish_transaction(ui, tx)?; + } } } else if args.git { Workspace::init_internal_git(ui.settings(), wc_path.clone())?; diff --git a/tests/test_git_colocated.rs b/tests/test_git_colocated.rs index bffdd09b5..191143ddd 100644 --- a/tests/test_git_colocated.rs +++ b/tests/test_git_colocated.rs @@ -24,6 +24,7 @@ fn test_git_colocated() { // Create a commit from jj and check that it's reflected in git std::fs::write(workspace_root.join("new-file"), "contents").unwrap(); test_env.jj_cmd_success(&workspace_root, &["close", "-m", "add a file"]); + test_env.jj_cmd_success(&workspace_root, &["git", "import"]); let stdout = test_env.jj_cmd_success(&workspace_root, &["log", "-T", "commit_id \" \" branches"]); insta::assert_snapshot!(stdout, @r###" diff --git a/tests/test_init_command.rs b/tests/test_init_command.rs index b4c560478..9a2eee021 100644 --- a/tests/test_init_command.rs +++ b/tests/test_init_command.rs @@ -12,8 +12,38 @@ // See the License for the specific language governing permissions and // limitations under the License. +use std::path::PathBuf; + use jujutsu::testutils::TestEnvironment; +fn init_git_repo(git_repo_path: &PathBuf) { + let git_repo = git2::Repository::init(&git_repo_path).unwrap(); + let git_blob_oid = git_repo.blob(b"some content").unwrap(); + let mut git_tree_builder = git_repo.treebuilder(None).unwrap(); + git_tree_builder + .insert("some-file", git_blob_oid, 0o100644) + .unwrap(); + let git_tree_id = git_tree_builder.write().unwrap(); + let git_tree = git_repo.find_tree(git_tree_id).unwrap(); + let git_signature = git2::Signature::new( + "Git User", + "git.user@example.com", + &git2::Time::new(123, 60), + ) + .unwrap(); + git_repo + .commit( + Some("refs/heads/my-branch"), + &git_signature, + &git_signature, + "My commit message", + &git_tree, + &[], + ) + .unwrap(); + git_repo.set_head("refs/heads/my-branch").unwrap(); +} + #[test] fn test_init_git_internal() { let test_env = TestEnvironment::default(); @@ -40,31 +70,7 @@ fn test_init_git_internal() { fn test_init_git_external() { let test_env = TestEnvironment::default(); let git_repo_path = test_env.env_root().join("git-repo"); - let git_repo = git2::Repository::init(&git_repo_path).unwrap(); - let git_blob_oid = git_repo.blob(b"some content").unwrap(); - let mut git_tree_builder = git_repo.treebuilder(None).unwrap(); - git_tree_builder - .insert("some-file", git_blob_oid, 0o100644) - .unwrap(); - let git_tree_id = git_tree_builder.write().unwrap(); - let git_tree = git_repo.find_tree(git_tree_id).unwrap(); - let git_signature = git2::Signature::new( - "Git User", - "git.user@example.com", - &git2::Time::new(123, 60), - ) - .unwrap(); - git_repo - .commit( - Some("refs/heads/my-branch"), - &git_signature, - &git_signature, - "My commit message", - &git_tree, - &[], - ) - .unwrap(); - git_repo.set_head("refs/heads/my-branch").unwrap(); + init_git_repo(&git_repo_path); let stdout = test_env.jj_cmd_success( test_env.env_root(), @@ -107,7 +113,7 @@ fn test_init_git_external() { fn test_init_git_colocated() { let test_env = TestEnvironment::default(); let workspace_root = test_env.env_root().join("repo"); - git2::Repository::init(&workspace_root).unwrap(); + init_git_repo(&workspace_root); let stdout = test_env.jj_cmd_success(&workspace_root, &["init", "--git-repo", "."]); // TODO: We should say "." instead of "" here insta::assert_snapshot!(stdout, @r###"Initialized repo in "" @@ -125,6 +131,13 @@ fn test_init_git_colocated() { assert!(git_target_file_contents .replace('\\', "/") .ends_with("../../../.git")); + + // Check that the Git repo's HEAD got checked out + let stdout = test_env.jj_cmd_success(&repo_path, &["log", "-r", "@-"]); + insta::assert_snapshot!(stdout, @r###" + o 8d698d4a8ee1 d3866db7e30a git.user@example.com 1970-01-01 01:02:03.000 +01:00 my-branch HEAD@git + ~ My commit message + "###); } #[test]