Assure that git-dependencies will get the correct storage, without ever unshallowing them.

This is an improvement over the previous version which would use unshallowing that effectively
makes a shallow repo *not* shallow.

Furthermore, we will now only fetch a single commit, each time we fetch, which should be faster
for the server as well as for the client.

We also make it possible to fetch individual commits that would be specified via Cargo.lock.

Author: Byron

src/cargo/sources/git/mod.rs

@@ -14,56 +14,36 @@ pub mod fetch {
     pub enum RemoteKind {
         /// A repository belongs to a git dependency.
         GitDependency,
-        /// A repository belongs to a git dependency, and due to usage of checking out specific revisions we can't
-        /// use shallow clones.
-        GitDependencyForbidShallow,
         /// A repository belongs to a Cargo registry.
         Registry,
     }
 
-    #[derive(Debug, Clone)]
-    pub enum History {
-        Shallow(gix::remote::fetch::Shallow),
-        Unshallow,
-    }
-
-    impl From<History> for gix::remote::fetch::Shallow {
-        fn from(value: History) -> Self {
-            match value {
-                History::Unshallow => gix::remote::fetch::Shallow::undo(),
-                History::Shallow(how) => how,
-            }
-        }
-    }
-
     impl RemoteKind {
         /// Obtain the kind of history we would want for a fetch from our remote knowing if the target repo is already shallow
         /// via `repo_is_shallow` along with gitoxide-specific feature configuration via `config`.
-        pub(crate) fn to_history(&self, repo_is_shallow: bool, config: &Config) -> History {
+        /// `rev_and_ref` is additional information that affects whether or not we may be shallow.
+        pub(crate) fn to_shallow_setting(
+            &self,
+            repo_is_shallow: bool,
+            config: &Config,
+        ) -> gix::remote::fetch::Shallow {
             let has_feature = |cb: &dyn Fn(GitoxideFeatures) -> bool| {
                 config
                     .cli_unstable()
                     .gitoxide
                     .map_or(false, |features| cb(features))
             };
-            let how = if repo_is_shallow {
-                if matches!(self, RemoteKind::GitDependencyForbidShallow) {
-                    return History::Unshallow;
-                } else {
-                    gix::remote::fetch::Shallow::NoChange
-                }
-            } else {
+
+            // maintain shallow-ness and keep downloading single commits, or see if we can do shallow clones
+            if !repo_is_shallow {
                 match self {
-                    RemoteKind::GitDependency if has_feature(&|git| git.shallow_deps) => {
-                        gix::remote::fetch::Shallow::DepthAtRemote(1.try_into().expect("non-zero"))
-                    }
-                    RemoteKind::Registry if has_feature(&|git| git.shallow_index) => {
-                        gix::remote::fetch::Shallow::DepthAtRemote(1.try_into().expect("non-zero"))
-                    }
-                    _ => gix::remote::fetch::Shallow::NoChange,
+                    RemoteKind::GitDependency if has_feature(&|git| git.shallow_deps) => {}
+                    RemoteKind::Registry if has_feature(&|git| git.shallow_index) => {}
+                    _ => return gix::remote::fetch::Shallow::NoChange,
                 }
             };
-            History::Shallow(how)
+
+            gix::remote::fetch::Shallow::DepthAtRemote(1.try_into().expect("non-zero"))
         }
     }
 

src/cargo/sources/git/source.rs

@@ -29,6 +29,14 @@ impl<'cfg> GitSource<'cfg> {
         assert!(source_id.is_git(), "id is not git, id={}", source_id);
 
         let remote = GitRemote::new(source_id.url());
+        let manifest_reference = source_id.git_reference().unwrap().clone();
+        let locked_rev =
+            match source_id.precise() {
+                Some(s) => Some(git2::Oid::from_str(s).with_context(|| {
+                    format!("precise value for git is not a git revision: {}", s)
+                })?),
+                None => None,
+            };
         let ident = ident_shallow(
             &source_id,
             config
@@ -39,13 +47,8 @@ impl<'cfg> GitSource<'cfg> {
 
         let source = GitSource {
             remote,
-            manifest_reference: source_id.git_reference().unwrap().clone(),
-            locked_rev: match source_id.precise() {
-                Some(s) => Some(git2::Oid::from_str(s).with_context(|| {
-                    format!("precise value for git is not a git revision: {}", s)
-                })?),
-                None => None,
-            },
+            manifest_reference,
+            locked_rev,
             source_id,
             path_source: None,
             ident,

src/cargo/sources/git/utils.rs

@@ -2,7 +2,7 @@
 //! authentication/cloning.
 
 use crate::core::{GitReference, Verbosity};
-use crate::sources::git::fetch::{History, RemoteKind};
+use crate::sources::git::fetch::RemoteKind;
 use crate::sources::git::oxide;
 use crate::sources::git::oxide::cargo_config_to_gitoxide_overrides;
 use crate::util::errors::CargoResult;
@@ -97,32 +97,25 @@ impl GitRemote {
         // if we can. If that can successfully load our revision then we've
         // populated the database with the latest version of `reference`, so
         // return that database and the rev we resolve to.
-        let remote_kind = if locked_rev.is_some() || matches!(reference, GitReference::Rev(_)) {
-            RemoteKind::GitDependencyForbidShallow
-        } else {
-            RemoteKind::GitDependency
-        };
         if let Some(mut db) = db {
-            let history = remote_kind.to_history(db.repo.is_shallow(), cargo_config);
+            let shallow =
+                RemoteKind::GitDependency.to_shallow_setting(db.repo.is_shallow(), cargo_config);
             fetch(
                 &mut db.repo,
                 self.url.as_str(),
                 reference,
                 cargo_config,
-                history,
+                shallow,
+                locked_rev,
             )
             .with_context(|| format!("failed to fetch into: {}", into.display()))?;
-            match locked_rev {
-                Some(rev) => {
-                    if db.contains(rev) {
-                        return Ok((db, rev));
-                    }
-                }
-                None => {
-                    if let Ok(rev) = reference.resolve(&db.repo) {
-                        return Ok((db, rev));
-                    }
-                }
+
+            let resolved_commit_hash = match locked_rev {
+                Some(rev) => db.contains(rev).then_some(rev),
+                None => reference.resolve(&db.repo).ok(),
+            };
+            if let Some(rev) = resolved_commit_hash {
+                return Ok((db, rev));
             }
         }
 
@@ -134,13 +127,14 @@ impl GitRemote {
         }
         paths::create_dir_all(into)?;
         let mut repo = init(into, true)?;
-        let history = remote_kind.to_history(repo.is_shallow(), cargo_config);
+        let shallow = RemoteKind::GitDependency.to_shallow_setting(repo.is_shallow(), cargo_config);
         fetch(
             &mut repo,
             self.url.as_str(),
             reference,
             cargo_config,
-            history,
+            shallow,
+            locked_rev,
         )
         .with_context(|| format!("failed to clone into: {}", into.display()))?;
         let rev = match locked_rev {
@@ -462,8 +456,9 @@ impl<'a> GitCheckout<'a> {
             cargo_config
                 .shell()
                 .status("Updating", format!("git submodule `{}`", url))?;
-            let history = RemoteKind::GitDependency.to_history(repo.is_shallow(), cargo_config);
-            fetch(&mut repo, &url, &reference, cargo_config, history).with_context(|| {
+            let shallow =
+                RemoteKind::GitDependency.to_shallow_setting(repo.is_shallow(), cargo_config);
+            fetch(&mut repo, &url, &reference, cargo_config, shallow, None).with_context(|| {
                 format!(
                     "failed to fetch submodule `{}` from {}",
                     child.name().unwrap_or(""),
@@ -842,7 +837,8 @@ pub fn fetch(
     orig_url: &str,
     reference: &GitReference,
     config: &Config,
-    history: History,
+    shallow: gix::remote::fetch::Shallow,
+    locked_rev: Option<git2::Oid>,
 ) -> CargoResult<()> {
     if config.frozen() {
         anyhow::bail!(
@@ -855,14 +851,20 @@ pub fn fetch(
     }
 
     // If we're fetching from GitHub, attempt GitHub's special fast path for
-    // testing if we've already got an up-to-date copy of the repository
-    let oid_to_fetch = match github_fast_path(repo, orig_url, reference, config) {
-        Ok(FastPathRev::UpToDate) => return Ok(()),
-        Ok(FastPathRev::NeedsFetch(rev)) => Some(rev),
-        Ok(FastPathRev::Indeterminate) => None,
-        Err(e) => {
-            debug!("failed to check github {:?}", e);
-            None
+    // testing if we've already got an up-to-date copy of the repository.
+    // With shallow histories this is a bit fuzzy, and we opt-out for now.
+    let is_shallow = repo.is_shallow() || !matches!(shallow, gix::remote::fetch::Shallow::NoChange);
+    let oid_to_fetch = if is_shallow {
+        None
+    } else {
+        match github_fast_path(repo, orig_url, reference, config) {
+            Ok(FastPathRev::UpToDate) => return Ok(()),
+            Ok(FastPathRev::NeedsFetch(rev)) => Some(rev),
+            Ok(FastPathRev::Indeterminate) => None,
+            Err(e) => {
+                debug!("failed to check github {:?}", e);
+                None
+            }
         }
     };
 
@@ -881,32 +883,44 @@ pub fn fetch(
     // The `+` symbol on the refspec means to allow a forced (fast-forward)
     // update which is needed if there is ever a force push that requires a
     // fast-forward.
-    match reference {
-        // For branches and tags we can fetch simply one reference and copy it
-        // locally, no need to fetch other branches/tags.
-        GitReference::Branch(b) => {
-            refspecs.push(format!("+refs/heads/{0}:refs/remotes/origin/{0}", b));
-        }
-        GitReference::Tag(t) => {
-            refspecs.push(format!("+refs/tags/{0}:refs/remotes/origin/tags/{0}", t));
-        }
-
-        GitReference::DefaultBranch => {
-            refspecs.push(String::from("+HEAD:refs/remotes/origin/HEAD"));
-        }
+    if let Some(rev) =
+        locked_rev.filter(|_| !matches!(shallow, gix::remote::fetch::Shallow::NoChange))
+    {
+        // If we want a specific revision and know about, obtain that specifically.
+        refspecs.push(format!("+{0}:refs/remotes/origin/HEAD", rev));
+    } else {
+        match reference {
+            // For branches and tags we can fetch simply one reference and copy it
+            // locally, no need to fetch other branches/tags.
+            GitReference::Branch(b) => {
+                refspecs.push(format!("+refs/heads/{0}:refs/remotes/origin/{0}", b));
+            }
+            GitReference::Tag(t) => {
+                refspecs.push(format!("+refs/tags/{0}:refs/remotes/origin/tags/{0}", t));
+            }
 
-        GitReference::Rev(rev) => {
-            if rev.starts_with("refs/") {
-                refspecs.push(format!("+{0}:{0}", rev));
-            } else if let Some(oid_to_fetch) = oid_to_fetch {
-                refspecs.push(format!("+{0}:refs/commit/{0}", oid_to_fetch));
-            } else {
-                // We don't know what the rev will point to. To handle this
-                // situation we fetch all branches and tags, and then we pray
-                // it's somewhere in there.
-                refspecs.push(String::from("+refs/heads/*:refs/remotes/origin/*"));
+            GitReference::DefaultBranch => {
                 refspecs.push(String::from("+HEAD:refs/remotes/origin/HEAD"));
-                tags = true;
+            }
+
+            GitReference::Rev(rev) => {
+                if rev.starts_with("refs/") {
+                    refspecs.push(format!("+{0}:{0}", rev));
+                } else if let Some(oid_to_fetch) = oid_to_fetch {
+                    refspecs.push(format!("+{0}:refs/commit/{0}", oid_to_fetch));
+                } else if is_shallow && git2::Oid::from_str(&rev).is_ok() {
+                    // There is a specific commit to fetch and we will just do so in shallow-mode only
+                    // to not disturb the previous logic. Note that with typical settings for shallowing,
+                    // we will just fetch a specific slice of the history.
+                    refspecs.push(format!("+{0}:refs/remotes/origin/HEAD", rev));
+                } else {
+                    // We don't know what the rev will point to. To handle this
+                    // situation we fetch all branches and tags, and then we pray
+                    // it's somewhere in there.
+                    refspecs.push(String::from("+refs/heads/*:refs/remotes/origin/*"));
+                    refspecs.push(String::from("+HEAD:refs/remotes/origin/HEAD"));
+                    tags = true;
+                }
             }
         }
     }
@@ -986,7 +1000,7 @@ pub fn fetch(
                         );
                         let outcome = connection
                             .prepare_fetch(&mut progress, gix::remote::ref_map::Options::default())?
-                            .with_shallow(history.clone().into())
+                            .with_shallow(shallow.clone().into())
                             .receive(&mut progress, should_interrupt)?;
                         Ok(outcome)
                     });

src/cargo/sources/registry/remote.rs

@@ -305,13 +305,14 @@ impl<'cfg> RegistryData for RemoteRegistry<'cfg> {
         // checkout.
         let url = self.source_id.url();
         let repo = self.repo.borrow_mut().unwrap();
-        let history = RemoteKind::Registry.to_history(repo.is_shallow(), self.config);
+        let shallow = RemoteKind::Registry.to_shallow_setting(repo.is_shallow(), self.config);
         git::fetch(
             repo,
             url.as_str(),
             &self.index_git_ref,
             self.config,
-            history,
+            shallow,
+            None,
         )
         .with_context(|| format!("failed to fetch `{}`", url))?;
 

tests/testsuite/git.rs

@@ -2221,8 +2221,8 @@ fn gitoxide_clones_registry_with_shallow_protocol_and_follow_up_fetch_maintains_
             .ancestors()
             .all()?
             .count(),
-        2,
-        "follow-up fetches maintain shallow-ness, whether it's specified or not, keeping shallow boundary where it is"
+        1,
+        "subsequent shallow fetches wont' fetch what's inbetween, only the single commit that we need while leveraging existing commits"
     );
     assert!(repo.is_shallow());
 
@@ -2238,8 +2238,8 @@ fn gitoxide_clones_registry_with_shallow_protocol_and_follow_up_fetch_maintains_
             .ancestors()
             .all()?
             .count(),
-        4,
-        "shallow boundaries are maintained on subsequent shallow fetches (we don't accidentally unshallow it)"
+        1,
+        "shallow boundaries are moved with each fetch to maintain only a single commit of history"
     );
     assert!(repo.is_shallow());
 
@@ -2358,8 +2358,8 @@ fn gitoxide_clones_registry_without_shallow_protocol_and_follow_up_fetch_uses_sh
             .ancestors()
             .all()?
             .count(),
-        3,
-        "shallow boundaries are maintained on subsequent shallow fetches (we don't accidentally unshallow it)"
+        1,
+        "subsequent shallow fetches wont' fetch what's inbetween, only the single commit that we need while leveraging existing commits"
     );
     assert!(shallow_repo.is_shallow());
 
@@ -2450,15 +2450,15 @@ fn gitoxide_unshallows_git_dependencies_that_may_not_be_shallow_anymore() -> any
         .run();
 
     assert!(
-        !db_clone.is_shallow(),
-        "the clone was unshallowed as we need the entire history for revision based checkouts"
+        db_clone.is_shallow(),
+        "we maintain shallowness and never unshallow"
     );
 
     Ok(())
 }
 
 #[cargo_test]
-fn gitoxide_uses_shallow_deps_only_when_no_revision_is_specified() -> anyhow::Result<()> {
+fn shallow_deps_work_with_revisions_and_branches_mixed_on_same_dependency() -> anyhow::Result<()> {
     let (bar, bar_repo) = git::new_repo("bar", |p| {
         p.file("Cargo.toml", &basic_manifest("bar", "1.0.0"))
             .file("src/lib.rs", "")
@@ -2507,8 +2507,8 @@ fn gitoxide_uses_shallow_deps_only_when_no_revision_is_specified() -> anyhow::Re
     );
     let db_clone = gix::open_opts(&db_paths[0], gix::open::Options::isolated())?;
     assert!(
-        !db_clone.is_shallow(),
-        "despite there being two checkouts, it manages to see that the db must not be shallow"
+        db_clone.is_shallow(),
+        "the repo is shallow while having all data it needs"
     );
 
     Ok(())