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

#48: Atomic files #50

Merged
merged 33 commits into from
Dec 14, 2023
Merged

#48: Atomic files #50

merged 33 commits into from
Dec 14, 2023

Conversation

gwendalF
Copy link
Collaborator

@gwendalF gwendalF commented Oct 1, 2023

Use atomic files whenever we want to use files. It ensure there are written atomicaly. Also we have the possibility to manage the case when the file is already present and do something with the data. Currently it's just overwritten but something better can be done
Fix #48

src/lib.rs Outdated Show resolved Hide resolved
src/link.rs Show resolved Hide resolved
src/atomic_file/mod.rs Outdated Show resolved Hide resolved
src/meta.rs Outdated Show resolved Hide resolved
src/atomic_file/atomic.rs Outdated Show resolved Hide resolved
src/atomic_file/mod.rs Outdated Show resolved Hide resolved
src/link.rs Outdated Show resolved Hide resolved
src/link.rs Outdated Show resolved Hide resolved
src/meta.rs Outdated Show resolved Hide resolved
src/atomic_file/mod.rs Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
src/atomic_file/atomic.rs Outdated Show resolved Hide resolved
src/atomic_file/atomic.rs Outdated Show resolved Hide resolved
@kirillt
Copy link
Member

kirillt commented Oct 16, 2023

@gwendalF have you implemented this, too?

If several versions with same number are found, take the version with local device id and log warning about conflicted versions. These conflicts can come only from remote devices, locally we can't produce conflicts.

src/prop.rs Outdated Show resolved Hide resolved
src/pdf.rs Outdated Show resolved Hide resolved
@mberry
Copy link
Collaborator

mberry commented Oct 17, 2023

Adding a Clippy CI check is good, but I'm getting 1 error here:

let file = File::open(&index_path)?;

It wants the borrow removed

@gwendalF
Copy link
Collaborator Author

Adding a Clippy CI check is good, but I'm getting 1 error here:

let file = File::open(&index_path)?;

It wants the borrow removed

We can remove the borrow since it does not appears to be used somewhere else so we can move the path on the open function

src/atomic_file/atomic.rs Outdated Show resolved Hide resolved
src/atomic_file/atomic.rs Outdated Show resolved Hide resolved
src/atomic_file/atomic.rs Outdated Show resolved Hide resolved
src/atomic_file/atomic.rs Outdated Show resolved Hide resolved
src/prop.rs Outdated Show resolved Hide resolved
src/prop.rs Outdated Show resolved Hide resolved
@gwendalF
Copy link
Collaborator Author

gwendalF commented Oct 18, 2023

I've updated the case to not override properties. May need check because there is a bunch of clone and staff, so we can surely do it better.

May need a visitor pattern to merge two serde_json::Value

@ARK-Builders ARK-Builders deleted a comment from GwendalFernet Oct 18, 2023
@ARK-Builders ARK-Builders deleted a comment from GwendalFernet Oct 18, 2023
@ARK-Builders ARK-Builders deleted a comment from GwendalFernet Oct 18, 2023
src/atomic_file/atomic.rs Outdated Show resolved Hide resolved
@gwendalF
Copy link
Collaborator Author

@kirillt I've updated the Link to use your solution of tempfile and moving to destination

src/link.rs Show resolved Hide resolved
@kirillt kirillt self-requested a review December 14, 2023 16:17
Copy link
Member

@kirillt kirillt left a comment

Choose a reason for hiding this comment

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

Good job

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Write files atomically
5 participants