Re: [PATCH 5/5] commit-graph: prepare commit graph

From: Taylor Blau <ttaylorr@github.com>
Date: Feb 1, 2021, 18:25 UTC
Message-ID: <YBhHaW7JpiYeNQIp@nand.local>
In-Reply-To: <dddeec30ebfc6d8403cc10395a9d0c331d1f7ad3.1612199707.git.gitgitgadget@gmail.com>

On Mon, Feb 01, 2021 at 05:15:07PM +0000, Derrick Stolee via GitGitGadget wrote:

Show 9 quoted lines

> From: Derrick Stolee <dstolee@microsoft.com>
>
> Before checking if the repository has a commit-graph loaded, be sure
> to run prepare_commit_graph(). This is necessary because without this
> instance we would not initialize the topo_levels slab for each of the
> struct commit_graphs in the chain before we start to parse the
> commits. This leads to possibly recomputing the topological levels for
> commits in lower layers even when we are adding a small number of
> commits on top.

I think that the situation arises as follows:

  - Prior to this patch, we didn't always prepare the commit-graph, so
    the topo_levels slab wasn't initialized either.

  - Then we try and compute the topo levels for new commits (which are
    likely to be decendants of older ones that are in commit-graphs and
    have their topo-levels already computed).

  - But in the course of computing topo-levels for the new commits, we
    have to recur on their ancestors, which *look* like they have
    uncomputed topo levels.

That all makes sense, but it may be worth cutting and pasting what I wrote into your patch to make it clearer.

> By properly initializing the topo_slab, we fix the previously broken
> case of a split commit graph where a base layer has the
> generation_data_overflow chunk.

Makes sense.

Show 5 quoted lines

> -test_expect_failure 'lower layers have overflow chunk' '
> +test_expect_success 'lower layers have overflow chunk' '
>  	cd "$TRASH_DIRECTORY/full" &&
>  	UNIX_EPOCH_ZERO="@0 +0000" &&
>  	FUTURE_DATE="@2147483646 +0000" &&

Terrific :-).

Thanks, Taylor