diff --git a/codex-rs/git-utils/src/git_config.rs b/codex-rs/git-utils/src/git_config.rs index 236ab89ef7..92496132a1 100644 --- a/codex-rs/git-utils/src/git_config.rs +++ b/codex-rs/git-utils/src/git_config.rs @@ -1,6 +1,71 @@ use std::path::Component; use std::path::Path; +pub(crate) fn parse_git_boolean(value: &[u8]) -> Option { + if value.eq_ignore_ascii_case(b"true") + || value.eq_ignore_ascii_case(b"yes") + || value.eq_ignore_ascii_case(b"on") + { + return Some(true); + } + if value.is_empty() + || value.eq_ignore_ascii_case(b"false") + || value.eq_ignore_ascii_case(b"no") + || value.eq_ignore_ascii_case(b"off") + { + return Some(false); + } + + // Git parses the remaining boolean spellings through `git_parse_int`: C + // base-0 syntax, an optional binary-unit suffix, and signed `int` bounds. + let value = std::str::from_utf8(value) + .ok()? + .trim_start_matches(|value: char| value.is_ascii_whitespace()); + let (negative, unsigned) = match value.as_bytes().first() { + Some(b'-') => (true, &value[1..]), + Some(b'+') => (false, &value[1..]), + Some(_) => (false, value), + None => return None, + }; + let (base, unsigned) = if unsigned.starts_with("0x") || unsigned.starts_with("0X") { + (16, &unsigned[2..]) + } else if unsigned.starts_with('0') { + (8, unsigned) + } else { + (10, unsigned) + }; + let digit_count = unsigned + .bytes() + .take_while(|byte| match base { + 8 => matches!(byte, b'0'..=b'7'), + 10 => byte.is_ascii_digit(), + 16 => byte.is_ascii_hexdigit(), + _ => false, + }) + .count(); + if digit_count == 0 { + return None; + } + let (digits, suffix) = unsigned.split_at(digit_count); + let factor = if suffix.is_empty() { + 1_i128 + } else if suffix.eq_ignore_ascii_case("k") { + 1024 + } else if suffix.eq_ignore_ascii_case("m") { + 1024 * 1024 + } else if suffix.eq_ignore_ascii_case("g") { + 1024 * 1024 * 1024 + } else { + return None; + }; + let magnitude = i128::from_str_radix(digits, base).ok()?; + let signed = if negative { -magnitude } else { magnitude }; + let value = signed.checked_mul(factor)?; + (i128::from(i32::MIN)..=i128::from(i32::MAX)) + .contains(&value) + .then_some(value != 0) +} + pub(crate) fn path_is_within(path: &Path, root: &Path) -> bool { let mut path_components = path.components(); for root_component in root.components() { diff --git a/codex-rs/git-utils/src/git_config_tests.rs b/codex-rs/git-utils/src/git_config_tests.rs index 3795caf2f8..825d9a597f 100644 --- a/codex-rs/git-utils/src/git_config_tests.rs +++ b/codex-rs/git-utils/src/git_config_tests.rs @@ -1,5 +1,69 @@ use super::*; +#[test] +fn git_boolean_accepts_native_git_spellings() { + for (value, expected) in [ + (b"true".as_slice(), true), + (b"TRUE".as_slice(), true), + (b"yes".as_slice(), true), + (b"On".as_slice(), true), + (b"1".as_slice(), true), + (b"2".as_slice(), true), + (b"-1".as_slice(), true), + (b"+1".as_slice(), true), + (b"01".as_slice(), true), + (b"0x1".as_slice(), true), + (b"0Xf".as_slice(), true), + (b"010".as_slice(), true), + (b"1k".as_slice(), true), + (b"1M".as_slice(), true), + (b"1g".as_slice(), true), + (b" 1".as_slice(), true), + (b"2147483647".as_slice(), true), + (b"-2147483648".as_slice(), true), + (b"-2g".as_slice(), true), + (b"false".as_slice(), false), + (b"FALSE".as_slice(), false), + (b"no".as_slice(), false), + (b"Off".as_slice(), false), + (b"".as_slice(), false), + (b"0".as_slice(), false), + (b"-0".as_slice(), false), + (b"+0".as_slice(), false), + (b"00".as_slice(), false), + (b"0x0".as_slice(), false), + (b"0k".as_slice(), false), + ] { + assert_eq!(parse_git_boolean(value), Some(expected), "value {value:?}"); + } +} + +#[test] +fn git_boolean_rejects_values_native_git_rejects() { + for value in [ + b"08".as_slice(), + b"-08".as_slice(), + b"2147483648".as_slice(), + b"-2147483649".as_slice(), + b"9223372036854775807".as_slice(), + b"2g".as_slice(), + b"-3g".as_slice(), + b"0x".as_slice(), + b"0b1".as_slice(), + b"1kb".as_slice(), + b"1foo".as_slice(), + b"1_0".as_slice(), + b"1 ".as_slice(), + b" ".as_slice(), + b"+".as_slice(), + b"-".as_slice(), + b"not-a-bool".as_slice(), + b"\xff".as_slice(), + ] { + assert_eq!(parse_git_boolean(value), None, "value {value:?}"); + } +} + #[test] fn path_containment_uses_component_boundaries() { let root = Path::new("/repo/root"); diff --git a/codex-rs/git-utils/src/repository_authority/plain_config.rs b/codex-rs/git-utils/src/repository_authority/plain_config.rs index b3a4253b77..1464ac9095 100644 --- a/codex-rs/git-utils/src/repository_authority/plain_config.rs +++ b/codex-rs/git-utils/src/repository_authority/plain_config.rs @@ -4,6 +4,7 @@ use std::path::PathBuf; use super::bytes_to_path; use super::read_bounded_file; +use crate::git_config::parse_git_boolean; #[derive(Debug, Eq, PartialEq)] pub(crate) enum CommonConfigAuthority { @@ -50,7 +51,7 @@ pub(crate) fn inspect_plain_common_config_authority( return Ok(CommonConfigAuthority::Unproven); } let bare = match unique_explicit_value(&config, "core", "bare") { - Ok(Some(value)) => match explicit_boolean(value.as_ref()) { + Ok(Some(value)) => match parse_git_boolean(value.as_ref()) { Some(value) => Some(value), None => return Ok(CommonConfigAuthority::Unproven), }, @@ -58,7 +59,7 @@ pub(crate) fn inspect_plain_common_config_authority( Err(()) => return Ok(CommonConfigAuthority::Unproven), }; match unique_explicit_value(&config, "extensions", "worktreeConfig") { - Ok(Some(value)) if explicit_boolean(value.as_ref()) == Some(false) => {} + Ok(Some(value)) if parse_git_boolean(value.as_ref()) == Some(false) => {} Ok(None) => {} Ok(Some(_)) | Err(()) => return Ok(CommonConfigAuthority::Unproven), } @@ -99,39 +100,29 @@ fn unique_explicit_value( let Some(sections) = config.sections_by_name(section_name) else { return Ok(None); }; - let mut occurrences = 0usize; - let mut explicit_values = Vec::new(); + let mut occurrence = None; for section in sections { if section.header().subsection_name().is_some() { return Err(()); } - occurrences += section - .value_names() - .filter(|name| { - let name: &str = name.as_ref(); - name.eq_ignore_ascii_case(value_name) - }) - .count(); - explicit_values.extend( - section - .values(value_name) - .into_iter() - .map(|value| value.as_ref().to_vec()), - ); + for name in section.value_names() { + let name: &str = name.as_ref(); + if !name.eq_ignore_ascii_case(value_name) { + continue; + } + if occurrence.is_some() { + return Err(()); + } + let value = section + .value_implicit(name) + .ok_or(())? + .map(|value| value.as_ref().to_vec()); + occurrence = Some(value); + } } - match (occurrences, explicit_values.len()) { - (0, 0) => Ok(None), - (1, 1) => Ok(explicit_values.pop()), - _ => Err(()), - } -} - -fn explicit_boolean(value: &[u8]) -> Option { - if value.eq_ignore_ascii_case(b"true") { - Some(true) - } else if value.eq_ignore_ascii_case(b"false") { - Some(false) - } else { - None + match occurrence { + None => Ok(None), + Some(Some(value)) => Ok(Some(value)), + Some(None) => Err(()), } } diff --git a/codex-rs/git-utils/src/repository_authority_tests.rs b/codex-rs/git-utils/src/repository_authority_tests.rs index 0447fa0d82..9f07340939 100644 --- a/codex-rs/git-utils/src/repository_authority_tests.rs +++ b/codex-rs/git-utils/src/repository_authority_tests.rs @@ -81,6 +81,36 @@ fn plain_common_bare_parser_matches_native_boolean_syntax_and_is_stricter_on_dup CommonConfigAuthority::Bare, Some(true), ), + ( + "yes", + b"[core]\n\tbare = yes\n".as_slice(), + CommonConfigAuthority::Bare, + Some(true), + ), + ( + "on", + b"[core]\n\tbare = on\n".as_slice(), + CommonConfigAuthority::Bare, + Some(true), + ), + ( + "numeric true", + b"[core]\n\tbare = 1\n".as_slice(), + CommonConfigAuthority::Bare, + Some(true), + ), + ( + "hex true", + b"[core]\n\tbare = 0x1\n".as_slice(), + CommonConfigAuthority::Bare, + Some(true), + ), + ( + "suffixed true", + b"[core]\n\tbare = 1k\n".as_slice(), + CommonConfigAuthority::Bare, + Some(true), + ), ( "false", b"[core]\n\tbare = false\n".as_slice(), @@ -133,13 +163,13 @@ fn plain_common_bare_parser_matches_native_boolean_syntax_and_is_stricter_on_dup } #[test] -fn common_bare_proof_accepts_only_explicit_boolean_literals() { +fn common_bare_proof_rejects_invalid_git_boolean_values() { for (name, body) in [ - ("empty", b"[core]\n\tbare =\n".as_slice()), - ("yes", b"[core]\n\tbare = yes\n".as_slice()), - ("on", b"[core]\n\tbare = on\n".as_slice()), - ("one", b"[core]\n\tbare = 1\n".as_slice()), ("leading zero", b"[core]\n\tbare = 08\n".as_slice()), + ( + "negative leading zero", + b"[core]\n\tbare = -08\n".as_slice(), + ), ( "positive overflow", b"[core]\n\tbare = 2147483648\n".as_slice(), @@ -152,6 +182,8 @@ fn common_bare_proof_accepts_only_explicit_boolean_literals() { "i64 max", b"[core]\n\tbare = 9223372036854775807\n".as_slice(), ), + ("suffix overflow", b"[core]\n\tbare = 2g\n".as_slice()), + ("trailing junk", b"[core]\n\tbare = 1foo\n".as_slice()), ] { let common = write_common_config(body); assert_eq!( @@ -162,6 +194,34 @@ fn common_bare_proof_accepts_only_explicit_boolean_literals() { } } +#[test] +fn common_bare_proof_accepts_explicit_false_worktree_config_spellings() { + for value in ["false", "no", "off", "", "0", "-0", "+0", "00", "0x0"] { + let common = write_common_config( + format!("[core]\n\tbare = yes\n[extensions]\n\tworktreeConfig = {value}\n").as_bytes(), + ); + assert_eq!( + inspect_plain_common_config_authority(common.path()).expect(value), + CommonConfigAuthority::Bare, + "authority result for worktreeConfig={value:?}" + ); + } +} + +#[test] +fn common_bare_proof_rejects_explicit_true_worktree_config_spellings() { + for value in ["true", "yes", "on", "1", "-1", "0x1", "1k"] { + let common = write_common_config( + format!("[core]\n\tbare = yes\n[extensions]\n\tworktreeConfig = {value}\n").as_bytes(), + ); + assert_eq!( + inspect_plain_common_config_authority(common.path()).expect(value), + CommonConfigAuthority::Unproven, + "authority result for worktreeConfig={value:?}" + ); + } +} + #[test] fn common_bare_proof_rejects_includes_worktree_config_and_relative_worktree() { let include_target = tempfile::NamedTempFile::new().expect("included config"); @@ -226,6 +286,25 @@ fn common_config_absolute_worktree_is_returned_as_authority() { ); } +#[test] +fn common_config_false_bare_spellings_allow_absolute_worktree() { + let worktree = tempfile::tempdir().expect("worktree"); + for bare in ["false", "no", "off", "", "0", "-0", "+0", "00", "0x0"] { + let common = write_common_config( + format!( + "[core]\n\tbare = {bare}\n\tworktree = {}\n", + git_config_path(worktree.path()) + ) + .as_bytes(), + ); + assert_eq!( + inspect_plain_common_config_authority(common.path()).expect(bare), + CommonConfigAuthority::Worktree(worktree.path().to_path_buf()), + "authority result for core.bare={bare:?}" + ); + } +} + #[test] fn common_config_rejects_contradictory_or_malformed_bare_with_absolute_worktree() { let worktree = tempfile::tempdir().expect("worktree");