{"thread":{"id":"58085","subject":"[RFC PATCH 0/4] move pruned objects to a separate repository","startedAt":"2022-06-29T18:46:15Z","lastAt":"2022-06-30T21:15:10Z","messageCount":9,"participants":["Taylor Blau","Jonathan Tan","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"458115","messageId":"cover.1656528343.git.me@ttaylorr.com","threadId":"58085","inReplyTo":null,"subject":"[RFC PATCH 0/4] move pruned objects to a separate repository","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-06-29T18:45:49Z","receivedAt":"2022-06-29T18:46:15Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Now that cruft packs are available in v2.37.0, here is an interesting\napplication of that new feature to enable a two-phase object pruning\napproach.\n\nThis came out of a discussion within GitHub about ways we could support\nstoring a set of pruned objects in \"limbo\" so that they were not\naccessible from the repository which pruned them, but instead stored in\na cruft pack in a separate repository which lists the original one as an\nalternate.\n\nThis makes it possible to take the collection of all pruned objects and\nstore them in a cruft pack in a separate repository. This repository\n(which I have been referring to as the \"expired.git\") can then be used\nas a donor repository for any missing objects (like the ones described\nby the race in [1]).\n\nThe first few patches are preparatory. The final one implements writing\nthe pruned objects separately. The trick is to write another cruft pack\nto a separate repository, with two tweaks:\n\n  - the `--cruft-expiration` value is set to \"never\", since we want to\n    keep around all of the objects we expired in the previous step, and\n\n  - the original cruft pack appears as a pack that we are going to keep,\n    meaning all unreachable objects that are stored in the original\n    cruft pack are excluded from the one we write to the \"expired.git\"\n    repository.\n\nYou can try this out yourself by doing something like:\n\n    $ git init --bare ../expired.git $ git repack --cruft\n    --cruft-expiration=1.day.ago -d \\\n    --expire-to=../expired.git/objects/pack/pack\n\nwhich will create two cruft packs:\n\n  - one in the repository which ran `git repack` containing all\n    unreachable objects written within the last day, and\n  - another in the \"expired.git\" repository which contains all\n    unreachable objects written prior to the last day\n\nThis series is an RFC for now since I'm interested in discussing whether\nor not this is a feature that people would actually want to use or not.\nBut if it is, I'm happy to polish this up and turn it into a\nnon-RFC-quality series ;-).\n\nIn the meantime, thanks for your review!\n\n[1]: https://lore.kernel.org/git/YryF+vkosJOXf+mQ@nand.local/\n\nTaylor Blau (4):\n  builtin/repack.c: pass \"out\" to `prepare_pack_objects`\n  builtin/repack.c: pass \"cruft_expiration\" to `write_cruft_pack`\n  builtin/repack.c: write cruft packs to arbitrary locations\n  builtin/repack.c: implement `--expire-to` for storing pruned objects\n\n Documentation/git-repack.txt |   6 ++\n builtin/repack.c             |  67 ++++++++++++++++---\n t/t7700-repack.sh            | 121 +++++++++++++++++++++++++++++++++++\n 3 files changed, 186 insertions(+), 8 deletions(-)\n\n-- \n2.37.0.1.g1379af2e9d\n"},{"id":"458116","messageId":"eff341cce1f037625c247ac5f168008c8676f5f8.1656528343.git.me@ttaylorr.com","threadId":"58085","inReplyTo":"cover.1656528343.git.me@ttaylorr.com","subject":"[RFC PATCH 1/4] builtin/repack.c: pass \"out\" to `prepare_pack_objects`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-06-29T18:45:54Z","receivedAt":"2022-06-29T18:46:24Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"`builtin/repack.c`'s `prepare_pack_objects()` is used to prepare a set\nof arguments to a `pack-objects` process which will generate a desired\npack.\n\nA future patch will add an `--expire-to` option which allows `git\nrepack` to write a cruft pack containing the pruned objects out to a\nseparate repository. Prepare for this by teaching that function to write\npacks to an arbitrary location specified by the caller.\n\nAll existing callers of `prepare_pack_objects()` will pass `packtmp` for\n`out`, retaining the existing behavior.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n builtin/repack.c | 11 ++++++-----\n 1 file changed, 6 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 4a7ae4cf48..025882a075 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -188,7 +188,8 @@ static void remove_redundant_pack(const char *dir_name, const char *base_name)\n }\n \n static void prepare_pack_objects(struct child_process *cmd,\n-\t\t\t\t const struct pack_objects_args *args)\n+\t\t\t\t const struct pack_objects_args *args,\n+\t\t\t\t const char *out)\n {\n \tstrvec_push(&cmd->args, \"pack-objects\");\n \tif (args->window)\n@@ -211,7 +212,7 @@ static void prepare_pack_objects(struct child_process *cmd,\n \t\tstrvec_push(&cmd->args,  \"--quiet\");\n \tif (delta_base_offset)\n \t\tstrvec_push(&cmd->args,  \"--delta-base-offset\");\n-\tstrvec_push(&cmd->args, packtmp);\n+\tstrvec_push(&cmd->args, out);\n \tcmd->git_cmd = 1;\n \tcmd->out = -1;\n }\n@@ -275,7 +276,7 @@ static void repack_promisor_objects(const struct pack_objects_args *args,\n \tFILE *out;\n \tstruct strbuf line = STRBUF_INIT;\n \n-\tprepare_pack_objects(&cmd, args);\n+\tprepare_pack_objects(&cmd, args, packtmp);\n \tcmd.in = -1;\n \n \t/*\n@@ -673,7 +674,7 @@ static int write_cruft_pack(const struct pack_objects_args *args,\n \tFILE *in, *out;\n \tint ret;\n \n-\tprepare_pack_objects(&cmd, args);\n+\tprepare_pack_objects(&cmd, args, packtmp);\n \n \tstrvec_push(&cmd.args, \"--cruft\");\n \tif (cruft_expiration)\n@@ -862,7 +863,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \n \tsigchain_push_common(remove_pack_on_signal);\n \n-\tprepare_pack_objects(&cmd, &po_args);\n+\tprepare_pack_objects(&cmd, &po_args, packtmp);\n \n \tshow_progress = !po_args.quiet && isatty(2);\n \n-- \n2.37.0.1.g1379af2e9d\n\n"},{"id":"458117","messageId":"d346b65a43ba4f4c0c76f9483243e5277b411b11.1656528415.git.me@ttaylorr.com","threadId":"58085","inReplyTo":"cover.1656528343.git.me@ttaylorr.com","subject":"[RFC PATCH 2/4] builtin/repack.c: pass \"cruft_expiration\" to `write_cruft_pack`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-06-29T18:47:11Z","receivedAt":"2022-06-29T18:47:18Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"`builtin/repack.c`'s `write_cruft_pack()` is used to generate the cruft\npack when `--cruft` is supplied. It uses a static variable\n\"cruft_expiration\" which is filled in by option parsing.\n\nA future patch will add an `--expire-to` option which allows `git\nrepack` to write a cruft pack containing the pruned objects out to a\nseparate repository. In order to implement this functionality, some\ncallers will have to pass a value for `cruft_expiration` different than\nthe one filled out by option parsing.\n\nPrepare for this by teaching `write_cruft_pack` to take a\n\"cruft_expiration\" parameter, instead of reading a single static\nvariable.\n\nThe (sole) existing caller of `write_cruft_pack()` will pass the value\nfor \"cruft_expiration\" filled in by option parsing, retaining existing\nbehavior. This means that we can make the variable local to\n`cmd_repack()`, and eliminate the static declaration.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n builtin/repack.c | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 025882a075..19e789d745 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -32,7 +32,6 @@ static int write_bitmaps = -1;\n static int use_delta_islands;\n static int run_update_server_info = 1;\n static char *packdir, *packtmp_name, *packtmp;\n-static char *cruft_expiration;\n \n static const char *const git_repack_usage[] = {\n \tN_(\"git repack [<options>]\"),\n@@ -664,6 +663,7 @@ static int write_midx_included_packs(struct string_list *include,\n \n static int write_cruft_pack(const struct pack_objects_args *args,\n \t\t\t    const char *pack_prefix,\n+\t\t\t    const char *cruft_expiration,\n \t\t\t    struct string_list *names,\n \t\t\t    struct string_list *existing_packs,\n \t\t\t    struct string_list *existing_kept_packs)\n@@ -747,6 +747,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \tstruct pack_objects_args cruft_po_args = {NULL};\n \tint geometric_factor = 0;\n \tint write_midx = 0;\n+\tconst char *cruft_expiration = NULL;\n \n \tstruct option builtin_repack_options[] = {\n \t\tOPT_BIT('a', NULL, &pack_everything,\n@@ -986,7 +987,8 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\tcruft_po_args.local = po_args.local;\n \t\tcruft_po_args.quiet = po_args.quiet;\n \n-\t\tret = write_cruft_pack(&cruft_po_args, pack_prefix, &names,\n+\t\tret = write_cruft_pack(&cruft_po_args, pack_prefix,\n+\t\t\t\t       cruft_expiration, &names,\n \t\t\t\t       &existing_nonkept_packs,\n \t\t\t\t       &existing_kept_packs);\n \t\tif (ret)\n-- \n2.37.0.1.g1379af2e9d\n\n"},{"id":"458118","messageId":"86bfb40904427f68d5094e00d568eaa1ad341cad.1656528415.git.me@ttaylorr.com","threadId":"58085","inReplyTo":"cover.1656528343.git.me@ttaylorr.com","subject":"[RFC PATCH 3/4] builtin/repack.c: write cruft packs to arbitrary locations","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-06-29T18:47:14Z","receivedAt":"2022-06-29T18:47:23Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In the following commit, a new write_cruft_pack() caller will be added\nwhich wants to write a cruft pack to an arbitrary location. Prepare for\nthis by adding a parameter which controls the destination of the cruft\npack.\n\nFor now, provide \"packtmp\" so that this commit does not change any\nbehavior.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n builtin/repack.c | 14 +++++++++++---\n 1 file changed, 11 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 19e789d745..ab976007e1 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -662,6 +662,7 @@ static int write_midx_included_packs(struct string_list *include,\n }\n \n static int write_cruft_pack(const struct pack_objects_args *args,\n+\t\t\t    const char *destination,\n \t\t\t    const char *pack_prefix,\n \t\t\t    const char *cruft_expiration,\n \t\t\t    struct string_list *names,\n@@ -673,8 +674,10 @@ static int write_cruft_pack(const struct pack_objects_args *args,\n \tstruct string_list_item *item;\n \tFILE *in, *out;\n \tint ret;\n+\tconst char *scratch;\n+\tint local = skip_prefix(destination, packdir, &scratch);\n \n-\tprepare_pack_objects(&cmd, args, packtmp);\n+\tprepare_pack_objects(&cmd, args, destination);\n \n \tstrvec_push(&cmd.args, \"--cruft\");\n \tif (cruft_expiration)\n@@ -714,7 +717,12 @@ static int write_cruft_pack(const struct pack_objects_args *args,\n \t\tif (line.len != the_hash_algo->hexsz)\n \t\t\tdie(_(\"repack: Expecting full hex object ID lines only \"\n \t\t\t      \"from pack-objects.\"));\n-\t\tstring_list_append(names, line.buf);\n+                /*\n+\t\t * avoid putting packs written outside of the repository in the\n+\t\t * list of names\n+\t\t */\n+\t\tif (local)\n+\t\t\tstring_list_append(names, line.buf);\n \t}\n \tfclose(out);\n \n@@ -987,7 +995,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\tcruft_po_args.local = po_args.local;\n \t\tcruft_po_args.quiet = po_args.quiet;\n \n-\t\tret = write_cruft_pack(&cruft_po_args, pack_prefix,\n+\t\tret = write_cruft_pack(&cruft_po_args, packtmp, pack_prefix,\n \t\t\t\t       cruft_expiration, &names,\n \t\t\t\t       &existing_nonkept_packs,\n \t\t\t\t       &existing_kept_packs);\n-- \n2.37.0.1.g1379af2e9d\n\n"},{"id":"458119","messageId":"9baf44b3174d82c9fae858c77955cacb5131aa91.1656528415.git.me@ttaylorr.com","threadId":"58085","inReplyTo":"cover.1656528343.git.me@ttaylorr.com","subject":"[RFC PATCH 4/4] builtin/repack.c: implement `--expire-to` for storing pruned objects","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-06-29T18:47:18Z","receivedAt":"2022-06-29T18:47:30Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"When pruning objects with `--cruft`, `git repack` offers some\nflexibility when selecting the set of which objects are pruned via the\n`--cruft-expiration` option.\n\nThis is useful for expiring objects which are older than the grace\nperiod, making races where to-be-pruned objects become reachable and\nthen ancestors of freshly pushed objects, leaving the repository in a\ncorrupt state after pruning substantially less likely.\n\nBut in practice, such races are impossible to avoid entirely, no matter\nhow long the grace period is. To prevent this race, it is often\nadvisable to temporarily put a repository into a read-only state. But in\npractice, this is not always practical, and so some middle ground would\nbe nice.\n\nThis patch introduces a new option, `--expire-to`, which teaches `git\nrepack` to write an additional cruft pack containing just the objects\nwhich were pruned from the repository. The caller can specify a\ndirectory outside of the current repository as the destination for this\nsecond cruft pack.\n\nThis makes it possible to prune objects from a repository, while still\nholding onto a supplemental copy of them outside of the original\nrepository. Having this copy on-disk makes it substantially easier to\nrecover objects when the aforementioned race is encountered.\n\n`--expire-to` is implemented in a somewhat convoluted manner, which is\nto take advantage of the fact that the first time `write_cruft_pack()`\nis called, it adds the name of the cruft pack to the `names` string\nlist. That means the second time we call `write_cruft_pack()`, objects\nin the previously-written cruft pack will be excluded.\n\nAs long as the caller ensures that no objects are expired during the\nsecond pass, this is sufficient to generate a cruft pack containing all\nobjects which don't appear in any of the new packs written by `git\nrepack`, including the cruft pack. In other words, all of the objects\nwhich are about to be pruned from the repository.\n\nIt is important to note that the destination in `--expire-to` does not\nnecessarily need to be a Git repository (though it can be) Notably, the\nexpired packs do not contain all ancestors of expired objects. So if the\nsource repository contains something like:\n\n              <unreachable>\n             /\n    C1 --- C2\n      \\\n       refs/heads/master\n\nwhere C2 is unreachable, but has a parent (C1) which is reachable, and\nC2 would be pruned, then the expiry pack will contain only C2, not C1.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n Documentation/git-repack.txt |   6 ++\n builtin/repack.c             |  40 ++++++++++++\n t/t7700-repack.sh            | 121 +++++++++++++++++++++++++++++++++++\n 3 files changed, 167 insertions(+)\n\ndiff --git a/Documentation/git-repack.txt b/Documentation/git-repack.txt\nindex 0bf13893d8..4017157949 100644\n--- a/Documentation/git-repack.txt\n+++ b/Documentation/git-repack.txt\n@@ -74,6 +74,12 @@ to the new separate pack will be written.\n \timmediately instead of waiting for the next `git gc` invocation.\n \tOnly useful with `--cruft -d`.\n \n+--expire-to=<dir>::\n+\tWrite a cruft pack containing pruned objects (if any) to the\n+\tdirectory `<dir>`. This option is useful for keeping a copy of\n+\tany pruned objects in a separate directory as a backup. Only\n+\tuseful with `--cruft -d`.\n+\n -l::\n \tPass the `--local` option to 'git pack-objects'. See\n \tlinkgit:git-pack-objects[1].\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex ab976007e1..d789150a2e 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -702,6 +702,10 @@ static int write_cruft_pack(const struct pack_objects_args *args,\n \t * By the time it is read here, it contains only the pack(s)\n \t * that were just written, which is exactly the set of packs we\n \t * want to consider kept.\n+\t *\n+\t * If `--expire-to` is given, the double-use served by `names`\n+\t * ensures that the pack written to `--expire-to` excludes any\n+\t * objects contained in the cruft pack.\n \t */\n \tin = xfdopen(cmd.in, \"w\");\n \tfor_each_string_list_item(item, names)\n@@ -756,6 +760,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \tint geometric_factor = 0;\n \tint write_midx = 0;\n \tconst char *cruft_expiration = NULL;\n+\tconst char *expire_to = NULL;\n \n \tstruct option builtin_repack_options[] = {\n \t\tOPT_BIT('a', NULL, &pack_everything,\n@@ -805,6 +810,8 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\t\t    N_(\"find a geometric progression with factor <N>\")),\n \t\tOPT_BOOL('m', \"write-midx\", &write_midx,\n \t\t\t   N_(\"write a multi-pack index of the resulting packs\")),\n+\t\tOPT_STRING(0, \"expire-to\", &expire_to, N_(\"dir\"),\n+\t\t\t   N_(\"pack prefix to store a pack containing pruned objects\")),\n \t\tOPT_END()\n \t};\n \n@@ -1001,6 +1008,39 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\t\t\t       &existing_kept_packs);\n \t\tif (ret)\n \t\t\treturn ret;\n+\n+\t\tif (delete_redundant && expire_to) {\n+\t\t\t/*\n+\t\t\t * If `--expire-to` is given with `-d`, it's possible\n+\t\t\t * that we're about to prune some objects. With cruft\n+\t\t\t * packs, pruning is implicit: any objects from existing\n+\t\t\t * packs that weren't picked up by new packs are removed\n+\t\t\t * when their packs are deleted.\n+\t\t\t *\n+\t\t\t * Generate an additional cruft pack, with one twist:\n+\t\t\t * `names` now includes the name of the cruft pack\n+\t\t\t * written in the previous step. So the contents of\n+\t\t\t * _this_ cruft pack exclude everything contained in the\n+\t\t\t * existing cruft pack (that is, all of the unreachable\n+\t\t\t * objects which are no older than\n+\t\t\t * `--cruft-expiration`).\n+\t\t\t *\n+\t\t\t * To make this work, cruft_expiration must become NULL\n+\t\t\t * so that this cruft pack doesn't actually prune any\n+\t\t\t * objects. If it were non-NULL, this call would always\n+\t\t\t * generate an empty pack (since every object not in the\n+\t\t\t * cruft pack generated above will have an mtime older\n+\t\t\t * than the expiration).\n+\t\t\t */\n+\t\t\tret = write_cruft_pack(&cruft_po_args, expire_to,\n+\t\t\t\t\t       pack_prefix,\n+\t\t\t\t\t       NULL,\n+\t\t\t\t\t       &names,\n+\t\t\t\t\t       &existing_nonkept_packs,\n+\t\t\t\t\t       &existing_kept_packs);\n+\t\t\tif (ret)\n+\t\t\t\treturn ret;\n+\t\t}\n \t}\n \n \tstring_list_sort(&names);\ndiff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\nindex ca45c4cd2c..7ffd3c7b54 100755\n--- a/t/t7700-repack.sh\n+++ b/t/t7700-repack.sh\n@@ -482,4 +482,125 @@ test_expect_success '-n overrides repack.updateServerInfo=true' '\n \ttest_server_info_missing\n '\n \n+test_expect_success '--expire to stores pruned objects (now)' '\n+\tgit init expire-to-now &&\n+\t(\n+\t\tcd expire-to-now &&\n+\n+\t\tgit branch -M main &&\n+\n+\t\ttest_commit base &&\n+\n+\t\tgit checkout -b cruft &&\n+\t\ttest_commit --no-tag cruft &&\n+\n+\t\tgit rev-list --objects --no-object-names main..cruft >moved.raw &&\n+\t\tsort moved.raw >moved.want &&\n+\n+\t\tgit rev-list --all --objects --no-object-names >expect.raw &&\n+\t\tsort expect.raw >expect &&\n+\n+\t\tgit checkout main &&\n+\t\tgit branch -D cruft &&\n+\t\tgit reflog expire --all --expire=all &&\n+\n+\t\tgit init --bare expired.git &&\n+\t\tgit repack -d \\\n+\t\t\t--cruft --cruft-expiration=\"now\" \\\n+\t\t\t--expire-to=\"expired.git/objects/pack/pack\" &&\n+\n+\t\texpired=\"$(ls expired.git/objects/pack/pack-*.idx)\" &&\n+\t\ttest_path_is_file \"${expired%.idx}.mtimes\" &&\n+\n+\t\t# Since the `--cruft-expiration` is \"now\", the effective\n+\t\t# behavior is to move _all_ unreachable objects out to\n+\t\t# the location in `--expire-to`.\n+\t\tgit show-index <$expired >expired.raw &&\n+\t\tcut -d\" \" -f2 expired.raw | sort >expired.objects &&\n+\t\tgit rev-list --all --objects --no-object-names \\\n+\t\t\t>remaining.objects &&\n+\n+\t\t# ...in other words, the combined contents of this\n+\t\t# repository and expired.git should be the same as the\n+\t\t# set of objects we started with.\n+\t\tcat expired.objects remaining.objects | sort >actual &&\n+\t\ttest_cmp expect actual &&\n+\n+\t\t# The \"moved\" objects (i.e., those in expired.git)\n+\t\t# should be the same as the cruft objects which were\n+\t\t# expired in the previous step.\n+\t\ttest_cmp moved.want expired.objects\n+\t)\n+'\n+\n+test_expect_success '--expire to stores pruned objects (5.minutes.ago)' '\n+\tgit init expire-to-5.minutes.ago &&\n+\t(\n+\t\tcd expire-to-5.minutes.ago &&\n+\n+\t\tgit branch -M main &&\n+\n+\t\ttest_commit base &&\n+\n+\t\t# Create two classes of unreachable objects, one which\n+\t\t# is older than 5 minutes (stale), and another which is\n+\t\t# newer (recent).\n+\t\tfor kind in stale recent\n+\t\tdo\n+\t\t\tgit checkout -b $kind main &&\n+\t\t\ttest_commit --no-tag $kind\n+\t\tdone &&\n+\n+\t\tgit rev-list --objects --no-object-names main..stale >in &&\n+\t\tstale=\"$(git pack-objects $objdir/pack/pack <in)\" &&\n+\t\tmtime=\"$(test-tool chmtime --get =-600 $objdir/pack/pack-$stale.pack)\" &&\n+\n+\t\t# expect holds the set of objects we expect to find in\n+\t\t# this repository after repacking\n+\t\tgit rev-list --objects --no-object-names recent >expect.raw &&\n+\t\tsort expect.raw >expect &&\n+\n+\t\t# moved.want holds the set of objects we expect to find\n+\t\t# in expired.git\n+\t\tgit rev-list --objects --no-object-names main..stale >out &&\n+\t\tsort out >moved.want &&\n+\n+\t\tgit checkout main &&\n+\t\tgit branch -D stale recent &&\n+\t\tgit reflog expire --all --expire=all &&\n+\t\tgit prune-packed &&\n+\n+\t\tgit init --bare expired.git &&\n+\t\tgit repack -d \\\n+\t\t\t--cruft --cruft-expiration=5.minutes.ago \\\n+\t\t\t--expire-to=\"expired.git/objects/pack/pack\" &&\n+\n+\t\t# Some of the remaining objects in this repository are\n+\t\t# unreachable, so use `cat-file --batch-all-objects`\n+\t\t# instead of `rev-list` to get their names\n+\t\tgit cat-file --batch-all-objects --batch-check=\"%(objectname)\" \\\n+\t\t\t>remaining.objects &&\n+\t\tsort remaining.objects >actual &&\n+\t\ttest_cmp expect actual &&\n+\n+\t\t(\n+\t\t\tcd expired.git &&\n+\n+\t\t\texpired=\"$(ls objects/pack/pack-*.mtimes)\" &&\n+\t\t\ttest-tool pack-mtimes $(basename $expired) >out &&\n+\t\t\tcut -d\" \" -f1 out | sort >../moved.got &&\n+\n+\t\t\t# Ensure that there are as many objects with the\n+\t\t\t# expected mtime as were moved to expired.git.\n+\t\t\t#\n+\t\t\t# In other words, ensure that the recorded\n+\t\t\t# mtimes of any moved objects was written\n+\t\t\t# correctly.\n+\t\t\tgrep \" $mtime$\" out >matching &&\n+\t\t\ttest_line_count = $(wc -l <../moved.want) matching\n+\t\t) &&\n+\t\ttest_cmp moved.want moved.got\n+\t)\n+'\n+\n test_done\n-- \n2.37.0.1.g1379af2e9d\n"},{"id":"458137","messageId":"20220629225405.1864460-1-jonathantanmy@google.com","threadId":"58085","inReplyTo":"cover.1656528343.git.me@ttaylorr.com","subject":"Re: [RFC PATCH 0/4] move pruned objects to a separate repository","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-06-29T22:54:04Z","receivedAt":"2022-06-29T22:55:11Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Taylor Blau <me@ttaylorr.com> writes:\n> This series is an RFC for now since I'm interested in discussing whether\n> or not this is a feature that people would actually want to use or not.\n> But if it is, I'm happy to polish this up and turn it into a\n> non-RFC-quality series ;-).\n> \n> In the meantime, thanks for your review!\n\nThanks for this patch set. I can see this being used by, say, someone\nwho wants to preserve a repo that rewinds branches all the time (the\nrefs would need to be backed-up separately, but at least this provides a\nway for objects to be stored efficiently, in that reachable objects are\nstill stored in the main repo and unreachable objects are stored in the\nbackup with no overlap between them).\n\nI think there is at least one more alternative that should be\nconsidered, though: since the cruft pack is unlikely to have its objects\n\"resurrected\" (since the reason why they're there is because they are\nunreachable), it is likely that the objects that are pruned are exactly\nthe same as those in the craft pack. So it would be more efficient to\njust unconditionally rename the cruft pack to the backup destination.\n\nHaving said that, if there is a compelling use case for repacking even\nwhen we're moving from cruft pack to backup, the design of this patch\nset looks good overall. There are some minor points (e.g. the naming of\nthe parameter \"out\" in patch 1), but I understand that this is an RFC\nand I'll wait for a non-RFC patch set before looking more closely at\nthese things.\n"},{"id":"458153","messageId":"Yr0OuwCyDot0wJjs@nand.local","threadId":"58085","inReplyTo":"20220629225405.1864460-1-jonathantanmy@google.com","subject":"Re: [RFC PATCH 0/4] move pruned objects to a separate repository","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-06-30T02:47:23Z","receivedAt":"2022-06-30T02:47:28Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Jun 29, 2022 at 03:54:04PM -0700, Jonathan Tan wrote:\n> Taylor Blau <me@ttaylorr.com> writes:\n> > This series is an RFC for now since I'm interested in discussing whether\n> > or not this is a feature that people would actually want to use or not.\n> > But if it is, I'm happy to polish this up and turn it into a\n> > non-RFC-quality series ;-).\n> >\n> > In the meantime, thanks for your review!\n>\n> Thanks for this patch set. I can see this being used by, say, someone\n> who wants to preserve a repo that rewinds branches all the time (the\n> refs would need to be backed-up separately, but at least this provides a\n> way for objects to be stored efficiently, in that reachable objects are\n> still stored in the main repo and unreachable objects are stored in the\n> backup with no overlap between them).\n\nYes, definitely.\n\nIf it helps, I can share a little bit about the motivating use-case\nwithin GitHub. All objects from a fork network are stored together in a\nrepository that we call the network.git, with individual forks keeping\ntrack of their own references.\n\nThe network.git repository can often grow quite large, and/or contain\ndata that the owner of an individual fork would like removed (e.g., they\naccidentally pushed sensitive credentials, force-pushed over it, but\nwould like the now-unreachable objects to be removed).\n\nWe don't usually do pruning GC's except during manual intervention or\nupon request through a support ticket. But when we do it is often\ninfeasible to lock the entire network's push traffic and reference\nupdates. So it is not an unheard of event to encounter the race that I\ndescribed above.\n\nThe idea is that, at least for non-sensitive pruning, we would move the\npruned objects to a separate repository and hold them there until we\ncould run `git fsck` on the repository after pruning and verify that the\nrepository is intact. If it is, then the expired.git repository can be\nemptied, too, permanently removing the pruned objects. If not, the\nexpired.git repository then becomes a donor for the missing objects,\nwhich are used to heal the corrupt main repository. Once *that* is done,\nand fsck comes back clean, then the expired.git repository can be\nremoved.\n\n> I think there is at least one more alternative that should be\n> considered, though: since the cruft pack is unlikely to have its objects\n> \"resurrected\" (since the reason why they're there is because they are\n> unreachable), it is likely that the objects that are pruned are exactly\n> the same as those in the craft pack. So it would be more efficient to\n> just unconditionally rename the cruft pack to the backup destination.\n\nThis isn't quite right. The contents that are written into the\nexpired.git repository is everything that *didn't* end up in the cruft\npack.\n\nSuppose your cruft expiration is 1.hour.ago, and your doing a repack on\nrepository foo.git, expiring objects into expired.git. There are three\ndisjoint sets of objects:\n\n  - reachable objects, which will stay in foo.git\n  - unreachable objects which were written within the last hour (and are\n    thus too new to prune) which will stay in foo.git\n  - unreachable objects which *weren't* written within the last hour\n    (and thus will be pruned) which are moved to a new pack in\n    expired.git (and removed from foo.git)\n\nSo the cruft pack in foo.git and the one written to expired.git are a\ndisjoint cut of the unreachable objects in foo.git based on their mtime,\nwith the recent objects staying in the source repository and the stale\nones moving to the expired.git repository.\n\nThe original implementation of this feature was to move the entire cruft\npack out of the way like you describe. This is sub-optimal because you\nare forced to generate that cruft pack with `--cruft-expiration=never`,\nsince you can't actually prune any objects when generating the cruft\npack, or they would be gone forever. But since you have to move the\nentire cruft pack out of the way, the visible effect looks like you\nactually pruned *all* unreachable objects, as if you had supplied\n`--cruft-expiration=now`.\n\nBeing able to expire just the objects which have aged out of the grace\nperiod should cause this race to happen less frequently in practice.\n\n> Having said that, if there is a compelling use case for repacking even\n> when we're moving from cruft pack to backup, the design of this patch\n> set looks good overall. There are some minor points (e.g. the naming of\n> the parameter \"out\" in patch 1), but I understand that this is an RFC\n> and I'll wait for a non-RFC patch set before looking more closely at\n> these things.\n\nThanks,\nTaylor\n"},{"id":"458166","messageId":"220630.86y1xeeeik.gmgdl@evledraar.gmail.com","threadId":"58085","inReplyTo":"cover.1656528343.git.me@ttaylorr.com","subject":"Re: [RFC PATCH 0/4] move pruned objects to a separate repository","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-06-30T08:00:27Z","receivedAt":"2022-06-30T08:34:18Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Jun 29 2022, Taylor Blau wrote:\n\n> Now that cruft packs are available in v2.37.0, here is an interesting\n> application of that new feature to enable a two-phase object pruning\n> approach.\n>\n> This came out of a discussion within GitHub about ways we could support\n> storing a set of pruned objects in \"limbo\" so that they were not\n> accessible from the repository which pruned them, but instead stored in\n> a cruft pack in a separate repository which lists the original one as an\n> alternate.\n>\n> This makes it possible to take the collection of all pruned objects and\n> store them in a cruft pack in a separate repository. This repository\n> (which I have been referring to as the \"expired.git\") can then be used\n> as a donor repository for any missing objects (like the ones described\n> by the race in [1]).\n> [...]\n> [1]: https://lore.kernel.org/git/YryF+vkosJOXf+mQ@nand.local/\n\nI think the best description of that race on-list is this by Jeff King,\nif so I think it would be nice to work it into a commit message (for\n4/4):\n\n\thttps://public-inbox.org/git/20190319001829.GL29661@sigill.intra.peff.net/\n\nDownthread of that, starting at:\n\n\thttps://public-inbox.org/git/878svjj4t5.fsf@evledraar.gmail.com/\n\nI describe a proposed mechanism to address the race condition, which\nseems to me be functionally the same as parts of what you're proposing\nhere. I.e. the \"limbo\" here being the same as the proposed \"gc\nquarantine\".\n\nThe main difference being one that this RFC leaves on the table, which\nis how you'd get these objects back into the non-cruft repository once\nthey're erroneously/racily expired. I imagined that we'd add it as a\nspecial alternate, read it last, and make the object reading code aware\nthat any object needed from such an alternate is one that we'd need to\nCOR over to our primary repository:\n\n\thttps://public-inbox.org/git/8736lnxlig.fsf@evledraar.gmail.com/\n\nWhereas it seems like you're imagining just having the \"cruft pack\"\nrepository around so that a support engineer can manually recover from\ncorruption, or have some other out-of-tree mechanism not part of this\nRFC to (semi-?)automate that step.\n\nIf you haven't it would be nice if you could read that thread & see if\nwhat I'm describing there is essentially a superset of what you have\nhere, and if any of the concerns Jeff King brought up there are ones you\nthink apply here.\n\nParticularly as there was a reference to an off-list (presumably at\nGitHub) discussion with Michael Haggerty about these sorts of races. I\ndon't know if either Jeff or Michael were involved in the discussions\nyou had.\n\nI think that the mechanism I proposed there was subtly different from\nwhat Jeff was concerned about being racy, but that thread was left\nhanging as the last reply is from me trying to clarify that point.\n\nSo maybe I'm wrong, but I think if that was the case you'd also be wrong\nabout this approach being viable, so it would be nice to clear that up\n:)\n\nI'd also be very interested to know if you have anything like my\nproposed auto-healing via a special alternate planned.  I think that\nwould allow aggressive pruning of live repositories not by fixing our\nunderlying race conditions, but by \"leaning into\" them as it were.\n\nI.e. we'd race even more, but as we could always auto-heal by \"no, I'll\nactually need that\" COR-ing the relevant object(s) back from the \"gc\nquarantine\" (or your \"cruft repository\") that would be OK.\n"},{"id":"458251","messageId":"20220630211504.2833463-1-jonathantanmy@google.com","threadId":"58085","inReplyTo":"Yr0OuwCyDot0wJjs@nand.local","subject":"Re: [RFC PATCH 0/4] move pruned objects to a separate repository","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-06-30T21:15:04Z","receivedAt":"2022-06-30T21:15:10Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Taylor Blau <me@ttaylorr.com> writes:\n> We don't usually do pruning GC's except during manual intervention or\n> upon request through a support ticket. But when we do it is often\n> infeasible to lock the entire network's push traffic and reference\n> updates. So it is not an unheard of event to encounter the race that I\n> described above.\n> \n> The idea is that, at least for non-sensitive pruning, we would move the\n> pruned objects to a separate repository and hold them there until we\n> could run `git fsck` on the repository after pruning and verify that the\n> repository is intact. If it is, then the expired.git repository can be\n> emptied, too, permanently removing the pruned objects. If not, the\n> expired.git repository then becomes a donor for the missing objects,\n> which are used to heal the corrupt main repository. Once *that* is done,\n> and fsck comes back clean, then the expired.git repository can be\n> removed.\n\nThanks for the information. Presumably, during such pruning GCs (because\nmanual intervention was needed or because of a support ticket), you\nwould use a cruft expiration of \"now\", not something like \"14 days ago\".\nIf so, more on this specific case later...\n\n> > I think there is at least one more alternative that should be\n> > considered, though: since the cruft pack is unlikely to have its objects\n> > \"resurrected\" (since the reason why they're there is because they are\n> > unreachable), it is likely that the objects that are pruned are exactly\n> > the same as those in the craft pack. So it would be more efficient to\n> > just unconditionally rename the cruft pack to the backup destination.\n> \n> This isn't quite right. The contents that are written into the\n> expired.git repository is everything that *didn't* end up in the cruft\n> pack.\n\nI reread what I wrote and realized that I didn't give a good description\nof what I was thinking. So hopefully this is a better one: I was\nthinking of the case in which GC is regularly run on a repo, say, every\n14 days with a 14-day expiration time. So on the first run you would\nhave:\n\n  a1.pack      a2.pack (+ .mtime)\n  Reachable    Unreachable (cruft pack)\n\nand on the second run, assuming this patch set is in effect:\n\n  b1.pack      b2.pack (+ .mtime)          expired.git/b3.pack\n  Reachable    Unreachable (cruft pack)    In expired.git\n\nand my idea was that it is very likely that a2.pack and\nexpired.git/b3.pack are the same, so I thought that a feature in which\ncruft packs could be moved instead of deleted would be more efficient.\n\nGoing back to the specific case at the start of this email. I now see\nthat my idea wouldn't work in that specific case, because you would want\nto generate expired.git packs from a repository that is unlikely to have\ncruft packs (or, at least, it is unlikely that the existing cruft packs\nmatch exactly what you would write in expired.git).\n\nIf it is likely that cruft pack(s) would only be written in one place\n(whether in the same repository or in expired.git, but not both),\nanother design option would be to be able to tell Git where to write\ncruft packs, but not give Git the ability to write cruft packs both to\nthe same repository and expired.git. (Unless in your use case, Taylor,\nyou envision that you would occasionally need to write cruft packs in\nboth places.) That would simplify the UI and the code a bit, but not by\nmuch (this patch set, as written, is already elegantly written), so I\ndon't feel too strongly about it and I would be happy with the current\npattern.\n\n\n"}]}