[codex] add write-side core operations#43
Conversation
Refs #21 Co-authored-by: Codex <codex@openai.com>
hartsock
left a comment
There was a problem hiding this comment.
Reviewer: Drake (drake-interactive, Claude Opus 4.7 / 1M context), on behalf of @hartsock.
Solid parts
- Per-method file layout (
pull.rs,push.rs,add.rs,commit.rs) matches issue #21. - Tests use the existing
fixturesmodule pattern fromfetch.rs. - "nothing to commit" → success contract is honored.
- New shared
run_githelper carries the same env-stripping shape asfetch.rs(GIT_DIR,GIT_WORK_TREE,GIT_INDEX_FILEremoved).
Issues to address before un-drafting
1. run_git introduced but fetch.rs doesn't adopt it
Two patterns now coexist: fetch.rs has its own inline Command::new("git")... returning (ok, stderr); the new write ops use super::run_git returning (ok, stderr, stdout). Drift risk: future hardening (e.g., adding GIT_TERMINAL_PROMPT=0 to disable credential prompts) would need to touch both. Either include the refactor here (~20 lines, mechanical) or open a follow-up issue and link it.
I'd push for the refactor in this PR — the codebase is already adopting the helper for everything new, so leaving fetch.rs as the outlier creates exactly the kind of subtle inconsistency that bites later.
2. commit's "nothing to commit" detection is locale-fragile
The match output.contains("nothing to commit") is case-folded (good) but English-only. Under LANG=fr_FR.UTF-8, git prints "rien à valider" and the heuristic misses. Set LC_ALL=C once, inside run_git, to stabilize stderr parsing for every future caller:
let out = Command::new("git")
.arg("-C")
.arg(path)
.args(args)
.env("LC_ALL", "C")
.env_remove("GIT_DIR")
.env_remove("GIT_WORK_TREE")
.env_remove("GIT_INDEX_FILE")
.output();One line, covers everything. (And folds neatly into the refactor in §1.)
3. Signature mismatch with issue #21 spec
Issue #21 says Result<(bool, String)> but this PR returns (bool, String). The PR matches the codebase pattern (fetch_result is (bool, String), not Result<...>), so the issue spec looks like a documentation error. Worth a one-line acknowledgement in the PR description so a reviewer doesn't get tripped up against the literal issue text.
4. commit_nothing_to_commit_counts_as_success test is weak
It asserts ok but doesn't verify that no commit was created. A "nothing to commit" code path that silently created a commit would still pass. Strengthen with a HEAD-unchanged invariant:
#[test]
fn commit_nothing_to_commit_counts_as_success() {
let repo = fixtures::repo();
let before = fixtures::git(repo.path(), &["rev-list", "--count", "HEAD"]);
let (ok, stderr) = super::commit(repo.path(), "noop");
let after = fixtures::git(repo.path(), &["rev-list", "--count", "HEAD"]);
assert!(ok, "{stderr}");
assert_eq!(before, after, "no commit should have been created");
}5. Test isolation asymmetry — comment, not a blocker
fixtures::git neutralizes host config (GIT_CONFIG_GLOBAL=/dev/null, sets identity); super::run_git does NOT — by design, for production (per fetch.rs's docstring). Net: tests exercising run_git still pick up the host's ~/.gitconfig. Tests pass on a vanilla host today; could break on a host with init.defaultBranch = master, core.hooksPath set, or commit.gpgsign = true.
Not a blocker — but a // NOTE: these tests assume a vanilla host git config near the test module(s) would save a future debugger an hour.
Recommended pre-merge checklist
- Refactor
fetch.rsontorun_githere, or open a follow-up issue and link it. - Add
.env("LC_ALL", "C")inrun_gitto stabilize "nothing to commit" detection. - Strengthen the
commit_nothing_to_committest with a HEAD-unchanged assertion (snippet above). - Note in PR body that signatures match the
fetch_resultpattern, not the literal issue spec.
Note on cross-PR ordering
This PR's Rust additions aren't yet callable from Python — that needs a follow-up PyO3-binding PR. PR #42's PUBLIC_EXTENSION_NAMES lists pull, push, etc. as aspirational entries that will become real after the binding lands. No action needed here, just flagging the dependency so the order of merges is clear: write-side Rust (this PR) → bindings → write-side Python tests.
Co-authored-by: Codex <codex@openai.com>
Co-authored-by: Codex <codex@openai.com>
Co-authored-by: Codex <codex@openai.com>
What changed
Why
This starts the M2 write-side Rust core while keeping Python exposure deferred to #23.
Note: these Rust core functions intentionally follow the existing fetch_result pattern of returning (bool, String) for success plus stderr/reason text, rather than the literal issue-spec spelling for every method signature.
Closes #21
Validation