Skip to content

fix(fetch): enumerate commit parents without requiring them to be present - #61

Merged
sanity merged 2 commits into
mainfrom
fix/60-parent-walk
Aug 4, 2026
Merged

fix(fetch): enumerate commit parents without requiring them to be present#61
sanity merged 2 commits into
mainfrom
fix/60-parent-walk

Conversation

@sanity

@sanity sanity commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Closes #60.

Problem

Cloning could abort with:

error: git rev-list --parents <sha> failed: error: Could not read <parent>
fatal: Failed to traverse parents of commit <sha>

while every bundle was present and healthy on the network — freenet-git rescue against the same URL fetched all of them without error.

Cause

walk_unresolved_parents exists to answer "which of this commit's parents do I still need?", so the fetch loop knows which bundle to download next. It answered that with git rev-list --parents -n 1, which traverses, and therefore fails with Could not read <parent> exactly when a parent is absent.

So the walk returned Err precisely when it had something useful to report. The clone aborted instead of fetching the bundle carrying that parent.

Fix

Use git cat-file commit <sha> and parse the parent headers. That reads a single object, so it is answerable whether or not the parents are local.

Two details worth flagging for review:

  • Header-only parsing. A commit object is headers, blank line, then free-form message. A message body can easily contain a line beginning with parent (a quoted log, a review note), so parsing past the blank line would invent parents out of prose. The parser stops at the blank line.
  • Tag peeling preserved. cat-file commit peels an annotated tag to its commit, matching commit_exists's <sha>^{commit}. walk_unresolved_parents_walks_through_annotated_tag_of_commit still passes.

Why no existing test caught this

Every prior test builds a synthetic orphan commit to stand in for "we only have the tip". An orphan has no parents, so git_commit_parents was never once called on a commit whose parent was missing — the missing commit short-circuits earlier, at commit_exists.

The new test copies a real commit object into a repo lacking its parent, which is the state a clone is genuinely in after installing one bundle and before fetching the one below it. On the old implementation it fails with the exact production error:

called `Result::unwrap()` on an `Err` value: git rev-list --parents 1392069c... failed:
error: Could not read e57b6341...
fatal: Failed to traverse parents of commit 1392069c...

Also added: git_commit_parents against an absent parent, and the root-commit (no parents) case, which guards a parser that might mistake a header or the message body for a parent.

Testing

  • New tests fail on the old implementation, pass on the new one. Full suite green, clippy -D warnings and fmt clean.
  • E2E, live network: the repo whose clone reproduced this now clones clean — 4/4 commits, correct content.
  • E2E regression: the 177-commit 96rknpy1GYhZ/freenet-stdlib demo, which exercises the tipped-bundle walk, clones identically to before the change (18 bundles downloaded, 5 dead-weight skipped, 190 commits).

[AI-assisted - Claude]

sanity added 2 commits August 3, 2026 22:50
…sent

Cloning a repo could abort with:

    error: git rev-list --parents <sha> failed: error: Could not read <parent>
    fatal: Failed to traverse parents of commit <sha>

even though every bundle was present and healthy on the network
(`freenet-git rescue` on the same URL fetched them all).

`walk_unresolved_parents` exists to answer "which of this commit's
parents do I still need?", so the fetch loop knows which bundle to
download next. It answered that question with `git rev-list --parents
-n 1`, which traverses — and therefore fails with `Could not read
<parent>` precisely when a parent is absent. The walk returned `Err`
exactly when it had something useful to report, so the clone aborted
instead of fetching the bundle carrying that parent.

Switched to `git cat-file commit <sha>`, which reads one object and is
answerable whether or not the parents are local. Only the header block
is parsed: a commit is headers, blank line, then free-form message, and
a message body can easily contain a line starting with "parent ", which
would otherwise be read as a parent. `cat-file commit` peels annotated
tags to their commit the same way `commit_exists` does via
`<sha>^{commit}`, so the tag-peeling behaviour is unchanged.

Why no existing test caught it: every prior test builds a *synthetic
orphan* commit to stand in for "we only have the tip". An orphan has no
parents, so `git_commit_parents` was never once called on a commit whose
parent was missing. The new test copies a real commit object into a repo
without its parent, which is the state a clone is genuinely in after
installing one bundle and before fetching the one below it. It fails on
the old implementation with the exact production error above.

Verified end to end: the repo whose clone reproduced this now clones
clean (4/4 commits, correct content), and the 177-commit freenet-stdlib
demo — which exercises the tipped-bundle walk — clones identically to
before (18 bundles, 5 dead-weight skipped, 190 commits).

Closes #60

[AI-assisted - Claude]
Review pass on the parsing change. Each of these was checked against
real `git cat-file commit` output first, then pinned:

- Merge commit: several `parent` headers, all of which must come back.
  Dropping one stops the walk chasing a whole side of the history and
  the clone silently lacks those commits.
- `parent <40-hex>` in the commit MESSAGE. Not hypothetical --
  `git commit -m` puts the body at column 0, exactly like a header. A
  parser without the blank-line break invents a parent out of prose and
  the walk hunts forever for a bundle holding an object that does not
  exist. Verified this test fails if the break is removed.
- Multi-line `gpgsig` header. The blank line inside PGP armor is stored
  as a line containing a single space, not an empty line, so it does not
  end header parsing early; continuation lines begin with a space and so
  can never match `parent `. Signed commits are common enough that this
  should not rest on reasoning alone.

Note also that parents always precede `author`/`committer`/`gpgsig` in
the object, so stopping early cannot lose one -- only stopping late can
invent one, which is what the break prevents.

[AI-assisted - Claude]
@sanity

sanity commented Aug 4, 2026

Copy link
Copy Markdown
Contributor Author

Review pass (Full tier: fetch/clone path)

Ran the lenses in-session and pushed the resulting coverage in c74b7b0.

Change-specific lens — git object format. This is now a parser for another program's output format, so I checked the real shapes against git cat-file commit before trusting any reasoning:

shape risk outcome
merge commit multiple parent headers; dropping one loses a whole side of history all returned, first-parent first — pinned
parent <sha> in the message body parsing past the blank line invents a parent from prose ignored — pinned, and verified the test fails if the break is removed
multi-line gpgsig a blank line inside PGP armor could end header parsing early armor blank line is stored as a single space, not empty; continuation lines start with a space so cannot match parent — pinned

One structural point that makes this safe in the direction that matters: parents always precede author/committer/gpgsig in a commit object, so stopping early cannot lose a parent. Only stopping late can invent one, and that is exactly what the blank-line break prevents.

Skeptical lens. The one deliberate behaviour change beyond the fix: a parent header whose sha is not 40 hex chars is now an error, where the old word-scanning loop silently skipped it. That is the sha-256 repo case, and freenet-git is sha-1 only throughout (CommitHash = [u8; 20], RefEntry.target: [u8; 20]). Previously such a repo would produce an empty parent list and a silently incomplete clone; now it fails loudly. Better, and unreachable for any repo that works today.

Testing lens. Mutation-checked rather than assumed: removing the header-only break fails git_commit_parents_ignores_parent_lines_in_the_commit_message; reverting to rev-list --parents fails both #60 regression tests with the exact production error.

Big-picture lens. Single caller (walk_unresolved_parents), whose surrounding logic is unchanged. No wire format, no contract, no signing. E2E on the live network confirms both directions: the repo that reproduced #60 now clones clean, and the 177-commit demo — which does exercise the tipped-bundle walk — clones byte-identically to before (18 bundles, 5 dead-weight skipped, 190 commits).

[AI-assisted - Claude]

@sanity
sanity merged commit 4244674 into main Aug 4, 2026
4 checks passed
sanity added a commit that referenced this pull request Aug 4, 2026
Ships the #61 fetch fix for #60: cloning a repo could abort with
"Failed to traverse parents of commit <sha>" while every bundle was
present and healthy on the network.

Unlike the workflow-only change in #57, this one is in shipped crate
code, and `cargo install freenet-git` is how users get the remote
helper -- so the fix reaches nobody until it is published.

Only the freenet-git crate changed; the sibling crates are unchanged
and stay at their published versions.

[AI-assisted - Claude]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

clone: parent walk fails on repos whose bundle chain has a gap (git rev-list --parents errors on missing parent)

1 participant