From: Tamir Duberstein Date: Fri, 12 Jun 2026 21:26:04 GMT Subject: Re: [PATCH v3 1/3] commit-reach: handle cycles in contains walk Message-ID: In-Reply-To: On Fri, Jun 12, 2026 at 2:53 AM Kristofer Karlsson wrote: > > On Fri, 12 Jun 2026 at 05:00, Tamir Duberstein 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. Yes. The test creates an ancestry cycle through replacement commit parents, so MAXREPLACEDEPTH does not catch this particular cycle. But I agree with the design conclusion: the history is malformed and the fallback only adds complexity. Done in v4. Thanks!