From ea81e6830fea1cb67791d23ae7f4f1ea55c821d6 Mon Sep 17 00:00:00 2001 From: Eric Kulcyk Date: Mon, 11 May 2026 14:13:41 -0700 Subject: [PATCH 01/13] Handle case-only file and directory renames in FastFetch on Windows Previously, FastFetch detected case-only renames (e.g. TestFolder -> testfolder) but suppressed them with a warning, leaving the working directory with stale casing. This change makes FastFetch propagate case renames to disk, matching the behavior of regular git checkout. Changes: - DiffTreeResult: Add SourcePath property to carry old-cased path for directory case renames - DiffHelper: Detect case-only renames and flow them through the pipeline instead of suppressing. Track case-renamed directories to properly filter child operations in FlushStagedQueues - CheckoutStage: Perform two-step directory rename (old -> temp -> new) to work around Directory.Move() failing on case-only renames on Windows - Update unit tests to verify SourcePath, delete counts, and directory operation filtering for case renames - Update functional test to verify casing with ignoreCase: false Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- GVFS/FastFetch/CheckoutStage.cs | 31 ++++- GVFS/GVFS.Common/Git/DiffTreeResult.cs | 6 + GVFS/GVFS.Common/Prefetch/Git/DiffHelper.cs | 113 ++++++++++++++---- .../Tests/FastFetchTests.cs | 4 +- .../Prefetch/DiffHelperTests.cs | 16 ++- 5 files changed, 143 insertions(+), 27 deletions(-) diff --git a/GVFS/FastFetch/CheckoutStage.cs b/GVFS/FastFetch/CheckoutStage.cs index 6874eff43..449738e62 100644 --- a/GVFS/FastFetch/CheckoutStage.cs +++ b/GVFS/FastFetch/CheckoutStage.cs @@ -173,13 +173,40 @@ private void HandleAllDirectoryOperations() case DiffTreeResult.Operations.Add: try { - Directory.CreateDirectory(absoluteTargetPath); + if (treeOp.SourcePath != null) + { + // Case-only rename: rename the directory from old casing to new casing + string absoluteSourcePath = Path.Combine(this.enlistment.WorkingDirectoryBackingRoot, treeOp.SourcePath); + if (Directory.Exists(absoluteSourcePath)) + { + // Directory.Move throws IOException for case-only renames, + // so use a two-step rename through a temporary name. + string tempPath = absoluteTargetPath.TrimEnd(Path.DirectorySeparatorChar) + "_caseRename_" + Guid.NewGuid().ToString("N"); + Directory.Move(absoluteSourcePath.TrimEnd(Path.DirectorySeparatorChar), tempPath); + Directory.Move(tempPath, absoluteTargetPath.TrimEnd(Path.DirectorySeparatorChar)); + } + else + { + // Parent directory may have already been renamed, fixing this child's path. + // Just ensure the directory exists. + Directory.CreateDirectory(absoluteTargetPath); + } + } + else + { + Directory.CreateDirectory(absoluteTargetPath); + } } catch (Exception ex) { EventMetadata metadata = new EventMetadata(); - metadata.Add("Operation", "CreateDirectory"); + metadata.Add("Operation", treeOp.SourcePath != null ? "RenameDirectory" : "CreateDirectory"); metadata.Add(nameof(treeOp.TargetPath), absoluteTargetPath); + if (treeOp.SourcePath != null) + { + metadata.Add(nameof(treeOp.SourcePath), treeOp.SourcePath); + } + this.tracer.RelatedError(metadata, ex.Message); this.HasFailures = true; } diff --git a/GVFS/GVFS.Common/Git/DiffTreeResult.cs b/GVFS/GVFS.Common/Git/DiffTreeResult.cs index 85eaaf559..1fc0d2a5d 100644 --- a/GVFS/GVFS.Common/Git/DiffTreeResult.cs +++ b/GVFS/GVFS.Common/Git/DiffTreeResult.cs @@ -38,6 +38,12 @@ public enum Operations public ushort SourceMode { get; set; } public ushort TargetMode { get; set; } + /// + /// When set, indicates this operation is a case-only rename from SourcePath to TargetPath. + /// Used on case-insensitive file systems to carry the old-cased path for directory renames. + /// + public string SourcePath { get; set; } + public static DiffTreeResult ParseFromDiffTreeLine(string line) { if (string.IsNullOrEmpty(line)) diff --git a/GVFS/GVFS.Common/Prefetch/Git/DiffHelper.cs b/GVFS/GVFS.Common/Prefetch/Git/DiffHelper.cs index f55d47d5f..84b585fbb 100644 --- a/GVFS/GVFS.Common/Prefetch/Git/DiffHelper.cs +++ b/GVFS/GVFS.Common/Prefetch/Git/DiffHelper.cs @@ -21,6 +21,10 @@ public class DiffHelper private HashSet stagedDirectoryOperations = new HashSet(new DiffTreeByNameComparer()); private HashSet stagedFileDeletes = new HashSet(GVFSPlatform.Instance.Constants.PathComparer); + // Tracks directories that were deleted as part of a case-only rename. + // Used by FlushStagedQueues to filter out child file deletes inside case-renamed directories. + private HashSet caseRenamedDirectoryDeletes = new HashSet(GVFSPlatform.Instance.Constants.PathComparer); + private Enlistment enlistment; private GitProcess git; @@ -175,6 +179,11 @@ private void FlushStagedQueues() .Select(d => d.TargetPath.TrimEnd(Path.DirectorySeparatorChar)), GVFSPlatform.Instance.Constants.PathComparer); + // Also include directories that were deleted as part of case-only renames. + // These were replaced by Adds in stagedDirectoryOperations but their children's + // file deletes should still be filtered out (the parent rename handles them). + deletedDirectories.UnionWith(this.caseRenamedDirectoryDeletes); + foreach (DiffTreeResult result in this.stagedDirectoryOperations) { string parentPath = Path.GetDirectoryName(result.TargetPath.TrimEnd(Path.DirectorySeparatorChar)); @@ -182,10 +191,16 @@ private void FlushStagedQueues() { if (result.Operation != DiffTreeResult.Operations.Delete) { - EventMetadata metadata = new EventMetadata(); - metadata.Add(nameof(result.TargetPath), result.TargetPath); - metadata.Add(TracingConstants.MessageKey.WarningMessage, "An operation is intended to go inside of a deleted folder"); - activity.RelatedError("InvalidOperation", metadata); + // For case renames, child directory Adds inside a case-renamed parent + // are expected. The parent rename will handle moving the children. + // Only warn if the parent is truly deleted (not case-renamed). + if (!this.caseRenamedDirectoryDeletes.Contains(parentPath)) + { + EventMetadata metadata = new EventMetadata(); + metadata.Add(nameof(result.TargetPath), result.TargetPath); + metadata.Add(TracingConstants.MessageKey.WarningMessage, "An operation is intended to go inside of a deleted folder"); + activity.RelatedError("InvalidOperation", metadata); + } } } else @@ -276,10 +291,10 @@ private void EnqueueOperationsFromDiffTreeLine(ITracer activity, string line) case DiffTreeResult.Operations.Delete: if (!this.stagedDirectoryOperations.Add(result)) { - EventMetadata metadata = new EventMetadata(); - metadata.Add(nameof(result.TargetPath), result.TargetPath); - metadata.Add(TracingConstants.MessageKey.WarningMessage, "A case change was attempted. It will not be reflected in the working directory."); - activity.RelatedEvent(EventLevel.Warning, "CaseConflict", metadata); + // A directory with the same name (case-insensitive) already exists. + // On case-insensitive file systems, this means the Delete came after an Add, + // which shouldn't happen (diff-tree outputs Deletes before Adds). + // Keep the existing Add to avoid deleting a folder from under ourselves. } break; @@ -287,10 +302,26 @@ private void EnqueueOperationsFromDiffTreeLine(ITracer activity, string line) case DiffTreeResult.Operations.Modify: if (!this.stagedDirectoryOperations.Add(result)) { - EventMetadata metadata = new EventMetadata(); - metadata.Add(nameof(result.TargetPath), result.TargetPath); - metadata.Add(TracingConstants.MessageKey.WarningMessage, "A case change was attempted. It will not be reflected in the working directory."); - activity.RelatedEvent(EventLevel.Warning, "CaseConflict", metadata); + // A directory with the same path (case-insensitive) was already staged. + // This is a case-only rename: the Delete was staged first, now the Add arrives. + // Find the existing entry to capture the old-cased path. + DiffTreeResult existingOp = null; + foreach (DiffTreeResult staged in this.stagedDirectoryOperations) + { + if (staged.TargetPath.Equals(result.TargetPath, GVFSPlatform.Instance.Constants.PathComparison)) + { + existingOp = staged; + break; + } + } + + if (existingOp != null && + !existingOp.TargetPath.Equals(result.TargetPath, StringComparison.Ordinal)) + { + // Case-only rename: store the old-cased path so CheckoutStage can rename the directory + result.SourcePath = existingOp.TargetPath; + this.caseRenamedDirectoryDeletes.Add(existingOp.TargetPath.TrimEnd(Path.DirectorySeparatorChar)); + } // Replace the delete with the add to make sure we don't delete a folder from under ourselves this.stagedDirectoryOperations.Remove(result); @@ -357,14 +388,29 @@ private bool ShouldIncludeResult(DiffTreeResult blobAdd) private void EnqueueFileDeleteOperation(ITracer activity, string targetPath) { + // Use case-sensitive check: if the exact same path (same casing) was already added, + // this is a true duplicate, not a case rename. Skip it. + // But if it matches case-insensitively only, this is a case rename — allow the delete through + // so the old-cased file is removed before the new-cased file is written. if (this.filesAdded.Contains(targetPath)) { - EventMetadata metadata = new EventMetadata(); - metadata.Add(nameof(targetPath), targetPath); - metadata.Add(TracingConstants.MessageKey.WarningMessage, "A case change was attempted. It will not be reflected in the working directory."); - activity.RelatedEvent(EventLevel.Warning, "CaseConflict", metadata); + // Check if any added file matches with exact (ordinal) casing + bool exactMatch = false; + foreach (string addedPath in this.filesAdded) + { + if (addedPath.Equals(targetPath, StringComparison.Ordinal)) + { + exactMatch = true; + break; + } + } - return; + if (exactMatch) + { + return; + } + + // Case-only difference: allow the delete so the old casing is removed from disk } this.stagedFileDeletes.Add(targetPath); @@ -389,12 +435,35 @@ private void EnqueueFileAddOperation(ITracer activity, DiffTreeResult operation) } } - if (this.stagedFileDeletes.Remove(operation.TargetPath)) + // On case-insensitive file systems, stagedFileDeletes uses case-insensitive comparison. + // Check if there's a staged delete that differs only in case — if so, keep it + // so the old-cased file is removed from disk before the new one is written. + if (this.stagedFileDeletes.Contains(operation.TargetPath)) { - EventMetadata metadata = new EventMetadata(); - metadata.Add(nameof(operation.TargetPath), operation.TargetPath); - metadata.Add(TracingConstants.MessageKey.WarningMessage, "A case change was attempted. It will not be reflected in the working directory."); - activity.RelatedEvent(EventLevel.Warning, "CaseConflict", metadata); + // Check if the staged delete has the exact same casing + bool exactMatch = false; + string existingDeletePath = null; + foreach (string deletePath in this.stagedFileDeletes) + { + if (deletePath.Equals(operation.TargetPath, GVFSPlatform.Instance.Constants.PathComparison)) + { + existingDeletePath = deletePath; + if (deletePath.Equals(operation.TargetPath, StringComparison.Ordinal)) + { + exactMatch = true; + } + + break; + } + } + + if (exactMatch) + { + // Same exact path: true delete+add, not a case rename. Remove the delete. + this.stagedFileDeletes.Remove(operation.TargetPath); + } + + // Case-only difference: keep the delete staged so old casing is removed from disk } this.FileAddOperations.AddOrUpdate( diff --git a/GVFS/GVFS.FunctionalTests/Tests/FastFetchTests.cs b/GVFS/GVFS.FunctionalTests/Tests/FastFetchTests.cs index 2a0989327..ad8db56cb 100644 --- a/GVFS/GVFS.FunctionalTests/Tests/FastFetchTests.cs +++ b/GVFS/GVFS.FunctionalTests/Tests/FastFetchTests.cs @@ -320,9 +320,9 @@ public void SuccessfullyChecksOutCaseChanges() try { - // Ignore case differences on case-insensitive filesystems + // Verify FastFetch produces correct casing matching git checkout, including on case-insensitive filesystems this.fastFetchRepoRoot.ShouldBeADirectory(FileSystemRunner.DefaultRunner) - .WithDeepStructure(FileSystemRunner.DefaultRunner, this.fastFetchControlRoot, ignoreCase: !FileSystemHelpers.CaseSensitiveFileSystem); + .WithDeepStructure(FileSystemRunner.DefaultRunner, this.fastFetchControlRoot, ignoreCase: false); } finally { diff --git a/GVFS/GVFS.UnitTests/Prefetch/DiffHelperTests.cs b/GVFS/GVFS.UnitTests/Prefetch/DiffHelperTests.cs index 0799ad11b..79082986d 100644 --- a/GVFS/GVFS.UnitTests/Prefetch/DiffHelperTests.cs +++ b/GVFS/GVFS.UnitTests/Prefetch/DiffHelperTests.cs @@ -114,10 +114,24 @@ public void ParsesCaseChangesAsAdds() diffBackwards.RequiredBlobs.Count.ShouldEqual(2); diffBackwards.FileAddOperations.Sum(list => list.Value.Count).ShouldEqual(2); + // File deletes inside case-renamed directories are filtered out by FlushStagedQueues diffBackwards.FileDeleteOperations.Count.ShouldEqual(0); - diffBackwards.TotalFileDeletes.ShouldEqual(0); + + // File deletes are now staged (not suppressed) so FlushStagedQueues can filter them properly + diffBackwards.TotalFileDeletes.ShouldEqual(2); diffBackwards.DirectoryOperations.ShouldNotContain(entry => entry.Operation == DiffTreeResult.Operations.Delete); + + // Only the top-level directory rename is enqueued; children are filtered because + // the parent rename moves them automatically + diffBackwards.DirectoryOperations.Count.ShouldEqual(1); + + // The enqueued directory operation should carry the old-cased path for the rename + DiffTreeResult dirOp = diffBackwards.DirectoryOperations.First(); + dirOp.SourcePath.ShouldNotBeNull(); + Assert.AreEqual("GVFLT_MultiThreadTest" + Path.DirectorySeparatorChar, dirOp.SourcePath); + Assert.AreEqual("GVFlt_MultiThreadTest" + Path.DirectorySeparatorChar, dirOp.TargetPath); + diffBackwards.TotalDirectoryOperations.ShouldEqual(3); } From 47a525348f90e02f1f74afbe83c140501951b2bc Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Mon, 11 May 2026 18:16:36 -0400 Subject: [PATCH 02/13] refactor: Extract directory-collision lookup helper in DiffHelper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Context: EnqueueOperationsFromDiffTreeLine handles the case where adding a new DiffTreeResult to stagedDirectoryOperations collides with an already- staged entry under the case-insensitive DiffTreeByNameComparer. When the collision happens, the code needs to find the existing staged entry (by case-insensitive path equality) so it can capture the old casing for a directory case rename. Today the Modify/Add branch is the only collision branch that performs that lookup, and the lookup is written inline as a foreach scan over the staged set. A symmetric Delete-after-Add ordering also needs the same lookup, so the inline pattern is about to be duplicated. Justification: Extracting the lookup before introducing the second caller keeps the upcoming bugfix focused on the behavior change. The helper has a single responsibility — find a staged DiffTreeResult by case- insensitive TargetPath — which is meaningful on its own and easy to test in isolation if needed. Mutating an item already in the HashSet remains safe because DiffTreeByNameComparer keys on TargetPath only; SourcePath (the field the callers will mutate next) is not part of the hash or equality. Implementation: Adds private FindStagedDirectoryOperation(string targetPath) which returns the first staged DiffTreeResult whose TargetPath matches under GVFSPlatform.Instance.Constants.PathComparison, or null if none. The existing inline foreach in the Modify/Add collision branch is replaced with a call to this helper. No observable behavior change. Co-Authored-By: Claude Opus 4.7 (1M context) --- GVFS/GVFS.Common/Prefetch/Git/DiffHelper.cs | 24 ++++++++++++--------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/GVFS/GVFS.Common/Prefetch/Git/DiffHelper.cs b/GVFS/GVFS.Common/Prefetch/Git/DiffHelper.cs index 84b585fbb..7eef4172b 100644 --- a/GVFS/GVFS.Common/Prefetch/Git/DiffHelper.cs +++ b/GVFS/GVFS.Common/Prefetch/Git/DiffHelper.cs @@ -305,16 +305,7 @@ private void EnqueueOperationsFromDiffTreeLine(ITracer activity, string line) // A directory with the same path (case-insensitive) was already staged. // This is a case-only rename: the Delete was staged first, now the Add arrives. // Find the existing entry to capture the old-cased path. - DiffTreeResult existingOp = null; - foreach (DiffTreeResult staged in this.stagedDirectoryOperations) - { - if (staged.TargetPath.Equals(result.TargetPath, GVFSPlatform.Instance.Constants.PathComparison)) - { - existingOp = staged; - break; - } - } - + DiffTreeResult existingOp = FindStagedDirectoryOperation(result.TargetPath); if (existingOp != null && !existingOp.TargetPath.Equals(result.TargetPath, StringComparison.Ordinal)) { @@ -478,6 +469,19 @@ private void EnqueueFileAddOperation(ITracer activity, DiffTreeResult operation) this.RequiredBlobs.Add(operation.TargetSha); } + private DiffTreeResult FindStagedDirectoryOperation(string targetPath) + { + foreach (DiffTreeResult staged in this.stagedDirectoryOperations) + { + if (staged.TargetPath.Equals(targetPath, GVFSPlatform.Instance.Constants.PathComparison)) + { + return staged; + } + } + + return null; + } + private class DiffTreeByNameComparer : IEqualityComparer { public bool Equals(DiffTreeResult x, DiffTreeResult y) From f10d0e199010f272e931085c85e03c19652dd4aa Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Mon, 11 May 2026 18:17:52 -0400 Subject: [PATCH 03/13] fix: Handle case-only directory rename when Add precedes Delete MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Context: Branch eric-pr added handling for case-only directory renames in FastFetch on Windows: when diff-tree reports a Delete and an Add for the same case-insensitive path, DiffHelper collapses the pair into a single Add carrying the old-cased path on SourcePath, and CheckoutStage performs a two-step Directory.Move to apply the casing change on disk. That logic lives in the Modify/Add collision branch of EnqueueOperationsFromDiffTreeLine: when an Add arrives and finds a Delete already staged for the same path, it captures the Delete's TargetPath onto SourcePath. The symmetric Delete branch was a no-op with a comment claiming "diff-tree outputs Deletes before Adds". That assumption is wrong. git diff-tree compares tree entries by byte order of the path name, so for a case-only rename the Add or Delete can appear first depending on which casing sorts lower. For example "GVFLT_*" -> "GVFlt_*" emits the Delete (uppercase 'L', 0x4C) before the Add (lowercase 'l', 0x6C), but the reverse rename "GVFlt_*" -> "GVFLT_*" emits the Add first. In the latter case the Delete branch fires, the staged Add is left without a SourcePath, and CheckoutStage falls back to plain Directory.CreateDirectory — the original-cased directory is never renamed and remains on disk. Justification: Two approaches were considered: (a) buffer the entire diff-tree output and sort it before parsing, forcing a stable ordering invariant; (b) make the staging code order-independent so it handles either arrival order correctly. Option (a) regresses memory: FastFetch streams diff-tree line-by-line today and is run against multi-million-file repos, so buffering the whole output is meaningful. Option (a) also relies on a git emit-order invariant that git does not document — making the C# layer robust is the right place to defend against future diff-tree changes. Option (b) is the smaller, more localized fix and is implemented here. Implementation: EnqueueOperationsFromDiffTreeLine's Delete-collision branch now mirrors the Modify/Add branch: if a HashSet collision under DiffTreeByNameComparer indicates an Add was already staged for the same case-insensitive path, find that Add via FindStagedDirectoryOperation and — when the existing TargetPath differs from the incoming TargetPath under ordinal comparison — annotate the staged Add's SourcePath with the old (incoming Delete's) casing, and register the old-cased path in caseRenamedDirectoryDeletes so FlushStagedQueues can filter children of the renamed directory. The staged entry is mutated in place. This is safe because DiffTreeByNameComparer hashes and compares only on TargetPath; the SourcePath field is a runtime annotation that the comparer does not see, so mutation does not corrupt the HashSet. A new fixture caseChangeAddFirst.txt exercises the reverse-direction rename ("GVFlt_*" -> "GVFLT_*") with the Add lines emitted before the Delete lines, and the matching test ParsesCaseChangesWhenAddPrecedesDelete asserts the staged directory operation carries the correct SourcePath and that child operations are filtered exactly as in the Delete-first case. Without this fix, the test fails because SourcePath is null. Co-Authored-By: Claude Opus 4.7 (1M context) --- GVFS/GVFS.Common/Prefetch/Git/DiffHelper.cs | 20 +++- .../Data/caseChangeAddFirst.txt | 10 ++ GVFS/GVFS.UnitTests/GVFS.UnitTests.csproj | 3 + .../Prefetch/DiffHelperTests.cs | 102 ++++++++++++------ 4 files changed, 100 insertions(+), 35 deletions(-) create mode 100644 GVFS/GVFS.UnitTests/Data/caseChangeAddFirst.txt diff --git a/GVFS/GVFS.Common/Prefetch/Git/DiffHelper.cs b/GVFS/GVFS.Common/Prefetch/Git/DiffHelper.cs index 7eef4172b..70ce856b4 100644 --- a/GVFS/GVFS.Common/Prefetch/Git/DiffHelper.cs +++ b/GVFS/GVFS.Common/Prefetch/Git/DiffHelper.cs @@ -291,10 +291,22 @@ private void EnqueueOperationsFromDiffTreeLine(ITracer activity, string line) case DiffTreeResult.Operations.Delete: if (!this.stagedDirectoryOperations.Add(result)) { - // A directory with the same name (case-insensitive) already exists. - // On case-insensitive file systems, this means the Delete came after an Add, - // which shouldn't happen (diff-tree outputs Deletes before Adds). - // Keep the existing Add to avoid deleting a folder from under ourselves. + // A directory with the same (case-insensitive) path was already + // staged as an Add. This is a case-only rename where diff-tree + // emitted the Add before the Delete. Either emit order is possible + // because git diff-tree compares tree entries by byte order, so + // whichever casing sorts lower appears first. + // + // Annotate the staged Add with the old-cased path so CheckoutStage + // can perform the rename. Keep the Add — never the Delete — to + // avoid deleting a folder out from under ourselves. + DiffTreeResult existingOp = FindStagedDirectoryOperation(result.TargetPath); + if (existingOp != null && + !existingOp.TargetPath.Equals(result.TargetPath, StringComparison.Ordinal)) + { + existingOp.SourcePath = result.TargetPath; + this.caseRenamedDirectoryDeletes.Add(result.TargetPath.TrimEnd(Path.DirectorySeparatorChar)); + } } break; diff --git a/GVFS/GVFS.UnitTests/Data/caseChangeAddFirst.txt b/GVFS/GVFS.UnitTests/Data/caseChangeAddFirst.txt new file mode 100644 index 000000000..a78c15071 --- /dev/null +++ b/GVFS/GVFS.UnitTests/Data/caseChangeAddFirst.txt @@ -0,0 +1,10 @@ +:000000 040000 0000000000000000000000000000000000000000 d813c8227132c3bf73c013f8913f207b4876b2bf A GVFLT_MultiThreadTest +:000000 040000 0000000000000000000000000000000000000000 1260ecb71f2be8eb92ea904c6dffa3e40eaaf1bf A GVFLT_MultiThreadTest/OpenForReadsSameTime +:000000 100644 0000000000000000000000000000000000000000 eabe8d5ec569cc7e199e77411ad935f101414032 A GVFLT_MultiThreadTest/OpenForReadsSameTime/test +:000000 040000 0000000000000000000000000000000000000000 1260ecb71f2be8eb92ea904c6dffa3e40eaaf1bf A GVFLT_MultiThreadTest/OpenForWritesSameTime +:000000 100644 0000000000000000000000000000000000000000 eabe8d5ec569cc7e199e77411ad935f101414032 A GVFLT_MultiThreadTest/OpenForWritesSameTime/test +:040000 000000 d813c8227132c3bf73c013f8913f207b4876b2bf 0000000000000000000000000000000000000000 D GVFlt_MultiThreadTest +:040000 000000 1260ecb71f2be8eb92ea904c6dffa3e40eaaf1bf 0000000000000000000000000000000000000000 D GVFlt_MultiThreadTest/OpenForReadsSameTime +:100644 000000 eabe8d5ec569cc7e199e77411ad935f101414032 0000000000000000000000000000000000000000 D GVFlt_MultiThreadTest/OpenForReadsSameTime/test +:040000 000000 1260ecb71f2be8eb92ea904c6dffa3e40eaaf1bf 0000000000000000000000000000000000000000 D GVFlt_MultiThreadTest/OpenForWritesSameTime +:100644 000000 eabe8d5ec569cc7e199e77411ad935f101414032 0000000000000000000000000000000000000000 D GVFlt_MultiThreadTest/OpenForWritesSameTime/test diff --git a/GVFS/GVFS.UnitTests/GVFS.UnitTests.csproj b/GVFS/GVFS.UnitTests/GVFS.UnitTests.csproj index 0df5f3263..4e0260983 100644 --- a/GVFS/GVFS.UnitTests/GVFS.UnitTests.csproj +++ b/GVFS/GVFS.UnitTests/GVFS.UnitTests.csproj @@ -39,6 +39,9 @@ Always + + Always + Always diff --git a/GVFS/GVFS.UnitTests/Prefetch/DiffHelperTests.cs b/GVFS/GVFS.UnitTests/Prefetch/DiffHelperTests.cs index 79082986d..12072e016 100644 --- a/GVFS/GVFS.UnitTests/Prefetch/DiffHelperTests.cs +++ b/GVFS/GVFS.UnitTests/Prefetch/DiffHelperTests.cs @@ -16,41 +16,41 @@ namespace GVFS.UnitTests.Prefetch { [TestFixtureSource(typeof(DataSources), nameof(DataSources.AllBools))] public class DiffHelperTests - { + { public DiffHelperTests(bool symLinkSupport) { this.IncludeSymLinks = symLinkSupport; } - + public bool IncludeSymLinks { get; set; } - - // Make two commits. The first should look like this: - // recursiveDelete - // recursiveDelete/subfolder - // recursiveDelete/subfolder/childFile.txt - // fileToBecomeFolder - // fileToDelete.txt - // fileToEdit.txt - // fileToRename.txt - // fileToRenameEdit.txt - // folderToBeFile - // folderToBeFile/childFile.txt - // folderToDelete - // folderToDelete/childFile.txt - // folderToEdit - // folderToEdit/childFile.txt - // folderToRename - // folderToRename/childFile.txt - // symLinkToBeCreated.txt - // - // The second should follow the action indicated by the file/folder name: - // eg. recursiveDelete should run "rmdir /s/q recursiveDelete" - // eg. folderToBeFile should be deleted and replaced with a file of the same name - // Note that each childFile.txt should have unique contents, but is only a placeholder to force git to add a folder. - // - // Then to generate the diffs, run: - // git diff-tree -r -t Head~1 Head > forward.txt - // git diff-tree -r -t Head Head ~1 > backward.txt + + // Make two commits. The first should look like this: + // recursiveDelete + // recursiveDelete/subfolder + // recursiveDelete/subfolder/childFile.txt + // fileToBecomeFolder + // fileToDelete.txt + // fileToEdit.txt + // fileToRename.txt + // fileToRenameEdit.txt + // folderToBeFile + // folderToBeFile/childFile.txt + // folderToDelete + // folderToDelete/childFile.txt + // folderToEdit + // folderToEdit/childFile.txt + // folderToRename + // folderToRename/childFile.txt + // symLinkToBeCreated.txt + // + // The second should follow the action indicated by the file/folder name: + // eg. recursiveDelete should run "rmdir /s/q recursiveDelete" + // eg. folderToBeFile should be deleted and replaced with a file of the same name + // Note that each childFile.txt should have unique contents, but is only a placeholder to force git to add a folder. + // + // Then to generate the diffs, run: + // git diff-tree -r -t Head~1 Head > forward.txt + // git diff-tree -r -t Head Head ~1 > backward.txt [TestCase] public void CanParseDiffForwards() { @@ -135,6 +135,46 @@ public void ParsesCaseChangesAsAdds() diffBackwards.TotalDirectoryOperations.ShouldEqual(3); } + // Mirror of ParsesCaseChangesAsAdds for the opposite emit order: the Add + // lines appear in the diff before the Delete lines. This happens when the + // new (target) casing sorts byte-wise before the old (source) casing, e.g. + // "GVFlt_*" -> "GVFLT_*" (uppercase 'L' < lowercase 'l'). + // + // The parser must produce the same staged state regardless of which + // ordering the diff-tree output uses. + [TestCase] + [Category(CategoryConstants.CaseInsensitiveFileSystemOnly)] + public void ParsesCaseChangesWhenAddPrecedesDelete() + { + MockTracer tracer = new MockTracer(); + DiffHelper diffBackwards = new DiffHelper(tracer, new Mock.Common.MockGVFSEnlistment(), new List(), new List(), includeSymLinks: this.IncludeSymLinks); + diffBackwards.ParseDiffFile(GetDataPath("caseChangeAddFirst.txt")); + + diffBackwards.RequiredBlobs.Count.ShouldEqual(2); + diffBackwards.FileAddOperations.Sum(list => list.Value.Count).ShouldEqual(2); + + // File deletes inside case-renamed directories are filtered out by FlushStagedQueues + diffBackwards.FileDeleteOperations.Count.ShouldEqual(0); + + // File deletes are staged (not suppressed) so FlushStagedQueues can filter them properly + diffBackwards.TotalFileDeletes.ShouldEqual(2); + + diffBackwards.DirectoryOperations.ShouldNotContain(entry => entry.Operation == DiffTreeResult.Operations.Delete); + + // Only the top-level directory rename is enqueued; children are filtered because + // the parent rename moves them automatically + diffBackwards.DirectoryOperations.Count.ShouldEqual(1); + + // The enqueued directory operation should carry the old-cased path for the rename + // even though the Add was staged first and the Delete arrived second. + DiffTreeResult dirOp = diffBackwards.DirectoryOperations.First(); + dirOp.SourcePath.ShouldNotBeNull(); + Assert.AreEqual("GVFlt_MultiThreadTest" + Path.DirectorySeparatorChar, dirOp.SourcePath); + Assert.AreEqual("GVFLT_MultiThreadTest" + Path.DirectorySeparatorChar, dirOp.TargetPath); + + diffBackwards.TotalDirectoryOperations.ShouldEqual(3); + } + // Delete a folder with two sub folders each with a single file // Readd it with a different casing and same contents [TestCase] @@ -186,4 +226,4 @@ private static string GetDataPath(string fileName) return Path.Combine(workingDirectory, "Data", fileName); } } -} +} From 89e9a13d1a1c1c13057bd44eb5da2ec19162c915 Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Mon, 11 May 2026 18:18:45 -0400 Subject: [PATCH 04/13] fix: Restore source directory when case-rename second move fails MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Context: CheckoutStage performs case-only directory renames via a two-step Directory.Move: source -> temp (a sibling of the target named with a random GUID suffix), then temp -> target. The two-step is required on Windows because Directory.Move throws IOException when the source and destination differ only in case. If the first move succeeds but the second fails — for example because an antivirus or indexer has opened a handle on the moved tree, the target path has unexpected ACLs, or transient I/O fails the second call — the existing catch handler logs the error, sets HasFailures, and leaves the directory stranded at the temp path. A subsequent FastFetch run sees no source at the original casing and falls through to Directory.CreateDirectory on the target path. The end state is an empty target directory next to a "*_caseRename_" directory holding the user's actual content, with no automatic recovery. Justification: The cleanest recovery is to undo the half-applied rename: if the second move throws, attempt to move the tree back from the temp path to the original source casing. A subsequent run then sees the same state as before the failed attempt and can retry deterministically. The restoration attempt itself can fail (same underlying I/O issue that broke the second move) so it is best-effort and swallows exceptions, leaving the outer catch handler to log the original failure. Compared to alternatives — staging the rename under .git/ instead of as a sibling, or running a cleanup pass for stranded temp directories on startup — this is the smallest change that addresses the data-loss path without introducing new state. Implementation: The two Directory.Move calls are wrapped so that a failure of the second Move triggers a best-effort Directory.Move(tempPath, trimmedSourcePath). Guarded by Directory.Exists checks so we never clobber a partially-restored source. After the recovery attempt, the original exception is re-thrown so the outer catch logs the actual failure (not a synthetic restore-success). The trimmed source and target paths are now hoisted to locals to keep the inner block readable. No behavior change on the success path or for non-rename Adds. Co-Authored-By: Claude Opus 4.7 (1M context) --- GVFS/FastFetch/CheckoutStage.cs | 32 +++++++++++++++++++++++++++++--- 1 file changed, 29 insertions(+), 3 deletions(-) diff --git a/GVFS/FastFetch/CheckoutStage.cs b/GVFS/FastFetch/CheckoutStage.cs index 449738e62..3f2bff473 100644 --- a/GVFS/FastFetch/CheckoutStage.cs +++ b/GVFS/FastFetch/CheckoutStage.cs @@ -181,9 +181,35 @@ private void HandleAllDirectoryOperations() { // Directory.Move throws IOException for case-only renames, // so use a two-step rename through a temporary name. - string tempPath = absoluteTargetPath.TrimEnd(Path.DirectorySeparatorChar) + "_caseRename_" + Guid.NewGuid().ToString("N"); - Directory.Move(absoluteSourcePath.TrimEnd(Path.DirectorySeparatorChar), tempPath); - Directory.Move(tempPath, absoluteTargetPath.TrimEnd(Path.DirectorySeparatorChar)); + string trimmedSourcePath = absoluteSourcePath.TrimEnd(Path.DirectorySeparatorChar); + string trimmedTargetPath = absoluteTargetPath.TrimEnd(Path.DirectorySeparatorChar); + string tempPath = trimmedTargetPath + "_caseRename_" + Guid.NewGuid().ToString("N"); + Directory.Move(trimmedSourcePath, tempPath); + try + { + Directory.Move(tempPath, trimmedTargetPath); + } + catch + { + // The first move succeeded but the second failed. The + // directory is now at tempPath. Try to restore the + // original casing so a retry sees a consistent + // working tree; if restoration also fails, the outer + // catch will log the original exception and the temp + // directory will be left behind for manual recovery. + if (Directory.Exists(tempPath) && !Directory.Exists(trimmedSourcePath)) + { + try + { + Directory.Move(tempPath, trimmedSourcePath); + } + catch + { + } + } + + throw; + } } else { From f622278ad3718fc9262c3e11624e6ac8640219ce Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Mon, 11 May 2026 18:19:50 -0400 Subject: [PATCH 05/13] chore: Remove dead variable in EnqueueFileAddOperation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Context: EnqueueFileAddOperation iterates stagedFileDeletes to decide whether a staged delete with a matching path differs from the incoming add only in case. The loop captures the matched delete path into a local named existingDeletePath, but nothing after the loop reads that local — only exactMatch is consumed. Justification: Dead state is misleading: a future reader expects an unused capture to be the seed for a planned use (logging the old casing, returning it from a helper) and may build new code around it. Removing the local makes the loop's actual contract — set exactMatch, then break — self-evident. Implementation: Drops the existingDeletePath declaration and the one assignment that populated it. No behavior change; tests continue to pass. Co-Authored-By: Claude Opus 4.7 (1M context) --- GVFS/GVFS.Common/Prefetch/Git/DiffHelper.cs | 2 -- 1 file changed, 2 deletions(-) diff --git a/GVFS/GVFS.Common/Prefetch/Git/DiffHelper.cs b/GVFS/GVFS.Common/Prefetch/Git/DiffHelper.cs index 70ce856b4..74bd09d65 100644 --- a/GVFS/GVFS.Common/Prefetch/Git/DiffHelper.cs +++ b/GVFS/GVFS.Common/Prefetch/Git/DiffHelper.cs @@ -445,12 +445,10 @@ private void EnqueueFileAddOperation(ITracer activity, DiffTreeResult operation) { // Check if the staged delete has the exact same casing bool exactMatch = false; - string existingDeletePath = null; foreach (string deletePath in this.stagedFileDeletes) { if (deletePath.Equals(operation.TargetPath, GVFSPlatform.Instance.Constants.PathComparison)) { - existingDeletePath = deletePath; if (deletePath.Equals(operation.TargetPath, StringComparison.Ordinal)) { exactMatch = true; From 826bd90152a4ead371833f16752794dd2efc4a6a Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Mon, 11 May 2026 18:28:10 -0400 Subject: [PATCH 06/13] refactor: Guard DiffHelper against accidental reuse MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Context: DiffHelper accumulates diff results into internal staged sets (stagedDirectoryOperations, stagedFileDeletes, filesAdded, and the new caseRenamedDirectoryDeletes) and into four public output collections: DirectoryOperations, FileDeleteOperations, FileAddOperations, and RequiredBlobs. FlushStagedQueues calls RequiredBlobs.CompleteAdding() at the end of each diff, which permanently closes the BlockingCollection. A second call to PerformDiff or ParseDiffFile on the same instance would also flow values into the now-frozen internal staged sets, which were never cleared between runs. caseRenamedDirectoryDeletes in particular would carry forward old-cased paths from the first diff and mis-filter children in the second. Today every caller in the tree constructs a fresh DiffHelper per diff, so the bug is latent. But the class never declared this constraint, the field initializers and the BlockingCollection together silently assume single-use, and a future caller could re-enter the methods expecting a usable result. Justification: Two approaches were considered: (a) reset internal staged sets at the start of each call and reinstantiate the output collections, or (b) detect reuse and throw at the entry points. Option (a) would invalidate any external code holding references to the output collections (they are exposed as public properties and consumed concurrently by other prefetch stages), so it would silently change semantics for anyone observing the queues across the boundary. Option (b) preserves the existing single-use intent, surfaces misuse loudly and early instead of via an obscure BlockingCollection exception deep in parsing, and requires no behavior change for current callers. Implementation: Adds a diffPerformed bool, set at the start of EnsureSingleUse — the new private helper called at the top of PerformDiff(string, string) and ParseDiffFile. A second invocation throws InvalidOperationException with a message that names the class and suggests constructing a new instance. The one-arg PerformDiff overload delegates to the two-arg overload and so is covered transitively. A new unit test DiffHelperThrowsOnReuse calls ParseDiffFile twice on the same instance and asserts the second call throws. Co-Authored-By: Claude Opus 4.7 (1M context) --- GVFS/GVFS.Common/Prefetch/Git/DiffHelper.cs | 21 +++++++++++++++++++ .../Prefetch/DiffHelperTests.cs | 10 +++++++++ 2 files changed, 31 insertions(+) diff --git a/GVFS/GVFS.Common/Prefetch/Git/DiffHelper.cs b/GVFS/GVFS.Common/Prefetch/Git/DiffHelper.cs index 74bd09d65..3737baaa4 100644 --- a/GVFS/GVFS.Common/Prefetch/Git/DiffHelper.cs +++ b/GVFS/GVFS.Common/Prefetch/Git/DiffHelper.cs @@ -27,6 +27,7 @@ public class DiffHelper private Enlistment enlistment; private GitProcess git; + private bool diffPerformed; public DiffHelper(ITracer tracer, Enlistment enlistment, IEnumerable fileList, IEnumerable folderList, bool includeSymLinks) : this(tracer, enlistment, new GitProcess(enlistment), fileList, folderList, includeSymLinks) @@ -97,6 +98,7 @@ public void PerformDiff(string targetCommitSha) public void PerformDiff(string sourceTreeSha, string targetTreeSha) { + this.EnsureSingleUse(); EventMetadata metadata = new EventMetadata(); metadata.Add("TargetTreeSha", targetTreeSha); metadata.Add("HeadTreeSha", sourceTreeSha); @@ -154,6 +156,7 @@ public void PerformDiff(string sourceTreeSha, string targetTreeSha) public void ParseDiffFile(string filename) { + this.EnsureSingleUse(); using (ITracer activity = this.tracer.StartActivity("PerformDiff", EventLevel.Informational)) { using (StreamReader file = new StreamReader(File.OpenRead(filename))) @@ -479,6 +482,24 @@ private void EnqueueFileAddOperation(ITracer activity, DiffTreeResult operation) this.RequiredBlobs.Add(operation.TargetSha); } + private void EnsureSingleUse() + { + // The output collections — DirectoryOperations, FileDeleteOperations, + // FileAddOperations, RequiredBlobs — are populated incrementally and + // RequiredBlobs.CompleteAdding() is called at the end of FlushStagedQueues. + // A second call would attempt to add to a completed BlockingCollection + // and throw deep in the parsing path, leaving partial output. The class + // is therefore intended to be single-use; instantiate a new DiffHelper + // for each diff. + if (this.diffPerformed) + { + throw new InvalidOperationException( + $"{nameof(DiffHelper)} has already produced a diff and cannot be reused. Construct a new instance."); + } + + this.diffPerformed = true; + } + private DiffTreeResult FindStagedDirectoryOperation(string targetPath) { foreach (DiffTreeResult staged in this.stagedDirectoryOperations) diff --git a/GVFS/GVFS.UnitTests/Prefetch/DiffHelperTests.cs b/GVFS/GVFS.UnitTests/Prefetch/DiffHelperTests.cs index 12072e016..0674f7f8c 100644 --- a/GVFS/GVFS.UnitTests/Prefetch/DiffHelperTests.cs +++ b/GVFS/GVFS.UnitTests/Prefetch/DiffHelperTests.cs @@ -196,6 +196,16 @@ public void ParsesCaseChangesAsRenames() diffBackwards.TotalDirectoryOperations.ShouldEqual(6); } + [TestCase] + public void DiffHelperThrowsOnReuse() + { + MockTracer tracer = new MockTracer(); + DiffHelper diff = new DiffHelper(tracer, new Mock.Common.MockGVFSEnlistment(), new List(), new List(), includeSymLinks: this.IncludeSymLinks); + diff.ParseDiffFile(GetDataPath("forward.txt")); + + Assert.Throws(() => diff.ParseDiffFile(GetDataPath("forward.txt"))); + } + [TestCase] public void DetectsFailuresInDiffTree() { From a120b442ae5939b02b83977c038fef1a4e8248dc Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Mon, 11 May 2026 18:29:07 -0400 Subject: [PATCH 07/13] refactor: Rename caseRenamedDirectoryDeletes to convey purpose MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Context: The internal HashSet introduced for case-rename support was named caseRenamedDirectoryDeletes, but it does not store Delete operations or paths of directories slated for deletion. It stores the old-cased TargetPath of each directory whose Delete entry was collapsed into a case-rename Add — paths that FlushStagedQueues uses to recognize and filter out child operations that would otherwise look like they belong to a deleted directory. The mismatch between the name and the contents made the two read sites — FlushStagedQueues's parent-path check and the Modify/Add and Delete collision branches — harder to follow on first reading. The field is also referenced in the comment chain of two new commits, so readers chasing context bounce between "deletes" in the name and "replaced by a rename" in the comments. Justification: Renaming clarifies what the set represents at every call site. The new name "directoriesReplacedByCaseRename" reads as a description of its contents and aligns with the comment that already explains it. The XML-comment block above the field is also expanded so the contract — *old-cased paths, used to suppress child operations* — is documented in one place instead of being implied across three unrelated branches. Implementation: Mechanical rename of all three references (the field declaration, the UnionWith call in FlushStagedQueues, the Add calls in the Modify/Add and Delete collision branches). No behavior change. Co-Authored-By: Claude Opus 4.7 (1M context) --- GVFS/GVFS.Common/Prefetch/Git/DiffHelper.cs | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/GVFS/GVFS.Common/Prefetch/Git/DiffHelper.cs b/GVFS/GVFS.Common/Prefetch/Git/DiffHelper.cs index 3737baaa4..79ef07afb 100644 --- a/GVFS/GVFS.Common/Prefetch/Git/DiffHelper.cs +++ b/GVFS/GVFS.Common/Prefetch/Git/DiffHelper.cs @@ -21,9 +21,12 @@ public class DiffHelper private HashSet stagedDirectoryOperations = new HashSet(new DiffTreeByNameComparer()); private HashSet stagedFileDeletes = new HashSet(GVFSPlatform.Instance.Constants.PathComparer); - // Tracks directories that were deleted as part of a case-only rename. - // Used by FlushStagedQueues to filter out child file deletes inside case-renamed directories. - private HashSet caseRenamedDirectoryDeletes = new HashSet(GVFSPlatform.Instance.Constants.PathComparer); + // Holds the old-cased paths of directories whose Delete was collapsed into an + // Add via case-only rename detection. FlushStagedQueues consults this set to + // suppress child operations (file deletes and child directory Adds) whose + // parent was case-renamed — those children are carried by the parent rename + // on disk and do not need separate operations. + private HashSet directoriesReplacedByCaseRename = new HashSet(GVFSPlatform.Instance.Constants.PathComparer); private Enlistment enlistment; private GitProcess git; @@ -185,7 +188,7 @@ private void FlushStagedQueues() // Also include directories that were deleted as part of case-only renames. // These were replaced by Adds in stagedDirectoryOperations but their children's // file deletes should still be filtered out (the parent rename handles them). - deletedDirectories.UnionWith(this.caseRenamedDirectoryDeletes); + deletedDirectories.UnionWith(this.directoriesReplacedByCaseRename); foreach (DiffTreeResult result in this.stagedDirectoryOperations) { @@ -197,7 +200,7 @@ private void FlushStagedQueues() // For case renames, child directory Adds inside a case-renamed parent // are expected. The parent rename will handle moving the children. // Only warn if the parent is truly deleted (not case-renamed). - if (!this.caseRenamedDirectoryDeletes.Contains(parentPath)) + if (!this.directoriesReplacedByCaseRename.Contains(parentPath)) { EventMetadata metadata = new EventMetadata(); metadata.Add(nameof(result.TargetPath), result.TargetPath); @@ -308,7 +311,7 @@ private void EnqueueOperationsFromDiffTreeLine(ITracer activity, string line) !existingOp.TargetPath.Equals(result.TargetPath, StringComparison.Ordinal)) { existingOp.SourcePath = result.TargetPath; - this.caseRenamedDirectoryDeletes.Add(result.TargetPath.TrimEnd(Path.DirectorySeparatorChar)); + this.directoriesReplacedByCaseRename.Add(result.TargetPath.TrimEnd(Path.DirectorySeparatorChar)); } } @@ -326,7 +329,7 @@ private void EnqueueOperationsFromDiffTreeLine(ITracer activity, string line) { // Case-only rename: store the old-cased path so CheckoutStage can rename the directory result.SourcePath = existingOp.TargetPath; - this.caseRenamedDirectoryDeletes.Add(existingOp.TargetPath.TrimEnd(Path.DirectorySeparatorChar)); + this.directoriesReplacedByCaseRename.Add(existingOp.TargetPath.TrimEnd(Path.DirectorySeparatorChar)); } // Replace the delete with the add to make sure we don't delete a folder from under ourselves From 3e80cc745ed25bbc68e68b790e9f285f8ba6d638 Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Mon, 11 May 2026 18:30:15 -0400 Subject: [PATCH 08/13] refactor: Restrict DiffTreeResult.SourcePath setter to assembly MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Context: DiffTreeResult.SourcePath was introduced as a parser annotation: DiffHelper sets it when it collapses a Delete+Add pair under the case-insensitive comparer into a single case-rename Add, and CheckoutStage reads it to perform the rename on disk. The setter was declared public, which exposed a runtime-only field as part of the type's external contract. Public mutability invites future code outside DiffHelper to set the field on operations the parser would never have annotated — for example on a Delete or a Modify entry, or on a file operation — and CheckoutStage's branches would then behave in ways that were never validated. The XML doc previously said only "Used on case- insensitive file systems to carry the old-cased path for directory renames," which did not communicate the intended single-producer contract. Justification: Restricting the setter to the assembly keeps the producer (DiffHelper) able to write it while ruling out external producers. The getter stays public because consumers in other assemblies (CheckoutStage in FastFetch, the existing DiffHelperTests assertions) need to read it. The single-producer rule is also documented in an expanded XML comment that names the parser and describes when the field is null versus non-null. Implementation: Changes "public string SourcePath { get; set; }" to "public string SourcePath { get; internal set; }" and rewrites the XML doc to describe the contract: who sets it, who reads it, when it is null. No build-time consumer outside the assembly was writing to it (only DiffHelper writes; CheckoutStage and the unit tests read), so this is a non-breaking visibility tightening. Co-Authored-By: Claude Opus 4.7 (1M context) --- GVFS/GVFS.Common/Git/DiffTreeResult.cs | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/GVFS/GVFS.Common/Git/DiffTreeResult.cs b/GVFS/GVFS.Common/Git/DiffTreeResult.cs index 1fc0d2a5d..abafa4597 100644 --- a/GVFS/GVFS.Common/Git/DiffTreeResult.cs +++ b/GVFS/GVFS.Common/Git/DiffTreeResult.cs @@ -39,10 +39,16 @@ public enum Operations public ushort TargetMode { get; set; } /// - /// When set, indicates this operation is a case-only rename from SourcePath to TargetPath. - /// Used on case-insensitive file systems to carry the old-cased path for directory renames. + /// Old-cased path of a case-only directory rename, set by DiffHelper when + /// collapsing a Delete+Add pair under the case-insensitive comparer. When + /// non-null the operation represents a rename from SourcePath to TargetPath + /// and consumers (currently CheckoutStage) must rename the directory on + /// disk instead of treating the operation as a plain Add. Always null for + /// file operations, Modify, Delete, and non-rename Add entries. The setter + /// is intentionally restricted to the assembly so only the parser can + /// produce this annotation. /// - public string SourcePath { get; set; } + public string SourcePath { get; internal set; } public static DiffTreeResult ParseFromDiffTreeLine(string line) { From bd6bcd86e3f5727c3075295c7f9b1e8b2a16da62 Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Mon, 11 May 2026 18:33:43 -0400 Subject: [PATCH 09/13] refactor: Replace HashSet linear scans with Dictionary lookups in DiffHelper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Context: Case-rename detection needs both case-insensitive containment (do we have *some* entry matching this path under PathComparer?) and case-sensitive comparison (does the stored casing exactly match the incoming path?). HashSet provides the first in O(1) but the second required a foreach scan of the entire set because HashSet has no API to retrieve the stored value from a case-insensitive lookup. Three diff-tree code paths inherited that scan: - EnqueueFileAddOperation, scanning stagedFileDeletes when an add arrives whose path matches a staged delete case-insensitively - EnqueueFileDeleteOperation, scanning filesAdded for the same reason on the inverse direction - The Modify/Add and Delete collision branches in EnqueueOperationsFromDiffTreeLine, scanning stagedDirectoryOperations via FindStagedDirectoryOperation to recover the staged DiffTreeResult and its TargetPath casing These scans are gated by a case-insensitive Contains check, so the inner loop only runs when there is actually a collision — but on each collision it walks every staged entry. The staged sets can reach millions of paths on first checkouts of large repositories (the Windows monorepo, Office, etc.), so a worst-case parse with many collisions degrades from O(N) to O(N^2). Justification: Dictionary keyed by GVFSPlatform.Instance.Constants.PathComparer solves both needs at once: the comparer drives O(1) case-insensitive key matching, and TryGetValue returns the originally stored value so callers can compare its casing ordinally without iterating. The storage shape lines up exactly with what the existing logic needs. DiffTreeByNameComparer becomes unused after stagedDirectoryOperations is rekeyed by TargetPath string, so it is removed in this commit along with FindStagedDirectoryOperation (now a single TryGetValue call). No public surface area changes. Implementation: - filesAdded, stagedDirectoryOperations, and stagedFileDeletes are declared as Dictionary instances. The two string-valued dictionaries map a path under the case-insensitive comparer to its original casing; the directory-valued dictionary maps the case-insensitive path to its staged DiffTreeResult. - HashSet.Add returning false becomes Dictionary.TryAdd returning false at every call site. The Remove+Add idiom used to "replace" a staged entry becomes a single indexer assignment. - Two foreach scans collapse into TryGetValue + ordinal compare; the collision branches in EnqueueOperationsFromDiffTreeLine consult the dictionary directly via the indexer because the immediately preceding TryAdd has already proven the key is present. - FlushStagedQueues iterates .Values for both stagedDirectoryOperations and stagedFileDeletes. - The dead helper FindStagedDirectoryOperation and the DiffTreeByNameComparer inner class are deleted. All tests continue to pass; no behavior change is intended beyond the performance characteristic. Co-Authored-By: Claude Opus 4.7 (1M context) --- GVFS/GVFS.Common/Prefetch/Git/DiffHelper.cs | 146 ++++++-------------- 1 file changed, 39 insertions(+), 107 deletions(-) diff --git a/GVFS/GVFS.Common/Prefetch/Git/DiffHelper.cs b/GVFS/GVFS.Common/Prefetch/Git/DiffHelper.cs index 79ef07afb..1ba4fad11 100644 --- a/GVFS/GVFS.Common/Prefetch/Git/DiffHelper.cs +++ b/GVFS/GVFS.Common/Prefetch/Git/DiffHelper.cs @@ -16,10 +16,17 @@ public class DiffHelper private HashSet exactFileList; private List patternList; private List folderList; - private HashSet filesAdded = new HashSet(GVFSPlatform.Instance.Constants.PathComparer); - - private HashSet stagedDirectoryOperations = new HashSet(new DiffTreeByNameComparer()); - private HashSet stagedFileDeletes = new HashSet(GVFSPlatform.Instance.Constants.PathComparer); + // The staged collections are keyed by the case-insensitive PathComparer on + // case-insensitive platforms so that two paths differing only in case map to + // the same entry. The dictionary value stores the original casing of the + // first path inserted, which case-rename detection compares against the + // incoming path to decide whether the collision is a rename or a true + // duplicate. Dictionary lookups keep this O(1); a HashSet would force a + // linear scan to recover the stored casing. + private Dictionary filesAdded = new Dictionary(GVFSPlatform.Instance.Constants.PathComparer); + + private Dictionary stagedDirectoryOperations = new Dictionary(GVFSPlatform.Instance.Constants.PathComparer); + private Dictionary stagedFileDeletes = new Dictionary(GVFSPlatform.Instance.Constants.PathComparer); // Holds the old-cased paths of directories whose Delete was collapsed into an // Add via case-only rename detection. FlushStagedQueues consults this set to @@ -180,7 +187,7 @@ private void FlushStagedQueues() { HashSet deletedDirectories = new HashSet( - this.stagedDirectoryOperations + this.stagedDirectoryOperations.Values .Where(d => d.Operation == DiffTreeResult.Operations.Delete) .Select(d => d.TargetPath.TrimEnd(Path.DirectorySeparatorChar)), GVFSPlatform.Instance.Constants.PathComparer); @@ -190,7 +197,7 @@ private void FlushStagedQueues() // file deletes should still be filtered out (the parent rename handles them). deletedDirectories.UnionWith(this.directoriesReplacedByCaseRename); - foreach (DiffTreeResult result in this.stagedDirectoryOperations) + foreach (DiffTreeResult result in this.stagedDirectoryOperations.Values) { string parentPath = Path.GetDirectoryName(result.TargetPath.TrimEnd(Path.DirectorySeparatorChar)); if (deletedDirectories.Contains(parentPath)) @@ -215,7 +222,7 @@ private void FlushStagedQueues() } } - foreach (string filePath in this.stagedFileDeletes) + foreach (string filePath in this.stagedFileDeletes.Values) { string parentPath = Path.GetDirectoryName(filePath); if (!deletedDirectories.Contains(parentPath)) @@ -243,16 +250,16 @@ private void EnqueueOperationsFromLsTreeLine(ITracer activity, string line) if (result.TargetIsDirectory) { - if (!this.stagedDirectoryOperations.Add(result)) + if (!this.stagedDirectoryOperations.TryAdd(result.TargetPath, result)) { EventMetadata metadata = new EventMetadata(); metadata.Add(nameof(result.TargetPath), result.TargetPath); metadata.Add(TracingConstants.MessageKey.WarningMessage, "File exists in tree with two different cases. Taking the last one."); this.tracer.RelatedEvent(EventLevel.Warning, "CaseConflict", metadata); - // Since we match only on filename, re-adding is the easiest way to update the set. - this.stagedDirectoryOperations.Remove(result); - this.stagedDirectoryOperations.Add(result); + // Two entries in the same tree differ only in case. Keep the + // last one parsed, matching the historical HashSet behavior. + this.stagedDirectoryOperations[result.TargetPath] = result; } } else @@ -295,7 +302,7 @@ private void EnqueueOperationsFromDiffTreeLine(ITracer activity, string line) switch (result.Operation) { case DiffTreeResult.Operations.Delete: - if (!this.stagedDirectoryOperations.Add(result)) + if (!this.stagedDirectoryOperations.TryAdd(result.TargetPath, result)) { // A directory with the same (case-insensitive) path was already // staged as an Add. This is a case-only rename where diff-tree @@ -306,9 +313,8 @@ private void EnqueueOperationsFromDiffTreeLine(ITracer activity, string line) // Annotate the staged Add with the old-cased path so CheckoutStage // can perform the rename. Keep the Add — never the Delete — to // avoid deleting a folder out from under ourselves. - DiffTreeResult existingOp = FindStagedDirectoryOperation(result.TargetPath); - if (existingOp != null && - !existingOp.TargetPath.Equals(result.TargetPath, StringComparison.Ordinal)) + DiffTreeResult existingOp = this.stagedDirectoryOperations[result.TargetPath]; + if (!existingOp.TargetPath.Equals(result.TargetPath, StringComparison.Ordinal)) { existingOp.SourcePath = result.TargetPath; this.directoriesReplacedByCaseRename.Add(result.TargetPath.TrimEnd(Path.DirectorySeparatorChar)); @@ -318,14 +324,12 @@ private void EnqueueOperationsFromDiffTreeLine(ITracer activity, string line) break; case DiffTreeResult.Operations.Add: case DiffTreeResult.Operations.Modify: - if (!this.stagedDirectoryOperations.Add(result)) + if (!this.stagedDirectoryOperations.TryAdd(result.TargetPath, result)) { // A directory with the same path (case-insensitive) was already staged. // This is a case-only rename: the Delete was staged first, now the Add arrives. - // Find the existing entry to capture the old-cased path. - DiffTreeResult existingOp = FindStagedDirectoryOperation(result.TargetPath); - if (existingOp != null && - !existingOp.TargetPath.Equals(result.TargetPath, StringComparison.Ordinal)) + DiffTreeResult existingOp = this.stagedDirectoryOperations[result.TargetPath]; + if (!existingOp.TargetPath.Equals(result.TargetPath, StringComparison.Ordinal)) { // Case-only rename: store the old-cased path so CheckoutStage can rename the directory result.SourcePath = existingOp.TargetPath; @@ -333,8 +337,7 @@ private void EnqueueOperationsFromDiffTreeLine(ITracer activity, string line) } // Replace the delete with the add to make sure we don't delete a folder from under ourselves - this.stagedDirectoryOperations.Remove(result); - this.stagedDirectoryOperations.Add(result); + this.stagedDirectoryOperations[result.TargetPath] = result; } break; @@ -401,28 +404,15 @@ private void EnqueueFileDeleteOperation(ITracer activity, string targetPath) // this is a true duplicate, not a case rename. Skip it. // But if it matches case-insensitively only, this is a case rename — allow the delete through // so the old-cased file is removed before the new-cased file is written. - if (this.filesAdded.Contains(targetPath)) + if (this.filesAdded.TryGetValue(targetPath, out string existingAddedPath) && + existingAddedPath.Equals(targetPath, StringComparison.Ordinal)) { - // Check if any added file matches with exact (ordinal) casing - bool exactMatch = false; - foreach (string addedPath in this.filesAdded) - { - if (addedPath.Equals(targetPath, StringComparison.Ordinal)) - { - exactMatch = true; - break; - } - } - - if (exactMatch) - { - return; - } - - // Case-only difference: allow the delete so the old casing is removed from disk + return; } - this.stagedFileDeletes.Add(targetPath); + // Either no prior add, or a case-only difference: allow the delete to be + // staged so the old casing is removed from disk before the new add lands. + this.stagedFileDeletes.TryAdd(targetPath, targetPath); } /// @@ -432,7 +422,7 @@ private void EnqueueFileAddOperation(ITracer activity, DiffTreeResult operation) { // Each filepath should be unique according to GVFSPlatform.Instance.Constants.PathComparer. // If there are duplicates, only the last parsed one should remain. - if (!this.filesAdded.Add(operation.TargetPath)) + if (!this.filesAdded.TryAdd(operation.TargetPath, operation.TargetPath)) { foreach (KeyValuePair> kvp in this.FileAddOperations) { @@ -444,33 +434,14 @@ private void EnqueueFileAddOperation(ITracer activity, DiffTreeResult operation) } } - // On case-insensitive file systems, stagedFileDeletes uses case-insensitive comparison. - // Check if there's a staged delete that differs only in case — if so, keep it - // so the old-cased file is removed from disk before the new one is written. - if (this.stagedFileDeletes.Contains(operation.TargetPath)) + // If a delete is already staged for the same path under the case-insensitive + // comparer, decide whether this is a true duplicate (same casing → drop the + // delete) or a case-only rename (different casing → keep the delete so the + // old casing is removed from disk before the new add lands). + if (this.stagedFileDeletes.TryGetValue(operation.TargetPath, out string existingDeletePath) && + existingDeletePath.Equals(operation.TargetPath, StringComparison.Ordinal)) { - // Check if the staged delete has the exact same casing - bool exactMatch = false; - foreach (string deletePath in this.stagedFileDeletes) - { - if (deletePath.Equals(operation.TargetPath, GVFSPlatform.Instance.Constants.PathComparison)) - { - if (deletePath.Equals(operation.TargetPath, StringComparison.Ordinal)) - { - exactMatch = true; - } - - break; - } - } - - if (exactMatch) - { - // Same exact path: true delete+add, not a case rename. Remove the delete. - this.stagedFileDeletes.Remove(operation.TargetPath); - } - - // Case-only difference: keep the delete staged so old casing is removed from disk + this.stagedFileDeletes.Remove(operation.TargetPath); } this.FileAddOperations.AddOrUpdate( @@ -503,44 +474,5 @@ private void EnsureSingleUse() this.diffPerformed = true; } - private DiffTreeResult FindStagedDirectoryOperation(string targetPath) - { - foreach (DiffTreeResult staged in this.stagedDirectoryOperations) - { - if (staged.TargetPath.Equals(targetPath, GVFSPlatform.Instance.Constants.PathComparison)) - { - return staged; - } - } - - return null; - } - - private class DiffTreeByNameComparer : IEqualityComparer - { - public bool Equals(DiffTreeResult x, DiffTreeResult y) - { - if (x.TargetPath != null) - { - if (y.TargetPath != null) - { - return x.TargetPath.Equals(y.TargetPath, GVFSPlatform.Instance.Constants.PathComparison); - } - - return false; - } - else - { - // both null means they're equal - return y.TargetPath == null; - } - } - - public int GetHashCode(DiffTreeResult obj) - { - return obj.TargetPath != null ? - GVFSPlatform.Instance.Constants.PathComparer.GetHashCode(obj.TargetPath) : 0; - } - } } } From abaa757a4b21d79a857e289cb970236d6b2bdbf4 Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Mon, 11 May 2026 18:35:30 -0400 Subject: [PATCH 10/13] feat: Restore tracing for case renames and duplicate diff entries MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Context: The original case-rename commit on this branch removed three RelatedEvent(Warning, "CaseConflict", ...) calls from the diff-tree parsing path. Those traces had two jobs: signaling that a case-only rename had been observed, and signaling a true duplicate diff entry for the same path. After removal, both outcomes were silent — case renames now happened invisibly, and any genuine diff anomaly that collapsed into a "case rename" was undetectable in logs. Silent success is mildly annoying for an operator; silent anomaly is a debuggability hole, because the same code path now handles two distinct conditions and the original logs no longer distinguish them. Justification: The two conditions deserve different log levels: a case rename is expected on case-insensitive filesystems and should be Informational so the operator can count them per fetch without alarm; a true duplicate (incoming and staged paths match ordinally) is rare and suggests a malformed diff stream, so it should be Warning so it surfaces in normal triage. The original "CaseConflict" event name conflated both. Replacing it with two distinct events ("CaseRename" and "DuplicateDiffEntry") gives each its own grep target without breaking the surviving ls-tree path's "CaseConflict" event (which keeps its original meaning — two sibling entries in the same tree differing only in case). Implementation: Two small static helpers — TraceCaseRename and TraceDuplicate — build EventMetadata with the kind ("File" or "Directory"), the operation, and the old/new paths or the conflicting path. Each collision branch in EnqueueOperationsFromDiffTreeLine and the case-rename branches in EnqueueFileAddOperation / EnqueueFileDeleteOperation now call one of the helpers depending on whether the comparison is ordinally equal or not. No behavior change beyond observability: the staged data structures, the FlushStagedQueues output, and the existing unit tests are unaffected. Co-Authored-By: Claude Opus 4.7 (1M context) --- GVFS/GVFS.Common/Prefetch/Git/DiffHelper.cs | 53 ++++++++++++++++++--- 1 file changed, 47 insertions(+), 6 deletions(-) diff --git a/GVFS/GVFS.Common/Prefetch/Git/DiffHelper.cs b/GVFS/GVFS.Common/Prefetch/Git/DiffHelper.cs index 1ba4fad11..386e4c214 100644 --- a/GVFS/GVFS.Common/Prefetch/Git/DiffHelper.cs +++ b/GVFS/GVFS.Common/Prefetch/Git/DiffHelper.cs @@ -318,6 +318,11 @@ private void EnqueueOperationsFromDiffTreeLine(ITracer activity, string line) { existingOp.SourcePath = result.TargetPath; this.directoriesReplacedByCaseRename.Add(result.TargetPath.TrimEnd(Path.DirectorySeparatorChar)); + TraceCaseRename(activity, "Directory", oldPath: result.TargetPath, newPath: existingOp.TargetPath); + } + else + { + TraceDuplicate(activity, "Directory", "Delete", result.TargetPath); } } @@ -334,6 +339,11 @@ private void EnqueueOperationsFromDiffTreeLine(ITracer activity, string line) // Case-only rename: store the old-cased path so CheckoutStage can rename the directory result.SourcePath = existingOp.TargetPath; this.directoriesReplacedByCaseRename.Add(existingOp.TargetPath.TrimEnd(Path.DirectorySeparatorChar)); + TraceCaseRename(activity, "Directory", oldPath: existingOp.TargetPath, newPath: result.TargetPath); + } + else + { + TraceDuplicate(activity, "Directory", result.Operation.ToString(), result.TargetPath); } // Replace the delete with the add to make sure we don't delete a folder from under ourselves @@ -404,10 +414,15 @@ private void EnqueueFileDeleteOperation(ITracer activity, string targetPath) // this is a true duplicate, not a case rename. Skip it. // But if it matches case-insensitively only, this is a case rename — allow the delete through // so the old-cased file is removed before the new-cased file is written. - if (this.filesAdded.TryGetValue(targetPath, out string existingAddedPath) && - existingAddedPath.Equals(targetPath, StringComparison.Ordinal)) + if (this.filesAdded.TryGetValue(targetPath, out string existingAddedPath)) { - return; + if (existingAddedPath.Equals(targetPath, StringComparison.Ordinal)) + { + TraceDuplicate(activity, "File", "Delete", targetPath); + return; + } + + TraceCaseRename(activity, "File", oldPath: targetPath, newPath: existingAddedPath); } // Either no prior add, or a case-only difference: allow the delete to be @@ -438,10 +453,17 @@ private void EnqueueFileAddOperation(ITracer activity, DiffTreeResult operation) // comparer, decide whether this is a true duplicate (same casing → drop the // delete) or a case-only rename (different casing → keep the delete so the // old casing is removed from disk before the new add lands). - if (this.stagedFileDeletes.TryGetValue(operation.TargetPath, out string existingDeletePath) && - existingDeletePath.Equals(operation.TargetPath, StringComparison.Ordinal)) + if (this.stagedFileDeletes.TryGetValue(operation.TargetPath, out string existingDeletePath)) { - this.stagedFileDeletes.Remove(operation.TargetPath); + if (existingDeletePath.Equals(operation.TargetPath, StringComparison.Ordinal)) + { + TraceDuplicate(activity, "File", "Add", operation.TargetPath); + this.stagedFileDeletes.Remove(operation.TargetPath); + } + else + { + TraceCaseRename(activity, "File", oldPath: existingDeletePath, newPath: operation.TargetPath); + } } this.FileAddOperations.AddOrUpdate( @@ -456,6 +478,25 @@ private void EnqueueFileAddOperation(ITracer activity, DiffTreeResult operation) this.RequiredBlobs.Add(operation.TargetSha); } + private static void TraceCaseRename(ITracer activity, string kind, string oldPath, string newPath) + { + EventMetadata metadata = new EventMetadata(); + metadata.Add("Kind", kind); + metadata.Add("OldPath", oldPath); + metadata.Add("NewPath", newPath); + activity.RelatedEvent(EventLevel.Informational, "CaseRename", metadata); + } + + private static void TraceDuplicate(ITracer activity, string kind, string operation, string targetPath) + { + EventMetadata metadata = new EventMetadata(); + metadata.Add("Kind", kind); + metadata.Add("Operation", operation); + metadata.Add(nameof(targetPath), targetPath); + metadata.Add(TracingConstants.MessageKey.WarningMessage, "Duplicate diff entry for the same path; later occurrence collapsed into earlier."); + activity.RelatedEvent(EventLevel.Warning, "DuplicateDiffEntry", metadata); + } + private void EnsureSingleUse() { // The output collections — DirectoryOperations, FileDeleteOperations, From f6c0cd714ca24ce86f5775863e6e1c44b6bbab38 Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Mon, 11 May 2026 18:37:01 -0400 Subject: [PATCH 11/13] refactor: Extract case-only directory rename to ApplyCaseOnlyDirectoryRename MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Context: HandleAllDirectoryOperations dequeues directory operations from the diff and dispatches them by Operation kind. The Modify/Add arm grew to a ~50-line inline body covering three subcases: a case-only rename (two-step Directory.Move through a temp path, plus rollback if the second move fails), a fallback for missing source (a parent rename already moved this child), and the plain Add path. That body sits inside a try/catch that also handles the plain Add, and the catch block walks the same treeOp.SourcePath branches to shape its EventMetadata. As the rename logic grew (rollback on failure, separate trimmed-path locals, nested try/catch), the overall switch arm became hard to scan: a reader has to mentally peel four levels of nesting to find the simple "is this an Add or a rename?" decision. Justification: The rename has a clear contract — given a DiffTreeResult with SourcePath set, apply it on disk — that fits naturally into a private method. Extracting it leaves the switch arm at one level of nesting and keeps the catch block at the call site so its EventMetadata shaping does not duplicate inside the helper. The behavior of the existing branches is preserved exactly, including the rollback-then-rethrow path so the outer catch still sees the original exception (not a synthesized restore failure). Implementation: ApplyCaseOnlyDirectoryRename takes the DiffTreeResult and the already-computed absoluteTargetPath. It returns early after a Directory.CreateDirectory fallback when the source is missing, and otherwise performs the trimmed-path two-step Move with the inner rollback try/catch. The caller's switch arm now reads as: if (treeOp.SourcePath != null) { this.ApplyCaseOnlyDirectoryRename(treeOp, absoluteTargetPath); } else { Directory.CreateDirectory(absoluteTargetPath); } No tests were added; all 796 existing tests continue to pass. Co-Authored-By: Claude Opus 4.7 (1M context) --- GVFS/FastFetch/CheckoutStage.cs | 99 +++++++++++++++++++-------------- 1 file changed, 57 insertions(+), 42 deletions(-) diff --git a/GVFS/FastFetch/CheckoutStage.cs b/GVFS/FastFetch/CheckoutStage.cs index 3f2bff473..1012b29ff 100644 --- a/GVFS/FastFetch/CheckoutStage.cs +++ b/GVFS/FastFetch/CheckoutStage.cs @@ -175,48 +175,7 @@ private void HandleAllDirectoryOperations() { if (treeOp.SourcePath != null) { - // Case-only rename: rename the directory from old casing to new casing - string absoluteSourcePath = Path.Combine(this.enlistment.WorkingDirectoryBackingRoot, treeOp.SourcePath); - if (Directory.Exists(absoluteSourcePath)) - { - // Directory.Move throws IOException for case-only renames, - // so use a two-step rename through a temporary name. - string trimmedSourcePath = absoluteSourcePath.TrimEnd(Path.DirectorySeparatorChar); - string trimmedTargetPath = absoluteTargetPath.TrimEnd(Path.DirectorySeparatorChar); - string tempPath = trimmedTargetPath + "_caseRename_" + Guid.NewGuid().ToString("N"); - Directory.Move(trimmedSourcePath, tempPath); - try - { - Directory.Move(tempPath, trimmedTargetPath); - } - catch - { - // The first move succeeded but the second failed. The - // directory is now at tempPath. Try to restore the - // original casing so a retry sees a consistent - // working tree; if restoration also fails, the outer - // catch will log the original exception and the temp - // directory will be left behind for manual recovery. - if (Directory.Exists(tempPath) && !Directory.Exists(trimmedSourcePath)) - { - try - { - Directory.Move(tempPath, trimmedSourcePath); - } - catch - { - } - } - - throw; - } - } - else - { - // Parent directory may have already been renamed, fixing this child's path. - // Just ensure the directory exists. - Directory.CreateDirectory(absoluteTargetPath); - } + this.ApplyCaseOnlyDirectoryRename(treeOp, absoluteTargetPath); } else { @@ -275,6 +234,62 @@ private void HandleAllDirectoryOperations() } } + /// + /// Apply a case-only directory rename produced by DiffHelper, where + /// .SourcePath carries the old casing and + /// is the new (post-rename) absolute path. + /// + /// Directory.Move throws IOException for case-only renames on Windows, so the + /// rename is performed in two steps through a temporary name. If the second + /// move fails the directory is moved back to the original casing so a retry + /// sees a consistent working tree. + /// + /// If the source directory is missing it usually means an outer parent rename + /// has already moved the children into place (Windows preserves child casing + /// through a parent rename when the children's tree SHAs were unchanged); the + /// fallback creates the target directory so the operation is idempotent. + /// Exceptions propagate to the caller's existing error handler. + /// + private void ApplyCaseOnlyDirectoryRename(DiffTreeResult treeOp, string absoluteTargetPath) + { + string absoluteSourcePath = Path.Combine(this.enlistment.WorkingDirectoryBackingRoot, treeOp.SourcePath); + if (!Directory.Exists(absoluteSourcePath)) + { + Directory.CreateDirectory(absoluteTargetPath); + return; + } + + string trimmedSourcePath = absoluteSourcePath.TrimEnd(Path.DirectorySeparatorChar); + string trimmedTargetPath = absoluteTargetPath.TrimEnd(Path.DirectorySeparatorChar); + string tempPath = trimmedTargetPath + "_caseRename_" + Guid.NewGuid().ToString("N"); + + Directory.Move(trimmedSourcePath, tempPath); + try + { + Directory.Move(tempPath, trimmedTargetPath); + } + catch + { + // The first move succeeded but the second failed. Try to restore the + // original casing so a retry starts from a consistent state; if + // restoration also fails, the outer catch will log the original + // exception and the temp directory will be left behind for manual + // recovery. + if (Directory.Exists(tempPath) && !Directory.Exists(trimmedSourcePath)) + { + try + { + Directory.Move(tempPath, trimmedSourcePath); + } + catch + { + } + } + + throw; + } + } + private void HandleAllFileDeleteOperations() { string path; From 745653cc7d1f0466b3483b77303ee4717a5bec1c Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Mon, 11 May 2026 18:38:55 -0400 Subject: [PATCH 12/13] test: Add coverage for file-only case rename MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Context: Case-rename detection lives in three places in DiffHelper: EnqueueOperationsFromDiffTreeLine's directory branches, EnqueueFileAddOperation, and EnqueueFileDeleteOperation. The existing fixtures (caseChange.txt, caseChangeAddFirst.txt) exercise only the directory branches — their test files all sit inside a case-renamed directory, so the file-level case-rename paths are indirectly suppressed by the parent-directory filtering in FlushStagedQueues. That leaves the standalone "rename foo.txt to FOO.txt with no directory casing changes" path completely uncovered. Both EnqueueFileAddOperation's case-rename branch and EnqueueFileDeleteOperation's case-rename branch ship without a test that exercises them end-to-end. Justification: A two-line fixture isolates the file-level path: one Delete entry for the old casing, one Add entry for the new casing, no directory operations. The resulting test pins the contract that DiffHelper both stages the delete (old casing, so the on-disk file is removed) and stages the add (new casing, so the replacement file is written), and that neither is filtered out as a duplicate. Implementation: New fixture Data/fileCaseChange.txt with a Delete for "foo.txt" followed by an Add for "FOO.txt" (the typical diff-tree emit order for that direction since "F" sorts before "f" byte-wise). New unit test ParsesFileOnlyCaseRename asserts RequiredBlobs has one entry, FileAddOperations and FileDeleteOperations each have one entry, and the casings flow through correctly: the delete carries "foo.txt" and the add carries "FOO.txt". The test is gated to case-insensitive filesystems because on a case-sensitive filesystem the two paths are distinct files and no case-rename detection runs (the existing ParsesCaseChangesAsRenames test covers the case-sensitive behavior). The csproj is updated so the fixture is copied to the test output directory. Co-Authored-By: Claude Opus 4.7 (1M context) --- GVFS/GVFS.UnitTests/Data/fileCaseChange.txt | 2 ++ GVFS/GVFS.UnitTests/GVFS.UnitTests.csproj | 3 ++ .../Prefetch/DiffHelperTests.cs | 28 +++++++++++++++++++ 3 files changed, 33 insertions(+) create mode 100644 GVFS/GVFS.UnitTests/Data/fileCaseChange.txt diff --git a/GVFS/GVFS.UnitTests/Data/fileCaseChange.txt b/GVFS/GVFS.UnitTests/Data/fileCaseChange.txt new file mode 100644 index 000000000..e4b68d1dd --- /dev/null +++ b/GVFS/GVFS.UnitTests/Data/fileCaseChange.txt @@ -0,0 +1,2 @@ +:100644 000000 eabe8d5ec569cc7e199e77411ad935f101414032 0000000000000000000000000000000000000000 D foo.txt +:000000 100644 0000000000000000000000000000000000000000 b6fc4c620b67d95f953a5c1c1230aaab5db5a1b0 A FOO.txt diff --git a/GVFS/GVFS.UnitTests/GVFS.UnitTests.csproj b/GVFS/GVFS.UnitTests/GVFS.UnitTests.csproj index 4e0260983..21dcbbb64 100644 --- a/GVFS/GVFS.UnitTests/GVFS.UnitTests.csproj +++ b/GVFS/GVFS.UnitTests/GVFS.UnitTests.csproj @@ -42,6 +42,9 @@ Always + + Always + Always diff --git a/GVFS/GVFS.UnitTests/Prefetch/DiffHelperTests.cs b/GVFS/GVFS.UnitTests/Prefetch/DiffHelperTests.cs index 0674f7f8c..0c8a7263b 100644 --- a/GVFS/GVFS.UnitTests/Prefetch/DiffHelperTests.cs +++ b/GVFS/GVFS.UnitTests/Prefetch/DiffHelperTests.cs @@ -175,6 +175,34 @@ public void ParsesCaseChangesWhenAddPrecedesDelete() diffBackwards.TotalDirectoryOperations.ShouldEqual(3); } + // File-level case rename ("foo.txt" -> "FOO.txt") with no directory case + // changes. The fixture emits a Delete for the old casing followed by an + // Add for the new casing; DiffHelper should stage both — the delete + // removes the old-cased file from disk, the add writes the new-cased + // file — so FlushStagedQueues hands both to the checkout stage. + [TestCase] + [Category(CategoryConstants.CaseInsensitiveFileSystemOnly)] + public void ParsesFileOnlyCaseRename() + { + MockTracer tracer = new MockTracer(); + DiffHelper diff = new DiffHelper(tracer, new Mock.Common.MockGVFSEnlistment(), new List(), new List(), includeSymLinks: this.IncludeSymLinks); + diff.ParseDiffFile(GetDataPath("fileCaseChange.txt")); + + diff.RequiredBlobs.Count.ShouldEqual(1); + diff.FileAddOperations.Sum(list => list.Value.Count).ShouldEqual(1); + diff.FileDeleteOperations.Count.ShouldEqual(1); + diff.TotalFileDeletes.ShouldEqual(1); + diff.TotalDirectoryOperations.ShouldEqual(0); + diff.DirectoryOperations.Count.ShouldEqual(0); + + // The delete keeps the old casing; the add carries the new casing. + string deletedPath = diff.FileDeleteOperations.ToArray()[0]; + Assert.AreEqual("foo.txt", deletedPath); + + string addedPath = diff.FileAddOperations.First().Value.First().Path; + Assert.AreEqual("FOO.txt", addedPath); + } + // Delete a folder with two sub folders each with a single file // Readd it with a different casing and same contents [TestCase] From cbd7c82874d5bb9ecaee6e455205cc0a9308828b Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Mon, 11 May 2026 18:41:01 -0400 Subject: [PATCH 13/13] test: Add coverage for nested directory case rename MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Context: The existing case-rename fixtures change only the top-level directory's casing. Their children sit inside the renamed parent but retain the parent's old casing in the diff. Nothing in the existing test suite exercises a diff where both an outer directory and an inner directory change casing in the same operation. That scenario is reachable in production whenever a contributor renames an entire path's casing (e.g., "Docs/API/" to "docs/api/") and is the most interesting interaction between the case-rename detection in EnqueueOperationsFromDiffTreeLine and the parent-path filter in FlushStagedQueues: the inner rename's parent path is in directoriesReplacedByCaseRename, so its Add must be suppressed because the outer rename's Directory.Move already carries the child on disk. If that suppression breaks, FastFetch would attempt to CreateDirectory the inner path after the outer rename has moved the entire subtree — at best a no-op, at worst a layered race. Justification: A six-line fixture (three Deletes, three Adds) is the smallest input that captures the nested case where both renames need to be detected *and* the inner one needs to be filtered. Asserting all of FileAddOperations, FileDeleteOperations, TotalFileDeletes, TotalDirectoryOperations, the enqueued DirectoryOperations count, and the SourcePath/TargetPath on the surviving op pins the contract end-to-end. Implementation: New Data/nestedCaseChange.txt with a "Outer/Inner/test" source tree and an "outer/inner/test" target tree. New unit test ParsesNestedCaseChanges asserts: - One blob is required (the file under the renamed subtree) - FileAddOperations carries the new-cased path - The file delete is filtered out by parent-path matching - TotalDirectoryOperations is 2 (both case-renames are staged) - DirectoryOperations.Count is 1 (only the outermost survives the parent-path filter) - The surviving directory op carries the old casing on SourcePath The test is gated to case-insensitive filesystems because on a case-sensitive filesystem the two paths are independent and no case-rename detection runs. The csproj is updated so the fixture is copied to the test output directory. Co-Authored-By: Claude Opus 4.7 (1M context) --- GVFS/GVFS.UnitTests/Data/nestedCaseChange.txt | 6 ++++ GVFS/GVFS.UnitTests/GVFS.UnitTests.csproj | 3 ++ .../Prefetch/DiffHelperTests.cs | 36 +++++++++++++++++++ 3 files changed, 45 insertions(+) create mode 100644 GVFS/GVFS.UnitTests/Data/nestedCaseChange.txt diff --git a/GVFS/GVFS.UnitTests/Data/nestedCaseChange.txt b/GVFS/GVFS.UnitTests/Data/nestedCaseChange.txt new file mode 100644 index 000000000..751384645 --- /dev/null +++ b/GVFS/GVFS.UnitTests/Data/nestedCaseChange.txt @@ -0,0 +1,6 @@ +:040000 000000 d813c8227132c3bf73c013f8913f207b4876b2bf 0000000000000000000000000000000000000000 D Outer +:040000 000000 1260ecb71f2be8eb92ea904c6dffa3e40eaaf1bf 0000000000000000000000000000000000000000 D Outer/Inner +:100644 000000 eabe8d5ec569cc7e199e77411ad935f101414032 0000000000000000000000000000000000000000 D Outer/Inner/test +:000000 040000 0000000000000000000000000000000000000000 d813c8227132c3bf73c013f8913f207b4876b2bf A outer +:000000 040000 0000000000000000000000000000000000000000 1260ecb71f2be8eb92ea904c6dffa3e40eaaf1bf A outer/inner +:000000 100644 0000000000000000000000000000000000000000 eabe8d5ec569cc7e199e77411ad935f101414032 A outer/inner/test diff --git a/GVFS/GVFS.UnitTests/GVFS.UnitTests.csproj b/GVFS/GVFS.UnitTests/GVFS.UnitTests.csproj index 21dcbbb64..0517e82f1 100644 --- a/GVFS/GVFS.UnitTests/GVFS.UnitTests.csproj +++ b/GVFS/GVFS.UnitTests/GVFS.UnitTests.csproj @@ -45,6 +45,9 @@ Always + + Always + Always diff --git a/GVFS/GVFS.UnitTests/Prefetch/DiffHelperTests.cs b/GVFS/GVFS.UnitTests/Prefetch/DiffHelperTests.cs index 0c8a7263b..03954c1ad 100644 --- a/GVFS/GVFS.UnitTests/Prefetch/DiffHelperTests.cs +++ b/GVFS/GVFS.UnitTests/Prefetch/DiffHelperTests.cs @@ -203,6 +203,42 @@ public void ParsesFileOnlyCaseRename() Assert.AreEqual("FOO.txt", addedPath); } + // Nested case rename: both an outer directory ("Outer" -> "outer") and + // an inner directory inside it ("Outer/Inner" -> "outer/inner") change + // casing in the same diff. Only the outermost rename should be enqueued + // for the checkout stage — the inner rename's parent path is in + // directoriesReplacedByCaseRename, so FlushStagedQueues suppresses it + // (the outer rename moves the whole subtree on disk and Windows + // preserves child casing through the move). The file inside the inner + // directory is similarly suppressed at the file-delete stage. + [TestCase] + [Category(CategoryConstants.CaseInsensitiveFileSystemOnly)] + public void ParsesNestedCaseChanges() + { + MockTracer tracer = new MockTracer(); + DiffHelper diff = new DiffHelper(tracer, new Mock.Common.MockGVFSEnlistment(), new List(), new List(), includeSymLinks: this.IncludeSymLinks); + diff.ParseDiffFile(GetDataPath("nestedCaseChange.txt")); + + diff.RequiredBlobs.Count.ShouldEqual(1); + diff.FileAddOperations.Sum(list => list.Value.Count).ShouldEqual(1); + + // File delete inside the case-renamed parent is filtered out. + diff.FileDeleteOperations.Count.ShouldEqual(0); + diff.TotalFileDeletes.ShouldEqual(1); + + // Two directory case-renames were collapsed into Adds in the + // staging dictionary; only the outermost survives the parent-path + // filter in FlushStagedQueues. + diff.TotalDirectoryOperations.ShouldEqual(2); + diff.DirectoryOperations.Count.ShouldEqual(1); + diff.DirectoryOperations.ShouldNotContain(entry => entry.Operation == DiffTreeResult.Operations.Delete); + + DiffTreeResult outerOp = diff.DirectoryOperations.First(); + outerOp.SourcePath.ShouldNotBeNull(); + Assert.AreEqual("Outer" + Path.DirectorySeparatorChar, outerOp.SourcePath); + Assert.AreEqual("outer" + Path.DirectorySeparatorChar, outerOp.TargetPath); + } + // Delete a folder with two sub folders each with a single file // Readd it with a different casing and same contents [TestCase]