Re: [PATCH 5/8] commit-graph: read `BIDX` chunk with `pair_chunk_expect()`

[Junio C Hamano <gitster@xxxxxxxxx>]

Taylor Blau <me@xxxxxxxxxxxx> writes:

> -static int graph_read_bloom_index(const unsigned char *chunk_start,
> -				  size_t chunk_size, void *data)
> -{
> -	struct commit_graph *g = data;
> -	if (chunk_size != g->num_commits * 4) {
> -		warning("commit-graph changed-path index chunk is too small");
> -		return -1;
> -	}
> ...
> @@ -461,8 +449,10 @@ struct commit_graph *parse_commit_graph(struct repo_settings *s,
>  	}
>  
>  	if (s->commit_graph_read_changed_paths) {
> +		if (pair_chunk_expect(cf, GRAPH_CHUNKID_BLOOMINDEXES,
> +				      &graph->chunk_bloom_indexes,
> +				      st_mult(graph->num_commits, 4)) == -1)
> +			warning(_("commit-graph changed-path index chunk is too small (%d)"), graph->num_commits * 4);
>  		read_chunk(cf, GRAPH_CHUNKID_BLOOMDATA,
>  			   graph_read_bloom_data, graph);
>  	}

Overall the series looked sane, but the need for each caller to
supply error messages, when the helper perfectly well knows how many
bytes the caller expected and how many bytes there are avaiable, was
a bit disturbing, as the level of detail given per each caller will
inevitably become uneven.  Even now, some give an error() while
others give a warning(), even though I suspect all of them should be
data errors.

I wonder if it makes sense to stuff the message template in the
pair_chunk_data structure and do

static int pair_chunk_expect_fn(const unsigned char *chunk_start,
				size_t chunk_size,
				void *data)
{
	struct pair_chunk_data *pcd = data;
	if (pcd->expected_size != chunk_size)
		return error(_(pcd->message), pcd->expected_size, chunk_size);
	*pcd->p = chunk_start;
	return 0;
}