-
Notifications
You must be signed in to change notification settings - Fork 122
feat(scorpio): complete MR layer creation #1233
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,4 +2,3 @@ | |
|
|
||
| pub mod issue_service; | ||
| pub mod mr_service; | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -3,46 +3,98 @@ use std::path::PathBuf; | |||||||||||||||||||||||||||||||
| use reqwest::Client; | ||||||||||||||||||||||||||||||||
| use serde::Deserialize; | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| use crate::manager::fetch::download_mr_files; | ||||||||||||||||||||||||||||||||
| use crate::util::config; | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| /// Single file record | ||||||||||||||||||||||||||||||||
| #[derive(Debug, Deserialize)] | ||||||||||||||||||||||||||||||||
| struct FileInfo { | ||||||||||||||||||||||||||||||||
| action: String, | ||||||||||||||||||||||||||||||||
| path: String, | ||||||||||||||||||||||||||||||||
| _sha: String, | ||||||||||||||||||||||||||||||||
| path: String, | ||||||||||||||||||||||||||||||||
| sha: String, | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| /// Response body for /files-list endpoint | ||||||||||||||||||||||||||||||||
| #[derive(Debug, Deserialize)] | ||||||||||||||||||||||||||||||||
| struct FilesListResp { | ||||||||||||||||||||||||||||||||
| data: Vec<FileInfo>, | ||||||||||||||||||||||||||||||||
| data: Vec<FileInfo>, | ||||||||||||||||||||||||||||||||
| err_message: String, | ||||||||||||||||||||||||||||||||
| req_result: bool, | ||||||||||||||||||||||||||||||||
| req_result: bool, | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| pub async fn build_mr_layer( | ||||||||||||||||||||||||||||||||
| link: &str, | ||||||||||||||||||||||||||||||||
| _mr_path:PathBuf | ||||||||||||||||||||||||||||||||
| ) -> Result<(), reqwest::Error> { | ||||||||||||||||||||||||||||||||
| mr_path: PathBuf, | ||||||||||||||||||||||||||||||||
| ) -> Result<(), Box<dyn std::error::Error>> { | ||||||||||||||||||||||||||||||||
| let files_list = fetch_files_list(link).await?; | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| if !files_list.req_result { | ||||||||||||||||||||||||||||||||
| println!("{}", files_list.err_message); | ||||||||||||||||||||||||||||||||
| return Ok(()); | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
| if !mr_path.exists() { | ||||||||||||||||||||||||||||||||
| std::fs::create_dir_all(&mr_path)?; | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| // Collect all files that need to be downloaded | ||||||||||||||||||||||||||||||||
| let mut download_files = Vec::new(); | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| for file in files_list.data { | ||||||||||||||||||||||||||||||||
| if file.action == "remove" { | ||||||||||||||||||||||||||||||||
| // ! ! ! Attention: | ||||||||||||||||||||||||||||||||
| // As for deleted files, the path should create a `whiteout file`, refer to create_whiteout in other files . | ||||||||||||||||||||||||||||||||
| println!("{}", file.path); | ||||||||||||||||||||||||||||||||
| let relative_path = file.path.strip_prefix('/').unwrap_or(&file.path); | ||||||||||||||||||||||||||||||||
| let file_path = mr_path.join(relative_path); | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
| let file_path = mr_path.join(relative_path); | |
| let file_path = mr_path.join(relative_path).canonicalize()?; | |
| if !file_path.starts_with(&mr_path) { | |
| eprintln!("Invalid file path detected: {}", file.path); | |
| continue; | |
| } |
Copilot
AI
Jul 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Using expect will panic on invalid input. Consider parsing with ? and returning an error so failures in SHA1 parsing are handled gracefully.
| let file_id = file.sha.parse().expect("Invalid SHA1 format"); | |
| let file_id = file.sha.parse()?; |
Copilot
AI
Jul 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The mode 0o777 is a magic number; consider defining a named constant or adding a comment explaining why this permission is chosen to improve readability.
| let mode = libc::S_IFCHR | 0o777; | |
| const WHITEOUT_FILE_PERMISSIONS: u32 = 0o777; // Full read, write, and execute permissions for whiteout files | |
| let mode = libc::S_IFCHR | WHITEOUT_FILE_PERMISSIONS; |
Copilot
AI
Jul 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code prints an error when mknod fails but continues execution. Consider returning an Err to propagate the failure and stop further processing when whiteout creation fails.
| eprintln!("Failed to create whiteout file {}: {}", file.path, err); | |
| return Err(Box::new(std::io::Error::new( | |
| err.kind(), | |
| format!("Failed to create whiteout file {}: {}", file.path, err), | |
| ))); |
Copilot
AI
Jul 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Printing the error without propagating it may cause silent failures in whiteout creation. Consider returning an error or handling it upstream instead of only logging.
| eprintln!("Failed to create whiteout file {}: {}", file.path, err); | |
| return Err(Box::new(std::io::Error::new( | |
| err.kind(), | |
| format!("Failed to create whiteout file {}: {}", file.path, err), | |
| ))); |
Copilot
AI
Jul 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Currently the error when creating a whiteout file is only logged; consider propagating this error via the function's Result to ensure upstream error handling instead of silently continuing.
| eprintln!("Failed to create whiteout file {}: {}", file.path, err); | |
| return Err(Box::new(std::io::Error::new( | |
| err.kind(), | |
| format!("Failed to create whiteout file {}: {}", file.path, err), | |
| ))); |
Copilot
AI
Jul 15, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Using println! for unexpected actions may be insufficient for production diagnostics—consider using a logger (e.g., log::warn!) for proper log levels and context.
Copilot
AI
Jul 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The implementation of fetch_files_list no longer calls .send().await, .error_for_status(), or .json(), so it returns a request builder instead of sending the HTTP request and parsing JSON. Restore the request chain to perform the request and deserialize the response.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The result of
overlay_mountis ignored, which can hide mounting errors. It would be better to handle or at least log any potential errors rather than discarding the result.