{"thread":{"id":"60201","subject":"[PATCH 0/8] repack: refactor pack snapshot-ing logic","startedAt":"2023-09-05T20:36:51Z","lastAt":"2023-09-15T10:10:29Z","messageCount":39,"participants":["Taylor Blau","Junio C Hamano","Patrick Steinhardt","Jeff King","Christian Couder"],"isPatch":true,"patchVersion":1,"patchTotal":8},"messages":[{"id":"481406","messageId":"cover.1693946195.git.me@ttaylorr.com","threadId":"60201","inReplyTo":null,"subject":"[PATCH 0/8] repack: refactor pack snapshot-ing logic","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-09-05T20:36:37Z","receivedAt":"2023-09-05T20:36:51Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"This series refactors some of the 'git repack' internals to keep track\nof existing packs in a set of string lists stored in a single structure,\ninstead of passing around multiple string lists to different functions\nthroughout the various paths.\n\nThe result is that the interface around pack deletion, marking packs as\nredundant, and handling the set of pre-existing packs (both kept and\nnon-kept) is significantly cleaner without introducing any functional\nchanges.\n\nI didn't mean to produce so much churn when I started writing these\npatches, which began as a simple effort to rename a couple of variables\nfor more consistency.\n\nThanks in advance for your review!\n\nTaylor Blau (8):\n builtin/repack.c: extract structure to store existing packs\n builtin/repack.c: extract marking packs for deletion\n builtin/repack.c: extract redundant pack cleanup for --geometric\n builtin/repack.c: extract redundant pack cleanup for existing packs\n builtin/repack.c: extract `has_existing_non_kept_packs()`\n builtin/repack.c: store existing cruft packs separately\n builtin/repack.c: drop `DELETE_PACK` macro\n builtin/repack.c: extract common cruft pack loop\n\n builtin/repack.c | 288 ++++++++++++++++++++++++++++-------------------\n 1 file changed, 174 insertions(+), 114 deletions(-)\n\n-- \n2.42.0.119.gca7d13e7bf\n"},{"id":"481407","messageId":"313537ef68892b15e772eaad8937a4a8c7ebbe61.1693946195.git.me@ttaylorr.com","threadId":"60201","inReplyTo":"cover.1693946195.git.me@ttaylorr.com","subject":"[PATCH 2/8] builtin/repack.c: extract marking packs for deletion","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-09-05T20:36:43Z","receivedAt":"2023-09-05T20:36:55Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"At the end of a repack (when given `-d`), Git attempts to remove any\npacks which have been made \"redundant\" as a result of the repacking\noperation. For example, an all-into-one (`-A` or `-a`) repack makes\nevery pre-existing pack which is not marked as kept redundant. Geometric\nrepacks (with `--geometric=`) make any packs which were rolled up\nredundant, and so on.\n\nBut before deleting the set of packs we think are redundant, we first\ncheck to see whether or not we just wrote a pack which is identical to\nany one of the packs we were going to delete. When this is the case, Git\nmust avoid deleting that pack, since it matches a pack we just wrote\n(so deleting it may cause the repository to become corrupt).\n\nRight now we only process the list of non-kept packs in a single pass.\nBut a future change will split the existing non-kept packs further into\ntwo lists: one for cruft packs, and another for non-cruft packs.\n\nFactor out this routine to prepare for calling it twice on two separate\nlists in a future patch.\n\nSigned-off-by: Taylor Blau \n---\n builtin/repack.c | 50 +++++++++++++++++++++++++++++++-----------------\n 1 file changed, 32 insertions(+), 18 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex c3ab89912e..708556836e 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -105,6 +105,36 @@ struct existing_packs {\n \t.non_kept_packs = STRING_LIST_INIT_DUP, \\\n }\n \n+static void mark_packs_for_deletion_1(struct string_list *names,\n+\t\t\t\t struct string_list *list)\n+{\n+\tstruct string_list_item *item;\n+\tconst int hexsz = the_hash_algo->hexsz;\n+\n+\tfor_each_string_list_item(item, list) {\n+\t\tchar *sha1;\n+\t\tsize_t len = strlen(item->string);\n+\t\tif (len < hexsz)\n+\t\t\tcontinue;\n+\t\tsha1 = item->string + len - hexsz;\n+\t\t/*\n+\t\t * Mark this pack for deletion, which ensures that this\n+\t\t * pack won't be included in a MIDX (if `--write-midx`\n+\t\t * was given) and that we will actually delete this pack\n+\t\t * (if `-d` was given).\n+\t\t */\n+\t\tif (!string_list_has_string(names, sha1))\n+\t\t\titem->util = (void*)(uintptr_t)((size_t)item->util | DELETE_PACK);\n+\t}\n+}\n+\n+static void mark_packs_for_deletion(struct existing_packs *existing,\n+\t\t\t\t struct string_list *names)\n+\n+{\n+\tmark_packs_for_deletion_1(names, &existing->non_kept_packs);\n+}\n+\n static void existing_packs_release(struct existing_packs *existing)\n {\n \tstring_list_clear(&existing->kept_packs, 0);\n@@ -1141,24 +1171,8 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t}\n \t/* End of pack replacement. */\n \n-\tif (delete_redundant && pack_everything & ALL_INTO_ONE) {\n-\t\tconst int hexsz = the_hash_algo->hexsz;\n-\t\tfor_each_string_list_item(item, &existing.non_kept_packs) {\n-\t\t\tchar *sha1;\n-\t\t\tsize_t len = strlen(item->string);\n-\t\t\tif (len < hexsz)\n-\t\t\t\tcontinue;\n-\t\t\tsha1 = item->string + len - hexsz;\n-\t\t\t/*\n-\t\t\t * Mark this pack for deletion, which ensures that this\n-\t\t\t * pack won't be included in a MIDX (if `--write-midx`\n-\t\t\t * was given) and that we will actually delete this pack\n-\t\t\t * (if `-d` was given).\n-\t\t\t */\n-\t\t\tif (!string_list_has_string(&names, sha1))\n-\t\t\t\titem->util = (void*)(uintptr_t)((size_t)item->util | DELETE_PACK);\n-\t\t}\n-\t}\n+\tif (delete_redundant && pack_everything & ALL_INTO_ONE)\n+\t\tmark_packs_for_deletion(&existing, &names);\n \n \tif (write_midx) {\n \t\tstruct string_list include = STRING_LIST_INIT_NODUP;\n-- \n2.42.0.119.gca7d13e7bf\n\n"},{"id":"481408","messageId":"e2cf87bb94da1a777f6b87ae3f32e036832839a6.1693946195.git.me@ttaylorr.com","threadId":"60201","inReplyTo":"cover.1693946195.git.me@ttaylorr.com","subject":"[PATCH 5/8] builtin/repack.c: extract `has_existing_non_kept_packs()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-09-05T20:36:51Z","receivedAt":"2023-09-05T20:36:55Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"When there is:\n\n - at least one pre-existing packfile (which is not marked as kept),\n - repacking with the `-d` flag, and\n - not doing a cruft repack\n\n, then we pass a handful of additional options to the inner\n`pack-objects` process, like `--unpack-unreachable`,\n`--keep-unreachable`, and `--pack-loose-unreachable`, in addition to\nmarking any packs we just wrote for promisor remotes as kept in-core\n(with `--keep-pack`, as opposed to the presence of a \".keep\" file on\ndisk).\n\nBecause we store both cruft and non-cruft packs together in the same\n`existing.non_kept_packs` list, it suffices to check its `nr` member to\nsee if it is zero or not.\n\nBut a following change will store cruft- and non-cruft packs separately,\nmeaning this check would break as a result. Prepare for this by\nextracting this part of the check into a new helper function called\n`has_existing_non_kept_packs()`.\n\nThis patch does not introduce any functional changes, but prepares us to\nmake a more isolated change in a subsequent patch.\n\nSigned-off-by: Taylor Blau \n---\n builtin/repack.c | 8 +++++++-\n 1 file changed, 7 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex f6717e334c..3f0789ff89 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -105,6 +105,11 @@ struct existing_packs {\n \t.non_kept_packs = STRING_LIST_INIT_DUP, \\\n }\n \n+static int has_existing_non_kept_packs(const struct existing_packs *existing)\n+{\n+\treturn existing->non_kept_packs.nr;\n+}\n+\n static void mark_packs_for_deletion_1(struct string_list *names,\n \t\t\t\t struct string_list *list)\n {\n@@ -1048,7 +1053,8 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \tif (pack_everything & ALL_INTO_ONE) {\n \t\trepack_promisor_objects(&po_args, &names);\n \n-\t\tif (existing.non_kept_packs.nr && delete_redundant &&\n+\t\tif (has_existing_non_kept_packs(&existing) &&\n+\t\t delete_redundant &&\n \t\t !(pack_everything & PACK_CRUFT)) {\n \t\t\tfor_each_string_list_item(item, &names) {\n \t\t\t\tstrvec_pushf(&cmd.args, \"--keep-pack=%s-%s.pack\",\n-- \n2.42.0.119.gca7d13e7bf\n\n"},{"id":"481409","messageId":"5c25ef87c1430e012a2e48b738b3b5aa760b4b0f.1693946195.git.me@ttaylorr.com","threadId":"60201","inReplyTo":"cover.1693946195.git.me@ttaylorr.com","subject":"[PATCH 3/8] builtin/repack.c: extract redundant pack cleanup for --geometric","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-09-05T20:36:46Z","receivedAt":"2023-09-05T20:36:58Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"To reduce the complexity of the already quite-long `cmd_repack()`\nimplementation, extract out the parts responsible for deleting redundant\npacks from a geometric repack out into its own sub-routine.\n\nSigned-off-by: Taylor Blau \n---\n builtin/repack.c | 52 +++++++++++++++++++++++++++---------------------\n 1 file changed, 29 insertions(+), 23 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 708556836e..d3e6326bb9 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -540,6 +540,32 @@ static struct packed_git *get_preferred_pack(struct pack_geometry *geometry)\n \treturn NULL;\n }\n \n+static void geometry_remove_redundant_packs(struct pack_geometry *geometry,\n+\t\t\t\t\t struct string_list *names,\n+\t\t\t\t\t struct existing_packs *existing)\n+{\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tuint32_t i;\n+\n+\tfor (i = 0; i < geometry->split; i++) {\n+\t\tstruct packed_git *p = geometry->pack[i];\n+\t\tif (string_list_has_string(names, hash_to_hex(p->hash)))\n+\t\t\tcontinue;\n+\n+\t\tstrbuf_reset(&buf);\n+\t\tstrbuf_addstr(&buf, pack_basename(p));\n+\t\tstrbuf_strip_suffix(&buf, \".pack\");\n+\n+\t\tif ((p->pack_keep) ||\n+\t\t (string_list_has_string(&existing->kept_packs, buf.buf)))\n+\t\t\tcontinue;\n+\n+\t\tremove_redundant_pack(packdir, buf.buf);\n+\t}\n+\n+\tstrbuf_release(&buf);\n+}\n+\n static void free_pack_geometry(struct pack_geometry *geometry)\n {\n \tif (!geometry)\n@@ -1201,29 +1227,9 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\t\tremove_redundant_pack(packdir, item->string);\n \t\t}\n \n-\t\tif (geometry.split_factor) {\n-\t\t\tstruct strbuf buf = STRBUF_INIT;\n-\n-\t\t\tuint32_t i;\n-\t\t\tfor (i = 0; i < geometry.split; i++) {\n-\t\t\t\tstruct packed_git *p = geometry.pack[i];\n-\t\t\t\tif (string_list_has_string(&names,\n-\t\t\t\t\t\t\t hash_to_hex(p->hash)))\n-\t\t\t\t\tcontinue;\n-\n-\t\t\t\tstrbuf_reset(&buf);\n-\t\t\t\tstrbuf_addstr(&buf, pack_basename(p));\n-\t\t\t\tstrbuf_strip_suffix(&buf, \".pack\");\n-\n-\t\t\t\tif ((p->pack_keep) ||\n-\t\t\t\t (string_list_has_string(&existing.kept_packs,\n-\t\t\t\t\t\t\t buf.buf)))\n-\t\t\t\t\tcontinue;\n-\n-\t\t\t\tremove_redundant_pack(packdir, buf.buf);\n-\t\t\t}\n-\t\t\tstrbuf_release(&buf);\n-\t\t}\n+\t\tif (geometry.split_factor)\n+\t\t\tgeometry_remove_redundant_packs(&geometry, &names,\n+\t\t\t\t\t\t\t&existing);\n \t\tif (show_progress)\n \t\t\topts |= PRUNE_PACKED_VERBOSE;\n \t\tprune_packed_objects(opts);\n-- \n2.42.0.119.gca7d13e7bf\n\n"},{"id":"481410","messageId":"5b48b7e3cc03c83465a3dcecaa98b9d2e9667084.1693946195.git.me@ttaylorr.com","threadId":"60201","inReplyTo":"cover.1693946195.git.me@ttaylorr.com","subject":"[PATCH 1/8] builtin/repack.c: extract structure to store existing packs","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-09-05T20:36:40Z","receivedAt":"2023-09-05T20:36:58Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"The repack machinery needs to keep track of which packfiles were present\nin the repository at the beginning of a repack, segmented by whether or\nnot each pack is marked as kept.\n\nThe names of these packs are stored in two `string_list`s, corresponding\nto kept- and non-kept packs, respectively. As a consequence, many\nfunctions within the repack code need to take both `string_list`s as\narguments, leading to code like this:\n\n ret = write_cruft_pack(&cruft_po_args, packtmp, pack_prefix,\n cruft_expiration, &names,\n &existing_nonkept_packs, /* <- */\n &existing_kept_packs); /* <- */\n\nWrap up this pair of `string_list`s into a single structure that stores\nboth. This saves us from having to pass both string lists separately,\nand prepares for adding additional fields to this structure.\n\nSigned-off-by: Taylor Blau \n---\n builtin/repack.c | 90 ++++++++++++++++++++++++++----------------------\n 1 file changed, 49 insertions(+), 41 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 2b43a5be08..c3ab89912e 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -95,14 +95,29 @@ static int repack_config(const char *var, const char *value,\n \treturn git_default_config(var, value, ctx, cb);\n }\n \n+struct existing_packs {\n+\tstruct string_list kept_packs;\n+\tstruct string_list non_kept_packs;\n+};\n+\n+#define EXISTING_PACKS_INIT { \\\n+\t.kept_packs = STRING_LIST_INIT_DUP, \\\n+\t.non_kept_packs = STRING_LIST_INIT_DUP, \\\n+}\n+\n+static void existing_packs_release(struct existing_packs *existing)\n+{\n+\tstring_list_clear(&existing->kept_packs, 0);\n+\tstring_list_clear(&existing->non_kept_packs, 0);\n+}\n+\n /*\n- * Adds all packs hex strings (pack-$HASH) to either fname_nonkept_list\n- * or fname_kept_list based on whether each pack has a corresponding\n+ * Adds all packs hex strings (pack-$HASH) to either packs->non_kept\n+ * or packs->kept based on whether each pack has a corresponding\n * .keep file or not. Packs without a .keep file are not to be kept\n * if we are going to pack everything into one file.\n */\n-static void collect_pack_filenames(struct string_list *fname_nonkept_list,\n-\t\t\t\t struct string_list *fname_kept_list,\n+static void collect_pack_filenames(struct existing_packs *existing,\n \t\t\t\t const struct string_list *extra_keep)\n {\n \tstruct packed_git *p;\n@@ -126,16 +141,16 @@ static void collect_pack_filenames(struct string_list *fname_nonkept_list,\n \t\tstrbuf_strip_suffix(&buf, \".pack\");\n \n \t\tif ((extra_keep->nr > 0 && i < extra_keep->nr) || p->pack_keep)\n-\t\t\tstring_list_append(fname_kept_list, buf.buf);\n+\t\t\tstring_list_append(&existing->kept_packs, buf.buf);\n \t\telse {\n \t\t\tstruct string_list_item *item;\n-\t\t\titem = string_list_append(fname_nonkept_list, buf.buf);\n+\t\t\titem = string_list_append(&existing->non_kept_packs, buf.buf);\n \t\t\tif (p->is_cruft)\n \t\t\t\titem->util = (void*)(uintptr_t)CRUFT_PACK;\n \t\t}\n \t}\n \n-\tstring_list_sort(fname_kept_list);\n+\tstring_list_sort(&existing->kept_packs);\n \tstrbuf_release(&buf);\n }\n \n@@ -327,7 +342,7 @@ static int geometry_cmp(const void *va, const void *vb)\n }\n \n static void init_pack_geometry(struct pack_geometry *geometry,\n-\t\t\t struct string_list *existing_kept_packs,\n+\t\t\t struct existing_packs *existing,\n \t\t\t const struct pack_objects_args *args)\n {\n \tstruct packed_git *p;\n@@ -344,23 +359,24 @@ static void init_pack_geometry(struct pack_geometry *geometry,\n \n \t\tif (!pack_kept_objects) {\n \t\t\t/*\n-\t\t\t * Any pack that has its pack_keep bit set will appear\n-\t\t\t * in existing_kept_packs below, but this saves us from\n-\t\t\t * doing a more expensive check.\n+\t\t\t * Any pack that has its pack_keep bit set will\n+\t\t\t * appear in existing->kept_packs below, but\n+\t\t\t * this saves us from doing a more expensive\n+\t\t\t * check.\n \t\t\t */\n \t\t\tif (p->pack_keep)\n \t\t\t\tcontinue;\n \n \t\t\t/*\n-\t\t\t * The pack may be kept via the --keep-pack option;\n-\t\t\t * check 'existing_kept_packs' to determine whether to\n-\t\t\t * ignore it.\n+\t\t\t * The pack may be kept via the --keep-pack\n+\t\t\t * option; check 'existing->kept_packs' to\n+\t\t\t * determine whether to ignore it.\n \t\t\t */\n \t\t\tstrbuf_reset(&buf);\n \t\t\tstrbuf_addstr(&buf, pack_basename(p));\n \t\t\tstrbuf_strip_suffix(&buf, \".pack\");\n \n-\t\t\tif (string_list_has_string(existing_kept_packs, buf.buf))\n+\t\t\tif (string_list_has_string(&existing->kept_packs, buf.buf))\n \t\t\t\tcontinue;\n \t\t}\n \t\tif (p->is_cruft)\n@@ -565,14 +581,13 @@ static void midx_snapshot_refs(struct tempfile *f)\n }\n \n static void midx_included_packs(struct string_list *include,\n-\t\t\t\tstruct string_list *existing_nonkept_packs,\n-\t\t\t\tstruct string_list *existing_kept_packs,\n+\t\t\t\tstruct existing_packs *existing,\n \t\t\t\tstruct string_list *names,\n \t\t\t\tstruct pack_geometry *geometry)\n {\n \tstruct string_list_item *item;\n \n-\tfor_each_string_list_item(item, existing_kept_packs)\n+\tfor_each_string_list_item(item, &existing->kept_packs)\n \t\tstring_list_insert(include, xstrfmt(\"%s.idx\", item->string));\n \tfor_each_string_list_item(item, names)\n \t\tstring_list_insert(include, xstrfmt(\"pack-%s.idx\", item->string));\n@@ -600,7 +615,7 @@ static void midx_included_packs(struct string_list *include,\n \t\t\tstring_list_insert(include, strbuf_detach(&buf, NULL));\n \t\t}\n \n-\t\tfor_each_string_list_item(item, existing_nonkept_packs) {\n+\t\tfor_each_string_list_item(item, &existing->non_kept_packs) {\n \t\t\tif (!((uintptr_t)item->util & CRUFT_PACK)) {\n \t\t\t\t/*\n \t\t\t\t * no need to check DELETE_PACK, since we're not\n@@ -611,7 +626,7 @@ static void midx_included_packs(struct string_list *include,\n \t\t\tstring_list_insert(include, xstrfmt(\"%s.idx\", item->string));\n \t\t}\n \t} else {\n-\t\tfor_each_string_list_item(item, existing_nonkept_packs) {\n+\t\tfor_each_string_list_item(item, &existing->non_kept_packs) {\n \t\t\tif ((uintptr_t)item->util & DELETE_PACK)\n \t\t\t\tcontinue;\n \t\t\tstring_list_insert(include, xstrfmt(\"%s.idx\", item->string));\n@@ -700,8 +715,7 @@ static int write_cruft_pack(const struct pack_objects_args *args,\n \t\t\t const char *pack_prefix,\n \t\t\t const char *cruft_expiration,\n \t\t\t struct string_list *names,\n-\t\t\t struct string_list *existing_packs,\n-\t\t\t struct string_list *existing_kept_packs)\n+\t\t\t struct existing_packs *existing)\n {\n \tstruct child_process cmd = CHILD_PROCESS_INIT;\n \tstruct strbuf line = STRBUF_INIT;\n@@ -744,9 +758,9 @@ static int write_cruft_pack(const struct pack_objects_args *args,\n \tin = xfdopen(cmd.in, \"w\");\n \tfor_each_string_list_item(item, names)\n \t\tfprintf(in, \"%s-%s.pack\\n\", pack_prefix, item->string);\n-\tfor_each_string_list_item(item, existing_packs)\n+\tfor_each_string_list_item(item, &existing->non_kept_packs)\n \t\tfprintf(in, \"-%s.pack\\n\", item->string);\n-\tfor_each_string_list_item(item, existing_kept_packs)\n+\tfor_each_string_list_item(item, &existing->kept_packs)\n \t\tfprintf(in, \"%s.pack\\n\", item->string);\n \tfclose(in);\n \n@@ -778,8 +792,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \tstruct child_process cmd = CHILD_PROCESS_INIT;\n \tstruct string_list_item *item;\n \tstruct string_list names = STRING_LIST_INIT_DUP;\n-\tstruct string_list existing_nonkept_packs = STRING_LIST_INIT_DUP;\n-\tstruct string_list existing_kept_packs = STRING_LIST_INIT_DUP;\n+\tstruct existing_packs existing = EXISTING_PACKS_INIT;\n \tstruct pack_geometry geometry = { 0 };\n \tstruct strbuf line = STRBUF_INIT;\n \tstruct tempfile *refs_snapshot = NULL;\n@@ -915,13 +928,12 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \tpacktmp_name = xstrfmt(\".tmp-%d-pack\", (int)getpid());\n \tpacktmp = mkpathdup(\"%s/%s\", packdir, packtmp_name);\n \n-\tcollect_pack_filenames(&existing_nonkept_packs, &existing_kept_packs,\n-\t\t\t &keep_pack_list);\n+\tcollect_pack_filenames(&existing, &keep_pack_list);\n \n \tif (geometry.split_factor) {\n \t\tif (pack_everything)\n \t\t\tdie(_(\"options '%s' and '%s' cannot be used together\"), \"--geometric\", \"-A/-a\");\n-\t\tinit_pack_geometry(&geometry, &existing_kept_packs, &po_args);\n+\t\tinit_pack_geometry(&geometry, &existing, &po_args);\n \t\tsplit_pack_geometry(&geometry);\n \t}\n \n@@ -965,7 +977,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \tif (pack_everything & ALL_INTO_ONE) {\n \t\trepack_promisor_objects(&po_args, &names);\n \n-\t\tif (existing_nonkept_packs.nr && delete_redundant &&\n+\t\tif (existing.non_kept_packs.nr && delete_redundant &&\n \t\t !(pack_everything & PACK_CRUFT)) {\n \t\t\tfor_each_string_list_item(item, &names) {\n \t\t\t\tstrvec_pushf(&cmd.args, \"--keep-pack=%s-%s.pack\",\n@@ -1054,8 +1066,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \n \t\tret = write_cruft_pack(&cruft_po_args, packtmp, pack_prefix,\n \t\t\t\t cruft_expiration, &names,\n-\t\t\t\t &existing_nonkept_packs,\n-\t\t\t\t &existing_kept_packs);\n+\t\t\t\t &existing);\n \t\tif (ret)\n \t\t\tgoto cleanup;\n \n@@ -1086,8 +1097,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\t\t\t\t pack_prefix,\n \t\t\t\t\t NULL,\n \t\t\t\t\t &names,\n-\t\t\t\t\t &existing_nonkept_packs,\n-\t\t\t\t\t &existing_kept_packs);\n+\t\t\t\t\t &existing);\n \t\t\tif (ret)\n \t\t\t\tgoto cleanup;\n \t\t}\n@@ -1133,7 +1143,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \n \tif (delete_redundant && pack_everything & ALL_INTO_ONE) {\n \t\tconst int hexsz = the_hash_algo->hexsz;\n-\t\tfor_each_string_list_item(item, &existing_nonkept_packs) {\n+\t\tfor_each_string_list_item(item, &existing.non_kept_packs) {\n \t\t\tchar *sha1;\n \t\t\tsize_t len = strlen(item->string);\n \t\t\tif (len < hexsz)\n@@ -1152,8 +1162,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \n \tif (write_midx) {\n \t\tstruct string_list include = STRING_LIST_INIT_NODUP;\n-\t\tmidx_included_packs(&include, &existing_nonkept_packs,\n-\t\t\t\t &existing_kept_packs, &names, &geometry);\n+\t\tmidx_included_packs(&include, &existing, &names, &geometry);\n \n \t\tret = write_midx_included_packs(&include, &geometry,\n \t\t\t\t\t\trefs_snapshot ? get_tempfile_path(refs_snapshot) : NULL,\n@@ -1172,7 +1181,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \n \tif (delete_redundant) {\n \t\tint opts = 0;\n-\t\tfor_each_string_list_item(item, &existing_nonkept_packs) {\n+\t\tfor_each_string_list_item(item, &existing.non_kept_packs) {\n \t\t\tif (!((uintptr_t)item->util & DELETE_PACK))\n \t\t\t\tcontinue;\n \t\t\tremove_redundant_pack(packdir, item->string);\n@@ -1193,7 +1202,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\t\t\tstrbuf_strip_suffix(&buf, \".pack\");\n \n \t\t\t\tif ((p->pack_keep) ||\n-\t\t\t\t (string_list_has_string(&existing_kept_packs,\n+\t\t\t\t (string_list_has_string(&existing.kept_packs,\n \t\t\t\t\t\t\t buf.buf)))\n \t\t\t\t\tcontinue;\n \n@@ -1224,8 +1233,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \n cleanup:\n \tstring_list_clear(&names, 1);\n-\tstring_list_clear(&existing_nonkept_packs, 0);\n-\tstring_list_clear(&existing_kept_packs, 0);\n+\texisting_packs_release(&existing);\n \tfree_pack_geometry(&geometry);\n \n \treturn ret;\n-- \n2.42.0.119.gca7d13e7bf\n\n"},{"id":"481411","messageId":"7bb543fef8b140726b6b3efc2bb2cb1a6384fcd9.1693946195.git.me@ttaylorr.com","threadId":"60201","inReplyTo":"cover.1693946195.git.me@ttaylorr.com","subject":"[PATCH 4/8] builtin/repack.c: extract redundant pack cleanup for existing packs","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-09-05T20:36:48Z","receivedAt":"2023-09-05T20:37:01Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"To remove redundant packs at the end of a repacking operation, Git uses\nits `remove_redundant_pack()` function in a loop over the set of\npre-existing, non-kept packs.\n\nIn a later commit, we will split this list into two, one for\npre-existing cruft pack(s), and another for non-cruft pack(s). Prepare\nfor this by factoring out the routine to loop over and delete redundant\npacks into its own function.\n\nInstead of calling `remove_redundant_pack()` directly, we now will call\n`remove_redundant_existing_packs()`, which itself dispatches a call to\n`remove_redundant_packs_1()`. Note that the geometric repacking code\nwill still call `remove_redundant_pack()` directly, but see the previous\ncommit for more details.\n\nHaving `remove_redundant_packs_1()` exist as a separate function may\nseem like overkill in this patch. However, a later patch will call\n`remove_redundant_packs_1()` once over two separate lists, so this\nrefactoring sets us up for that.\n\nSigned-off-by: Taylor Blau \n---\n builtin/repack.c | 45 ++++++++++++++++++++++++++++-----------------\n 1 file changed, 28 insertions(+), 17 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex d3e6326bb9..f6717e334c 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -135,6 +135,33 @@ static void mark_packs_for_deletion(struct existing_packs *existing,\n \tmark_packs_for_deletion_1(names, &existing->non_kept_packs);\n }\n \n+static void remove_redundant_pack(const char *dir_name, const char *base_name)\n+{\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tstruct multi_pack_index *m = get_local_multi_pack_index(the_repository);\n+\tstrbuf_addf(&buf, \"%s.pack\", base_name);\n+\tif (m && midx_contains_pack(m, buf.buf))\n+\t\tclear_midx_file(the_repository);\n+\tstrbuf_insertf(&buf, 0, \"%s/\", dir_name);\n+\tunlink_pack_path(buf.buf, 1);\n+\tstrbuf_release(&buf);\n+}\n+\n+static void remove_redundant_packs_1(struct string_list *packs)\n+{\n+\tstruct string_list_item *item;\n+\tfor_each_string_list_item(item, packs) {\n+\t\tif (!((uintptr_t)item->util & DELETE_PACK))\n+\t\t\tcontinue;\n+\t\tremove_redundant_pack(packdir, item->string);\n+\t}\n+}\n+\n+static void remove_redundant_existing_packs(struct existing_packs *existing)\n+{\n+\tremove_redundant_packs_1(&existing->non_kept_packs);\n+}\n+\n static void existing_packs_release(struct existing_packs *existing)\n {\n \tstring_list_clear(&existing->kept_packs, 0);\n@@ -184,18 +211,6 @@ static void collect_pack_filenames(struct existing_packs *existing,\n \tstrbuf_release(&buf);\n }\n \n-static void remove_redundant_pack(const char *dir_name, const char *base_name)\n-{\n-\tstruct strbuf buf = STRBUF_INIT;\n-\tstruct multi_pack_index *m = get_local_multi_pack_index(the_repository);\n-\tstrbuf_addf(&buf, \"%s.pack\", base_name);\n-\tif (m && midx_contains_pack(m, buf.buf))\n-\t\tclear_midx_file(the_repository);\n-\tstrbuf_insertf(&buf, 0, \"%s/\", dir_name);\n-\tunlink_pack_path(buf.buf, 1);\n-\tstrbuf_release(&buf);\n-}\n-\n static void prepare_pack_objects(struct child_process *cmd,\n \t\t\t\t const struct pack_objects_args *args,\n \t\t\t\t const char *out)\n@@ -1221,11 +1236,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \n \tif (delete_redundant) {\n \t\tint opts = 0;\n-\t\tfor_each_string_list_item(item, &existing.non_kept_packs) {\n-\t\t\tif (!((uintptr_t)item->util & DELETE_PACK))\n-\t\t\t\tcontinue;\n-\t\t\tremove_redundant_pack(packdir, item->string);\n-\t\t}\n+\t\tremove_redundant_existing_packs(&existing);\n \n \t\tif (geometry.split_factor)\n \t\t\tgeometry_remove_redundant_packs(&geometry, &names,\n-- \n2.42.0.119.gca7d13e7bf\n\n"},{"id":"481412","messageId":"414a558883830a29924710126960074b37ab97fc.1693946195.git.me@ttaylorr.com","threadId":"60201","inReplyTo":"cover.1693946195.git.me@ttaylorr.com","subject":"[PATCH 6/8] builtin/repack.c: store existing cruft packs separately","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-09-05T20:36:54Z","receivedAt":"2023-09-05T20:37:31Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"When repacking with the `--write-midx` option, we invoke the function\n`midx_included_packs()` in order to produce the list of packs we want to\ninclude in the resulting MIDX.\n\nThis list is comprised of:\n\n - existing .keep packs\n - any pack(s) which were written earlier in the same process\n - any unchanged packs when doing a `--geometric` repack\n - any cruft packs\n\nPrior to this patch, we stored pre-existing cruft and non-cruft packs\ntogether (provided those packs are non-kept). This meant we needed an\nadditional bit to indicate which non-kept pack(s) were cruft versus\nthose that aren't.\n\nBut alternatively we can store cruft packs in a separate list, avoiding\nthe need for this extra bit, and simplifying the code below.\n\nSigned-off-by: Taylor Blau \n---\n builtin/repack.c | 39 +++++++++++++++++++++++----------------\n 1 file changed, 23 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 3f0789ff89..478fab96c9 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -27,7 +27,6 @@\n #define PACK_CRUFT 4\n \n #define DELETE_PACK 1\n-#define CRUFT_PACK 2\n \n static int pack_everything;\n static int delta_base_offset = 1;\n@@ -98,16 +97,18 @@ static int repack_config(const char *var, const char *value,\n struct existing_packs {\n \tstruct string_list kept_packs;\n \tstruct string_list non_kept_packs;\n+\tstruct string_list cruft_packs;\n };\n \n #define EXISTING_PACKS_INIT { \\\n \t.kept_packs = STRING_LIST_INIT_DUP, \\\n \t.non_kept_packs = STRING_LIST_INIT_DUP, \\\n+\t.cruft_packs = STRING_LIST_INIT_DUP, \\\n }\n \n static int has_existing_non_kept_packs(const struct existing_packs *existing)\n {\n-\treturn existing->non_kept_packs.nr;\n+\treturn existing->non_kept_packs.nr || existing->cruft_packs.nr;\n }\n \n static void mark_packs_for_deletion_1(struct string_list *names,\n@@ -138,6 +139,7 @@ static void mark_packs_for_deletion(struct existing_packs *existing,\n \n {\n \tmark_packs_for_deletion_1(names, &existing->non_kept_packs);\n+\tmark_packs_for_deletion_1(names, &existing->cruft_packs);\n }\n \n static void remove_redundant_pack(const char *dir_name, const char *base_name)\n@@ -165,12 +167,14 @@ static void remove_redundant_packs_1(struct string_list *packs)\n static void remove_redundant_existing_packs(struct existing_packs *existing)\n {\n \tremove_redundant_packs_1(&existing->non_kept_packs);\n+\tremove_redundant_packs_1(&existing->cruft_packs);\n }\n \n static void existing_packs_release(struct existing_packs *existing)\n {\n \tstring_list_clear(&existing->kept_packs, 0);\n \tstring_list_clear(&existing->non_kept_packs, 0);\n+\tstring_list_clear(&existing->cruft_packs, 0);\n }\n \n /*\n@@ -204,12 +208,10 @@ static void collect_pack_filenames(struct existing_packs *existing,\n \n \t\tif ((extra_keep->nr > 0 && i < extra_keep->nr) || p->pack_keep)\n \t\t\tstring_list_append(&existing->kept_packs, buf.buf);\n-\t\telse {\n-\t\t\tstruct string_list_item *item;\n-\t\t\titem = string_list_append(&existing->non_kept_packs, buf.buf);\n-\t\t\tif (p->is_cruft)\n-\t\t\t\titem->util = (void*)(uintptr_t)CRUFT_PACK;\n-\t\t}\n+\t\telse if (p->is_cruft)\n+\t\t\tstring_list_append(&existing->cruft_packs, buf.buf);\n+\t\telse\n+\t\t\tstring_list_append(&existing->non_kept_packs, buf.buf);\n \t}\n \n \tstring_list_sort(&existing->kept_packs);\n@@ -691,14 +693,11 @@ static void midx_included_packs(struct string_list *include,\n \t\t\tstring_list_insert(include, strbuf_detach(&buf, NULL));\n \t\t}\n \n-\t\tfor_each_string_list_item(item, &existing->non_kept_packs) {\n-\t\t\tif (!((uintptr_t)item->util & CRUFT_PACK)) {\n-\t\t\t\t/*\n-\t\t\t\t * no need to check DELETE_PACK, since we're not\n-\t\t\t\t * doing an ALL_INTO_ONE repack\n-\t\t\t\t */\n-\t\t\t\tcontinue;\n-\t\t\t}\n+\t\tfor_each_string_list_item(item, &existing->cruft_packs) {\n+\t\t\t/*\n+\t\t\t * no need to check DELETE_PACK, since we're not\n+\t\t\t * doing an ALL_INTO_ONE repack\n+\t\t\t */\n \t\t\tstring_list_insert(include, xstrfmt(\"%s.idx\", item->string));\n \t\t}\n \t} else {\n@@ -707,6 +706,12 @@ static void midx_included_packs(struct string_list *include,\n \t\t\t\tcontinue;\n \t\t\tstring_list_insert(include, xstrfmt(\"%s.idx\", item->string));\n \t\t}\n+\n+\t\tfor_each_string_list_item(item, &existing->cruft_packs) {\n+\t\t\tif ((uintptr_t)item->util & DELETE_PACK)\n+\t\t\t\tcontinue;\n+\t\t\tstring_list_insert(include, xstrfmt(\"%s.idx\", item->string));\n+\t\t}\n \t}\n }\n \n@@ -836,6 +841,8 @@ static int write_cruft_pack(const struct pack_objects_args *args,\n \t\tfprintf(in, \"%s-%s.pack\\n\", pack_prefix, item->string);\n \tfor_each_string_list_item(item, &existing->non_kept_packs)\n \t\tfprintf(in, \"-%s.pack\\n\", item->string);\n+\tfor_each_string_list_item(item, &existing->cruft_packs)\n+\t\tfprintf(in, \"-%s.pack\\n\", item->string);\n \tfor_each_string_list_item(item, &existing->kept_packs)\n \t\tfprintf(in, \"%s.pack\\n\", item->string);\n \tfclose(in);\n-- \n2.42.0.119.gca7d13e7bf\n\n"},{"id":"481413","messageId":"559b487e2ab056c79367a673188764e4cdce3c96.1693946195.git.me@ttaylorr.com","threadId":"60201","inReplyTo":"cover.1693946195.git.me@ttaylorr.com","subject":"[PATCH 7/8] builtin/repack.c: drop `DELETE_PACK` macro","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-09-05T20:36:56Z","receivedAt":"2023-09-05T20:37:36Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Signed-off-by: Taylor Blau \n---\n builtin/repack.c | 18 ++++++++++--------\n 1 file changed, 10 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 478fab96c9..6110598a69 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -26,8 +26,6 @@\n #define LOOSEN_UNREACHABLE 2\n #define PACK_CRUFT 4\n \n-#define DELETE_PACK 1\n-\n static int pack_everything;\n static int delta_base_offset = 1;\n static int pack_kept_objects = -1;\n@@ -96,6 +94,10 @@ static int repack_config(const char *var, const char *value,\n \n struct existing_packs {\n \tstruct string_list kept_packs;\n+\t/*\n+\t * for both non_kept_packs, and cruft_packs, a non-NULL\n+\t * 'util' field indicates the pack should be deleted.\n+\t */\n \tstruct string_list non_kept_packs;\n \tstruct string_list cruft_packs;\n };\n@@ -130,7 +132,7 @@ static void mark_packs_for_deletion_1(struct string_list *names,\n \t\t * (if `-d` was given).\n \t\t */\n \t\tif (!string_list_has_string(names, sha1))\n-\t\t\titem->util = (void*)(uintptr_t)((size_t)item->util | DELETE_PACK);\n+\t\t\titem->util = (void*)1;\n \t}\n }\n \n@@ -158,7 +160,7 @@ static void remove_redundant_packs_1(struct string_list *packs)\n {\n \tstruct string_list_item *item;\n \tfor_each_string_list_item(item, packs) {\n-\t\tif (!((uintptr_t)item->util & DELETE_PACK))\n+\t\tif (!item->util)\n \t\t\tcontinue;\n \t\tremove_redundant_pack(packdir, item->string);\n \t}\n@@ -695,20 +697,20 @@ static void midx_included_packs(struct string_list *include,\n \n \t\tfor_each_string_list_item(item, &existing->cruft_packs) {\n \t\t\t/*\n-\t\t\t * no need to check DELETE_PACK, since we're not\n-\t\t\t * doing an ALL_INTO_ONE repack\n+\t\t\t * no need to check for deleted packs, since we're\n+\t\t\t * not doing an ALL_INTO_ONE repack\n \t\t\t */\n \t\t\tstring_list_insert(include, xstrfmt(\"%s.idx\", item->string));\n \t\t}\n \t} else {\n \t\tfor_each_string_list_item(item, &existing->non_kept_packs) {\n-\t\t\tif ((uintptr_t)item->util & DELETE_PACK)\n+\t\t\tif (item->util)\n \t\t\t\tcontinue;\n \t\t\tstring_list_insert(include, xstrfmt(\"%s.idx\", item->string));\n \t\t}\n \n \t\tfor_each_string_list_item(item, &existing->cruft_packs) {\n-\t\t\tif ((uintptr_t)item->util & DELETE_PACK)\n+\t\t\tif (item->util)\n \t\t\t\tcontinue;\n \t\t\tstring_list_insert(include, xstrfmt(\"%s.idx\", item->string));\n \t\t}\n-- \n2.42.0.119.gca7d13e7bf\n\n"},{"id":"481414","messageId":"ca7d13e7bf7725caefaceab6abd05dae3a7958c3.1693946195.git.me@ttaylorr.com","threadId":"60201","inReplyTo":"cover.1693946195.git.me@ttaylorr.com","subject":"[PATCH 8/8] builtin/repack.c: extract common cruft pack loop","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-09-05T20:37:00Z","receivedAt":"2023-09-05T20:37:38Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"When generating the list of packs to store in a MIDX (when given the\n`--write-midx` option), we include any cruft packs both during\n--geometric and non-geometric repacks.\n\nBut the rules for when we do and don't have to check whether any of\nthose cruft packs were queued for deletion differ slightly between the\ntwo cases.\n\nBut the two can be unified, provided there is a little bit of extra\ndetail added in the comment to clarify when it is safe to avoid checking\nfor any pending deletions (and why it is OK to do so even when not\nrequired).\n\nSigned-off-by: Taylor Blau \n---\n builtin/repack.c | 30 ++++++++++++++++++------------\n 1 file changed, 18 insertions(+), 12 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 6110598a69..e6a1ef9a09 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -695,25 +695,31 @@ static void midx_included_packs(struct string_list *include,\n \t\t\tstring_list_insert(include, strbuf_detach(&buf, NULL));\n \t\t}\n \n-\t\tfor_each_string_list_item(item, &existing->cruft_packs) {\n-\t\t\t/*\n-\t\t\t * no need to check for deleted packs, since we're\n-\t\t\t * not doing an ALL_INTO_ONE repack\n-\t\t\t */\n-\t\t\tstring_list_insert(include, xstrfmt(\"%s.idx\", item->string));\n-\t\t}\n \t} else {\n \t\tfor_each_string_list_item(item, &existing->non_kept_packs) {\n \t\t\tif (item->util)\n \t\t\t\tcontinue;\n \t\t\tstring_list_insert(include, xstrfmt(\"%s.idx\", item->string));\n \t\t}\n+\t}\n \n-\t\tfor_each_string_list_item(item, &existing->cruft_packs) {\n-\t\t\tif (item->util)\n-\t\t\t\tcontinue;\n-\t\t\tstring_list_insert(include, xstrfmt(\"%s.idx\", item->string));\n-\t\t}\n+\tfor_each_string_list_item(item, &existing->cruft_packs) {\n+\t\t/*\n+\t\t * When doing a --geometric repack, there is no need to check\n+\t\t * for deleted packs, since we're by definition not doing an\n+\t\t * ALL_INTO_ONE repack (hence no packs will be deleted).\n+\t\t * Otherwise we must check for and exclude any packs which are\n+\t\t * enqueued for deletion.\n+\t\t *\n+\t\t * So we could omit the conditional below in the --geometric\n+\t\t * case, but doing so is unnecessary since no packs are marked\n+\t\t * as pending deletion (since we only call\n+\t\t * `mark_packs_for_deletion()` when doing an all-into-one\n+\t\t * repack).\n+\t\t */\n+\t\tif (item->util)\n+\t\t\tcontinue;\n+\t\tstring_list_insert(include, xstrfmt(\"%s.idx\", item->string));\n \t}\n }\n \n-- \n2.42.0.119.gca7d13e7bf\n"},{"id":"481443","messageId":"xmqqh6o7nsf2.fsf@gitster.g","threadId":"60201","inReplyTo":"559b487e2ab056c79367a673188764e4cdce3c96.1693946195.git.me@ttaylorr.com","subject":"Re: [PATCH 7/8] builtin/repack.c: drop `DELETE_PACK` macro","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-09-06T17:05:37Z","receivedAt":"2023-09-06T17:05:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> Signed-off-by: Taylor Blau \n> ---\n> builtin/repack.c | 18 ++++++++++--------\n> 1 file changed, 10 insertions(+), 8 deletions(-)\n\nThe reason being...?\n\n> @@ -130,7 +132,7 @@ static void mark_packs_for_deletion_1(struct string_list *names,\n> \t\t * (if `-d` was given).\n> \t\t */\n> \t\tif (!string_list_has_string(names, sha1))\n> -\t\t\titem->util = (void*)(uintptr_t)((size_t)item->util | DELETE_PACK);\n> +\t\t\titem->util = (void*)1;\n\nPresumably the literal \"1\" is promoted to an appropriate type\n(uintptr_t) here?\n\n\n We originally planned to use the .util member to store a bitset\n to represent various attributes for each pack, and defined\n DELETE_PACK as one of the possible bits, but over the N years of\n its existence, we never found the need for the second bit.\n\nor something?\n\nIt is not a bad idea to demote .util from a set of bits to just a\n\"is it NULL?\" boolean, but updating the above to something like\n\n\t#define DELETE_PACK ((void*)(uintptr_t)1)\n\titem->util = DELETE_PACK;\n\nmay still reap the same simplification benefit without introducing\nthe \"what is that _one_ about? can it be _two_ without changing the\nmeaning or what?\" puzzlement.\n\nOther than that, everything else in the series looked quite\nstraight-forward and sensible.\n\nThanks.\n\n\n"},{"id":"481444","messageId":"ZPi1c98o2fKB/U+e@nand.local","threadId":"60201","inReplyTo":"xmqqh6o7nsf2.fsf@gitster.g","subject":"Re: [PATCH 7/8] builtin/repack.c: drop `DELETE_PACK` macro","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-09-06T17:22:59Z","receivedAt":"2023-09-06T17:23:04Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Sep 06, 2023 at 10:05:37AM -0700, Junio C Hamano wrote:\n> Taylor Blau writes:\n>\n> > Signed-off-by: Taylor Blau \n> > ---\n> > builtin/repack.c | 18 ++++++++++--------\n> > 1 file changed, 10 insertions(+), 8 deletions(-)\n>\n> The reason being...?\n\nWow, I have no idea how this got sent out without a commit message! At\nleast it was signed off ;-).\n\nHere's a replacement that I cooked up, with your Helped-by. Let me know\nif you want to replace this patch manually, or if you'd prefer a reroll\nof the series. Either is fine with me! :-)\n\n--- 8< ---\nSubject: [PATCH] builtin/repack.c: treat string_list_item util as booleans\n\nThe `->util` field corresponding to each string_list_item used to track\nthe existence of some pack at the beginning of a repack operation was\noriginally intended to be used as a bitfield.\n\nThis bitfield tracked:\n\n - (1 << 0): whether or not the pack should be deleted\n - (1 << 1): whether or not the pack is cruft\n\nThe previous commit removed the use of the second bit, meaning that we\ncan treat this field as a boolean instead of a bitset.\n\nHelped-by: Junio C Hamano \nSigned-off-by: Taylor Blau \n---\n builtin/repack.c | 18 +++++++++++-------\n 1 file changed, 11 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 478fab96c9..575e69b020 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -26,7 +26,7 @@\n #define LOOSEN_UNREACHABLE 2\n #define PACK_CRUFT 4\n\n-#define DELETE_PACK 1\n+#define DELETE_PACK ((void*)(uintptr_t)1)\n\n static int pack_everything;\n static int delta_base_offset = 1;\n@@ -96,6 +96,10 @@ static int repack_config(const char *var, const char *value,\n\n struct existing_packs {\n \tstruct string_list kept_packs;\n+\t/*\n+\t * for both non_kept_packs, and cruft_packs, a non-NULL\n+\t * 'util' field indicates the pack should be deleted.\n+\t */\n \tstruct string_list non_kept_packs;\n \tstruct string_list cruft_packs;\n };\n@@ -130,7 +134,7 @@ static void mark_packs_for_deletion_1(struct string_list *names,\n \t\t * (if `-d` was given).\n \t\t */\n \t\tif (!string_list_has_string(names, sha1))\n-\t\t\titem->util = (void*)(uintptr_t)((size_t)item->util | DELETE_PACK);\n+\t\t\titem->util = DELETE_PACK;\n \t}\n }\n\n@@ -158,7 +162,7 @@ static void remove_redundant_packs_1(struct string_list *packs)\n {\n \tstruct string_list_item *item;\n \tfor_each_string_list_item(item, packs) {\n-\t\tif (!((uintptr_t)item->util & DELETE_PACK))\n+\t\tif (!item->util)\n \t\t\tcontinue;\n \t\tremove_redundant_pack(packdir, item->string);\n \t}\n@@ -695,20 +699,20 @@ static void midx_included_packs(struct string_list *include,\n\n \t\tfor_each_string_list_item(item, &existing->cruft_packs) {\n \t\t\t/*\n-\t\t\t * no need to check DELETE_PACK, since we're not\n-\t\t\t * doing an ALL_INTO_ONE repack\n+\t\t\t * no need to check for deleted packs, since we're\n+\t\t\t * not doing an ALL_INTO_ONE repack\n \t\t\t */\n \t\t\tstring_list_insert(include, xstrfmt(\"%s.idx\", item->string));\n \t\t}\n \t} else {\n \t\tfor_each_string_list_item(item, &existing->non_kept_packs) {\n-\t\t\tif ((uintptr_t)item->util & DELETE_PACK)\n+\t\t\tif (item->util)\n \t\t\t\tcontinue;\n \t\t\tstring_list_insert(include, xstrfmt(\"%s.idx\", item->string));\n \t\t}\n\n \t\tfor_each_string_list_item(item, &existing->cruft_packs) {\n-\t\t\tif ((uintptr_t)item->util & DELETE_PACK)\n+\t\t\tif (item->util)\n \t\t\t\tcontinue;\n \t\t\tstring_list_insert(include, xstrfmt(\"%s.idx\", item->string));\n \t\t}\n--\n2.38.0.16.g393fd4c6db\n\n--- >8 ---\n\nThanks,\nTaylor\n"},{"id":"481467","messageId":"ZPmHpqHNzXF0Jbu6@tanuki","threadId":"60201","inReplyTo":"ZPi1c98o2fKB/U+e@nand.local","subject":"Re: [PATCH 7/8] builtin/repack.c: drop `DELETE_PACK` macro","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-09-07T08:19:50Z","receivedAt":"2023-09-07T15:51:31Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Sep 06, 2023 at 01:22:59PM -0400, Taylor Blau wrote:\n> On Wed, Sep 06, 2023 at 10:05:37AM -0700, Junio C Hamano wrote:\n> > Taylor Blau writes:\n> >\n> > > Signed-off-by: Taylor Blau \n> > > ---\n> > > builtin/repack.c | 18 ++++++++++--------\n> > > 1 file changed, 10 insertions(+), 8 deletions(-)\n> >\n> > The reason being...?\n> \n> Wow, I have no idea how this got sent out without a commit message! At\n> least it was signed off ;-).\n> \n> Here's a replacement that I cooked up, with your Helped-by. Let me know\n> if you want to replace this patch manually, or if you'd prefer a reroll\n> of the series. Either is fine with me! :-)\n\nPersonally I think that the original version is still more readable. If\nyou simply see `if (item->util)` you don't really have any indicator\nwhat this actually means, whereas previously the more verbose check for\n`if ((uintptr)item->util & DELETE_PACK)` gives the reader a nice hint\nwhat this may be about.\n\nIf the intent is to make this check a bit prettier, how about we instead\nintroduce a helper function like the following:\n\n```\nstatic inline int pack_marked_for_deletion(const struct string_list_item *item)\n{\n return (uintptr) item->util & DELETE_PACK;\n}\n```\n\nOther than that the rest of this series looks good to me, thanks.\n\nPatrick\n\n> --- 8< ---\n> Subject: [PATCH] builtin/repack.c: treat string_list_item util as booleans\n> \n> The `->util` field corresponding to each string_list_item used to track\n> the existence of some pack at the beginning of a repack operation was\n> originally intended to be used as a bitfield.\n> \n> This bitfield tracked:\n> \n> - (1 << 0): whether or not the pack should be deleted\n> - (1 << 1): whether or not the pack is cruft\n> \n> The previous commit removed the use of the second bit, meaning that we\n> can treat this field as a boolean instead of a bitset.\n> \n> Helped-by: Junio C Hamano \n> Signed-off-by: Taylor Blau \n> ---\n> builtin/repack.c | 18 +++++++++++-------\n> 1 file changed, 11 insertions(+), 7 deletions(-)\n> \n> diff --git a/builtin/repack.c b/builtin/repack.c\n> index 478fab96c9..575e69b020 100644\n> --- a/builtin/repack.c\n> +++ b/builtin/repack.c\n> @@ -26,7 +26,7 @@\n> #define LOOSEN_UNREACHABLE 2\n> #define PACK_CRUFT 4\n> \n> -#define DELETE_PACK 1\n> +#define DELETE_PACK ((void*)(uintptr_t)1)\n> \n> static int pack_everything;\n> static int delta_base_offset = 1;\n> @@ -96,6 +96,10 @@ static int repack_config(const char *var, const char *value,\n> \n> struct existing_packs {\n> \tstruct string_list kept_packs;\n> +\t/*\n> +\t * for both non_kept_packs, and cruft_packs, a non-NULL\n> +\t * 'util' field indicates the pack should be deleted.\n> +\t */\n> \tstruct string_list non_kept_packs;\n> \tstruct string_list cruft_packs;\n> };\n> @@ -130,7 +134,7 @@ static void mark_packs_for_deletion_1(struct string_list *names,\n> \t\t * (if `-d` was given).\n> \t\t */\n> \t\tif (!string_list_has_string(names, sha1))\n> -\t\t\titem->util = (void*)(uintptr_t)((size_t)item->util | DELETE_PACK);\n> +\t\t\titem->util = DELETE_PACK;\n> \t}\n> }\n> \n> @@ -158,7 +162,7 @@ static void remove_redundant_packs_1(struct string_list *packs)\n> {\n> \tstruct string_list_item *item;\n> \tfor_each_string_list_item(item, packs) {\n> -\t\tif (!((uintptr_t)item->util & DELETE_PACK))\n> +\t\tif (!item->util)\n> \t\t\tcontinue;\n> \t\tremove_redundant_pack(packdir, item->string);\n> \t}\n> @@ -695,20 +699,20 @@ static void midx_included_packs(struct string_list *include,\n> \n> \t\tfor_each_string_list_item(item, &existing->cruft_packs) {\n> \t\t\t/*\n> -\t\t\t * no need to check DELETE_PACK, since we're not\n> -\t\t\t * doing an ALL_INTO_ONE repack\n> +\t\t\t * no need to check for deleted packs, since we're\n> +\t\t\t * not doing an ALL_INTO_ONE repack\n> \t\t\t */\n> \t\t\tstring_list_insert(include, xstrfmt(\"%s.idx\", item->string));\n> \t\t}\n> \t} else {\n> \t\tfor_each_string_list_item(item, &existing->non_kept_packs) {\n> -\t\t\tif ((uintptr_t)item->util & DELETE_PACK)\n> +\t\t\tif (item->util)\n> \t\t\t\tcontinue;\n> \t\t\tstring_list_insert(include, xstrfmt(\"%s.idx\", item->string));\n> \t\t}\n> \n> \t\tfor_each_string_list_item(item, &existing->cruft_packs) {\n> -\t\t\tif ((uintptr_t)item->util & DELETE_PACK)\n> +\t\t\tif (item->util)\n> \t\t\t\tcontinue;\n> \t\t\tstring_list_insert(include, xstrfmt(\"%s.idx\", item->string));\n> \t\t}\n> --\n> 2.38.0.16.g393fd4c6db\n> \n> --- >8 ---\n> \n> Thanks,\n> Taylor\n"},{"id":"481497","messageId":"xmqq1qf9lugh.fsf@gitster.g","threadId":"60201","inReplyTo":"ZPmHpqHNzXF0Jbu6@tanuki","subject":"Re: [PATCH 7/8] builtin/repack.c: drop `DELETE_PACK` macro","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-09-07T18:16:46Z","receivedAt":"2023-09-07T18:17:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> If the intent is to make this check a bit prettier, how about we instead\n> introduce a helper function like the following:\n>\n> ```\n> static inline int pack_marked_for_deletion(const struct string_list_item *item)\n> {\n> return (uintptr) item->util & DELETE_PACK;\n> }\n> ```\n\nGood suggestion.\n\nOr just check if it is NULL (with the new code that only cares about\nthe NULL-ness). Regardless of the implementation, having such a\nhelper would document the intent of the code better, especially if\nthere are multiple places that make that check.\n\n> Other than that the rest of this series looks good to me, thanks.\n>\n> Patrick\n\nThanks.\n"},{"id":"481502","messageId":"20230907075400.GA1260718@coredump.intra.peff.net","threadId":"60201","inReplyTo":"5b48b7e3cc03c83465a3dcecaa98b9d2e9667084.1693946195.git.me@ttaylorr.com","subject":"Re: [PATCH 1/8] builtin/repack.c: extract structure to store existing packs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-09-07T07:54:00Z","receivedAt":"2023-09-07T19:00:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 05, 2023 at 04:36:40PM -0400, Taylor Blau wrote:\n\n> The repack machinery needs to keep track of which packfiles were present\n> in the repository at the beginning of a repack, segmented by whether or\n> not each pack is marked as kept.\n> \n> The names of these packs are stored in two `string_list`s, corresponding\n> to kept- and non-kept packs, respectively. As a consequence, many\n> functions within the repack code need to take both `string_list`s as\n> arguments, leading to code like this:\n> \n> ret = write_cruft_pack(&cruft_po_args, packtmp, pack_prefix,\n> cruft_expiration, &names,\n> &existing_nonkept_packs, /* <- */\n> &existing_kept_packs); /* <- */\n> \n> Wrap up this pair of `string_list`s into a single structure that stores\n> both. This saves us from having to pass both string lists separately,\n> and prepares for adding additional fields to this structure.\n\nMakes sense. Even without any additional fields, the grouping makes the\ncode a bit easier to follow.\n\nPatch is noisy, but looks correct. :)\n\n-Peff\n"},{"id":"481503","messageId":"20230907075931.GB1260718@coredump.intra.peff.net","threadId":"60201","inReplyTo":"313537ef68892b15e772eaad8937a4a8c7ebbe61.1693946195.git.me@ttaylorr.com","subject":"Re: [PATCH 2/8] builtin/repack.c: extract marking packs for deletion","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-09-07T07:59:31Z","receivedAt":"2023-09-07T19:06:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 05, 2023 at 04:36:43PM -0400, Taylor Blau wrote:\n\n> At the end of a repack (when given `-d`), Git attempts to remove any\n> packs which have been made \"redundant\" as a result of the repacking\n> operation. For example, an all-into-one (`-A` or `-a`) repack makes\n> every pre-existing pack which is not marked as kept redundant. Geometric\n> repacks (with `--geometric=`) make any packs which were rolled up\n> redundant, and so on.\n> \n> But before deleting the set of packs we think are redundant, we first\n> check to see whether or not we just wrote a pack which is identical to\n> any one of the packs we were going to delete. When this is the case, Git\n> must avoid deleting that pack, since it matches a pack we just wrote\n> (so deleting it may cause the repository to become corrupt).\n> \n> Right now we only process the list of non-kept packs in a single pass.\n> But a future change will split the existing non-kept packs further into\n> two lists: one for cruft packs, and another for non-cruft packs.\n> \n> Factor out this routine to prepare for calling it twice on two separate\n> lists in a future patch.\n\nThere are really two \"factor outs\" here: we pull the code from\ncmd_repack() into a helper, and then the helper is also just a thin\nwrapper around its \"_1\" variant. That latter part isn't needed yet, but\nI can guess from your description that we'll eventually have the main\nfunction dispatch to the \"_1\" helper for lists.\n\nThe main caller in cmd_repack() could do that double-dispatch, but then\nthis code:\n\n> +\tif (delete_redundant && pack_everything & ALL_INTO_ONE)\n> +\t\tmark_packs_for_deletion(&existing, &names);\n\nwould know more about how \"existing_packs\" work than it needs to. So\nthis seems like a good split (and the two-liner above is making\ncmd_repack() much more readable than the big loop it had in the\npre-image).\n\n-Peff\n"},{"id":"481504","messageId":"20230907080026.GC1260718@coredump.intra.peff.net","threadId":"60201","inReplyTo":"5c25ef87c1430e012a2e48b738b3b5aa760b4b0f.1693946195.git.me@ttaylorr.com","subject":"Re: [PATCH 3/8] builtin/repack.c: extract redundant pack cleanup for --geometric","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-09-07T08:00:26Z","receivedAt":"2023-09-07T19:07:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 05, 2023 at 04:36:46PM -0400, Taylor Blau wrote:\n\n> To reduce the complexity of the already quite-long `cmd_repack()`\n> implementation, extract out the parts responsible for deleting redundant\n> packs from a geometric repack out into its own sub-routine.\n\nMakes sense. Again, I'm happy to see some of these large functional\npieces of cmd_repack() moved into sub-functions. Even if we never call\nthem twice, IMHO it is a readability improvement to give them meaningful\nnames.\n\n-Peff\n"},{"id":"481505","messageId":"20230907080240.GD1260718@coredump.intra.peff.net","threadId":"60201","inReplyTo":"7bb543fef8b140726b6b3efc2bb2cb1a6384fcd9.1693946195.git.me@ttaylorr.com","subject":"Re: [PATCH 4/8] builtin/repack.c: extract redundant pack cleanup for existing packs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-09-07T08:02:40Z","receivedAt":"2023-09-07T19:09:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 05, 2023 at 04:36:48PM -0400, Taylor Blau wrote:\n\n> To remove redundant packs at the end of a repacking operation, Git uses\n> its `remove_redundant_pack()` function in a loop over the set of\n> pre-existing, non-kept packs.\n> \n> In a later commit, we will split this list into two, one for\n> pre-existing cruft pack(s), and another for non-cruft pack(s). Prepare\n> for this by factoring out the routine to loop over and delete redundant\n> packs into its own function.\n> \n> Instead of calling `remove_redundant_pack()` directly, we now will call\n> `remove_redundant_existing_packs()`, which itself dispatches a call to\n> `remove_redundant_packs_1()`. Note that the geometric repacking code\n> will still call `remove_redundant_pack()` directly, but see the previous\n> commit for more details.\n> \n> Having `remove_redundant_packs_1()` exist as a separate function may\n> seem like overkill in this patch. However, a later patch will call\n> `remove_redundant_packs_1()` once over two separate lists, so this\n> refactoring sets us up for that.\n\nHeh, so this is basically the same \"_1\" case discussed in the earlier\npatch. This commit message explains the split a bit better, IMHO. :)\n(Not worth re-rolling; just thinking out loud).\n\n-Peff\n"},{"id":"481506","messageId":"20230907080428.GE1260718@coredump.intra.peff.net","threadId":"60201","inReplyTo":"e2cf87bb94da1a777f6b87ae3f32e036832839a6.1693946195.git.me@ttaylorr.com","subject":"Re: [PATCH 5/8] builtin/repack.c: extract `has_existing_non_kept_packs()`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-09-07T08:04:28Z","receivedAt":"2023-09-07T19:11:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 05, 2023 at 04:36:51PM -0400, Taylor Blau wrote:\n\n> But a following change will store cruft- and non-cruft packs separately,\n> meaning this check would break as a result. Prepare for this by\n> extracting this part of the check into a new helper function called\n> `has_existing_non_kept_packs()`.\n> \n> This patch does not introduce any functional changes, but prepares us to\n> make a more isolated change in a subsequent patch.\n\nOK, that makes sense.\n\n-Peff\n"},{"id":"481507","messageId":"20230907080913.GF1260718@coredump.intra.peff.net","threadId":"60201","inReplyTo":"414a558883830a29924710126960074b37ab97fc.1693946195.git.me@ttaylorr.com","subject":"Re: [PATCH 6/8] builtin/repack.c: store existing cruft packs separately","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-09-07T08:09:13Z","receivedAt":"2023-09-07T19:15:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 05, 2023 at 04:36:54PM -0400, Taylor Blau wrote:\n\n> When repacking with the `--write-midx` option, we invoke the function\n> `midx_included_packs()` in order to produce the list of packs we want to\n> include in the resulting MIDX.\n> \n> This list is comprised of:\n> \n> - existing .keep packs\n> - any pack(s) which were written earlier in the same process\n> - any unchanged packs when doing a `--geometric` repack\n> - any cruft packs\n> \n> Prior to this patch, we stored pre-existing cruft and non-cruft packs\n> together (provided those packs are non-kept). This meant we needed an\n> additional bit to indicate which non-kept pack(s) were cruft versus\n> those that aren't.\n> \n> But alternatively we can store cruft packs in a separate list, avoiding\n> the need for this extra bit, and simplifying the code below.\n\nOK. Getting rid of the extra bit is nice. We shorten code that only\ncares about cruft packs like:\n\n for each pack\n if (pack is cruft)\n ...\n\nto just:\n\n for each cruft_pack\n ...\n\nwhich is good. But the flip side is that any existing code which looks\nat the combined list now has to do:\n\n for each pack\n ...\n for each cruft_pack\n ...\n\nI think there's just one case of that, here:\n\n> @@ -707,6 +706,12 @@ static void midx_included_packs(struct string_list *include,\n> \t\t\t\tcontinue;\n> \t\t\tstring_list_insert(include, xstrfmt(\"%s.idx\", item->string));\n> \t\t}\n> +\n> +\t\tfor_each_string_list_item(item, &existing->cruft_packs) {\n> +\t\t\tif ((uintptr_t)item->util & DELETE_PACK)\n> +\t\t\t\tcontinue;\n> +\t\t\tstring_list_insert(include, xstrfmt(\"%s.idx\", item->string));\n> +\t\t}\n> \t}\n> }\n\nIt may be an OK price to pay if this lets us keep cleaning things up\n(especially if we could get rid of that util casting entirely!). Let's\nread on...\n\n-Peff\n"},{"id":"481509","messageId":"20230907085828.GG1260718@coredump.intra.peff.net","threadId":"60201","inReplyTo":"ZPi1c98o2fKB/U+e@nand.local","subject":"Re: [PATCH 7/8] builtin/repack.c: drop `DELETE_PACK` macro","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-09-07T08:58:28Z","receivedAt":"2023-09-07T20:06:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 06, 2023 at 01:22:59PM -0400, Taylor Blau wrote:\n\n> --- 8< ---\n> Subject: [PATCH] builtin/repack.c: treat string_list_item util as booleans\n> \n> The `->util` field corresponding to each string_list_item used to track\n> the existence of some pack at the beginning of a repack operation was\n> originally intended to be used as a bitfield.\n> \n> This bitfield tracked:\n> \n> - (1 << 0): whether or not the pack should be deleted\n> - (1 << 1): whether or not the pack is cruft\n> \n> The previous commit removed the use of the second bit, meaning that we\n> can treat this field as a boolean instead of a bitset.\n\nI do think the boolean is syntactically a little nicer than the bit-set,\njust because of the casting we have to with the void pointer). After\nreading the earlier parts, I was hoping the culmination of this series\nwould be dropping the use of util entirely (presumably in favor of\nseparate lists). But maybe that would be too disruptive; I didn't try\nit.\n\n> -#define DELETE_PACK 1\n> +#define DELETE_PACK ((void*)(uintptr_t)1)\n> [...]\n> @@ -130,7 +134,7 @@ static void mark_packs_for_deletion_1(struct string_list *names,\n> \t\t * (if `-d` was given).\n> \t\t */\n> \t\tif (!string_list_has_string(names, sha1))\n> -\t\t\titem->util = (void*)(uintptr_t)((size_t)item->util | DELETE_PACK);\n> +\t\t\titem->util = DELETE_PACK;\n> \t}\n> }\n\nI do like the use of the macro here to make the meaning of the boolean\nmore plain, since the name \"util\" is totally meaningless (but we are\nstuck with it).\n\nBut on the other side, things get more mysterious:\n\n> @@ -158,7 +162,7 @@ static void remove_redundant_packs_1(struct string_list *packs)\n> {\n> \tstruct string_list_item *item;\n> \tfor_each_string_list_item(item, packs) {\n> -\t\tif (!((uintptr_t)item->util & DELETE_PACK))\n> +\t\tif (!item->util)\n> \t\t\tcontinue;\n\nThis is syntactically much nicer, but the meaning of \"util\" as a boolean\nfor \"we should delete this\" is lost.\n\nSo I dunno. The end result of this patch is more readable syntactically,\nbut arguably less so semantically. Unless we have a good reason to ditch\nthe bit-set entirely, I wonder if we could have the best of both with\nsome macro helpers. Or even a set of matching helper functions like:\n\n void pack_mark_for_deletion(struct string_list_item *item)\n {\n\t(uintptr_t)item->util = 1;\n }\n\n int pack_should_deleted(const struct string_list_item *item)\n {\n\treturn item->util;\n }\n\nwhich could be a bool or a bit-set; the callers no longer need to care\nand get to use human-readable names.\n\nI dunno. It is a file-local convention and there aren't that many spots,\nso maybe it is not worth worrying too much about. I'm pretty sure that I\ngot confused by the use of \"util\" here when looking at the code before,\nas it did not have the DELETE_PACK name until your 72263ffc32\n(builtin/repack.c: use named flags for existing_packs, 2022-05-20). But\nmaybe the comment that you added is sufficient.\n\nIf we had generic pointer-bitset macros, then perhaps other string-list\nusers could benefit, too. I thought maybe fast-export could use this for\nits mark_to_ptr() stuff, but that is storing a whole 32-bit value, not a\nbitset. So maybe this is just a weird localized thing.\n\nA more radical idea is that we don't care very much about the data\nstructure as long as it is ordered. So we could just do:\n\n struct existing_pack {\n struct list_head list;\n int to_delete;\n char name[FLEX_ARRAY];\n };\n\nand ditch string-list entirely. That lets us use \"to_delete\" in a\nnatural way. Though I suspect it makes all the _other_ code unreadable,\nas we have to allocate them, deal with list_entry(), and so on.\n\nSo I guess I'm fine with any path (including the one in this patch).\n\n-Peff\n"},{"id":"481510","messageId":"20230907090031.GH1260718@coredump.intra.peff.net","threadId":"60201","inReplyTo":"cover.1693946195.git.me@ttaylorr.com","subject":"Re: [PATCH 0/8] repack: refactor pack snapshot-ing logic","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-09-07T09:00:31Z","receivedAt":"2023-09-07T20:07:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 05, 2023 at 04:36:37PM -0400, Taylor Blau wrote:\n\n> This series refactors some of the 'git repack' internals to keep track\n> of existing packs in a set of string lists stored in a single structure,\n> instead of passing around multiple string lists to different functions\n> throughout the various paths.\n> \n> The result is that the interface around pack deletion, marking packs as\n> redundant, and handling the set of pre-existing packs (both kept and\n> non-kept) is significantly cleaner without introducing any functional\n> changes.\n> \n> I didn't mean to produce so much churn when I started writing these\n> patches, which began as a simple effort to rename a couple of variables\n> for more consistency.\n\nThese all look pretty reasonable to me. I wasn't quite sure about patch\n7 and left some comments there, but all of the options are kind of ugly.\nSince it's limited to that one file, it may not be worth spending too\nmuch time or brain-power on trying to polish it further.\n\n-Peff\n"},{"id":"481529","messageId":"ZPpKbKqJhBJcqyl3@nand.local","threadId":"60201","inReplyTo":"20230907075931.GB1260718@coredump.intra.peff.net","subject":"Re: [PATCH 2/8] builtin/repack.c: extract marking packs for deletion","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-09-07T22:10:52Z","receivedAt":"2023-09-07T22:11:01Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Sep 07, 2023 at 03:59:31AM -0400, Jeff King wrote:\n> There are really two \"factor outs\" here: we pull the code from\n> cmd_repack() into a helper, and then the helper is also just a thin\n> wrapper around its \"_1\" variant. That latter part isn't needed yet, but\n> I can guess from your description that we'll eventually have the main\n> function dispatch to the \"_1\" helper for lists.\n\nYeah... the \"_1\" variant looks ugly in isolation in this patch, but I\nthink makes things cleaner in subsequent patches.\n\nThanks,\nTaylor\n"},{"id":"481809","messageId":"cover.1694632644.git.me@ttaylorr.com","threadId":"60201","inReplyTo":"cover.1693946195.git.me@ttaylorr.com","subject":"[PATCH v2 0/8] repack: refactor pack snapshot-ing logic","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-09-13T19:17:32Z","receivedAt":"2023-09-13T19:17:37Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Here is a small reroll of my series to clean up some of the internals of\n'git repack' used to track the set of existing packs.\n\nMuch is unchanged from the last round, save for some additional clean-up\non how we handle the '->util' field for each pack's string_list_item in\nresponse to very helpful review from those CC'd.\n\nAs usual, a range-diff is available below for convenience. Thanks in\nadvance for your review!\n\nTaylor Blau (8):\n builtin/repack.c: extract structure to store existing packs\n builtin/repack.c: extract marking packs for deletion\n builtin/repack.c: extract redundant pack cleanup for --geometric\n builtin/repack.c: extract redundant pack cleanup for existing packs\n builtin/repack.c: extract `has_existing_non_kept_packs()`\n builtin/repack.c: store existing cruft packs separately\n builtin/repack.c: avoid directly inspecting \"util\"\n builtin/repack.c: extract common cruft pack loop\n\n builtin/repack.c | 293 +++++++++++++++++++++++++++++------------------\n 1 file changed, 180 insertions(+), 113 deletions(-)\n\nRange-diff against v1:\n1: 5b48b7e3cc = 1: 2e26beff22 builtin/repack.c: extract structure to store existing packs\n2: 313537ef68 = 2: 62d916169d builtin/repack.c: extract marking packs for deletion\n3: 5c25ef87c1 = 3: 7ed45804ea builtin/repack.c: extract redundant pack cleanup for --geometric\n4: 7bb543fef8 = 4: 82057de4cf builtin/repack.c: extract redundant pack cleanup for existing packs\n5: e2cf87bb94 = 5: f4f7b4c08f builtin/repack.c: extract `has_existing_non_kept_packs()`\n6: 414a558883 = 6: d68a88dbd5 builtin/repack.c: store existing cruft packs separately\n7: 559b487e2a ! 7: 481a29599b builtin/repack.c: drop `DELETE_PACK` macro\n @@ Metadata\n Author: Taylor Blau \n \n ## Commit message ##\n - builtin/repack.c: drop `DELETE_PACK` macro\n + builtin/repack.c: avoid directly inspecting \"util\"\n \n + The `->util` field corresponding to each string_list_item is used to\n + track the existence of some pack at the beginning of a repack operation\n + was originally intended to be used as a bitfield.\n +\n + This bitfield tracked:\n +\n + - (1 << 0): whether or not the pack should be deleted\n + - (1 << 1): whether or not the pack is cruft\n +\n + The previous commit removed the use of the second bit, but a future\n + patch (from a different series than this one) will introduce a new use\n + of it.\n +\n + So we could stop treating the util pointer as a bitfield and instead\n + start treating it as if it were a boolean. But this would require some\n + backtracking when that later patch is applied.\n +\n + Instead, let's avoid touching the ->util field directly, and instead\n + introduce convenience functions like:\n +\n + - pack_mark_for_deletion()\n + - pack_is_marked_for_deletion()\n +\n + Helped-by: Junio C Hamano \n + Helped-by: Jeff King \n + Helped-by: Patrick Steinhardt \n Signed-off-by: Taylor Blau \n \n ## builtin/repack.c ##\n -@@\n - #define LOOSEN_UNREACHABLE 2\n - #define PACK_CRUFT 4\n +@@ builtin/repack.c: static int has_existing_non_kept_packs(const struct existing_packs *existing)\n + \treturn existing->non_kept_packs.nr || existing->cruft_packs.nr;\n + }\n \n --#define DELETE_PACK 1\n --\n - static int pack_everything;\n - static int delta_base_offset = 1;\n - static int pack_kept_objects = -1;\n -@@ builtin/repack.c: static int repack_config(const char *var, const char *value,\n - \n - struct existing_packs {\n - \tstruct string_list kept_packs;\n -+\t/*\n -+\t * for both non_kept_packs, and cruft_packs, a non-NULL\n -+\t * 'util' field indicates the pack should be deleted.\n -+\t */\n - \tstruct string_list non_kept_packs;\n - \tstruct string_list cruft_packs;\n - };\n ++static void pack_mark_for_deletion(struct string_list_item *item)\n ++{\n ++\titem->util = (void*)((uintptr_t)item->util | DELETE_PACK);\n ++}\n ++\n ++static int pack_is_marked_for_deletion(struct string_list_item *item)\n ++{\n ++\treturn (uintptr_t)item->util & DELETE_PACK;\n ++}\n ++\n + static void mark_packs_for_deletion_1(struct string_list *names,\n + \t\t\t\t struct string_list *list)\n + {\n @@ builtin/repack.c: static void mark_packs_for_deletion_1(struct string_list *names,\n \t\t * (if `-d` was given).\n \t\t */\n \t\tif (!string_list_has_string(names, sha1))\n -\t\t\titem->util = (void*)(uintptr_t)((size_t)item->util | DELETE_PACK);\n -+\t\t\titem->util = (void*)1;\n ++\t\t\tpack_mark_for_deletion(item);\n \t}\n }\n \n @@ builtin/repack.c: static void remove_redundant_packs_1(struct string_list *packs\n \tstruct string_list_item *item;\n \tfor_each_string_list_item(item, packs) {\n -\t\tif (!((uintptr_t)item->util & DELETE_PACK))\n -+\t\tif (!item->util)\n ++\t\tif (!pack_is_marked_for_deletion(item))\n \t\t\tcontinue;\n \t\tremove_redundant_pack(packdir, item->string);\n \t}\n @@ builtin/repack.c: static void midx_included_packs(struct string_list *include,\n - \n - \t\tfor_each_string_list_item(item, &existing->cruft_packs) {\n - \t\t\t/*\n --\t\t\t * no need to check DELETE_PACK, since we're not\n --\t\t\t * doing an ALL_INTO_ONE repack\n -+\t\t\t * no need to check for deleted packs, since we're\n -+\t\t\t * not doing an ALL_INTO_ONE repack\n - \t\t\t */\n - \t\t\tstring_list_insert(include, xstrfmt(\"%s.idx\", item->string));\n \t\t}\n \t} else {\n \t\tfor_each_string_list_item(item, &existing->non_kept_packs) {\n -\t\t\tif ((uintptr_t)item->util & DELETE_PACK)\n -+\t\t\tif (item->util)\n ++\t\t\tif (pack_is_marked_for_deletion(item))\n \t\t\t\tcontinue;\n \t\t\tstring_list_insert(include, xstrfmt(\"%s.idx\", item->string));\n \t\t}\n \n \t\tfor_each_string_list_item(item, &existing->cruft_packs) {\n -\t\t\tif ((uintptr_t)item->util & DELETE_PACK)\n -+\t\t\tif (item->util)\n ++\t\t\tif (pack_is_marked_for_deletion(item))\n \t\t\t\tcontinue;\n \t\t\tstring_list_insert(include, xstrfmt(\"%s.idx\", item->string));\n \t\t}\n8: ca7d13e7bf ! 8: 68748eb9c8 builtin/repack.c: extract common cruft pack loop\n @@ Commit message\n \n ## builtin/repack.c ##\n @@ builtin/repack.c: static void midx_included_packs(struct string_list *include,\n + \n \t\t\tstring_list_insert(include, strbuf_detach(&buf, NULL));\n \t\t}\n - \n +-\n -\t\tfor_each_string_list_item(item, &existing->cruft_packs) {\n -\t\t\t/*\n --\t\t\t * no need to check for deleted packs, since we're\n --\t\t\t * not doing an ALL_INTO_ONE repack\n +-\t\t\t * no need to check DELETE_PACK, since we're not\n +-\t\t\t * doing an ALL_INTO_ONE repack\n -\t\t\t */\n -\t\t\tstring_list_insert(include, xstrfmt(\"%s.idx\", item->string));\n -\t\t}\n \t} else {\n \t\tfor_each_string_list_item(item, &existing->non_kept_packs) {\n - \t\t\tif (item->util)\n + \t\t\tif (pack_is_marked_for_deletion(item))\n \t\t\t\tcontinue;\n \t\t\tstring_list_insert(include, xstrfmt(\"%s.idx\", item->string));\n \t\t}\n +\t}\n \n -\t\tfor_each_string_list_item(item, &existing->cruft_packs) {\n --\t\t\tif (item->util)\n +-\t\t\tif (pack_is_marked_for_deletion(item))\n -\t\t\t\tcontinue;\n -\t\t\tstring_list_insert(include, xstrfmt(\"%s.idx\", item->string));\n -\t\t}\n @@ builtin/repack.c: static void midx_included_packs(struct string_list *include,\n +\t\t * `mark_packs_for_deletion()` when doing an all-into-one\n +\t\t * repack).\n +\t\t */\n -+\t\tif (item->util)\n ++\t\tif (pack_is_marked_for_deletion(item))\n +\t\t\tcontinue;\n +\t\tstring_list_insert(include, xstrfmt(\"%s.idx\", item->string));\n \t}\n-- \n2.42.0.166.g68748eb9c8\n"},{"id":"481810","messageId":"2e26beff22414282ecbbe846113d55845c7d0df8.1694632644.git.me@ttaylorr.com","threadId":"60201","inReplyTo":"cover.1694632644.git.me@ttaylorr.com","subject":"[PATCH v2 1/8] builtin/repack.c: extract structure to store existing packs","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-09-13T19:17:35Z","receivedAt":"2023-09-13T19:17:46Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"The repack machinery needs to keep track of which packfiles were present\nin the repository at the beginning of a repack, segmented by whether or\nnot each pack is marked as kept.\n\nThe names of these packs are stored in two `string_list`s, corresponding\nto kept- and non-kept packs, respectively. As a consequence, many\nfunctions within the repack code need to take both `string_list`s as\narguments, leading to code like this:\n\n ret = write_cruft_pack(&cruft_po_args, packtmp, pack_prefix,\n cruft_expiration, &names,\n &existing_nonkept_packs, /* <- */\n &existing_kept_packs); /* <- */\n\nWrap up this pair of `string_list`s into a single structure that stores\nboth. This saves us from having to pass both string lists separately,\nand prepares for adding additional fields to this structure.\n\nSigned-off-by: Taylor Blau \n---\n builtin/repack.c | 90 ++++++++++++++++++++++++++----------------------\n 1 file changed, 49 insertions(+), 41 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 6943c5ba11..67bf86567f 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -95,14 +95,29 @@ static int repack_config(const char *var, const char *value,\n \treturn git_default_config(var, value, ctx, cb);\n }\n \n+struct existing_packs {\n+\tstruct string_list kept_packs;\n+\tstruct string_list non_kept_packs;\n+};\n+\n+#define EXISTING_PACKS_INIT { \\\n+\t.kept_packs = STRING_LIST_INIT_DUP, \\\n+\t.non_kept_packs = STRING_LIST_INIT_DUP, \\\n+}\n+\n+static void existing_packs_release(struct existing_packs *existing)\n+{\n+\tstring_list_clear(&existing->kept_packs, 0);\n+\tstring_list_clear(&existing->non_kept_packs, 0);\n+}\n+\n /*\n- * Adds all packs hex strings (pack-$HASH) to either fname_nonkept_list\n- * or fname_kept_list based on whether each pack has a corresponding\n+ * Adds all packs hex strings (pack-$HASH) to either packs->non_kept\n+ * or packs->kept based on whether each pack has a corresponding\n * .keep file or not. Packs without a .keep file are not to be kept\n * if we are going to pack everything into one file.\n */\n-static void collect_pack_filenames(struct string_list *fname_nonkept_list,\n-\t\t\t\t struct string_list *fname_kept_list,\n+static void collect_pack_filenames(struct existing_packs *existing,\n \t\t\t\t const struct string_list *extra_keep)\n {\n \tstruct packed_git *p;\n@@ -126,16 +141,16 @@ static void collect_pack_filenames(struct string_list *fname_nonkept_list,\n \t\tstrbuf_strip_suffix(&buf, \".pack\");\n \n \t\tif ((extra_keep->nr > 0 && i < extra_keep->nr) || p->pack_keep)\n-\t\t\tstring_list_append(fname_kept_list, buf.buf);\n+\t\t\tstring_list_append(&existing->kept_packs, buf.buf);\n \t\telse {\n \t\t\tstruct string_list_item *item;\n-\t\t\titem = string_list_append(fname_nonkept_list, buf.buf);\n+\t\t\titem = string_list_append(&existing->non_kept_packs, buf.buf);\n \t\t\tif (p->is_cruft)\n \t\t\t\titem->util = (void*)(uintptr_t)CRUFT_PACK;\n \t\t}\n \t}\n \n-\tstring_list_sort(fname_kept_list);\n+\tstring_list_sort(&existing->kept_packs);\n \tstrbuf_release(&buf);\n }\n \n@@ -327,7 +342,7 @@ static int geometry_cmp(const void *va, const void *vb)\n }\n \n static void init_pack_geometry(struct pack_geometry *geometry,\n-\t\t\t struct string_list *existing_kept_packs,\n+\t\t\t struct existing_packs *existing,\n \t\t\t const struct pack_objects_args *args)\n {\n \tstruct packed_git *p;\n@@ -344,23 +359,24 @@ static void init_pack_geometry(struct pack_geometry *geometry,\n \n \t\tif (!pack_kept_objects) {\n \t\t\t/*\n-\t\t\t * Any pack that has its pack_keep bit set will appear\n-\t\t\t * in existing_kept_packs below, but this saves us from\n-\t\t\t * doing a more expensive check.\n+\t\t\t * Any pack that has its pack_keep bit set will\n+\t\t\t * appear in existing->kept_packs below, but\n+\t\t\t * this saves us from doing a more expensive\n+\t\t\t * check.\n \t\t\t */\n \t\t\tif (p->pack_keep)\n \t\t\t\tcontinue;\n \n \t\t\t/*\n-\t\t\t * The pack may be kept via the --keep-pack option;\n-\t\t\t * check 'existing_kept_packs' to determine whether to\n-\t\t\t * ignore it.\n+\t\t\t * The pack may be kept via the --keep-pack\n+\t\t\t * option; check 'existing->kept_packs' to\n+\t\t\t * determine whether to ignore it.\n \t\t\t */\n \t\t\tstrbuf_reset(&buf);\n \t\t\tstrbuf_addstr(&buf, pack_basename(p));\n \t\t\tstrbuf_strip_suffix(&buf, \".pack\");\n \n-\t\t\tif (string_list_has_string(existing_kept_packs, buf.buf))\n+\t\t\tif (string_list_has_string(&existing->kept_packs, buf.buf))\n \t\t\t\tcontinue;\n \t\t}\n \t\tif (p->is_cruft)\n@@ -565,14 +581,13 @@ static void midx_snapshot_refs(struct tempfile *f)\n }\n \n static void midx_included_packs(struct string_list *include,\n-\t\t\t\tstruct string_list *existing_nonkept_packs,\n-\t\t\t\tstruct string_list *existing_kept_packs,\n+\t\t\t\tstruct existing_packs *existing,\n \t\t\t\tstruct string_list *names,\n \t\t\t\tstruct pack_geometry *geometry)\n {\n \tstruct string_list_item *item;\n \n-\tfor_each_string_list_item(item, existing_kept_packs)\n+\tfor_each_string_list_item(item, &existing->kept_packs)\n \t\tstring_list_insert(include, xstrfmt(\"%s.idx\", item->string));\n \tfor_each_string_list_item(item, names)\n \t\tstring_list_insert(include, xstrfmt(\"pack-%s.idx\", item->string));\n@@ -600,7 +615,7 @@ static void midx_included_packs(struct string_list *include,\n \t\t\tstring_list_insert(include, strbuf_detach(&buf, NULL));\n \t\t}\n \n-\t\tfor_each_string_list_item(item, existing_nonkept_packs) {\n+\t\tfor_each_string_list_item(item, &existing->non_kept_packs) {\n \t\t\tif (!((uintptr_t)item->util & CRUFT_PACK)) {\n \t\t\t\t/*\n \t\t\t\t * no need to check DELETE_PACK, since we're not\n@@ -611,7 +626,7 @@ static void midx_included_packs(struct string_list *include,\n \t\t\tstring_list_insert(include, xstrfmt(\"%s.idx\", item->string));\n \t\t}\n \t} else {\n-\t\tfor_each_string_list_item(item, existing_nonkept_packs) {\n+\t\tfor_each_string_list_item(item, &existing->non_kept_packs) {\n \t\t\tif ((uintptr_t)item->util & DELETE_PACK)\n \t\t\t\tcontinue;\n \t\t\tstring_list_insert(include, xstrfmt(\"%s.idx\", item->string));\n@@ -700,8 +715,7 @@ static int write_cruft_pack(const struct pack_objects_args *args,\n \t\t\t const char *pack_prefix,\n \t\t\t const char *cruft_expiration,\n \t\t\t struct string_list *names,\n-\t\t\t struct string_list *existing_packs,\n-\t\t\t struct string_list *existing_kept_packs)\n+\t\t\t struct existing_packs *existing)\n {\n \tstruct child_process cmd = CHILD_PROCESS_INIT;\n \tstruct strbuf line = STRBUF_INIT;\n@@ -743,9 +757,9 @@ static int write_cruft_pack(const struct pack_objects_args *args,\n \tin = xfdopen(cmd.in, \"w\");\n \tfor_each_string_list_item(item, names)\n \t\tfprintf(in, \"%s-%s.pack\\n\", pack_prefix, item->string);\n-\tfor_each_string_list_item(item, existing_packs)\n+\tfor_each_string_list_item(item, &existing->non_kept_packs)\n \t\tfprintf(in, \"-%s.pack\\n\", item->string);\n-\tfor_each_string_list_item(item, existing_kept_packs)\n+\tfor_each_string_list_item(item, &existing->kept_packs)\n \t\tfprintf(in, \"%s.pack\\n\", item->string);\n \tfclose(in);\n \n@@ -777,8 +791,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \tstruct child_process cmd = CHILD_PROCESS_INIT;\n \tstruct string_list_item *item;\n \tstruct string_list names = STRING_LIST_INIT_DUP;\n-\tstruct string_list existing_nonkept_packs = STRING_LIST_INIT_DUP;\n-\tstruct string_list existing_kept_packs = STRING_LIST_INIT_DUP;\n+\tstruct existing_packs existing = EXISTING_PACKS_INIT;\n \tstruct pack_geometry geometry = { 0 };\n \tstruct strbuf line = STRBUF_INIT;\n \tstruct tempfile *refs_snapshot = NULL;\n@@ -914,13 +927,12 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \tpacktmp_name = xstrfmt(\".tmp-%d-pack\", (int)getpid());\n \tpacktmp = mkpathdup(\"%s/%s\", packdir, packtmp_name);\n \n-\tcollect_pack_filenames(&existing_nonkept_packs, &existing_kept_packs,\n-\t\t\t &keep_pack_list);\n+\tcollect_pack_filenames(&existing, &keep_pack_list);\n \n \tif (geometry.split_factor) {\n \t\tif (pack_everything)\n \t\t\tdie(_(\"options '%s' and '%s' cannot be used together\"), \"--geometric\", \"-A/-a\");\n-\t\tinit_pack_geometry(&geometry, &existing_kept_packs, &po_args);\n+\t\tinit_pack_geometry(&geometry, &existing, &po_args);\n \t\tsplit_pack_geometry(&geometry);\n \t}\n \n@@ -964,7 +976,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \tif (pack_everything & ALL_INTO_ONE) {\n \t\trepack_promisor_objects(&po_args, &names);\n \n-\t\tif (existing_nonkept_packs.nr && delete_redundant &&\n+\t\tif (existing.non_kept_packs.nr && delete_redundant &&\n \t\t !(pack_everything & PACK_CRUFT)) {\n \t\t\tfor_each_string_list_item(item, &names) {\n \t\t\t\tstrvec_pushf(&cmd.args, \"--keep-pack=%s-%s.pack\",\n@@ -1055,8 +1067,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \n \t\tret = write_cruft_pack(&cruft_po_args, packtmp, pack_prefix,\n \t\t\t\t cruft_expiration, &names,\n-\t\t\t\t &existing_nonkept_packs,\n-\t\t\t\t &existing_kept_packs);\n+\t\t\t\t &existing);\n \t\tif (ret)\n \t\t\tgoto cleanup;\n \n@@ -1087,8 +1098,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\t\t\t\t pack_prefix,\n \t\t\t\t\t NULL,\n \t\t\t\t\t &names,\n-\t\t\t\t\t &existing_nonkept_packs,\n-\t\t\t\t\t &existing_kept_packs);\n+\t\t\t\t\t &existing);\n \t\t\tif (ret)\n \t\t\t\tgoto cleanup;\n \t\t}\n@@ -1134,7 +1144,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \n \tif (delete_redundant && pack_everything & ALL_INTO_ONE) {\n \t\tconst int hexsz = the_hash_algo->hexsz;\n-\t\tfor_each_string_list_item(item, &existing_nonkept_packs) {\n+\t\tfor_each_string_list_item(item, &existing.non_kept_packs) {\n \t\t\tchar *sha1;\n \t\t\tsize_t len = strlen(item->string);\n \t\t\tif (len < hexsz)\n@@ -1153,8 +1163,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \n \tif (write_midx) {\n \t\tstruct string_list include = STRING_LIST_INIT_NODUP;\n-\t\tmidx_included_packs(&include, &existing_nonkept_packs,\n-\t\t\t\t &existing_kept_packs, &names, &geometry);\n+\t\tmidx_included_packs(&include, &existing, &names, &geometry);\n \n \t\tret = write_midx_included_packs(&include, &geometry,\n \t\t\t\t\t\trefs_snapshot ? get_tempfile_path(refs_snapshot) : NULL,\n@@ -1173,7 +1182,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \n \tif (delete_redundant) {\n \t\tint opts = 0;\n-\t\tfor_each_string_list_item(item, &existing_nonkept_packs) {\n+\t\tfor_each_string_list_item(item, &existing.non_kept_packs) {\n \t\t\tif (!((uintptr_t)item->util & DELETE_PACK))\n \t\t\t\tcontinue;\n \t\t\tremove_redundant_pack(packdir, item->string);\n@@ -1194,7 +1203,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\t\t\tstrbuf_strip_suffix(&buf, \".pack\");\n \n \t\t\t\tif ((p->pack_keep) ||\n-\t\t\t\t (string_list_has_string(&existing_kept_packs,\n+\t\t\t\t (string_list_has_string(&existing.kept_packs,\n \t\t\t\t\t\t\t buf.buf)))\n \t\t\t\t\tcontinue;\n \n@@ -1225,8 +1234,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \n cleanup:\n \tstring_list_clear(&names, 1);\n-\tstring_list_clear(&existing_nonkept_packs, 0);\n-\tstring_list_clear(&existing_kept_packs, 0);\n+\texisting_packs_release(&existing);\n \tfree_pack_geometry(&geometry);\n \n \treturn ret;\n-- \n2.42.0.166.g68748eb9c8\n\n"},{"id":"481811","messageId":"62d916169d33a7569d83b72dfb1cb90d56568f01.1694632644.git.me@ttaylorr.com","threadId":"60201","inReplyTo":"cover.1694632644.git.me@ttaylorr.com","subject":"[PATCH v2 2/8] builtin/repack.c: extract marking packs for deletion","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-09-13T19:17:46Z","receivedAt":"2023-09-13T19:17:50Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"At the end of a repack (when given `-d`), Git attempts to remove any\npacks which have been made \"redundant\" as a result of the repacking\noperation. For example, an all-into-one (`-A` or `-a`) repack makes\nevery pre-existing pack which is not marked as kept redundant. Geometric\nrepacks (with `--geometric=`) make any packs which were rolled up\nredundant, and so on.\n\nBut before deleting the set of packs we think are redundant, we first\ncheck to see whether or not we just wrote a pack which is identical to\nany one of the packs we were going to delete. When this is the case, Git\nmust avoid deleting that pack, since it matches a pack we just wrote\n(so deleting it may cause the repository to become corrupt).\n\nRight now we only process the list of non-kept packs in a single pass.\nBut a future change will split the existing non-kept packs further into\ntwo lists: one for cruft packs, and another for non-cruft packs.\n\nFactor out this routine to prepare for calling it twice on two separate\nlists in a future patch.\n\nSigned-off-by: Taylor Blau \n---\n builtin/repack.c | 50 +++++++++++++++++++++++++++++++-----------------\n 1 file changed, 32 insertions(+), 18 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 67bf86567f..0eee430951 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -105,6 +105,36 @@ struct existing_packs {\n \t.non_kept_packs = STRING_LIST_INIT_DUP, \\\n }\n \n+static void mark_packs_for_deletion_1(struct string_list *names,\n+\t\t\t\t struct string_list *list)\n+{\n+\tstruct string_list_item *item;\n+\tconst int hexsz = the_hash_algo->hexsz;\n+\n+\tfor_each_string_list_item(item, list) {\n+\t\tchar *sha1;\n+\t\tsize_t len = strlen(item->string);\n+\t\tif (len < hexsz)\n+\t\t\tcontinue;\n+\t\tsha1 = item->string + len - hexsz;\n+\t\t/*\n+\t\t * Mark this pack for deletion, which ensures that this\n+\t\t * pack won't be included in a MIDX (if `--write-midx`\n+\t\t * was given) and that we will actually delete this pack\n+\t\t * (if `-d` was given).\n+\t\t */\n+\t\tif (!string_list_has_string(names, sha1))\n+\t\t\titem->util = (void*)(uintptr_t)((size_t)item->util | DELETE_PACK);\n+\t}\n+}\n+\n+static void mark_packs_for_deletion(struct existing_packs *existing,\n+\t\t\t\t struct string_list *names)\n+\n+{\n+\tmark_packs_for_deletion_1(names, &existing->non_kept_packs);\n+}\n+\n static void existing_packs_release(struct existing_packs *existing)\n {\n \tstring_list_clear(&existing->kept_packs, 0);\n@@ -1142,24 +1172,8 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t}\n \t/* End of pack replacement. */\n \n-\tif (delete_redundant && pack_everything & ALL_INTO_ONE) {\n-\t\tconst int hexsz = the_hash_algo->hexsz;\n-\t\tfor_each_string_list_item(item, &existing.non_kept_packs) {\n-\t\t\tchar *sha1;\n-\t\t\tsize_t len = strlen(item->string);\n-\t\t\tif (len < hexsz)\n-\t\t\t\tcontinue;\n-\t\t\tsha1 = item->string + len - hexsz;\n-\t\t\t/*\n-\t\t\t * Mark this pack for deletion, which ensures that this\n-\t\t\t * pack won't be included in a MIDX (if `--write-midx`\n-\t\t\t * was given) and that we will actually delete this pack\n-\t\t\t * (if `-d` was given).\n-\t\t\t */\n-\t\t\tif (!string_list_has_string(&names, sha1))\n-\t\t\t\titem->util = (void*)(uintptr_t)((size_t)item->util | DELETE_PACK);\n-\t\t}\n-\t}\n+\tif (delete_redundant && pack_everything & ALL_INTO_ONE)\n+\t\tmark_packs_for_deletion(&existing, &names);\n \n \tif (write_midx) {\n \t\tstruct string_list include = STRING_LIST_INIT_NODUP;\n-- \n2.42.0.166.g68748eb9c8\n\n"},{"id":"481812","messageId":"7ed45804ea02cdb709de1776daac857425f744fd.1694632644.git.me@ttaylorr.com","threadId":"60201","inReplyTo":"cover.1694632644.git.me@ttaylorr.com","subject":"[PATCH v2 3/8] builtin/repack.c: extract redundant pack cleanup for --geometric","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-09-13T19:17:49Z","receivedAt":"2023-09-13T19:17:55Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"To reduce the complexity of the already quite-long `cmd_repack()`\nimplementation, extract out the parts responsible for deleting redundant\npacks from a geometric repack out into its own sub-routine.\n\nSigned-off-by: Taylor Blau \n---\n builtin/repack.c | 52 +++++++++++++++++++++++++++---------------------\n 1 file changed, 29 insertions(+), 23 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 0eee430951..71366811e9 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -540,6 +540,32 @@ static struct packed_git *get_preferred_pack(struct pack_geometry *geometry)\n \treturn NULL;\n }\n \n+static void geometry_remove_redundant_packs(struct pack_geometry *geometry,\n+\t\t\t\t\t struct string_list *names,\n+\t\t\t\t\t struct existing_packs *existing)\n+{\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tuint32_t i;\n+\n+\tfor (i = 0; i < geometry->split; i++) {\n+\t\tstruct packed_git *p = geometry->pack[i];\n+\t\tif (string_list_has_string(names, hash_to_hex(p->hash)))\n+\t\t\tcontinue;\n+\n+\t\tstrbuf_reset(&buf);\n+\t\tstrbuf_addstr(&buf, pack_basename(p));\n+\t\tstrbuf_strip_suffix(&buf, \".pack\");\n+\n+\t\tif ((p->pack_keep) ||\n+\t\t (string_list_has_string(&existing->kept_packs, buf.buf)))\n+\t\t\tcontinue;\n+\n+\t\tremove_redundant_pack(packdir, buf.buf);\n+\t}\n+\n+\tstrbuf_release(&buf);\n+}\n+\n static void free_pack_geometry(struct pack_geometry *geometry)\n {\n \tif (!geometry)\n@@ -1202,29 +1228,9 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\t\tremove_redundant_pack(packdir, item->string);\n \t\t}\n \n-\t\tif (geometry.split_factor) {\n-\t\t\tstruct strbuf buf = STRBUF_INIT;\n-\n-\t\t\tuint32_t i;\n-\t\t\tfor (i = 0; i < geometry.split; i++) {\n-\t\t\t\tstruct packed_git *p = geometry.pack[i];\n-\t\t\t\tif (string_list_has_string(&names,\n-\t\t\t\t\t\t\t hash_to_hex(p->hash)))\n-\t\t\t\t\tcontinue;\n-\n-\t\t\t\tstrbuf_reset(&buf);\n-\t\t\t\tstrbuf_addstr(&buf, pack_basename(p));\n-\t\t\t\tstrbuf_strip_suffix(&buf, \".pack\");\n-\n-\t\t\t\tif ((p->pack_keep) ||\n-\t\t\t\t (string_list_has_string(&existing.kept_packs,\n-\t\t\t\t\t\t\t buf.buf)))\n-\t\t\t\t\tcontinue;\n-\n-\t\t\t\tremove_redundant_pack(packdir, buf.buf);\n-\t\t\t}\n-\t\t\tstrbuf_release(&buf);\n-\t\t}\n+\t\tif (geometry.split_factor)\n+\t\t\tgeometry_remove_redundant_packs(&geometry, &names,\n+\t\t\t\t\t\t\t&existing);\n \t\tif (show_progress)\n \t\t\topts |= PRUNE_PACKED_VERBOSE;\n \t\tprune_packed_objects(opts);\n-- \n2.42.0.166.g68748eb9c8\n\n"},{"id":"481813","messageId":"82057de4cf3cbba67eb2cbbd8199909ed2108b82.1694632644.git.me@ttaylorr.com","threadId":"60201","inReplyTo":"cover.1694632644.git.me@ttaylorr.com","subject":"[PATCH v2 4/8] builtin/repack.c: extract redundant pack cleanup for existing packs","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-09-13T19:17:51Z","receivedAt":"2023-09-13T19:17:56Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"To remove redundant packs at the end of a repacking operation, Git uses\nits `remove_redundant_pack()` function in a loop over the set of\npre-existing, non-kept packs.\n\nIn a later commit, we will split this list into two, one for\npre-existing cruft pack(s), and another for non-cruft pack(s). Prepare\nfor this by factoring out the routine to loop over and delete redundant\npacks into its own function.\n\nInstead of calling `remove_redundant_pack()` directly, we now will call\n`remove_redundant_existing_packs()`, which itself dispatches a call to\n`remove_redundant_packs_1()`. Note that the geometric repacking code\nwill still call `remove_redundant_pack()` directly, but see the previous\ncommit for more details.\n\nHaving `remove_redundant_packs_1()` exist as a separate function may\nseem like overkill in this patch. However, a later patch will call\n`remove_redundant_packs_1()` once over two separate lists, so this\nrefactoring sets us up for that.\n\nSigned-off-by: Taylor Blau \n---\n builtin/repack.c | 45 ++++++++++++++++++++++++++++-----------------\n 1 file changed, 28 insertions(+), 17 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 71366811e9..b5fb14c017 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -135,6 +135,33 @@ static void mark_packs_for_deletion(struct existing_packs *existing,\n \tmark_packs_for_deletion_1(names, &existing->non_kept_packs);\n }\n \n+static void remove_redundant_pack(const char *dir_name, const char *base_name)\n+{\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tstruct multi_pack_index *m = get_local_multi_pack_index(the_repository);\n+\tstrbuf_addf(&buf, \"%s.pack\", base_name);\n+\tif (m && midx_contains_pack(m, buf.buf))\n+\t\tclear_midx_file(the_repository);\n+\tstrbuf_insertf(&buf, 0, \"%s/\", dir_name);\n+\tunlink_pack_path(buf.buf, 1);\n+\tstrbuf_release(&buf);\n+}\n+\n+static void remove_redundant_packs_1(struct string_list *packs)\n+{\n+\tstruct string_list_item *item;\n+\tfor_each_string_list_item(item, packs) {\n+\t\tif (!((uintptr_t)item->util & DELETE_PACK))\n+\t\t\tcontinue;\n+\t\tremove_redundant_pack(packdir, item->string);\n+\t}\n+}\n+\n+static void remove_redundant_existing_packs(struct existing_packs *existing)\n+{\n+\tremove_redundant_packs_1(&existing->non_kept_packs);\n+}\n+\n static void existing_packs_release(struct existing_packs *existing)\n {\n \tstring_list_clear(&existing->kept_packs, 0);\n@@ -184,18 +211,6 @@ static void collect_pack_filenames(struct existing_packs *existing,\n \tstrbuf_release(&buf);\n }\n \n-static void remove_redundant_pack(const char *dir_name, const char *base_name)\n-{\n-\tstruct strbuf buf = STRBUF_INIT;\n-\tstruct multi_pack_index *m = get_local_multi_pack_index(the_repository);\n-\tstrbuf_addf(&buf, \"%s.pack\", base_name);\n-\tif (m && midx_contains_pack(m, buf.buf))\n-\t\tclear_midx_file(the_repository);\n-\tstrbuf_insertf(&buf, 0, \"%s/\", dir_name);\n-\tunlink_pack_path(buf.buf, 1);\n-\tstrbuf_release(&buf);\n-}\n-\n static void prepare_pack_objects(struct child_process *cmd,\n \t\t\t\t const struct pack_objects_args *args,\n \t\t\t\t const char *out)\n@@ -1222,11 +1237,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \n \tif (delete_redundant) {\n \t\tint opts = 0;\n-\t\tfor_each_string_list_item(item, &existing.non_kept_packs) {\n-\t\t\tif (!((uintptr_t)item->util & DELETE_PACK))\n-\t\t\t\tcontinue;\n-\t\t\tremove_redundant_pack(packdir, item->string);\n-\t\t}\n+\t\tremove_redundant_existing_packs(&existing);\n \n \t\tif (geometry.split_factor)\n \t\t\tgeometry_remove_redundant_packs(&geometry, &names,\n-- \n2.42.0.166.g68748eb9c8\n\n"},{"id":"481814","messageId":"f4f7b4c08f682b5ebca474e8c1d51d31a2da76b8.1694632644.git.me@ttaylorr.com","threadId":"60201","inReplyTo":"cover.1694632644.git.me@ttaylorr.com","subject":"[PATCH v2 5/8] builtin/repack.c: extract `has_existing_non_kept_packs()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-09-13T19:17:54Z","receivedAt":"2023-09-13T19:18:00Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"When there is:\n\n - at least one pre-existing packfile (which is not marked as kept),\n - repacking with the `-d` flag, and\n - not doing a cruft repack\n\n, then we pass a handful of additional options to the inner\n`pack-objects` process, like `--unpack-unreachable`,\n`--keep-unreachable`, and `--pack-loose-unreachable`, in addition to\nmarking any packs we just wrote for promisor remotes as kept in-core\n(with `--keep-pack`, as opposed to the presence of a \".keep\" file on\ndisk).\n\nBecause we store both cruft and non-cruft packs together in the same\n`existing.non_kept_packs` list, it suffices to check its `nr` member to\nsee if it is zero or not.\n\nBut a following change will store cruft- and non-cruft packs separately,\nmeaning this check would break as a result. Prepare for this by\nextracting this part of the check into a new helper function called\n`has_existing_non_kept_packs()`.\n\nThis patch does not introduce any functional changes, but prepares us to\nmake a more isolated change in a subsequent patch.\n\nSigned-off-by: Taylor Blau \n---\n builtin/repack.c | 8 +++++++-\n 1 file changed, 7 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex b5fb14c017..9ebc2e774b 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -105,6 +105,11 @@ struct existing_packs {\n \t.non_kept_packs = STRING_LIST_INIT_DUP, \\\n }\n \n+static int has_existing_non_kept_packs(const struct existing_packs *existing)\n+{\n+\treturn existing->non_kept_packs.nr;\n+}\n+\n static void mark_packs_for_deletion_1(struct string_list *names,\n \t\t\t\t struct string_list *list)\n {\n@@ -1047,7 +1052,8 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \tif (pack_everything & ALL_INTO_ONE) {\n \t\trepack_promisor_objects(&po_args, &names);\n \n-\t\tif (existing.non_kept_packs.nr && delete_redundant &&\n+\t\tif (has_existing_non_kept_packs(&existing) &&\n+\t\t delete_redundant &&\n \t\t !(pack_everything & PACK_CRUFT)) {\n \t\t\tfor_each_string_list_item(item, &names) {\n \t\t\t\tstrvec_pushf(&cmd.args, \"--keep-pack=%s-%s.pack\",\n-- \n2.42.0.166.g68748eb9c8\n\n"},{"id":"481815","messageId":"d68a88dbd5606d532b91761ae1acdb410fa30e85.1694632644.git.me@ttaylorr.com","threadId":"60201","inReplyTo":"cover.1694632644.git.me@ttaylorr.com","subject":"[PATCH v2 6/8] builtin/repack.c: store existing cruft packs separately","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-09-13T19:17:57Z","receivedAt":"2023-09-13T19:18:04Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"When repacking with the `--write-midx` option, we invoke the function\n`midx_included_packs()` in order to produce the list of packs we want to\ninclude in the resulting MIDX.\n\nThis list is comprised of:\n\n - existing .keep packs\n - any pack(s) which were written earlier in the same process\n - any unchanged packs when doing a `--geometric` repack\n - any cruft packs\n\nPrior to this patch, we stored pre-existing cruft and non-cruft packs\ntogether (provided those packs are non-kept). This meant we needed an\nadditional bit to indicate which non-kept pack(s) were cruft versus\nthose that aren't.\n\nBut alternatively we can store cruft packs in a separate list, avoiding\nthe need for this extra bit, and simplifying the code below.\n\nSigned-off-by: Taylor Blau \n---\n builtin/repack.c | 39 +++++++++++++++++++++++----------------\n 1 file changed, 23 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 9ebc2e774b..8103a9d308 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -27,7 +27,6 @@\n #define PACK_CRUFT 4\n \n #define DELETE_PACK 1\n-#define CRUFT_PACK 2\n \n static int pack_everything;\n static int delta_base_offset = 1;\n@@ -98,16 +97,18 @@ static int repack_config(const char *var, const char *value,\n struct existing_packs {\n \tstruct string_list kept_packs;\n \tstruct string_list non_kept_packs;\n+\tstruct string_list cruft_packs;\n };\n \n #define EXISTING_PACKS_INIT { \\\n \t.kept_packs = STRING_LIST_INIT_DUP, \\\n \t.non_kept_packs = STRING_LIST_INIT_DUP, \\\n+\t.cruft_packs = STRING_LIST_INIT_DUP, \\\n }\n \n static int has_existing_non_kept_packs(const struct existing_packs *existing)\n {\n-\treturn existing->non_kept_packs.nr;\n+\treturn existing->non_kept_packs.nr || existing->cruft_packs.nr;\n }\n \n static void mark_packs_for_deletion_1(struct string_list *names,\n@@ -138,6 +139,7 @@ static void mark_packs_for_deletion(struct existing_packs *existing,\n \n {\n \tmark_packs_for_deletion_1(names, &existing->non_kept_packs);\n+\tmark_packs_for_deletion_1(names, &existing->cruft_packs);\n }\n \n static void remove_redundant_pack(const char *dir_name, const char *base_name)\n@@ -165,12 +167,14 @@ static void remove_redundant_packs_1(struct string_list *packs)\n static void remove_redundant_existing_packs(struct existing_packs *existing)\n {\n \tremove_redundant_packs_1(&existing->non_kept_packs);\n+\tremove_redundant_packs_1(&existing->cruft_packs);\n }\n \n static void existing_packs_release(struct existing_packs *existing)\n {\n \tstring_list_clear(&existing->kept_packs, 0);\n \tstring_list_clear(&existing->non_kept_packs, 0);\n+\tstring_list_clear(&existing->cruft_packs, 0);\n }\n \n /*\n@@ -204,12 +208,10 @@ static void collect_pack_filenames(struct existing_packs *existing,\n \n \t\tif ((extra_keep->nr > 0 && i < extra_keep->nr) || p->pack_keep)\n \t\t\tstring_list_append(&existing->kept_packs, buf.buf);\n-\t\telse {\n-\t\t\tstruct string_list_item *item;\n-\t\t\titem = string_list_append(&existing->non_kept_packs, buf.buf);\n-\t\t\tif (p->is_cruft)\n-\t\t\t\titem->util = (void*)(uintptr_t)CRUFT_PACK;\n-\t\t}\n+\t\telse if (p->is_cruft)\n+\t\t\tstring_list_append(&existing->cruft_packs, buf.buf);\n+\t\telse\n+\t\t\tstring_list_append(&existing->non_kept_packs, buf.buf);\n \t}\n \n \tstring_list_sort(&existing->kept_packs);\n@@ -691,14 +693,11 @@ static void midx_included_packs(struct string_list *include,\n \t\t\tstring_list_insert(include, strbuf_detach(&buf, NULL));\n \t\t}\n \n-\t\tfor_each_string_list_item(item, &existing->non_kept_packs) {\n-\t\t\tif (!((uintptr_t)item->util & CRUFT_PACK)) {\n-\t\t\t\t/*\n-\t\t\t\t * no need to check DELETE_PACK, since we're not\n-\t\t\t\t * doing an ALL_INTO_ONE repack\n-\t\t\t\t */\n-\t\t\t\tcontinue;\n-\t\t\t}\n+\t\tfor_each_string_list_item(item, &existing->cruft_packs) {\n+\t\t\t/*\n+\t\t\t * no need to check DELETE_PACK, since we're not\n+\t\t\t * doing an ALL_INTO_ONE repack\n+\t\t\t */\n \t\t\tstring_list_insert(include, xstrfmt(\"%s.idx\", item->string));\n \t\t}\n \t} else {\n@@ -707,6 +706,12 @@ static void midx_included_packs(struct string_list *include,\n \t\t\t\tcontinue;\n \t\t\tstring_list_insert(include, xstrfmt(\"%s.idx\", item->string));\n \t\t}\n+\n+\t\tfor_each_string_list_item(item, &existing->cruft_packs) {\n+\t\t\tif ((uintptr_t)item->util & DELETE_PACK)\n+\t\t\t\tcontinue;\n+\t\t\tstring_list_insert(include, xstrfmt(\"%s.idx\", item->string));\n+\t\t}\n \t}\n }\n \n@@ -835,6 +840,8 @@ static int write_cruft_pack(const struct pack_objects_args *args,\n \t\tfprintf(in, \"%s-%s.pack\\n\", pack_prefix, item->string);\n \tfor_each_string_list_item(item, &existing->non_kept_packs)\n \t\tfprintf(in, \"-%s.pack\\n\", item->string);\n+\tfor_each_string_list_item(item, &existing->cruft_packs)\n+\t\tfprintf(in, \"-%s.pack\\n\", item->string);\n \tfor_each_string_list_item(item, &existing->kept_packs)\n \t\tfprintf(in, \"%s.pack\\n\", item->string);\n \tfclose(in);\n-- \n2.42.0.166.g68748eb9c8\n\n"},{"id":"481816","messageId":"481a29599b1871af4f58a68bdf227b1fe477d365.1694632644.git.me@ttaylorr.com","threadId":"60201","inReplyTo":"cover.1694632644.git.me@ttaylorr.com","subject":"[PATCH v2 7/8] builtin/repack.c: avoid directly inspecting \"util\"","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-09-13T19:18:00Z","receivedAt":"2023-09-13T19:18:06Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"The `->util` field corresponding to each string_list_item is used to\ntrack the existence of some pack at the beginning of a repack operation\nwas originally intended to be used as a bitfield.\n\nThis bitfield tracked:\n\n - (1 << 0): whether or not the pack should be deleted\n - (1 << 1): whether or not the pack is cruft\n\nThe previous commit removed the use of the second bit, but a future\npatch (from a different series than this one) will introduce a new use\nof it.\n\nSo we could stop treating the util pointer as a bitfield and instead\nstart treating it as if it were a boolean. But this would require some\nbacktracking when that later patch is applied.\n\nInstead, let's avoid touching the ->util field directly, and instead\nintroduce convenience functions like:\n\n - pack_mark_for_deletion()\n - pack_is_marked_for_deletion()\n\nHelped-by: Junio C Hamano \nHelped-by: Jeff King \nHelped-by: Patrick Steinhardt \nSigned-off-by: Taylor Blau \n---\n builtin/repack.c | 18 ++++++++++++++----\n 1 file changed, 14 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 8103a9d308..e9a091fbbc 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -111,6 +111,16 @@ static int has_existing_non_kept_packs(const struct existing_packs *existing)\n \treturn existing->non_kept_packs.nr || existing->cruft_packs.nr;\n }\n \n+static void pack_mark_for_deletion(struct string_list_item *item)\n+{\n+\titem->util = (void*)((uintptr_t)item->util | DELETE_PACK);\n+}\n+\n+static int pack_is_marked_for_deletion(struct string_list_item *item)\n+{\n+\treturn (uintptr_t)item->util & DELETE_PACK;\n+}\n+\n static void mark_packs_for_deletion_1(struct string_list *names,\n \t\t\t\t struct string_list *list)\n {\n@@ -130,7 +140,7 @@ static void mark_packs_for_deletion_1(struct string_list *names,\n \t\t * (if `-d` was given).\n \t\t */\n \t\tif (!string_list_has_string(names, sha1))\n-\t\t\titem->util = (void*)(uintptr_t)((size_t)item->util | DELETE_PACK);\n+\t\t\tpack_mark_for_deletion(item);\n \t}\n }\n \n@@ -158,7 +168,7 @@ static void remove_redundant_packs_1(struct string_list *packs)\n {\n \tstruct string_list_item *item;\n \tfor_each_string_list_item(item, packs) {\n-\t\tif (!((uintptr_t)item->util & DELETE_PACK))\n+\t\tif (!pack_is_marked_for_deletion(item))\n \t\t\tcontinue;\n \t\tremove_redundant_pack(packdir, item->string);\n \t}\n@@ -702,13 +712,13 @@ static void midx_included_packs(struct string_list *include,\n \t\t}\n \t} else {\n \t\tfor_each_string_list_item(item, &existing->non_kept_packs) {\n-\t\t\tif ((uintptr_t)item->util & DELETE_PACK)\n+\t\t\tif (pack_is_marked_for_deletion(item))\n \t\t\t\tcontinue;\n \t\t\tstring_list_insert(include, xstrfmt(\"%s.idx\", item->string));\n \t\t}\n \n \t\tfor_each_string_list_item(item, &existing->cruft_packs) {\n-\t\t\tif ((uintptr_t)item->util & DELETE_PACK)\n+\t\t\tif (pack_is_marked_for_deletion(item))\n \t\t\t\tcontinue;\n \t\t\tstring_list_insert(include, xstrfmt(\"%s.idx\", item->string));\n \t\t}\n-- \n2.42.0.166.g68748eb9c8\n\n"},{"id":"481817","messageId":"68748eb9c865ba8252852713533e2f47e2cac489.1694632644.git.me@ttaylorr.com","threadId":"60201","inReplyTo":"cover.1694632644.git.me@ttaylorr.com","subject":"[PATCH v2 8/8] builtin/repack.c: extract common cruft pack loop","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-09-13T19:18:03Z","receivedAt":"2023-09-13T19:18:14Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"When generating the list of packs to store in a MIDX (when given the\n`--write-midx` option), we include any cruft packs both during\n--geometric and non-geometric repacks.\n\nBut the rules for when we do and don't have to check whether any of\nthose cruft packs were queued for deletion differ slightly between the\ntwo cases.\n\nBut the two can be unified, provided there is a little bit of extra\ndetail added in the comment to clarify when it is safe to avoid checking\nfor any pending deletions (and why it is OK to do so even when not\nrequired).\n\nSigned-off-by: Taylor Blau \n---\n builtin/repack.c | 31 ++++++++++++++++++-------------\n 1 file changed, 18 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex e9a091fbbc..529e13120d 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -702,26 +702,31 @@ static void midx_included_packs(struct string_list *include,\n \n \t\t\tstring_list_insert(include, strbuf_detach(&buf, NULL));\n \t\t}\n-\n-\t\tfor_each_string_list_item(item, &existing->cruft_packs) {\n-\t\t\t/*\n-\t\t\t * no need to check DELETE_PACK, since we're not\n-\t\t\t * doing an ALL_INTO_ONE repack\n-\t\t\t */\n-\t\t\tstring_list_insert(include, xstrfmt(\"%s.idx\", item->string));\n-\t\t}\n \t} else {\n \t\tfor_each_string_list_item(item, &existing->non_kept_packs) {\n \t\t\tif (pack_is_marked_for_deletion(item))\n \t\t\t\tcontinue;\n \t\t\tstring_list_insert(include, xstrfmt(\"%s.idx\", item->string));\n \t\t}\n+\t}\n \n-\t\tfor_each_string_list_item(item, &existing->cruft_packs) {\n-\t\t\tif (pack_is_marked_for_deletion(item))\n-\t\t\t\tcontinue;\n-\t\t\tstring_list_insert(include, xstrfmt(\"%s.idx\", item->string));\n-\t\t}\n+\tfor_each_string_list_item(item, &existing->cruft_packs) {\n+\t\t/*\n+\t\t * When doing a --geometric repack, there is no need to check\n+\t\t * for deleted packs, since we're by definition not doing an\n+\t\t * ALL_INTO_ONE repack (hence no packs will be deleted).\n+\t\t * Otherwise we must check for and exclude any packs which are\n+\t\t * enqueued for deletion.\n+\t\t *\n+\t\t * So we could omit the conditional below in the --geometric\n+\t\t * case, but doing so is unnecessary since no packs are marked\n+\t\t * as pending deletion (since we only call\n+\t\t * `mark_packs_for_deletion()` when doing an all-into-one\n+\t\t * repack).\n+\t\t */\n+\t\tif (pack_is_marked_for_deletion(item))\n+\t\t\tcontinue;\n+\t\tstring_list_insert(include, xstrfmt(\"%s.idx\", item->string));\n \t}\n }\n \n-- \n2.42.0.166.g68748eb9c8\n"},{"id":"481818","messageId":"xmqqa5tpluyj.fsf@gitster.g","threadId":"60201","inReplyTo":"cover.1694632644.git.me@ttaylorr.com","subject":"Re: [PATCH v2 0/8] repack: refactor pack snapshot-ing logic","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-09-13T19:44:04Z","receivedAt":"2023-09-13T19:44:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> Here is a small reroll of my series to clean up some of the internals of\n> 'git repack' used to track the set of existing packs.\n>\n> Much is unchanged from the last round, save for some additional clean-up\n> on how we handle the '->util' field for each pack's string_list_item in\n> response to very helpful review from those CC'd.\n\nThe change to [7/8] was as expected and looking good. Let's see if\nwe see additional reviews from others, plan to declare victory and\nmerge it to 'next' by early next week at the latest, if not sooner.\n\nChristian, your cc/repack-sift-filtered-objects-to-separate-pack\ntopic will have to interact with this topic when merged to 'seen',\nso it would be good for you to give a review on these patches (if\nonly to understand the new world order) and optionally make a trial\nmerge between the two to see how well they work together and what\nadjustment will be needed when you eventually rebase your topic on\ntop. Actual rebasing can wait until this topic graduates, but trial\nmerge is something you can immediately do in the meantime to prepare\nfor the future.\n\nThanks.\n"},{"id":"481820","messageId":"20230914000750.GA1709736@coredump.intra.peff.net","threadId":"60201","inReplyTo":"xmqqa5tpluyj.fsf@gitster.g","subject":"Re: [PATCH v2 0/8] repack: refactor pack snapshot-ing logic","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-09-14T00:07:50Z","receivedAt":"2023-09-14T00:07:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 13, 2023 at 12:44:04PM -0700, Junio C Hamano wrote:\n\n> Taylor Blau writes:\n> \n> > Here is a small reroll of my series to clean up some of the internals of\n> > 'git repack' used to track the set of existing packs.\n> >\n> > Much is unchanged from the last round, save for some additional clean-up\n> > on how we handle the '->util' field for each pack's string_list_item in\n> > response to very helpful review from those CC'd.\n> \n> The change to [7/8] was as expected and looking good. Let's see if\n> we see additional reviews from others, plan to declare victory and\n> merge it to 'next' by early next week at the latest, if not sooner.\n\nThis looks great to me. The motivation in the revised patch 7 is much\neasier to follow, and the end result is much nicer to read. :)\n\n-Peff\n"},{"id":"481840","messageId":"ZQLhg43mCVF3-vjp@tanuki","threadId":"60201","inReplyTo":"20230914000750.GA1709736@coredump.intra.peff.net","subject":"Re: [PATCH v2 0/8] repack: refactor pack snapshot-ing logic","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-09-14T10:33:39Z","receivedAt":"2023-09-14T10:34:05Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Sep 13, 2023 at 08:07:50PM -0400, Jeff King wrote:\n> On Wed, Sep 13, 2023 at 12:44:04PM -0700, Junio C Hamano wrote:\n> \n> > Taylor Blau writes:\n> > \n> > > Here is a small reroll of my series to clean up some of the internals of\n> > > 'git repack' used to track the set of existing packs.\n> > >\n> > > Much is unchanged from the last round, save for some additional clean-up\n> > > on how we handle the '->util' field for each pack's string_list_item in\n> > > response to very helpful review from those CC'd.\n> > \n> > The change to [7/8] was as expected and looking good. Let's see if\n> > we see additional reviews from others, plan to declare victory and\n> > merge it to 'next' by early next week at the latest, if not sooner.\n> \n> This looks great to me. The motivation in the revised patch 7 is much\n> easier to follow, and the end result is much nicer to read. :)\n> \n> -Peff\n\nAgreed, the patch series loogs good to me now. Thanks!\n\nPatrick\n"},{"id":"481842","messageId":"CAP8UFD1rDb-iYf4LYb7n=K4KpQ-JR-JK4TkQpGJ-TCfTNFFbnA@mail.gmail.com","threadId":"60201","inReplyTo":"xmqqa5tpluyj.fsf@gitster.g","subject":"Re: [PATCH v2 0/8] repack: refactor pack snapshot-ing logic","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2023-09-14T11:10:38Z","receivedAt":"2023-09-14T11:10:54Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Wed, Sep 13, 2023 at 9:44 PM Junio C Hamano wrote:\n>\n> Taylor Blau writes:\n>\n> > Here is a small reroll of my series to clean up some of the internals of\n> > 'git repack' used to track the set of existing packs.\n> >\n> > Much is unchanged from the last round, save for some additional clean-up\n> > on how we handle the '->util' field for each pack's string_list_item in\n> > response to very helpful review from those CC'd.\n>\n> The change to [7/8] was as expected and looking good. Let's see if\n> we see additional reviews from others, plan to declare victory and\n> merge it to 'next' by early next week at the latest, if not sooner.\n>\n> Christian, your cc/repack-sift-filtered-objects-to-separate-pack\n> topic will have to interact with this topic when merged to 'seen',\n> so it would be good for you to give a review on these patches (if\n> only to understand the new world order) and optionally make a trial\n> merge between the two to see how well they work together and what\n> adjustment will be needed when you eventually rebase your topic on\n> top. Actual rebasing can wait until this topic graduates, but trial\n> merge is something you can immediately do in the meantime to prepare\n> for the future.\n\nOk, I will try to review and merge this with\ncc/repack-sift-filtered-objects-to-separate-pack soon.\n"},{"id":"481845","messageId":"ZQNKkn0YYLUyN5Ih@nand.local","threadId":"60201","inReplyTo":"CAP8UFD1rDb-iYf4LYb7n=K4KpQ-JR-JK4TkQpGJ-TCfTNFFbnA@mail.gmail.com","subject":"Re: [PATCH v2 0/8] repack: refactor pack snapshot-ing logic","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-09-14T18:01:54Z","receivedAt":"2023-09-14T18:01:58Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Sep 14, 2023 at 01:10:38PM +0200, Christian Couder wrote:\n> Ok, I will try to review and merge this with\n> cc/repack-sift-filtered-objects-to-separate-pack soon.\n\nI took a look at how much/little effort was going to be required, and\nluckily the changes are isolated only to a single patch. It's just your\nf1ffa71e8f (repack: add `--filter=` option, 2023-09-11),\nand in particular the `write_filtered_pack()` function.\n\nI started messing around with it myself and generated the following\nfixup! which can be applied on top of your version of f1ffa71e8f. It's\nmostly straightforward, but there is a gotcha that the loop over\nnon-kept packs has to change to:\n\n for_each_string_list_item(item, &existing->non_kept_packs)\n /* ... */\n for_each_string_list_item(item, &existing->cruft_packs)\n /* ... */\n\n, instead of just the first loop over non_kept_packs, since cruft packs\nare stored in a separate list.\n\nIn any event, here's the fixup! I generated on top of that patch:\n\n--- 8< ---\nSubject: [PATCH] fixup! repack: add `--filter=` option\n\nSigned-off-by: Taylor Blau \n---\n builtin/repack.c | 25 +++++++++++--------------\n 1 file changed, 11 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 120f4241c0..0d23323d05 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -834,15 +834,13 @@ static int finish_pack_objects_cmd(struct child_process *cmd,\n static int write_filtered_pack(const struct pack_objects_args *args,\n \t\t\t const char *destination,\n \t\t\t const char *pack_prefix,\n-\t\t\t struct string_list *keep_pack_list,\n-\t\t\t struct string_list *names,\n-\t\t\t struct string_list *existing_packs,\n-\t\t\t struct string_list *existing_kept_packs)\n+\t\t\t struct existing_packs *existing,\n+\t\t\t struct string_list *names)\n {\n \tstruct child_process cmd = CHILD_PROCESS_INIT;\n \tstruct string_list_item *item;\n \tFILE *in;\n-\tint ret, i;\n+\tint ret;\n \tconst char *caret;\n \tconst char *scratch;\n \tint local = skip_prefix(destination, packdir, &scratch);\n@@ -853,9 +851,8 @@ static int write_filtered_pack(const struct pack_objects_args *args,\n\n \tif (!pack_kept_objects)\n \t\tstrvec_push(&cmd.args, \"--honor-pack-keep\");\n-\tfor (i = 0; i < keep_pack_list->nr; i++)\n-\t\tstrvec_pushf(&cmd.args, \"--keep-pack=%s\",\n-\t\t\t keep_pack_list->items[i].string);\n+\tfor_each_string_list_item(item, &existing->kept_packs)\n+\t\tstrvec_pushf(&cmd.args, \"--keep-pack=%s\", item->string);\n\n \tcmd.in = -1;\n\n@@ -872,10 +869,12 @@ static int write_filtered_pack(const struct pack_objects_args *args,\n \tin = xfdopen(cmd.in, \"w\");\n \tfor_each_string_list_item(item, names)\n \t\tfprintf(in, \"^%s-%s.pack\\n\", pack_prefix, item->string);\n-\tfor_each_string_list_item(item, existing_packs)\n+\tfor_each_string_list_item(item, &existing->non_kept_packs)\n+\t\tfprintf(in, \"%s.pack\\n\", item->string);\n+\tfor_each_string_list_item(item, &existing->cruft_packs)\n \t\tfprintf(in, \"%s.pack\\n\", item->string);\n \tcaret = pack_kept_objects ? \"\" : \"^\";\n-\tfor_each_string_list_item(item, existing_kept_packs)\n+\tfor_each_string_list_item(item, &existing->kept_packs)\n \t\tfprintf(in, \"%s%s.pack\\n\", caret, item->string);\n \tfclose(in);\n\n@@ -1261,10 +1260,8 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\tret = write_filtered_pack(&po_args,\n \t\t\t\t\t packtmp,\n \t\t\t\t\t find_pack_prefix(packdir, packtmp),\n-\t\t\t\t\t &keep_pack_list,\n-\t\t\t\t\t &names,\n-\t\t\t\t\t &existing_nonkept_packs,\n-\t\t\t\t\t &existing_kept_packs);\n+\t\t\t\t\t &existing,\n+\t\t\t\t\t &names);\n \t\tif (ret)\n \t\t\tgoto cleanup;\n \t}\n--\n2.42.0.137.g6fe1dff026\n--- >8 ---\n\nThanks,\nTaylor\n"},{"id":"481865","messageId":"CAP8UFD0aqZx6_d1AeJO0SBCLo7+AOJaVM93CCk5h5+fd7=VW+w@mail.gmail.com","threadId":"60201","inReplyTo":"ZQNKkn0YYLUyN5Ih@nand.local","subject":"Re: [PATCH v2 0/8] repack: refactor pack snapshot-ing logic","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2023-09-15T05:56:44Z","receivedAt":"2023-09-15T05:57:02Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Thu, Sep 14, 2023 at 8:01 PM Taylor Blau wrote:\n>\n> On Thu, Sep 14, 2023 at 01:10:38PM +0200, Christian Couder wrote:\n> > Ok, I will try to review and merge this with\n> > cc/repack-sift-filtered-objects-to-separate-pack soon.\n>\n> I took a look at how much/little effort was going to be required, and\n> luckily the changes are isolated only to a single patch. It's just your\n> f1ffa71e8f (repack: add `--filter=` option, 2023-09-11),\n> and in particular the `write_filtered_pack()` function.\n>\n> I started messing around with it myself and generated the following\n> fixup! which can be applied on top of your version of f1ffa71e8f. It's\n> mostly straightforward, but there is a gotcha that the loop over\n> non-kept packs has to change to:\n>\n> for_each_string_list_item(item, &existing->non_kept_packs)\n> /* ... */\n> for_each_string_list_item(item, &existing->cruft_packs)\n> /* ... */\n>\n> , instead of just the first loop over non_kept_packs, since cruft packs\n> are stored in a separate list.\n>\n> In any event, here's the fixup! I generated on top of that patch:\n\nThanks a lot! I will very likely use it in the new version I will send\nafter your series has graduated.\n"},{"id":"481868","messageId":"CAP8UFD043ut1h-3SW+YLoT1ZpSrRHBMi15w6rMmKqAp2UbV0Vw@mail.gmail.com","threadId":"60201","inReplyTo":"f4f7b4c08f682b5ebca474e8c1d51d31a2da76b8.1694632644.git.me@ttaylorr.com","subject":"Re: [PATCH v2 5/8] builtin/repack.c: extract `has_existing_non_kept_packs()`","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2023-09-15T10:02:49Z","receivedAt":"2023-09-15T10:05:38Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Thu, Sep 14, 2023 at 12:13 AM Taylor Blau wrote:\n>\n> When there is:\n>\n> - at least one pre-existing packfile (which is not marked as kept),\n> - repacking with the `-d` flag, and\n> - not doing a cruft repack\n>\n> , then we pass a handful of additional options to the inner\n\nNit (not worth a reroll): I think the comma at the beginning of the\nabove line would be better after \"not doing a cruft repack\".\n\n> `pack-objects` process, like `--unpack-unreachable`,\n> `--keep-unreachable`, and `--pack-loose-unreachable`, in addition to\n> marking any packs we just wrote for promisor remotes as kept in-core\n> (with `--keep-pack`, as opposed to the presence of a \".keep\" file on\n> disk).\n"},{"id":"481869","messageId":"CAP8UFD3nPeF1Dy+cbPAYTFS7AdZJFdGs8qnE5gESU5CQ9h2eng@mail.gmail.com","threadId":"60201","inReplyTo":"cover.1694632644.git.me@ttaylorr.com","subject":"Re: [PATCH v2 0/8] repack: refactor pack snapshot-ing logic","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2023-09-15T10:09:53Z","receivedAt":"2023-09-15T10:10:29Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Wed, Sep 13, 2023 at 9:17 PM Taylor Blau wrote:\n>\n> Here is a small reroll of my series to clean up some of the internals of\n> 'git repack' used to track the set of existing packs.\n>\n> Much is unchanged from the last round, save for some additional clean-up\n> on how we handle the '->util' field for each pack's string_list_item in\n> response to very helpful review from those CC'd.\n>\n> As usual, a range-diff is available below for convenience. Thanks in\n> advance for your review!\n>\n> Taylor Blau (8):\n> builtin/repack.c: extract structure to store existing packs\n> builtin/repack.c: extract marking packs for deletion\n> builtin/repack.c: extract redundant pack cleanup for --geometric\n> builtin/repack.c: extract redundant pack cleanup for existing packs\n> builtin/repack.c: extract `has_existing_non_kept_packs()`\n> builtin/repack.c: store existing cruft packs separately\n> builtin/repack.c: avoid directly inspecting \"util\"\n> builtin/repack.c: extract common cruft pack loop\n\nI think it would be a bit nicer with s/builtin\\/repack.c/repack/ in\nall the above commit subjects, but I don't think it's worth a reroll.\n\nExcept for another very small nit in a commit message also not worth a\nreroll, this LGTM.\n\nThanks,\nChristian.\n"}]}