repack: respect --keep-pack with geometric repack

Update 'repack' to ignore packs named on the command line with the
'--keep-pack' option. Specifically, modify 'init_pack_geometry()' to treat
command line-kept packs the same way it treats packs with an on-disk '.keep'
file (that is, skip the pack and do not include it in the 'geometry'
structure).

Without this handling, a '--keep-pack' pack would be included in the
'geometry' structure. If the pack is *before* the geometry split line (with
at least one other pack and/or loose objects present), 'repack' assumes the
pack's contents are "rolled up" into another pack via 'pack-objects'.
However, because the internally-invoked 'pack-objects' properly excludes
'--keep-pack' objects, any new pack it creates will not contain the kept
objects. Finally, 'repack' deletes the '--keep-pack' as "redundant" (since
it assumes 'pack-objects' created a new pack with its contents), resulting
in possible object loss and repository corruption.

Add a test ensuring that '--keep-pack' packs are now appropriately handled.

Co-authored-by: Taylor Blau <me@ttaylorr.com>
Signed-off-by: Victoria Dye <vdye@github.com>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/builtin/repack.c b/builtin/repack.c
index d1a563d5b65666b68596ad353a945e8e11c6ac70..ea56e92ad57a1423638421e105df30a111e7bd1c 100644 (file)
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -137,6 +137,8 @@ static void collect_pack_filenames(struct string_list *fname_nonkept_list,
                        string_list_append_nodup(fname_nonkept_list, fname);
        }
        closedir(dir);
+
+       string_list_sort(fname_kept_list);
 }
 
 static void remove_redundant_pack(const char *dir_name, const char *base_name)
@@ -332,17 +334,38 @@ 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;
 
        for (p = get_all_packs(the_repository); p; p = p->next) {
-               if (!pack_kept_objects && p->pack_keep)
-                       continue;
+               if (!pack_kept_objects) {
+                       /*
+                        * Any pack that has its pack_keep bit set will appear
+                        * in existing_kept_packs below, but this saves us from
+                        * doing a more expensive check.
+                        */
+                       if (p->pack_keep)
+                               continue;
+
+                       /*
+                        * The pack may be kept via the --keep-pack option;
+                        * check 'existing_kept_packs' to determine whether to
+                        * ignore it.
+                        */
+                       strbuf_reset(&buf);
+                       strbuf_addstr(&buf, pack_basename(p));
+                       strbuf_strip_suffix(&buf, ".pack");
+
+                       if (string_list_has_string(existing_kept_packs, buf.buf))
+                               continue;
+               }
 
                ALLOC_GROW(geometry->pack,
                           geometry->pack_nr + 1,
@@ -353,6 +376,7 @@ static void init_pack_geometry(struct pack_geometry **geometry_p)
        }
 
        QSORT(geometry->pack, geometry->pack_nr, geometry_cmp);
+       strbuf_release(&buf);
 }
 
 static void split_pack_geometry(struct pack_geometry *geometry, int factor)
@@ -714,17 +738,20 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
                strbuf_release(&path);
        }
 
+       packdir = mkpathdup("%s/pack", get_object_directory());
+       packtmp_name = xstrfmt(".tmp-%d-pack", (int)getpid());
+       packtmp = mkpathdup("%s/%s", packdir, packtmp_name);
+
+       collect_pack_filenames(&existing_nonkept_packs, &existing_kept_packs,
+                              &keep_pack_list);
+
        if (geometric_factor) {
                if (pack_everything)
                        die(_("options '%s' and '%s' cannot be used together"), "--geometric", "-A/-a");
-               init_pack_geometry(&geometry);
+               init_pack_geometry(&geometry, &existing_kept_packs);
                split_pack_geometry(geometry, geometric_factor);
        }
 
-       packdir = mkpathdup("%s/pack", get_object_directory());
-       packtmp_name = xstrfmt(".tmp-%d-pack", (int)getpid());
-       packtmp = mkpathdup("%s/%s", packdir, packtmp_name);
-
        sigchain_push_common(remove_pack_on_signal);
 
        prepare_pack_objects(&cmd, &po_args);
@@ -764,9 +791,6 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
        if (use_delta_islands)
                strvec_push(&cmd.args, "--delta-islands");
 
-       collect_pack_filenames(&existing_nonkept_packs, &existing_kept_packs,
-                              &keep_pack_list);
-
        if (pack_everything & ALL_INTO_ONE) {
                repack_promisor_objects(&po_args, &names);
 

diff --git a/t/t7703-repack-geometric.sh b/t/t7703-repack-geometric.sh
index bdbbcbf1eca88fef1c2dcd6632d880d3848c842d..91bb2b37a8bf56cbf43fb22c4a9a5b433ef9d8a0 100755 (executable)
--- a/t/t7703-repack-geometric.sh
+++ b/t/t7703-repack-geometric.sh
@@ -180,6 +180,34 @@ test_expect_success '--geometric ignores kept packs' '
        )
 '
 
+test_expect_success '--geometric ignores --keep-pack packs' '
+       git init geometric &&
+       test_when_finished "rm -fr geometric" &&
+       (
+               cd geometric &&
+
+               # Create two equal-sized packs
+               test_commit kept && # 3 objects
+               git repack -d &&
+               test_commit pack && # 3 objects
+               git repack -d &&
+
+               find $objdir/pack -type f -name "*.pack" | sort >packs.before &&
+               git repack --geometric 2 -dm \
+                       --keep-pack="$(basename "$(head -n 1 packs.before)")" >out &&
+               find $objdir/pack -type f -name "*.pack" | sort >packs.after &&
+
+               # Packs should not have changed (only one non-kept pack, no
+               # loose objects), but $midx should now exist.
+               grep "Nothing new to pack" out &&
+               test_path_is_file $midx &&
+
+               test_cmp packs.before packs.after &&
+
+               git fsck
+       )
+'
+
 test_expect_success '--geometric chooses largest MIDX preferred pack' '
        git init geometric &&
        test_when_finished "rm -fr geometric" &&