From ae234111459b94dbd58cdad53d1e12c4fbbae98e Mon Sep 17 00:00:00 2001 From: Xiaowei Lu Date: Mon, 2 Dec 2024 15:57:04 -0800 Subject: [PATCH] rust-remove: handle non-eden directory Summary: ## Background We are migrating eden remove command from Python to Rust for better error handling and code readability. ## This Diff Implemented how we should handle the request when the user is trying to remove a non-eden directory. The handling logic is the same as the Python CLI: - For a directory managed by Eden but not a root directory of the repo, we warn the user and do nothing. - For a directory that is not managed by Eden, we prompt the user and go removing it after getting the permission. Reviewed By: muirdm Differential Revision: D65802306 fbshipit-source-id: 1e55adeef550b09da89bab83f0fc4351b8aca8c2 --- eden/fs/cli_rs/edenfs-commands/src/remove.rs | 124 ++++++++++++++----- eden/scm/tests/test-eden-remove.t | 40 +++++- 2 files changed, 127 insertions(+), 37 deletions(-) diff --git a/eden/fs/cli_rs/edenfs-commands/src/remove.rs b/eden/fs/cli_rs/edenfs-commands/src/remove.rs index fcd7de7db8d10..1d6814f7bcaae 100644 --- a/eden/fs/cli_rs/edenfs-commands/src/remove.rs +++ b/eden/fs/cli_rs/edenfs-commands/src/remove.rs @@ -8,14 +8,13 @@ //! edenfsctl remove use std::fmt; use std::fs; +#[cfg(unix)] use std::fs::Permissions; use std::io::ErrorKind; #[cfg(unix)] use std::os::unix::fs::PermissionsExt; use std::path::Path; use std::path::PathBuf; -#[cfg(windows)] -use std::process::Command; use anyhow::anyhow; use anyhow::Context; @@ -32,8 +31,6 @@ use fail::fail_point; use io::IO; use termlogger::TermLogger; use tracing::debug; -use tracing::error; -use tracing::warn; use crate::ExitCode; use crate::Subcommand; @@ -107,13 +104,7 @@ impl SanityCheck { .with_context(|| format!("Error canonicalizing path {}", context.original_path))?; context.canonical_path = path; - if context.io.prompt_user(construct_start_prompt(context))? { - return Ok(Some(State::Determination(Determination {}))); - } - - Err(anyhow!( - "User did not confirm the removal. Stopping. Nothing removed!" - )) + Ok(Some(State::Determination(Determination {}))) } } @@ -134,38 +125,61 @@ impl Determination { debug!("{} is determined as a directory", context); - if self.is_active_eden_mount(context) { + if self.is_active_eden_mount(&context.canonical_path) { debug!("path {} is determined to be an active eden mount", context); return Ok(Some(State::ActiveEdenMount(ActiveEdenMount {}))); } - error!("Determination State for directory is not implemented!"); - Err(anyhow!("Rust remove(Determination) is not implemented!")) + debug!("{} is not an active eden mount", path.display()); + + // Check if it's a directory managed under eden + let mut path_copy = context.canonical_path.clone(); + loop { + if path_copy.pop() { + if self.is_active_eden_mount(&path_copy) { + return Err(anyhow!( + "{} is not the root of checkout {}, not deleting", + context, + path_copy.display() + )); + } else { + continue; + } + } + break; + } + + // Maybe it's a directory that is left after unmount + // If so, unregister it and clean from there + if path_in_eden_config(context.canonical_path.as_path()).await? { + return Ok(Some(State::InactiveEdenMount(InactiveEdenMount {}))); + } + + // It's a directory that is not listed inside config.json + // We don't know how to handle it properly, so move to "Unknown" state + // and try to handle from there with "the best efforts". + Ok(Some(State::Unknown(Unknown {}))) } #[cfg(unix)] - fn is_active_eden_mount(&self, context: &RemoveContext) -> bool { + fn is_active_eden_mount(&self, path: &Path) -> bool { // For Linux and Mac, an active Eden mount should have a dir named ".eden" under the // repo root and there should be a symlink named "root" which points to the repo root - let unix_eden_dot_dir_path = context.canonical_path.join(".eden").join("root"); + let unix_eden_dot_dir_path = path.join(".eden").join("root"); match unix_eden_dot_dir_path.canonicalize() { - Ok(resolved_path) => resolved_path == context.canonical_path, - Err(_) => { - warn!("{} is not an active eden mount", context); - false - } + Ok(resolved_path) => resolved_path == path, + _ => false, } } #[cfg(windows)] - fn is_active_eden_mount(&self, context: &RemoveContext) -> bool { + fn is_active_eden_mount(&self, path: &Path) -> bool { // For Windows, an active Eden mount should have a dir named ".eden" under the // repo and there should be a file named "config" under the ".eden" dir - let config_path = context.canonical_path.join(".eden").join("config"); + let config_path = path.join(".eden").join("config"); if !config_path.exists() { - warn!("{} is not an active eden mount", context); return false; } true @@ -196,6 +210,11 @@ struct ActiveEdenMount {} impl ActiveEdenMount { async fn next(&self, context: &mut RemoveContext) -> Result> { // TODO: stop process first + if !context.io.prompt_user(construct_start_prompt(context))? { + return Err(anyhow!( + "User did not confirm the removal. Stopping. Nothing removed!" + )); + } context .io @@ -278,7 +297,7 @@ impl CleanUp { Ok(None) } else { context.io.info(format!( - "Cleaning up the directory left by repo {} ...", + "Cleaning up the directory {} ...", context.original_path )); self.clean_mount_point(&context.canonical_path) @@ -325,12 +344,7 @@ async fn validate_state_run(context: &mut RemoveContext) -> Result .io .info("Checking eden mount list and file system to verify the removal...".to_string()); // check eden list - let mut mounts = get_mounts(EdenFsInstance::global()) - .await - .with_context(|| anyhow!("Failed to call eden list"))?; - let entry_key = dunce::simplified(context.canonical_path.as_path()); - mounts.retain(|mount_path_key, _| dunce::simplified(mount_path_key) == entry_key); - if !mounts.is_empty() { + if path_in_eden_config(context.canonical_path.as_path()).await? { return Err(anyhow!("Repo {} is still mounted", context)); } @@ -357,6 +371,37 @@ async fn validate_state_run(context: &mut RemoveContext) -> Result } } +#[derive(Debug)] +struct Unknown {} +impl Unknown { + async fn next(&self, context: &mut RemoveContext) -> Result> { + // If this directory is empty, we want to let the user know and let the + // user decide if we should proceed + // Otherwise, ask the user if we should try our best to remove it + let prompt = match fs::read_dir(context.canonical_path.as_path()) + .with_context(|| format!("Failed to list contents under {}", context))? + .count() + == 0 + { + true => format!("{} is not an eden mount and it's empty.", context), + false => format!( + "{} is a non-empty directory that is not an eden mount.\n\ + Any files in this directory will be lost forever. \n\ + Do you still want to remove it?", + context + ), + }; + + if context.io.prompt_user(prompt)? { + return Ok(Some(State::CleanUp(CleanUp {}))); + } + + Err(anyhow!( + "User did not confirm the removal. Stopping. Nothing removed!" + )) + } +} + #[derive(Debug)] enum State { // function states (no real action performed) @@ -364,12 +409,12 @@ enum State { Determination(Determination), Validation, - // // removal states (harmful operations) + // removal states (harmful operations) ActiveEdenMount(ActiveEdenMount), InactiveEdenMount(InactiveEdenMount), CleanUp(CleanUp), RegFile(RegFile), - // Unknown, + Unknown(Unknown), } impl fmt::Display for State { @@ -385,6 +430,7 @@ impl fmt::Display for State { State::CleanUp(_) => "CleanUp", State::InactiveEdenMount(_) => "InactiveEdenMount", State::Validation => "Validation", + State::Unknown(_) => "Unknown", } ) } @@ -410,6 +456,7 @@ impl State { State::InactiveEdenMount(inner) => inner.next(context).await, State::CleanUp(inner) => inner.next(context).await, State::Validation => validate_state_run(context).await, + State::Unknown(inner) => inner.next(context).await, } } } @@ -452,6 +499,16 @@ impl Subcommand for RemoveCmd { } } +async fn path_in_eden_config(path: &Path) -> Result { + let mut mounts = get_mounts(EdenFsInstance::global()) + .await + .with_context(|| anyhow!("Failed to call eden list"))?; + let entry_key = dunce::simplified(path); + mounts.retain(|mount_path_key, _| dunce::simplified(mount_path_key) == entry_key); + + Ok(!mounts.is_empty()) +} + // Object responsible to print messages to stdout or generate prompt // for the user and receive response struct Messenger { @@ -622,6 +679,5 @@ mod tests { matches!(state, State::Determination(_)), "Expected Determination state" ); - assert!(state.run(&mut dir_context).await.is_err()); } } diff --git a/eden/scm/tests/test-eden-remove.t b/eden/scm/tests/test-eden-remove.t index 708c17791d4c5..c2cf55cf957db 100644 --- a/eden/scm/tests/test-eden-remove.t +++ b/eden/scm/tests/test-eden-remove.t @@ -34,13 +34,47 @@ file is now gone ls: $TESTTMP/wcrepo/file.txt: $ENOENT$ [1] -create a test directory +create a directory outside of any eden repo + $ mkdir $TESTTMP/i-am-not-eden + +eden remove this directory, answer "no" when there is prompt + $ EDENFSCTL_ONLY_RUST=true eden remove -q -n $TESTTMP/i-am-not-eden + Error: User did not confirm the removal. Stopping. Nothing removed! + [1] + +the directory should still be there + $ ls $TESTTMP/i-am-not-eden | wc -l + 0 + +eden remove this directory, answer "yes" when there is prompt + $ EDENFSCTL_ONLY_RUST=true eden remove -q -y $TESTTMP/i-am-not-eden + +the directory should now be gone + $ ls $TESTTMP/i-am-not-eden + ls: $TESTTMP/i-am-not-eden: $ENOENT$ + [1] + +create the directory outside of any eden repo again + $ mkdir $TESTTMP/i-am-not-eden + +this time, put some files into it + $ touch $TESTTMP/i-am-not-eden/file + +eden remove this directory, answer "yes" when there is prompt + $ EDENFSCTL_ONLY_RUST=true eden remove -q -y $TESTTMP/i-am-not-eden + +the directory should also be gone now + $ ls $TESTTMP/i-am-not-eden + ls: $TESTTMP/i-am-not-eden: $ENOENT$ + [1] + +create a test directory inside eden $ mkdir $TESTTMP/wcrepo/test_dir -eden remove this directory should also see error about Determination state +eden remove this directory should just give error since it's not the root dir $ EDENFSCTL_ONLY_RUST=true eden remove -q -y $TESTTMP/wcrepo/test_dir - Error: Rust remove(Determination) is not implemented! + Error: $TESTTMP/wcrepo/test_dir is not the root of checkout $TESTTMP/wcrepo, not deleting [1] eden list should give two repos