-
Notifications
You must be signed in to change notification settings - Fork 5.5k
Diff command #2476
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
Diff command #2476
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.
Codex Review: Here are some suggestions.
Reply with @codex fix comments
to fix any unresolved comments.
About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you open a pull request that is ready for review, or mark a draft as ready for review. You can also ask for a review by commenting "@codex review".
Codex can also answer questions or update the PR. Try commenting "@codex fix this CI failure" or "@codex address that feedback".
codex-rs/core/src/git_info.rs
Outdated
|
||
/// Returns the closest git sha to HEAD that is on a remote as well as the diff to that sha. | ||
pub async fn git_diff_to_remote(cwd: &Path) -> Option<GitDiffToRemote> { | ||
let is_git_repo = run_git_command_with_timeout(&["rev-parse", "--git-dir"], cwd) |
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.
Hmm, should run_git_command_with_timeout()
add these env vars like we do in the unit tests:
let envs = vec![
("GIT_CONFIG_GLOBAL", "/dev/null"),
("GIT_CONFIG_NOSYSTEM", "1"),
];
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.
I don't think so. We don't for other non-test use cases. I think we set these for tests so local git settings don't interfere with tests but when not in a test, we want this to respect their git settings.
codex-rs/core/src/git_info.rs
Outdated
|
||
#[derive(Serialize, Deserialize, Clone, Debug)] | ||
pub struct GitDiffToRemote { | ||
pub sha: 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.
Consider sha1::Digest
as the type instead of String
.
codex-rs/core/src/git_info.rs
Outdated
.lines() | ||
.map(|s| s.to_string()) | ||
.collect(); | ||
for file in untracked { |
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.
Consider running these in parallel?
|
||
#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, TS)] | ||
#[serde(rename_all = "camelCase")] | ||
pub struct GitDiffToRemoteParams { |
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.
Create a specific response type?
No description provided.