Merge branch 'fix-no-want-detection'

Author: Byron

gix/src/remote/connection/fetch/negotiate.rs

@@ -68,6 +68,7 @@ pub(crate) fn mark_complete_and_common_ref(
     graph: &mut gix_negotiate::Graph<'_>,
     ref_map: &fetch::RefMap,
     shallow: &fetch::Shallow,
+    mapping_is_ignored: impl Fn(&fetch::Mapping) -> bool,
 ) -> Result<Action, Error> {
     if let fetch::Shallow::Deepen(0) = shallow {
         // Avoid deepening (relative) with zero as it seems to upset the server. Git also doesn't actually
@@ -89,6 +90,7 @@ pub(crate) fn mark_complete_and_common_ref(
     let mut cutoff_date = None::<SecondsSinceUnixEpoch>;
     let mut num_mappings_with_change = 0;
     let mut remote_ref_target_known: Vec<bool> = std::iter::repeat(false).take(ref_map.mappings.len()).collect();
+    let mut remote_ref_included: Vec<bool> = std::iter::repeat(false).take(ref_map.mappings.len()).collect();
 
     for (mapping_idx, mapping) in ref_map.mappings.iter().enumerate() {
         let want_id = mapping.remote.as_id();
@@ -98,9 +100,13 @@ pub(crate) fn mark_complete_and_common_ref(
             r.target().try_id().map(ToOwned::to_owned)
         });
 
-        // Like git, we don't let known unchanged mappings participate in the tree traversal
-        if want_id.zip(have_id).map_or(true, |(want, have)| want != have) {
-            num_mappings_with_change += 1;
+        // Even for ignored mappings we want to know if the `want` is already present locally, so skip nothing else.
+        if !mapping_is_ignored(mapping) {
+            remote_ref_included[mapping_idx] = true;
+            // Like git, we don't let known unchanged mappings participate in the tree traversal
+            if want_id.zip(have_id).map_or(true, |(want, have)| want != have) {
+                num_mappings_with_change += 1;
+            }
         }
 
         if let Some(commit) = want_id
@@ -114,11 +120,15 @@ pub(crate) fn mark_complete_and_common_ref(
         }
     }
 
-    // If any kind of shallowing operation is desired, the server may still create a pack for us.
     if matches!(shallow, Shallow::NoChange) {
         if num_mappings_with_change == 0 {
             return Ok(Action::NoChange);
-        } else if remote_ref_target_known.iter().all(|known| *known) {
+        } else if remote_ref_target_known
+            .iter()
+            .zip(remote_ref_included)
+            .filter_map(|(known, included)| included.then_some(known))
+            .all(|known| *known)
+        {
             return Ok(Action::SkipToRefUpdate);
         }
     }
@@ -167,40 +177,50 @@ pub(crate) fn mark_complete_and_common_ref(
     })
 }
 
-/// Add all `wants` to `arguments`, which is the unpeeled direct target that the advertised remote ref points to.
-pub(crate) fn add_wants(
-    repo: &crate::Repository,
-    arguments: &mut gix_protocol::fetch::Arguments,
-    ref_map: &fetch::RefMap,
-    mapping_known: &[bool],
-    shallow: &fetch::Shallow,
+/// Create a predicate that checks if a refspec mapping should be ignored.
+///
+/// We want to ignore mappings during negotiation if they would be handled implicitly by the server, which is the case
+/// when tags would be sent implicitly due to `Tags::Included`.
+pub(crate) fn make_refmapping_ignore_predicate(
     fetch_tags: fetch::Tags,
-) {
+    ref_map: &fetch::RefMap,
+) -> impl Fn(&fetch::Mapping) -> bool + '_ {
     // With included tags, we have to keep mappings of tags to handle them later when updating refs, but we don't want to
     // explicitly `want` them as the server will determine by itself which tags are pointing to a commit it wants to send.
     // If we would not exclude implicit tag mappings like this, we would get too much of the graph.
     let tag_refspec_to_ignore = matches!(fetch_tags, crate::remote::fetch::Tags::Included)
         .then(|| fetch_tags.to_refspec())
         .flatten();
+    move |mapping| {
+        tag_refspec_to_ignore.map_or(false, |tag_spec| {
+            mapping
+                .spec_index
+                .implicit_index()
+                .and_then(|idx| ref_map.extra_refspecs.get(idx))
+                .map_or(false, |spec| spec.to_ref() == tag_spec)
+        })
+    }
+}
 
+/// Add all `wants` to `arguments`, which is the unpeeled direct target that the advertised remote ref points to.
+pub(crate) fn add_wants(
+    repo: &crate::Repository,
+    arguments: &mut gix_protocol::fetch::Arguments,
+    ref_map: &fetch::RefMap,
+    mapping_known: &[bool],
+    shallow: &fetch::Shallow,
+    mapping_is_ignored: impl Fn(&fetch::Mapping) -> bool,
+) {
     // When using shallow, we can't exclude `wants` as the remote won't send anything then. Thus we have to resend everything
     // we have as want instead to get exactly the same graph, but possibly deepened.
     let is_shallow = !matches!(shallow, fetch::Shallow::NoChange);
     let wants = ref_map
         .mappings
         .iter()
         .zip(mapping_known)
-        .filter_map(|(m, known)| (is_shallow || !*known).then_some(m));
+        .filter_map(|(m, known)| (is_shallow || !*known).then_some(m))
+        .filter(|m| !mapping_is_ignored(m));
     for want in wants {
-        // Here we ignore implicit tag mappings if needed.
-        if tag_refspec_to_ignore.map_or(false, |tag_spec| {
-            want.spec_index
-                .implicit_index()
-                .and_then(|idx| ref_map.extra_refspecs.get(idx))
-                .map_or(false, |spec| spec.to_ref() == tag_spec)
-        }) {
-            continue;
-        }
         let id_on_remote = want.remote.as_id();
         if !arguments.can_use_ref_in_want() || matches!(want.remote, fetch::Source::ObjectId(_)) {
             if let Some(id) = id_on_remote {

gix/src/remote/connection/fetch/receive_pack.rs

@@ -138,6 +138,7 @@ where
             &mut graph,
             &self.ref_map,
             &self.shallow,
+            negotiate::make_refmapping_ignore_predicate(con.remote.fetch_tags, &self.ref_map),
         )?;
         let mut previous_response = None::<gix_protocol::fetch::Response>;
         let mut round = 1;
@@ -155,7 +156,7 @@ where
                     &self.ref_map,
                     remote_ref_target_known,
                     &self.shallow,
-                    con.remote.fetch_tags,
+                    negotiate::make_refmapping_ignore_predicate(con.remote.fetch_tags, &self.ref_map),
                 );
                 let is_stateless =
                     arguments.is_stateless(!con.transport.connection_persists_across_multiple_requests());

gix/src/remote/fetch.rs

@@ -6,7 +6,7 @@ pub mod negotiate {
     pub use super::super::connection::fetch::negotiate::Error;
     #[cfg(any(feature = "blocking-network-client", feature = "async-network-client"))]
     pub(crate) use super::super::connection::fetch::negotiate::{
-        add_wants, mark_complete_and_common_ref, one_round, Action,
+        add_wants, make_refmapping_ignore_predicate, mark_complete_and_common_ref, one_round, Action,
     };
 }
 

gix/tests/remote/fetch.rs

@@ -335,38 +335,68 @@ mod blocking_and_async_io {
         async(feature = "async-network-client-async-std", async_std::test)
     )]
     async fn fetch_empty_pack() -> crate::Result {
-        let daemon = spawn_git_daemon_if_async(repo_path("base"))?;
-        for (fetch_tags, expected_ref_count) in [
-            (fetch::Tags::None, 1),
-            (fetch::Tags::Included, 7),
-            (fetch::Tags::All, 7),
+        for version in [
+            gix::protocol::transport::Protocol::V1,
+            gix::protocol::transport::Protocol::V2,
         ] {
-            let (repo, _tmp) = repo_rw("two-origins");
-            let mut remote = into_daemon_remote_if_async(
-                repo.head()?
-                    .into_remote(Fetch)
-                    .expect("present")?
-                    .with_fetch_tags(fetch_tags),
-                daemon.as_ref(),
-                None,
-            );
-            remote.replace_refspecs(Some("HEAD:refs/remotes/origin/does-not-yet-exist"), Fetch)?;
-
-            let res = remote
-                .connect(Fetch)
-                .await?
-                .prepare_fetch(gix::progress::Discard, Default::default())
-                .await?
-                .receive(gix::progress::Discard, &AtomicBool::default())
-                .await?;
+            for (shallow_args, expected) in [(None, [1, 7, 7]), (Some("--depth=2"), [1, 2, 2])] {
+                if version == gix::protocol::transport::Protocol::V1 && shallow_args.is_some() {
+                    // TODO: We cannot yet handle shallow mode for V1 as it will send shallow-info as part of the handshake :/.
+                    //       It's probaby not the most important thing to be able to clone from a shallow remote anyway.
+                    continue;
+                }
+                for (fetch_tags, expected_ref_count) in [fetch::Tags::None, fetch::Tags::Included, fetch::Tags::All]
+                    .into_iter()
+                    .zip(expected)
+                {
+                    let (mut repo, _tmp) = try_repo_rw_args(
+                        "two-origins",
+                        shallow_args,
+                        if shallow_args.is_some() {
+                            Mode::CloneWithShallowSupport
+                        } else {
+                            Mode::FastClone
+                        },
+                    )?;
+                    let daemon = spawn_git_daemon_if_async(
+                        repo.work_dir()
+                            .expect("non-bare")
+                            .ancestors()
+                            .nth(1)
+                            .expect("parent")
+                            .join("base"),
+                    )?;
+                    repo.config_snapshot_mut().set_value(
+                        &gix::config::tree::Protocol::VERSION,
+                        (version as u8).to_string().as_str(),
+                    )?;
+                    let mut remote = into_daemon_remote_if_async(
+                        repo.head()?
+                            .into_remote(Fetch)
+                            .expect("present")?
+                            .with_fetch_tags(fetch_tags),
+                        daemon.as_ref(),
+                        None,
+                    );
+                    remote.replace_refspecs(Some("HEAD:refs/remotes/origin/does-not-yet-exist"), Fetch)?;
+
+                    let res = remote
+                        .connect(Fetch)
+                        .await?
+                        .prepare_fetch(gix::progress::Discard, Default::default())
+                        .await?
+                        .receive(gix::progress::Discard, &AtomicBool::default())
+                        .await?;
 
-            match res.status {
-                fetch::Status::NoPackReceived { update_refs } => {
-                    assert_eq!(update_refs.edits.len(), expected_ref_count);
+                    match res.status {
+                    fetch::Status::NoPackReceived { update_refs } => {
+                        assert_eq!(update_refs.edits.len(), expected_ref_count, "{shallow_args:?}|{fetch_tags:?}");
+                    },
+                    _ => unreachable!(
+                        "{shallow_args:?}|{fetch_tags:?}: default negotiation is able to realize nothing is required and doesn't get to receiving a pack"
+                    ),
+                }
                 }
-                _ => unreachable!(
-                    "default negotiation is able to realize nothing is required and doesn't get to receiving a pack"
-                ),
             }
         }
         Ok(())