diff --git a/ICSharpCode.ILSpyX/Settings/ILSpySettings.cs b/ICSharpCode.ILSpyX/Settings/ILSpySettings.cs index fad106dc4..ab0abd0fa 100644 --- a/ICSharpCode.ILSpyX/Settings/ILSpySettings.cs +++ b/ICSharpCode.ILSpyX/Settings/ILSpySettings.cs @@ -17,6 +17,7 @@ // DEALINGS IN THE SOFTWARE. using System; +using System.Globalization; using System.IO; using System.Xml; using System.Xml.Linq; @@ -113,6 +114,11 @@ namespace ICSharpCode.ILSpyX.Settings } catch (XmlException) { + // The file cannot be parsed, and it is about to be replaced by the one written + // below. It is the only copy of whatever the user had - assembly lists above all - + // and a file that fails to parse is usually one typo away from readable, so it is + // kept instead of overwritten (issue #2919). + KeepUnreadableFile(config); doc = new XDocument(new XElement("ILSpy")); } doc.Root!.SetAttributeValue("version", DecompilerVersionInfo.Major + "." + DecompilerVersionInfo.Minor + "." + DecompilerVersionInfo.Build + "." + DecompilerVersionInfo.Revision); @@ -122,6 +128,33 @@ namespace ICSharpCode.ILSpyX.Settings } } + /// + /// Moves a settings file that could not be read out of the way, under a name that says what + /// it is and never replaces an earlier one - two bad starts in a row must not cost the copy + /// that still has the data. + /// + static void KeepUnreadableFile(string config) + { + try + { + if (!File.Exists(config)) + return; + string baseName = config + ".broken-" + DateTime.Now.ToString("yyyyMMdd-HHmmss", CultureInfo.InvariantCulture); + string kept = baseName; + for (int attempt = 1; File.Exists(kept); attempt++) + kept = baseName + "-" + attempt; + File.Move(config, kept); + } + catch (IOException) + { + // Keeping the file is what matters; failing to name the copy must not stop settings + // from being saved. + } + catch (UnauthorizedAccessException) + { + } + } + static string GetConfigFile() { return SettingsFilePathProvider?.Invoke() ?? throw new ArgumentNullException(nameof(SettingsFilePathProvider)); diff --git a/ILSpy.Tests/Settings/MalformedSettingsFileTests.cs b/ILSpy.Tests/Settings/MalformedSettingsFileTests.cs new file mode 100644 index 000000000..11adaf852 --- /dev/null +++ b/ILSpy.Tests/Settings/MalformedSettingsFileTests.cs @@ -0,0 +1,127 @@ +// Copyright (c) 2026 Siegfried Pammer +// +// Permission is hereby granted, free of charge, to any person obtaining a copy of this +// software and associated documentation files (the "Software"), to deal in the Software +// without restriction, including without limitation the rights to use, copy, modify, merge, +// publish, distribute, sublicense, and/or sell copies of the Software, and to permit persons +// to whom the Software is furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in all copies or +// substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLIED, +// INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS FOR A PARTICULAR +// PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE +// FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR +// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER +// DEALINGS IN THE SOFTWARE. + +using System; +using System.IO; +using System.Linq; +using System.Xml.Linq; + +using ICSharpCode.ILSpyX.Settings; + +using NUnit.Framework; + +namespace ICSharpCode.ILSpy.Tests.Settings; + +/// +/// A settings file ILSpy cannot parse is the only copy of everything the user put in it - assembly +/// lists above all, which people build up over years and edit by hand. Replacing it with defaults +/// on the next save destroys that, without a word and without a copy (issue #2919). +/// +[TestFixture] +public class MalformedSettingsFileTests +{ + const string MalformedSettings = """ + + + C:\important\one.dll + C:\important\two.dll + + """; + + Func? savedProvider; + string tempDir = ""; + string configFile = ""; + + [SetUp] + public void SetUp() + { + savedProvider = ILSpySettings.SettingsFilePathProvider; + tempDir = Path.Combine(Path.GetTempPath(), "ILSpyMalformedSettings_" + Guid.NewGuid().ToString("N")); + Directory.CreateDirectory(tempDir); + configFile = Path.Combine(tempDir, "ILSpy.xml"); + ILSpySettings.SettingsFilePathProvider = () => configFile; + } + + [TearDown] + public void TearDown() + { + ILSpySettings.SettingsFilePathProvider = savedProvider; + try + { + Directory.Delete(tempDir, recursive: true); + } + catch + { + // best effort + } + } + + static void SaveSomething() + { + ILSpySettings.Load().SaveSettings(new XElement("TestSection", new XAttribute("value", "1"))); + } + + [Test] + public void WhatCouldNotBeParsedIsKept() + { + File.WriteAllText(configFile, MalformedSettings); + + SaveSomething(); + + var backups = Directory.GetFiles(tempDir).Where(f => f != configFile).ToList(); + Assert.That(backups, Has.Count.EqualTo(1), "the unparseable file is kept alongside the new one"); + Assert.That(File.ReadAllText(backups[0]), Is.EqualTo(MalformedSettings), "kept as it was"); + } + + [Test] + public void SettingsAreStillWritten() + { + File.WriteAllText(configFile, MalformedSettings); + + SaveSomething(); + + var written = XDocument.Load(configFile); + Assert.That(written.Root!.Element("TestSection"), Is.Not.Null, "the save itself goes through"); + } + + [Test] + public void AnEarlierBackupIsNotOverwritten() + { + // Two bad startups in a row must not cost the first file, which is the one with the data. + File.WriteAllText(configFile, MalformedSettings); + SaveSomething(); + File.WriteAllText(configFile, ""); + SaveSomething(); + + var backups = Directory.GetFiles(tempDir).Where(f => f != configFile).ToList(); + Assert.That(backups, Has.Count.EqualTo(2)); + Assert.That(backups.Select(File.ReadAllText), Has.One.EqualTo(MalformedSettings)); + } + + [Test] + public void AGoodFileIsLeftAlone() + { + File.WriteAllText(configFile, ""); + + SaveSomething(); + + Assert.That(Directory.GetFiles(tempDir), Has.Length.EqualTo(1), "nothing is copied aside"); + var written = XDocument.Load(configFile); + Assert.That(written.Root!.Element("Existing"), Is.Not.Null, "and the settings are still there"); + } +}