Re: [PATCH v2 1/8] midx: expose `write_midx_file_only()` publicly

From: Taylor Blau <me@ttaylorr.com>
To: Jonathan Tan <jonathantanmy@google.com>
Cc: me@ttaylorr.com, git@vger.kernel.org, peff@peff.net,
	gitster@pobox.com, avarab@gmail.com
Subject: Re: [PATCH v2 1/8] midx: expose `write_midx_file_only()` publicly
Date: Wed, 22 Sep 2021 23:09:48 -0400	[thread overview]
Message-ID: <YUvv/HG3jzOwtwzZ@nand.local> (raw)
In-Reply-To: <20210922231423.2610917-1-jonathantanmy@google.com>

On Wed, Sep 22, 2021 at 04:14:23PM -0700, Jonathan Tan wrote:
> > @@ -1237,7 +1253,7 @@ static int write_midx_internal(const char *object_dir,
> >
> >  	QSORT(ctx.info, ctx.nr, pack_info_compare);
> >
> > -	if (packs_to_drop && packs_to_drop->nr) {
> > +	if (ctx.m && packs_to_drop && packs_to_drop->nr) {
> >  		int drop_index = 0;
> >  		int missing_drops = 0;
> >
>
> I couldn't figure out why this requires ctx.m now.

Me either; this must have been a stray change that got dragged along. I
dropped in -- thanks for pointing it out.

> > @@ -62,6 +63,10 @@ int midx_contains_pack(struct multi_pack_index *m, const char *idx_or_pack_name)
> >  int prepare_multi_pack_index_one(struct repository *r, const char *object_dir, int local);
> >
> >  int write_midx_file(const char *object_dir, const char *preferred_pack_name, unsigned flags);
> > +int write_midx_file_only(const char *object_dir,
> > +			 struct string_list *packs_to_include,
> > +			 const char *preferred_pack_name,
> > +			 unsigned flags);
>
> It took me a while to figure out that this function doesn't only write a
> MIDX file, but writes an MIDX file only for certain packs. Maybe worth
> adding a comment here (e.g. "Write an MIDX file only for the given
> packs").

Nitpicking a little, it does still write a (single) MIDX file, but that
MIDX only includes the packs listed in packs_to_include. I can add a
comment to that effect.

Thanks,
Taylor