Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions codex-rs/core/src/git_info_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ use codex_exec_server::LOCAL_FS;
use codex_git_utils::GitInfo;
use codex_git_utils::GitSha;
use codex_git_utils::collect_git_info;
use codex_git_utils::get_git_repo_root_with_fs;
use codex_git_utils::get_has_changes;
use codex_git_utils::git_diff_to_remote;
use codex_git_utils::recent_commits;
Expand Down Expand Up @@ -526,6 +527,20 @@ async fn resolve_root_git_project_for_trust_returns_none_outside_repo() {
);
}

#[tokio::test]
async fn get_git_repo_root_with_fs_detects_gitdir_pointer() {
let tmp = TempDir::new().expect("tempdir");
let proj = tmp.path().join("proj");
let nested = proj.join("nested");
std::fs::create_dir_all(&nested).unwrap();
std::fs::write(proj.join(".git"), "gitdir: /tmp/fake-worktree\n").unwrap();

assert_eq!(
get_git_repo_root_with_fs(LOCAL_FS.as_ref(), &nested.abs()).await,
Some(proj.abs())
);
}

#[tokio::test]
async fn resolve_root_git_project_for_trust_regular_repo_returns_repo_root() {
let temp_dir = TempDir::new().expect("Failed to create temp dir");
Expand Down
14 changes: 12 additions & 2 deletions codex-rs/core/src/session/turn.rs
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ use codex_analytics::build_track_events_context;
use codex_async_utils::OrCancelExt;
use codex_features::Feature;
use codex_git_utils::get_git_repo_root;
use codex_git_utils::get_git_repo_root_with_fs;
use codex_hooks::HookEvent;
use codex_hooks::HookEventAfterAgent;
use codex_hooks::HookPayload;
Expand Down Expand Up @@ -370,8 +371,17 @@ pub(crate) async fn run_turn(
// Although from the perspective of codex.rs, TurnDiffTracker has the lifecycle of a Task which contains
// many turns, from the perspective of the user, it is a single turn.
#[allow(deprecated)]
let display_root = get_git_repo_root(turn_context.cwd.as_path())
.unwrap_or_else(|| turn_context.cwd.clone().into_path_buf());
let display_root = match turn_context.environments.primary() {
Some(turn_environment) => get_git_repo_root_with_fs(
turn_environment.environment.get_filesystem().as_ref(),
&turn_environment.cwd,
)
.await
.unwrap_or_else(|| turn_environment.cwd.clone())
.into_path_buf(),
None => get_git_repo_root(turn_context.cwd.as_path())
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need the fallback?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.primary() returns Option - so either we need a fallback or should throw

.unwrap_or_else(|| turn_context.cwd.clone().into_path_buf()),
};
let turn_diff_tracker = Arc::new(tokio::sync::Mutex::new(TurnDiffTracker::with_display_root(
display_root,
)));
Expand Down
24 changes: 19 additions & 5 deletions codex-rs/git-utils/src/info.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,23 @@ pub fn get_git_repo_root(base_dir: &Path) -> Option<PathBuf> {
find_ancestor_git_entry(base).map(|(repo_root, _)| repo_root)
}

/// Return the repository root for `cwd` using the provided filesystem.
///
/// This mirrors [`get_git_repo_root`] for local paths, but works when `cwd`
/// only exists inside a selected remote environment.
pub async fn get_git_repo_root_with_fs(
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we convert the original and pass LOCAL_FS where needed?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we would need to convert existing callers to be async which blows this up a bit. im just fixing tests here, can do the LOCAL_FS conversion in a separate PR

fs: &dyn ExecutorFileSystem,
cwd: &AbsolutePathBuf,
) -> Option<AbsolutePathBuf> {
let base = match fs.get_metadata(cwd, /*sandbox*/ None).await {
Ok(metadata) if metadata.is_directory => cwd.clone(),
_ => cwd.parent()?,
};
find_ancestor_git_entry_with_fs(fs, &base)
.await
.map(|(repo_root, _)| repo_root)
}

/// Timeout for git commands to prevent freezing on large repositories
const GIT_COMMAND_TIMEOUT: TokioDuration = TokioDuration::from_secs(5);
const DISABLED_HOOKS_PATH: &str = if cfg!(windows) { "NUL" } else { "/dev/null" };
Expand Down Expand Up @@ -729,11 +746,8 @@ pub async fn resolve_root_git_project_for_trust(
fs: &dyn ExecutorFileSystem,
cwd: &AbsolutePathBuf,
) -> Option<AbsolutePathBuf> {
let base = match fs.get_metadata(cwd, /*sandbox*/ None).await {
Ok(metadata) if metadata.is_directory => cwd.clone(),
_ => cwd.parent()?,
};
let (repo_root, dot_git) = find_ancestor_git_entry_with_fs(fs, &base).await?;
let repo_root = get_git_repo_root_with_fs(fs, cwd).await?;
let dot_git = repo_root.join(".git");
if fs
.get_metadata(&dot_git, /*sandbox*/ None)
.await
Expand Down
1 change: 1 addition & 0 deletions codex-rs/git-utils/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ pub use info::default_branch_name;
pub use info::get_git_remote_urls;
pub use info::get_git_remote_urls_assume_git_repo;
pub use info::get_git_repo_root;
pub use info::get_git_repo_root_with_fs;
pub use info::get_has_changes;
pub use info::get_head_commit_hash;
pub use info::git_diff_to_remote;
Expand Down
Loading