On Thu, May 15, 2025 at 01:11:47PM +0000, Johannes Schindelin via GitGitGadget wrote:
> The code is a bit too hard to reason about to fully assess whether the
> `fill_commit_graph_info()` function is called at all after
> `write_commit_graph()` returns (and hence the stack variable
> `topo_levels` goes out of context).
>
> Let's simply make sure that the stack address is no longer used at that
> stage, thereby making the code quite a bit easier to reason about.
Yep, I think this is a good practice in general. If the topo_levels
member is never used outside of writing, I wonder if it could live in a
separate data structure. But that is a much bigger refactor that I don't
think we need to tackle here.
> diff --git a/commit-graph.c b/commit-graph.c
> index 9f0115dac9b5..d052c1bf15c5 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -2683,6 +2683,15 @@ cleanup:
> oid_array_clear(&ctx.oids);
> clear_topo_level_slab(&topo_levels);
>
> + if (ctx.r->objects->commit_graph) {
> + struct commit_graph *g = ctx.r->objects->commit_graph;
> +
> + while (g) {
> + g->topo_levels = NULL;
> + g = g->base_graph;
> + }
> + }
This just clears the pointers to the local variable. Looks good.
-Peff
