From daad62d8798056b5e9f24b6aa97e58c6745ee6f9 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 7 Oct 2026 18:29:47 +0000 Subject: [PATCH 1/2] Report inputs that share an output name, and reject --output equal to --input [patch] Every output is .png, so save.png and save.bmp wrote the same file: the later one won while the summary counted both. ProcessDirectory now remembers the names it has written in a run and counts a collision as a failed file. Validate rejects an output directory that is the input directory, which used to replace the source icons with their masks and exit 0. Fixes #120 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01WyG38ppCkkDDCmkSnea826 --- CLAUDE.md | 13 +++++++--- IconHelper.Test/ArgumentsTests.cs | 16 +++++++++++++ IconHelper.Test/ProcessDirectoryTests.cs | 25 ++++++++++++++++++++ IconHelper/Arguments.cs | 30 ++++++++++++++++++++---- IconHelper/IconHelper.cs | 16 ++++++++++++- README.md | 6 +++-- 6 files changed, 96 insertions(+), 10 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 82a60eb..38f5d56 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -151,14 +151,12 @@ help text and exited 0. Do not "fix" these silently, they are documented in the README as limitations: -- Output extensions are rewritten to `.png`, so two inputs sharing a base name (`a.png`, `a.jpg`) - collide and the later one wins. - A run that fails every single file still exits `2`, the same as a run that failed one file. The exit status says "something failed", the summary line says how much. ### Fixed Bugs Worth Knowing About -Both are covered by regression tests. Do not reintroduce them. +Each is covered by regression tests. Do not reintroduce them. - **Bounding-box off-by-one.** The crop used `right - left`, but `right` and `bottom` are *inclusive* indices of the last opaque pixel, so the span needs `+ 1`. Every icon used to lose its rightmost @@ -181,6 +179,15 @@ Both are covered by regression tests. Do not reintroduce them. `PaddingThatFitsIsAppliedExactlyAndNotClamped` and `ProcessDirectoryTests.SmallArtworkWithValidatedPaddingDoesNotFailTheBatch`. +- **Inputs overwrote each other, and the output could overwrite the input.** Every output is + `.png`, so `save.png` and `save.bmp` wrote the same file: the later one won while the + summary counted both, and `--output` equal to `--input` replaced the source icons with their masks + and exited 0. `ProcessDirectory` now remembers the names it has written in a run and counts a + collision as a failed file, and `Validate` rejects an output directory that is the input directory + (`Arguments.PathComparison` decides case sensitivity per platform). Pinned by + `ProcessDirectoryTests.InputsSharingABaseNameDoNotOverwriteEachOther` and + `ArgumentsTests.ValidateRejectsAnOutputThatIsTheInputDirectory`. + ## Testing `IconHelper.Test` is an MSTest project (`MSTest.Sdk`, Microsoft Testing Platform). Both it and the diff --git a/IconHelper.Test/ArgumentsTests.cs b/IconHelper.Test/ArgumentsTests.cs index dee59b1..ca3f708 100644 --- a/IconHelper.Test/ArgumentsTests.cs +++ b/IconHelper.Test/ArgumentsTests.cs @@ -198,6 +198,22 @@ public void ValidateAcceptsAnOutputDirectoryThatDoesNotExistYet() Assert.IsTrue(args.Validate(out _), "The output directory is created on demand, so it need not exist yet."); } + [TestMethod] + public void ValidateRejectsAnOutputThatIsTheInputDirectory() + { + // Every output is .png, so writing into the input directory replaces the .png + // sources with their own masks. + using TempDirectory temp = new(); + Arguments args = ValidArguments(temp); + args.OutputPath = args.InputPath + Path.DirectorySeparatorChar; + + bool valid = args.Validate(out Collection errors); + + Assert.IsFalse(valid); + Assert.HasCount(1, errors); + Assert.Contains("--output must not be the --input directory", errors[0]); + } + [TestMethod] public void ValidateRejectsEmptyPaths() { diff --git a/IconHelper.Test/ProcessDirectoryTests.cs b/IconHelper.Test/ProcessDirectoryTests.cs index a107b19..affadef 100644 --- a/IconHelper.Test/ProcessDirectoryTests.cs +++ b/IconHelper.Test/ProcessDirectoryTests.cs @@ -68,6 +68,31 @@ public void RewritesTheOutputExtensionToPng() Assert.IsFalse(File.Exists(Path.Combine(output, "photo.jpg")), "The original extension should not be reused."); } + [TestMethod] + public void InputsSharingABaseNameDoNotOverwriteEachOther() + { + // Both map to save.png. Writing both left only the later file on disk while the summary + // counted two, so the second is reported as failed instead and the first is kept. + using TempDirectory temp = new(); + string input = temp.Combine("in"); + string output = temp.Combine("out"); + Directory.CreateDirectory(input); + WritePng(Path.Combine(input, "save.png"), 64); + + using (Image bitmap = TestImages.Blank(64, 64)) + { + TestImages.FillRect(bitmap, 16, 16, 32, 32, new Rgba32(255, 255, 255, 255)); + bitmap.SaveAsBmp(Path.Combine(input, "save.bmp")); + } + + BatchResult result = IconHelper.ProcessDirectory(ArgumentsFor(input, output), NamedColors.White); + + Assert.AreEqual(1, result.Written, "Only one output file can exist for the shared name."); + Assert.AreEqual(1, result.Failed, "The colliding input must be reported, not silently overwritten."); + Assert.HasCount(result.Written, Directory.GetFiles(output), "The written count must match the files on disk."); + Assert.AreEqual(IconHelper.ExitSomeFilesFailed, IconHelper.ExitCodeFor(result)); + } + [TestMethod] public void SkipsFilesAlreadyMarkedAsGenerated() { diff --git a/IconHelper/Arguments.cs b/IconHelper/Arguments.cs index 0fc97ca..8cf4902 100644 --- a/IconHelper/Arguments.cs +++ b/IconHelper/Arguments.cs @@ -55,14 +55,23 @@ internal bool Validate(out Collection errors) errors.Add("Padding must be less than half the size of the image."); } - if (!TryResolveInput(out _, out string? inputError)) + bool inputResolved = TryResolveInput(out AbsoluteDirectoryPath? input, out string? inputError); + if (!inputResolved) { - errors.Add(inputError); + errors.Add(inputError!); } - if (!TryResolveOutput(out _, out string? outputError)) + bool outputResolved = TryResolveOutput(out AbsoluteDirectoryPath? output, out string? outputError); + if (!outputResolved) { - errors.Add(outputError); + errors.Add(outputError!); + } + + // Every output is a .png named after its input, so writing into the input directory replaces + // any .png source with its own recoloured mask, and the original artwork is gone. + if (inputResolved && outputResolved && IsSameDirectory(input!, output!)) + { + errors.Add($"--output must not be the --input directory, or the source icons are overwritten: {(string)output!}"); } if (!ColorParser.TryParse(Color, out _)) @@ -73,6 +82,19 @@ internal bool Validate(out Collection errors) return errors.Count == 0; } + /// + /// How file and directory names compare on this platform. Windows and macOS file systems are case + /// insensitive by default, so Icons and icons name the same directory there. + /// + internal static StringComparison PathComparison { get; } = + OperatingSystem.IsWindows() || OperatingSystem.IsMacOS() ? StringComparison.OrdinalIgnoreCase : StringComparison.Ordinal; + + private static bool IsSameDirectory(AbsoluteDirectoryPath first, AbsoluteDirectoryPath second) => + string.Equals( + Path.TrimEndingDirectorySeparator((string)first), + Path.TrimEndingDirectorySeparator((string)second), + PathComparison); + /// /// Resolves to an absolute directory that must already exist. /// diff --git a/IconHelper/IconHelper.cs b/IconHelper/IconHelper.cs index 56f9478..658f0de 100644 --- a/IconHelper/IconHelper.cs +++ b/IconHelper/IconHelper.cs @@ -113,6 +113,11 @@ internal static BatchResult ProcessDirectory(Arguments args, Color color) int processed = 0; int failed = 0; + + // Output name to the input that produced it. Every output is .png, so inputs that + // share a base name (save.png, save.bmp) map to one file, and writing both would leave only + // the last while counting two. + Dictionary written = new(StringComparer.FromComparison(Arguments.PathComparison)); System.Collections.ObjectModel.Collection files = Directory.GetFiles(inputDirectory, "*").ToCollection(); foreach (string? file in files) { @@ -125,6 +130,14 @@ internal static BatchResult ProcessDirectory(Arguments args, Color color) continue; } + string outputName = $"{Path.GetFileNameWithoutExtension(file)}.png"; + if (written.TryGetValue(outputName, out string? earlierInput)) + { + Console.WriteLine($"Failed to process {file}: its output {outputName} was already written from {earlierInput}."); + failed++; + continue; + } + try { Console.WriteLine($"Processing {file}..."); @@ -135,10 +148,11 @@ internal static BatchResult ProcessDirectory(Arguments args, Color color) // Always write a .png extension, since the encoder always writes PNG data. FileName // rejects anything carrying a directory separator, and the / operator composes the // two into an absolute file path. - FileName outputFileName = FileName.Create($"{Path.GetFileNameWithoutExtension(file)}.png"); + FileName outputFileName = FileName.Create(outputName); AbsoluteFilePath outputFilePath = outputDirectory / outputFileName; image.SaveAsPng(outputFilePath, Encoder); + written.Add(outputName, file); processed++; } catch (Exception e) diff --git a/README.md b/README.md index 5cad723..d5720e9 100644 --- a/README.md +++ b/README.md @@ -293,8 +293,10 @@ for how many succeeded. ## Notes and Limitations - Output is always PNG, and the extension is rewritten to match, so `logo.jpg` becomes `logo.png`. If - the input directory holds two files with the same base name but different extensions, the later one - overwrites the earlier. + the input directory holds two files with the same base name but different extensions, the first one + processed is written and the other is reported as failed, so the run exits `2`. Rename one of them. +- `--output` must not be the `--input` directory, because the outputs would replace the `.png` + sources. That is rejected with exit code `1`. - Input formats are whatever ImageSharp can decode (PNG, JPEG, BMP, GIF, TGA, TIFF, WebP, PBM, QOI). Vector formats such as SVG are not supported. - The tool only ever shrinks artwork. Passing a `--size` larger than the source icon leaves it at its From 7ec2f6f50ce9e2ac8cfed959b1d2e3b5409b20e9 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 7 Oct 2026 18:32:04 +0000 Subject: [PATCH 2/2] Use Path.Join for the collision test's input paths Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01WyG38ppCkkDDCmkSnea826 --- IconHelper.Test/ProcessDirectoryTests.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/IconHelper.Test/ProcessDirectoryTests.cs b/IconHelper.Test/ProcessDirectoryTests.cs index affadef..757c8b2 100644 --- a/IconHelper.Test/ProcessDirectoryTests.cs +++ b/IconHelper.Test/ProcessDirectoryTests.cs @@ -77,12 +77,12 @@ public void InputsSharingABaseNameDoNotOverwriteEachOther() string input = temp.Combine("in"); string output = temp.Combine("out"); Directory.CreateDirectory(input); - WritePng(Path.Combine(input, "save.png"), 64); + WritePng(Path.Join(input, "save.png"), 64); using (Image bitmap = TestImages.Blank(64, 64)) { TestImages.FillRect(bitmap, 16, 16, 32, 32, new Rgba32(255, 255, 255, 255)); - bitmap.SaveAsBmp(Path.Combine(input, "save.bmp")); + bitmap.SaveAsBmp(Path.Join(input, "save.bmp")); } BatchResult result = IconHelper.ProcessDirectory(ArgumentsFor(input, output), NamedColors.White);