Re: [PATCH 03/11] commit-graph: avoid malloc'ing a local variable
- From
Jeff King <peff@peff.net>
- Date
- May 15, 2025, 19:54 UTC
- Message-ID
- <20250515195428.GD3320240@coredump.intra.peff.net>
- In-Reply-To
- <5a3a8880a68f8c69c2af39d2e32ebb56eb5fa483.1747314709.git.gitgitgadget@gmail.com>
On Thu, May 15, 2025 at 01:11:41PM +0000, Johannes Schindelin via GitGitGadget wrote:
Show 5 quoted lines
> From: Johannes Schindelin <johannes.schindelin@gmx.de> > > We do need a context to write the commit graph, but that context is only > needed during the life time of `commit_graph_write()`, therefore it can > easily be a stack variable.
Yay. I am in favor of using stack variables when possible as a general rule.
Show 37 quoted lines
> diff --git a/commit-graph.c b/commit-graph.c
> index 6394752b0b08..9f0115dac9b5 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -2509,7 +2509,17 @@ int write_commit_graph(struct object_directory *odb,
> const struct commit_graph_opts *opts)
> {
> struct repository *r = the_repository;
> - struct write_commit_graph_context *ctx;
> + struct write_commit_graph_context ctx = {
> + .r = r,
> + .odb = odb,
> + .append = flags & COMMIT_GRAPH_WRITE_APPEND ? 1 : 0,
> + .report_progress = flags & COMMIT_GRAPH_WRITE_PROGRESS ? 1 : 0,
> + .split = flags & COMMIT_GRAPH_WRITE_SPLIT ? 1 : 0,
> + .opts = opts,
> + .total_bloom_filter_data_size = 0,
> + .write_generation_data = (get_configured_generation_version(r) == 2),
> + .num_generation_data_overflows = 0,
> + };
> uint32_t i;
> int res = 0;
> int replace = 0;
> @@ -2531,17 +2541,6 @@ int write_commit_graph(struct object_directory *odb,
> return 0;
> }
>
> - CALLOC_ARRAY(ctx, 1);
> - ctx->r = r;
> - ctx->odb = odb;
> - ctx->append = flags & COMMIT_GRAPH_WRITE_APPEND ? 1 : 0;
> - ctx->report_progress = flags & COMMIT_GRAPH_WRITE_PROGRESS ? 1 : 0;
> - ctx->split = flags & COMMIT_GRAPH_WRITE_SPLIT ? 1 : 0;
> - ctx->opts = opts;
> - ctx->total_bloom_filter_data_size = 0;
> - ctx->write_generation_data = (get_configured_generation_version(r) == 2);
> - ctx->num_generation_data_overflows = 0;OK, this moves the initialization to the top of the function. So to review this for correctness, we must make sure that we do not change the values of any of those variables between the two spots (i.e., in the diff context that is omitted).
Most of it looks fine. Our call to get_configured_generation_version() now happens earlier, before the call to prepare_repo_settings(). I think that is OK, because the former calls repo_config_get_int() directly. It does seem like a potential maintenance problem if that call is ever rolled into prepare_repo_settings().
So maybe OK, but the smaller change would be to just replace the calloc with a memset(), and s/->/./ on the subsequent lines.
-Peff