From: Patrick Steinhardt <ps@pks.im>
Date: Fri, 11 Sep 2026 12:23:31 GMT
Subject: Re: [PATCH v2 09/10] builtin/fsck: move multi-pack index verification into the packed source
Message-ID: <aqPyw2mHJ9kt-xna@pks.im>
In-Reply-To: <877bksnior.fsf@emacs.iotcl.com>

On Fri, Sep 11, 2026 at 01:14:44PM +0200, Toon Claes wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> > diff --git a/odb/source-packed.c b/odb/source-packed.c
> > index 2b5dc502f5..9f42552377 100644
> > --- a/odb/source-packed.c
> > +++ b/odb/source-packed.c
> > @@ -14,6 +14,7 @@
> >  #include "packfile.h"
> >  #include "pack-bitmap.h"
> >  #include "progress.h"
> > +#include "run-command.h"
> >  
> >  static int find_pack_entry(struct odb_source_packed *store,
> >  			   const struct object_id *oid,
> > @@ -897,6 +898,29 @@ static int verify_reverse_indices(struct odb_source_packed *source,
> >  	return res;
> >  }
> >  
> > +static int verify_midx(struct odb_source_packed *source,
> > +		       struct odb_fsck_options *opts)
> > +{
> > +	struct child_process midx_verify = CHILD_PROCESS_INIT;
> > +	int ret = 0;
> 
> I don't see much reason to use a `ret` value instead of using early
> returns instead.

Fair enough.

> > +
> > +	if (!source->base.odb->repo->settings.core_multi_pack_index)
> 
> Because we cannot ensure where this function was called from, shall we
> BUG() if (!settings.initialized)?

Good point, but I think it's preferable to call
`prepare_repo_settings()` instead.

> > @@ -912,6 +936,9 @@ static int odb_source_packed_fsck(struct odb_source *source,
> >  	if (verify_bitmap_files(packed))
> >  		ret = -1;
> >  
> > +	if (verify_midx(packed, opts) < 0)
> 
> Any reason why you're checking negative value here and not in the if
> above?

Not specifically, and in theory both could check for `< 0`. But I
refrained from doing so when moving around `verify_bitmap_file()`
because in the preimage we didn't check for a negative value, either,
and it would have thus caused more questions.

So I think I'd leave this part as-is.

Patrick