Merge pull request #2238 from GitoxideLabs/copilot/update-refspec-parsing-logic

Allow complex glob patterns in one-sided refspecs with full wildmatch support

Author: Byron

Cargo.lock

@@ -18,10 +18,12 @@ 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"
 smallvec = "1.15.1"
 
 [dev-dependencies]
 gix-testtools = { path = "../tests/tools" }
+insta = "1.43.2"

gix-refspec/Cargo.toml

@@ -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<Needle<'a>>,
     pub(crate) rhs: Option<Needle<'a>>,
@@ -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<RefSpecRef<'a>> 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'\\')
 }

gix-refspec/src/match_group/util.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)

gix-refspec/src/parse.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<ObjectId>,
+    }
+
+    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(),
+            }
+        }
+    }
+}

gix-refspec/tests/refspec/match_group.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 { .. }
+        ));
+    }
+}