Re: [PATCH v6 2/3] revision: add peek functions for lookahead

From: Kristofer Karlsson <krka@spotify.com>
Date: Jun 22, 2026, 08:28 UTC
Message-ID: <CAL71e4OQ_kGb+UwHgikHG236-8BVtc7P9OdpV4i4UzYRCoPczw@mail.gmail.com>
In-Reply-To: <20260620-ps-pre-commit-indent-v6-2-cdc6d8fd5fbc@gmail.com>

Show 8 quoted lines

> On Sat, 21 Jun 2026, Pablo Sabater <pabloosabaterr@gmail.com> wrote:
> The graph code in a subsequent commit needs to be able to look ahead in
> order to set indentation-related flags.
>
> Using revs->commits is brittle and the data structure that holds the
> pending commits might change in the future.
>
> Add two functions that abstract this for the graph.

The abstraction is a step in the right direction, but I think there is a deeper issue with the peek-based approach. I tried to understand the problem and ended up with an alternative that I think is simpler and also fixes the three test_expect_failure cases in t4218.

> +struct commit *revision_peek_next_commit (struct rev_info *revs)

Show 6 quoted lines

> +int revision_has_commits_after (struct rev_info *revs, int n)
> +{
> +               for (size_t i = 0; i < info->topo_queue.nr && visible < n; i++) {
> +                       struct commit *c = info->topo_queue.array[i].data;
> +                       if (get_commit_action(revs, c) == commit_show)
> +                               visible++;

Scanning the pending queue does not work, because it may not contain all relevant entries yet. Processing the first entry in the queue may affect the second entry.

There is also a second problem: commits in the queue have not been through simplify_commit() yet, so their parent lists are still the raw ones. graph_is_visual_root_candidate() checks "parents == NULL", but with a pathspec filter a commit's TREESAME parent might get removed by simplification, turning the commit into a visual root. Peeking at the raw queue misses this, which is the cause of the t4218 test_expect_failure cases.

The solution is to skip peeking entirely and instead call get_revision_internal() to populate a small lookahead buffer - it only needs two slots.

    struct git_graph {
        // ...
        struct commit *lookahead[2];
        int lookahead_nr;
    }

    while (revs->graph->lookahead_nr < 2) {
        struct commit *next = get_revision_internal(revs);
        if (!next)
            break;
        graph_push_lookahead(revs->graph, next);
    }

After prototyping this locally, the three test_expect_failure cases in t4218 went away (though I had to do some minor tweaks to ensure it become fully deterministic by ticking the commit timestamps.

One subtlety worth mentioning: get_revision_internal() sets SHOWN on commits, so lookahead commits are marked SHOWN before graph_update() processes them. This makes graph_is_interesting() think they are already displayed. The fix is a small check in graph_is_interesting() that recognizes commits in the lookahead buffer as interesting regardless of their SHOWN flag.

    for (i = 0; i < graph->lookahead_nr; i++)
        if (graph->lookahead[i] == commit)
            return 1;
    // other checks after this ...

This approach ultimately removes the need for revision_peek_next_commit() and revision_has_commits_after() entirely - the graph code no longer needs to peek at rev_info internals.

Kristofer