Re: [PATCH v3 1/3] commit-reach: handle cycles in contains walk
From: Kristofer Karlsson <hidden>
Date: 2026-06-12 06:53:53
On Fri, 12 Jun 2026 at 05:00, Tamir Duberstein [off-list ref] wrote:
The memoized contains traversal used by git tag assumes that commit ancestry is acyclic. Replacement refs can violate that assumption, causing it to keep pushing an already active commit until memory is exhausted.
The cycle detection itself makes sense, but would it be simpler to just die() when a cycle is found rather than falling back to a second reachability walk? A cycle in the commit graph means replacement refs are misconfigured. The existing code already loops forever when it hits one, so detecting and dying is strictly an improvement. The fallback adds a second codepath through the function, discards all cached results (so later candidates redo work), and papers over what is really a broken invariant. do_lookup_replace_object() already dies when replacement refs chain deeper than MAXREPLACEDEPTH (which covers cycles), so the existing contract treats this as a fatal configuration error. parse_commit_or_die() sets the same precedent within the walk itself. Kristofer