Re: [PATCH v2 07/11] commit-graph: check chunk sizes after writing

[Derrick Stolee <stolee@gmail.com>]

On 6/25/2020 3:25 AM, René Scharfe wrote:
> Am 23.06.20 um 19:47 schrieb SZEDER Gábor via GitGitGadget:
>> From: =?UTF-8?q?SZEDER=20G=C3=A1bor?= <szeder.dev@gmail.com>
>>
>> In my experience while experimenting with new commit-graph chunks,
>> early versions of the corresponding new write_commit_graph_my_chunk()
>> functions are, sadly but not surprisingly, often buggy, and write more
>> or less data than they are supposed to, especially if the chunk size
>> is not directly proportional to the number of commits.  This then
>> causes all kinds of issues when reading such a bogus commit-graph
>> file, raising the question of whether the writing or the reading part
>> happens to be buggy this time.
>>
>> Let's catch such issues early, already when writing the commit-graph
>> file, and check that each write_graph_chunk_*() function wrote the
>> amount of data that it was expected to, and what has been encoded in
>> the Chunk Lookup table.  Now that all commit-graph chunks are written
>> in a loop we can do this check in a single place for all chunks, and
>> any chunks added in the future will get checked as well.
>>
>> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
>> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
>> ---
>>  commit-graph.c | 9 +++++++++
>>  1 file changed, 9 insertions(+)
>>
>> diff --git a/commit-graph.c b/commit-graph.c
>> index 086fc2d070..1de6800d74 100644
>> --- a/commit-graph.c
>> +++ b/commit-graph.c
>> @@ -1683,12 +1683,21 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)
>>  			num_chunks * ctx->commits.nr);
>>  	}
>>
>> +	chunk_offset = f->total + f->offset;
>>  	for (i = 0; i < num_chunks; i++) {
>> +		uint64_t end_offset;
>> +
> 
> Hmm, the added code looks complicated because it keeps state outside the
> loop, but it could be replace by this:
> 
> 		uint64_t start_offset = f->total + f->offset;
> 
>>  		if (chunks[i].write_fn(f, ctx)) {
>>  			error(_("failed writing chunk with id %"PRIx32""),
>>  			      chunks[i].id);
>>  			return -1;
>>  		}
>> +
>> +		end_offset = f->total + f->offset;
>> +		if (end_offset - chunk_offset != chunks[i].size)
>> +			BUG("expected to write %"PRId64" bytes to chunk %"PRIx32", but wrote %"PRId64" instead",
>> +			    chunks[i].size, chunks[i].id, end_offset - chunk_offset);
>> +		chunk_offset = end_offset;
> 
> ... and that:
> 
> 		if (f->total + f->offset != start_offset + chunks[i].size)
> 			BUG(...);

Thanks! I agree this approach is simpler and less prone to
bugs since we are using the local state.

-Stolee