From 42cdc3f533ac34812baa63f31a96475eacc83de6 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Fri, 20 Dec 2024 19:24:54 +0100 Subject: [PATCH 1/6] fix: properly indicate the end of interaction --- gitoxide-core/src/net.rs | 18 +++++++++++++++++- gitoxide-core/src/pack/receive.rs | 6 +++--- 2 files changed, 20 insertions(+), 4 deletions(-) diff --git a/gitoxide-core/src/net.rs b/gitoxide-core/src/net.rs index 52f7661e89a..e5c62d52927 100644 --- a/gitoxide-core/src/net.rs +++ b/gitoxide-core/src/net.rs @@ -36,4 +36,20 @@ mod impls { } #[cfg(any(feature = "async-client", feature = "blocking-client"))] -pub use gix::protocol::transport::connect; +#[gix::protocol::maybe_async::maybe_async] +pub async fn connect( + url: Url, + options: gix::protocol::transport::client::connect::Options, +) -> Result< + gix::protocol::SendFlushOnDrop>, + gix::protocol::transport::client::connect::Error, +> +where + Url: TryInto, + gix::url::parse::Error: From, +{ + Ok(gix::protocol::SendFlushOnDrop::new( + gix::protocol::transport::connect(url, options).await?, + false, + )) +} diff --git a/gitoxide-core/src/pack/receive.rs b/gitoxide-core/src/pack/receive.rs index 34cbd9c9e83..019a046351a 100644 --- a/gitoxide-core/src/pack/receive.rs +++ b/gitoxide-core/src/pack/receive.rs @@ -64,7 +64,7 @@ where let agent = gix::protocol::agent(gix::env::agent()); let mut handshake = gix::protocol::fetch::handshake( - &mut transport, + &mut transport.inner, gix::protocol::credentials::builtin, vec![("agent".into(), Some(agent.clone()))], &mut progress, @@ -85,7 +85,7 @@ where &fetch_refspecs, gix::protocol::fetch::Context { handshake: &mut handshake, - transport: &mut transport, + transport: &mut transport.inner, user_agent: user_agent.clone(), trace_packetlines, }, @@ -114,7 +114,7 @@ where &ctx.should_interrupt, gix::protocol::fetch::Context { handshake: &mut handshake, - transport: &mut transport, + transport: &mut transport.inner, user_agent, trace_packetlines, }, From a5f0d295cfc7183e5c0b35b72610e933f91059d9 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Fri, 20 Dec 2024 19:11:56 +0100 Subject: [PATCH 2/6] fix!: assure that empty fetches due to no objects wanted are terminated early --- gix-protocol/src/fetch/error.rs | 4 ++-- gix-protocol/src/fetch/function.rs | 10 ++++++---- gix-protocol/src/fetch/negotiate.rs | 9 ++++++++- gix-protocol/src/fetch/types.rs | 4 +++- 4 files changed, 19 insertions(+), 8 deletions(-) diff --git a/gix-protocol/src/fetch/error.rs b/gix-protocol/src/fetch/error.rs index 149268e5f12..b7329ecfc83 100644 --- a/gix-protocol/src/fetch/error.rs +++ b/gix-protocol/src/fetch/error.rs @@ -21,8 +21,8 @@ pub enum Error { LockShallowFile(#[from] gix_lock::acquire::Error), #[error("Receiving objects from shallow remotes is prohibited due to the value of `clone.rejectShallow`")] RejectShallowRemote, - #[error("Failed to consume the pack sent by the remove")] - ConsumePack(Box), + #[error("Failed to consume the pack sent by the remote")] + ConsumePack(#[source] Box), #[error("Failed to read remaining bytes in stream")] ReadRemainingBytes(#[source] std::io::Error), } diff --git a/gix-protocol/src/fetch/function.rs b/gix-protocol/src/fetch/function.rs index 57b7549cba6..4df87b3672d 100644 --- a/gix-protocol/src/fetch/function.rs +++ b/gix-protocol/src/fetch/function.rs @@ -22,12 +22,12 @@ use std::sync::atomic::{AtomicBool, Ordering}; /// * …update local refs /// * …end the interaction after the fetch /// -/// Note that the interaction will never be ended, even on error or failure, leaving it up to the caller to do that, maybe +/// **Note that the interaction will never be ended**, even on error or failure, leaving it up to the caller to do that, maybe /// with the help of [`SendFlushOnDrop`](crate::SendFlushOnDrop) which can wrap `transport`. /// Generally, the `transport` is left in a state that allows for more commands to be run. /// -/// Return `Ok(None)` if there was nothing to do because all remote refs are at the same state as they are locally, or `Ok(Some(outcome))` -/// to inform about all the changes that were made. +/// Return `Ok(None)` if there was nothing to do because all remote refs are at the same state as they are locally, +/// or there was nothing wanted, or `Ok(Some(outcome))` to inform about all the changes that were made. #[maybe_async::maybe_async] pub async fn fetch( negotiate: &mut impl Negotiate, @@ -91,7 +91,9 @@ where negotiate::Action::MustNegotiate { remote_ref_target_known, } => { - negotiate.add_wants(&mut arguments, remote_ref_target_known); + if !negotiate.add_wants(&mut arguments, remote_ref_target_known) { + return Ok(None); + } let mut rounds = Vec::new(); let is_stateless = arguments.is_stateless(!transport.connection_persists_across_multiple_requests()); let mut state = negotiate::one_round::State::new(is_stateless); diff --git a/gix-protocol/src/fetch/negotiate.rs b/gix-protocol/src/fetch/negotiate.rs index 9dd4f3222e5..d28feef86a2 100644 --- a/gix-protocol/src/fetch/negotiate.rs +++ b/gix-protocol/src/fetch/negotiate.rs @@ -284,6 +284,9 @@ pub fn make_refmapping_ignore_predicate(fetch_tags: Tags, ref_map: &RefMap) -> i /// * `ref_map` is the state of refs as known on the remote. /// * `shallow` defines if the history should be shallow. /// * `mapping_is_ignored` is typically initialized with [`make_refmapping_ignore_predicate`]. +/// +/// Returns `true` if at least one [want](crate::fetch::Arguments::want()) was added, or `false` otherwise. +/// Note that not adding a single want can make the remote hang, so it's avoided on the client side by ending the fetch operation. pub fn add_wants( objects: &impl gix_object::FindHeader, arguments: &mut crate::fetch::Arguments, @@ -291,10 +294,11 @@ pub fn add_wants( remote_ref_target_known: &[bool], shallow: &Shallow, mapping_is_ignored: impl Fn(&refmap::Mapping) -> bool, -) { +) -> 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, Shallow::NoChange); + let mut has_want = false; let wants = ref_map .mappings .iter() @@ -306,6 +310,7 @@ pub fn add_wants( if !arguments.can_use_ref_in_want() || matches!(want.remote, refmap::Source::ObjectId(_)) { if let Some(id) = id_on_remote { arguments.want(id); + has_want = true; } } else { arguments.want_ref( @@ -313,6 +318,7 @@ pub fn add_wants( .as_name() .expect("name available if this isn't an object id"), ); + has_want = true; } let id_is_annotated_tag_we_have = id_on_remote .and_then(|id| objects.try_header(id).ok().flatten().map(|h| (id, h))) @@ -324,6 +330,7 @@ pub fn add_wants( arguments.have(tag_on_remote); } } + has_want } /// Remove all commits that are more recent than the cut-off, which is the commit time of the oldest common commit we have with the server. diff --git a/gix-protocol/src/fetch/types.rs b/gix-protocol/src/fetch/types.rs index 207266eb301..fbc40b9239a 100644 --- a/gix-protocol/src/fetch/types.rs +++ b/gix-protocol/src/fetch/types.rs @@ -70,7 +70,9 @@ mod with_fetch { /// Typically invokes [`negotiate::mark_complete_and_common_ref()`]. fn mark_complete_and_common_ref(&mut self) -> Result; /// Typically invokes [`negotiate::add_wants()`]. - fn add_wants(&mut self, arguments: &mut fetch::Arguments, remote_ref_target_known: &[bool]); + /// Returns `true` if wants were added, or `false` if the negotiation should be aborted. + #[must_use] + fn add_wants(&mut self, arguments: &mut fetch::Arguments, remote_ref_target_known: &[bool]) -> bool; /// Typically invokes [`negotiate::one_round()`]. fn one_round( &mut self, From 41b6571b9f331c018672fcd0bb7d5ce0f8885178 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Fri, 20 Dec 2024 19:14:47 +0100 Subject: [PATCH 3/6] adapt to changes in `gix-protocol` --- gitoxide-core/src/pack/receive.rs | 5 ++++- gix/src/remote/connection/fetch/receive_pack.rs | 4 ++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/gitoxide-core/src/pack/receive.rs b/gitoxide-core/src/pack/receive.rs index 019a046351a..8fbee2be6f8 100644 --- a/gitoxide-core/src/pack/receive.rs +++ b/gitoxide-core/src/pack/receive.rs @@ -140,10 +140,13 @@ impl gix::protocol::fetch::Negotiate for Negotiate<'_> { }) } - fn add_wants(&mut self, arguments: &mut Arguments, _remote_ref_target_known: &[bool]) { + fn add_wants(&mut self, arguments: &mut Arguments, _remote_ref_target_known: &[bool]) -> bool { + let mut has_want = false; for id in self.refmap.mappings.iter().filter_map(|m| m.remote.as_id()) { arguments.want(id); + has_want = true; } + has_want } fn one_round( diff --git a/gix/src/remote/connection/fetch/receive_pack.rs b/gix/src/remote/connection/fetch/receive_pack.rs index ab474bb50c3..ef328da2d38 100644 --- a/gix/src/remote/connection/fetch/receive_pack.rs +++ b/gix/src/remote/connection/fetch/receive_pack.rs @@ -274,7 +274,7 @@ impl gix_protocol::fetch::Negotiate for Negotiate<'_, '_, '_> { ) } - fn add_wants(&mut self, arguments: &mut Arguments, remote_ref_target_known: &[bool]) { + fn add_wants(&mut self, arguments: &mut Arguments, remote_ref_target_known: &[bool]) -> bool { negotiate::add_wants( self.objects, arguments, @@ -282,7 +282,7 @@ impl gix_protocol::fetch::Negotiate for Negotiate<'_, '_, '_> { remote_ref_target_known, self.shallow, negotiate::make_refmapping_ignore_predicate(self.tags, self.ref_map), - ); + ) } fn one_round( From 377ca46162d9ecbb5e5e27eca889f95fd8556b50 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Fri, 20 Dec 2024 19:39:30 +0100 Subject: [PATCH 4/6] Assure we don't try to not have wants in pack-receive Otherwise we will fail to produce a nice error message. --- gitoxide-core/src/pack/receive.rs | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/gitoxide-core/src/pack/receive.rs b/gitoxide-core/src/pack/receive.rs index 8fbee2be6f8..426905da529 100644 --- a/gitoxide-core/src/pack/receive.rs +++ b/gitoxide-core/src/pack/receive.rs @@ -3,6 +3,7 @@ use crate::pack::receive::protocol::fetch::negotiate; use crate::OutputFormat; use gix::config::tree::Key; use gix::protocol::maybe_async; +use gix::remote::fetch::Error; use gix::DynNestedProgress; pub use gix::{ hash::ObjectId, @@ -92,6 +93,15 @@ where gix::protocol::fetch::refmap::init::Options::default(), ) .await?; + + if refmap.mappings.is_empty() && !refmap.remote_refs.is_empty() { + return Err(Error::NoMapping { + refspecs: refmap.refspecs.clone(), + num_remote_refs: refmap.remote_refs.len(), + } + .into()); + } + let mut negotiate = Negotiate { refmap: &refmap }; gix::protocol::fetch( &mut negotiate, From 466fe524451064339a4e603526ea3a5bc30b6fb8 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Fri, 20 Dec 2024 19:51:17 +0100 Subject: [PATCH 5/6] re-enable running git-daemon tests on CI It became clear that the issue was home-made. The remote interactions of the 'free' variants never terminated the connection, which could have left the daemon hanging. It might have filled up its 32 slots, leading to rejection later, maybe. --- tests/journey/gix.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/journey/gix.sh b/tests/journey/gix.sh index a7acfc8f70e..dd180cff8c7 100644 --- a/tests/journey/gix.sh +++ b/tests/journey/gix.sh @@ -109,7 +109,7 @@ title "gix (with repository)" # for some reason, on CI the daemon always shuts down before we can connect, # or isn't actually ready despite having accepted the first connection already. - (not_on_ci with "git:// protocol" + (with "git:// protocol" launch-git-daemon (with "version 1" it "generates the correct output" && { @@ -278,7 +278,7 @@ title "gix commit-graph" ) ) fi - (not_on_ci with "git:// protocol" + (with "git:// protocol" launch-git-daemon (with "version 1" (with "NO output directory" From 5c21ebc3f523bbe64cb84bbcdf39a2c284ba1df1 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Fri, 20 Dec 2024 20:19:15 +0100 Subject: [PATCH 6/6] fix: respect interrupt-requests during negotiation It is possible for the negotiation to go on forever if implemented incorrectly on the client side. Hence it's useful to gracefully interrupt it. --- gix-protocol/src/fetch/function.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/gix-protocol/src/fetch/function.rs b/gix-protocol/src/fetch/function.rs index 4df87b3672d..8ae294167cc 100644 --- a/gix-protocol/src/fetch/function.rs +++ b/gix-protocol/src/fetch/function.rs @@ -101,6 +101,11 @@ where let _round = gix_trace::detail!("negotiate round", round = rounds.len() + 1); progress.step(); progress.set_name(format!("negotiate (round {})", rounds.len() + 1)); + if should_interrupt.load(Ordering::Relaxed) { + return Err(Error::Negotiate(negotiate::Error::NegotiationFailed { + rounds: rounds.len(), + })); + } let is_done = match negotiate.one_round(&mut state, &mut arguments, previous_response.as_ref()) { Ok((round, is_done)) => {