Auto merge of #14169 - epage:split-sopource, r=ehuss

refactor(source): Clean up after PathSource/RecursivePathSource split

### What does this PR try to resolve?

This is a follow up to #13993 and prep for future improvements (e.g. cargo script, #10752)

### How should we test and review this PR?

### Additional information

Author: bors

src/cargo/ops/cargo_add/mod.rs

@@ -349,11 +349,8 @@ fn resolve_dependency(
             }
             selected
         } else {
-            let source = crate::sources::PathSource::new(&path, src.source_id()?, gctx);
-            let package = source
-                .read_packages()?
-                .pop()
-                .expect("read_packages errors when no packages");
+            let mut source = crate::sources::PathSource::new(&path, src.source_id()?, gctx);
+            let package = source.root_package()?;
             Dependency::from(package.summary())
         };
         selected

src/cargo/ops/cargo_install.rs

@@ -141,7 +141,7 @@ impl<'gctx> InstallablePackage<'gctx> {
                 select_pkg(
                     &mut src,
                     dep,
-                    |path: &mut PathSource<'_>| path.read_packages(),
+                    |path: &mut PathSource<'_>| path.root_package().map(|p| vec![p]),
                     gctx,
                     current_rust_version,
                 )?

src/cargo/ops/cargo_package.rs

@@ -224,7 +224,7 @@ fn prepare_archive(
 ) -> CargoResult<Vec<ArchiveFile>> {
     let gctx = ws.gctx();
     let mut src = PathSource::new(pkg.root(), pkg.package_id().source_id(), gctx);
-    src.update()?;
+    src.load()?;
 
     if opts.check_metadata {
         check_metadata(pkg, gctx)?;

src/cargo/ops/cargo_uninstall.rs

@@ -87,7 +87,7 @@ fn uninstall_cwd(root: &Filesystem, bins: &[String], gctx: &GlobalContext) -> Ca
     let pkg = select_pkg(
         &mut src,
         None,
-        |path: &mut PathSource<'_>| path.read_packages(),
+        |path: &mut PathSource<'_>| path.root_package().map(|p| vec![p]),
         gctx,
         None,
     )?;

src/cargo/ops/resolve.rs

@@ -460,7 +460,7 @@ pub fn add_overrides<'a>(
     for (path, definition) in paths {
         let id = SourceId::for_path(&path)?;
         let mut source = RecursivePathSource::new(&path, id, ws.gctx());
-        source.update().with_context(|| {
+        source.load().with_context(|| {
             format!(
                 "failed to update path override `{}` \
                  (defined in `{}`)",

src/cargo/sources/directory.rs

@@ -175,7 +175,7 @@ impl<'gctx> Source for DirectorySource<'gctx> {
             }
 
             let mut src = PathSource::new(&path, self.source_id, self.gctx);
-            src.update()?;
+            src.load()?;
             let mut pkg = src.root_package()?;
 
             let cksum_file = path.join(".cargo-checksum.json");

src/cargo/sources/git/source.rs

@@ -361,7 +361,7 @@ impl<'gctx> Source for GitSource<'gctx> {
         self.path_source = Some(path_source);
         self.short_id = Some(short_id.as_str().into());
         self.locked_rev = Revision::Locked(actual_rev);
-        self.path_source.as_mut().unwrap().update()?;
+        self.path_source.as_mut().unwrap().load()?;
 
         // Hopefully this shouldn't incur too much of a performance hit since
         // most of this should already be in cache since it was just

src/cargo/sources/path.rs

@@ -29,8 +29,6 @@ pub struct PathSource<'gctx> {
     source_id: SourceId,
     /// The root path of this source.
     path: PathBuf,
-    /// Whether this source has updated all package information it may contain.
-    updated: bool,
     /// Packages that this sources has discovered.
     package: Option<Package>,
     gctx: &'gctx GlobalContext,
@@ -45,7 +43,6 @@ impl<'gctx> PathSource<'gctx> {
         Self {
             source_id,
             path: path.to_path_buf(),
-            updated: false,
             package: None,
             gctx,
         }
@@ -59,7 +56,6 @@ impl<'gctx> PathSource<'gctx> {
         Self {
             source_id,
             path,
-            updated: true,
             package: Some(pkg),
             gctx,
         }
@@ -69,7 +65,7 @@ impl<'gctx> PathSource<'gctx> {
     pub fn root_package(&mut self) -> CargoResult<Package> {
         trace!("root_package; source={:?}", self);
 
-        self.update()?;
+        self.load()?;
 
         match &self.package {
             Some(pkg) => Ok(pkg.clone()),
@@ -80,23 +76,6 @@ impl<'gctx> PathSource<'gctx> {
         }
     }
 
-    /// Returns the packages discovered by this source. It may walk the
-    /// filesystem if package information haven't yet updated.
-    pub fn read_packages(&self) -> CargoResult<Vec<Package>> {
-        if self.updated {
-            Ok(self.package.clone().into_iter().collect())
-        } else {
-            let pkg = self.read_package()?;
-            Ok(vec![pkg])
-        }
-    }
-
-    fn read_package(&self) -> CargoResult<Package> {
-        let path = self.path.join("Cargo.toml");
-        let pkg = ops::read_package(&path, self.source_id, self.gctx)?;
-        Ok(pkg)
-    }
-
     /// List all files relevant to building this package inside this source.
     ///
     /// This function will use the appropriate methods to determine the
@@ -112,10 +91,10 @@ impl<'gctx> PathSource<'gctx> {
     }
 
     /// Gets the last modified file in a package.
-    pub fn last_modified_file(&self, pkg: &Package) -> CargoResult<(FileTime, PathBuf)> {
-        if !self.updated {
+    fn last_modified_file(&self, pkg: &Package) -> CargoResult<(FileTime, PathBuf)> {
+        if self.package.is_none() {
             return Err(internal(format!(
-                "BUG: source `{:?}` was not updated",
+                "BUG: source `{:?}` was not loaded",
                 self.path
             )));
         }
@@ -128,14 +107,19 @@ impl<'gctx> PathSource<'gctx> {
     }
 
     /// Discovers packages inside this source if it hasn't yet done.
-    pub fn update(&mut self) -> CargoResult<()> {
-        if !self.updated {
+    pub fn load(&mut self) -> CargoResult<()> {
+        if self.package.is_none() {
             self.package = Some(self.read_package()?);
-            self.updated = true;
         }
 
         Ok(())
     }
+
+    fn read_package(&self) -> CargoResult<Package> {
+        let path = self.path.join("Cargo.toml");
+        let pkg = ops::read_package(&path, self.source_id, self.gctx)?;
+        Ok(pkg)
+    }
 }
 
 impl<'gctx> Debug for PathSource<'gctx> {
@@ -151,7 +135,7 @@ impl<'gctx> Source for PathSource<'gctx> {
         kind: QueryKind,
         f: &mut dyn FnMut(IndexSummary),
     ) -> Poll<CargoResult<()>> {
-        self.update()?;
+        self.load()?;
         if let Some(s) = self.package.as_ref().map(|p| p.summary()) {
             let matched = match kind {
                 QueryKind::Exact => dep.matches(s),
@@ -179,7 +163,7 @@ impl<'gctx> Source for PathSource<'gctx> {
 
     fn download(&mut self, id: PackageId) -> CargoResult<MaybePackage> {
         trace!("getting packages; id={}", id);
-        self.update()?;
+        self.load()?;
         let pkg = self.package.iter().find(|pkg| pkg.package_id() == id);
         pkg.cloned()
             .map(MaybePackage::Ready)
@@ -213,7 +197,7 @@ impl<'gctx> Source for PathSource<'gctx> {
     }
 
     fn block_until_ready(&mut self) -> CargoResult<()> {
-        self.update()
+        self.load()
     }
 
     fn invalidate_cache(&mut self) {
@@ -232,8 +216,8 @@ pub struct RecursivePathSource<'gctx> {
     source_id: SourceId,
     /// The root path of this source.
     path: PathBuf,
-    /// Whether this source has updated all package information it may contain.
-    updated: bool,
+    /// Whether this source has loaded all package information it may contain.
+    loaded: bool,
     /// Packages that this sources has discovered.
     packages: Vec<Package>,
     gctx: &'gctx GlobalContext,
@@ -252,22 +236,26 @@ impl<'gctx> RecursivePathSource<'gctx> {
         Self {
             source_id,
             path: root.to_path_buf(),
-            updated: false,
+            loaded: false,
             packages: Vec::new(),
             gctx,
         }
     }
 
     /// Returns the packages discovered by this source. It may walk the
-    /// filesystem if package information haven't yet updated.
+    /// filesystem if package information haven't yet loaded.
     pub fn read_packages(&self) -> CargoResult<Vec<Package>> {
-        if self.updated {
+        if self.loaded {
             Ok(self.packages.clone())
         } else {
-            ops::read_packages(&self.path, self.source_id, self.gctx)
+            self.read_packages_inner()
         }
     }
 
+    fn read_packages_inner(&self) -> CargoResult<Vec<Package>> {
+        ops::read_packages(&self.path, self.source_id, self.gctx)
+    }
+
     /// List all files relevant to building this package inside this source.
     ///
     /// This function will use the appropriate methods to determine the
@@ -283,10 +271,10 @@ impl<'gctx> RecursivePathSource<'gctx> {
     }
 
     /// Gets the last modified file in a package.
-    pub fn last_modified_file(&self, pkg: &Package) -> CargoResult<(FileTime, PathBuf)> {
-        if !self.updated {
+    fn last_modified_file(&self, pkg: &Package) -> CargoResult<(FileTime, PathBuf)> {
+        if !self.loaded {
             return Err(internal(format!(
-                "BUG: source `{:?}` was not updated",
+                "BUG: source `{:?}` was not loaded",
                 self.path
             )));
         }
@@ -299,11 +287,10 @@ impl<'gctx> RecursivePathSource<'gctx> {
     }
 
     /// Discovers packages inside this source if it hasn't yet done.
-    pub fn update(&mut self) -> CargoResult<()> {
-        if !self.updated {
-            let packages = self.read_packages()?;
-            self.packages.extend(packages.into_iter());
-            self.updated = true;
+    pub fn load(&mut self) -> CargoResult<()> {
+        if !self.loaded {
+            self.packages = self.read_packages_inner()?;
+            self.loaded = true;
         }
 
         Ok(())
@@ -323,7 +310,7 @@ impl<'gctx> Source for RecursivePathSource<'gctx> {
         kind: QueryKind,
         f: &mut dyn FnMut(IndexSummary),
     ) -> Poll<CargoResult<()>> {
-        self.update()?;
+        self.load()?;
         for s in self.packages.iter().map(|p| p.summary()) {
             let matched = match kind {
                 QueryKind::Exact => dep.matches(s),
@@ -351,7 +338,7 @@ impl<'gctx> Source for RecursivePathSource<'gctx> {
 
     fn download(&mut self, id: PackageId) -> CargoResult<MaybePackage> {
         trace!("getting packages; id={}", id);
-        self.update()?;
+        self.load()?;
         let pkg = self.packages.iter().find(|pkg| pkg.package_id() == id);
         pkg.cloned()
             .map(MaybePackage::Ready)
@@ -385,7 +372,7 @@ impl<'gctx> Source for RecursivePathSource<'gctx> {
     }
 
     fn block_until_ready(&mut self) -> CargoResult<()> {
-        self.update()
+        self.load()
     }
 
     fn invalidate_cache(&mut self) {

src/cargo/sources/registry/mod.rs

@@ -719,7 +719,7 @@ impl<'gctx> RegistrySource<'gctx> {
             .unpack_package(package, path)
             .with_context(|| format!("failed to unpack package `{}`", package))?;
         let mut src = PathSource::new(&path, self.source_id, self.gctx);
-        src.update()?;
+        src.load()?;
         let mut pkg = match src.download(package)? {
             MaybePackage::Ready(pkg) => pkg,
             MaybePackage::Download { .. } => unreachable!(),