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

[Junio C Hamano <gitster@pobox.com>]

Taylor Blau <me@ttaylorr.com> writes:

> +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 know one of the original places did the "divide the whole by
per-record size and see if it matches the number of records", the
same as we see above, but the check in the above could also be

	if (chunk_size != st_mult(pcd->record_size, pcd->record_nr))
		return -1;

which would also catch the case where chunk_size is not a multiple
of the record size.  Your conversion of OOFF in midx.c loses this
protection as the original uses the multiplication-and-compare, but
the rewrite to call pair_chunk_expect would call the above and
checks with the truncating-divide-and-compare.

Does the distinction matter?  I dunno.  If the record/chunk
alignment is asserted elsewhere, then the distinction should not
matter, but even if it were, seeing a truncating division used in
any validation makes my skin tingle.

Other than that, the series was a pleasant read.

Thanks.