fix(gitplumb): use NUL-delimited cat-file --batch to survive newline paths - #18
Conversation
sagenschneider
left a comment
There was a problem hiding this comment.
Thanks for this — the diagnosis is right and the -Z protocol is implemented correctly. One blocker before merge.
-Z needs git ≥ 2.42 (Aug 2023), and on older git the failure is silent. git cat-file --batch -Z exits non-zero on the unknown option on older git (verified on debian:bookworm-slim / git 2.39: exit 129, "unknown option"). When that happens the persistent batch process dies on startup and every blob() call silently returns None — the gate stops reading file content and under-scores (or crashes on a broken-pipe write), with nothing pointing at the cause.
This is not just a Docker concern. The Action is a composite action (action.yml), so it runs on the runner's git, not the image. Exposed environments include ubuntu-22.04 runners (git 2.34), self-hosted runners on older LTS/RHEL, and standalone pip install on any older distro. (FWIW the Docker image itself is currently fine — python:3.12-slim now tracks Debian trixie / git 2.47.)
Given the bug being fixed — a literal newline in a filename — is extremely rare, silently breaking common LTS environments isn't a good trade as written.
Suggested change: feature-detect -Z once and fall back to the newline protocol when it's unavailable, so old environments keep working (they just retain the pre-existing rare newline-filename bug — no regression) while git ≥ 2.42 gets the fix. I've pushed exactly that, plus regression tests, in #26 building directly on your commit (your authorship preserved). Happy to fold it back here instead if you'd prefer to keep it on this PR.
Fixes #12
GitRepo.blob()talked to a persistentgit cat-file --batchsubprocess with newline-delimited requests/responses. A legal-but-rare filename containing a literal newline (reachable via_dequote_pathdecoding a quoted diff header) desyncs the protocol for the rest of the batch session, corrupting every subsequent read.Switches to
cat-file --batch -Z(NUL-delimited): requests are written asf"{rev}:{path}\0", and the header is read up to the next NUL instead ofreadline().Full suite has 10 pre-existing failures on a clean checkout of
main(unrelated to this change, verified in this sandboxed test environment); no new failures introduced.