Fix jj init --git-repo fails and leaves broken .jj folder

This commit fixes #1305

Before this commit, running `jj init --git-repo=./` in a folder that
does not have a .git would cause jj to panick and leave an unfinished corrupted jj repo.

This commit fixes that by changing the call chain to return an error
instead of calling .unwrap() and panicking. This commit also adds logic to delete the unfinished jj
repository when the git backend initialization failed.

Before this commit, running the above command would result in the following
```
Running `jj/target/debug/jj init --git-repo=./`
thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Error { code: -3, klass: 2, message: "failed to resolve path '/Users/kevincliao/github/jj/test-repo/.jj/repo/store/../../../.git': No such file or directory" }', lib/src/git_backend.rs:83:75
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
```

After this commit, the result is the following and the jj repo is deleted:
```
Running `jj/target/debug/jj init --git-repo=./`
Error: Failed to access the repository: Error: Failed to open git repository: failed to resolve path '/Users/kevincliao/github/jj/test-repo/.jj/repo/store/../../../.git': No such file or directory; class=Os (2); code=NotFound (-3)
```

examples/custom-backend/main.rs

@@ -54,7 +54,7 @@ fn run_custom_command(
             let wc_path = command_helper.cwd();
             // Initialize a workspace with the custom backend
             Workspace::init_with_backend(command_helper.settings(), wc_path, |store_path| {
-                Box::new(JitBackend::init(store_path))
+                Ok(Box::new(JitBackend::init(store_path)))
             })?;
             Ok(())
         }

lib/src/git_backend.rs

@@ -73,16 +73,17 @@ impl GitBackend {
         GitBackend::new(git_repo, extra_metadata_store)
     }
 
-    pub fn init_external(store_path: &Path, git_repo_path: &Path) -> Self {
+    pub fn init_external(store_path: &Path, git_repo_path: &Path) -> Result<Self, BackendError> {
         let extra_path = store_path.join("extra");
         std::fs::create_dir(&extra_path).unwrap();
         let mut git_target_file = File::create(store_path.join("git_target")).unwrap();
         git_target_file
             .write_all(git_repo_path.to_str().unwrap().as_bytes())
             .unwrap();
-        let repo = git2::Repository::open(store_path.join(git_repo_path)).unwrap();
+        let repo = git2::Repository::open(store_path.join(git_repo_path))
+            .map_err(|err| BackendError::Other(format!("Failed to open git repository: {err}")))?;
         let extra_metadata_store = TableStore::init(extra_path, HASH_LENGTH);
-        GitBackend::new(repo, extra_metadata_store)
+        Ok(GitBackend::new(repo, extra_metadata_store))
     }
 
     pub fn load(store_path: &Path) -> Self {
@@ -769,7 +770,7 @@ mod tests {
         // Check that the git commit above got the hash we expect
         assert_eq!(git_commit_id.as_bytes(), commit_id.as_bytes());
 
-        let store = GitBackend::init_external(store_path, &git_repo_path);
+        let store = GitBackend::init_external(store_path, &git_repo_path).unwrap();
         let commit = store.read_commit(&commit_id).unwrap();
         assert_eq!(&commit.change_id, &change_id);
         assert_eq!(commit.parents, vec![CommitId::from_bytes(&[0; 20])]);
@@ -839,7 +840,7 @@ mod tests {
         let git_repo_path = temp_dir.path().join("git");
         let git_repo = git2::Repository::init(&git_repo_path).unwrap();
 
-        let backend = GitBackend::init_external(store_path, &git_repo_path);
+        let backend = GitBackend::init_external(store_path, &git_repo_path).unwrap();
         let mut commit = Commit {
             parents: vec![],
             predecessors: vec![],

lib/src/repo.rs

@@ -100,6 +100,14 @@ impl Debug for ReadonlyRepo {
     }
 }
 
+#[derive(Error, Debug)]
+pub enum RepoInitError {
+    #[error(transparent)]
+    Backend(#[from] BackendError),
+    #[error(transparent)]
+    Path(#[from] PathError),
+}
+
 impl ReadonlyRepo {
     pub fn default_op_store_factory() -> impl FnOnce(&Path) -> Box<dyn OpStore> {
         |store_path| Box::new(SimpleOpStore::init(store_path))
@@ -123,17 +131,17 @@ impl ReadonlyRepo {
     pub fn init(
         user_settings: &UserSettings,
         repo_path: &Path,
-        backend_factory: impl FnOnce(&Path) -> Box<dyn Backend>,
+        backend_factory: impl FnOnce(&Path) -> Result<Box<dyn Backend>, BackendError>,
         op_store_factory: impl FnOnce(&Path) -> Box<dyn OpStore>,
         op_heads_store_factory: impl FnOnce(&Path) -> Box<dyn OpHeadsStore>,
         index_store_factory: impl FnOnce(&Path) -> Box<dyn IndexStore>,
         submodule_store_factory: impl FnOnce(&Path) -> Box<dyn SubmoduleStore>,
-    ) -> Result<Arc<ReadonlyRepo>, PathError> {
+    ) -> Result<Arc<ReadonlyRepo>, RepoInitError> {
         let repo_path = repo_path.canonicalize().context(repo_path)?;
 
         let store_path = repo_path.join("store");
         fs::create_dir(&store_path).context(&store_path)?;
-        let backend = backend_factory(&store_path);
+        let backend = backend_factory(&store_path)?;
         let backend_path = store_path.join("type");
         fs::write(&backend_path, backend.name()).context(&backend_path)?;
         let store = Store::new(backend);

lib/src/workspace.rs

@@ -19,15 +19,15 @@ use std::sync::Arc;
 
 use thiserror::Error;
 
-use crate::backend::Backend;
+use crate::backend::{Backend, BackendError};
 use crate::git_backend::GitBackend;
 use crate::index::IndexStore;
 use crate::local_backend::LocalBackend;
 use crate::op_heads_store::OpHeadsStore;
 use crate::op_store::{OpStore, WorkspaceId};
 use crate::repo::{
-    CheckOutCommitError, IoResultExt, PathError, ReadonlyRepo, Repo, RepoLoader, StoreFactories,
-    StoreLoadError,
+    CheckOutCommitError, IoResultExt, PathError, ReadonlyRepo, Repo, RepoInitError, RepoLoader,
+    StoreFactories, StoreLoadError,
 };
 use crate::settings::UserSettings;
 use crate::submodule_store::SubmoduleStore;
@@ -43,6 +43,8 @@ pub enum WorkspaceInitError {
     CheckOutCommit(#[from] CheckOutCommitError),
     #[error(transparent)]
     Path(#[from] PathError),
+    #[error(transparent)]
+    Backend(#[from] BackendError),
 }
 
 #[derive(Error, Debug)]
@@ -130,7 +132,7 @@ impl Workspace {
         workspace_root: &Path,
     ) -> Result<(Self, Arc<ReadonlyRepo>), WorkspaceInitError> {
         Self::init_with_backend(user_settings, workspace_root, |store_path| {
-            Box::new(LocalBackend::init(store_path))
+            Ok(Box::new(LocalBackend::init(store_path)))
         })
     }
 
@@ -141,7 +143,7 @@ impl Workspace {
         workspace_root: &Path,
     ) -> Result<(Self, Arc<ReadonlyRepo>), WorkspaceInitError> {
         Self::init_with_backend(user_settings, workspace_root, |store_path| {
-            Box::new(GitBackend::init_internal(store_path))
+            Ok(Box::new(GitBackend::init_internal(store_path)))
         })
     }
 
@@ -153,47 +155,60 @@ impl Workspace {
         git_repo_path: &Path,
     ) -> Result<(Self, Arc<ReadonlyRepo>), WorkspaceInitError> {
         Self::init_with_backend(user_settings, workspace_root, |store_path| {
-            Box::new(GitBackend::init_external(store_path, git_repo_path))
+            Ok(Box::new(GitBackend::init_external(
+                store_path,
+                git_repo_path,
+            )?))
         })
     }
 
     pub fn init_with_factories(
         user_settings: &UserSettings,
         workspace_root: &Path,
-        backend_factory: impl FnOnce(&Path) -> Box<dyn Backend>,
+        backend_factory: impl FnOnce(&Path) -> Result<Box<dyn Backend>, BackendError>,
         op_store_factory: impl FnOnce(&Path) -> Box<dyn OpStore>,
         op_heads_store_factory: impl FnOnce(&Path) -> Box<dyn OpHeadsStore>,
         index_store_factory: impl FnOnce(&Path) -> Box<dyn IndexStore>,
         submodule_store_factory: impl FnOnce(&Path) -> Box<dyn SubmoduleStore>,
     ) -> Result<(Self, Arc<ReadonlyRepo>), WorkspaceInitError> {
         let jj_dir = create_jj_dir(workspace_root)?;
-        let repo_dir = jj_dir.join("repo");
-        std::fs::create_dir(&repo_dir).context(&repo_dir)?;
-        let repo = ReadonlyRepo::init(
-            user_settings,
-            &repo_dir,
-            backend_factory,
-            op_store_factory,
-            op_heads_store_factory,
-            index_store_factory,
-            submodule_store_factory,
-        )?;
-        let (working_copy, repo) = init_working_copy(
-            user_settings,
-            &repo,
-            workspace_root,
-            &jj_dir,
-            WorkspaceId::default(),
-        )?;
-        let repo_loader = repo.loader();
-        let workspace = Workspace::new(workspace_root, working_copy, repo_loader)?;
-        Ok((workspace, repo))
+        (|| {
+            let repo_dir = jj_dir.join("repo");
+            std::fs::create_dir(&repo_dir).context(&repo_dir)?;
+            let repo = ReadonlyRepo::init(
+                user_settings,
+                &repo_dir,
+                backend_factory,
+                op_store_factory,
+                op_heads_store_factory,
+                index_store_factory,
+                submodule_store_factory,
+            )
+            .map_err(|repo_init_err| match repo_init_err {
+                RepoInitError::Backend(err) => WorkspaceInitError::Backend(err),
+                RepoInitError::Path(err) => WorkspaceInitError::Path(err),
+            })?;
+            let (working_copy, repo) = init_working_copy(
+                user_settings,
+                &repo,
+                workspace_root,
+                &jj_dir,
+                WorkspaceId::default(),
+            )?;
+            let repo_loader = repo.loader();
+            let workspace = Workspace::new(workspace_root, working_copy, repo_loader)?;
+            Ok((workspace, repo))
+        })()
+        .map_err(|err| {
+            let _ = std::fs::remove_dir_all(jj_dir);
+            err
+        })
     }
 
     pub fn init_with_backend(
         user_settings: &UserSettings,
         workspace_root: &Path,
-        backend_factory: impl FnOnce(&Path) -> Box<dyn Backend>,
+        backend_factory: impl FnOnce(&Path) -> Result<Box<dyn Backend>, BackendError>,
     ) -> Result<(Self, Arc<ReadonlyRepo>), WorkspaceInitError> {
         Self::init_with_factories(
             user_settings,

lib/tests/test_git.rs

@@ -850,7 +850,12 @@ impl GitRepoData {
         let repo = ReadonlyRepo::init(
             &settings,
             &jj_repo_dir,
-            |store_path| Box::new(GitBackend::init_external(store_path, &git_repo_dir)),
+            |store_path| {
+                Ok(Box::new(GitBackend::init_external(
+                    store_path,
+                    &git_repo_dir,
+                )?))
+            },
             ReadonlyRepo::default_op_store_factory(),
             ReadonlyRepo::default_op_heads_store_factory(),
             ReadonlyRepo::default_index_store_factory(),
@@ -1417,7 +1422,12 @@ fn test_init() {
     let repo = ReadonlyRepo::init(
         &settings,
         &jj_repo_dir,
-        |store_path| Box::new(GitBackend::init_external(store_path, &git_repo_dir)),
+        |store_path| {
+            Ok(Box::new(GitBackend::init_external(
+                store_path,
+                &git_repo_dir,
+            )?))
+        },
         ReadonlyRepo::default_op_store_factory(),
         ReadonlyRepo::default_op_heads_store_factory(),
         ReadonlyRepo::default_index_store_factory(),
@@ -1677,7 +1687,12 @@ fn set_up_push_repos(settings: &UserSettings, temp_dir: &TempDir) -> PushTestSet
     let jj_repo = ReadonlyRepo::init(
         settings,
         &jj_repo_dir,
-        |store_path| Box::new(GitBackend::init_external(store_path, &clone_repo_dir)),
+        |store_path| {
+            Ok(Box::new(GitBackend::init_external(
+                store_path,
+                &clone_repo_dir,
+            )?))
+        },
         ReadonlyRepo::default_op_store_factory(),
         ReadonlyRepo::default_op_heads_store_factory(),
         ReadonlyRepo::default_index_store_factory(),

lib/testutils/src/lib.rs

@@ -19,7 +19,7 @@ use std::path::{Path, PathBuf};
 use std::sync::{Arc, Once};
 
 use itertools::Itertools;
-use jujutsu_lib::backend::{FileId, TreeId, TreeValue};
+use jujutsu_lib::backend::{Backend, BackendError, FileId, TreeId, TreeValue};
 use jujutsu_lib::commit::Commit;
 use jujutsu_lib::commit_builder::CommitBuilder;
 use jujutsu_lib::git_backend::GitBackend;
@@ -95,7 +95,9 @@ impl TestRepo {
             ReadonlyRepo::init(
                 &settings,
                 &repo_dir,
-                |store_path| Box::new(GitBackend::init_external(store_path, &git_path)),
+                |store_path| -> Result<Box<dyn Backend>, BackendError> {
+                    Ok(Box::new(GitBackend::init_external(store_path, &git_path)?))
+                },
                 ReadonlyRepo::default_op_store_factory(),
                 ReadonlyRepo::default_op_heads_store_factory(),
                 ReadonlyRepo::default_index_store_factory(),
@@ -106,7 +108,9 @@ impl TestRepo {
             ReadonlyRepo::init(
                 &settings,
                 &repo_dir,
-                |store_path| Box::new(LocalBackend::init(store_path)),
+                |store_path| -> Result<Box<dyn Backend>, BackendError> {
+                    Ok(Box::new(LocalBackend::init(store_path)))
+                },
                 ReadonlyRepo::default_op_store_factory(),
                 ReadonlyRepo::default_op_heads_store_factory(),
                 ReadonlyRepo::default_index_store_factory(),

src/cli_util.rs

@@ -176,6 +176,9 @@ impl From<WorkspaceInitError> for CommandError {
             WorkspaceInitError::Path(err) => {
                 CommandError::InternalError(format!("Failed to access the repository: {err}"))
             }
+            WorkspaceInitError::Backend(err) => {
+                user_error(format!("Failed to access the repository: {err}"))
+            }
         }
     }
 }

tests/test_init_command.rs

@@ -115,7 +115,7 @@ fn test_init_git_external() {
 }
 
 #[test]
-fn test_init_git_external_bad_path() {
+fn test_init_git_external_non_existent_directory() {
     let test_env = TestEnvironment::default();
     let stderr = test_env.jj_cmd_failure(
         test_env.env_root(),
@@ -126,6 +126,24 @@ fn test_init_git_external_bad_path() {
     "###);
 }
 
+#[test]
+fn test_init_git_external_non_existent_git_directory() {
+    let test_env = TestEnvironment::default();
+    let workspace_root = test_env.env_root().join("repo");
+    let stderr =
+        test_env.jj_cmd_failure(test_env.env_root(), &["init", "repo", "--git-repo", "repo"]);
+
+    insta::with_settings!({filters => vec![
+        (r"(Error: Failed to open git repository:)(?s).*", "Error: Failed to open git repository:"),
+    ]}, {
+        insta::assert_snapshot!(&stderr, @r###"
+        Error: Failed to access the repository: Error: Failed to open git repository:
+        "###);
+    });
+    let jj_path = workspace_root.join(".jj");
+    assert!(!jj_path.exists());
+}
+
 #[test]
 fn test_init_git_colocated() {
     let test_env = TestEnvironment::default();