{"thread":{"id":"57906","subject":"[PATCH 0/3] repack: handle --keep-pack, --max-pack-size for geometric repacks","startedAt":"2022-05-20T19:01:48Z","lastAt":"2022-05-20T23:22:09Z","messageCount":10,"participants":["Taylor Blau","Victoria Dye","Derrick Stolee","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"455615","messageId":"cover.1653073280.git.me@ttaylorr.com","threadId":"57906","inReplyTo":null,"subject":"[PATCH 0/3] repack: handle --keep-pack, --max-pack-size for geometric repacks","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-05-20T19:01:41Z","receivedAt":"2022-05-20T19:01:48Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"This series fixes two issues that Victoria and I noticed while working on an\nunrelated issue yesterday.\n\n  - The first patch comes from Victoria's earlier submission[1], and addresses\n    an issue where packs specified as kept via the `--keep-pack` option could\n    potentially be removed (without rewriting their objects) during a\n    `--geometric` repack.\n\n    The first patch is Victoria's alone, with some minor fixups applied from my\n    review in [2]. It's included in this series since it's related, and avoids\n    any conflicts when merging.\n\n  - The latter two patches are mine, and address an issue where specifying a\n    `--max-pack-size` value during a `--geometric` repack could result in object\n    loss because of a false positive in our \"did we write a pack with this\n    name?\" check (which can occur when the list of packs we wrote isn't sorted).\n\n    The first of these two patches demonstrates the issue (done in a separate\n    patch, since the scenario is quite involved), and the second patch fixes the\n    bug.\n\nThanks in advance for your review.\n\n[1]: https://lore.kernel.org/git/pull.1235.git.1653064572170.gitgitgadget@gmail.com/\n[2]: https://lore.kernel.org/git/YofJLv8+x5J7yPmf@nand.local/\n\nTaylor Blau (2):\n  t7703: demonstrate object corruption with pack.packSizeLimit\n  builtin/repack.c: ensure that `names` is sorted\n\nVictoria Dye (1):\n  repack: respect --keep-pack with geometric repack\n\n builtin/repack.c            | 49 ++++++++++++++++++------\n t/t7703-repack-geometric.sh | 75 +++++++++++++++++++++++++++++++++++++\n 2 files changed, 112 insertions(+), 12 deletions(-)\n\n-- \n2.36.1.94.gb0d54bedca\n"},{"id":"455616","messageId":"035d1a40ce73c3a556367c992286d4e631d6f4ef.1653073280.git.me@ttaylorr.com","threadId":"57906","inReplyTo":"cover.1653073280.git.me@ttaylorr.com","subject":"[PATCH 1/3] repack: respect --keep-pack with geometric repack","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-05-20T19:01:45Z","receivedAt":"2022-05-20T19:01:55Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?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>\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n builtin/repack.c            | 46 ++++++++++++++++++++++++++++---------\n t/t7703-repack-geometric.sh | 28 ++++++++++++++++++++++\n 2 files changed, 63 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex d1a563d5b6..ea56e92ad5 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -137,6 +137,8 @@ static void collect_pack_filenames(struct string_list *fname_nonkept_list,\n \t\t\tstring_list_append_nodup(fname_nonkept_list, fname);\n \t}\n \tclosedir(dir);\n+\n+\tstring_list_sort(fname_kept_list);\n }\n \n static void remove_redundant_pack(const char *dir_name, const char *base_name)\n@@ -332,17 +334,38 @@ 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 \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\t/*\n+\t\t\t * Any pack that has its pack_keep bit set will appear\n+\t\t\t * in existing_kept_packs below, but this saves us from\n+\t\t\t * doing a more expensive check.\n+\t\t\t */\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 +376,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 +738,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 +791,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 bdbbcbf1ec..91bb2b37a8 100755\n--- a/t/t7703-repack-geometric.sh\n+++ b/t/t7703-repack-geometric.sh\n@@ -180,6 +180,34 @@ 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\tgit repack -d &&\n+\t\ttest_commit pack && # 3 objects\n+\t\tgit repack -d &&\n+\n+\t\tfind $objdir/pack -type f -name \"*.pack\" | sort >packs.before &&\n+\t\tgit repack --geometric 2 -dm \\\n+\t\t\t--keep-pack=\"$(basename \"$(head -n 1 packs.before)\")\" >out &&\n+\t\tfind $objdir/pack -type f -name \"*.pack\" | sort >packs.after &&\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\tgrep \"Nothing new to pack\" out &&\n+\t\ttest_path_is_file $midx &&\n+\n+\t\ttest_cmp packs.before packs.after &&\n+\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-- \n2.36.1.94.gb0d54bedca\n\n"},{"id":"455617","messageId":"08da02fa74c211ae1019cb0a9f4e30cc239e1ab9.1653073280.git.me@ttaylorr.com","threadId":"57906","inReplyTo":"cover.1653073280.git.me@ttaylorr.com","subject":"[PATCH 2/3] t7703: demonstrate object corruption with pack.packSizeLimit","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-05-20T19:01:48Z","receivedAt":"2022-05-20T19:01:58Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"When doing a `--geometric=<d>` repack, `git repack` determines a\nsplitting point among packs ordered by their object count such that:\n\n  - each pack above the split has at least `<d>` times as many objects\n    as the next-largest pack by object count, and\n  - the first pack above the split has at least `<d>` times as many\n    object as the sum of all packs below the split line combined\n\n`git repack` then creates a pack containing all of the objects contained\nin packs below the split line by running `git pack-objects\n--stdin-packs` underneath. Once packs are moved into place, then any\npacks below the split line are removed, since their objects were just\ncombined into a new pack.\n\nBut `git repack` tries to be careful to avoid removing a pack that it\njust wrote, by checking:\n\n    struct packed_git *p = geometry->pack[i];\n    if (string_list_has_string(&names, hash_to_hex(p->hash)))\n      continue;\n\nin the `delete_redundant` and `geometric` conditional towards the end of\n`cmd_repack`.\n\nBut it's possible to trick `git repack` into not recognizing a pack that\nit just wrote when `names` is out-of-order (which violates\n`string_list_has_string()`'s assumption that the list is sorted and thus\nbinary search-able).\n\nWhen this happens in just the right circumstances, it is possible to\nremove a pack that we just wrote, leading to object corruption.\n\nLuckily, this is quite difficult to provoke in practice (for a couple of\nreasons):\n\n  - we ordinarily write just one pack, so `names` usually contains just\n    one entry, and is thus sorted\n  - when we do write more than one pack (e.g., due to `--max-pack-size`)\n    we have to: (a) write a pack identical to one that already\n    exists, (b) have that pack be below the split line, and (c) have\n    the set of packs written by `pack-objects` occur in an order which\n    tricks `string_list_has_string()`.\n\nDemonstrate the above scenario in a failing test, which causes `git\nrepack --geometric` to write a pack which occurs below the split line,\n_and_ fail to recognize that it wrote that pack.\n\nThe following patch will fix this bug.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n t/t7703-repack-geometric.sh | 47 +++++++++++++++++++++++++++++++++++++\n 1 file changed, 47 insertions(+)\n\ndiff --git a/t/t7703-repack-geometric.sh b/t/t7703-repack-geometric.sh\nindex 91bb2b37a8..2cd1de7295 100755\n--- a/t/t7703-repack-geometric.sh\n+++ b/t/t7703-repack-geometric.sh\n@@ -7,6 +7,7 @@ test_description='git repack --geometric works correctly'\n GIT_TEST_MULTI_PACK_INDEX=0\n \n objdir=.git/objects\n+packdir=$objdir/pack\n midx=$objdir/pack/multi-pack-index\n \n test_expect_success '--geometric with no packs' '\n@@ -230,4 +231,50 @@ test_expect_success '--geometric chooses largest MIDX preferred pack' '\n \t)\n '\n \n+test_expect_failure '--geometric with pack.packSizeLimit' '\n+\tgit init pack-rewrite &&\n+\ttest_when_finished \"rm -fr pack-rewrite\" &&\n+\t(\n+\t\tcd pack-rewrite &&\n+\n+\t\ttest-tool genrandom foo 1048576 >foo &&\n+\t\ttest-tool genrandom bar 1048576 >bar &&\n+\n+\t\tgit add foo bar &&\n+\t\ttest_tick &&\n+\t\tgit commit -m base &&\n+\n+\t\tgit rev-parse HEAD:foo HEAD:bar >p1.objects &&\n+\t\tgit rev-parse HEAD HEAD^{tree} >p2.objects &&\n+\n+\t\t# These two packs each contain two objects, so the following\n+\t\t# `--geometric` repack will try to combine them.\n+\t\tp1=\"$(git pack-objects $packdir/pack <p1.objects)\" &&\n+\t\tp2=\"$(git pack-objects $packdir/pack <p2.objects)\" &&\n+\n+\t\t# Remove any loose objects in packs, since we do not want extra\n+\t\t# copies around (which would mask over potential object\n+\t\t# corruption issues).\n+\t\tgit prune-packed &&\n+\n+\t\t# Both p1 and p2 will be rolled up, but pack-objects will write\n+\t\t# three packs:\n+\t\t#\n+\t\t#   - one containing object \"foo\",\n+\t\t#   - another containing object \"bar\",\n+\t\t#   - a final pack containing the commit and tree objects\n+\t\t#     (identical to p2 above)\n+\t\tgit repack --geometric 2 -d --max-pack-size=1048576 &&\n+\n+\t\t# Ensure `repack` can detect that the third pack it wrote\n+\t\t# (containing just the tree and commit objects) was identical to\n+\t\t# one that was below the geometric split, so that we can save it\n+\t\t# from deletion.\n+\t\t#\n+\t\t# If `repack` fails to do that, we will incorrectly delete p2,\n+\t\t# causing object corruption.\n+\t\tgit fsck\n+\t)\n+'\n+\n test_done\n-- \n2.36.1.94.gb0d54bedca\n\n"},{"id":"455618","messageId":"a3d25b9e5dda9a999597115933bf0ce70dce51b2.1653073280.git.me@ttaylorr.com","threadId":"57906","inReplyTo":"cover.1653073280.git.me@ttaylorr.com","subject":"[PATCH 3/3] builtin/repack.c: ensure that `names` is sorted","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-05-20T19:01:51Z","receivedAt":"2022-05-20T19:02:08Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"The previous patch demonstrates a scenario where the list of packs\nwritten by `pack-objects` (and stored in the `names` string_list) is\nout-of-order, and can thus cause us to delete packs we shouldn't.\n\nThis patch resolves that bug by ensuring that `names` is sorted in all\ncases, not just when\n\n    delete_redundant && pack_everything & ALL_INTO_ONE\n\nis true.\n\nBecause we did sort `names` in that case (which, prior to `--geometric`\nrepacks, was the only time we would actually delete packs, this is only\na bug for `--geometric` repacks.\n\nIt would be sufficient to only sort `names` when `delete_redundant` is\nset to a non-zero value. But sorting a small list of strings is cheap,\nand it is defensive against future calls to `string_list_has_string()`\non this list.\n\nCo-discovered-by: Victoria Dye <vdye@github.com>\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n builtin/repack.c            | 3 ++-\n t/t7703-repack-geometric.sh | 2 +-\n 2 files changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex ea56e92ad5..0e4aae80c0 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -856,6 +856,8 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \tif (!names.nr && !po_args.quiet)\n \t\tprintf_ln(_(\"Nothing new to pack.\"));\n \n+\tstring_list_sort(&names);\n+\n \tfor_each_string_list_item(item, &names) {\n \t\titem->util = (void *)(uintptr_t)populate_pack_exts(item->string);\n \t}\n@@ -896,7 +898,6 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \n \tif (delete_redundant && pack_everything & ALL_INTO_ONE) {\n \t\tconst int hexsz = the_hash_algo->hexsz;\n-\t\tstring_list_sort(&names);\n \t\tfor_each_string_list_item(item, &existing_nonkept_packs) {\n \t\t\tchar *sha1;\n \t\t\tsize_t len = strlen(item->string);\ndiff --git a/t/t7703-repack-geometric.sh b/t/t7703-repack-geometric.sh\nindex 2cd1de7295..da87f8b2d8 100755\n--- a/t/t7703-repack-geometric.sh\n+++ b/t/t7703-repack-geometric.sh\n@@ -231,7 +231,7 @@ test_expect_success '--geometric chooses largest MIDX preferred pack' '\n \t)\n '\n \n-test_expect_failure '--geometric with pack.packSizeLimit' '\n+test_expect_success '--geometric with pack.packSizeLimit' '\n \tgit init pack-rewrite &&\n \ttest_when_finished \"rm -fr pack-rewrite\" &&\n \t(\n-- \n2.36.1.94.gb0d54bedca\n"},{"id":"455624","messageId":"a060d672-df82-95a3-072a-7ab7b0566556@github.com","threadId":"57906","inReplyTo":"08da02fa74c211ae1019cb0a9f4e30cc239e1ab9.1653073280.git.me@ttaylorr.com","subject":"Re: [PATCH 2/3] t7703: demonstrate object corruption with pack.packSizeLimit","fromName":"Victoria Dye","fromEmail":"vdye@github.com","sentAt":"2022-05-20T19:42:58Z","receivedAt":"2022-05-20T19:43:03Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"Taylor Blau wrote:\n> When doing a `--geometric=<d>` repack, `git repack` determines a\n> splitting point among packs ordered by their object count such that:\n> \n>   - each pack above the split has at least `<d>` times as many objects\n>     as the next-largest pack by object count, and\n>   - the first pack above the split has at least `<d>` times as many\n>     object as the sum of all packs below the split line combined\n> \n> `git repack` then creates a pack containing all of the objects contained\n> in packs below the split line by running `git pack-objects\n> --stdin-packs` underneath. Once packs are moved into place, then any\n> packs below the split line are removed, since their objects were just\n> combined into a new pack.\n> \n> But `git repack` tries to be careful to avoid removing a pack that it\n> just wrote, by checking:\n> \n>     struct packed_git *p = geometry->pack[i];\n>     if (string_list_has_string(&names, hash_to_hex(p->hash)))\n>       continue;\n> \n> in the `delete_redundant` and `geometric` conditional towards the end of\n> `cmd_repack`.\n> \n> But it's possible to trick `git repack` into not recognizing a pack that\n> it just wrote when `names` is out-of-order (which violates\n> `string_list_has_string()`'s assumption that the list is sorted and thus\n> binary search-able).\n> \n> When this happens in just the right circumstances, it is possible to\n> remove a pack that we just wrote, leading to object corruption.\n> \n> Luckily, this is quite difficult to provoke in practice (for a couple of\n> reasons):\n> \n>   - we ordinarily write just one pack, so `names` usually contains just\n>     one entry, and is thus sorted\n>   - when we do write more than one pack (e.g., due to `--max-pack-size`)\n>     we have to: (a) write a pack identical to one that already\n>     exists, (b) have that pack be below the split line, and (c) have\n>     the set of packs written by `pack-objects` occur in an order which\n>     tricks `string_list_has_string()`.\n> \n> Demonstrate the above scenario in a failing test, which causes `git\n> repack --geometric` to write a pack which occurs below the split line,\n> _and_ fail to recognize that it wrote that pack.\n> \n> The following patch will fix this bug.\n> \n> Signed-off-by: Taylor Blau <me@ttaylorr.com>\n> ---\n>  t/t7703-repack-geometric.sh | 47 +++++++++++++++++++++++++++++++++++++\n>  1 file changed, 47 insertions(+)\n> \n> diff --git a/t/t7703-repack-geometric.sh b/t/t7703-repack-geometric.sh\n> index 91bb2b37a8..2cd1de7295 100755\n> --- a/t/t7703-repack-geometric.sh\n> +++ b/t/t7703-repack-geometric.sh\n> @@ -7,6 +7,7 @@ test_description='git repack --geometric works correctly'\n>  GIT_TEST_MULTI_PACK_INDEX=0\n>  \n>  objdir=.git/objects\n> +packdir=$objdir/pack\n>  midx=$objdir/pack/multi-pack-index\n>  \n>  test_expect_success '--geometric with no packs' '\n> @@ -230,4 +231,50 @@ test_expect_success '--geometric chooses largest MIDX preferred pack' '\n>  \t)\n>  '\n>  \n> +test_expect_failure '--geometric with pack.packSizeLimit' '\n> +\tgit init pack-rewrite &&\n> +\ttest_when_finished \"rm -fr pack-rewrite\" &&\n> +\t(\n> +\t\tcd pack-rewrite &&\n> +\n> +\t\ttest-tool genrandom foo 1048576 >foo &&\n> +\t\ttest-tool genrandom bar 1048576 >bar &&\n> +\n\nI was a bit worried about this test being flaky in the future (relying on\nparticular pseudorandomly-generated file contents and the subsequent\nordering of hashes on the packs). But, since neither 'genrandom' nor the\npack hash generation seem likely to change (and I can't come up with an\nalternative to this approach anyway), the test looks good as-is. \n\n> +\t\tgit add foo bar &&\n> +\t\ttest_tick &&\n> +\t\tgit commit -m base &&\n> +\n> +\t\tgit rev-parse HEAD:foo HEAD:bar >p1.objects &&\n> +\t\tgit rev-parse HEAD HEAD^{tree} >p2.objects &&\n> +\n> +\t\t# These two packs each contain two objects, so the following\n> +\t\t# `--geometric` repack will try to combine them.\n> +\t\tp1=\"$(git pack-objects $packdir/pack <p1.objects)\" &&\n> +\t\tp2=\"$(git pack-objects $packdir/pack <p2.objects)\" &&\n> +\n> +\t\t# Remove any loose objects in packs, since we do not want extra\n> +\t\t# copies around (which would mask over potential object\n> +\t\t# corruption issues).\n> +\t\tgit prune-packed &&\n> +\n> +\t\t# Both p1 and p2 will be rolled up, but pack-objects will write\n> +\t\t# three packs:\n> +\t\t#\n> +\t\t#   - one containing object \"foo\",\n> +\t\t#   - another containing object \"bar\",\n> +\t\t#   - a final pack containing the commit and tree objects\n> +\t\t#     (identical to p2 above)\n> +\t\tgit repack --geometric 2 -d --max-pack-size=1048576 &&\n> +\n> +\t\t# Ensure `repack` can detect that the third pack it wrote\n> +\t\t# (containing just the tree and commit objects) was identical to\n> +\t\t# one that was below the geometric split, so that we can save it\n> +\t\t# from deletion.\n> +\t\t#\n> +\t\t# If `repack` fails to do that, we will incorrectly delete p2,\n> +\t\t# causing object corruption.\n> +\t\tgit fsck\n> +\t)\n> +'\n> +\n>  test_done\n\n"},{"id":"455626","messageId":"5517f0f8-871c-a155-7d8c-d0d6086bb7d2@github.com","threadId":"57906","inReplyTo":"cover.1653073280.git.me@ttaylorr.com","subject":"Re: [PATCH 0/3] repack: handle --keep-pack, --max-pack-size for geometric repacks","fromName":"Victoria Dye","fromEmail":"vdye@github.com","sentAt":"2022-05-20T19:46:12Z","receivedAt":"2022-05-20T19:46:20Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"Taylor Blau wrote:\n> This series fixes two issues that Victoria and I noticed while working on an\n> unrelated issue yesterday.\n> \n>   - The first patch comes from Victoria's earlier submission[1], and addresses\n>     an issue where packs specified as kept via the `--keep-pack` option could\n>     potentially be removed (without rewriting their objects) during a\n>     `--geometric` repack.\n> \n>     The first patch is Victoria's alone, with some minor fixups applied from my\n>     review in [2]. It's included in this series since it's related, and avoids\n>     any conflicts when merging.\n> \n\nI'm happy with the fixes you applied here and don't have anything else I'd\nlike to add this patch.\n\n>   - The latter two patches are mine, and address an issue where specifying a\n>     `--max-pack-size` value during a `--geometric` repack could result in object\n>     loss because of a false positive in our \"did we write a pack with this\n>     name?\" check (which can occur when the list of packs we wrote isn't sorted).\n> \n>     The first of these two patches demonstrates the issue (done in a separate\n>     patch, since the scenario is quite involved), and the second patch fixes the\n>     bug.\n> \n\nI was worried about the robustness of the test, but some deeper diving\nrevealed that it should produce consistent results. Otherwise, the fix\nitself is a straightforward (albeit hard to find in the first place). These\ntwo patches look good to me!\n\n> Thanks in advance for your review.\n> \n> [1]: https://lore.kernel.org/git/pull.1235.git.1653064572170.gitgitgadget@gmail.com/\n> [2]: https://lore.kernel.org/git/YofJLv8+x5J7yPmf@nand.local/\n> \n> Taylor Blau (2):\n>   t7703: demonstrate object corruption with pack.packSizeLimit\n>   builtin/repack.c: ensure that `names` is sorted\n> \n> Victoria Dye (1):\n>   repack: respect --keep-pack with geometric repack\n> \n>  builtin/repack.c            | 49 ++++++++++++++++++------\n>  t/t7703-repack-geometric.sh | 75 +++++++++++++++++++++++++++++++++++++\n>  2 files changed, 112 insertions(+), 12 deletions(-)\n> \n\n"},{"id":"455629","messageId":"b151f2bb-bdc7-e004-f1d4-e1a83c2b73db@github.com","threadId":"57906","inReplyTo":"5517f0f8-871c-a155-7d8c-d0d6086bb7d2@github.com","subject":"Re: [PATCH 0/3] repack: handle --keep-pack, --max-pack-size for geometric repacks","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-05-20T20:05:29Z","receivedAt":"2022-05-20T20:05:36Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 5/20/2022 3:46 PM, Victoria Dye wrote:\n> Taylor Blau wrote:\n>> This series fixes two issues that Victoria and I noticed while working on an\n>> unrelated issue yesterday.\n>>\n>>   - The first patch comes from Victoria's earlier submission[1], and addresses\n>>     an issue where packs specified as kept via the `--keep-pack` option could\n>>     potentially be removed (without rewriting their objects) during a\n>>     `--geometric` repack.\n>>\n>>     The first patch is Victoria's alone, with some minor fixups applied from my\n>>     review in [2]. It's included in this series since it's related, and avoids\n>>     any conflicts when merging.\n>>\n> \n> I'm happy with the fixes you applied here and don't have anything else I'd\n> like to add this patch.\n> \n>>   - The latter two patches are mine, and address an issue where specifying a\n>>     `--max-pack-size` value during a `--geometric` repack could result in object\n>>     loss because of a false positive in our \"did we write a pack with this\n>>     name?\" check (which can occur when the list of packs we wrote isn't sorted).\n>>\n>>     The first of these two patches demonstrates the issue (done in a separate\n>>     patch, since the scenario is quite involved), and the second patch fixes the\n>>     bug.\n>>\n> \n> I was worried about the robustness of the test, but some deeper diving\n> revealed that it should produce consistent results. Otherwise, the fix\n> itself is a straightforward (albeit hard to find in the first place). These\n> two patches look good to me!\n> \n>> Thanks in advance for your review.\n\nI'm chiming in to say that I also read these patches and think they\nare good. Couldn't find a way to improve them.\n\nThanks,\n-Stolee\n"},{"id":"455631","messageId":"xmqqmtfbgc1f.fsf@gitster.g","threadId":"57906","inReplyTo":"08da02fa74c211ae1019cb0a9f4e30cc239e1ab9.1653073280.git.me@ttaylorr.com","subject":"Re: [PATCH 2/3] t7703: demonstrate object corruption with pack.packSizeLimit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-05-20T20:54:36Z","receivedAt":"2022-05-20T20:54:47Z","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> When this happens in just the right circumstances, it is possible to\n> remove a pack that we just wrote, leading to object corruption.\n>\n> Luckily, this is quite difficult to provoke in practice (for a couple of\n> reasons):\n\nI'd call it unlucky that it is hard to trigger, actually ;-) With a\nsystem like Git with more than a few handful of users, even an issue\nthat is hard-to-trigger is bound to hit somebody every day, but it\nit is hard to trigger without the right condition, it is hard to\ndebug.\n\nThanks for finding and fixing (presumably we need a call to keep the\nlist sorted at the right places?)\n\n>   - we ordinarily write just one pack, so `names` usually contains just\n>     one entry, and is thus sorted\n>   - when we do write more than one pack (e.g., due to `--max-pack-size`)\n>     we have to: (a) write a pack identical to one that already\n>     exists, (b) have that pack be below the split line, and (c) have\n>     the set of packs written by `pack-objects` occur in an order which\n>     tricks `string_list_has_string()`.\n>\n> Demonstrate the above scenario in a failing test, which causes `git\n> repack --geometric` to write a pack which occurs below the split line,\n> _and_ fail to recognize that it wrote that pack.\n>\n> The following patch will fix this bug.\n>\n> Signed-off-by: Taylor Blau <me@ttaylorr.com>\n> ---\n>  t/t7703-repack-geometric.sh | 47 +++++++++++++++++++++++++++++++++++++\n>  1 file changed, 47 insertions(+)\n>\n> diff --git a/t/t7703-repack-geometric.sh b/t/t7703-repack-geometric.sh\n> index 91bb2b37a8..2cd1de7295 100755\n> --- a/t/t7703-repack-geometric.sh\n> +++ b/t/t7703-repack-geometric.sh\n> @@ -7,6 +7,7 @@ test_description='git repack --geometric works correctly'\n>  GIT_TEST_MULTI_PACK_INDEX=0\n>  \n>  objdir=.git/objects\n> +packdir=$objdir/pack\n>  midx=$objdir/pack/multi-pack-index\n>  \n>  test_expect_success '--geometric with no packs' '\n> @@ -230,4 +231,50 @@ test_expect_success '--geometric chooses largest MIDX preferred pack' '\n>  \t)\n>  '\n>  \n> +test_expect_failure '--geometric with pack.packSizeLimit' '\n> +\tgit init pack-rewrite &&\n> +\ttest_when_finished \"rm -fr pack-rewrite\" &&\n> +\t(\n> +\t\tcd pack-rewrite &&\n> +\n> +\t\ttest-tool genrandom foo 1048576 >foo &&\n> +\t\ttest-tool genrandom bar 1048576 >bar &&\n> +\n> +\t\tgit add foo bar &&\n> +\t\ttest_tick &&\n> +\t\tgit commit -m base &&\n> +\n> +\t\tgit rev-parse HEAD:foo HEAD:bar >p1.objects &&\n> +\t\tgit rev-parse HEAD HEAD^{tree} >p2.objects &&\n> +\n> +\t\t# These two packs each contain two objects, so the following\n> +\t\t# `--geometric` repack will try to combine them.\n> +\t\tp1=\"$(git pack-objects $packdir/pack <p1.objects)\" &&\n> +\t\tp2=\"$(git pack-objects $packdir/pack <p2.objects)\" &&\n> +\n> +\t\t# Remove any loose objects in packs, since we do not want extra\n> +\t\t# copies around (which would mask over potential object\n> +\t\t# corruption issues).\n> +\t\tgit prune-packed &&\n> +\n> +\t\t# Both p1 and p2 will be rolled up, but pack-objects will write\n> +\t\t# three packs:\n> +\t\t#\n> +\t\t#   - one containing object \"foo\",\n> +\t\t#   - another containing object \"bar\",\n> +\t\t#   - a final pack containing the commit and tree objects\n> +\t\t#     (identical to p2 above)\n> +\t\tgit repack --geometric 2 -d --max-pack-size=1048576 &&\n> +\n> +\t\t# Ensure `repack` can detect that the third pack it wrote\n> +\t\t# (containing just the tree and commit objects) was identical to\n> +\t\t# one that was below the geometric split, so that we can save it\n> +\t\t# from deletion.\n> +\t\t#\n> +\t\t# If `repack` fails to do that, we will incorrectly delete p2,\n> +\t\t# causing object corruption.\n> +\t\tgit fsck\n> +\t)\n> +'\n> +\n>  test_done\n"},{"id":"455632","messageId":"xmqqilpzgbz7.fsf@gitster.g","threadId":"57906","inReplyTo":"b151f2bb-bdc7-e004-f1d4-e1a83c2b73db@github.com","subject":"Re: [PATCH 0/3] repack: handle --keep-pack, --max-pack-size for geometric repacks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-05-20T20:55:56Z","receivedAt":"2022-05-20T20:56:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee <derrickstolee@github.com> writes:\n\n>> I was worried about the robustness of the test, but some deeper diving\n>> revealed that it should produce consistent results. Otherwise, the fix\n>> itself is a straightforward (albeit hard to find in the first place). These\n>> two patches look good to me!\n>> \n>>> Thanks in advance for your review.\n>\n> I'm chiming in to say that I also read these patches and think they\n> are good. Couldn't find a way to improve them.\n\nThanks, all.  Queued.\n"},{"id":"455658","messageId":"YogimAfbnkxCG8IP@nand.local","threadId":"57906","inReplyTo":"a060d672-df82-95a3-072a-7ab7b0566556@github.com","subject":"Re: [PATCH 2/3] t7703: demonstrate object corruption with pack.packSizeLimit","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-05-20T23:22:00Z","receivedAt":"2022-05-20T23:22:09Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, May 20, 2022 at 12:42:58PM -0700, Victoria Dye wrote:\n> > @@ -230,4 +231,50 @@ test_expect_success '--geometric chooses largest MIDX preferred pack' '\n> >  \t)\n> >  '\n> >\n> > +test_expect_failure '--geometric with pack.packSizeLimit' '\n> > +\tgit init pack-rewrite &&\n> > +\ttest_when_finished \"rm -fr pack-rewrite\" &&\n> > +\t(\n> > +\t\tcd pack-rewrite &&\n> > +\n> > +\t\ttest-tool genrandom foo 1048576 >foo &&\n> > +\t\ttest-tool genrandom bar 1048576 >bar &&\n> > +\n>\n> I was a bit worried about this test being flaky in the future (relying on\n> particular pseudorandomly-generated file contents and the subsequent\n> ordering of hashes on the packs). But, since neither 'genrandom' nor the\n> pack hash generation seem likely to change (and I can't come up with an\n> alternative to this approach anyway), the test looks good as-is.\n\nNote that the \"random\" contents aren't so random (though I suspect\nyou're talking about _how_ genrandom interprets the seed changing), and\nthat we're really only depending on genrandom here to create a large\namount of data.\n\nWe are relying on the pack hashes appearing in a certain order, so in\nthat sense this test could \"break\" even if pack-objects reported the\npacks it wrote in a different order.\n\nBut I agree in the sense that I also cannot come up with a less brittle\napproach for writing this test ;).\n\nThanks,\nTaylor\n"}]}