ODP-5235: Make backups atomic by default #6
Conversation
This adds some more specific error types so that we can pass more usable information to a user when something goes wrong. This includes giving more suggestions of how to proceed.
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
This enables safer backups by cloning/updating them in a temporary directory, then moves them into the correct place if and only if they succeed. This behaviour can be overridden with --no-atomic when creating backups.
This enables safer backups by cloning/updating them in a temporary directory, then moves them into the correct place if and only if they succeed. This behaviour can be overridden with --no-atomic when creating backups.
JeffreySmith
force-pushed
the
ODP-5235
branch
from
6345db2 to
3aa0291
Compare
JeffreySmith
force-pushed
the
ODP-5235
branch
from
a896407 to
ee40fc3
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR implements atomic backups for repository operations by ensuring all data is written to temporary directories first before being moved to the final location. This prevents incomplete or corrupted backups if operations are interrupted.
Key changes:
- Modified backup functions to use temporary paths during cloning/updating operations
- Added atomic parameter to backup commands with default true value
- Implemented recursive file copying utility for atomic updates
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/utils/mod.rs | Renamed file_replace module to file_utils |
| src/utils/file_utils.rs | Added copy_recursive function and updated documentation |
| src/github/match_args.rs | Updated backup function calls to include atomic parameter |
| src/github/branch.rs | Updated import path for file utilities |
| src/github/backup.rs | Implemented atomic backup logic with temporary directories |
| src/cli.rs | Added atomic flag to backup command configuration |
| README.md | Updated documentation to describe atomic backup functionality |
| Cargo.toml | Enabled strip option in release profile |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
src/github/backup.rs
Outdated
| let actual_commmand = if atomic { | ||
| tmp_path.clone() | ||
| } else { | ||
| path.clone() | ||
| }; | ||
| Err(GitError::ExecutionError { | ||
| command: format!("git remote update -C {path}"), | ||
| command: format!("git remote update -C {actual_commmand}"), |
There was a problem hiding this comment.
Corrected spelling of 'commmand' to 'command'.
src/github/backup.rs
Outdated
| Ok(s) => { | ||
| if atomic && tmp_directory.exists() { | ||
| // If the clone failed, remove the directory to avoid leaving a broken repo | ||
| std::fs::remove_dir_all(&clone_output)?; |
There was a problem hiding this comment.
Variable clone_output is used instead of tmp_path. In atomic mode, when cleaning up after failure, the temporary directory should be removed, not the clone output path.
| std::fs::remove_dir_all(&clone_output)?; | |
| std::fs::remove_dir_all(tmp_directory)?; |
src/github/backup.rs
Outdated
| if atomic && Path::new(&tmp_path).exists() { | ||
| // If the clone failed, remove the directory to avoid leaving a broken repo | ||
| std::fs::remove_dir_all(&path)?; | ||
| std::fs::remove_dir_all(&clone_output)?; |
There was a problem hiding this comment.
Variable clone_output is used instead of tmp_path. In atomic mode, when cleaning up after a failed clone, the temporary directory should be removed, not the clone output path.
src/github/backup.rs
Outdated
| Err(e) => { | ||
| if atomic && Path::new(&tmp_path).exists() { | ||
| // If the clone failed, remove the directory to avoid leaving a broken repo | ||
| std::fs::remove_dir_all(&clone_output).ok(); |
There was a problem hiding this comment.
Variable clone_output is used instead of tmp_path. In atomic mode, when cleaning up after an error, the temporary directory should be removed, not the clone output path.
This makes it so that the whole repository is fetched into a temp directory, then moved into the right place afterwards. This also applies to updating, where a copy of the repository is made first, then updated, and the original is only replaced after it has been successfully updated.