diff --git a/cargo-smart-release/Cargo.lock b/cargo-smart-release/Cargo.lock index d327b537771..4c1d9a57741 100644 --- a/cargo-smart-release/Cargo.lock +++ b/cargo-smart-release/Cargo.lock @@ -189,9 +189,9 @@ dependencies = [ [[package]] name = "cargo_metadata" -version = "0.15.4" +version = "0.17.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "eee4243f1f26fc7a42710e7439c149e2b10b05472f88090acce52632f231a73a" +checksum = "e7daec1a2a2129eeba1644b220b4647ec537b0b5d4bfd6876fcc5a540056b592" dependencies = [ "camino", "cargo-platform", @@ -309,9 +309,9 @@ dependencies = [ [[package]] name = "crates-index" -version = "2.0.0" +version = "2.1.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "3feb29cc02fb72a5999ea88eaf38e82fb9ef453a746925d3bf74ebc25940c34c" +checksum = "a2d899e96f67edf44af465951d34f354c7bc04d10a42b813537cd70d62dfbb50" dependencies = [ "gix", "hex", diff --git a/cargo-smart-release/Cargo.toml b/cargo-smart-release/Cargo.toml index c64d3a9e623..1b1952c06a0 100644 --- a/cargo-smart-release/Cargo.toml +++ b/cargo-smart-release/Cargo.toml @@ -28,11 +28,11 @@ gix = { version = "^0.50.0", default-features = false, features = ["max-performa anyhow = "1.0.42" clap = { version = "4.1.0", features = ["derive", "cargo"] } env_logger = { version = "0.10.0", default-features = false, features = ["humantime", "auto-color"] } -cargo_metadata = "0.15.0" +cargo_metadata = "0.17.0" log = "0.4.14" toml_edit = "0.19.1" semver = "1.0.4" -crates-index = { version = "2.0.0", default-features = false, features = ["git-performance", "git-https"] } +crates-index = { version = "2.1.0", default-features = false, features = ["git-performance", "git-https"] } cargo_toml = "0.15.1" winnow = "0.5.1" git-conventional = "0.12.0" diff --git a/cargo-smart-release/src/crates_index.rs b/cargo-smart-release/src/crates_index.rs index 45884f4c3a1..7695ae12a32 100644 --- a/cargo-smart-release/src/crates_index.rs +++ b/cargo-smart-release/src/crates_index.rs @@ -1,5 +1,3 @@ -use std::path::PathBuf; - pub struct Index { inner: Option, } @@ -7,13 +5,8 @@ pub struct Index { impl Index { /// Like the original one, but doesn't create it if it doesn't exist pub fn new_cargo_default() -> Result { - let path = default_path(); Ok(Index { - inner: if path.is_dir() { - crates_index::GitIndex::new_cargo_default()?.into() - } else { - None - }, + inner: crates_index::GitIndex::try_new_cargo_default()?, }) } @@ -29,9 +22,3 @@ impl Index { self.inner.as_ref().and_then(|idx| idx.crate_(name)) } } - -fn default_path() -> PathBuf { - crates_index::local_path_and_canonical_url(crates_index::git::URL, None) - .expect("defaults are well-known") - .0 -} diff --git a/gitoxide-core/src/repository/fetch.rs b/gitoxide-core/src/repository/fetch.rs index 9786c15892b..311273d2b2b 100644 --- a/gitoxide-core/src/repository/fetch.rs +++ b/gitoxide-core/src/repository/fetch.rs @@ -18,6 +18,7 @@ pub const PROGRESS_RANGE: std::ops::RangeInclusive = 1..=3; pub(crate) mod function { use anyhow::bail; + use gix::remote::fetch::refs::update::TypeChange; use gix::{prelude::ObjectIdExt, refspec::match_group::validate::Fix, remote::fetch::Status}; use layout::{ backends::svg::SVGWriter, @@ -261,11 +262,28 @@ pub(crate) mod function { crate::repository::remote::refs::print_ref(&mut out, r)?; } }; + let mode_and_type = update.type_change.map_or_else( + || format!("{}", update.mode), + |type_change| { + format!( + "{} ({})", + update.mode, + match type_change { + TypeChange::DirectToSymbolic => { + "direct ref overwrites symbolic" + } + TypeChange::SymbolicToDirect => { + "symbolic ref overwrites direct" + } + } + ) + }, + ); match edit { Some(edit) => { - writeln!(out, " -> {} [{}]", edit.name, update.mode) + writeln!(out, " -> {} [{mode_and_type}]", edit.name) } - None => writeln!(out, " [{}]", update.mode), + None => writeln!(out, " [{mode_and_type}]"), }?; } consume_skipped_tags(&mut skipped_due_to_implicit_tag, &mut out)?; diff --git a/gitoxide-core/src/repository/remote.rs b/gitoxide-core/src/repository/remote.rs index a76d4602a00..7a3f44be7a1 100644 --- a/gitoxide-core/src/repository/remote.rs +++ b/gitoxide-core/src/repository/remote.rs @@ -244,6 +244,8 @@ mod refs_impl { }, Symbolic { path: String, + #[cfg_attr(feature = "serde", serde(skip_serializing_if = "Option::is_none"))] + tag: Option, target: String, object: String, }, @@ -265,10 +267,12 @@ mod refs_impl { }, handshake::Ref::Symbolic { full_ref_name: path, + tag, target, object, } => JsonRef::Symbolic { path: path.to_string(), + tag: tag.map(|t| t.to_string()), target: target.to_string(), object: object.to_string(), }, @@ -298,9 +302,15 @@ mod refs_impl { } => write!(&mut out, "{tag} {path} object:{object}").map(|_| tag.as_ref()), handshake::Ref::Symbolic { full_ref_name: path, + tag, target, object, - } => write!(&mut out, "{object} {path} symref-target:{target}").map(|_| object.as_ref()), + } => match tag { + Some(tag) => { + write!(&mut out, "{tag} {path} symref-target:{target} peeled:{object}").map(|_| tag.as_ref()) + } + None => write!(&mut out, "{object} {path} symref-target:{target}").map(|_| object.as_ref()), + }, handshake::Ref::Unborn { full_ref_name, target } => { static NULL: gix::hash::ObjectId = gix::hash::ObjectId::null(gix::hash::Kind::Sha1); write!(&mut out, "unborn {full_ref_name} symref-target:{target}").map(|_| NULL.as_ref()) diff --git a/gix-actor/src/signature/decode.rs b/gix-actor/src/signature/decode.rs index d9c1499ea78..2d80df72f6e 100644 --- a/gix-actor/src/signature/decode.rs +++ b/gix-actor/src/signature/decode.rs @@ -2,6 +2,7 @@ pub(crate) mod function { use bstr::ByteSlice; use btoi::btoi; use gix_date::{time::Sign, OffsetInSeconds, SecondsSinceUnixEpoch, Time}; + use nom::multi::many1_count; use nom::{ branch::alt, bytes::complete::{tag, take, take_until, take_while_m_n}, @@ -10,6 +11,7 @@ pub(crate) mod function { sequence::{terminated, tuple}, IResult, }; + use std::cell::RefCell; use crate::{IdentityRef, SignatureRef}; @@ -19,7 +21,9 @@ pub(crate) mod function { pub fn decode<'a, E: ParseError<&'a [u8]> + ContextError<&'a [u8]>>( i: &'a [u8], ) -> IResult<&'a [u8], SignatureRef<'a>, E> { - let (i, (identity, _, time, tzsign, hours, minutes)) = context( + use nom::Parser; + let tzsign = RefCell::new(b'-'); // TODO: there should be no need for this. + let (i, (identity, _, time, _tzsign_count, hours, minutes)) = context( " <> <+|->", tuple(( identity, @@ -31,7 +35,13 @@ pub(crate) mod function { .map_err(|_| nom::Err::Error(E::from_error_kind(i, nom::error::ErrorKind::MapRes))) }) }), - context("+|-", alt((tag(b"-"), tag(b"+")))), + context( + "+|-", + alt(( + many1_count(tag(b"-")).map(|_| *tzsign.borrow_mut() = b'-'), // TODO: this should be a non-allocating consumer of consecutive tags + many1_count(tag(b"+")).map(|_| *tzsign.borrow_mut() = b'+'), + )), + ), context("HH", |i| { take_while_m_n(2usize, 2, is_digit)(i).and_then(|(i, v)| { btoi::(v) @@ -40,7 +50,7 @@ pub(crate) mod function { }) }), context("MM", |i| { - take_while_m_n(2usize, 2, is_digit)(i).and_then(|(i, v)| { + take_while_m_n(1usize, 2, is_digit)(i).and_then(|(i, v)| { btoi::(v) .map(|v| (i, v)) .map_err(|_| nom::Err::Error(E::from_error_kind(i, nom::error::ErrorKind::MapRes))) @@ -49,8 +59,9 @@ pub(crate) mod function { )), )(i)?; - debug_assert!(tzsign[0] == b'-' || tzsign[0] == b'+', "parser assure it's +|- only"); - let sign = if tzsign[0] == b'-' { Sign::Minus } else { Sign::Plus }; // + let tzsign = tzsign.into_inner(); + debug_assert!(tzsign == b'-' || tzsign == b'+', "parser assure it's +|- only"); + let sign = if tzsign == b'-' { Sign::Minus } else { Sign::Plus }; // let offset = (hours * 3600 + minutes * 60) * if sign == Sign::Minus { -1 } else { 1 }; Ok(( @@ -148,6 +159,16 @@ mod tests { ); } + #[test] + fn negative_offset_double_dash() { + assert_eq!( + decode(b"name 1288373970 --700") + .expect("parse to work") + .1, + signature("name", "name@example.com", 1288373970, Sign::Minus, -252000) + ); + } + #[test] fn empty_name_and_email() { assert_eq!( diff --git a/gix-object/tests/commit/from_bytes.rs b/gix-object/tests/commit/from_bytes.rs index 9fd137e7dda..26f5be64142 100644 --- a/gix-object/tests/commit/from_bytes.rs +++ b/gix-object/tests/commit/from_bytes.rs @@ -160,6 +160,32 @@ fn pre_epoch() -> crate::Result { Ok(()) } +#[test] +fn double_dash_special_time_offset() -> crate::Result { + let signature = || SignatureRef { + name: "name".into(), + email: "name@example.com".into(), + time: Time { + seconds: 1288373970, + offset: -252000, + sign: Sign::Minus, + }, + }; + assert_eq!( + CommitRef::from_bytes(&fixture_name("commit", "double-dash-date-offset.txt"))?, + CommitRef { + tree: b"0a851d7a2a66084ab10516c406a405d147e974ad".as_bstr(), + parents: SmallVec::from(vec![b"31350f4f0f459485eff2131517e3450cf251f6fa".as_bstr()]), + author: signature(), + committer: signature(), + encoding: None, + message: "msg\n".into(), + extra_headers: vec![] + } + ); + Ok(()) +} + #[test] fn with_trailer() -> crate::Result { let kim = SignatureRef { diff --git a/gix-object/tests/fixtures/commit/double-dash-date-offset.txt b/gix-object/tests/fixtures/commit/double-dash-date-offset.txt new file mode 100644 index 00000000000..b7e792e9594 --- /dev/null +++ b/gix-object/tests/fixtures/commit/double-dash-date-offset.txt @@ -0,0 +1,6 @@ +tree 0a851d7a2a66084ab10516c406a405d147e974ad +parent 31350f4f0f459485eff2131517e3450cf251f6fa +author name 1288373970 --700 +committer name 1288373970 --700 + +msg diff --git a/gix-odb/src/store_impls/dynamic/load_index.rs b/gix-odb/src/store_impls/dynamic/load_index.rs index b6522c3698e..84ba3b48f9a 100644 --- a/gix-odb/src/store_impls/dynamic/load_index.rs +++ b/gix-odb/src/store_impls/dynamic/load_index.rs @@ -493,6 +493,11 @@ impl super::Store { // Unlike libgit2, do not sort by modification date, but by size and put the biggest indices first. That way // the chance to hit an object should be higher. We leave it to the handle to sort by LRU. // Git itself doesn't change the order which may safe time, but we want it to be stable which also helps some tests. + // NOTE: this will work well for well-packed repos or those using geometric repacking, but force us to open a lot + // of files when dealing with new objects, as there is no notion of recency here as would be with unmaintained + // repositories. Different algorithms should be provided, like newest packs first, and possibly a mix of both + // with big packs first, then sorting by recency for smaller packs. + // We also want to implement `fetch.unpackLimit` to alleviate this issue a little. indices_by_modification_time.sort_by(|l, r| l.2.cmp(&r.2).reverse()); Ok(indices_by_modification_time) } diff --git a/gix-protocol/src/fetch/arguments/blocking_io.rs b/gix-protocol/src/fetch/arguments/blocking_io.rs index 571792148f1..c946d46e144 100644 --- a/gix-protocol/src/fetch/arguments/blocking_io.rs +++ b/gix-protocol/src/fetch/arguments/blocking_io.rs @@ -45,9 +45,7 @@ impl Arguments { } transport.invoke( Command::Fetch.as_str(), - self.features - .iter() - .filter_map(|(k, v)| v.as_ref().map(|v| (*k, Some(v.as_ref())))), + self.features.iter().filter(|(_, v)| v.is_some()).cloned(), Some(std::mem::replace(&mut self.args, retained_state).into_iter()), ) } diff --git a/gix-protocol/src/fetch/arguments/mod.rs b/gix-protocol/src/fetch/arguments/mod.rs index d887492b4a9..50145bb1504 100644 --- a/gix-protocol/src/fetch/arguments/mod.rs +++ b/gix-protocol/src/fetch/arguments/mod.rs @@ -165,20 +165,30 @@ impl Arguments { pub fn use_include_tag(&mut self) { debug_assert!(self.supports_include_tag, "'include-tag' feature required"); if self.supports_include_tag { - match self.version { - gix_transport::Protocol::V0 | gix_transport::Protocol::V1 => { - let features = self - .features_for_first_want - .as_mut() - .expect("call use_include_tag before want()"); - features.push("include-tag".into()) - } - gix_transport::Protocol::V2 => { - self.args.push("include-tag".into()); - } + self.add_feature("include-tag"); + } + } + + /// Add the given `feature`, unconditionally. + /// + /// Note that sending an unknown or unsupported feature may cause the remote to terminate + /// the connection. Use this method if you know what you are doing *and* there is no specialized + /// method for this, e.g. [`Self::use_include_tag()`]. + pub fn add_feature(&mut self, feature: &str) { + match self.version { + gix_transport::Protocol::V0 | gix_transport::Protocol::V1 => { + let features = self + .features_for_first_want + .as_mut() + .expect("call add_feature before first want()"); + features.push(feature.into()) + } + gix_transport::Protocol::V2 => { + self.args.push(feature.into()); } } } + fn prefixed(&mut self, prefix: &str, value: impl fmt::Display) { self.args.push(format!("{prefix}{value}").into()); } diff --git a/gix-protocol/src/fetch/tests.rs b/gix-protocol/src/fetch/tests.rs index 80dc752dd2d..93cf6e8db78 100644 --- a/gix-protocol/src/fetch/tests.rs +++ b/gix-protocol/src/fetch/tests.rs @@ -319,6 +319,7 @@ mod arguments { assert!(arguments.is_stateless(true), "V2 is stateless…"); assert!(arguments.is_stateless(false), "…in all cases"); + arguments.add_feature("no-progress"); arguments.deepen(1); arguments.deepen_relative(); arguments.want(id("7b333369de1221f9bfbbe03a3a13e9a09bc1c907")); @@ -329,6 +330,7 @@ mod arguments { b"0012command=fetch 0001000ethin-pack 000eofs-delta +0010no-progress 000ddeepen 1 0014deepen-relative 0032want 7b333369de1221f9bfbbe03a3a13e9a09bc1c907 @@ -347,6 +349,7 @@ mod arguments { let mut t = transport(&mut out, *is_stateful); let mut arguments = arguments_v2(Some("shallow")); + arguments.add_feature("no-progress"); arguments.deepen(1); arguments.deepen_since(12345); arguments.shallow(id("7b333369de1221f9bfbbe03a3a13e9a09bc1c9ff")); @@ -362,6 +365,7 @@ mod arguments { b"0012command=fetch 0001000ethin-pack 000eofs-delta +0010no-progress 000ddeepen 1 0017deepen-since 12345 0035shallow 7b333369de1221f9bfbbe03a3a13e9a09bc1c9ff @@ -371,6 +375,7 @@ mod arguments { 00000012command=fetch 0001000ethin-pack 000eofs-delta +0010no-progress 000ddeepen 1 0017deepen-since 12345 0035shallow 7b333369de1221f9bfbbe03a3a13e9a09bc1c9ff diff --git a/gix-protocol/src/handshake/mod.rs b/gix-protocol/src/handshake/mod.rs index 6d70ed14500..28243e96da7 100644 --- a/gix-protocol/src/handshake/mod.rs +++ b/gix-protocol/src/handshake/mod.rs @@ -21,7 +21,7 @@ pub enum Ref { /// The hash of the object the ref points to. object: gix_hash::ObjectId, }, - /// A symbolic ref pointing to `target` ref, which in turn points to an `object` + /// A symbolic ref pointing to `target` ref, which in turn, ultimately after possibly following `tag`, points to an `object` Symbolic { /// The name at which the symbolic ref is located, like `HEAD`. full_ref_name: BString, @@ -31,7 +31,11 @@ pub enum Ref { /// /// [#205]: https://github.com/Byron/gitoxide/issues/205 target: BString, - /// The hash of the object the `target` ref points to. + /// The hash of the annotated tag the ref points to, if present. + /// + /// Note that this field is also `None` if `full_ref_name` is a lightweight tag. + tag: Option, + /// The hash of the object the `target` ref ultimately points to. object: gix_hash::ObjectId, }, /// A ref is unborn on the remote and just points to the initial, unborn branch, as is the case in a newly initialized repository diff --git a/gix-protocol/src/handshake/refs/mod.rs b/gix-protocol/src/handshake/refs/mod.rs index 889842e4c3f..39c6c85a942 100644 --- a/gix-protocol/src/handshake/refs/mod.rs +++ b/gix-protocol/src/handshake/refs/mod.rs @@ -38,10 +38,17 @@ impl Ref { /// If `unborn`, the first object id will be the null oid. pub fn unpack(&self) -> (&BStr, Option<&gix_hash::oid>, Option<&gix_hash::oid>) { match self { - Ref::Direct { full_ref_name, object } - | Ref::Symbolic { - full_ref_name, object, .. - } => (full_ref_name.as_ref(), Some(object), None), + Ref::Direct { full_ref_name, object } => (full_ref_name.as_ref(), Some(object), None), + Ref::Symbolic { + full_ref_name, + tag, + object, + .. + } => ( + full_ref_name.as_ref(), + Some(tag.as_deref().unwrap_or(object)), + tag.as_deref().map(|_| object.as_ref()), + ), Ref::Peeled { full_ref_name, tag: object, diff --git a/gix-protocol/src/handshake/refs/shared.rs b/gix-protocol/src/handshake/refs/shared.rs index 046a2a1b1ba..45aefe68b23 100644 --- a/gix-protocol/src/handshake/refs/shared.rs +++ b/gix-protocol/src/handshake/refs/shared.rs @@ -8,20 +8,33 @@ impl From for Ref { InternalRef::Symbolic { path, target: Some(target), + tag, object, } => Ref::Symbolic { full_ref_name: path, target, + tag, object, }, InternalRef::Symbolic { path, target: None, + tag: None, object, } => Ref::Direct { full_ref_name: path, object, }, + InternalRef::Symbolic { + path, + target: None, + tag: Some(tag), + object, + } => Ref::Peeled { + full_ref_name: path, + tag, + object, + }, InternalRef::Peeled { path, tag, object } => Ref::Peeled { full_ref_name: path, tag, @@ -56,6 +69,7 @@ pub(crate) enum InternalRef { /// /// The latter is more of an edge case, please [this issue][#205] for details. target: Option, + tag: Option, object: gix_hash::ObjectId, }, /// extracted from V1 capabilities, which contain some important symbolic refs along with their targets @@ -155,6 +169,7 @@ pub(in crate::handshake::refs) fn parse_v1( Some(position) => match out_refs.swap_remove(position) { InternalRef::SymbolicForLookup { path: _, target } => out_refs.push(InternalRef::Symbolic { path: path.into(), + tag: None, // TODO: figure out how annotated tags work here. object, target, }), @@ -172,7 +187,7 @@ pub(in crate::handshake::refs) fn parse_v1( pub(in crate::handshake::refs) fn parse_v2(line: &BStr) -> Result { let trimmed = line.trim_end(); - let mut tokens = trimmed.splitn(3, |b| *b == b' '); + let mut tokens = trimmed.splitn(4, |b| *b == b' '); match (tokens.next(), tokens.next()) { (Some(hex_hash), Some(path)) => { let id = if hex_hash == b"unborn" { @@ -183,7 +198,9 @@ pub(in crate::handshake::refs) fn parse_v2(line: &BStr) -> Result { if path.is_empty() { return Err(Error::MalformedV2RefLine(trimmed.to_owned().into())); } - Ok(if let Some(attribute) = tokens.next() { + let mut symref_target = None; + let mut peeled = None; + for attribute in tokens.by_ref().take(2) { let mut tokens = attribute.splitn(2, |b| *b == b':'); match (tokens.next(), tokens.next()) { (Some(attribute), Some(value)) => { @@ -191,32 +208,12 @@ pub(in crate::handshake::refs) fn parse_v2(line: &BStr) -> Result { return Err(Error::MalformedV2RefLine(trimmed.to_owned().into())); } match attribute { - b"peeled" => Ref::Peeled { - full_ref_name: path.into(), - object: gix_hash::ObjectId::from_hex(value.as_bytes())?, - tag: id.ok_or(Error::InvariantViolation { - message: "got 'unborn' as tag target", - })?, - }, - b"symref-target" => match value { - b"(null)" => Ref::Direct { - full_ref_name: path.into(), - object: id.ok_or(Error::InvariantViolation { - message: "got 'unborn' while (null) was a symref target", - })?, - }, - name => match id { - Some(id) => Ref::Symbolic { - full_ref_name: path.into(), - object: id, - target: name.into(), - }, - None => Ref::Unborn { - full_ref_name: path.into(), - target: name.into(), - }, - }, - }, + b"peeled" => { + peeled = Some(gix_hash::ObjectId::from_hex(value.as_bytes())?); + } + b"symref-target" => { + symref_target = Some(value); + } _ => { return Err(Error::UnknownAttribute { attribute: attribute.to_owned().into(), @@ -227,13 +224,53 @@ pub(in crate::handshake::refs) fn parse_v2(line: &BStr) -> Result { } _ => return Err(Error::MalformedV2RefLine(trimmed.to_owned().into())), } - } else { - Ref::Direct { + } + if tokens.next().is_some() { + return Err(Error::MalformedV2RefLine(trimmed.to_owned().into())); + } + Ok(match (symref_target, peeled) { + (Some(target_name), peeled) => match target_name { + b"(null)" => match peeled { + None => Ref::Direct { + full_ref_name: path.into(), + object: id.ok_or(Error::InvariantViolation { + message: "got 'unborn' while (null) was a symref target", + })?, + }, + Some(peeled) => Ref::Peeled { + full_ref_name: path.into(), + object: peeled, + tag: id.ok_or(Error::InvariantViolation { + message: "got 'unborn' while (null) was a symref target", + })?, + }, + }, + name => match id { + Some(id) => Ref::Symbolic { + full_ref_name: path.into(), + tag: peeled.map(|_| id), + object: peeled.unwrap_or(id), + target: name.into(), + }, + None => Ref::Unborn { + full_ref_name: path.into(), + target: name.into(), + }, + }, + }, + (None, Some(peeled)) => Ref::Peeled { + full_ref_name: path.into(), + object: peeled, + tag: id.ok_or(Error::InvariantViolation { + message: "got 'unborn' as tag target", + })?, + }, + (None, None) => Ref::Direct { object: id.ok_or(Error::InvariantViolation { message: "got 'unborn' as object name of direct reference", })?, full_ref_name: path.into(), - } + }, }) } _ => Err(Error::MalformedV2RefLine(trimmed.to_owned().into())), diff --git a/gix-protocol/src/handshake/refs/tests.rs b/gix-protocol/src/handshake/refs/tests.rs index 7d995da5cba..3a6af8d2fbc 100644 --- a/gix-protocol/src/handshake/refs/tests.rs +++ b/gix-protocol/src/handshake/refs/tests.rs @@ -17,6 +17,7 @@ unborn refs/heads/symbolic symref-target:refs/heads/target 808e50d724f604f69ab93c6da2919c014667bedb refs/heads/main 7fe1b98b39423b71e14217aa299a03b7c937d656 refs/tags/foo peeled:808e50d724f604f69ab93c6da2919c014667bedb 7fe1b98b39423b71e14217aa299a03b7c937d6ff refs/tags/blaz +978f927e6397113757dfec6332e7d9c7e356ac25 refs/heads/symbolic symref-target:refs/tags/v1.0 peeled:4d979abcde5cea47b079c38850828956c9382a56 " .as_bytes(), ); @@ -29,6 +30,7 @@ unborn refs/heads/symbolic symref-target:refs/heads/target Ref::Symbolic { full_ref_name: "HEAD".into(), target: "refs/heads/main".into(), + tag: None, object: oid("808e50d724f604f69ab93c6da2919c014667bedb") }, Ref::Direct { @@ -56,8 +58,14 @@ unborn refs/heads/symbolic symref-target:refs/heads/target full_ref_name: "refs/tags/blaz".into(), object: oid("7fe1b98b39423b71e14217aa299a03b7c937d6ff") }, + Ref::Symbolic { + full_ref_name: "refs/heads/symbolic".into(), + target: "refs/tags/v1.0".into(), + tag: Some(oid("978f927e6397113757dfec6332e7d9c7e356ac25")), + object: oid("4d979abcde5cea47b079c38850828956c9382a56") + }, ] - ) + ); } #[maybe_async::test(feature = "blocking-client", async(feature = "async-client", async_std::test))] @@ -86,6 +94,7 @@ dce0ea858eef7ff61ad345cc5cdac62203fb3c10 refs/tags/gix-commitgraph-v0.0.0 Ref::Symbolic { full_ref_name: "HEAD".into(), target: "refs/heads/main".into(), + tag: None, object: oid("73a6868963993a3328e7d8fe94e5a6ac5078a944") }, Ref::Direct { diff --git a/gix-protocol/tests/fetch/v1.rs b/gix-protocol/tests/fetch/v1.rs index 2a113dff77f..f31ea39d7d7 100644 --- a/gix-protocol/tests/fetch/v1.rs +++ b/gix-protocol/tests/fetch/v1.rs @@ -81,6 +81,7 @@ async fn ls_remote() -> crate::Result { handshake::Ref::Symbolic { full_ref_name: "HEAD".into(), object: oid("808e50d724f604f69ab93c6da2919c014667bedb"), + tag: None, target: "refs/heads/master".into() }, handshake::Ref::Direct { diff --git a/gix-protocol/tests/fetch/v2.rs b/gix-protocol/tests/fetch/v2.rs index 351b74e358f..c29e0d08930 100644 --- a/gix-protocol/tests/fetch/v2.rs +++ b/gix-protocol/tests/fetch/v2.rs @@ -81,6 +81,7 @@ async fn ls_remote() -> crate::Result { handshake::Ref::Symbolic { full_ref_name: "HEAD".into(), object: oid("808e50d724f604f69ab93c6da2919c014667bedb"), + tag: None, target: "refs/heads/master".into() }, handshake::Ref::Direct { diff --git a/gix-ref/src/store/file/transaction/commit.rs b/gix-ref/src/store/file/transaction/commit.rs index fbe06cfd6a4..829e45ebb8e 100644 --- a/gix-ref/src/store/file/transaction/commit.rs +++ b/gix-ref/src/store/file/transaction/commit.rs @@ -50,7 +50,8 @@ impl<'s, 'p> Transaction<'s, 'p> { if update_reflog { let log_update = match new { Target::Symbolic(_) => { - // no reflog for symref changes, unless the ref is new and we can obtain a peeled id + // Special HACK: no reflog for symref changes as there is no OID involved which the reflog needs. + // Unless, the ref is new and we can obtain a peeled id // identified by the expectation of what could be there, as is the case when cloning. match expected { PreviousValue::ExistingMustMatch(Target::Peeled(oid)) => { @@ -61,6 +62,8 @@ impl<'s, 'p> Transaction<'s, 'p> { } Target::Peeled(new_oid) => { let previous = match expected { + // Here, this means that the ref already existed, and that it will receive (even transitively) + // the given value PreviousValue::MustExistAndMatch(Target::Peeled(oid)) => Some(oid.to_owned()), _ => None, } diff --git a/gix-ref/src/transaction/mod.rs b/gix-ref/src/transaction/mod.rs index 77ab7349d6d..47d05010807 100644 --- a/gix-ref/src/transaction/mod.rs +++ b/gix-ref/src/transaction/mod.rs @@ -64,7 +64,7 @@ pub enum Change { /// The desired change to the reference log. log: LogChange, /// The expected value already present in the reference. - /// If a ref was existing previously it will be overwritten at `MustExistAndMatch(actual_value)` for use after + /// If a ref was existing previously this field will be overwritten with `MustExistAndMatch(actual_value)` for use after /// the transaction was committed successfully. expected: PreviousValue, /// The new state of the reference, either for updating an existing one or creating a new one. @@ -94,7 +94,6 @@ impl Change { /// Return references to values that are in common between all variants and denote the previous observed value. pub fn previous_value(&self) -> Option> { match self { - // TODO: use or-patterns once MRV is larger than 1.52 (and this is supported) Change::Update { expected: PreviousValue::MustExistAndMatch(previous) | PreviousValue::ExistingMustMatch(previous), .. diff --git a/gix-ref/tests/file/transaction/prepare_and_commit/create_or_update/mod.rs b/gix-ref/tests/file/transaction/prepare_and_commit/create_or_update/mod.rs index ae77d68faeb..b74d4b96761 100644 --- a/gix-ref/tests/file/transaction/prepare_and_commit/create_or_update/mod.rs +++ b/gix-ref/tests/file/transaction/prepare_and_commit/create_or_update/mod.rs @@ -424,7 +424,8 @@ fn symbolic_reference_writes_reflog_if_previous_value_is_set() -> crate::Result ); assert!( head.log_exists(&store), - "reflog is written for new symbolic ref with information about the peeled target id" + "reflog is written for new symbolic ref with information about the peeled target id, \ + as special accommodation for the state during clone to allow us to get a peeled id into the log" ); assert!(store.try_find_loose(referent)?.is_none(), "referent wasn't created"); @@ -499,11 +500,6 @@ fn symbolic_head_missing_referent_then_update_referent() -> crate::Result { mode: RefLog::AndReference, force_create_reflog: false, }; - let log_only = { - let mut l = log.clone(); - l.mode = RefLog::Only; - l - }; let edits = store .transaction() .prepare( @@ -526,7 +522,11 @@ fn symbolic_head_missing_referent_then_update_referent() -> crate::Result { vec![ RefEdit { change: Change::Update { - log: log_only.clone(), + log: { + let mut l = log.clone(); + l.mode = RefLog::Only; + l + }, new: new.clone(), expected: PreviousValue::MustExistAndMatch(new_head_value.clone()), }, @@ -537,7 +537,7 @@ fn symbolic_head_missing_referent_then_update_referent() -> crate::Result { change: Change::Update { log, new: new.clone(), - expected: PreviousValue::Any, + expected: PreviousValue::Any, // there is no previous value, so we can't put `MustExistAndMatch` here. }, name: referent.try_into()?, deref: false, diff --git a/gix-transport/src/client/blocking_io/http/reqwest/remote.rs b/gix-transport/src/client/blocking_io/http/reqwest/remote.rs index 476a82df5d3..b6972072eed 100644 --- a/gix-transport/src/client/blocking_io/http/reqwest/remote.rs +++ b/gix-transport/src/client/blocking_io/http/reqwest/remote.rs @@ -155,6 +155,20 @@ impl Default for Remote { /// utilities impl Remote { + fn restore_thread_after_failure(&mut self) -> http::Error { + let err_that_brought_thread_down = self + .handle + .take() + .expect("thread handle present") + .join() + .expect("handler thread should never panic") + .expect_err("something should have gone wrong with curl (we join on error only)"); + *self = Remote::default(); + http::Error::InitHttpClient { + source: Box::new(err_that_brought_thread_down), + } + } + fn make_request( &mut self, url: &str, @@ -179,14 +193,18 @@ impl Remote { None => continue, }; } - self.request + if self + .request .send(Request { url: url.to_owned(), headers: header_map, upload_body_kind, config: self.config.clone(), }) - .expect("the remote cannot be down at this point"); + .is_err() + { + return Err(self.restore_thread_after_failure()); + } let Response { headers, @@ -195,15 +213,7 @@ impl Remote { } = match self.response.recv() { Ok(res) => res, Err(_) => { - let err = self - .handle - .take() - .expect("always present") - .join() - .expect("no panic") - .expect_err("no receiver means thread is down with init error"); - *self = Self::default(); - return Err(http::Error::InitHttpClient { source: Box::new(err) }); + return Err(self.restore_thread_after_failure()); } }; diff --git a/gix/src/clone/fetch/util.rs b/gix/src/clone/fetch/util.rs index c6899c02d0d..20a3fa99915 100644 --- a/gix/src/clone/fetch/util.rs +++ b/gix/src/clone/fetch/util.rs @@ -76,6 +76,7 @@ pub fn update_head( gix_protocol::handshake::Ref::Symbolic { full_ref_name, target, + tag: _, object, } if full_ref_name == "HEAD" => (Some(object.as_ref()), Some(target)), gix_protocol::handshake::Ref::Direct { full_ref_name, object } if full_ref_name == "HEAD" => { diff --git a/gix/src/reference/log.rs b/gix/src/reference/log.rs index b516e6499eb..2fea1782cc4 100644 --- a/gix/src/reference/log.rs +++ b/gix/src/reference/log.rs @@ -12,6 +12,11 @@ impl<'repo> Reference<'repo> { pub fn log_iter(&self) -> gix_ref::file::log::iter::Platform<'_, '_> { self.inner.log_iter(&self.repo.refs) } + + /// Return true if a reflog is present for this reference. + pub fn log_exists(&self) -> bool { + self.inner.log_exists(&self.repo.refs) + } } /// Generate a message typical for git commit logs based on the given `operation`, commit `message` and `num_parents` of the commit. diff --git a/gix/src/reference/mod.rs b/gix/src/reference/mod.rs index e2ee0d3b2cf..13098739fa4 100644 --- a/gix/src/reference/mod.rs +++ b/gix/src/reference/mod.rs @@ -62,6 +62,7 @@ impl<'repo> Reference<'repo> { } } +/// Peeling impl<'repo> Reference<'repo> { /// Follow all symbolic targets this reference might point to and peel the underlying object /// to the end of the chain, and return it. @@ -81,6 +82,18 @@ impl<'repo> Reference<'repo> { pub fn into_fully_peeled_id(mut self) -> Result, peel::Error> { self.peel_to_id_in_place() } + + /// Follow this symbolic reference one level and return the ref it refers to. + /// + /// Returns `None` if this is not a symbolic reference, hence the leaf of the chain. + pub fn follow(&self) -> Option, gix_ref::file::find::existing::Error>> { + self.inner.follow(&self.repo.refs).map(|res| { + res.map(|r| Reference { + inner: r, + repo: self.repo, + }) + }) + } } mod edits; diff --git a/gix/src/remote/connection/fetch/update_refs/mod.rs b/gix/src/remote/connection/fetch/update_refs/mod.rs index c47f5d74f63..a26f923065f 100644 --- a/gix/src/remote/connection/fetch/update_refs/mod.rs +++ b/gix/src/remote/connection/fetch/update_refs/mod.rs @@ -11,7 +11,10 @@ use crate::{ ext::ObjectIdExt, remote::{ fetch, - fetch::{refs::update::Mode, RefLogMessage, Source}, + fetch::{ + refs::update::{Mode, TypeChange}, + RefLogMessage, Source, + }, }, Repository, }; @@ -23,14 +26,20 @@ pub mod update; #[derive(Debug, Clone, PartialEq, Eq)] pub struct Update { /// The way the update was performed. - pub mode: update::Mode, + pub mode: Mode, + /// If not `None`, the update also affects the type of the reference. This also implies that `edit_index` is not None. + pub type_change: Option, /// The index to the edit that was created from the corresponding mapping, or `None` if there was no local ref. pub edit_index: Option, } -impl From for Update { +impl From for Update { fn from(mode: Mode) -> Self { - Update { mode, edit_index: None } + Update { + mode, + type_change: None, + edit_index: None, + } } } @@ -42,6 +51,14 @@ impl From for Update { /// `action` is the prefix used for reflog entries, and is typically "fetch". /// /// It can be used to produce typical information that one is used to from `git fetch`. +/// +/// We will reject updates only if… +/// +/// * …fast-forward rules are violated +/// * …the local ref is currently checked out +/// * …existing refs would not become 'unborn', i.e. point to a reference that doesn't exist and won't be created due to ref-specs +/// +/// With these safeguards in place, one can handle each naturally and implement mirrors or bare repos easily. #[allow(clippy::too_many_arguments)] pub(crate) fn update( repo: &Repository, @@ -56,6 +73,7 @@ pub(crate) fn update( let _span = gix_trace::detail!("update_refs()", mappings = mappings.len()); let mut edits = Vec::new(); let mut updates = Vec::new(); + let mut edit_indices_to_validate = Vec::new(); let implicit_tag_refspec = fetch_tags .to_refspec() @@ -76,46 +94,56 @@ pub(crate) fn update( }) }, ) { - let remote_id = match remote.as_id() { - Some(id) => id, - None => continue, - }; - if dry_run == fetch::DryRun::No && !repo.objects.contains(remote_id) { - let update = if is_implicit_tag { - update::Mode::ImplicitTagNotSentByRemote.into() - } else { - update::Mode::RejectedSourceObjectNotFound { id: remote_id.into() }.into() - }; - updates.push(update); - continue; + // `None` only if unborn. + let remote_id = remote.as_id(); + if matches!(dry_run, fetch::DryRun::No) && !remote_id.map_or(true, |id| repo.objects.contains(id)) { + if let Some(remote_id) = remote_id.filter(|id| !repo.objects.contains(id)) { + let update = if is_implicit_tag { + Mode::ImplicitTagNotSentByRemote.into() + } else { + Mode::RejectedSourceObjectNotFound { id: remote_id.into() }.into() + }; + updates.push(update); + continue; + } } - let checked_out_branches = worktree_branches(repo)?; - let (mode, edit_index) = match local { + let mut checked_out_branches = worktree_branches(repo)?; + let (mode, edit_index, type_change) = match local { Some(name) => { let (mode, reflog_message, name, previous_value) = match repo.try_find_reference(name)? { Some(existing) => { - if let Some(wt_dir) = checked_out_branches.get(existing.name()) { - let mode = update::Mode::RejectedCurrentlyCheckedOut { - worktree_dir: wt_dir.to_owned(), + if let Some(wt_dirs) = checked_out_branches.get_mut(existing.name()) { + wt_dirs.sort(); + wt_dirs.dedup(); + let mode = Mode::RejectedCurrentlyCheckedOut { + worktree_dirs: wt_dirs.to_owned(), }; updates.push(mode.into()); continue; } - match existing.target() { - TargetRef::Symbolic(_) => { - updates.push(update::Mode::RejectedSymbolic.into()); - continue; - } - TargetRef::Peeled(local_id) => { - let previous_value = - PreviousValue::MustExistAndMatch(Target::Peeled(local_id.to_owned())); + + match existing + .try_id() + .map_or_else(|| existing.clone().peel_to_id_in_place(), Ok) + .map(crate::Id::detach) + { + Ok(local_id) => { + let remote_id = match remote_id { + Some(id) => id, + None => { + // we don't allow to go back to unborn state if there is a local reference already present. + // Note that we will be changing it to a symbolic reference just fine. + updates.push(Mode::RejectedToReplaceWithUnborn.into()); + continue; + } + }; let (mode, reflog_message) = if local_id == remote_id { - (update::Mode::NoChangeNeeded, "no update will be performed") + (Mode::NoChangeNeeded, "no update will be performed") } else if let Some(gix_ref::Category::Tag) = existing.name().category() { if spec.allow_non_fast_forward() { - (update::Mode::Forced, "updating tag") + (Mode::Forced, "updating tag") } else { - updates.push(update::Mode::RejectedTagUpdate.into()); + updates.push(Mode::RejectedTagUpdate.into()); continue; } } else { @@ -129,16 +157,16 @@ pub(crate) fn update( .and_then(|c| { c.committer().map(|a| a.time.seconds).map_err(|_| ()) }).and_then(|local_commit_time| - remote_id - .to_owned() - .ancestors(|id, buf| repo.objects.find_commit_iter(id, buf)) - .sorting( - gix_traverse::commit::Sorting::ByCommitTimeNewestFirstCutoffOlderThan { - seconds: local_commit_time - }, - ) - .map_err(|_| ()) - ); + remote_id + .to_owned() + .ancestors(|id, buf| repo.objects.find_commit_iter(id, buf)) + .sorting( + gix_traverse::commit::Sorting::ByCommitTimeNewestFirstCutoffOlderThan { + seconds: local_commit_time + }, + ) + .map_err(|_| ()) + ); match ancestors { Ok(mut ancestors) => { ancestors.any(|cid| cid.map_or(false, |c| c.id == local_id)) @@ -153,20 +181,41 @@ pub(crate) fn update( }; if is_fast_forward { ( - update::Mode::FastForward, + Mode::FastForward, matches!(dry_run, fetch::DryRun::Yes) .then(|| "fast-forward (guessed in dry-run)") .unwrap_or("fast-forward"), ) } else if force { - (update::Mode::Forced, "forced-update") + (Mode::Forced, "forced-update") } else { - updates.push(update::Mode::RejectedNonFastForward.into()); + updates.push(Mode::RejectedNonFastForward.into()); continue; } }; - (mode, reflog_message, existing.name().to_owned(), previous_value) + ( + mode, + reflog_message, + existing.name().to_owned(), + PreviousValue::MustExistAndMatch(existing.target().into_owned()), + ) } + Err(crate::reference::peel::Error::ToId(gix_ref::peel::to_id::Error::Follow(_))) => { + // An unborn reference, always allow it to be changed to whatever the remote wants. + ( + if existing.target().try_name().map(gix_ref::FullNameRef::as_bstr) + == remote.as_target() + { + Mode::NoChangeNeeded + } else { + Mode::Forced + }, + "change unborn ref", + existing.name().to_owned(), + PreviousValue::MustExistAndMatch(existing.target().into_owned()), + ) + } + Err(err) => return Err(err.into()), } } None => { @@ -177,13 +226,37 @@ pub(crate) fn update( _ => "storing ref", }; ( - update::Mode::New, + Mode::New, reflog_msg, name, - PreviousValue::ExistingMustMatch(Target::Peeled(remote_id.to_owned())), + PreviousValue::ExistingMustMatch(new_value_by_remote(remote, mappings)?), ) } }; + + let new = new_value_by_remote(remote, mappings)?; + let type_change = match (&previous_value, &new) { + ( + PreviousValue::ExistingMustMatch(Target::Peeled(_)) + | PreviousValue::MustExistAndMatch(Target::Peeled(_)), + Target::Symbolic(_), + ) => Some(TypeChange::DirectToSymbolic), + ( + PreviousValue::ExistingMustMatch(Target::Symbolic(_)) + | PreviousValue::MustExistAndMatch(Target::Symbolic(_)), + Target::Peeled(_), + ) => Some(TypeChange::SymbolicToDirect), + _ => None, + }; + // We are here because this edit should work and fast-forward rules are respected. + // But for setting a symref-target, we have to be sure that the target already exists + // or will exists. To be sure all rules are respected, we delay the check to when the + // edit-list has been built. + let edit_index = edits.len(); + if matches!(new, Target::Symbolic(_)) { + let anticipated_update_index = updates.len(); + edit_indices_to_validate.push((anticipated_update_index, edit_index)); + } let edit = RefEdit { change: Change::Update { log: LogChange { @@ -192,38 +265,52 @@ pub(crate) fn update( message: message.compose(reflog_message), }, expected: previous_value, - new: if let Source::Ref(gix_protocol::handshake::Ref::Symbolic { target, .. }) = &remote { - match mappings.iter().find_map(|m| { - m.remote.as_name().and_then(|name| { - (name == target) - .then(|| m.local.as_ref().and_then(|local| local.try_into().ok())) - .flatten() - }) - }) { - Some(local_branch) => { - // This is always safe because… - // - the reference may exist already - // - if it doesn't exist it will be created - we are here because it's in the list of mappings after all - // - if it exists and is updated, and the update is rejected due to non-fastforward for instance, the - // target reference still exists and we can point to it. - Target::Symbolic(local_branch) - } - None => Target::Peeled(remote_id.into()), - } - } else { - Target::Peeled(remote_id.into()) - }, + new, }, name, + // We must not deref symrefs or we will overwrite their destination, which might be checked out + // and we don't check for that case. deref: false, }; - let edit_index = edits.len(); edits.push(edit); - (mode, Some(edit_index)) + (mode, Some(edit_index), type_change) } - None => (update::Mode::NoChangeNeeded, None), + None => (Mode::NoChangeNeeded, None, None), }; - updates.push(Update { mode, edit_index }) + updates.push(Update { + mode, + type_change, + edit_index, + }) + } + + for (update_index, edit_index) in edit_indices_to_validate { + let edit = &edits[edit_index]; + if update_needs_adjustment_as_edits_symbolic_target_is_missing(edit, repo, &edits) { + let edit = &mut edits[edit_index]; + let update = &mut updates[update_index]; + + update.mode = Mode::RejectedToReplaceWithUnborn; + update.type_change = None; + + match edit.change { + Change::Update { + ref expected, + ref mut new, + ref mut log, + .. + } => match expected { + PreviousValue::MustExistAndMatch(existing) => { + *new = existing.clone(); + log.message = "no-op".into(); + } + _ => unreachable!("at this point it can only be one variant"), + }, + Change::Delete { .. } => { + unreachable!("we don't do that here") + } + }; + } } let edits = match dry_run { @@ -258,16 +345,113 @@ pub(crate) fn update( Ok(update::Outcome { edits, updates }) } -fn worktree_branches(repo: &Repository) -> Result, update::Error> { - let mut map = BTreeMap::new(); - if let Some((wt_dir, head_ref)) = repo.work_dir().zip(repo.head_ref().ok().flatten()) { - map.insert(head_ref.inner.name, wt_dir.to_owned()); +/// Figure out if target of `edit` points to a reference that doesn't exist in `repo` and won't exist as it's not in any of `edits`. +/// If so, return true. +fn update_needs_adjustment_as_edits_symbolic_target_is_missing( + edit: &RefEdit, + repo: &Repository, + edits: &[RefEdit], +) -> bool { + match edit.change.new_value().expect("here we need a symlink") { + TargetRef::Peeled(_) => unreachable!("BUG: we already know it's symbolic"), + TargetRef::Symbolic(new_target_ref) => { + match &edit.change { + Change::Update { expected, .. } => match expected { + PreviousValue::MustExistAndMatch(current_target) => { + if let Target::Symbolic(current_target_name) = current_target { + if current_target_name.as_ref() == new_target_ref { + return false; // no-op are always fine + } + let current_is_unborn = repo.refs.try_find(current_target_name).ok().flatten().is_none(); + if current_is_unborn { + return false; + } + } + } + PreviousValue::ExistingMustMatch(_) => return false, // this means the ref doesn't exist locally, so we can create unborn refs anyway + _ => { + unreachable!("BUG: we don't do that here") + } + }, + Change::Delete { .. } => { + unreachable!("we don't ever delete here") + } + }; + let target_ref_exists_locally = repo.refs.try_find(new_target_ref).ok().flatten().is_some(); + if target_ref_exists_locally { + return false; + } + + let target_ref_will_be_created = edits.iter().any(|edit| edit.name.as_ref() == new_target_ref); + !target_ref_will_be_created + } } +} + +fn new_value_by_remote(remote: &Source, mappings: &[fetch::Mapping]) -> Result { + let remote_id = remote.as_id(); + Ok( + if let Source::Ref( + gix_protocol::handshake::Ref::Symbolic { target, .. } | gix_protocol::handshake::Ref::Unborn { target, .. }, + ) = &remote + { + match mappings.iter().find_map(|m| { + m.remote.as_name().and_then(|name| { + (name == target) + .then(|| m.local.as_ref().and_then(|local| local.try_into().ok())) + .flatten() + }) + }) { + // Map the target on the remote to the local branch name, which should be covered by refspecs. + Some(local_branch) => { + // This is always safe because… + // - the reference may exist already + // - if it doesn't exist it will be created - we are here because it's in the list of mappings after all + // - if it exists and is updated, and the update is rejected due to non-fastforward for instance, the + // target reference still exists and we can point to it. + Target::Symbolic(local_branch) + } + None => { + // If we can't map it, it's usually a an unborn branch causing this. + // If not, it means we might create an unborn branch and earlier we made sure that we don't turn + // a perfectly good local branch unto an unborn branch. + // However, we can freely change unborn local branches. + Target::Symbolic(target.try_into()?) + } + } + } else { + Target::Peeled(remote_id.expect("unborn case handled earlier").to_owned()) + }, + ) +} + +fn insert_head( + head: Option>, + out: &mut BTreeMap>, +) -> Result<(), update::Error> { + if let Some((head, wd)) = head.and_then(|head| head.repo.work_dir().map(|wd| (head, wd))) { + out.entry("HEAD".try_into().expect("valid")) + .or_default() + .push(wd.to_owned()); + let mut ref_chain = Vec::new(); + let mut cursor = head.try_into_referent(); + while let Some(ref_) = cursor { + ref_chain.push(ref_.name().to_owned()); + cursor = ref_.follow().transpose()?; + } + for name in ref_chain { + out.entry(name).or_default().push(wd.to_owned()); + } + } + Ok(()) +} + +fn worktree_branches(repo: &Repository) -> Result>, update::Error> { + let mut map = BTreeMap::new(); + insert_head(repo.head().ok(), &mut map)?; for proxy in repo.worktrees()? { let repo = proxy.into_repo_with_possibly_inaccessible_worktree()?; - if let Some((wt_dir, head_ref)) = repo.work_dir().zip(repo.head_ref().ok().flatten()) { - map.insert(head_ref.inner.name, wt_dir.to_owned()); - } + insert_head(repo.head().ok(), &mut map)?; } Ok(map) } diff --git a/gix/src/remote/connection/fetch/update_refs/tests.rs b/gix/src/remote/connection/fetch/update_refs/tests.rs index c9db08a89d3..f0470cb3e0b 100644 --- a/gix/src/remote/connection/fetch/update_refs/tests.rs +++ b/gix/src/remote/connection/fetch/update_refs/tests.rs @@ -31,6 +31,10 @@ mod update { gix_testtools::scripted_fixture_read_only_with_args("make_fetch_repos.sh", [base_repo_path()]).unwrap(); gix::open_opts(dir.join(name), restricted()).unwrap() } + fn named_repo(name: &str) -> gix::Repository { + let dir = gix_testtools::scripted_fixture_read_only("make_remote_repos.sh").unwrap(); + gix::open_opts(dir.join(name), restricted()).unwrap() + } fn repo_rw(name: &str) -> (gix::Repository, gix_testtools::tempfile::TempDir) { let dir = gix_testtools::scripted_fixture_writable_with_args( "make_fetch_repos.sh", @@ -41,8 +45,10 @@ mod update { let repo = gix::open_opts(dir.path().join(name), restricted()).unwrap(); (repo, dir) } - use gix_ref::{transaction::Change, TargetRef}; + use gix_ref::transaction::{LogChange, PreviousValue, RefEdit, RefLog}; + use gix_ref::{transaction::Change, Target, TargetRef}; + use crate::remote::fetch::refs::update::TypeChange; use crate::{ bstr::BString, remote::{ @@ -112,7 +118,7 @@ mod update { ( "+refs/remotes/origin/g:refs/heads/main", fetch::refs::update::Mode::RejectedCurrentlyCheckedOut { - worktree_dir: repo.work_dir().expect("present").to_owned(), + worktree_dirs: vec![repo.work_dir().expect("present").to_owned()], }, None, "checked out branches cannot be written, as it requires a merge of sorts which isn't done here", @@ -148,6 +154,7 @@ mod update { assert_eq!( out.updates, vec![fetch::refs::Update { + type_change: None, mode: expected_mode.clone(), edit_index: reflog_message.map(|_| 0), }], @@ -211,8 +218,9 @@ mod update { out.updates, vec![fetch::refs::Update { mode: fetch::refs::update::Mode::RejectedCurrentlyCheckedOut { - worktree_dir: root.join(path_from_root), + worktree_dirs: vec![root.join(path_from_root)], }, + type_change: None, edit_index: None, }], "{spec}: checked-out checks are done before checking if a change would actually be required (here it isn't)" @@ -223,10 +231,370 @@ mod update { } #[test] - fn local_symbolic_refs_are_never_written() { + fn unborn_remote_branches_can_be_created_locally_if_they_are_new() -> Result { + let repo = named_repo("unborn"); + let (mappings, specs) = mapping_from_spec("HEAD:refs/remotes/origin/HEAD", &repo); + assert_eq!(mappings.len(), 1); + let out = fetch::refs::update( + &repo, + prefixed("action"), + &mappings, + &specs, + &[], + fetch::Tags::None, + fetch::DryRun::Yes, + fetch::WritePackedRefs::Never, + )?; + assert_eq!( + out.updates, + vec![fetch::refs::Update { + mode: fetch::refs::update::Mode::New, + type_change: None, + edit_index: Some(0) + }] + ); + assert_eq!(out.edits.len(), 1, "we are OK with creating unborn refs"); + Ok(()) + } + + #[test] + fn unborn_remote_branches_can_update_local_unborn_branches() -> Result { + let repo = named_repo("unborn"); + let (mappings, specs) = mapping_from_spec("HEAD:refs/heads/existing-unborn-symbolic", &repo); + assert_eq!(mappings.len(), 1); + let out = fetch::refs::update( + &repo, + prefixed("action"), + &mappings, + &specs, + &[], + fetch::Tags::None, + fetch::DryRun::Yes, + fetch::WritePackedRefs::Never, + )?; + assert_eq!( + out.updates, + vec![fetch::refs::Update { + mode: fetch::refs::update::Mode::NoChangeNeeded, + type_change: None, + edit_index: Some(0) + }] + ); + assert_eq!(out.edits.len(), 1, "we are OK with updating unborn refs"); + assert_eq!( + out.edits[0], + RefEdit { + change: Change::Update { + log: LogChange { + mode: RefLog::AndReference, + force_create_reflog: false, + message: "action: change unborn ref".into(), + }, + expected: PreviousValue::MustExistAndMatch(Target::Symbolic( + "refs/heads/main".try_into().expect("valid"), + )), + new: Target::Symbolic("refs/heads/main".try_into().expect("valid")), + }, + name: "refs/heads/existing-unborn-symbolic".try_into().expect("valid"), + deref: false, + } + ); + + let (mappings, specs) = mapping_from_spec("HEAD:refs/heads/existing-unborn-symbolic-other", &repo); + assert_eq!(mappings.len(), 1); + let out = fetch::refs::update( + &repo, + prefixed("action"), + &mappings, + &specs, + &[], + fetch::Tags::None, + fetch::DryRun::Yes, + fetch::WritePackedRefs::Never, + )?; + assert_eq!( + out.updates, + vec![fetch::refs::Update { + mode: fetch::refs::update::Mode::Forced, + type_change: None, + edit_index: Some(0) + }] + ); + assert_eq!( + out.edits.len(), + 1, + "we are OK with creating unborn refs even without actually forcing it" + ); + assert_eq!( + out.edits[0], + RefEdit { + change: Change::Update { + log: LogChange { + mode: RefLog::AndReference, + force_create_reflog: false, + message: "action: change unborn ref".into(), + }, + expected: PreviousValue::MustExistAndMatch(Target::Symbolic( + "refs/heads/other".try_into().expect("valid"), + )), + new: Target::Symbolic("refs/heads/main".try_into().expect("valid")), + }, + name: "refs/heads/existing-unborn-symbolic-other".try_into().expect("valid"), + deref: false, + } + ); + Ok(()) + } + + #[test] + fn remote_symbolic_refs_with_locally_unavailable_target_result_in_inborn_branches() -> Result { + let remote_repo = named_repo("one-commit-with-symref"); + let local_repo = named_repo("unborn"); + let (mappings, specs) = mapping_from_spec("refs/heads/symbolic:refs/heads/new", &remote_repo); + assert_eq!(mappings.len(), 1); + + let out = fetch::refs::update( + &local_repo, + prefixed("action"), + &mappings, + &specs, + &[], + fetch::Tags::None, + fetch::DryRun::Yes, + fetch::WritePackedRefs::Never, + )?; + assert_eq!( + out.updates, + vec![fetch::refs::Update { + mode: fetch::refs::update::Mode::New, + type_change: None, + edit_index: Some(0) + }] + ); + assert_eq!(out.edits.len(), 1); + assert_eq!( + out.edits[0], + RefEdit { + change: Change::Update { + log: LogChange { + mode: RefLog::AndReference, + force_create_reflog: false, + message: "action: storing head".into(), + }, + expected: PreviousValue::ExistingMustMatch(Target::Symbolic( + "refs/heads/branch".try_into().expect("valid"), + )), + new: Target::Symbolic("refs/heads/branch".try_into().expect("valid")), + }, + name: "refs/heads/new".try_into().expect("valid"), + deref: false, + }, + "we create local-refs whose targets aren't present yet, even though the remote knows them.\ + This leaves the caller with assuring all refs are mentioned in mappings." + ); + Ok(()) + } + + #[test] + fn remote_symbolic_refs_with_locally_unavailable_target_dont_overwrite_valid_local_branches() -> Result { + let remote_repo = named_repo("one-commit-with-symref"); + let local_repo = named_repo("one-commit-with-symref-missing-branch"); + let (mappings, specs) = mapping_from_spec("refs/heads/symbolic:refs/heads/valid-locally", &remote_repo); + assert_eq!(mappings.len(), 1); + + let out = fetch::refs::update( + &local_repo, + prefixed("action"), + &mappings, + &specs, + &[], + fetch::Tags::None, + fetch::DryRun::Yes, + fetch::WritePackedRefs::Never, + )?; + assert_eq!( + out.updates, + vec![fetch::refs::Update { + mode: fetch::refs::update::Mode::RejectedToReplaceWithUnborn, + type_change: None, + edit_index: Some(0) + }] + ); + assert_eq!(out.edits.len(), 1); + assert_eq!( + out.edits[0], + RefEdit { + change: Change::Update { + log: LogChange { + mode: RefLog::AndReference, + force_create_reflog: false, + message: "no-op".into(), + }, + expected: PreviousValue::MustExistAndMatch(Target::Peeled(hex_to_id( + "66f16e4e8baf5c77bb6d0484495bebea80e916ce" + ))), + new: Target::Peeled(hex_to_id("66f16e4e8baf5c77bb6d0484495bebea80e916ce")), + }, + name: "refs/heads/valid-locally".try_into().expect("valid"), + deref: false, + }, + "edits are created for logistical reasons (we don't currently update edit-indices) so have to create a no-op" + ); + Ok(()) + } + + #[test] + fn unborn_remote_refs_dont_overwrite_valid_local_refs() -> Result { + let remote_repo = named_repo("unborn"); + let local_repo = named_repo("one-commit-with-symref"); + let (mappings, specs) = + mapping_from_spec("refs/heads/existing-unborn-symbolic:refs/heads/branch", &remote_repo); + assert_eq!(mappings.len(), 1); + + let out = fetch::refs::update( + &local_repo, + prefixed("action"), + &mappings, + &specs, + &[], + fetch::Tags::None, + fetch::DryRun::Yes, + fetch::WritePackedRefs::Never, + )?; + assert_eq!( + out.updates, + vec![fetch::refs::Update { + mode: fetch::refs::update::Mode::RejectedToReplaceWithUnborn, + type_change: None, + edit_index: None + }], + "we don't overwrite locally present refs with unborn ones for safety" + ); + assert_eq!(out.edits.len(), 0); + Ok(()) + } + + #[test] + fn local_symbolic_refs_can_be_overwritten() { let repo = repo("two-origins"); - for source in ["refs/heads/main", "refs/heads/symbolic", "HEAD"] { - let (mappings, specs) = mapping_from_spec(&format!("{source}:refs/heads/symbolic"), &repo); + for (source, destination, expected_update, expected_edit) in [ + ( + // attempt to overwrite HEAD isn't possible as the matching engine will normalize the path. That way, `HEAD` + // can never be set. This is by design (of git) and we follow it. + "refs/heads/symbolic", + "HEAD", + fetch::refs::Update { + mode: fetch::refs::update::Mode::New, + type_change: None, + edit_index: Some(0), + }, + Some(RefEdit { + change: Change::Update { + log: LogChange { + mode: RefLog::AndReference, + force_create_reflog: false, + message: "action: storing head".into(), + }, + expected: PreviousValue::ExistingMustMatch(Target::Symbolic( + "refs/heads/main".try_into().expect("valid"), + )), + new: Target::Symbolic("refs/heads/main".try_into().expect("valid")), + }, + name: "refs/heads/HEAD".try_into().expect("valid"), + deref: false, + }), + ), + ( + // attempt to overwrite checked out branch fails + "refs/remotes/origin/b", // strange, but the remote-refs are simulated and based on local refs + "refs/heads/main", + fetch::refs::Update { + mode: fetch::refs::update::Mode::RejectedCurrentlyCheckedOut { + worktree_dirs: vec![repo.work_dir().expect("present").to_owned()], + }, + type_change: None, + edit_index: None, + }, + None, + ), + ( + // symbolic becomes direct + "refs/heads/main", + "refs/heads/symbolic", + fetch::refs::Update { + mode: fetch::refs::update::Mode::NoChangeNeeded, + type_change: Some(TypeChange::SymbolicToDirect), + edit_index: Some(0), + }, + Some(RefEdit { + change: Change::Update { + log: LogChange { + mode: RefLog::AndReference, + force_create_reflog: false, + message: "action: no update will be performed".into(), + }, + expected: PreviousValue::MustExistAndMatch(Target::Symbolic( + "refs/heads/main".try_into().expect("valid"), + )), + new: Target::Peeled(hex_to_id("f99771fe6a1b535783af3163eba95a927aae21d5")), + }, + name: "refs/heads/symbolic".try_into().expect("valid"), + deref: false, + }), + ), + ( + // direct becomes symbolic + "refs/heads/symbolic", + "refs/remotes/origin/a", + fetch::refs::Update { + mode: fetch::refs::update::Mode::NoChangeNeeded, + type_change: Some(TypeChange::DirectToSymbolic), + edit_index: Some(0), + }, + Some(RefEdit { + change: Change::Update { + log: LogChange { + mode: RefLog::AndReference, + force_create_reflog: false, + message: "action: no update will be performed".into(), + }, + expected: PreviousValue::MustExistAndMatch(Target::Peeled(hex_to_id( + "f99771fe6a1b535783af3163eba95a927aae21d5", + ))), + new: Target::Symbolic("refs/heads/main".try_into().expect("valid")), + }, + name: "refs/remotes/origin/a".try_into().expect("valid"), + deref: false, + }), + ), + ( + // symbolic to symbolic (same) + "refs/heads/symbolic", + "refs/heads/symbolic", + fetch::refs::Update { + mode: fetch::refs::update::Mode::NoChangeNeeded, + type_change: None, + edit_index: Some(0), + }, + Some(RefEdit { + change: Change::Update { + log: LogChange { + mode: RefLog::AndReference, + force_create_reflog: false, + message: "action: no update will be performed".into(), + }, + expected: PreviousValue::MustExistAndMatch(Target::Symbolic( + "refs/heads/main".try_into().expect("valid"), + )), + new: Target::Symbolic("refs/heads/main".try_into().expect("valid")), + }, + name: "refs/heads/symbolic".try_into().expect("valid"), + deref: false, + }), + ), + ] { + let (mappings, specs) = mapping_from_spec(&format!("{source}:{destination}"), &repo); + assert_eq!(mappings.len(), 1); let out = fetch::refs::update( &repo, prefixed("action"), @@ -239,15 +607,11 @@ mod update { ) .unwrap(); - assert_eq!(out.edits.len(), 0); - assert_eq!( - out.updates, - vec![fetch::refs::Update { - mode: fetch::refs::update::Mode::RejectedSymbolic, - edit_index: None - }], - "we don't overwrite these as the checked-out check needs to consider much more than it currently does, we are playing it safe" - ); + assert_eq!(out.edits.len(), usize::from(expected_edit.is_some())); + assert_eq!(out.updates, vec![expected_update]); + if let Some(expected) = expected_edit { + assert_eq!(out.edits, vec![expected]); + } } } @@ -275,17 +639,19 @@ mod update { ) .unwrap(); - assert_eq!(out.edits.len(), 1); + assert_eq!(out.edits.len(), 2, "symbolic refs are handled just like any other ref"); assert_eq!( out.updates, vec![ fetch::refs::Update { mode: fetch::refs::update::Mode::New, + type_change: None, edit_index: Some(0) }, fetch::refs::Update { - mode: fetch::refs::update::Mode::RejectedSymbolic, - edit_index: None + mode: fetch::refs::update::Mode::NoChangeNeeded, + type_change: Some(TypeChange::SymbolicToDirect), + edit_index: Some(1) } ], ); @@ -303,7 +669,7 @@ mod update { } #[test] - fn local_direct_refs_are_never_written_with_symbolic_ones_but_see_only_the_destination() { + fn local_direct_refs_are_written_with_symbolic_ones() { let repo = repo("two-origins"); let (mappings, specs) = mapping_from_spec("refs/heads/symbolic:refs/heads/not-currently-checked-out", &repo); let out = fetch::refs::update( @@ -323,6 +689,7 @@ mod update { out.updates, vec![fetch::refs::Update { mode: fetch::refs::update::Mode::NoChangeNeeded, + type_change: Some(fetch::refs::update::TypeChange::DirectToSymbolic), edit_index: Some(0) }], ); @@ -349,6 +716,7 @@ mod update { out.updates, vec![fetch::refs::Update { mode: fetch::refs::update::Mode::New, + type_change: None, edit_index: Some(0), }], ); @@ -399,10 +767,12 @@ mod update { vec![ fetch::refs::Update { mode: fetch::refs::update::Mode::New, + type_change: None, edit_index: Some(0), }, fetch::refs::Update { mode: fetch::refs::update::Mode::NoChangeNeeded, + type_change: None, edit_index: Some(1), } ], @@ -446,6 +816,7 @@ mod update { out.updates, vec![fetch::refs::Update { mode: fetch::refs::update::Mode::FastForward, + type_change: None, edit_index: Some(0), }], "The caller has to be aware and note that dry-runs can't know about fast-forwards as they don't have remote objects" @@ -480,6 +851,7 @@ mod update { out.updates, vec![fetch::refs::Update { mode: fetch::refs::update::Mode::RejectedNonFastForward, + type_change: None, edit_index: None, }] ); @@ -502,6 +874,7 @@ mod update { out.updates, vec![fetch::refs::Update { mode: fetch::refs::update::Mode::FastForward, + type_change: None, edit_index: Some(0), }] ); @@ -535,6 +908,7 @@ mod update { out.updates, vec![fetch::refs::Update { mode: fetch::refs::update::Mode::FastForward, + type_change: None, edit_index: Some(0), }] ); @@ -548,12 +922,15 @@ mod update { } } - fn mapping_from_spec(spec: &str, repo: &gix::Repository) -> (Vec, Vec) { + fn mapping_from_spec( + spec: &str, + remote_repo: &gix::Repository, + ) -> (Vec, Vec) { let spec = gix_refspec::parse(spec.into(), gix_refspec::parse::Operation::Fetch).unwrap(); let group = gix_refspec::MatchGroup::from_fetch_specs(Some(spec)); - let references = repo.references().unwrap(); + let references = remote_repo.references().unwrap(); let mut references: Vec<_> = references.all().unwrap().map(|r| into_remote_ref(r.unwrap())).collect(); - references.push(into_remote_ref(repo.find_reference("HEAD").unwrap())); + references.push(into_remote_ref(remote_repo.find_reference("HEAD").unwrap())); let mappings = group .match_remotes(references.iter().map(remote_ref_to_item)) .mappings @@ -582,11 +959,14 @@ mod update { }, TargetRef::Symbolic(name) => { let target = name.as_bstr().into(); - let id = r.peel_to_id_in_place().unwrap(); - gix_protocol::handshake::Ref::Symbolic { - full_ref_name, - target, - object: id.detach(), + match r.peel_to_id_in_place() { + Ok(id) => gix_protocol::handshake::Ref::Symbolic { + full_ref_name, + target, + tag: None, + object: id.detach(), + }, + Err(_) => gix_protocol::handshake::Ref::Unborn { full_ref_name, target }, } } } @@ -594,9 +974,10 @@ mod update { fn remote_ref_to_item(r: &gix_protocol::handshake::Ref) -> gix_refspec::match_group::Item<'_> { let (full_ref_name, target, object) = r.unpack(); + static NULL: gix_hash::ObjectId = gix_hash::Kind::Sha1.null(); gix_refspec::match_group::Item { full_ref_name, - target: target.expect("no unborn HEAD"), + target: target.unwrap_or(NULL.as_ref()), object, } } diff --git a/gix/src/remote/connection/fetch/update_refs/update.rs b/gix/src/remote/connection/fetch/update_refs/update.rs index 6eda1ffc07a..7e31effab61 100644 --- a/gix/src/remote/connection/fetch/update_refs/update.rs +++ b/gix/src/remote/connection/fetch/update_refs/update.rs @@ -19,6 +19,10 @@ mod error { OpenWorktreeRepo(#[from] crate::open::Error), #[error("Could not find local commit for fast-forward ancestor check")] FindCommit(#[from] crate::object::find::existing::Error), + #[error("Could not peel symbolic local reference to its ID")] + PeelToId(#[from] crate::reference::peel::Error), + #[error("Failed to follow a symbolic reference to assure worktree isn't affected")] + FollowSymref(#[from] gix_ref::file::find::existing::Error), } } @@ -35,11 +39,14 @@ pub struct Outcome { pub updates: Vec, } -/// Describe the way a ref was updated +/// Describe the way a ref was updated, with particular focus on how the (peeled) target commit was affected. +/// +/// Note that for all the variants that signal a change or `NoChangeNeeded` it's additionally possible to change the target type +/// from symbolic to direct, or the other way around. #[derive(Debug, Clone, PartialEq, Eq)] pub enum Mode { /// No change was attempted as the remote ref didn't change compared to the current ref, or because no remote ref was specified - /// in the ref-spec. + /// in the ref-spec. Note that the expected value is still asserted to uncover potential race conditions with other processes. NoChangeNeeded, /// The old ref's commit was an ancestor of the new one, allowing for a fast-forward without a merge. FastForward, @@ -62,14 +69,19 @@ pub enum Mode { RejectedTagUpdate, /// The reference update would not have been a fast-forward, and force is not specified in the ref-spec. RejectedNonFastForward, - /// The update of a local symbolic reference was rejected. - RejectedSymbolic, + /// The remote has an unborn symbolic reference where we have one that is set. This means the remote + /// has reset itself to a newly initialized state or a state that is highly unusual. + /// It may also mean that the remote knows the target name, but it's not available locally and not included in the ref-mappings + /// to be created, so we would effectively change a valid local ref into one that seems unborn, which is rejected. + /// Note that this mode may have an associated ref-edit that is a no-op, or current-state assertion, for logistical reasons only + /// and having no edit would be preferred. + RejectedToReplaceWithUnborn, /// The update was rejected because the branch is checked out in the given worktree_dir. /// /// Note that the check applies to any known worktree, whether it's present on disk or not. RejectedCurrentlyCheckedOut { - /// The path to the worktree directory where the branch is checked out. - worktree_dir: PathBuf, + /// The path(s) to the worktree directory where the branch is checked out. + worktree_dirs: Vec, }, } @@ -84,12 +96,16 @@ impl std::fmt::Display for Mode { Mode::RejectedSourceObjectNotFound { id } => return write!(f, "rejected ({id} not found)"), Mode::RejectedTagUpdate => "rejected (would overwrite existing tag)", Mode::RejectedNonFastForward => "rejected (non-fast-forward)", - Mode::RejectedSymbolic => "rejected (refusing to write symbolic refs)", - Mode::RejectedCurrentlyCheckedOut { worktree_dir } => { + Mode::RejectedToReplaceWithUnborn => "rejected (refusing to overwrite existing with unborn ref)", + Mode::RejectedCurrentlyCheckedOut { worktree_dirs } => { return write!( f, "rejected (cannot write into checked-out branch at \"{}\")", - worktree_dir.display() + worktree_dirs + .iter() + .filter_map(|d| d.to_str()) + .collect::>() + .join(", ") ) } } @@ -97,6 +113,15 @@ impl std::fmt::Display for Mode { } } +/// Indicates that a ref changes its type. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Ord, PartialOrd, Hash)] +pub enum TypeChange { + /// A local direct reference is changed into a symbolic one. + DirectToSymbolic, + /// A local symbolic reference is changed into a direct one. + SymbolicToDirect, +} + impl Outcome { /// Produce an iterator over all information used to produce the this outcome, ref-update by ref-update, using the `mappings` /// used when producing the ref update. diff --git a/gix/src/remote/fetch.rs b/gix/src/remote/fetch.rs index eb40403e5c3..54eaea7ec6a 100644 --- a/gix/src/remote/fetch.rs +++ b/gix/src/remote/fetch.rs @@ -154,6 +154,18 @@ impl Source { } } + /// Return the target that this symbolic ref is pointing to, or `None` if it is no symbolic ref. + pub fn as_target(&self) -> Option<&crate::bstr::BStr> { + match self { + Source::ObjectId(_) => None, + Source::Ref(r) => match r { + gix_protocol::handshake::Ref::Peeled { .. } | gix_protocol::handshake::Ref::Direct { .. } => None, + gix_protocol::handshake::Ref::Symbolic { target, .. } + | gix_protocol::handshake::Ref::Unborn { target, .. } => Some(target.as_ref()), + }, + } + } + /// Returns the peeled id of this instance, that is the object that can't be de-referenced anymore. pub fn peeled_id(&self) -> Option<&gix_hash::oid> { match self { diff --git a/gix/tests/clone/mod.rs b/gix/tests/clone/mod.rs index a8af782de4b..fa49df94712 100644 --- a/gix/tests/clone/mod.rs +++ b/gix/tests/clone/mod.rs @@ -348,8 +348,22 @@ mod blocking_io { .expect("computable") .1 .starts_with_str(remote_name)); - let mut logs = r.log_iter(); - assert_reflog(logs.all()); + match r.target() { + TargetRef::Peeled(_) => { + let mut logs = r.log_iter(); + assert_reflog(logs.all()); + } + TargetRef::Symbolic(_) => { + // TODO: it *should* be possible to set the reflog here based on the referent if deref = true + // when setting up the edits. But it doesn't seem to work. Also, some tests are + // missing for `leaf_referent_previous_oid`. + assert!( + !r.log_exists(), + "symbolic refs don't have object ids, so they can't get \ + into the reflog as these need previous and new oid" + ) + } + } } } let mut out_of_graph_tags = Vec::new(); diff --git a/gix/tests/fixtures/make_remote_repos.sh b/gix/tests/fixtures/make_remote_repos.sh index 76cd2e4eb8a..5bcdbe9e069 100644 --- a/gix/tests/fixtures/make_remote_repos.sh +++ b/gix/tests/fixtures/make_remote_repos.sh @@ -350,3 +350,22 @@ function optimize_repo() { optimize_repo ) ) + +git init unborn +(cd unborn + git symbolic-ref refs/heads/existing-unborn-symbolic refs/heads/main + git symbolic-ref refs/heads/existing-unborn-symbolic-other refs/heads/other +) + +git init one-commit-with-symref +(cd one-commit-with-symref + touch content && git add content && git commit -m "init" + git checkout -b branch + git symbolic-ref refs/heads/symbolic refs/heads/branch + git checkout main +) + +git clone one-commit-with-symref one-commit-with-symref-missing-branch +(cd one-commit-with-symref-missing-branch + git branch valid-locally +) diff --git a/gix/tests/reference/mod.rs b/gix/tests/reference/mod.rs index 950239b2ac1..c4a4821f94c 100644 --- a/gix/tests/reference/mod.rs +++ b/gix/tests/reference/mod.rs @@ -23,7 +23,7 @@ mod find { use std::convert::TryInto; use gix_ref as refs; - use gix_ref::FullNameRef; + use gix_ref::{FullName, FullNameRef, Target}; use crate::util::hex_to_id; @@ -64,6 +64,25 @@ mod find { assert_eq!(symbolic_ref.into_fully_peeled_id()?, the_commit, "idempotency"); Ok(()) } + + #[test] + fn and_follow() -> crate::Result { + let repo = repo()?; + let mut symbolic_ref = repo.find_reference("multi-link-target1")?; + let first_hop = Target::Symbolic(FullName::try_from("refs/tags/multi-link-target2").expect("valid")); + assert_eq!(symbolic_ref.target(), first_hop.to_ref()); + + let second_hop = Target::Symbolic(FullName::try_from("refs/remotes/origin/multi-link-target3").expect("valid")); + symbolic_ref = symbolic_ref.follow().expect("another hop")?; + assert_eq!(symbolic_ref.target(), second_hop.to_ref()); + + let last_hop = Target::Peeled(hex_to_id("134385f6d781b7e97062102c6a483440bfda2a03")); + symbolic_ref = symbolic_ref.follow().expect("another hop")?; + assert_eq!(symbolic_ref.target(), last_hop.to_ref()); + + assert!(symbolic_ref.follow().is_none(), "direct references can't be followed"); + Ok(()) + } } #[test] diff --git a/gix/tests/remote/fetch.rs b/gix/tests/remote/fetch.rs index 9c8d50719be..2452ea08808 100644 --- a/gix/tests/remote/fetch.rs +++ b/gix/tests/remote/fetch.rs @@ -559,10 +559,23 @@ mod blocking_and_async_io { ); let edit = &update_refs.edits[1]; assert_eq!(edit.name.as_bstr(), "refs/remotes/changes-on-top-of-origin/symbolic"); - assert!( - edit.change.new_value().expect("no deletion").try_id().is_some(), - "on the remote this is a symbolic ref, we just write its destination object id though" - ); + match version.unwrap_or_default() { + gix::protocol::transport::Protocol::V2 => { + assert!( + edit.change.new_value().expect("no deletion").try_name().is_some(), + "{version:?} on the remote this is a symbolic ref, and we maintain it as symref information is kept" + ); + } + gix::protocol::transport::Protocol::V1 => { + assert!( + edit.change.new_value().expect("no deletion").try_id().is_some(), + "on the remote this is a symbolic ref, but in V1 symrefs are not visible" + ); + } + gix::protocol::transport::Protocol::V0 => { + unreachable!("we don't test this here as there is no need") + } + } assert!( !write_pack_bundle.keep_path.map_or(false, |f| f.is_file()), @@ -589,10 +602,12 @@ mod blocking_and_async_io { vec![ fetch::refs::Update { mode: fetch::refs::update::Mode::New, + type_change: None, edit_index: Some(0), }, fetch::refs::Update { mode: fetch::refs::update::Mode::New, + type_change: None, edit_index: Some(1), } ] @@ -604,21 +619,32 @@ mod blocking_and_async_io { ) { let edit = edit.expect("refedit present even if it's a no-op"); if dry_run { - assert_eq!( - edit.change.new_value().expect("no deletions").id(), - mapping.remote.as_id().expect("no unborn") - ); + match edit.change.new_value().expect("no deletions") { + gix_ref::TargetRef::Peeled(id) => { + assert_eq!(id, mapping.remote.as_id().expect("no unborn")) + } + gix_ref::TargetRef::Symbolic(target) => { + assert_eq!(target.as_bstr(), mapping.remote.as_target().expect("no direct ref")) + } + } assert!( repo.try_find_reference(edit.name.as_ref())?.is_none(), "no ref created in dry-run mode" ); } else { let r = repo.find_reference(edit.name.as_ref()).unwrap(); - assert_eq!( - r.id(), - *mapping.remote.as_id().expect("no unborn"), - "local reference should point to remote id" - ); + match r.target() { + gix_ref::TargetRef::Peeled(id) => { + assert_eq!( + id, + mapping.remote.as_id().expect("no unborn"), + "local reference should point to remote id" + ); + } + gix_ref::TargetRef::Symbolic(target) => { + assert_eq!(target.as_bstr(), mapping.remote.as_target().expect("no direct ref")) + } + } } } }