Skip to content

Commit a146d14

Browse files
committed
fix: don't attempt negotiation without any refspec-mappings (#1405)
Previously, when a shallow operation was specified, it was possible to skip past the no-change test and attempt to negotiate even though there was nothing to want. This is now 'fixed' by simply aborting early if there is no refspec mapping at all. Further, it aborts as early as possible with a nicer error message, after all, there is no use in having no mapping. Note that it's explicitly implemented such that obtaining such an empty refmap is fine, but trying to receive it is not. That way, the user can obtain information about the server without anything being an error.
1 parent 8644d0f commit a146d14

File tree

4 files changed

+19
-1
lines changed

4 files changed

+19
-1
lines changed

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

+5
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,11 @@ pub enum Error {
4747
NegotiationAlgorithmConfig(#[from] config::key::GenericErrorWithValue),
4848
#[error("Failed to read remaining bytes in stream")]
4949
ReadRemainingBytes(#[source] std::io::Error),
50+
#[error("None of the refspec(s) {} matched any of the {num_remote_refs} refs on the remote", refspecs.iter().map(|r| r.to_ref().instruction().to_bstring().to_string()).collect::<Vec<_>>().join(", "))]
51+
NoMapping {
52+
refspecs: Vec<gix_refspec::RefSpec>,
53+
num_remote_refs: usize,
54+
},
5055
}
5156

5257
impl gix_protocol::transport::IsSpuriousError for Error {

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

+4-1
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,7 @@ pub(crate) enum Action {
6161
/// Finally, we also mark tips in the `negotiator` in one go to avoid traversing all refs twice, since we naturally encounter all tips during
6262
/// our own walk.
6363
///
64-
/// Return whether or not we should negotiate, along with a queue for later use.
64+
/// Return whether we should negotiate, along with a queue for later use.
6565
pub(crate) fn mark_complete_and_common_ref(
6666
repo: &crate::Repository,
6767
negotiator: &mut dyn gix_negotiate::Negotiator,
@@ -71,6 +71,9 @@ pub(crate) fn mark_complete_and_common_ref(
7171
mapping_is_ignored: impl Fn(&fetch::Mapping) -> bool,
7272
) -> Result<Action, Error> {
7373
let _span = gix_trace::detail!("mark_complete_and_common_ref", mappings = ref_map.mappings.len());
74+
if ref_map.mappings.is_empty() {
75+
return Ok(Action::NoChange);
76+
}
7477
if let fetch::Shallow::Deepen(0) = shallow {
7578
// Avoid deepening (relative) with zero as it seems to upset the server. Git also doesn't actually
7679
// perform the negotiation for some reason (couldn't find it in code).

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

+9
Original file line numberDiff line numberDiff line change
@@ -91,6 +91,15 @@ where
9191
let _span = gix_trace::coarse!("fetch::Prepare::receive()");
9292
let mut con = self.con.take().expect("receive() can only be called once");
9393

94+
if self.ref_map.mappings.is_empty() && !self.ref_map.remote_refs.is_empty() {
95+
let mut specs = con.remote.fetch_specs.clone();
96+
specs.extend(self.ref_map.extra_refspecs.clone());
97+
return Err(Error::NoMapping {
98+
refspecs: specs,
99+
num_remote_refs: self.ref_map.remote_refs.len(),
100+
});
101+
}
102+
94103
let handshake = &self.ref_map.handshake;
95104
let protocol_version = handshake.server_protocol_version;
96105

gix/src/remote/connection/ref_map.rs

+1
Original file line numberDiff line numberDiff line change
@@ -145,6 +145,7 @@ where
145145
}
146146
}))
147147
.validated()?;
148+
148149
let mappings = res.mappings;
149150
let mappings = mappings
150151
.into_iter()

0 commit comments

Comments
 (0)