Properly handle deletion of nested worktrees

Cargo.lock

@@ -80,9 +80,9 @@ checksum = "baf1de4339761588bc0619e3cbc0120ee582ebb74b53b4efbf79117bd2da40fd"
 
 [[package]]
 name = "clap"
-version = "3.2.6"
+version = "3.2.7"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "9f1fe12880bae935d142c8702d500c63a4e8634b6c3c57ad72bf978fc7b6249a"
+checksum = "5b7b16274bb247b45177db843202209b12191b631a14a9d06e41b3777d6ecf14"
 dependencies = [
  "atty",
  "bitflags",
@@ -97,9 +97,9 @@ dependencies = [
 
 [[package]]
 name = "clap_derive"
-version = "3.2.6"
+version = "3.2.7"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "ed6db9e867166a43a53f7199b5e4d1f522a1e5bd626654be263c999ce59df39a"
+checksum = "759bf187376e1afa7b85b959e6a664a3e7a95203415dba952ad19139e798f902"
 dependencies = [
  "heck",
  "proc-macro-error",
@@ -110,9 +110,9 @@ dependencies = [
 
 [[package]]
 name = "clap_lex"
-version = "0.2.3"
+version = "0.2.4"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "87eba3c8c7f42ef17f6c659fc7416d0f4758cd3e58861ee63c5fa4a4dde649e4"
+checksum = "2850f2f5a82cbf437dd5af4d49848fbdfc27c157c3d010345776f952765261c5"
 dependencies = [
  "os_str_bytes",
 ]
@@ -535,9 +535,9 @@ dependencies = [
 
 [[package]]
 name = "linked-hash-map"
-version = "0.5.4"
+version = "0.5.6"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "7fb9b38af92608140b86b693604b9ffcc5824240a484d1ecd4795bacb2fe88f3"
+checksum = "0717cef1bc8b636c6e1c1bbdefc09e6322da8a9321966e8928ef80d20f7f770f"
 
 [[package]]
 name = "lock_api"
@@ -895,9 +895,9 @@ dependencies = [
 
 [[package]]
 name = "serde_json"
-version = "1.0.81"
+version = "1.0.82"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "9b7ce2b32a1aed03c558dc61a5cd328f15aff2dbc17daad8fb8af04d2100e15c"
+checksum = "82c2c1fdcd807d1098552c5b9a36e425e42e9fbd7c6a37a8425f390f781f7fa7"
 dependencies = [
  "itoa",
  "ryu",
@@ -974,9 +974,9 @@ dependencies = [
 
 [[package]]
 name = "smallvec"
-version = "1.8.0"
+version = "1.8.1"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "f2dd574626839106c320a323308629dcb1acfc96e32a8cba364ddc61ac23ee83"
+checksum = "cc88c725d61fc6c3132893370cac4a0200e3fedf5da8331c570664b1987f5ca2"
 
 [[package]]
 name = "socket2"
@@ -1002,9 +1002,9 @@ checksum = "063e6045c0e62079840579a7e47a355ae92f60eb74daaf156fb1e84ba164e63f"
 
 [[package]]
 name = "strum_macros"
-version = "0.24.0"
+version = "0.24.2"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "6878079b17446e4d3eba6192bb0a2950d5b14f0ed8424b852310e5a94345d0ef"
+checksum = "4faebde00e8ff94316c01800f9054fd2ba77d30d9e922541913051d1d978918b"
 dependencies = [
  "heck",
  "proc-macro2",
@@ -1129,9 +1129,9 @@ dependencies = [
 
 [[package]]
 name = "tracing-core"
-version = "0.1.27"
+version = "0.1.28"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "7709595b8878a4965ce5e87ebf880a7d39c9afc6837721b21a5a816a8117d921"
+checksum = "7b7358be39f2f274f322d2aaed611acc57f382e8eb1e5b48cb9ae30933495ce7"
 dependencies = [
  "once_cell",
 ]
@@ -1160,9 +1160,9 @@ checksum = "5bd2fe26506023ed7b5e1e315add59d6f584c621d037f9368fea9cfb988f368c"
 
 [[package]]
 name = "unicode-normalization"
-version = "0.1.19"
+version = "0.1.20"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "d54590932941a9e9266f0832deed84ebe1bf2e4c9e4a3554d393d18f5e854bf9"
+checksum = "81dee68f85cab8cf68dec42158baf3a79a1cdc065a8b103025965d6ccb7f6cbd"
 dependencies = [
  "tinyvec",
 ]

Cargo.toml

@@ -28,7 +28,7 @@ rust-version = "1.57"
 license = "GPL-3.0-only"
 
 [profile.e2e-tests]
-inherits = "release"
+inherits = "dev"
 
 [lib]
 name = "grm"
@@ -54,7 +54,7 @@ version = "=0.14.4"
 version = "=2.1.0"
 
 [dependencies.clap]
-version = "=3.2.6"
+version = "=3.2.7"
 features = ["derive", "cargo"]
 
 [dependencies.console]
@@ -70,7 +70,7 @@ version = "=6.0.0"
 version = "=0.8.24"
 
 [dependencies.serde_json]
-version = "=1.0.81"
+version = "=1.0.82"
 
 [dependencies.isahc]
 version = "=1.7.2"

e2e_tests/test_worktrees.py

@@ -9,13 +9,21 @@ import datetime
 import os.path
 
 
-@pytest.mark.parametrize("config_enabled", [True, False])
+@pytest.mark.parametrize(
-@pytest.mark.parametrize("config_has_default_remote_prefix", [True, False])
+    "config_setup",
-@pytest.mark.parametrize("config_has_default_track_enabled", [True, False])
+    (
+        (False, False, False),
+        (True, False, False),
+        (True, False, True),
+        (True, True, False),
+        (True, True, True),
+    ),
+)
 @pytest.mark.parametrize("explicit_notrack", [True, False])
 @pytest.mark.parametrize("explicit_track", [True, False])
-@pytest.mark.parametrize("local_branch_exists", [True, False])
+@pytest.mark.parametrize(
-@pytest.mark.parametrize("local_branch_has_tracking_branch", [True, False])
+    "local_branch_setup", ((False, False), (True, False), (True, True))
+)
 @pytest.mark.parametrize("remote_branch_already_exists", [True, False])
 @pytest.mark.parametrize("remote_branch_with_prefix_already_exists", [True, False])
 @pytest.mark.parametrize(
@@ -32,13 +40,10 @@ import os.path
 @pytest.mark.parametrize("track_differs_from_existing_branch_upstream", [True, False])
 @pytest.mark.parametrize("worktree_with_slash", [True, False])
 def test_worktree_add(
-    config_enabled,
+    config_setup,
-    config_has_default_remote_prefix,
-    config_has_default_track_enabled,
     explicit_notrack,
     explicit_track,
-    local_branch_exists,
+    local_branch_setup,
-    local_branch_has_tracking_branch,
     remote_branch_already_exists,
     remote_branch_with_prefix_already_exists,
     remote_setup,
@@ -46,6 +51,12 @@ def test_worktree_add(
     worktree_with_slash,
 ):
     (remote_count, default_remote, remotes_differ) = remote_setup
+    (
+        config_enabled,
+        config_has_default_remote_prefix,
+        config_has_default_track_enabled,
+    ) = config_setup
+    (local_branch_exists, local_branch_has_tracking_branch) = local_branch_setup
     has_remotes = True if remote_count > 0 else False
 
     if worktree_with_slash:
@@ -578,6 +589,30 @@ def test_worktree_delete():
         assert "test" not in [str(b) for b in repo.branches]
 
 
+@pytest.mark.parametrize("has_other_worktree", [True, False])
+def test_worktree_delete_in_subfolder(has_other_worktree):
+    with TempGitRepositoryWorktree.get(funcname()) as (base_dir, _commit):
+        cmd = grm(["wt", "add", "dir/test", "--track", "origin/test"], cwd=base_dir)
+        assert cmd.returncode == 0
+        assert "dir" in os.listdir(base_dir)
+
+        if has_other_worktree is True:
+            cmd = grm(
+                ["wt", "add", "dir/test2", "--track", "origin/test"], cwd=base_dir
+            )
+            assert cmd.returncode == 0
+            assert {"test", "test2"} == set(os.listdir(os.path.join(base_dir, "dir")))
+        else:
+            assert {"test"} == set(os.listdir(os.path.join(base_dir, "dir")))
+
+        cmd = grm(["wt", "delete", "dir/test"], cwd=base_dir)
+        assert cmd.returncode == 0
+        if has_other_worktree is True:
+            assert {"test2"} == set(os.listdir(os.path.join(base_dir, "dir")))
+        else:
+            assert "dir" not in os.listdir(base_dir)
+
+
 def test_worktree_delete_refusal_no_tracking_branch():
     with TempGitRepositoryWorktree.get(funcname()) as (base_dir, _commit):
         cmd = grm(["wt", "add", "test"], cwd=base_dir)

src/grm/main.rs

@@ -528,8 +528,6 @@ fn main() {
                     }
                 }
                 cmd::WorktreeAction::Delete(action_args) => {
-                    let worktree_dir = cwd.join(&action_args.name);
-
                     let worktree_config = match repo::read_worktree_root_config(&cwd) {
                         Ok(config) => config,
                         Err(error) => {
@@ -547,8 +545,9 @@ fn main() {
                     });
 
                     match repo.remove_worktree(
+                        &cwd,
                         &action_args.name,
-                        &worktree_dir,
+                        Path::new(&action_args.name),
                         action_args.force,
                         &worktree_config,
                     ) {

src/lib.rs

@@ -14,8 +14,6 @@ pub mod table;
 pub mod tree;
 pub mod worktree;
 
-const BRANCH_NAMESPACE_SEPARATOR: &str = "/";
-
 /// Find all git repositories under root, recursively
 ///
 /// The bool in the return value specifies whether there is a repository

src/repo.rs

@@ -1153,18 +1153,21 @@ impl RepoHandle {
 
     pub fn remove_worktree(
         &self,
+        base_dir: &Path,
         name: &str,
         worktree_dir: &Path,
         force: bool,
         worktree_config: &Option<WorktreeRootConfig>,
     ) -> Result<(), WorktreeRemoveFailureReason> {
-        if !worktree_dir.exists() {
+        let fullpath = base_dir.join(worktree_dir);
+
+        if !fullpath.exists() {
             return Err(WorktreeRemoveFailureReason::Error(format!(
                 "{} does not exist",
                 name
             )));
         }
-        let worktree_repo = RepoHandle::open(worktree_dir, false).map_err(|error| {
+        let worktree_repo = RepoHandle::open(&fullpath, false).map_err(|error| {
             WorktreeRemoveFailureReason::Error(format!("Error opening repo: {}", error))
         })?;
 
@@ -1176,12 +1179,11 @@ impl RepoHandle {
             WorktreeRemoveFailureReason::Error(format!("Failed getting name of branch: {}", error))
         })?;
 
-        if branch_name != name
+        if branch_name != name {
-            && !branch_name.ends_with(&format!("{}{}", super::BRANCH_NAMESPACE_SEPARATOR, name))
-        {
             return Err(WorktreeRemoveFailureReason::Error(format!(
-                "Branch \"{}\" is checked out in worktree, this does not look correct",
+                "Branch \"{}\" is checked out in worktree \"{}\", this does not look correct",
-                &branch_name
+                &branch_name,
+                &worktree_dir.display(),
             )));
         }
 
@@ -1251,13 +1253,47 @@ impl RepoHandle {
             }
         }
 
-        if let Err(e) = std::fs::remove_dir_all(&worktree_dir) {
+        // worktree_dir is a relative path, starting from base_dir. We walk it
+        // upwards (from subdirectory to parent directories) and remove each
+        // component, in case it is empty. Only the leaf directory can be
+        // removed unconditionally (as it contains the worktree itself).
+        if let Err(e) = std::fs::remove_dir_all(&fullpath) {
             return Err(WorktreeRemoveFailureReason::Error(format!(
                 "Error deleting {}: {}",
                 &worktree_dir.display(),
                 e
             )));
         }
+
+        if let Some(current_dir) = worktree_dir.parent() {
+            for current_dir in current_dir.ancestors() {
+                let current_dir = base_dir.join(current_dir);
+                println!("deleting {}", current_dir.display());
+                if current_dir
+                    .read_dir()
+                    .map_err(|error| {
+                        WorktreeRemoveFailureReason::Error(format!(
+                            "Error reading {}: {}",
+                            &current_dir.display(),
+                            error
+                        ))
+                    })?
+                    .next()
+                    .is_none()
+                {
+                    if let Err(e) = std::fs::remove_dir_all(&current_dir) {
+                        return Err(WorktreeRemoveFailureReason::Error(format!(
+                            "Error deleting {}: {}",
+                            &worktree_dir.display(),
+                            e
+                        )));
+                    }
+                } else {
+                    break;
+                }
+            }
+        }
+
         self.prune_worktree(name)
             .map_err(WorktreeRemoveFailureReason::Error)?;
         branch
@@ -1310,7 +1346,13 @@ impl RepoHandle {
         {
             let repo_dir = &directory.join(&worktree.name());
             if repo_dir.exists() {
-                match self.remove_worktree(worktree.name(), repo_dir, false, &config) {
+                match self.remove_worktree(
+                    directory,
+                    worktree.name(),
+                    Path::new(worktree.name()),
+                    false,
+                    &config,
+                ) {
                     Ok(_) => print_success(&format!("Worktree {} deleted", &worktree.name())),
                     Err(error) => match error {
                         WorktreeRemoveFailureReason::Changes(changes) => {