Strip a leading byte order mark once, not twice, for an encoding with a preamble [patch]

RunCommand.Test/RunCommandTests.cs

@@ -3,6 +3,7 @@
 namespace ktsu.RunCommand.Test;
 
 using System.Diagnostics;
+using System.Globalization;
 using System.Runtime.CompilerServices;
 using System.Text;
 using System.Runtime.InteropServices;
@@ -1185,6 +1186,57 @@ public void AUtf8ByteOrderMarkIsStrippedFromTheStartOfOutput()
 		Assert.AreEqual("hello", RunOverPath(path, StrictUtf8));
 	}
 
+	/// <summary>
+	/// Returns the encoding a byte-order-mark test names, along with that encoding's own BOM.
+	/// </summary>
+	/// <remarks>
+	/// Named rather than passed because <see cref="DataRowAttribute"/> only takes constants. The
+	/// cases split on whether the encoding has a preamble: <see cref="StreamReader"/> skips a
+	/// matching preamble itself whatever detectEncodingFromByteOrderMarks says, so one with a
+	/// preamble must not be stripped a second time by the library.
+	/// </remarks>
+	private static (Encoding Encoding, byte[] Bom) GetByteOrderMarkCase(string name) => name switch
+	{
+		"Encoding.UTF8" => (Encoding.UTF8, [0xEF, 0xBB, 0xBF]),
+		"UTF8Encoding(false)" => (new UTF8Encoding(false), [0xEF, 0xBB, 0xBF]),
+		"Encoding.Unicode" => (Encoding.Unicode, [0xFF, 0xFE]),
+		"UnicodeEncoding(false, false)" => (new UnicodeEncoding(bigEndian: false, byteOrderMark: false), [0xFF, 0xFE]),
+		_ => throw new ArgumentOutOfRangeException(nameof(name), name, "Unknown encoding case."),
+	};
+
+	[TestMethod]
+	[DataRow("Encoding.UTF8")]
+	[DataRow("UTF8Encoding(false)")]
+	[DataRow("Encoding.Unicode")]
+	[DataRow("UnicodeEncoding(false, false)")]
+	public void OnlyTheByteOrderMarkIsStrippedWhenARealU_FEFFFollowsIt(string encodingName)
+	{
+		// A BOM, then U+FEFF that is part of the output, then "A". The BOM goes and the content
+		// stays, whichever of two equivalent encodings the caller passed. With a preamble the reader
+		// dropped the BOM and the library then dropped the real U+FEFF after it.
+		(Encoding encoding, byte[] bom) = GetByteOrderMarkCase(encodingName);
+		byte[] bytes = [.. bom, .. encoding.GetBytes("\uFEFFA")];
+		string path = WriteBytesForTest(bytes, $"{nameof(OnlyTheByteOrderMarkIsStrippedWhenARealU_FEFFFollowsIt)}.{new string([.. encodingName.Where(char.IsLetterOrDigit)])}");
+
+		string output = RunOverPath(path, encoding);
+
+		Assert.AreEqual("FEFF 0041", string.Join(' ', output.Select(c => ((int)c).ToString("X4", CultureInfo.InvariantCulture))));
+	}
+
+	[TestMethod]
+	[DataRow("Encoding.UTF8")]
+	[DataRow("UTF8Encoding(false)")]
+	[DataRow("Encoding.Unicode")]
+	[DataRow("UnicodeEncoding(false, false)")]
+	public void ALoneByteOrderMarkIsStrippedWhicheverEncodingReadsIt(string encodingName)
+	{
+		(Encoding encoding, byte[] bom) = GetByteOrderMarkCase(encodingName);
+		byte[] bytes = [.. bom, .. encoding.GetBytes("hello")];
+		string path = WriteBytesForTest(bytes, $"{nameof(ALoneByteOrderMarkIsStrippedWhicheverEncodingReadsIt)}.{new string([.. encodingName.Where(char.IsLetterOrDigit)])}");
+
+		Assert.AreEqual("hello", RunOverPath(path, encoding));
+	}
+
 	[TestMethod]
 	public void ADecodeFailureIsNotDiscardedWhenTheProcessKeepsRunning()
 	{

RunCommand/AsyncProcessStreamReader.cs

@@ -32,6 +32,12 @@
 	private readonly StreamReader errorStream =
 		new(process.StandardError.BaseStream, outputHandler.Encoding, detectEncodingFromByteOrderMarks: false);
 
+	// StreamReader skips the encoding's own preamble at the start of a stream whatever
+	// detectEncodingFromByteOrderMarks says, so for an encoding that has one (Encoding.UTF8, the
+	// default, and Encoding.Unicode among them) the BOM is already gone by the time a chunk arrives.
+	// Stripping again there removed a U+FEFF that was real output.
+	private readonly bool readerSkipsByteOrderMark = outputHandler.Encoding.GetPreamble().Length > 0;
+
 	public void Dispose()
 	{
 		outputStream.Dispose();
@@ -66,7 +72,7 @@
 		TaskCompletionSource<bool> cancellationSource = new(TaskCreationOptions.RunContinuationsAsynchronously);
 
 		using CancellationTokenRegistration registration = cancellationToken.Register(
 			static state => ((TaskCompletionSource<bool>)state!).TrySetResult(true),
 			cancellationSource);
 
 		Task cancelled = cancellationSource.Task;
@@ -204,11 +210,13 @@
 	/// Drops a byte order mark from the front of a stream's first chunk.
 	/// </summary>
 	/// <remarks>
-	/// Turning off the detection above also turned off the stripping that came with it, and a
-	/// leading U+FEFF in captured output is a change no caller asked for. Every encoding decodes its
-	/// own BOM to U+FEFF, so dropping that one character covers each of them without guessing at the
-	/// encoding. A BOM belonging to a different encoding no longer decodes to U+FEFF, which is
-	/// exactly the case that should surface as mis-decoded bytes rather than be silently honoured.
+	/// Turning off the detection above also turned off the stripping that came with it for an
+	/// encoding with no preamble, and a leading U+FEFF in captured output is a change no caller asked
+	/// for. Every encoding decodes its own BOM to U+FEFF, so dropping that one character covers each
+	/// of them without guessing at the encoding. A BOM belonging to a different encoding no longer
+	/// decodes to U+FEFF, which is exactly the case that should surface as mis-decoded bytes rather
+	/// than be silently honoured. An encoding with a preamble has had its BOM skipped by the reader
+	/// already, so a U+FEFF at the front of its first chunk is content and is kept.
 	/// </remarks>
 	/// <param name="data">The chunk just read.</param>
 	/// <param name="isStandardOutput">Whether the chunk came from standard output.</param>
@@ -226,7 +234,7 @@
 			errorHasEmitted = true;
 		}
 
-		if (hasEmitted || data[0] != ByteOrderMark)
+		if (hasEmitted || readerSkipsByteOrderMark || data[0] != ByteOrderMark)
 		{
 			return data;
 		}