Addressing review comments

Author: alexcrichton

src/cargo/core/resolver/mod.rs

@@ -169,7 +169,7 @@ this could be indicative of a few possible situations:
       but was replaced with one that doesn't
     * the lock file is corrupt
 
-unable to verify that `{0}` was the same as before in either situation
+unable to verify that `{0}` is the same as when the lockfile was generated
 ", id, id.source_id())
 
                 // If the checksums aren't equal, and neither is None, then they
@@ -185,7 +185,7 @@ this could be indicative of a few possible errors:
     * a replacement source in use (e.g. a mirror) returned a different checksum
     * the source itself may be corrupt in one way or another
 
-unable to verify that `{0}` was the same as before in any situation
+unable to verify that `{0}` is the same as when the lockfile was generated
 ", id);
                 }
             }

src/cargo/ops/cargo_rustc/fingerprint.rs

@@ -57,6 +57,16 @@ pub fn prepare_target<'a, 'cfg>(cx: &mut Context<'a, 'cfg>,
     let compare = compare_old_fingerprint(&loc, &*fingerprint);
     log_compare(unit, &compare);
 
+    // If our comparison failed (e.g. we're going to trigger a rebuild of this
+    // crate), then we also ensure the source of the crate passes all
+    // verification checks before we build it.
+    //
+    // The `Source::verify` method is intended to allow sources to execute
+    // pre-build checks to ensure that the relevant source code is all
+    // up-to-date and as expected. This is currently used primarily for
+    // directory sources which will use this hook to perform an integrity check
+    // on all files in the source to ensure they haven't changed. If they have
+    // changed then an error is issued.
     if compare.is_err() {
         let source_id = unit.pkg.package_id().source_id();
         let sources = cx.packages.sources();

src/cargo/sources/registry/index.rs

@@ -1,33 +1,36 @@
 use std::collections::HashMap;
 use std::io::prelude::*;
-use std::fs::{File, OpenOptions};
+use std::fs::File;
 use std::path::Path;
 
 use rustc_serialize::json;
 
 use core::dependency::{Dependency, DependencyInner, Kind};
 use core::{SourceId, Summary, PackageId, Registry};
 use sources::registry::{RegistryPackage, RegistryDependency, INDEX_LOCK};
-use util::{CargoResult, ChainError, internal, Filesystem, human, Config};
+use util::{CargoResult, ChainError, internal, Filesystem, Config};
 
 pub struct RegistryIndex<'cfg> {
     source_id: SourceId,
     path: Filesystem,
     cache: HashMap<String, Vec<(Summary, bool)>>,
     hashes: HashMap<(String, String), String>, // (name, vers) => cksum
     config: &'cfg Config,
+    locked: bool,
 }
 
 impl<'cfg> RegistryIndex<'cfg> {
     pub fn new(id: &SourceId,
                path: &Filesystem,
-               config: &'cfg Config) -> RegistryIndex<'cfg> {
+               config: &'cfg Config,
+               locked: bool) -> RegistryIndex<'cfg> {
         RegistryIndex {
             source_id: id.clone(),
             path: path.clone(),
             cache: HashMap::new(),
             hashes: HashMap::new(),
             config: config,
+            locked: locked,
         }
     }
 
@@ -52,53 +55,57 @@ impl<'cfg> RegistryIndex<'cfg> {
         if self.cache.contains_key(name) {
             return Ok(self.cache.get(name).unwrap());
         }
-        // If the lock file doesn't already exist then this'll cause *someone*
-        // to create it. We don't actually care who creates it, and if it's
-        // already there this should have no effect.
-        drop(OpenOptions::new()
-                        .write(true)
-                        .create(true)
-                        .open(self.path.join(INDEX_LOCK).into_path_unlocked()));
-        let lock = self.path.open_ro(Path::new(INDEX_LOCK),
-                                     self.config,
-                                     "the registry index");
-        let file = lock.and_then(|lock| {
-            let path = lock.path().parent().unwrap();
-            let fs_name = name.chars().flat_map(|c| {
-                c.to_lowercase()
-            }).collect::<String>();
-
-            // see module comment for why this is structured the way it is
-            let path = match fs_name.len() {
-                1 => path.join("1").join(&fs_name),
-                2 => path.join("2").join(&fs_name),
-                3 => path.join("3").join(&fs_name[..1]).join(&fs_name),
-                _ => path.join(&fs_name[0..2])
-                         .join(&fs_name[2..4])
-                         .join(&fs_name),
-            };
-            File::open(&path).map_err(human)
-        });
-        let summaries = match file {
+        let summaries = try!(self.load_summaries(name));
+        let summaries = summaries.into_iter().filter(|summary| {
+            summary.0.package_id().name() == name
+        }).collect();
+        self.cache.insert(name.to_string(), summaries);
+        Ok(self.cache.get(name).unwrap())
+    }
+
+    fn load_summaries(&mut self, name: &str) -> CargoResult<Vec<(Summary, bool)>> {
+        let (path, _lock) = if self.locked {
+            let lock = self.path.open_ro(Path::new(INDEX_LOCK),
+                                         self.config,
+                                         "the registry index");
+            match lock {
+                Ok(lock) => {
+                    (lock.path().parent().unwrap().to_path_buf(), Some(lock))
+                }
+                Err(_) => return Ok(Vec::new()),
+            }
+        } else {
+            (self.path.clone().into_path_unlocked(), None)
+        };
+
+        let fs_name = name.chars().flat_map(|c| {
+            c.to_lowercase()
+        }).collect::<String>();
+
+        // see module comment for why this is structured the way it is
+        let path = match fs_name.len() {
+            1 => path.join("1").join(&fs_name),
+            2 => path.join("2").join(&fs_name),
+            3 => path.join("3").join(&fs_name[..1]).join(&fs_name),
+            _ => path.join(&fs_name[0..2])
+                     .join(&fs_name[2..4])
+                     .join(&fs_name),
+        };
+        match File::open(&path) {
             Ok(mut f) => {
                 let mut contents = String::new();
                 try!(f.read_to_string(&mut contents));
                 let ret: CargoResult<Vec<(Summary, bool)>>;
                 ret = contents.lines().filter(|l| l.trim().len() > 0)
                               .map(|l| self.parse_registry_package(l))
                               .collect();
-                try!(ret.chain_error(|| {
+                ret.chain_error(|| {
                     internal(format!("failed to parse registry's information \
                                       for: {}", name))
-                }))
+                })
             }
-            Err(..) => Vec::new(),
-        };
-        let summaries = summaries.into_iter().filter(|summary| {
-            summary.0.package_id().name() == name
-        }).collect();
-        self.cache.insert(name.to_string(), summaries);
-        Ok(self.cache.get(name).unwrap())
+            Err(..) => Ok(Vec::new()),
+        }
     }
 
     /// Parse a line from the registry's index file into a Summary for a

src/cargo/sources/registry/local.rs

@@ -75,12 +75,17 @@ impl<'cfg> RegistryData for LocalRegistry<'cfg> {
 
         // We don't actually need to download anything per-se, we just need to
         // verify the checksum matches the .crate file itself.
-        let mut data = Vec::new();
-        try!(crate_file.read_to_end(&mut data).chain_error(|| {
-            human(format!("failed to read `{}`", crate_file.path().display()))
-        }));
         let mut state = Sha256::new();
-        state.update(&data);
+        let mut buf = [0; 64 * 1024];
+        loop {
+            let n = try!(crate_file.read(&mut buf).chain_error(|| {
+                human(format!("failed to read `{}`", crate_file.path().display()))
+            }));
+            if n == 0 {
+                break
+            }
+            state.update(&buf[..n]);
+        }
         if state.finish().to_hex() != checksum {
             bail!("failed to verify the checksum of `{}`", pkg)
         }

src/cargo/sources/registry/mod.rs

@@ -181,6 +181,7 @@ pub struct RegistrySource<'cfg> {
     updated: bool,
     ops: Box<RegistryData + 'cfg>,
     index: index::RegistryIndex<'cfg>,
+    index_locked: bool,
 }
 
 #[derive(RustcDecodable)]
@@ -240,29 +241,32 @@ impl<'cfg> RegistrySource<'cfg> {
                   config: &'cfg Config) -> RegistrySource<'cfg> {
         let name = short_name(source_id);
         let ops = remote::RemoteRegistry::new(source_id, config, &name);
-        RegistrySource::new(source_id, config, &name, Box::new(ops))
+        RegistrySource::new(source_id, config, &name, Box::new(ops), true)
     }
 
     pub fn local(source_id: &SourceId,
                  path: &Path,
                  config: &'cfg Config) -> RegistrySource<'cfg> {
         let name = short_name(source_id);
         let ops = local::LocalRegistry::new(path, config, &name);
-        RegistrySource::new(source_id, config, &name, Box::new(ops))
+        RegistrySource::new(source_id, config, &name, Box::new(ops), false)
     }
 
     fn new(source_id: &SourceId,
            config: &'cfg Config,
            name: &str,
-           ops: Box<RegistryData + 'cfg>) -> RegistrySource<'cfg> {
+           ops: Box<RegistryData + 'cfg>,
+           index_locked: bool) -> RegistrySource<'cfg> {
         RegistrySource {
             src_path: config.registry_source_path().join(name),
             config: config,
             source_id: source_id.clone(),
             updated: false,
             index: index::RegistryIndex::new(source_id,
                                              ops.index_path(),
-                                             config),
+                                             config,
+                                             index_locked),
+            index_locked: index_locked,
             ops: ops,
         }
     }
@@ -306,7 +310,8 @@ impl<'cfg> RegistrySource<'cfg> {
         let path = self.ops.index_path();
         self.index = index::RegistryIndex::new(&self.source_id,
                                                path,
-                                               self.config);
+                                               self.config,
+                                               self.index_locked);
         Ok(())
     }
 }

src/cargo/sources/registry/remote.rs

@@ -53,7 +53,12 @@ impl<'cfg> RegistryData for RemoteRegistry<'cfg> {
     }
 
     fn update_index(&mut self) -> CargoResult<()> {
-        // Ensure that this'll actually succeed later on.
+        // Ensure that we'll actually be able to acquire an HTTP handle later on
+        // once we start trying to download crates. This will weed out any
+        // problems with `.cargo/config` configuration related to HTTP.
+        //
+        // This way if there's a problem the error gets printed before we even
+        // hit the index, which may not actually read this configuration.
         try!(ops::http_handle(self.config));
 
         // Then we actually update the index

tests/directory.rs

@@ -287,7 +287,7 @@ this could be indicative of a few possible errors:
     * a replacement source in use (e.g. a mirror) returned a different checksum
     * the source itself may be corrupt in one way or another
 
-unable to verify that `foo v0.1.0` was the same as before in any situation
+unable to verify that `foo v0.1.0` is the same as when the lockfile was generated
 
 "));
 }

tests/local-registry.rs

@@ -344,7 +344,7 @@ this could be indicative of a few possible errors:
     * a replacement source in use (e.g. a mirror) returned a different checksum
     * the source itself may be corrupt in one way or another
 
-unable to verify that `foo v0.0.1` was the same as before in any situation
+unable to verify that `foo v0.0.1` is the same as when the lockfile was generated
 
 "));
 }

tests/lockfile-compat.rs

@@ -155,7 +155,7 @@ this could be indicative of a few possible errors:
     * a replacement source in use (e.g. a mirror) returned a different checksum
     * the source itself may be corrupt in one way or another
 
-unable to verify that `foo v0.1.0` was the same as before in any situation
+unable to verify that `foo v0.1.0` is the same as when the lockfile was generated
 
 "));
 }
@@ -272,7 +272,7 @@ this could be indicative of a few possible situations:
       but was replaced with one that doesn't
     * the lock file is corrupt
 
-unable to verify that `foo v0.1.0 ([..])` was the same as before in either situation
+unable to verify that `foo v0.1.0 ([..])` is the same as when the lockfile was generated
 
 "));
 }