Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
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
1 change: 1 addition & 0 deletions crates/git-cache-api/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ pub fn app_result(config: AppConfig) -> CoreResult<Router> {
pub async fn app_result_async(config: AppConfig) -> CoreResult<Router> {
let git_remote_enabled = config.git_remote.enabled;
let state = Arc::new(ApiState::try_new_async(config).await?);
state.domain.git.ensure_supported_binary_version().await?;
router(git_remote_enabled, state)
}
Comment on lines 59 to 82

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🚩 Version check only applies to the async initialization path

The ensure_supported_binary_version() call is added only to app_result_async (crates/git-cache-api/src/lib.rs:51), which is the production entry point used in main.rs:16. The sync app_result and app functions—used by all integration tests (e.g. tests/correctness.rs:61, tests/git_protocol.rs:62)—do not perform the check. This is likely intentional: tests run with whatever git the CI provides and the sync path can't easily call async code. However, app_result is a public API, so any external consumer using the sync path would silently skip the version gate. Worth confirming this is the desired contract.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Intentional: the gate targets the server startup path (mainapp_result_async) so a misbuilt runtime image fails at boot rather than with request-time 400s. The sync app_result/app constructors stay check-free on purpose — they're used by tests that must run against whatever git the host provides, and a blocking subprocess call doesn't fit the sync path. Callers on the sync path can still invoke Git::ensure_supported_binary_version() themselves if they want the gate.


Expand Down
88 changes: 88 additions & 0 deletions crates/git-cache-git/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,30 @@ use tracing::{debug, info};

pub const DEFAULT_OUTPUT_LIMIT: usize = 4 * 1024 * 1024;

/// Minimum supported `(major, minor)` version of the `git` binary.
/// `GIT_NO_LAZY_FETCH` landed in git 2.45; older binaries ignore it and die
/// with `fatal: could not fetch ... from promisor remote` instead of
/// reporting missing promisor objects as missing.
pub const MIN_GIT_VERSION: (u32, u32) = (2, 45);

/// Parse `git version` output (e.g. `"git version 2.47.3"`, possibly with a
/// platform suffix) into `(major, minor, patch)`.
pub fn parse_git_version(text: &str) -> Option<(u32, u32, u32)> {
let version = text.trim().strip_prefix("git version")?.trim();
let version = version.split_whitespace().next()?;
let mut parts = version.split('.');
let major = parts.next()?.parse().ok()?;
let minor = parts.next()?.parse().ok()?;
let patch = parts
.next()
.map(|part| {
let digits: String = part.chars().take_while(char::is_ascii_digit).collect();
digits.parse().unwrap_or(0)
})
.unwrap_or(0);
Some((major, minor, patch))
}

#[derive(Debug, Clone)]
pub struct Git {
binary: PathBuf,
Expand Down Expand Up @@ -899,6 +923,34 @@ impl Git {
.await
}

/// Report the version of the configured `git` binary as
/// `(major, minor, patch)`.
pub async fn binary_version(&self) -> Result<(u32, u32, u32)> {
let output = self.run(None, ["version"]).await?;
let text = output.stdout_utf8("version")?;
parse_git_version(&text).ok_or_else(|| {
GitCacheError::Internal(format!("could not parse git version from {text:?}"))
})
}

/// Fail fast when the configured `git` binary is older than
/// [`MIN_GIT_VERSION`]. Older binaries silently ignore
/// `GIT_NO_LAZY_FETCH` and attempt lazy promisor fetches for missing
/// objects, which surface as fatal errors on blobless-clone read paths
/// instead of "missing" object reports.
pub async fn ensure_supported_binary_version(&self) -> Result<()> {
let (major, minor, patch) = self.binary_version().await?;
let (min_major, min_minor) = MIN_GIT_VERSION;
if (major, minor) < (min_major, min_minor) {
return Err(GitCacheError::Internal(format!(
"git binary {major}.{minor}.{patch} is too old: \
{min_major}.{min_minor} or newer is required for reliable \
GIT_NO_LAZY_FETCH support"
)));
}
Ok(())
}

pub async fn run_no_lazy<I, S>(&self, cwd: Option<&Path>, args: I) -> Result<GitOutput>
where
I: IntoIterator<Item = S>,
Expand Down Expand Up @@ -1260,6 +1312,42 @@ mod tests {
use super::*;
use std::time::{SystemTime, UNIX_EPOCH};

// ── parse_git_version tests ─────────────────────────────────────

#[test]
fn parse_git_version_plain() {
assert_eq!(parse_git_version("git version 2.47.3\n"), Some((2, 47, 3)));
assert_eq!(parse_git_version("git version 2.39.5"), Some((2, 39, 5)));
}

#[test]
fn parse_git_version_with_platform_suffix() {
assert_eq!(
parse_git_version("git version 2.39.5 (Apple Git-154)\n"),
Some((2, 39, 5))
);
assert_eq!(
parse_git_version("git version 2.47.0.windows.1"),
Some((2, 47, 0))
);
assert_eq!(
parse_git_version("git version 2.46.0-rc1"),
Some((2, 46, 0))
);
}

#[test]
fn parse_git_version_missing_patch() {
assert_eq!(parse_git_version("git version 2.45"), Some((2, 45, 0)));
}

#[test]
fn parse_git_version_rejects_garbage() {
assert_eq!(parse_git_version(""), None);
assert_eq!(parse_git_version("not git"), None);
assert_eq!(parse_git_version("git version two.three"), None);
}

// ── reject_ref_arg tests ────────────────────────────────────────

#[test]
Expand Down
Loading