Re: [PATCH 0/8] chunk-format: introduce `pair_chunk_expect()` convenience API

From: Jeff King <peff@peff.net>
Date: Oct 20, 2023, 10:23 UTC
Message-ID: <20231020102307.GB2673857@coredump.intra.peff.net>
In-Reply-To: <cover.1697225110.git.me@ttaylorr.com>

On Fri, Oct 13, 2023 at 03:25:12PM -0400, Taylor Blau wrote:

Show 10 quoted lines

> While reviewing this series, I noticed a couple of spots that would be
> helped by having a convenience function that stores the start of a
> chunk in a designated location *after* checking that the chunk has the
> expected size.
> 
> I called this function `pair_chunk_expect()` and touched up seven spots
> that I think could benefit from having a convenience function like this.
> 
> This applies directly on top of 'jk/chunk-bounds'. Thanks in advance for
> your review!

I'm still a little skeptical of this approach, just because I think it it discourages adding further checks. And in particular, I was planning to add monotonicity checks to the midx OIDF chunk based on the discussion in the earlier thread. And likewise, I think I probably should have put the commit-graph checks into its OIDF reader, rather than saving them for verify_commit_graph_lite(). That would match the way we check the validity of the other chunks.

I haven't actually started writing those patches, though, so I'm not sure of the full details yet.

-Peff