Skip to content

fix: handle case-sensitive file overwrite on Windows and add tests #1259

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

ana1250
Copy link
Contributor

@ana1250 ana1250 commented Apr 2, 2025

This PR fixes an issue (#1140) where copying a file with the same name but different casing on Windows resulted in an "already in use" error. It aligns the behavior with the native file system.

Changes

  • Fixed file overwrite logic to properly handle case-insensitive paths on Windows.
  • Added Windows-specific checks to mimic System.IO.File.Copy System.IO.File.Move and System.IO.File.Replace behavior.
  • Implemented test cases to verify expected behavior for case-sensitive and case-insensitive file systems.
  • Ensured that the fix does not affect Linux behavior.

Testing

  • Verified that the fix works correctly on Windows by checking file existence before and after copying,moving and replacing file with different cases
  • Ran test cases on both Windows and Linux to ensure no unintended side effects.

test: add tests for Windows file copy behavior
@ana1250 ana1250 force-pushed the CaseCheck_Windows_FileOperations_Bug branch from 1277a9c to fc24241 Compare April 2, 2025 17:25
Copy link
Member

@vbreuss vbreuss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ana1250, looks really good.
I just have some minor suggestions on improving the tests...

@ana1250 ana1250 requested a review from vbreuss April 4, 2025 12:34
@mergify mergify bot merged commit 928bc38 into TestableIO:main Apr 4, 2025
9 checks passed
Copy link

github-actions bot commented Apr 4, 2025

This is addressed in release v22.0.13.

@github-actions github-actions bot added the state: released Issues that are released label Apr 4, 2025
@ana1250
Copy link
Contributor Author

ana1250 commented Apr 4, 2025

Thanks! Looking forward to contributing more.

@@ -163,6 +163,11 @@ public override void Copy(string sourceFileName, string destFileName, bool overw
throw CommonExceptions.FileAlreadyExists(destFileName);
}

if (string.Equals(sourceFileName, destFileName, StringComparison.OrdinalIgnoreCase) && XFS.IsWindowsPlatform())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in all the other cases we use
mockFileDataAccessor.StringOperations.Equals(sourceFileName, destinationFileName) instead of relying on
string.Equals(..., ..., StringComparison.OrdinalIgnoreCase)

Is intentional or an oversight?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be an oversight.
We had one case here where we used string.Equals on purpose, as it should compare case-sensitive in all operating systems, but here it seems I overlooked it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
state: released Issues that are released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants