Download/clone sources to a temporary path first

This fixes an issue where if someone tried to reinstall they would be
left without any plugins because they would all be nuked up front prior
to trying to download them.

Author: rossmacarthur

src/editor.rs

@@ -8,7 +8,7 @@ use std::{
 
 use anyhow::{anyhow, bail, Context as ResultExt, Result};
 
-use crate::edit;
+use crate::{edit, util::TempPath};
 
 /// Possible environment variables.
 const ENV_VARS: &[&str] = &["VISUAL", "EDITOR"];
@@ -21,12 +21,6 @@ const EDITORS: &[&str] = &["code --wait", "nano", "vim", "vi", "emacs"];
 #[cfg(target_os = "windows")]
 const EDITORS: &[&str] = &["code.exe --wait", "notepad.exe"];
 
-/// Holds a temporary file path that is removed when dropped.
-struct TempFile {
-    /// The file path that we will edit.
-    path: PathBuf,
-}
-
 /// Represents the default editor.
 pub struct Editor {
     /// The path to the editor binary.
@@ -40,7 +34,7 @@ pub struct Child {
     /// A handle for the editor child process.
     child: process::Child,
     /// The temporary file that the editor is editing.
-    file: TempFile,
+    file: TempPath,
 }
 
 /// Convert a string command to a binary and the rest of the arguments.
@@ -54,14 +48,6 @@ where
     Some((bin, args))
 }
 
-impl TempFile {
-    /// Create a new `TempFile`.
-    fn new(original_path: &Path) -> Self {
-        let path = original_path.with_extension(format!("tmp-{}.toml", process::id()));
-        Self { path }
-    }
-}
-
 impl Editor {
     /// Create a new default `Editor`.
     ///
@@ -80,12 +66,12 @@ impl Editor {
 
     /// Open a file for editing with initial contents.
     pub fn edit(self, path: &Path, contents: &str) -> Result<Child> {
-        let file = TempFile::new(path);
+        let file = TempPath::new(path);
         let Self { bin, args } = self;
-        fs::write(&file.path, &contents).context("failed to write to temporary file")?;
+        fs::write(file.path(), &contents).context("failed to write to temporary file")?;
         let child = Command::new(bin)
             .args(args)
-            .arg(&file.path)
+            .arg(file.path())
             .spawn()
             .context("failed to spawn editor subprocess")?;
         Ok(Child { child, file })
@@ -99,7 +85,7 @@ impl Child {
         let exit_status = child.wait()?;
         if exit_status.success() {
             let contents =
-                fs::read_to_string(&file.path).context("failed to read from temporary file")?;
+                fs::read_to_string(file.path()).context("failed to read from temporary file")?;
             if contents == original_contents {
                 bail!("aborted editing!");
             } else {
@@ -111,9 +97,3 @@ impl Child {
         }
     }
 }
-
-impl Drop for TempFile {
-    fn drop(&mut self) {
-        fs::remove_file(&self.path).ok();
-    }
-}

src/lock.rs

@@ -7,7 +7,7 @@ use std::{
     cmp,
     collections::{HashMap, HashSet},
     convert::TryInto,
-    fmt, fs, io,
+    fmt, fs,
     path::{Path, PathBuf},
     result, sync,
 };
@@ -24,7 +24,7 @@ use walkdir::WalkDir;
 use crate::{
     config::{Config, ExternalPlugin, GitReference, InlinePlugin, Plugin, Source, Template},
     context::{LockContext as Context, Settings, SettingsExt},
-    util::git,
+    util::{git, TempPath},
 };
 
 /// The maximmum number of threads to use while downloading sources.
@@ -166,55 +166,60 @@ impl Source {
         url: Url,
         reference: Option<GitReference>,
     ) -> Result<LockedSource> {
-        if ctx.reinstall {
-            if let Err(e) = fs::remove_dir_all(&dir) {
-                if e.kind() != io::ErrorKind::NotFound {
-                    return Err(e).with_context(s!("failed to remove dir `{}`", &dir.display()));
+        let checkout = |repo, status| -> Result<()> {
+            match reference {
+                Some(reference) => {
+                    git::checkout(&repo, reference.lock(&repo)?.0)?;
+                    git::submodule_update(&repo).context("failed to recursively update")?;
+                    status!(ctx, status, &format!("{}@{}", &url, reference));
+                }
+                None => {
+                    git::submodule_update(&repo).context("failed to recursively update")?;
+                    status!(ctx, status, &url);
                 }
             }
-        }
-
-        let (cloned, repo) = git::clone_or_open(&url, &dir)?;
-        let status = if cloned { "Cloned" } else { "Checked" };
+            Ok(())
+        };
 
-        // Checkout the configured revision.
-        if let Some(reference) = reference {
-            git::checkout(&repo, reference.lock(&repo)?.0)?;
-            status!(ctx, status, &format!("{}@{}", &url, reference));
-        } else {
-            status!(ctx, status, &url);
+        if !ctx.reinstall {
+            if let Ok(repo) = git::open(&dir) {
+                checkout(repo, "Checked")?;
+                return Ok(LockedSource { dir, file: None });
+            }
         }
 
-        // Recursively update Git submodules.
-        git::submodule_update(&repo).context("failed to recursively update submodules")?;
+        let temp_dir = TempPath::new(&dir);
+        let repo = git::clone(&url, &temp_dir.path())?;
+        checkout(repo, "Cloned")?;
+        temp_dir
+            .rename(&dir)
+            .context("failed to rename temporary clone directory")?;
 
         Ok(LockedSource { dir, file: None })
     }
 
     /// Downloads a Remote source.
     fn lock_remote(ctx: &Context, dir: PathBuf, file: PathBuf, url: Url) -> Result<LockedSource> {
-        if ctx.reinstall {
-            if let Err(e) = fs::remove_file(&file) {
-                if e.kind() != io::ErrorKind::NotFound {
-                    return Err(e).with_context(s!("failed to remove file `{}`", &file.display()));
-                }
-            }
-        }
-
-        if file.exists() {
+        if !ctx.reinstall && file.exists() {
             status!(ctx, "Checked", &url);
-        } else {
-            fs::create_dir_all(&dir)
-                .with_context(s!("failed to create dir `{}`", dir.display()))?;
-            let mut response = reqwest::blocking::get(url.clone())
-                .with_context(s!("failed to download `{}`", url))?;
-            let mut out = fs::File::create(&file)
-                .with_context(s!("failed to create `{}`", file.display()))?;
-            io::copy(&mut response, &mut out)
-                .with_context(s!("failed to copy contents to `{}`", file.display()))?;
-            status!(ctx, "Fetched", &url);
+            return Ok(LockedSource {
+                dir,
+                file: Some(file),
+            });
         }
 
+        let mut response =
+            reqwest::blocking::get(url.clone()).with_context(s!("failed to download `{}`", url))?;
+        fs::create_dir_all(&dir).with_context(s!("failed to create dir `{}`", dir.display()))?;
+        let mut temp_file = TempPath::new(&file);
+        temp_file.write(&mut response).with_context(s!(
+            "failed to copy contents to `{}`",
+            temp_file.path().display()
+        ))?;
+        temp_file
+            .rename(&file)
+            .context("failed to rename temporary download file")?;
+
         Ok(LockedSource {
             dir,
             file: Some(file),
@@ -757,7 +762,7 @@ mod tests {
     use super::*;
     use std::{
         fs,
-        io::{Read, Write},
+        io::{self, Read, Write},
         process::Command,
         thread, time,
     };
@@ -920,15 +925,14 @@ mod tests {
         thread::sleep(time::Duration::from_secs(1));
         ctx.reinstall = true;
         let locked = Source::lock_git(&ctx, dir.to_path_buf(), url, None).unwrap();
-
         assert_eq!(locked.dir, dir);
         assert_eq!(locked.file, None);
         let repo = git2::Repository::open(&dir).unwrap();
         assert_eq!(
             repo.head().unwrap().target().unwrap().to_string(),
             "be8fde277e76f35efbe46848fb352cee68549962"
         );
-        assert!(fs::metadata(&dir).unwrap().modified().unwrap() > modified)
+        assert!(fs::metadata(&dir).unwrap().modified().unwrap() > modified);
     }
 
     #[test]

src/util.rs

@@ -4,7 +4,7 @@ use std::{
     fs::{self, File},
     io,
     path::{Path, PathBuf},
-    time,
+    process, time,
 };
 
 use anyhow::{Context as ResultExt, Error, Result};
@@ -22,6 +22,19 @@ pub fn underlying_io_error_kind(error: &Error) -> Option<io::ErrorKind> {
     None
 }
 
+/// Remove a file or directory.
+fn nuke_path(path: &Path) -> io::Result<()> {
+    if path.is_dir() {
+        fs::remove_dir_all(path)
+    } else {
+        fs::remove_file(path)
+    }
+}
+
+/////////////////////////////////////////////////////////////////////////
+// PathExt trait
+/////////////////////////////////////////////////////////////////////////
+
 /// An extension trait for [`Path`] types.
 ///
 /// [`Path`]: https://doc.rust-lang.org/std/path/struct.Path.html
@@ -84,6 +97,73 @@ impl PathExt for Path {
     }
 }
 
+/////////////////////////////////////////////////////////////////////////
+// TempPath type
+/////////////////////////////////////////////////////////////////////////
+
+/// Holds a temporary directory or file path that is removed when dropped.
+pub struct TempPath {
+    /// The temporary directory or file path.
+    pub path: Option<PathBuf>,
+}
+
+impl TempPath {
+    /// Create a new `TempPath`.
+    pub fn new(original_path: &Path) -> Self {
+        let mut path = original_path.parent().unwrap().to_path_buf();
+        let mut file_name = original_path.file_stem().unwrap().to_os_string();
+        file_name.push(format!("-tmp-{}", process::id()));
+        if let Some(ext) = original_path.extension() {
+            file_name.push(".");
+            file_name.push(ext);
+        }
+        path.push(file_name);
+        Self { path: Some(path) }
+    }
+
+    /// Access the underlying `Path`.
+    pub fn path(&self) -> &Path {
+        self.path.as_ref().unwrap()
+    }
+
+    /// Copy the contents of a stream to this `TempPath`.
+    pub fn write<R>(&mut self, mut reader: &mut R) -> io::Result<()>
+    where
+        R: io::Read,
+    {
+        let mut file = fs::File::create(self.path())?;
+        io::copy(&mut reader, &mut file)?;
+        Ok(())
+    }
+
+    /// Move the temporary path to a new location.
+    pub fn rename(mut self, new_path: &Path) -> io::Result<()> {
+        if let Err(err) = nuke_path(new_path) {
+            if err.kind() != io::ErrorKind::NotFound {
+                return Err(err);
+            }
+        };
+        if let Some(path) = &self.path {
+            fs::rename(path, new_path)?;
+            // This is so that the Drop impl doesn't try delete a non-existent file.
+            self.path = None;
+        }
+        Ok(())
+    }
+}
+
+impl Drop for TempPath {
+    fn drop(&mut self) {
+        if let Some(path) = &self.path {
+            nuke_path(&path).ok();
+        }
+    }
+}
+
+/////////////////////////////////////////////////////////////////////////
+// Mutex type
+/////////////////////////////////////////////////////////////////////////
+
 #[derive(Debug)]
 pub struct Mutex(File);
 
@@ -123,11 +203,15 @@ impl Drop for Mutex {
     }
 }
 
+/////////////////////////////////////////////////////////////////////////
+// Git module
+/////////////////////////////////////////////////////////////////////////
+
 pub mod git {
     use std::path::Path;
 
     use git2::{
-        build::RepoBuilder, BranchType, Cred, CredentialType, Error, ErrorCode, FetchOptions, Oid,
+        build::RepoBuilder, BranchType, Cred, CredentialType, Error, FetchOptions, Oid,
         RemoteCallbacks, Repository, ResetType,
     };
     use url::Url;
@@ -158,28 +242,21 @@ pub mod git {
         f(opts)
     }
 
-    /// Clone or open a Git repository.
-    pub fn clone_or_open(url: &Url, dir: &Path) -> anyhow::Result<(bool, Repository)> {
+    /// Open a Git repository.
+    pub fn open(dir: &Path) -> anyhow::Result<Repository> {
+        let repo = Repository::open(dir)
+            .with_context(s!("failed to open repository at `{}`", dir.display()))?;
+        Ok(repo)
+    }
+
+    /// Clone a Git repository.
+    pub fn clone(url: &Url, dir: &Path) -> anyhow::Result<Repository> {
         with_fetch_options(|opts| {
-            let mut cloned = false;
-            let repo = match RepoBuilder::new()
+            let repo = RepoBuilder::new()
                 .fetch_options(opts)
                 .clone(url.as_str(), dir)
-            {
-                Ok(repo) => {
-                    cloned = true;
-                    repo
-                }
-                Err(e) => {
-                    if e.code() == ErrorCode::Exists {
-                        Repository::open(dir)
-                            .with_context(s!("failed to open repository at `{}`", dir.display()))?
-                    } else {
-                        return Err(e).with_context(s!("failed to git clone `{}`", url));
-                    }
-                }
-            };
-            Ok((cloned, repo))
+                .with_context(s!("failed to git clone `{}`", url))?;
+            Ok(repo)
         })
     }