workspace: move search for .jj/ directory from Repo to Workspace

lib/src/repo.rs

@@ -126,12 +126,6 @@ pub enum RepoInitError {
     DestinationExists(PathBuf),
 }
 
-#[derive(Error, Debug, PartialEq)]
-pub enum RepoLoadError {
-    #[error("There is no Jujutsu repo in {0}")]
-    NoRepoHere(PathBuf),
-}
-
 impl ReadonlyRepo {
     pub fn init_local(
         settings: &UserSettings,
@@ -239,11 +233,8 @@ impl ReadonlyRepo {
         })
     }
 
-    pub fn load(
+    pub fn load(user_settings: &UserSettings, repo_path: PathBuf) -> Arc<ReadonlyRepo> {
-        user_settings: &UserSettings,
+        RepoLoader::init(user_settings, repo_path).load_at_head()
-        wc_path: PathBuf,
-    ) -> Result<Arc<ReadonlyRepo>, RepoLoadError> {
-        Ok(RepoLoader::init(user_settings, wc_path)?.load_at_head())
     }
 
     pub fn loader(&self) -> RepoLoader {
@@ -351,26 +342,8 @@ pub struct RepoLoader {
     index_store: Arc<IndexStore>,
 }
 
-fn find_repo_dir(mut wc_dir: &Path) -> Option<PathBuf> {
-    loop {
-        let repo_path = wc_dir.join(".jj");
-        if repo_path.is_dir() {
-            return Some(repo_path);
-        }
-        if let Some(wc_dir_parent) = wc_dir.parent() {
-            wc_dir = wc_dir_parent;
-        } else {
-            return None;
-        }
-    }
-}
-
 impl RepoLoader {
-    pub fn init(
+    pub fn init(user_settings: &UserSettings, repo_path: PathBuf) -> Self {
-        user_settings: &UserSettings,
-        wc_path: PathBuf,
-    ) -> Result<RepoLoader, RepoLoadError> {
-        let repo_path = find_repo_dir(&wc_path).ok_or(RepoLoadError::NoRepoHere(wc_path))?;
         let wc_path = repo_path.parent().unwrap().to_owned();
         let store_path = repo_path.join("store");
         if store_path.is_file() {
@@ -398,7 +371,7 @@ impl RepoLoader {
         let op_store: Arc<dyn OpStore> = Arc::new(SimpleOpStore::load(repo_path.join("op_store")));
         let op_heads_store = Arc::new(OpHeadsStore::load(repo_path.join("op_heads")));
         let index_store = Arc::new(IndexStore::load(repo_path.join("index")));
-        Ok(RepoLoader {
+        Self {
             wc_path,
             repo_path,
             repo_settings,
@@ -406,7 +379,7 @@ impl RepoLoader {
             op_store,
             op_heads_store,
             index_store,
-        })
+        }
     }
 
     pub fn repo_path(&self) -> &PathBuf {

lib/src/workspace.rs

@@ -12,13 +12,21 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-use std::path::PathBuf;
+use std::path::{Path, PathBuf};
 use std::sync::Arc;
 
-use crate::repo::{ReadonlyRepo, RepoInitError, RepoLoadError, RepoLoader};
+use thiserror::Error;
+
+use crate::repo::{ReadonlyRepo, RepoInitError, RepoLoader};
 use crate::settings::UserSettings;
 use crate::working_copy::WorkingCopy;
 
+#[derive(Error, Debug, PartialEq)]
+pub enum WorkspaceLoadError {
+    #[error("There is no Jujutsu repo in {0}")]
+    NoWorkspaceHere(PathBuf),
+}
+
 /// Represents a workspace, i.e. what's typically the .jj/ directory and its
 /// parent.
 pub struct Workspace {
@@ -64,11 +72,12 @@ impl Workspace {
 
     pub fn load(
         user_settings: &UserSettings,
-        workspace_root: PathBuf,
+        workspace_path: PathBuf,
-    ) -> Result<Self, RepoLoadError> {
+    ) -> Result<Self, WorkspaceLoadError> {
-        // TODO: Move the find_repo_dir() call from RepoLoader::init() to here
+        let repo_path = find_repo_dir(&workspace_path)
-        let repo_loader = RepoLoader::init(user_settings, workspace_root)?;
+            .ok_or(WorkspaceLoadError::NoWorkspaceHere(workspace_path))?;
-        let workspace_root = repo_loader.working_copy_path().clone();
+        let workspace_root = repo_path.parent().unwrap().to_owned();
+        let repo_loader = RepoLoader::init(user_settings, repo_path);
         Ok(Self::from_repo_loader(workspace_root, repo_loader))
     }
 
@@ -106,3 +115,17 @@ impl Workspace {
         &mut self.working_copy
     }
 }
+
+fn find_repo_dir(mut workspace_root: &Path) -> Option<PathBuf> {
+    loop {
+        let repo_path = workspace_root.join(".jj");
+        if repo_path.is_dir() {
+            return Some(repo_path);
+        }
+        if let Some(wc_dir_parent) = workspace_root.parent() {
+            workspace_root = wc_dir_parent;
+        } else {
+            return None;
+        }
+    }
+}

lib/tests/test_bad_locking.rs

@@ -107,7 +107,7 @@ fn test_bad_locking_children(use_git: bool) {
     // Simulate a write of a commit that happens on one machine
     let machine1_root = TempDir::new().unwrap().into_path();
     copy_directory(workspace_root, &machine1_root);
-    let machine1_repo = ReadonlyRepo::load(&settings, machine1_root.clone()).unwrap();
+    let machine1_repo = ReadonlyRepo::load(&settings, machine1_root.join(".jj"));
     let mut machine1_tx = machine1_repo.start_transaction("test");
     let child1 = testutils::create_random_commit(&settings, &machine1_repo)
         .set_parents(vec![initial.id().clone()])
@@ -117,7 +117,7 @@ fn test_bad_locking_children(use_git: bool) {
     // Simulate a write of a commit that happens on another machine
     let machine2_root = TempDir::new().unwrap().into_path();
     copy_directory(workspace_root, &machine2_root);
-    let machine2_repo = ReadonlyRepo::load(&settings, machine2_root.clone()).unwrap();
+    let machine2_repo = ReadonlyRepo::load(&settings, machine2_root.join(".jj"));
     let mut machine2_tx = machine2_repo.start_transaction("test");
     let child2 = testutils::create_random_commit(&settings, &machine2_repo)
         .set_parents(vec![initial.id().clone()])
@@ -128,7 +128,7 @@ fn test_bad_locking_children(use_git: bool) {
     // both machines
     let merged_path = TempDir::new().unwrap().into_path();
     merge_directories(&machine1_root, workspace_root, &machine2_root, &merged_path);
-    let merged_repo = ReadonlyRepo::load(&settings, merged_path).unwrap();
+    let merged_repo = ReadonlyRepo::load(&settings, merged_path.join(".jj"));
     assert!(merged_repo.view().heads().contains(child1.id()));
     assert!(merged_repo.view().heads().contains(child2.id()));
     let op_id = merged_repo.op_id().clone();
@@ -167,10 +167,10 @@ fn test_bad_locking_interrupted(use_git: bool) {
 
     copy_directory(&backup_path, &op_heads_dir);
     // Reload the repo and check that only the new head is present.
-    let reloaded_repo = ReadonlyRepo::load(&settings, repo.repo_path().clone()).unwrap();
+    let reloaded_repo = ReadonlyRepo::load(&settings, repo.repo_path().clone());
     assert_eq!(reloaded_repo.op_id(), &op_id);
     // Reload once more to make sure that the .jj/op_heads/ directory was updated
     // correctly.
-    let reloaded_repo = ReadonlyRepo::load(&settings, repo.repo_path().clone()).unwrap();
+    let reloaded_repo = ReadonlyRepo::load(&settings, repo.repo_path().clone());
     assert_eq!(reloaded_repo.op_id(), &op_id);
 }

lib/tests/test_commit_concurrent.rs

@@ -84,7 +84,7 @@ fn test_commit_parallel_instances(use_git: bool) {
     let mut threads = vec![];
     for _ in 0..num_threads {
         let settings = settings.clone();
-        let repo = ReadonlyRepo::load(&settings, repo.repo_path().clone()).unwrap();
+        let repo = ReadonlyRepo::load(&settings, repo.repo_path().clone());
         let handle = thread::spawn(move || {
             let mut tx = repo.start_transaction("test");
             testutils::create_random_commit(&settings, &repo).write_to_repo(tx.mut_repo());
@@ -97,7 +97,7 @@ fn test_commit_parallel_instances(use_git: bool) {
     }
     // One commit per thread plus the commit from the initial checkout on top of the
     // root commit
-    let repo = ReadonlyRepo::load(&settings, repo.repo_path().clone()).unwrap();
+    let repo = ReadonlyRepo::load(&settings, repo.repo_path().clone());
     assert_eq!(repo.view().heads().len(), num_threads + 1);
 
     // One operation for initializing the repo (containing the root id and the

lib/tests/test_index.rs

@@ -262,7 +262,7 @@ fn test_index_commits_previous_operations(use_git: bool) {
     std::fs::remove_dir_all(&index_operations_dir).unwrap();
     std::fs::create_dir(&index_operations_dir).unwrap();
 
-    let repo = ReadonlyRepo::load(&settings, repo.repo_path().clone()).unwrap();
+    let repo = ReadonlyRepo::load(&settings, repo.repo_path().clone());
     let index = repo.index();
     // There should be the root commit and the working copy commit, plus
     // 3 more
@@ -309,7 +309,7 @@ fn test_index_commits_incremental(use_git: bool) {
     let commit_c = child_commit(&settings, &repo, &commit_b).write_to_repo(tx.mut_repo());
     tx.commit();
 
-    let repo = ReadonlyRepo::load(&settings, repo.repo_path().clone()).unwrap();
+    let repo = ReadonlyRepo::load(&settings, repo.repo_path().clone());
     let index = repo.index();
     // There should be the root commit and the working copy commit, plus
     // 3 more
@@ -354,7 +354,7 @@ fn test_index_commits_incremental_empty_transaction(use_git: bool) {
 
     repo.start_transaction("test").commit();
 
-    let repo = ReadonlyRepo::load(&settings, repo.repo_path().clone()).unwrap();
+    let repo = ReadonlyRepo::load(&settings, repo.repo_path().clone());
     let index = repo.index();
     // There should be the root commit and the working copy commit, plus
     // 1 more

lib/tests/test_load_repo.rs

@@ -12,37 +12,10 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-use jujutsu_lib::repo::{ReadonlyRepo, RepoLoadError, RepoLoader};
+use jujutsu_lib::repo::RepoLoader;
 use jujutsu_lib::testutils;
 use test_case::test_case;
 
-#[test]
-fn test_load_bad_path() {
-    let settings = testutils::user_settings();
-    let temp_dir = tempfile::tempdir().unwrap();
-    let wc_path = temp_dir.path().to_owned();
-    // We haven't created a repo in the wc_path, so it should fail to load.
-    let result = ReadonlyRepo::load(&settings, wc_path.clone());
-    assert_eq!(result.err(), Some(RepoLoadError::NoRepoHere(wc_path)));
-}
-
-#[test_case(false ; "local backend")]
-#[test_case(true ; "git backend")]
-fn test_load_from_subdir(use_git: bool) {
-    let settings = testutils::user_settings();
-    let test_workspace = testutils::init_repo(&settings, use_git);
-    let repo = &test_workspace.repo;
-    let workspace_root = test_workspace.workspace.workspace_root().clone();
-
-    let subdir = workspace_root.join("dir").join("subdir");
-    std::fs::create_dir_all(subdir.clone()).unwrap();
-    let same_repo = ReadonlyRepo::load(&settings, subdir);
-    assert!(same_repo.is_ok());
-    let same_repo = same_repo.unwrap();
-    assert_eq!(same_repo.repo_path(), repo.repo_path());
-    assert_eq!(same_repo.working_copy_path(), repo.working_copy_path());
-}
-
 #[test_case(false ; "local backend")]
 #[test_case(true ; "git backend")]
 fn test_load_at_operation(use_git: bool) {
@@ -60,13 +33,13 @@ fn test_load_at_operation(use_git: bool) {
 
     // If we load the repo at head, we should not see the commit since it was
     // removed
-    let loader = RepoLoader::init(&settings, repo.repo_path().clone()).unwrap();
+    let loader = RepoLoader::init(&settings, repo.repo_path().clone());
     let head_repo = loader.load_at_head();
     assert!(!head_repo.view().heads().contains(commit.id()));
 
     // If we load the repo at the previous operation, we should see the commit since
     // it has not been removed yet
-    let loader = RepoLoader::init(&settings, repo.repo_path().clone()).unwrap();
+    let loader = RepoLoader::init(&settings, repo.repo_path().clone());
     let old_repo = loader.load_at(repo.operation());
     assert!(old_repo.view().heads().contains(commit.id()));
 }

lib/tests/test_workspace.rs

@@ -0,0 +1,46 @@
+// Copyright 2021 Google LLC
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// https://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+use jujutsu_lib::testutils;
+use jujutsu_lib::workspace::{Workspace, WorkspaceLoadError};
+use test_case::test_case;
+
+#[test]
+fn test_load_bad_path() {
+    let settings = testutils::user_settings();
+    let temp_dir = tempfile::tempdir().unwrap();
+    let workspace_root = temp_dir.path().to_owned();
+    // We haven't created a repo in the workspace_root, so it should fail to load.
+    let result = Workspace::load(&settings, workspace_root.clone());
+    assert_eq!(
+        result.err(),
+        Some(WorkspaceLoadError::NoWorkspaceHere(workspace_root))
+    );
+}
+
+#[test_case(false ; "local backend")]
+#[test_case(true ; "git backend")]
+fn test_load_from_subdir(use_git: bool) {
+    let settings = testutils::user_settings();
+    let test_workspace = testutils::init_repo(&settings, use_git);
+    let workspace = &test_workspace.workspace;
+
+    let subdir = workspace.workspace_root().join("dir").join("subdir");
+    std::fs::create_dir_all(subdir.clone()).unwrap();
+    let same_workspace = Workspace::load(&settings, subdir);
+    assert!(same_workspace.is_ok());
+    let same_workspace = same_workspace.unwrap();
+    assert_eq!(same_workspace.repo_path(), workspace.repo_path());
+    assert_eq!(same_workspace.workspace_root(), workspace.workspace_root());
+}

src/commands.rs

@@ -45,7 +45,7 @@ use jujutsu_lib::op_heads_store::OpHeadsStore;
 use jujutsu_lib::op_store::{OpStore, OpStoreError, OperationId, RefTarget};
 use jujutsu_lib::operation::Operation;
 use jujutsu_lib::refs::{classify_branch_push_action, BranchPushAction};
-use jujutsu_lib::repo::{MutableRepo, ReadonlyRepo, RepoInitError, RepoLoadError, RepoRef};
+use jujutsu_lib::repo::{MutableRepo, ReadonlyRepo, RepoInitError, RepoRef};
 use jujutsu_lib::repo_path::RepoPath;
 use jujutsu_lib::revset::{RevsetError, RevsetExpression, RevsetParseError};
 use jujutsu_lib::revset_graph_iterator::RevsetGraphEdgeType;
@@ -55,7 +55,7 @@ use jujutsu_lib::store::Store;
 use jujutsu_lib::transaction::Transaction;
 use jujutsu_lib::tree::TreeDiffIterator;
 use jujutsu_lib::working_copy::{CheckoutStats, WorkingCopy};
-use jujutsu_lib::workspace::Workspace;
+use jujutsu_lib::workspace::{Workspace, WorkspaceLoadError};
 use jujutsu_lib::{conflicts, diff, files, git, revset, tree};
 use maplit::{hashmap, hashset};
 use pest::Parser;
@@ -154,7 +154,7 @@ impl<'args> CommandHelper<'args> {
         let wc_path = ui.cwd().join(wc_path_str);
         let workspace = match Workspace::load(ui.settings(), wc_path) {
             Ok(workspace) => workspace,
-            Err(RepoLoadError::NoRepoHere(wc_path)) => {
+            Err(WorkspaceLoadError::NoWorkspaceHere(wc_path)) => {
                 let mut message = format!("There is no jj repo in \"{}\"", wc_path_str);
                 let git_dir = wc_path.join(".git");
                 if git_dir.is_dir() {