From 5e52d383ed4941a27249aae717f1e63a19c5e92c Mon Sep 17 00:00:00 2001 From: Simon Cropp Date: Sat, 22 Aug 2026 13:05:30 +1000 Subject: [PATCH] Do not read a repeated tool name as an uninstalled one ToolsOrder.Sort looked each requested tool up in Definitions.Tools, which holds every tool whether it is on the machine or not - so the lookup never failed for a tool that was merely not installed, and throwForNoTool could not mean what it said. What did make the lookup fail was a repeat, since the first occurrence removed the definition from the list. So DiffEngine_ToolOrder=VisualStudio,VisualStudio threw "is not installed" for a tool that was, and it did so from DiffTools' static constructor - which makes it permanent. Every later use of DiffTools in that process is a TypeInitializationException, for a typo in an environment variable. Distinct the order, since a repeated name is a typo and not a request for two. And move the throw to InitTools, where being installed is actually decided, keeping it to tools the caller named rather than to the remainder that is appended anyway. The first version of the duplicate test named VisualStudio and failed for the right reason - it is not installed here - so it now picks an installed tool from DiffTools.Resolved. --- .../ToolOrderDuplicateTests.cs | 57 +++++++++++++++++++ src/DiffEngine/DiffTools_Add.cs | 14 ++++- src/DiffEngine/ToolsOrder.cs | 27 +++++---- 3 files changed, 85 insertions(+), 13 deletions(-) create mode 100644 src/DiffEngine.Tests/ToolOrderDuplicateTests.cs diff --git a/src/DiffEngine.Tests/ToolOrderDuplicateTests.cs b/src/DiffEngine.Tests/ToolOrderDuplicateTests.cs new file mode 100644 index 00000000..5410db3b --- /dev/null +++ b/src/DiffEngine.Tests/ToolOrderDuplicateTests.cs @@ -0,0 +1,57 @@ +/// +/// A repeated name in DiffEngine_ToolOrder. +/// +/// Sort looked each requested tool up in Definitions, which holds every tool whether it is +/// installed or not - so the lookup only ever failed on a second occurrence, because the first had +/// removed it from the list. That was then reported as "is not installed", which was untrue, and +/// since DiffTools resolves the order in a static constructor it was also permanent: every later +/// use of DiffTools in that process threw TypeInitializationException. +/// +/// +[NotInParallel] +public class ToolOrderDuplicateTests : + IDisposable +{ + [Test] + public async Task ARepeatedToolIsNotAnUninstalledOne() + { + // An installed one, chosen from what this machine actually has, since throwForNoTool is + // the flag under test and naming an absent tool would throw for the right reason + var installed = DiffTools.Resolved.FirstOrDefault(_ => _.Tool != null)?.Tool; + if (installed == null) + { + // No built in tool resolved here, so there is nothing to repeat + return; + } + + DiffTools.UseOrder(true, installed.Value, installed.Value); + + // Still ordered, rather than dropped along with the duplicate + await Assert.That(DiffTools.Resolved.Any(_ => _.Tool == installed)).IsTrue(); + } + + /// + /// And the flag still means what it says for a tool that genuinely is not here. + /// + [Test] + public async Task AnUninstalledToolStillThrows() + { + var installed = DiffTools.Resolved + .Where(_ => _.Tool != null) + .Select(_ => _.Tool!.Value) + .ToHashSet(); + + var absent = Enum.GetValues().FirstOrDefault(_ => !installed.Contains(_)); + if (installed.Count == Enum.GetValues().Length) + { + // Every tool installed, which no real machine is + return; + } + + await Assert.That(() => DiffTools.UseOrder(true, absent)) + .Throws(); + } + + public void Dispose() => + DiffTools.Reset(); +} diff --git a/src/DiffEngine/DiffTools_Add.cs b/src/DiffEngine/DiffTools_Add.cs index 9b9af4db..2ca26ac6 100644 --- a/src/DiffEngine/DiffTools_Add.cs +++ b/src/DiffEngine/DiffTools_Add.cs @@ -118,10 +118,10 @@ static void InitTools(bool throwForNoTool, IEnumerable order) firstTextTool = null; resolved.Clear(); - foreach (var definition in ToolsOrder.Sort(throwForNoTool, order).Reverse()) + foreach (var (definition, requested) in ToolsOrder.Sort(order).Reverse()) { var tool = definition.Tool; - AddTool( + var added = AddTool( tool.ToString(), tool, definition.AutoRefresh, @@ -133,6 +133,16 @@ static void InitTools(bool throwForNoTool, IEnumerable order) definition.UseShellExecute, definition.CreateNoWindow, definition.KillLockingProcess); + + // Here rather than in Sort, because this is where being installed is decided: Sort + // works from Definitions, which holds every tool whether it is on the machine or not, + // so it could never answer this question + if (added == null && + requested && + throwForNoTool) + { + throw new($"`DiffEngine_ToolOrder` is configured to use '{tool}' but it is not installed."); + } } custom.Reverse(); diff --git a/src/DiffEngine/ToolsOrder.cs b/src/DiffEngine/ToolsOrder.cs index 2bac6e42..58904c02 100644 --- a/src/DiffEngine/ToolsOrder.cs +++ b/src/DiffEngine/ToolsOrder.cs @@ -1,28 +1,33 @@ static class ToolsOrder { - public static IEnumerable Sort(bool throwForNoTool, IEnumerable order) + /// + /// The requested tools first, in the order asked for, then everything else. Each is flagged + /// with whether it was asked for, so the caller can tell a tool the user named and could not + /// have from one that simply is not installed on this machine and was never mentioned. + /// + public static IEnumerable<(Definition Definition, bool Requested)> Sort(IEnumerable order) { var allTools = Definitions.Tools.ToList(); - foreach (var diffTool in order) + // Distinct, because a repeated name is a typo rather than a request for two of something. + // Without it the second occurrence found nothing - the first had already removed it - and + // that was reported as "is not installed", which was both untrue and, from a static + // constructor, permanent: DiffEngine_ToolOrder=VisualStudio,VisualStudio turned every + // later use of DiffTools into a TypeInitializationException + foreach (var diffTool in order.Distinct()) { var definition = allTools.SingleOrDefault(_ => _.Tool == diffTool); if (definition == null) { - if (!throwForNoTool) - { - continue; - } - - throw new($"`DiffEngine_ToolOrder` is configured to use '{diffTool}' but it is not installed."); + continue; } - yield return definition; + yield return (definition, true); allTools.Remove(definition); } foreach (var definition in allTools) { - yield return definition; + yield return (definition, false); } } -} \ No newline at end of file +}