picatz / picatz/flowstate

git.log: a commit past the parent bound fails the call even when `since` excludes it

Open
#717 2 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

kind/bug status/blocked
Dominant language
Go
Stars
9
Forks
0
Avg merge
3h 3m
Merged PRs (30d)
509

Description

Raised by Codex on #681, against the parent bound that PR moved into the traversal. The move is right — checked any later, the parents are already on the walk's stack, which is the allocation the bound exists to refuse — but it lands the check ahead of a filter that could have made it unnecessary.

The interaction

walkPage layers iterators: multiRootCommitIter traverses, newPathFilteringCommitIter may wrap it, and object.NewCommitLimitIterFromIter applies since on the outside. since is therefore evaluated after the traversal has already popped a commit and expanded its parents.

So for a repository with an octopus merge of more than maxLogParents (64) parents, a git.log call fails with InvalidInput even when the caller's since window ends well after that commit — and the remedy the diagnostic offers ("narrow the walk with since or path") is exactly the thing that cannot help, because the cutoff never gets to speak. The recent, in-range commits the caller asked for are not returned at all.

Rare, but not hypothetical: octopus merges past 64 parents exist in real history.

Why it was not fixed on the PR

The fix is to give the traversal the cutoff, so an out-of-range commit stops the walk before the parent expansion is reached — which means multiRootCommitIter growing a notion of since (and, for the same reason, of the path filter) rather than those staying layers above it. That is a change to how the three iterators compose, not a line inside the bound, and #681 is a bounds PR.

The current behaviour is at least the fail-closed direction: it refuses rather than silently omitting commits, and #681's other half makes sure it refuses as an error rather than as a cursor that can never make progress.

Shape of a fix

Push the since cutoff (and ideally the path predicate) into multiRootCommitIter so the traversal can decline to expand a commit it is not going to emit anyway, leaving the parent bound to fire only for commits genuinely in range.

Codex thread: https://github.com/picatz/flowstate/pull/681#discussion_r3790548627

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start by reading multiRootCommitIter and how it is layered with newPathFilteringCommitIter and object.NewCommitLimitIterFromIter in walkPage. Reproduce an octopus merge with more than 64 parents using a git.log call with since, then verify that out-of-range commits do not trigger the parent bound while in-range commits still do.

Written by the indexing model from the issue text.

Assessment

Tech stack
git, go
Domain
backend
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.