Repository navigation
Delegate repository-path glob matching to ktsu.TextFilter #33
Description
What's hand-rolled
RepositoryAllowList matches a request path against each upstream's configured Repositories
glob patterns by hand-translating every pattern into an anchored Regex:
GitLfsCache/GitLfsCache/Upstreams/RepositoryAllowList.cs
Lines 36 to 84 in e32df4e
| private readonly Dictionary<string, Regex[]> _patterns = options.Value.Upstreams | |
| .ToDictionary( | |
| pair => pair.Key, | |
| pair => pair.Value.Repositories | |
| .Where(pattern => !string.IsNullOrWhiteSpace(pattern)) | |
| .Select(Translate) | |
| .ToArray(), | |
| StringComparer.OrdinalIgnoreCase); | |
| /// <inheritdoc /> | |
| public bool IsAllowed(string upstream, string path) | |
| { | |
| Ensure.NotNull(upstream); | |
| Ensure.NotNull(path); | |
| if (!_patterns.TryGetValue(upstream, out Regex[]? patterns)) | |
| { | |
| return false; | |
| } | |
| string candidate = path.Trim('/'); | |
| return patterns.Any(pattern => pattern.IsMatch(candidate)); | |
| } | |
| /// <summary> | |
| /// Translates a glob pattern to an anchored regular expression. | |
| /// </summary> | |
| /// <remarks> | |
| /// The pattern is escaped first so every character is literal, then the two wildcards are put | |
| /// back. Order matters: <c>**</c> is restored before <c>*</c>, otherwise the first star of a | |
| /// double would be consumed as a single. Escaping produces <c>\*</c>, and the replacement for | |
| /// <c>**</c> contains no backslash, so the second replacement cannot reach inside it. | |
| /// </remarks> | |
| /// <param name="pattern">The configured pattern.</param> | |
| /// <returns>The compiled expression.</returns> | |
| private static Regex Translate(string pattern) | |
| { | |
| string escaped = Regex.Escape(pattern.Trim('/')); | |
| string expression = escaped | |
| .Replace(@"\*\*", ".*", StringComparison.Ordinal) | |
| .Replace(@"\*", "[^/]*", StringComparison.Ordinal); | |
| return new Regex( | |
| $"^{expression}$", | |
| RegexOptions.IgnoreCase | RegexOptions.CultureInvariant, | |
| MatchTimeout); | |
| } |
private static Regex Translate(string pattern)
{
string escaped = Regex.Escape(pattern.Trim('/'));
string expression = escaped
.Replace(@"\*\*", ".*", StringComparison.Ordinal)
.Replace(@"\*", "[^/]*", StringComparison.Ordinal);
return new Regex(
$"^{expression}$",
RegexOptions.IgnoreCase | RegexOptions.CultureInvariant,
MatchTimeout);
}IsAllowed then does patterns.Any(pattern => pattern.IsMatch(candidate)) over the per-upstream
array these produce. The class's own remarks note the 100ms MatchTimeout exists only because "a
regular expression built from configuration and run against a request path is exactly the shape
that should carry a timeout" — i.e. this is defensive code around a hand-built regex, for a problem
a purpose-built glob matcher doesn't have in the first place (no regex, no backtracking, nothing to
time out).
What ktsu.TextFilter provides
ktsu.TextFilter.TextFilter.IsMatch(string text, string filter, TextFilterType filterType = TextFilterType.Glob, TextFilterMatchOptions textFilterMatchOptions = TextFilterMatchOptions.ByWordAny):
With TextFilterMatchOptions.ByWholeString (so the path isn't split on spaces —
ExtractTextTokens at L200 returns [text] for that option), glob matching goes through
DoesMatchGlob (L240) → AnyTokenMatchesGlobFilter (L306), which parses the pattern with
DotNet.Globbing.Glob.Parse and matches with glob.IsMatch. DotNet.Glob's own semantics are the
same two wildcards this code reimplements: * matches within one path segment, ** crosses / —
exactly the [^/]* / .* split Translate builds by hand, without building or running a regex at
all (so there is nothing for MatchTimeout to guard against).
Each of RepositoryAllowList's multiple patterns per upstream can be tried in turn, the same way
patterns.Any(...) does today:
repositoryPatterns.Any(pattern =>
TextFilter.IsMatch(candidate, pattern, TextFilterType.Glob, TextFilterMatchOptions.ByWholeString));Why it's worth it
Mostly a correctness and maintenance trade, not a line-count win — Translate is short, but it is
also the kind of code the comment above it is nervous about: an escape-then-selectively-unescape
regex builder, with an explicit note that pattern order (** before *) matters or the wrong
wildcard gets consumed. DotNet.Glob is a dedicated glob implementation with its own test suite for
exactly this segment-vs-cross-segment distinction, and matching through it removes the regex layer
(and its timeout) entirely rather than defending it.
Compatibility
- Subject (
GitLfsCache) targets:net10.0only (ASP.NET Core component, by design — see the
TargetFrameworkcomment inGitLfsCache.csproj). ktsu.TextFiltertargets:net10.0(its changelog records "migrate to dotnet 10"; the current
TextFilter.csprojsets no explicitTargetFramework, taking thektsu.Sdkdefault).- Dependency direction:
ktsu.TextFilter's ownDirectory.Packages.propsdepends only on
DotNet.Glob,ktsu.Extensions, andktsu.FuzzySearch— nothing back towardGitLfsCache, so
no cycle.
Sketch
Before:
private readonly Dictionary<string, Regex[]> _patterns = options.Value.Upstreams
.ToDictionary(
pair => pair.Key,
pair => pair.Value.Repositories
.Where(pattern => !string.IsNullOrWhiteSpace(pattern))
.Select(Translate)
.ToArray(),
StringComparer.OrdinalIgnoreCase);
...
return patterns.Any(pattern => pattern.IsMatch(candidate));After:
private readonly Dictionary<string, string[]> _patterns = options.Value.Upstreams
.ToDictionary(
pair => pair.Key,
pair => pair.Value.Repositories
.Where(pattern => !string.IsNullOrWhiteSpace(pattern))
.Select(pattern => pattern.Trim('/'))
.ToArray(),
StringComparer.OrdinalIgnoreCase);
...
string candidate = path.Trim('/');
return patterns.Any(pattern =>
TextFilter.IsMatch(candidate, pattern, TextFilterType.Glob, TextFilterMatchOptions.ByWholeString));Caveats
- Case sensitivity differs. The current
Translatebuilds every regex with
RegexOptions.IgnoreCase, and the class remarks call this out deliberately ("a pattern that fails
only because someone typedStudiois a support ticket rather than a control").TextFilter.IsMatch
does not expose a case-insensitivity option for glob matching, so the candidate that would produce
the same behaviour is lower-casing bothcandidateand each pattern before the call (DotNet.Glob
patterns are ordinal), which should be verified againstRepositoryAllowListTests.csbefore
adopting. - Pattern vocabulary widens.
DotNet.Globalso understands?(single character) and
{a,b}/[...]groups, which today are literal characters underRegex.Escape. A pattern such as
a repository literally namedfoo?barwould change meaning. Worth a scan of any repositories
currently configured with those characters before switching. TextFilter.IsMatch's glob path is designed primarily for space-tokenized search-box filtering
(ByWordAny/ByWordAll,+required/-excludedtoken prefixes); using it here relies on the
less-exercisedByWholeStringbranch ofExtractTextTokens/ExtractGlobFilterTokens, which is a
narrower slice of that library's test coverage than its primary use case.
Activity
matt-edmondson commentedon Sep 15, 2026
Triage
Category: Improvement
Priority: Low
Area:
GitLfsCache— repository-path glob matchingLow: nothing broken, consolidation onto
ktsu.TextFilter.Suggested assignment: none specific.
Related:
ktsu-dev/ImGuiApp#409delegates multi-pattern glob filtering to the same library in the same audit rotation — separate repos, not a duplicate. Tracked underktsu-dev/.github#15.Open PR covering this: none.
Suggested next step: confirm
ktsu.TextFilter's glob semantics match the current implementation before swapping —*crossing path separators,**support, and case sensitivity are where hand-rolled and library globs typically diverge.That check matters more here than in the ImGuiApp counterpart. If these patterns decide which repository paths are served from or admitted to the cache, a subtle widening or narrowing of the match set is an access-control change wearing the clothes of a refactor. Worth establishing what the patterns actually gate before treating this as pure cleanup, and covering the boundary cases with tests either way.
Generated by Claude Code
matt-edmondson commentedon Sep 21, 2026
Triage
Measured
ktsu.TextFilter's glob path againstRepositoryAllowListTestscase by case, usingTextFilterType.Glob+TextFilterMatchOptions.ByWholeStringexactly as the sketch proposes. The good news first: the segment semantics are an exact match, which was the main thing in doubt.**studio/game.git/info/lfs/objects/batch**anything/at/allstudio/**studio/game.git/…/batchstudio/**someone-else/game.git/…/batchstudio/*studio/game.gitstudio/*studio/game.git/…/batchstudio/game.git/**studio/game.git/…/batchstudio/game.git/**studio/gameXgit/…/batch**""(empty)studio/**Studio/Game.git/…/batchSo
*stops at/,**crosses it, and.stays literal —DotNet.GlobreproducesTranslateexactly on all of that.The two that differ
Case. As the issue predicted.
IsAllowed_DiffersOnlyByCase_IsAllowedfails. Fixable by lower-casing both the candidate and the patterns, as suggested.**no longer matches the empty string. Not previously noted.IsAllowed_DoubleStar_AllowsEverythingassertsIsAllowed("github", string.Empty)is true;TextFilter.IsMatch("", "**", …)returns false, so that needs its own special case.Vocabulary widening, confirmed.
?and[...]do become metacharacters — patternfoo?barmatchesfooXbar, andfoo[abr]matchesfooarather than the literalfoo[abr]. ({a,b}stayed literal in this version.) Worth noting the class documents itself as "a resource control and not an access control" — upstream still authorizes every call — so this widens cache scoping rather than access, which makes it much less alarming than it first sounds.Why I stopped rather than pushing it
The swap needs a lower-casing layer plus an empty-string special case to hold today's behaviour, and accepts the
?/[...]change — in order to delete a 6-lineTranslatethat has no backtracking construct, is fully covered by the tests above, and whoseMatchTimeoutthe class already describes as belt-and-braces ("cannot fire in practice").That is a worse trade than the issue anticipated: the adaptation layer replaces the hand-rolled code rather than removing it, and it arrives with a new dependency and a widened pattern vocabulary on a control surface. Two compensating workarounds to reach behaviour parity is the signal I stopped on.
Not arguing the direction is wrong — if consolidating on
ktsu.TextFilteracross the org is the goal, this is still a candidate, and the mismatches above are all small and pinned by existing tests. But whether it is worth the trade here is a maintainer's call rather than something to land unattended, so I have left it unassigned.Best unblocker would be upstream: a case-insensitivity option on
TextFilter.IsMatch's glob path would remove the larger of the two workarounds and make this close to a drop-in.Generated by Claude Code
matt-edmondson commentedon Sep 22, 2026
The upstream unblocker this asked for is now in flight: ktsu-dev/TextFilter#98 (issue ktsu-dev/TextFilter#97) adds an opt-in
TextFilterCaseSensitivity, routed toGlobOptions.Evaluation.CaseInsensitiveon the glob path.That removes the larger of the two workarounds identified above. The
**-does-not-match-empty-string finding is recorded in #97 but deliberately not changed, so it would still need its own special case here.Not reopening the adoption question — the trade this issue stopped on (a new dependency and a widened
?/[...]vocabulary on a cache-scoping surface, to delete a 6-lineTranslate) is unchanged by the case fix and is still a maintainer's call.Generated by Claude Code
matt-edmondson commentedon Sep 24, 2026
Triage
Category: Improvement (cross-library reuse) — blocked on a maintainer decision
Priority: Low
Area: repository-path glob matching;
ktsu.TextFilteradoptionRe-triaging because the update above (2026-09-22) changed the blocker list without changing the decision, and the issue is open and unassigned.
Low, and the reason is the one the note above lands on: the benefit is deleting a 6-line
Translate, and the cost is a new runtime dependency plus a widened?/[...]pattern vocabulary on a surface that scopes a cache. That trade does not move when the case-sensitivity gap closes, so the upstream progress is genuinely good news that unblocks nothing on its own. Nothing is broken today; the existing translation works.Suggested assignment: whoever owns this repo's dependency footprint — this is a "do we want the dependency at all" call, not an implementation question.
Duplicates: none. Part of the
ktsu-reusesweep tracked in ktsu-dev/.github#15, and one of several in that rotation to stop at the same shape of trade (see ktsu-dev/Coder#71, ktsu-dev/Navigation#46, ktsu-dev/Keybinding#93, ktsu-dev/GitBranchStateCache#27 — all reuse candidates where measuring found the swap is not free). That pattern is itself worth noticing: five of the reuse issues have now independently concluded that the dependency costs more than the deleted code, which suggests the sweep's default assumption wants revisiting rather than each issue re-deriving it.Already in progress: the upstream unblocker only — ktsu-dev/TextFilter#98 (issue #97) adds an opt-in
TextFilterCaseSensitivityrouted toGlobOptions.Evaluation.CaseInsensitive. No open PR against this repository.Suggested next step: decide yes or no, and if the answer is no, close it as won't-do rather than leaving it open against a possible future. Two things to have in hand when deciding:
**-does-not-match-empty-string divergence is recorded in TextFilter#97 but deliberately not being changed, so adopting still needs a special case here. The swap is "delete 6 lines, add a dependency, add a special case" — not "delete 6 lines".?or[a-z], reverting to the localTranslatebreaks their configuration. If adoption goes ahead, that argues for landing it in a release where the widened syntax is announced deliberately rather than as an implementation detail.Generated by Claude Code
matt-edmondson commentedon Sep 28, 2026
Decision (maintainer, 2026-09-28)
ktsu.TextFilter. Accept the new dependency. This follows the org-wide reuse rule "prefer reuse, accept costs; fix gaps upstream" (Cross-library reuse audit log .github#15).**empty-match divergence. Either add a special case here or, preferably under the reuse rule, fix it upstream in TextFilter (it is recorded in TextFilter#97).?/[...]pattern syntax and document it. Announce it in the release notes and the configuration docs, because configs that use it cannot be reverted cleanly.TextFilterCaseSensitivity) once it ships, so a local lower-casing layer is not needed.Next reader: once TextFilter#98 is released, replace
TranslatewithTextFilter.IsMatch(..., TextFilterType.Glob, TextFilterMatchOptions.ByWholeString)and case-insensitive matching. KeepIsAllowed_DoubleStar_AllowsEverythinggreen, and document the widened glob syntax.Generated by Claude Code
matt-edmondson commentedon Sep 28, 2026
Triage
This is blocked again, by a later upstream change. TextFilter#98 shipped, but TextFilter 1.7.9 also changed glob semantics so that
*no longer stops at/. Adopting TextFilter now would widen every single-star pattern in a deployed config. I've unassigned myself and discarded the branch. Nothing is pushed.What I found
I did the swap as the 2026-09-28 decision describes:
TextFilter.IsMatch(candidate, pattern, TextFilterType.Glob, TextFilterMatchOptions.ByWholeString, TextFilterCaseSensitivity.CaseInsensitive), with a local special case for the empty path. Against ktsu.TextFilter 1.7.9, one test fails:IsAllowed_SingleStar_DoesNotCrossASegmentfails becausestudio/*now allowsstudio/game.git/info/lfs/objects/batch.I bisected the published packages with the same call:
studio/*studio/game.gitstudio/*studio/game.git/info/lfs/objects/batch*studio/xa/*/ca/b/x/cThe case option does not affect this. The result is the same with
CaseSensitive.The 1.7.9 release notes say this is deliberate: "Let glob
*and?match across/and\[patch]" (branchfix/glob-wildcards-cross-separators). This is not an accidental regression. TextFilter's glob is now a flat text glob, not a path glob.Why that blocks this issue
The README documents repository patterns as "
*inside one segment and**across segments". The allow-list tests pin that. On 1.7.9,studio/*would behave likestudio/**. That silently widens a cache-scoping control for anyone who wrote a single star. It is also the "hard to walk back" kind of change the 2026-09-24 triage warned about.What does work, so it is not in doubt
With 1.7.8, everything the decision asked for holds:
TextFilterCaseSensitivity.CaseInsensitive, with no lower-casing layer?and[...]as the accepted widened syntax**-only special case for the empty pathThe full suite passed except the single-star test. Every allow-list test passed on 1.7.8 in isolation.
Pinning 1.7.8 is not a real option, because dependabot would bump it to 1.7.9 and CI would go red on the single-star test.
What would unblock it (a maintainer's call)
*and?inside a/segment and routes to DotNet.Glob's default behaviour, which is still segment-aware. With that option this becomes the drop-in the decision describes.*would mean the same as**for repository patterns. Update the README and the single-star test, and announce it in the release notes.Translate: close this issue as won't-do, since TextFilter no longer models path globs.Generated by Claude Code