Skip to content

Commit

Permalink
cli: fix crash when initializing workspace colocated with Git repo
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
martinvonz committed Mar 31, 2022
1 parent 9568666 commit 9e3867a
Show file tree
Hide file tree
Showing 4 changed files with 56 additions and 39 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Expand Down
27 changes: 14 additions & 13 deletions src/commands.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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())?;
Expand Down
1 change: 1 addition & 0 deletions tests/test_git_colocated.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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###"
Expand Down
65 changes: 39 additions & 26 deletions tests/test_init_command.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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",
"[email protected]",
&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();
Expand All @@ -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",
"[email protected]",
&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(),
Expand Down Expand Up @@ -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 ""
Expand All @@ -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 [email protected] 1970-01-01 01:02:03.000 +01:00 my-branch HEAD@git
~ My commit message
"###);
}

#[test]
Expand Down

0 comments on commit 9e3867a

Please sign in to comment.