Re: [PATCH 6/9] commit-graph: use fanout value for graph size

From: Jeff King <peff@peff.net>
Date: Nov 9, 2023, 21:38 UTC
Message-ID: <20231109213813.GB2728242@coredump.intra.peff.net>
In-Reply-To: <ZU1NNcP/ARxK6D89@nand.local>

On Thu, Nov 09, 2023 at 04:20:53PM -0500, Taylor Blau wrote:

Show 16 quoted lines

> On Thu, Nov 09, 2023 at 02:24:35AM -0500, Jeff King wrote:
> > @@ -323,7 +320,8 @@ static int graph_read_oid_lookup(const unsigned char *chunk_start,
> >  {
> >  	struct commit_graph *g = data;
> >  	g->chunk_oid_lookup = chunk_start;
> > -	g->num_commits = chunk_size / g->hash_len;
> > +	if (chunk_size / g->hash_len != g->num_commits)
> > +		return error(_("commit-graph OID lookup chunk is the wrong size"));
> >  	return 0;
> >  }
> 
> My understanding is that you need this error message to come from
> graph_read_oid_lookup() in order to pass the "detect incorrect fanout
> final value" test, but I wish that we didn't have to, since having the
> more-or-less duplicate error messages in the latter "reader notices
> fanout/lookup table mismatch" is somewhat unfortunate.

I'm not sure what you mean here. We notice the problem in the reading code, which is used in turn by the verify code. So both of these tests:

Show 17 quoted lines

> >  test_expect_success 'detect incorrect fanout final value' '
> >  	corrupt_graph_and_verify $GRAPH_BYTE_FANOUT2 "\01" \
> > -		"oid table and fanout disagree on size"
> > +		"OID lookup chunk is the wrong size"
> >  '
> >
> >  test_expect_success 'detect incorrect OID order' '
> > @@ -850,7 +850,8 @@ test_expect_success 'reader notices too-small oid fanout chunk' '
> >  test_expect_success 'reader notices fanout/lookup table mismatch' '
> >  	check_corrupt_chunk OIDF 1020 "FFFFFFFF" &&
> >  	cat >expect.err <<-\EOF &&
> > -	error: commit-graph oid table and fanout disagree on size
> > +	error: commit-graph OID lookup chunk is the wrong size
> > +	error: commit-graph required OID lookup chunk missing or corrupted
> >  	EOF
> >  	test_cmp expect.err err
> >  '

will see a message regardless of where we put it. Or by "duplicate" did you mean the two-line:

  > > +   error: commit-graph OID lookup chunk is the wrong size
  > > +   error: commit-graph required OID lookup chunk missing or corrupted

output. That's because our chunk of the return value from read_chunk() is a bit lazy, and does not distinguish "missing chunk" (where we should write a string saying so) from errors produced by the callback (where a more specific error message has already been written). That's true of all of the read_chunk() callers, including the midx ones.

This is one of the ways that pair_chunk_expect() could do better without adding a lot of code, because it can check the return value from read_chunk(). It doesn't help the other cases (like OIDF) that still have to call read_chunk() themselves, though. Possibly read_chunk() should just take a flag to indicate that it should complain when the chunk is missing. And then callers could just do:

  if (read_chunk(cf, id, our_callback, CHUNK_REQUIRED)
	return -1; /* no need to complain; either our_callback() did, or
	              read_chunk() itself */

-Peff