diff mbox series

[3/3] builtin/repack.c: ensure that `names` is sorted

Message ID a3d25b9e5dda9a999597115933bf0ce70dce51b2.1653073280.git.me@ttaylorr.com (mailing list archive)
State Accepted
Commit 66731ff921a56cf3e7049fae614fcf7eec663bde
Headers show
Series repack: handle --keep-pack, --max-pack-size for geometric repacks | expand

Commit Message

Taylor Blau May 20, 2022, 7:01 p.m. UTC
The previous patch demonstrates a scenario where the list of packs
written by `pack-objects` (and stored in the `names` string_list) is
out-of-order, and can thus cause us to delete packs we shouldn't.

This patch resolves that bug by ensuring that `names` is sorted in all
cases, not just when

    delete_redundant && pack_everything & ALL_INTO_ONE

is true.

Because we did sort `names` in that case (which, prior to `--geometric`
repacks, was the only time we would actually delete packs, this is only
a bug for `--geometric` repacks.

It would be sufficient to only sort `names` when `delete_redundant` is
set to a non-zero value. But sorting a small list of strings is cheap,
and it is defensive against future calls to `string_list_has_string()`
on this list.

Co-discovered-by: Victoria Dye <vdye@github.com>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
 builtin/repack.c            | 3 ++-
 t/t7703-repack-geometric.sh | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)
diff mbox series


diff --git a/builtin/repack.c b/builtin/repack.c
index ea56e92ad5..0e4aae80c0 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -856,6 +856,8 @@  int cmd_repack(int argc, const char **argv, const char *prefix)
 	if (!names.nr && !po_args.quiet)
 		printf_ln(_("Nothing new to pack."));
+	string_list_sort(&names);
 	for_each_string_list_item(item, &names) {
 		item->util = (void *)(uintptr_t)populate_pack_exts(item->string);
@@ -896,7 +898,6 @@  int cmd_repack(int argc, const char **argv, const char *prefix)
 	if (delete_redundant && pack_everything & ALL_INTO_ONE) {
 		const int hexsz = the_hash_algo->hexsz;
-		string_list_sort(&names);
 		for_each_string_list_item(item, &existing_nonkept_packs) {
 			char *sha1;
 			size_t len = strlen(item->string);
diff --git a/t/t7703-repack-geometric.sh b/t/t7703-repack-geometric.sh
index 2cd1de7295..da87f8b2d8 100755
--- a/t/t7703-repack-geometric.sh
+++ b/t/t7703-repack-geometric.sh
@@ -231,7 +231,7 @@  test_expect_success '--geometric chooses largest MIDX preferred pack' '
-test_expect_failure '--geometric with pack.packSizeLimit' '
+test_expect_success '--geometric with pack.packSizeLimit' '
 	git init pack-rewrite &&
 	test_when_finished "rm -fr pack-rewrite" &&