use NtSetInformationFile if os.Rename fails - #220
Conversation
477163b to
f141cb9
Compare
|
This obviously needs some windows-specific tests. |
kolyshkin
left a comment
kolyshkin
left a comment
There was a problem hiding this comment.
nit: you've introduced atomicwriterRenameAt in the first commit, and then rename it to atomicwriterRename in the second commit. Why not use the right name from the start?
nit: stuttering at the name (atomicwriter.atomicwriterRename).
sorry I thought in your initial review comment, you suggested to remove 'At' ? |
Yes, but here I'm not saying |
e0379f6 to
88cf225
Compare
Signed-off-by: Manju Karikatti <manju.karikatti@docker.com> Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
88cf225 to
8eea723
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The Windows fallback mishandles some paths, fails under pointer checking, and lacks targeted fallback coverage.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds a Windows-specific rename fallback using NtSetInformationFile for atomic file replacement.
Changes:
- Routes atomic writer renames through platform-specific implementations.
- Adds Windows native rename handling and Unix compatibility.
- Adds basic rename tests and promotes
x/systo a direct dependency.
File summaries
| File | Description |
|---|---|
atomicwriter/go.mod |
Makes x/sys a direct dependency. |
atomicwriter/atomicwriter.go |
Uses the platform rename helper. |
atomicwriter/atomicwriter_windows.go |
Implements the Windows fallback. |
atomicwriter/atomicwriter_unix.go |
Retains standard Unix rename behavior. |
atomicwriter/atomicwriter_test.go |
Tests basic rename behavior. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 4
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| infoEx := (*fileRenameInformationEx)(unsafe.Pointer(&bufferEx[0])) | ||
| infoEx.Flags = renameInfoEx.Flags | ||
| infoEx.FileNameLength = uint32(fileNameLen) | ||
| copy((*[windows.MAX_LONG_PATH]uint16)(unsafe.Pointer(&infoEx.FileName[0]))[:fileNameLen/2:fileNameLen/2], newPathUTF16) |
| info := (*fileRenameInformation)(unsafe.Pointer(&buffer[0])) | ||
| info.ReplaceIfExists = windows.FILE_RENAME_REPLACE_IF_EXISTS | windows.FILE_RENAME_POSIX_SEMANTICS | ||
| info.FileNameLength = uint32(fileNameLen) | ||
| copy((*[windows.MAX_LONG_PATH]uint16)(unsafe.Pointer(&info.FileName[0]))[:fileNameLen/2:fileNameLen/2], newPathUTF16) |
|
|
||
| // NtSetInformationFile requires an absolute NT path (\??\C:\...) when | ||
| // RootDirectory is NULL. | ||
| ntNewPath := `\??\` + newpath |
| if err := os.WriteFile(dstPath, []byte("original content"), fileMode); err != nil { | ||
| t.Fatalf("Error writing destination file: %v", err) | ||
| } | ||
|
|
||
| if err := atomicwriterRename(srcPath, dstPath); err != nil { | ||
| t.Fatalf("Error renaming file: %v", err) | ||
| } |
This PR adds
atomicwriterRenameAtwhich usesNtSetInformationFileto rename a file on Windows.