{"thread":{"id":"58476","subject":"[PATCH v2 0/3] midx: use `--stdin-packs` to implement `repack`","startedAt":"2022-09-20T20:40:20Z","lastAt":"2022-09-23T19:05:23Z","messageCount":10,"participants":["Taylor Blau","Jeff King","Junio C Hamano","Derrick Stolee"],"isPatch":true,"patchVersion":2,"patchTotal":3},"messages":[{"id":"463340","messageId":"cover.1663706401.git.me@ttaylorr.com","threadId":"58476","inReplyTo":"YyokIf%2FSd7SYztKQ@nand.local","subject":"[PATCH v2 0/3] midx: use `--stdin-packs` to implement `repack`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-09-20T20:40:14Z","receivedAt":"2022-09-20T20:40:20Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Here's a few patches that replace the existing \"feed each OID\none-by-one\" approach to implement the `repack` sub-command of the\n`multi-pack-index` builtin with one that uses `pack-objects`'s\n`--stdin-packs` option.\n\nThere is an additional patch at the beginning of this series in order to\nextract the mtime-sorted list of packs to rollup from their home in\n`fill_included_packs_batch()`. There's also one more on the end to unify\nthe `include_pack` array into the `repack_info` struct(s) themselves.\n\nThe second patch is the substantive one. Thanks for all of the review so\nfar! :-)\n\nTaylor Blau (3):\n  midx.c: compute pack_info array outside of fill_included_packs_batch()\n  midx.c: use `pack-objects --stdin-packs` when repacking\n  midx.c: unify `include_pack` array into `pack_info` struct\n\n midx.c | 76 +++++++++++++++++++++++++++++-----------------------------\n 1 file changed, 38 insertions(+), 38 deletions(-)\n\nRange-diff against v1:\n-:  ---------- > 1:  a501776f6e midx.c: compute pack_info array outside of fill_included_packs_batch()\n1:  7e987eb9d7 ! 2:  4218d9e08a midx.c: use `pack-objects --stdin-packs` when repacking\n    @@ midx.c: int midx_repack(struct repository *r, const char *object_dir, size_t bat\n      \tstruct strbuf base_name = STRBUF_INIT;\n     +\tstruct strbuf scratch = STRBUF_INIT;\n      \tstruct multi_pack_index *m = lookup_multi_pack_index(r, object_dir);\n    + \tstruct repack_info *pack_info = NULL;\n      \n    - \t/*\n     @@ midx.c: int midx_repack(struct repository *r, const char *object_dir, size_t batch_size,\n      \trepo_config_get_bool(r, \"repack.usedeltabaseoffset\", &delta_base_offset);\n      \trepo_config_get_bool(r, \"repack.usedeltaislands\", &use_delta_islands);\n    @@ midx.c: int midx_repack(struct repository *r, const char *object_dir, size_t bat\n     -\t\tstruct object_id oid;\n     -\t\tuint32_t pack_int_id = nth_midxed_pack_int_id(m, i);\n     +\tfor (i = 0; i < m->num_packs; i++) {\n    -+\t\tstrbuf_reset(&scratch);\n    ++\t\tstruct repack_info *info = &pack_info[i];\n      \n     -\t\tif (!include_pack[pack_int_id])\n     -\t\t\tcontinue;\n    -+\t\tstrbuf_addstr(&scratch, m->pack_names[i]);\n    -+\t\tstrbuf_strip_suffix(&scratch, \".idx\");\n    -+\t\tstrbuf_addstr(&scratch, \".pack\");\n    ++\t\tstrbuf_reset(&scratch);\n      \n     -\t\tnth_midxed_object_oid(&oid, m, i);\n     -\t\tfprintf(cmd_in, \"%s\\n\", oid_to_hex(&oid));\n    -+\t\tfprintf(cmd_in, \"%s%s\\n\", include_pack[i] ? \"\" : \"^\", scratch.buf);\n    ++\t\tstrbuf_addstr(&scratch, m->pack_names[info->pack_int_id]);\n    ++\t\tstrbuf_strip_suffix(&scratch, \".idx\");\n    ++\t\tstrbuf_addstr(&scratch, \".pack\");\n    ++\n    ++\t\tfprintf(cmd_in, \"%s%s\\n\", include_pack[info->pack_int_id] ? \"\" : \"^\", scratch.buf);\n      \t}\n      \tfclose(cmd_in);\n     +\tstrbuf_release(&scratch);\n-:  ---------- > 3:  81e9ccc323 midx.c: unify `include_pack` array into `pack_info` struct\n-- \n2.37.0.1.g1379af2e9d\n"},{"id":"463341","messageId":"a501776f6eaeef6fcddf2cf9a349f8888fabcb0f.1663706401.git.me@ttaylorr.com","threadId":"58476","inReplyTo":"cover.1663706401.git.me@ttaylorr.com","subject":"[PATCH v2 1/3] midx.c: compute pack_info array outside of fill_included_packs_batch()","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-09-20T20:40:17Z","receivedAt":"2022-09-20T20:40:28Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"The fill_included_packs_batch() function needs to compute a list of\npacks which are included in the MIDX, and then sort that list according\nto the pack mtimes.\n\nSince packs are listed in the `m->pack_names` array in SHA-1 order, we\nhave to reconstruct their mtimes by reading the `pack_mtime` member of\nthe struct `packed_git` in order to determine the value by which to\nsort.\n\nThis list is only needed by the fill_included_packs_batch(), but the\nsubsequent commit will want to have this list accessible in this\nfunction's caller, `midx_repack()`.\n\nPrepare for that by teaching `midx_repack()` to compute, pass in, and\nthen free that list so it is available throughout the body of\n`midx_repack()`.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n midx.c | 40 ++++++++++++++++++++--------------------\n 1 file changed, 20 insertions(+), 20 deletions(-)\n\ndiff --git a/midx.c b/midx.c\nindex c27d0e5f15..db0a70c5af 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -1905,32 +1905,16 @@ static int fill_included_packs_all(struct repository *r,\n \n static int fill_included_packs_batch(struct repository *r,\n \t\t\t\t     struct multi_pack_index *m,\n+\t\t\t\t     struct repack_info *pack_info,\n \t\t\t\t     unsigned char *include_pack,\n \t\t\t\t     size_t batch_size)\n {\n \tuint32_t i, packs_to_repack;\n \tsize_t total_size;\n-\tstruct repack_info *pack_info = xcalloc(m->num_packs, sizeof(struct repack_info));\n \tint pack_kept_objects = 0;\n \n \trepo_config_get_bool(r, \"repack.packkeptobjects\", &pack_kept_objects);\n \n-\tfor (i = 0; i < m->num_packs; i++) {\n-\t\tpack_info[i].pack_int_id = i;\n-\n-\t\tif (prepare_midx_pack(r, m, i))\n-\t\t\tcontinue;\n-\n-\t\tpack_info[i].mtime = m->packs[i]->mtime;\n-\t}\n-\n-\tfor (i = 0; batch_size && i < m->num_objects; i++) {\n-\t\tuint32_t pack_int_id = nth_midxed_pack_int_id(m, i);\n-\t\tpack_info[pack_int_id].referenced_objects++;\n-\t}\n-\n-\tQSORT(pack_info, m->num_packs, compare_by_mtime);\n-\n \ttotal_size = 0;\n \tpacks_to_repack = 0;\n \tfor (i = 0; total_size < batch_size && i < m->num_packs; i++) {\n@@ -1957,8 +1941,6 @@ static int fill_included_packs_batch(struct repository *r,\n \t\tinclude_pack[pack_int_id] = 1;\n \t}\n \n-\tfree(pack_info);\n-\n \tif (packs_to_repack < 2)\n \t\treturn 1;\n \n@@ -1974,6 +1956,7 @@ int midx_repack(struct repository *r, const char *object_dir, size_t batch_size,\n \tFILE *cmd_in;\n \tstruct strbuf base_name = STRBUF_INIT;\n \tstruct multi_pack_index *m = lookup_multi_pack_index(r, object_dir);\n+\tstruct repack_info *pack_info = NULL;\n \n \t/*\n \t * When updating the default for these configuration\n@@ -1987,9 +1970,25 @@ int midx_repack(struct repository *r, const char *object_dir, size_t batch_size,\n \t\treturn 0;\n \n \tCALLOC_ARRAY(include_pack, m->num_packs);\n+\tCALLOC_ARRAY(pack_info, m->num_packs);\n+\n+\tfor (i = 0; i < m->num_packs; i++) {\n+\t\tpack_info[i].pack_int_id = i;\n+\n+\t\tif (prepare_midx_pack(r, m, i))\n+\t\t\tcontinue;\n+\n+\t\tpack_info[i].mtime = m->packs[i]->mtime;\n+\t}\n+\n+\tfor (i = 0; batch_size && i < m->num_objects; i++)\n+\t\tpack_info[nth_midxed_pack_int_id(m, i)].referenced_objects++;\n+\n+\tQSORT(pack_info, m->num_packs, compare_by_mtime);\n \n \tif (batch_size) {\n-\t\tif (fill_included_packs_batch(r, m, include_pack, batch_size))\n+\t\tif (fill_included_packs_batch(r, m, pack_info, include_pack,\n+\t\t\t\t\t      batch_size))\n \t\t\tgoto cleanup;\n \t} else if (fill_included_packs_all(r, m, include_pack))\n \t\tgoto cleanup;\n@@ -2047,6 +2046,7 @@ int midx_repack(struct repository *r, const char *object_dir, size_t batch_size,\n \tresult = write_midx_internal(object_dir, NULL, NULL, NULL, NULL, flags);\n \n cleanup:\n+\tfree(pack_info);\n \tfree(include_pack);\n \treturn result;\n }\n-- \n2.37.0.1.g1379af2e9d\n\n"},{"id":"463342","messageId":"4218d9e08aba629d8f64b5a999f60d12e5d8785b.1663706401.git.me@ttaylorr.com","threadId":"58476","inReplyTo":"cover.1663706401.git.me@ttaylorr.com","subject":"[PATCH v2 2/3] midx.c: use `pack-objects --stdin-packs` when repacking","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-09-20T20:40:19Z","receivedAt":"2022-09-20T20:40:30Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"The `git multi-pack-index repack` command aggregates all objects in\ncertain packs contained in the MIDX, largely dictated by the value of\nits `--batch-size` parameter.\n\nTo create the new pack, the MIDX machinery spawns a `pack-objects`\nsub-process, which it feeds the object IDs in each of the pack(s) that\nit wants to aggregate.\n\nThis implementation, which dates back to ce1e4a105b (midx: implement\nmidx_repack(), 2019-06-10), predates the `--stdin-packs` mode in\n`pack-objects`. This mode (which was introduced a couple of years later\nin 339bce27f4 (builtin/pack-objects.c: add '--stdin-packs' option,\n2021-02-22)) allows `pack-objects` callers to specify the contents of\nthe output pack by indicating which packs to aggregate.\n\nThis patch replaces the pre-`--stdin-packs` invocation (where each\nobject is given to `pack-objects` one by one) with the more modern\n`--stdin-packs` option.\n\nThis allows us to avoid some CPU cycles serializing and deserializing\nevery object ID in all of the packs we're aggregating. It also avoids us\nhaving to send a potentially large amount of data down to\n`pack-objects`.\n\nBut more importantly, it generates slightly higher quality (read: more\ntightly compressed) packs, because of the reachability traversal that\n`--stdin-packs` does after the fact in order to gather namehash values\nwhich seed the delta selection process.\n\nIn practice, this seems to add a slight amount of overhead (on the order\nof a few seconds for git.git broken up into ~100 packs), in exchange for\na modest reduction (on the order of ~3.5%) in the resulting pack size.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n midx.c | 19 +++++++++++--------\n 1 file changed, 11 insertions(+), 8 deletions(-)\n\ndiff --git a/midx.c b/midx.c\nindex db0a70c5af..5fbf964bba 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -1955,6 +1955,7 @@ int midx_repack(struct repository *r, const char *object_dir, size_t batch_size,\n \tstruct child_process cmd = CHILD_PROCESS_INIT;\n \tFILE *cmd_in;\n \tstruct strbuf base_name = STRBUF_INIT;\n+\tstruct strbuf scratch = STRBUF_INIT;\n \tstruct multi_pack_index *m = lookup_multi_pack_index(r, object_dir);\n \tstruct repack_info *pack_info = NULL;\n \n@@ -1996,7 +1997,7 @@ int midx_repack(struct repository *r, const char *object_dir, size_t batch_size,\n \trepo_config_get_bool(r, \"repack.usedeltabaseoffset\", &delta_base_offset);\n \trepo_config_get_bool(r, \"repack.usedeltaislands\", &use_delta_islands);\n \n-\tstrvec_push(&cmd.args, \"pack-objects\");\n+\tstrvec_pushl(&cmd.args, \"pack-objects\", \"--stdin-packs\", NULL);\n \n \tstrbuf_addstr(&base_name, object_dir);\n \tstrbuf_addstr(&base_name, \"/pack/pack\");\n@@ -2025,17 +2026,19 @@ int midx_repack(struct repository *r, const char *object_dir, size_t batch_size,\n \n \tcmd_in = xfdopen(cmd.in, \"w\");\n \n-\tfor (i = 0; i < m->num_objects; i++) {\n-\t\tstruct object_id oid;\n-\t\tuint32_t pack_int_id = nth_midxed_pack_int_id(m, i);\n+\tfor (i = 0; i < m->num_packs; i++) {\n+\t\tstruct repack_info *info = &pack_info[i];\n \n-\t\tif (!include_pack[pack_int_id])\n-\t\t\tcontinue;\n+\t\tstrbuf_reset(&scratch);\n \n-\t\tnth_midxed_object_oid(&oid, m, i);\n-\t\tfprintf(cmd_in, \"%s\\n\", oid_to_hex(&oid));\n+\t\tstrbuf_addstr(&scratch, m->pack_names[info->pack_int_id]);\n+\t\tstrbuf_strip_suffix(&scratch, \".idx\");\n+\t\tstrbuf_addstr(&scratch, \".pack\");\n+\n+\t\tfprintf(cmd_in, \"%s%s\\n\", include_pack[info->pack_int_id] ? \"\" : \"^\", scratch.buf);\n \t}\n \tfclose(cmd_in);\n+\tstrbuf_release(&scratch);\n \n \tif (finish_command(&cmd)) {\n \t\terror(_(\"could not finish pack-objects\"));\n-- \n2.37.0.1.g1379af2e9d\n\n"},{"id":"463343","messageId":"81e9ccc3232f1a372adc9b06e2a93e9d9b26c80d.1663706401.git.me@ttaylorr.com","threadId":"58476","inReplyTo":"cover.1663706401.git.me@ttaylorr.com","subject":"[PATCH v2 3/3] midx.c: unify `include_pack` array into `pack_info` struct","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-09-20T20:40:22Z","receivedAt":"2022-09-20T20:40:34Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"The MIDX repack code uses a separate byte-array to track which packs are\n\"included\" in the rollup. Now that an array of `repack_info` structs is\navailable for the lifetime of the `midx_repack()` function, we can\ninstead use a new bit in that struct's record instead of maintaining a\nseparate array.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n midx.c | 23 ++++++++++-------------\n 1 file changed, 10 insertions(+), 13 deletions(-)\n\ndiff --git a/midx.c b/midx.c\nindex 5fbf964bba..59db9162ea 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -1865,6 +1865,7 @@ struct repack_info {\n \ttimestamp_t mtime;\n \tuint32_t referenced_objects;\n \tuint32_t pack_int_id;\n+\tunsigned include : 1;\n };\n \n static int compare_by_mtime(const void *a_, const void *b_)\n@@ -1883,7 +1884,7 @@ static int compare_by_mtime(const void *a_, const void *b_)\n \n static int fill_included_packs_all(struct repository *r,\n \t\t\t\t   struct multi_pack_index *m,\n-\t\t\t\t   unsigned char *include_pack)\n+\t\t\t\t   struct repack_info *pack_info)\n {\n \tuint32_t i, count = 0;\n \tint pack_kept_objects = 0;\n@@ -1896,7 +1897,7 @@ static int fill_included_packs_all(struct repository *r,\n \t\tif (!pack_kept_objects && m->packs[i]->pack_keep)\n \t\t\tcontinue;\n \n-\t\tinclude_pack[i] = 1;\n+\t\tpack_info[i].include = 1;\n \t\tcount++;\n \t}\n \n@@ -1906,7 +1907,6 @@ static int fill_included_packs_all(struct repository *r,\n static int fill_included_packs_batch(struct repository *r,\n \t\t\t\t     struct multi_pack_index *m,\n \t\t\t\t     struct repack_info *pack_info,\n-\t\t\t\t     unsigned char *include_pack,\n \t\t\t\t     size_t batch_size)\n {\n \tuint32_t i, packs_to_repack;\n@@ -1918,8 +1918,8 @@ static int fill_included_packs_batch(struct repository *r,\n \ttotal_size = 0;\n \tpacks_to_repack = 0;\n \tfor (i = 0; total_size < batch_size && i < m->num_packs; i++) {\n-\t\tint pack_int_id = pack_info[i].pack_int_id;\n-\t\tstruct packed_git *p = m->packs[pack_int_id];\n+\t\tstruct repack_info *info = &pack_info[i];\n+\t\tstruct packed_git *p = m->packs[info->pack_int_id];\n \t\tsize_t expected_size;\n \n \t\tif (!p)\n@@ -1938,7 +1938,8 @@ static int fill_included_packs_batch(struct repository *r,\n \n \t\tpacks_to_repack++;\n \t\ttotal_size += expected_size;\n-\t\tinclude_pack[pack_int_id] = 1;\n+\n+\t\tinfo->include = 1;\n \t}\n \n \tif (packs_to_repack < 2)\n@@ -1951,7 +1952,6 @@ int midx_repack(struct repository *r, const char *object_dir, size_t batch_size,\n {\n \tint result = 0;\n \tuint32_t i;\n-\tunsigned char *include_pack;\n \tstruct child_process cmd = CHILD_PROCESS_INIT;\n \tFILE *cmd_in;\n \tstruct strbuf base_name = STRBUF_INIT;\n@@ -1970,7 +1970,6 @@ int midx_repack(struct repository *r, const char *object_dir, size_t batch_size,\n \tif (!m)\n \t\treturn 0;\n \n-\tCALLOC_ARRAY(include_pack, m->num_packs);\n \tCALLOC_ARRAY(pack_info, m->num_packs);\n \n \tfor (i = 0; i < m->num_packs; i++) {\n@@ -1988,10 +1987,9 @@ int midx_repack(struct repository *r, const char *object_dir, size_t batch_size,\n \tQSORT(pack_info, m->num_packs, compare_by_mtime);\n \n \tif (batch_size) {\n-\t\tif (fill_included_packs_batch(r, m, pack_info, include_pack,\n-\t\t\t\t\t      batch_size))\n+\t\tif (fill_included_packs_batch(r, m, pack_info, batch_size))\n \t\t\tgoto cleanup;\n-\t} else if (fill_included_packs_all(r, m, include_pack))\n+\t} else if (fill_included_packs_all(r, m, pack_info))\n \t\tgoto cleanup;\n \n \trepo_config_get_bool(r, \"repack.usedeltabaseoffset\", &delta_base_offset);\n@@ -2035,7 +2033,7 @@ int midx_repack(struct repository *r, const char *object_dir, size_t batch_size,\n \t\tstrbuf_strip_suffix(&scratch, \".idx\");\n \t\tstrbuf_addstr(&scratch, \".pack\");\n \n-\t\tfprintf(cmd_in, \"%s%s\\n\", include_pack[info->pack_int_id] ? \"\" : \"^\", scratch.buf);\n+\t\tfprintf(cmd_in, \"%s%s\\n\", info->include ? \"\" : \"^\", scratch.buf);\n \t}\n \tfclose(cmd_in);\n \tstrbuf_release(&scratch);\n@@ -2050,6 +2048,5 @@ int midx_repack(struct repository *r, const char *object_dir, size_t batch_size,\n \n cleanup:\n \tfree(pack_info);\n-\tfree(include_pack);\n \treturn result;\n }\n-- \n2.37.0.1.g1379af2e9d\n"},{"id":"463350","messageId":"Yyo2mJZt6B0JVGwA@coredump.intra.peff.net","threadId":"58476","inReplyTo":"cover.1663706401.git.me@ttaylorr.com","subject":"Re: [PATCH v2 0/3] midx: use `--stdin-packs` to implement `repack`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-09-20T21:54:32Z","receivedAt":"2022-09-20T21:54:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 20, 2022 at 04:40:14PM -0400, Taylor Blau wrote:\n\n> Here's a few patches that replace the existing \"feed each OID\n> one-by-one\" approach to implement the `repack` sub-command of the\n> `multi-pack-index` builtin with one that uses `pack-objects`'s\n> `--stdin-packs` option.\n> \n> There is an additional patch at the beginning of this series in order to\n> extract the mtime-sorted list of packs to rollup from their home in\n> `fill_included_packs_batch()`. There's also one more on the end to unify\n> the `include_pack` array into the `repack_info` struct(s) themselves.\n\nThe first and third make sense to me, and it looks like a nice cleanup.\n\nThe middle one looks fine, modulo all of our earlier discussion about\nthe extra traversal being fine. I think it would be good t get a review\nfrom Stolee as the person who invented \"git multi-index-pack repack\" and\nwould understand its intended use. Presumably this is all weird \"I have\na giant Windows repository\" packing strategy. :)\n\n-Peff\n"},{"id":"463384","messageId":"xmqq35ck35p0.fsf@gitster.g","threadId":"58476","inReplyTo":"cover.1663706401.git.me@ttaylorr.com","subject":"Re: [PATCH v2 0/3] midx: use `--stdin-packs` to implement `repack`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-21T19:09:31Z","receivedAt":"2022-09-21T19:09:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau <me@ttaylorr.com> writes:\n\n> Here's a few patches that replace the existing \"feed each OID\n> one-by-one\" approach to implement the `repack` sub-command of the\n> `multi-pack-index` builtin with one that uses `pack-objects`'s\n> `--stdin-packs` option.\n\nOne question.  How is this series expected to interact with the\n7-patch series about ignoring cruft pack while \"midx repack\" etc.?\n\nI guess it is not so urgent during the -rc period, so I'll stop at\nasking that question without reading further, at least for now.\n\nThanks.\n"},{"id":"463529","messageId":"5172dbb7-61d1-7249-f9bb-d760e6f4450a@github.com","threadId":"58476","inReplyTo":"4218d9e08aba629d8f64b5a999f60d12e5d8785b.1663706401.git.me@ttaylorr.com","subject":"Re: [PATCH v2 2/3] midx.c: use `pack-objects --stdin-packs` when repacking","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-09-23T18:23:30Z","receivedAt":"2022-09-23T18:23:44Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 9/20/2022 4:40 PM, Taylor Blau wrote:\n\n> +\tfor (i = 0; i < m->num_packs; i++) {\n> +\t\tstruct repack_info *info = &pack_info[i];\n>  \n> -\t\tif (!include_pack[pack_int_id])\n> -\t\t\tcontinue;\n\n...\n\n> +\t\tfprintf(cmd_in, \"%s%s\\n\", include_pack[info->pack_int_id] ? \"\" : \"^\", scratch.buf);\n\nOutside of how the object set is provided (a list of objects or a list of\npacks) it seems this is equivalent. The only difference is that we are\nwriting a line for _every_ pack in the multi-pack-index, but we preface\nthe line with \"^\" to say \"not this pack\".\n\nDo you know if there is any reason to do this explicitly? Does this\nchange the set of objects in any way (perhaps by not including\nduplicates that are tracked in those other packs)?\n\nPersonally, I would have kept the \"if (...) continue;\" pattern, so maybe\nyou had a concrete idea why this approach is better.\n\nThanks,\n-Stolee\n"},{"id":"463530","messageId":"Yy364WAutGIdXCub@nand.local","threadId":"58476","inReplyTo":"5172dbb7-61d1-7249-f9bb-d760e6f4450a@github.com","subject":"Re: [PATCH v2 2/3] midx.c: use `pack-objects --stdin-packs` when repacking","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-09-23T18:28:49Z","receivedAt":"2022-09-23T18:28:55Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Sep 23, 2022 at 02:23:30PM -0400, Derrick Stolee wrote:\n> Do you know if there is any reason to do this explicitly? Does this\n> change the set of objects in any way (perhaps by not including\n> duplicates that are tracked in those other packs)?\n\nYes. The \"^\" lines become excluded packs from the perspective of the\nfollow-on reachability traversal to discover the namehashes. So as soon\nas we hit an object contained in one of the packs marked as excluded,\nwe'll halt the traversal along that direction, since we know that we're\nnot going to pick up any objects in those packs.\n\nSo you could omit them, and you'd get the same resulting pack, but it\nwould take longer to generate since we wouldn't be stopping the\nfollow-on traversal as early as possible.\n\nThanks,\nTaylor\n"},{"id":"463531","messageId":"Yy37C/tHg5Quto30@nand.local","threadId":"58476","inReplyTo":"xmqq35ck35p0.fsf@gitster.g","subject":"Re: [PATCH v2 0/3] midx: use `--stdin-packs` to implement `repack`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-09-23T18:29:31Z","receivedAt":"2022-09-23T18:29:39Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Sep 21, 2022 at 12:09:31PM -0700, Junio C Hamano wrote:\n> Taylor Blau <me@ttaylorr.com> writes:\n>\n> > Here's a few patches that replace the existing \"feed each OID\n> > one-by-one\" approach to implement the `repack` sub-command of the\n> > `multi-pack-index` builtin with one that uses `pack-objects`'s\n> > `--stdin-packs` option.\n>\n> One question.  How is this series expected to interact with the\n> 7-patch series about ignoring cruft pack while \"midx repack\" etc.?\n\nThe first round merges cleanly, but the second round will have a slight\nconflict. I'll send you a new version of this series based on whatever\nis in `master` following the release.\n\nThanks,\nTaylor\n"},{"id":"463544","messageId":"c8c42bd6-4aef-d310-b988-108bf74ebb96@github.com","threadId":"58476","inReplyTo":"Yy364WAutGIdXCub@nand.local","subject":"Re: [PATCH v2 2/3] midx.c: use `pack-objects --stdin-packs` when repacking","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-09-23T19:05:08Z","receivedAt":"2022-09-23T19:05:23Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 9/23/2022 2:28 PM, Taylor Blau wrote:\n> On Fri, Sep 23, 2022 at 02:23:30PM -0400, Derrick Stolee wrote:\n>> Do you know if there is any reason to do this explicitly? Does this\n>> change the set of objects in any way (perhaps by not including\n>> duplicates that are tracked in those other packs)?\n> \n> Yes. The \"^\" lines become excluded packs from the perspective of the\n> follow-on reachability traversal to discover the namehashes. So as soon\n> as we hit an object contained in one of the packs marked as excluded,\n> we'll halt the traversal along that direction, since we know that we're\n> not going to pick up any objects in those packs.\n> \n> So you could omit them, and you'd get the same resulting pack, but it\n> would take longer to generate since we wouldn't be stopping the\n> follow-on traversal as early as possible.\n\nExcellent. Thanks for explaining that!\n\n-Stolee\n"}]}