{"thread":{"id":"57904","subject":"[PATCH] repack: respect --keep-pack with geometric repack","startedAt":"2022-05-20T16:36:20Z","lastAt":"2022-05-20T23:26:28Z","messageCount":8,"participants":["Victoria Dye via GitGitGadget","Taylor Blau","Victoria Dye","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"455576","messageId":"pull.1235.git.1653064572170.gitgitgadget@gmail.com","threadId":"57904","inReplyTo":null,"subject":"[PATCH] repack: respect --keep-pack with geometric repack","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-05-20T16:36:12Z","receivedAt":"2022-05-20T16:36:20Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"From: Victoria Dye <vdye@github.com>\n\nUpdate 'repack' to ignore packs named on the command line with the\n'--keep-pack' option. Specifically, modify 'init_pack_geometry()' to treat\ncommand line-kept packs the same way it treats packs with an on-disk '.keep'\nfile (that is, skip the pack and do not include it in the 'geometry'\nstructure).\n\nWithout this handling, a '--keep-pack' pack would be included in the\n'geometry' structure. If the pack is *before* the geometry split line (with\nat least one other pack and/or loose objects present), 'repack' assumes the\npack's contents are \"rolled up\" into another pack via 'pack-objects'.\nHowever, because the internally-invoked 'pack-objects' properly excludes\n'--keep-pack' objects, any new pack it creates will not contain the kept\nobjects. Finally, 'repack' deletes the '--keep-pack' as \"redundant\" (since\nit assumes 'pack-objects' created a new pack with its contents), resulting\nin possible object loss and repository corruption.\n\nAdd a test ensuring that '--keep-pack' packs are now appropriately handled.\n\nCo-authored-by: Taylor Blau <me@ttaylorr.com>\nSigned-off-by: Victoria Dye <vdye@github.com>\n---\n    repack: respect --keep-pack with geometric repack\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1235%2Fvdye%2Fbugfix%2Frepack-kept-pack-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1235/vdye/bugfix/repack-kept-pack-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1235\n\n builtin/repack.c            | 40 +++++++++++++++++++++++++++----------\n t/t7703-repack-geometric.sh | 34 +++++++++++++++++++++++++++++++\n 2 files changed, 63 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex d1a563d5b65..0b636676056 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -332,17 +332,34 @@ static int geometry_cmp(const void *va, const void *vb)\n \treturn 0;\n }\n \n-static void init_pack_geometry(struct pack_geometry **geometry_p)\n+static void init_pack_geometry(struct pack_geometry **geometry_p,\n+\t\t\t       struct string_list *existing_kept_packs)\n {\n \tstruct packed_git *p;\n \tstruct pack_geometry *geometry;\n+\tstruct strbuf buf = STRBUF_INIT;\n \n \t*geometry_p = xcalloc(1, sizeof(struct pack_geometry));\n \tgeometry = *geometry_p;\n \n+\tstring_list_sort(existing_kept_packs);\n \tfor (p = get_all_packs(the_repository); p; p = p->next) {\n-\t\tif (!pack_kept_objects && p->pack_keep)\n-\t\t\tcontinue;\n+\t\tif (!pack_kept_objects) {\n+\t\t\tif (p->pack_keep)\n+\t\t\t\tcontinue;\n+\n+\t\t\t/*\n+\t\t\t * The pack may be kept via the --keep-pack option;\n+\t\t\t * check 'existing_kept_packs' to determine whether to\n+\t\t\t * ignore it.\n+\t\t\t */\n+\t\t\tstrbuf_reset(&buf);\n+\t\t\tstrbuf_addstr(&buf, pack_basename(p));\n+\t\t\tstrbuf_strip_suffix(&buf, \".pack\");\n+\n+\t\t\tif (string_list_has_string(existing_kept_packs, buf.buf))\n+\t\t\t\tcontinue;\n+\t\t}\n \n \t\tALLOC_GROW(geometry->pack,\n \t\t\t   geometry->pack_nr + 1,\n@@ -353,6 +370,7 @@ static void init_pack_geometry(struct pack_geometry **geometry_p)\n \t}\n \n \tQSORT(geometry->pack, geometry->pack_nr, geometry_cmp);\n+\tstrbuf_release(&buf);\n }\n \n static void split_pack_geometry(struct pack_geometry *geometry, int factor)\n@@ -714,17 +732,20 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\tstrbuf_release(&path);\n \t}\n \n+\tpackdir = mkpathdup(\"%s/pack\", get_object_directory());\n+\tpacktmp_name = xstrfmt(\".tmp-%d-pack\", (int)getpid());\n+\tpacktmp = mkpathdup(\"%s/%s\", packdir, packtmp_name);\n+\n+\tcollect_pack_filenames(&existing_nonkept_packs, &existing_kept_packs,\n+\t\t\t       &keep_pack_list);\n+\n \tif (geometric_factor) {\n \t\tif (pack_everything)\n \t\t\tdie(_(\"options '%s' and '%s' cannot be used together\"), \"--geometric\", \"-A/-a\");\n-\t\tinit_pack_geometry(&geometry);\n+\t\tinit_pack_geometry(&geometry, &existing_kept_packs);\n \t\tsplit_pack_geometry(geometry, geometric_factor);\n \t}\n \n-\tpackdir = mkpathdup(\"%s/pack\", get_object_directory());\n-\tpacktmp_name = xstrfmt(\".tmp-%d-pack\", (int)getpid());\n-\tpacktmp = mkpathdup(\"%s/%s\", packdir, packtmp_name);\n-\n \tsigchain_push_common(remove_pack_on_signal);\n \n \tprepare_pack_objects(&cmd, &po_args);\n@@ -764,9 +785,6 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \tif (use_delta_islands)\n \t\tstrvec_push(&cmd.args, \"--delta-islands\");\n \n-\tcollect_pack_filenames(&existing_nonkept_packs, &existing_kept_packs,\n-\t\t\t       &keep_pack_list);\n-\n \tif (pack_everything & ALL_INTO_ONE) {\n \t\trepack_promisor_objects(&po_args, &names);\n \ndiff --git a/t/t7703-repack-geometric.sh b/t/t7703-repack-geometric.sh\nindex bdbbcbf1eca..f5ac23413d5 100755\n--- a/t/t7703-repack-geometric.sh\n+++ b/t/t7703-repack-geometric.sh\n@@ -180,6 +180,40 @@ test_expect_success '--geometric ignores kept packs' '\n \t)\n '\n \n+test_expect_success '--geometric ignores --keep-pack packs' '\n+\tgit init geometric &&\n+\ttest_when_finished \"rm -fr geometric\" &&\n+\t(\n+\t\tcd geometric &&\n+\n+\t\t# Create two equal-sized packs\n+\t\ttest_commit kept && # 3 objects\n+\t\ttest_commit pack && # 3 objects\n+\n+\t\tKEPT=$(git pack-objects --revs $objdir/pack/pack <<-EOF\n+\t\trefs/tags/kept\n+\t\tEOF\n+\t\t) &&\n+\t\tPACK=$(git pack-objects --revs $objdir/pack/pack <<-EOF\n+\t\trefs/tags/pack\n+\t\t^refs/tags/kept\n+\t\tEOF\n+\t\t) &&\n+\n+\t\t# Prune loose objects that are now packed into PACK and KEEP\n+\t\tgit prune-packed &&\n+\n+\t\tgit repack --geometric 2 -dm --keep-pack=pack-$KEPT.pack >out &&\n+\n+\t\t# Packs should not have changed (only one non-kept pack, no\n+\t\t# loose objects), but midx should now exist.\n+\t\ttest_i18ngrep \"Nothing new to pack\" out &&\n+\t\ttest_path_is_file $midx &&\n+\t\ttest_path_is_file $objdir/pack/pack-$KEPT.pack &&\n+\t\tgit fsck\n+\t)\n+'\n+\n test_expect_success '--geometric chooses largest MIDX preferred pack' '\n \tgit init geometric &&\n \ttest_when_finished \"rm -fr geometric\" &&\n\nbase-commit: 277cf0bc36094f6dc4297d8c9cef79df045b735d\n-- \ngitgitgadget\n"},{"id":"455577","messageId":"YofJLv8+x5J7yPmf@nand.local","threadId":"57904","inReplyTo":"pull.1235.git.1653064572170.gitgitgadget@gmail.com","subject":"Re: [PATCH] repack: respect --keep-pack with geometric repack","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-05-20T17:00:30Z","receivedAt":"2022-05-20T17:00:37Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi Victoria,\n\nOn Fri, May 20, 2022 at 04:36:12PM +0000, Victoria Dye via GitGitGadget wrote:\n> From: Victoria Dye <vdye@github.com>\n>\n> Update 'repack' to ignore packs named on the command line with the\n> '--keep-pack' option. Specifically, modify 'init_pack_geometry()' to treat\n> command line-kept packs the same way it treats packs with an on-disk '.keep'\n> file (that is, skip the pack and do not include it in the 'geometry'\n> structure).\n>\n> Without this handling, a '--keep-pack' pack would be included in the\n> 'geometry' structure. If the pack is *before* the geometry split line (with\n> at least one other pack and/or loose objects present), 'repack' assumes the\n> pack's contents are \"rolled up\" into another pack via 'pack-objects'.\n> However, because the internally-invoked 'pack-objects' properly excludes\n> '--keep-pack' objects, any new pack it creates will not contain the kept\n> objects. Finally, 'repack' deletes the '--keep-pack' as \"redundant\" (since\n> it assumes 'pack-objects' created a new pack with its contents), resulting\n> in possible object loss and repository corruption.\n\nNicely found and explained. Having discussed this fix with you already\noff-list, this approach (to treat kept packs as excluded from the list\nof packs in the `geometry` structure regardless of whether they are kept\non disk or in-core) makes sense to me.\n\nI left a couple of small notes on the patch below, but since I have some\npatches that deal with a separate issue in the `git repack --geometric`\ncode coming, do you want to combine forces (and I can send a\nlightly-reworked version of this patch as a part of my series)?\n\n> @@ -332,17 +332,34 @@ static int geometry_cmp(const void *va, const void *vb)\n>  \treturn 0;\n>  }\n>\n> -static void init_pack_geometry(struct pack_geometry **geometry_p)\n> +static void init_pack_geometry(struct pack_geometry **geometry_p,\n> +\t\t\t       struct string_list *existing_kept_packs)\n>  {\n>  \tstruct packed_git *p;\n>  \tstruct pack_geometry *geometry;\n> +\tstruct strbuf buf = STRBUF_INIT;\n>\n>  \t*geometry_p = xcalloc(1, sizeof(struct pack_geometry));\n>  \tgeometry = *geometry_p;\n>\n> +\tstring_list_sort(existing_kept_packs);\n\nWould it be worth sorting this as early as in collect_pack_filenames()?\nFor our purposes in this patch, this works as-is, but it may be\ndefensive to try and minimize the time that list has unsorted contents.\n\n>  \tfor (p = get_all_packs(the_repository); p; p = p->next) {\n> -\t\tif (!pack_kept_objects && p->pack_keep)\n> -\t\t\tcontinue;\n> +\t\tif (!pack_kept_objects) {\n> +\t\t\tif (p->pack_keep)\n> +\t\t\t\tcontinue;\n\n(You mentioned this to me off-list, but I'll repeat it here since it\nwasn't obvious to me on first read): this check for `p->pack_keep` isn't\nstrictly necessary, since any packs that have their `pack_keep` bit set\nwill appear in the `existing_kept_packs` list.\n\nBut it does give us a fast path to avoid having to check that list, so\nit's worth checking that bit to avoid a slightly more expensive check\nwhere possible.\n\n> +\t\t\t/*\n> +\t\t\t * The pack may be kept via the --keep-pack option;\n> +\t\t\t * check 'existing_kept_packs' to determine whether to\n> +\t\t\t * ignore it.\n> +\t\t\t */\n> +\t\t\tstrbuf_reset(&buf);\n> +\t\t\tstrbuf_addstr(&buf, pack_basename(p));\n> +\t\t\tstrbuf_strip_suffix(&buf, \".pack\");\n> +\n> +\t\t\tif (string_list_has_string(existing_kept_packs, buf.buf))\n> +\t\t\t\tcontinue;\n\nIt's too bad that we have to do this check at all, and can't rely on the\n`pack_keep_in_core` in the same way as we check `p->pack_keep`. But\nlifting that restriction is a more invasive change, so I'm happy to\nrely on the contents of existing_kept_packs here in the meantime.\n\n> +\t\t}\n>\n>  \t\tALLOC_GROW(geometry->pack,\n>  \t\t\t   geometry->pack_nr + 1,\n> @@ -353,6 +370,7 @@ static void init_pack_geometry(struct pack_geometry **geometry_p)\n>  \t}\n>\n>  \tQSORT(geometry->pack, geometry->pack_nr, geometry_cmp);\n> +\tstrbuf_release(&buf);\n>  }\n>\n>  static void split_pack_geometry(struct pack_geometry *geometry, int factor)\n> @@ -714,17 +732,20 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n>  \t\tstrbuf_release(&path);\n>  \t}\n>\n> +\tpackdir = mkpathdup(\"%s/pack\", get_object_directory());\n> +\tpacktmp_name = xstrfmt(\".tmp-%d-pack\", (int)getpid());\n> +\tpacktmp = mkpathdup(\"%s/%s\", packdir, packtmp_name);\n> +\n> +\tcollect_pack_filenames(&existing_nonkept_packs, &existing_kept_packs,\n> +\t\t\t       &keep_pack_list);\n> +\n\nMakes sense; we have to initialize existing_kept_packs before arranging\nthe list of packs for the `--geometric` split. And presumably\n`collect_pack_filenames()` relies on `packdir`, `packtmp_name`, and\n`packtmp` being setup ahead of time, too.\n\n>  \tif (geometric_factor) {\n>  \t\tif (pack_everything)\n>  \t\t\tdie(_(\"options '%s' and '%s' cannot be used together\"), \"--geometric\", \"-A/-a\");\n> -\t\tinit_pack_geometry(&geometry);\n> +\t\tinit_pack_geometry(&geometry, &existing_kept_packs);\n>  \t\tsplit_pack_geometry(geometry, geometric_factor);\n>  \t}\n>\n> -\tpackdir = mkpathdup(\"%s/pack\", get_object_directory());\n> -\tpacktmp_name = xstrfmt(\".tmp-%d-pack\", (int)getpid());\n> -\tpacktmp = mkpathdup(\"%s/%s\", packdir, packtmp_name);\n> -\n>  \tsigchain_push_common(remove_pack_on_signal);\n>\n>  \tprepare_pack_objects(&cmd, &po_args);\n> @@ -764,9 +785,6 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n>  \tif (use_delta_islands)\n>  \t\tstrvec_push(&cmd.args, \"--delta-islands\");\n>\n> -\tcollect_pack_filenames(&existing_nonkept_packs, &existing_kept_packs,\n> -\t\t\t       &keep_pack_list);\n> -\n>  \tif (pack_everything & ALL_INTO_ONE) {\n>  \t\trepack_promisor_objects(&po_args, &names);\n>\n> diff --git a/t/t7703-repack-geometric.sh b/t/t7703-repack-geometric.sh\n> index bdbbcbf1eca..f5ac23413d5 100755\n> --- a/t/t7703-repack-geometric.sh\n> +++ b/t/t7703-repack-geometric.sh\n> @@ -180,6 +180,40 @@ test_expect_success '--geometric ignores kept packs' '\n>  \t)\n>  '\n>\n> +test_expect_success '--geometric ignores --keep-pack packs' '\n> +\tgit init geometric &&\n> +\ttest_when_finished \"rm -fr geometric\" &&\n> +\t(\n> +\t\tcd geometric &&\n> +\n> +\t\t# Create two equal-sized packs\n> +\t\ttest_commit kept && # 3 objects\n> +\t\ttest_commit pack && # 3 objects\n> +\n> +\t\tKEPT=$(git pack-objects --revs $objdir/pack/pack <<-EOF\n> +\t\trefs/tags/kept\n> +\t\tEOF\n> +\t\t) &&\n> +\t\tPACK=$(git pack-objects --revs $objdir/pack/pack <<-EOF\n> +\t\trefs/tags/pack\n> +\t\t^refs/tags/kept\n> +\t\tEOF\n> +\t\t) &&\n\nNit; we don't care about the name of $PACK, so it would probably be fine\nto avoid storing the `PACK` variable. We could write these packs with\njust `git repack -d` after each `test_commit` (which would avoid us\nhaving to call `prune-packed`).\n\nDoes it matter which one is kept? I don't think so, since AFAICT the\ncritical bit is that we mark one of the packs being rolled up as a\n`--keep-pack`.\n\n> +\t\t# Prune loose objects that are now packed into PACK and KEEP\n> +\t\tgit prune-packed &&\n> +\n> +\t\tgit repack --geometric 2 -dm --keep-pack=pack-$KEPT.pack >out &&\n> +\n> +\t\t# Packs should not have changed (only one non-kept pack, no\n> +\t\t# loose objects), but midx should now exist.\n> +\t\ttest_i18ngrep \"Nothing new to pack\" out &&\n\nNit; test_i18ngrep here should just be \"grep\".\n\n> +\t\ttest_path_is_file $midx &&\n> +\t\ttest_path_is_file $objdir/pack/pack-$KEPT.pack &&\n> +\t\tgit fsck\n> +\t)\n\n\nThanks,\nTaylor\n"},{"id":"455580","messageId":"bb19159b-6d0c-a683-a58e-95ebdc128627@github.com","threadId":"57904","inReplyTo":"YofJLv8+x5J7yPmf@nand.local","subject":"Re: [PATCH] repack: respect --keep-pack with geometric repack","fromName":"Victoria Dye","fromEmail":"vdye@github.com","sentAt":"2022-05-20T17:27:21Z","receivedAt":"2022-05-20T17:27:27Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"Taylor Blau wrote:\n> Hi Victoria,\n> \n> On Fri, May 20, 2022 at 04:36:12PM +0000, Victoria Dye via GitGitGadget wrote:\n>> From: Victoria Dye <vdye@github.com>\n>>\n>> Update 'repack' to ignore packs named on the command line with the\n>> '--keep-pack' option. Specifically, modify 'init_pack_geometry()' to treat\n>> command line-kept packs the same way it treats packs with an on-disk '.keep'\n>> file (that is, skip the pack and do not include it in the 'geometry'\n>> structure).\n>>\n>> Without this handling, a '--keep-pack' pack would be included in the\n>> 'geometry' structure. If the pack is *before* the geometry split line (with\n>> at least one other pack and/or loose objects present), 'repack' assumes the\n>> pack's contents are \"rolled up\" into another pack via 'pack-objects'.\n>> However, because the internally-invoked 'pack-objects' properly excludes\n>> '--keep-pack' objects, any new pack it creates will not contain the kept\n>> objects. Finally, 'repack' deletes the '--keep-pack' as \"redundant\" (since\n>> it assumes 'pack-objects' created a new pack with its contents), resulting\n>> in possible object loss and repository corruption.\n> \n> Nicely found and explained. Having discussed this fix with you already\n> off-list, this approach (to treat kept packs as excluded from the list\n> of packs in the `geometry` structure regardless of whether they are kept\n> on disk or in-core) makes sense to me.\n> \n> I left a couple of small notes on the patch below, but since I have some\n> patches that deal with a separate issue in the `git repack --geometric`\n> code coming, do you want to combine forces (and I can send a\n> lightly-reworked version of this patch as a part of my series)?\n> \n\nWorks for me! I'm happy with all the suggested changes you noted below\n(moving the 'string_list_sort' and cleaning up the test), so feel free to\ninclude them in your series.\n\nThanks!\n\n>> @@ -332,17 +332,34 @@ static int geometry_cmp(const void *va, const void *vb)\n>>  \treturn 0;\n>>  }\n>>\n>> -static void init_pack_geometry(struct pack_geometry **geometry_p)\n>> +static void init_pack_geometry(struct pack_geometry **geometry_p,\n>> +\t\t\t       struct string_list *existing_kept_packs)\n>>  {\n>>  \tstruct packed_git *p;\n>>  \tstruct pack_geometry *geometry;\n>> +\tstruct strbuf buf = STRBUF_INIT;\n>>\n>>  \t*geometry_p = xcalloc(1, sizeof(struct pack_geometry));\n>>  \tgeometry = *geometry_p;\n>>\n>> +\tstring_list_sort(existing_kept_packs);\n> \n> Would it be worth sorting this as early as in collect_pack_filenames()?\n> For our purposes in this patch, this works as-is, but it may be\n> defensive to try and minimize the time that list has unsorted contents.\n> \n\nI went back and forth on this, eventually settling on this to keep the\n'string_list_sort' as close as possible to where the sorted list is needed.\nI'm still pretty indifferent, though, so moving it to the end of\n'collect_pack_filenames()' is fine with me.\n\n>>  \tfor (p = get_all_packs(the_repository); p; p = p->next) {\n>> -\t\tif (!pack_kept_objects && p->pack_keep)\n>> -\t\t\tcontinue;\n>> +\t\tif (!pack_kept_objects) {\n>> +\t\t\tif (p->pack_keep)\n>> +\t\t\t\tcontinue;\n> \n> (You mentioned this to me off-list, but I'll repeat it here since it\n> wasn't obvious to me on first read): this check for `p->pack_keep` isn't\n> strictly necessary, since any packs that have their `pack_keep` bit set\n> will appear in the `existing_kept_packs` list.\n> \n> But it does give us a fast path to avoid having to check that list, so\n> it's worth checking that bit to avoid a slightly more expensive check\n> where possible.\n> \n>> +\t\t\t/*\n>> +\t\t\t * The pack may be kept via the --keep-pack option;\n>> +\t\t\t * check 'existing_kept_packs' to determine whether to\n>> +\t\t\t * ignore it.\n>> +\t\t\t */\n>> +\t\t\tstrbuf_reset(&buf);\n>> +\t\t\tstrbuf_addstr(&buf, pack_basename(p));\n>> +\t\t\tstrbuf_strip_suffix(&buf, \".pack\");\n>> +\n>> +\t\t\tif (string_list_has_string(existing_kept_packs, buf.buf))\n>> +\t\t\t\tcontinue;\n> \n> It's too bad that we have to do this check at all, and can't rely on the\n> `pack_keep_in_core` in the same way as we check `p->pack_keep`. But\n> lifting that restriction is a more invasive change, so I'm happy to\n> rely on the contents of existing_kept_packs here in the meantime.\n> \n>> +\t\t}\n>>\n>>  \t\tALLOC_GROW(geometry->pack,\n>>  \t\t\t   geometry->pack_nr + 1,\n>> @@ -353,6 +370,7 @@ static void init_pack_geometry(struct pack_geometry **geometry_p)\n>>  \t}\n>>\n>>  \tQSORT(geometry->pack, geometry->pack_nr, geometry_cmp);\n>> +\tstrbuf_release(&buf);\n>>  }\n>>\n>>  static void split_pack_geometry(struct pack_geometry *geometry, int factor)\n>> @@ -714,17 +732,20 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n>>  \t\tstrbuf_release(&path);\n>>  \t}\n>>\n>> +\tpackdir = mkpathdup(\"%s/pack\", get_object_directory());\n>> +\tpacktmp_name = xstrfmt(\".tmp-%d-pack\", (int)getpid());\n>> +\tpacktmp = mkpathdup(\"%s/%s\", packdir, packtmp_name);\n>> +\n>> +\tcollect_pack_filenames(&existing_nonkept_packs, &existing_kept_packs,\n>> +\t\t\t       &keep_pack_list);\n>> +\n> \n> Makes sense; we have to initialize existing_kept_packs before arranging\n> the list of packs for the `--geometric` split. And presumably\n> `collect_pack_filenames()` relies on `packdir`, `packtmp_name`, and\n> `packtmp` being setup ahead of time, too.\n> \n>>  \tif (geometric_factor) {\n>>  \t\tif (pack_everything)\n>>  \t\t\tdie(_(\"options '%s' and '%s' cannot be used together\"), \"--geometric\", \"-A/-a\");\n>> -\t\tinit_pack_geometry(&geometry);\n>> +\t\tinit_pack_geometry(&geometry, &existing_kept_packs);\n>>  \t\tsplit_pack_geometry(geometry, geometric_factor);\n>>  \t}\n>>\n>> -\tpackdir = mkpathdup(\"%s/pack\", get_object_directory());\n>> -\tpacktmp_name = xstrfmt(\".tmp-%d-pack\", (int)getpid());\n>> -\tpacktmp = mkpathdup(\"%s/%s\", packdir, packtmp_name);\n>> -\n>>  \tsigchain_push_common(remove_pack_on_signal);\n>>\n>>  \tprepare_pack_objects(&cmd, &po_args);\n>> @@ -764,9 +785,6 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n>>  \tif (use_delta_islands)\n>>  \t\tstrvec_push(&cmd.args, \"--delta-islands\");\n>>\n>> -\tcollect_pack_filenames(&existing_nonkept_packs, &existing_kept_packs,\n>> -\t\t\t       &keep_pack_list);\n>> -\n>>  \tif (pack_everything & ALL_INTO_ONE) {\n>>  \t\trepack_promisor_objects(&po_args, &names);\n>>\n>> diff --git a/t/t7703-repack-geometric.sh b/t/t7703-repack-geometric.sh\n>> index bdbbcbf1eca..f5ac23413d5 100755\n>> --- a/t/t7703-repack-geometric.sh\n>> +++ b/t/t7703-repack-geometric.sh\n>> @@ -180,6 +180,40 @@ test_expect_success '--geometric ignores kept packs' '\n>>  \t)\n>>  '\n>>\n>> +test_expect_success '--geometric ignores --keep-pack packs' '\n>> +\tgit init geometric &&\n>> +\ttest_when_finished \"rm -fr geometric\" &&\n>> +\t(\n>> +\t\tcd geometric &&\n>> +\n>> +\t\t# Create two equal-sized packs\n>> +\t\ttest_commit kept && # 3 objects\n>> +\t\ttest_commit pack && # 3 objects\n>> +\n>> +\t\tKEPT=$(git pack-objects --revs $objdir/pack/pack <<-EOF\n>> +\t\trefs/tags/kept\n>> +\t\tEOF\n>> +\t\t) &&\n>> +\t\tPACK=$(git pack-objects --revs $objdir/pack/pack <<-EOF\n>> +\t\trefs/tags/pack\n>> +\t\t^refs/tags/kept\n>> +\t\tEOF\n>> +\t\t) &&\n> \n> Nit; we don't care about the name of $PACK, so it would probably be fine\n> to avoid storing the `PACK` variable. We could write these packs with\n> just `git repack -d` after each `test_commit` (which would avoid us\n> having to call `prune-packed`).\n> \n\nMakes sense.\n\n> Does it matter which one is kept? I don't think so, since AFAICT the\n> critical bit is that we mark one of the packs being rolled up as a\n> `--keep-pack`.\n> \n\nCorrect, the two packs in this test are just two same-sized (or, more\ngenerally, non-geometrically progressing) packs with non-overlapping\ncontent.\n\n>> +\t\t# Prune loose objects that are now packed into PACK and KEEP\n>> +\t\tgit prune-packed &&\n>> +\n>> +\t\tgit repack --geometric 2 -dm --keep-pack=pack-$KEPT.pack >out &&\n>> +\n>> +\t\t# Packs should not have changed (only one non-kept pack, no\n>> +\t\t# loose objects), but midx should now exist.\n>> +\t\ttest_i18ngrep \"Nothing new to pack\" out &&\n> \n> Nit; test_i18ngrep here should just be \"grep\".\n> \n\nThanks for pointing this out - I've been a bit unsure of the difference for\na while, but this pushed me to figure out the difference and I found the\nnote in 'test-lib-functions.sh' clarifying that 'test_i18ngrep' is\ndeprecated.\n\n>> +\t\ttest_path_is_file $midx &&\n>> +\t\ttest_path_is_file $objdir/pack/pack-$KEPT.pack &&\n>> +\t\tgit fsck\n>> +\t)\n> \n> \n> Thanks,\n> Taylor\n\n"},{"id":"455619","messageId":"YofmhcBLrni0kY/s@nand.local","threadId":"57904","inReplyTo":"bb19159b-6d0c-a683-a58e-95ebdc128627@github.com","subject":"Re: [PATCH] repack: respect --keep-pack with geometric repack","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-05-20T19:05:41Z","receivedAt":"2022-05-20T19:06:29Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, May 20, 2022 at 10:27:21AM -0700, Victoria Dye wrote:\n> > I left a couple of small notes on the patch below, but since I have some\n> > patches that deal with a separate issue in the `git repack --geometric`\n> > code coming, do you want to combine forces (and I can send a\n> > lightly-reworked version of this patch as a part of my series)?\n>\n> Works for me! I'm happy with all the suggested changes you noted below\n> (moving the 'string_list_sort' and cleaning up the test), so feel free to\n> include them in your series.\n>\n> Thanks!\n\nNo problem; I (re-)sent this patch as 1/3 in my series which should be\navailable via the archive at:\n\n    https://lore.kernel.org/git/cover.1653073280.git.me@ttaylorr.com/T/#t\n\nIt looks like we're on the same page with respect to my suggestions, but\nfeel free to take a look at the reworked version of this patch (and the\nnew ones on top, too) and let me know what you think.\n\n> >> @@ -332,17 +332,34 @@ static int geometry_cmp(const void *va, const void *vb)\n> >>  \treturn 0;\n> >>  }\n> >>\n> >> -static void init_pack_geometry(struct pack_geometry **geometry_p)\n> >> +static void init_pack_geometry(struct pack_geometry **geometry_p,\n> >> +\t\t\t       struct string_list *existing_kept_packs)\n> >>  {\n> >>  \tstruct packed_git *p;\n> >>  \tstruct pack_geometry *geometry;\n> >> +\tstruct strbuf buf = STRBUF_INIT;\n> >>\n> >>  \t*geometry_p = xcalloc(1, sizeof(struct pack_geometry));\n> >>  \tgeometry = *geometry_p;\n> >>\n> >> +\tstring_list_sort(existing_kept_packs);\n> >\n> > Would it be worth sorting this as early as in collect_pack_filenames()?\n> > For our purposes in this patch, this works as-is, but it may be\n> > defensive to try and minimize the time that list has unsorted contents.\n>\n> I went back and forth on this, eventually settling on this to keep the\n> 'string_list_sort' as close as possible to where the sorted list is needed.\n> I'm still pretty indifferent, though, so moving it to the end of\n> 'collect_pack_filenames()' is fine with me.\n\nI'm fine with it either way. I opted to sorting the list in\ncollect_pack_filenames() since I think it's slightly more fool-proof,\nbut I also don't have strong feelings about its placement.\n\nThanks,\nTaylor\n"},{"id":"455630","messageId":"xmqqr14oey2a.fsf@gitster.g","threadId":"57904","inReplyTo":"YofJLv8+x5J7yPmf@nand.local","subject":"Re: [PATCH] repack: respect --keep-pack with geometric repack","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-05-20T20:41:49Z","receivedAt":"2022-05-20T20:42:00Z","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>>  \t*geometry_p = xcalloc(1, sizeof(struct pack_geometry));\n>>  \tgeometry = *geometry_p;\n>>\n>> +\tstring_list_sort(existing_kept_packs);\n>\n> Would it be worth sorting this as early as in collect_pack_filenames()?\n> For our purposes in this patch, this works as-is, but it may be\n> defensive to try and minimize the time that list has unsorted contents.\n\nIt does not matter too much but after reading the latest one before\ncoming back to this original thread, I actually thought that sorting\nit too early was questionable.  Nobody other than this code cares\nabout the list being sorted, and if it turns out that the look-up in\nthe loop need to be optimized, it is plausible that we do not need\nthis list sorted when we populate a hashmap out of its contents, for\nexample.\n\n>>  \tfor (p = get_all_packs(the_repository); p; p = p->next) {\n>> -\t\tif (!pack_kept_objects && p->pack_keep)\n>> -\t\t\tcontinue;\n>> +\t\tif (!pack_kept_objects) {\n>> +\t\t\tif (p->pack_keep)\n>> +\t\t\t\tcontinue;\n>\n> (You mentioned this to me off-list, but I'll repeat it here since it\n> wasn't obvious to me on first read): this check for `p->pack_keep` isn't\n> strictly necessary, since any packs that have their `pack_keep` bit set\n> will appear in the `existing_kept_packs` list.\n>\n> But it does give us a fast path to avoid having to check that list, so\n> it's worth checking that bit to avoid a slightly more expensive check\n> where possible.\n\nThat invites a related but different question.  Doesn't p->pack_keep\nand string_list_has_string(existing_kept_packs, name_of_pack(p))\nmirror each other?  Can a pack appear on the existing_kept_packs\nlist without having .pack_keep bit set and under what condition?\n\nIt is answered by the comment below, I presume?\n\n>> +\t\t\t/*\n>> +\t\t\t * The pack may be kept via the --keep-pack option;\n>> +\t\t\t * check 'existing_kept_packs' to determine whether to\n>> +\t\t\t * ignore it.\n>> +\t\t\t */\n\nOK.  So there are two classes of packs we want to exclude from the\ngeometry repacking.  Those that already have .pack_keep bit set, and\nthose that are _are_ newly making into kept packs that do not yet\nhave .pack_keep bit set.  Makes sense.\n\n"},{"id":"455633","messageId":"xmqqee0ng8fj.fsf@gitster.g","threadId":"57904","inReplyTo":"xmqqr14oey2a.fsf@gitster.g","subject":"Re: [PATCH] repack: respect --keep-pack with geometric repack","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-05-20T22:12:32Z","receivedAt":"2022-05-20T22:12:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n>>> +\t\t\t/*\n>>> +\t\t\t * The pack may be kept via the --keep-pack option;\n>>> +\t\t\t * check 'existing_kept_packs' to determine whether to\n>>> +\t\t\t * ignore it.\n>>> +\t\t\t */\n>\n> OK.  So there are two classes of packs we want to exclude from the\n> geometry repacking.  Those that already have .pack_keep bit set, and\n> those that are _are_ newly making into kept packs that do not yet\n> have .pack_keep bit set.  Makes sense.\n\nAnd with another topic in-flight combined, we have the third class\nthat we would want to exclude here, i.e. the ones that are \"cruft\".\n\n(Noticed while rebuilding 'seen').\n\n"},{"id":"455634","messageId":"YogUP+K8BL67WOPM@nand.local","threadId":"57904","inReplyTo":"xmqqee0ng8fj.fsf@gitster.g","subject":"Re: [PATCH] repack: respect --keep-pack with geometric repack","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-05-20T22:20:47Z","receivedAt":"2022-05-20T22:20:53Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, May 20, 2022 at 03:12:32PM -0700, Junio C Hamano wrote:\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n> >>> +\t\t\t/*\n> >>> +\t\t\t * The pack may be kept via the --keep-pack option;\n> >>> +\t\t\t * check 'existing_kept_packs' to determine whether to\n> >>> +\t\t\t * ignore it.\n> >>> +\t\t\t */\n> >\n> > OK.  So there are two classes of packs we want to exclude from the\n> > geometry repacking.  Those that already have .pack_keep bit set, and\n> > those that are _are_ newly making into kept packs that do not yet\n> > have .pack_keep bit set.  Makes sense.\n>\n> And with another topic in-flight combined, we have the third class\n> that we would want to exclude here, i.e. the ones that are \"cruft\".\n\nIndeed. tb/cruft-packs handles this by:\n\n    if (p->is_cruft)\n      continue;\n\nsince we would never want to roll up the objects in a cruft pack during\ngeometric repacking.\n\nThere is likely a small conflict you had to resolve, but the resolution\non 'seen' looks as-expected to me.\n\nThanks,\nTaylor\n"},{"id":"455659","messageId":"Yogjmn9BcOBc/5ed@nand.local","threadId":"57904","inReplyTo":"xmqqr14oey2a.fsf@gitster.g","subject":"Re: [PATCH] repack: respect --keep-pack with geometric repack","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-05-20T23:26:18Z","receivedAt":"2022-05-20T23:26:28Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, May 20, 2022 at 01:41:49PM -0700, Junio C Hamano wrote:\n> Taylor Blau <me@ttaylorr.com> writes:\n>\n> >>  \t*geometry_p = xcalloc(1, sizeof(struct pack_geometry));\n> >>  \tgeometry = *geometry_p;\n> >>\n> >> +\tstring_list_sort(existing_kept_packs);\n> >\n> > Would it be worth sorting this as early as in collect_pack_filenames()?\n> > For our purposes in this patch, this works as-is, but it may be\n> > defensive to try and minimize the time that list has unsorted contents.\n>\n> It does not matter too much but after reading the latest one before\n> coming back to this original thread, I actually thought that sorting\n> it too early was questionable.  Nobody other than this code cares\n> about the list being sorted, and if it turns out that the look-up in\n> the loop need to be optimized, it is plausible that we do not need\n> this list sorted when we populate a hashmap out of its contents, for\n> example.\n\nMy thinking was simply that sorting it as often as possible (by doing so\nimmediately after appending all of our newly written packs into it) was\nthe most defensive approach that we could take. At least in the sense of\nanother caller which _does_ depend on `names` sorted, and makes the same\nmistake of forgetting to call `string_list_sort()` itself.\n\nI suspect that this is mostly academic anyway, and that either would be\nfine, so if others feel strongly I'm happy to send an updated version\n(though TBH I would be just as happy with what we already have in the\nseries I posted here).\n\n> >>  \tfor (p = get_all_packs(the_repository); p; p = p->next) {\n> >> -\t\tif (!pack_kept_objects && p->pack_keep)\n> >> -\t\t\tcontinue;\n> >> +\t\tif (!pack_kept_objects) {\n> >> +\t\t\tif (p->pack_keep)\n> >> +\t\t\t\tcontinue;\n> >\n> > (You mentioned this to me off-list, but I'll repeat it here since it\n> > wasn't obvious to me on first read): this check for `p->pack_keep` isn't\n> > strictly necessary, since any packs that have their `pack_keep` bit set\n> > will appear in the `existing_kept_packs` list.\n> >\n> > But it does give us a fast path to avoid having to check that list, so\n> > it's worth checking that bit to avoid a slightly more expensive check\n> > where possible.\n>\n> That invites a related but different question.  Doesn't p->pack_keep\n> and string_list_has_string(existing_kept_packs, name_of_pack(p))\n> mirror each other?  Can a pack appear on the existing_kept_packs\n> list without having .pack_keep bit set and under what condition?\n\nUnfortunately not. `repack` is much more keen to manipulate the results\nof calling readdir() over $GIT_DIR/objects/pack than it is to, say, call\n`get_all_packs()` and mark `p->pack_keep_in_core`.\n\nIn a future where `repack` does not interact with the packdir at such a\nlow level, I don't think we would have a use for a function like\n`collect_pack_filenames()` at all. But that is a much larger change than\nthis one ;).\n\n> It is answered by the comment below, I presume?\n>\n> >> +\t\t\t/*\n> >> +\t\t\t * The pack may be kept via the --keep-pack option;\n> >> +\t\t\t * check 'existing_kept_packs' to determine whether to\n> >> +\t\t\t * ignore it.\n> >> +\t\t\t */\n>\n> OK.  So there are two classes of packs we want to exclude from the\n> geometry repacking.  Those that already have .pack_keep bit set, and\n> those that are _are_ newly making into kept packs that do not yet\n> have .pack_keep bit set.  Makes sense.\n\nYep (and also \"yep\" to your note below about adding cruft packs into the\nmix).\n\nThanks,\nTaylor\n"}]}