Skip to content

fix(gitengine): thread context through history walking - #25

Merged
hammadmajid merged 1 commit into
fix/16-signal-force-exitfrom
fix/13-gitengine-context
Sep 5, 2026
Merged

hammadmajid merged 1 commit into
fix/16-signal-force-exitfrom
fix/13-gitengine-context

Conversation

@hammadmajid

@hammadmajid hammadmajid commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Description

The entire history-walk phase was deaf to cancellation. On a large repository this is the dominant phase, so Ctrl-C did nothing at all.

Closes #13

Type of Change

  • Bug fix (non-breaking change fixing an issue)
  • New feature (non-breaking change adding functionality)
  • Performance improvement
  • Refactoring or code cleanup
  • Documentation update
  • CI/CD or build workflow change

Key Changes

HistoryWalker had no ctx field and Walk took no context. collectOrderedCommits BFS-walked the whole commit DAG, doing a packfile seek plus zlib inflate per commit, before a single occurrence reached the caller — and the caller's only cancellation check lived inside the per-occurrence callback, which was never invoked until that phase completed. With --all, every ref is seeded. traverseExclude was a second unbounded uncancellable walk, reached for every exclude in an A..B rev-range, so v1.0..HEAD walked the entire ancestry of v1.0.

Even during tree traversal the callback was the only checkpoint, and long stretches did real I/O while emitting nothing: diffTreesAndEmit prunes identical subtrees, emitBlobOccurrence's merge check calls readCommit and FindTreeEntry (which loops ReadObject per path component) for every extra parent of every merge commit, and traverseTreeHelper recurses over full trees.

Threaded context.Context through Walk, collectOrderedCommits, traverseExclude (which gains an error return), walkCommitBlobs, diffTreesAndEmit, emitBlobOccurrence, TraverseTree, traverseTreeHelper, and FindTreeEntry.

Guards are plain ctx.Err() reads, never select: these are non-blocking hot paths and an atomic load is free next to the ReadObject it guards. They sit where they gate I/O — head of for pq.Len() > 0, head of traverseExclude's stack loop, head of Walk's per-commit loop, entry of diffTreesAndEmit before the tree read pair, head of the extra-parent loop in emitBlobOccurrence, and the two tree loops — and deliberately not inside the two-pointer entry comparison, where the check would dominate the work.

The call site in runContext becomes walker.Walk(ctx, ...) and the now-redundant in-callback check is removed.

Verification & Testing

  • Ran go test -v -count=1 ./...
  • Ran go test -race -shuffle=on -count=1 ./...
  • Ran go vet ./...
  • Added or updated unit/integration tests
  • Tested manually against sample Git repository histories
go build ./...                                             clean
go vet ./internal/gitengine/ ./cmd/... ./test/...          clean
go test -race -shuffle=on -count=1 ./internal/gitengine/   ok 1.22s

New tests assert work actually performed, via a counting ObjectReader — not just the returned error:

  • TestHistoryWalkerCancelDuringCommitCollection — 400-commit history, cancel on read 20; asserts context.Canceled, zero occurrences emitted, and ≤ 25 of 400 reads. This is the phase that was completely deaf.
  • TestHistoryWalkerCancelMidWalk — measures an uncancelled baseline first, then cancels from the callback while returning nil, so the walker itself must observe it; asserts ≤ 5 reads after cancel and total < baseline.
  • TestTraverseExcludeCancellation — the exclude side of an A..B range aborts instead of draining the ancestry.
  • TestTraverseTreeCancellation — tree traversal stops at the entry that cancelled.

Mutation: stripping all four guards fails all four tests. Stripping only the collectOrderedCommits guard fails TestHistoryWalkerCancelDuringCommitCollection with 400 reads (cancelled at 20) out of 400 commits — proving that assertion defends that specific phase rather than just the returned error.

Smoke: built ./cmd/grg and ran grg --all 'HistoryWalker' against a real repository; returns hits end to end.

Note

Stacked on #24. Together with #24 this is what makes Ctrl-C actually work: this PR makes the first signal effective, #24 makes the second one an escape hatch.

Checklist

  • gofmt clean
  • Every new test mutation-checked: reverting the fix makes it fail
  • This layer builds and passes the full race suite on its own, not just at the top of the stack

The entire history-walk phase was uncancellable, making it the primary
Ctrl-C dead zone. `collectOrderedCommits` BFS-walked the whole commit
DAG -- a packfile seek plus zlib inflate per commit -- before a single
occurrence reached the caller, and `cmd/grg/main.go` checked `ctx.Err()`
only inside the per-occurrence callback, which was never invoked until
that phase completed. With `--all` every ref is seeded, so on a large
repository SIGINT had literally no effect. `traverseExclude` was a second
unbounded uncancellable walk, reached once per exclude of an `A..B`
rev-range. Even during tree traversal the callback was the sole
cancellation point while long stretches did real I/O and emitted nothing:
subtree-OID pruning, the merge-commit `FindTreeEntry` probe per extra
parent, and full-tree recursion in `traverseTreeHelper`.

`HistoryWalker.Walk` now takes a leading `context.Context` and threads it
through `collectOrderedCommits`, `traverseExclude` (which gains an error
return), `walkCommitBlobs`, `diffTreesAndEmit`, and
`emitBlobOccurrence`, plus `TraverseTree`/`traverseTreeHelper` and
`FindTreeEntry` in tree.go. Guards are plain `ctx.Err()` loads rather
than `select`, placed at loop heads where they gate the `ReadObject`
that follows and never inside the innermost per-entry comparison, so
they cost nothing next to the I/O they guard.

The redundant `ctx.Err()` check inside the callback in `runContext` is
gone; the surrounding `errors.Is(err, context.Canceled)` handling is
unchanged. Signature call sites in existing tests were updated
mechanically.

New tests assert on work actually performed via a counting
`ObjectReader`: cancellation during commit collection stops within a few
reads of the trip point and emits nothing (the phase that was completely
deaf), mid-walk cancellation stops after the emitting commit and reads
far fewer objects than a full drain, and the exclude walk and tree
traversal both abort instead of draining. Removing only the
`collectOrderedCommits` guard makes the collection test fail on read
count.

Closes #13
@hammadmajid
hammadmajid force-pushed the fix/13-gitengine-context branch from 6b26a6c to 1d9862e Compare September 5, 2026 14:16
@hammadmajid
hammadmajid marked this pull request as ready for review September 5, 2026 14:19
@hammadmajid
hammadmajid merged commit 22a2c9f into main Sep 5, 2026
3 of 6 checks passed
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.

gitengine: history walking accepts no context, so Ctrl-C is ignored for the whole walk phase

1 participant