Refactor glob pattern handling to use globwalk crate and remove custom parsing logic

[sourcery-ai]

The current implementation includes a significant amount of custom code for glob pattern parsing, validation, and normalization (e.g., normalize_separators, BraceValidator, process_glob_entry, and related error handling). This custom logic could be greatly simplified and made more robust by replacing it with the globwalk crate, which natively supports features such as brace expansion, escape handling, platform-native separators, and file-only filtering.

Suggested steps for refactoring:

1. Add globwalk = "0.8" to Cargo.toml.
2. Replace the custom parsing/validation/normalization logic with a function using GlobWalkerBuilder from globwalk, as shown below:

   use globwalk::{GlobWalkerBuilder, WalkError};
   use minijinja::{Error, ErrorKind};
   
   pub(crate) fn glob_paths(pattern: &str) -> Result<Vec<String>, Error> {
       let walker = GlobWalkerBuilder::new(".", pattern)
           .case_insensitive(false)
           .require_literal_separator(true)
           .require_literal_leading_dot(false)
           .build()
           .map_err(|e| Error::new(ErrorKind::InvalidPattern, e.to_string()))?;
   
       let paths = walker
           .filter_map(|r| r.ok())
           .filter(|e| e.file_type().is_file())
           .map(|e| e.path().to_string_lossy().replace("\\", "/"))
           .collect();
   
       Ok(paths)
   }

3. Remove all custom glob parsing and validation code, including:
   • CharContext, BraceValidator, BraceValidationState
   • normalize_separators, force_literal_escapes, process_backslash
   • process_glob_entry, fetch_metadata, open_root_dir, and custom error plumbing

Benefits:

• Maintains all current features (brace expansion, escape handling, separator normalization, file filtering)
• Reduces code complexity by ~500 lines
• Leverages a well-tested external crate

Action items:

• Refactor glob pattern handling to use globwalk as described
• Remove obsolete custom parsing/validation code
• Ensure all existing tests pass and add new ones if necessary to cover edge cases

This change will simplify maintenance and improve reliability.

---

I created this issue for @leynos from #168 (comment).

Tips and commands

Getting Help

• Contact our support team for questions or feedback.
• Visit our documentation for detailed guides and information.
• Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

[leynos]

@coderabbitai looking at the codebase, is this issue still relevant? If the issue has been fixed in the code, does the documentation reflect this?

If it is still relevant, please provide an assessment of the issue criticality (critical, high, medium or low), based on risk of the issue impacting the user and the anticipated blast radius.

Once a decision has been made, apply the appropriate label.

If the issue is documentation ONLY (i.e., no code changes needed), apply the documentation label in addition to the appropriate criticality label.

Please also provide an impact of the required changes.

[coderabbitai]

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Fetch details of issue #171
gh issue view 171 --json title,body,labels,state,author

Length of output: 2949

---

🏁 Script executed:

#!/bin/bash
# Check if globwalk is already in Cargo.toml
echo "=== Checking Cargo.toml for globwalk ==="
cat Cargo.toml

echo -e "\n=== Searching for custom glob functions mentioned in issue ==="
# Search for the specific functions mentioned in the issue
rg -n "BraceValidator|normalize_separators|process_glob_entry|CharContext" 

echo -e "\n=== Search for glob-related code ==="
rg -n "fn glob_paths" -A 5

echo -e "\n=== Check for any glob module or file ==="
fd -e rs | xargs grep -l "glob" | head -20

Length of output: 6891

---

🏁 Script executed:

#!/bin/bash
# Check the size and complexity of the custom glob code
echo "=== Line count of glob.rs ==="
wc -l src/manifest/glob.rs

echo -e "\n=== View the glob.rs file ==="
cat src/manifest/glob.rs

echo -e "\n=== Check for documentation about glob patterns ==="
rg -n "glob" README.md docs/ -i 2>/dev/null || echo "No docs directory or README mentions"

echo -e "\n=== Search for glob in markdown files ==="
fd -e md | xargs grep -l "glob" 2>/dev/null || echo "No glob mentions in markdown files"

Length of output: 24187

---

🏁 Script executed:

#!/bin/bash
# Check test coverage for glob functionality
echo "=== Check glob tests ==="
cat tests/manifest_glob_tests.rs

echo -e "\n=== Count test cases ==="
rg -n "#\[test\]|#\[rstest\]" tests/manifest_glob_tests.rs | wc -l

Length of output: 11561

---

🏁 Script executed:

#!/bin/bash
# Apply the appropriate labels to the issue
gh issue edit 171 --add-label "low"

Length of output: 44

---

@leynos, I've completed the assessment of issue #171. Here are my findings:

Issue Status: Still Relevant

The custom glob pattern handling code described in the issue is still present in the codebase. I confirmed that:

• The file src/manifest/glob.rs contains 525 lines of custom code, including all the structures and functions mentioned: CharContext, BraceValidator, normalize_separators, and process_glob_entry
• The project currently uses glob = "0.3.3" and walkdir = "2.5" dependencies, not the suggested globwalk crate
• No refactoring towards globwalk has been implemented

Documentation Status

The documentation is comprehensive and up-to-date for the user-facing glob() function. Multiple documents cover its usage extensively (README.md, users-guide.md, netsuke-design.md). However, the documentation focuses on user-facing functionality rather than internal implementation details, which is appropriate.

Criticality Assessment: LOW

I've applied the low label to this issue. Here's my rationale:

Risk factors:

• User impact: None - this is purely internal implementation with no user-facing bugs
• Blast radius: Low - the refactoring is internal only, with no API changes
• Current functionality: Works correctly with good test coverage (11+ test cases)
• Type: Technical debt / code quality improvement

Benefits:

• Reduced maintenance burden
• Simpler, more maintainable code
• Leverage well-tested external crate

Impact of Required Changes

Scope: Large internal refactoring

• Remove ~500 lines of custom parsing/validation logic
• Add globwalk = "0.8" dependency
• Replace custom implementation with GlobWalkerBuilder API
• Remove obsolete code: CharContext, BraceValidator, BraceValidationState, separator normalization functions, and custom error handling

Risk mitigation:

• Excellent test coverage exists (tests/manifest_glob_tests.rs with comprehensive cases)
• All existing tests must pass after refactoring
• No user-facing changes expected
• Functionality should remain identical

Effort: Medium - straightforward replacement but requires careful testing of edge cases (escaping, brace expansion, separator handling, case sensitivity, directory filtering)

Recommendation: This is a worthwhile quality improvement when capacity permits, but not urgent given the current implementation works reliably.

[coderabbitai]

⛔ Blocked by #242

This issue is blocked by expanding test coverage for the glob validation logic before undertaking the refactoring work.