From 5e2b4e44f38af03ba4dd1418a672f3fe411feb20 Mon Sep 17 00:00:00 2001 From: James Maes Date: Tue, 10 Nov 2026 14:19:24 -0600 Subject: [PATCH 1/2] fix(recovery): plans are checked by where paths end up; suggestions are valid Two defects in how rename choices for Windows-invalid paths were prepared before they reached the engine: - The Resolve dialog's pre-flight validated each replacement as typed. A renamed entry inside a renamed folder is written with the folder's original name in front, so the check saw the invalid name and refused a plan the engine accepts; only skipping the file got past it. - --sanitize-paths trusted the suggested names. Two paths can be given the same suggestion, and some suggestions were themselves invalid: "x?." became "x?", and "COM3.txt" became "COM3.txt_", which is as reserved as the name it replaces. The repository then failed to sanitize. Now: - PathRecovery.EffectivePathOf says where a path ends up once the renames of it and of the folders above it are applied (nowhere, when it or a folder above it is skipped). The pre-flight validates those places. - Every suggested name is put through the rules until it passes, and a reserved device name is fixed in its base name ("COM3_.txt"). - The automatic plan moved next to the planner (PathRecoveryPlanner.FromSuggestions) and skips what its renames cannot place, so its result always passes the pre-flight. Closes #149 Co-Authored-By: Claude Fable 5.2 Signed-off-by: James Maes --- docs/CLI.md | 8 +- gclo.Cli/SyncCommand.cs | 31 +--- .../PathRecoveryEffectivePathTests.cs | 97 +++++++++++ gclo.Engine.Tests/PathRecoveryPlannerTests.cs | 157 ++++++++++++++++++ .../WindowsPathValidationTests.cs | 44 ++++- gclo.Engine/PathRecovery.cs | 35 +++- gclo.Engine/WindowsPathValidator.cs | 29 +++- gclo.ViewModels/PathRecoveryPlanner.cs | 94 ++++++++++- 8 files changed, 452 insertions(+), 43 deletions(-) create mode 100644 gclo.Engine.Tests/PathRecoveryEffectivePathTests.cs diff --git a/docs/CLI.md b/docs/CLI.md index 92a6c75..16eadac 100644 --- a/docs/CLI.md +++ b/docs/CLI.md @@ -131,9 +131,11 @@ performs a plain checkout and `--sanitize-paths` has nothing to do. With `--sanitize-paths`, gclo checks such a repository out anyway: - Each offending path is renamed on disk to a safe suggested name - (`aux` → `aux_`, `spec?.md` → `spec_.md`). -- Paths with no safe automatic rename (case-only collisions) are **skipped**: - they are not materialized on disk. + (`aux` → `aux_`, `com3.txt` → `com3_.txt`, `spec?.md` → `spec_.md`). +- Paths with no safe automatic rename are **skipped**: they are not + materialized on disk. That covers case-only collisions, and paths whose + suggested names would collide with each other (`a:b` and `a?b` would both + become `a_b`). - A note listing the renames goes to stderr (text modes) and to the activity log, and the repository counts as **cloned** — the exit code stays `0`. The human summary calls the count out: diff --git a/gclo.Cli/SyncCommand.cs b/gclo.Cli/SyncCommand.cs index 70406b3..04a30f3 100644 --- a/gclo.Cli/SyncCommand.cs +++ b/gclo.Cli/SyncCommand.cs @@ -540,7 +540,7 @@ internal static class SyncCommand try { cancellationToken.ThrowIfCancellationRequested(); - PathRecovery recovery = BuildRecovery(pending.Paths); + PathRecovery recovery = PathRecoveryPlanner.FromSuggestions(pending.Paths); await git.ApplyRecoveryAsync(path, recovery, cancellationToken).ConfigureAwait(false); string detail = DescribeRecovery(recovery); @@ -577,35 +577,6 @@ internal static class SyncCommand - /// - /// Builds the automatic recovery: each offending path is renamed to its suggested - /// name; paths without one (e.g. case-only collisions, which a rename to the same - /// suggestion could not resolve) are skipped entirely. - /// - private static PathRecovery BuildRecovery(IReadOnlyList paths) - { - // Skip wins: a path that also has a suggestion-less problem is not materialized. - var skipped = new HashSet( - paths.Where(p => string.IsNullOrEmpty(p.SuggestedName)).Select(p => p.RepoPath), - StringComparer.Ordinal); - - var renames = new Dictionary(StringComparer.Ordinal); - foreach(InvalidPathInfo info in paths) - { - if(info.SuggestedName is not { Length: > 0 } suggestion || skipped.Contains(info.RepoPath)) - { - continue; - } - // The suggestion replaces the last segment — the validator reports each - // offending segment as a repo path ending in that segment. - int slash = info.RepoPath.LastIndexOf('/'); - renames[info.RepoPath] = slash < 0 ? suggestion : info.RepoPath[..(slash + 1)] + suggestion; - } - return new PathRecovery(renames, skipped); - } - - - private static string DescribeRecovery(PathRecovery recovery) => string.Join("; ", recovery.SegmentRenames .Select(r => $"'{r.Key}' -> '{r.Value}'") diff --git a/gclo.Engine.Tests/PathRecoveryEffectivePathTests.cs b/gclo.Engine.Tests/PathRecoveryEffectivePathTests.cs new file mode 100644 index 0000000..777e24a --- /dev/null +++ b/gclo.Engine.Tests/PathRecoveryEffectivePathTests.cs @@ -0,0 +1,97 @@ +/* + * Copyright (c) 2026 James Maes (KofTwentyTwo) + * SPDX-License-Identifier: MIT + */ + +namespace gclo.Engine.Tests; + + +/// +/// is the one definition of where a path +/// ends up once renames and skips are applied (#149). +/// +public sealed class PathRecoveryEffectivePathTests +{ + private static PathRecovery Recovery((string From, string To)[]? renames = null, string[]? skips = null) + => new( + (renames ?? []).ToDictionary(r => r.From, r => r.To, StringComparer.Ordinal), + new HashSet(skips ?? [], StringComparer.Ordinal)); + + + + [Theory] + [InlineData("readme.txt")] + [InlineData("src/app/main.cs")] + public void APathNothingApplyTo_StaysWhereItIs(string path) + { + Assert.Equal(path, Recovery(renames: [("other:file.txt", "other_file.txt")], skips: ["gone"]).EffectivePathOf(path)); + } + + + + [Fact] + public void ARenamedFile_TakesTheLastSegmentOfItsReplacement_UnderItsOwnFolder() + { + PathRecovery recovery = Recovery(renames: [("docs/bad:name.txt", "docs/bad_name.txt")]); + + Assert.Equal("docs/bad_name.txt", recovery.EffectivePathOf("docs/bad:name.txt")); + } + + + + [Fact] + public void OnlyTheLastSegmentOfAReplacementCounts_WhateverFolderItIsWrittenWith() + { + // A replacement cannot move an entry to another folder. + PathRecovery recovery = Recovery(renames: [("docs/bad:name.txt", "somewhere/else/bad_name.txt")]); + + Assert.Equal("docs/bad_name.txt", recovery.EffectivePathOf("docs/bad:name.txt")); + } + + + + [Fact] + public void ARenamedFolder_MovesEverythingUnderIt() + { + PathRecovery recovery = Recovery(renames: [("logs/12:00", "logs/12_00")]); + + Assert.Equal("logs/12_00", recovery.EffectivePathOf("logs/12:00")); + Assert.Equal("logs/12_00/run.log", recovery.EffectivePathOf("logs/12:00/run.log")); + Assert.Equal("logs/12_00/deep/er/run.log", recovery.EffectivePathOf("logs/12:00/deep/er/run.log")); + Assert.Equal("logs/other/run.log", recovery.EffectivePathOf("logs/other/run.log")); + } + + + + [Fact] + public void RenamesOfAFolderAndOfAnEntryInsideIt_Compose() + { + PathRecovery recovery = Recovery(renames: [("logs/12:00", "logs/12_00"), ("logs/12:00/run:1.log", "logs/12:00/run_1.log")]); + + Assert.Equal("logs/12_00/run_1.log", recovery.EffectivePathOf("logs/12:00/run:1.log")); + } + + + + [Fact] + public void ASkippedPath_AndEverythingUnderASkippedFolder_EndsUpNowhere() + { + PathRecovery recovery = Recovery(renames: [("gone/bad:name.txt", "gone/bad_name.txt")], skips: ["gone", "single:file.txt"]); + + Assert.Null(recovery.EffectivePathOf("single:file.txt")); + Assert.Null(recovery.EffectivePathOf("gone")); + Assert.Null(recovery.EffectivePathOf("gone/kept.txt")); + Assert.Null(recovery.EffectivePathOf("gone/bad:name.txt")); + Assert.Equal("gone-too/kept.txt", recovery.EffectivePathOf("gone-too/kept.txt")); + } + + + + [Theory] + [InlineData(null)] + [InlineData("")] + public void NoPath_IsACallerError(string? path) + { + Assert.ThrowsAny(() => Recovery().EffectivePathOf(path!)); + } +} diff --git a/gclo.Engine.Tests/PathRecoveryPlannerTests.cs b/gclo.Engine.Tests/PathRecoveryPlannerTests.cs index 239a7a2..69a6e2b 100644 --- a/gclo.Engine.Tests/PathRecoveryPlannerTests.cs +++ b/gclo.Engine.Tests/PathRecoveryPlannerTests.cs @@ -97,4 +97,161 @@ public sealed class PathRecoveryPlannerTests { Assert.Throws(() => Build(null!)); } + + + + // ---------------------------------------------------------------- destinations, not replacements as typed (#149) + + [Fact] + public void Build_RenamedEntryInsideARenamedFolder_IsSound_AlthoughItsReplacementStillShowsTheOldFolderName() + { + // The dialog writes each replacement with the row's ORIGINAL parent in front; + // the engine applies only the last segment, under the folder's new name. + Plan plan = Build( + [ + new Choice("logs/12:00", "logs/12_00", Skip: false), + new Choice("logs/12:00/run:1.log", "logs/12:00/run_1.log", Skip: false), + ]); + + Assert.Empty(plan.Problems); + Assert.Equal("logs/12_00/run_1.log", plan.Recovery.EffectivePathOf("logs/12:00/run:1.log")); + } + + + + [Fact] + public void Build_RenamedEntryInsideARenamedFolder_IsStillCheckedByItsOwnNewName() + { + Plan plan = Build( + [ + new Choice("logs/12:00", "logs/12_00", Skip: false), + new Choice("logs/12:00/run:1.log", "logs/12:00/run?1.log", Skip: false), + ]); + + string problem = Assert.Single(plan.Problems); + Assert.Contains("'logs/12_00/run?1.log' cannot be used", problem); + } + + + + [Fact] + public void Build_TwoRowsThatEndUpAtTheSamePlace_AreAProblem_EvenWhenTheirReplacementsDiffer() + { + // Different folders as typed, the same folder once the folder's rename applies. + Plan plan = Build( + [ + new Choice("a:b", "a_b", Skip: false), + new Choice("a:b/x:1.txt", "a:b/x_1.txt", Skip: false), + new Choice("a:b/x?1.txt", "a:b/x_1.txt", Skip: false), + ]); + + string problem = Assert.Single(plan.Problems); + Assert.Contains("'a_b/x_1.txt' cannot be used: more than one path maps to this destination", problem); + } + + + + [Fact] + public void Build_RenamedEntryUnderASkippedFolder_IsNotChecked_BecauseItIsNotWritten() + { + Plan plan = Build( + [ + new Choice("skipped:dir", null, Skip: true), + new Choice("skipped:dir/still:bad.txt", "skipped:dir/still?bad.txt", Skip: false), + ]); + + Assert.Empty(plan.Problems); + Assert.Null(plan.Recovery.EffectivePathOf("skipped:dir/still:bad.txt")); + } + + + + // ---------------------------------------------------------------- the automatic plan (#149) + + [Fact] + public void FromSuggestions_RenamesWhatHasASuggestion_AndSkipsWhatHasNone() + { + PathRecovery recovery = PathRecoveryPlanner.FromSuggestions( + [ + new InvalidPathInfo("bad:name.txt", "invalid", "bad_name.txt"), + new InvalidPathInfo("dir/CON", "reserved", "CON_"), + new InvalidPathInfo("README.md", "differs only by case from 'readme.md'", null), + ]); + + Assert.Equal("bad_name.txt", recovery.SegmentRenames["bad:name.txt"]); + Assert.Equal("dir/CON_", recovery.SegmentRenames["dir/CON"]); + Assert.Equal(["README.md"], recovery.SkippedPaths, StringComparer.Ordinal); + } + + + + [Fact] + public void FromSuggestions_APathThatAlsoHasAProblemWithoutASuggestion_IsSkipped() + { + PathRecovery recovery = PathRecoveryPlanner.FromSuggestions( + [ + new InvalidPathInfo("x:y.txt", "invalid", "x_y.txt"), + new InvalidPathInfo("x:y.txt", "differs only by case from 'X:Y.txt'", null), + ]); + + Assert.Empty(recovery.SegmentRenames); + Assert.Equal(["x:y.txt"], recovery.SkippedPaths, StringComparer.Ordinal); + } + + + + [Fact] + public void FromSuggestions_TwoPathsGivenTheSameName_AreSkipped_RatherThanFailingTheApply() + { + PathRecovery recovery = PathRecoveryPlanner.FromSuggestions( + [ + new InvalidPathInfo("a:b", "invalid", "a_b"), + new InvalidPathInfo("a?b", "invalid", "a_b"), + new InvalidPathInfo("fine:name.txt", "invalid", "fine_name.txt"), + ]); + + Assert.Equal("fine_name.txt", Assert.Single(recovery.SegmentRenames).Value); + Assert.Equal(new[] { "a:b", "a?b" }, recovery.SkippedPaths.Order(StringComparer.Ordinal), StringComparer.Ordinal); + // What comes out always passes the pre-flight the dialog runs. + Assert.Empty(Build(recovery.SegmentRenames.Select(r => new Choice(r.Key, r.Value, Skip: false)).ToList()).Problems); + } + + + + [Fact] + public void FromSuggestions_AnInvalidFolderAndAnInvalidEntryInsideIt_AreBothRenamed_AndCompose() + { + PathRecovery recovery = PathRecoveryPlanner.FromSuggestions( + [ + new InvalidPathInfo("logs/12:00", "invalid", "12_00"), + new InvalidPathInfo("logs/12:00/run:1.log", "invalid", "run_1.log"), + ]); + + Assert.Empty(recovery.SkippedPaths); + Assert.Equal("logs/12_00/run_1.log", recovery.EffectivePathOf("logs/12:00/run:1.log")); + } + + + + [Fact] + public void FromSuggestions_TwoEntriesCollidingInsideARenamedFolder_AreSkipped_AndTheFolderKeepsItsRename() + { + PathRecovery recovery = PathRecoveryPlanner.FromSuggestions( + [ + new InvalidPathInfo("a:b", "invalid", "a_b"), + new InvalidPathInfo("a:b/x:1", "invalid", "x_1"), + new InvalidPathInfo("a:b/x?1", "invalid", "x_1"), + ]); + + Assert.Equal("a_b", Assert.Single(recovery.SegmentRenames).Value); + Assert.Equal(new[] { "a:b/x:1", "a:b/x?1" }, recovery.SkippedPaths.Order(StringComparer.Ordinal), StringComparer.Ordinal); + } + + + + [Fact] + public void FromSuggestions_Null_Throws() + { + Assert.Throws(() => PathRecoveryPlanner.FromSuggestions(null!)); + } } diff --git a/gclo.Engine.Tests/WindowsPathValidationTests.cs b/gclo.Engine.Tests/WindowsPathValidationTests.cs index 7e27944..701a698 100644 --- a/gclo.Engine.Tests/WindowsPathValidationTests.cs +++ b/gclo.Engine.Tests/WindowsPathValidationTests.cs @@ -46,7 +46,12 @@ public sealed class WindowsPathValidationTests : IDisposable [InlineData("trailing-dot.", "trailing space or dot", "trailing-dot")] [InlineData("bad:colon.txt", "invalid on Windows", "bad_colon.txt")] [InlineData("CON", "reserved Windows device name", "CON_")] - [InlineData("COM3.txt", "reserved Windows device name", "COM3.txt_")] + [InlineData("COM3.txt", "reserved Windows device name", "COM3_.txt")] + [InlineData("aux.tar.gz", "reserved Windows device name", "aux_.tar.gz")] + [InlineData("x?.", "trailing space or dot", "x_")] + [InlineData("report?. .", "trailing space or dot", "report_")] + [InlineData("CON .", "trailing space or dot", "CON_")] + [InlineData("...", "trailing space or dot", "_")] public void Validate_FlagsInvalidSegment_WithReasonAndSuggestion( string entryName, string expectedReasonFragment, string expectedSuggestion) { @@ -101,6 +106,43 @@ public sealed class WindowsPathValidationTests : IDisposable + [Theory] + [InlineData("x?.")] + [InlineData(" . ")] + [InlineData("CON")] + [InlineData("con.txt")] + [InlineData("COM1?.log ")] + [InlineData("a:b*c|d")] + [InlineData("nul.")] + [InlineData("LPT9. .")] + public void ValidatePaths_EverySuggestedName_IsItselfValid(string segment) + { + // One rule's fix can trip another; a suggestion that would be rejected is worse + // than none (#149). + InvalidPathInfo info = Assert.Single(WindowsPathValidator.ValidatePaths([segment])); + + Assert.NotNull(info.SuggestedName); + Assert.Empty(WindowsPathValidator.ValidatePaths([info.SuggestedName!])); + } + + + + [Fact] + public void ValidatePaths_AnOverlongReservedName_StillGetsASuggestionThatIsValid() + { + // The reserved-name fix adds a character and the length fix takes one away: + // the two must settle rather than undo each other. + string segment = "CON." + new string('x', 251); + Assert.Equal(255, segment.Length); + + InvalidPathInfo info = Assert.Single(WindowsPathValidator.ValidatePaths([segment])); + + Assert.Empty(WindowsPathValidator.ValidatePaths([info.SuggestedName!])); + Assert.StartsWith("CON_.", info.SuggestedName, StringComparison.Ordinal); + } + + + [Fact] public void Validate_FlagsCaseOnlyCollision() { diff --git a/gclo.Engine/PathRecovery.cs b/gclo.Engine/PathRecovery.cs index d12a3ea..b3ac1d0 100644 --- a/gclo.Engine/PathRecovery.cs +++ b/gclo.Engine/PathRecovery.cs @@ -23,4 +23,37 @@ namespace gclo.Engine; /// public sealed record PathRecovery( IReadOnlyDictionary SegmentRenames, - IReadOnlySet SkippedPaths); + IReadOnlySet SkippedPaths) +{ + /// + /// Where ends up in the working tree once this + /// recovery is applied: each part of the path takes the last segment of its own + /// rename, if it has one, under the place its parent folder ended up. Null when the + /// path, or a folder above it, is skipped. This is the one definition of how + /// renames compose; anything that needs to know a destination before the tree is + /// written (a pre-flight check, an automatic plan) asks here. + /// + public string? EffectivePathOf(string repoPath) + { + ArgumentException.ThrowIfNullOrEmpty(repoPath); + + string[] segments = repoPath.Split('/'); + string original = ""; + string effective = ""; + for(int i = 0; i < segments.Length; i++) + { + original = i == 0 ? segments[i] : $"{original}/{segments[i]}"; + if(SkippedPaths.Contains(original)) + { + return null; + } + + string name = SegmentRenames.TryGetValue(original, out string? mapped) + ? mapped[(mapped.LastIndexOf('/') + 1)..] + : segments[i]; + effective = i == 0 ? name : $"{effective}/{name}"; + } + + return effective; + } +} diff --git a/gclo.Engine/WindowsPathValidator.cs b/gclo.Engine/WindowsPathValidator.cs index a42e385..05bdd90 100644 --- a/gclo.Engine/WindowsPathValidator.cs +++ b/gclo.Engine/WindowsPathValidator.cs @@ -225,7 +225,7 @@ public static class WindowsPathValidator { if(ValidateSegment(segments[i]) is { } problem) { - invalid.Add(new InvalidPathInfo(prefix, problem.Reason, problem.Suggestion)); + invalid.Add(new InvalidPathInfo(prefix, problem.Reason, Settle(problem.Suggestion))); } if(seenCi.TryGetValue(prefix, out string? original)) { @@ -244,7 +244,28 @@ public static class WindowsPathValidator - private static (string Reason, string? Suggestion)? ValidateSegment(string segment) + /// + /// A suggested name that is itself valid. One rule's fix can break another rule + /// ("x?." loses its dot and still holds a '?'), so the suggestion is put through + /// the rules again until it passes; proposing a name that would be rejected helps + /// nobody (#149). Every rule's fix settles within a few rounds; the bound is there + /// so that a future rule which does not cannot loop. + /// + private static string Settle(string suggestion) + { + const int MaxRounds = 8; + + for(int round = 0; round < MaxRounds && ValidateSegment(suggestion) is { } problem; round++) + { + suggestion = problem.Suggestion; + } + + return suggestion; + } + + + + private static (string Reason, string Suggestion)? ValidateSegment(string segment) { if(segment.Length == 0) { @@ -278,7 +299,9 @@ public static class WindowsPathValidator string baseName = segment.Split('.')[0]; if(s_reservedNames.Contains(baseName)) { - return ($"'{baseName}' is a reserved Windows device name", segment + "_"); + // The device name is the part before the first dot, whatever follows it + // ("COM3.txt" is as reserved as "COM3"), so that part is what changes. + return ($"'{baseName}' is a reserved Windows device name", baseName + "_" + segment[baseName.Length..]); } return null; diff --git a/gclo.ViewModels/PathRecoveryPlanner.cs b/gclo.ViewModels/PathRecoveryPlanner.cs index bc0078c..13e5d62 100644 --- a/gclo.ViewModels/PathRecoveryPlanner.cs +++ b/gclo.ViewModels/PathRecoveryPlanner.cs @@ -35,9 +35,11 @@ public static class PathRecoveryPlanner /// /// Builds the plan. A row that is neither skipped nor renamed is always a problem - /// (every listed path is invalid by definition), and replacement names go through - /// the same Windows rules as a checkout would, including two rows renamed onto one - /// destination. + /// (every listed path is invalid by definition), and the places the renamed paths + /// end up go through the same Windows rules as a checkout would, including two rows + /// renamed onto one destination. What is validated is the destination, not the + /// replacement as typed: a renamed entry inside a renamed folder is written with its + /// folder's ORIGINAL name in front, and lands under the folder's new one (#149). /// public static Plan Build(IReadOnlyList choices) { @@ -66,11 +68,93 @@ public static class PathRecoveryPlanner } } - foreach(InvalidPathInfo stillInvalid in WindowsPathValidator.ValidatePaths(renames.Values)) + var recovery = new PathRecovery(renames, skipped); + foreach(InvalidPathInfo stillInvalid in WindowsPathValidator.ValidatePaths(Destinations(recovery).Select(d => d.Effective))) { problems.Add($"'{stillInvalid.RepoPath}' cannot be used: {stillInvalid.Reason}."); } - return new Plan(new PathRecovery(renames, skipped), problems); + return new Plan(recovery, problems); + } + + + + /// + /// The recovery an unattended run applies (--sanitize-paths): every path with + /// a suggested name is renamed to it, every path without one is skipped, and then + /// whatever the renames cannot place after all (two paths given the same name, a + /// name that clashes with a folder) is skipped as well, until the plan is sound. + /// The result never fails the pre-flight that runs (#149). + /// + public static PathRecovery FromSuggestions(IReadOnlyList paths) + { + ArgumentNullException.ThrowIfNull(paths); + + // Skip wins: a path that also has a suggestion-less problem is not materialized. + var skipped = new HashSet( + paths.Where(p => string.IsNullOrEmpty(p.SuggestedName)).Select(p => p.RepoPath), + StringComparer.Ordinal); + + var renames = new Dictionary(StringComparer.Ordinal); + foreach(InvalidPathInfo info in paths) + { + if(info.SuggestedName is not { Length: > 0 } suggestion || skipped.Contains(info.RepoPath)) + { + continue; + } + + // The suggestion replaces the last segment: the validator reports each + // offending segment as a repo path ending in that segment. + int slash = info.RepoPath.LastIndexOf('/'); + renames[info.RepoPath] = slash < 0 ? suggestion : info.RepoPath[..(slash + 1)] + suggestion; + } + + while(true) + { + var recovery = new PathRecovery(renames, skipped); + List<(string Original, string Effective)> destinations = Destinations(recovery); + var unusable = new HashSet( + WindowsPathValidator.ValidatePaths(destinations.Select(d => d.Effective)).Select(p => p.RepoPath), + StringComparer.Ordinal); + if(unusable.Count == 0) + { + return recovery; + } + + // Every reported path is one of the destinations or a folder above one, so + // each round turns at least one rename into a skip. + foreach((string original, string effective) in destinations) + { + if(unusable.Contains(effective) || unusable.Any(folder => effective.StartsWith(folder + "/", StringComparison.Ordinal))) + { + renames.Remove(original); + skipped.Add(original); + } + } + } + } + + + + /// + /// Where each renamed path ends up (see ), + /// leaving out renamed paths under a skipped folder and renamed folders that have a + /// renamed entry inside them: such a folder is already part of that entry's + /// destination, and listed on its own it would read as a file of the same name. + /// + private static List<(string Original, string Effective)> Destinations(PathRecovery recovery) + { + var placed = new List<(string Original, string Effective)>(); + foreach(string original in recovery.SegmentRenames.Keys) + { + if(recovery.EffectivePathOf(original) is { } effective) + { + placed.Add((original, effective)); + } + } + + return placed + .Where(p => !placed.Any(other => other.Effective.StartsWith(p.Effective + "/", StringComparison.Ordinal))) + .ToList(); } } -- 2.54.0 From 2bf2d1c52947dae3606aaae3ed0bdca675bfe8e7 Mon Sep 17 00:00:00 2001 From: James Maes Date: Tue, 10 Nov 2026 14:53:35 -0600 Subject: [PATCH 2/2] fix(recovery): folders by original path, linear cost, one policy for clashes Follow-up to the independent review of this branch. - The pre-flight dropped a destination whenever another destination lay "inside" it, which hid real clashes: a file renamed onto a name that another rename uses as a folder passed, and the engine then rejected the plan. A renamed path now counts as a folder only when another renamed ORIGINAL path lies inside it. - Two folders given the same name were merged or left out depending on whether one of them held another invalid name. They are now always a clash: reported by the pre-flight, both skipped by the automatic plan. - The pairwise comparison took a minute for 50,000 paths, in the dialog's click handler too. Folder membership is now a set lookup. - A replacement that names another folder is a problem, since only the name is applied. Problems name the rows they come from and are reported once. - An overlong name is cut between characters, never through one. - --sanitize-paths prints where each renamed path lands and counts only the renames that are applied. - Tests: the automatic plan against the real engine; file and folder clashes; a problem on a folder above a destination (the branch that ends the loop); a name that needs three rounds to settle; 50,000 paths. Refs #149 Co-Authored-By: Claude Fable 5.2 Signed-off-by: James Maes --- docs/CLI.md | 5 +- gclo.Cli.Tests/SyncCommandTests.cs | 31 ++++ gclo.Cli/SyncCommand.cs | 24 +-- gclo.Engine.Tests/PathRecoveryPlannerTests.cs | 151 ++++++++++++++++ .../SuggestedRecoveryAgainstEngineTests.cs | 126 ++++++++++++++ .../WindowsPathValidationTests.cs | 16 ++ gclo.Engine/WindowsPathValidator.cs | 6 +- gclo.ViewModels/PathRecoveryPlanner.cs | 162 ++++++++++++++---- 8 files changed, 478 insertions(+), 43 deletions(-) create mode 100644 gclo.Engine.Tests/SuggestedRecoveryAgainstEngineTests.cs diff --git a/docs/CLI.md b/docs/CLI.md index 16eadac..7c8d988 100644 --- a/docs/CLI.md +++ b/docs/CLI.md @@ -135,7 +135,10 @@ With `--sanitize-paths`, gclo checks such a repository out anyway: - Paths with no safe automatic rename are **skipped**: they are not materialized on disk. That covers case-only collisions, and paths whose suggested names would collide with each other (`a:b` and `a?b` would both - become `a_b`). + become `a_b`; two folders in that position are both skipped, not merged). + A suggested name that collides with a path the repository already has + (`a:b` beside an existing `a_b`) is not detected up front: that repository + fails with the colliding path named, and nothing is checked out. - A note listing the renames goes to stderr (text modes) and to the activity log, and the repository counts as **cloned** — the exit code stays `0`. The human summary calls the count out: diff --git a/gclo.Cli.Tests/SyncCommandTests.cs b/gclo.Cli.Tests/SyncCommandTests.cs index b6c72d4..db75a9b 100644 --- a/gclo.Cli.Tests/SyncCommandTests.cs +++ b/gclo.Cli.Tests/SyncCommandTests.cs @@ -658,6 +658,37 @@ public sealed class SyncCommandTests : IDisposable + [Fact] + public async Task SanitizePaths_SaysWhereEachPathLands_AndCountsOnlyWhatIsApplied() + { + using var console = new ConsoleCapture(); + var lister = new FakeRepoLister { Result = [Repo("a")] }; + var git = new FakeGit + { + OnClone = (_, _, _, _, _) => Task.FromException(new InvalidRepositoryPathsException( + [ + new InvalidPathInfo("logs/12:00", "invalid", "12_00"), + new InvalidPathInfo("logs/12:00/run:1.log", "invalid", "run_1.log"), + new InvalidPathInfo("Dir", "differs only by case from 'dir'", null), // skipped, with what is in it + new InvalidPathInfo("Dir/x:1", "invalid", "x_1"), + ])), + OnApplyRecovery = (_, _, _) => Task.CompletedTask, + }; + + int code = await Run(lister, git, "--org", "acme", "--target", Target, "--token-env", EnvVar, "--sanitize-paths"); + + Assert.Equal(0, code); + // The entry inside the renamed folder lands under the folder's new name, and + // the rename under the skipped folder is never applied: not listed, not counted. + Assert.Contains("a Sanitized 2 renamed, 1 skipped: ", console.Error); + Assert.Contains("'logs/12:00' -> 'logs/12_00'", console.Error); + Assert.Contains("'logs/12:00/run:1.log' -> 'logs/12_00/run_1.log'", console.Error); + Assert.Contains("skipped 'Dir'", console.Error); + Assert.DoesNotContain("x_1", console.Error); + } + + + [Fact] public async Task SanitizePaths_RecoveryFails_LeavesRepoFailed() { diff --git a/gclo.Cli/SyncCommand.cs b/gclo.Cli/SyncCommand.cs index 04a30f3..5c2fcb8 100644 --- a/gclo.Cli/SyncCommand.cs +++ b/gclo.Cli/SyncCommand.cs @@ -543,17 +543,28 @@ internal static class SyncCommand PathRecovery recovery = PathRecoveryPlanner.FromSuggestions(pending.Paths); await git.ApplyRecoveryAsync(path, recovery, cancellationToken).ConfigureAwait(false); - string detail = DescribeRecovery(recovery); + // Where each renamed path really lands (a rename inside a renamed folder + // lands under the folder's new name); a rename under a skipped folder is + // never applied, and is neither listed nor counted. + List<(string Original, string Destination)> applied = recovery.SegmentRenames.Keys + .Select(original => (Original: original, Destination: recovery.EffectivePathOf(original))) + .Where(r => r.Destination is not null) + .Select(r => (r.Original, r.Destination!)) + .OrderBy(r => r.Original, StringComparer.Ordinal) + .ToList(); + string detail = string.Join("; ", applied + .Select(r => $"'{r.Original}' -> '{r.Destination}'") + .Concat(recovery.SkippedPaths.Order(StringComparer.Ordinal).Select(s => $"skipped '{s}'"))); if(announce) { Terminal.Error( - $"{pending.Repo} Sanitized {recovery.SegmentRenames.Count} renamed, " + $"{pending.Repo} Sanitized {applied.Count} renamed, " + $"{recovery.SkippedPaths.Count} skipped: {detail}"); } log.Info($"{pending.Repo}: cloned with sanitized paths: {detail}"); printer.RecordSanitized(new SyncSanitized( pending.Repo, - recovery.SegmentRenames.Count, + applied.Count, recovery.SkippedPaths.Count, recovery.SkippedPaths.Order(StringComparer.Ordinal).ToList())); progress.Report(new RepoProgress(pending.Repo, SyncStatus.Done)); @@ -577,13 +588,6 @@ internal static class SyncCommand - private static string DescribeRecovery(PathRecovery recovery) - => string.Join("; ", recovery.SegmentRenames - .Select(r => $"'{r.Key}' -> '{r.Value}'") - .Concat(recovery.SkippedPaths.Select(s => $"skipped '{s}'"))); - - - /// One repository held back from failure reporting for a sanitization attempt. private sealed record PendingRecovery(string Repo, string Error, IReadOnlyList Paths); diff --git a/gclo.Engine.Tests/PathRecoveryPlannerTests.cs b/gclo.Engine.Tests/PathRecoveryPlannerTests.cs index 69a6e2b..4a914a1 100644 --- a/gclo.Engine.Tests/PathRecoveryPlannerTests.cs +++ b/gclo.Engine.Tests/PathRecoveryPlannerTests.cs @@ -166,6 +166,87 @@ public sealed class PathRecoveryPlannerTests + [Fact] + public void Build_AFileRenamedOntoANameThatIsAlsoAFolderOfAnotherRename_IsAProblem() + { + // Two unrelated originals: a file that becomes "a_b", and an entry inside the + // existing folder "a_b". Their destinations nest, which is a clash, not a + // folder with its content. + Plan plan = Build( + [ + new Choice("a:b", "a_b", Skip: false), + new Choice("a_b/c:d", "a_b/c_d", Skip: false), + ]); + + string problem = Assert.Single(plan.Problems); + Assert.Contains("'a_b' cannot be used: maps to both a file and a directory (from 'a:b')", problem); + } + + + + [Fact] + public void Build_AFolderAndAFileRenamedOntoOneName_IsAProblem() + { + Plan plan = Build( + [ + new Choice("a:b", "a_b", Skip: false), + new Choice("a:b/x:1", "a:b/x_1", Skip: false), + new Choice("a?b", "a_b", Skip: false), + ]); + + Assert.Contains(plan.Problems, p => p.StartsWith("'a_b' cannot be used", StringComparison.Ordinal)); + } + + + + [Fact] + public void Build_TwoFoldersRenamedOntoOneName_AreAProblem_NamedWithTheRowsTheyComeFrom() + { + Plan plan = Build( + [ + new Choice("a:b", "a_b", Skip: false), + new Choice("a:b/x:1", "a:b/x_1", Skip: false), + new Choice("a?b", "a_b", Skip: false), + new Choice("a?b/y:1", "a?b/y_1", Skip: false), + ]); + + string problem = Assert.Single(plan.Problems); + Assert.Equal("'a_b' cannot be used: more than one path maps to this destination (from 'a:b', 'a?b').", problem); + } + + + + [Fact] + public void Build_ThreeRowsAtOnePlace_AreReportedOnce() + { + Plan plan = Build( + [ + new Choice("x:1", "x_1", Skip: false), + new Choice("x?1", "x_1", Skip: false), + new Choice("x*1", "x_1", Skip: false), + ]); + + string problem = Assert.Single(plan.Problems); + Assert.Contains("(from 'x*1', 'x:1', 'x?1')", problem); + } + + + + [Theory] + [InlineData("docs/bad:name.txt", "elsewhere/bad_name.txt")] + [InlineData("docs/bad:name.txt", "docs/sub/bad_name.txt")] + [InlineData("docs/bad:name.txt", "bad_name.txt")] + [InlineData("bad:name.txt", "docs/bad_name.txt")] + public void Build_AReplacementInAnotherFolder_IsAProblem_BecauseOnlyTheNameIsApplied(string path, string replacement) + { + Plan plan = Build([new Choice(path, replacement, Skip: false)]); + + string problem = Assert.Single(plan.Problems); + Assert.Contains($"'{replacement}' would move '{path}' to another folder", problem); + } + + + // ---------------------------------------------------------------- the automatic plan (#149) [Fact] @@ -249,6 +330,76 @@ public sealed class PathRecoveryPlannerTests + [Fact] + public void FromSuggestions_TwoFoldersGivenTheSameName_AreBothSkipped_WithEverythingInThem() + { + PathRecovery recovery = PathRecoveryPlanner.FromSuggestions( + [ + new InvalidPathInfo("a:b", "invalid", "a_b"), + new InvalidPathInfo("a:b/x:1", "invalid", "x_1"), + new InvalidPathInfo("a?b", "invalid", "a_b"), + new InvalidPathInfo("a?b/y:1", "invalid", "y_1"), + new InvalidPathInfo("fine:name.txt", "invalid", "fine_name.txt"), + ]); + + Assert.Equal("fine_name.txt", Assert.Single(recovery.SegmentRenames).Value); + Assert.Contains("a:b", recovery.SkippedPaths); + Assert.Contains("a?b", recovery.SkippedPaths); + Assert.Null(recovery.EffectivePathOf("a:b/x:1")); + Assert.Null(recovery.EffectivePathOf("a?b/y:1")); + } + + + + [Fact] + public void FromSuggestions_AnEntryWhoseFolderIsInvalidAndNotListed_IsSkipped_AndThePlanIsFinished() + { + // The problem is on the folder above the destination, not on the destination + // itself; the entry is what gets skipped, and the loop ends. + PathRecovery recovery = PathRecoveryPlanner.FromSuggestions( + [new InvalidPathInfo("logs/12:00/run:1.log", "invalid", "run_1.log")]); + + Assert.Empty(recovery.SegmentRenames); + Assert.Equal(["logs/12:00/run:1.log"], recovery.SkippedPaths, StringComparer.Ordinal); + } + + + + [Fact] + public void FromSuggestions_TwoNamesThatWouldDifferOnlyByCase_KeepTheFirstAndSkipTheSecond() + { + PathRecovery recovery = PathRecoveryPlanner.FromSuggestions( + [ + new InvalidPathInfo("a:B", "invalid", "a_B"), + new InvalidPathInfo("a:b", "invalid", "a_b"), + ]); + + Assert.Equal("a_B", Assert.Single(recovery.SegmentRenames).Value); + Assert.Equal(["a:b"], recovery.SkippedPaths, StringComparer.Ordinal); + } + + + + [Fact] + public void FromSuggestions_AndBuild_StayFast_ForARepositoryWithTensOfThousandsOfInvalidPaths() + { + // Timestamps in file names are the usual way to get this many. The work per path + // must not grow with the number of paths (a pairwise comparison took a minute). + var paths = Enumerable.Range(0, 50_000) + .Select(i => new InvalidPathInfo($"data/2026-01-01T00:00:{i}.json", "invalid", $"2026-01-01T00_00_{i}.json")) + .ToList(); + var clock = System.Diagnostics.Stopwatch.StartNew(); + + PathRecovery recovery = PathRecoveryPlanner.FromSuggestions(paths); + Plan plan = Build(recovery.SegmentRenames.Select(r => new Choice(r.Key, r.Value, Skip: false)).ToList()); + + Assert.Equal(50_000, recovery.SegmentRenames.Count); + Assert.Empty(plan.Problems); + Assert.True(clock.Elapsed < TimeSpan.FromSeconds(20), $"took {clock.Elapsed}"); + } + + + [Fact] public void FromSuggestions_Null_Throws() { diff --git a/gclo.Engine.Tests/SuggestedRecoveryAgainstEngineTests.cs b/gclo.Engine.Tests/SuggestedRecoveryAgainstEngineTests.cs new file mode 100644 index 0000000..66bedc7 --- /dev/null +++ b/gclo.Engine.Tests/SuggestedRecoveryAgainstEngineTests.cs @@ -0,0 +1,126 @@ +/* + * Copyright (c) 2026 James Maes (KofTwentyTwo) + * SPDX-License-Identifier: MIT + */ + +using gclo.ViewModels; +using static gclo.Engine.Tests.GitTestHelpers; + + +namespace gclo.Engine.Tests; + + +/// +/// The plan builds from the paths a +/// clone reports is one the real engine accepts and applies (#149). Real repositories +/// on the local file system, like . +/// +public sealed class SuggestedRecoveryAgainstEngineTests : IDisposable +{ + private const string Token = "test-token"; + + private readonly string _root = + Path.Combine(Path.GetTempPath(), "gclo-tests", Guid.NewGuid().ToString("N")); + + private readonly LibGit2GitClient _client = new(); + + + + public SuggestedRecoveryAgainstEngineTests() + { + Directory.CreateDirectory(_root); + } + + + + public void Dispose() => TryDeleteDirectory(_root); + + + + [Fact] + public async Task AnInvalidFolderWithAnInvalidEntryInside_BothRenamed_UnderTheFoldersNewName() + { + string target = await SanitizeAsync(("logs/12:00/run:1.log", "run"), ("logs/12:00/ok.txt", "ok"), ("keep.txt", "keep")); + + Assert.Equal("run", File.ReadAllText(Path.Combine(target, "logs", "12_00", "run_1.log"))); + Assert.Equal("ok", File.ReadAllText(Path.Combine(target, "logs", "12_00", "ok.txt"))); + Assert.Equal("keep", File.ReadAllText(Path.Combine(target, "keep.txt"))); + } + + + + [Fact] + public async Task TwoFilesGivenTheSameName_AreBothLeftOut_AndTheRestIsWritten() + { + string target = await SanitizeAsync(("a:b", "one"), ("a?b", "two"), ("keep.txt", "keep")); + + Assert.False(File.Exists(Path.Combine(target, "a_b"))); + Assert.Equal("keep", File.ReadAllText(Path.Combine(target, "keep.txt"))); + } + + + + [Fact] + public async Task TwoFoldersGivenTheSameName_AreBothLeftOut_NeverMergedIntoOne() + { + string target = await SanitizeAsync(("a:b/f1.txt", "one"), ("a?b/f2.txt", "two"), ("keep.txt", "keep")); + + Assert.False(Directory.Exists(Path.Combine(target, "a_b"))); + Assert.Equal("keep", File.ReadAllText(Path.Combine(target, "keep.txt"))); + } + + + + [Fact] + public async Task TwoFoldersGivenTheSameName_AreLeftOut_AlsoWhenTheyHoldInvalidEntriesOfTheirOwn() + { + // Whether two subtrees are merged or left out must not depend on whether one of + // them happens to contain another invalid name. + string target = await SanitizeAsync(("a:b/x:1.txt", "one"), ("a?b/y:1.txt", "two"), ("keep.txt", "keep")); + + Assert.False(Directory.Exists(Path.Combine(target, "a_b"))); + Assert.Equal("keep", File.ReadAllText(Path.Combine(target, "keep.txt"))); + } + + + + [Fact] + public async Task AFileWhoseNewNameIsAlsoAFolderThatGetsARenamedEntry_IsLeftOut_AndTheFolderIsWritten() + { + string target = await SanitizeAsync(("a:b", "file"), ("a_b/c:d.txt", "inside"), ("a_b/ok.txt", "ok")); + + Assert.Equal("ok", File.ReadAllText(Path.Combine(target, "a_b", "ok.txt"))); + Assert.True(Directory.Exists(Path.Combine(target, "a_b"))); + } + + + + [Fact] + public async Task AFolderAndAFileGivenTheSameName_AreBothLeftOut() + { + string target = await SanitizeAsync(("a:b/x:1.txt", "inside"), ("a:b/ok.txt", "ok"), ("a?b", "file"), ("keep.txt", "keep")); + + Assert.False(File.Exists(Path.Combine(target, "a_b"))); + Assert.False(Directory.Exists(Path.Combine(target, "a_b"))); + Assert.Equal("keep", File.ReadAllText(Path.Combine(target, "keep.txt"))); + } + + + + /// + /// Clones a forged repository (which reports its invalid paths), builds the + /// automatic plan from that report, and applies it; returns the working directory. + /// + private async Task SanitizeAsync(params (string Name, string Content)[] entries) + { + string source = CreateForgedRepo(_root, entries); + string target = Path.Combine(_root, "clone-" + Guid.NewGuid().ToString("N")[..8]); + InvalidRepositoryPathsException reported = await Assert.ThrowsAsync( + () => _client.CloneAsync(source, target, Token, null, CancellationToken.None)); + + PathRecovery recovery = PathRecoveryPlanner.FromSuggestions(reported.Paths); + await _client.ApplyRecoveryAsync(target, recovery, CancellationToken.None); + + return target; + } +} diff --git a/gclo.Engine.Tests/WindowsPathValidationTests.cs b/gclo.Engine.Tests/WindowsPathValidationTests.cs index 701a698..8aa8b9f 100644 --- a/gclo.Engine.Tests/WindowsPathValidationTests.cs +++ b/gclo.Engine.Tests/WindowsPathValidationTests.cs @@ -115,6 +115,7 @@ public sealed class WindowsPathValidationTests : IDisposable [InlineData("a:b*c|d")] [InlineData("nul.")] [InlineData("LPT9. .")] + [InlineData("CON.t?t.")] // three rules, one after the other public void ValidatePaths_EverySuggestedName_IsItselfValid(string segment) { // One rule's fix can trip another; a suggestion that would be rejected is worse @@ -143,6 +144,21 @@ public sealed class WindowsPathValidationTests : IDisposable + [Fact] + public void ValidatePaths_AnOverlongName_IsCutBetweenCharacters_NeverThroughOne() + { + // An emoji is two code units; the limit falls between them. + string emoji = char.ConvertFromUtf32(0x1F600); + string segment = new string('x', 254) + emoji + "tail"; + + InvalidPathInfo info = Assert.Single(WindowsPathValidator.ValidatePaths([segment])); + + Assert.Equal(new string('x', 254), info.SuggestedName); + Assert.Empty(WindowsPathValidator.ValidatePaths([info.SuggestedName!])); + } + + + [Fact] public void Validate_FlagsCaseOnlyCollision() { diff --git a/gclo.Engine/WindowsPathValidator.cs b/gclo.Engine/WindowsPathValidator.cs index 05bdd90..b50191f 100644 --- a/gclo.Engine/WindowsPathValidator.cs +++ b/gclo.Engine/WindowsPathValidator.cs @@ -273,7 +273,11 @@ public static class WindowsPathValidator } if(segment.Length > MaxSegmentLength) { - return ($"segment longer than {MaxSegmentLength} characters", segment[..MaxSegmentLength]); + // Cut between characters: a pair of code units for one character is kept + // whole or dropped whole, never split (the half would be written to disk and + // then stored as something else). + int keep = char.IsHighSurrogate(segment[MaxSegmentLength - 1]) ? MaxSegmentLength - 1 : MaxSegmentLength; + return ($"segment longer than {MaxSegmentLength} characters", segment[..keep]); } if(WorkingTree.IsGitDirectoryName(segment)) { diff --git a/gclo.ViewModels/PathRecoveryPlanner.cs b/gclo.ViewModels/PathRecoveryPlanner.cs index 13e5d62..c66b170 100644 --- a/gclo.ViewModels/PathRecoveryPlanner.cs +++ b/gclo.ViewModels/PathRecoveryPlanner.cs @@ -16,6 +16,11 @@ namespace gclo.ViewModels; /// engine, and re-opening with a fresh list (#30). The engine still re-validates the /// full effective tree on apply; this is the pre-flight for what the user can see. /// +/// +/// The pre-flight sees the listed paths only, not the rest of the tree. A new name +/// that collides with a path that was valid all along (and so is not listed) is +/// caught by the engine, not here. +/// public static class PathRecoveryPlanner { /// One row's choice. @@ -35,11 +40,12 @@ public static class PathRecoveryPlanner /// /// Builds the plan. A row that is neither skipped nor renamed is always a problem - /// (every listed path is invalid by definition), and the places the renamed paths - /// end up go through the same Windows rules as a checkout would, including two rows - /// renamed onto one destination. What is validated is the destination, not the - /// replacement as typed: a renamed entry inside a renamed folder is written with its - /// folder's ORIGINAL name in front, and lands under the folder's new one (#149). + /// (every listed path is invalid by definition); a replacement may change a path's + /// name but not its folder; and the places the renamed paths end up go through the + /// same Windows rules as a checkout would, including two rows renamed onto one + /// destination. What is validated is the destination, not the replacement as typed: + /// a renamed entry inside a renamed folder is written with its folder's ORIGINAL + /// name in front, and lands under the folder's new one (#149). /// public static Plan Build(IReadOnlyList choices) { @@ -61,6 +67,12 @@ public static class PathRecoveryPlanner else if(choice.ReplacementPath is { } replacement) { renames[choice.RepoPath] = replacement; + if(!string.Equals(FolderOf(replacement), FolderOf(choice.RepoPath), StringComparison.Ordinal)) + { + // Only the last segment is applied; saying nothing would let the user + // believe the entry moves to the folder they typed. + problems.Add($"'{replacement}' would move '{choice.RepoPath}' to another folder: only its name can be changed."); + } } else { @@ -69,9 +81,10 @@ public static class PathRecoveryPlanner } var recovery = new PathRecovery(renames, skipped); - foreach(InvalidPathInfo stillInvalid in WindowsPathValidator.ValidatePaths(Destinations(recovery).Select(d => d.Effective))) + List placements = Place(recovery); + foreach((string destination, string reason) in ProblemsOf(placements)) { - problems.Add($"'{stillInvalid.RepoPath}' cannot be used: {stillInvalid.Reason}."); + problems.Add($"'{destination}' cannot be used: {reason}{Origins(placements, destination)}."); } return new Plan(recovery, problems); @@ -83,8 +96,10 @@ public static class PathRecoveryPlanner /// The recovery an unattended run applies (--sanitize-paths): every path with /// a suggested name is renamed to it, every path without one is skipped, and then /// whatever the renames cannot place after all (two paths given the same name, a - /// name that clashes with a folder) is skipped as well, until the plan is sound. - /// The result never fails the pre-flight that runs (#149). + /// name that is also a folder's) is skipped as well, until the listed paths no + /// longer clash with each other. Two folders given the same name are both skipped, + /// never merged into one. The result never fails the pre-flight that + /// runs (#149). /// public static PathRecovery FromSuggestions(IReadOnlyList paths) { @@ -105,30 +120,27 @@ public static class PathRecoveryPlanner // The suggestion replaces the last segment: the validator reports each // offending segment as a repo path ending in that segment. - int slash = info.RepoPath.LastIndexOf('/'); - renames[info.RepoPath] = slash < 0 ? suggestion : info.RepoPath[..(slash + 1)] + suggestion; + renames[info.RepoPath] = FolderOf(info.RepoPath) + suggestion; } while(true) { var recovery = new PathRecovery(renames, skipped); - List<(string Original, string Effective)> destinations = Destinations(recovery); - var unusable = new HashSet( - WindowsPathValidator.ValidatePaths(destinations.Select(d => d.Effective)).Select(p => p.RepoPath), - StringComparer.Ordinal); + List placements = Place(recovery); + var unusable = new HashSet(ProblemsOf(placements).Select(p => p.Destination), StringComparer.Ordinal); if(unusable.Count == 0) { return recovery; } - // Every reported path is one of the destinations or a folder above one, so - // each round turns at least one rename into a skip. - foreach((string original, string effective) in destinations) + // Every reported destination is where a placement ends up, or a folder above + // one, so each round turns at least one rename into a skip. + foreach(Placement placement in placements) { - if(unusable.Contains(effective) || unusable.Any(folder => effective.StartsWith(folder + "/", StringComparison.Ordinal))) + if(IsAtOrUnder(placement.Effective, unusable)) { - renames.Remove(original); - skipped.Add(original); + renames.Remove(placement.Original); + skipped.Add(placement.Original); } } } @@ -136,25 +148,113 @@ public static class PathRecoveryPlanner + /// A renamed path and where it ends up. + /// The repository path that is renamed. + /// Where it lands (see ). + /// True when another renamed path lies inside this one, which makes it a folder for certain. + private sealed record Placement(string Original, string Effective, bool HoldsRenamedEntries); + + + /// - /// Where each renamed path ends up (see ), - /// leaving out renamed paths under a skipped folder and renamed folders that have a - /// renamed entry inside them: such a folder is already part of that entry's - /// destination, and listed on its own it would read as a file of the same name. + /// Every renamed path that is written at all (not under a skipped folder), with its + /// destination, and whether it is known to be a folder: one that holds another + /// renamed path. Decided by the ORIGINAL paths, which are the tree's; two unrelated + /// originals whose destinations happen to nest are a clash, not a folder and its + /// content. /// - private static List<(string Original, string Effective)> Destinations(PathRecovery recovery) + private static List Place(PathRecovery recovery) { var placed = new List<(string Original, string Effective)>(); + var foldersOfPlaced = new HashSet(StringComparer.Ordinal); foreach(string original in recovery.SegmentRenames.Keys) { - if(recovery.EffectivePathOf(original) is { } effective) + if(recovery.EffectivePathOf(original) is not { } effective) { - placed.Add((original, effective)); + continue; + } + + placed.Add((original, effective)); + for(int slash = original.LastIndexOf('/'); slash > 0; slash = original.LastIndexOf('/', slash - 1)) + { + foldersOfPlaced.Add(original[..slash]); } } - return placed - .Where(p => !placed.Any(other => other.Effective.StartsWith(p.Effective + "/", StringComparison.Ordinal))) - .ToList(); + return placed.Select(p => new Placement(p.Original, p.Effective, foldersOfPlaced.Contains(p.Original))).ToList(); } + + + + /// + /// The destinations that cannot be used, each with the reason. Destinations of + /// entries go through the Windows rules together, which also finds two of them at + /// one place and an entry where a folder is needed. A folder that holds renamed + /// entries is part of their destinations already, and is checked for the one thing + /// that would not show there: another renamed path, folder or not, given its name. + /// + private static List<(string Destination, string Reason)> ProblemsOf(List placements) + { + var problems = new List<(string Destination, string Reason)>(); + var reported = new HashSet(StringComparer.Ordinal); + foreach(InvalidPathInfo invalid in WindowsPathValidator.ValidatePaths(placements.Where(p => !p.HoldsRenamedEntries).Select(p => p.Effective))) + { + if(reported.Add(invalid.RepoPath)) + { + problems.Add((invalid.RepoPath, invalid.Reason)); + } + } + + foreach(IGrouping sameName in placements.GroupBy(p => p.Effective, StringComparer.OrdinalIgnoreCase)) + { + if(sameName.Count() > 1 && sameName.Any(p => p.HoldsRenamedEntries)) + { + foreach(string destination in sameName.Select(p => p.Effective).Distinct(StringComparer.Ordinal)) + { + if(reported.Add(destination)) + { + problems.Add((destination, "more than one path maps to this destination")); + } + } + } + } + + return problems; + } + + + + /// The rows behind a destination, for a message: the user sees originals, not where they land. + private static string Origins(List placements, string destination) + { + var originals = placements + .Where(p => string.Equals(p.Effective, destination, StringComparison.Ordinal)) + .Select(p => p.Original) + .Where(original => !string.Equals(original, destination, StringComparison.Ordinal)) + .Order(StringComparer.Ordinal) + .ToList(); + return originals.Count == 0 ? "" : $" (from {string.Join(", ", originals.Select(o => $"'{o}'"))})"; + } + + + + /// True when is one of or lies under one of them. + private static bool IsAtOrUnder(string path, HashSet paths) + { + for(int end = path.Length; end > 0; end = path.LastIndexOf('/', end - 1)) + { + if(paths.Contains(path[..end])) + { + return true; + } + } + + return false; + } + + + + /// The folder part of a repository path, with its trailing slash; empty at the root. + private static string FolderOf(string repoPath) + => repoPath[..(repoPath.LastIndexOf('/') + 1)]; } -- 2.54.0