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>

Victoria Dye committed May 20, 2022 at 15:01 UTC 4b5a808bb906896e07ca147645b33feb81e91c4c
2 files changed +63 -11
builtin/repack.c
+35 -11
@@ -137,6 +137,8 @@ static void collect_pack_filenames(struct string_list *fname_nonkept_list,
137 string_list_append_nodup(fname_nonkept_list, fname);
138 }
139 closedir(dir);
140 +
141 + string_list_sort(fname_kept_list);
142 }
143
144 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)
334 return 0;
335 }
336
335 -static void init_pack_geometry(struct pack_geometry **geometry_p)
337 +static void init_pack_geometry(struct pack_geometry **geometry_p,
338 + struct string_list *existing_kept_packs)
339 {
340 struct packed_git *p;
341 struct pack_geometry *geometry;
342 + struct strbuf buf = STRBUF_INIT;
343
344 *geometry_p = xcalloc(1, sizeof(struct pack_geometry));
345 geometry = *geometry_p;
346
347 for (p = get_all_packs(the_repository); p; p = p->next) {
344 - if (!pack_kept_objects && p->pack_keep)
345 - continue;
348 + if (!pack_kept_objects) {
349 + /*
350 + * Any pack that has its pack_keep bit set will appear
351 + * in existing_kept_packs below, but this saves us from
352 + * doing a more expensive check.
353 + */
354 + if (p->pack_keep)
355 + continue;
356 +
357 + /*
358 + * The pack may be kept via the --keep-pack option;
359 + * check 'existing_kept_packs' to determine whether to
360 + * ignore it.
361 + */
362 + strbuf_reset(&buf);
363 + strbuf_addstr(&buf, pack_basename(p));
364 + strbuf_strip_suffix(&buf, ".pack");
365 +
366 + if (string_list_has_string(existing_kept_packs, buf.buf))
367 + continue;
368 + }
369
370 ALLOC_GROW(geometry->pack,
371 geometry->pack_nr + 1,
@@ -353,6 +376,7 @@ static void init_pack_geometry(struct pack_geometry **geometry_p)
376 }
377
378 QSORT(geometry->pack, geometry->pack_nr, geometry_cmp);
379 + strbuf_release(&buf);
380 }
381
382 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)
738 strbuf_release(&path);
739 }
740
741 + packdir = mkpathdup("%s/pack", get_object_directory());
742 + packtmp_name = xstrfmt(".tmp-%d-pack", (int)getpid());
743 + packtmp = mkpathdup("%s/%s", packdir, packtmp_name);
744 +
745 + collect_pack_filenames(&existing_nonkept_packs, &existing_kept_packs,
746 + &keep_pack_list);
747 +
748 if (geometric_factor) {
749 if (pack_everything)
750 die(_("options '%s' and '%s' cannot be used together"), "--geometric", "-A/-a");
720 - init_pack_geometry(&geometry);
751 + init_pack_geometry(&geometry, &existing_kept_packs);
752 split_pack_geometry(geometry, geometric_factor);
753 }
754
724 - packdir = mkpathdup("%s/pack", get_object_directory());
725 - packtmp_name = xstrfmt(".tmp-%d-pack", (int)getpid());
726 - packtmp = mkpathdup("%s/%s", packdir, packtmp_name);
727 -
755 sigchain_push_common(remove_pack_on_signal);
756
757 prepare_pack_objects(&cmd, &po_args);
@@ -764,9 +791,6 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
791 if (use_delta_islands)
792 strvec_push(&cmd.args, "--delta-islands");
793
767 - collect_pack_filenames(&existing_nonkept_packs, &existing_kept_packs,
768 - &keep_pack_list);
769 -
794 if (pack_everything & ALL_INTO_ONE) {
795 repack_promisor_objects(&po_args, &names);
796
t/t7703-repack-geometric.sh
+28
@@ -180,6 +180,34 @@ test_expect_success '--geometric ignores kept packs' '
180 )
181 '
182
183 +test_expect_success '--geometric ignores --keep-pack packs' '
184 + git init geometric &&
185 + test_when_finished "rm -fr geometric" &&
186 + (
187 + cd geometric &&
188 +
189 + # Create two equal-sized packs
190 + test_commit kept && # 3 objects
191 + git repack -d &&
192 + test_commit pack && # 3 objects
193 + git repack -d &&
194 +
195 + find $objdir/pack -type f -name "*.pack" | sort >packs.before &&
196 + git repack --geometric 2 -dm \
197 + --keep-pack="$(basename "$(head -n 1 packs.before)")" >out &&
198 + find $objdir/pack -type f -name "*.pack" | sort >packs.after &&
199 +
200 + # Packs should not have changed (only one non-kept pack, no
201 + # loose objects), but $midx should now exist.
202 + grep "Nothing new to pack" out &&
203 + test_path_is_file $midx &&
204 +
205 + test_cmp packs.before packs.after &&
206 +
207 + git fsck
208 + )
209 +'
210 +
211 test_expect_success '--geometric chooses largest MIDX preferred pack' '
212 git init geometric &&
213 test_when_finished "rm -fr geometric" &&