On Thu, Aug 28, 2025 at 01:45:36PM -0700, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
>
> > From: Derrick Stolee <stolee@xxxxxxxxx>
> >
> > This instance of setting the result to 1 before going to cleanup was
> > accidentally removed in fcb2205b77 (midx: implement support for writing
> > incremental MIDX chains, 2024-08-06).
> >
> > Signed-off-by: Derrick Stolee <stolee@xxxxxxxxx>
> > ---
> > midx-write.c | 1 +
> > 1 file changed, 1 insertion(+)
>
> The cover letter made it sound as if [1/5] was the only fix and the
> rest was clean-up, but unless all callers of write_midx_internal()
> ignores the return value from it, this surely would change the
> behaviour of the program, no?
I think the cover letter is saying that only the first patch is required
to fix the crash that was reported, and the rest are clean-ups.
Not that this isn't a bug in its own right, but it's definitely distinct
from the one that is addressed in the previous patch.
> And the results from write-midx_file_only(), write_midx_file(), and
> expire_midx_packs(), the three callers of this _internal() function,
> all seem to be used in builtin/multi-pack-index.c so wouldn't this
> also be a fix?
>
> Not that I endorse "0 is success and any non-zero value is an error",
> but this does not look like a mere clean-up to me.
>
> > diff --git a/midx-write.c b/midx-write.c
> > index d8f9679868..85b2d471ef 100644
> > --- a/midx-write.c
> > +++ b/midx-write.c
> > @@ -1106,6 +1106,7 @@ static int write_midx_internal(struct repository *r, const char *object_dir,
> > m = m->base_midx;
> > }
> > } else if (ctx.m && fill_packs_from_midx(&ctx)) {
> > + result = 1;
> > goto cleanup;
It might be nice to have a test that confirms this behavior (though the
changes look obviously correct to me). It should suffice to write a
MIDX, drop one of its packs, and then try to write it again with a new
pack.
Thanks,
Taylor
