-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Fix symlink handling in CopyDirectory method #3914
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR enhances the CopyDirectory
method to properly detect and preserve symbolic links when copying directories and files.
- Adds early detection for directory symlinks and creates matching links in the target.
- Updates file-copy logic to skip already up-to-date files and supports file symlinks.
- Removes the duplicate
sourceDir
declaration and centralizes its initialization.
Comments suppressed due to low confidence (1)
src/Runner.Sdk/Util/IOUtil.cs:392
- The new symlink-handling branches (for directories at lines 395–408 and files at lines 426–434) lack unit tests. Please add non-trivial L0 tests under
Test/L0
to verify both directory and file symlink scenarios.
// Get the file contents of the directory to copy.
} | ||
|
||
// Check if source is a symlink | ||
if (!sourceFile.Attributes.HasFlag(FileAttributes.ReparsePoint)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] For file symlinks, add a check to skip recreating an existing link when the target matches, similar to the directory case, to avoid unnecessary filesystem operations.
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this logic may have been covered by the section above that checks for the existence of target
Fixes actions#3234 This change adds proper support for symbolic links in the CopyDirectory method. NOTE: This patch was developed with GitHub Copilot assistance. I has **NO** C# experience at all and is not familiar with actions/runner. This change has not been tested in a local environment. Please review this patch carefully. Signed-off-by: Chen Linxuan <me@black-desk.cn>
I don't even have a full C# development environment, so if there's anything the project maintainers would like to change, it's perfectly fine to push it directly to my branch and merge it, and I hope this will be fixed soon. |
Fixes #3234
This change adds proper support for symbolic links
in the CopyDirectory method.
NOTE: This patch was developed with GitHub Copilot
assistance. I has NO C# experience at all and
is not familiar with actions/runner. This change
has not been tested in a local environment. Please
review this patch carefully.
Signed-off-by: Chen Linxuan me@black-desk.cn