From 78b0f36e30145f828025c72e90edb598c18b9274 Mon Sep 17 00:00:00 2001 From: Didn't read the style guide <50516935+lordkekz@users.noreply.github.com> Date: Mon, 18 Nov 2024 20:28:27 +0100 Subject: [PATCH] Convert creates tempdir in parent of destination (#99) Fixes issue https://github.com/9999years/git-prole/issues/96 I tested the change on some repos (no longer gives the error) and ran the tests with `nix flake check`. Please do review this carefully since I'm still pretty new to Rust. ^^ ### Change summary `git prole convert` now creates a tempdir in the parent of the determined destination of the converted repo. This prevents some errors when the /tmp directory is on a different filesystem, such as a tmpfs. The tempdir will be deleted if and only if it is empty after conversion. This should always be the case and is to prevent any data loss. --------- Co-authored-by: Rebecca Turner --- clippy.toml | 8 ++++++++ src/convert.rs | 37 ++++++++++++++++++++++++++++++++++++- src/fs.rs | 10 ++++++++++ src/path_display.rs | 7 +++++-- src/utf8tempdir.rs | 4 ++-- test-harness/src/lib.rs | 2 +- 6 files changed, 62 insertions(+), 6 deletions(-) diff --git a/clippy.toml b/clippy.toml index 53284b8..e354cef 100644 --- a/clippy.toml +++ b/clippy.toml @@ -62,6 +62,14 @@ reason = "Use git_prole::fs::read_to_string" path = "std::fs::read_to_string" reason = "Use git_prole::fs::read_to_string" +[[disallowed-methods]] +path = "fs_err::read_dir" +reason = "Use git_prole::fs::read_dir" + +[[disallowed-methods]] +path = "std::fs::read_dir" +reason = "Use git_prole::fs::read_dir" + [[disallowed-methods]] path = "fs_err::copy" reason = "Use git_prole::fs::copy" diff --git a/src/convert.rs b/src/convert.rs index 8e01208..b0eddac 100644 --- a/src/convert.rs +++ b/src/convert.rs @@ -211,7 +211,6 @@ where // suffix removed // - Otherwise just use the git dir path. - let tempdir = Utf8TempDir::new()?.into_path(); // Tests: // - `convert_from_bare` // - `convert_bare_dot_git` @@ -224,8 +223,14 @@ where let destination_name = destination .file_name() .ok_or_else(|| miette!("Destination has no basename: {destination}"))?; + let destination_parent = destination + .parent() + .ok_or_else(|| miette!("Destination has no parent: {destination}"))? + .to_path_buf(); tracing::debug!(%destination, "Destination determined"); + let tempdir = Utf8TempDir::new(&destination_parent)?.into_path(); + let default_branch = match opts.default_branch { // Tests: // - `convert_explicit_default_branch` @@ -550,6 +555,8 @@ where ); tracing::info!("You may need to `cd .` to refresh your shell"); + remove_tempdir_if_empty(&self.tempdir)?; + Ok(()) } @@ -663,3 +670,31 @@ impl MainWorktreePlan { self.inner.destination(convert_plan).join(".git") } } + +fn remove_tempdir_if_empty(tempdir: &Utf8Path) -> miette::Result<()> { + let contents = fs::read_dir(tempdir)?.collect::>(); + // From `std::fs::read_dir` documentation: + // > Entries for the current and parent directories (typically . and ..) are skipped. + if !contents.is_empty() { + tracing::warn!( + "Temporary directory isn't empty: {}\n{}", + tempdir.display_path_cwd(), + display_dir_contents(&contents) + ); + } else { + fs::remove_dir(tempdir)?; + } + + Ok(()) +} + +fn display_dir_contents(contents: &[std::io::Result]) -> String { + format_bulleted_list(contents.iter().map(|item| { + match item { + Ok(entry) => entry.file_name().display_path_cwd(), + Err(error) => error + .if_supports_color(Stream::Stdout, |text| text.red()) + .to_string(), + } + })) +} diff --git a/src/fs.rs b/src/fs.rs index 3d7ead4..955c6a3 100644 --- a/src/fs.rs +++ b/src/fs.rs @@ -3,6 +3,7 @@ use std::fmt::Debug; use std::path::Path; +use std::path::PathBuf; use miette::IntoDiagnostic; use tracing::instrument; @@ -72,3 +73,12 @@ where #[expect(clippy::disallowed_methods)] fs_err::write(path, contents).into_diagnostic() } + +#[instrument(level = "trace")] +pub fn read_dir

(path: P) -> miette::Result +where + P: Into + Debug, +{ + #[expect(clippy::disallowed_methods)] + fs_err::read_dir(path).into_diagnostic() +} diff --git a/src/path_display.rs b/src/path_display.rs index c571b1d..c9cdd88 100644 --- a/src/path_display.rs +++ b/src/path_display.rs @@ -30,10 +30,13 @@ pub trait PathDisplay: Debug + AsRef { impl

PathDisplay for P where - P: AsRef + AsRef + Debug, + P: AsRef + Debug, { fn display_path_from(&self, base: impl AsRef) -> String { - try_display(self, base).unwrap_or_else(|| display_backup(self)) + let path = self.as_ref(); + Utf8Path::from_path(path) + .and_then(|utf8path| try_display(utf8path, base)) + .unwrap_or_else(|| display_backup(self)) } } diff --git a/src/utf8tempdir.rs b/src/utf8tempdir.rs index 9081a9d..cad6914 100644 --- a/src/utf8tempdir.rs +++ b/src/utf8tempdir.rs @@ -14,8 +14,8 @@ pub struct Utf8TempDir { } impl Utf8TempDir { - pub fn new() -> miette::Result { - let inner = tempfile::tempdir().into_diagnostic()?; + pub fn new(parent_dir: &Utf8PathBuf) -> miette::Result { + let inner = tempfile::tempdir_in(parent_dir).into_diagnostic()?; let path = inner.path().to_owned().try_into().into_diagnostic()?; Ok(Self { inner: Some(inner), diff --git a/test-harness/src/lib.rs b/test-harness/src/lib.rs index cdd3672..dbe4baa 100644 --- a/test-harness/src/lib.rs +++ b/test-harness/src/lib.rs @@ -29,7 +29,7 @@ pub struct GitProle { impl GitProle { pub fn new() -> miette::Result { - let mut tempdir = Utf8TempDir::new()?; + let mut tempdir = Utf8TempDir::new(&Utf8PathBuf::from("./"))?; if std::env::var("KEEP_TEMP").is_ok() { tempdir.persist();