From: Jiang Xin <zhiyou.jx@xxxxxxxxxxxxxxx>
SZEDER reported that test case t5323 has different test result on MacOS.
This is because `cmp_pack_list_reverse` cannot give identical result
when two pack being sorted has the same size of remaining_objects.
Changes to the sorting function will make consistent test result for
t5323.
The new algorithm to find redundant packs is a trade-off to save memory
resources, and the result of it may be different with old one, and may
be not the best result sometimes. Update t5323 for the new algorithm.
Reported-by: SZEDER Gábor <szeder.dev@xxxxxxxxx>
Signed-off-by: Jiang Xin <zhiyou.jx@xxxxxxxxxxxxxxx>
Signed-off-by: Junio C Hamano <gitster@xxxxxxxxx>
---
builtin/pack-redundant.c | 24 ++++++++++++++++--------
t/t5323-pack-redundant.sh | 18 +++++++++---------
2 files changed, 25 insertions(+), 17 deletions(-)
diff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c
index 15cdf233c4..29ff5e99cb 100644
--- a/builtin/pack-redundant.c
+++ b/builtin/pack-redundant.c
@@ -33,6 +33,7 @@ static struct pack_list {
struct packed_git *pack;
struct llist *unique_objects;
struct llist *remaining_objects;
+ size_t all_objects_size;
} *local_packs = NULL, *altodb_packs = NULL;
static struct llist_item *free_nodes;
@@ -340,19 +341,25 @@ static inline off_t pack_set_bytecount(struct pack_list *pl)
return ret;
}
-static int cmp_pack_list_reverse(const void *a, const void *b)
+static int cmp_remaining_objects(const void *a, const void *b)
{
struct pack_list *pl_a = *((struct pack_list **)a);
struct pack_list *pl_b = *((struct pack_list **)b);
- size_t sz_a = pl_a->remaining_objects->size;
- size_t sz_b = pl_b->remaining_objects->size;
- if (sz_a == sz_b)
- return 0;
- else if (sz_a < sz_b)
+ if (pl_a->remaining_objects->size == pl_b->remaining_objects->size) {
+ /* have the same remaining_objects, big pack first */
+ if (pl_a->all_objects_size == pl_b->all_objects_size)
+ return 0;
+ else if (pl_a->all_objects_size < pl_b->all_objects_size)
+ return 1;
+ else
+ return -1;
+ } else if (pl_a->remaining_objects->size < pl_b->remaining_objects->size) {
+ /* sort by remaining objects, more objects first */
return 1;
- else
+ } else {
return -1;
+ }
}
/* Sort pack_list, greater size of remaining_objects first */
@@ -370,7 +377,7 @@ static void sort_pack_list(struct pack_list **pl)
for (n = 0, p = *pl; p; p = p->next)
ary[n++] = p;
- QSORT(ary, n, cmp_pack_list_reverse);
+ QSORT(ary, n, cmp_remaining_objects);
/* link them back again */
for (i = 0; i < n - 1; i++)
@@ -511,6 +518,7 @@ static struct pack_list * add_pack(struct packed_git *p)
llist_insert_back(l.remaining_objects, (const struct object_id *)(base + off));
off += step;
}
+ l.all_objects_size = l.remaining_objects->size;
l.unique_objects = NULL;
if (p->pack_local)
return pack_list_insert(&local_packs, &l);
diff --git a/t/t5323-pack-redundant.sh b/t/t5323-pack-redundant.sh
index 897acaa365..e642240d71 100755
--- a/t/t5323-pack-redundant.sh
+++ b/t/t5323-pack-redundant.sh
@@ -210,19 +210,19 @@ test_expect_success 'create pack 4, 5' '
# | T A B C D E F G H I J K L M N O P Q R
# ----+--------------------------------------
# P1 | x x x x x x x x
-# P2* | ! ! ! ! ! ! !
-# P3 | x x x x x x
+# P2 | x x x x x x x
+# P3* | ! ! ! ! ! !
# P4 | x x x x x
# P5 | x x x x
# ----+--------------------------------------
# ALL | x x x x x x x x x x x x x x x x x x
#
#############################################################################
-test_expect_failure 'one of pack-2/pack-3 is redundant (failed on Mac)' '
+test_expect_success 'one of pack-2/pack-3 is redundant' '
(
cd "$master_repo" &&
cat >expect <<-EOF &&
- P2:$P2
+ P3:$P3
EOF
git pack-redundant --all >out &&
format_packfiles <out >actual &&
@@ -250,7 +250,7 @@ test_expect_success 'create pack 6, 7' '
# ALL | x x x x x x x x x x x x x x x x x x x
#
#############################################################################
-test_expect_failure 'pack 2, 4, and 6 are redundant (failed on Mac)' '
+test_expect_success 'pack 2, 4, and 6 are redundant' '
(
cd "$master_repo" &&
cat >expect <<-EOF &&
@@ -285,7 +285,7 @@ test_expect_success 'create pack 8' '
# ALL | x x x x x x x x x x x x x x x x x x x
#
#############################################################################
-test_expect_failure 'pack-8 (subset of pack-1) is also redundant (failed on Mac)' '
+test_expect_success 'pack-8 (subset of pack-1) is also redundant' '
(
cd "$master_repo" &&
cat >expect <<-EOF &&
@@ -309,7 +309,7 @@ test_expect_success 'clean loose objects' '
)
'
-test_expect_failure 'remove redundant packs and pass fsck (failed on Mac)' '
+test_expect_success 'remove redundant packs and pass fsck' '
(
cd "$master_repo" &&
git pack-redundant --all | xargs rm &&
@@ -329,7 +329,7 @@ test_expect_success 'setup shared.git' '
)
'
-test_expect_failure 'no redundant packs without --alt-odb (failed on Mac)' '
+test_expect_success 'no redundant packs without --alt-odb' '
(
cd "$shared_repo" &&
git pack-redundant --all >out &&
@@ -362,7 +362,7 @@ test_expect_failure 'no redundant packs without --alt-odb (failed on Mac)' '
# ALL | x x x x x x x x x x x x x x x x x x x
#
#############################################################################
-test_expect_failure 'pack-redundant --verbose: show duplicate packs in stderr (failed on Mac)' '
+test_expect_success 'pack-redundant --verbose: show duplicate packs in stderr' '
(
cd "$shared_repo" &&
cat >expect <<-EOF &&
--
2.20.1.103.ged0fc2ca7b
