Skip to content
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

Introduce atomic move and write of file #4060

Merged
merged 1 commit into from
Nov 1, 2021

Conversation

catap
Copy link
Contributor

@catap catap commented Sep 14, 2021

The idea of this changes is simple: let move file to some temporary name inside distance folder, and after the file is already copy it renames to expected name.

When someone tries to save anything it also moves file to trigger OS level notification for change FS.

This commit also enforce that beets.util.move shouldn't be used to move directories as it described in comment.

Thus, this is fixed #3849

@catap catap changed the title Introduce atomic move of file Introduce atomic move and write of file Sep 15, 2021
@catap catap force-pushed the atomic-move branch 4 times, most recently from cfd6b79 to 90f2259 Compare September 15, 2021 13:19
@catap
Copy link
Contributor Author

catap commented Sep 17, 2021

@sampsyo any chances to review it?

@sampsyo
Copy link
Member

sampsyo commented Sep 17, 2021

Hi, and thanks for your contribution! It's in my queue, but sometimes it can take me a while to have the bandwidth for a full review. Fortunately, the beets community is broad and doesn't just contain me, so other folks might also be able to take a look! You could consider putting out a broader call for reviewers in Discussions, etc., if you think that's warranted.

Copy link
Member

@wisp3rwind wisp3rwind left a comment

Choose a reason for hiding this comment

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

Not a full review, but some initial thoughts/requests/comments. Let's be certain that this is correct before merging; the fallout might otherwise be huge.

beets/library.py Outdated Show resolved Hide resolved
beets/util/__init__.py Outdated Show resolved Hide resolved
beets/util/__init__.py Outdated Show resolved Hide resolved
beets/util/__init__.py Outdated Show resolved Hide resolved
beets/util/__init__.py Show resolved Hide resolved
@catap catap force-pushed the atomic-move branch 2 times, most recently from 84aff3f to 1a5ddf8 Compare September 17, 2021 13:50
@catap
Copy link
Contributor Author

catap commented Sep 20, 2021

@wisp3rwind / @sampsyo ping?

@catap
Copy link
Contributor Author

catap commented Sep 28, 2021

@sampsyo ping!

@wisp3rwind
Copy link
Member

It's not been forgotten! I'll try to have another look soon.

@catap
Copy link
Contributor Author

catap commented Oct 11, 2021

@wisp3rwind just friendly reminder

@catap
Copy link
Contributor Author

catap commented Oct 19, 2021

@sampsyo any hope to move forward on it? :)

Copy link
Member

@wisp3rwind wisp3rwind left a comment

Choose a reason for hiding this comment

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

Looks good to me 🎉 Sorry for the silence here, I've recently had much less time (and motivation) to spend on beets than before.

See the inline comment for a small typo in the log messages. Would you mind adding a changelog entry?

beets/util/__init__.py Outdated Show resolved Hide resolved
@catap
Copy link
Contributor Author

catap commented Oct 30, 2021

@wisp3rwind I've incorporated your changes and rebased to last master.

@wisp3rwind
Copy link
Member

@wisp3rwind I've incorporated your changes and rebased to last master.

Great! A small style issue (line to long) has crept in in this last change, though.

The idea of this changes is simple: let move file to some temporary name
inside distance folder, and after the file is already copy it renames to
expected name.

When someone tries to save anything it also moves file to trigger OS
level notification for change FS.

This commit also enforce that `beets.util.move` shouldn't be used to
move directories as it described in comment.

Thus, this is fixed beetbox#3849
@catap
Copy link
Contributor Author

catap commented Oct 30, 2021

@wisp3rwind everything is green now!

@wisp3rwind wisp3rwind merged commit 813dea1 into beetbox:master Nov 1, 2021
wisp3rwind added a commit that referenced this pull request Nov 1, 2021
@wisp3rwind
Copy link
Member

Great! I've added a changelog entry for this in 5578d07

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.

Option to copy files atomically
3 participants