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

quoted

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 (local)

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.

quoted

quoted

@@ -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