{"thread":{"id":"58683","subject":"[PATCH 0/4] repack: implement `--expire-to` option","startedAt":"2022-10-24T20:33:13Z","lastAt":"2022-11-07T19:52:26Z","messageCount":14,"participants":["Taylor Blau","Junio C Hamano","Derrick Stolee"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"465630","messageId":"cover.1666636974.git.me@ttaylorr.com","threadId":"58683","inReplyTo":null,"subject":"[PATCH 0/4] repack: implement `--expire-to` option","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-24T18:42:58Z","receivedAt":"2022-10-24T20:33:13Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"This series polishes off the RFC [1] I sent over the summer teaching\n`git repack` how to move pruned objects into a separate cruft pack in a\ndifferent repository.\n\nWe've been using this series at GitHub for some time now as an\nescape-hatch mechanism for quickly recovering pruned objects that run\nafoul of the \"advertise-then-prune\" race described in [2].\n\nThe general idea is that we write out a second cruft pack to capture the\npruned objects, with two minor changes:\n\n  - The second cruft pack counts the first cruft pack as \"saved\",\n    meaning that we'll exclude its objects (thus ignoring both reachable\n    and unreachable-but-recent objects from the second cruft pack).\n\n  - The second cruft pack does *not* exclude objects based on age (i.e.,\n    it acts as if `--cruft-expiration=never` were given) so as to avoid\n    excluding objects which are being pruned due to their age.\n\nThe first few patches are preparatory, and the substantive change is\nimplemented in the fourth patch. I would suggest that reviewers start\nthere, and then read the first three patches.\n\nSince this series basically boils down to adding one new conditional,\nlike:\n\n    if (delete_redundant && expire_to) {\n        ret = write_cruft_pack(...):\n        if (ret)\n            return ret;\n    }\n\nThe (second) call to `write_cruft_pack()` comes with a rather extensive\ninline comment explaining what is going on ;-).\n\nThanks in advance for your review.\n\n[1]: https://lore.kernel.org/git/cover.1656528343.git.me@ttaylorr.com/\n[2]: https://lore.kernel.org/git/20190319001829.GL29661@sigill.intra.peff.net/\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.38.0.16.g393fd4c6db\n"},{"id":"465631","messageId":"1dd4136f6199ac050cec5eb671c36ae05fbf3bdd.1666636974.git.me@ttaylorr.com","threadId":"58683","inReplyTo":"cover.1666636974.git.me@ttaylorr.com","subject":"[PATCH 1/4] builtin/repack.c: pass \"out\" to `prepare_pack_objects`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-24T18:43:03Z","receivedAt":"2022-10-24T20:33:23Z","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 a5bacc7797..0a7bd57636 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@@ -861,7 +862,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.38.0.16.g393fd4c6db\n\n"},{"id":"465632","messageId":"7d731d8dd5ebe0570a5dd8a88b3dd3104a79592a.1666636974.git.me@ttaylorr.com","threadId":"58683","inReplyTo":"cover.1666636974.git.me@ttaylorr.com","subject":"[PATCH 2/4] builtin/repack.c: pass \"cruft_expiration\" to `write_cruft_pack`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-24T18:43:06Z","receivedAt":"2022-10-24T20:33:35Z","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 0a7bd57636..1184e8c257 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@@ -746,6 +746,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@@ -985,7 +986,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.38.0.16.g393fd4c6db\n\n"},{"id":"465633","messageId":"6376d15c9c9adce883dba86ef5e5219f803aa9bf.1666636974.git.me@ttaylorr.com","threadId":"58683","inReplyTo":"cover.1666636974.git.me@ttaylorr.com","subject":"[PATCH 4/4] builtin/repack.c: implement `--expire-to` for storing pruned objects","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-24T18:43:12Z","receivedAt":"2022-10-24T20:33:51Z","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 [1].\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\n[1]: https://lore.kernel.org/git/20190319001829.GL29661@sigill.intra.peff.net/\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 a5386ac893..3bc18e0b2f 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@@ -755,6 +759,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@@ -804,6 +809,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@@ -1000,6 +1007,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..17ee6fc2cc 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 || return 1\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.38.0.16.g393fd4c6db\n"},{"id":"465634","messageId":"c0f4ec92a057fdab905447bb917ff09e9bcaaab3.1666636974.git.me@ttaylorr.com","threadId":"58683","inReplyTo":"cover.1666636974.git.me@ttaylorr.com","subject":"[PATCH 3/4] builtin/repack.c: write cruft packs to arbitrary locations","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-24T18:43:09Z","receivedAt":"2022-10-24T20:35:00Z","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 1184e8c257..a5386ac893 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@@ -986,7 +994,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.38.0.16.g393fd4c6db\n\n"},{"id":"465658","messageId":"xmqqa65lrltc.fsf@gitster.g","threadId":"58683","inReplyTo":"1dd4136f6199ac050cec5eb671c36ae05fbf3bdd.1666636974.git.me@ttaylorr.com","subject":"Re: [PATCH 1/4] builtin/repack.c: pass \"out\" to `prepare_pack_objects`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-24T20:47:11Z","receivedAt":"2022-10-24T22:45:40Z","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> `builtin/repack.c`'s `prepare_pack_objects()` is used to prepare a set\n> of arguments to a `pack-objects` process which will generate a desired\n> pack.\n>\n> A future patch will add an `--expire-to` option which allows `git\n> repack` to write a cruft pack containing the pruned objects out to a\n> separate repository. Prepare for this by teaching that function to write\n> packs to an arbitrary location specified by the caller.\n>\n> All existing callers of `prepare_pack_objects()` will pass `packtmp` for\n> `out`, retaining the existing behavior.\n\nIt does make sense to allow the caller to specify the name of the\ntemporary file to be used, but is \"out\" a good name for that?  The\nother two arguments are self explanatory both by their type and the\nname, but this is of type \"const char *\" that does not convey what\nthe string is about at all, so giging a good name to the parameter\nis more important than for others.\n\nThe patch text itself is very straight-forward.  Thanks.\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> @@ -861,7 +862,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"},{"id":"465661","messageId":"xmqq1qqwsyd2.fsf@gitster.g","threadId":"58683","inReplyTo":"c0f4ec92a057fdab905447bb917ff09e9bcaaab3.1666636974.git.me@ttaylorr.com","subject":"Re: [PATCH 3/4] builtin/repack.c: write cruft packs to arbitrary locations","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-24T21:30:49Z","receivedAt":"2022-10-24T23:11:24Z","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> In the following commit, a new write_cruft_pack() caller will be added\n> which wants to write a cruft pack to an arbitrary location. Prepare for\n> this by adding a parameter which controls the destination of the cruft\n> pack.\n>\n> For now, provide \"packtmp\" so that this commit does not change any\n> behavior.\n>\n> Signed-off-by: Taylor Blau <me@ttaylorr.com>\n> ---\n>  builtin/repack.c | 14 +++++++++++---\n>  1 file changed, 11 insertions(+), 3 deletions(-)\n>\n> diff --git a/builtin/repack.c b/builtin/repack.c\n> index 1184e8c257..a5386ac893 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\nHmph.  In an earlier step we got rid of the hardcoded assumption on\nwhere the packtmp is, and we are passing destination in (where the\nexisting callers call us with packtmp) to make it even better, but\nwe acquire the dependency on packdir global with this step.  It's\njust a couple of file-scope static global variables, so it is not a\nhuge deal.\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\nEven if we do not want to contaminate the \"names\" list with packs\nthat are not in the repository, wouldn't our caller still want to be\nable to tell what packs they are?\n\nWhat I am wondering is if it makes more sense to have the caller\npass &names (which can be NULL to just discard the output from the\npack-objects command) so that this function can concentrate on what\nit does (i.e. formulate the command to write cruft packs and then\nreport the packs that are created), without having to worry about\nthe management of the &names thing, which can be done by the caller\nof this function?  We are already passing &names, so it may be the\nmatter of caller deciding to pass &names or NULL based on the value\nof destination it passes to the function?\n\n> @@ -986,7 +994,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\nFor example, this callsite will always want to pass &names because\nit is always about local pack, right?\n\nThanks.\n"},{"id":"465662","messageId":"xmqq5yg9rlne.fsf@gitster.g","threadId":"58683","inReplyTo":"7d731d8dd5ebe0570a5dd8a88b3dd3104a79592a.1666636974.git.me@ttaylorr.com","subject":"Re: [PATCH 2/4] builtin/repack.c: pass \"cruft_expiration\" to `write_cruft_pack`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-24T20:50:45Z","receivedAt":"2022-10-24T23:31:12Z","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> `builtin/repack.c`'s `write_cruft_pack()` is used to generate the cruft\n> pack when `--cruft` is supplied. It uses a static variable\n> \"cruft_expiration\" which is filled in by option parsing.\n>\n> A future patch will add an `--expire-to` option which allows `git\n> repack` to write a cruft pack containing the pruned objects out to a\n> separate repository. In order to implement this functionality, some\n> callers will have to pass a value for `cruft_expiration` different than\n> the one filled out by option parsing.\n>\n> Prepare for this by teaching `write_cruft_pack` to take a\n> \"cruft_expiration\" parameter, instead of reading a single static\n> variable.\n>\n> The (sole) existing caller of `write_cruft_pack()` will pass the value\n> for \"cruft_expiration\" filled in by option parsing, retaining existing\n> behavior. This means that we can make the variable local to\n> `cmd_repack()`, and eliminate the static declaration.\n\nLooks good to me.\n"},{"id":"465969","messageId":"Y1wwt0H9JXFsE0Yn@nand.local","threadId":"58683","inReplyTo":"xmqq1qqwsyd2.fsf@gitster.g","subject":"Re: [PATCH 3/4] builtin/repack.c: write cruft packs to arbitrary locations","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-28T19:42:47Z","receivedAt":"2022-10-28T19:42:55Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Oct 24, 2022 at 02:30:49PM -0700, Junio C Hamano wrote:\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>\n> Even if we do not want to contaminate the \"names\" list with packs\n> that are not in the repository, wouldn't our caller still want to be\n> able to tell what packs they are?\n>\n> What I am wondering is if it makes more sense to have the caller\n> pass &names (which can be NULL to just discard the output from the\n> pack-objects command) so that this function can concentrate on what\n> it does (i.e. formulate the command to write cruft packs and then\n> report the packs that are created), without having to worry about\n> the management of the &names thing, which can be done by the caller\n> of this function?  We are already passing &names, so it may be the\n> matter of caller deciding to pass &names or NULL based on the value\n> of destination it passes to the function?\n\nIt would be nice if it were possible to avoid passing `names` entirely\nhere, but we still need it to determine the set of packs we already\nwrote a few lines above when we write the input to `pack-objects --cruft`.\n\nThanks,\nTaylor\n"},{"id":"466720","messageId":"fbb7b151-e808-ff3b-7c32-ded0c5f51cb7@github.com","threadId":"58683","inReplyTo":"xmqqa65lrltc.fsf@gitster.g","subject":"Re: [PATCH 1/4] builtin/repack.c: pass \"out\" to `prepare_pack_objects`","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-11-07T19:28:06Z","receivedAt":"2022-11-07T19:28:12Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 10/24/22 4:47 PM, Junio C Hamano wrote:\n> Taylor Blau <me@ttaylorr.com> writes:\n> \n>> `builtin/repack.c`'s `prepare_pack_objects()` is used to prepare a set\n>> of arguments to a `pack-objects` process which will generate a desired\n>> pack.\n>>\n>> A future patch will add an `--expire-to` option which allows `git\n>> repack` to write a cruft pack containing the pruned objects out to a\n>> separate repository. Prepare for this by teaching that function to write\n>> packs to an arbitrary location specified by the caller.\n>>\n>> All existing callers of `prepare_pack_objects()` will pass `packtmp` for\n>> `out`, retaining the existing behavior.\n> \n> It does make sense to allow the caller to specify the name of the\n> temporary file to be used, but is \"out\" a good name for that?  The\n> other two arguments are self explanatory both by their type and the\n> name, but this is of type \"const char *\" that does not convey what\n> the string is about at all, so giging a good name to the parameter\n> is more important than for others.\n> \n> The patch text itself is very straight-forward.  Thanks.\n\nI agree that the patch is nice and simple.\n\nAs for a name, this parameter specifies a file prefix.\nPerhaps 'pack_prefix' would be a good name for this?\n\nThanks,\n-Stolee\n"},{"id":"466721","messageId":"2c59cbca-05fb-8aef-a671-61b4a463a368@github.com","threadId":"58683","inReplyTo":"c0f4ec92a057fdab905447bb917ff09e9bcaaab3.1666636974.git.me@ttaylorr.com","subject":"Re: [PATCH 3/4] builtin/repack.c: write cruft packs to arbitrary locations","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-11-07T19:32:05Z","receivedAt":"2022-11-07T19:32:12Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 10/24/22 2:43 PM, Taylor Blau wrote:\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\nThis line looked oddly out-of-alignment with the next one.\n\nIt seems that the comment is preceded by spaces and not\ntabs. Perhaps Junio fixed this during his application of\nthe patch to keep the builds happy.\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\nLGTM.\n\nThanks,\n-Stolee\n"},{"id":"466723","messageId":"21e870f7-0f86-7021-d141-9e83286adea7@github.com","threadId":"58683","inReplyTo":"6376d15c9c9adce883dba86ef5e5219f803aa9bf.1666636974.git.me@ttaylorr.com","subject":"Re: [PATCH 4/4] builtin/repack.c: implement `--expire-to` for storing pruned objects","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-11-07T19:42:52Z","receivedAt":"2022-11-07T19:42:58Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 10/24/22 2:43 PM, Taylor Blau wrote:\n> When pruning objects with `--cruft`, `git repack` offers some\n> flexibility when selecting the set of which objects are pruned via the\n> `--cruft-expiration` option.\n\nThis patch looks good.\n\nCreating these cruft packs in the expire directory does help\nremove the main problem of the race condition. This currently\nrequires some external system to check that no races happened\nand it is safe to delete the pack in the expire directory.\nAlso, that external tool is needed to move that expired pack\n(or preferably, only the necessary objects) back into the\nrepository.\n\nSome future series could find a way to handle those situations,\nperhaps within 'git fsck' to pull missing reachable objects\nfrom the expire directory. Definitely not required for this\nseries, though.\n\nThanks,\n-Stolee\n"},{"id":"466725","messageId":"Y2lh5HargaEQrgy+@nand.local","threadId":"58683","inReplyTo":"2c59cbca-05fb-8aef-a671-61b4a463a368@github.com","subject":"Re: [PATCH 3/4] builtin/repack.c: write cruft packs to arbitrary locations","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-11-07T19:52:04Z","receivedAt":"2022-11-07T19:52:09Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Nov 07, 2022 at 02:32:05PM -0500, Derrick Stolee wrote:\n> On 10/24/22 2:43 PM, Taylor Blau wrote:\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>\n> This line looked oddly out-of-alignment with the next one.\n>\n> It seems that the comment is preceded by spaces and not\n> tabs. Perhaps Junio fixed this during his application of\n> the patch to keep the builds happy.\n\nOdd. In my copy, which I got from gitster/git before Junio went offline,\nthe alignment looks fine.\n\nThanks for double checking, though.\n\nThanks,\nTaylor\n"},{"id":"466726","messageId":"Y2lh9abWORNkgp50@nand.local","threadId":"58683","inReplyTo":"21e870f7-0f86-7021-d141-9e83286adea7@github.com","subject":"Re: [PATCH 4/4] builtin/repack.c: implement `--expire-to` for storing pruned objects","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-11-07T19:52:21Z","receivedAt":"2022-11-07T19:52:26Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Nov 07, 2022 at 02:42:52PM -0500, Derrick Stolee wrote:\n> On 10/24/22 2:43 PM, Taylor Blau wrote:\n> > When pruning objects with `--cruft`, `git repack` offers some\n> > flexibility when selecting the set of which objects are pruned via the\n> > `--cruft-expiration` option.\n>\n> This patch looks good.\n>\n> Creating these cruft packs in the expire directory does help\n> remove the main problem of the race condition. This currently\n> requires some external system to check that no races happened\n> and it is safe to delete the pack in the expire directory.\n> Also, that external tool is needed to move that expired pack\n> (or preferably, only the necessary objects) back into the\n> repository.\n>\n> Some future series could find a way to handle those situations,\n> perhaps within 'git fsck' to pull missing reachable objects\n> from the expire directory. Definitely not required for this\n> series, though.\n\nThanks. Let's start merging this one down.\n\nThanks,\nTaylor\n"}]}