Re: [PATCH 1/7] chunk-format: introduce `pair_chunk_expect()` helper

From: Jeff King <peff@peff.net>
Date: Nov 10, 2023, 22:08 UTC
Message-ID: <20231110220828.GI2758295@coredump.intra.peff.net>
In-Reply-To: <af5fe3b7237caeba8f970e967933db96c83a230e.1699569246.git.me@ttaylorr.com>

On Thu, Nov 09, 2023 at 05:34:11PM -0500, Taylor Blau wrote:

Show 10 quoted lines

> +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...

Show 13 quoted lines

> +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