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 +}