Here is a small reroll of my series to clean up some of the internals of
'git repack' used to track the set of existing packs.
Much is unchanged from the last round, save for some additional clean-up
on how we handle the '->util' field for each pack's string_list_item in
response to very helpful review from those CC'd.
As usual, a range-diff is available below for convenience. Thanks in
advance for your review!
Taylor Blau (8):
builtin/repack.c: extract structure to store existing packs
builtin/repack.c: extract marking packs for deletion
builtin/repack.c: extract redundant pack cleanup for --geometric
builtin/repack.c: extract redundant pack cleanup for existing packs
builtin/repack.c: extract `has_existing_non_kept_packs()`
builtin/repack.c: store existing cruft packs separately
builtin/repack.c: avoid directly inspecting "util"
builtin/repack.c: extract common cruft pack loop
builtin/repack.c | 293 +++++++++++++++++++++++++++++------------------
1 file changed, 180 insertions(+), 113 deletions(-)
Range-diff against v1:
1: 5b48b7e3cc = 1: 2e26beff22 builtin/repack.c: extract structure to store existing packs
2: 313537ef68 = 2: 62d916169d builtin/repack.c: extract marking packs for deletion
3: 5c25ef87c1 = 3: 7ed45804ea builtin/repack.c: extract redundant pack cleanup for --geometric
4: 7bb543fef8 = 4: 82057de4cf builtin/repack.c: extract redundant pack cleanup for existing packs
5: e2cf87bb94 = 5: f4f7b4c08f builtin/repack.c: extract `has_existing_non_kept_packs()`
6: 414a558883 = 6: d68a88dbd5 builtin/repack.c: store existing cruft packs separately
7: 559b487e2a ! 7: 481a29599b builtin/repack.c: drop `DELETE_PACK` macro
@@ Metadata
Author: Taylor Blau <me@xxxxxxxxxxxx>
## Commit message ##
- builtin/repack.c: drop `DELETE_PACK` macro
+ builtin/repack.c: avoid directly inspecting "util"
+ The `->util` field corresponding to each string_list_item is used to
+ track the existence of some pack at the beginning of a repack operation
+ was originally intended to be used as a bitfield.
+
+ This bitfield tracked:
+
+ - (1 << 0): whether or not the pack should be deleted
+ - (1 << 1): whether or not the pack is cruft
+
+ The previous commit removed the use of the second bit, but a future
+ patch (from a different series than this one) will introduce a new use
+ of it.
+
+ So we could stop treating the util pointer as a bitfield and instead
+ start treating it as if it were a boolean. But this would require some
+ backtracking when that later patch is applied.
+
+ Instead, let's avoid touching the ->util field directly, and instead
+ introduce convenience functions like:
+
+ - pack_mark_for_deletion()
+ - pack_is_marked_for_deletion()
+
+ Helped-by: Junio C Hamano <gitster@xxxxxxxxx>
+ Helped-by: Jeff King <peff@xxxxxxxx>
+ Helped-by: Patrick Steinhardt <ps@xxxxxx>
Signed-off-by: Taylor Blau <me@xxxxxxxxxxxx>
## builtin/repack.c ##
-@@
- #define LOOSEN_UNREACHABLE 2
- #define PACK_CRUFT 4
+@@ builtin/repack.c: static int has_existing_non_kept_packs(const struct existing_packs *existing)
+ return existing->non_kept_packs.nr || existing->cruft_packs.nr;
+ }
--#define DELETE_PACK 1
--
- static int pack_everything;
- static int delta_base_offset = 1;
- static int pack_kept_objects = -1;
-@@ builtin/repack.c: static int repack_config(const char *var, const char *value,
-
- struct existing_packs {
- struct string_list kept_packs;
-+ /*
-+ * for both non_kept_packs, and cruft_packs, a non-NULL
-+ * 'util' field indicates the pack should be deleted.
-+ */
- struct string_list non_kept_packs;
- struct string_list cruft_packs;
- };
++static void pack_mark_for_deletion(struct string_list_item *item)
++{
++ item->util = (void*)((uintptr_t)item->util | DELETE_PACK);
++}
++
++static int pack_is_marked_for_deletion(struct string_list_item *item)
++{
++ return (uintptr_t)item->util & DELETE_PACK;
++}
++
+ static void mark_packs_for_deletion_1(struct string_list *names,
+ struct string_list *list)
+ {
@@ builtin/repack.c: static void mark_packs_for_deletion_1(struct string_list *names,
* (if `-d` was given).
*/
if (!string_list_has_string(names, sha1))
- item->util = (void*)(uintptr_t)((size_t)item->util | DELETE_PACK);
-+ item->util = (void*)1;
++ pack_mark_for_deletion(item);
}
}
@@ builtin/repack.c: static void remove_redundant_packs_1(struct string_list *packs
struct string_list_item *item;
for_each_string_list_item(item, packs) {
- if (!((uintptr_t)item->util & DELETE_PACK))
-+ if (!item->util)
++ if (!pack_is_marked_for_deletion(item))
continue;
remove_redundant_pack(packdir, item->string);
}
@@ builtin/repack.c: static void midx_included_packs(struct string_list *include,
-
- for_each_string_list_item(item, &existing->cruft_packs) {
- /*
-- * no need to check DELETE_PACK, since we're not
-- * doing an ALL_INTO_ONE repack
-+ * no need to check for deleted packs, since we're
-+ * not doing an ALL_INTO_ONE repack
- */
- string_list_insert(include, xstrfmt("%s.idx", item->string));
}
} else {
for_each_string_list_item(item, &existing->non_kept_packs) {
- if ((uintptr_t)item->util & DELETE_PACK)
-+ if (item->util)
++ if (pack_is_marked_for_deletion(item))
continue;
string_list_insert(include, xstrfmt("%s.idx", item->string));
}
for_each_string_list_item(item, &existing->cruft_packs) {
- if ((uintptr_t)item->util & DELETE_PACK)
-+ if (item->util)
++ if (pack_is_marked_for_deletion(item))
continue;
string_list_insert(include, xstrfmt("%s.idx", item->string));
}
8: ca7d13e7bf ! 8: 68748eb9c8 builtin/repack.c: extract common cruft pack loop
@@ Commit message
## builtin/repack.c ##
@@ builtin/repack.c: static void midx_included_packs(struct string_list *include,
+
string_list_insert(include, strbuf_detach(&buf, NULL));
}
-
+-
- for_each_string_list_item(item, &existing->cruft_packs) {
- /*
-- * no need to check for deleted packs, since we're
-- * not doing an ALL_INTO_ONE repack
+- * no need to check DELETE_PACK, since we're not
+- * doing an ALL_INTO_ONE repack
- */
- string_list_insert(include, xstrfmt("%s.idx", item->string));
- }
} else {
for_each_string_list_item(item, &existing->non_kept_packs) {
- if (item->util)
+ if (pack_is_marked_for_deletion(item))
continue;
string_list_insert(include, xstrfmt("%s.idx", item->string));
}
+ }
- for_each_string_list_item(item, &existing->cruft_packs) {
-- if (item->util)
+- if (pack_is_marked_for_deletion(item))
- continue;
- string_list_insert(include, xstrfmt("%s.idx", item->string));
- }
@@ builtin/repack.c: static void midx_included_packs(struct string_list *include,
+ * `mark_packs_for_deletion()` when doing an all-into-one
+ * repack).
+ */
-+ if (item->util)
++ if (pack_is_marked_for_deletion(item))
+ continue;
+ string_list_insert(include, xstrfmt("%s.idx", item->string));
}
--
2.42.0.166.g68748eb9c8
