fileMode-fix for issue #1694 Windows bug #1710
Conversation
windows symlinks are strange and they are not treated as directories in go, so using just fileInfo.Dir isn't enough.
WalkthroughThe pull request introduces a new constant and function in the Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/util/utilfn/utilfn.go (1)
624-628: Improve theisWinSymlinkfunction implementation.Consider the following improvements:
- Move the
WIN_SYMLINKconstant outside the function to avoid recreation on each call.- Follow Go naming conventions for the constant.
- Add documentation for the function.
+// winSymlink represents the mode bits for a Windows symbolic link +const winSymlink = os.ModeSymlink + +// isWinSymlink checks if the given file mode indicates a Windows symbolic link func isWinSymlink(bits os.FileMode) bool { - WIN_SYMLINK := os.ModeSymlink - return (bits&WIN_SYMLINK > 0) + return bits&winSymlink != 0 }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/util/utilfn/utilfn.go(2 hunks)
🔇 Additional comments (1)
pkg/util/utilfn/utilfn.go (1)
638-638: Verify symlink-to-file handling.The change correctly identifies Windows symlinks, but we should verify the assumption that all symlinks should be treated as directories. What if a symlink points to a regular file?
✅ Verification successful
Implementation is correct and safe for all symlink types
The
isWinSymlinkfunction uses Go's standardos.ModeSymlinkcheck, which correctly identifies symlinks regardless of whether they point to files or directories. The function name is slightly misleading as it works for all symlinks, not just Windows ones, but the implementation is safe and appropriate for skipping mime-type detection on symlinks.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if there are any symlinks pointing to non-directories in the codebase # This helps verify our assumption about treating all symlinks as directories # Look for symlink declarations or creations in tests rg -l "os\.Symlink|filepath\.Symlink" . --type go # Look for existing symlinks in the codebase that point to files find . -type l -exec sh -c 'file -h "{}" | grep -v "directory"' \;Length of output: 121
Script:
#!/bin/bash # Get the implementation of isWinSymlink and surrounding context rg -A 5 -B 5 "func isWinSymlink" . # Get the full context of DetectMimeType function ast-grep --pattern 'func DetectMimeType($$$) $$ { $$$ }' # Look for Windows-specific symlink handling patterns rg -A 3 -B 3 "windows.*symlink|symlink.*windows" . --type go -iLength of output: 883
|
Hey for some reason golang fs package uses 0x8000000 which is not the real FILE_ATTRIBUTE value of windows, |
|
I tested this on Linux and it doesn't seem to affect anything so I think we're good there. What tool were you using to make symlinks on Windows? When I use |
mklink |
|
seems like it should also support |
Hey give one more commit and I'll see what I can do. forgot to account for file symlinks and I'm going to have to hardcore the flags for that in think, might as well resolve the junction problem if I can. |
|
|
or should I just check the . in the filename to work with the inconsistent symlink lol? |
|
Maybe this isn't necessary? What if /D is uncommon to use? I don't know, but the junction flag wont interfere and if it's the standard then I doubt this bug will be common, i hate cmd. |
Added a cheesy workaround to distinguish between files and dir when softlinks are used.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/util/utilfn/utilfn.go(2 hunks)
🔇 Additional comments (1)
pkg/util/utilfn/utilfn.go (1)
660-660: LGTM! Directory detection now handles Windows symlinks correctly.The modification correctly integrates the Windows symlink detection into the mime type detection logic.
pkg/util/utilfn/utilfn.go
Outdated
| func WinSymlinkDir(path string, bits os.FileMode) bool { | ||
| /* for the cursed softlink on windows | ||
| the compability fileInfo interface gives us no way to determine its pointing type */ | ||
| isFileSymlink := func(filepath string) bool { | ||
| Length := len(filepath) - 1 | ||
| maxFileDotExt := 4 // should cover most file extensions | ||
| for i := Length; i >= (Length - maxFileDotExt); i-- { | ||
| if filepath[i] == '.' { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } | ||
|
|
||
| winSymlink_flags := uint32(bits >> 12) | ||
| FLAG_SOFTLINK, FLAG_JUNCTION := uint32(0x8000), uint32(0x80) | ||
|
|
||
| if winSymlink_flags == FLAG_SOFTLINK { | ||
| return !isFileSymlink(path) | ||
| } else if winSymlink_flags == FLAG_JUNCTION { | ||
| return true | ||
| } else { | ||
| return false | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve robustness and maintainability of symlink detection.
The implementation has several areas that need improvement:
- Magic numbers should be defined as named constants with documentation.
- The
isFileSymlinkfunction has potential edge cases and could be improved. - Variable naming should follow Go conventions.
Consider applying these improvements:
// Windows-specific file mode flags for symlinks and junctions
+const (
+ winFlagSoftlink = uint32(0x8000) // FILE_ATTRIBUTE_REPARSE_POINT
+ winFlagJunction = uint32(0x80) // FILE_ATTRIBUTE_JUNCTION
+)
func WinSymlinkDir(path string, bits os.FileMode) bool {
- /* for the cursed softlink on windows
- the compability fileInfo interface gives us no way to determine its pointing type */
+ // Windows compatibility layer doesn't expose symlink target type through fileInfo
+ // so we need to check file attributes and extension patterns
isFileSymlink := func(filepath string) bool {
+ if len(filepath) == 0 {
+ return false
+ }
- Length := len(filepath) - 1
- maxFileDotExt := 4 // should cover most file extensions
- for i := Length; i >= (Length - maxFileDotExt); i-- {
- if filepath[i] == '.' {
- return true
- }
- }
- return false
+ return strings.LastIndex(filepath, ".") > strings.LastIndex(filepath, "/")
}
- winSymlink_flags := uint32(bits >> 12)
- FLAG_SOFTLINK, FLAG_JUNCTION := uint32(0x8000), uint32(0x80)
+ flags := uint32(bits >> 12)
- if winSymlink_flags == FLAG_SOFTLINK {
+ if flags == winFlagSoftlink {
return !isFileSymlink(path)
- } else if winSymlink_flags == FLAG_JUNCTION {
+ } else if flags == winFlagJunction {
return true
} else {
return false
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func WinSymlinkDir(path string, bits os.FileMode) bool { | |
| /* for the cursed softlink on windows | |
| the compability fileInfo interface gives us no way to determine its pointing type */ | |
| isFileSymlink := func(filepath string) bool { | |
| Length := len(filepath) - 1 | |
| maxFileDotExt := 4 // should cover most file extensions | |
| for i := Length; i >= (Length - maxFileDotExt); i-- { | |
| if filepath[i] == '.' { | |
| return true | |
| } | |
| } | |
| return false | |
| } | |
| winSymlink_flags := uint32(bits >> 12) | |
| FLAG_SOFTLINK, FLAG_JUNCTION := uint32(0x8000), uint32(0x80) | |
| if winSymlink_flags == FLAG_SOFTLINK { | |
| return !isFileSymlink(path) | |
| } else if winSymlink_flags == FLAG_JUNCTION { | |
| return true | |
| } else { | |
| return false | |
| } | |
| } | |
| // Windows-specific file mode flags for symlinks and junctions | |
| const ( | |
| winFlagSoftlink = uint32(0x8000) // FILE_ATTRIBUTE_REPARSE_POINT | |
| winFlagJunction = uint32(0x80) // FILE_ATTRIBUTE_JUNCTION | |
| ) | |
| func WinSymlinkDir(path string, bits os.FileMode) bool { | |
| // Windows compatibility layer doesn't expose symlink target type through fileInfo | |
| // so we need to check file attributes and extension patterns | |
| isFileSymlink := func(filepath string) bool { | |
| if len(filepath) == 0 { | |
| return false | |
| } | |
| return strings.LastIndex(filepath, ".") > strings.LastIndex(filepath, "/") | |
| } | |
| flags := uint32(bits >> 12) | |
| if flags == winFlagSoftlink { | |
| return !isFileSymlink(path) | |
| } else if flags == winFlagJunction { | |
| return true | |
| } else { | |
| return false | |
| } | |
| } |
💡 Codebase verification
Add test coverage for Windows symlink handling
The WinSymlinkDir function lacks test coverage, which is critical for Windows-specific functionality. Please add tests to cover:
- Windows directory symlinks (
mklink /D) withFLAG_SOFTLINK(0x8000) - Directory junctions (
mklink /J) withFLAG_JUNCTION(0x80) - File symlinks with various extensions
- Edge cases for the file extension detection logic
- Bit manipulation validation for
os.FileModeflags
🔗 Analysis chain
Verify test coverage for Windows symlink handling.
Please ensure that test cases are added to cover:
- Directory symlinks created with
mklink /D - Directory junctions created with
mklink /J - File symlinks
- Edge cases with various file extensions
Let's check for existing tests:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for test files that might contain Windows symlink tests
rg -l "mklink|symlink|junction" --type go --glob "*_test.go"
# Search for test functions related to symlinks
ast-grep --pattern 'func Test$_(t *testing.T) {
$$$
mklink $$$
$$$
}'
Length of output: 140
Script:
#!/bin/bash
# Check for test files in the same directory
fd "utilfn_test.go" pkg/util/utilfn/
# Search for WinSymlinkDir usage in test files
rg "WinSymlinkDir" --type go --glob "*_test.go" -C 3
# Look for any symlink-related test code
ast-grep --pattern 'func Test$_(t *testing.T) {
$$$
FileMode $$$
$$$
}'
# Check if the function is used anywhere in the codebase
rg "WinSymlinkDir" -C 3
Length of output: 967
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/util/utilfn/utilfn.go (1)
628-633: Improve robustness of file extension detection.The current implementation might have edge cases with paths containing multiple dots. Consider using
filepath.Ext()for more reliable extension detection.isFileSymlink := func(filepath string) bool { if len(filepath) == 0 { return false } - return strings.LastIndex(filepath, ".") > strings.LastIndex(filepath, "/") + return filepath.Ext(filepath) != "" }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/util/utilfn/utilfn.go(2 hunks)
🔇 Additional comments (4)
pkg/util/utilfn/utilfn.go (4)
620-623: LGTM! Well-documented Windows file attribute constants.The constants are correctly typed and accurately represent Windows FILE_ATTRIBUTE flags.
625-644: Add test coverage for Windows symlink handling.Please add tests to cover:
- Directory symlinks created with
mklink /D- Directory junctions created with
mklink /J- File symlinks with various extensions
- Edge cases with empty paths
Would you like me to help create comprehensive test cases for this function?
660-660: LGTM! Clean integration of Windows symlink detection.The modification elegantly handles Windows symlinks while maintaining the existing behavior for other systems.
635-643: Verify cross-platform compatibility.While the Windows-specific flags are properly isolated, let's verify that these changes don't affect other platforms.
✅ Verification successful
Cross-platform compatibility verified
The Windows-specific symlink handling is properly isolated in the
WinSymlinkDirfunction and won't affect other platforms. The bit operations using Windows-specific flags are safe as these flag values won't match on other operating systems.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any existing symlink-related code that might conflict # Look for other symlink detection code rg -l "os\.ModeSymlink|symlink|FileMode" --type go # Check for any platform-specific file mode operations ast-grep --pattern 'os.FileMode $_ = $_'Length of output: 172
Script:
#!/bin/bash # Check symlink-related code implementation in the identified files rg "os\.ModeSymlink|symlink|FileMode" -A 5 -B 5 pkg/util/utilfn/utilfn.go pkg/wavebase/wavebase.go pkg/wshrpc/wshrpctypes.go # Look for platform-specific code rg "windows|linux|darwin" --type go pkg/util/utilfn/utilfn.goLength of output: 4335
|
Just pulled and it looks great to me, thanks for fixing this! Are you ready to merge? |
windows symlinks are strange and they are not treated as directories in go, so using just fileInfo.Dir isn't enough.
Now treated as directories.
PR branch
main branch
bug issue link: #1694