diff --git a/codex-rs/core/src/git_info_tests.rs b/codex-rs/core/src/git_info_tests.rs index f3be795669..24413a18d6 100644 --- a/codex-rs/core/src/git_info_tests.rs +++ b/codex-rs/core/src/git_info_tests.rs @@ -320,6 +320,19 @@ async fn assert_test_git(cwd: &Path, args: &[&str]) { ); } +async fn test_git_stdout(cwd: &Path, args: &[&str]) -> String { + let output = run_test_git(cwd, args).await; + assert!( + output.status.success(), + "git {args:?} failed: {}", + String::from_utf8_lossy(&output.stderr) + ); + String::from_utf8(output.stdout) + .expect("git output should be UTF-8") + .trim() + .to_string() +} + #[tokio::test] async fn test_default_branch_discovery_uses_verified_remote_head() { let temp_dir = TempDir::new().expect("Failed to create temp dir"); @@ -456,6 +469,203 @@ async fn test_default_branch_discovery_from_linked_worktree() { ); } +#[tokio::test] +async fn test_git_diff_to_remote_preserves_verified_remote_head_identity() { + let temp_dir = TempDir::new().expect("Failed to create temp dir"); + let repo_path = create_test_git_repo(&temp_dir).await; + assert_test_git(&repo_path, &["branch", "-M", "main"]).await; + assert_test_git( + &repo_path, + &["remote", "add", "origin", "/nonexistent/origin.git"], + ) + .await; + assert_test_git( + &repo_path, + &["remote", "add", "upstream", "/nonexistent/upstream.git"], + ) + .await; + + assert_test_git(&repo_path, &["checkout", "-b", "origin-main", "main"]).await; + fs::write(repo_path.join("which.txt"), "origin\n").expect("write origin file"); + assert_test_git(&repo_path, &["add", "which.txt"]).await; + assert_test_git(&repo_path, &["commit", "-m", "origin main"]).await; + let origin_sha = test_git_stdout(&repo_path, &["rev-parse", "HEAD"]).await; + assert_test_git( + &repo_path, + &["update-ref", "refs/remotes/origin/main", &origin_sha], + ) + .await; + + assert_test_git(&repo_path, &["checkout", "main"]).await; + assert_test_git(&repo_path, &["checkout", "-b", "upstream-main", "main"]).await; + fs::write(repo_path.join("which.txt"), "upstream\n").expect("write upstream file"); + assert_test_git(&repo_path, &["add", "which.txt"]).await; + assert_test_git(&repo_path, &["commit", "-m", "upstream main"]).await; + let upstream_sha = test_git_stdout(&repo_path, &["rev-parse", "HEAD"]).await; + assert_test_git( + &repo_path, + &["update-ref", "refs/remotes/upstream/main", &upstream_sha], + ) + .await; + assert_test_git( + &repo_path, + &[ + "symbolic-ref", + "refs/remotes/upstream/HEAD", + "refs/remotes/upstream/main", + ], + ) + .await; + + assert_test_git(&repo_path, &["checkout", "-b", "feature", "upstream-main"]).await; + fs::write(repo_path.join("feature.txt"), "feature\n").expect("write feature file"); + assert_test_git(&repo_path, &["add", "feature.txt"]).await; + assert_test_git(&repo_path, &["commit", "-m", "feature work"]).await; + + assert_eq!( + default_branch_name(&repo_path).await, + Some("main".to_string()) + ); + let state = git_diff_to_remote(&repo_path) + .await + .expect("Should collect working tree state"); + assert_eq!(state.sha, GitSha::new(&upstream_sha)); + assert!(state.diff.contains("feature.txt")); + assert!(!state.diff.contains("which.txt")); + assert!(!state.diff.contains("-origin")); + assert!(!state.diff.contains("+upstream")); +} + +#[tokio::test] +async fn test_git_diff_to_remote_keeps_remote_default_when_current_name_matches() { + let temp_dir = TempDir::new().expect("Failed to create temp dir"); + let repo_path = create_test_git_repo(&temp_dir).await; + assert_test_git(&repo_path, &["branch", "-M", "base"]).await; + assert_test_git( + &repo_path, + &["remote", "add", "origin", "/nonexistent/origin.git"], + ) + .await; + assert_test_git( + &repo_path, + &["remote", "add", "upstream", "/nonexistent/upstream.git"], + ) + .await; + + assert_test_git(&repo_path, &["checkout", "-b", "origin-release", "base"]).await; + fs::write(repo_path.join("origin-one.txt"), "origin one\n").expect("write origin file"); + assert_test_git(&repo_path, &["add", "origin-one.txt"]).await; + assert_test_git(&repo_path, &["commit", "-m", "origin release one"]).await; + fs::write(repo_path.join("origin-two.txt"), "origin two\n").expect("write origin file"); + assert_test_git(&repo_path, &["add", "origin-two.txt"]).await; + assert_test_git(&repo_path, &["commit", "-m", "origin release two"]).await; + let origin_sha = test_git_stdout(&repo_path, &["rev-parse", "HEAD"]).await; + assert_test_git( + &repo_path, + &["update-ref", "refs/remotes/origin/release/v1", &origin_sha], + ) + .await; + + assert_test_git(&repo_path, &["checkout", "base"]).await; + assert_test_git(&repo_path, &["checkout", "-b", "upstream-release", "base"]).await; + fs::write(repo_path.join("upstream.txt"), "upstream\n").expect("write upstream file"); + assert_test_git(&repo_path, &["add", "upstream.txt"]).await; + assert_test_git(&repo_path, &["commit", "-m", "upstream release"]).await; + let upstream_sha = test_git_stdout(&repo_path, &["rev-parse", "HEAD"]).await; + assert_test_git( + &repo_path, + &[ + "update-ref", + "refs/remotes/upstream/release/v1", + &upstream_sha, + ], + ) + .await; + assert_test_git( + &repo_path, + &[ + "symbolic-ref", + "refs/remotes/upstream/HEAD", + "refs/remotes/upstream/release/v1", + ], + ) + .await; + + assert_test_git( + &repo_path, + &["checkout", "-b", "release/v1", "upstream-release"], + ) + .await; + fs::write(repo_path.join("local.txt"), "local\n").expect("write local file"); + assert_test_git(&repo_path, &["add", "local.txt"]).await; + assert_test_git(&repo_path, &["commit", "-m", "local release work"]).await; + + assert_eq!( + default_branch_name(&repo_path).await, + Some("release/v1".to_string()) + ); + let state = git_diff_to_remote(&repo_path) + .await + .expect("Should collect working tree state"); + assert_eq!(state.sha, GitSha::new(&upstream_sha)); + assert!(state.diff.contains("local.txt")); + assert!(!state.diff.contains("upstream.txt")); + assert!(!state.diff.contains("origin-one.txt")); + assert!(!state.diff.contains("origin-two.txt")); +} + +#[tokio::test] +async fn test_git_diff_to_remote_preserves_expanded_remote_ref_identity() { + let temp_dir = TempDir::new().expect("Failed to create temp dir"); + let repo_path = create_test_git_repo(&temp_dir).await; + assert_test_git(&repo_path, &["branch", "-M", "base"]).await; + assert_test_git( + &repo_path, + &["remote", "add", "origin", "/nonexistent/origin.git"], + ) + .await; + assert_test_git( + &repo_path, + &["remote", "add", "upstream", "/nonexistent/upstream.git"], + ) + .await; + + assert_test_git(&repo_path, &["checkout", "-b", "origin-topic", "base"]).await; + fs::write(repo_path.join("origin.txt"), "origin\n").expect("write origin file"); + assert_test_git(&repo_path, &["add", "origin.txt"]).await; + assert_test_git(&repo_path, &["commit", "-m", "origin topic"]).await; + let origin_sha = test_git_stdout(&repo_path, &["rev-parse", "HEAD"]).await; + assert_test_git( + &repo_path, + &["update-ref", "refs/remotes/origin/topic", &origin_sha], + ) + .await; + + assert_test_git(&repo_path, &["checkout", "base"]).await; + assert_test_git(&repo_path, &["checkout", "-b", "feature", "base"]).await; + fs::write(repo_path.join("feature.txt"), "feature\n").expect("write feature file"); + assert_test_git(&repo_path, &["add", "feature.txt"]).await; + assert_test_git(&repo_path, &["commit", "-m", "feature work"]).await; + assert_test_git(&repo_path, &["checkout", "-b", "upstream-topic", "feature"]).await; + fs::write(repo_path.join("upstream-only.txt"), "upstream\n").expect("write upstream-only file"); + assert_test_git(&repo_path, &["add", "upstream-only.txt"]).await; + assert_test_git(&repo_path, &["commit", "-m", "upstream topic"]).await; + let upstream_sha = test_git_stdout(&repo_path, &["rev-parse", "HEAD"]).await; + assert_test_git( + &repo_path, + &["update-ref", "refs/remotes/upstream/topic", &upstream_sha], + ) + .await; + assert_test_git(&repo_path, &["checkout", "feature"]).await; + + assert_eq!(default_branch_name(&repo_path).await, None); + let state = git_diff_to_remote(&repo_path) + .await + .expect("Should collect working tree state"); + assert_eq!(state.sha, GitSha::new(&upstream_sha)); + assert_ne!(state.sha, GitSha::new(&origin_sha)); +} + #[cfg(unix)] #[tokio::test] async fn test_default_branch_discovery_does_not_invoke_remote_transport() { diff --git a/codex-rs/git-utils/src/info.rs b/codex-rs/git-utils/src/info.rs index 31750f62fa..f6db5db539 100644 --- a/codex-rs/git-utils/src/info.rs +++ b/codex-rs/git-utils/src/info.rs @@ -478,12 +478,34 @@ async fn get_git_remotes(cwd: &Path) -> Option> { Some(remotes) } +#[derive(Clone, Debug, Eq, Hash, PartialEq)] +struct BranchCandidate { + name: String, + remote_ref: Option, +} + +impl BranchCandidate { + fn local(name: String) -> Self { + Self { + name, + remote_ref: None, + } + } + + fn remote(name: String, remote_ref: String) -> Self { + Self { + name, + remote_ref: Some(remote_ref), + } + } +} + /// Attempt to determine the repository's default branch name. /// /// Preference order: /// 1) The symbolic ref at `refs/remotes//HEAD` for the first remote (origin prioritized) /// 2) Local fallback to existing `main` or `master` if present -async fn get_default_branch(cwd: &Path) -> Option { +async fn get_default_branch(cwd: &Path) -> Option { // Prefer the first remote (with origin prioritized) let remotes = get_git_remotes(cwd).await.unwrap_or_default(); for remote in remotes { @@ -493,7 +515,9 @@ async fn get_default_branch(cwd: &Path) -> Option { } // No remote-derived default; try common local defaults if they exist - get_default_branch_local(cwd).await + get_default_branch_local(cwd) + .await + .map(BranchCandidate::local) } /// Resolves one remote's symbolic HEAD using only local refs. @@ -501,7 +525,10 @@ async fn get_default_branch(cwd: &Path) -> Option { /// The symbolic target must remain inside the selected remote's namespace and /// resolve to an existing ref. This rejects stale or cross-remote symbolic refs /// while preserving branch names that contain `/`. -async fn get_remote_default_branch_from_symbolic_ref(cwd: &Path, remote: &str) -> Option { +async fn get_remote_default_branch_from_symbolic_ref( + cwd: &Path, + remote: &str, +) -> Option { let remote_head = format!("refs/remotes/{remote}/HEAD"); let symref_output = run_git_command_with_timeout(&["symbolic-ref", "--quiet", &remote_head], cwd).await?; @@ -519,7 +546,10 @@ async fn get_remote_default_branch_from_symbolic_ref(cwd: &Path, remote: &str) - let verify = run_git_command_with_timeout(&["rev-parse", "--verify", "--quiet", target], cwd).await?; - verify.status.success().then(|| branch.to_string()) + verify + .status + .success() + .then(|| BranchCandidate::remote(branch.to_string(), target.to_string())) } /// Determine the repository's default branch name, if available. @@ -529,7 +559,9 @@ async fn get_remote_default_branch_from_symbolic_ref(cwd: &Path, remote: &str) - /// `master`. Returns `None` when the information cannot be determined, for /// example when the current directory is not inside a Git repository. pub async fn default_branch_name(cwd: &Path) -> Option { - get_default_branch(cwd).await + get_default_branch(cwd) + .await + .map(|candidate| candidate.name) } /// Attempt to determine the repository's default branch name from local branches. @@ -556,7 +588,7 @@ async fn get_default_branch_local(cwd: &Path) -> Option { /// Build an ancestry of branches starting at the current branch and ending at the /// repository's default branch (if determinable).. -async fn branch_ancestry(cwd: &Path) -> Option> { +async fn branch_ancestry(cwd: &Path) -> Option> { // Discover current branch (ignore detached HEAD by treating it as None) let current_branch = run_git_command_with_timeout(&["rev-parse", "--abbrev-ref", "HEAD"], cwd) .await @@ -568,21 +600,21 @@ async fn branch_ancestry(cwd: &Path) -> Option> { } }) .map(|s| s.trim().to_string()) - .filter(|s| s != "HEAD"); + .filter(|s| s != "HEAD") + .map(BranchCandidate::local); // Discover default branch let default_branch = get_default_branch(cwd).await; - let mut ancestry: Vec = Vec::new(); - let mut seen: HashSet = HashSet::new(); - if let Some(cb) = current_branch.clone() { + let mut ancestry: Vec = Vec::new(); + let mut seen: HashSet = HashSet::new(); + if let Some(cb) = current_branch { seen.insert(cb.clone()); ancestry.push(cb); } if let Some(db) = default_branch - && !seen.contains(&db) + && seen.insert(db.clone()) { - seen.insert(db.clone()); ancestry.push(db); } @@ -595,7 +627,7 @@ async fn branch_ancestry(cwd: &Path) -> Option> { if let Some(output) = run_git_command_with_timeout( &[ "for-each-ref", - "--format=%(refname:short)", + "--format=%(refname)", "--contains=HEAD", &format!("refs/remotes/{remote}"), ], @@ -606,14 +638,16 @@ async fn branch_ancestry(cwd: &Path) -> Option> { && let Ok(text) = String::from_utf8(output.stdout) { for line in text.lines() { - let short = line.trim(); - // Expect format like: "origin/feature"; extract the branch path after "remote/" - if let Some(stripped) = short.strip_prefix(&format!("{remote}/")) + let remote_ref = line.trim(); + let remote_ref_prefix = format!("refs/remotes/{remote}/"); + if let Some(stripped) = remote_ref.strip_prefix(&remote_ref_prefix) && !stripped.is_empty() - && !seen.contains(stripped) { - seen.insert(stripped.to_string()); - ancestry.push(stripped.to_string()); + let candidate = + BranchCandidate::remote(stripped.to_string(), remote_ref.to_string()); + if seen.insert(candidate.clone()) { + ancestry.push(candidate); + } } } } @@ -623,102 +657,75 @@ async fn branch_ancestry(cwd: &Path) -> Option> { Some(ancestry) } -// Helper for a single branch: return the remote SHA if present on any remote -// and the distance (commits ahead of HEAD) for that branch. The first item is -// None if the branch is not present on any remote. Returns None if distance -// could not be computed due to git errors/timeouts. +// Helper for a single branch: return the selected remote SHA and the distance +// (commits ahead of that SHA) for the current HEAD. Remote-qualified candidates +// retain their exact ref; local candidates use the first matching remote ref. +// Returns None if no remote ref exists or a git error/timeout occurs. async fn branch_remote_and_distance( cwd: &Path, - branch: &str, + branch: &BranchCandidate, remotes: &[String], -) -> Option<(Option, usize)> { - // Try to find the first remote ref that exists for this branch (origin prioritized by caller). - let mut found_remote_sha: Option = None; - let mut found_remote_ref: Option = None; - for remote in remotes { - let remote_ref = format!("refs/remotes/{remote}/{branch}"); - let Some(verify_output) = - run_git_command_with_timeout(&["rev-parse", "--verify", "--quiet", &remote_ref], cwd) - .await - else { - // Mirror previous behavior: if the verify call times out/fails at the process level, - // treat the entire branch as unusable. - return None; - }; +) -> Option<(GitSha, usize)> { + let remote_sha = if let Some(remote_ref) = branch.remote_ref.as_deref() { + let verify_output = + run_git_command_with_timeout(&["rev-parse", "--verify", "--quiet", remote_ref], cwd) + .await?; if !verify_output.status.success() { - continue; - } - let Ok(sha) = String::from_utf8(verify_output.stdout) else { - // Mirror previous behavior and skip the entire branch on parse failure. return None; - }; - found_remote_sha = Some(GitSha::new(sha.trim())); - found_remote_ref = Some(remote_ref); - break; - } - - // Compute distance as the number of commits HEAD is ahead of the branch. - // Prefer local branch name if it exists; otherwise fall back to the remote ref (if any). - let count_output = if let Some(local_count) = - run_git_command_with_timeout(&["rev-list", "--count", &format!("{branch}..HEAD")], cwd) - .await - { - if local_count.status.success() { - local_count - } else if let Some(remote_ref) = &found_remote_ref { - match run_git_command_with_timeout( - &["rev-list", "--count", &format!("{remote_ref}..HEAD")], + } + let sha = String::from_utf8(verify_output.stdout).ok()?; + GitSha::new(sha.trim()) + } else { + let mut found_remote_sha: Option = None; + for remote in remotes { + let remote_ref = format!("refs/remotes/{remote}/{}", branch.name); + let Some(verify_output) = run_git_command_with_timeout( + &["rev-parse", "--verify", "--quiet", &remote_ref], cwd, ) .await - { - Some(remote_count) => remote_count, - None => return None, + else { + return None; + }; + if !verify_output.status.success() { + continue; } - } else { - return None; + let sha = String::from_utf8(verify_output.stdout).ok()?; + found_remote_sha = Some(GitSha::new(sha.trim())); + break; } - } else if let Some(remote_ref) = &found_remote_ref { - match run_git_command_with_timeout( - &["rev-list", "--count", &format!("{remote_ref}..HEAD")], - cwd, - ) - .await - { - Some(remote_count) => remote_count, - None => return None, - } - } else { - return None; + found_remote_sha? }; + // Resolve distance from the selected SHA, rather than allowing a same-named + // local branch to silently replace the verified remote candidate. + let count_output = run_git_command_with_timeout( + &["rev-list", "--count", &format!("{}..HEAD", remote_sha.0)], + cwd, + ) + .await?; if !count_output.status.success() { return None; } - let Ok(distance_str) = String::from_utf8(count_output.stdout) else { - return None; - }; - let Ok(distance) = distance_str.trim().parse::() else { - return None; - }; + let distance_str = String::from_utf8(count_output.stdout).ok()?; + let distance = distance_str.trim().parse::().ok()?; - Some((found_remote_sha, distance)) + Some((remote_sha, distance)) } // Finds the closest sha that exist on any of branches and also exists on any of the remotes. -async fn find_closest_sha(cwd: &Path, branches: &[String], remotes: &[String]) -> Option { +async fn find_closest_sha( + cwd: &Path, + branches: &[BranchCandidate], + remotes: &[String], +) -> Option { // A sha and how many commits away from HEAD it is. let mut closest_sha: Option<(GitSha, usize)> = None; for branch in branches { - let Some((maybe_remote_sha, distance)) = - branch_remote_and_distance(cwd, branch, remotes).await + let Some((remote_sha, distance)) = branch_remote_and_distance(cwd, branch, remotes).await else { continue; }; - let Some(remote_sha) = maybe_remote_sha else { - // Preserve existing behavior: skip branches that are not present on a remote. - continue; - }; match &closest_sha { None => closest_sha = Some((remote_sha, distance)), Some((_, best_distance)) if distance < *best_distance => {