Re: [PATCH] repack: respect --keep-pack with geometric repack

From: Taylor Blau <me@ttaylorr.com>
Date: May 20, 2022, 19:05 UTC
Message-ID: <YofmhcBLrni0kY/s@nand.local>
In-Reply-To: <bb19159b-6d0c-a683-a58e-95ebdc128627@github.com>

On Fri, May 20, 2022 at 10:27:21AM -0700, Victoria Dye wrote:

Show 10 quoted lines

> > I left a couple of small notes on the patch below, but since I have some
> > patches that deal with a separate issue in the `git repack --geometric`
> > code coming, do you want to combine forces (and I can send a
> > lightly-reworked version of this patch as a part of my series)?
>
> Works for me! I'm happy with all the suggested changes you noted below
> (moving the 'string_list_sort' and cleaning up the test), so feel free to
> include them in your series.
>
> Thanks!

No problem; I (re-)sent this patch as 1/3 in my series which should be available via the archive at:

    https://lore.kernel.org/git/cover.1653073280.git.me@ttaylorr.com/T/#t

It looks like we're on the same page with respect to my suggestions, but feel free to take a look at the reworked version of this patch (and the new ones on top, too) and let me know what you think.

Show 25 quoted lines

> >> @@ -332,17 +332,34 @@ static int geometry_cmp(const void *va, const void *vb)
> >>  	return 0;
> >>  }
> >>
> >> -static void init_pack_geometry(struct pack_geometry **geometry_p)
> >> +static void init_pack_geometry(struct pack_geometry **geometry_p,
> >> +			       struct string_list *existing_kept_packs)
> >>  {
> >>  	struct packed_git *p;
> >>  	struct pack_geometry *geometry;
> >> +	struct strbuf buf = STRBUF_INIT;
> >>
> >>  	*geometry_p = xcalloc(1, sizeof(struct pack_geometry));
> >>  	geometry = *geometry_p;
> >>
> >> +	string_list_sort(existing_kept_packs);
> >
> > Would it be worth sorting this as early as in collect_pack_filenames()?
> > For our purposes in this patch, this works as-is, but it may be
> > defensive to try and minimize the time that list has unsorted contents.
>
> I went back and forth on this, eventually settling on this to keep the
> 'string_list_sort' as close as possible to where the sorted list is needed.
> I'm still pretty indifferent, though, so moving it to the end of
> 'collect_pack_filenames()' is fine with me.

I'm fine with it either way. I opted to sorting the list in collect_pack_filenames() since I think it's slightly more fool-proof, but I also don't have strong feelings about its placement.

Thanks, Taylor