Skip to content

Delegate repository-path glob matching to ktsu.TextFilter #33

Open

Description

@matt-edmondson

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:

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):

https://github.com/ktsu-dev/TextFilter/blob/153eb86606b81d81461663fcf1c1c3748fbfd4e3/TextFilter/TextFilter.cs#L193-L194

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.0 only (ASP.NET Core component, by design — see the
    TargetFramework comment in GitLfsCache.csproj).
  • ktsu.TextFilter targets: net10.0 (its changelog records "migrate to dotnet 10"; the current
    TextFilter.csproj sets no explicit TargetFramework, taking the ktsu.Sdk default).
  • Dependency direction: ktsu.TextFilter's own Directory.Packages.props depends only on
    DotNet.Glob, ktsu.Extensions, and ktsu.FuzzySearch — nothing back toward GitLfsCache, 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 Translate builds every regex with
    RegexOptions.IgnoreCase, and the class remarks call this out deliberately ("a pattern that fails
    only because someone typed Studio is 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 both candidate and each pattern before the call (DotNet.Glob
    patterns are ordinal), which should be verified against RepositoryAllowListTests.cs before
    adopting.
  • Pattern vocabulary widens. DotNet.Glob also understands ? (single character) and
    {a,b}/[...] groups, which today are literal characters under Regex.Escape. A pattern such as
    a repository literally named foo?bar would 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/-excluded token prefixes); using it here relies on the
    less-exercised ByWholeString branch of ExtractTextTokens/ExtractGlobFilterTokens, which is a
    narrower slice of that library's test coverage than its primary use case.

Activity

  1. matt-edmondson commented on Sep 15, 2026

    @matt-edmondson
    ContributorAuthor

    Triage

    Category: Improvement
    Priority: Low
    Area: GitLfsCache — repository-path glob matching

    Low: nothing broken, consolidation onto ktsu.TextFilter.

    Suggested assignment: none specific.

    Related: ktsu-dev/ImGuiApp#409 delegates multi-pattern glob filtering to the same library in the same audit rotation — separate repos, not a duplicate. Tracked under ktsu-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

  2. matt-edmondson commented on Sep 21, 2026

    @matt-edmondson
    ContributorAuthor

    Triage

    Measured ktsu.TextFilter's glob path against RepositoryAllowListTests case by case, using TextFilterType.Glob + TextFilterMatchOptions.ByWholeString exactly as the sketch proposes. The good news first: the segment semantics are an exact match, which was the main thing in doubt.

    pattern path today TextFilter
    ** studio/game.git/info/lfs/objects/batch allow allow
    ** anything/at/all allow allow
    studio/** studio/game.git/…/batch allow allow
    studio/** someone-else/game.git/…/batch refuse refuse
    studio/* studio/game.git allow allow
    studio/* studio/game.git/…/batch refuse refuse
    studio/game.git/** studio/game.git/…/batch allow allow
    studio/game.git/** studio/gameXgit/…/batch refuse refuse
    ** "" (empty) allow refuse ❌
    studio/** Studio/Game.git/…/batch allow refuse ❌

    So * stops at /, ** crosses it, and . stays literal — DotNet.Glob reproduces Translate exactly on all of that.

    The two that differ

    Case. As the issue predicted. IsAllowed_DiffersOnlyByCase_IsAllowed fails. Fixable by lower-casing both the candidate and the patterns, as suggested.

    ** no longer matches the empty string. Not previously noted. IsAllowed_DoubleStar_AllowsEverything asserts IsAllowed("github", string.Empty) is true; TextFilter.IsMatch("", "**", …) returns false, so that needs its own special case.

    Vocabulary widening, confirmed. ? and [...] do become metacharacters — pattern foo?bar matches fooXbar, and foo[abr] matches fooa rather than the literal foo[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-line Translate that has no backtracking construct, is fully covered by the tests above, and whose MatchTimeout the 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.TextFilter across 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

  3. matt-edmondson commented on Sep 22, 2026

    @matt-edmondson
    ContributorAuthor

    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 to GlobOptions.Evaluation.CaseInsensitive on 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-line Translate) is unchanged by the case fix and is still a maintainer's call.


    Generated by Claude Code

  4. matt-edmondson commented on Sep 24, 2026

    @matt-edmondson
    ContributorAuthor

    Triage

    Category: Improvement (cross-library reuse) — blocked on a maintainer decision
    Priority: Low
    Area: repository-path glob matching; ktsu.TextFilter adoption

    Re-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-reuse sweep 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 TextFilterCaseSensitivity routed to GlobOptions.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:

    1. The **-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".
    2. Widening the pattern vocabulary is the part that is hard to walk back. Once a user's cache-scoping config contains ? or [a-z], reverting to the local Translate breaks 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

  5. matt-edmondson commented on Sep 28, 2026

    @matt-edmondson
    ContributorAuthor

    Decision (maintainer, 2026-09-28)

    • Adopt 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).
    • Handle the ** 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).
    • Accept the wider ? / [...] pattern syntax and document it. Announce it in the release notes and the configuration docs, because configs that use it cannot be reverted cleanly.
    • Case-insensitivity comes from TextFilter#98 (TextFilterCaseSensitivity) once it ships, so a local lower-casing layer is not needed.

    Next reader: once TextFilter#98 is released, replace Translate with TextFilter.IsMatch(..., TextFilterType.Glob, TextFilterMatchOptions.ByWholeString) and case-insensitive matching. Keep IsAllowed_DoubleStar_AllowsEverything green, and document the widened glob syntax.


    Generated by Claude Code

  6. matt-edmondson commented on Sep 28, 2026

    @matt-edmondson
    ContributorAuthor

    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_DoesNotCrossASegment fails because studio/* now allows studio/game.git/info/lfs/objects/batch.

    I bisected the published packages with the same call:

    pattern path 1.7.0 – 1.7.8 1.7.9 DotNet.Glob directly
    studio/* studio/game.git match match match
    studio/* studio/game.git/info/lfs/objects/batch no match match no match
    * studio/x no match match no match
    a/*/c a/b/x/c no match match no match

    The 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]" (branch fix/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 like studio/**. 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:

    • segment semantics
    • case-insensitive matching through TextFilterCaseSensitivity.CaseInsensitive, with no lower-casing layer
    • ? and [...] as the accepted widened syntax
    • an **-only special case for the empty path

    The 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)

    1. Upstream (preferred under the reuse rule): add a path-aware option to TextFilter's glob, e.g. an option that keeps * 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.
    2. Accept the widening here: * would mean the same as ** for repository patterns. Update the README and the single-star test, and announce it in the release notes.
    3. Keep Translate: close this issue as won't-do, since TextFilter no longer models path globs.

    Generated by Claude Code

  7. removed their assignment
    on Sep 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions