From: Taylor Blau <me@ttaylorr.com>
To: git@vger.kernel.org
Cc: Junio C Hamano <gitster@pobox.com>, Jeff King <peff@peff.net>,
Patrick Steinhardt <ps@pks.im>
Subject: [PATCH v2 0/8] repack: refactor pack snapshot-ing logic
Date: Wed, 13 Sep 2023 15:17:32 -0400 [thread overview]
Message-ID: <cover.1694632644.git.me@ttaylorr.com> (raw)
In-Reply-To: <cover.1693946195.git.me@ttaylorr.com>
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@ttaylorr.com>
## 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@pobox.com>
+ Helped-by: Jeff King <peff@peff.net>
+ Helped-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
## 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
next prev parent reply other threads:[~2023-09-13 19:17 UTC|newest]
Thread overview: 39+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-09-05 20:36 [PATCH 0/8] repack: refactor pack snapshot-ing logic Taylor Blau
2023-09-05 20:36 ` [PATCH 1/8] builtin/repack.c: extract structure to store existing packs Taylor Blau
2023-09-07 7:54 ` Jeff King
2023-09-05 20:36 ` [PATCH 2/8] builtin/repack.c: extract marking packs for deletion Taylor Blau
2023-09-07 7:59 ` Jeff King
2023-09-07 22:10 ` Taylor Blau
2023-09-05 20:36 ` [PATCH 3/8] builtin/repack.c: extract redundant pack cleanup for --geometric Taylor Blau
2023-09-07 8:00 ` Jeff King
2023-09-05 20:36 ` [PATCH 4/8] builtin/repack.c: extract redundant pack cleanup for existing packs Taylor Blau
2023-09-07 8:02 ` Jeff King
2023-09-05 20:36 ` [PATCH 5/8] builtin/repack.c: extract `has_existing_non_kept_packs()` Taylor Blau
2023-09-07 8:04 ` Jeff King
2023-09-05 20:36 ` [PATCH 6/8] builtin/repack.c: store existing cruft packs separately Taylor Blau
2023-09-07 8:09 ` Jeff King
2023-09-05 20:36 ` [PATCH 7/8] builtin/repack.c: drop `DELETE_PACK` macro Taylor Blau
2023-09-06 17:05 ` Junio C Hamano
2023-09-06 17:22 ` Taylor Blau
2023-09-07 8:19 ` Patrick Steinhardt
2023-09-07 18:16 ` Junio C Hamano
2023-09-07 8:58 ` Jeff King
2023-09-05 20:37 ` [PATCH 8/8] builtin/repack.c: extract common cruft pack loop Taylor Blau
2023-09-07 9:00 ` [PATCH 0/8] repack: refactor pack snapshot-ing logic Jeff King
2023-09-13 19:17 ` Taylor Blau [this message]
2023-09-13 19:17 ` [PATCH v2 1/8] builtin/repack.c: extract structure to store existing packs Taylor Blau
2023-09-13 19:17 ` [PATCH v2 2/8] builtin/repack.c: extract marking packs for deletion Taylor Blau
2023-09-13 19:17 ` [PATCH v2 3/8] builtin/repack.c: extract redundant pack cleanup for --geometric Taylor Blau
2023-09-13 19:17 ` [PATCH v2 4/8] builtin/repack.c: extract redundant pack cleanup for existing packs Taylor Blau
2023-09-13 19:17 ` [PATCH v2 5/8] builtin/repack.c: extract `has_existing_non_kept_packs()` Taylor Blau
2023-09-15 10:02 ` Christian Couder
2023-09-13 19:17 ` [PATCH v2 6/8] builtin/repack.c: store existing cruft packs separately Taylor Blau
2023-09-13 19:18 ` [PATCH v2 7/8] builtin/repack.c: avoid directly inspecting "util" Taylor Blau
2023-09-13 19:18 ` [PATCH v2 8/8] builtin/repack.c: extract common cruft pack loop Taylor Blau
2023-09-13 19:44 ` [PATCH v2 0/8] repack: refactor pack snapshot-ing logic Junio C Hamano
2023-09-14 0:07 ` Jeff King
2023-09-14 10:33 ` Patrick Steinhardt
2023-09-14 11:10 ` Christian Couder
2023-09-14 18:01 ` Taylor Blau
2023-09-15 5:56 ` Christian Couder
2023-09-15 10:09 ` Christian Couder
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=cover.1694632644.git.me@ttaylorr.com \
--to=me@ttaylorr.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=peff@peff.net \
--cc=ps@pks.im \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).