Skip to content

Conversation

gpeal
Copy link
Collaborator

@gpeal gpeal commented Aug 20, 2025

No description provided.

@gpeal gpeal requested a review from bolinfest August 20, 2025 00:09
Copy link
Contributor

@chatgpt-codex-connector chatgpt-codex-connector bot left a 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".


/// 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)
Copy link
Collaborator

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"),
        ];

Copy link
Collaborator Author

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.


#[derive(Serialize, Deserialize, Clone, Debug)]
pub struct GitDiffToRemote {
pub sha: String,
Copy link
Collaborator

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.

.lines()
.map(|s| s.to_string())
.collect();
for file in untracked {
Copy link
Collaborator

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 {
Copy link
Collaborator

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?

@gpeal gpeal merged commit 77148a5 into main Aug 20, 2025
15 checks passed
@gpeal gpeal deleted the gpeal/diff-to-origin branch August 20, 2025 02:50
@github-actions github-actions bot locked and limited conversation to collaborators Aug 20, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants