Skip to content

use NtSetInformationFile if os.Rename fails#220

Open
karman-docker wants to merge 2 commits into
moby:mainfrom
karman-docker:add_retry_for_rename
Open

use NtSetInformationFile if os.Rename fails#220
karman-docker wants to merge 2 commits into
moby:mainfrom
karman-docker:add_retry_for_rename

Conversation

@karman-docker

Copy link
Copy Markdown

This PR adds atomicwriterRenameAt which uses NtSetInformationFile to rename a file on Windows.

Signed-off-by: Manju Karikatti <manju.karikatti@docker.com>
Comment thread atomicwriter/atomicwriter_unix.go Outdated
@kolyshkin

Copy link
Copy Markdown
Collaborator

This obviously needs some windows-specific tests.

@kolyshkin kolyshkin left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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).

@karman-docker

Copy link
Copy Markdown
Author

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' ?

@kolyshkin

Copy link
Copy Markdown
Collaborator

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 At should be back, I am saying the right name should be introduced from the start, i.e. in the first commit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants