Re: [PATCH 03/17] commit-graph: use chunk-format write API

From: Taylor Blau <me@ttaylorr.com>
Date: Jan 27, 2021, 02:47 UTC
Message-ID: <YBDUOGLkiQ7ocl+D@nand.local>
In-Reply-To: <a3d6177a352643721fdc07512629b48d1213157a.1611676886.git.gitgitgadget@gmail.com>

On Tue, Jan 26, 2021 at 04:01:12PM +0000, Derrick Stolee via GitGitGadget wrote:

Show 8 quoted lines

> From: Derrick Stolee <dstolee@microsoft.com>
>
> The commit-graph write logic is ready to make use of the chunk-format
> write API. Each chunk write method is already in the correct prototype.
> We only need to use the 'struct chunkfile' pointer and the correct API
> calls.
>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>

Nicely done. The majority of this patch was remarkably easy to read, which I attribute to you doing the necessary prep work to make the callbacks usable by the new API. Thank you.

Show 5 quoted lines

> @@ -1941,6 +1896,7 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)
>
>  	close_commit_graph(ctx->r->objects);
>  	finalize_hashfile(f, file_hash.hash, CSUM_HASH_IN_STREAM | CSUM_FSYNC);
> +	free_chunkfile(cf);

Since chunkfiles are so tightly coupled to hashfiles (i.e., you can only "construct" a chunkfile given a 'struct hashfile*'), I wonder whether this should be:

    finalize_chunkfile(cf, ...)

instead. It seems kind of weird to give up ownership of 'f' down to the chunkfile API only to reach down into it again.

I could even buy that you'd always want to finalize and free a chunkfile at the same time, and so perhaps the calls could be combined, but that may be a step too far.

Thanks, Taylor