From f22a1b0711fb096cc9d29713ae951225317d12f6 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 23 Sep 2026 19:30:37 +0000 Subject: [PATCH] fix: canonicalize the upstream key, not just the repository path [patch] MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit BranchStateHandler.Resolve built its MirrorKey from the caller's literal upstream segment while canonicalizing only the sibling repository path: MirrorKey key = new(route.Upstream, Canonicalize(route.RepositoryPath)); IUpstreamRegistry.TryResolve and IRepositoryAllowList.IsAllowed both key their dictionaries with StringComparer.OrdinalIgnoreCase, so "GitHub" and "github" pass as one configured upstream. Everything downstream of that compares ordinally, so the two spellings produced two bare mirrors of the same repository, two credential probes, and two diffs — exactly the duplication issue #24 removed for repository-path casing, reachable through the other half of the key. Both halves now go through Canonicalize. Nothing sent to the forge changes: the upstream base URL comes from configuration, not from the caller's segment, and the repository URL is still built from the caller's spelling of the path. Fixes ktsu-dev/GitBranchStateCache#37 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_018VSTy8Ye7JXqeFqvhrmnRt --- .gitignore | 18 ++++ .../Integration/RepositoryIdentityTests.cs | 90 +++++++++++++++++-- .../Endpoints/BranchStateHandler.cs | 39 +++++--- 3 files changed, 128 insertions(+), 19 deletions(-) diff --git a/.gitignore b/.gitignore index dc0470a..e043c9f 100644 --- a/.gitignore +++ b/.gitignore @@ -203,6 +203,11 @@ PublishScripts/ **/[Pp]ackages/* # except build/, which is used as an MSBuild target. !**/[Pp]ackages/build/ +# and except a Unity project's Packages/, which is source: Unity's package manifest and its +# resolved lock file are both meant to be committed, and a NuGet restore folder never contains +# a file by either name. +!**/[Pp]ackages/manifest.json +!**/[Pp]ackages/packages-lock.json # Uncomment if necessary however generally it will be regenerated when needed #!**/[Pp]ackages/repositories.config # NuGet v3's project.json files produces more ignorable files @@ -651,3 +656,16 @@ Temporary Items # ImGui.ini files imgui.ini + +# Game engine projects +# +# Godot: the import cache, and the mono/temp bin+obj a C# build writes. +.godot/ + +# Unity: .meta files are source, not the Visual Studio C++ build artifact that the `*.meta` rule +# further up targets. Unity generates one per asset and it carries the GUID that scenes, prefabs +# and serialized references point at, so ignoring them gives every clone fresh GUIDs and silently +# breaks those references - including for a plug-in whose .dll is itself a build output. This +# negation has to come after that rule to win, and is scoped to the asset tree so the Visual +# Studio artifact stays ignored everywhere else. +!**/[Aa]ssets/**/*.meta diff --git a/GitBranchStateCache.Tests/Integration/RepositoryIdentityTests.cs b/GitBranchStateCache.Tests/Integration/RepositoryIdentityTests.cs index ff8239b..48fe136 100644 --- a/GitBranchStateCache.Tests/Integration/RepositoryIdentityTests.cs +++ b/GitBranchStateCache.Tests/Integration/RepositoryIdentityTests.cs @@ -12,11 +12,12 @@ namespace ktsu.GitBranchStateCache.Tests.Integration; /// What counts as "the same repository" once a request has been accepted. /// /// -/// The allow-list matches case insensitively on purpose, so two callers can address one repository by -/// two spellings and both be served. Everything derived afterwards — the mirror directory, the -/// coalescing key, the diff cache key, the admission key — compares ordinally, so unless the path is -/// canonicalised once at the point it is accepted, one repository quietly becomes two of everything. -/// That is the whole of this service's purpose inverted: it would do the work twice rather than once. +/// The upstream registry and the allow-list both match case insensitively on purpose, so two callers +/// can address one repository by two spellings — of the repository path, of the upstream key, or of +/// both — and both be served. Everything derived afterwards — the mirror directory, the coalescing +/// key, the diff cache key, the admission key — compares ordinally, so unless the key is canonicalised +/// once at the point it is accepted, one repository quietly becomes two of everything. That is the +/// whole of this service's purpose inverted: it would do the work twice rather than once. /// [TestClass] public class RepositoryIdentityTests @@ -32,6 +33,12 @@ public class RepositoryIdentityTests /// The same repository as a client that cloned it with different casing spells it. private const string CallerSpelling = "/v1/github/Studio/Game.git/state"; + /// + /// The same repository again, reached through the upstream key spelled the way a forge brands + /// itself rather than the way the configuration keys it. + /// + private const string UpstreamCallerSpelling = "/v1/GitHub/studio/game.git/state"; + private const string Body = $$"""{"base":"{{ClientBase}}","branchPatterns":["origin/main"]}"""; private static void Seed(ScriptedGit git) @@ -126,4 +133,77 @@ public async Task ARequestSpelledDifferentlyFromTheAllowList_MirrorsUnderTheCano "game.git", "mirror.git"))); } + + [TestMethod] + public async Task TwoSpellingsOfOneUpstream_AreBothServed() + { + // The premise for the upstream key, matching the one above for the repository path: the registry + // and the allow-list both resolve it case insensitively, so a caller that writes the forge's own + // branding rather than the configuration key is still addressing the configured upstream. + await using ServiceFixture fixture = await ServiceFixture.StartAsync(); + Seed(fixture.Git); + + using HttpResponseMessage configured = await fixture.Client.SendAsync(Request(ConfiguredSpelling)); + using HttpResponseMessage caller = await fixture.Client.SendAsync(Request(UpstreamCallerSpelling)); + + Assert.AreEqual(HttpStatusCode.OK, configured.StatusCode); + Assert.AreEqual(HttpStatusCode.OK, caller.StatusCode); + } + + [TestMethod] + public async Task TwoSpellingsOfOneUpstream_ShareOneMirrorFetchDiffAndAdmission() + { + // The same duplication the repository-path case guards against, reached through the sibling half + // of the key instead. One repository, so one clone, one credential probe, and one diff. + await using ServiceFixture fixture = await ServiceFixture.StartAsync(); + Seed(fixture.Git); + + using HttpResponseMessage configured = await fixture.Client.SendAsync(Request(ConfiguredSpelling)); + using HttpResponseMessage caller = await fixture.Client.SendAsync(Request(UpstreamCallerSpelling)); + + Assert.AreEqual(HttpStatusCode.OK, configured.StatusCode); + Assert.AreEqual(HttpStatusCode.OK, caller.StatusCode); + + Assert.AreEqual(1, fixture.Git.CountOf("clone")); + Assert.AreEqual(1, fixture.Git.CountOf("ls-remote")); + Assert.AreEqual(1, fixture.Git.CountOf("diff-tree")); + } + + [TestMethod] + public async Task TwoSpellingsOfOneUpstream_ProduceOneMirrorDirectory() + { + // Asserted against the volume, because an upstream key that is not canonicalised becomes a + // second top-level directory holding a complete duplicate of every repository under it. + await using ServiceFixture fixture = await ServiceFixture.StartAsync(); + Seed(fixture.Git); + + using HttpResponseMessage configured = await fixture.Client.SendAsync(Request(ConfiguredSpelling)); + using HttpResponseMessage caller = await fixture.Client.SendAsync(Request(UpstreamCallerSpelling)); + + IDirectory directory = fixture.FileSystem.Directory; + + Assert.HasCount( + 1, + directory.GetDirectories(ServiceFixture.MirrorRoot, "mirror.git", SearchOption.AllDirectories)); + } + + [TestMethod] + public async Task ARequestWithTheUpstreamSpelledDifferently_MirrorsUnderTheCanonicalUpstream() + { + // The canonical form is lower case for the upstream key as much as for the path, so the volume + // carries one directory per configured upstream whatever casing reached the service. + await using ServiceFixture fixture = await ServiceFixture.StartAsync(); + Seed(fixture.Git); + + using HttpResponseMessage response = await fixture.Client.SendAsync(Request(UpstreamCallerSpelling)); + + Assert.AreEqual(HttpStatusCode.OK, response.StatusCode); + + Assert.IsTrue(fixture.FileSystem.Directory.Exists(fixture.FileSystem.Path.Combine( + ServiceFixture.MirrorRoot, + "github", + "studio", + "game.git", + "mirror.git"))); + } } diff --git a/GitBranchStateCache/Endpoints/BranchStateHandler.cs b/GitBranchStateCache/Endpoints/BranchStateHandler.cs index 90ce8e8..1b7599b 100644 --- a/GitBranchStateCache/Endpoints/BranchStateHandler.cs +++ b/GitBranchStateCache/Endpoints/BranchStateHandler.cs @@ -407,7 +407,7 @@ .. context.Request.Query["pattern"] return null; } - MirrorKey key = new(route.Upstream, Canonicalize(route.RepositoryPath)); + MirrorKey key = new(Canonicalize(route.Upstream), Canonicalize(route.RepositoryPath)); if (!mirrors.TryResolve(key, out string? directory) || !UpstreamUrl.TryCombine(upstreamBase!, route.RepositoryPath, out Uri? repositoryUrl)) @@ -419,26 +419,37 @@ .. context.Request.Query["pattern"] } /// - /// Reduces a repository path to the one spelling this service knows it by. + /// Reduces one half of a — the upstream key or the repository path — to the + /// one spelling this service knows it by. /// /// - /// This exists because the allow-list immediately above it matches case insensitively, deliberately - /// and for good reasons of its own, while every identity derived from the path afterwards compares - /// ordinally: the mirror directory on a case-sensitive volume, the coalescing key that keeps - /// concurrent work on one repository to a single fetch, the diff cache key built from it, and the - /// admission key. Pass the caller's literal spelling on and one repository addressed two ways is - /// two mirrors on disk, fetched twice per heartbeat, diffed twice, and probed twice — which is the - /// duplication this service exists to remove, reintroduced by a difference the allow-list has - /// already ruled irrelevant. So it is canonicalised once, here, at the only point that decides what - /// a request is about. Removing this does not simplify anything; it silently doubles the cost of - /// every repository whose callers do not agree on casing. + /// This exists because the upstream resolution and the allow-list immediately above it both match + /// case insensitively, deliberately and for good reasons of their own, while every identity derived + /// from the key afterwards compares ordinally: the mirror directory on a case-sensitive volume, the + /// coalescing key that keeps concurrent work on one repository to a single fetch, the diff cache key + /// built from it, and the admission key. Pass the caller's literal spelling on and one repository + /// addressed two ways is two mirrors on disk, fetched twice per heartbeat, diffed twice, and probed + /// twice — which is the duplication this service exists to remove, reintroduced by a difference the + /// checks above have already ruled irrelevant. So it is canonicalised once, here, at the only point + /// that decides what a request is about. Removing this does not simplify anything; it silently + /// doubles the cost of every repository whose callers do not agree on casing. + /// + /// Both halves of the key go through this, and for the same reason. Canonicalising only the + /// repository path leaves the identical duplication reachable through the upstream key instead, + /// because and + /// accept GitHub and github as one configured upstream while everything downstream of + /// them tells the two apart. + /// /// /// Only this service's own bookkeeping is canonicalised. What is sent to the forge keeps the /// caller's spelling, because the forge is the authority on how it spells its own repository names - /// and this service should not be rewriting a URL on its behalf. + /// and this service should not be rewriting a URL on its behalf. The upstream key never reaches the + /// forge at all: the base URL comes from configuration, not from the caller's segment. /// /// - private static string Canonicalize(string repositoryPath) => repositoryPath.ToLowerInvariant(); + /// The upstream key or repository path as the caller spelled it. + /// The spelling this service records the request under. + private static string Canonicalize(string segment) => segment.ToLowerInvariant(); private static bool TryParsePatterns( IReadOnlyList? requested,