From 5ab25b8151c7d304086aa89178d20b6828d1f2cf Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 30 Oct 2026 00:26:07 +0000 Subject: [PATCH] Remember when a branch shares no history with the base [patch] FindMergeBaseAsync cached only a found merge base, so an orphan branch such as gh-pages re-ran a full-history git merge-base on every /state request. Exit code 1 with no output is now cached as "no merge base"; timeouts and other errors are still retried. Fixes ktsu-dev/GitBranchStateCache#105 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_017vfJW3f2Szc4ZLoCTbPhuX --- .../Mirrors/MirrorQueriesTests.cs | 36 +++++++++++++++++ GitBranchStateCache/Mirrors/MirrorQueries.cs | 40 ++++++++++++++++--- 2 files changed, 70 insertions(+), 6 deletions(-) diff --git a/GitBranchStateCache.Tests/Mirrors/MirrorQueriesTests.cs b/GitBranchStateCache.Tests/Mirrors/MirrorQueriesTests.cs index 4175072..32ec470 100644 --- a/GitBranchStateCache.Tests/Mirrors/MirrorQueriesTests.cs +++ b/GitBranchStateCache.Tests/Mirrors/MirrorQueriesTests.cs @@ -83,6 +83,42 @@ public async Task FindMergeBaseAsync_AFailure_IsNotRemembered() Assert.AreEqual(2, runner.CountOf("merge-base")); } + [TestMethod] + public async Task FindMergeBaseAsync_UnrelatedHistories_RunsGitOnce() + { + // An orphan branch such as gh-pages shares no history with the base. git has to walk both + // histories in full to say so, and the answer never changes, so it is asked only once. + FakeGitRunner runner = new() + { + Respond = _ => new GitResult(1, string.Empty, string.Empty, TimedOut: false), + }; + MirrorQueries queries = Build(runner); + + string? first = await queries.FindMergeBaseAsync(Mirror, Base, Tip, CancellationToken.None); + string? second = await queries.FindMergeBaseAsync(Mirror, Base, Tip, CancellationToken.None); + + Assert.IsNull(first); + Assert.IsNull(second); + Assert.AreEqual(1, runner.CountOf("merge-base")); + } + + [TestMethod] + public async Task FindMergeBaseAsync_AnError_IsNotRemembered() + { + // Exit codes above 1 are errors, such as a missing object, and say nothing about the commits. + FakeGitRunner runner = new() + { + Respond = _ => new GitResult(128, string.Empty, "fatal: Not a valid commit name", TimedOut: false), + }; + MirrorQueries queries = Build(runner); + + string? first = await queries.FindMergeBaseAsync(Mirror, Base, Tip, CancellationToken.None); + await queries.FindMergeBaseAsync(Mirror, Base, Tip, CancellationToken.None); + + Assert.IsNull(first); + Assert.AreEqual(2, runner.CountOf("merge-base")); + } + [TestMethod] public async Task FindMergeBaseAsync_OverTheLimit_ForgetsTheLeastRecentlyUsed() { diff --git a/GitBranchStateCache/Mirrors/MirrorQueries.cs b/GitBranchStateCache/Mirrors/MirrorQueries.cs index dbe5379..edf467b 100644 --- a/GitBranchStateCache/Mirrors/MirrorQueries.cs +++ b/GitBranchStateCache/Mirrors/MirrorQueries.cs @@ -14,6 +14,11 @@ namespace ktsu.GitBranchStateCache.Mirrors; /// invalidating, and every editor's heartbeat asks for the same few hundred of them until someone /// pulls or a branch moves. Without this, a request naming a few hundred branches starts a few hundred /// git processes before it can answer anything from the diff cache. +/// +/// "No common history" is remembered too, for the same reason: it is just as immutable, and it is the +/// most expensive answer git gives, because it has to walk both histories in full to reach it. A +/// timeout or any other failure says nothing about the commits and is never remembered. +/// /// /// Runs git. /// The configured options. @@ -22,6 +27,11 @@ public sealed class MirrorQueries(IGitRunner runner, IOptions>> _mergeBases = new(StringComparer.Ordinal); + /// + /// Stands in the cache for "these two commits share no history". An object id is never empty. + /// + private const string NoMergeBase = ""; + private readonly LinkedList> _recency = new(); private readonly Lock _gate = new(); @@ -66,7 +76,7 @@ public async Task ContainsCommitAsync( // produce the same key. string cacheKey = $"{directory}\n{first}\n{second}"; - if (TryRead(cacheKey) is string cached) + if (TryRead(cacheKey, out string? cached)) { return cached; } @@ -83,10 +93,16 @@ public async Task ContainsCommitAsync( }, cancellationToken).ConfigureAwait(false); + if (IsUnrelatedHistories(result)) + { + // A real answer rather than a failure: there is nothing to diff against, and there never + // will be for these two commits. + Publish(cacheKey, NoMergeBase); + return null; + } + if (!result.Succeeded) { - // Exit code 1 means the histories are unrelated, which is a real answer rather than a - // failure, and both are handled the same way: there is nothing to diff against. return null; } @@ -101,18 +117,30 @@ public async Task ContainsCommitAsync( return merged; } - private string? TryRead(string cacheKey) + /// + /// Whether git answered that the two commits share no history, as opposed to failing or timing out. + /// + /// + /// git merge-base exits 1 with nothing on standard output when there is no common ancestor. + /// Errors such as a missing object exit with a higher code and say why on standard error. + /// + private static bool IsUnrelatedHistories(GitResult result) => + !result.TimedOut && result.ExitCode == 1 && string.IsNullOrWhiteSpace(result.StandardOutput); + + private bool TryRead(string cacheKey, out string? mergeBase) { lock (_gate) { if (!_mergeBases.TryGetValue(cacheKey, out LinkedListNode>? node) || node is null) { - return null; + mergeBase = null; + return false; } _recency.Remove(node); _recency.AddFirst(node); - return node.Value.Value; + mergeBase = node.Value.Value.Length == 0 ? null : node.Value.Value; + return true; } }