| Message ID | 20231003202752.GD7812@coredump.intra.peff.net (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | ac6d45d11f24921ead899c74569b717a7895f4a5 |
| Headers | show |
| Series | some commit-graph leak fixes | expand |
On Tue, Oct 03, 2023 at 04:27:52PM -0400, Jeff King wrote: > When closing and freeing a commit-graph, the main entry point is > close_commit_graph(), which then uses close_commit_graph_one() to > recurse through the base_graph links and free each one. > > Commit 957ba814bf (commit-graph: when closing the graph, also release > the slab, 2021-09-08) put the call to clear the slab into the recursive > function, but this is pointless: there's only a single global slab > variable. It works OK in practice because clearing the slab is > idempotent, but it makes the code harder to reason about and refactor. Well reasoned and explained, this change makes perfect sense to me. Thanks, Taylor
diff --git a/commit-graph.c b/commit-graph.c index 5e8a3a5085..dc54ef4776 100644 --- a/commit-graph.c +++ b/commit-graph.c @@ -728,13 +728,13 @@ static void close_commit_graph_one(struct commit_graph *g) if (!g) return; - clear_commit_graph_data_slab(&commit_graph_data_slab); close_commit_graph_one(g->base_graph); free_commit_graph(g); } void close_commit_graph(struct raw_object_store *o) { + clear_commit_graph_data_slab(&commit_graph_data_slab); close_commit_graph_one(o->commit_graph); o->commit_graph = NULL; }
When closing and freeing a commit-graph, the main entry point is close_commit_graph(), which then uses close_commit_graph_one() to recurse through the base_graph links and free each one. Commit 957ba814bf (commit-graph: when closing the graph, also release the slab, 2021-09-08) put the call to clear the slab into the recursive function, but this is pointless: there's only a single global slab variable. It works OK in practice because clearing the slab is idempotent, but it makes the code harder to reason about and refactor. Move it into the parent function so it's only called once (and there are no other direct callers of the recursive close_commit_graph_one(), so we are not hurting them). Signed-off-by: Jeff King <peff@peff.net> --- commit-graph.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)