diff --git a/Cargo.lock b/Cargo.lock index 82468ee5370..b565828fd95 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2252,10 +2252,12 @@ name = "gix-refspec" version = "0.32.0" dependencies = [ "bstr", + "gix-glob", "gix-hash", "gix-revision", "gix-testtools", "gix-validate", + "insta", "smallvec", "thiserror 2.0.17", ] diff --git a/gix-refspec/Cargo.toml b/gix-refspec/Cargo.toml index 2c60ce1bd93..c7513cc2c50 100644 --- a/gix-refspec/Cargo.toml +++ b/gix-refspec/Cargo.toml @@ -18,6 +18,7 @@ doctest = false gix-revision = { version = "^0.36.0", path = "../gix-revision", default-features = false } gix-validate = { version = "^0.10.1", path = "../gix-validate" } gix-hash = { version = "^0.20.0", path = "../gix-hash" } +gix-glob = { version = "^0.22.1", path = "../gix-glob" } bstr = { version = "1.12.0", default-features = false, features = ["std"] } thiserror = "2.0.17" @@ -25,3 +26,4 @@ smallvec = "1.15.1" [dev-dependencies] gix-testtools = { path = "../tests/tools" } +insta = "1.43.2" \ No newline at end of file diff --git a/gix-refspec/src/match_group/util.rs b/gix-refspec/src/match_group/util.rs index c4012717e57..532b04630fd 100644 --- a/gix-refspec/src/match_group/util.rs +++ b/gix-refspec/src/match_group/util.rs @@ -6,6 +6,7 @@ use gix_hash::ObjectId; use crate::{match_group::Item, RefSpecRef}; /// A type keeping enough information about a ref-spec to be able to efficiently match it against multiple matcher items. +#[derive(Debug)] pub struct Matcher<'a> { pub(crate) lhs: Option>, pub(crate) rhs: Option>, @@ -46,6 +47,7 @@ pub(crate) enum Needle<'a> { FullName(&'a BStr), PartialName(&'a BStr), Glob { name: &'a BStr, asterisk_pos: usize }, + Pattern(&'a BStr), Object(ObjectId), } @@ -102,6 +104,17 @@ impl<'a> Needle<'a> { let end = item.full_ref_name.len() - tail.len(); Match::GlobRange(*asterisk_pos..end) } + Needle::Pattern(pattern) => { + if gix_glob::wildmatch( + pattern, + item.full_ref_name, + gix_glob::wildmatch::Mode::NO_MATCH_SLASH_LITERAL, + ) { + Match::Normal + } else { + Match::None + } + } Needle::Object(id) => { if *id == item.target { return Match::Normal; @@ -137,7 +150,11 @@ impl<'a> Needle<'a> { name.insert_str(0, "refs/heads/"); Cow::Owned(name.into()) } + (Needle::Pattern(name), None) => Cow::Borrowed(name), (Needle::Glob { .. }, None) => unreachable!("BUG: no range provided for glob pattern"), + (Needle::Pattern(_), Some(_)) => { + unreachable!("BUG: range provided for pattern, but patterns don't use ranges") + } (_, Some(_)) => { unreachable!("BUG: range provided even though needle wasn't a glob. Globs are symmetric.") } @@ -168,9 +185,28 @@ impl<'a> From<&'a BStr> for Needle<'a> { impl<'a> From> for Matcher<'a> { fn from(v: RefSpecRef<'a>) -> Self { - Matcher { + let mut m = Matcher { lhs: v.src.map(Into::into), rhs: v.dst.map(Into::into), + }; + if m.rhs.is_none() { + if let Some(src) = v.src { + if must_use_pattern_matching(src) { + m.lhs = Some(Needle::Pattern(src)); + } + } } + m + } +} + +/// Check if a pattern is complex enough to require wildmatch instead of simple glob matching +fn must_use_pattern_matching(pattern: &BStr) -> bool { + let asterisk_count = pattern.iter().filter(|&&b| b == b'*').count(); + if asterisk_count > 1 { + return true; } + pattern + .iter() + .any(|&b| b == b'?' || b == b'[' || b == b']' || b == b'\\') } diff --git a/gix-refspec/src/parse.rs b/gix-refspec/src/parse.rs index 398d93294ad..6844d2f7ecb 100644 --- a/gix-refspec/src/parse.rs +++ b/gix-refspec/src/parse.rs @@ -116,9 +116,11 @@ pub(crate) mod function { *spec = "HEAD".into(); } } - let (src, src_had_pattern) = validated(src, operation == Operation::Push && dst.is_some())?; - let (dst, dst_had_pattern) = validated(dst, false)?; - if mode != Mode::Negative && src_had_pattern != dst_had_pattern { + let is_one_sided = dst.is_none(); + let (src, src_had_pattern) = validated(src, operation == Operation::Push && dst.is_some(), is_one_sided)?; + let (dst, dst_had_pattern) = validated(dst, false, false)?; + // For one-sided refspecs, we don't need to check for pattern balance + if !is_one_sided && mode != Mode::Negative && src_had_pattern != dst_had_pattern { return Err(Error::PatternUnbalanced); } @@ -149,20 +151,31 @@ pub(crate) mod function { spec.len() >= gix_hash::Kind::shortest().len_in_hex() && spec.iter().all(u8::is_ascii_hexdigit) } - fn validated(spec: Option<&BStr>, allow_revspecs: bool) -> Result<(Option<&BStr>, bool), Error> { + fn validated( + spec: Option<&BStr>, + allow_revspecs: bool, + is_one_sided: bool, + ) -> Result<(Option<&BStr>, bool), Error> { match spec { Some(spec) => { let glob_count = spec.iter().filter(|b| **b == b'*').take(2).count(); if glob_count > 1 { - return Err(Error::PatternUnsupported { pattern: spec.into() }); + // For one-sided refspecs, allow any number of globs without validation + if !is_one_sided { + return Err(Error::PatternUnsupported { pattern: spec.into() }); + } } - let has_globs = glob_count == 1; + // Check if there are any globs (one or more asterisks) + let has_globs = glob_count > 0; if has_globs { - let mut buf = smallvec::SmallVec::<[u8; 256]>::with_capacity(spec.len()); - buf.extend_from_slice(spec); - let glob_pos = buf.find_byte(b'*').expect("glob present"); - buf[glob_pos] = b'a'; - gix_validate::reference::name_partial(buf.as_bstr())?; + // For one-sided refspecs, skip validation of glob patterns + if !is_one_sided { + let mut buf = smallvec::SmallVec::<[u8; 256]>::with_capacity(spec.len()); + buf.extend_from_slice(spec); + let glob_pos = buf.find_byte(b'*').expect("glob present"); + buf[glob_pos] = b'a'; + gix_validate::reference::name_partial(buf.as_bstr())?; + } } else { gix_validate::reference::name_partial(spec) .map_err(Error::from) diff --git a/gix-refspec/tests/refspec/match_group.rs b/gix-refspec/tests/refspec/match_group.rs index 42cece6799f..032dc360338 100644 --- a/gix-refspec/tests/refspec/match_group.rs +++ b/gix-refspec/tests/refspec/match_group.rs @@ -184,3 +184,160 @@ mod multiple { ); } } + +mod complex_globs { + use bstr::BString; + use gix_hash::ObjectId; + use gix_refspec::{parse::Operation, MatchGroup}; + + #[test] + fn one_sided_complex_glob_patterns_can_be_parsed() { + // The key change is that complex glob patterns with multiple asterisks + // can now be parsed for one-sided refspecs + let spec = gix_refspec::parse("refs/*/foo/*".into(), Operation::Fetch); + assert!(spec.is_ok(), "Should parse complex glob pattern for one-sided refspec"); + + let spec = gix_refspec::parse("refs/*/*/bar".into(), Operation::Fetch); + assert!( + spec.is_ok(), + "Should parse complex glob pattern with multiple asterisks" + ); + + let spec = gix_refspec::parse("refs/heads/[a-z.]/release/*".into(), Operation::Fetch); + assert!(spec.is_ok(), "Should parse complex glob pattern"); + + // Two-sided refspecs with multiple asterisks should still fail + let spec = gix_refspec::parse("refs/*/foo/*:refs/remotes/*".into(), Operation::Fetch); + assert!(spec.is_err(), "Two-sided refspecs with multiple asterisks should fail"); + } + + #[test] + fn one_sided_simple_glob_patterns_match() { + // Test that simple glob patterns (one asterisk) work correctly with matching + let refs = [ + new_ref("refs/heads/feature/foo", "1111111111111111111111111111111111111111"), + new_ref("refs/heads/bugfix/bar", "2222222222222222222222222222222222222222"), + new_ref("refs/tags/v1.0", "3333333333333333333333333333333333333333"), + new_ref("refs/pull/123", "4444444444444444444444444444444444444444"), + ]; + let items: Vec<_> = refs.iter().map(|r| r.to_item()).collect(); + + // Test: refs/heads/* should match all refs under refs/heads/ + let spec = gix_refspec::parse("refs/heads/*".into(), Operation::Fetch).unwrap(); + let group = MatchGroup::from_fetch_specs([spec]); + let outcome = group.match_lhs(items.iter().copied()); + + insta::assert_debug_snapshot!(outcome.mappings, @r#" + [ + Mapping { + item_index: Some( + 0, + ), + lhs: FullName( + "refs/heads/feature/foo", + ), + rhs: None, + spec_index: 0, + }, + Mapping { + item_index: Some( + 1, + ), + lhs: FullName( + "refs/heads/bugfix/bar", + ), + rhs: None, + spec_index: 0, + }, + ] + "#); + + // Test: refs/tags/* should match all refs under refs/tags/ + let items: Vec<_> = refs.iter().map(|r| r.to_item()).collect(); + let spec = gix_refspec::parse("refs/tags/v[0-9]*".into(), Operation::Fetch).unwrap(); + let group = MatchGroup::from_fetch_specs([spec]); + let outcome = group.match_lhs(items.iter().copied()); + + insta::assert_debug_snapshot!(outcome.mappings, @r#" + [ + Mapping { + item_index: Some( + 2, + ), + lhs: FullName( + "refs/tags/v1.0", + ), + rhs: None, + spec_index: 0, + }, + ] + "#); + } + + #[test] + fn one_sided_glob_with_suffix_matches() { + // Test that glob patterns with suffix work correctly + let refs = [ + new_ref("refs/heads/feature", "1111111111111111111111111111111111111111"), + new_ref("refs/heads/feat", "2222222222222222222222222222222222222222"), + new_ref("refs/heads/main", "3333333333333333333333333333333333333333"), + ]; + let items: Vec<_> = refs.iter().map(|r| r.to_item()).collect(); + + // Test: refs/heads/feat* should match refs/heads/feature and refs/heads/feat + let spec = gix_refspec::parse("refs/heads/feat*".into(), Operation::Fetch).unwrap(); + let group = MatchGroup::from_fetch_specs([spec]); + let outcome = group.match_lhs(items.iter().copied()); + let mappings = outcome.mappings; + + insta::assert_debug_snapshot!(mappings, @r#" + [ + Mapping { + item_index: Some( + 0, + ), + lhs: FullName( + "refs/heads/feature", + ), + rhs: None, + spec_index: 0, + }, + Mapping { + item_index: Some( + 1, + ), + lhs: FullName( + "refs/heads/feat", + ), + rhs: None, + spec_index: 0, + }, + ] + "#); + } + + fn new_ref(name: &str, id_hex: &str) -> Ref { + Ref { + name: name.into(), + target: ObjectId::from_hex(id_hex.as_bytes()).unwrap(), + object: None, + } + } + + #[derive(Debug, Clone)] + struct Ref { + name: BString, + target: ObjectId, + object: Option, + } + + impl Ref { + fn to_item(&self) -> gix_refspec::match_group::Item<'_> { + gix_refspec::match_group::Item { + full_ref_name: self.name.as_ref(), + target: &self.target, + object: self.object.as_deref(), + } + } + } +} diff --git a/gix-refspec/tests/refspec/parse/fetch.rs b/gix-refspec/tests/refspec/parse/fetch.rs index c10a47801b2..c3e500d780a 100644 --- a/gix-refspec/tests/refspec/parse/fetch.rs +++ b/gix-refspec/tests/refspec/parse/fetch.rs @@ -174,3 +174,42 @@ fn ampersand_on_left_hand_side_is_head() { fn empty_refspec_is_enough_for_fetching_head_into_fetchhead() { assert_parse("", Instruction::Fetch(Fetch::Only { src: b("HEAD") })); } + +#[test] +fn complex_glob_patterns_are_allowed_in_one_sided_refspecs() { + // Complex patterns with multiple asterisks should work for one-sided refspecs + assert_parse( + "refs/*/foo/*", + Instruction::Fetch(Fetch::Only { src: b("refs/*/foo/*") }), + ); + + assert_parse( + "+refs/heads/*/release/*", + Instruction::Fetch(Fetch::Only { + src: b("refs/heads/*/release/*"), + }), + ); + + // Even more complex patterns + assert_parse( + "refs/*/*/branch", + Instruction::Fetch(Fetch::Only { + src: b("refs/*/*/branch"), + }), + ); +} + +#[test] +fn complex_glob_patterns_still_fail_for_two_sided_refspecs() { + // Two-sided refspecs with complex patterns (multiple asterisks) should still fail + for spec in [ + "refs/*/foo/*:refs/remotes/origin/*", + "refs/*/*:refs/remotes/*", + "a/*/c/*:b/*", + ] { + assert!(matches!( + try_parse(spec, Operation::Fetch).unwrap_err(), + Error::PatternUnsupported { .. } + )); + } +} diff --git a/gix-refspec/tests/refspec/parse/invalid.rs b/gix-refspec/tests/refspec/parse/invalid.rs index db591d2e0b3..54902422378 100644 --- a/gix-refspec/tests/refspec/parse/invalid.rs +++ b/gix-refspec/tests/refspec/parse/invalid.rs @@ -27,24 +27,33 @@ fn whitespace() { #[test] fn complex_patterns_with_more_than_one_asterisk() { + // For one-sided refspecs, complex patterns are now allowed for op in [Operation::Fetch, Operation::Push] { - for spec in ["a/*/c/*", "a**:**b", "+:**/"] { + assert!(try_parse("a/*/c/*", op).is_ok()); + } + + // For two-sided refspecs, complex patterns should still fail + for op in [Operation::Fetch, Operation::Push] { + for spec in ["a/*/c/*:x/*/y/*", "a**:**b", "+:**/"] { assert!(matches!( try_parse(spec, op).unwrap_err(), Error::PatternUnsupported { .. } )); } } + + // Negative specs with multiple patterns still fail assert!(matches!( try_parse("^*/*", Operation::Fetch).unwrap_err(), - Error::PatternUnsupported { .. } + Error::NegativeGlobPattern )); } #[test] fn both_sides_need_pattern_if_one_uses_it() { + // For two-sided refspecs, both sides still need patterns if one uses it for op in [Operation::Fetch, Operation::Push] { - for spec in ["refs/*/a", ":a/*", "+:a/*", "a*:b/c", "a:b/*"] { + for spec in [":a/*", "+:a/*", "a*:b/c", "a:b/*"] { assert!( matches!(try_parse(spec, op).unwrap_err(), Error::PatternUnbalanced), "{}", @@ -52,6 +61,11 @@ fn both_sides_need_pattern_if_one_uses_it() { ); } } + + // One-sided refspecs with patterns are now allowed + for op in [Operation::Fetch, Operation::Push] { + assert!(try_parse("refs/*/a", op).is_ok()); + } } #[test] diff --git a/gix-refspec/tests/refspec/parse/mod.rs b/gix-refspec/tests/refspec/parse/mod.rs index 0e0cf77d56f..61a4a2803a4 100644 --- a/gix-refspec/tests/refspec/parse/mod.rs +++ b/gix-refspec/tests/refspec/parse/mod.rs @@ -45,6 +45,10 @@ fn baseline() { ), true, ) => {} // we prefer failing fast, git let's it pass + // We now allow complex glob patterns in one-sided refspecs + (None, false) if is_one_sided_glob_pattern(spec, op) => { + // This is an intentional behavior change: we allow complex globs in one-sided refspecs + } _ => { eprintln!("{err_code} {res:?} {} {:?}", kind.as_bstr(), spec.as_bstr()); mismatch += 1; @@ -66,6 +70,15 @@ fn baseline() { panics ); } + + fn is_one_sided_glob_pattern(spec: &[u8], op: Operation) -> bool { + use bstr::ByteSlice; + matches!(op, Operation::Fetch) + && spec + .to_str() + .map(|s| s.contains('*') && !s.contains(':')) + .unwrap_or(false) + } } #[test] diff --git a/gix/tests/gix/config/tree.rs b/gix/tests/gix/config/tree.rs index 686f26f461d..22190235f94 100644 --- a/gix/tests/gix/config/tree.rs +++ b/gix/tests/gix/config/tree.rs @@ -1125,17 +1125,17 @@ mod remote { assert_eq!( Remote::FETCH - .try_into_refspec(bcow("*/*/*"), gix_refspec::parse::Operation::Fetch) + .try_into_refspec(bcow("*/*/*:refs/heads/*"), gix_refspec::parse::Operation::Fetch) .unwrap_err() .to_string(), - "The refspec at \"remote..fetch=*/*/*\" could not be parsed" + "The refspec at \"remote..fetch=*/*/*:refs/heads/*\" could not be parsed" ); assert_eq!( Remote::PUSH - .try_into_refspec(bcow("*/*/*"), gix_refspec::parse::Operation::Push) + .try_into_refspec(bcow("*/*/*:refs/heads/*"), gix_refspec::parse::Operation::Push) .unwrap_err() .to_string(), - "The refspec at \"remote..push=*/*/*\" could not be parsed" + "The refspec at \"remote..push=*/*/*:refs/heads/*\" could not be parsed" ); } }