Auto merge of #5110 - alexcrichton:reset-harder, r=matklad

Try a lot harder to recover corrupt git repos

We've received a lot of intermittent bug reports historically about corrupt git
repositories. These inevitably happens as Cargo is ctrl-c'd or for whatever
other reason, and to provide a better user experience Cargo strives to
automatically handle these situations by blowing away the old checkout for a new
update.

This commit adds a new test which attempts to pathologically corrupt a git
database and checkout in an attempt to expose bugs in Cargo. Sure enough there
were some more locations that we needed to handle gracefully for corrupt git
checkouts. Notable inclusions were:

* The `fetch` operation in libgit2 would fail due to corrupt references. This
  starts by adding an explicit whitelist for classes of errors coming out of
  `fetch` to auto-retry by blowing away the repository. We need to be super
  careful here as network errors commonly come out of this function and we don't
  want to too aggressively re-clone.

* After a `fetch` succeeded a repository could fail to actual resolve a
  git reference to the actual revision we want. This indicated that we indeed
  needed to blow everything away and re-clone entirely again.

* When creating a checkout from a database the `reset` operation might fail due
  to a corrupt local database of the checkout itself. If this happens we needed
  to just blow it away and try again.

There's likely more lurking situations where we need to re-clone but I figure we
can discover those over time.

Author: bors

.travis.yml

@@ -34,7 +34,7 @@ matrix:
       script:
         - cargo test
         - cargo doc --no-deps
-        - (cd src/doc && mdbook build --no-create --dest-dir ../../target/doc)
+        - (cd src/doc && mdbook build --dest-dir ../../target/doc)
 
   exclude:
     - rust: stable

src/cargo/ops/cargo_clean.rs

@@ -5,6 +5,7 @@ use std::path::Path;
 use core::{Profiles, Workspace};
 use util::Config;
 use util::errors::{CargoResult, CargoResultExt};
+use util::paths;
 use ops::{self, Context, BuildConfig, Kind, Unit};
 
 pub struct CleanOptions<'a> {
@@ -99,14 +100,14 @@ fn rm_rf(path: &Path, config: &Config) -> CargoResult<()> {
     let m = fs::metadata(path);
     if m.as_ref().map(|s| s.is_dir()).unwrap_or(false) {
         config.shell().verbose(|shell| {shell.status("Removing", path.display())})?;
-        fs::remove_dir_all(path).chain_err(|| {
+        paths::remove_dir_all(path).chain_err(|| {
             format_err!("could not remove build directory")
         })?;
     } else if m.is_ok() {
         config.shell().verbose(|shell| {shell.status("Removing", path.display())})?;
-        fs::remove_file(path).chain_err(|| {
+        paths::remove_file(path).chain_err(|| {
             format_err!("failed to remove build artifact")
         })?;
     }
     Ok(())
-}
+}

src/cargo/ops/cargo_install.rs

@@ -17,6 +17,7 @@ use sources::{GitSource, PathSource, SourceConfigMap};
 use util::{Config, internal};
 use util::{Filesystem, FileLock};
 use util::errors::{CargoResult, CargoResultExt};
+use util::paths;
 
 #[derive(Deserialize, Serialize)]
 #[serde(untagged)]
@@ -47,7 +48,7 @@ impl Transaction {
 impl Drop for Transaction {
     fn drop(&mut self) {
         for bin in self.bins.iter() {
-            let _ = fs::remove_file(bin);
+            let _ = paths::remove_file(bin);
         }
     }
 }
@@ -319,7 +320,7 @@ fn install_one(root: &Filesystem,
         // Don't bother grabbing a lock as we're going to blow it all away
         // anyway.
         let target_dir = ws.target_dir().into_path_unlocked();
-        fs::remove_dir_all(&target_dir)?;
+        paths::remove_dir_all(&target_dir)?;
     }
 
     Ok(())
@@ -667,7 +668,7 @@ pub fn uninstall_one(root: &Filesystem,
     write_crate_list(&crate_metadata, metadata)?;
     for bin in to_remove {
         config.shell().status("Removing", bin.display())?;
-        fs::remove_file(bin)?;
+        paths::remove_file(bin)?;
     }
 
     Ok(())

src/cargo/ops/cargo_package.rs

@@ -304,8 +304,8 @@ fn run_verify(ws: &Workspace, tar: &FileLock, opts: &PackageOpts) -> CargoResult
 
     let f = GzDecoder::new(tar.file());
     let dst = tar.parent().join(&format!("{}-{}", pkg.name(), pkg.version()));
-    if fs::metadata(&dst).is_ok() {
-        fs::remove_dir_all(&dst)?;
+    if dst.exists() {
+        paths::remove_dir_all(&dst)?;
     }
     let mut archive = Archive::new(f);
     archive.unpack(dst.parent().unwrap())?;

src/cargo/ops/cargo_rustc/mod.rs

@@ -15,6 +15,7 @@ use core::manifest::Lto;
 use core::shell::ColorChoice;
 use util::{self, ProcessBuilder, machine_message};
 use util::{Config, internal, profile, join_paths};
+use util::paths;
 use util::errors::{CargoResult, CargoResultExt, Internal};
 use util::Freshness;
 
@@ -385,9 +386,7 @@ fn rustc<'a, 'cfg>(cx: &mut Context<'a, 'cfg>,
             if filename.extension() == Some(OsStr::new("rmeta")) {
                 let dst = root.join(filename).with_extension("rlib");
                 if dst.exists() {
-                    fs::remove_file(&dst).chain_err(|| {
-                        format!("Could not remove file: {}.", dst.display())
-                    })?;
+                    paths::remove_file(&dst)?;
                 }
             }
         }
@@ -541,9 +540,7 @@ fn link_targets<'a, 'cfg>(cx: &mut Context<'a, 'cfg>,
                 continue
             }
             if dst.exists() {
-                fs::remove_file(&dst).chain_err(|| {
-                    format!("failed to remove: {}", dst.display())
-                })?;
+                paths::remove_file(&dst)?;
             }
 
             let link_result = if src.is_dir() {

src/cargo/ops/cargo_rustc/output_depinfo.rs

@@ -1,10 +1,11 @@
 use std::collections::HashSet;
-use std::io::{Write, BufWriter, ErrorKind};
-use std::fs::{self, File};
+use std::io::{Write, BufWriter};
+use std::fs::File;
 use std::path::{Path, PathBuf};
 
 use ops::{Context, Unit};
 use util::{CargoResult, internal};
+use util::paths;
 use ops::cargo_rustc::fingerprint;
 
 fn render_filename<P: AsRef<Path>>(path: P, basedir: Option<&str>) -> CargoResult<String> {
@@ -81,13 +82,12 @@ pub fn output_depinfo<'a, 'b>(context: &mut Context<'a, 'b>, unit: &Unit<'a>) ->
                     write!(outfile, " {}", render_filename(dep, basedir)?)?;
                 }
                 writeln!(outfile, "")?;
-            } else if let Err(err) = fs::remove_file(output_path) {
-                // dep-info generation failed, so delete output file. This will usually
-                // cause the build system to always rerun the build rule, which is correct
-                // if inefficient.
-                if err.kind() != ErrorKind::NotFound {
-                    return Err(err.into());
-                }
+
+            // dep-info generation failed, so delete output file. This will
+            // usually cause the build system to always rerun the build
+            // rule, which is correct if inefficient.
+            } else if output_path.exists() {
+                paths::remove_file(output_path)?;
             }
         }
     }

src/cargo/sources/git/source.rs

@@ -6,7 +6,7 @@ use core::source::{Source, SourceId};
 use core::GitReference;
 use core::{Package, PackageId, Summary, Registry, Dependency};
 use util::Config;
-use util::errors::{CargoResult, Internal};
+use util::errors::CargoResult;
 use util::hex::short_hash;
 use sources::PathSource;
 use sources::git::utils::{GitRemote, GitRevision};
@@ -170,9 +170,7 @@ impl<'cfg> Source for GitSource<'cfg> {
 
             trace!("updating git source `{:?}`", self.remote);
 
-            let repo = self.remote.checkout(&db_path, self.config)?;
-            let rev = repo.rev_for(&self.reference).map_err(Internal::new)?;
-            (repo, rev)
+            self.remote.checkout(&db_path, &self.reference, self.config)?
         } else {
             (self.remote.db_at(&db_path)?, actual_rev.unwrap())
         };