@matheusbsbs / matheus-bagliano / commits / 3c1e2c2113

builtin/repack.c: avoid making cruft packs preferred

When doing a `--geometric` repack, we make sure that the preferred pack (if writing a MIDX) is the largest pack that we *didn't* repack. That has the effect of keeping the preferred pack in sync with the pack containing a majority of the repository's reachable objects. But if the repository happens to double in size, we'll repack everything. Here we don't specify any `--preferred-pack`, and instead let the MIDX code choose. In the past, that worked fine, since there would only be one pack to choose from: the one we just wrote. But it's no longer necessarily the case that there is one pack to choose from. It's possible that the repository also has a cruft pack, too. If the cruft pack happens to come earlier in lexical order (and has an earlier mtime than any non-cruft pack), we'll pick that pack as preferred. This makes it impossible to reuse chunks of the reachable pack verbatim from pack-objects, so is sub-optimal. Luckily, this is a somewhat rare circumstance to be in, since we would have to repack the entire repository during a `--geometric` repack, and the cruft pack would have to sort ahead of the pack we just created. Signed-off-by: Taylor Blau <me@ttaylorr.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Taylor Blau committed Oct 3, 2023 at 17:54 UTC 3c1e2c2113842b8462803ef8c9aca596eacfd3af

2 files changed +85 -1

builtin/repack.c

+46 -1

@@ -355,6 +355,18 @@ static struct generated_pack_data *populate_pack_exts(const char *name)

355 return data;

356 }

357

358 +static int has_pack_ext(const struct generated_pack_data *data,

359 + const char *ext)

360 +{

361 + int i;

362 + for (i = 0; i < ARRAY_SIZE(exts); i++) {

363 + if (strcmp(exts[i].name, ext))

364 + continue;

365 + return !!data->tempfiles[i];

366 + }

367 + BUG("unknown pack extension: '%s'", ext);

368 +}

369 +

370 static void repack_promisor_objects(const struct pack_objects_args *args,

371 struct string_list *names)

372 {

@@ -772,6 +784,7 @@ static void midx_included_packs(struct string_list *include,

784

785 static int write_midx_included_packs(struct string_list *include,

786 struct pack_geometry *geometry,

787 + struct string_list *names,

788 const char *refs_snapshot,

789 int show_progress, int write_bitmaps)

790 {

@@ -801,6 +814,38 @@ static int write_midx_included_packs(struct string_list *include,

814 if (preferred)

815 strvec_pushf(&cmd.args, "--preferred-pack=%s",

816 pack_basename(preferred));

817 + else if (names->nr) {

818 + /* The largest pack was repacked, meaning that either

819 + * one or two packs exist depending on whether the

820 + * repository has a cruft pack or not.

821 + *

822 + * Select the non-cruft one as preferred to encourage

823 + * pack-reuse among packs containing reachable objects

824 + * over unreachable ones.

825 + *

826 + * (Note we could write multiple packs here if

827 + * `--max-pack-size` was given, but any one of them

828 + * will suffice, so pick the first one.)

829 + */

830 + for_each_string_list_item(item, names) {

831 + struct generated_pack_data *data = item->util;

832 + if (has_pack_ext(data, ".mtimes"))

833 + continue;

834 +

835 + strvec_pushf(&cmd.args, "--preferred-pack=pack-%s.pack",

836 + item->string);

837 + break;

838 + }

839 + } else {

840 + /*

841 + * No packs were kept, and no packs were written. The

842 + * only thing remaining are .keep packs (unless

843 + * --pack-kept-objects was given).

844 + *

845 + * Set the `--preferred-pack` arbitrarily here.

846 + */

847 + ;

848 + }

849

850 if (refs_snapshot)

851 strvec_pushf(&cmd.args, "--refs-snapshot=%s", refs_snapshot);

@@ -1360,7 +1405,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)

1405 struct string_list include = STRING_LIST_INIT_NODUP;

1406 midx_included_packs(&include, &existing, &names, &geometry);

1407

1363 - ret = write_midx_included_packs(&include, &geometry,

1408 + ret = write_midx_included_packs(&include, &geometry, &names,

1409 refs_snapshot ? get_tempfile_path(refs_snapshot) : NULL,

1410 show_progress, write_bitmaps > 0);

1411

t/t7704-repack-cruft.sh

+39

@@ -372,4 +372,43 @@ test_expect_success '--max-cruft-size ignores non-local packs' '

372 )

373 '

374

375 +test_expect_success 'reachable packs are preferred over cruft ones' '

376 + repo="cruft-preferred-packs" &&

377 + git init "$repo" &&

378 + (

379 + cd "$repo" &&

380 +

381 + # This test needs to exercise careful control over when a MIDX

382 + # is and is not written. Unset the corresponding TEST variable

383 + # accordingly.

384 + sane_unset GIT_TEST_MULTI_PACK_INDEX &&

385 +

386 + test_commit base &&

387 + test_commit --no-tag cruft &&

388 +

389 + non_cruft="$(echo base | git pack-objects --revs $packdir/pack)" &&

390 + # Write a cruft pack which both (a) sorts ahead of the non-cruft

391 + # pack in lexical order, and (b) has an older mtime to appease

392 + # the MIDX preferred pack selection routine.

393 + cruft="$(echo pack-$non_cruft.pack | git pack-objects --cruft $packdir/pack-A)" &&

394 + test-tool chmtime -1000 $packdir/pack-A-$cruft.pack &&

395 +

396 + test_commit other &&

397 + git repack -d &&

398 +

399 + git repack --geometric 2 -d --write-midx --write-bitmap-index &&

400 +

401 + # After repacking, there are two packs left: one reachable one

402 + # (which is the result of combining both of the existing two

403 + # non-cruft packs), and one cruft pack.

404 + find .git/objects/pack -type f -name "*.pack" >packs &&

405 + test_line_count = 2 packs &&

406 +

407 + # Make sure that the pack we just wrote is marked as preferred,

408 + # not the cruft one.

409 + pack="$(test-tool read-midx --preferred-pack $objdir)" &&

410 + test_path_is_missing "$packdir/$(basename "$pack" ".idx").mtimes"

411 + )

412 +'

413 +

414 test_done