repo: remove repo_path from ReadonlyRepo and RepoLoader

When loading repos on a server, there is no repo path. We currently use a placeholder at Google. This patch will let us not do that. I think we can remove the paths from `Workspace` too, but I'll leave that for later, if at all.
jj-vcs · Sep 7, 2024 · 6d8ab26 · 6d8ab26
1 parent 5fcc66c
commit 6d8ab26
Show file tree

Hide file tree

Showing 3 changed files with 19 additions and 37 deletions.
diff --git a/lib/src/repo.rs b/lib/src/repo.rs
@@ -21,7 +21,6 @@ use std::fmt::Debug;
 use std::fmt::Formatter;
 use std::fs;
 use std::path::Path;
-use std::path::PathBuf;
 use std::slice;
 use std::sync::Arc;
 
@@ -122,7 +121,6 @@ pub trait Repo {
 }
 
 pub struct ReadonlyRepo {
-    repo_path: PathBuf,
     store: Arc<Store>,
     op_store: Arc<dyn OpStore>,
     op_heads_store: Arc<dyn OpHeadsStore>,
@@ -138,10 +136,7 @@ pub struct ReadonlyRepo {
 
 impl Debug for ReadonlyRepo {
     fn fmt(&self, f: &mut Formatter<'_>) -> Result<(), std::fmt::Error> {
-        f.debug_struct("Repo")
-            .field("repo_path", &self.repo_path)
-            .field("store", &self.store)
-            .finish()
+        f.debug_struct("Repo").field("store", &self.store).finish()
     }
 }
 
@@ -239,7 +234,6 @@ impl ReadonlyRepo {
             // be initialized properly.
             .map_err(|err| BackendInitError(err.into()))?;
         let repo = Arc::new(ReadonlyRepo {
-            repo_path,
             store,
             op_store,
             op_heads_store,
@@ -260,7 +254,6 @@ impl ReadonlyRepo {
 
     pub fn loader(&self) -> RepoLoader {
         RepoLoader {
-            repo_path: self.repo_path.clone(),
             repo_settings: self.settings.clone(),
             store: self.store.clone(),
             op_store: self.op_store.clone(),
@@ -270,10 +263,6 @@ impl ReadonlyRepo {
         }
     }
 
-    pub fn repo_path(&self) -> &PathBuf {
-        &self.repo_path
-    }
-
     pub fn op_id(&self) -> &OperationId {
         self.operation.id()
     }
@@ -632,7 +621,6 @@ pub enum RepoLoaderError {
 /// a given operation.
 #[derive(Clone)]
 pub struct RepoLoader {
-    repo_path: PathBuf,
     repo_settings: RepoSettings,
     store: Arc<Store>,
     op_store: Arc<dyn OpStore>,
@@ -643,7 +631,6 @@ pub struct RepoLoader {
 
 impl RepoLoader {
     pub fn new(
-        repo_path: PathBuf,
         repo_settings: RepoSettings,
         store: Arc<Store>,
         op_store: Arc<dyn OpStore>,
@@ -652,7 +639,6 @@ impl RepoLoader {
         submodule_store: Arc<dyn SubmoduleStore>,
     ) -> Self {
         Self {
-            repo_path,
             repo_settings,
             store,
             op_store,
@@ -687,7 +673,6 @@ impl RepoLoader {
                 .load_submodule_store(user_settings, &repo_path.join("submodule_store"))?,
         );
         Ok(Self {
-            repo_path: repo_path.to_path_buf(),
             repo_settings,
             store,
             op_store,
@@ -697,10 +682,6 @@ impl RepoLoader {
         })
     }
 
-    pub fn repo_path(&self) -> &PathBuf {
-        &self.repo_path
-    }
-
     pub fn store(&self) -> &Arc<Store> {
         &self.store
     }
@@ -743,7 +724,6 @@ impl RepoLoader {
         index: Box<dyn ReadonlyIndex>,
     ) -> Arc<ReadonlyRepo> {
         let repo = ReadonlyRepo {
-            repo_path: self.repo_path.clone(),
             store: self.store.clone(),
             op_store: self.op_store.clone(),
             op_heads_store: self.op_heads_store.clone(),
@@ -813,7 +793,6 @@ impl RepoLoader {
     ) -> Result<Arc<ReadonlyRepo>, RepoLoaderError> {
         let index = self.index_store.get_index_at_op(&operation, &self.store)?;
         let repo = ReadonlyRepo {
-            repo_path: self.repo_path.clone(),
             store: self.store.clone(),
             op_store: self.op_store.clone(),
             op_heads_store: self.op_heads_store.clone(),

diff --git a/lib/src/workspace.rs b/lib/src/workspace.rs
@@ -101,6 +101,7 @@ pub struct Workspace {
     // Path to the workspace root (typically the parent of a .jj/ directory), which is where
     // working copy files live.
     workspace_root: PathBuf,
+    repo_path: PathBuf,
     repo_loader: RepoLoader,
     working_copy: Box<dyn WorkingCopy>,
 }
@@ -150,24 +151,28 @@ fn init_working_copy(
 impl Workspace {
     pub fn new(
         workspace_root: &Path,
+        repo_path: PathBuf,
         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,
+            repo_path,
             working_copy,
             repo_loader,
         ))
     }
 
     pub fn new_no_canonicalize(
         workspace_root: PathBuf,
+        repo_path: PathBuf,
         working_copy: Box<dyn WorkingCopy>,
         repo_loader: RepoLoader,
     ) -> Self {
         Self {
             workspace_root,
+            repo_path,
             repo_loader,
             working_copy,
         }
@@ -309,7 +314,7 @@ impl Workspace {
                 workspace_id,
             )?;
             let repo_loader = repo.loader();
-            let workspace = Workspace::new(workspace_root, working_copy, repo_loader)?;
+            let workspace = Workspace::new(workspace_root, repo_dir, working_copy, repo_loader)?;
             Ok((workspace, repo))
         })()
         .inspect_err(|_err| {
@@ -367,7 +372,7 @@ impl Workspace {
             working_copy_factory,
             workspace_id,
         )?;
-        let workspace = Workspace::new(workspace_root, working_copy, repo.loader())?;
+        let workspace = Workspace::new(workspace_root, repo_dir, working_copy, repo.loader())?;
         Ok((workspace, repo))
     }
 
@@ -391,7 +396,7 @@ impl Workspace {
     }
 
     pub fn repo_path(&self) -> &PathBuf {
-        self.repo_loader.repo_path()
+        &self.repo_path
     }
 
     pub fn repo_loader(&self) -> &RepoLoader {
@@ -527,7 +532,7 @@ impl WorkspaceLoaderFactory for DefaultWorkspaceLoaderFactory {
 #[derive(Clone, Debug)]
 struct DefaultWorkspaceLoader {
     workspace_root: PathBuf,
-    repo_dir: PathBuf,
+    repo_path: PathBuf,
     working_copy_state_path: PathBuf,
 }
 
@@ -559,7 +564,7 @@ impl DefaultWorkspaceLoader {
         let working_copy_state_path = jj_dir.join("working_copy");
         Ok(Self {
             workspace_root: workspace_root.to_owned(),
-            repo_dir,
+            repo_path: repo_dir,
             working_copy_state_path,
         })
     }
@@ -571,7 +576,7 @@ impl WorkspaceLoader for DefaultWorkspaceLoader {
     }
 
     fn repo_path(&self) -> &Path {
-        &self.repo_dir
+        &self.repo_path
     }
 
     fn load(
@@ -581,10 +586,15 @@ impl WorkspaceLoader for DefaultWorkspaceLoader {
         working_copy_factories: &WorkingCopyFactories,
     ) -> Result<Workspace, WorkspaceLoadError> {
         let repo_loader =
-            RepoLoader::init_from_file_system(user_settings, &self.repo_dir, store_factories)?;
+            RepoLoader::init_from_file_system(user_settings, &self.repo_path, store_factories)?;
         let working_copy_factory = get_working_copy_factory(self, working_copy_factories)?;
         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(
+            &self.workspace_root,
+            self.repo_path.clone(),
+            working_copy,
+            repo_loader,
+        )?;
         Ok(workspace)
     }
 

diff --git a/lib/tests/test_init.rs b/lib/tests/test_init.rs
@@ -42,7 +42,6 @@ fn test_init_local() {
         .backend_impl()
         .downcast_ref::<GitBackend>()
         .is_none());
-    assert_eq!(repo.repo_path(), &canonical.join(".jj").join("repo"));
     assert_eq!(workspace.workspace_root(), &canonical);
 
     // Just test that we can write a commit to the store
@@ -62,7 +61,6 @@ fn test_init_internal_git() {
         .downcast_ref::<GitBackend>()
         .unwrap();
     let repo_path = canonical.join(".jj").join("repo");
-    assert_eq!(repo.repo_path(), &repo_path);
     assert_eq!(workspace.workspace_root(), &canonical);
     assert_eq!(
         git_backend.git_repo_path(),
@@ -91,7 +89,6 @@ fn test_init_colocated_git() {
         .downcast_ref::<GitBackend>()
         .unwrap();
     let repo_path = canonical.join(".jj").join("repo");
-    assert_eq!(repo.repo_path(), &repo_path);
     assert_eq!(workspace.workspace_root(), &canonical);
     assert_eq!(git_backend.git_repo_path(), canonical.join(".git"));
     assert_eq!(git_backend.git_workdir(), Some(canonical.as_ref()));
@@ -124,10 +121,6 @@ fn test_init_external_git() {
         .backend_impl()
         .downcast_ref::<GitBackend>()
         .unwrap();
-    assert_eq!(
-        repo.repo_path(),
-        &canonical.join("jj").join(".jj").join("repo")
-    );
     assert_eq!(workspace.workspace_root(), &canonical.join("jj"));
     assert_eq!(
         git_backend.git_repo_path(),