Skip a branch whose name is not UTF-8 instead of failing every ref listing [patch]

GitBranchStateCache.Tests/Fakes/ListLogger.cs

@@ -0,0 +1,31 @@
+// Copyright (c) 2023-2026 ktsu-dev contributors
+
+namespace ktsu.GitBranchStateCache.Tests.Fakes;
+
+using Microsoft.Extensions.Logging;
+
+/// <summary>
+/// A logger that records every formatted message it is given.
+/// </summary>
+/// <typeparam name="T">The category.</typeparam>
+/// <param name="messages">Where the messages go.</param>
+internal sealed class ListLogger<T>(List<string> messages) : ILogger<T>
+{
+	public IDisposable? BeginScope<TState>(TState state)
+		where TState : notnull => null;
+
+	public bool IsEnabled(LogLevel logLevel) => true;
+
+	public void Log<TState>(
+		LogLevel logLevel,
+		EventId eventId,
+		TState state,
+		Exception? exception,
+		Func<TState, Exception?, string> formatter)
+	{
+		lock (messages)
+		{
+			messages.Add(formatter(state, exception));
+		}
+	}
+}

GitBranchStateCache.Tests/Git/RealRepositoryTests.cs

@@ -9,6 +9,7 @@
 using ktsu.GitBranchStateCache.Git;
 using ktsu.GitBranchStateCache.Mirrors;
 using ktsu.GitBranchStateCache.Refs;
+using ktsu.GitBranchStateCache.Tests.Fakes;
 using Microsoft.Extensions.Options;
 using Microsoft.VisualStudio.TestTools.UnitTesting;
 
@@ -109,6 +110,37 @@
 		Assert.IsTrue(branches!.Any(branch => branch.Name == "origin/feature/ui" && branch.Tip == _featureTip));
 	}
 
+	[TestMethod]
+	public async Task RefResolver_ABranchNameThatIsNotUtf8_IsSkipped()
+	{
+		// A copy of the mirror, so the bad ref does not leak into the other tests. The ref goes in
+		// through packed-refs because a file name with a lone 0xE9 byte cannot be spelled from .NET.
+		string copy = Path.Combine(Path.GetDirectoryName(_mirror)!, $"latin1-{Guid.NewGuid():N}.git");
+		Git(Path.GetDirectoryName(_mirror)!, "clone", "--bare", "--quiet", _mirror, copy);
+		byte[] line = [.. Encoding.ASCII.GetBytes($"{_featureTip} refs/heads/feature/caf"), 0xE9, (byte)'\n'];
+		string packedRefs = Path.Combine(copy, "packed-refs");
+
+		// The appended line is out of order, so the header must stop promising a sorted file, or git
+		// binary-searches past it.
+		string existing = (await File.ReadAllTextAsync(packedRefs)).Replace(" sorted", string.Empty, StringComparison.Ordinal);
+		await File.WriteAllBytesAsync(packedRefs, [.. Encoding.ASCII.GetBytes(existing), .. line]);
+		Assert.Contains("caf", Git(copy, "for-each-ref", "refs/heads/feature/"), "git did not take the fixture ref.");
+
+		List<string> skipped = [];
+		ListLogger<RefResolver> logger = new(skipped);
+
+		IReadOnlyList<BranchRef>? branches = await new RefResolver(Runner, Options.Create(Settings), logger)
+			.ListAsync(copy, CancellationToken.None);
+
+		Assert.IsNotNull(branches, "One undecodable branch failed the whole listing.");
+		Assert.IsTrue(branches!.Any(branch => branch.Name == "origin/main" && branch.Tip == _mainTip));
+		Assert.IsTrue(branches!.Any(branch => branch.Name == "origin/feature/ui"));
+		Assert.IsFalse(branches!.Any(branch => branch.Name.Contains("caf", StringComparison.Ordinal)));
+		Assert.HasCount(1, skipped);
+		Assert.Contains("72656673", skipped[0]);
+		Assert.Contains("636166E9", skipped[0]);
+	}
+
 	[TestMethod]
 	public async Task RefResolver_ABrokenMirrorInsideAnotherRepository_ReadsNothing()
 	{

GitBranchStateCache.Tests/Integration/ScriptedGit.cs

@@ -3,6 +3,7 @@
 namespace ktsu.GitBranchStateCache.Tests.Integration;
 
 using System.Collections.Concurrent;
+using System.Text;
 using ktsu.GitBranchStateCache.Git;
 using Testably.Abstractions.Testing;
 
@@ -34,6 +35,12 @@ internal sealed class ScriptedGit(MockFileSystem fileSystem) : IGitRunner
 	/// <summary>Gets the commits the mirror holds.</summary>
 	public HashSet<string> Commits { get; } = new(StringComparer.Ordinal);
 
+	/// <summary>
+	/// Gets extra <c>for-each-ref</c> lines, one character per byte, for refs whose names are not UTF-8
+	/// and so cannot be held in <see cref="Branches"/>.
+	/// </summary>
+	public List<string> RawRefLines { get; } = [];
+
 	/// <summary>Gets or sets whether an ls-remote succeeds.</summary>
 	public bool AdmitsCredentials { get; set; } = true;
 
@@ -76,7 +83,7 @@ public async Task<GitResult> RunAsync(GitInvocation invocation, CancellationToke
 			"commit-graph" => CommitGraphFails
 				? new GitResult(128, string.Empty, "fatal: could not write commit-graph", TimedOut: false)
 				: Success(string.Empty),
-			"for-each-ref" => Success(ForEachRef()),
+			"for-each-ref" => ForEachRef(invocation),
 			"cat-file" => CatFile(arguments),
 			"merge-base" => MergeBase(arguments),
 			"diff-tree" => Diff(arguments),
@@ -94,6 +101,19 @@ private GitResult Clone(IReadOnlyList<string> arguments)
 		return Success(string.Empty);
 	}
 
+	private GitResult ForEachRef(GitInvocation invocation)
+	{
+		if (invocation.ReadStandardOutputAsBytes)
+		{
+			return Success(Encoding.Latin1.GetString(Encoding.UTF8.GetBytes(ForEachRef())) + string.Concat(RawRefLines));
+		}
+
+		// What GitRunner's strict decoding does with a byte that is not UTF-8.
+		return RawRefLines.Count > 0
+			? new GitResult(-1, string.Empty, "git produced output that is not valid UTF-8.", TimedOut: false)
+			: Success(ForEachRef());
+	}
+
 	private string ForEachRef() =>
 		string.Concat(Branches.Select(branch => $"{branch.Value} refs/heads/{branch.Key}\n"));
 

GitBranchStateCache.Tests/Integration/StateFlowTests.cs

@@ -540,4 +540,31 @@
 
 		Assert.AreEqual(fixture.Time.GetUtcNow(), body.GetProperty("refsAsOf").GetDateTimeOffset());
 	}
+
+	[TestMethod]
+	public async Task ABranchNameThatIsNotUtf8_IsSkipped_AndEveryOtherBranchIsStillServed()
+	{
+		// "caf\xe9": a Latin-1 name git accepts. It used to fail the whole ref listing, so every request
+		// for the repository was a 502 until the branch was deleted upstream (#114).
+		await using ServiceFixture fixture = await ServiceFixture.StartAsync();
+		Seed(fixture.Git);
+		fixture.Git.RawRefLines.Add($"{MainTip} refs/heads/feature/caf\u00e9\n");
+
+		using HttpRequestMessage branchesRequest = new(
+			HttpMethod.Get,
+			"/v1/github/studio/game.git/branches?pattern=origin/*");
+		branchesRequest.Headers.Authorization = AuthenticationHeaderValue.Parse(Credential);
+
+		using HttpResponseMessage branches = await fixture.Client.SendAsync(branchesRequest);
+		JsonElement branchesBody = await ReadAsync(branches);
+
+		Assert.AreEqual(HttpStatusCode.OK, branches.StatusCode);
+		string?[] names = [.. branchesBody.GetProperty("branches").EnumerateArray()
+			.Select(branch => branch.GetProperty("name").GetString())];
+		CollectionAssert.AreEquivalent(new[] { "origin/main", "origin/release/1.0" }, names);
+
+		using HttpResponseMessage state = await fixture.Client.SendAsync(StateRequest(Body()));
+
+		Assert.AreEqual(HttpStatusCode.OK, state.StatusCode);
+	}
 }

GitBranchStateCache.Tests/Refs/RefResolverTests.cs

@@ -22,6 +22,21 @@ [.. patterns.Select(pattern =>
 			return parsed!;
 		})];
 
+	[TestMethod]
+	public void DecodeLines_DropsOnlyTheLineThatIsNotUtf8()
+	{
+		// One character per byte: "caf\u00e9" is the single byte 0xE9, and "\u00c3\u00a9" is the UTF-8
+		// encoding of é, which must still come through as é.
+		string output = $"{MainTip} refs/heads/main\n{ReleaseTip} refs/heads/caf\u00e9\n{ReleaseTip} refs/heads/caf\u00c3\u00a9\n";
+		List<string> skipped = [];
+
+		string decoded = RefResolver.DecodeLines(output, skipped.Add);
+
+		Assert.AreEqual($"{MainTip} refs/heads/main\n{ReleaseTip} refs/heads/café\n", decoded);
+		Assert.HasCount(1, skipped);
+		Assert.AreEqual("726566732F68656164732F636166E9", skipped[0]);
+	}
+
 	[TestMethod]
 	public void Parse_ReportsBranchesUnderTheRemoteName()
 	{

GitBranchStateCache/Git/GitInvocation.cs

@@ -50,6 +50,18 @@ public sealed class GitInvocation
 	/// </remarks>
 	public bool DiscardStandardOutput { get; init; }
 
+	/// <summary>
+	/// Gets whether standard output is returned undecoded, as one character per byte.
+	/// </summary>
+	/// <remarks>
+	/// For a command whose output holds several independent records, any of which may not be UTF-8.
+	/// Strict decoding of the whole output would let one record fail every other one, so the caller
+	/// splits the output into records first and decodes each one itself
+	/// (ktsu-dev/GitBranchStateCache#114). Each character's value is the byte git wrote, so
+	/// <see cref="System.Text.Encoding.Latin1"/> turns the text back into those bytes exactly.
+	/// </remarks>
+	public bool ReadStandardOutputAsBytes { get; init; }
+
 	/// <summary>Gets how long the command may run before it is killed.</summary>
 	public required TimeSpan Timeout { get; init; }
 }

GitBranchStateCache/Git/GitRunner.cs

@@ -83,7 +83,7 @@
 			: new(
 				chunk => standardOutput.Append(chunk),
 				chunk => standardError.Append(chunk),
-				StrictUtf8);
+				invocation.ReadStandardOutputAsBytes ? Encoding.Latin1 : StrictUtf8);
 
 		CommandOptions commandOptions = new()
 		{

GitBranchStateCache/Refs/RefLog.cs

@@ -0,0 +1,17 @@
+// Copyright (c) 2023-2026 ktsu-dev contributors
+
+namespace ktsu.GitBranchStateCache.Refs;
+
+using Microsoft.Extensions.Logging;
+
+/// <summary>
+/// Source-generated log messages for reading refs.
+/// </summary>
+internal static partial class RefLog
+{
+	[LoggerMessage(
+		EventId = 4000,
+		Level = LogLevel.Warning,
+		Message = "Skipped a ref in the mirror at '{Directory}' whose name is not valid UTF-8 (bytes {RefNameHex}). Every other branch is still served; rename or delete it upstream to serve it.")]
+	public static partial void UndecodableRefSkipped(ILogger logger, string directory, string refNameHex);
+}

GitBranchStateCache/Refs/RefResolver.cs

@@ -2,8 +2,11 @@
 
 namespace ktsu.GitBranchStateCache.Refs;
 
+using System.Text;
 using ktsu.GitBranchStateCache.Configuration;
 using ktsu.GitBranchStateCache.Git;
+using Microsoft.Extensions.Logging;
+using Microsoft.Extensions.Logging.Abstractions;
 using Microsoft.Extensions.Options;
 
 /// <summary>
@@ -17,10 +20,20 @@ namespace ktsu.GitBranchStateCache.Refs;
 /// </remarks>
 /// <param name="runner">Runs git.</param>
 /// <param name="options">The configured options.</param>
-public sealed class RefResolver(IGitRunner runner, IOptions<GitBranchStateCacheOptions> options) : IRefResolver
+/// <param name="logger">Where a skipped ref is reported, or null to report nothing.</param>
+public sealed class RefResolver(
+	IGitRunner runner,
+	IOptions<GitBranchStateCacheOptions> options,
+	ILogger<RefResolver>? logger = null) : IRefResolver
 {
 	private const string HeadPrefix = "refs/heads/";
 
+	private static readonly Encoding StrictUtf8 = new UTF8Encoding(
+		encoderShouldEmitUTF8Identifier: false,
+		throwOnInvalidBytes: true);
+
+	private readonly ILogger _logger = logger ?? NullLogger<RefResolver>.Instance;
+
 	/// <inheritdoc />
 	public async Task<IReadOnlyList<BranchRef>?> ListAsync(string directory, CancellationToken cancellationToken)
 	{
@@ -33,6 +46,10 @@ public sealed class RefResolver(IGitRunner runner, IOptions<GitBranchStateCacheO
 				// Reading local refs never touches the network, so the fetch budget would be the wrong
 				// bound for it. Anything slower than a probe here means the volume is in trouble.
 				Timeout = options.Value.ProbeTimeout,
+
+				// git does not require ref names to be UTF-8, and one that is not must not take every
+				// other branch down with it. Each line is decoded on its own below.
+				ReadStandardOutputAsBytes = true,
 			},
 			cancellationToken).ConfigureAwait(false);
 
@@ -41,7 +58,53 @@ public sealed class RefResolver(IGitRunner runner, IOptions<GitBranchStateCacheO
 			return null;
 		}
 
-		return Parse(result.StandardOutput, options.Value.RemoteName);
+		string decoded = DecodeLines(
+			result.StandardOutput,
+			undecodable => RefLog.UndecodableRefSkipped(_logger, directory, undecodable));
+
+		return Parse(decoded, options.Value.RemoteName);
+	}
+
+	/// <summary>
+	/// Decodes <c>for-each-ref</c> output read as bytes, one line at a time, dropping any line that is
+	/// not UTF-8.
+	/// </summary>
+	/// <remarks>
+	/// A name that cannot be decoded cannot be written into a response faithfully either, so the ref
+	/// is skipped rather than served under a name with a replacement character in it, which would look
+	/// fine and match nothing the client has.
+	/// </remarks>
+	/// <param name="output">The output, one character per byte.</param>
+	/// <param name="skipped">Told the hex of each ref name that was dropped.</param>
+	/// <returns>The lines that decoded, each followed by a line feed.</returns>
+	internal static string DecodeLines(string output, Action<string> skipped)
+	{
+		Ensure.NotNull(output);
+		Ensure.NotNull(skipped);
+
+		StringBuilder decoded = new();
+
+		foreach (string line in output.Split('\n'))
+		{
+			if (line.Length == 0)
+			{
+				continue;
+			}
+
+			byte[] bytes = Encoding.Latin1.GetBytes(line);
+
+			try
+			{
+				decoded.Append(StrictUtf8.GetString(bytes)).Append('\n');
+			}
+			catch (DecoderFallbackException)
+			{
+				int separator = Array.IndexOf(bytes, (byte)' ');
+				skipped(Convert.ToHexString(bytes, separator + 1, bytes.Length - separator - 1));
+			}
+		}
+
+		return decoded.ToString();
 	}
 
 	/// <summary>