workspace: move path canonicalization out of init_with_backend()

We have not used libgit2 for gitignores since commit 88f7f473
(2021-05-13).

cli/examples/custom-backend/main.rs

@@ -76,7 +76,7 @@ fn run_custom_command(
             // Initialize a workspace with the custom backend
             Workspace::init_with_backend(
                 command_helper.settings(),
-                wc_path,
+                wc_path.canonicalize()?,
                 &|settings, store_path| Ok(Box::new(JitBackend::init(settings, store_path)?)),
                 Signer::from_settings(command_helper.settings())
                     .map_err(WorkspaceInitError::SignInit)?,

cli/examples/custom-working-copy/main.rs

@@ -68,7 +68,7 @@ fn run_custom_command(
             };
             Workspace::init_with_factories(
                 command_helper.settings(),
-                wc_path,
+                wc_path.canonicalize()?,
                 &backend_initializer,
                 Signer::from_settings(command_helper.settings())
                     .map_err(WorkspaceInitError::SignInit)?,

lib/src/local_working_copy.rs

@@ -579,11 +579,9 @@ impl TreeState {
 
     fn empty(store: Arc<Store>, working_copy_path: PathBuf, state_path: PathBuf) -> TreeState {
         let tree_id = store.empty_merged_tree_id();
-        // Canonicalize the working copy path because "repo/." makes libgit2 think that
-        // everything should be ignored
         TreeState {
             store,
-            working_copy_path: working_copy_path.canonicalize().unwrap(),
+            working_copy_path,
             state_path,
             tree_id,
             file_states: FileStatesMap::new(),

lib/src/repo.rs

@@ -175,7 +175,7 @@ impl ReadonlyRepo {
     #[allow(clippy::too_many_arguments)]
     pub fn init(
         user_settings: &UserSettings,
-        repo_path: &Path,
+        repo_path: PathBuf,
         backend_initializer: &BackendInitializer,
         signer: Signer,
         op_store_initializer: &OpStoreInitializer,
@@ -183,8 +183,6 @@ impl ReadonlyRepo {
         index_store_initializer: &IndexStoreInitializer,
         submodule_store_initializer: &SubmoduleStoreInitializer,
     ) -> 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_initializer(user_settings, &store_path)?;

lib/src/workspace.rs

@@ -149,19 +149,6 @@ fn init_working_copy(
 
 impl Workspace {
     pub fn new(
-        workspace_root: &Path,
-        working_copy: Box<dyn WorkingCopy>,
-        repo_loader: RepoLoader,
-    ) -> Result<Workspace, PathError> {
-        let workspace_root = workspace_root.canonicalize().context(workspace_root)?;
-        Ok(Self::new_no_canonicalize(
-            workspace_root,
-            working_copy,
-            repo_loader,
-        ))
-    }
-
-    pub fn new_no_canonicalize(
         workspace_root: PathBuf,
         working_copy: Box<dyn WorkingCopy>,
         repo_loader: RepoLoader,
@@ -177,6 +164,7 @@ impl Workspace {
         user_settings: &UserSettings,
         workspace_root: &Path,
     ) -> Result<(Self, Arc<ReadonlyRepo>), WorkspaceInitError> {
+        let workspace_root = workspace_root.canonicalize().context(workspace_root)?;
         let backend_initializer: &BackendInitializer =
             &|_settings, store_path| Ok(Box::new(LocalBackend::init(store_path)));
         let signer = Signer::from_settings(user_settings)?;
@@ -190,6 +178,7 @@ impl Workspace {
         user_settings: &UserSettings,
         workspace_root: &Path,
     ) -> Result<(Self, Arc<ReadonlyRepo>), WorkspaceInitError> {
+        let workspace_root = workspace_root.canonicalize().context(workspace_root)?;
         let backend_initializer: &BackendInitializer = &|settings, store_path| {
             Ok(Box::new(crate::git_backend::GitBackend::init_internal(
                 settings, store_path,
@@ -206,18 +195,12 @@ impl Workspace {
         user_settings: &UserSettings,
         workspace_root: &Path,
     ) -> Result<(Self, Arc<ReadonlyRepo>), WorkspaceInitError> {
+        let workspace_root = workspace_root.canonicalize().context(workspace_root)?;
         let backend_initializer = |settings: &UserSettings,
                                    store_path: &Path|
          -> Result<Box<dyn crate::backend::Backend>, _> {
-            // TODO: Clean up path normalization. store_path is canonicalized by
-            // ReadonlyRepo::init(). workspace_root will be canonicalized by
-            // Workspace::new(), but it's not yet here.
             let store_relative_workspace_root =
-                if let Ok(workspace_root) = workspace_root.canonicalize() {
-                    crate::file_util::relative_path(store_path, &workspace_root)
-                } else {
-                    workspace_root.to_owned()
-                };
+                crate::file_util::relative_path(store_path, &workspace_root);
             let backend = crate::git_backend::GitBackend::init_colocated(
                 settings,
                 store_path,
@@ -226,7 +209,12 @@ impl Workspace {
             Ok(Box::new(backend))
         };
         let signer = Signer::from_settings(user_settings)?;
-        Self::init_with_backend(user_settings, workspace_root, &backend_initializer, signer)
+        Self::init_with_backend(
+            user_settings,
+            workspace_root.clone(),
+            &backend_initializer,
+            signer,
+        )
     }
 
     /// Initializes a workspace with an existing Git repo at the specified path.
@@ -239,25 +227,19 @@ impl Workspace {
         workspace_root: &Path,
         git_repo_path: &Path,
     ) -> Result<(Self, Arc<ReadonlyRepo>), WorkspaceInitError> {
+        let workspace_root = workspace_root.canonicalize().context(workspace_root)?;
         let backend_initializer = |settings: &UserSettings,
                                    store_path: &Path|
          -> Result<Box<dyn crate::backend::Backend>, _> {
             // If the git repo is inside the workspace, use a relative path to it so the
             // whole workspace can be moved without breaking.
-            // TODO: Clean up path normalization. store_path is canonicalized by
-            // ReadonlyRepo::init(). workspace_root will be canonicalized by
-            // Workspace::new(), but it's not yet here.
-            let store_relative_git_repo_path = match (
-                workspace_root.canonicalize(),
-                crate::git_backend::canonicalize_git_repo_path(git_repo_path),
-            ) {
-                (Ok(workspace_root), Ok(git_repo_path))
-                    if git_repo_path.starts_with(&workspace_root) =>
-                {
-                    crate::file_util::relative_path(store_path, &git_repo_path)
-                }
-                _ => git_repo_path.to_owned(),
-            };
+            let store_relative_git_repo_path =
+                match crate::git_backend::canonicalize_git_repo_path(git_repo_path) {
+                    Ok(git_repo_path) if git_repo_path.starts_with(&workspace_root) => {
+                        crate::file_util::relative_path(store_path, &git_repo_path)
+                    }
+                    _ => git_repo_path.to_owned(),
+                };
             let backend = crate::git_backend::GitBackend::init_external(
                 settings,
                 store_path,
@@ -266,13 +248,19 @@ impl Workspace {
             Ok(Box::new(backend))
         };
         let signer = Signer::from_settings(user_settings)?;
-        Self::init_with_backend(user_settings, workspace_root, &backend_initializer, signer)
+        Self::init_with_backend(
+            user_settings,
+            workspace_root.clone(),
+            &backend_initializer,
+            signer,
+        )
     }
 
+    /// The `workspace_root` should be canonicalized.
     #[allow(clippy::too_many_arguments)]
     pub fn init_with_factories(
         user_settings: &UserSettings,
-        workspace_root: &Path,
+        workspace_root: PathBuf,
         backend_initializer: &BackendInitializer,
         signer: Signer,
         op_store_initializer: &OpStoreInitializer,
@@ -282,13 +270,13 @@ impl Workspace {
         working_copy_factory: &dyn WorkingCopyFactory,
         workspace_id: WorkspaceId,
     ) -> Result<(Self, Arc<ReadonlyRepo>), WorkspaceInitError> {
-        let jj_dir = create_jj_dir(workspace_root)?;
+        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,
+                repo_dir,
                 backend_initializer,
                 signer,
                 op_store_initializer,
@@ -303,13 +291,13 @@ impl Workspace {
             let (working_copy, repo) = init_working_copy(
                 user_settings,
                 &repo,
-                workspace_root,
+                &workspace_root,
                 &jj_dir,
                 working_copy_factory,
                 workspace_id,
             )?;
             let repo_loader = repo.loader();
-            let workspace = Workspace::new(workspace_root, working_copy, repo_loader)?;
+            let workspace = Workspace::new(workspace_root, working_copy, repo_loader);
             Ok((workspace, repo))
         })()
         .inspect_err(|_err| {
@@ -317,9 +305,10 @@ impl Workspace {
         })
     }
 
+    /// The `workspace_root` should be canonicalized.
     pub fn init_with_backend(
         user_settings: &UserSettings,
-        workspace_root: &Path,
+        workspace_root: PathBuf,
         backend_initializer: &BackendInitializer,
         signer: Signer,
     ) -> Result<(Self, Arc<ReadonlyRepo>), WorkspaceInitError> {
@@ -344,7 +333,8 @@ impl Workspace {
         working_copy_factory: &dyn WorkingCopyFactory,
         workspace_id: WorkspaceId,
     ) -> Result<(Self, Arc<ReadonlyRepo>), WorkspaceInitError> {
-        let jj_dir = create_jj_dir(workspace_root)?;
+        let workspace_root = workspace_root.canonicalize().context(workspace_root)?;
+        let jj_dir = create_jj_dir(&workspace_root)?;
 
         let repo_dir = repo.repo_path().canonicalize().context(repo.repo_path())?;
         let repo_file_path = jj_dir.join("repo");
@@ -361,12 +351,12 @@ impl Workspace {
         let (working_copy, repo) = init_working_copy(
             user_settings,
             repo,
-            workspace_root,
+            &workspace_root,
             &jj_dir,
             working_copy_factory,
             workspace_id,
         )?;
-        let workspace = Workspace::new(workspace_root, working_copy, repo.loader())?;
+        let workspace = Workspace::new(workspace_root, working_copy, repo.loader());
         Ok((workspace, repo))
     }
 
@@ -579,8 +569,12 @@ impl WorkspaceLoader for DefaultWorkspaceLoader {
     ) -> Result<Workspace, WorkspaceLoadError> {
         let repo_loader = RepoLoader::init(user_settings, &self.repo_dir, store_factories)?;
         let working_copy_factory = get_working_copy_factory(self, working_copy_factories)?;
+        let workspace_root = self
+            .workspace_root
+            .canonicalize()
+            .context(&self.workspace_root)?;
         let working_copy = self.load_working_copy(repo_loader.store(), working_copy_factory)?;
-        let workspace = Workspace::new(&self.workspace_root, working_copy, repo_loader)?;
+        let workspace = Workspace::new(workspace_root, working_copy, repo_loader);
         Ok(workspace)
     }
 

lib/tests/test_git.rs

@@ -1322,7 +1322,7 @@ impl GitRepoData {
         std::fs::create_dir(&jj_repo_dir).unwrap();
         let repo = ReadonlyRepo::init(
             &settings,
-            &jj_repo_dir,
+            jj_repo_dir,
             &|settings, store_path| {
                 Ok(Box::new(GitBackend::init_external(
                     settings,
@@ -2214,7 +2214,7 @@ fn test_init() {
     std::fs::create_dir(&jj_repo_dir).unwrap();
     let repo = &ReadonlyRepo::init(
         &settings,
-        &jj_repo_dir,
+        jj_repo_dir,
         &|settings, store_path| {
             Ok(Box::new(GitBackend::init_external(
                 settings,
@@ -2563,7 +2563,7 @@ fn set_up_push_repos(settings: &UserSettings, temp_dir: &TempDir) -> PushTestSet
     std::fs::create_dir(&jj_repo_dir).unwrap();
     let jj_repo = ReadonlyRepo::init(
         settings,
-        &jj_repo_dir,
+        jj_repo_dir,
         &|settings, store_path| {
             Ok(Box::new(GitBackend::init_external(
                 settings,

lib/testutils/src/lib.rs

@@ -37,6 +37,7 @@ use jj_lib::backend::Timestamp;
 use jj_lib::backend::TreeValue;
 use jj_lib::commit::Commit;
 use jj_lib::commit_builder::CommitBuilder;
+use jj_lib::file_util::IoResultExt;
 use jj_lib::git_backend::GitBackend;
 use jj_lib::local_backend::LocalBackend;
 use jj_lib::merged_tree::MergedTree;
@@ -165,10 +166,11 @@ impl TestRepo {
 
         let repo_dir = temp_dir.path().join("repo");
         fs::create_dir(&repo_dir).unwrap();
+        let repo_dir = repo_dir.canonicalize().context(&repo_dir).unwrap();
 
         let repo = ReadonlyRepo::init(
             settings,
-            &repo_dir,
+            repo_dir,
             &move |settings, store_path| backend.init_backend(settings, store_path),
             Signer::from_settings(settings).unwrap(),
             ReadonlyRepo::default_op_store_initializer(),
@@ -232,7 +234,7 @@ impl TestWorkspace {
 
         let (workspace, repo) = Workspace::init_with_backend(
             settings,
-            &workspace_root,
+            workspace_root,
             &move |settings, store_path| backend.init_backend(settings, store_path),
             signer,
         )