{"thread":{"id":"61537","subject":"[PATCH 0/8] midx-write: miscellaneous clean-ups for incremental MIDXs","startedAt":"2024-05-23T16:38:03Z","lastAt":"2024-05-31T08:28:24Z","messageCount":34,"participants":["Taylor Blau","Patrick Steinhardt","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":8},"messages":[{"id":"495420","messageId":"cover.1716482279.git.me@ttaylorr.com","threadId":"61537","inReplyTo":null,"subject":"[PATCH 0/8] midx-write: miscellaneous clean-ups for incremental MIDXs","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-05-23T16:37:59Z","receivedAt":"2024-05-23T16:38:03Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"This series has a grab-bag of midx-write related cleanups that I pulled\nout of a larger series to implement incremental MIDX chains.\n\nThe main goal of this series is to support reading an existing MIDX when\nwriting a new one with `--stdin-packs`, since we need this capability\nfor incremental MIDXs. Along the way there are a few other\nquality-of-life improvements and refactorings. The first patch in this\nseries fixes an existing segfault that can be triggered on released\nversions of Git (but has no interesting security implications).\n\nAdmittedly, this series isn't all that interesting on its own, but it\ndoes chip away at the larger series, so it should be easier to review.\nHopefully sending this one separately is OK, and in either event, thank\nyou in advance for your review!\n\nTaylor Blau (8):\n midx-write.c: tolerate `--preferred-pack` without bitmaps\n midx-write.c: reduce argument count for `get_sorted_entries()`\n midx-write.c: pass `start_pack` to `get_sorted_entries()`\n midx-write.c: extract `should_include_pack()`\n midx-write.c: extract `fill_packs_from_midx()`\n midx-write.c: support reading an existing MIDX with `packs_to_include`\n midx: replace `get_midx_rev_filename()` with a generic helper\n pack-bitmap.c: reimplement `midx_bitmap_filename()` with helper\n\n midx-write.c | 157 ++++++++++++++++++------------------\n midx.c | 14 ++--\n midx.h | 6 +-\n pack-bitmap.c | 5 +-\n pack-revindex.c | 3 +-\n t/t5319-multi-pack-index.sh | 23 ++++++\n 6 files changed, 118 insertions(+), 90 deletions(-)\n\n\nbase-commit: 4365c6fcf96caac73dcc412aa25db34cf8df48d5\n-- \n2.45.1.217.g9bb58e2bf5a.dirty\n"},{"id":"495421","messageId":"c753bc379b005ecaf131f8f1ae9c5b80b2712759.1716482279.git.me@ttaylorr.com","threadId":"61537","inReplyTo":"cover.1716482279.git.me@ttaylorr.com","subject":"[PATCH 1/8] midx-write.c: tolerate `--preferred-pack` without bitmaps","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-05-23T16:38:03Z","receivedAt":"2024-05-23T16:38:05Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"When passing a preferred pack to the MIDX write machinery, we ensure\nthat the given preferred pack is non-empty since 5d3cd09a808 (midx:\nreject empty `--preferred-pack`'s, 2021-08-31).\n\nHowever packs are only loaded (via `write_midx_internal()`, though a\nsubsequent patch will refactor this code out to its own function) when\nthe `MIDX_WRITE_REV_INDEX` flag is set.\n\nSo if a caller runs:\n\n $ git multi-pack-index write --preferred-pack=...\n\nwith both (a) an existing MIDX, and (b) specifies a pack from that MIDX\nas the preferred one, without passing `--bitmap`, then the check added\nin 5d3cd09a808 will result in a segfault.\n\nNote that packs loaded from disk which don't appear in an existing MIDX\ndo not trigger this issue, as those packs are loaded unconditionally. We\nconditionally load packs from a MIDX since we tolerate MIDXs whose\npacks do not resolve (i.e., via the MIDX write after removing\nunreferenced packs via 'git multi-pack-index expire').\n\nIn practice, this isn't possible to trigger when running `git\nmulti-pack-index write` from via `git repack`, as the latter always\npasses `--stdin-packs`, which prevents us from loading an existing MIDX,\nas it forces all packs to be read from disk.\n\nBut a future commit in this series will change that behavior to\nunconditionally load an existing MIDX, even with `--stdin-packs`, making\nthis behavior trigger-able from 'repack' much more easily.\n\nPrevent this from being an issue by removing the segfault altogether by\ncalling `prepare_midx_pack()` on packs loaded from an existing MIDX when\neither the `MIDX_WRITE_REV_INDEX` flag is set *or* we specified a\n`--preferred-pack`.\n\nSigned-off-by: Taylor Blau \n---\n midx-write.c | 8 +++++++-\n t/t5319-multi-pack-index.sh | 23 +++++++++++++++++++++++\n 2 files changed, 30 insertions(+), 1 deletion(-)\n\ndiff --git a/midx-write.c b/midx-write.c\nindex 9d096d5a28..03e95ae821 100644\n--- a/midx-write.c\n+++ b/midx-write.c\n@@ -930,11 +930,17 @@ static int write_midx_internal(const char *object_dir,\n \t\tfor (i = 0; i < ctx.m->num_packs; i++) {\n \t\t\tALLOC_GROW(ctx.info, ctx.nr + 1, ctx.alloc);\n \n-\t\t\tif (flags & MIDX_WRITE_REV_INDEX) {\n+\t\t\tif (flags & MIDX_WRITE_REV_INDEX ||\n+\t\t\t preferred_pack_name) {\n \t\t\t\t/*\n \t\t\t\t * If generating a reverse index, need to have\n \t\t\t\t * packed_git's loaded to compare their\n \t\t\t\t * mtimes and object count.\n+\t\t\t\t *\n+\t\t\t\t * If a preferred pack is specified,\n+\t\t\t\t * need to have packed_git's loaded to\n+\t\t\t\t * ensure the chosen preferred pack has\n+\t\t\t\t * a non-zero object count.\n \t\t\t\t */\n \t\t\t\tif (prepare_midx_pack(the_repository, ctx.m, i)) {\n \t\t\t\t\terror(_(\"could not load pack\"));\ndiff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\nindex dd09134db0..10d2a6bf92 100755\n--- a/t/t5319-multi-pack-index.sh\n+++ b/t/t5319-multi-pack-index.sh\n@@ -350,6 +350,29 @@ test_expect_success 'preferred packs must be non-empty' '\n \t)\n '\n \n+test_expect_success 'preferred pack from existing MIDX without bitmaps' '\n+\tgit init preferred-without-bitmaps &&\n+\t(\n+\t\tcd preferred-without-bitmaps &&\n+\n+\t\ttest_commit one &&\n+\t\tpack=\"$(git pack-objects --all $objdir/pack/pack \n---\n midx-write.c | 41 ++++++++++++++++++-----------------------\n 1 file changed, 18 insertions(+), 23 deletions(-)\n\ndiff --git a/midx-write.c b/midx-write.c\nindex 03e95ae821..ad32e8953d 100644\n--- a/midx-write.c\n+++ b/midx-write.c\n@@ -299,21 +299,17 @@ static void midx_fanout_add_pack_fanout(struct midx_fanout *fanout,\n * Copy only the de-duplicated entries (selected by most-recent modified time\n * of a packfile containing the object).\n */\n-static struct pack_midx_entry *get_sorted_entries(struct multi_pack_index *m,\n-\t\t\t\t\t\t struct pack_info *info,\n-\t\t\t\t\t\t uint32_t nr_packs,\n-\t\t\t\t\t\t size_t *nr_objects,\n-\t\t\t\t\t\t int preferred_pack)\n+static struct pack_midx_entry *get_sorted_entries(struct write_midx_context *ctx)\n {\n \tuint32_t cur_fanout, cur_pack, cur_object;\n \tsize_t alloc_objects, total_objects = 0;\n \tstruct midx_fanout fanout = { 0 };\n \tstruct pack_midx_entry *deduplicated_entries = NULL;\n-\tuint32_t start_pack = m ? m->num_packs : 0;\n+\tuint32_t start_pack = ctx->m ? ctx->m->num_packs : 0;\n \n-\tfor (cur_pack = start_pack; cur_pack < nr_packs; cur_pack++)\n+\tfor (cur_pack = start_pack; cur_pack < ctx->nr; cur_pack++)\n \t\ttotal_objects = st_add(total_objects,\n-\t\t\t\t info[cur_pack].p->num_objects);\n+\t\t\t\t ctx->info[cur_pack].p->num_objects);\n \n \t/*\n \t * As we de-duplicate by fanout value, we expect the fanout\n@@ -324,25 +320,25 @@ static struct pack_midx_entry *get_sorted_entries(struct multi_pack_index *m,\n \n \tALLOC_ARRAY(fanout.entries, fanout.alloc);\n \tALLOC_ARRAY(deduplicated_entries, alloc_objects);\n-\t*nr_objects = 0;\n+\tctx->entries_nr = 0;\n \n \tfor (cur_fanout = 0; cur_fanout < 256; cur_fanout++) {\n \t\tfanout.nr = 0;\n \n-\t\tif (m)\n-\t\t\tmidx_fanout_add_midx_fanout(&fanout, m, cur_fanout,\n-\t\t\t\t\t\t preferred_pack);\n+\t\tif (ctx->m)\n+\t\t\tmidx_fanout_add_midx_fanout(&fanout, ctx->m, cur_fanout,\n+\t\t\t\t\t\t ctx->preferred_pack_idx);\n \n-\t\tfor (cur_pack = start_pack; cur_pack < nr_packs; cur_pack++) {\n-\t\t\tint preferred = cur_pack == preferred_pack;\n+\t\tfor (cur_pack = start_pack; cur_pack < ctx->nr; cur_pack++) {\n+\t\t\tint preferred = cur_pack == ctx->preferred_pack_idx;\n \t\t\tmidx_fanout_add_pack_fanout(&fanout,\n-\t\t\t\t\t\t info, cur_pack,\n+\t\t\t\t\t\t ctx->info, cur_pack,\n \t\t\t\t\t\t preferred, cur_fanout);\n \t\t}\n \n-\t\tif (-1 < preferred_pack && preferred_pack < start_pack)\n-\t\t\tmidx_fanout_add_pack_fanout(&fanout, info,\n-\t\t\t\t\t\t preferred_pack, 1,\n+\t\tif (-1 < ctx->preferred_pack_idx && ctx->preferred_pack_idx < start_pack)\n+\t\t\tmidx_fanout_add_pack_fanout(&fanout, ctx->info,\n+\t\t\t\t\t\t ctx->preferred_pack_idx, 1,\n \t\t\t\t\t\t cur_fanout);\n \n \t\tmidx_fanout_sort(&fanout);\n@@ -356,12 +352,12 @@ static struct pack_midx_entry *get_sorted_entries(struct multi_pack_index *m,\n \t\t\t\t\t\t&fanout.entries[cur_object].oid))\n \t\t\t\tcontinue;\n \n-\t\t\tALLOC_GROW(deduplicated_entries, st_add(*nr_objects, 1),\n+\t\t\tALLOC_GROW(deduplicated_entries, st_add(ctx->entries_nr, 1),\n \t\t\t\t alloc_objects);\n-\t\t\tmemcpy(&deduplicated_entries[*nr_objects],\n+\t\t\tmemcpy(&deduplicated_entries[ctx->entries_nr],\n \t\t\t &fanout.entries[cur_object],\n \t\t\t sizeof(struct pack_midx_entry));\n-\t\t\t(*nr_objects)++;\n+\t\t\tctx->entries_nr++;\n \t\t}\n \t}\n \n@@ -1055,8 +1051,7 @@ static int write_midx_internal(const char *object_dir,\n \t\t}\n \t}\n \n-\tctx.entries = get_sorted_entries(ctx.m, ctx.info, ctx.nr, &ctx.entries_nr,\n-\t\t\t\t\t ctx.preferred_pack_idx);\n+\tctx.entries = get_sorted_entries(&ctx);\n \n \tctx.large_offsets_needed = 0;\n \tfor (i = 0; i < ctx.entries_nr; i++) {\n-- \n2.45.1.217.g9bb58e2bf5a.dirty\n\n"},{"id":"495423","messageId":"7acf4557dcb2240cb43eadebfd21b5c37515ba7f.1716482279.git.me@ttaylorr.com","threadId":"61537","inReplyTo":"cover.1716482279.git.me@ttaylorr.com","subject":"[PATCH 3/8] midx-write.c: pass `start_pack` to `get_sorted_entries()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-05-23T16:38:09Z","receivedAt":"2024-05-23T16:38:12Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"The function `get_sorted_entries()` is broadly responsible for\nbuilding an array of the objects to be written into a MIDX based on the\nprovided list of packs.\n\nIf we have loaded an existing MIDX, however, we may not use all of its\npacks, despite loading them into the ctx->info array.\n\nThe existing implementation simply skips past the first\nctx->m->num_packs (if ctx->m is non-NULL, indicating that we loaded an\nexisting MIDX). Future changes (outside the scope of this patch series)\nto the MIDX code will require us to skip *at most* that number[^1].\n\nWe could tag each pack with a bit that indicates the pack's contents\nshould be included in the MIDX. But we can just as easily determine the\nnumber of packs to skip by passing in the number of packs we learned\nabout after processing an existing MIDX.\n\n[^1]: Kind of. The real number will be bounded by the number of packs in\n a MIDX layer, and the number of packs in its base layer(s), but that\n concept hasn't been fully defined yet.\n\nSigned-off-by: Taylor Blau \n---\n midx-write.c | 10 ++++++----\n 1 file changed, 6 insertions(+), 4 deletions(-)\n\ndiff --git a/midx-write.c b/midx-write.c\nindex ad32e8953d..cf7e391b6e 100644\n--- a/midx-write.c\n+++ b/midx-write.c\n@@ -299,13 +299,13 @@ static void midx_fanout_add_pack_fanout(struct midx_fanout *fanout,\n * Copy only the de-duplicated entries (selected by most-recent modified time\n * of a packfile containing the object).\n */\n-static struct pack_midx_entry *get_sorted_entries(struct write_midx_context *ctx)\n+static struct pack_midx_entry *get_sorted_entries(struct write_midx_context *ctx,\n+\t\t\t\t\t\t uint32_t start_pack)\n {\n \tuint32_t cur_fanout, cur_pack, cur_object;\n \tsize_t alloc_objects, total_objects = 0;\n \tstruct midx_fanout fanout = { 0 };\n \tstruct pack_midx_entry *deduplicated_entries = NULL;\n-\tuint32_t start_pack = ctx->m ? ctx->m->num_packs : 0;\n \n \tfor (cur_pack = start_pack; cur_pack < ctx->nr; cur_pack++)\n \t\ttotal_objects = st_add(total_objects,\n@@ -886,7 +886,7 @@ static int write_midx_internal(const char *object_dir,\n {\n \tstruct strbuf midx_name = STRBUF_INIT;\n \tunsigned char midx_hash[GIT_MAX_RAWSZ];\n-\tuint32_t i;\n+\tuint32_t i, start_pack;\n \tstruct hashfile *f = NULL;\n \tstruct lock_file lk;\n \tstruct write_midx_context ctx = { 0 };\n@@ -954,6 +954,8 @@ static int write_midx_internal(const char *object_dir,\n \t\t}\n \t}\n \n+\tstart_pack = ctx.nr;\n+\n \tctx.pack_paths_checked = 0;\n \tif (flags & MIDX_PROGRESS)\n \t\tctx.progress = start_delayed_progress(_(\"Adding packfiles to multi-pack-index\"), 0);\n@@ -1051,7 +1053,7 @@ static int write_midx_internal(const char *object_dir,\n \t\t}\n \t}\n \n-\tctx.entries = get_sorted_entries(&ctx);\n+\tctx.entries = get_sorted_entries(&ctx, start_pack);\n \n \tctx.large_offsets_needed = 0;\n \tfor (i = 0; i < ctx.entries_nr; i++) {\n-- \n2.45.1.217.g9bb58e2bf5a.dirty\n\n"},{"id":"495424","messageId":"3908546ea85eb36a27ce6bd681a3c2152ff005f5.1716482279.git.me@ttaylorr.com","threadId":"61537","inReplyTo":"cover.1716482279.git.me@ttaylorr.com","subject":"[PATCH 4/8] midx-write.c: extract `should_include_pack()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-05-23T16:38:13Z","receivedAt":"2024-05-23T16:38:15Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"The add_pack_to_midx() callback used via for_each_file_in_pack_dir() is\nused to add packs with .idx files to the MIDX being written.\n\nWithin this function, we have a pair of checks that discards packs\nwhich:\n\n - appear in an existing MIDX, if we successfully read an existing MIDX\n from disk\n\n - or, appear in the \"to_include\" list, if invoking the MIDX write\n machinery with the `--stdin-packs` command-line argument.\n\nIn a future commit will want to call a slight variant of these checks\nfrom the code that reuses all packs from an existing MIDX, as well as\nthe current location via add_pack_to_midx(). The latter will be\nmodified in subsequent commits to only reuse packs which appear in the\nto_include list, if one was given.\n\nPrepare for that step by extracting these checks as a subroutine that\nmay be called from both places.\n\nSigned-off-by: Taylor Blau \n---\n midx-write.c | 48 ++++++++++++++++++++++++++++--------------------\n 1 file changed, 28 insertions(+), 20 deletions(-)\n\ndiff --git a/midx-write.c b/midx-write.c\nindex cf7e391b6e..f593cf1593 100644\n--- a/midx-write.c\n+++ b/midx-write.c\n@@ -100,6 +100,32 @@ struct write_midx_context {\n \tstruct string_list *to_include;\n };\n \n+static int should_include_pack(const struct write_midx_context *ctx,\n+\t\t\t const char *file_name)\n+{\n+\t/*\n+\t * Note that at most one of ctx->m and ctx->to_include are set,\n+\t * so we are testing midx_contains_pack() and\n+\t * string_list_has_string() independently (guarded by the\n+\t * appropriate NULL checks).\n+\t *\n+\t * We could support passing to_include while reusing an existing\n+\t * MIDX, but don't currently since the reuse process drags\n+\t * forward all packs from an existing MIDX (without checking\n+\t * whether or not they appear in the to_include list).\n+\t *\n+\t * If we added support for that, these next two conditional\n+\t * should be performed independently (likely checking\n+\t * to_include before the existing MIDX).\n+\t */\n+\tif (ctx->m && midx_contains_pack(ctx->m, file_name))\n+\t\treturn 0;\n+\telse if (ctx->to_include &&\n+\t\t !string_list_has_string(ctx->to_include, file_name))\n+\t\treturn 0;\n+\treturn 1;\n+}\n+\n static void add_pack_to_midx(const char *full_path, size_t full_path_len,\n \t\t\t const char *file_name, void *data)\n {\n@@ -108,29 +134,11 @@ static void add_pack_to_midx(const char *full_path, size_t full_path_len,\n \n \tif (ends_with(file_name, \".idx\")) {\n \t\tdisplay_progress(ctx->progress, ++ctx->pack_paths_checked);\n-\t\t/*\n-\t\t * Note that at most one of ctx->m and ctx->to_include are set,\n-\t\t * so we are testing midx_contains_pack() and\n-\t\t * string_list_has_string() independently (guarded by the\n-\t\t * appropriate NULL checks).\n-\t\t *\n-\t\t * We could support passing to_include while reusing an existing\n-\t\t * MIDX, but don't currently since the reuse process drags\n-\t\t * forward all packs from an existing MIDX (without checking\n-\t\t * whether or not they appear in the to_include list).\n-\t\t *\n-\t\t * If we added support for that, these next two conditional\n-\t\t * should be performed independently (likely checking\n-\t\t * to_include before the existing MIDX).\n-\t\t */\n-\t\tif (ctx->m && midx_contains_pack(ctx->m, file_name))\n-\t\t\treturn;\n-\t\telse if (ctx->to_include &&\n-\t\t\t !string_list_has_string(ctx->to_include, file_name))\n+\n+\t\tif (!should_include_pack(ctx, file_name))\n \t\t\treturn;\n \n \t\tALLOC_GROW(ctx->info, ctx->nr + 1, ctx->alloc);\n-\n \t\tp = add_packed_git(full_path, full_path_len, 0);\n \t\tif (!p) {\n \t\t\twarning(_(\"failed to add packfile '%s'\"),\n-- \n2.45.1.217.g9bb58e2bf5a.dirty\n\n"},{"id":"495425","messageId":"dc813ea1cad4ce50694f21c3bd44174bd414f2d5.1716482279.git.me@ttaylorr.com","threadId":"61537","inReplyTo":"cover.1716482279.git.me@ttaylorr.com","subject":"[PATCH 5/8] midx-write.c: extract `fill_packs_from_midx()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-05-23T16:38:16Z","receivedAt":"2024-05-23T16:38:19Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"When write_midx_internal() loads an existing MIDX, all packs are copied\nforward into the new MIDX. Improve the readability of\nwrite_midx_internal() by extracting this functionality out into a\nseparate function.\n\nSigned-off-by: Taylor Blau \n---\n midx-write.c | 68 +++++++++++++++++++++++++++++-----------------------\n 1 file changed, 38 insertions(+), 30 deletions(-)\n\ndiff --git a/midx-write.c b/midx-write.c\nindex f593cf1593..9712ac044f 100644\n--- a/midx-write.c\n+++ b/midx-write.c\n@@ -885,6 +885,40 @@ static struct multi_pack_index *lookup_multi_pack_index(struct repository *r,\n \treturn result;\n }\n \n+static int fill_packs_from_midx(struct write_midx_context *ctx,\n+\t\t\t\tconst char *preferred_pack_name, uint32_t flags)\n+{\n+\tuint32_t i;\n+\n+\tfor (i = 0; i < ctx->m->num_packs; i++) {\n+\t\tALLOC_GROW(ctx->info, ctx->nr + 1, ctx->alloc);\n+\n+\t\tif (flags & MIDX_WRITE_REV_INDEX || preferred_pack_name) {\n+\t\t\t/*\n+\t\t\t * If generating a reverse index, need to have\n+\t\t\t * packed_git's loaded to compare their\n+\t\t\t * mtimes and object count.\n+\t\t\t *\n+\t\t\t *\n+\t\t\t * If a preferred pack is specified, need to\n+\t\t\t * have packed_git's loaded to ensure the chosen\n+\t\t\t * preferred pack has a non-zero object count.\n+\t\t\t */\n+\t\t\tif (prepare_midx_pack(the_repository, ctx->m, i))\n+\t\t\t\treturn error(_(\"could not load pack\"));\n+\n+\t\t\tif (open_pack_index(ctx->m->packs[i]))\n+\t\t\t\tdie(_(\"could not open index for %s\"),\n+\t\t\t\t ctx->m->packs[i]->pack_name);\n+\t\t}\n+\n+\t\tfill_pack_info(&ctx->info[ctx->nr++], ctx->m->packs[i],\n+\t\t\t ctx->m->pack_names[i], i);\n+\t}\n+\n+\treturn 0;\n+}\n+\n static int write_midx_internal(const char *object_dir,\n \t\t\t struct string_list *packs_to_include,\n \t\t\t struct string_list *packs_to_drop,\n@@ -930,36 +964,10 @@ static int write_midx_internal(const char *object_dir,\n \tctx.info = NULL;\n \tALLOC_ARRAY(ctx.info, ctx.alloc);\n \n-\tif (ctx.m) {\n-\t\tfor (i = 0; i < ctx.m->num_packs; i++) {\n-\t\t\tALLOC_GROW(ctx.info, ctx.nr + 1, ctx.alloc);\n-\n-\t\t\tif (flags & MIDX_WRITE_REV_INDEX ||\n-\t\t\t preferred_pack_name) {\n-\t\t\t\t/*\n-\t\t\t\t * If generating a reverse index, need to have\n-\t\t\t\t * packed_git's loaded to compare their\n-\t\t\t\t * mtimes and object count.\n-\t\t\t\t *\n-\t\t\t\t * If a preferred pack is specified,\n-\t\t\t\t * need to have packed_git's loaded to\n-\t\t\t\t * ensure the chosen preferred pack has\n-\t\t\t\t * a non-zero object count.\n-\t\t\t\t */\n-\t\t\t\tif (prepare_midx_pack(the_repository, ctx.m, i)) {\n-\t\t\t\t\terror(_(\"could not load pack\"));\n-\t\t\t\t\tresult = 1;\n-\t\t\t\t\tgoto cleanup;\n-\t\t\t\t}\n-\n-\t\t\t\tif (open_pack_index(ctx.m->packs[i]))\n-\t\t\t\t\tdie(_(\"could not open index for %s\"),\n-\t\t\t\t\t ctx.m->packs[i]->pack_name);\n-\t\t\t}\n-\n-\t\t\tfill_pack_info(&ctx.info[ctx.nr++], ctx.m->packs[i],\n-\t\t\t\t ctx.m->pack_names[i], i);\n-\t\t}\n+\tif (ctx.m && fill_packs_from_midx(&ctx, preferred_pack_name,\n+\t\t\t\t\t flags) < 0) {\n+\t\tresult = 1;\n+\t\tgoto cleanup;\n \t}\n \n \tstart_pack = ctx.nr;\n-- \n2.45.1.217.g9bb58e2bf5a.dirty\n\n"},{"id":"495426","messageId":"61268114c6562ba882210fd94b3f336efcb5c486.1716482279.git.me@ttaylorr.com","threadId":"61537","inReplyTo":"cover.1716482279.git.me@ttaylorr.com","subject":"[PATCH 6/8] midx-write.c: support reading an existing MIDX with `packs_to_include`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-05-23T16:38:19Z","receivedAt":"2024-05-23T16:38:22Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Avoid unconditionally copying all packs from an existing MIDX into a new\nMIDX by checking that packs added via `fill_packs_from_midx()` don't\nappear in the `to_include` set, if one was provided.\n\nDo so by calling `should_include_pack()` from both `add_pack_to_midx()`\nand `fill_packs_from_midx()`.\n\nIn order to make this work, teach `should_include_pack()` a new\n\"exclude_from_midx\" parameter, which allows skipping the first check.\nThis is done so that the caller in `fill_packs_from_midx()` doesn't\nreject all of the packs it provided since they appear in an existing\nMIDX by definition.\n\nSigned-off-by: Taylor Blau \n---\n midx-write.c | 42 +++++++++++-------------------------------\n 1 file changed, 11 insertions(+), 31 deletions(-)\n\ndiff --git a/midx-write.c b/midx-write.c\nindex 9712ac044f..36ac4ab65b 100644\n--- a/midx-write.c\n+++ b/midx-write.c\n@@ -101,27 +101,13 @@ struct write_midx_context {\n };\n \n static int should_include_pack(const struct write_midx_context *ctx,\n-\t\t\t const char *file_name)\n+\t\t\t const char *file_name,\n+\t\t\t int exclude_from_midx)\n {\n-\t/*\n-\t * Note that at most one of ctx->m and ctx->to_include are set,\n-\t * so we are testing midx_contains_pack() and\n-\t * string_list_has_string() independently (guarded by the\n-\t * appropriate NULL checks).\n-\t *\n-\t * We could support passing to_include while reusing an existing\n-\t * MIDX, but don't currently since the reuse process drags\n-\t * forward all packs from an existing MIDX (without checking\n-\t * whether or not they appear in the to_include list).\n-\t *\n-\t * If we added support for that, these next two conditional\n-\t * should be performed independently (likely checking\n-\t * to_include before the existing MIDX).\n-\t */\n-\tif (ctx->m && midx_contains_pack(ctx->m, file_name))\n+\tif (exclude_from_midx && ctx->m && midx_contains_pack(ctx->m, file_name))\n \t\treturn 0;\n-\telse if (ctx->to_include &&\n-\t\t !string_list_has_string(ctx->to_include, file_name))\n+\tif (ctx->to_include && !string_list_has_string(ctx->to_include,\n+\t\t\t\t\t\t file_name))\n \t\treturn 0;\n \treturn 1;\n }\n@@ -135,7 +121,7 @@ static void add_pack_to_midx(const char *full_path, size_t full_path_len,\n \tif (ends_with(file_name, \".idx\")) {\n \t\tdisplay_progress(ctx->progress, ++ctx->pack_paths_checked);\n \n-\t\tif (!should_include_pack(ctx, file_name))\n+\t\tif (!should_include_pack(ctx, file_name, 1))\n \t\t\treturn;\n \n \t\tALLOC_GROW(ctx->info, ctx->nr + 1, ctx->alloc);\n@@ -891,6 +877,9 @@ static int fill_packs_from_midx(struct write_midx_context *ctx,\n \tuint32_t i;\n \n \tfor (i = 0; i < ctx->m->num_packs; i++) {\n+\t\tif (!should_include_pack(ctx, ctx->m->pack_names[i], 0))\n+\t\t\tcontinue;\n+\n \t\tALLOC_GROW(ctx->info, ctx->nr + 1, ctx->alloc);\n \n \t\tif (flags & MIDX_WRITE_REV_INDEX || preferred_pack_name) {\n@@ -945,15 +934,7 @@ static int write_midx_internal(const char *object_dir,\n \t\tdie_errno(_(\"unable to create leading directories of %s\"),\n \t\t\t midx_name.buf);\n \n-\tif (!packs_to_include) {\n-\t\t/*\n-\t\t * Only reference an existing MIDX when not filtering which\n-\t\t * packs to include, since all packs and objects are copied\n-\t\t * blindly from an existing MIDX if one is present.\n-\t\t */\n-\t\tctx.m = lookup_multi_pack_index(the_repository, object_dir);\n-\t}\n-\n+\tctx.m = lookup_multi_pack_index(the_repository, object_dir);\n \tif (ctx.m && !midx_checksum_valid(ctx.m)) {\n \t\twarning(_(\"ignoring existing multi-pack-index; checksum mismatch\"));\n \t\tctx.m = NULL;\n@@ -962,6 +943,7 @@ static int write_midx_internal(const char *object_dir,\n \tctx.nr = 0;\n \tctx.alloc = ctx.m ? ctx.m->num_packs : 16;\n \tctx.info = NULL;\n+\tctx.to_include = packs_to_include;\n \tALLOC_ARRAY(ctx.info, ctx.alloc);\n \n \tif (ctx.m && fill_packs_from_midx(&ctx, preferred_pack_name,\n@@ -978,8 +960,6 @@ static int write_midx_internal(const char *object_dir,\n \telse\n \t\tctx.progress = NULL;\n \n-\tctx.to_include = packs_to_include;\n-\n \tfor_each_file_in_pack_dir(object_dir, add_pack_to_midx, &ctx);\n \tstop_progress(&ctx.progress);\n \n-- \n2.45.1.217.g9bb58e2bf5a.dirty\n\n"},{"id":"495427","messageId":"f4c0167f437a04943b67d286ebcbbb7e0c5aa39e.1716482279.git.me@ttaylorr.com","threadId":"61537","inReplyTo":"cover.1716482279.git.me@ttaylorr.com","subject":"[PATCH 7/8] midx: replace `get_midx_rev_filename()` with a generic helper","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-05-23T16:38:22Z","receivedAt":"2024-05-23T16:38:25Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Commit f894081deae (pack-revindex: read multi-pack reverse indexes,\n2021-03-30) introduced the `get_midx_rev_filename()` helper (later\nmodified by commit 60980aed786 (midx.c: write MIDX filenames to\nstrbuf, 2021-10-26)).\n\nThis function returns the location of the classic \".rev\" files we used\nto write for MIDXs (prior to 95e8383bac1 (midx.c: make changing the\npreferred pack safe, 2022-01-25)), which is always of the form:\n\n $GIT_DIR/objects/pack/multi-pack-index-$HASH.rev\n\nReplace this function with a generic helper that populates a strbuf with\nthe above form, replacing the \".rev\" extension with a caller-provided\nargument.\n\nThis will allow us to remove a similarly-defined function in the\npack-bitmap code (used to determine the location of a MIDX .bitmap file)\nby reimplementing it in terms of `get_midx_filename_ext()`.\n\nSigned-off-by: Taylor Blau \n---\n midx.c | 14 ++++++++------\n midx.h | 6 +++++-\n pack-revindex.c | 3 ++-\n 3 files changed, 15 insertions(+), 8 deletions(-)\n\ndiff --git a/midx.c b/midx.c\nindex 6f07de3688..bc4797196f 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -24,14 +24,16 @@ const unsigned char *get_midx_checksum(struct multi_pack_index *m)\n }\n \n void get_midx_filename(struct strbuf *out, const char *object_dir)\n+{\n+\tget_midx_filename_ext(out, object_dir, NULL, NULL);\n+}\n+\n+void get_midx_filename_ext(struct strbuf *out, const char *object_dir,\n+\t\t\t const unsigned char *hash, const char *ext)\n {\n \tstrbuf_addf(out, \"%s/pack/multi-pack-index\", object_dir);\n-}\n-\n-void get_midx_rev_filename(struct strbuf *out, struct multi_pack_index *m)\n-{\n-\tget_midx_filename(out, m->object_dir);\n-\tstrbuf_addf(out, \"-%s.rev\", hash_to_hex(get_midx_checksum(m)));\n+\tif (ext)\n+\t\tstrbuf_addf(out, \"-%s.%s\", hash_to_hex(hash), ext);\n }\n \n static int midx_read_oid_fanout(const unsigned char *chunk_start,\ndiff --git a/midx.h b/midx.h\nindex dc477dff44..8554f2d616 100644\n--- a/midx.h\n+++ b/midx.h\n@@ -74,9 +74,13 @@ struct multi_pack_index {\n #define MIDX_WRITE_BITMAP_HASH_CACHE (1 << 3)\n #define MIDX_WRITE_BITMAP_LOOKUP_TABLE (1 << 4)\n \n+#define MIDX_EXT_REV \"rev\"\n+#define MIDX_EXT_BITMAP \"bitmap\"\n+\n const unsigned char *get_midx_checksum(struct multi_pack_index *m);\n void get_midx_filename(struct strbuf *out, const char *object_dir);\n-void get_midx_rev_filename(struct strbuf *out, struct multi_pack_index *m);\n+void get_midx_filename_ext(struct strbuf *out, const char *object_dir,\n+\t\t\t const unsigned char *hash, const char *ext);\n \n struct multi_pack_index *load_multi_pack_index(const char *object_dir, int local);\n int prepare_midx_pack(struct repository *r, struct multi_pack_index *m, uint32_t pack_int_id);\ndiff --git a/pack-revindex.c b/pack-revindex.c\nindex a7624d8be8..fc63aa76a2 100644\n--- a/pack-revindex.c\n+++ b/pack-revindex.c\n@@ -381,7 +381,8 @@ int load_midx_revindex(struct multi_pack_index *m)\n \ttrace2_data_string(\"load_midx_revindex\", the_repository,\n \t\t\t \"source\", \"rev\");\n \n-\tget_midx_rev_filename(&revindex_name, m);\n+\tget_midx_filename_ext(&revindex_name, m->object_dir,\n+\t\t\t get_midx_checksum(m), MIDX_EXT_REV);\n \n \tret = load_revindex_from_disk(revindex_name.buf,\n \t\t\t\t m->num_objects,\n-- \n2.45.1.217.g9bb58e2bf5a.dirty\n\n"},{"id":"495428","messageId":"79e3f7f83f4085eab03de6d0809e8b5d1bae4411.1716482279.git.me@ttaylorr.com","threadId":"61537","inReplyTo":"cover.1716482279.git.me@ttaylorr.com","subject":"[PATCH 8/8] pack-bitmap.c: reimplement `midx_bitmap_filename()` with helper","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-05-23T16:38:26Z","receivedAt":"2024-05-23T16:38:28Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Now that we have the `get_midx_filename_ext()` helper, we can\nreimplement the `midx_bitmap_filename()` function in terms of it.\n\nSigned-off-by: Taylor Blau \n---\n pack-bitmap.c | 5 ++---\n 1 file changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex 35c5ef9d3c..fe8e8a51d3 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -309,9 +309,8 @@ static int load_bitmap_entries_v1(struct bitmap_index *index)\n char *midx_bitmap_filename(struct multi_pack_index *midx)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n-\n-\tget_midx_filename(&buf, midx->object_dir);\n-\tstrbuf_addf(&buf, \"-%s.bitmap\", hash_to_hex(get_midx_checksum(midx)));\n+\tget_midx_filename_ext(&buf, midx->object_dir, get_midx_checksum(midx),\n+\t\t\t MIDX_EXT_BITMAP);\n \n \treturn strbuf_detach(&buf, NULL);\n }\n-- \n2.45.1.217.g9bb58e2bf5a.dirty\n"},{"id":"495787","messageId":"ZlbdnkFIg1H_KQxS@tanuki","threadId":"61537","inReplyTo":"c753bc379b005ecaf131f8f1ae9c5b80b2712759.1716482279.git.me@ttaylorr.com","subject":"Re: [PATCH 1/8] midx-write.c: tolerate `--preferred-pack` without bitmaps","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-05-29T07:47:52Z","receivedAt":"2024-05-29T07:47:59Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, May 23, 2024 at 12:38:03PM -0400, Taylor Blau wrote:\n> When passing a preferred pack to the MIDX write machinery, we ensure\n> that the given preferred pack is non-empty since 5d3cd09a808 (midx:\n> reject empty `--preferred-pack`'s, 2021-08-31).\n> \n> However packs are only loaded (via `write_midx_internal()`, though a\n> subsequent patch will refactor this code out to its own function) when\n> the `MIDX_WRITE_REV_INDEX` flag is set.\n> \n> So if a caller runs:\n> \n> $ git multi-pack-index write --preferred-pack=...\n> \n> with both (a) an existing MIDX, and (b) specifies a pack from that MIDX\n> as the preferred one, without passing `--bitmap`, then the check added\n> in 5d3cd09a808 will result in a segfault.\n\nThe check you're talking about is the following one, right?\n\n if (ctx.preferred_pack_idx > -1) {\n struct packed_git *preferred = ctx.info[ctx.preferred_pack_idx].p;\n if (!preferred->num_objects) {\n error(_(\"cannot select preferred pack %s with no objects\"),\n preferred->pack_name);\n result = 1;\n goto cleanup;\n }\n }\n\nAnd the segfault is because the index wasn't populated, and thus\n`ctx.info[ctx.preferred_pack_idx].p == NULL`?\n\n> Note that packs loaded from disk which don't appear in an existing MIDX\n> do not trigger this issue, as those packs are loaded unconditionally. We\n> conditionally load packs from a MIDX since we tolerate MIDXs whose\n> packs do not resolve (i.e., via the MIDX write after removing\n> unreferenced packs via 'git multi-pack-index expire').\n> \n> In practice, this isn't possible to trigger when running `git\n> multi-pack-index write` from via `git repack`, as the latter always\n\ns/from via/via/\n\n> passes `--stdin-packs`, which prevents us from loading an existing MIDX,\n> as it forces all packs to be read from disk.\n> \n> But a future commit in this series will change that behavior to\n> unconditionally load an existing MIDX, even with `--stdin-packs`, making\n> this behavior trigger-able from 'repack' much more easily.\n> \n> Prevent this from being an issue by removing the segfault altogether by\n\nRemoving segfaults is always good :)\n\n> calling `prepare_midx_pack()` on packs loaded from an existing MIDX when\n> either the `MIDX_WRITE_REV_INDEX` flag is set *or* we specified a\n> `--preferred-pack`.\n> \n> Signed-off-by: Taylor Blau \n> ---\n> midx-write.c | 8 +++++++-\n> t/t5319-multi-pack-index.sh | 23 +++++++++++++++++++++++\n> 2 files changed, 30 insertions(+), 1 deletion(-)\n> \n> diff --git a/midx-write.c b/midx-write.c\n> index 9d096d5a28..03e95ae821 100644\n> --- a/midx-write.c\n> +++ b/midx-write.c\n> @@ -930,11 +930,17 @@ static int write_midx_internal(const char *object_dir,\n> \t\tfor (i = 0; i < ctx.m->num_packs; i++) {\n> \t\t\tALLOC_GROW(ctx.info, ctx.nr + 1, ctx.alloc);\n> \n> -\t\t\tif (flags & MIDX_WRITE_REV_INDEX) {\n> +\t\t\tif (flags & MIDX_WRITE_REV_INDEX ||\n> +\t\t\t preferred_pack_name) {\n> \t\t\t\t/*\n> \t\t\t\t * If generating a reverse index, need to have\n> \t\t\t\t * packed_git's loaded to compare their\n> \t\t\t\t * mtimes and object count.\n> +\t\t\t\t *\n> +\t\t\t\t * If a preferred pack is specified,\n> +\t\t\t\t * need to have packed_git's loaded to\n> +\t\t\t\t * ensure the chosen preferred pack has\n> +\t\t\t\t * a non-zero object count.\n> \t\t\t\t */\n> \t\t\t\tif (prepare_midx_pack(the_repository, ctx.m, i)) {\n> \t\t\t\t\terror(_(\"could not load pack\"));\n\nWe now end up loading all packs, but in practice it should be sufficient\nto only load the preferred pack, right? Is there a particular reason why\nwe now load all packs?\n\nPatrick\n"},{"id":"495788","messageId":"ZlbdrczSqKgueK0g@tanuki","threadId":"61537","inReplyTo":"07dad5a5812794be6e355b1e0eb3722d452f292b.1716482279.git.me@ttaylorr.com","subject":"Re: [PATCH 2/8] midx-write.c: reduce argument count for `get_sorted_entries()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-05-29T07:47:57Z","receivedAt":"2024-05-29T07:48:02Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, May 23, 2024 at 12:38:06PM -0400, Taylor Blau wrote:\n> diff --git a/midx-write.c b/midx-write.c\n> index 03e95ae821..ad32e8953d 100644\n> --- a/midx-write.c\n> +++ b/midx-write.c\n> @@ -299,21 +299,17 @@ static void midx_fanout_add_pack_fanout(struct midx_fanout *fanout,\n> * Copy only the de-duplicated entries (selected by most-recent modified time\n> * of a packfile containing the object).\n> */\n> -static struct pack_midx_entry *get_sorted_entries(struct multi_pack_index *m,\n> -\t\t\t\t\t\t struct pack_info *info,\n> -\t\t\t\t\t\t uint32_t nr_packs,\n> -\t\t\t\t\t\t size_t *nr_objects,\n> -\t\t\t\t\t\t int preferred_pack)\n> +static struct pack_midx_entry *get_sorted_entries(struct write_midx_context *ctx)\n> {\n> \tuint32_t cur_fanout, cur_pack, cur_object;\n> \tsize_t alloc_objects, total_objects = 0;\n> \tstruct midx_fanout fanout = { 0 };\n> \tstruct pack_midx_entry *deduplicated_entries = NULL;\n> -\tuint32_t start_pack = m ? m->num_packs : 0;\n> +\tuint32_t start_pack = ctx->m ? ctx->m->num_packs : 0;\n> \n> -\tfor (cur_pack = start_pack; cur_pack < nr_packs; cur_pack++)\n> +\tfor (cur_pack = start_pack; cur_pack < ctx->nr; cur_pack++)\n> \t\ttotal_objects = st_add(total_objects,\n> -\t\t\t\t info[cur_pack].p->num_objects);\n> +\t\t\t\t ctx->info[cur_pack].p->num_objects);\n> \n> \t/*\n> \t * As we de-duplicate by fanout value, we expect the fanout\n> @@ -324,25 +320,25 @@ static struct pack_midx_entry *get_sorted_entries(struct multi_pack_index *m,\n> \n> \tALLOC_ARRAY(fanout.entries, fanout.alloc);\n> \tALLOC_ARRAY(deduplicated_entries, alloc_objects);\n> -\t*nr_objects = 0;\n> +\tctx->entries_nr = 0;\n\nNit: I think it's a bit surprising that a getter function would modify\nthe passed in structure. It's also a bit puzzling that we assign\n`entries_nr` in here, but rely on the caller to set the corresponding\n`entries` field. I think we should either have the caller assign both\nfields, or we should rename the function and assign both of these fields\nin the function.\n\nPatrick\n"},{"id":"495789","messageId":"Zlbdsz5VSlX89HfH@tanuki","threadId":"61537","inReplyTo":"7acf4557dcb2240cb43eadebfd21b5c37515ba7f.1716482279.git.me@ttaylorr.com","subject":"Re: [PATCH 3/8] midx-write.c: pass `start_pack` to `get_sorted_entries()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-05-29T07:48:03Z","receivedAt":"2024-05-29T07:48:08Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, May 23, 2024 at 12:38:09PM -0400, Taylor Blau wrote:\n> The function `get_sorted_entries()` is broadly responsible for\n> building an array of the objects to be written into a MIDX based on the\n> provided list of packs.\n> \n> If we have loaded an existing MIDX, however, we may not use all of its\n> packs, despite loading them into the ctx->info array.\n> \n> The existing implementation simply skips past the first\n> ctx->m->num_packs (if ctx->m is non-NULL, indicating that we loaded an\n> existing MIDX). Future changes (outside the scope of this patch series)\n> to the MIDX code will require us to skip *at most* that number[^1].\n> \n> We could tag each pack with a bit that indicates the pack's contents\n> should be included in the MIDX. But we can just as easily determine the\n> number of packs to skip by passing in the number of packs we learned\n> about after processing an existing MIDX.\n\nWill we always want to skip packs from the start of the array? Or may it\nhappen that we want to skip packs in the middle of it? It's a bit hard\nto judge because there isn't a ton of context when exactly we'll want to\nskip, and why.\n\nPatrick\n"},{"id":"495790","messageId":"ZlbduH9UZZIm8pNh@tanuki","threadId":"61537","inReplyTo":"3908546ea85eb36a27ce6bd681a3c2152ff005f5.1716482279.git.me@ttaylorr.com","subject":"Re: [PATCH 4/8] midx-write.c: extract `should_include_pack()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-05-29T07:48:08Z","receivedAt":"2024-05-29T07:48:12Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, May 23, 2024 at 12:38:13PM -0400, Taylor Blau wrote:\n> The add_pack_to_midx() callback used via for_each_file_in_pack_dir() is\n> used to add packs with .idx files to the MIDX being written.\n> \n> Within this function, we have a pair of checks that discards packs\n> which:\n> \n> - appear in an existing MIDX, if we successfully read an existing MIDX\n> from disk\n> \n> - or, appear in the \"to_include\" list, if invoking the MIDX write\n> machinery with the `--stdin-packs` command-line argument.\n> \n> In a future commit will want to call a slight variant of these checks\n\nEither s/In a/A/ or s/commit/&, we/.\n\nPatrick\n"},{"id":"495791","messageId":"ZlbdxJmIBvgV_VIF@tanuki","threadId":"61537","inReplyTo":"61268114c6562ba882210fd94b3f336efcb5c486.1716482279.git.me@ttaylorr.com","subject":"Re: [PATCH 6/8] midx-write.c: support reading an existing MIDX with `packs_to_include`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-05-29T07:48:20Z","receivedAt":"2024-05-29T07:48:24Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, May 23, 2024 at 12:38:19PM -0400, Taylor Blau wrote:\n> Avoid unconditionally copying all packs from an existing MIDX into a new\n> MIDX by checking that packs added via `fill_packs_from_midx()` don't\n> appear in the `to_include` set, if one was provided.\n> \n> Do so by calling `should_include_pack()` from both `add_pack_to_midx()`\n> and `fill_packs_from_midx()`.\n\nThis is missing an explanation why exactly we want that. Is the current\nbehaviour a bug? Is it a preparation for a future change? Is this change\nexpected to modify any existing behaviour?\n\nReading through the patch we now unconditionally load the existing MIDX\nwhen writing a new one, but I'm not exactly sure what the effect of that\nis going to be.\n\n[snip]\n> diff --git a/midx-write.c b/midx-write.c\n> index 9712ac044f..36ac4ab65b 100644\n> --- a/midx-write.c\n> +++ b/midx-write.c\n> @@ -101,27 +101,13 @@ struct write_midx_context {\n> };\n> \n> static int should_include_pack(const struct write_midx_context *ctx,\n> -\t\t\t const char *file_name)\n> +\t\t\t const char *file_name,\n> +\t\t\t int exclude_from_midx)\n> {\n> -\t/*\n> -\t * Note that at most one of ctx->m and ctx->to_include are set,\n> -\t * so we are testing midx_contains_pack() and\n> -\t * string_list_has_string() independently (guarded by the\n> -\t * appropriate NULL checks).\n> -\t *\n> -\t * We could support passing to_include while reusing an existing\n> -\t * MIDX, but don't currently since the reuse process drags\n> -\t * forward all packs from an existing MIDX (without checking\n> -\t * whether or not they appear in the to_include list).\n> -\t *\n> -\t * If we added support for that, these next two conditional\n> -\t * should be performed independently (likely checking\n> -\t * to_include before the existing MIDX).\n> -\t */\n> -\tif (ctx->m && midx_contains_pack(ctx->m, file_name))\n> +\tif (exclude_from_midx && ctx->m && midx_contains_pack(ctx->m, file_name))\n> \t\treturn 0;\n> -\telse if (ctx->to_include &&\n> -\t\t !string_list_has_string(ctx->to_include, file_name))\n> +\tif (ctx->to_include && !string_list_has_string(ctx->to_include,\n> +\t\t\t\t\t\t file_name))\n\nThe second branch is a no-op change, right? The only change was that you\nconverted from `else if` to `if`. I'd propose to either keep this as-is,\nor to do this change in the preceding patch already that introduces this\nfunction.\n\nPatrick\n"},{"id":"495865","messageId":"ZlesVWe/22lFTgGl@nand.local","threadId":"61537","inReplyTo":"ZlbdnkFIg1H_KQxS@tanuki","subject":"Re: [PATCH 1/8] midx-write.c: tolerate `--preferred-pack` without bitmaps","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-05-29T22:29:41Z","receivedAt":"2024-05-29T22:29:51Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, May 29, 2024 at 09:47:52AM +0200, Patrick Steinhardt wrote:\n> On Thu, May 23, 2024 at 12:38:03PM -0400, Taylor Blau wrote:\n> > When passing a preferred pack to the MIDX write machinery, we ensure\n> > that the given preferred pack is non-empty since 5d3cd09a808 (midx:\n> > reject empty `--preferred-pack`'s, 2021-08-31).\n> >\n> > However packs are only loaded (via `write_midx_internal()`, though a\n> > subsequent patch will refactor this code out to its own function) when\n> > the `MIDX_WRITE_REV_INDEX` flag is set.\n> >\n> > So if a caller runs:\n> >\n> > $ git multi-pack-index write --preferred-pack=...\n> >\n> > with both (a) an existing MIDX, and (b) specifies a pack from that MIDX\n> > as the preferred one, without passing `--bitmap`, then the check added\n> > in 5d3cd09a808 will result in a segfault.\n>\n> The check you're talking about is the following one, right?\n>\n> if (ctx.preferred_pack_idx > -1) {\n> struct packed_git *preferred = ctx.info[ctx.preferred_pack_idx].p;\n> if (!preferred->num_objects) {\n> error(_(\"cannot select preferred pack %s with no objects\"),\n> preferred->pack_name);\n> result = 1;\n> goto cleanup;\n> }\n> }\n>\n> And the segfault is because the index wasn't populated, and thus\n> `ctx.info[ctx.preferred_pack_idx].p == NULL`?\n\nExactly.\n\n> > Note that packs loaded from disk which don't appear in an existing MIDX\n> > do not trigger this issue, as those packs are loaded unconditionally. We\n> > conditionally load packs from a MIDX since we tolerate MIDXs whose\n> > packs do not resolve (i.e., via the MIDX write after removing\n> > unreferenced packs via 'git multi-pack-index expire').\n> >\n> > In practice, this isn't possible to trigger when running `git\n> > multi-pack-index write` from via `git repack`, as the latter always\n>\n> s/from via/via/\n\nAh, oops, thanks.\n\n> > passes `--stdin-packs`, which prevents us from loading an existing MIDX,\n> > as it forces all packs to be read from disk.\n> >\n> > But a future commit in this series will change that behavior to\n> > unconditionally load an existing MIDX, even with `--stdin-packs`, making\n> > this behavior trigger-able from 'repack' much more easily.\n> >\n> > Prevent this from being an issue by removing the segfault altogether by\n>\n> Removing segfaults is always good :)\n>\n> > calling `prepare_midx_pack()` on packs loaded from an existing MIDX when\n> > either the `MIDX_WRITE_REV_INDEX` flag is set *or* we specified a\n> > `--preferred-pack`.\n> >\n> > Signed-off-by: Taylor Blau \n> > ---\n> > midx-write.c | 8 +++++++-\n> > t/t5319-multi-pack-index.sh | 23 +++++++++++++++++++++++\n> > 2 files changed, 30 insertions(+), 1 deletion(-)\n> >\n> > diff --git a/midx-write.c b/midx-write.c\n> > index 9d096d5a28..03e95ae821 100644\n> > --- a/midx-write.c\n> > +++ b/midx-write.c\n> > @@ -930,11 +930,17 @@ static int write_midx_internal(const char *object_dir,\n> > \t\tfor (i = 0; i < ctx.m->num_packs; i++) {\n> > \t\t\tALLOC_GROW(ctx.info, ctx.nr + 1, ctx.alloc);\n> >\n> > -\t\t\tif (flags & MIDX_WRITE_REV_INDEX) {\n> > +\t\t\tif (flags & MIDX_WRITE_REV_INDEX ||\n> > +\t\t\t preferred_pack_name) {\n> > \t\t\t\t/*\n> > \t\t\t\t * If generating a reverse index, need to have\n> > \t\t\t\t * packed_git's loaded to compare their\n> > \t\t\t\t * mtimes and object count.\n> > +\t\t\t\t *\n> > +\t\t\t\t * If a preferred pack is specified,\n> > +\t\t\t\t * need to have packed_git's loaded to\n> > +\t\t\t\t * ensure the chosen preferred pack has\n> > +\t\t\t\t * a non-zero object count.\n> > \t\t\t\t */\n> > \t\t\t\tif (prepare_midx_pack(the_repository, ctx.m, i)) {\n> > \t\t\t\t\terror(_(\"could not load pack\"));\n>\n> We now end up loading all packs, but in practice it should be sufficient\n> to only load the preferred pack, right? Is there a particular reason why\n> we now load all packs?\n\nI had originally considered that, but discarded the idea because it\nseemed like it might be fragile to depend on those two points in the\ncode having the exact same notion of what the preferred pack is.\n\nSince it's cheap enough to load all of these packs in unconditionally\nanyways, that's the route that I ended up taking here.\n\nThanks,\nTaylor\n"},{"id":"495866","messageId":"Zletr6Q7QBgQ0CgQ@nand.local","threadId":"61537","inReplyTo":"ZlbdrczSqKgueK0g@tanuki","subject":"Re: [PATCH 2/8] midx-write.c: reduce argument count for `get_sorted_entries()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-05-29T22:35:27Z","receivedAt":"2024-05-29T22:35:29Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, May 29, 2024 at 09:47:57AM +0200, Patrick Steinhardt wrote:\n> Nit: I think it's a bit surprising that a getter function would modify\n> the passed in structure. It's also a bit puzzling that we assign\n> `entries_nr` in here, but rely on the caller to set the corresponding\n> `entries` field. I think we should either have the caller assign both\n> fields, or we should rename the function and assign both of these fields\n> in the function.\n\nThanks, I agree and should have changed the name of this function in the\nexisting round.\n\nI renamed it to \"compute_sorted_entries()\" which takes only a single\n\"struct write_midx_context *\", and assigns both \"ctx->entries\" and\n\"ctx->entries_nr\".\n\nThanks,\nTaylor\n"},{"id":"495867","messageId":"Zlet3mhO+tLT4yj8@nand.local","threadId":"61537","inReplyTo":"Zlbdsz5VSlX89HfH@tanuki","subject":"Re: [PATCH 3/8] midx-write.c: pass `start_pack` to `get_sorted_entries()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-05-29T22:36:14Z","receivedAt":"2024-05-29T22:36:17Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, May 29, 2024 at 09:48:03AM +0200, Patrick Steinhardt wrote:\n> Will we always want to skip packs from the start of the array? Or may it\n> happen that we want to skip packs in the middle of it? It's a bit hard\n> to judge because there isn't a ton of context when exactly we'll want to\n> skip, and why.\n\nIt's always from the start of the array, but I'll add some more context\ninto the patch message as to why this is.\n\nThanks,\nTaylor\n"},{"id":"495870","messageId":"Zleu90gpuXt/qipa@nand.local","threadId":"61537","inReplyTo":"ZlbduH9UZZIm8pNh@tanuki","subject":"Re: [PATCH 4/8] midx-write.c: extract `should_include_pack()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-05-29T22:40:55Z","receivedAt":"2024-05-29T22:40:57Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, May 29, 2024 at 09:48:08AM +0200, Patrick Steinhardt wrote:\n> On Thu, May 23, 2024 at 12:38:13PM -0400, Taylor Blau wrote:\n> > The add_pack_to_midx() callback used via for_each_file_in_pack_dir() is\n> > used to add packs with .idx files to the MIDX being written.\n> >\n> > Within this function, we have a pair of checks that discards packs\n> > which:\n> >\n> > - appear in an existing MIDX, if we successfully read an existing MIDX\n> > from disk\n> >\n> > - or, appear in the \"to_include\" list, if invoking the MIDX write\n> > machinery with the `--stdin-packs` command-line argument.\n> >\n> > In a future commit will want to call a slight variant of these checks\n>\n> Either s/In a/A/ or s/commit/&, we/.\n\nThanks for your careful review, it is much appreciated! I meant to write\nthe first one, and will correct it in the subsequent round :-).\n\nThanks,\nTaylor\n"},{"id":"495871","messageId":"ZlewQ0NXmJTw4hWt@nand.local","threadId":"61537","inReplyTo":"ZlbdxJmIBvgV_VIF@tanuki","subject":"Re: [PATCH 6/8] midx-write.c: support reading an existing MIDX with `packs_to_include`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-05-29T22:46:27Z","receivedAt":"2024-05-29T22:46:30Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, May 29, 2024 at 09:48:20AM +0200, Patrick Steinhardt wrote:\n> On Thu, May 23, 2024 at 12:38:19PM -0400, Taylor Blau wrote:\n> > Avoid unconditionally copying all packs from an existing MIDX into a new\n> > MIDX by checking that packs added via `fill_packs_from_midx()` don't\n> > appear in the `to_include` set, if one was provided.\n> >\n> > Do so by calling `should_include_pack()` from both `add_pack_to_midx()`\n> > and `fill_packs_from_midx()`.\n>\n> This is missing an explanation why exactly we want that. Is the current\n> behaviour a bug? Is it a preparation for a future change? Is this change\n> expected to modify any existing behaviour?\n>\n> Reading through the patch we now unconditionally load the existing MIDX\n> when writing a new one, but I'm not exactly sure what the effect of that\n> is going to be.\n\nVery fair. The short answer is that this is a prerequisite for the\nincremental MIDX series that I'm working on. The longer answer is that\nan incremental MIDX-aware writer needs to be able to consult with the\nexisting MIDX (if one exists) and exclude any objects which already\nappear in an earlier layer of the MIDX. This is done because we cannot\nhave the same object appear in multiple layers of the MIDX, for reasons\nthat are probably not interesting to this series.\n\nI put a more concise version of the explanation above into this patch\nwhich I'll send another round of in v2 of this series shortly.\n\n> [snip]\n> > diff --git a/midx-write.c b/midx-write.c\n> > index 9712ac044f..36ac4ab65b 100644\n> > --- a/midx-write.c\n> > +++ b/midx-write.c\n> > @@ -101,27 +101,13 @@ struct write_midx_context {\n> > };\n> >\n> > static int should_include_pack(const struct write_midx_context *ctx,\n> > -\t\t\t const char *file_name)\n> > +\t\t\t const char *file_name,\n> > +\t\t\t int exclude_from_midx)\n> > {\n> > -\t/*\n> > -\t * Note that at most one of ctx->m and ctx->to_include are set,\n> > -\t * so we are testing midx_contains_pack() and\n> > -\t * string_list_has_string() independently (guarded by the\n> > -\t * appropriate NULL checks).\n> > -\t *\n> > -\t * We could support passing to_include while reusing an existing\n> > -\t * MIDX, but don't currently since the reuse process drags\n> > -\t * forward all packs from an existing MIDX (without checking\n> > -\t * whether or not they appear in the to_include list).\n> > -\t *\n> > -\t * If we added support for that, these next two conditional\n> > -\t * should be performed independently (likely checking\n> > -\t * to_include before the existing MIDX).\n> > -\t */\n> > -\tif (ctx->m && midx_contains_pack(ctx->m, file_name))\n> > +\tif (exclude_from_midx && ctx->m && midx_contains_pack(ctx->m, file_name))\n> > \t\treturn 0;\n> > -\telse if (ctx->to_include &&\n> > -\t\t !string_list_has_string(ctx->to_include, file_name))\n> > +\tif (ctx->to_include && !string_list_has_string(ctx->to_include,\n> > +\t\t\t\t\t\t file_name))\n>\n> The second branch is a no-op change, right? The only change was that you\n> converted from `else if` to `if`. I'd propose to either keep this as-is,\n> or to do this change in the preceding patch already that introduces this\n> function.\n\nIt is a no-op, but I would rather keep these separate to keep the\nprevious step a pure code movement rather than introducing any textual\nchanges.\n\nThanks,\nTaylor\n"},{"id":"495872","messageId":"cover.1717023301.git.me@ttaylorr.com","threadId":"61537","inReplyTo":"cover.1716482279.git.me@ttaylorr.com","subject":"[PATCH v2 0/8] midx-write: miscellaneous clean-ups for incremental MIDXs","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-05-29T22:55:15Z","receivedAt":"2024-05-29T22:55:19Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"This is a small reroll of my series which has a grab-bag of midx-write\nrelated cleanups that I pulled out of a larger series to implement\nincremental MIDX chains.\n\nThis series is mostly the same as last time, but notable changes\ninclude:\n\n - renamed `get_sorted_entries()` to `compute_sorted_entries()` to\n avoid confusion that the function performs a side-effecting write to\n `ctx->entries_nr`.\n\n - fixed a handful of typos\n\n - add explanations in a couple of the patches to better motivate the\n change.\n\nThanks to Patrick Steinhardt for his careful review on the previous\nround!\n\nTaylor Blau (8):\n midx-write.c: tolerate `--preferred-pack` without bitmaps\n midx-write.c: reduce argument count for `get_sorted_entries()`\n midx-write.c: pass `start_pack` to `compute_sorted_entries()`\n midx-write.c: extract `should_include_pack()`\n midx-write.c: extract `fill_packs_from_midx()`\n midx-write.c: support reading an existing MIDX with `packs_to_include`\n midx: replace `get_midx_rev_filename()` with a generic helper\n pack-bitmap.c: reimplement `midx_bitmap_filename()` with helper\n\n midx-write.c | 161 ++++++++++++++++++------------------\n midx.c | 14 ++--\n midx.h | 6 +-\n pack-bitmap.c | 5 +-\n pack-revindex.c | 3 +-\n t/t5319-multi-pack-index.sh | 23 ++++++\n 6 files changed, 119 insertions(+), 93 deletions(-)\n\nRange-diff against v1:\n1: c753bc379b ! 1: ad268bd264 midx-write.c: tolerate `--preferred-pack` without bitmaps\n @@ Commit message\n unreferenced packs via 'git multi-pack-index expire').\n \n In practice, this isn't possible to trigger when running `git\n - multi-pack-index write` from via `git repack`, as the latter always\n - passes `--stdin-packs`, which prevents us from loading an existing MIDX,\n - as it forces all packs to be read from disk.\n + multi-pack-index write` from `git repack`, as the latter always passes\n + `--stdin-packs`, which prevents us from loading an existing MIDX, as it\n + forces all packs to be read from disk.\n \n But a future commit in this series will change that behavior to\n unconditionally load an existing MIDX, even with `--stdin-packs`, making\n2: 07dad5a581 ! 2: 9d422efe6e midx-write.c: reduce argument count for `get_sorted_entries()`\n @@ Commit message\n \n Simplify the declaration of this function by taking a single pointer to\n the whole `struct write_midx_context` instead of various members within\n - it.\n + it. Since this function is now computing the entire result (populating\n + both `ctx->entries`, and `ctx->entries_nr`), rename it to something that\n + doesn't start with \"get_\" to make clear that this function has a\n + side-effect.\n \n Signed-off-by: Taylor Blau \n \n @@ midx-write.c: static void midx_fanout_add_pack_fanout(struct midx_fanout *fanout\n -\t\t\t\t\t\t uint32_t nr_packs,\n -\t\t\t\t\t\t size_t *nr_objects,\n -\t\t\t\t\t\t int preferred_pack)\n -+static struct pack_midx_entry *get_sorted_entries(struct write_midx_context *ctx)\n ++static void compute_sorted_entries(struct write_midx_context *ctx)\n {\n \tuint32_t cur_fanout, cur_pack, cur_object;\n \tsize_t alloc_objects, total_objects = 0;\n \tstruct midx_fanout fanout = { 0 };\n - \tstruct pack_midx_entry *deduplicated_entries = NULL;\n +-\tstruct pack_midx_entry *deduplicated_entries = NULL;\n -\tuint32_t start_pack = m ? m->num_packs : 0;\n +\tuint32_t start_pack = ctx->m ? ctx->m->num_packs : 0;\n \n @@ midx-write.c: static void midx_fanout_add_pack_fanout(struct midx_fanout *fanout\n \t/*\n \t * As we de-duplicate by fanout value, we expect the fanout\n @@ midx-write.c: static struct pack_midx_entry *get_sorted_entries(struct multi_pack_index *m,\n + \talloc_objects = fanout.alloc = total_objects > 3200 ? total_objects / 200 : 16;\n \n \tALLOC_ARRAY(fanout.entries, fanout.alloc);\n - \tALLOC_ARRAY(deduplicated_entries, alloc_objects);\n +-\tALLOC_ARRAY(deduplicated_entries, alloc_objects);\n -\t*nr_objects = 0;\n ++\tALLOC_ARRAY(ctx->entries, alloc_objects);\n +\tctx->entries_nr = 0;\n \n \tfor (cur_fanout = 0; cur_fanout < 256; cur_fanout++) {\n @@ midx-write.c: static struct pack_midx_entry *get_sorted_entries(struct multi_pac\n \t\t\t\tcontinue;\n \n -\t\t\tALLOC_GROW(deduplicated_entries, st_add(*nr_objects, 1),\n -+\t\t\tALLOC_GROW(deduplicated_entries, st_add(ctx->entries_nr, 1),\n ++\t\t\tALLOC_GROW(ctx->entries, st_add(ctx->entries_nr, 1),\n \t\t\t\t alloc_objects);\n -\t\t\tmemcpy(&deduplicated_entries[*nr_objects],\n -+\t\t\tmemcpy(&deduplicated_entries[ctx->entries_nr],\n ++\t\t\tmemcpy(&ctx->entries[ctx->entries_nr],\n \t\t\t &fanout.entries[cur_object],\n \t\t\t sizeof(struct pack_midx_entry));\n -\t\t\t(*nr_objects)++;\n @@ midx-write.c: static struct pack_midx_entry *get_sorted_entries(struct multi_pac\n \t\t}\n \t}\n \n + \tfree(fanout.entries);\n +-\treturn deduplicated_entries;\n + }\n + \n + static int write_midx_pack_names(struct hashfile *f, void *data)\n @@ midx-write.c: static int write_midx_internal(const char *object_dir,\n \t\t}\n \t}\n \n -\tctx.entries = get_sorted_entries(ctx.m, ctx.info, ctx.nr, &ctx.entries_nr,\n -\t\t\t\t\t ctx.preferred_pack_idx);\n -+\tctx.entries = get_sorted_entries(&ctx);\n ++\tcompute_sorted_entries(&ctx);\n \n \tctx.large_offsets_needed = 0;\n \tfor (i = 0; i < ctx.entries_nr; i++) {\n3: 7acf4557dc ! 3: e81296f8cc midx-write.c: pass `start_pack` to `get_sorted_entries()`\n @@ Metadata\n Author: Taylor Blau \n \n ## Commit message ##\n - midx-write.c: pass `start_pack` to `get_sorted_entries()`\n + midx-write.c: pass `start_pack` to `compute_sorted_entries()`\n \n - The function `get_sorted_entries()` is broadly responsible for\n + The function `compute_sorted_entries()` is broadly responsible for\n building an array of the objects to be written into a MIDX based on the\n provided list of packs.\n \n @@ Commit message\n \n The existing implementation simply skips past the first\n ctx->m->num_packs (if ctx->m is non-NULL, indicating that we loaded an\n - existing MIDX). Future changes (outside the scope of this patch series)\n - to the MIDX code will require us to skip *at most* that number[^1].\n + existing MIDX). This is because we read objects in packs from an\n + existing MIDX via the MIDX itself, rather than from the pack-level\n + fanout to guarantee a de-duplicated result (see: a40498a1265 (midx: use\n + existing midx when writing new one, 2018-07-12)).\n +\n + Future changes (outside the scope of this patch series) to the MIDX code\n + will require us to skip *at most* that number[^1].\n \n We could tag each pack with a bit that indicates the pack's contents\n should be included in the MIDX. But we can just as easily determine the\n @@ midx-write.c: static void midx_fanout_add_pack_fanout(struct midx_fanout *fanout\n * Copy only the de-duplicated entries (selected by most-recent modified time\n * of a packfile containing the object).\n */\n --static struct pack_midx_entry *get_sorted_entries(struct write_midx_context *ctx)\n -+static struct pack_midx_entry *get_sorted_entries(struct write_midx_context *ctx,\n -+\t\t\t\t\t\t uint32_t start_pack)\n +-static void compute_sorted_entries(struct write_midx_context *ctx)\n ++static void compute_sorted_entries(struct write_midx_context *ctx,\n ++\t\t\t\t uint32_t start_pack)\n {\n \tuint32_t cur_fanout, cur_pack, cur_object;\n \tsize_t alloc_objects, total_objects = 0;\n \tstruct midx_fanout fanout = { 0 };\n - \tstruct pack_midx_entry *deduplicated_entries = NULL;\n -\tuint32_t start_pack = ctx->m ? ctx->m->num_packs : 0;\n \n \tfor (cur_pack = start_pack; cur_pack < ctx->nr; cur_pack++)\n @@ midx-write.c: static int write_midx_internal(const char *object_dir,\n \t\t}\n \t}\n \n --\tctx.entries = get_sorted_entries(&ctx);\n -+\tctx.entries = get_sorted_entries(&ctx, start_pack);\n +-\tcompute_sorted_entries(&ctx);\n ++\tcompute_sorted_entries(&ctx, start_pack);\n \n \tctx.large_offsets_needed = 0;\n \tfor (i = 0; i < ctx.entries_nr; i++) {\n4: 3908546ea8 ! 4: 9cd9257465 midx-write.c: extract `should_include_pack()`\n @@ Commit message\n - or, appear in the \"to_include\" list, if invoking the MIDX write\n machinery with the `--stdin-packs` command-line argument.\n \n - In a future commit will want to call a slight variant of these checks\n - from the code that reuses all packs from an existing MIDX, as well as\n - the current location via add_pack_to_midx(). The latter will be\n - modified in subsequent commits to only reuse packs which appear in the\n - to_include list, if one was given.\n + A future commit will want to call a slight variant of these checks from\n + the code that reuses all packs from an existing MIDX, as well as the\n + current location via add_pack_to_midx(). The latter will be modified in\n + subsequent commits to only reuse packs which appear in the to_include\n + list, if one was given.\n \n Prepare for that step by extracting these checks as a subroutine that\n may be called from both places.\n5: dc813ea1ca = 5: 1bb890e87c midx-write.c: extract `fill_packs_from_midx()`\n6: 61268114c6 ! 6: fe187b1939 midx-write.c: support reading an existing MIDX with `packs_to_include`\n @@ Commit message\n reject all of the packs it provided since they appear in an existing\n MIDX by definition.\n \n + The sum total of this change is that we are now able to read and\n + reference objects in an existing MIDX even when given a non-NULL\n + `packs_to_include`. This is a prerequisite step for incremental MIDXs,\n + which need to load any existing MIDX (if one is present) in order to\n + determine whether or not an object already appears in an earlier portion\n + of the MIDX to avoid duplicating it across multiple portions.\n +\n Signed-off-by: Taylor Blau \n \n ## midx-write.c ##\n7: f4c0167f43 = 7: f4f977c1c7 midx: replace `get_midx_rev_filename()` with a generic helper\n8: 79e3f7f83f = 8: bcadaf9278 pack-bitmap.c: reimplement `midx_bitmap_filename()` with helper\n\nbase-commit: 3a57aa566a21e7a510c64881bc6bdff7eb397988\n-- \n2.45.1.321.gbcadaf92783\n"},{"id":"495873","messageId":"ad268bd264e34f02f9c32e31db696c7acc1efc2b.1717023301.git.me@ttaylorr.com","threadId":"61537","inReplyTo":"cover.1717023301.git.me@ttaylorr.com","subject":"[PATCH v2 1/8] midx-write.c: tolerate `--preferred-pack` without bitmaps","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-05-29T22:55:19Z","receivedAt":"2024-05-29T22:55:22Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"When passing a preferred pack to the MIDX write machinery, we ensure\nthat the given preferred pack is non-empty since 5d3cd09a808 (midx:\nreject empty `--preferred-pack`'s, 2021-08-31).\n\nHowever packs are only loaded (via `write_midx_internal()`, though a\nsubsequent patch will refactor this code out to its own function) when\nthe `MIDX_WRITE_REV_INDEX` flag is set.\n\nSo if a caller runs:\n\n $ git multi-pack-index write --preferred-pack=...\n\nwith both (a) an existing MIDX, and (b) specifies a pack from that MIDX\nas the preferred one, without passing `--bitmap`, then the check added\nin 5d3cd09a808 will result in a segfault.\n\nNote that packs loaded from disk which don't appear in an existing MIDX\ndo not trigger this issue, as those packs are loaded unconditionally. We\nconditionally load packs from a MIDX since we tolerate MIDXs whose\npacks do not resolve (i.e., via the MIDX write after removing\nunreferenced packs via 'git multi-pack-index expire').\n\nIn practice, this isn't possible to trigger when running `git\nmulti-pack-index write` from `git repack`, as the latter always passes\n`--stdin-packs`, which prevents us from loading an existing MIDX, as it\nforces all packs to be read from disk.\n\nBut a future commit in this series will change that behavior to\nunconditionally load an existing MIDX, even with `--stdin-packs`, making\nthis behavior trigger-able from 'repack' much more easily.\n\nPrevent this from being an issue by removing the segfault altogether by\ncalling `prepare_midx_pack()` on packs loaded from an existing MIDX when\neither the `MIDX_WRITE_REV_INDEX` flag is set *or* we specified a\n`--preferred-pack`.\n\nSigned-off-by: Taylor Blau \n---\n midx-write.c | 8 +++++++-\n t/t5319-multi-pack-index.sh | 23 +++++++++++++++++++++++\n 2 files changed, 30 insertions(+), 1 deletion(-)\n\ndiff --git a/midx-write.c b/midx-write.c\nindex 2fa120c213..591b79bcbf 100644\n--- a/midx-write.c\n+++ b/midx-write.c\n@@ -935,11 +935,17 @@ static int write_midx_internal(const char *object_dir,\n \t\tfor (i = 0; i < ctx.m->num_packs; i++) {\n \t\t\tALLOC_GROW(ctx.info, ctx.nr + 1, ctx.alloc);\n \n-\t\t\tif (flags & MIDX_WRITE_REV_INDEX) {\n+\t\t\tif (flags & MIDX_WRITE_REV_INDEX ||\n+\t\t\t preferred_pack_name) {\n \t\t\t\t/*\n \t\t\t\t * If generating a reverse index, need to have\n \t\t\t\t * packed_git's loaded to compare their\n \t\t\t\t * mtimes and object count.\n+\t\t\t\t *\n+\t\t\t\t * If a preferred pack is specified,\n+\t\t\t\t * need to have packed_git's loaded to\n+\t\t\t\t * ensure the chosen preferred pack has\n+\t\t\t\t * a non-zero object count.\n \t\t\t\t */\n \t\t\t\tif (prepare_midx_pack(the_repository, ctx.m, i)) {\n \t\t\t\t\terror(_(\"could not load pack\"));\ndiff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\nindex dd09134db0..10d2a6bf92 100755\n--- a/t/t5319-multi-pack-index.sh\n+++ b/t/t5319-multi-pack-index.sh\n@@ -350,6 +350,29 @@ test_expect_success 'preferred packs must be non-empty' '\n \t)\n '\n \n+test_expect_success 'preferred pack from existing MIDX without bitmaps' '\n+\tgit init preferred-without-bitmaps &&\n+\t(\n+\t\tcd preferred-without-bitmaps &&\n+\n+\t\ttest_commit one &&\n+\t\tpack=\"$(git pack-objects --all $objdir/pack/pack entries`, and `ctx->entries_nr`), rename it to something that\ndoesn't start with \"get_\" to make clear that this function has a\nside-effect.\n\nSigned-off-by: Taylor Blau \n---\n midx-write.c | 45 +++++++++++++++++++--------------------------\n 1 file changed, 19 insertions(+), 26 deletions(-)\n\ndiff --git a/midx-write.c b/midx-write.c\nindex 591b79bcbf..15965ceb70 100644\n--- a/midx-write.c\n+++ b/midx-write.c\n@@ -299,21 +299,16 @@ static void midx_fanout_add_pack_fanout(struct midx_fanout *fanout,\n * Copy only the de-duplicated entries (selected by most-recent modified time\n * of a packfile containing the object).\n */\n-static struct pack_midx_entry *get_sorted_entries(struct multi_pack_index *m,\n-\t\t\t\t\t\t struct pack_info *info,\n-\t\t\t\t\t\t uint32_t nr_packs,\n-\t\t\t\t\t\t size_t *nr_objects,\n-\t\t\t\t\t\t int preferred_pack)\n+static void compute_sorted_entries(struct write_midx_context *ctx)\n {\n \tuint32_t cur_fanout, cur_pack, cur_object;\n \tsize_t alloc_objects, total_objects = 0;\n \tstruct midx_fanout fanout = { 0 };\n-\tstruct pack_midx_entry *deduplicated_entries = NULL;\n-\tuint32_t start_pack = m ? m->num_packs : 0;\n+\tuint32_t start_pack = ctx->m ? ctx->m->num_packs : 0;\n \n-\tfor (cur_pack = start_pack; cur_pack < nr_packs; cur_pack++)\n+\tfor (cur_pack = start_pack; cur_pack < ctx->nr; cur_pack++)\n \t\ttotal_objects = st_add(total_objects,\n-\t\t\t\t info[cur_pack].p->num_objects);\n+\t\t\t\t ctx->info[cur_pack].p->num_objects);\n \n \t/*\n \t * As we de-duplicate by fanout value, we expect the fanout\n@@ -323,26 +318,26 @@ static struct pack_midx_entry *get_sorted_entries(struct multi_pack_index *m,\n \talloc_objects = fanout.alloc = total_objects > 3200 ? total_objects / 200 : 16;\n \n \tALLOC_ARRAY(fanout.entries, fanout.alloc);\n-\tALLOC_ARRAY(deduplicated_entries, alloc_objects);\n-\t*nr_objects = 0;\n+\tALLOC_ARRAY(ctx->entries, alloc_objects);\n+\tctx->entries_nr = 0;\n \n \tfor (cur_fanout = 0; cur_fanout < 256; cur_fanout++) {\n \t\tfanout.nr = 0;\n \n-\t\tif (m)\n-\t\t\tmidx_fanout_add_midx_fanout(&fanout, m, cur_fanout,\n-\t\t\t\t\t\t preferred_pack);\n+\t\tif (ctx->m)\n+\t\t\tmidx_fanout_add_midx_fanout(&fanout, ctx->m, cur_fanout,\n+\t\t\t\t\t\t ctx->preferred_pack_idx);\n \n-\t\tfor (cur_pack = start_pack; cur_pack < nr_packs; cur_pack++) {\n-\t\t\tint preferred = cur_pack == preferred_pack;\n+\t\tfor (cur_pack = start_pack; cur_pack < ctx->nr; cur_pack++) {\n+\t\t\tint preferred = cur_pack == ctx->preferred_pack_idx;\n \t\t\tmidx_fanout_add_pack_fanout(&fanout,\n-\t\t\t\t\t\t info, cur_pack,\n+\t\t\t\t\t\t ctx->info, cur_pack,\n \t\t\t\t\t\t preferred, cur_fanout);\n \t\t}\n \n-\t\tif (-1 < preferred_pack && preferred_pack < start_pack)\n-\t\t\tmidx_fanout_add_pack_fanout(&fanout, info,\n-\t\t\t\t\t\t preferred_pack, 1,\n+\t\tif (-1 < ctx->preferred_pack_idx && ctx->preferred_pack_idx < start_pack)\n+\t\t\tmidx_fanout_add_pack_fanout(&fanout, ctx->info,\n+\t\t\t\t\t\t ctx->preferred_pack_idx, 1,\n \t\t\t\t\t\t cur_fanout);\n \n \t\tmidx_fanout_sort(&fanout);\n@@ -356,17 +351,16 @@ static struct pack_midx_entry *get_sorted_entries(struct multi_pack_index *m,\n \t\t\t\t\t\t&fanout.entries[cur_object].oid))\n \t\t\t\tcontinue;\n \n-\t\t\tALLOC_GROW(deduplicated_entries, st_add(*nr_objects, 1),\n+\t\t\tALLOC_GROW(ctx->entries, st_add(ctx->entries_nr, 1),\n \t\t\t\t alloc_objects);\n-\t\t\tmemcpy(&deduplicated_entries[*nr_objects],\n+\t\t\tmemcpy(&ctx->entries[ctx->entries_nr],\n \t\t\t &fanout.entries[cur_object],\n \t\t\t sizeof(struct pack_midx_entry));\n-\t\t\t(*nr_objects)++;\n+\t\t\tctx->entries_nr++;\n \t\t}\n \t}\n \n \tfree(fanout.entries);\n-\treturn deduplicated_entries;\n }\n \n static int write_midx_pack_names(struct hashfile *f, void *data)\n@@ -1060,8 +1054,7 @@ static int write_midx_internal(const char *object_dir,\n \t\t}\n \t}\n \n-\tctx.entries = get_sorted_entries(ctx.m, ctx.info, ctx.nr, &ctx.entries_nr,\n-\t\t\t\t\t ctx.preferred_pack_idx);\n+\tcompute_sorted_entries(&ctx);\n \n \tctx.large_offsets_needed = 0;\n \tfor (i = 0; i < ctx.entries_nr; i++) {\n-- \n2.45.1.321.gbcadaf92783\n\n"},{"id":"495875","messageId":"e81296f8ccf8b47758d0ec4fbb04f0f2b356b490.1717023301.git.me@ttaylorr.com","threadId":"61537","inReplyTo":"cover.1717023301.git.me@ttaylorr.com","subject":"[PATCH v2 3/8] midx-write.c: pass `start_pack` to `compute_sorted_entries()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-05-29T22:55:28Z","receivedAt":"2024-05-29T22:55:31Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"The function `compute_sorted_entries()` is broadly responsible for\nbuilding an array of the objects to be written into a MIDX based on the\nprovided list of packs.\n\nIf we have loaded an existing MIDX, however, we may not use all of its\npacks, despite loading them into the ctx->info array.\n\nThe existing implementation simply skips past the first\nctx->m->num_packs (if ctx->m is non-NULL, indicating that we loaded an\nexisting MIDX). This is because we read objects in packs from an\nexisting MIDX via the MIDX itself, rather than from the pack-level\nfanout to guarantee a de-duplicated result (see: a40498a1265 (midx: use\nexisting midx when writing new one, 2018-07-12)).\n\nFuture changes (outside the scope of this patch series) to the MIDX code\nwill require us to skip *at most* that number[^1].\n\nWe could tag each pack with a bit that indicates the pack's contents\nshould be included in the MIDX. But we can just as easily determine the\nnumber of packs to skip by passing in the number of packs we learned\nabout after processing an existing MIDX.\n\n[^1]: Kind of. The real number will be bounded by the number of packs in\n a MIDX layer, and the number of packs in its base layer(s), but that\n concept hasn't been fully defined yet.\n\nSigned-off-by: Taylor Blau \n---\n midx-write.c | 10 ++++++----\n 1 file changed, 6 insertions(+), 4 deletions(-)\n\ndiff --git a/midx-write.c b/midx-write.c\nindex 15965ceb70..949a66e973 100644\n--- a/midx-write.c\n+++ b/midx-write.c\n@@ -299,12 +299,12 @@ static void midx_fanout_add_pack_fanout(struct midx_fanout *fanout,\n * Copy only the de-duplicated entries (selected by most-recent modified time\n * of a packfile containing the object).\n */\n-static void compute_sorted_entries(struct write_midx_context *ctx)\n+static void compute_sorted_entries(struct write_midx_context *ctx,\n+\t\t\t\t uint32_t start_pack)\n {\n \tuint32_t cur_fanout, cur_pack, cur_object;\n \tsize_t alloc_objects, total_objects = 0;\n \tstruct midx_fanout fanout = { 0 };\n-\tuint32_t start_pack = ctx->m ? ctx->m->num_packs : 0;\n \n \tfor (cur_pack = start_pack; cur_pack < ctx->nr; cur_pack++)\n \t\ttotal_objects = st_add(total_objects,\n@@ -889,7 +889,7 @@ static int write_midx_internal(const char *object_dir,\n {\n \tstruct strbuf midx_name = STRBUF_INIT;\n \tunsigned char midx_hash[GIT_MAX_RAWSZ];\n-\tuint32_t i;\n+\tuint32_t i, start_pack;\n \tstruct hashfile *f = NULL;\n \tstruct lock_file lk;\n \tstruct write_midx_context ctx = { 0 };\n@@ -957,6 +957,8 @@ static int write_midx_internal(const char *object_dir,\n \t\t}\n \t}\n \n+\tstart_pack = ctx.nr;\n+\n \tctx.pack_paths_checked = 0;\n \tif (flags & MIDX_PROGRESS)\n \t\tctx.progress = start_delayed_progress(_(\"Adding packfiles to multi-pack-index\"), 0);\n@@ -1054,7 +1056,7 @@ static int write_midx_internal(const char *object_dir,\n \t\t}\n \t}\n \n-\tcompute_sorted_entries(&ctx);\n+\tcompute_sorted_entries(&ctx, start_pack);\n \n \tctx.large_offsets_needed = 0;\n \tfor (i = 0; i < ctx.entries_nr; i++) {\n-- \n2.45.1.321.gbcadaf92783\n\n"},{"id":"495876","messageId":"9cd92574659d49b5365b97cc2352b4cc8c09959d.1717023301.git.me@ttaylorr.com","threadId":"61537","inReplyTo":"cover.1717023301.git.me@ttaylorr.com","subject":"[PATCH v2 4/8] midx-write.c: extract `should_include_pack()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-05-29T22:55:32Z","receivedAt":"2024-05-29T22:55:35Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"The add_pack_to_midx() callback used via for_each_file_in_pack_dir() is\nused to add packs with .idx files to the MIDX being written.\n\nWithin this function, we have a pair of checks that discards packs\nwhich:\n\n - appear in an existing MIDX, if we successfully read an existing MIDX\n from disk\n\n - or, appear in the \"to_include\" list, if invoking the MIDX write\n machinery with the `--stdin-packs` command-line argument.\n\nA future commit will want to call a slight variant of these checks from\nthe code that reuses all packs from an existing MIDX, as well as the\ncurrent location via add_pack_to_midx(). The latter will be modified in\nsubsequent commits to only reuse packs which appear in the to_include\nlist, if one was given.\n\nPrepare for that step by extracting these checks as a subroutine that\nmay be called from both places.\n\nSigned-off-by: Taylor Blau \n---\n midx-write.c | 48 ++++++++++++++++++++++++++++--------------------\n 1 file changed, 28 insertions(+), 20 deletions(-)\n\ndiff --git a/midx-write.c b/midx-write.c\nindex 949a66e973..a80709726a 100644\n--- a/midx-write.c\n+++ b/midx-write.c\n@@ -100,6 +100,32 @@ struct write_midx_context {\n \tstruct string_list *to_include;\n };\n \n+static int should_include_pack(const struct write_midx_context *ctx,\n+\t\t\t const char *file_name)\n+{\n+\t/*\n+\t * Note that at most one of ctx->m and ctx->to_include are set,\n+\t * so we are testing midx_contains_pack() and\n+\t * string_list_has_string() independently (guarded by the\n+\t * appropriate NULL checks).\n+\t *\n+\t * We could support passing to_include while reusing an existing\n+\t * MIDX, but don't currently since the reuse process drags\n+\t * forward all packs from an existing MIDX (without checking\n+\t * whether or not they appear in the to_include list).\n+\t *\n+\t * If we added support for that, these next two conditional\n+\t * should be performed independently (likely checking\n+\t * to_include before the existing MIDX).\n+\t */\n+\tif (ctx->m && midx_contains_pack(ctx->m, file_name))\n+\t\treturn 0;\n+\telse if (ctx->to_include &&\n+\t\t !string_list_has_string(ctx->to_include, file_name))\n+\t\treturn 0;\n+\treturn 1;\n+}\n+\n static void add_pack_to_midx(const char *full_path, size_t full_path_len,\n \t\t\t const char *file_name, void *data)\n {\n@@ -108,29 +134,11 @@ static void add_pack_to_midx(const char *full_path, size_t full_path_len,\n \n \tif (ends_with(file_name, \".idx\")) {\n \t\tdisplay_progress(ctx->progress, ++ctx->pack_paths_checked);\n-\t\t/*\n-\t\t * Note that at most one of ctx->m and ctx->to_include are set,\n-\t\t * so we are testing midx_contains_pack() and\n-\t\t * string_list_has_string() independently (guarded by the\n-\t\t * appropriate NULL checks).\n-\t\t *\n-\t\t * We could support passing to_include while reusing an existing\n-\t\t * MIDX, but don't currently since the reuse process drags\n-\t\t * forward all packs from an existing MIDX (without checking\n-\t\t * whether or not they appear in the to_include list).\n-\t\t *\n-\t\t * If we added support for that, these next two conditional\n-\t\t * should be performed independently (likely checking\n-\t\t * to_include before the existing MIDX).\n-\t\t */\n-\t\tif (ctx->m && midx_contains_pack(ctx->m, file_name))\n-\t\t\treturn;\n-\t\telse if (ctx->to_include &&\n-\t\t\t !string_list_has_string(ctx->to_include, file_name))\n+\n+\t\tif (!should_include_pack(ctx, file_name))\n \t\t\treturn;\n \n \t\tALLOC_GROW(ctx->info, ctx->nr + 1, ctx->alloc);\n-\n \t\tp = add_packed_git(full_path, full_path_len, 0);\n \t\tif (!p) {\n \t\t\twarning(_(\"failed to add packfile '%s'\"),\n-- \n2.45.1.321.gbcadaf92783\n\n"},{"id":"495877","messageId":"1bb890e87cf6db9e464d7753e781a9e5d70e0b12.1717023301.git.me@ttaylorr.com","threadId":"61537","inReplyTo":"cover.1717023301.git.me@ttaylorr.com","subject":"[PATCH v2 5/8] midx-write.c: extract `fill_packs_from_midx()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-05-29T22:55:36Z","receivedAt":"2024-05-29T22:55:38Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"When write_midx_internal() loads an existing MIDX, all packs are copied\nforward into the new MIDX. Improve the readability of\nwrite_midx_internal() by extracting this functionality out into a\nseparate function.\n\nSigned-off-by: Taylor Blau \n---\n midx-write.c | 68 +++++++++++++++++++++++++++++-----------------------\n 1 file changed, 38 insertions(+), 30 deletions(-)\n\ndiff --git a/midx-write.c b/midx-write.c\nindex a80709726a..c93e1ae305 100644\n--- a/midx-write.c\n+++ b/midx-write.c\n@@ -888,6 +888,40 @@ static struct multi_pack_index *lookup_multi_pack_index(struct repository *r,\n \treturn result;\n }\n \n+static int fill_packs_from_midx(struct write_midx_context *ctx,\n+\t\t\t\tconst char *preferred_pack_name, uint32_t flags)\n+{\n+\tuint32_t i;\n+\n+\tfor (i = 0; i < ctx->m->num_packs; i++) {\n+\t\tALLOC_GROW(ctx->info, ctx->nr + 1, ctx->alloc);\n+\n+\t\tif (flags & MIDX_WRITE_REV_INDEX || preferred_pack_name) {\n+\t\t\t/*\n+\t\t\t * If generating a reverse index, need to have\n+\t\t\t * packed_git's loaded to compare their\n+\t\t\t * mtimes and object count.\n+\t\t\t *\n+\t\t\t *\n+\t\t\t * If a preferred pack is specified, need to\n+\t\t\t * have packed_git's loaded to ensure the chosen\n+\t\t\t * preferred pack has a non-zero object count.\n+\t\t\t */\n+\t\t\tif (prepare_midx_pack(the_repository, ctx->m, i))\n+\t\t\t\treturn error(_(\"could not load pack\"));\n+\n+\t\t\tif (open_pack_index(ctx->m->packs[i]))\n+\t\t\t\tdie(_(\"could not open index for %s\"),\n+\t\t\t\t ctx->m->packs[i]->pack_name);\n+\t\t}\n+\n+\t\tfill_pack_info(&ctx->info[ctx->nr++], ctx->m->packs[i],\n+\t\t\t ctx->m->pack_names[i], i);\n+\t}\n+\n+\treturn 0;\n+}\n+\n static int write_midx_internal(const char *object_dir,\n \t\t\t struct string_list *packs_to_include,\n \t\t\t struct string_list *packs_to_drop,\n@@ -933,36 +967,10 @@ static int write_midx_internal(const char *object_dir,\n \tctx.info = NULL;\n \tALLOC_ARRAY(ctx.info, ctx.alloc);\n \n-\tif (ctx.m) {\n-\t\tfor (i = 0; i < ctx.m->num_packs; i++) {\n-\t\t\tALLOC_GROW(ctx.info, ctx.nr + 1, ctx.alloc);\n-\n-\t\t\tif (flags & MIDX_WRITE_REV_INDEX ||\n-\t\t\t preferred_pack_name) {\n-\t\t\t\t/*\n-\t\t\t\t * If generating a reverse index, need to have\n-\t\t\t\t * packed_git's loaded to compare their\n-\t\t\t\t * mtimes and object count.\n-\t\t\t\t *\n-\t\t\t\t * If a preferred pack is specified,\n-\t\t\t\t * need to have packed_git's loaded to\n-\t\t\t\t * ensure the chosen preferred pack has\n-\t\t\t\t * a non-zero object count.\n-\t\t\t\t */\n-\t\t\t\tif (prepare_midx_pack(the_repository, ctx.m, i)) {\n-\t\t\t\t\terror(_(\"could not load pack\"));\n-\t\t\t\t\tresult = 1;\n-\t\t\t\t\tgoto cleanup;\n-\t\t\t\t}\n-\n-\t\t\t\tif (open_pack_index(ctx.m->packs[i]))\n-\t\t\t\t\tdie(_(\"could not open index for %s\"),\n-\t\t\t\t\t ctx.m->packs[i]->pack_name);\n-\t\t\t}\n-\n-\t\t\tfill_pack_info(&ctx.info[ctx.nr++], ctx.m->packs[i],\n-\t\t\t\t ctx.m->pack_names[i], i);\n-\t\t}\n+\tif (ctx.m && fill_packs_from_midx(&ctx, preferred_pack_name,\n+\t\t\t\t\t flags) < 0) {\n+\t\tresult = 1;\n+\t\tgoto cleanup;\n \t}\n \n \tstart_pack = ctx.nr;\n-- \n2.45.1.321.gbcadaf92783\n\n"},{"id":"495878","messageId":"fe187b1939d9c74782306cd3e4fcd2f25451ad69.1717023301.git.me@ttaylorr.com","threadId":"61537","inReplyTo":"cover.1717023301.git.me@ttaylorr.com","subject":"[PATCH v2 6/8] midx-write.c: support reading an existing MIDX with `packs_to_include`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-05-29T22:55:39Z","receivedAt":"2024-05-29T22:55:42Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Avoid unconditionally copying all packs from an existing MIDX into a new\nMIDX by checking that packs added via `fill_packs_from_midx()` don't\nappear in the `to_include` set, if one was provided.\n\nDo so by calling `should_include_pack()` from both `add_pack_to_midx()`\nand `fill_packs_from_midx()`.\n\nIn order to make this work, teach `should_include_pack()` a new\n\"exclude_from_midx\" parameter, which allows skipping the first check.\nThis is done so that the caller in `fill_packs_from_midx()` doesn't\nreject all of the packs it provided since they appear in an existing\nMIDX by definition.\n\nThe sum total of this change is that we are now able to read and\nreference objects in an existing MIDX even when given a non-NULL\n`packs_to_include`. This is a prerequisite step for incremental MIDXs,\nwhich need to load any existing MIDX (if one is present) in order to\ndetermine whether or not an object already appears in an earlier portion\nof the MIDX to avoid duplicating it across multiple portions.\n\nSigned-off-by: Taylor Blau \n---\n midx-write.c | 42 +++++++++++-------------------------------\n 1 file changed, 11 insertions(+), 31 deletions(-)\n\ndiff --git a/midx-write.c b/midx-write.c\nindex c93e1ae305..8b96c566e7 100644\n--- a/midx-write.c\n+++ b/midx-write.c\n@@ -101,27 +101,13 @@ struct write_midx_context {\n };\n \n static int should_include_pack(const struct write_midx_context *ctx,\n-\t\t\t const char *file_name)\n+\t\t\t const char *file_name,\n+\t\t\t int exclude_from_midx)\n {\n-\t/*\n-\t * Note that at most one of ctx->m and ctx->to_include are set,\n-\t * so we are testing midx_contains_pack() and\n-\t * string_list_has_string() independently (guarded by the\n-\t * appropriate NULL checks).\n-\t *\n-\t * We could support passing to_include while reusing an existing\n-\t * MIDX, but don't currently since the reuse process drags\n-\t * forward all packs from an existing MIDX (without checking\n-\t * whether or not they appear in the to_include list).\n-\t *\n-\t * If we added support for that, these next two conditional\n-\t * should be performed independently (likely checking\n-\t * to_include before the existing MIDX).\n-\t */\n-\tif (ctx->m && midx_contains_pack(ctx->m, file_name))\n+\tif (exclude_from_midx && ctx->m && midx_contains_pack(ctx->m, file_name))\n \t\treturn 0;\n-\telse if (ctx->to_include &&\n-\t\t !string_list_has_string(ctx->to_include, file_name))\n+\tif (ctx->to_include && !string_list_has_string(ctx->to_include,\n+\t\t\t\t\t\t file_name))\n \t\treturn 0;\n \treturn 1;\n }\n@@ -135,7 +121,7 @@ static void add_pack_to_midx(const char *full_path, size_t full_path_len,\n \tif (ends_with(file_name, \".idx\")) {\n \t\tdisplay_progress(ctx->progress, ++ctx->pack_paths_checked);\n \n-\t\tif (!should_include_pack(ctx, file_name))\n+\t\tif (!should_include_pack(ctx, file_name, 1))\n \t\t\treturn;\n \n \t\tALLOC_GROW(ctx->info, ctx->nr + 1, ctx->alloc);\n@@ -894,6 +880,9 @@ static int fill_packs_from_midx(struct write_midx_context *ctx,\n \tuint32_t i;\n \n \tfor (i = 0; i < ctx->m->num_packs; i++) {\n+\t\tif (!should_include_pack(ctx, ctx->m->pack_names[i], 0))\n+\t\t\tcontinue;\n+\n \t\tALLOC_GROW(ctx->info, ctx->nr + 1, ctx->alloc);\n \n \t\tif (flags & MIDX_WRITE_REV_INDEX || preferred_pack_name) {\n@@ -948,15 +937,7 @@ static int write_midx_internal(const char *object_dir,\n \t\tdie_errno(_(\"unable to create leading directories of %s\"),\n \t\t\t midx_name.buf);\n \n-\tif (!packs_to_include) {\n-\t\t/*\n-\t\t * Only reference an existing MIDX when not filtering which\n-\t\t * packs to include, since all packs and objects are copied\n-\t\t * blindly from an existing MIDX if one is present.\n-\t\t */\n-\t\tctx.m = lookup_multi_pack_index(the_repository, object_dir);\n-\t}\n-\n+\tctx.m = lookup_multi_pack_index(the_repository, object_dir);\n \tif (ctx.m && !midx_checksum_valid(ctx.m)) {\n \t\twarning(_(\"ignoring existing multi-pack-index; checksum mismatch\"));\n \t\tctx.m = NULL;\n@@ -965,6 +946,7 @@ static int write_midx_internal(const char *object_dir,\n \tctx.nr = 0;\n \tctx.alloc = ctx.m ? ctx.m->num_packs : 16;\n \tctx.info = NULL;\n+\tctx.to_include = packs_to_include;\n \tALLOC_ARRAY(ctx.info, ctx.alloc);\n \n \tif (ctx.m && fill_packs_from_midx(&ctx, preferred_pack_name,\n@@ -981,8 +963,6 @@ static int write_midx_internal(const char *object_dir,\n \telse\n \t\tctx.progress = NULL;\n \n-\tctx.to_include = packs_to_include;\n-\n \tfor_each_file_in_pack_dir(object_dir, add_pack_to_midx, &ctx);\n \tstop_progress(&ctx.progress);\n \n-- \n2.45.1.321.gbcadaf92783\n\n"},{"id":"495879","messageId":"f4f977c1c71ceed339361e8463bace9edc42ec45.1717023301.git.me@ttaylorr.com","threadId":"61537","inReplyTo":"cover.1717023301.git.me@ttaylorr.com","subject":"[PATCH v2 7/8] midx: replace `get_midx_rev_filename()` with a generic helper","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-05-29T22:55:42Z","receivedAt":"2024-05-29T22:55:45Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Commit f894081deae (pack-revindex: read multi-pack reverse indexes,\n2021-03-30) introduced the `get_midx_rev_filename()` helper (later\nmodified by commit 60980aed786 (midx.c: write MIDX filenames to\nstrbuf, 2021-10-26)).\n\nThis function returns the location of the classic \".rev\" files we used\nto write for MIDXs (prior to 95e8383bac1 (midx.c: make changing the\npreferred pack safe, 2022-01-25)), which is always of the form:\n\n $GIT_DIR/objects/pack/multi-pack-index-$HASH.rev\n\nReplace this function with a generic helper that populates a strbuf with\nthe above form, replacing the \".rev\" extension with a caller-provided\nargument.\n\nThis will allow us to remove a similarly-defined function in the\npack-bitmap code (used to determine the location of a MIDX .bitmap file)\nby reimplementing it in terms of `get_midx_filename_ext()`.\n\nSigned-off-by: Taylor Blau \n---\n midx.c | 14 ++++++++------\n midx.h | 6 +++++-\n pack-revindex.c | 3 ++-\n 3 files changed, 15 insertions(+), 8 deletions(-)\n\ndiff --git a/midx.c b/midx.c\nindex 6f07de3688..bc4797196f 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -24,14 +24,16 @@ const unsigned char *get_midx_checksum(struct multi_pack_index *m)\n }\n \n void get_midx_filename(struct strbuf *out, const char *object_dir)\n+{\n+\tget_midx_filename_ext(out, object_dir, NULL, NULL);\n+}\n+\n+void get_midx_filename_ext(struct strbuf *out, const char *object_dir,\n+\t\t\t const unsigned char *hash, const char *ext)\n {\n \tstrbuf_addf(out, \"%s/pack/multi-pack-index\", object_dir);\n-}\n-\n-void get_midx_rev_filename(struct strbuf *out, struct multi_pack_index *m)\n-{\n-\tget_midx_filename(out, m->object_dir);\n-\tstrbuf_addf(out, \"-%s.rev\", hash_to_hex(get_midx_checksum(m)));\n+\tif (ext)\n+\t\tstrbuf_addf(out, \"-%s.%s\", hash_to_hex(hash), ext);\n }\n \n static int midx_read_oid_fanout(const unsigned char *chunk_start,\ndiff --git a/midx.h b/midx.h\nindex dc477dff44..8554f2d616 100644\n--- a/midx.h\n+++ b/midx.h\n@@ -74,9 +74,13 @@ struct multi_pack_index {\n #define MIDX_WRITE_BITMAP_HASH_CACHE (1 << 3)\n #define MIDX_WRITE_BITMAP_LOOKUP_TABLE (1 << 4)\n \n+#define MIDX_EXT_REV \"rev\"\n+#define MIDX_EXT_BITMAP \"bitmap\"\n+\n const unsigned char *get_midx_checksum(struct multi_pack_index *m);\n void get_midx_filename(struct strbuf *out, const char *object_dir);\n-void get_midx_rev_filename(struct strbuf *out, struct multi_pack_index *m);\n+void get_midx_filename_ext(struct strbuf *out, const char *object_dir,\n+\t\t\t const unsigned char *hash, const char *ext);\n \n struct multi_pack_index *load_multi_pack_index(const char *object_dir, int local);\n int prepare_midx_pack(struct repository *r, struct multi_pack_index *m, uint32_t pack_int_id);\ndiff --git a/pack-revindex.c b/pack-revindex.c\nindex a7624d8be8..fc63aa76a2 100644\n--- a/pack-revindex.c\n+++ b/pack-revindex.c\n@@ -381,7 +381,8 @@ int load_midx_revindex(struct multi_pack_index *m)\n \ttrace2_data_string(\"load_midx_revindex\", the_repository,\n \t\t\t \"source\", \"rev\");\n \n-\tget_midx_rev_filename(&revindex_name, m);\n+\tget_midx_filename_ext(&revindex_name, m->object_dir,\n+\t\t\t get_midx_checksum(m), MIDX_EXT_REV);\n \n \tret = load_revindex_from_disk(revindex_name.buf,\n \t\t\t\t m->num_objects,\n-- \n2.45.1.321.gbcadaf92783\n\n"},{"id":"495880","messageId":"bcadaf92783f2e6182e6b6f34dd2c90965bf8752.1717023301.git.me@ttaylorr.com","threadId":"61537","inReplyTo":"cover.1717023301.git.me@ttaylorr.com","subject":"[PATCH v2 8/8] pack-bitmap.c: reimplement `midx_bitmap_filename()` with helper","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-05-29T22:55:45Z","receivedAt":"2024-05-29T22:55:48Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Now that we have the `get_midx_filename_ext()` helper, we can\nreimplement the `midx_bitmap_filename()` function in terms of it.\n\nSigned-off-by: Taylor Blau \n---\n pack-bitmap.c | 5 ++---\n 1 file changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex 35c5ef9d3c..fe8e8a51d3 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -309,9 +309,8 @@ static int load_bitmap_entries_v1(struct bitmap_index *index)\n char *midx_bitmap_filename(struct multi_pack_index *midx)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n-\n-\tget_midx_filename(&buf, midx->object_dir);\n-\tstrbuf_addf(&buf, \"-%s.bitmap\", hash_to_hex(get_midx_checksum(midx)));\n+\tget_midx_filename_ext(&buf, midx->object_dir, get_midx_checksum(midx),\n+\t\t\t MIDX_EXT_BITMAP);\n \n \treturn strbuf_detach(&buf, NULL);\n }\n-- \n2.45.1.321.gbcadaf92783\n"},{"id":"495892","messageId":"20240530065943.GA1949834@coredump.intra.peff.net","threadId":"61537","inReplyTo":"e81296f8ccf8b47758d0ec4fbb04f0f2b356b490.1717023301.git.me@ttaylorr.com","subject":"Re: [PATCH v2 3/8] midx-write.c: pass `start_pack` to `compute_sorted_entries()`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-05-30T06:59:43Z","receivedAt":"2024-05-30T06:59:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 29, 2024 at 06:55:28PM -0400, Taylor Blau wrote:\n\n> The function `compute_sorted_entries()` is broadly responsible for\n> building an array of the objects to be written into a MIDX based on the\n> provided list of packs.\n> \n> If we have loaded an existing MIDX, however, we may not use all of its\n> packs, despite loading them into the ctx->info array.\n> \n> The existing implementation simply skips past the first\n> ctx->m->num_packs (if ctx->m is non-NULL, indicating that we loaded an\n> existing MIDX). This is because we read objects in packs from an\n> existing MIDX via the MIDX itself, rather than from the pack-level\n> fanout to guarantee a de-duplicated result (see: a40498a1265 (midx: use\n> existing midx when writing new one, 2018-07-12)).\n\nHmm, if the last patch had not removed the separate array/nr pair for\npacks, then you could just pass (array + start_pack, nr - start_pack). :)\n\nBut I think it is probably reasonable to keep the notion of \"here are\nall the packs\" and \"you are skipping some of them\" clear through the\ncall stack anyway. Especially if the notion of start_pack gets more\ncomplicated:\n\n> Future changes (outside the scope of this patch series) to the MIDX code\n> will require us to skip *at most* that number[^1].\n\nSo this patch seems fine to me.\n\n-Peff\n"},{"id":"495895","messageId":"20240530070540.GB1949834@coredump.intra.peff.net","threadId":"61537","inReplyTo":"fe187b1939d9c74782306cd3e4fcd2f25451ad69.1717023301.git.me@ttaylorr.com","subject":"Re: [PATCH v2 6/8] midx-write.c: support reading an existing MIDX with `packs_to_include`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-05-30T07:05:40Z","receivedAt":"2024-05-30T07:05:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 29, 2024 at 06:55:39PM -0400, Taylor Blau wrote:\n\n> static int should_include_pack(const struct write_midx_context *ctx,\n> -\t\t\t const char *file_name)\n> +\t\t\t const char *file_name,\n> +\t\t\t int exclude_from_midx)\n\nHmm, so we grow a new flag...\n\n> {\n> -\t/*\n> -\t * Note that at most one of ctx->m and ctx->to_include are set,\n> -\t * so we are testing midx_contains_pack() and\n> -\t * string_list_has_string() independently (guarded by the\n> -\t * appropriate NULL checks).\n> -\t *\n> -\t * We could support passing to_include while reusing an existing\n> -\t * MIDX, but don't currently since the reuse process drags\n> -\t * forward all packs from an existing MIDX (without checking\n> -\t * whether or not they appear in the to_include list).\n> -\t *\n> -\t * If we added support for that, these next two conditional\n> -\t * should be performed independently (likely checking\n> -\t * to_include before the existing MIDX).\n> -\t */\n> -\tif (ctx->m && midx_contains_pack(ctx->m, file_name))\n> +\tif (exclude_from_midx && ctx->m && midx_contains_pack(ctx->m, file_name))\n> \t\treturn 0;\n> -\telse if (ctx->to_include &&\n> -\t\t !string_list_has_string(ctx->to_include, file_name))\n> +\tif (ctx->to_include && !string_list_has_string(ctx->to_include,\n> +\t\t\t\t\t\t file_name))\n> \t\treturn 0;\n\n...that disables half of what the function is checking.\n\nIt makes me wonder if it should just be two functions (or if the callers\nshould just inline them, since they are themselves basically\none-liners). But I think this is getting into bike-shedding territory.\nI'm OK with it as-is.\n\n-Peff\n"},{"id":"495896","messageId":"20240530071057.GC1949834@coredump.intra.peff.net","threadId":"61537","inReplyTo":"f4f977c1c71ceed339361e8463bace9edc42ec45.1717023301.git.me@ttaylorr.com","subject":"Re: [PATCH v2 7/8] midx: replace `get_midx_rev_filename()` with a generic helper","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-05-30T07:10:57Z","receivedAt":"2024-05-30T07:10:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 29, 2024 at 06:55:42PM -0400, Taylor Blau wrote:\n\n> Commit f894081deae (pack-revindex: read multi-pack reverse indexes,\n> 2021-03-30) introduced the `get_midx_rev_filename()` helper (later\n> modified by commit 60980aed786 (midx.c: write MIDX filenames to\n> strbuf, 2021-10-26)).\n> \n> This function returns the location of the classic \".rev\" files we used\n> to write for MIDXs (prior to 95e8383bac1 (midx.c: make changing the\n> preferred pack safe, 2022-01-25)), which is always of the form:\n> \n> $GIT_DIR/objects/pack/multi-pack-index-$HASH.rev\n> \n> Replace this function with a generic helper that populates a strbuf with\n> the above form, replacing the \".rev\" extension with a caller-provided\n> argument.\n\nMakes sense, as we have similar routines for packfiles.\n\n> +void get_midx_filename_ext(struct strbuf *out, const char *object_dir,\n> +\t\t\t const unsigned char *hash, const char *ext)\n> {\n> \tstrbuf_addf(out, \"%s/pack/multi-pack-index\", object_dir);\n> -}\n> -\n> -void get_midx_rev_filename(struct strbuf *out, struct multi_pack_index *m)\n> -{\n> -\tget_midx_filename(out, m->object_dir);\n> -\tstrbuf_addf(out, \"-%s.rev\", hash_to_hex(get_midx_checksum(m)));\n> +\tif (ext)\n> +\t\tstrbuf_addf(out, \"-%s.%s\", hash_to_hex(hash), ext);\n> }\n\nThis bare \"const unsigned char *hash\" caught my eye, as we've mostly\nbeen removing them. But it was present in the original, too; it was just\nhidden in the return from get_midx_checksum().\n\nAnd I'm not sure what the non-ugly version is. We implicitly use\nthe_hash_algo for these kinds of trailer checksums, and calling them\n\"struct object_id\" is probably even more confusing. I guess they could\nbe \"struct csum_file_trailer\" or something, but I'm not sure that would\nactually make anything more clear. Anyway, none of this is new in your\npatch and we can ignore it for now.\n\n-Peff\n"},{"id":"495897","messageId":"20240530071432.GD1949834@coredump.intra.peff.net","threadId":"61537","inReplyTo":"cover.1717023301.git.me@ttaylorr.com","subject":"Re: [PATCH v2 0/8] midx-write: miscellaneous clean-ups for incremental MIDXs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-05-30T07:14:32Z","receivedAt":"2024-05-30T07:14:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 29, 2024 at 06:55:15PM -0400, Taylor Blau wrote:\n\n> This is a small reroll of my series which has a grab-bag of midx-write\n> related cleanups that I pulled out of a larger series to implement\n> incremental MIDX chains.\n\nThese all look pretty reasonable to me. Thanks for breaking them off of\nthe larger series. I think it's generally nice to get things in smaller\nchunks.\n\nSometimes it is a little tough to evaluate refactorings without seeing\nthe larger context in which they'd be used. But all of these were either\nimmediate improvements, or didn't take much imagination to see where\nthey'd make later things easier.\n\n-Peff\n"},{"id":"495958","messageId":"ZliGJ0EYSfj2LhyY@nand.local","threadId":"61537","inReplyTo":"20240530071432.GD1949834@coredump.intra.peff.net","subject":"Re: [PATCH v2 0/8] midx-write: miscellaneous clean-ups for incremental MIDXs","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-05-30T13:59:03Z","receivedAt":"2024-05-30T13:59:06Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, May 30, 2024 at 03:14:32AM -0400, Jeff King wrote:\n> On Wed, May 29, 2024 at 06:55:15PM -0400, Taylor Blau wrote:\n>\n> > This is a small reroll of my series which has a grab-bag of midx-write\n> > related cleanups that I pulled out of a larger series to implement\n> > incremental MIDX chains.\n>\n> These all look pretty reasonable to me. Thanks for breaking them off of\n> the larger series. I think it's generally nice to get things in smaller\n> chunks.\n\nThanks for reviewing! It is much appreciated, as always :-).\n\n> Sometimes it is a little tough to evaluate refactorings without seeing\n> the larger context in which they'd be used. But all of these were either\n> immediate improvements, or didn't take much imagination to see where\n> they'd make later things easier.\n\nYeah, I feel like this is something that I'm still trying to find the\nright balance with. I think with the pseudo-merge bitmaps series, I\nerred on the side of making the series too large, which made it\ndifficult to easily review.\n\nSeeing the review process on that series made me a little uneasy about\nsending the incremental MIDX bitmaps series, which was growing similarly\nlarge. I think that seeing these 8 patches or so get sent in preparation\nmakes the later series easier to review, but I agree that they are\nsomewhat unsatisfying on their own.\n\nI think in this case it was a reasonable trade-off to make, but you\ncould probably make a compelling case either way there ;-).\n\nOn the bright side, what was once a ~30 patch series is now three series\n(of which this is the first one). The main series is now 13 or so\npatches, the end state of which is to have incremental MIDXs working\nwithout support for reachability bitmaps. The final series I'm planning\nin this area adds support for reachability bitmaps with incremental\nMIDXs, but that series is only half a dozen or so patches.\n\nI'm hoping that by sending it in chunks, the end-to-end review process\nwill be more straightforward and approachable. If you have any other\nideas to make it easier, definitely let me know!\n\nThanks,\nTaylor\n"},{"id":"495980","messageId":"ZlmKIqmoLydp6cKT@tanuki","threadId":"61537","inReplyTo":"20240530071432.GD1949834@coredump.intra.peff.net","subject":"Re: [PATCH v2 0/8] midx-write: miscellaneous clean-ups for incremental MIDXs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-05-31T08:28:18Z","receivedAt":"2024-05-31T08:28:24Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, May 30, 2024 at 03:14:32AM -0400, Jeff King wrote:\n> On Wed, May 29, 2024 at 06:55:15PM -0400, Taylor Blau wrote:\n> \n> > This is a small reroll of my series which has a grab-bag of midx-write\n> > related cleanups that I pulled out of a larger series to implement\n> > incremental MIDX chains.\n> \n> These all look pretty reasonable to me. Thanks for breaking them off of\n> the larger series. I think it's generally nice to get things in smaller\n> chunks.\n> \n> Sometimes it is a little tough to evaluate refactorings without seeing\n> the larger context in which they'd be used. But all of these were either\n> immediate improvements, or didn't take much imagination to see where\n> they'd make later things easier.\n\nI've read through the range-diff and didn't spot anything unexpected\nthere. So this version looks good from my point of view, thanks.\n\nAnd I very much agree that it's nice to split out smaller topics like\nthis. I think it's okay to send patch series that have dozens of commits\nwhen most of the commits are trivial. But the changes in the MIDX aren't\nthat and require the reviewer to dive deep. For me this has the result\nthat I need an hour or more on such large patch series to review them.\nAnd because I often do not find the time for that I push them onto my\npile of shame of stuff that I do want to review.\n\nI get though that this isn't always easy, and in any case it's a\ntradeoff.\n\nPatrick\n"}]}