Re: [PATCH v3 1/3] commit-reach: handle cycles in contains walk

From: Kristofer Karlsson <krka@spotify.com>
Date: Jun 12, 2026, 06:53 UTC
Message-ID: <CAL71e4PRqN9iPCzvgwC1Vtj-kzn4Udv+v1LTFSUXtGnC5KGrpA@mail.gmail.com>
In-Reply-To: <20260611-ref-filter-memoized-contains-v3-1-b26af3dba285@gmail.com>

On Fri, 12 Jun 2026 at 05:00, Tamir Duberstein <tamird@gmail.com> wrote:

Show 6 quoted lines

>
> 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