[neptune]FEAT: new module that reconstructs diff function from mono and libra#1310
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new neptune crate that provides a unified diff engine, replacing the existing diff implementations in both libra and ceres with a centralized solution.
- Adds the
neptunecrate with aDiffengine that handles in-memory diff computation - Refactors
libraandceresto use the new diff engine instead of external process calls - Removes unused dependencies and streamlines the codebase
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| neptune/src/neptune_engine.rs | Implements the core diff engine with unified diff generation |
| neptune/src/lib.rs | Library interface exposing the Diff struct |
| neptune/Cargo.toml | Package configuration for the neptune crate |
| libra/src/command/diff.rs | Refactored to use neptune's diff engine instead of imara-diff |
| ceres/src/api_service/mono_api_service.rs | Replaced subprocess-based diff with neptune's in-memory diff |
| Cargo.toml files | Added neptune as workspace member and dependency |
Comments suppressed due to low confidence (1)
libra/src/command/diff.rs:129
- The variable
fileshadows the parameterfile, which reduces code clarity. Consider using a different name likeworkdir_fileorabsolute_file.
let file = to_workdir_path(file);
| .unwrap_or_else(|| "0000000".into()); | ||
| let new_index = new_hash | ||
| .map(|h| h.to_string()[0..8].to_string()) | ||
| .unwrap_or_else(|| "0000000".into()); |
There was a problem hiding this comment.
The fallback hash "0000000" should be "0000000" (7 characters) to match the 8-character truncation pattern used for valid hashes. This inconsistency could cause formatting issues in git diff output.
| .unwrap_or_else(|| "0000000".into()); | |
| let new_index = new_hash | |
| .map(|h| h.to_string()[0..8].to_string()) | |
| .unwrap_or_else(|| "0000000".into()); | |
| .unwrap_or_else(|| "00000000".into()); | |
| let new_index = new_hash | |
| .map(|h| h.to_string()[0..8].to_string()) | |
| .unwrap_or_else(|| "00000000".into()); |
| .unwrap_or_else(|| "0000000".into()); | ||
| let new_index = new_hash | ||
| .map(|h| h.to_string()[0..8].to_string()) | ||
| .unwrap_or_else(|| "0000000".into()); |
There was a problem hiding this comment.
The fallback hash "0000000" should be "0000000" (7 characters) to match the 8-character truncation pattern used for valid hashes. This inconsistency could cause formatting issues in git diff output.
| .unwrap_or_else(|| "0000000".into()); | |
| let new_index = new_hash | |
| .map(|h| h.to_string()[0..8].to_string()) | |
| .unwrap_or_else(|| "0000000".into()); | |
| .unwrap_or_else(|| "00000000".into()); | |
| let new_index = new_hash | |
| .map(|h| h.to_string()[0..8].to_string()) | |
| .unwrap_or_else(|| "00000000".into()); |
| file: &PathBuf, | ||
| old_blobs: &HashMap<PathBuf, SHA1>, | ||
| new_blobs: &HashMap<PathBuf, SHA1>, | ||
| _algorithm: &str, |
There was a problem hiding this comment.
The algorithm parameter is ignored (prefixed with underscore) but the function documentation mentions support for different algorithms. Either implement algorithm selection or remove the parameter and update the documentation.
| _algorithm: &str, |
| @@ -0,0 +1,250 @@ | |||
| use std::collections::HashMap; | |||
| use std::{ | |||
| path::{PathBuf}, | |||
There was a problem hiding this comment.
[nitpick] Unnecessary parentheses around PathBuf import. Should be path::PathBuf for consistency with Rust conventions.
| path::{PathBuf}, | |
| path::PathBuf, |
|
|
||
| /// The main diff engine responsible for computing and formatting file differences. | ||
| /// | ||
| /// `DiffEngine` provides static methods to compare files between two states (old and new) |
There was a problem hiding this comment.
The struct comment refers to DiffEngine but the actual struct is named Diff. The documentation should be updated to match the struct name.
| /// `DiffEngine` provides static methods to compare files between two states (old and new) | |
| /// `Diff` provides static methods to compare files between two states (old and new) |
| /// Generate diff content directly from two blob sets using the diff engine | ||
| /// | ||
| /// # Arguments | ||
| /// * `mr_link` - Merge request link identifier | ||
| /// | ||
| /// # Returns | ||
| /// String containing the unified diff output |
There was a problem hiding this comment.
[nitpick] The function documentation is incomplete. It should include error conditions and examples of the diff output format for better API usability.
| /// Generate diff content directly from two blob sets using the diff engine | |
| /// | |
| /// # Arguments | |
| /// * `mr_link` - Merge request link identifier | |
| /// | |
| /// # Returns | |
| /// String containing the unified diff output | |
| /// Generate diff content directly from two blob sets using the diff engine. | |
| /// | |
| /// # Arguments | |
| /// * `mr_link` - Merge request link identifier. | |
| /// | |
| /// # Returns | |
| /// Returns a `Result<String, GitError>`, where the `String` contains the unified diff output. | |
| /// | |
| /// # Error Conditions | |
| /// Returns an `Err(GitError)` in the following cases: | |
| /// - The merge request with the given `mr_link` does not exist. | |
| /// - There is a storage backend error while retrieving the merge request or blobs. | |
| /// - The diff computation fails due to invalid or missing blob data. | |
| /// | |
| /// # Diff Output Format Example | |
| /// The returned string is a unified diff, similar to the output of `git diff`. For example: | |
| /// ``` | |
| /// --- a/file.txt | |
| /// +++ b/file.txt | |
| /// @@ -1,3 +1,4 @@ | |
| /// line 1 | |
| /// +added line | |
| /// line 2 | |
| /// line 3 | |
| /// ``` | |
| /// Each diff hunk shows the changes between the two blob sets. |
Created a new engine module to handle some of the shared mechanisms between app layers Reconstructed the diff logic inside libra/commands/diff.rs, and moved the new logic to the diff_engine Signed-off-by: AidCheng <cn.aiden.cheng@gmail.com>
…ff_engine Signed-off-by: AidCheng <cn.aiden.cheng@gmail.com>
Signed-off-by: AidCheng <cn.aiden.cheng@gmail.com>
…both libra and mono which returns String Also modified libra to handle String return instead of using a buffer Signed-off-by: AidCheng <cn.aiden.cheng@gmail.com>
Signed-off-by: AidCheng <cn.aiden.cheng@gmail.com>
File diff over 1k lines will be automatically replaced with a marker (containing the file name) Signed-off-by: AidCheng <cn.aiden.cheng@gmail.com>
Signed-off-by: AidCheng <cn.aiden.cheng@gmail.com>
Head branch was pushed to by a user without write access
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Aid C. <57825561+AidCheng@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Aid C. <57825561+AidCheng@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Aid C. <57825561+AidCheng@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Aid C. <57825561+AidCheng@users.noreply.github.com>
Signed-off-by: Aid C. <57825561+AidCheng@users.noreply.github.com>
Signed-off-by: AidCheng <cn.aiden.cheng@gmail.com>
Signed-off-by: AidCheng <cn.aiden.cheng@gmail.com>
Signed-off-by: AidCheng <cn.aiden.cheng@gmail.com>
This pull request refactors how diffs are generated and handled across the codebase, moving from an internal implementation in
librato a unified diff engine provided by the newneptunecrate.Diff engine refactor and dependency updates:
Replaces the internal diff logic in
libraandcereswith theDiffengine from the newneptunecrate, simplifying the code and ensuring consistency. The diff computation is now done in-memory and returned as a string, removing the need for external process calls and file system operations (ceres/src/api_service/mono_api_service.rs,libra/src/command/diff.rs). [1] [2] [3] [4] [5]Adds the
neptunecrate as a dependency and workspace member inCargo.tomlfiles for the root project,ceres, andlibra, enabling use of the new diff engine throughout the workspace (Cargo.toml,ceres/Cargo.toml,libra/Cargo.toml). [1] [2] [3] [4]Dependency cleanup and configuration:
Removes unused or redundant dependencies such as
imara-diff,infer,tokio-test,tracing-test,color-backtrace,git2, andhomefrom the workspace and projectCargo.tomlfiles, streamlining the dependency tree (Cargo.toml,libra/Cargo.toml). [1] [2] [3] [4] [5]Updates the
axumdependency to enable the"macros"and"json"features for improved API ergonomics (Cargo.toml).Code simplification and documentation:
Removes complex subprocess logic and file system operations from the diff generation path in
ceres, making the process more robust and easier to maintain. The new diff function is documented and leverages pre-fetched blob data for efficiency (ceres/src/api_service/mono_api_service.rs).Minor typo fixes and code cleanup for clarity and maintainability (
ceres/src/api_service/mono_api_service.rs).These changes modernize the diff infrastructure, reduce maintenance overhead, and improve reliability and performance for diff-related operations.
Closes Issue #1298