-
Notifications
You must be signed in to change notification settings - Fork 3.9k
[wip] Allow skipping git history for fetching git dependencies #18107
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
base: main
Are you sure you want to change the base?
Conversation
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.
This is the final PR Bugbot will review for you during this billing cycle
Your free Bugbot reviews will reset on December 7
Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
| shallow_clone_repo(git_url, git_path, dep_name)?; | ||
|
|
||
| shallow_fetch_latest_origin_rev(git_path, git_rev, dep_name)?; | ||
| switch_to_fetched_rev(git_path, git_rev, dep_name)?; |
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.
Bug: Dependency Fetch Silently Fails
The fetch_new_dependency_shallow function doesn't check the exit status of shallow_fetch_latest_origin_rev and switch_to_fetched_rev. If these git commands fail (non-zero exit code), the function returns Ok(()) instead of an error. This differs from update_dependency_shallow which properly checks the status, creating inconsistent error handling.
| anyhow::anyhow!("Failed to clone Git repository for package '{}'", dep_name) | ||
| })?; | ||
| Ok(()) | ||
| } |
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.
Bug: Git Clone Errors Go Unnoticed
The deep_clone_repo function uses .output() but doesn't check if the git command succeeded via output.status.success(). If git fails (invalid URL, network error, permission denied), the function returns Ok(()) instead of propagating the error, causing silent failures during dependency cloning.
| ) | ||
| })?; | ||
| Ok(()) | ||
| } |
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.
Bug: Git Commands Fail Silently
The shallow_clone_repo function uses .output() but doesn't check if the git command succeeded via output.status.success(). If git fails (invalid URL, network error, permission denied), the function returns Ok(()) instead of propagating the error, causing silent failures during shallow cloning.
| ) | ||
| })?; | ||
| Ok(()) | ||
| } |
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.
Bug: Silent Failures in Git Checkout
The checkout_rev function uses .output() but doesn't check if the git command succeeded via output.status.success(). If git checkout fails (invalid ref, detached HEAD issues), the function returns Ok(()) instead of propagating the error, causing silent failures when checking out specific revisions.
| .output()?; | ||
| let stdout = String::from_utf8(output.stdout)?; | ||
| Ok(stdout.trim().to_string()) | ||
| } |
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.
Bug: Incorrectly parsing failed command output.
The get_existing_rev function doesn't check output.status.success() before parsing stdout. If git fails (invalid ref, not a git repo), it attempts to parse error output as a valid revision string, potentially returning empty strings or error messages instead of failing properly.
| .output()?; | ||
| let stdout = String::from_utf8(output.stdout)?; | ||
| Ok(stdout.trim().to_string()) | ||
| } |
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.
Bug: Confusing Git Errors for Valid Tags
The get_existing_tag function doesn't check output.status.success() before parsing stdout. If git fails (not a git repo, command error), it attempts to parse error output as a valid tag string, potentially returning empty strings or error messages instead of failing properly.
APTOS_GIT_OPTIMIZEDenvironment variableNote
Refactors git operations into a new module and enables optional shallow clone/fetch for git dependencies when APTOS_GIT_OPTIMIZED is set.
resolution/git.rsmodule: Centralizes git ops (fetch/updatedeep/shallow,confirm_git_available,get_existing_rev/tag).fetch_new_dependency_shallowandupdate_dependency_shallowusing--depth 1andFETCH_HEAD.APTOS_GIT_OPTIMIZEDto choose shallow vs deep clone/update paths.resolution_graph.rsnow callsgitmodule for all git actions; skips updates when current rev/tag matches; removes inline git command code and localconfirm_git_available.pub(crate) mod git;inresolution/mod.rs.Written by Cursor Bugbot for commit 102df06. This will update automatically on new commits. Configure here.