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

Message

Taylor Blau Oct. 13, 2023, 7:25 p.m. UTC

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!

Taylor Blau (8):
  chunk-format: introduce `pair_chunk_expect()` helper
  commit-graph: read `OIDF` chunk with `pair_chunk_expect()`
  commit-graph: read `CDAT` chunk with `pair_chunk_expect()`
  commit-graph: read `GDAT` chunk with `pair_chunk_expect()`
  commit-graph: read `BIDX` chunk with `pair_chunk_expect()`
  midx: read `OIDF` chunk with `pair_chunk_expect()`
  midx: read `OIDL` chunk with `pair_chunk_expect()`
  midx: read `OOFF` chunk with `pair_chunk_expect()`

 chunk-format.c | 22 +++++++++++++++++
 chunk-format.h | 12 +++++++++-
 commit-graph.c | 65 +++++++++++++-------------------------------------
 midx.c         | 58 ++++++++++++--------------------------------
 4 files changed, 65 insertions(+), 92 deletions(-)

Comments

Jeff King Oct. 20, 2023, 10:23 a.m. UTC | #1

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

> 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