repack: prepare for incremental MIDX-based repacking
Subsystems (2)
PATCH v2 46/49 repack: move `pack_kept_objects` to `struct pack_objects_args`
In reply to Taylor BlauThe "pack_kept_objects" variable is defined as static to the repack builtin, but is inherently related to the pack-objects arguments that the builtin uses when generating new packs.
Move that field into the "struct pack_objects_args", and shuffle around where we append the corresponding command-line option when preparing a pack-objects process. Specifically:
- write_cruft_pack() always wants to pass "--honor-pack-keep", so explicitly set the pack_kept_objects field to "0" when initializing the write_pack_opts struct before calling write_cruft_pack().
- write_filtered_pack() no longer needs to handle writing the command-line option "--honor-pack-keep" when preparing a pack-objects process, since its call to prepare_pack_objects() will have already taken care of that.
write_filtered_pack() also reads the pack_kept_objects field to determine whether to write the existing kept packs with a leading "^" character, so update that to read through the po_args pointer instead.
- cmd_repack() also no longer has to write the "--honor-pack-keep" flag explicitly, since this is also handled via its call to prepare_pack_objects().
Since there is a default value for "pack_kept_objects" that relies on whether or not we are writing a bitmap (and not writing a MIDX), extract a default initializer for struct pack_objects_args that keeps this conditional default behavior.
--- builtin/repack.c | 20 +++++++------------- repack-geometry.c | 5 ++--- repack.c | 2 ++ repack.h | 9 ++++++--- 4 files changed, 17 insertions(+), 19 deletions(-)
builtin/repack.c +7 -13
@@ -33,7 +33,6 @@#define RETAIN_PACK 2static int pack_everything; -static int pack_kept_objects =-1 ;static int write_bitmaps =-1 ;static int use_delta_islands;static int run_update_server_info =1 ;
@@ -68,7 +67,7 @@ static int repack_config(const char *var, const char *value,return 0 ; }if (!strcmp (var,"repack.packkeptobjects" )) { - pack_kept_objects =git_config_bool (var, value); + po_args->pack_kept_objects =git_config_bool (var, value);return 0 ; } if (!strcmp(var, "repack.writebitmaps") ||
@@ -122,8 +121,6 @@ static int write_filtered_pack(const struct write_pack_opts *opts,strvec_push (&cmd.args,"--stdin-packs" ); -if (!pack_kept_objects) -strvec_push (&cmd.args,"--honor-pack-keep" );for_each_string_list_item (item, &existing->kept_packs)strvec_pushf (&cmd.args,"--keep-pack=%s" , item->string);
@@ -146,7 +143,7 @@ static int write_filtered_pack(const struct write_pack_opts *opts,fprintf (in,"%s.pack\n" , item->string);for_each_string_list_item (item, &existing->cruft_packs)fprintf (in,"%s.pack\n" , item->string); - caret = pack_kept_objects ?"" :"^" ; + caret = opts->po_args->pack_kept_objects ?"" :"^" ;for_each_string_list_item (item, &existing->kept_packs)fprintf (in,"%s%s.pack\n" , caret, item->string);fclose (in);
@@ -208,7 +205,6 @@ static int write_cruft_pack(const struct write_pack_opts *opts,strvec_pushf (&cmd.args,"--cruft-expiration=%s" , cruft_expiration); -strvec_push (&cmd.args,"--honor-pack-keep" );strvec_push (&cmd.args,"--non-empty" ); cmd.in =-1 ;
@@ -332,7 +328,7 @@ int cmd_repack(int argc,OPT_UNSIGNED (0 ,"max-pack-size" , &po_args.max_pack_size,N_ ("maximum size of each packfile" )),OPT_PARSE_LIST_OBJECTS_FILTER (&po_args.filter_options), -OPT_BOOL (0 ,"pack-kept-objects" , &pack_kept_objects, +OPT_BOOL (0 ,"pack-kept-objects" , &po_args.pack_kept_objects,N_ ("repack objects in packs marked with .keep" )),OPT_STRING_LIST (0 ,"keep-pack" , &keep_pack_list,N_ ("name" ), N_("do not repack this pack")),
@@ -378,8 +374,8 @@ int cmd_repack(int argc, (!(pack_everything & ALL_INTO_ONE) || !is_bare_repository())) write_bitmaps =0 ; } -if (pack_kept_objects <0 ) - pack_kept_objects = write_bitmaps >0 && !write_midx; +if (po_args.pack_kept_objects <0 ) + po_args.pack_kept_objects = write_bitmaps >0 && !write_midx;if (write_bitmaps && !(pack_everything &ALL_INTO_ONE ) && !write_midx)die (_ (incremental_bitmap_conflict_error));
@@ -420,8 +416,7 @@ int cmd_repack(int argc,if (geometry.split_factor) {if (pack_everything)die (_ ("options '%s' and '%s' cannot be used together" ),"--geometric" ,"-A/-a" ); -pack_geometry_init (&geometry, &existing, &po_args, - pack_kept_objects); +pack_geometry_init (&geometry, &existing, &po_args);pack_geometry_split (&geometry); }
@@ -430,8 +425,6 @@ int cmd_repack(int argc, show_progress = !po_args.quiet &&isatty (2 );strvec_push (&cmd.args,"--keep-true-parents" ); -if (!pack_kept_objects) -strvec_push (&cmd.args,"--honor-pack-keep" );for (i =0 ; i < keep_pack_list.nr; i++)strvec_pushf (&cmd.args,"--keep-pack=%s" , keep_pack_list.items[i].string);
@@ -581,6 +574,7 @@ int cmd_repack(int argc, cruft_po_args.local = po_args.local; cruft_po_args.quiet = po_args.quiet; cruft_po_args.delta_base_offset = po_args.delta_base_offset; + cruft_po_args.pack_kept_objects =0 ; ret = write_cruft_pack(&opts, cruft_expiration, combine_cruft_below_size, &names,
repack-geometry.c +2 -3
@@ -27,8 +27,7 @@ static int pack_geometry_cmp(const void *va, const void *vb)void pack_geometry_init (struct pack_geometry *geometry,struct existing_packs *existing, -const struct pack_objects_args *args, -int pack_kept_objects) +const struct pack_objects_args *args) {struct packfile_store *packs = existing->repo->objects->packfiles;struct packed_git *p;
@@ -43,7 +42,7 @@ void pack_geometry_init(struct pack_geometry *geometry,*/ continue ; -if (!pack_kept_objects) { +if (!args->pack_kept_objects) {/* * Any pack that has its pack_keep bit set will * appear in existing->kept_packs below, but
repack.c +2 -0
@@ -38,6 +38,8 @@ void prepare_pack_objects(struct child_process *cmd,strvec_push (&cmd->args,"--quiet" );if (args->delta_base_offset)strvec_push (&cmd->args,"--delta-base-offset" ); +if (!args->pack_kept_objects) +strvec_push (&cmd->args,"--honor-pack-keep" );strvec_push (&cmd->args, out); cmd->git_cmd =1 ; cmd->out =-1 ;
repack.h +6 -3
@@ -17,10 +17,14 @@ struct pack_objects_args {int name_hash_version;int path_walk;int delta_base_offset; +int pack_kept_objects;struct list_objects_filter_options filter_options; }; -#define PACK_OBJECTS_ARGS_INIT { .delta_base_offset = 1 } +#define PACK_OBJECTS_ARGS_INIT { \ + .delta_base_offset = 1, \ + .pack_kept_objects = -1, \ +}struct child_process ;
@@ -104,8 +108,7 @@ struct pack_geometry {void pack_geometry_init (struct pack_geometry *geometry,struct existing_packs *existing, -const struct pack_objects_args *args, -int pack_kept_objects); +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,
Signature · 1 line
2.51.0.540.ga7423965ad8
PATCH v2 47/49 repack: move `write_filtered_pack()` out of the builtin
In reply to Taylor BlauIn a similar fashion as in previous commits, move the function write_filtered_pack() out of the builtin and into its own compilation unit.
This function is now part of the repack.h API, but implemented in its own "repack-filtered.c" unit as it is a separate component from other kinds of repacking operations.
--- Makefile | 1 + builtin/repack.c | 46 ------------------------------------------ meson.build | 1 + repack-filtered.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++ repack.h | 4 ++++ 5 files changed, 57 insertions(+), 46 deletions(-) create mode 100644 repack-filtered.c
Makefile +1 -0
@@ -1250,6 +1250,7 @@ LIB_OBJS += refs/ref-cache.o LIB_OBJS += refspec.o LIB_OBJS += remote.o LIB_OBJS += repack.o +LIB_OBJS += repack-filtered.o LIB_OBJS += repack-geometry.o LIB_OBJS += repack-midx.o LIB_OBJS += repack-promisor.o
builtin/repack.c +0 -46
52 lines not on this page · raw message
meson.build +1 -0
@@ -463,6 +463,7 @@ libgit_sources = [ 'reftable/writer.c', 'remote.c', 'repack.c', + 'repack-filtered.c', 'repack-geometry.c', 'repack-midx.c', 'repack-promisor.c',
repack-filtered.c new file +51 -0
51 lines not on this page · raw message
repack.h +4 -0
@@ -133,4 +133,8 @@ struct repack_write_midx_opts {void midx_snapshot_refs (struct repository *repo,struct tempfile *f);int write_midx_included_packs (struct repack_write_midx_opts *opts); +int write_filtered_pack (const struct write_pack_opts *opts, +struct existing_packs *existing, +struct string_list *names); +#endif /* REPACK_H */
Signature · 1 line
2.51.0.540.ga7423965ad8
PATCH v2 48/49 repack: move `write_cruft_pack()` out of the builtin
In reply to Taylor BlauIn an identical fashion as the previous commit, move the function write_cruft_pack() into its own compilation unit, and make the function visible through the repack.h API.
--- Makefile | 1 + builtin/repack.c | 94 --------------------------------------------- meson.build | 1 + repack-cruft.c | 99 ++++++++++++++++++++++++++++++++++++++++++++++++ repack.h | 6 +++ 5 files changed, 107 insertions(+), 94 deletions(-) create mode 100644 repack-cruft.c
Makefile +1 -0
@@ -1250,6 +1250,7 @@ LIB_OBJS += refs/ref-cache.o LIB_OBJS += refspec.o LIB_OBJS += remote.o LIB_OBJS += repack.o +LIB_OBJS += repack-cruft.o LIB_OBJS += repack-filtered.o LIB_OBJS += repack-geometry.o LIB_OBJS += repack-midx.o
builtin/repack.c +0 -94
100 lines not on this page · raw message
meson.build +1 -0
@@ -463,6 +463,7 @@ libgit_sources = [ 'reftable/writer.c', 'remote.c', 'repack.c', + 'repack-cruft.c', 'repack-filtered.c', 'repack-geometry.c', 'repack-midx.c',
repack-cruft.c new file +99 -0
99 lines not on this page · raw message
repack.h +6 -0
@@ -137,4 +137,10 @@ int write_filtered_pack(const struct write_pack_opts *opts,struct existing_packs *existing, struct string_list *names); +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); +#endif /* REPACK_H */
Signature · 1 line
2.51.0.540.ga7423965ad8
PATCH v2 49/49 builtin/repack.c: clean up unused `#include`s
In reply to Taylor BlauOver the past several dozen commits, we have moved a large amount of functionality out of the repack builtin and into other files like repack.c, repack-cruft.c, repack-filtered.c, repack-midx.c, and repack-promisor.c.
These files specify the minimal set of #includes that they need to compile successfully, but we did not change the set of #includes in the repack builtin itself.
Now that the code movement is complete, let's clean up that set of #includes and trim down the builtin to include the minimal amount of external headers necessary to compile.
--- builtin/repack.c | 9 --------- 1 file changed, 9 deletions(-)
builtin/repack.c +0 -9
@@ -4,26 +4,17 @@#include "builtin.h" #include "config.h" #include "environment.h" -#include "gettext.h" -#include "hex.h" #include "parse-options.h" #include "path.h" #include "run-command.h" #include "server-info.h" -#include "strbuf.h" #include "string-list.h" -#include "strvec.h" #include "midx.h" #include "packfile.h" #include "prune-packed.h" -#include "odb.h" #include "promisor-remote.h" #include "repack.h" #include "shallow.h" -#include "pack.h" -#include "pack-bitmap.h" -#include "refs.h" -#include "list-objects-filter-options.h" #define ALL_INTO_ONE 1#define LOOSEN_UNREACHABLE 2
Signature · 1 line
2.51.0.540.ga7423965ad8
Quoted 24 lines · Taylor Blau
Note to the maintainer:
* This series has been rebased on to 'master' (which is
4b71b29477(The seventeenth batch, 2025-10-10) at the time of writing) to avoid semantic conflicts now that ps/packfile-store has been merged.This is a relatively small reroll of my series to clean up the repack builtin by introducing a
repack.hAPI, which is the first of ~three series that will implement incremental MIDX/bitmap-based repacking.A range-diff is included below for convenience, but the changes since v1 are generally limited to the following:
* Wording tweaks and a couple of minor typo fixes.
* Dropping explicit casts out of 'void *'.
* Clarification in commit "
builtin/repack.c: introducestruct write_pack_opts" that additional cleanup follows in the upcoming patches.* Marking parts of the new API as const where possible.
Quoted 8 lines
* Using 'bool' as the return type and simplifying the implementation of
write_pack_opts_is_local().* Avoid shadowing "struct
write_pack_optsopts" incmd_repack().Outside of that, the series is unchanged, and I am hopeful that this round looks good to reviewers so that we can move on to the more interesting parts of incremental MIDX/bitmap repacking ;-).
I didn't quite feel like reviewing all of these patches again, so I only had a look at the range-diff. The changes in there all look good to me and address my feedback.
So I feel like all of this is in a reasonably good shape and a definitive improvement for our code base. Thanks!
Patrick
Quoted 22 lines · Patrick Steinhardt
Quoted 13 lines
A range-diff is included below for convenience, but the changes since v1 are generally limited to the following:
* Wording tweaks and a couple of minor typo fixes.
* Dropping explicit casts out of 'void *'.
* Clarification in commit "
builtin/repack.c: introducestruct write_pack_opts" that additional cleanup follows in the upcoming patches.* Marking parts of the new API as const where possible.
Quoted 8 lines
* Using 'bool' as the return type and simplifying the implementation of
write_pack_opts_is_local().* Avoid shadowing "struct
write_pack_optsopts" incmd_repack().Outside of that, the series is unchanged, and I am hopeful that this round looks good to reviewers so that we can move on to the more interesting parts of incremental MIDX/bitmap repacking ;-).
Quoted 6 lines
I didn't quite feel like reviewing all of these patches again, so I only had a look at the range-diff. The changes in there all look good to me and address my feedback.
So I feel like all of this is in a reasonably good shape and a definitive improvement for our code base. Thanks!
;-)
Thanks for a vote of confidence. With what I saw in v1 round, I agree these are in good shape.
Thanks.
Quoted 11 lines · Junio C Hamano
Quoted 6 lines
I didn't quite feel like reviewing all of these patches again, so I only had a look at the range-diff. The changes in there all look good to me and address my feedback.
So I feel like all of this is in a reasonably good shape and a definitive improvement for our code base. Thanks!
;-)
Thanks for a vote of confidence. With what I saw in v1 round, I agree these are in good shape.
Thanks, both :-).
Thanks,
Taylor
Quoted 11 lines · Junio C Hamano
Quoted 6 lines
I didn't quite feel like reviewing all of these patches again, so I only had a look at the range-diff. The changes in there all look good to me and address my feedback.
So I feel like all of this is in a reasonably good shape and a definitive improvement for our code base. Thanks!
;-)
Thanks for a vote of confidence. With what I saw in v1 round, I agree these are in good shape.
I likewise looked over the range-diff, and it covered all of my actionable comments from the v1 review.
-Peff