feat: libra add cherry-pick subcommand.#1270
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
09aeb92 to
ac8d1e3
Compare
ac8d1e3 to
28bb776
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR implements the libra cherry-pick subcommand to apply changes from existing commits onto the current branch. The feature allows selective application of commits from one branch to another without merging the entire branch.
Key Changes:
- Added cherry-pick command with support for single/multiple commits and
--no-commitoption - Implemented three-way merge logic to apply commit changes
- Added comprehensive test coverage for various cherry-pick scenarios
Reviewed Changes
Copilot reviewed 7 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| libra/src/command/cherry_pick.rs | Core implementation of cherry-pick functionality with three-way merge logic |
| libra/src/command/mod.rs | Exposed the new cherry_pick module |
| libra/src/cli.rs | Added cherry-pick command to CLI interface |
| libra/tests/command/cherry_pick_test.rs | Comprehensive test suite covering basic, multi-commit, and error scenarios |
| libra/tests/command/mod.rs | Added cherry_pick_test module |
| libra/tests/command/revert_test.rs | Updated CommitArgs with new disable_pre field |
| aria/contents/docs/libra/command/cherry-pick/index.mdx | Documentation for the cherry-pick command |
Comments suppressed due to low confidence (1)
libra/tests/command/cherry_pick_test.rs:434
- The test executes cherry-pick with a non-existent commit but doesn't assert that it actually fails or check the error behavior. Add assertions to verify the expected error handling.
// Test cherry-picking non-existent commit should fail gracefully
| /// | ||
| /// This is used when applying cherry-pick changes to update the index state. | ||
| fn update_index_entry(index: &mut Index, path: &Path, hash: SHA1) { | ||
| let blob = mercury::internal::object::blob::Blob::load(&hash); |
There was a problem hiding this comment.
The Blob::load call doesn't handle potential errors. If the blob cannot be loaded, this will likely panic or return an invalid blob. Consider adding error handling with a Result return type.
| let blob = mercury::internal::object::blob::Blob::load(&hash); | |
| let blob = match mercury::internal::object::blob::Blob::load(&hash) { | |
| Ok(blob) => blob, | |
| Err(e) => { | |
| eprintln!("Failed to load blob for hash {}: {}", hash, e); | |
| return; // Exit the function if the blob cannot be loaded | |
| } | |
| }; |
| for path_buf in &tracked_paths { | ||
| let path_str = path_buf.to_str().unwrap(); | ||
| if let Some(entry) = index.get(path_str, 0) { | ||
| let blob = mercury::internal::object::blob::Blob::load(&entry.hash); |
There was a problem hiding this comment.
Same issue as line 245 - Blob::load call lacks error handling. This could cause the function to panic if the blob cannot be loaded from storage.
| let blob = mercury::internal::object::blob::Blob::load(&entry.hash); | |
| let blob = mercury::internal::object::blob::Blob::load(&entry.hash) | |
| .map_err(|e| format!("Failed to load blob: {}", e))?; |
| if args.no_commit && args.commits.len() > 1 { | ||
| eprintln!("fatal: cannot cherry-pick multiple commits with --no-commit"); | ||
| eprintln!("(use 'libra commit' to save the changes from the first cherry-pick)"); | ||
| return; | ||
| } |
There was a problem hiding this comment.
[nitpick] This artificial limitation differs from Git's behavior and reduces functionality. Consider implementing proper multi-commit support with --no-commit or documenting this as a known limitation in the help text.
| if args.no_commit && args.commits.len() > 1 { | |
| eprintln!("fatal: cannot cherry-pick multiple commits with --no-commit"); | |
| eprintln!("(use 'libra commit' to save the changes from the first cherry-pick)"); | |
| return; | |
| } | |
| // TODO: Implement proper multi-commit support for --no-commit to align with Git's behavior. |
| /// TODO: Now, it will apply the picked commits exactly as they are, | ||
| /// without resolving any conflicts, and it will not take the state of the staging area into account. | ||
| pub async fn execute(args: CherryPickArgs) { | ||
| if !util::check_repo_exist() { | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
The TODO comment indicates incomplete conflict resolution functionality. This is a significant limitation that should be documented in the command help or user documentation, not just in code comments.
| /// TODO: Now, it will apply the picked commits exactly as they are, | |
| /// without resolving any conflicts, and it will not take the state of the staging area into account. | |
| pub async fn execute(args: CherryPickArgs) { | |
| if !util::check_repo_exist() { | |
| return; | |
| } | |
| /// | |
| /// Note: The cherry-pick operation applies commits exactly as they are, without resolving conflicts | |
| /// or considering the state of the staging area. | |
| pub async fn execute(args: CherryPickArgs) { | |
| if !util::check_repo_exist() { | |
| return; | |
| } | |
| println!("Note: Conflicts will not be resolved, and the staging area will not be considered during cherry-pick."); |
Part of #1239
实现libra cherry-pick 子命令的基本功能
测试: