Fix #129: unwritable parent directory in graveyard

src/lib.rs

@@ -360,32 +360,43 @@ fn build_dirs_to_create_from_graveyard(
     dirs_to_create
 }
 
-/// Create directories with specified permissions
-fn create_dirs_with_permissions(dirs_to_create: &[DirToCreate]) -> Result<(), Error> {
-    // Create directories one by one in order (parent to child)
-    // This assumes dirs_to_create is ordered from root to leaf
+/// Create the missing directories needed for a copy operation.
+///
+/// Important: permissions are applied *after* the copy finishes; otherwise a non-writable parent
+/// (e.g. mode 0555) can prevent creating deeper directories or writing the file itself.
+fn create_dirs_for_copy(dirs_to_create: &[DirToCreate]) -> Result<Vec<DirToCreate>, Error> {
+    let mut created = Vec::new();
+
+    // Create directories one by one in order (parent to child).
+    // This assumes `dirs_to_create` is ordered from root to leaf.
     for dir in dirs_to_create {
-        if !dir.path.exists() {
-            // Create just this directory (parent should already exist)
-            fs::create_dir(&dir.path).map_err(|e| {
+        if dir.path.exists() {
+            continue;
+        }
+
+        fs::create_dir(&dir.path).map_err(|e| {
+            Error::new(
+                e.kind(),
+                format!("Failed to create directory {}: {}", dir.path.display(), e),
+            )
+        })?;
+        created.push(dir.clone());
+    }
+
+    Ok(created)
+}
+
+fn apply_dir_permissions(dirs: &[DirToCreate]) -> Result<(), Error> {
+    for dir in dirs.iter().rev() {
+        if let Some(perms) = &dir.permissions {
+            fs::set_permissions(&dir.path, perms.clone()).map_err(|e| {
                 Error::new(
                     e.kind(),
-                    format!("Failed to create directory {}: {}", dir.path.display(), e),
+                    format!("Failed to set permissions on {}: {}", dir.path.display(), e),
                 )
             })?;
-
-            // Set permissions if we have them
-            if let Some(perms) = &dir.permissions {
-                fs::set_permissions(&dir.path, perms.clone()).map_err(|e| {
-                    Error::new(
-                        e.kind(),
-                        format!("Failed to set permissions on {}: {}", dir.path.display(), e),
-                    )
-                })?;
-            }
         }
     }
-
     Ok(())
 }
 
@@ -408,10 +419,12 @@ pub fn move_target(
     }
 
     // If that didn't work, then we need to copy and rm.
-    create_dirs_with_permissions(dirs_to_create)?;
+    let created_dirs = create_dirs_for_copy(dirs_to_create)?;
 
     if fs::symlink_metadata(target)?.is_dir() {
-        move_dir(target, dest, mode, stream, force)
+        let moved = move_dir(target, dest, mode, stream, force)?;
+        apply_dir_permissions(&created_dirs)?;
+        Ok(moved)
     } else {
         let moved = copy_file(target, dest, mode, stream, force).map_err(|e| {
             Error::new(
@@ -429,6 +442,7 @@ pub fn move_target(
                 format!("Failed to remove file: {}", target.display()),
             )
         })?;
+        apply_dir_permissions(&created_dirs)?;
         Ok(moved)
     }
 }
@@ -442,6 +456,8 @@ pub fn move_dir(
     stream: &mut impl Write,
     force: bool,
 ) -> Result<bool, Error> {
+    let mut dest_dirs_and_perms: Vec<(PathBuf, fs::Permissions)> = Vec::new();
+
     // Walk the source, creating directories and copying files as needed
     for entry in WalkDir::new(target).into_iter().filter_map(Result::ok) {
         // Path without the top-level directory
@@ -463,20 +479,15 @@ pub fn move_dir(
                 )
             })?;
 
-            // Preserve directory permissions
+            // Preserve directory permissions, but apply after traversal so we can
+            // still create children under non-writable directories.
             let source_metadata = fs::metadata(entry.path()).map_err(|e| {
                 Error::new(
                     e.kind(),
                     format!("Failed to get metadata for: {}", entry.path().display()),
                 )
             })?;
-            let source_perms = source_metadata.permissions();
-            fs::set_permissions(&dest_dir, source_perms).map_err(|e| {
-                Error::new(
-                    e.kind(),
-                    format!("Failed to set permissions on: {}", dest_dir.display()),
-                )
-            })?;
+            dest_dirs_and_perms.push((dest_dir, source_metadata.permissions()));
         } else {
             copy_file(entry.path(), &dest.join(orphan), mode, stream, force).map_err(|e| {
                 Error::new(
@@ -497,6 +508,16 @@ pub fn move_dir(
         )
     })?;
 
+    // Apply collected perms from leaf to root to minimize traversal surprises.
+    for (dest_dir, perms) in dest_dirs_and_perms.into_iter().rev() {
+        fs::set_permissions(&dest_dir, perms).map_err(|e| {
+            Error::new(
+                e.kind(),
+                format!("Failed to set permissions on: {}", dest_dir.display()),
+            )
+        })?;
+    }
+
     Ok(true)
 }
 
@@ -571,23 +592,20 @@ pub fn copy_file(
 }
 
 pub fn get_graveyard(graveyard: Option<PathBuf>) -> PathBuf {
-    graveyard.map_or_else(
-        || {
-            if let Ok(env_graveyard) = env::var("RIP_GRAVEYARD") {
-                PathBuf::from(env_graveyard)
-            } else if let Ok(mut env_graveyard) = env::var("XDG_DATA_HOME") {
-                if !env_graveyard.ends_with(std::path::MAIN_SEPARATOR) {
-                    env_graveyard.push(std::path::MAIN_SEPARATOR);
-                }
-                env_graveyard.push_str("graveyard");
-                PathBuf::from(env_graveyard)
-            } else {
-                let user = util::get_user();
-                env::temp_dir().join(format!("graveyard-{user}"))
+    graveyard.unwrap_or_else(|| {
+        if let Ok(env_graveyard) = env::var("RIP_GRAVEYARD") {
+            PathBuf::from(env_graveyard)
+        } else if let Ok(mut env_graveyard) = env::var("XDG_DATA_HOME") {
+            if !env_graveyard.ends_with(std::path::MAIN_SEPARATOR) {
+                env_graveyard.push(std::path::MAIN_SEPARATOR);
             }
-        },
-        |flag| flag,
-    )
+            env_graveyard.push_str("graveyard");
+            PathBuf::from(env_graveyard)
+        } else {
+            let user = util::get_user();
+            env::temp_dir().join(format!("graveyard-{user}"))
+        }
+    })
 }
 
 /// Testing module for exposing internal functions to unit tests.

src/main.rs

@@ -14,8 +14,8 @@ fn main() -> ExitCode {
     match &cli.command {
         Some(Commands::Completions { shell }) => {
             let result = completions::generate_shell_completions(shell, &mut io::stdout());
-            if result.is_err() {
-                eprintln!("{}", result.unwrap_err());
+            if let Err(e) = result {
+                eprintln!("{e}");
                 return ExitCode::FAILURE;
             }
         }

tests/integration_tests.rs

@@ -1887,3 +1887,76 @@ fn test_unbury_directory_permissions(
         );
     }
 }
+
+#[cfg(unix)]
+#[test]
+fn test_issue_129_readonly_parent_dir_breaks_first_bury() {
+    struct ScopedEnv {
+        saved_env_vars: [Option<String>; 2],
+        saved_allow_rename: Option<std::ffi::OsString>,
+    }
+
+    impl Drop for ScopedEnv {
+        fn drop(&mut self) {
+            std::env::remove_var("__RIP_ALLOW_RENAME");
+            if let Some(v) = self.saved_allow_rename.clone() {
+                std::env::set_var("__RIP_ALLOW_RENAME", v);
+            }
+            restore_env_vars(self.saved_env_vars.clone());
+        }
+    }
+
+    let _env_lock = aquire_lock();
+    let scoped = ScopedEnv {
+        saved_env_vars: cache_and_remove_env_vars(),
+        saved_allow_rename: std::env::var_os("__RIP_ALLOW_RENAME"),
+    };
+
+    // Force the copy path (so directory creation happens before copying).
+    std::env::set_var("__RIP_ALLOW_RENAME", "false");
+
+    let tmp = tempdir().unwrap();
+    std::env::set_var("XDG_DATA_HOME", tmp.path().join("xdg-data-home"));
+    let graveyard = rip2::get_graveyard(None);
+
+    let src_root = tmp.path().join("src");
+    let ro_parent = src_root.join("readonly_parent");
+    let child_dir = ro_parent.join("child");
+    fs::create_dir_all(&child_dir).unwrap();
+
+    let file_path = child_dir.join("somefile.txt");
+    fs::write(&file_path, b"hello\n").unwrap();
+
+    // Read-only intermediate dir; rip2 propagates this into the graveyard,
+    // but should still be able to create deeper mirrored directories.
+    fs::set_permissions(&ro_parent, fs::Permissions::from_mode(0o555)).unwrap();
+
+    let mut log = Vec::new();
+    let res = rip2::run(
+        &Args {
+            targets: vec![file_path],
+            ..Args::default()
+        },
+        TestMode,
+        &mut log,
+    );
+
+    drop(scoped);
+    res.expect("bury should succeed even if an intermediate source dir is 0555");
+
+    let ro_parent_abs = dunce::canonicalize(&ro_parent).unwrap();
+    let grave_ro_parent = util::join_absolute(&graveyard, ro_parent_abs);
+    assert!(grave_ro_parent.exists(), "mirrored dir should exist");
+
+    let grave_child_dir = grave_ro_parent.join("child");
+    assert!(
+        grave_child_dir.exists(),
+        "child dir should be creatable under mirrored 0555 dir"
+    );
+
+    let grave_file = grave_child_dir.join("somefile.txt");
+    assert!(grave_file.exists(), "file should be copied into graveyard");
+
+    let mode = fs::metadata(&grave_ro_parent).unwrap().permissions().mode() & 0o777;
+    assert_eq!(mode, 0o555, "mirrored dir should retain 0555 perms");
+}