Pull Request feedback

Author: arlosi

src/cargo/core/registry.rs

@@ -637,7 +637,7 @@ impl<'cfg> Registry for PackageRegistry<'cfg> {
                 let source = self.sources.get_mut(dep.source_id());
                 match (override_summary, source) {
                     (Some(_), None) => {
-                        return Err(anyhow::anyhow!("override found but no real ones")).into()
+                        return Poll::Ready(Err(anyhow::anyhow!("override found but no real ones")))
                     }
                     (None, None) => return Poll::Ready(Ok(())),
 
@@ -678,8 +678,7 @@ impl<'cfg> Registry for PackageRegistry<'cfg> {
                     // the summaries it gives us though.
                     (Some(override_summary), Some(source)) => {
                         if !patches.is_empty() {
-                            return Err(anyhow::anyhow!("found patches and a path override"))
-                                .into();
+                            return Poll::Ready(Err(anyhow::anyhow!("found patches and a path override")))
                         }
                         let mut n = 0;
                         let mut to_warn = None;
@@ -704,7 +703,7 @@ impl<'cfg> Registry for PackageRegistry<'cfg> {
         };
 
         if n > 1 {
-            return Err(anyhow::anyhow!("found an override with a non-locked list")).into();
+            return Poll::Ready(Err(anyhow::anyhow!("found an override with a non-locked list")))
         } else if let Some(summary) = to_warn {
             self.warn_bad_override(&override_summary, &summary)?;
         }
@@ -865,7 +864,7 @@ fn summary_for_patch(
         let mut vers: Vec<_> = summaries.iter().map(|summary| summary.version()).collect();
         vers.sort();
         let versions: Vec<_> = vers.into_iter().map(|v| v.to_string()).collect();
-        return Err(anyhow::anyhow!(
+        return Poll::Ready(Err(anyhow::anyhow!(
             "patch for `{}` in `{}` resolved to more than one candidate\n\
             Found versions: {}\n\
             Update the patch definition to select only one package.\n\
@@ -875,8 +874,7 @@ fn summary_for_patch(
             orig_patch.source_id(),
             versions.join(", "),
             versions.last().unwrap()
-        ))
-        .into();
+        )))
     }
     assert!(summaries.is_empty());
     // No summaries found, try to help the user figure out what is wrong.
@@ -932,7 +930,7 @@ fn summary_for_patch(
             format!("versions `{}`", strs.join(", "))
         }
     };
-    Err(if found.is_empty() {
+    Poll::Ready(Err(if found.is_empty() {
         anyhow::anyhow!(
             "The patch location `{}` does not appear to contain any packages \
             matching the name `{}`.",
@@ -950,6 +948,5 @@ fn summary_for_patch(
             found,
             orig_patch.version_req()
         )
-    })
-    .into()
+    }))
 }

src/cargo/core/source/mod.rs

@@ -105,7 +105,13 @@ pub trait Source {
     /// as yanked. This ignores the yanked whitelist.
     fn is_yanked(&mut self, _pkg: PackageId) -> CargoResult<bool>;
 
-    /// Block until all outstanding Poll::Pending requests are Poll::Ready.
+    /// Block until all outstanding Poll::Pending requests are `Poll::Ready`.
+    ///
+    /// After calling this function, the source should return `Poll::Ready` for
+    /// any queries that previously returned `Poll::Pending`.
+    ///
+    /// If no queries previously returned `Poll::Pending`, and `invalidate_cache`
+    /// was not called, this function should be a no-op.
     fn block_until_ready(&mut self) -> CargoResult<()>;
 }
 

src/cargo/sources/directory.rs

@@ -37,8 +37,58 @@ impl<'cfg> DirectorySource<'cfg> {
             updated: false,
         }
     }
+}
+
+impl<'cfg> Debug for DirectorySource<'cfg> {
+    fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
+        write!(f, "DirectorySource {{ root: {:?} }}", self.root)
+    }
+}
+
+impl<'cfg> Source for DirectorySource<'cfg> {
+    fn query(&mut self, dep: &Dependency, f: &mut dyn FnMut(Summary)) -> Poll<CargoResult<()>> {
+        if !self.updated {
+            return Poll::Pending;
+        }
+        let packages = self.packages.values().map(|p| &p.0);
+        let matches = packages.filter(|pkg| dep.matches(pkg.summary()));
+        for summary in matches.map(|pkg| pkg.summary().clone()) {
+            f(summary);
+        }
+        Poll::Ready(Ok(()))
+    }
 
-    fn update(&mut self) -> CargoResult<()> {
+    fn fuzzy_query(
+        &mut self,
+        _dep: &Dependency,
+        f: &mut dyn FnMut(Summary),
+    ) -> Poll<CargoResult<()>> {
+        if !self.updated {
+            return Poll::Pending;
+        }
+        let packages = self.packages.values().map(|p| &p.0);
+        for summary in packages.map(|pkg| pkg.summary().clone()) {
+            f(summary);
+        }
+        Poll::Ready(Ok(()))
+    }
+
+    fn supports_checksums(&self) -> bool {
+        true
+    }
+
+    fn requires_precise(&self) -> bool {
+        true
+    }
+
+    fn source_id(&self) -> SourceId {
+        self.source_id
+    }
+
+    fn block_until_ready(&mut self) -> CargoResult<()> {
+        if self.updated {
+            return Ok(());
+        }
         self.packages.clear();
         let entries = self.root.read_dir().with_context(|| {
             format!(
@@ -109,55 +159,9 @@ impl<'cfg> DirectorySource<'cfg> {
             self.packages.insert(pkg.package_id(), (pkg, cksum));
         }
 
+        self.updated = true;
         Ok(())
     }
-}
-
-impl<'cfg> Debug for DirectorySource<'cfg> {
-    fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
-        write!(f, "DirectorySource {{ root: {:?} }}", self.root)
-    }
-}
-
-impl<'cfg> Source for DirectorySource<'cfg> {
-    fn query(&mut self, dep: &Dependency, f: &mut dyn FnMut(Summary)) -> Poll<CargoResult<()>> {
-        if !self.updated {
-            return Poll::Pending;
-        }
-        let packages = self.packages.values().map(|p| &p.0);
-        let matches = packages.filter(|pkg| dep.matches(pkg.summary()));
-        for summary in matches.map(|pkg| pkg.summary().clone()) {
-            f(summary);
-        }
-        Poll::Ready(Ok(()))
-    }
-
-    fn fuzzy_query(
-        &mut self,
-        _dep: &Dependency,
-        f: &mut dyn FnMut(Summary),
-    ) -> Poll<CargoResult<()>> {
-        if !self.updated {
-            return Poll::Pending;
-        }
-        let packages = self.packages.values().map(|p| &p.0);
-        for summary in packages.map(|pkg| pkg.summary().clone()) {
-            f(summary);
-        }
-        Poll::Ready(Ok(()))
-    }
-
-    fn supports_checksums(&self) -> bool {
-        true
-    }
-
-    fn requires_precise(&self) -> bool {
-        true
-    }
-
-    fn source_id(&self) -> SourceId {
-        self.source_id
-    }
 
     fn download(&mut self, id: PackageId) -> CargoResult<MaybePackage> {
         self.packages
@@ -219,11 +223,7 @@ impl<'cfg> Source for DirectorySource<'cfg> {
         Ok(false)
     }
 
-    fn block_until_ready(&mut self) -> CargoResult<()> {
-        self.update()?;
-        self.updated = true;
-        Ok(())
+    fn invalidate_cache(&mut self) {
+        // Path source has no local cache.
     }
-
-    fn invalidate_cache(&mut self) {}
 }

src/cargo/sources/git/source.rs

@@ -20,7 +20,6 @@ pub struct GitSource<'cfg> {
     path_source: Option<PathSource<'cfg>>,
     ident: String,
     config: &'cfg Config,
-    updated: bool,
 }
 
 impl<'cfg> GitSource<'cfg> {
@@ -43,7 +42,6 @@ impl<'cfg> GitSource<'cfg> {
             path_source: None,
             ident,
             config,
-            updated: false,
         };
 
         Ok(source)
@@ -55,13 +53,72 @@ impl<'cfg> GitSource<'cfg> {
 
     pub fn read_packages(&mut self) -> CargoResult<Vec<Package>> {
         if self.path_source.is_none() {
-            self.update()?;
+            self.invalidate_cache();
+            self.block_until_ready()?;
         }
         self.path_source.as_mut().unwrap().read_packages()
     }
+}
+
+fn ident(id: &SourceId) -> String {
+    let ident = id
+        .canonical_url()
+        .raw_canonicalized_url()
+        .path_segments()
+        .and_then(|s| s.rev().next())
+        .unwrap_or("");
+
+    let ident = if ident.is_empty() { "_empty" } else { ident };
+
+    format!("{}-{}", ident, short_hash(id.canonical_url()))
+}
 
-    fn update(&mut self) -> CargoResult<()> {
-        if self.updated {
+impl<'cfg> Debug for GitSource<'cfg> {
+    fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
+        write!(f, "git repo at {}", self.remote.url())?;
+
+        match self.manifest_reference.pretty_ref() {
+            Some(s) => write!(f, " ({})", s),
+            None => Ok(()),
+        }
+    }
+}
+
+impl<'cfg> Source for GitSource<'cfg> {
+    fn query(&mut self, dep: &Dependency, f: &mut dyn FnMut(Summary)) -> Poll<CargoResult<()>> {
+        if let Some(src) = self.path_source.as_mut() {
+            src.query(dep, f)
+        } else {
+            Poll::Pending
+        }
+    }
+
+    fn fuzzy_query(
+        &mut self,
+        dep: &Dependency,
+        f: &mut dyn FnMut(Summary),
+    ) -> Poll<CargoResult<()>> {
+        if let Some(src) = self.path_source.as_mut() {
+            src.fuzzy_query(dep, f)
+        } else {
+            Poll::Pending
+        }
+    }
+
+    fn supports_checksums(&self) -> bool {
+        false
+    }
+
+    fn requires_precise(&self) -> bool {
+        true
+    }
+
+    fn source_id(&self) -> SourceId {
+        self.source_id
+    }
+
+    fn block_until_ready(&mut self) -> CargoResult<()> {
+        if self.path_source.is_some() {
             return Ok(());
         }
 
@@ -92,10 +149,10 @@ impl<'cfg> GitSource<'cfg> {
             // doesn't have it.
             (locked_rev, db) => {
                 if self.config.offline() {
-                    return Err(anyhow::anyhow!(
+                    anyhow::bail!(
                         "can't checkout from '{}': you are in the offline mode (--offline)",
                         self.remote.url()
-                    ));
+                    );
                 }
                 self.config.shell().status(
                     "Updating",
@@ -133,67 +190,7 @@ impl<'cfg> GitSource<'cfg> {
 
         self.path_source = Some(path_source);
         self.locked_rev = Some(actual_rev);
-        self.path_source.as_mut().unwrap().update()?;
-        self.updated = true;
-        Ok(())
-    }
-}
-
-fn ident(id: &SourceId) -> String {
-    let ident = id
-        .canonical_url()
-        .raw_canonicalized_url()
-        .path_segments()
-        .and_then(|s| s.rev().next())
-        .unwrap_or("");
-
-    let ident = if ident.is_empty() { "_empty" } else { ident };
-
-    format!("{}-{}", ident, short_hash(id.canonical_url()))
-}
-
-impl<'cfg> Debug for GitSource<'cfg> {
-    fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
-        write!(f, "git repo at {}", self.remote.url())?;
-
-        match self.manifest_reference.pretty_ref() {
-            Some(s) => write!(f, " ({})", s),
-            None => Ok(()),
-        }
-    }
-}
-
-impl<'cfg> Source for GitSource<'cfg> {
-    fn query(&mut self, dep: &Dependency, f: &mut dyn FnMut(Summary)) -> Poll<CargoResult<()>> {
-        if let Some(src) = self.path_source.as_mut() {
-            src.query(dep, f)
-        } else {
-            Poll::Pending
-        }
-    }
-
-    fn fuzzy_query(
-        &mut self,
-        dep: &Dependency,
-        f: &mut dyn FnMut(Summary),
-    ) -> Poll<CargoResult<()>> {
-        if let Some(src) = self.path_source.as_mut() {
-            src.fuzzy_query(dep, f)
-        } else {
-            Poll::Pending
-        }
-    }
-
-    fn supports_checksums(&self) -> bool {
-        false
-    }
-
-    fn requires_precise(&self) -> bool {
-        true
-    }
-
-    fn source_id(&self) -> SourceId {
-        self.source_id
+        self.path_source.as_mut().unwrap().update()
     }
 
     fn download(&mut self, id: PackageId) -> CargoResult<MaybePackage> {
@@ -226,13 +223,7 @@ impl<'cfg> Source for GitSource<'cfg> {
         Ok(false)
     }
 
-    fn block_until_ready(&mut self) -> CargoResult<()> {
-        self.update()
-    }
-
-    fn invalidate_cache(&mut self) {
-        self.updated = false;
-    }
+    fn invalidate_cache(&mut self) {}
 }
 
 #[cfg(test)]