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@xxxxxxxxx>
>>
>> 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@xxxxxxxxx>
>> Signed-off-by: Derrick Stolee <dstolee@xxxxxxxxxxxxx>
>> ---
>> 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
