repack: combine '--geometric' and '--cruft'
cover repack: combine '--geometric' and '--cruft'
First, a short note. This series is an RFC because I have not had the chance to review and test it as thoroughly as I normally would, and because we are deep in the -rc phase.
I wanted to get this series off my backlog since I have decided to leave GitHub at the end of the month for a new role. I will still be contributing to Git in my new role (which I will start in the early part of July), but wanted to get this off my backlog nonetheless.
This series teaches git repack how to combine --geometric and --cruft.
Today these two modes are mutually exclusive, since --cruft implies -a, and -a is fundamentally incompatible with --geometric. As a result, repositories have to choose between keeping reachable objects in a geometric progression of packs and collecting unreachable objects into cruft packs.
The goal of this series is to to be able to do both simultaneously. When both options are given, 'git repack' rolls up the selected non-cruft packs as usual while collecting unreachable objects separately into a cruft pack. That means a command like
$ git repack -d --geometric=2 --cruft --combine-cruft-below-size=1G
will keep reachable non-cruft packs in a geometric progression, while combining sufficiently-small cruft packs (along with newly-discovered unreachable objects) into a fresh cruft pack.
The series is structured roughly as follows:
* The first two patches prepare the cruft pack machinery for the later changes by making non-kept pack exclusion unconditional and extracting a helper for looking up packs in an existing_packs list.
* The next four patches route geometric pack deletion through the common existing_packs machinery. They mark packs above the geometric split as retained, teach incremental-MIDX retention not to keep packs that are being rolled up, switch geometric repacks over to the common deletion path, and then remove the old geometry-specific deletion helper.
* The next three patches teach pack-objects the new pieces needed by this mode. The main addition is --stdin-packs=follow-reachable, which walks from reference tips and includes only reachable objects from the selected packs, while still allowing traversal through excluded-open packs and stopping at excluded-closed ones. The following patch teaches that mode to use --refs-snapshot, so that pack-objects and the MIDX bitmap writer can agree on the same set of tips.
* The final patch wires everything together in git repack, including teaching the cruft writer how to interpret the geometric split when choosing which packs to include or exclude.
Thanks in advance for your review!
Taylor Blau (10):
repack: unconditionally exclude non-kept packs
repack: extract locate_existing_pack() helper
repack: mark geometric progression of packs as retained
repack: teach MIDX retention about geometric rollups
repack: delete geometric packs via existing_packs
repack-geometry: drop unused redundant-pack removal
pack-objects: extract stdin_packs_add_all_pack_entries()
pack-objects: introduce '--stdin-packs=follow-reachable'
pack-objects: support '--refs-snapshot' with 'follow-reachable'
repack: support combining '--geometric' with '--cruft'
Documentation/git-pack-objects.adoc | 25 +++ Documentation/git-repack.adoc | 11 ++ builtin/pack-objects.c | 276 ++++++++++++++++++++++++---- builtin/repack.c | 38 ++-- repack-cruft.c | 29 ++- repack-geometry.c | 44 ----- repack.c | 101 +++++++++- repack.h | 15 +- t/t5331-pack-objects-stdin.sh | 201 ++++++++++++++++++++ t/t7704-repack-cruft.sh | 251 +++++++++++++++++++++++++ 10 files changed, 878 insertions(+), 113 deletions(-)
base-commit: ab776a62a78576513ee121424adb19597fbb7613
Signature · 1 line
2.55.0.rc2.10.g29e31820dce
RFC PATCH 1/10 repack: unconditionally exclude non-kept packs
In write_cruft_pack(), we handle excluding objects found in non-kept packs from being included in the cruft pack via two code paths:
* When using '--combine-cruft-below-size' (provided that we are not expiring cruft objects), we use the aptly-named combine_small_cruft_packs() function.
* In all other cases, we handle it directly in the 'else' branch of the same conditional.
Simplify this by moving the non-kept pack exclusion out of the conditional entirely, so that non-kept packs are always excluded regardless of whether we are combining small cruft packs or not.
This is a preparatory refactor for a subsequent change that will use the pack_geometry struct when available to determine which non-kept packs to exclude.
--- repack-cruft.c | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-)
repack-cruft.c +4 -10
@@ -9,7 +9,6 @@ static void combine_small_cruft_packs(FILE *in, off_t combine_cruft_below_size, {struct packed_git *p;struct strbuf buf =STRBUF_INIT ; -size_t i;repo_for_each_pack (existing->repo, p) {if (!(p->is_cruft && p->pack_local))
@@ -30,10 +29,6 @@ static void combine_small_cruft_packs(FILE *in, off_t combine_cruft_below_size, } } -for (i =0 ; i < existing->non_kept_packs.nr; i++) -fprintf (in,"-%s.pack\n" , - existing->non_kept_packs.items[i].string); -strbuf_release (&buf); }
@@ -80,15 +75,14 @@ int write_cruft_pack(const struct write_pack_opts *opts, in =xfdopen (cmd.in,"w" );for_each_string_list_item (item, names)fprintf (in,"%s-%s.pack\n" , pack_prefix, item->string); -if (combine_cruft_below_size && !cruft_expiration) { +if (combine_cruft_below_size && !cruft_expiration)combine_small_cruft_packs (in, combine_cruft_below_size, existing); - }else { -for_each_string_list_item (item, &existing->non_kept_packs) -fprintf (in,"-%s.pack\n" , item->string); +else for_each_string_list_item (item, &existing->cruft_packs)fprintf (in,"-%s.pack\n" , item->string); - } +for_each_string_list_item (item, &existing->non_kept_packs) +fprintf (in,"-%s.pack\n" , item->string);for_each_string_list_item (item, &existing->kept_packs)fprintf (in,"%s.pack\n" , item->string);fclose (in);
Signature · 1 line
2.55.0.rc2.10.g29e31820dce
RFC PATCH 2/10 repack: extract `locate_existing_pack()` helper
In reply to Taylor BlauFactor out the lookup from existing_packs_retain_cruft() that converts a pack basename to a string_list_item into a reusable static helper function, locate_existing_pack().
A subsequent commit will introduce a new function which will need to perform this same lookup against a different string_list.
--- repack.c | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-)
repack.c +17 -6
@@ -226,21 +226,32 @@ static void existing_packs_mark_for_deletion_1(const struct git_hash_algo *algop } } +static struct string_list_item *locate_existing_pack (struct string_list *list, +struct packed_git *p) +{ +struct strbuf buf =STRBUF_INIT ; +struct string_list_item *item; + +strbuf_addstr (&buf,pack_basename (p)); +strbuf_strip_suffix (&buf,".pack" ); + + item =string_list_lookup (list, buf.buf); + +strbuf_release (&buf); + +return item; +} +void existing_packs_retain_cruft (struct existing_packs *existing,struct packed_git *cruft) { -struct strbuf buf =STRBUF_INIT ;struct string_list_item *item; -strbuf_addstr (&buf,pack_basename (cruft)); -strbuf_strip_suffix (&buf,".pack" ); - - item =string_list_lookup (&existing->cruft_packs, buf.buf); + item =locate_existing_pack (&existing->cruft_packs, cruft);if (!item)BUG ("could not find cruft pack '%s'" ,pack_basename (cruft));existing_packs_mark_retained (item); -strbuf_release (&buf); } void existing_packs_mark_for_deletion(struct existing_packs *existing,
Signature · 1 line
2.55.0.rc2.10.g29e31820dce
RFC PATCH 3/10 repack: mark geometric progression of packs as retained
In reply to Taylor BlauIn non-geometric repacks, any packs which repack wishes to delete are handled via the existing_packs struct, which has a mechanism to retain would-be-deleted packs (e.g., if we happened to write a new pack identical to one otherwise marked for deletion).
In geometric repacks, repack removes any rewritten packs (alternatively, any packs which were combined in order to restore a geometric progression) by enumerating them via pack_geometry_remove_redundant().
Prepare to use the existing_packs deletion machinery for geometric repacks by marking any non-kept packs above the geometric split line as retained. Do the same for promisor packs, which have their own split point.
This commit only records which packs the later deletion pass must keep; it does not change which packs are written or removed.
--- builtin/repack.c | 2 ++ repack.c | 27 +++++++++++++++++++++++++++ repack.h | 3 +++ 3 files changed, 32 insertions(+)
builtin/repack.c +2 -0
@@ -325,6 +325,8 @@ int cmd_repack(int argc, }pack_geometry_init (&geometry, &existing, &po_args);pack_geometry_split (&geometry); + +existing_packs_retain_from_geometry (&existing, &geometry); }prepare_pack_objects (&cmd, &po_args, packtmp);
repack.c +27 -0
@@ -254,6 +254,33 @@ void existing_packs_retain_cruft(struct existing_packs *existing,existing_packs_mark_retained (item); } +static void existing_packs_retain_non_kept (struct existing_packs *existing, +struct packed_git *p) +{ +struct string_list_item *item; + +if (!p->pack_local) +return ; + + item =locate_existing_pack (&existing->non_kept_packs, p); +if (!item) +BUG ("could not find non-kept pack '%s'" ,pack_basename (p)); + +existing_packs_mark_retained (item); +} + +void existing_packs_retain_from_geometry (struct existing_packs *existing, +const struct pack_geometry *geometry) +{ +uint32_t i; + +for (i = geometry->split; i < geometry->pack_nr; i++) +existing_packs_retain_non_kept (existing, geometry->pack[i]); +for (i = geometry->promisor_split; i < geometry->promisor_pack_nr; i++) +existing_packs_retain_non_kept (existing, + geometry->promisor_pack[i]); +} +void existing_packs_mark_for_deletion (struct existing_packs *existing,struct string_list *names)
repack.h +3 -0
@@ -54,6 +54,7 @@ int finish_pack_objects_cmd(const struct git_hash_algo *algop,struct repository ;struct packed_git ; +struct pack_geometry ;struct existing_packs {struct repository *repo;
@@ -82,6 +83,8 @@ int existing_packs_has_non_kept(const struct existing_packs *existing);int existing_pack_is_marked_for_deletion (struct string_list_item *item);void existing_packs_retain_cruft (struct existing_packs *existing,struct packed_git *cruft); +void existing_packs_retain_from_geometry (struct existing_packs *existing, +const struct pack_geometry *geometry);void existing_packs_mark_for_deletion (struct existing_packs *existing,struct string_list *names);void existing_packs_retain_midx_packs (struct existing_packs *existing);
Signature · 1 line
2.55.0.rc2.10.g29e31820dce
RFC PATCH 4/10 repack: teach MIDX retention about geometric rollups
In reply to Taylor BlauWhen writing an incremental MIDX, existing_packs_retain_midx_packs() marks packs in the existing MIDX chain as retained. This keeps them from being deleted by the later existing_packs deletion pass, since retained MIDX layers may still refer to those packs.
Geometric repacks need a narrower rule. Packs below the split are rolled up into the newly-written pack, and should remain eligible for deletion even if the old MIDX chain mentions them. Packs above the split were marked as retained by the previous commit.
Teach existing_packs_retain_midx_packs() to skip packs which are part of the geometric rollup. This does not change the current caller's behavior, since geometric repacks do not yet use the existing_packs deletion path.
--- builtin/repack.c | 2 +- repack.c | 43 +++++++++++++++++++++++++++++++++++++++++-- repack.h | 3 ++- 3 files changed, 44 insertions(+), 4 deletions(-)
builtin/repack.c +1 -1
@@ -576,7 +576,7 @@ int cmd_repack(int argc,if (delete_redundant && pack_everything &ALL_INTO_ONE ) {if (write_midx ==REPACK_WRITE_MIDX_INCREMENTAL ) -existing_packs_retain_midx_packs (&existing); +existing_packs_retain_midx_packs (&existing, &geometry);existing_packs_mark_for_deletion (&existing, &names); }
repack.c +41 -2
62 lines not on this page · raw message
repack.h +2 -1
@@ -87,7 +87,8 @@ void existing_packs_retain_from_geometry(struct existing_packs *existing, const struct pack_geometry *geometry);void existing_packs_mark_for_deletion (struct existing_packs *existing,struct string_list *names); -void existing_packs_retain_midx_packs (struct existing_packs *existing); +void existing_packs_retain_midx_packs (struct existing_packs *existing, +const struct pack_geometry *geometry);void existing_packs_remove_redundant (struct existing_packs *existing,const char *packdir,bool wrote_incremental_midx);
Signature · 1 line
2.55.0.rc2.10.g29e31820dce
Re: [RFC PATCH 04/10] repack: teach MIDX retention about geometric rollups
Quoted 11 lines · Taylor Blau
repack.c +11
-0+static int pack_geometry_contains_pack (struct packed_git **packs, +uint32_t packs_nr, +const char *base) +{ +struct strbuf buf =STRBUF_INIT ; +uint32_t i; + +for (i =0 ; i < packs_nr; i++) { +strbuf_reset (&buf); +strbuf_addstr (&buf,pack_basename (packs[i])); +strbuf_strip_suffix (&buf,".pack" );
repack.c void existing_packs_mark_for_deletion(struct existing_packs *existing, · line 315
Quoted 10 lines
repack.c +10
-0+ +if (!strcmp (buf.buf, base)) { +strbuf_release (&buf); +return 1 ; + } + } + +strbuf_release (&buf); +return 0 ; +}
It feels slightly inefficient to repeatedly strbuf_reset(), strbuf_addstr(), and strbuf_strip_suffix() in the loop. I do not know if my understanding of what existing_packs_retain_midx_packs() passes down in buf.buf as base is correct or not, but if so, wouldn't it equivalent to
for (uint32_t i = 0; i < packs_nr; i++) {
const char *pack_name = pack_basename(packs[i]);
const char *suffix;
if (skip_prefix(pack_name, base, &suffix) &&
!strcmp(suffix, ".pack"))
return 1;
}perhaps?
Starting from "/path/to/objects/pack/pack-deadbeef.pack", you take the basename of it to have "pack-deadbeef.pack" in buf, strip out the ".pack" suffix to get "pack-deadbeef" in buf and then compare it with the base.
Instead, pack_name in the rewitten one becomes the basename of the packfile path, i.e., "pack-deadbeef.pack", then we see if it begins with base and take the remainder in suffix, and finally we check if that remaining suffix is ".pack".
Which should be equivalent.
repack.c void existing_packs_retain_midx_packs(struct existing_packs *existing) · line 352
Quoted 13 lines
+ * freshly-written pack supersedes them. When doing a geometric repack,
+ * packs below the split are rewritten into the new MIDX tip and should
+ * remain eligible for deletion.
*/repack.c +2
-1-void existing_packs_retain_midx_packs (struct existing_packs *existing) +void existing_packs_retain_midx_packs (struct existing_packs *existing, +const struct pack_geometry *geometry) {struct string_list_item *item;struct strbuf buf =STRBUF_INIT ;@@ -315,6 +351,9 @@ void existing_packs_retain_midx_packs(struct existing_packs *existing)strbuf_strip_suffix (&buf,".pack" );strbuf_strip_suffix (&buf,".idx" );
Not a fault of this patch, but it makes the hairs on the back of my head tingle to see that a bogus input like "pack-foobar.idx.pack" happily is taken, while "pack-foobar.pack.idx", an equally bogus input, is not.
Quoted 2 lines
repack.c +2
-0+if (pack_geometry_contains_rollup (geometry, buf.buf)) +continue ;
Re: [RFC PATCH 04/10] repack: teach MIDX retention about geometric rollups
Quoted 39 lines · Junio C Hamano
Quoted 11 lines · Taylor Blau
repack.c +11
-0+static int pack_geometry_contains_pack (struct packed_git **packs, +uint32_t packs_nr, +const char *base) +{ +struct strbuf buf =STRBUF_INIT ; +uint32_t i; + +for (i =0 ; i < packs_nr; i++) { +strbuf_reset (&buf); +strbuf_addstr (&buf,pack_basename (packs[i])); +strbuf_strip_suffix (&buf,".pack" );repack.c void existing_packs_mark_for_deletion(struct existing_packs *existing, · line 315
Quoted 10 lines
repack.c +10
-0+ +if (!strcmp (buf.buf, base)) { +strbuf_release (&buf); +return 1 ; + } + } + +strbuf_release (&buf); +return 0 ; +}It feels slightly inefficient to repeatedly
strbuf_reset(),strbuf_addstr(), andstrbuf_strip_suffix()in the loop. I do not know if my understanding of whatexisting_packs_retain_midx_packs()passes down in buf.buf as base is correct or not, but if so, wouldn't it equivalent tofor (uint32_t i = 0; i < packs_nr; i++) { const char *pack_name = pack_basename(packs[i]); const char *suffix; if (skip_prefix(pack_name, base, &suffix) && !strcmp(suffix, ".pack")) return 1; }
Quoted 6 lines
perhaps?
Starting from "/path/to/objects/pack/pack-deadbeef.pack", you take the basename of it to have "pack-deadbeef.pack" in buf, strip out the ".pack" suffix to get "pack-deadbeef" in buf and then compare it with the base.
I think that this would work nicely. I think that the skip_prefix() variant is easy enough to read, and is clearly more efficient.
Quoted 17 lines
Instead,
pack_namein the rewitten one becomes the basename of the packfile path, i.e., "pack-deadbeef.pack", then we see if it begins with base and take the remainder in suffix, and finally we check if that remaining suffix is ".pack".Which should be equivalent.
Quoted 10 lines
+ * freshly-written pack supersedes them. When doing a geometric repack,
+ * packs below the split are rewritten into the new MIDX tip and should
+ * remain eligible for deletion.
*/repack.c +2
-1-void existing_packs_retain_midx_packs (struct existing_packs *existing) +void existing_packs_retain_midx_packs (struct existing_packs *existing, +const struct pack_geometry *geometry) {struct string_list_item *item;struct strbuf buf =STRBUF_INIT ;
Quoted 8 lines
repack.c void existing_packs_retain_midx_packs(struct existing_packs *existing) · line 352
Quoted 3 lines
repack.c +0
-0@@ -315,6 +351,9 @@ void existing_packs_retain_midx_packs(struct existing_packs *existing)strbuf_strip_suffix (&buf,".pack" );strbuf_strip_suffix (&buf,".idx" );Not a fault of this patch, but it makes the hairs on the back of my head tingle to see that a bogus input like "pack-foobar.idx.pack" happily is taken, while "pack-foobar.pack.idx", an equally bogus input, is not.
Yeah, this is gross (and my fault). Presumably I was swapping out one variant for another and didn't stage the removal of one of the strip_suffix() calls.
Thanks,
Taylor
RFC PATCH 5/10 repack: delete geometric packs via existing_packs
In reply to Taylor BlauNow that packs above the geometric split are marked as retained, teach geometric repacks to use the existing_packs deletion machinery instead of calling pack_geometry_remove_redundant().
This lets geometric repacks share the same mark-then-remove path as all-into-one repacks: packs below the split are marked for deletion, and packs above the split are ignored because they were retained earlier.
When doing a geometric repack without --combine-cruft-below-size, retain all cruft packs before marking anything for deletion. Geometric repacks do not rewrite cruft packs in that mode, so the common deletion path must not remove them.
--- builtin/repack.c | 11 +++++------ repack.c | 8 ++++++++ repack.h | 1 + 3 files changed, 14 insertions(+), 6 deletions(-)
builtin/repack.c +5 -6
@@ -574,10 +574,13 @@ int cmd_repack(int argc, packtmp);/* End of pack replacement. */ -if (delete_redundant && pack_everything &ALL_INTO_ONE ) { +if (delete_redundant) {if (write_midx ==REPACK_WRITE_MIDX_INCREMENTAL )existing_packs_retain_midx_packs (&existing, &geometry); -existing_packs_mark_for_deletion (&existing, &names); +if (geometry.split_factor && !combine_cruft_below_size) +existing_packs_retain_all_cruft (&existing); +if (pack_everything &ALL_INTO_ONE || geometry.split_factor) +existing_packs_mark_for_deletion (&existing, &names); }if (write_midx !=REPACK_WRITE_MIDX_NONE ) {
@@ -609,10 +612,6 @@ int cmd_repack(int argc,existing_packs_remove_redundant (&existing, packdir, wrote_incremental_midx); -if (geometry.split_factor) -pack_geometry_remove_redundant (&geometry, &names, - &existing, packdir, - wrote_incremental_midx);if (show_progress) opts |=PRUNE_PACKED_VERBOSE ;prune_packed_objects (opts);
repack.c +8 -0
@@ -242,6 +242,14 @@ static struct string_list_item *locate_existing_pack(struct string_list *list,return item; } +void existing_packs_retain_all_cruft (struct existing_packs *existing) +{ +struct string_list_item *item; + +for_each_string_list_item (item, &existing->cruft_packs) +existing_packs_mark_retained (item); +} +void existing_packs_retain_cruft (struct existing_packs *existing,struct packed_git *cruft) {
repack.h +1 -0
@@ -81,6 +81,7 @@ void existing_packs_collect(struct existing_packs *existing, const struct string_list *extra_keep);int existing_packs_has_non_kept (const struct existing_packs *existing);int existing_pack_is_marked_for_deletion (struct string_list_item *item); +void existing_packs_retain_all_cruft (struct existing_packs *existing);void existing_packs_retain_cruft (struct existing_packs *existing,struct packed_git *cruft); void existing_packs_retain_from_geometry(struct existing_packs *existing,
Signature · 1 line
2.55.0.rc2.10.g29e31820dce
RFC PATCH 6/10 repack-geometry: drop unused redundant-pack removal
In reply to Taylor BlauThe previous commit stopped using pack_geometry_remove_redundant() when deleting packs after a geometric repack. The existing_packs machinery now handles the same removal after geometric packs are marked for deletion.
Remove the unused geometry-specific helper and its declaration.
--- repack-geometry.c | 44 -------------------------------------------- repack.h | 5 ----- 2 files changed, 49 deletions(-)
repack-geometry.c +0 -44
@@ -245,50 +245,6 @@ struct packed_git *pack_geometry_preferred_pack(struct pack_geometry *geometry)return NULL ; } -static void remove_redundant_packs (struct packed_git **pack, -uint32_t pack_nr, -struct string_list *names, -struct existing_packs *existing, -const char *packdir, -bool wrote_incremental_midx) -{ -const struct git_hash_algo *algop = existing->repo->hash_algo; -struct strbuf buf =STRBUF_INIT ; -uint32_t i; - -for (i =0 ; i < pack_nr; i++) { -struct packed_git *p = pack[i]; -if (string_list_has_string (names,hash_to_hex_algop (p->hash, - algop))) -continue ; - -strbuf_reset (&buf); -strbuf_addstr (&buf,pack_basename (p)); -strbuf_strip_suffix (&buf,".pack" ); - -if ((p->pack_keep) || - (string_list_has_string (&existing->kept_packs, buf.buf))) -continue ; - -repack_remove_redundant_pack (existing->repo, packdir, buf.buf, - wrote_incremental_midx); - } - -strbuf_release (&buf); -} - -void pack_geometry_remove_redundant (struct pack_geometry *geometry, -struct string_list *names, -struct existing_packs *existing, -const char *packdir, -bool wrote_incremental_midx) -{ -remove_redundant_packs (geometry->pack, geometry->split, - names, existing, packdir, wrote_incremental_midx); -remove_redundant_packs (geometry->promisor_pack, geometry->promisor_split, - names, existing, packdir, wrote_incremental_midx); -} -void pack_geometry_release (struct pack_geometry *geometry) {if (!geometry)
repack.h +0 -5
@@ -134,11 +134,6 @@ void pack_geometry_init(struct pack_geometry *geometry, const struct pack_objects_args *args);void pack_geometry_split (struct pack_geometry *geometry);struct packed_git *pack_geometry_preferred_pack (struct pack_geometry *geometry); -void pack_geometry_remove_redundant (struct pack_geometry *geometry, -struct string_list *names, -struct existing_packs *existing, -const char *packdir, -bool wrote_incremental_midx);void pack_geometry_release (struct pack_geometry *geometry);struct tempfile ;
Signature · 1 line
2.55.0.rc2.10.g29e31820dce
RFC PATCH 7/10 pack-objects: extract `stdin_packs_add_all_pack_entries()`
In reply to Taylor BlauExtract the pack enumeration loop from stdin_packs_add_pack_entries() into a separate stdin_packs_add_all_pack_entries() helper, and have the caller dispatch to it based on the stdin_packs_mode.
This prepares for a subsequent commit which will introduce an alternate code path for '--stdin-packs=follow-reachable' that determines the set of objects to include via a reachability walk rather than eagerly adding all objects from included packs.
--- builtin/pack-objects.c | 49 ++++++++++++++++++++++++------------------ 1 file changed, 28 insertions(+), 21 deletions(-)
builtin/pack-objects.c +28 -21
@@ -3933,30 +3933,12 @@ static int stdin_packs_include_check(struct commit *commit, void *data)return stdin_packs_include_check_obj ((struct object *)commit, data); } -static void stdin_packs_add_pack_entries (struct strmap *packs, -struct rev_info *revs) +static void stdin_packs_add_all_pack_entries (struct string_list *keys, +struct rev_info *revs) { -struct string_list keys =STRING_LIST_INIT_NODUP ;struct string_list_item *item; -struct hashmap_iter iter; -struct strmap_entry *entry; -strmap_for_each_entry (packs, &iter, entry) { -struct stdin_pack_info *info = entry->value; -if (!info->p) -die (_ ("could not find pack '%s'" ), entry->key); - -string_list_append (&keys, entry->key)->util = info; - } - -/* -* Order packs by ascending mtime; use QSORT directly to access the -* string_list_item's ->util pointer, which string_list_sort() does not -* provide. -*/ -QSORT (keys.items, keys.nr, pack_mtime_cmp); - -for_each_string_list_item (item, &keys) { +for_each_string_list_item (item, keys) {struct stdin_pack_info *info = item->util;if (info->kind &STDIN_PACK_EXCLUDE_OPEN ) {
@@ -3977,6 +3959,31 @@ static void stdin_packs_add_pack_entries(struct strmap *packs, revs, ODB_FOR_EACH_OBJECT_PACK_ORDER); } +} + +static void stdin_packs_add_pack_entries (struct strmap *packs, +struct rev_info *revs) +{ +struct string_list keys =STRING_LIST_INIT_NODUP ; +struct hashmap_iter iter; +struct strmap_entry *entry; + +strmap_for_each_entry (packs, &iter, entry) { +struct stdin_pack_info *info = entry->value; +if (!info->p) +die (_ ("could not find pack '%s'" ), entry->key); + +string_list_append (&keys, entry->key)->util = info; + } + +/* +* Order packs by ascending mtime; use QSORT directly to access the +* string_list_item's ->util pointer, which string_list_sort() does not +* provide. +*/ +QSORT (keys.items, keys.nr, pack_mtime_cmp); + +stdin_packs_add_all_pack_entries (&keys, revs);string_list_clear (&keys,0 ); }
Signature · 1 line
2.55.0.rc2.10.g29e31820dce
RFC PATCH 8/10 pack-objects: introduce '--stdin-packs=follow-reachable'
In reply to Taylor BlauIntroduce a new '--stdin-packs=follow-reachable' mode. Like '--stdin-packs=follow', this mode recognizes the '!' (excluded-open) pack prefix and halts at '^' (excluded-closed) packs.
Unlike 'follow', which eagerly includes all objects from listed packs and then walks reachability to rescue additional objects, the new 'follow-reachable' mode uses reference tips as its traversal starting points and only includes objects that are both reachable AND belong to an included pack (or are reachable from a commit or tag in one):
- Objects in included packs: added to the output if reachable.
- Objects reachable from included-pack commits but in unknown packs:
added to the output (rescued).
- Objects in excluded-open ('!') packs: not included, but the traversal continues through them.
- Objects in excluded-closed ('^') packs: not included, and the traversal halts.
The implementation uses a two-phase approach:
1. In the first phase, commits and tags in included packs (and loose, when --unpacked is given) are marked with a flag bit (IN_INCLUDED_PACK). A commit-only walk from ref tips then identifies which marked objects are reachable, halting at excluded-closed packs.
2. In the second phase, every reachable marked object (from the previous step) becomes a tip for a full object traversal whose show_object_pack_hint() and show_commit_pack_hint() callbacks add discovered objects (obeying the usual constraints imposed by want_object_in_pack()).
When '--unpacked' is given, reachable loose objects are included in the output while unreachable loose objects are left alone. This is achieved by marking loose commits and tags with IN_INCLUDED_PACK during the first phase, so the pre-walk discovers them naturally.
--- Documentation/git-pack-objects.adoc | 17 +++ builtin/pack-objects.c | 185 +++++++++++++++++++++++-- t/t5331-pack-objects-stdin.sh | 201 ++++++++++++++++++++++++++++ 3 files changed, 393 insertions(+), 10 deletions(-)
Documentation/git-pack-objects.adoc +17 -0
@@ -113,6 +113,23 @@ This mode is useful, for example, to resurrect once-unreachable objects found in cruft packs to generate packs which are closed under reachability up to the boundary set by the excluded packs. + +When `mode` is "follow-reachable", the same pack prefixes are recognized +as in "follow" (`!` for excluded-open, `^` for excluded-closed). However, +instead of including all objects from included packs, only objects that +are reachable from reference tips AND belong to an included pack (or are +reachable from a commit in one) are included. Objects in excluded-open +packs are traversed but not included; objects in excluded-closed packs +halt the traversal. ++ +This mode is designed for geometric repacking with cruft packs, where +the output pack should contain only reachable objects so that unreachable +ones can be collected separately. ++ +When `--unpacked` is given alongside `--stdin-packs=follow-reachable`, +reachable loose objects are also included in the output pack, while +unreachable loose objects are left alone. This includes both loose +commits and annotated tag objects. ++ Incompatible with `--revs`, or options that imply `--revs` (such as `--all`), with the exception of `--unpacked`, which is compatible.
builtin/pack-objects.c +175 -10
256 lines not on this page · raw message
t/t5331-pack-objects-stdin.sh +201 -0
205 lines not on this page · raw message
Signature · 1 line
2.55.0.rc2.10.g29e31820dce
Re: [RFC PATCH 08/10] pack-objects: introduce '--stdin-packs=follow-reachable'
Quoted 14 lines · Taylor Blau
builtin/pack-objects.c +14
-0+/* +* Flag bit set on commits that belong to an included pack during +* '--stdin-packs=follow-reachable'. Used by the pre-walk to +* identify which reachable commits should be tips for the main +* object traversal. +*/ +#define IN_INCLUDED_PACK (1u<<11) + +static int mark_included_pack_tip (const struct object_id *oid, +struct packed_git *p, +uint32_t pos, +void *data) +{ +struct rev_info *main_revs = data;
builtin/pack-objects.c static int stdin_packs_include_check(struct commit *commit, void *data) · line 3962
Quoted 10 lines
builtin/pack-objects.c +10
-0+off_t ofs =nth_packed_object_offset (p, pos); +enum object_type type; +struct object_info oi =OBJECT_INFO_INIT ; +struct object *obj; + + oi.typep = &type; +if (packed_object_info (p, ofs, &oi) <0 ) +return 0 ; +if (type !=OBJ_COMMIT && type !=OBJ_TAG ) +return 0 ;
We do not care about non commits, non tags.
builtin/pack-objects.c static int stdin_packs_include_check(struct commit *commit, void *data) · line 3971
Quoted 8 lines
builtin/pack-objects.c +8
-0+ obj =parse_object (the_repository, oid); +if (!obj) +return 0 ; + + obj->flags |=IN_INCLUDED_PACK ; + +if (type ==OBJ_TAG && main_revs) +add_pending_object (main_revs, obj,"" );
Any tag object is added to the pending list of the second phase traversal here. Doesn't this retain unreachable tags and (unreachable) objects that are only reachable from these unreachable tags found in the packfile? Don't we want to limit this code to add only tags that actually are reachable from refs, or something?
Review
Quoted 2 lines
(quoted diff) +2
-0+ return 0; +}
The other function ...
builtin/pack-objects.c static int stdin_packs_include_check(struct commit *commit, void *data) · line 3977
Quoted 3 lines
builtin/pack-objects.c +3
-0+static int mark_loose_object_tip (const struct object_id *oid, + struct object_info *oi UNUSED, +void *data)
... is structured in a very similar way, and gives the same puzzlement to me.
Re: [RFC PATCH 08/10] pack-objects: introduce '--stdin-packs=follow-reachable'
Quoted 20 lines · Junio C Hamano
Quoted 18 lines · Taylor Blau
builtin/pack-objects.c +18
-0+/* +* Flag bit set on commits that belong to an included pack during +* '--stdin-packs=follow-reachable'. Used by the pre-walk to +* identify which reachable commits should be tips for the main +* object traversal. +*/ +#define IN_INCLUDED_PACK (1u<<11) + +static int mark_included_pack_tip (const struct object_id *oid, +struct packed_git *p, +uint32_t pos, +void *data) +{ +struct rev_info *main_revs = data; +off_t ofs =nth_packed_object_offset (p, pos); +enum object_type type; +struct object_info oi =OBJECT_INFO_INIT ; +struct object *obj;
Quoted 8 lines
builtin/pack-objects.c static int stdin_packs_include_check(struct commit *commit, void *data) · line 3962
Quoted 6 lines
builtin/pack-objects.c +6
-0+ + oi.typep = &type; +if (packed_object_info (p, ofs, &oi) <0 ) +return 0 ; +if (type !=OBJ_COMMIT && type !=OBJ_TAG ) +return 0 ;We do not care about non commits, non tags.
This should not be an &&, but rather an ||. We only want to handle objects which are either commits *or* tags via this function.
Quoted 6 lines
Quoted 6 lines
builtin/pack-objects.c +6
-0+ obj =parse_object (the_repository, oid); +if (!obj) +return 0 ; + + obj->flags |=IN_INCLUDED_PACK ; +
Quoted 8 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+ if (type == OBJ_TAG && main_revs) + add_pending_object(main_revs, obj, "");Any tag object is added to the pending list of the second phase traversal here. Doesn't this retain unreachable tags and (unreachable) objects that are only reachable from these unreachable tags found in the packfile? Don't we want to limit this code to add only tags that actually are reachable from refs, or something?
Hmm. You're right, if we have an unreachable annotated tag object in one of the STDIN_PACK_INCLUDE packs, then we'll pick it up and place it in the non-cruft pack.
I'd have to think more about how to handle this properly to ensure that *only* reachable objects are included. We probably want to re-order this code so that we start a traversal from the reference tips, then mark reachable objects as starting points based on whether or not they appear in an included pack (and correspondingly do not appear in an excluded one).
That adjusts how the traversal would work, I think, since we should be able to pick up the resulting set of objects in a single pass. But I'd have to think more about it...
Thanks,
Taylor
Re: [RFC PATCH 08/10] pack-objects: introduce '--stdin-packs=follow-reachable'
Quoted 9 lines · Taylor Blau
Quoted 6 lines
builtin/pack-objects.c static int stdin_packs_include_check(struct commit *commit, void *data) · line 3962
Quoted 4 lines
builtin/pack-objects.c +4
-0+if (packed_object_info (p, ofs, &oi) <0 ) +return 0 ; +if (type !=OBJ_COMMIT && type !=OBJ_TAG ) +return 0 ;We do not care about non commits, non tags.
This should not be an &&, but rather an ||. We only want to handle objects which are either commits *or* tags via this function.
My comment above did not mean to say anything is wrong in the code; I was just thinking aloud. We return for blob or tree because they are not commit and they are not tag. If we say "||" then we return for everybody, because anything that is a commit (failing the LHS of the "||") cannot be a tag (satisfying the RHS of the "||").
Re: [RFC PATCH 08/10] pack-objects: introduce '--stdin-packs=follow-reachable'
Quoted 9 lines · Junio C Hamano
Quoted 7 lines · Taylor Blau
Quoted 6 lines
builtin/pack-objects.c static int stdin_packs_include_check(struct commit *commit, void *data) · line 3962
Quoted 4 lines
builtin/pack-objects.c +4
-0+if (packed_object_info (p, ofs, &oi) <0 ) +return 0 ; +if (type !=OBJ_COMMIT && type !=OBJ_TAG ) +return 0 ;We do not care about non commits, non tags.
Quoted 6 lines
Quoted 2 lines
This should not be an &&, but rather an ||. We only want to handle objects which are either commits *or* tags via this function.
My comment above did not mean to say anything is wrong in the code; I was just thinking aloud. We return for blob or tree because they are not commit and they are not tag. [...]
Absolutely. Sorry about that, I have no idea how I tricked myself into misreading the patch here. The second sentence of what I wrote above is correct, but not the first.
Thanks,
Taylor
RFC PATCH 9/10 pack-objects: support '--refs-snapshot' with 'follow-reachable'
In reply to Taylor BlauThe '--stdin-packs=follow-reachable' mode walks from reference tips to determine which objects in included packs are reachable. Without a snapshot, pack-objects discovers refs by iterating live references, which may change between the time the repack writes the geometric pack and the time it writes the MIDX bitmap.
If a reference is updated during that window, the set of reachable objects seen by pack-objects may differ from the set seen by the MIDX bitmap writer. This can cause reachable objects to end up in the cruft pack (because pack-objects did not see the reference that makes them reachable) rather than the geometric pack. While this does not cause data loss, it has two undesirable consequences:
- Reachable objects in the cruft pack cannot receive bitmap coverage (since the cruft pack may be excluded from the MIDX when 'repack.midxMustContainCruft' is false).
- Serving fetches that need those objects requires loading the cruft pack, which may contain many unrelated unreachable objects.
To avoid this, teach pack-objects to accept '--refs-snapshot=<path>' when used with '--stdin-packs=follow-reachable'. The snapshot file uses the same format as the MIDX bitmap writer: one hex OID per line, with an optional '+' prefix for preferred bitmap commits.
'pack-objects' happily ignores the '+' prefix for indicating preferred bitmap commits as a convenience, so that the ref-snapshot can be shared between the MIDX generation machinery and 'pack-objects'.
--- Documentation/git-pack-objects.adoc | 8 +++++ builtin/pack-objects.c | 46 +++++++++++++++++++++++++++-- 2 files changed, 52 insertions(+), 2 deletions(-)
Documentation/git-pack-objects.adoc +8 -0
@@ -133,6 +133,14 @@ commits and annotated tag objects. Incompatible with `--revs`, or options that imply `--revs` (such as `--all`), with the exception of `--unpacked`, which is compatible. +--refs-snapshot=<path>:: + When used with `--stdin-packs=follow-reachable`, read reference + tips from `<path>` instead of iterating live references. The file + format is one hex object ID per line, with an optional `+` prefix + (for preferred bitmap commits). This ensures a consistent view of + references when the same snapshot is shared with other tools (e.g., + the MIDX bitmap writer). + --cruft:: Packs unreachable objects into a separate "cruft" pack, denoted by the existence of a `.mtimes` file. Typically used by `git
builtin/pack-objects.c +44 -2
76 lines not on this page · raw message
Signature · 1 line
2.55.0.rc2.10.g29e31820dce
RFC PATCH 10/10 repack: support combining '--geometric' with '--cruft'
In reply to Taylor BlauTeach 'git repack' to accept '--geometric' and '--cruft' together. When both are given, the geometric repack rolls up non-cruft packs as usual, and a separate cruft pack is written to collect unreachable objects.
Previously, '--cruft' implied ALL_INTO_ONE, which is fundamentally incompatible with geometric repacking. Relax this so that '--cruft' only implies ALL_INTO_ONE when '--geometric' is not also given.
When combining the two modes:
- Use the new '--stdin-packs=follow-reachable' mode so that only reachable objects from the rolled-up packs (and any reachable loose objects) appear in the geometric pack. Unreachable objects are left for the cruft writer to collect.
- Plumb our pack_geometry into write_cruft_pack(), so that the latter can tell 'pack-objects' which non-kept packs are below the split (excluded, so their unreachable objects are candidates for the cruft pack) versus above the split (included, so they are treated as reachable).
- Handle promisor packs in the cruft writer's geometry path, since promisor packs have their own split point.
- Use the refs snapshot (when available) so that pack-objects and the MIDX bitmap writer see the same set of reference tips.
--- Documentation/git-repack.adoc | 11 ++ builtin/repack.c | 23 +++- repack-cruft.c | 23 +++- repack.h | 3 +- t/t7704-repack-cruft.sh | 251 ++++++++++++++++++++++++++++++++++ 5 files changed, 300 insertions(+), 11 deletions(-)
Documentation/git-repack.adoc +11 -0
@@ -70,6 +70,11 @@ to the new separate pack will be written. are packed into a separate cruft pack. Unreachable objects can be pruned using the normal expiry rules with the next `git gc` invocation (see linkgit:git-gc[1]). Incompatible with `-k`. ++ +When combined with `--geometric`, `--cruft` does not imply `-a`. Instead, +the geometric repack rolls up packs as usual, and a separate cruft pack is +written to collect unreachable objects. Only reachable objects from the +rolled-up packs are included in the resulting geometric pack. --cruft-expiration=<approxidate>:: Expire unreachable objects older than `<approxidate>`
@@ -245,6 +250,12 @@ progression. Loose objects are implicitly included in this "roll-up", without respect to their reachability. This is subject to change in the future. + +When combined with `--cruft`, only reachable objects from rolled-up packs +are included in the geometric pack, along with any reachable loose objects. +Unreachable objects (both from rolled-up packs and loose) are collected +into a separate cruft pack. Existing cruft packs are retained. See +`--cruft` above for details. ++ When writing a multi-pack bitmap, `git repack` selects the largest resulting pack as the preferred pack for object selection by the MIDX (see linkgit:git-multi-pack-index[1]).
builtin/repack.c +16 -7
@@ -260,7 +260,7 @@ int cmd_repack(int argc, keep_unreachable,"-k/--keep-unreachable" , pack_everything & PACK_CRUFT, "--cruft"); -if (pack_everything &PACK_CRUFT ) +if (pack_everything &PACK_CRUFT && !geometry.split_factor) pack_everything |=ALL_INTO_ONE ;if (write_bitmaps <0 ) {
@@ -296,7 +296,8 @@ int cmd_repack(int argc,die (_ ("invalid value for %s: %d" ),"--midx-new-layer-threshold" , config_ctx.midx_new_layer_threshold); -if (write_midx !=REPACK_WRITE_MIDX_NONE && write_bitmaps) { +if ((write_midx !=REPACK_WRITE_MIDX_NONE && write_bitmaps) || + (geometry.split_factor && (pack_everything &PACK_CRUFT ))) {struct strbuf path =STRBUF_INIT ; strbuf_addf(&path, "%s/%s_XXXXXX",
@@ -317,7 +318,7 @@ int cmd_repack(int argc,existing_packs_collect (&existing, &keep_pack_list);if (geometry.split_factor) { -if (pack_everything) +if (pack_everything & ~PACK_CRUFT )die (_ ("options '%s' and '%s' cannot be used together" ),"--geometric" ,"-A/-a" );if (write_midx ==REPACK_WRITE_MIDX_INCREMENTAL ) { geometry.midx_layer_threshold = config_ctx.midx_new_layer_threshold;
@@ -393,10 +394,16 @@ int cmd_repack(int argc,pack_geometry_repack_promisors (repo, &po_args, &geometry, &names, packtmp); -if (midx_must_contain_cruft) +if (pack_everything &PACK_CRUFT ) { +strvec_push (&cmd.args,"--stdin-packs=follow-reachable" ); +if (refs_snapshot) +strvec_pushf (&cmd.args,"--refs-snapshot=%s" , +get_tempfile_path (refs_snapshot)); + }else if (midx_must_contain_cruft)strvec_push (&cmd.args,"--stdin-packs" );else strvec_push (&cmd.args,"--stdin-packs=follow" ); +strvec_push (&cmd.args,"--unpacked" ); }else {strvec_push (&cmd.args,"--unpacked" );
@@ -431,7 +438,8 @@ int cmd_repack(int argc,const char *basename =pack_basename (geometry.pack[i]);char marker ='^' ; -if (!midx_must_contain_cruft && +if ((pack_everything &PACK_CRUFT || + !midx_must_contain_cruft) && !string_list_has_string (&existing.midx_packs, basename)) {/*
@@ -505,7 +513,8 @@ int cmd_repack(int argc, ret =write_cruft_pack (&opts, cruft_expiration, combine_cruft_below_size, &names, - &existing); + &existing, + geometry.split_factor ? &geometry :NULL );if (ret) goto cleanup;
@@ -540,7 +549,7 @@ int cmd_repack(int argc,*/ opts.destination = expire_to; ret =write_cruft_pack (&opts,NULL ,0ul , &names, - &existing); + &existing,NULL );if (ret) goto cleanup; }
repack-cruft.c +20 -3
@@ -36,7 +36,8 @@ int write_cruft_pack(const struct write_pack_opts *opts,const char *cruft_expiration, unsigned long combine_cruft_below_size, struct string_list *names, - struct existing_packs *existing) + struct existing_packs *existing, + struct pack_geometry *geometry) { struct child_process cmd = CHILD_PROCESS_INIT;struct string_list_item *item;
@@ -81,8 +82,24 @@ int write_cruft_pack(const struct write_pack_opts *opts,else for_each_string_list_item (item, &existing->cruft_packs)fprintf (in,"-%s.pack\n" , item->string); -for_each_string_list_item (item, &existing->non_kept_packs) -fprintf (in,"-%s.pack\n" , item->string); +if (geometry) { +uint32_t j; +for (j =0 ; j < geometry->split; j++) +fprintf (in,"-%s\n" , +pack_basename (geometry->pack[j])); +for (; j < geometry->pack_nr; j++) +fprintf (in,"%s\n" , +pack_basename (geometry->pack[j])); +for (j =0 ; j < geometry->promisor_split; j++) +fprintf (in,"-%s\n" , +pack_basename (geometry->promisor_pack[j])); +for (; j < geometry->promisor_pack_nr; j++) +fprintf (in,"%s\n" , +pack_basename (geometry->promisor_pack[j])); + }else { +for_each_string_list_item (item, &existing->non_kept_packs) +fprintf (in,"-%s.pack\n" , item->string); + }for_each_string_list_item (item, &existing->kept_packs)fprintf (in,"%s.pack\n" , item->string);fclose (in);
repack.h +2 -1
@@ -169,6 +169,7 @@ int write_cruft_pack(const struct write_pack_opts *opts,const char *cruft_expiration, unsigned long combine_cruft_below_size, struct string_list *names, - struct existing_packs *existing); + struct existing_packs *existing, + struct pack_geometry *geometry);#endif /* REPACK_H */
t/t7704-repack-cruft.sh +251 -0
255 lines not on this page · raw message
Signature · 1 line
2.55.0.rc2.10.g29e31820dce
Quoted 3 lines · Taylor Blau
First, a short note. This series is an RFC because I have not had the chance to review and test it as thoroughly as I normally would, and because we are deep in the -rc phase.
And a release was made. Has the topic had a chance to be further polished, or did it turn out that it was perfect to begin with?