On Thu, Nov 09, 2023 at 05:34:11PM -0500, Taylor Blau wrote:
> +static int pair_chunk_expect_fn(const unsigned char *chunk_start,
> + size_t chunk_size,
> + void *data)
> +{
> + struct pair_chunk_data *pcd = data;
> + if (chunk_size / pcd->record_size != pcd->record_nr)
> + return -1;
> + *pcd->p = chunk_start;
> + return 0;
> +}
I think this is taking us backwards in terms of the output the user sees
(see the message I just sent elsewhere in the thread). The user won't be
able to tell the difference between a missing chunk and one with the
wrong size.
And we miss the opportunity to give more details about the expected and
detected sizes of the chunks.
If the two-line error message really bothers you, then...
> +int pair_chunk_expect(struct chunkfile *cf,
> + uint32_t chunk_id,
> + const unsigned char **p,
> + size_t record_size,
> + size_t record_nr)
> +{
> + struct pair_chunk_data pcd = {
> + .p = p,
> + .record_size = record_size,
> + .record_nr = record_nr,
> + };
> + return read_chunk(cf, chunk_id, pair_chunk_expect_fn, &pcd);
> +}
...this is where we could take a CHUNK_REQUIRED flag, and so:
ret = read_chunk(...);
/* pair_chunk_expect_fn() wrote an error already for other cases */
if (ret == CHUNK_MISSING)
error("chunk not found");
return ret;
And then the callers become a very nice:
if (pair_chunk_expect(cf, id, &ptr, size, nr, CHUNK_REQUIRED))
return -1;
You probably would need to pass some kind of string giving more context
for the error messages, though. We can show the chunk id, but the
context of "commit-graph" vs "midx" is important.
-Peff
