fix(gix-config-value)!: parse a boolean's number like git does - #2980
fix(gix-config-value)!: parse a boolean's number like git does#2980Roshan Ramani (rawsun007) wants to merge 2 commits into
Conversation
`git_parse_maybe_bool_text()` hands the numeric fallback to `git_parse_int()`, so it takes the same bases and `k`/`m`/`g` suffixes as an integer and is bounded to a C `int`. This used a plain decimal `i64::from_str`, so `0x0`, `1k` and `-0x10` were refused while `08` and values past `INT_MAX` were accepted. Routed through `Integer`, which already matches `git` since GitoxideLabs#2967. Expectations recorded from `git config --type=bool` on git 2.50.1. Assisted-by: Claude Opus 5 (Claude Code)
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d2a4da7ec4
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "Codex (@codex) address that feedback".
| /// So `git config --type=bool` reads `0x0` as false and `1k` as true, and refuses | ||
| /// `2g` and `08`. | ||
| fn parse_as_git_int(value: &BStr) -> Option<i32> { | ||
| Integer::try_from(value).ok()?.to_decimal()?.try_into().ok() |
There was a problem hiding this comment.
On the inspected Ubuntu 24.04/Git 2.43.0 compatibility environment, git -c foo.bar=-0x80000000 config --type=bool foo.bar rejects the value, whereas Integer produces -2147483648 and this i32 conversion succeeds, making gix return true; the previous decimal-only parser rejected this hexadecimal spelling. Use the pre-2.50 lower bound (-i32::MAX) for the repository's Ubuntu-targeted semantics, or otherwise avoid newly accepting this spelling.
AGENTS.md reference: AGENTS.md:L156-L156
Useful? React with 👍 / 👎.
`git_parse_signed()` moved its lower bound from `-max` to `-max - 1` in 2.50, so `-2147483648` is refused by 2.43 and taken by 2.50. It parsed here in its decimal spelling before this branch, so it keeps parsing. Assisted-by: Claude Opus 5 (Claude Code)
|
Codex is right about the boundary and about the compatibility target, so this is a judgement call for you rather than something I want to decide quietly. From git's own /* v2.43.0 */ if ((val < 0 && -max / factor > val) || …
/* v2.50.0 */ if ((val < 0 && (-max - 1) / factor > val) || …With Where I would push back slightly is the framing of "newly accepting". The bound is on the value, not the spelling, and So I have left the bound at Posted by Claude Opus 5 running in Claude Code, through Roshan's account, per CONTRIBUTING.md. |
1 participant
Written by Claude Opus 5 in Claude Code, through Roshan's account, per the identification rule in CONTRIBUTING.md. He has not read the diff line by line yet.
Same seam as #2967, one type over.
git_parse_maybe_bool_text()falls back togit_parse_int(), so a boolean's number takes the integer bases and thek/m/gsuffixes, and is bounded to a Cintrather than to 64 bits.Booleanused a plain decimali64::from_str, which diverges in both directions.Recorded from
git -c foo.bar=<input> config --type=bool foo.baron git 2.50.1:0x00x10,0X1F,-0x101k,2m0k082147483648,4294967296,i64::MAX2gThe fix routes the fallback through
Integer, which has matchedgitsince #2967, and then narrows toi32. So the bases and suffixes come for free and the range check is the only added rule.010and017were already true and stay true, but now because they are octal rather than because a decimal parse happened to be non-zero.Marked breaking:
08and anything pastINT_MAXused to parse and now error. Happy to drop the!if you read a parser correction as non-breaking, as with #2967.cargo test -p gix-config-valueis 58 passing. Reverting onlysrc/boolean.rsfails the two new tests plusfrom_str_err, which gained the08case. No existing assertion needed changing:+10and-1still parse, sinceIntegeraccepts a sign.cargo fmtapplied; clippy reports only the pre-existing removed-lint warning the whole workspace emits.