{"thread":{"id":"59731","subject":"[PATCH v3 0/2] pack-objects: introduce `pack.extraRecentObjectsHook`","startedAt":"2023-05-11T23:20:46Z","lastAt":"2023-06-12T21:21:02Z","messageCount":28,"participants":["Taylor Blau","Junio C Hamano","Jeff King","Glen Choo"],"isPatch":true,"patchVersion":3,"patchTotal":2},"messages":[{"id":"477106","messageId":"cover.1683847221.git.me@ttaylorr.com","threadId":"59731","inReplyTo":null,"subject":"[PATCH v3 0/2] pack-objects: introduce `pack.extraRecentObjectsHook`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-05-11T23:20:31Z","receivedAt":"2023-05-11T23:20:46Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Here is a reworked version of the patch which introduced a new\nconfiguration `pack.extraCruftTips` to keep additional objects from\npruning during a cruft pack-generating GC.\n\nThe third round was significantly more complicated than necessary, and I\nthink this round represents a couple of improvements thanks to a very\nhelpful set of reviews from Junio and Peff:\n\n - The new code does not change the existing cruft pack implementation\n whatsoever. This is nice, since cruft packs will be the default in\n the next release, so changing that implementation carries additional\n risk.\n\n - The new code is also not specific to cruft packs, meaning that you\n can do things like:\n\n $ git -c pack.extraRecentObjectsHook=... \\\n repack -Ad --unpack-unreachable=1.hour.ago\n\n and have it write out loose copies of any object(s) mentioned by one\n or more of the configured hooks.\n\nI am hopeful that others think this version is in a good spot. As in\nearlier rounds, I would appreciate an extra careful review on this\ntopic, because of the changing default I mentioned earlier.\n\nThanks in advance for your review.\n\nTaylor Blau (2):\n reachable.c: extract `obj_is_recent()`\n builtin/pack-objects.c: introduce `pack.recentObjectsHook`\n\n Documentation/config/pack.txt | 13 ++\n builtin/pack-objects.c | 1 +\n reachable.c | 100 +++++++++++++++-\n t/t5329-pack-objects-cruft.sh | 171 +++++++++++++++++++++++++++\n t/t7701-repack-unpack-unreachable.sh | 22 ++++\n 5 files changed, 303 insertions(+), 4 deletions(-)\n\nRange-diff against v2:\n-: ---------- > 1: f5f3b0f334 reachable.c: extract `obj_is_recent()`\n1: da1711b13b ! 2: 2ce8a79fa4 builtin/pack-objects.c: introduce `pack.extraCruftTips`\n @@ Metadata\n Author: Taylor Blau \n \n ## Commit message ##\n - builtin/pack-objects.c: introduce `pack.extraCruftTips`\n + builtin/pack-objects.c: introduce `pack.recentObjectsHook`\n \n This patch introduces a new multi-valued configuration option,\n - `pack.extraCruftTips` as an alternative means to mark certain objects in\n - the cruft pack as rescued, even if they would otherwise be pruned.\n + `pack.recentObjectsHook` as a means to mark certain objects as recent,\n + regardless of their age.\n \n - Traditionally, cruft packs are generated in one of two ways:\n + Depending on whether or not we are generating a cruft pack, this allows\n + the caller to do one of two things:\n \n - - When not pruning, all packed and loose objects which do not appear\n - in the any of the packs which wil remain (as indicated over stdin,\n - with `--cruft`) are packed separately into a cruft pack.\n + - If generating a cruft pack, the caller is able to retain additional\n + objects via the cruft pack, even if they would have otherwise been\n + pruned due to their age.\n \n - - When pruning, the above process happens, with two additional tweaks.\n - Instead of adding every object into the cruft pack, only objects\n - which have mtime that is within the grace period are kept. In\n - addition, a \"rescuing\" traversal is performed over the remaining\n - cruft objects to keep around cruft objects which would have aged out\n - of the repository, but are reachable from other cruft objects which\n - have not yet aged out.\n + - If not generating a cruft pack, the caller is likewise able to\n + retain additional objects as loose.\n \n - This works well for repositories which have all of their \"interesting\"\n - objects worth keeping around reachable via at least one reference.\n -\n - However, there is no option to be able to keep around certain objects\n + There is currently no option to be able to keep around certain objects\n that have otherwise aged out of the grace period. The only way to retain\n those objects is:\n \n - to point a reference at them, which may be undesirable or\n infeasible,\n +\n - to track them via the reflog, which may be undesirable since the\n reflog's lifetime is limited to that of the reference it's tracking\n (and callers may want to keep those unreachable objects around for\n longer)\n +\n - to extend the grace period, which may keep around other objects that\n the caller *does* want to discard,\n +\n - or, to force the caller to construct the pack of objects they want\n to keep themselves, and then mark the pack as kept by adding a\n \".keep\" file.\n \n - This patch introduces a new configuration, `pack.extraCruftTips` which\n - allows the caller to specify a program (or set of programs) whose output\n - is treated as a set of objects to treat as \"kept\", even if they are\n - unreachable and have aged out of the retention period.\n + This patch introduces a new configuration, `pack.recentObjectsHook`\n + which allows the caller to specify a program (or set of programs) whose\n + output is treated as a set of objects to treat as recent, regardless of\n + their true age.\n \n - The implementation is straightforward: after determining the set of\n - objects to pack into the cruft pack (either by calling\n - `enumerate_cruft_objects()` or `enumerate_and_traverse_cruft_objects()`)\n - we figure out if there are any program(s) we need to consult in order to\n - determine if there are any objects which we need to \"rescue\". We then\n - add those as tips to another reachability traversal, marking every\n - object along the way as cruft (and thus retaining it in the cruft pack).\n + The implementation is straightforward. In either case (cruft packs or\n + not), Git enumerates recent objects via\n + `add_unseen_recent_objects_to_traversal()`. That enumerates loose and\n + packed objects, and eventually calls add_recent_object() on any objects\n + for which `want_recent_object()`'s conditions are met.\n +\n + This patch modifies the recency condition from simply \"is the mtime of\n + this object more recent than the cutoff?\" to \"[...] or, is this object\n + mentioned by at least one `pack.recentObjectsHook`?\".\n +\n + We then add those as tips to another reachability traversal (along with\n + any recent objects, if pruning), marking every object along the way\n + (either adding it to the cruft pack, or writing it out as a loose\n + object).\n \n A potential alternative here is to introduce a new mode to alter the\n contents of the reachable pack instead of the cruft one. One could\n - imagine a new option to `pack-objects`, say `--extra-tips` that does the\n - same thing as above, adding the visited set of objects along the\n - traversal to the pack.\n + imagine a new option to `pack-objects`, say `--extra-reachable-tips`\n + that does the same thing as above, adding the visited set of objects\n + along the traversal to the pack.\n \n But this has the unfortunate side-effect of altering the reachability\n closure of that pack. If parts of the unreachable object graph mentioned\n - by one or more of the \"extra tips\" programs is not closed, then the\n - resulting pack won't be either. This makes it impossible in the general\n - case to write out reachability bitmaps for that pack, since closure is a\n - requirement there.\n + by one or more of the \"extra reachable tips\" programs is not closed,\n + then the resulting pack won't be either. This makes it impossible in the\n + general case to write out reachability bitmaps for that pack, since\n + closure is a requirement there.\n \n - Instead, keep these unreachable objects in the cruft pack instead, to\n - ensure that we can continue to have a pack containing just reachable\n - objects, which is always safe to write a bitmap over.\n + Instead, keep these unreachable objects in the cruft pack (or set of\n + unreachable, loose objects) instead, to ensure that we can continue to\n + have a pack containing just reachable objects, which is always safe to\n + write a bitmap over.\n \n + Helped-by: Jeff King \n Signed-off-by: Taylor Blau \n \n ## Documentation/config/pack.txt ##\n @@ Documentation/config/pack.txt: pack.deltaCacheLimit::\n \tresult once the best match for all objects is found.\n \tDefaults to 1000. Maximum value is 65535.\n \n -+pack.extraCruftTips::\n -+\tWhen generating a cruft pack, use the shell to execute the\n -+\tspecified command(s), and interpret their output as additional\n -+\ttips of objects to keep in the cruft pack, regardless of their\n -+\tage.\n ++pack.recentObjectsHook::\n ++\tWhen considering the recency of an object (e.g., when generating\n ++\ta cruft pack or storing unreachable objects as loose), use the\n ++\tshell to execute the specified command(s). Interpret their\n ++\toutput as object IDs which Git will consider as \"recent\",\n ++\tregardless of their age.\n ++\n +Output must contain exactly one hex object ID per line, and nothing\n +else. Objects which cannot be found in the repository are ignored.\n -+Multiple hooks are supported, but all must exit successfully, else no\n -+cruft pack will be generated.\n ++Multiple hooks are supported, but all must exit successfully, else the\n ++operation (either generating a cruft pack or unpacking unreachable\n ++objects) will be halted.\n +\n pack.threads::\n \tSpecifies the number of threads to spawn when searching for best\n @@ builtin/pack-objects.c\n \n /*\n * Objects we are going to pack are collected in the `to_pack` structure.\n -@@ builtin/pack-objects.c: static void enumerate_and_traverse_cruft_objects(struct string_list *fresh_packs\n - \tstop_progress(&progress_state);\n - }\n +\n + ## reachable.c ##\n +@@\n + #include \"object-store.h\"\n + #include \"pack-bitmap.h\"\n + #include \"pack-mtimes.h\"\n ++#include \"config.h\"\n ++#include \"run-command.h\"\n \n -+static int enumerate_extra_cruft_tips_1(struct rev_info *revs, const char *args)\n + struct connectivity_progress {\n + \tstruct progress *progress;\n +@@ reachable.c: struct recent_data {\n + \ttimestamp_t timestamp;\n + \treport_recent_object_fn *cb;\n + \tint ignore_in_core_kept_packs;\n ++\n ++\tstruct oidset extra_recent_oids;\n ++\tint extra_recent_oids_loaded;\n + };\n + \n ++static int run_one_pack_recent_objects_hook(struct oidset *set,\n ++\t\t\t\t\t const char *args)\n +{\n +\tstruct child_process cmd = CHILD_PROCESS_INIT;\n +\tstruct strbuf buf = STRBUF_INIT;\n -+\tFILE *out = NULL;\n ++\tFILE *out;\n +\tint ret = 0;\n +\n +\tcmd.use_shell = 1;\n +\tcmd.out = -1;\n +\n +\tstrvec_push(&cmd.args, args);\n -+\tstrvec_pushv(&cmd.env, (const char **)local_repo_env);\n +\n -+\tif (start_command(&cmd)) {\n -+\t\tret = -1;\n -+\t\tgoto done;\n -+\t}\n ++\tif (start_command(&cmd))\n ++\t\treturn -1;\n +\n +\tout = xfdopen(cmd.out, \"r\");\n -+\twhile (strbuf_getline_lf(&buf, out) != EOF) {\n ++\twhile (strbuf_getline(&buf, out) != EOF) {\n +\t\tstruct object_id oid;\n -+\t\tstruct object *obj;\n +\t\tconst char *rest;\n +\n +\t\tif (parse_oid_hex(buf.buf, &oid, &rest) || *rest) {\n @@ builtin/pack-objects.c: static void enumerate_and_traverse_cruft_objects(struct\n +\t\t\tgoto done;\n +\t\t}\n +\n -+\t\tobj = parse_object(the_repository, &oid);\n -+\t\tif (!obj)\n -+\t\t\tcontinue;\n -+\n -+\t\tdisplay_progress(progress_state, ++nr_seen);\n -+\t\tadd_pending_object(revs, obj, \"\");\n ++\t\toidset_insert(set, &oid);\n +\t}\n +\n +\tret = finish_command(&cmd);\n @@ builtin/pack-objects.c: static void enumerate_and_traverse_cruft_objects(struct\n +\treturn ret;\n +}\n +\n -+static int enumerate_extra_cruft_tips(void)\n ++static void load_pack_recent_objects(struct recent_data *data)\n +{\n -+\tstruct rev_info revs;\n +\tconst struct string_list *programs;\n +\tint ret = 0;\n +\tsize_t i;\n +\n -+\tif (git_config_get_string_multi(\"pack.extracrufttips\", &programs))\n -+\t\treturn ret;\n ++\tdata->extra_recent_oids_loaded = 1;\n +\n -+\trepo_init_revisions(the_repository, &revs, NULL);\n ++\tif (git_config_get_string_multi(\"pack.recentobjectshook\", &programs))\n ++\t\treturn;\n +\n -+\trevs.tag_objects = 1;\n -+\trevs.tree_objects = 1;\n -+\trevs.blob_objects = 1;\n -+\n -+\trevs.include_check = cruft_include_check;\n -+\trevs.include_check_obj = cruft_include_check_obj;\n -+\n -+\trevs.ignore_missing_links = 1;\n -+\n -+\tif (progress)\n -+\t\tprogress_state = start_progress(_(\"Enumerating extra cruft tips\"), 0);\n -+\tnr_seen = 0;\n +\tfor (i = 0; i < programs->nr; i++) {\n -+\t\tret = enumerate_extra_cruft_tips_1(&revs,\n -+\t\t\t\t\t\t programs->items[i].string);\n ++\t\tret = run_one_pack_recent_objects_hook(&data->extra_recent_oids,\n ++\t\t\t\t\t\t programs->items[i].string);\n +\t\tif (ret)\n +\t\t\tbreak;\n +\t}\n -+\tstop_progress(&progress_state);\n +\n +\tif (ret)\n +\t\tdie(_(\"unable to enumerate additional cruft tips\"));\n -+\n -+\tif (prepare_revision_walk(&revs))\n -+\t\tdie(_(\"revision walk setup failed\"));\n -+\tif (progress)\n -+\t\tprogress_state = start_progress(_(\"Traversing extra cruft objects\"), 0);\n -+\tnr_seen = 0;\n -+\n -+\t/*\n -+\t * Retain all objects reachable from extra tips via\n -+\t * show_cruft_commit(), and show_cruft_object(), regardless of\n -+\t * their mtime.\n -+\t */\n -+\ttraverse_commit_list(&revs, show_cruft_commit, show_cruft_object, NULL);\n -+\tstop_progress(&progress_state);\n -+\n -+\treturn ret;\n +}\n +\n - static void read_cruft_objects(void)\n + static int obj_is_recent(const struct object_id *oid, timestamp_t mtime,\n + \t\t\t struct recent_data *data)\n {\n - \tstruct strbuf buf = STRBUF_INIT;\n -@@ builtin/pack-objects.c: static void read_cruft_objects(void)\n - \telse\n - \t\tenumerate_cruft_objects();\n +-\treturn mtime > data->timestamp;\n ++\tif (mtime > data->timestamp)\n ++\t\treturn 1;\n ++\n ++\tif (!data->extra_recent_oids_loaded)\n ++\t\tload_pack_recent_objects(data);\n ++\treturn oidset_contains(&data->extra_recent_oids, oid);\n + }\n + \n + static void add_recent_object(const struct object_id *oid,\n +@@ reachable.c: static int want_recent_object(struct recent_data *data,\n + \t\t\t const struct object_id *oid)\n + {\n + \tif (data->ignore_in_core_kept_packs &&\n +-\t has_object_kept_pack(oid, IN_CORE_KEEP_PACKS))\n ++\t has_object_kept_pack(oid, IN_CORE_KEEP_PACKS)) {\n ++\t\tif (!data->extra_recent_oids_loaded)\n ++\t\t\tload_pack_recent_objects(data);\n ++\t\tif (oidset_contains(&data->extra_recent_oids, oid))\n ++\t\t\treturn 1;\n ++\n + \t\treturn 0;\n ++\t}\n + \treturn 1;\n + }\n + \n +@@ reachable.c: int add_unseen_recent_objects_to_traversal(struct rev_info *revs,\n + \tdata.cb = cb;\n + \tdata.ignore_in_core_kept_packs = ignore_in_core_kept_packs;\n \n -+\tenumerate_extra_cruft_tips();\n ++\toidset_init(&data.extra_recent_oids, 0);\n ++\tdata.extra_recent_oids_loaded = 0;\n +\n - \tstrbuf_release(&buf);\n - \tstring_list_clear(&discard_packs, 0);\n - \tstring_list_clear(&fresh_packs, 0);\n + \tr = for_each_loose_object(add_recent_loose, &data,\n + \t\t\t\t FOR_EACH_OBJECT_LOCAL_ONLY);\n + \tif (r)\n +-\t\treturn r;\n ++\t\tgoto done;\n + \n + \tflags = FOR_EACH_OBJECT_LOCAL_ONLY | FOR_EACH_OBJECT_PACK_ORDER;\n + \tif (ignore_in_core_kept_packs)\n + \t\tflags |= FOR_EACH_OBJECT_SKIP_IN_CORE_KEPT_PACKS;\n + \n +-\treturn for_each_packed_object(add_recent_packed, &data, flags);\n ++\tr = for_each_packed_object(add_recent_packed, &data, flags);\n ++\n ++done:\n ++\toidset_clear(&data.extra_recent_oids);\n ++\n ++\treturn r;\n + }\n + \n + static int mark_object_seen(const struct object_id *oid,\n \n ## t/t5329-pack-objects-cruft.sh ##\n @@ t/t5329-pack-objects-cruft.sh: test_expect_success 'cruft objects are freshend via loose' '\n @@ t/t5329-pack-objects-cruft.sh: test_expect_success 'cruft objects are freshend v\n +\t\twrite_script extra-tips <<-EOF &&\n +\t\techo $cruft_old\n +\t\tEOF\n -+\t\tgit config pack.extraCruftTips ./extra-tips &&\n ++\t\tgit config pack.recentObjectsHook ./extra-tips &&\n +\n +\t\tgit repack --cruft --cruft-expiration=now -d &&\n +\n @@ t/t5329-pack-objects-cruft.sh: test_expect_success 'cruft objects are freshend v\n +\n +\t\t# Ensure that the \"old\" objects are removed after\n +\t\t# dropping the pack.extraCruftTips hook.\n -+\t\tgit config --unset pack.extraCruftTips &&\n ++\t\tgit config --unset pack.recentObjectsHook &&\n +\t\tgit repack --cruft --cruft-expiration=now -d &&\n +\n +\t\tmtimes=\"$(ls .git/objects/pack/pack-*.mtimes)\" &&\n @@ t/t5329-pack-objects-cruft.sh: test_expect_success 'cruft objects are freshend v\n +\n +\t\t# ensure that each extra cruft tip is saved by its\n +\t\t# respective hook\n -+\t\tgit config --add pack.extraCruftTips ./extra-tips.a &&\n -+\t\tgit config --add pack.extraCruftTips ./extra-tips.b &&\n ++\t\tgit config --add pack.recentObjectsHook ./extra-tips.a &&\n ++\t\tgit config --add pack.recentObjectsHook ./extra-tips.b &&\n +\t\tgit repack --cruft --cruft-expiration=now -d &&\n +\n +\t\tmtimes=\"$(ls .git/objects/pack/pack-*.mtimes)\" &&\n @@ t/t5329-pack-objects-cruft.sh: test_expect_success 'cruft objects are freshend v\n +\t\ttest_cmp cruft.expect cruft.actual &&\n +\n +\t\t# ensure that a dirty exit halts cruft pack generation\n -+\t\tgit config --add pack.extraCruftTips ./extra-tips.c &&\n -+\t\ttest_must_fail git repack --cruft -d 2>err &&\n ++\t\tgit config --add pack.recentObjectsHook ./extra-tips.c &&\n ++\t\ttest_must_fail git repack --cruft --cruft-expiration=now -d 2>err &&\n +\t\tgrep \"unable to enumerate additional cruft tips\" err &&\n +\n +\t\t# and that the existing cruft pack is left alone\n @@ t/t5329-pack-objects-cruft.sh: test_expect_success 'cruft objects are freshend v\n +\t\twrite_script extra-tips <<-EOF &&\n +\t\techo $blob\n +\t\tEOF\n -+\t\tgit config pack.extraCruftTips ./extra-tips &&\n ++\t\tgit config pack.recentObjectsHook ./extra-tips &&\n +\n +\t\tgit repack --cruft --cruft-expiration=now -d &&\n +\n @@ t/t5329-pack-objects-cruft.sh: test_expect_success 'cruft objects are freshend v\n +'\n +\n test_done\n +\n + ## t/t7701-repack-unpack-unreachable.sh ##\n +@@ t/t7701-repack-unpack-unreachable.sh: test_expect_success 'do not bother loosening old objects' '\n + \ttest_must_fail git cat-file -p $obj2\n + '\n + \n ++test_expect_success 'extra recent tips are kept regardless of age' '\n ++\tobj1=$(echo one | git hash-object -w --stdin) &&\n ++\tobj2=$(echo two | git hash-object -w --stdin) &&\n ++\tpack1=$(echo $obj1 | git pack-objects .git/objects/pack/pack) &&\n ++\tpack2=$(echo $obj2 | git pack-objects .git/objects/pack/pack) &&\n ++\tgit prune-packed &&\n ++\n ++\tgit cat-file -p $obj1 &&\n ++\tgit cat-file -p $obj2 &&\n ++\n ++\twrite_script extra-tips <<-EOF &&\n ++\techo $obj2\n ++\tEOF\n ++\tgit config pack.recentObjectsHook ./extra-tips &&\n ++\n ++\ttest-tool chmtime =-86400 .git/objects/pack/pack-$pack2.pack &&\n ++\tgit repack -A -d --unpack-unreachable=1.hour.ago &&\n ++\n ++\tgit cat-file -p $obj1 &&\n ++\tgit cat-file -p $obj2\n ++'\n ++\n + test_expect_success 'keep packed objects found only in index' '\n + \techo my-unique-content >file &&\n + \tgit add file &&\n-- \n2.40.1.554.g2ce8a79fa4\n"},{"id":"477107","messageId":"f5f3b0f334f6c26ba2f9252723a9b539371375e3.1683847221.git.me@ttaylorr.com","threadId":"59731","inReplyTo":"cover.1683847221.git.me@ttaylorr.com","subject":"[PATCH v3 1/2] reachable.c: extract `obj_is_recent()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-05-11T23:20:34Z","receivedAt":"2023-05-11T23:20:49Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"When enumerating objects in order to add recent ones (i.e. those whose\nmtime is strictly newer than the cutoff) as tips of a reachability\ntraversal, `add_recent_object()` discards objects which do not meet the\nrecency criteria.\n\nThe subsequent commit will make checking whether or not an object is\nrecent also consult the list of hooks in `pack.recentHook`. Isolate this\ncheck in its own function to keep the additional complexity outside of\n`add_recent_object()`.\n\nSigned-off-by: Taylor Blau \n---\n reachable.c | 8 +++++++-\n 1 file changed, 7 insertions(+), 1 deletion(-)\n\ndiff --git a/reachable.c b/reachable.c\nindex 55bb114353..7a42da5d39 100644\n--- a/reachable.c\n+++ b/reachable.c\n@@ -69,6 +69,12 @@ struct recent_data {\n \tint ignore_in_core_kept_packs;\n };\n \n+static int obj_is_recent(const struct object_id *oid, timestamp_t mtime,\n+\t\t\t struct recent_data *data)\n+{\n+\treturn mtime > data->timestamp;\n+}\n+\n static void add_recent_object(const struct object_id *oid,\n \t\t\t struct packed_git *pack,\n \t\t\t off_t offset,\n@@ -78,7 +84,7 @@ static void add_recent_object(const struct object_id *oid,\n \tstruct object *obj;\n \tenum object_type type;\n \n-\tif (mtime <= data->timestamp)\n+\tif (!obj_is_recent(oid, mtime, data))\n \t\treturn;\n \n \t/*\n-- \n2.40.1.554.g2ce8a79fa4\n\n"},{"id":"477108","messageId":"2ce8a79fa4bf98947728da4e6e22304a2f203fac.1683847221.git.me@ttaylorr.com","threadId":"59731","inReplyTo":"cover.1683847221.git.me@ttaylorr.com","subject":"[PATCH v3 2/2] builtin/pack-objects.c: introduce `pack.recentObjectsHook`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-05-11T23:20:37Z","receivedAt":"2023-05-11T23:21:04Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"This patch introduces a new multi-valued configuration option,\n`pack.recentObjectsHook` as a means to mark certain objects as recent,\nregardless of their age.\n\nDepending on whether or not we are generating a cruft pack, this allows\nthe caller to do one of two things:\n\n - If generating a cruft pack, the caller is able to retain additional\n objects via the cruft pack, even if they would have otherwise been\n pruned due to their age.\n\n - If not generating a cruft pack, the caller is likewise able to\n retain additional objects as loose.\n\nThere is currently no option to be able to keep around certain objects\nthat have otherwise aged out of the grace period. The only way to retain\nthose objects is:\n\n - to point a reference at them, which may be undesirable or\n infeasible,\n\n - to track them via the reflog, which may be undesirable since the\n reflog's lifetime is limited to that of the reference it's tracking\n (and callers may want to keep those unreachable objects around for\n longer)\n\n - to extend the grace period, which may keep around other objects that\n the caller *does* want to discard,\n\n - or, to force the caller to construct the pack of objects they want\n to keep themselves, and then mark the pack as kept by adding a\n \".keep\" file.\n\nThis patch introduces a new configuration, `pack.recentObjectsHook`\nwhich allows the caller to specify a program (or set of programs) whose\noutput is treated as a set of objects to treat as recent, regardless of\ntheir true age.\n\nThe implementation is straightforward. In either case (cruft packs or\nnot), Git enumerates recent objects via\n`add_unseen_recent_objects_to_traversal()`. That enumerates loose and\npacked objects, and eventually calls add_recent_object() on any objects\nfor which `want_recent_object()`'s conditions are met.\n\nThis patch modifies the recency condition from simply \"is the mtime of\nthis object more recent than the cutoff?\" to \"[...] or, is this object\nmentioned by at least one `pack.recentObjectsHook`?\".\n\nWe then add those as tips to another reachability traversal (along with\nany recent objects, if pruning), marking every object along the way\n(either adding it to the cruft pack, or writing it out as a loose\nobject).\n\nA potential alternative here is to introduce a new mode to alter the\ncontents of the reachable pack instead of the cruft one. One could\nimagine a new option to `pack-objects`, say `--extra-reachable-tips`\nthat does the same thing as above, adding the visited set of objects\nalong the traversal to the pack.\n\nBut this has the unfortunate side-effect of altering the reachability\nclosure of that pack. If parts of the unreachable object graph mentioned\nby one or more of the \"extra reachable tips\" programs is not closed,\nthen the resulting pack won't be either. This makes it impossible in the\ngeneral case to write out reachability bitmaps for that pack, since\nclosure is a requirement there.\n\nInstead, keep these unreachable objects in the cruft pack (or set of\nunreachable, loose objects) instead, to ensure that we can continue to\nhave a pack containing just reachable objects, which is always safe to\nwrite a bitmap over.\n\nHelped-by: Jeff King \nSigned-off-by: Taylor Blau \n---\n Documentation/config/pack.txt | 13 ++\n builtin/pack-objects.c | 1 +\n reachable.c | 94 ++++++++++++++-\n t/t5329-pack-objects-cruft.sh | 171 +++++++++++++++++++++++++++\n t/t7701-repack-unpack-unreachable.sh | 22 ++++\n 5 files changed, 297 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/config/pack.txt b/Documentation/config/pack.txt\nindex d4c7c9d4e4..560cb42223 100644\n--- a/Documentation/config/pack.txt\n+++ b/Documentation/config/pack.txt\n@@ -67,6 +67,19 @@ pack.deltaCacheLimit::\n \tresult once the best match for all objects is found.\n \tDefaults to 1000. Maximum value is 65535.\n \n+pack.recentObjectsHook::\n+\tWhen considering the recency of an object (e.g., when generating\n+\ta cruft pack or storing unreachable objects as loose), use the\n+\tshell to execute the specified command(s). Interpret their\n+\toutput as object IDs which Git will consider as \"recent\",\n+\tregardless of their age.\n++\n+Output must contain exactly one hex object ID per line, and nothing\n+else. Objects which cannot be found in the repository are ignored.\n+Multiple hooks are supported, but all must exit successfully, else the\n+operation (either generating a cruft pack or unpacking unreachable\n+objects) will be halted.\n+\n pack.threads::\n \tSpecifies the number of threads to spawn when searching for best\n \tdelta matches. This requires that linkgit:git-pack-objects[1]\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex a5b466839b..bd6ad016d6 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -44,6 +44,7 @@\n #include \"pack-mtimes.h\"\n #include \"parse-options.h\"\n #include \"wrapper.h\"\n+#include \"run-command.h\"\n \n /*\n * Objects we are going to pack are collected in the `to_pack` structure.\ndiff --git a/reachable.c b/reachable.c\nindex 7a42da5d39..4cca6fe93f 100644\n--- a/reachable.c\n+++ b/reachable.c\n@@ -16,6 +16,8 @@\n #include \"object-store.h\"\n #include \"pack-bitmap.h\"\n #include \"pack-mtimes.h\"\n+#include \"config.h\"\n+#include \"run-command.h\"\n \n struct connectivity_progress {\n \tstruct progress *progress;\n@@ -67,12 +69,82 @@ struct recent_data {\n \ttimestamp_t timestamp;\n \treport_recent_object_fn *cb;\n \tint ignore_in_core_kept_packs;\n+\n+\tstruct oidset extra_recent_oids;\n+\tint extra_recent_oids_loaded;\n };\n \n+static int run_one_pack_recent_objects_hook(struct oidset *set,\n+\t\t\t\t\t const char *args)\n+{\n+\tstruct child_process cmd = CHILD_PROCESS_INIT;\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tFILE *out;\n+\tint ret = 0;\n+\n+\tcmd.use_shell = 1;\n+\tcmd.out = -1;\n+\n+\tstrvec_push(&cmd.args, args);\n+\n+\tif (start_command(&cmd))\n+\t\treturn -1;\n+\n+\tout = xfdopen(cmd.out, \"r\");\n+\twhile (strbuf_getline(&buf, out) != EOF) {\n+\t\tstruct object_id oid;\n+\t\tconst char *rest;\n+\n+\t\tif (parse_oid_hex(buf.buf, &oid, &rest) || *rest) {\n+\t\t\tret = error(_(\"invalid extra cruft tip: '%s'\"), buf.buf);\n+\t\t\tgoto done;\n+\t\t}\n+\n+\t\toidset_insert(set, &oid);\n+\t}\n+\n+\tret = finish_command(&cmd);\n+\n+done:\n+\tif (out)\n+\t\tfclose(out);\n+\tstrbuf_release(&buf);\n+\tchild_process_clear(&cmd);\n+\n+\treturn ret;\n+}\n+\n+static void load_pack_recent_objects(struct recent_data *data)\n+{\n+\tconst struct string_list *programs;\n+\tint ret = 0;\n+\tsize_t i;\n+\n+\tdata->extra_recent_oids_loaded = 1;\n+\n+\tif (git_config_get_string_multi(\"pack.recentobjectshook\", &programs))\n+\t\treturn;\n+\n+\tfor (i = 0; i < programs->nr; i++) {\n+\t\tret = run_one_pack_recent_objects_hook(&data->extra_recent_oids,\n+\t\t\t\t\t\t programs->items[i].string);\n+\t\tif (ret)\n+\t\t\tbreak;\n+\t}\n+\n+\tif (ret)\n+\t\tdie(_(\"unable to enumerate additional cruft tips\"));\n+}\n+\n static int obj_is_recent(const struct object_id *oid, timestamp_t mtime,\n \t\t\t struct recent_data *data)\n {\n-\treturn mtime > data->timestamp;\n+\tif (mtime > data->timestamp)\n+\t\treturn 1;\n+\n+\tif (!data->extra_recent_oids_loaded)\n+\t\tload_pack_recent_objects(data);\n+\treturn oidset_contains(&data->extra_recent_oids, oid);\n }\n \n static void add_recent_object(const struct object_id *oid,\n@@ -126,8 +198,14 @@ static int want_recent_object(struct recent_data *data,\n \t\t\t const struct object_id *oid)\n {\n \tif (data->ignore_in_core_kept_packs &&\n-\t has_object_kept_pack(oid, IN_CORE_KEEP_PACKS))\n+\t has_object_kept_pack(oid, IN_CORE_KEEP_PACKS)) {\n+\t\tif (!data->extra_recent_oids_loaded)\n+\t\t\tload_pack_recent_objects(data);\n+\t\tif (oidset_contains(&data->extra_recent_oids, oid))\n+\t\t\treturn 1;\n+\n \t\treturn 0;\n+\t}\n \treturn 1;\n }\n \n@@ -199,16 +277,24 @@ int add_unseen_recent_objects_to_traversal(struct rev_info *revs,\n \tdata.cb = cb;\n \tdata.ignore_in_core_kept_packs = ignore_in_core_kept_packs;\n \n+\toidset_init(&data.extra_recent_oids, 0);\n+\tdata.extra_recent_oids_loaded = 0;\n+\n \tr = for_each_loose_object(add_recent_loose, &data,\n \t\t\t\t FOR_EACH_OBJECT_LOCAL_ONLY);\n \tif (r)\n-\t\treturn r;\n+\t\tgoto done;\n \n \tflags = FOR_EACH_OBJECT_LOCAL_ONLY | FOR_EACH_OBJECT_PACK_ORDER;\n \tif (ignore_in_core_kept_packs)\n \t\tflags |= FOR_EACH_OBJECT_SKIP_IN_CORE_KEPT_PACKS;\n \n-\treturn for_each_packed_object(add_recent_packed, &data, flags);\n+\tr = for_each_packed_object(add_recent_packed, &data, flags);\n+\n+done:\n+\toidset_clear(&data.extra_recent_oids);\n+\n+\treturn r;\n }\n \n static int mark_object_seen(const struct object_id *oid,\ndiff --git a/t/t5329-pack-objects-cruft.sh b/t/t5329-pack-objects-cruft.sh\nindex 303f7a5d84..ffd6c2eeb6 100755\n--- a/t/t5329-pack-objects-cruft.sh\n+++ b/t/t5329-pack-objects-cruft.sh\n@@ -739,4 +739,175 @@ test_expect_success 'cruft objects are freshend via loose' '\n \t)\n '\n \n+test_expect_success 'additional cruft tips may be specified via pack.extraCruftTips' '\n+\tgit init repo &&\n+\ttest_when_finished \"rm -fr repo\" &&\n+\t(\n+\t\tcd repo &&\n+\n+\t\t# Create a handful of objects.\n+\t\t#\n+\t\t# - one reachable commit, \"base\", designated for the reachable\n+\t\t# pack\n+\t\t# - one unreachable commit, \"cruft.discard\", which is marked\n+\t\t# for deletion\n+\t\t# - one unreachable commit, \"cruft.old\", which would be marked\n+\t\t# for deletion, but is rescued as an extra cruft tip\n+\t\t# - one unreachable commit, \"cruft.new\", which is not marked\n+\t\t# for deletion\n+\t\ttest_commit base &&\n+\t\tgit branch -M main &&\n+\n+\t\tgit checkout --orphan discard &&\n+\t\tgit rm -fr . &&\n+\t\ttest_commit --no-tag cruft.discard &&\n+\n+\t\tgit checkout --orphan old &&\n+\t\tgit rm -fr . &&\n+\t\ttest_commit --no-tag cruft.old &&\n+\t\tcruft_old=\"$(git rev-parse HEAD)\" &&\n+\n+\t\tgit checkout --orphan new &&\n+\t\tgit rm -fr . &&\n+\t\ttest_commit --no-tag cruft.new &&\n+\t\tcruft_new=\"$(git rev-parse HEAD)\" &&\n+\n+\t\tgit checkout main &&\n+\t\tgit branch -D discard old new &&\n+\t\tgit reflog expire --all --expire=all &&\n+\n+\t\t# mark cruft.old with an mtime that is many minutes\n+\t\t# older than the expiration period, and mark cruft.new\n+\t\t# with an mtime that is in the future (and thus not\n+\t\t# eligible for pruning).\n+\t\ttest-tool chmtime -2000 \"$objdir/$(test_oid_to_path $cruft_old)\" &&\n+\t\ttest-tool chmtime +1000 \"$objdir/$(test_oid_to_path $cruft_new)\" &&\n+\n+\t\t# Write the list of cruft objects we expect to\n+\t\t# accumulate, which is comprised of everything reachable\n+\t\t# from cruft.old and cruft.new, but not cruft.discard.\n+\t\tgit rev-list --objects --no-object-names \\\n+\t\t\t$cruft_old $cruft_new >cruft.raw &&\n+\t\tsort cruft.raw >cruft.expect &&\n+\n+\t\t# Write the script to list extra tips, which are limited\n+\t\t# to cruft.old, in this case.\n+\t\twrite_script extra-tips <<-EOF &&\n+\t\techo $cruft_old\n+\t\tEOF\n+\t\tgit config pack.recentObjectsHook ./extra-tips &&\n+\n+\t\tgit repack --cruft --cruft-expiration=now -d &&\n+\n+\t\tmtimes=\"$(ls .git/objects/pack/pack-*.mtimes)\" &&\n+\t\tgit show-index <${mtimes%.mtimes}.idx >cruft &&\n+\t\tcut -d\" \" -f2 cruft | sort >cruft.actual &&\n+\t\ttest_cmp cruft.expect cruft.actual &&\n+\n+\t\t# Ensure that the \"old\" objects are removed after\n+\t\t# dropping the pack.extraCruftTips hook.\n+\t\tgit config --unset pack.recentObjectsHook &&\n+\t\tgit repack --cruft --cruft-expiration=now -d &&\n+\n+\t\tmtimes=\"$(ls .git/objects/pack/pack-*.mtimes)\" &&\n+\t\tgit show-index <${mtimes%.mtimes}.idx >cruft &&\n+\t\tcut -d\" \" -f2 cruft | sort >cruft.actual &&\n+\n+\t\tgit rev-list --objects --no-object-names $cruft_new >cruft.raw &&\n+\t\tcp cruft.expect cruft.old &&\n+\t\tsort cruft.raw >cruft.expect &&\n+\t\ttest_cmp cruft.expect cruft.actual &&\n+\n+\t\t# ensure objects which are no longer in the cruft pack were\n+\t\t# removed from the repository\n+\t\tfor object in $(comm -13 cruft.expect cruft.old)\n+\t\tdo\n+\t\t\ttest_must_fail git cat-file -t $object || return 1\n+\t\tdone\n+\t)\n+'\n+\n+test_expect_success 'multi-valued pack.extraCruftTips' '\n+\tgit init repo &&\n+\ttest_when_finished \"rm -fr repo\" &&\n+\t(\n+\t\tcd repo &&\n+\n+\t\ttest_commit base &&\n+\t\tgit branch -M main &&\n+\n+\t\tgit checkout --orphan cruft.a &&\n+\t\tgit rm -fr . &&\n+\t\ttest_commit --no-tag cruft.a &&\n+\t\tcruft_a=\"$(git rev-parse HEAD)\" &&\n+\n+\t\tgit checkout --orphan cruft.b &&\n+\t\tgit rm -fr . &&\n+\t\ttest_commit --no-tag cruft.b &&\n+\t\tcruft_b=\"$(git rev-parse HEAD)\" &&\n+\n+\t\tgit checkout main &&\n+\t\tgit branch -D cruft.a cruft.b &&\n+\t\tgit reflog expire --all --expire=all &&\n+\n+\t\techo \"echo $cruft_a\" | write_script extra-tips.a &&\n+\t\techo \"echo $cruft_b\" | write_script extra-tips.b &&\n+\t\techo \"false\" | write_script extra-tips.c &&\n+\n+\t\tgit rev-list --objects --no-object-names $cruft_a $cruft_b \\\n+\t\t\t>cruft.raw &&\n+\t\tsort cruft.raw >cruft.expect &&\n+\n+\t\t# ensure that each extra cruft tip is saved by its\n+\t\t# respective hook\n+\t\tgit config --add pack.recentObjectsHook ./extra-tips.a &&\n+\t\tgit config --add pack.recentObjectsHook ./extra-tips.b &&\n+\t\tgit repack --cruft --cruft-expiration=now -d &&\n+\n+\t\tmtimes=\"$(ls .git/objects/pack/pack-*.mtimes)\" &&\n+\t\tgit show-index <${mtimes%.mtimes}.idx >cruft &&\n+\t\tcut -d\" \" -f2 cruft | sort >cruft.actual &&\n+\t\ttest_cmp cruft.expect cruft.actual &&\n+\n+\t\t# ensure that a dirty exit halts cruft pack generation\n+\t\tgit config --add pack.recentObjectsHook ./extra-tips.c &&\n+\t\ttest_must_fail git repack --cruft --cruft-expiration=now -d 2>err &&\n+\t\tgrep \"unable to enumerate additional cruft tips\" err &&\n+\n+\t\t# and that the existing cruft pack is left alone\n+\t\ttest_path_is_file \"$mtimes\"\n+\t)\n+'\n+\n+test_expect_success 'additional cruft blobs via pack.extraCruftTips' '\n+\tgit init repo &&\n+\ttest_when_finished \"rm -fr repo\" &&\n+\t(\n+\t\tcd repo &&\n+\n+\t\ttest_commit base &&\n+\n+\t\tblob=$(echo \"unreachable\" | git hash-object -w --stdin) &&\n+\n+\t\t# mark the unreachable blob we wrote above as having\n+\t\t# aged out of the retention period\n+\t\ttest-tool chmtime -2000 \"$objdir/$(test_oid_to_path $blob)\" &&\n+\n+\t\t# Write the script to list extra tips, which is just the\n+\t\t# extra blob as above.\n+\t\twrite_script extra-tips <<-EOF &&\n+\t\techo $blob\n+\t\tEOF\n+\t\tgit config pack.recentObjectsHook ./extra-tips &&\n+\n+\t\tgit repack --cruft --cruft-expiration=now -d &&\n+\n+\t\tmtimes=\"$(ls .git/objects/pack/pack-*.mtimes)\" &&\n+\t\tgit show-index <${mtimes%.mtimes}.idx >cruft &&\n+\t\tcut -d\" \" -f2 cruft >actual &&\n+\t\techo $blob >expect &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n test_done\ndiff --git a/t/t7701-repack-unpack-unreachable.sh b/t/t7701-repack-unpack-unreachable.sh\nindex ebb267855f..d2eea6e754 100755\n--- a/t/t7701-repack-unpack-unreachable.sh\n+++ b/t/t7701-repack-unpack-unreachable.sh\n@@ -113,6 +113,28 @@ test_expect_success 'do not bother loosening old objects' '\n \ttest_must_fail git cat-file -p $obj2\n '\n \n+test_expect_success 'extra recent tips are kept regardless of age' '\n+\tobj1=$(echo one | git hash-object -w --stdin) &&\n+\tobj2=$(echo two | git hash-object -w --stdin) &&\n+\tpack1=$(echo $obj1 | git pack-objects .git/objects/pack/pack) &&\n+\tpack2=$(echo $obj2 | git pack-objects .git/objects/pack/pack) &&\n+\tgit prune-packed &&\n+\n+\tgit cat-file -p $obj1 &&\n+\tgit cat-file -p $obj2 &&\n+\n+\twrite_script extra-tips <<-EOF &&\n+\techo $obj2\n+\tEOF\n+\tgit config pack.recentObjectsHook ./extra-tips &&\n+\n+\ttest-tool chmtime =-86400 .git/objects/pack/pack-$pack2.pack &&\n+\tgit repack -A -d --unpack-unreachable=1.hour.ago &&\n+\n+\tgit cat-file -p $obj1 &&\n+\tgit cat-file -p $obj2\n+'\n+\n test_expect_success 'keep packed objects found only in index' '\n \techo my-unique-content >file &&\n \tgit add file &&\n-- \n2.40.1.554.g2ce8a79fa4\n"},{"id":"477109","messageId":"ZF143FzAjTZJ00/O@nand.local","threadId":"59731","inReplyTo":"cover.1683847221.git.me@ttaylorr.com","subject":"Re: [PATCH v3 0/2] pack-objects: introduce `pack.extraRecentObjectsHook`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-05-11T23:23:08Z","receivedAt":"2023-05-11T23:23:14Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, May 11, 2023 at 07:20:31PM -0400, Taylor Blau wrote:\n> Here is a reworked version of the patch which introduced a new\n> configuration `pack.extraCruftTips` to keep additional objects from\n> pruning during a cruft pack-generating GC.\n\nI'm starting a new thread disconnected from the old one since the\napproach is significantly different (but mostly because I forgot to set\n`--in-reply-to` when preparing these patches ;-)).\n\nStrictly speaking, I think this is technically \"v4\" of the original\ntopic, so v3 here is a mislabel.\n\nThose interested in following along with the earlier round(s) of\ndiscussion can see the original thread at [1].\n\nThanks,\nTaylor\n\n[1]: https://lore.kernel.org/git/8af478ebe34539b68ffb9b353bbb1372dfca3871.1682011600.git.me@ttaylorr.com/\n\n"},{"id":"477111","messageId":"xmqqttwisaj5.fsf@gitster.g","threadId":"59731","inReplyTo":"cover.1683847221.git.me@ttaylorr.com","subject":"Re: [PATCH v3 0/2] pack-objects: introduce `pack.extraRecentObjectsHook`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-05-11T23:39:26Z","receivedAt":"2023-05-11T23:39:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> Here is a reworked version of the patch which introduced a new\n> configuration `pack.extraCruftTips` to keep additional objects from\n> pruning during a cruft pack-generating GC.\n>\n> The third round was significantly more complicated than necessary, and I\n\nHopefully you just meant \"the previous round\"; in a follow-up you\nmention that this is v4 and not v3, which is also a good explanation\nfor that \"third round was too complex\".\n\n> - The new code does not change the existing cruft pack implementation\n> whatsoever. This is nice, since cruft packs will be the default in\n> the next release, so changing that implementation carries additional\n> risk.\n>\n> - The new code is also not specific to cruft packs, meaning that you\n> can do things like:\n>\n> $ git -c pack.extraRecentObjectsHook=... \\\n> repack -Ad --unpack-unreachable=1.hour.ago\n>\n> and have it write out loose copies of any object(s) mentioned by one\n> or more of the configured hooks.\n\nOK.\n\n> I am hopeful that others think this version is in a good spot. As in\n> earlier rounds, I would appreciate an extra careful review on this\n> topic, because of the changing default I mentioned earlier.\n>\n> Thanks in advance for your review.\n\nThanks for working on it. I am in the process of pushing out\ntoday's integration cycle already, so this topic will have to wait\nuntil tomorrow's, though.\n\n\n\n"},{"id":"477112","messageId":"ZF1+5PSgo3zAOYS+@nand.local","threadId":"59731","inReplyTo":"xmqqttwisaj5.fsf@gitster.g","subject":"Re: [PATCH v3 0/2] pack-objects: introduce `pack.extraRecentObjectsHook`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-05-11T23:48:52Z","receivedAt":"2023-05-11T23:49:03Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, May 11, 2023 at 04:39:26PM -0700, Junio C Hamano wrote:\n> Taylor Blau writes:\n>\n> > Here is a reworked version of the patch which introduced a new\n> > configuration `pack.extraCruftTips` to keep additional objects from\n> > pruning during a cruft pack-generating GC.\n> >\n> > The third round was significantly more complicated than necessary, and I\n>\n> Hopefully you just meant \"the previous round\"; in a follow-up you\n> mention that this is v4 and not v3, which is also a good explanation\n> for that \"third round was too complex\".\n\nYes, this series being the true fourth round, I was referring to the\nversion I sent in:\n\n https://lore.kernel.org/git/27a7f16aab35b5cac391d9831aadb0f2e2146313.1683151485.git.me@ttaylorr.com/\n\n...which this version is significantly less complicated than.\n\n> > I am hopeful that others think this version is in a good spot. As in\n> > earlier rounds, I would appreciate an extra careful review on this\n> > topic, because of the changing default I mentioned earlier.\n> >\n> > Thanks in advance for your review.\n>\n> Thanks for working on it. I am in the process of pushing out\n> today's integration cycle already, so this topic will have to wait\n> until tomorrow's, though.\n\nNo problem. Happy merging ;-).\n\nThanks,\nTaylor\n"},{"id":"477130","messageId":"20230512045853.GA2479240@coredump.intra.peff.net","threadId":"59731","inReplyTo":"2ce8a79fa4bf98947728da4e6e22304a2f203fac.1683847221.git.me@ttaylorr.com","subject":"Re: [PATCH v3 2/2] builtin/pack-objects.c: introduce `pack.recentObjectsHook`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-05-12T04:58:53Z","receivedAt":"2023-05-12T04:58:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 11, 2023 at 07:20:37PM -0400, Taylor Blau wrote:\n\n> +static int run_one_pack_recent_objects_hook(struct oidset *set,\n> +\t\t\t\t\t const char *args)\n> +{\n> +\tstruct child_process cmd = CHILD_PROCESS_INIT;\n> +\tstruct strbuf buf = STRBUF_INIT;\n> +\tFILE *out;\n> +\tint ret = 0;\n> +\n> +\tcmd.use_shell = 1;\n> +\tcmd.out = -1;\n> +\n> +\tstrvec_push(&cmd.args, args);\n> +\n> +\tif (start_command(&cmd))\n> +\t\treturn -1;\n> +\n> +\tout = xfdopen(cmd.out, \"r\");\n> +\twhile (strbuf_getline(&buf, out) != EOF) {\n> +\t\tstruct object_id oid;\n> +\t\tconst char *rest;\n> +\n> +\t\tif (parse_oid_hex(buf.buf, &oid, &rest) || *rest) {\n> +\t\t\tret = error(_(\"invalid extra cruft tip: '%s'\"), buf.buf);\n> +\t\t\tgoto done;\n> +\t\t}\n> +\n> +\t\toidset_insert(set, &oid);\n> +\t}\n> +\n> +\tret = finish_command(&cmd);\n> +\n> +done:\n\nI haven't looked closely at this whole patch yet (and I especially want\nto look at the new tests since this approach covers more cases), but I\ndid notice that this version of the function still has the \"we don't\nreap the child on parse failure\" problem I described in:\n\n https://lore.kernel.org/git/20230505221921.GE3321533@coredump.intra.peff.net/\n\nI'll try to give the whole patch a more thorough review tomorrow.\n\n-Peff\n"},{"id":"477218","messageId":"20230512212456.GA2495860@coredump.intra.peff.net","threadId":"59731","inReplyTo":"2ce8a79fa4bf98947728da4e6e22304a2f203fac.1683847221.git.me@ttaylorr.com","subject":"Re: [PATCH v3 2/2] builtin/pack-objects.c: introduce `pack.recentObjectsHook`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-05-12T21:24:56Z","receivedAt":"2023-05-12T21:25:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 11, 2023 at 07:20:37PM -0400, Taylor Blau wrote:\n\n> This patch introduces a new multi-valued configuration option,\n> `pack.recentObjectsHook` as a means to mark certain objects as recent,\n> regardless of their age.\n> \n> Depending on whether or not we are generating a cruft pack, this allows\n> the caller to do one of two things:\n> \n> - If generating a cruft pack, the caller is able to retain additional\n> objects via the cruft pack, even if they would have otherwise been\n> pruned due to their age.\n> \n> - If not generating a cruft pack, the caller is likewise able to\n> retain additional objects as loose.\n\nI think this description suffers a bit from being adapted from the\noriginal patch which was targeting cruft packs. It's not clear to me\nwhat \"the caller\" means here. And really, I think this is getting into\nthe details before giving an overview and motivation.\n\nI'd expect something the rationale to be something like:\n\n 1. Git considers the recent-ness of objects when deciding how to handle\n unreachable objects (discarding those that are too old, and keeping\n new-enough ones)\n\n 2. you might want to consider more objects as \"recent\" because you know\n something Git doesn't (that certain objects are more interesting\n than others; specific examples may help here)\n\n 3. so this patch gives you a way to override the recent-ness check for\n certain objects\n\nAnd then get into the details of the implementation, why other solutions\ndon't work, etc.\n\n> There is currently no option to be able to keep around certain objects\n> that have otherwise aged out of the grace period. The only way to retain\n> those objects is:\n> \n> - to point a reference at them, which may be undesirable or\n> infeasible,\n> \n> - to track them via the reflog, which may be undesirable since the\n> reflog's lifetime is limited to that of the reference it's tracking\n> (and callers may want to keep those unreachable objects around for\n> longer)\n> \n> - to extend the grace period, which may keep around other objects that\n> the caller *does* want to discard,\n> \n> - or, to force the caller to construct the pack of objects they want\n> to keep themselves, and then mark the pack as kept by adding a\n> \".keep\" file.\n\nOne option I don't see here is: update the mtime on the objects you want\nto salvage.\n\nWhy would we want this patch instead of just having the caller update\nthe mtimes of objects (or in a cruft-pack world, call a command that\nrewrites the .mtimes file with new values)?\n\nI can think of some possible arguments against it (you might want to\nretain the old mtimes, or you might find it a hassle to have to\ncontinually update them before gc kills them). But I think the commit\nmessage should probably make those arguments.\n\nLikewise, I think you'd want to argue why the \".keep\" approach isn't as\ngood (and I guess it is \"having extra packs is awkward especially as you\nroll forward your cruft packs\").\n\n> This patch introduces a new configuration, `pack.recentObjectsHook`\n> which allows the caller to specify a program (or set of programs) whose\n> output is treated as a set of objects to treat as recent, regardless of\n> their true age.\n\nI was going to complain about putting this in the \"pack\" section,\nbecause I thought by touching reachable.c, we'd also affect git-prune.\nBut I don't think we do, because it does its own direct mtime check on\nthe loose objects.\n\nBut I'm not sure that's the right behavior.\n\nIt feels like even before your patch, this is a huge gap in our\nobject-retention strategy. During repacking, we try to avoid dropping\nobjects which are reachable from recent-but-unreachable things we're\nkeeping (since otherwise it effectively corrupts those recent objects,\nmaking them less valuable to keep). But git-prune will happily drop them\nanyway!\n\nAnd I think the same thing would apply to your hook. If the hook says\n\"object XYZ is precious even if unreachable, keep it\", then git-prune\nignoring that seems like it would be a source of errors.\n\nI suspect both could be fixed by having git-prune trigger the same\nadd_unseen_recent_objects_to_traversal() call either as part of\nthe perform_reachability_traversal() walk, or maybe in its own walk (I\nthink maybe it has to be its own because the second walk should avoid\ncomplaining about missing objects).\n\n> We then add those as tips to another reachability traversal (along with\n> any recent objects, if pruning), marking every object along the way\n> (either adding it to the cruft pack, or writing it out as a loose\n> object).\n\nI didn't understand this \"if pruning\" comment. If we are not pruning at\nall, wouldn't we skip the extra traversal entirely, since we know we are\nsaving everything?\n\n> A potential alternative here is to introduce a new mode to alter the\n> contents of the reachable pack instead of the cruft one. One could\n> imagine a new option to `pack-objects`, say `--extra-reachable-tips`\n> that does the same thing as above, adding the visited set of objects\n> along the traversal to the pack.\n> \n> But this has the unfortunate side-effect of altering the reachability\n> closure of that pack. If parts of the unreachable object graph mentioned\n> by one or more of the \"extra reachable tips\" programs is not closed,\n> then the resulting pack won't be either. This makes it impossible in the\n> general case to write out reachability bitmaps for that pack, since\n> closure is a requirement there.\n\nMakes sense.\n\n> +\twhile (strbuf_getline(&buf, out) != EOF) {\n> +\t\tstruct object_id oid;\n> +\t\tconst char *rest;\n> +\n> +\t\tif (parse_oid_hex(buf.buf, &oid, &rest) || *rest) {\n> +\t\t\tret = error(_(\"invalid extra cruft tip: '%s'\"), buf.buf);\n> +\t\t\tgoto done;\n> +\t\t}\n> +\n> +\t\toidset_insert(set, &oid);\n> +\t}\n> +\n> +\tret = finish_command(&cmd);\n> +\n> +done:\n\nI mentioned the \"goto done\" issue in another email.\n\n> static int obj_is_recent(const struct object_id *oid, timestamp_t mtime,\n> \t\t\t struct recent_data *data)\n> {\n> -\treturn mtime > data->timestamp;\n> +\tif (mtime > data->timestamp)\n> +\t\treturn 1;\n> +\n> +\tif (!data->extra_recent_oids_loaded)\n> +\t\tload_pack_recent_objects(data);\n> +\treturn oidset_contains(&data->extra_recent_oids, oid);\n> }\n\nOK, so this is where we override the timestamp check. Makes sense.\n\n> @@ -126,8 +198,14 @@ static int want_recent_object(struct recent_data *data,\n> \t\t\t const struct object_id *oid)\n> {\n> \tif (data->ignore_in_core_kept_packs &&\n> -\t has_object_kept_pack(oid, IN_CORE_KEEP_PACKS))\n> +\t has_object_kept_pack(oid, IN_CORE_KEEP_PACKS)) {\n> +\t\tif (!data->extra_recent_oids_loaded)\n> +\t\t\tload_pack_recent_objects(data);\n> +\t\tif (oidset_contains(&data->extra_recent_oids, oid))\n> +\t\t\treturn 1;\n> +\n> \t\treturn 0;\n> +\t}\n> \treturn 1;\n> }\n\nThis hunk I'm less sure about. The purpose of this function is that the\ncaller has told us about some packs which are \"special\", and we avoid\nadding their objects to the traversal.\n\nThis kicks in for cruft packs, when the git-repack caller says \"I just\nmade pack xyz.pack; do not bother saving anything in it to the cruft\npack, since xyz.pack is here to stay\". So if a hook says \"you should\nkeep object X\", why would we want to override that check? It is already\na reachable object that has been packed into xyz.pack, so we know there\nis no point in even considering its recency.\n\n> @@ -199,16 +277,24 @@ int add_unseen_recent_objects_to_traversal(struct rev_info *revs,\n> \tdata.cb = cb;\n> \tdata.ignore_in_core_kept_packs = ignore_in_core_kept_packs;\n> \n> +\toidset_init(&data.extra_recent_oids, 0);\n> +\tdata.extra_recent_oids_loaded = 0;\n> +\n> \tr = for_each_loose_object(add_recent_loose, &data,\n> \t\t\t\t FOR_EACH_OBJECT_LOCAL_ONLY);\n> \tif (r)\n> -\t\treturn r;\n> +\t\tgoto done;\n> \n> \tflags = FOR_EACH_OBJECT_LOCAL_ONLY | FOR_EACH_OBJECT_PACK_ORDER;\n> \tif (ignore_in_core_kept_packs)\n> \t\tflags |= FOR_EACH_OBJECT_SKIP_IN_CORE_KEPT_PACKS;\n> \n> -\treturn for_each_packed_object(add_recent_packed, &data, flags);\n> +\tr = for_each_packed_object(add_recent_packed, &data, flags);\n> +\n> +done:\n> +\toidset_clear(&data.extra_recent_oids);\n> +\n> +\treturn r;\n> }\n\nThis handles the cleanup that I was too lazy to do in my earlier patch.\nGood. :)\n\n> --- a/t/t5329-pack-objects-cruft.sh\n> +++ b/t/t5329-pack-objects-cruft.sh\n> @@ -739,4 +739,175 @@ test_expect_success 'cruft objects are freshend via loose' '\n> \t)\n> '\n> \n> +test_expect_success 'additional cruft tips may be specified via pack.extraCruftTips' '\n\nThis title (and others below) seems out of date. :)\n\n> diff --git a/t/t7701-repack-unpack-unreachable.sh b/t/t7701-repack-unpack-unreachable.sh\n> index ebb267855f..d2eea6e754 100755\n> --- a/t/t7701-repack-unpack-unreachable.sh\n> +++ b/t/t7701-repack-unpack-unreachable.sh\n> @@ -113,6 +113,28 @@ test_expect_success 'do not bother loosening old objects' '\n> \ttest_must_fail git cat-file -p $obj2\n> '\n> \n> +test_expect_success 'extra recent tips are kept regardless of age' '\n> +\tobj1=$(echo one | git hash-object -w --stdin) &&\n> +\tobj2=$(echo two | git hash-object -w --stdin) &&\n> +\tpack1=$(echo $obj1 | git pack-objects .git/objects/pack/pack) &&\n> +\tpack2=$(echo $obj2 | git pack-objects .git/objects/pack/pack) &&\n> +\tgit prune-packed &&\n> +\n> +\tgit cat-file -p $obj1 &&\n> +\tgit cat-file -p $obj2 &&\n> +\n> +\twrite_script extra-tips <<-EOF &&\n> +\techo $obj2\n> +\tEOF\n> +\tgit config pack.recentObjectsHook ./extra-tips &&\n> +\n> +\ttest-tool chmtime =-86400 .git/objects/pack/pack-$pack2.pack &&\n> +\tgit repack -A -d --unpack-unreachable=1.hour.ago &&\n> +\n> +\tgit cat-file -p $obj1 &&\n> +\tgit cat-file -p $obj2\n> +'\n\nAnd this is the new test in this iteration covering the \"repack -A\"\ncase.\n\nIt is checking that $obj2, which our hook mentions, is saved. It also\nchecks that $obj1 is saved because it is still recent. But there are two\nother possibly interesting cases:\n\n - an object that is too old and is _not_ saved. It seems useful to\n confirm that the new patch does not simply break the ability to drop\n objects. ;)\n\n - an object that is reachable from $obj2 is also saved. From a\n white-box perspective this is less interesting, because we should\n already test elsewhere that this works for recent objects, and we\n know the new feature is implemented by faking recency. But it might\n be worth it for completeness, and because it's easy to do (making\n $obj2 a tag pointing to a blob should work).\n\n-Peff\n"},{"id":"477227","messageId":"ZF6xT34/Zq5pT9fG@nand.local","threadId":"59731","inReplyTo":"20230512212456.GA2495860@coredump.intra.peff.net","subject":"Re: [PATCH v3 2/2] builtin/pack-objects.c: introduce `pack.recentObjectsHook`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-05-12T21:36:15Z","receivedAt":"2023-05-12T21:36:21Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, May 12, 2023 at 05:24:56PM -0400, Jeff King wrote:\n> > This patch introduces a new configuration, `pack.recentObjectsHook`\n> > which allows the caller to specify a program (or set of programs) whose\n> > output is treated as a set of objects to treat as recent, regardless of\n> > their true age.\n>\n> I was going to complain about putting this in the \"pack\" section,\n> because I thought by touching reachable.c, we'd also affect git-prune.\n> But I don't think we do, because it does its own direct mtime check on\n> the loose objects.\n>\n> But I'm not sure that's the right behavior.\n>\n> It feels like even before your patch, this is a huge gap in our\n> object-retention strategy. During repacking, we try to avoid dropping\n> objects which are reachable from recent-but-unreachable things we're\n> keeping (since otherwise it effectively corrupts those recent objects,\n> making them less valuable to keep). But git-prune will happily drop them\n> anyway!\n>\n> And I think the same thing would apply to your hook. If the hook says\n> \"object XYZ is precious even if unreachable, keep it\", then git-prune\n> ignoring that seems like it would be a source of errors.\n>\n> I suspect both could be fixed by having git-prune trigger the same\n> add_unseen_recent_objects_to_traversal() call either as part of\n> the perform_reachability_traversal() walk, or maybe in its own walk (I\n> think maybe it has to be its own because the second walk should avoid\n> complaining about missing objects).\n\nI might be missing something, but I think we already (kind of) do the\nright thing here.\n\nAFAICT, the path is:\n\n - cmd_prune()\n - for_each_loose_file_in_objdir()\n - prune_object() (as a callback to the above)\n - is_object_reachable()\n - perform_reachability_traversal()\n - mark_reachable_objects()\n - add_unseen_recent_objects_to_traversal()\n\nThat only happens when `mark_recent != 0`, though.\n\nThanks,\nTaylor\n"},{"id":"477229","messageId":"20230512214542.GB2495860@coredump.intra.peff.net","threadId":"59731","inReplyTo":"20230512212456.GA2495860@coredump.intra.peff.net","subject":"Re: [PATCH v3 2/2] builtin/pack-objects.c: introduce `pack.recentObjectsHook`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-05-12T21:45:42Z","receivedAt":"2023-05-12T21:45:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 12, 2023 at 05:24:56PM -0400, Jeff King wrote:\n\n> > This patch introduces a new configuration, `pack.recentObjectsHook`\n> > which allows the caller to specify a program (or set of programs) whose\n> > output is treated as a set of objects to treat as recent, regardless of\n> > their true age.\n> \n> I was going to complain about putting this in the \"pack\" section,\n> because I thought by touching reachable.c, we'd also affect git-prune.\n> But I don't think we do, because it does its own direct mtime check on\n> the loose objects.\n> \n> But I'm not sure that's the right behavior.\n> \n> It feels like even before your patch, this is a huge gap in our\n> object-retention strategy. During repacking, we try to avoid dropping\n> objects which are reachable from recent-but-unreachable things we're\n> keeping (since otherwise it effectively corrupts those recent objects,\n> making them less valuable to keep). But git-prune will happily drop them\n> anyway!\n> \n> And I think the same thing would apply to your hook. If the hook says\n> \"object XYZ is precious even if unreachable, keep it\", then git-prune\n> ignoring that seems like it would be a source of errors.\n> \n> I suspect both could be fixed by having git-prune trigger the same\n> add_unseen_recent_objects_to_traversal() call either as part of\n> the perform_reachability_traversal() walk, or maybe in its own walk (I\n> think maybe it has to be its own because the second walk should avoid\n> complaining about missing objects).\n\n I am happy to say that I was wrong here, and git-prune behaves as\nit should, courtesy of d3038d22f9 (prune: keep objects reachable from\nrecent objects, 2014-10-15). The magic happens in mark_reachable_objects(),\nwhich handles walking the recent objects by calling...you guessed it,\nadd_unseen_recent_objects_to_traversal().\n\nSo it does the right thing now, and your patch should kick in\nautomatically for git-prune, too. But I think we'd want two things:\n\n 1. Should the config variable name be made more generic to match?\n Maybe \"core\" is too broad (though certainly I'd expect it to apply\n anywhere in Git where we check recent-ness of objects), but perhaps\n \"gc\" would make sense (even though it is not strictly part of the\n gc command, it is within that realm of concepts).\n\n 2. We probably want a test covering git-prune in this situation.\n\nThe thing that confused me when looking at the code earlier is that\ngit-prune itself checks the mtime of the objects, too, even if they were\nnot mentioned in the recent-but-reachable walk. That seems redundant to\nme, but somehow it isn't. If I do:\n\ndiff --git a/builtin/prune.c b/builtin/prune.c\nindex 5dc9b20720..22b7ce4b10 100644\n--- a/builtin/prune.c\n+++ b/builtin/prune.c\n@@ -92,7 +92,7 @@ static int prune_object(const struct object_id *oid, const char *fullpath,\n \t\treturn 0;\n \t}\n \tif (st.st_mtime > expire)\n-\t\treturn 0;\n+\t\tBUG(\"object should have been saved via is_object_reachable!\");\n \tif (show_only || verbose) {\n \t\tenum object_type type = oid_object_info(the_repository, oid,\n \t\t\t\t\t\t\tNULL);\n\nthen t5304 shows some failures. I'm not quite sure what is going on, but\n_if_ that mtime check in prune_object() is important, then it probably\nshould also respect the hook mechanism. So there may be a (3) to add to\nyour patch there.\n\n-Peff\n"},{"id":"477230","messageId":"20230512214618.GC2495860@coredump.intra.peff.net","threadId":"59731","inReplyTo":"ZF6xT34/Zq5pT9fG@nand.local","subject":"Re: [PATCH v3 2/2] builtin/pack-objects.c: introduce `pack.recentObjectsHook`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-05-12T21:46:18Z","receivedAt":"2023-05-12T21:46:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 12, 2023 at 05:36:15PM -0400, Taylor Blau wrote:\n\n> > I suspect both could be fixed by having git-prune trigger the same\n> > add_unseen_recent_objects_to_traversal() call either as part of\n> > the perform_reachability_traversal() walk, or maybe in its own walk (I\n> > think maybe it has to be its own because the second walk should avoid\n> > complaining about missing objects).\n> \n> I might be missing something, but I think we already (kind of) do the\n> right thing here.\n> \n> AFAICT, the path is:\n> \n> - cmd_prune()\n> - for_each_loose_file_in_objdir()\n> - prune_object() (as a callback to the above)\n> - is_object_reachable()\n> - perform_reachability_traversal()\n> - mark_reachable_objects()\n> - add_unseen_recent_objects_to_traversal()\n> \n> That only happens when `mark_recent != 0`, though.\n\nYeah, I'm sorry, I was totally wrong here. See the mail I just sent that\ncrossed paths with yours.\n\n-Peff\n"},{"id":"477233","messageId":"20230512220117.GA2971903@coredump.intra.peff.net","threadId":"59731","inReplyTo":"20230512214542.GB2495860@coredump.intra.peff.net","subject":"Re: [PATCH v3 2/2] builtin/pack-objects.c: introduce `pack.recentObjectsHook`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-05-12T22:01:17Z","receivedAt":"2023-05-12T22:01:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 12, 2023 at 05:45:43PM -0400, Jeff King wrote:\n\n> The thing that confused me when looking at the code earlier is that\n> git-prune itself checks the mtime of the objects, too, even if they were\n> not mentioned in the recent-but-reachable walk. That seems redundant to\n> me, but somehow it isn't. If I do:\n> \n> diff --git a/builtin/prune.c b/builtin/prune.c\n> index 5dc9b20720..22b7ce4b10 100644\n> --- a/builtin/prune.c\n> +++ b/builtin/prune.c\n> @@ -92,7 +92,7 @@ static int prune_object(const struct object_id *oid, const char *fullpath,\n> \t\treturn 0;\n> \t}\n> \tif (st.st_mtime > expire)\n> -\t\treturn 0;\n> +\t\tBUG(\"object should have been saved via is_object_reachable!\");\n> \tif (show_only || verbose) {\n> \t\tenum object_type type = oid_object_info(the_repository, oid,\n> \t\t\t\t\t\t\tNULL);\n> \n> then t5304 shows some failures. I'm not quite sure what is going on, but\n> _if_ that mtime check in prune_object() is important, then it probably\n> should also respect the hook mechanism. So there may be a (3) to add to\n> your patch there.\n\nAh, I see. The problem is that \"git prune --expire=never\" relies on this\ncheck. In mark_reachable_objects(), we take a \"0\" expiration as \"do not\nbother adding these objects to the traversal\", so it's here that we\ncatch them and say \"ah, we are not expiring anything, so keep this\".\n\nIf my hack above is switched to:\n\ndiff --git a/builtin/prune.c b/builtin/prune.c\nindex 5dc9b20720..108305cd26 100644\n--- a/builtin/prune.c\n+++ b/builtin/prune.c\n@@ -91,8 +91,11 @@ static int prune_object(const struct object_id *oid, const char *fullpath,\n \t\terror(\"Could not stat '%s'\", fullpath);\n \t\treturn 0;\n \t}\n-\tif (st.st_mtime > expire)\n-\t\treturn 0;\n+\tif (st.st_mtime > expire) {\n+\t\tif (!expire)\n+\t\t\treturn 0; /* we are not expiring anything! */\n+\t\tBUG(\"object should have been saved via is_object_reachable!\");\n+\t}\n \tif (show_only || verbose) {\n \t\tenum object_type type = oid_object_info(the_repository, oid,\n \t\t\t\t\t\t\tNULL);\n\nthen all of the tests pass. I do suspect this code could trigger racily\nin the real world (we do our traversal, and then a new object is added,\nwhich is of course recent). So we would not want to drop the check.\n\nIs it important there for your patch to kick in and say \"we were\nspecifically asked to consider this object recent, so do so\"? I think it\nshouldn't matter, because any such object is by definition recent,\nhaving an mtime greater than or equal to when we started (it cannot even\nget racily old while we traverse, because we decide the cutoff time as\nan absolute value even before starting the traversal). The strict\ngreater-than in the check makes it feel like an off-by-one problem, but\nunless you are saying \"now\" it must be at least 1 second in the past\nanyway.\n\nIt does make me wonder why \"git prune --expire=never\" does not simply\nexit immediately. Isn't there by definition nothing useful to do?\n\n-Peff\n"},{"id":"477239","messageId":"xmqq4johnnjr.fsf@gitster.g","threadId":"59731","inReplyTo":"20230512220117.GA2971903@coredump.intra.peff.net","subject":"Re: [PATCH v3 2/2] builtin/pack-objects.c: introduce `pack.recentObjectsHook`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-05-12T23:21:44Z","receivedAt":"2023-05-12T23:21:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> It does make me wonder why \"git prune --expire=never\" does not simply\n> exit immediately. Isn't there by definition nothing useful to do?\n\nI think the answer to the question is \"no\", but if I have to guess\nwhy such a low-hanging fruit optimization possibility has not been\nexploited so far is because it does not optimize a useful case,\nperhaps? IOW, falling into \"if it hurts, then don't do it\" category?\n\n"},{"id":"477241","messageId":"20230513001117.GA2974285@coredump.intra.peff.net","threadId":"59731","inReplyTo":"xmqq4johnnjr.fsf@gitster.g","subject":"Re: [PATCH v3 2/2] builtin/pack-objects.c: introduce `pack.recentObjectsHook`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-05-13T00:11:17Z","receivedAt":"2023-05-13T00:11:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 12, 2023 at 04:21:44PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > It does make me wonder why \"git prune --expire=never\" does not simply\n> > exit immediately. Isn't there by definition nothing useful to do?\n> \n> I think the answer to the question is \"no\", but if I have to guess\n> why such a low-hanging fruit optimization possibility has not been\n> exploited so far is because it does not optimize a useful case,\n> perhaps? IOW, falling into \"if it hurts, then don't do it\" category?\n\nI think it would come up any time you run \"git gc\", if you've set\ngc.pruneExpire to \"never\". And then it wastes time running a full object\nwalk (which is 30+ seconds for linux.git) even though it won't do\nanything useful.\n\nCuriously, git-gc is sprinkled with \"if (prune_expire)\" logic, including\nskipping the call to git-prune entirely. But that only kicks in if you\nrun \"git gc --no-prune\". If \"never\" is truly the same thing (and I\ncannot off the top of my head think of a reason that it isn't), then\nthis:\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex f3942188a6..5118535a4d 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -610,6 +610,8 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n \n \tif (prune_expire && parse_expiry_date(prune_expire, &dummy))\n \t\tdie(_(\"failed to parse prune expiry value %s\"), prune_expire);\n+\tif (prune_expire && !dummy)\n+\t\tprune_expire = NULL; /* \"never\" same as --no-prune */\n \n \tif (aggressive) {\n \t\tstrvec_push(&repack, \"-f\");\n\nwould bring the two cases in sync (I tried to minimize the diff for\nillustration; probably some light refactoring would be in order).\n\nI guess nobody cared because it is not that common to set pruneExpire to\n\"never\". We did something like that at GitHub, but we always drove\nrepack and prune through our own scripts, not through git-gc. I don't\nhave access to those scripts anymore, but I'm pretty sure they just\nskipped calling git-prune entirely for this case.\n\nSo yeah, I think it may just be a curiosity that nobody noticed and\nbothered to optimize it. I am tempted to work the above into a proper\npatch. We even do something similar already for the reflog expiration\nvariables.\n\n-Peff\n"},{"id":"477242","messageId":"20230513001158.GA3814811@coredump.intra.peff.net","threadId":"59731","inReplyTo":"20230513001117.GA2974285@coredump.intra.peff.net","subject":"Re: [PATCH v3 2/2] builtin/pack-objects.c: introduce `pack.recentObjectsHook`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-05-13T00:11:58Z","receivedAt":"2023-05-13T00:12:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 12, 2023 at 08:11:18PM -0400, Jeff King wrote:\n\n> So yeah, I think it may just be a curiosity that nobody noticed and\n> bothered to optimize it. I am tempted to work the above into a proper\n> patch. We even do something similar already for the reflog expiration\n> variables.\n\nOh, and in case it was not clear: this is all well off Taylor's original\ntopic. Whatever we do (or don't do) should be its own series.\n\n-Peff\n"},{"id":"477315","messageId":"ZGKS3gB5B7fbetFb@nand.local","threadId":"59731","inReplyTo":"20230512045853.GA2479240@coredump.intra.peff.net","subject":"Re: [PATCH v3 2/2] builtin/pack-objects.c: introduce `pack.recentObjectsHook`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-05-15T20:15:26Z","receivedAt":"2023-05-15T20:15:45Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, May 12, 2023 at 12:58:53AM -0400, Jeff King wrote:\n> I haven't looked closely at this whole patch yet (and I especially want\n> to look at the new tests since this approach covers more cases), but I\n> did notice that this version of the function still has the \"we don't\n> reap the child on parse failure\" problem I described in:\n>\n> https://lore.kernel.org/git/20230505221921.GE3321533@coredump.intra.peff.net/\n\nHmmph. I could have sworn that I remember including that feedback in the\nnew round, but I must have dropped it on the floor somewhere.\n\nThanks for pointing it out, I'm 99% sure that it'll be in the next round\n;-).\n\nThanks,\nTaylor\n"},{"id":"477320","messageId":"ZGKYOeMGs4fGO5bz@nand.local","threadId":"59731","inReplyTo":"20230512212456.GA2495860@coredump.intra.peff.net","subject":"Re: [PATCH v3 2/2] builtin/pack-objects.c: introduce `pack.recentObjectsHook`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-05-15T20:38:17Z","receivedAt":"2023-05-15T20:38:23Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, May 12, 2023 at 05:24:56PM -0400, Jeff King wrote:\n> I think this description suffers a bit from being adapted from the\n> original patch which was targeting cruft packs. It's not clear to me\n> what \"the caller\" means here. And really, I think this is getting into\n> the details before giving an overview and motivation.\n>\n> I'd expect something the rationale to be something like:\n\nRe-reading it myself, I tend to agree with you. I modified it quite a\nbit, and I'm much happier with the result. Thanks for mentioning it.\n\n> One option I don't see here is: update the mtime on the objects you want\n> to salvage.\n>\n> Why would we want this patch instead of just having the caller update\n> the mtimes of objects (or in a cruft-pack world, call a command that\n> rewrites the .mtimes file with new values)?\n>\n> I can think of some possible arguments against it (you might want to\n> retain the old mtimes, or you might find it a hassle to have to\n> continually update them before gc kills them). But I think the commit\n> message should probably make those arguments.\n\nI agree with everything you wrote here.\n\n> > We then add those as tips to another reachability traversal (along with\n> > any recent objects, if pruning), marking every object along the way\n> > (either adding it to the cruft pack, or writing it out as a loose\n> > object).\n>\n> I didn't understand this \"if pruning\" comment. If we are not pruning at\n> all, wouldn't we skip the extra traversal entirely, since we know we are\n> saving everything?\n\nI was talking about the rescuing traversal for generating a cruft pack.\nBut I ended up dropping this whole paragraph anyway, since I don't think\nit's adding anything in the context of the new patch message.\n\n> > @@ -126,8 +198,14 @@ static int want_recent_object(struct recent_data *data,\n> > \t\t\t const struct object_id *oid)\n> > {\n> > \tif (data->ignore_in_core_kept_packs &&\n> > -\t has_object_kept_pack(oid, IN_CORE_KEEP_PACKS))\n> > +\t has_object_kept_pack(oid, IN_CORE_KEEP_PACKS)) {\n> > +\t\tif (!data->extra_recent_oids_loaded)\n> > +\t\t\tload_pack_recent_objects(data);\n> > +\t\tif (oidset_contains(&data->extra_recent_oids, oid))\n> > +\t\t\treturn 1;\n> > +\n> > \t\treturn 0;\n> > +\t}\n> > \treturn 1;\n> > }\n>\n> This hunk I'm less sure about. The purpose of this function is that the\n> caller has told us about some packs which are \"special\", and we avoid\n> adding their objects to the traversal.\n>\n> This kicks in for cruft packs, when the git-repack caller says \"I just\n> made pack xyz.pack; do not bother saving anything in it to the cruft\n> pack, since xyz.pack is here to stay\". So if a hook says \"you should\n> keep object X\", why would we want to override that check? It is already\n> a reachable object that has been packed into xyz.pack, so we know there\n> is no point in even considering its recency.\n\nYup, you're absolutely right here. Thanks for catching it.\n\n> > --- a/t/t5329-pack-objects-cruft.sh\n> > +++ b/t/t5329-pack-objects-cruft.sh\n> > @@ -739,4 +739,175 @@ test_expect_success 'cruft objects are freshend via loose' '\n> > \t)\n> > '\n> >\n> > +test_expect_success 'additional cruft tips may be specified via pack.extraCruftTips' '\n>\n> This title (and others below) seems out of date. :)\n\nThanks for noticing, fixed.\n\n> > diff --git a/t/t7701-repack-unpack-unreachable.sh b/t/t7701-repack-unpack-unreachable.sh\n> > index ebb267855f..d2eea6e754 100755\n> > --- a/t/t7701-repack-unpack-unreachable.sh\n> > +++ b/t/t7701-repack-unpack-unreachable.sh\n> > @@ -113,6 +113,28 @@ test_expect_success 'do not bother loosening old objects' '\n> > \ttest_must_fail git cat-file -p $obj2\n> > '\n> >\n> > +test_expect_success 'extra recent tips are kept regardless of age' '\n> > +\tobj1=$(echo one | git hash-object -w --stdin) &&\n> > +\tobj2=$(echo two | git hash-object -w --stdin) &&\n> > +\tpack1=$(echo $obj1 | git pack-objects .git/objects/pack/pack) &&\n> > +\tpack2=$(echo $obj2 | git pack-objects .git/objects/pack/pack) &&\n> > +\tgit prune-packed &&\n> > +\n> > +\tgit cat-file -p $obj1 &&\n> > +\tgit cat-file -p $obj2 &&\n> > +\n> > +\twrite_script extra-tips <<-EOF &&\n> > +\techo $obj2\n> > +\tEOF\n> > +\tgit config pack.recentObjectsHook ./extra-tips &&\n> > +\n> > +\ttest-tool chmtime =-86400 .git/objects/pack/pack-$pack2.pack &&\n> > +\tgit repack -A -d --unpack-unreachable=1.hour.ago &&\n> > +\n> > +\tgit cat-file -p $obj1 &&\n> > +\tgit cat-file -p $obj2\n> > +'\n>\n> And this is the new test in this iteration covering the \"repack -A\"\n> case.\n>\n> It is checking that $obj2, which our hook mentions, is saved. It also\n> checks that $obj1 is saved because it is still recent. But there are two\n> other possibly interesting cases:\n>\n> - an object that is too old and is _not_ saved. It seems useful to\n> confirm that the new patch does not simply break the ability to drop\n> objects. ;)\n>\n> - an object that is reachable from $obj2 is also saved. From a\n> white-box perspective this is less interesting, because we should\n> already test elsewhere that this works for recent objects, and we\n> know the new feature is implemented by faking recency. But it might\n> be worth it for completeness, and because it's easy to do (making\n> $obj2 a tag pointing to a blob should work).\n\nAll very good cases to check for. Here's a patch on top (which I'll\nobviously squash into my new version, but figured I'd send it as a\nresponse to you directly, too):\n\n--- 8< ---\nS\ndiff --git a/t/t7701-repack-unpack-unreachable.sh b/t/t7701-repack-unpack-unreachable.sh\nindex d2eea6e754..fa2df6016b 100755\n--- a/t/t7701-repack-unpack-unreachable.sh\n+++ b/t/t7701-repack-unpack-unreachable.sh\n@@ -116,23 +116,33 @@ test_expect_success 'do not bother loosening old objects' '\n test_expect_success 'extra recent tips are kept regardless of age' '\n \tobj1=$(echo one | git hash-object -w --stdin) &&\n \tobj2=$(echo two | git hash-object -w --stdin) &&\n+\tobj3=$(echo three | git hash-object -w --stdin) &&\n \tpack1=$(echo $obj1 | git pack-objects .git/objects/pack/pack) &&\n \tpack2=$(echo $obj2 | git pack-objects .git/objects/pack/pack) &&\n+\tpack3=$(echo $obj3 | git pack-objects .git/objects/pack/pack) &&\n \tgit prune-packed &&\n\n \tgit cat-file -p $obj1 &&\n \tgit cat-file -p $obj2 &&\n+\tgit cat-file -p $obj3 &&\n\n-\twrite_script extra-tips <<-EOF &&\n-\techo $obj2\n+\tgit tag -a -m tag obj2-tag $obj2 &&\n+\tobj2_tag=\"$(git rev-parse obj2-tag)\" &&\n+\n+\n+\twrite_script precious-objects <<-EOF &&\n+\techo $obj2_tag\n \tEOF\n-\tgit config pack.recentObjectsHook ./extra-tips &&\n+\tgit config pack.recentObjectsHook ./precious-objects &&\n\n \ttest-tool chmtime =-86400 .git/objects/pack/pack-$pack2.pack &&\n+\ttest-tool chmtime =-86400 .git/objects/pack/pack-$pack3.pack &&\n \tgit repack -A -d --unpack-unreachable=1.hour.ago &&\n\n \tgit cat-file -p $obj1 &&\n-\tgit cat-file -p $obj2\n+\tgit cat-file -p $obj2 &&\n+\tgit cat-file -p $obj2_tag &&\n+\ttest_must_fail git cat-file -p $obj3\n '\n\n test_expect_success 'keep packed objects found only in index' '\n--- >8 ---\n\nThanks,\nTaylor\n"},{"id":"477321","messageId":"ZGKavGdYrFvjVex7@nand.local","threadId":"59731","inReplyTo":"20230512214542.GB2495860@coredump.intra.peff.net","subject":"Re: [PATCH v3 2/2] builtin/pack-objects.c: introduce `pack.recentObjectsHook`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-05-15T20:49:00Z","receivedAt":"2023-05-15T20:49:06Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, May 12, 2023 at 05:45:42PM -0400, Jeff King wrote:\n> On Fri, May 12, 2023 at 05:24:56PM -0400, Jeff King wrote:\n>\n> > > This patch introduces a new configuration, `pack.recentObjectsHook`\n> > > which allows the caller to specify a program (or set of programs) whose\n> > > output is treated as a set of objects to treat as recent, regardless of\n> > > their true age.\n> >\n> > I was going to complain about putting this in the \"pack\" section,\n> > because I thought by touching reachable.c, we'd also affect git-prune.\n> > But I don't think we do, because it does its own direct mtime check on\n> > the loose objects.\n> >\n> > But I'm not sure that's the right behavior.\n> >\n> > It feels like even before your patch, this is a huge gap in our\n> > object-retention strategy. During repacking, we try to avoid dropping\n> > objects which are reachable from recent-but-unreachable things we're\n> > keeping (since otherwise it effectively corrupts those recent objects,\n> > making them less valuable to keep). But git-prune will happily drop them\n> > anyway!\n> >\n> > And I think the same thing would apply to your hook. If the hook says\n> > \"object XYZ is precious even if unreachable, keep it\", then git-prune\n> > ignoring that seems like it would be a source of errors.\n> >\n> > I suspect both could be fixed by having git-prune trigger the same\n> > add_unseen_recent_objects_to_traversal() call either as part of\n> > the perform_reachability_traversal() walk, or maybe in its own walk (I\n> > think maybe it has to be its own because the second walk should avoid\n> > complaining about missing objects).\n>\n> I am happy to say that I was wrong here, and git-prune behaves as\n> it should, courtesy of d3038d22f9 (prune: keep objects reachable from\n> recent objects, 2014-10-15). The magic happens in mark_reachable_objects(),\n> which handles walking the recent objects by calling...you guessed it,\n> add_unseen_recent_objects_to_traversal().\n\nPhew. Thanks for digging into it before I was able to respond. I'm glad\nthat this works (though I agree that we should add a test).\n\n> So it does the right thing now, and your patch should kick in\n> automatically for git-prune, too. But I think we'd want two things:\n>\n> 1. Should the config variable name be made more generic to match?\n> Maybe \"core\" is too broad (though certainly I'd expect it to apply\n> anywhere in Git where we check recent-ness of objects), but perhaps\n> \"gc\" would make sense (even though it is not strictly part of the\n> gc command, it is within that realm of concepts).\n\n\"core\" does feel pretty broad. There's some precedence for adding\nhook-like configuration there, at least with\n`core.alternateRefsCommand`. But I think that was an appropriate choice\ngiven the scope of that feature.\n\nI think that calling it \"gc.recentObjectsHook\" makes the most sense.\n\n> 2. We probably want a test covering git-prune in this situation.\n\nYup.\n\nThanks,\nTaylor\n"},{"id":"477331","messageId":"cover.1684196634.git.me@ttaylorr.com","threadId":"59731","inReplyTo":"cover.1683847221.git.me@ttaylorr.com","subject":"[PATCH v4 0/2] gc: introduce `gc.recentObjectsHook`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-05-16T00:23:58Z","receivedAt":"2023-05-16T00:24:07Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Here is another reworked version of the patch which introduced a new\nconfiguration `gc.extraCruftTips` to keep additional objects when\npruning which might have otherwise aged out of the repository.\n\nNotable changes since last time include:\n\n - updating the name of this configuration to \"gc.recentObjectsHook\",\n - significantly reworking the substantive patch's message\n - improved test coverage to cover more cases of loose object pruning,\n as well as `git prune`'s interaction with `gc.recentObjectsHook`.\n\n(Again, since we're expecting -rc0, I don't expect a ton of movement on\nthis series before 2.41 is shipped, but I figured I'd share it out\nanyways to get it off of my laptop).\n\nThanks in advance for your review.\n\nTaylor Blau (2):\n reachable.c: extract `obj_is_recent()`\n gc: introduce `gc.recentObjectsHook`\n\n Documentation/config/gc.txt | 13 ++\n reachable.c | 85 ++++++++++++-\n t/t5304-prune.sh | 14 +++\n t/t5329-pack-objects-cruft.sh | 171 +++++++++++++++++++++++++++\n t/t7701-repack-unpack-unreachable.sh | 31 +++++\n 5 files changed, 311 insertions(+), 3 deletions(-)\n\nRange-diff against v3:\n1: f5f3b0f334 = 1: 9c1b59c8cf reachable.c: extract `obj_is_recent()`\n2: 2ce8a79fa4 ! 2: 18e50d2517 builtin/pack-objects.c: introduce `pack.recentObjectsHook`\n @@ Metadata\n Author: Taylor Blau \n \n ## Commit message ##\n - builtin/pack-objects.c: introduce `pack.recentObjectsHook`\n + gc: introduce `gc.recentObjectsHook`\n \n This patch introduces a new multi-valued configuration option,\n - `pack.recentObjectsHook` as a means to mark certain objects as recent,\n - regardless of their age.\n + `gc.recentObjectsHook` as a means to mark certain objects as recent (and\n + thus exempt from garbage collection), regardless of their age.\n \n - Depending on whether or not we are generating a cruft pack, this allows\n - the caller to do one of two things:\n + When performing a garbage collection operation on a repository with\n + unreachable objects, Git makes its decision on what to do with those\n + object(s) bed on how recent the objects are or not. Generally speaking,\n + unreachable-but-recent objects stay in the repository, and older objects\n + are discarded.\n \n - - If generating a cruft pack, the caller is able to retain additional\n - objects via the cruft pack, even if they would have otherwise been\n - pruned due to their age.\n + However, we have no convenient way to keep certain precious, unreachable\n + objects around in the repository, even if they have aged out and would\n + be pruned. Our options today consist of:\n \n - - If not generating a cruft pack, the caller is likewise able to\n - retain additional objects as loose.\n + - Point references at the reachability tips of any objects you\n + consider precious, which may be undesirable or infeasible.\n \n - There is currently no option to be able to keep around certain objects\n - that have otherwise aged out of the grace period. The only way to retain\n - those objects is:\n -\n - - to point a reference at them, which may be undesirable or\n - infeasible,\n -\n - - to track them via the reflog, which may be undesirable since the\n + - Track them via the reflog, which may be undesirable since the\n reflog's lifetime is limited to that of the reference it's tracking\n (and callers may want to keep those unreachable objects around for\n - longer)\n + longer).\n \n - - to extend the grace period, which may keep around other objects that\n - the caller *does* want to discard,\n + - Extend the grace period, which may keep around other objects that\n + the caller *does* want to discard.\n \n - - or, to force the caller to construct the pack of objects they want\n + - Manually modify the mtimes of objects you want to keep. If those\n + objects are already loose, this is easy enough to do (you can just\n + enumerate and `touch -m` each one).\n +\n + But if they are packed, you will either end up modifying the mtimes\n + of *all* objects in that pack, or be forced to write out a loose\n + copy of that object, both of which may be undesirable. Even worse,\n + if they are in a cruft pack, that requires modifying its `*.mtimes`\n + file by hand, since there is no exposed plumbing for this.\n +\n + - Force the caller to construct the pack of objects they want\n to keep themselves, and then mark the pack as kept by adding a\n - \".keep\" file.\n + \".keep\" file. This works, but is burdensome for the caller, and\n + having extra packs is awkward as you roll forward your cruft pack.\n \n - This patch introduces a new configuration, `pack.recentObjectsHook`\n - which allows the caller to specify a program (or set of programs) whose\n - output is treated as a set of objects to treat as recent, regardless of\n - their true age.\n + This patch introduces a new option to the above list via the\n + `gc.recentObjectsHook` configuration, which allows the caller to\n + specify a program (or set of programs) whose output is treated as a set\n + of objects to treat as recent, regardless of their true age.\n \n - The implementation is straightforward. In either case (cruft packs or\n - not), Git enumerates recent objects via\n - `add_unseen_recent_objects_to_traversal()`. That enumerates loose and\n + The implementation is straightforward. Git enumerates recent objects via\n + `add_unseen_recent_objects_to_traversal()`, which enumerates loose and\n packed objects, and eventually calls add_recent_object() on any objects\n for which `want_recent_object()`'s conditions are met.\n \n This patch modifies the recency condition from simply \"is the mtime of\n this object more recent than the cutoff?\" to \"[...] or, is this object\n - mentioned by at least one `pack.recentObjectsHook`?\".\n + mentioned by at least one `gc.recentObjectsHook`?\".\n \n - We then add those as tips to another reachability traversal (along with\n - any recent objects, if pruning), marking every object along the way\n - (either adding it to the cruft pack, or writing it out as a loose\n - object).\n + Depending on whether or not we are generating a cruft pack, this allows\n + the caller to do one of two things:\n +\n + - If generating a cruft pack, the caller is able to retain additional\n + objects via the cruft pack, even if they would have otherwise been\n + pruned due to their age.\n +\n + - If not generating a cruft pack, the caller is likewise able to\n + retain additional objects as loose.\n \n A potential alternative here is to introduce a new mode to alter the\n contents of the reachable pack instead of the cruft one. One could\n @@ Commit message\n Helped-by: Jeff King \n Signed-off-by: Taylor Blau \n \n - ## Documentation/config/pack.txt ##\n -@@ Documentation/config/pack.txt: pack.deltaCacheLimit::\n - \tresult once the best match for all objects is found.\n - \tDefaults to 1000. Maximum value is 65535.\n + ## Documentation/config/gc.txt ##\n +@@ Documentation/config/gc.txt: or rebase occurring. Since these changes are not part of the current\n + project most users will want to expire them sooner, which is why the\n + default is more aggressive than `gc.reflogExpire`.\n \n -+pack.recentObjectsHook::\n ++gc.recentObjectsHook::\n +\tWhen considering the recency of an object (e.g., when generating\n +\ta cruft pack or storing unreachable objects as loose), use the\n +\tshell to execute the specified command(s). Interpret their\n @@ Documentation/config/pack.txt: pack.deltaCacheLimit::\n +operation (either generating a cruft pack or unpacking unreachable\n +objects) will be halted.\n +\n - pack.threads::\n - \tSpecifies the number of threads to spawn when searching for best\n - \tdelta matches. This requires that linkgit:git-pack-objects[1]\n -\n - ## builtin/pack-objects.c ##\n -@@\n - #include \"pack-mtimes.h\"\n - #include \"parse-options.h\"\n - #include \"wrapper.h\"\n -+#include \"run-command.h\"\n - \n - /*\n - * Objects we are going to pack are collected in the `to_pack` structure.\n + gc.rerereResolved::\n + \tRecords of conflicted merge you resolved earlier are\n + \tkept for this many days when 'git rerere gc' is run.\n \n ## reachable.c ##\n @@\n @@ reachable.c: struct recent_data {\n +\tint extra_recent_oids_loaded;\n };\n \n -+static int run_one_pack_recent_objects_hook(struct oidset *set,\n ++static int run_one_gc_recent_objects_hook(struct oidset *set,\n +\t\t\t\t\t const char *args)\n +{\n +\tstruct child_process cmd = CHILD_PROCESS_INIT;\n @@ reachable.c: struct recent_data {\n +\n +\t\tif (parse_oid_hex(buf.buf, &oid, &rest) || *rest) {\n +\t\t\tret = error(_(\"invalid extra cruft tip: '%s'\"), buf.buf);\n -+\t\t\tgoto done;\n ++\t\t\tbreak;\n +\t\t}\n +\n +\t\toidset_insert(set, &oid);\n +\t}\n +\n -+\tret = finish_command(&cmd);\n ++\tfclose(out);\n ++\tret |= finish_command(&cmd);\n +\n -+done:\n -+\tif (out)\n -+\t\tfclose(out);\n +\tstrbuf_release(&buf);\n -+\tchild_process_clear(&cmd);\n -+\n +\treturn ret;\n +}\n +\n -+static void load_pack_recent_objects(struct recent_data *data)\n ++static void load_gc_recent_objects(struct recent_data *data)\n +{\n +\tconst struct string_list *programs;\n +\tint ret = 0;\n @@ reachable.c: struct recent_data {\n +\n +\tdata->extra_recent_oids_loaded = 1;\n +\n -+\tif (git_config_get_string_multi(\"pack.recentobjectshook\", &programs))\n ++\tif (git_config_get_string_multi(\"gc.recentobjectshook\", &programs))\n +\t\treturn;\n +\n +\tfor (i = 0; i < programs->nr; i++) {\n -+\t\tret = run_one_pack_recent_objects_hook(&data->extra_recent_oids,\n ++\t\tret = run_one_gc_recent_objects_hook(&data->extra_recent_oids,\n +\t\t\t\t\t\t programs->items[i].string);\n +\t\tif (ret)\n -+\t\t\tbreak;\n ++\t\t\tdie(_(\"unable to enumerate additional cruft tips\"));\n +\t}\n -+\n -+\tif (ret)\n -+\t\tdie(_(\"unable to enumerate additional cruft tips\"));\n +}\n +\n static int obj_is_recent(const struct object_id *oid, timestamp_t mtime,\n @@ reachable.c: struct recent_data {\n +\t\treturn 1;\n +\n +\tif (!data->extra_recent_oids_loaded)\n -+\t\tload_pack_recent_objects(data);\n ++\t\tload_gc_recent_objects(data);\n +\treturn oidset_contains(&data->extra_recent_oids, oid);\n }\n \n static void add_recent_object(const struct object_id *oid,\n -@@ reachable.c: static int want_recent_object(struct recent_data *data,\n - \t\t\t const struct object_id *oid)\n - {\n - \tif (data->ignore_in_core_kept_packs &&\n --\t has_object_kept_pack(oid, IN_CORE_KEEP_PACKS))\n -+\t has_object_kept_pack(oid, IN_CORE_KEEP_PACKS)) {\n -+\t\tif (!data->extra_recent_oids_loaded)\n -+\t\t\tload_pack_recent_objects(data);\n -+\t\tif (oidset_contains(&data->extra_recent_oids, oid))\n -+\t\t\treturn 1;\n -+\n - \t\treturn 0;\n -+\t}\n - \treturn 1;\n - }\n - \n @@ reachable.c: int add_unseen_recent_objects_to_traversal(struct rev_info *revs,\n \tdata.cb = cb;\n \tdata.ignore_in_core_kept_packs = ignore_in_core_kept_packs;\n @@ reachable.c: int add_unseen_recent_objects_to_traversal(struct rev_info *revs,\n \n static int mark_object_seen(const struct object_id *oid,\n \n + ## t/t5304-prune.sh ##\n +@@ t/t5304-prune.sh: test_expect_success 'old reachable-from-recent retained with bitmaps' '\n + \ttest_must_fail git cat-file -e $to_drop\n + '\n + \n ++test_expect_success 'gc.recentObjectsHook' '\n ++\tadd_blob &&\n ++\ttest-tool chmtime =-86500 $BLOB_FILE &&\n ++\n ++\twrite_script precious-objects <<-EOF &&\n ++\techo $BLOB\n ++\tEOF\n ++\ttest_config gc.recentObjectsHook ./precious-objects &&\n ++\n ++\tgit prune --expire=now &&\n ++\n ++\tgit cat-file -p $BLOB\n ++'\n ++\n + test_done\n +\n ## t/t5329-pack-objects-cruft.sh ##\n @@ t/t5329-pack-objects-cruft.sh: test_expect_success 'cruft objects are freshend via loose' '\n \t)\n '\n \n -+test_expect_success 'additional cruft tips may be specified via pack.extraCruftTips' '\n ++test_expect_success 'gc.recentObjectsHook' '\n +\tgit init repo &&\n +\ttest_when_finished \"rm -fr repo\" &&\n +\t(\n @@ t/t5329-pack-objects-cruft.sh: test_expect_success 'cruft objects are freshend v\n +\t\twrite_script extra-tips <<-EOF &&\n +\t\techo $cruft_old\n +\t\tEOF\n -+\t\tgit config pack.recentObjectsHook ./extra-tips &&\n ++\t\tgit config gc.recentObjectsHook ./extra-tips &&\n +\n +\t\tgit repack --cruft --cruft-expiration=now -d &&\n +\n @@ t/t5329-pack-objects-cruft.sh: test_expect_success 'cruft objects are freshend v\n +\t\ttest_cmp cruft.expect cruft.actual &&\n +\n +\t\t# Ensure that the \"old\" objects are removed after\n -+\t\t# dropping the pack.extraCruftTips hook.\n -+\t\tgit config --unset pack.recentObjectsHook &&\n ++\t\t# dropping the gc.recentObjectsHook hook.\n ++\t\tgit config --unset gc.recentObjectsHook &&\n +\t\tgit repack --cruft --cruft-expiration=now -d &&\n +\n +\t\tmtimes=\"$(ls .git/objects/pack/pack-*.mtimes)\" &&\n @@ t/t5329-pack-objects-cruft.sh: test_expect_success 'cruft objects are freshend v\n +\t)\n +'\n +\n -+test_expect_success 'multi-valued pack.extraCruftTips' '\n ++test_expect_success 'multi-valued gc.recentObjectsHook' '\n +\tgit init repo &&\n +\ttest_when_finished \"rm -fr repo\" &&\n +\t(\n @@ t/t5329-pack-objects-cruft.sh: test_expect_success 'cruft objects are freshend v\n +\n +\t\t# ensure that each extra cruft tip is saved by its\n +\t\t# respective hook\n -+\t\tgit config --add pack.recentObjectsHook ./extra-tips.a &&\n -+\t\tgit config --add pack.recentObjectsHook ./extra-tips.b &&\n ++\t\tgit config --add gc.recentObjectsHook ./extra-tips.a &&\n ++\t\tgit config --add gc.recentObjectsHook ./extra-tips.b &&\n +\t\tgit repack --cruft --cruft-expiration=now -d &&\n +\n +\t\tmtimes=\"$(ls .git/objects/pack/pack-*.mtimes)\" &&\n @@ t/t5329-pack-objects-cruft.sh: test_expect_success 'cruft objects are freshend v\n +\t\ttest_cmp cruft.expect cruft.actual &&\n +\n +\t\t# ensure that a dirty exit halts cruft pack generation\n -+\t\tgit config --add pack.recentObjectsHook ./extra-tips.c &&\n ++\t\tgit config --add gc.recentObjectsHook ./extra-tips.c &&\n +\t\ttest_must_fail git repack --cruft --cruft-expiration=now -d 2>err &&\n +\t\tgrep \"unable to enumerate additional cruft tips\" err &&\n +\n @@ t/t5329-pack-objects-cruft.sh: test_expect_success 'cruft objects are freshend v\n +\t)\n +'\n +\n -+test_expect_success 'additional cruft blobs via pack.extraCruftTips' '\n ++test_expect_success 'additional cruft blobs via gc.recentObjectsHook' '\n +\tgit init repo &&\n +\ttest_when_finished \"rm -fr repo\" &&\n +\t(\n @@ t/t5329-pack-objects-cruft.sh: test_expect_success 'cruft objects are freshend v\n +\t\twrite_script extra-tips <<-EOF &&\n +\t\techo $blob\n +\t\tEOF\n -+\t\tgit config pack.recentObjectsHook ./extra-tips &&\n ++\t\tgit config gc.recentObjectsHook ./extra-tips &&\n +\n +\t\tgit repack --cruft --cruft-expiration=now -d &&\n +\n @@ t/t7701-repack-unpack-unreachable.sh: test_expect_success 'do not bother looseni\n \ttest_must_fail git cat-file -p $obj2\n '\n \n -+test_expect_success 'extra recent tips are kept regardless of age' '\n ++test_expect_success 'gc.recentObjectsHook' '\n +\tobj1=$(echo one | git hash-object -w --stdin) &&\n +\tobj2=$(echo two | git hash-object -w --stdin) &&\n ++\tobj3=$(echo three | git hash-object -w --stdin) &&\n +\tpack1=$(echo $obj1 | git pack-objects .git/objects/pack/pack) &&\n +\tpack2=$(echo $obj2 | git pack-objects .git/objects/pack/pack) &&\n ++\tpack3=$(echo $obj3 | git pack-objects .git/objects/pack/pack) &&\n +\tgit prune-packed &&\n +\n +\tgit cat-file -p $obj1 &&\n +\tgit cat-file -p $obj2 &&\n ++\tgit cat-file -p $obj3 &&\n +\n -+\twrite_script extra-tips <<-EOF &&\n -+\techo $obj2\n ++\tgit tag -a -m tag obj2-tag $obj2 &&\n ++\tobj2_tag=\"$(git rev-parse obj2-tag)\" &&\n ++\n ++\twrite_script precious-objects <<-EOF &&\n ++\techo $obj2_tag\n +\tEOF\n -+\tgit config pack.recentObjectsHook ./extra-tips &&\n ++\tgit config gc.recentObjectsHook ./precious-objects &&\n +\n +\ttest-tool chmtime =-86400 .git/objects/pack/pack-$pack2.pack &&\n ++\ttest-tool chmtime =-86400 .git/objects/pack/pack-$pack3.pack &&\n +\tgit repack -A -d --unpack-unreachable=1.hour.ago &&\n +\n +\tgit cat-file -p $obj1 &&\n -+\tgit cat-file -p $obj2\n ++\tgit cat-file -p $obj2 &&\n ++\tgit cat-file -p $obj2_tag &&\n ++\ttest_must_fail git cat-file -p $obj3\n +'\n +\n test_expect_success 'keep packed objects found only in index' '\n-- \n2.40.1.558.g18e50d2517\n"},{"id":"477332","messageId":"9c1b59c8cf34f6f17cd3f1fdf851434abf3d6f0a.1684196634.git.me@ttaylorr.com","threadId":"59731","inReplyTo":"cover.1684196634.git.me@ttaylorr.com","subject":"[PATCH v4 1/2] reachable.c: extract `obj_is_recent()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-05-16T00:24:03Z","receivedAt":"2023-05-16T00:24:09Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"When enumerating objects in order to add recent ones (i.e. those whose\nmtime is strictly newer than the cutoff) as tips of a reachability\ntraversal, `add_recent_object()` discards objects which do not meet the\nrecency criteria.\n\nThe subsequent commit will make checking whether or not an object is\nrecent also consult the list of hooks in `pack.recentHook`. Isolate this\ncheck in its own function to keep the additional complexity outside of\n`add_recent_object()`.\n\nSigned-off-by: Taylor Blau \n---\n reachable.c | 8 +++++++-\n 1 file changed, 7 insertions(+), 1 deletion(-)\n\ndiff --git a/reachable.c b/reachable.c\nindex 55bb114353..7a42da5d39 100644\n--- a/reachable.c\n+++ b/reachable.c\n@@ -69,6 +69,12 @@ struct recent_data {\n \tint ignore_in_core_kept_packs;\n };\n \n+static int obj_is_recent(const struct object_id *oid, timestamp_t mtime,\n+\t\t\t struct recent_data *data)\n+{\n+\treturn mtime > data->timestamp;\n+}\n+\n static void add_recent_object(const struct object_id *oid,\n \t\t\t struct packed_git *pack,\n \t\t\t off_t offset,\n@@ -78,7 +84,7 @@ static void add_recent_object(const struct object_id *oid,\n \tstruct object *obj;\n \tenum object_type type;\n \n-\tif (mtime <= data->timestamp)\n+\tif (!obj_is_recent(oid, mtime, data))\n \t\treturn;\n \n \t/*\n-- \n2.40.1.558.g18e50d2517\n\n"},{"id":"477333","messageId":"18e50d2517ba4cc81d4bafb0b029ca7a770f23a7.1684196634.git.me@ttaylorr.com","threadId":"59731","inReplyTo":"cover.1684196634.git.me@ttaylorr.com","subject":"[PATCH v4 2/2] gc: introduce `gc.recentObjectsHook`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-05-16T00:24:06Z","receivedAt":"2023-05-16T00:24:19Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"This patch introduces a new multi-valued configuration option,\n`gc.recentObjectsHook` as a means to mark certain objects as recent (and\nthus exempt from garbage collection), regardless of their age.\n\nWhen performing a garbage collection operation on a repository with\nunreachable objects, Git makes its decision on what to do with those\nobject(s) bed on how recent the objects are or not. Generally speaking,\nunreachable-but-recent objects stay in the repository, and older objects\nare discarded.\n\nHowever, we have no convenient way to keep certain precious, unreachable\nobjects around in the repository, even if they have aged out and would\nbe pruned. Our options today consist of:\n\n - Point references at the reachability tips of any objects you\n consider precious, which may be undesirable or infeasible.\n\n - Track them via the reflog, which may be undesirable since the\n reflog's lifetime is limited to that of the reference it's tracking\n (and callers may want to keep those unreachable objects around for\n longer).\n\n - Extend the grace period, which may keep around other objects that\n the caller *does* want to discard.\n\n - Manually modify the mtimes of objects you want to keep. If those\n objects are already loose, this is easy enough to do (you can just\n enumerate and `touch -m` each one).\n\n But if they are packed, you will either end up modifying the mtimes\n of *all* objects in that pack, or be forced to write out a loose\n copy of that object, both of which may be undesirable. Even worse,\n if they are in a cruft pack, that requires modifying its `*.mtimes`\n file by hand, since there is no exposed plumbing for this.\n\n - Force the caller to construct the pack of objects they want\n to keep themselves, and then mark the pack as kept by adding a\n \".keep\" file. This works, but is burdensome for the caller, and\n having extra packs is awkward as you roll forward your cruft pack.\n\nThis patch introduces a new option to the above list via the\n`gc.recentObjectsHook` configuration, which allows the caller to\nspecify a program (or set of programs) whose output is treated as a set\nof objects to treat as recent, regardless of their true age.\n\nThe implementation is straightforward. Git enumerates recent objects via\n`add_unseen_recent_objects_to_traversal()`, which enumerates loose and\npacked objects, and eventually calls add_recent_object() on any objects\nfor which `want_recent_object()`'s conditions are met.\n\nThis patch modifies the recency condition from simply \"is the mtime of\nthis object more recent than the cutoff?\" to \"[...] or, is this object\nmentioned by at least one `gc.recentObjectsHook`?\".\n\nDepending on whether or not we are generating a cruft pack, this allows\nthe caller to do one of two things:\n\n - If generating a cruft pack, the caller is able to retain additional\n objects via the cruft pack, even if they would have otherwise been\n pruned due to their age.\n\n - If not generating a cruft pack, the caller is likewise able to\n retain additional objects as loose.\n\nA potential alternative here is to introduce a new mode to alter the\ncontents of the reachable pack instead of the cruft one. One could\nimagine a new option to `pack-objects`, say `--extra-reachable-tips`\nthat does the same thing as above, adding the visited set of objects\nalong the traversal to the pack.\n\nBut this has the unfortunate side-effect of altering the reachability\nclosure of that pack. If parts of the unreachable object graph mentioned\nby one or more of the \"extra reachable tips\" programs is not closed,\nthen the resulting pack won't be either. This makes it impossible in the\ngeneral case to write out reachability bitmaps for that pack, since\nclosure is a requirement there.\n\nInstead, keep these unreachable objects in the cruft pack (or set of\nunreachable, loose objects) instead, to ensure that we can continue to\nhave a pack containing just reachable objects, which is always safe to\nwrite a bitmap over.\n\nHelped-by: Jeff King \nSigned-off-by: Taylor Blau \n---\n Documentation/config/gc.txt | 13 ++\n reachable.c | 79 ++++++++++++-\n t/t5304-prune.sh | 14 +++\n t/t5329-pack-objects-cruft.sh | 171 +++++++++++++++++++++++++++\n t/t7701-repack-unpack-unreachable.sh | 31 +++++\n 5 files changed, 305 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/config/gc.txt b/Documentation/config/gc.txt\nindex 7f95c866e1..16190be877 100644\n--- a/Documentation/config/gc.txt\n+++ b/Documentation/config/gc.txt\n@@ -130,6 +130,19 @@ or rebase occurring. Since these changes are not part of the current\n project most users will want to expire them sooner, which is why the\n default is more aggressive than `gc.reflogExpire`.\n \n+gc.recentObjectsHook::\n+\tWhen considering the recency of an object (e.g., when generating\n+\ta cruft pack or storing unreachable objects as loose), use the\n+\tshell to execute the specified command(s). Interpret their\n+\toutput as object IDs which Git will consider as \"recent\",\n+\tregardless of their age.\n++\n+Output must contain exactly one hex object ID per line, and nothing\n+else. Objects which cannot be found in the repository are ignored.\n+Multiple hooks are supported, but all must exit successfully, else the\n+operation (either generating a cruft pack or unpacking unreachable\n+objects) will be halted.\n+\n gc.rerereResolved::\n \tRecords of conflicted merge you resolved earlier are\n \tkept for this many days when 'git rerere gc' is run.\ndiff --git a/reachable.c b/reachable.c\nindex 7a42da5d39..e82ab33444 100644\n--- a/reachable.c\n+++ b/reachable.c\n@@ -16,6 +16,8 @@\n #include \"object-store.h\"\n #include \"pack-bitmap.h\"\n #include \"pack-mtimes.h\"\n+#include \"config.h\"\n+#include \"run-command.h\"\n \n struct connectivity_progress {\n \tstruct progress *progress;\n@@ -67,12 +69,75 @@ struct recent_data {\n \ttimestamp_t timestamp;\n \treport_recent_object_fn *cb;\n \tint ignore_in_core_kept_packs;\n+\n+\tstruct oidset extra_recent_oids;\n+\tint extra_recent_oids_loaded;\n };\n \n+static int run_one_gc_recent_objects_hook(struct oidset *set,\n+\t\t\t\t\t const char *args)\n+{\n+\tstruct child_process cmd = CHILD_PROCESS_INIT;\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tFILE *out;\n+\tint ret = 0;\n+\n+\tcmd.use_shell = 1;\n+\tcmd.out = -1;\n+\n+\tstrvec_push(&cmd.args, args);\n+\n+\tif (start_command(&cmd))\n+\t\treturn -1;\n+\n+\tout = xfdopen(cmd.out, \"r\");\n+\twhile (strbuf_getline(&buf, out) != EOF) {\n+\t\tstruct object_id oid;\n+\t\tconst char *rest;\n+\n+\t\tif (parse_oid_hex(buf.buf, &oid, &rest) || *rest) {\n+\t\t\tret = error(_(\"invalid extra cruft tip: '%s'\"), buf.buf);\n+\t\t\tbreak;\n+\t\t}\n+\n+\t\toidset_insert(set, &oid);\n+\t}\n+\n+\tfclose(out);\n+\tret |= finish_command(&cmd);\n+\n+\tstrbuf_release(&buf);\n+\treturn ret;\n+}\n+\n+static void load_gc_recent_objects(struct recent_data *data)\n+{\n+\tconst struct string_list *programs;\n+\tint ret = 0;\n+\tsize_t i;\n+\n+\tdata->extra_recent_oids_loaded = 1;\n+\n+\tif (git_config_get_string_multi(\"gc.recentobjectshook\", &programs))\n+\t\treturn;\n+\n+\tfor (i = 0; i < programs->nr; i++) {\n+\t\tret = run_one_gc_recent_objects_hook(&data->extra_recent_oids,\n+\t\t\t\t\t\t programs->items[i].string);\n+\t\tif (ret)\n+\t\t\tdie(_(\"unable to enumerate additional cruft tips\"));\n+\t}\n+}\n+\n static int obj_is_recent(const struct object_id *oid, timestamp_t mtime,\n \t\t\t struct recent_data *data)\n {\n-\treturn mtime > data->timestamp;\n+\tif (mtime > data->timestamp)\n+\t\treturn 1;\n+\n+\tif (!data->extra_recent_oids_loaded)\n+\t\tload_gc_recent_objects(data);\n+\treturn oidset_contains(&data->extra_recent_oids, oid);\n }\n \n static void add_recent_object(const struct object_id *oid,\n@@ -199,16 +264,24 @@ int add_unseen_recent_objects_to_traversal(struct rev_info *revs,\n \tdata.cb = cb;\n \tdata.ignore_in_core_kept_packs = ignore_in_core_kept_packs;\n \n+\toidset_init(&data.extra_recent_oids, 0);\n+\tdata.extra_recent_oids_loaded = 0;\n+\n \tr = for_each_loose_object(add_recent_loose, &data,\n \t\t\t\t FOR_EACH_OBJECT_LOCAL_ONLY);\n \tif (r)\n-\t\treturn r;\n+\t\tgoto done;\n \n \tflags = FOR_EACH_OBJECT_LOCAL_ONLY | FOR_EACH_OBJECT_PACK_ORDER;\n \tif (ignore_in_core_kept_packs)\n \t\tflags |= FOR_EACH_OBJECT_SKIP_IN_CORE_KEPT_PACKS;\n \n-\treturn for_each_packed_object(add_recent_packed, &data, flags);\n+\tr = for_each_packed_object(add_recent_packed, &data, flags);\n+\n+done:\n+\toidset_clear(&data.extra_recent_oids);\n+\n+\treturn r;\n }\n \n static int mark_object_seen(const struct object_id *oid,\ndiff --git a/t/t5304-prune.sh b/t/t5304-prune.sh\nindex 662ae9b152..a635fe98f8 100755\n--- a/t/t5304-prune.sh\n+++ b/t/t5304-prune.sh\n@@ -350,4 +350,18 @@ test_expect_success 'old reachable-from-recent retained with bitmaps' '\n \ttest_must_fail git cat-file -e $to_drop\n '\n \n+test_expect_success 'gc.recentObjectsHook' '\n+\tadd_blob &&\n+\ttest-tool chmtime =-86500 $BLOB_FILE &&\n+\n+\twrite_script precious-objects <<-EOF &&\n+\techo $BLOB\n+\tEOF\n+\ttest_config gc.recentObjectsHook ./precious-objects &&\n+\n+\tgit prune --expire=now &&\n+\n+\tgit cat-file -p $BLOB\n+'\n+\n test_done\ndiff --git a/t/t5329-pack-objects-cruft.sh b/t/t5329-pack-objects-cruft.sh\nindex 303f7a5d84..3ae61ca995 100755\n--- a/t/t5329-pack-objects-cruft.sh\n+++ b/t/t5329-pack-objects-cruft.sh\n@@ -739,4 +739,175 @@ test_expect_success 'cruft objects are freshend via loose' '\n \t)\n '\n \n+test_expect_success 'gc.recentObjectsHook' '\n+\tgit init repo &&\n+\ttest_when_finished \"rm -fr repo\" &&\n+\t(\n+\t\tcd repo &&\n+\n+\t\t# Create a handful of objects.\n+\t\t#\n+\t\t# - one reachable commit, \"base\", designated for the reachable\n+\t\t# pack\n+\t\t# - one unreachable commit, \"cruft.discard\", which is marked\n+\t\t# for deletion\n+\t\t# - one unreachable commit, \"cruft.old\", which would be marked\n+\t\t# for deletion, but is rescued as an extra cruft tip\n+\t\t# - one unreachable commit, \"cruft.new\", which is not marked\n+\t\t# for deletion\n+\t\ttest_commit base &&\n+\t\tgit branch -M main &&\n+\n+\t\tgit checkout --orphan discard &&\n+\t\tgit rm -fr . &&\n+\t\ttest_commit --no-tag cruft.discard &&\n+\n+\t\tgit checkout --orphan old &&\n+\t\tgit rm -fr . &&\n+\t\ttest_commit --no-tag cruft.old &&\n+\t\tcruft_old=\"$(git rev-parse HEAD)\" &&\n+\n+\t\tgit checkout --orphan new &&\n+\t\tgit rm -fr . &&\n+\t\ttest_commit --no-tag cruft.new &&\n+\t\tcruft_new=\"$(git rev-parse HEAD)\" &&\n+\n+\t\tgit checkout main &&\n+\t\tgit branch -D discard old new &&\n+\t\tgit reflog expire --all --expire=all &&\n+\n+\t\t# mark cruft.old with an mtime that is many minutes\n+\t\t# older than the expiration period, and mark cruft.new\n+\t\t# with an mtime that is in the future (and thus not\n+\t\t# eligible for pruning).\n+\t\ttest-tool chmtime -2000 \"$objdir/$(test_oid_to_path $cruft_old)\" &&\n+\t\ttest-tool chmtime +1000 \"$objdir/$(test_oid_to_path $cruft_new)\" &&\n+\n+\t\t# Write the list of cruft objects we expect to\n+\t\t# accumulate, which is comprised of everything reachable\n+\t\t# from cruft.old and cruft.new, but not cruft.discard.\n+\t\tgit rev-list --objects --no-object-names \\\n+\t\t\t$cruft_old $cruft_new >cruft.raw &&\n+\t\tsort cruft.raw >cruft.expect &&\n+\n+\t\t# Write the script to list extra tips, which are limited\n+\t\t# to cruft.old, in this case.\n+\t\twrite_script extra-tips <<-EOF &&\n+\t\techo $cruft_old\n+\t\tEOF\n+\t\tgit config gc.recentObjectsHook ./extra-tips &&\n+\n+\t\tgit repack --cruft --cruft-expiration=now -d &&\n+\n+\t\tmtimes=\"$(ls .git/objects/pack/pack-*.mtimes)\" &&\n+\t\tgit show-index <${mtimes%.mtimes}.idx >cruft &&\n+\t\tcut -d\" \" -f2 cruft | sort >cruft.actual &&\n+\t\ttest_cmp cruft.expect cruft.actual &&\n+\n+\t\t# Ensure that the \"old\" objects are removed after\n+\t\t# dropping the gc.recentObjectsHook hook.\n+\t\tgit config --unset gc.recentObjectsHook &&\n+\t\tgit repack --cruft --cruft-expiration=now -d &&\n+\n+\t\tmtimes=\"$(ls .git/objects/pack/pack-*.mtimes)\" &&\n+\t\tgit show-index <${mtimes%.mtimes}.idx >cruft &&\n+\t\tcut -d\" \" -f2 cruft | sort >cruft.actual &&\n+\n+\t\tgit rev-list --objects --no-object-names $cruft_new >cruft.raw &&\n+\t\tcp cruft.expect cruft.old &&\n+\t\tsort cruft.raw >cruft.expect &&\n+\t\ttest_cmp cruft.expect cruft.actual &&\n+\n+\t\t# ensure objects which are no longer in the cruft pack were\n+\t\t# removed from the repository\n+\t\tfor object in $(comm -13 cruft.expect cruft.old)\n+\t\tdo\n+\t\t\ttest_must_fail git cat-file -t $object || return 1\n+\t\tdone\n+\t)\n+'\n+\n+test_expect_success 'multi-valued gc.recentObjectsHook' '\n+\tgit init repo &&\n+\ttest_when_finished \"rm -fr repo\" &&\n+\t(\n+\t\tcd repo &&\n+\n+\t\ttest_commit base &&\n+\t\tgit branch -M main &&\n+\n+\t\tgit checkout --orphan cruft.a &&\n+\t\tgit rm -fr . &&\n+\t\ttest_commit --no-tag cruft.a &&\n+\t\tcruft_a=\"$(git rev-parse HEAD)\" &&\n+\n+\t\tgit checkout --orphan cruft.b &&\n+\t\tgit rm -fr . &&\n+\t\ttest_commit --no-tag cruft.b &&\n+\t\tcruft_b=\"$(git rev-parse HEAD)\" &&\n+\n+\t\tgit checkout main &&\n+\t\tgit branch -D cruft.a cruft.b &&\n+\t\tgit reflog expire --all --expire=all &&\n+\n+\t\techo \"echo $cruft_a\" | write_script extra-tips.a &&\n+\t\techo \"echo $cruft_b\" | write_script extra-tips.b &&\n+\t\techo \"false\" | write_script extra-tips.c &&\n+\n+\t\tgit rev-list --objects --no-object-names $cruft_a $cruft_b \\\n+\t\t\t>cruft.raw &&\n+\t\tsort cruft.raw >cruft.expect &&\n+\n+\t\t# ensure that each extra cruft tip is saved by its\n+\t\t# respective hook\n+\t\tgit config --add gc.recentObjectsHook ./extra-tips.a &&\n+\t\tgit config --add gc.recentObjectsHook ./extra-tips.b &&\n+\t\tgit repack --cruft --cruft-expiration=now -d &&\n+\n+\t\tmtimes=\"$(ls .git/objects/pack/pack-*.mtimes)\" &&\n+\t\tgit show-index <${mtimes%.mtimes}.idx >cruft &&\n+\t\tcut -d\" \" -f2 cruft | sort >cruft.actual &&\n+\t\ttest_cmp cruft.expect cruft.actual &&\n+\n+\t\t# ensure that a dirty exit halts cruft pack generation\n+\t\tgit config --add gc.recentObjectsHook ./extra-tips.c &&\n+\t\ttest_must_fail git repack --cruft --cruft-expiration=now -d 2>err &&\n+\t\tgrep \"unable to enumerate additional cruft tips\" err &&\n+\n+\t\t# and that the existing cruft pack is left alone\n+\t\ttest_path_is_file \"$mtimes\"\n+\t)\n+'\n+\n+test_expect_success 'additional cruft blobs via gc.recentObjectsHook' '\n+\tgit init repo &&\n+\ttest_when_finished \"rm -fr repo\" &&\n+\t(\n+\t\tcd repo &&\n+\n+\t\ttest_commit base &&\n+\n+\t\tblob=$(echo \"unreachable\" | git hash-object -w --stdin) &&\n+\n+\t\t# mark the unreachable blob we wrote above as having\n+\t\t# aged out of the retention period\n+\t\ttest-tool chmtime -2000 \"$objdir/$(test_oid_to_path $blob)\" &&\n+\n+\t\t# Write the script to list extra tips, which is just the\n+\t\t# extra blob as above.\n+\t\twrite_script extra-tips <<-EOF &&\n+\t\techo $blob\n+\t\tEOF\n+\t\tgit config gc.recentObjectsHook ./extra-tips &&\n+\n+\t\tgit repack --cruft --cruft-expiration=now -d &&\n+\n+\t\tmtimes=\"$(ls .git/objects/pack/pack-*.mtimes)\" &&\n+\t\tgit show-index <${mtimes%.mtimes}.idx >cruft &&\n+\t\tcut -d\" \" -f2 cruft >actual &&\n+\t\techo $blob >expect &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n test_done\ndiff --git a/t/t7701-repack-unpack-unreachable.sh b/t/t7701-repack-unpack-unreachable.sh\nindex ebb267855f..ba428c18a8 100755\n--- a/t/t7701-repack-unpack-unreachable.sh\n+++ b/t/t7701-repack-unpack-unreachable.sh\n@@ -113,6 +113,37 @@ test_expect_success 'do not bother loosening old objects' '\n \ttest_must_fail git cat-file -p $obj2\n '\n \n+test_expect_success 'gc.recentObjectsHook' '\n+\tobj1=$(echo one | git hash-object -w --stdin) &&\n+\tobj2=$(echo two | git hash-object -w --stdin) &&\n+\tobj3=$(echo three | git hash-object -w --stdin) &&\n+\tpack1=$(echo $obj1 | git pack-objects .git/objects/pack/pack) &&\n+\tpack2=$(echo $obj2 | git pack-objects .git/objects/pack/pack) &&\n+\tpack3=$(echo $obj3 | git pack-objects .git/objects/pack/pack) &&\n+\tgit prune-packed &&\n+\n+\tgit cat-file -p $obj1 &&\n+\tgit cat-file -p $obj2 &&\n+\tgit cat-file -p $obj3 &&\n+\n+\tgit tag -a -m tag obj2-tag $obj2 &&\n+\tobj2_tag=\"$(git rev-parse obj2-tag)\" &&\n+\n+\twrite_script precious-objects <<-EOF &&\n+\techo $obj2_tag\n+\tEOF\n+\tgit config gc.recentObjectsHook ./precious-objects &&\n+\n+\ttest-tool chmtime =-86400 .git/objects/pack/pack-$pack2.pack &&\n+\ttest-tool chmtime =-86400 .git/objects/pack/pack-$pack3.pack &&\n+\tgit repack -A -d --unpack-unreachable=1.hour.ago &&\n+\n+\tgit cat-file -p $obj1 &&\n+\tgit cat-file -p $obj2 &&\n+\tgit cat-file -p $obj2_tag &&\n+\ttest_must_fail git cat-file -p $obj3\n+'\n+\n test_expect_success 'keep packed objects found only in index' '\n \techo my-unique-content >file &&\n \tgit add file &&\n-- \n2.40.1.558.g18e50d2517\n"},{"id":"477704","messageId":"kl6l1qj5z56p.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"59731","inReplyTo":"18e50d2517ba4cc81d4bafb0b029ca7a770f23a7.1684196634.git.me@ttaylorr.com","subject":"Re: [PATCH v4 2/2] gc: introduce `gc.recentObjectsHook`","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2023-05-24T23:21:34Z","receivedAt":"2023-05-24T23:21:47Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Hi Taylor! It was great seeing you at Review Club today :)\n\nIf you'd like, the notes are available at:\n\n https://docs.google.com/document/d/14L8BAumGTpsXpjDY8VzZ4rRtpAjuGrFSRqn3stCuS_w/edit\n\nbut that's optional, since a ground rule is that all important feedback\nshould be sent to the mailing list.\n\nTaylor Blau writes:\n\n> This patch introduces a new multi-valued configuration option,\n> `gc.recentObjectsHook` as a means to mark certain objects as recent (and\n> thus exempt from garbage collection), regardless of their age.\n>\n> However, we have no convenient way to keep certain precious, unreachable\n> objects around in the repository, even if they have aged out and would\n> be pruned. Our options today consist of:\n>\n> - Point references at the reachability tips of any objects you\n> consider precious, which may be undesirable or infeasible.\n\nWhat I like about the hook is that it matches the desired use case quite\nwell - if an object is precious but unreachable, it can't be because Git\nthinks it's precious. Rather, something else thinks it's precious, so\nthe obvious fix is for Git to learn how to listen to that tool. Based\noff only what's in this commit message though, this feature doesn't seem\nsufficiently justified yet. In particular, it's not immediately clear\nhow this fits in with the alternatives, especially pointing refs at\nprecious objects (which is probably the most popular way people have\nbeen doing this). It would be useful to flesh out why keeping these\nextra refs are either \"undesirable\" or \"infeasible\". Presumably, you\nalready have some idea of why this is the case for the GitHub 'audit\nlog'.\n\nAnother potential use case I can think of is client-side third party Git\ntools that implement custom workflows on top of a Git repo, e.g.\ngit-branchless (https://github.com/arxanas/git-branchless) and jj\n(https://github.com/martinvonz/jj/, btw I contribute a little to jj\ntoo). Both of those tools reference commits that Git can consider\nunreachable (e.g. they support anonymous branches and \"undo\"\noperations), and so they both create custom refs to keep those commits\nalive. The number of refs isn't huge, though it is more than what a\ntypical repo would see (maybe in the 100s), and ISTR that some users\nhave reported that it's enough to noticeably slow down the \"git fetch\"\nnegotiation. I don't think managing these refs is \"infeasible\", and\nthere are workarounds to shrink the number of refs (e.g. creating an\noctopus merge of the commits you want and pointing a ref at it), but it\nwould certainly be a lot easier to use the hook instead.\n\nI've cc-ed the maintainers of both projects here in case they have more\nto share.\n\n> +gc.recentObjectsHook::\n\nI have a small preference to use \"command\" instead of \"hook\" to avoid\nconfusion with Git hooks (I've already observed some of this confusion\nin off-list conversations). There's some precedent for \"hook\" in\n`uploadpack.packObjectsHook`, but that's a one-off (and it used to\nconfuse me a lot too :P).\n\n> +\tWhen considering the recency of an object (e.g., when generating\n> +\ta cruft pack or storing unreachable objects as loose), use the\n> +\tshell to execute the specified command(s). Interpret their\n> +\toutput as object IDs which Git will consider as \"recent\",\n> +\tregardless of their age.\n\nFrom a potential user's POV, two things seem unclear:\n\n- What does \"recenct\" mean in this context? Does it just mean\n \"precious\"?\n- What objects do I need to list? Is it every object I want to keep or\n just the reachable tips?\n\nDocumentation/git-gc.txt has some pointers:\n\n . Any object with modification time newer than the `--prune` date is kept,\n along with everything reachable from it.\n\nbut it seems quite hard to discover this and put the pieces together. I\nwonder if we could make this easier by:\n\n- Replacing \"recent\" with \"precious\" (if this is really what we mean),\n and the fact that we use the recency check is an implementation\n detail.\n- Explicitly saying that everything reachable from the object is also\n kept.\n\nIn the code changes, I noticed a few out-of-date references to \"cruft\ntips\", but everything else looked reasonable to me.\n\n> diff --git a/t/t5329-pack-objects-cruft.sh b/t/t5329-pack-objects-cruft.sh\n> index 303f7a5d84..3ae61ca995 100755\n> --- a/t/t5329-pack-objects-cruft.sh\n> +++ b/t/t5329-pack-objects-cruft.sh\n> @@ -739,4 +739,175 @@ test_expect_success 'cruft objects are freshend via loose' '\n> \t)\n> '\n> \n> +test_expect_success 'gc.recentObjectsHook' '\n> +\tgit init repo &&\n> +\ttest_when_finished \"rm -fr repo\" &&\n> +\t(\n> +\t\tcd repo &&\n> +\n> +\t\t# Create a handful of objects.\n> +\t\t#\n> +\t\t# - one reachable commit, \"base\", designated for the reachable\n> +\t\t# pack\n> +\t\t# - one unreachable commit, \"cruft.discard\", which is marked\n> +\t\t# for deletion\n> +\t\t# - one unreachable commit, \"cruft.old\", which would be marked\n> +\t\t# for deletion, but is rescued as an extra cruft tip\n> +\t\t# - one unreachable commit, \"cruft.new\", which is not marked\n> +\t\t# for deletion\n> +\t\ttest_commit base &&\n> +\t\tgit branch -M main &&\n> +\n> +\t\tgit checkout --orphan discard &&\n> +\t\tgit rm -fr . &&\n> +\t\ttest_commit --no-tag cruft.discard &&\n> +\n> +\t\tgit checkout --orphan old &&\n> +\t\tgit rm -fr . &&\n> +\t\ttest_commit --no-tag cruft.old &&\n> +\t\tcruft_old=\"$(git rev-parse HEAD)\" &&\n> +\n> +\t\tgit checkout --orphan new &&\n> +\t\tgit rm -fr . &&\n> +\t\ttest_commit --no-tag cruft.new &&\n> +\t\tcruft_new=\"$(git rev-parse HEAD)\" &&\n> +\n> +\t\tgit checkout main &&\n> +\t\tgit branch -D discard old new &&\n> +\t\tgit reflog expire --all --expire=all &&\n> +\n> +\t\t# mark cruft.old with an mtime that is many minutes\n> +\t\t# older than the expiration period, and mark cruft.new\n> +\t\t# with an mtime that is in the future (and thus not\n> +\t\t# eligible for pruning).\n> +\t\ttest-tool chmtime -2000 \"$objdir/$(test_oid_to_path $cruft_old)\" &&\n> +\t\ttest-tool chmtime +1000 \"$objdir/$(test_oid_to_path $cruft_new)\" &&\n> +\n> +\t\t# Write the list of cruft objects we expect to\n> +\t\t# accumulate, which is comprised of everything reachable\n> +\t\t# from cruft.old and cruft.new, but not cruft.discard.\n> +\t\tgit rev-list --objects --no-object-names \\\n> +\t\t\t$cruft_old $cruft_new >cruft.raw &&\n> +\t\tsort cruft.raw >cruft.expect &&\n> +\n> +\t\t# Write the script to list extra tips, which are limited\n> +\t\t# to cruft.old, in this case.\n> +\t\twrite_script extra-tips <<-EOF &&\n> +\t\techo $cruft_old\n> +\t\tEOF\n> +\t\tgit config gc.recentObjectsHook ./extra-tips &&\n> +\n> +\t\tgit repack --cruft --cruft-expiration=now -d &&\n> +\n> +\t\tmtimes=\"$(ls .git/objects/pack/pack-*.mtimes)\" &&\n> +\t\tgit show-index <${mtimes%.mtimes}.idx >cruft &&\n> +\t\tcut -d\" \" -f2 cruft | sort >cruft.actual &&\n> +\t\ttest_cmp cruft.expect cruft.actual &&\n> +\n> +\t\t# Ensure that the \"old\" objects are removed after\n> +\t\t# dropping the gc.recentObjectsHook hook.\n> +\t\tgit config --unset gc.recentObjectsHook &&\n> +\t\tgit repack --cruft --cruft-expiration=now -d &&\n> +\n> +\t\tmtimes=\"$(ls .git/objects/pack/pack-*.mtimes)\" &&\n> +\t\tgit show-index <${mtimes%.mtimes}.idx >cruft &&\n> +\t\tcut -d\" \" -f2 cruft | sort >cruft.actual &&\n> +\n> +\t\tgit rev-list --objects --no-object-names $cruft_new >cruft.raw &&\n> +\t\tcp cruft.expect cruft.old &&\n> +\t\tsort cruft.raw >cruft.expect &&\n> +\t\ttest_cmp cruft.expect cruft.actual &&\n> +\n> +\t\t# ensure objects which are no longer in the cruft pack were\n> +\t\t# removed from the repository\n> +\t\tfor object in $(comm -13 cruft.expect cruft.old)\n> +\t\tdo\n> +\t\t\ttest_must_fail git cat-file -t $object || return 1\n> +\t\tdone\n> +\t)\n\nThanks for the comments. The tests are quite verbose, and I wouldn't\nbe able to understand them otherwise. I thought it would be nice to have\na test helper to check that the cruft pack contains the expected objects\n(replacing the \"git show-index\" + \"cut | sort\" + \"test_cmp\" recipe), but\nthis test fits in with the existing style, so I don't consider that a\nblocker.\n"},{"id":"478155","messageId":"ZIELIAQmW9qWyUdM@nand.local","threadId":"59731","inReplyTo":"kl6l1qj5z56p.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re: [PATCH v4 2/2] gc: introduce `gc.recentObjectsHook`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-06-07T22:56:32Z","receivedAt":"2023-06-07T22:57:07Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi Glen,\n\nApologies for my delayed response. Now that we are on the other side of\n2.41 I think that it's the right time to pick this back up again.\n\nOn Wed, May 24, 2023 at 04:21:34PM -0700, Glen Choo wrote:\n> Hi Taylor! It was great seeing you at Review Club today :)\n\nIt was fun :-).\n\n> It would be useful to flesh out why keeping these extra refs are\n> either \"undesirable\" or \"infeasible\". Presumably, you already have\n> some idea of why this is the case for the GitHub 'audit log'.\n\nYes, thanks for suggesting it. I think the gap in my explanation is\n\"[...] if there are many such objects\". Hopefully that clarifies it.\n\n> Another potential use case I can think of is client-side third party Git\n> tools that implement custom workflows on top of a Git repo, e.g.\n> git-branchless (https://github.com/arxanas/git-branchless) and jj\n> (https://github.com/martinvonz/jj/, btw I contribute a little to jj\n> too).\n\nI thought that this was an interesting part of the discussion. I hadn't\nthought of it when writing up these patches, but I think that it could\nbe potentially useful for those tools if they want to keep around some\nprecious set of metadata objects without having to point refs at them.\n\nIt also introduces a little bit of a higher barrier between the tool and\nuser to destroy those objects. Without pinning them with this hook, all\na user has to do to remove them is drop the reference(s) which points at\nthem, and then GC. Now they'd have to modify the hook, etc.\n\n> > +gc.recentObjectsHook::\n>\n> I have a small preference to use \"command\" instead of \"hook\" to avoid\n> confusion with Git hooks (I've already observed some of this confusion\n> in off-list conversations). There's some precedent for \"hook\" in\n> `uploadpack.packObjectsHook`, but that's a one-off (and it used to\n> confuse me a lot too :P).\n\nUnless you feel strongly, let's leave it as-is. \"gc.recentObjectsHook\"\nis the third iteration of this name, and I'd like to avoid spending much\nmore time on naming if we can help it.\n\n> > +\tWhen considering the recency of an object (e.g., when generating\n> > +\ta cruft pack or storing unreachable objects as loose), use the\n> > +\tshell to execute the specified command(s). Interpret their\n> > +\toutput as object IDs which Git will consider as \"recent\",\n> > +\tregardless of their age.\n>\n> >From a potential user's POV, two things seem unclear:\n>\n> - What does \"recenct\" mean in this context? Does it just mean\n> \"precious\"?\n> - What objects do I need to list? Is it every object I want to keep or\n> just the reachable tips?\n\nTo answer your questions: recency is referring to the \"mtime\" of an\nobject [^1], not whether or not it is precious. I clarified this by\nremoving the term \"recent\" from this sentence altogether, to instead\nread:\n\n \"When considering whether or not to remove an object [...]\"\n\nYou only need to list the tips, since Git will treat the output of the\nhook as input to a traversal which allows for missing objects. Any\nobject visited along that traversal will be kept in the repository and\nrescued from deletion. I tried to clarify this by adding a final\nsentence (emphasis mine):\n\n \"By treating their mtimes as \"now\", any objects **(and their\n descendants)** mentioned in the output will be kept regardless of\n their true age.\"\n\n> In the code changes, I noticed a few out-of-date references to \"cruft\n> tips\", but everything else looked reasonable to me.\n\nThanks, I'll clean those up and resubmit it with the above fixes\nsquashed in.\n\nThanks,\nTaylor\n\n[^1]: an object's mtime is the most recent of (1) the st_mtime of a its\n loose object file, if it exists, (2) the st_mtime of any non-cruft\n pack(s) that contain that object, or (3) the value listed in the .mtimes\n file of the cruft pack corresponding to that object's entry.\n"},{"id":"478156","messageId":"cover.1686178684.git.me@ttaylorr.com","threadId":"59731","inReplyTo":"cover.1683847221.git.me@ttaylorr.com","subject":"[PATCH v5 0/2] gc: introduce `gc.recentObjectsHook`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-06-07T22:58:07Z","receivedAt":"2023-06-07T22:58:15Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Here is a version of my series to implement the mutli-valued\n\"gc.recentObjectHook\" configuration, which allows users to define custom\nprograms whose output determines an additional set of objects to retain\n(along with their descendants) when GCing, regardless of their true age.\n\nIt is largely the same as the previous round, with the following\nchanges:\n\n - a small handful of wordsmithing changes in the second patch's\n message thanks to a very helpful review from the Google team's\n \"Review Club\" meeting.\n\n - removed a pair of stray references to \"cruft tips\" in favor of\n \"recent objects\".\n\n - rebased onto v2.41.0\n\nBased on previous rounds of review, I think that this version should be\npretty much ready to go. But I'd appreciate some extra eyes on it just\nto make sure.\n\nThanks in advance for your (hopefully final!) review.\n\nTaylor Blau (2):\n reachable.c: extract `obj_is_recent()`\n gc: introduce `gc.recentObjectsHook`\n\n Documentation/config/gc.txt | 15 +++\n reachable.c | 85 ++++++++++++-\n t/t5304-prune.sh | 14 +++\n t/t5329-pack-objects-cruft.sh | 171 +++++++++++++++++++++++++++\n t/t7701-repack-unpack-unreachable.sh | 31 +++++\n 5 files changed, 313 insertions(+), 3 deletions(-)\n\nRange-diff against v4:\n1: 9c1b59c8cf = 1: 38c4c4a17f reachable.c: extract `obj_is_recent()`\n2: 18e50d2517 ! 2: f661b54941 gc: introduce `gc.recentObjectsHook`\n @@ Commit message\n be pruned. Our options today consist of:\n \n - Point references at the reachability tips of any objects you\n - consider precious, which may be undesirable or infeasible.\n + consider precious, which may be undesirable or infeasible if there\n + are many such objects.\n \n - Track them via the reflog, which may be undesirable since the\n reflog's lifetime is limited to that of the reference it's tracking\n @@ Documentation/config/gc.txt: or rebase occurring. Since these changes are not p\n default is more aggressive than `gc.reflogExpire`.\n \n +gc.recentObjectsHook::\n -+\tWhen considering the recency of an object (e.g., when generating\n -+\ta cruft pack or storing unreachable objects as loose), use the\n -+\tshell to execute the specified command(s). Interpret their\n -+\toutput as object IDs which Git will consider as \"recent\",\n -+\tregardless of their age.\n ++\tWhen considering whether or not to remove an object (either when\n ++\tgenerating a cruft pack or storing unreachable objects as\n ++\tloose), use the shell to execute the specified command(s).\n ++\tInterpret their output as object IDs which Git will consider as\n ++\t\"recent\", regardless of their age. By treating their mtimes as\n ++\t\"now\", any objects (and their descendants) mentioned in the\n ++\toutput will be kept regardless of their true age.\n ++\n +Output must contain exactly one hex object ID per line, and nothing\n +else. Objects which cannot be found in the repository are ignored.\n @@ reachable.c: struct recent_data {\n +\t\tret = run_one_gc_recent_objects_hook(&data->extra_recent_oids,\n +\t\t\t\t\t\t programs->items[i].string);\n +\t\tif (ret)\n -+\t\t\tdie(_(\"unable to enumerate additional cruft tips\"));\n ++\t\t\tdie(_(\"unable to enumerate additional recent objects\"));\n +\t}\n +}\n +\n @@ t/t5329-pack-objects-cruft.sh: test_expect_success 'cruft objects are freshend v\n +\t\t# ensure that a dirty exit halts cruft pack generation\n +\t\tgit config --add gc.recentObjectsHook ./extra-tips.c &&\n +\t\ttest_must_fail git repack --cruft --cruft-expiration=now -d 2>err &&\n -+\t\tgrep \"unable to enumerate additional cruft tips\" err &&\n ++\t\tgrep \"unable to enumerate additional recent objects\" err &&\n +\n +\t\t# and that the existing cruft pack is left alone\n +\t\ttest_path_is_file \"$mtimes\"\n-- \n2.41.0.2.gaaae24b3a6.dirty\n"},{"id":"478157","messageId":"38c4c4a17fc71dd301bbdbf6c9515fcd75053478.1686178684.git.me@ttaylorr.com","threadId":"59731","inReplyTo":"cover.1686178684.git.me@ttaylorr.com","subject":"[PATCH v5 1/2] reachable.c: extract `obj_is_recent()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-06-07T22:58:12Z","receivedAt":"2023-06-07T22:58:39Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"When enumerating objects in order to add recent ones (i.e. those whose\nmtime is strictly newer than the cutoff) as tips of a reachability\ntraversal, `add_recent_object()` discards objects which do not meet the\nrecency criteria.\n\nThe subsequent commit will make checking whether or not an object is\nrecent also consult the list of hooks in `pack.recentHook`. Isolate this\ncheck in its own function to keep the additional complexity outside of\n`add_recent_object()`.\n\nSigned-off-by: Taylor Blau \n---\n reachable.c | 8 +++++++-\n 1 file changed, 7 insertions(+), 1 deletion(-)\n\ndiff --git a/reachable.c b/reachable.c\nindex 55bb114353..7a42da5d39 100644\n--- a/reachable.c\n+++ b/reachable.c\n@@ -69,6 +69,12 @@ struct recent_data {\n \tint ignore_in_core_kept_packs;\n };\n \n+static int obj_is_recent(const struct object_id *oid, timestamp_t mtime,\n+\t\t\t struct recent_data *data)\n+{\n+\treturn mtime > data->timestamp;\n+}\n+\n static void add_recent_object(const struct object_id *oid,\n \t\t\t struct packed_git *pack,\n \t\t\t off_t offset,\n@@ -78,7 +84,7 @@ static void add_recent_object(const struct object_id *oid,\n \tstruct object *obj;\n \tenum object_type type;\n \n-\tif (mtime <= data->timestamp)\n+\tif (!obj_is_recent(oid, mtime, data))\n \t\treturn;\n \n \t/*\n-- \n2.41.0.2.gaaae24b3a6.dirty\n\n"},{"id":"478158","messageId":"f661b54941a12c9bd8e226aebb4f0d53c10479c8.1686178684.git.me@ttaylorr.com","threadId":"59731","inReplyTo":"cover.1686178684.git.me@ttaylorr.com","subject":"[PATCH v5 2/2] gc: introduce `gc.recentObjectsHook`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-06-07T22:58:17Z","receivedAt":"2023-06-07T22:58:45Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"This patch introduces a new multi-valued configuration option,\n`gc.recentObjectsHook` as a means to mark certain objects as recent (and\nthus exempt from garbage collection), regardless of their age.\n\nWhen performing a garbage collection operation on a repository with\nunreachable objects, Git makes its decision on what to do with those\nobject(s) bed on how recent the objects are or not. Generally speaking,\nunreachable-but-recent objects stay in the repository, and older objects\nare discarded.\n\nHowever, we have no convenient way to keep certain precious, unreachable\nobjects around in the repository, even if they have aged out and would\nbe pruned. Our options today consist of:\n\n - Point references at the reachability tips of any objects you\n consider precious, which may be undesirable or infeasible if there\n are many such objects.\n\n - Track them via the reflog, which may be undesirable since the\n reflog's lifetime is limited to that of the reference it's tracking\n (and callers may want to keep those unreachable objects around for\n longer).\n\n - Extend the grace period, which may keep around other objects that\n the caller *does* want to discard.\n\n - Manually modify the mtimes of objects you want to keep. If those\n objects are already loose, this is easy enough to do (you can just\n enumerate and `touch -m` each one).\n\n But if they are packed, you will either end up modifying the mtimes\n of *all* objects in that pack, or be forced to write out a loose\n copy of that object, both of which may be undesirable. Even worse,\n if they are in a cruft pack, that requires modifying its `*.mtimes`\n file by hand, since there is no exposed plumbing for this.\n\n - Force the caller to construct the pack of objects they want\n to keep themselves, and then mark the pack as kept by adding a\n \".keep\" file. This works, but is burdensome for the caller, and\n having extra packs is awkward as you roll forward your cruft pack.\n\nThis patch introduces a new option to the above list via the\n`gc.recentObjectsHook` configuration, which allows the caller to\nspecify a program (or set of programs) whose output is treated as a set\nof objects to treat as recent, regardless of their true age.\n\nThe implementation is straightforward. Git enumerates recent objects via\n`add_unseen_recent_objects_to_traversal()`, which enumerates loose and\npacked objects, and eventually calls add_recent_object() on any objects\nfor which `want_recent_object()`'s conditions are met.\n\nThis patch modifies the recency condition from simply \"is the mtime of\nthis object more recent than the cutoff?\" to \"[...] or, is this object\nmentioned by at least one `gc.recentObjectsHook`?\".\n\nDepending on whether or not we are generating a cruft pack, this allows\nthe caller to do one of two things:\n\n - If generating a cruft pack, the caller is able to retain additional\n objects via the cruft pack, even if they would have otherwise been\n pruned due to their age.\n\n - If not generating a cruft pack, the caller is likewise able to\n retain additional objects as loose.\n\nA potential alternative here is to introduce a new mode to alter the\ncontents of the reachable pack instead of the cruft one. One could\nimagine a new option to `pack-objects`, say `--extra-reachable-tips`\nthat does the same thing as above, adding the visited set of objects\nalong the traversal to the pack.\n\nBut this has the unfortunate side-effect of altering the reachability\nclosure of that pack. If parts of the unreachable object graph mentioned\nby one or more of the \"extra reachable tips\" programs is not closed,\nthen the resulting pack won't be either. This makes it impossible in the\ngeneral case to write out reachability bitmaps for that pack, since\nclosure is a requirement there.\n\nInstead, keep these unreachable objects in the cruft pack (or set of\nunreachable, loose objects) instead, to ensure that we can continue to\nhave a pack containing just reachable objects, which is always safe to\nwrite a bitmap over.\n\nHelped-by: Jeff King \nSigned-off-by: Taylor Blau \n---\n Documentation/config/gc.txt | 15 +++\n reachable.c | 79 ++++++++++++-\n t/t5304-prune.sh | 14 +++\n t/t5329-pack-objects-cruft.sh | 171 +++++++++++++++++++++++++++\n t/t7701-repack-unpack-unreachable.sh | 31 +++++\n 5 files changed, 307 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/config/gc.txt b/Documentation/config/gc.txt\nindex 7f95c866e1..ca47eb2008 100644\n--- a/Documentation/config/gc.txt\n+++ b/Documentation/config/gc.txt\n@@ -130,6 +130,21 @@ or rebase occurring. Since these changes are not part of the current\n project most users will want to expire them sooner, which is why the\n default is more aggressive than `gc.reflogExpire`.\n \n+gc.recentObjectsHook::\n+\tWhen considering whether or not to remove an object (either when\n+\tgenerating a cruft pack or storing unreachable objects as\n+\tloose), use the shell to execute the specified command(s).\n+\tInterpret their output as object IDs which Git will consider as\n+\t\"recent\", regardless of their age. By treating their mtimes as\n+\t\"now\", any objects (and their descendants) mentioned in the\n+\toutput will be kept regardless of their true age.\n++\n+Output must contain exactly one hex object ID per line, and nothing\n+else. Objects which cannot be found in the repository are ignored.\n+Multiple hooks are supported, but all must exit successfully, else the\n+operation (either generating a cruft pack or unpacking unreachable\n+objects) will be halted.\n+\n gc.rerereResolved::\n \tRecords of conflicted merge you resolved earlier are\n \tkept for this many days when 'git rerere gc' is run.\ndiff --git a/reachable.c b/reachable.c\nindex 7a42da5d39..60a7336b87 100644\n--- a/reachable.c\n+++ b/reachable.c\n@@ -16,6 +16,8 @@\n #include \"object-store.h\"\n #include \"pack-bitmap.h\"\n #include \"pack-mtimes.h\"\n+#include \"config.h\"\n+#include \"run-command.h\"\n \n struct connectivity_progress {\n \tstruct progress *progress;\n@@ -67,12 +69,75 @@ struct recent_data {\n \ttimestamp_t timestamp;\n \treport_recent_object_fn *cb;\n \tint ignore_in_core_kept_packs;\n+\n+\tstruct oidset extra_recent_oids;\n+\tint extra_recent_oids_loaded;\n };\n \n+static int run_one_gc_recent_objects_hook(struct oidset *set,\n+\t\t\t\t\t const char *args)\n+{\n+\tstruct child_process cmd = CHILD_PROCESS_INIT;\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tFILE *out;\n+\tint ret = 0;\n+\n+\tcmd.use_shell = 1;\n+\tcmd.out = -1;\n+\n+\tstrvec_push(&cmd.args, args);\n+\n+\tif (start_command(&cmd))\n+\t\treturn -1;\n+\n+\tout = xfdopen(cmd.out, \"r\");\n+\twhile (strbuf_getline(&buf, out) != EOF) {\n+\t\tstruct object_id oid;\n+\t\tconst char *rest;\n+\n+\t\tif (parse_oid_hex(buf.buf, &oid, &rest) || *rest) {\n+\t\t\tret = error(_(\"invalid extra cruft tip: '%s'\"), buf.buf);\n+\t\t\tbreak;\n+\t\t}\n+\n+\t\toidset_insert(set, &oid);\n+\t}\n+\n+\tfclose(out);\n+\tret |= finish_command(&cmd);\n+\n+\tstrbuf_release(&buf);\n+\treturn ret;\n+}\n+\n+static void load_gc_recent_objects(struct recent_data *data)\n+{\n+\tconst struct string_list *programs;\n+\tint ret = 0;\n+\tsize_t i;\n+\n+\tdata->extra_recent_oids_loaded = 1;\n+\n+\tif (git_config_get_string_multi(\"gc.recentobjectshook\", &programs))\n+\t\treturn;\n+\n+\tfor (i = 0; i < programs->nr; i++) {\n+\t\tret = run_one_gc_recent_objects_hook(&data->extra_recent_oids,\n+\t\t\t\t\t\t programs->items[i].string);\n+\t\tif (ret)\n+\t\t\tdie(_(\"unable to enumerate additional recent objects\"));\n+\t}\n+}\n+\n static int obj_is_recent(const struct object_id *oid, timestamp_t mtime,\n \t\t\t struct recent_data *data)\n {\n-\treturn mtime > data->timestamp;\n+\tif (mtime > data->timestamp)\n+\t\treturn 1;\n+\n+\tif (!data->extra_recent_oids_loaded)\n+\t\tload_gc_recent_objects(data);\n+\treturn oidset_contains(&data->extra_recent_oids, oid);\n }\n \n static void add_recent_object(const struct object_id *oid,\n@@ -199,16 +264,24 @@ int add_unseen_recent_objects_to_traversal(struct rev_info *revs,\n \tdata.cb = cb;\n \tdata.ignore_in_core_kept_packs = ignore_in_core_kept_packs;\n \n+\toidset_init(&data.extra_recent_oids, 0);\n+\tdata.extra_recent_oids_loaded = 0;\n+\n \tr = for_each_loose_object(add_recent_loose, &data,\n \t\t\t\t FOR_EACH_OBJECT_LOCAL_ONLY);\n \tif (r)\n-\t\treturn r;\n+\t\tgoto done;\n \n \tflags = FOR_EACH_OBJECT_LOCAL_ONLY | FOR_EACH_OBJECT_PACK_ORDER;\n \tif (ignore_in_core_kept_packs)\n \t\tflags |= FOR_EACH_OBJECT_SKIP_IN_CORE_KEPT_PACKS;\n \n-\treturn for_each_packed_object(add_recent_packed, &data, flags);\n+\tr = for_each_packed_object(add_recent_packed, &data, flags);\n+\n+done:\n+\toidset_clear(&data.extra_recent_oids);\n+\n+\treturn r;\n }\n \n static int mark_object_seen(const struct object_id *oid,\ndiff --git a/t/t5304-prune.sh b/t/t5304-prune.sh\nindex f367327441..b4df545e5a 100755\n--- a/t/t5304-prune.sh\n+++ b/t/t5304-prune.sh\n@@ -350,4 +350,18 @@ test_expect_success 'old reachable-from-recent retained with bitmaps' '\n \ttest_must_fail git cat-file -e $to_drop\n '\n \n+test_expect_success 'gc.recentObjectsHook' '\n+\tadd_blob &&\n+\ttest-tool chmtime =-86500 $BLOB_FILE &&\n+\n+\twrite_script precious-objects <<-EOF &&\n+\techo $BLOB\n+\tEOF\n+\ttest_config gc.recentObjectsHook ./precious-objects &&\n+\n+\tgit prune --expire=now &&\n+\n+\tgit cat-file -p $BLOB\n+'\n+\n test_done\ndiff --git a/t/t5329-pack-objects-cruft.sh b/t/t5329-pack-objects-cruft.sh\nindex 303f7a5d84..45667d4999 100755\n--- a/t/t5329-pack-objects-cruft.sh\n+++ b/t/t5329-pack-objects-cruft.sh\n@@ -739,4 +739,175 @@ test_expect_success 'cruft objects are freshend via loose' '\n \t)\n '\n \n+test_expect_success 'gc.recentObjectsHook' '\n+\tgit init repo &&\n+\ttest_when_finished \"rm -fr repo\" &&\n+\t(\n+\t\tcd repo &&\n+\n+\t\t# Create a handful of objects.\n+\t\t#\n+\t\t# - one reachable commit, \"base\", designated for the reachable\n+\t\t# pack\n+\t\t# - one unreachable commit, \"cruft.discard\", which is marked\n+\t\t# for deletion\n+\t\t# - one unreachable commit, \"cruft.old\", which would be marked\n+\t\t# for deletion, but is rescued as an extra cruft tip\n+\t\t# - one unreachable commit, \"cruft.new\", which is not marked\n+\t\t# for deletion\n+\t\ttest_commit base &&\n+\t\tgit branch -M main &&\n+\n+\t\tgit checkout --orphan discard &&\n+\t\tgit rm -fr . &&\n+\t\ttest_commit --no-tag cruft.discard &&\n+\n+\t\tgit checkout --orphan old &&\n+\t\tgit rm -fr . &&\n+\t\ttest_commit --no-tag cruft.old &&\n+\t\tcruft_old=\"$(git rev-parse HEAD)\" &&\n+\n+\t\tgit checkout --orphan new &&\n+\t\tgit rm -fr . &&\n+\t\ttest_commit --no-tag cruft.new &&\n+\t\tcruft_new=\"$(git rev-parse HEAD)\" &&\n+\n+\t\tgit checkout main &&\n+\t\tgit branch -D discard old new &&\n+\t\tgit reflog expire --all --expire=all &&\n+\n+\t\t# mark cruft.old with an mtime that is many minutes\n+\t\t# older than the expiration period, and mark cruft.new\n+\t\t# with an mtime that is in the future (and thus not\n+\t\t# eligible for pruning).\n+\t\ttest-tool chmtime -2000 \"$objdir/$(test_oid_to_path $cruft_old)\" &&\n+\t\ttest-tool chmtime +1000 \"$objdir/$(test_oid_to_path $cruft_new)\" &&\n+\n+\t\t# Write the list of cruft objects we expect to\n+\t\t# accumulate, which is comprised of everything reachable\n+\t\t# from cruft.old and cruft.new, but not cruft.discard.\n+\t\tgit rev-list --objects --no-object-names \\\n+\t\t\t$cruft_old $cruft_new >cruft.raw &&\n+\t\tsort cruft.raw >cruft.expect &&\n+\n+\t\t# Write the script to list extra tips, which are limited\n+\t\t# to cruft.old, in this case.\n+\t\twrite_script extra-tips <<-EOF &&\n+\t\techo $cruft_old\n+\t\tEOF\n+\t\tgit config gc.recentObjectsHook ./extra-tips &&\n+\n+\t\tgit repack --cruft --cruft-expiration=now -d &&\n+\n+\t\tmtimes=\"$(ls .git/objects/pack/pack-*.mtimes)\" &&\n+\t\tgit show-index <${mtimes%.mtimes}.idx >cruft &&\n+\t\tcut -d\" \" -f2 cruft | sort >cruft.actual &&\n+\t\ttest_cmp cruft.expect cruft.actual &&\n+\n+\t\t# Ensure that the \"old\" objects are removed after\n+\t\t# dropping the gc.recentObjectsHook hook.\n+\t\tgit config --unset gc.recentObjectsHook &&\n+\t\tgit repack --cruft --cruft-expiration=now -d &&\n+\n+\t\tmtimes=\"$(ls .git/objects/pack/pack-*.mtimes)\" &&\n+\t\tgit show-index <${mtimes%.mtimes}.idx >cruft &&\n+\t\tcut -d\" \" -f2 cruft | sort >cruft.actual &&\n+\n+\t\tgit rev-list --objects --no-object-names $cruft_new >cruft.raw &&\n+\t\tcp cruft.expect cruft.old &&\n+\t\tsort cruft.raw >cruft.expect &&\n+\t\ttest_cmp cruft.expect cruft.actual &&\n+\n+\t\t# ensure objects which are no longer in the cruft pack were\n+\t\t# removed from the repository\n+\t\tfor object in $(comm -13 cruft.expect cruft.old)\n+\t\tdo\n+\t\t\ttest_must_fail git cat-file -t $object || return 1\n+\t\tdone\n+\t)\n+'\n+\n+test_expect_success 'multi-valued gc.recentObjectsHook' '\n+\tgit init repo &&\n+\ttest_when_finished \"rm -fr repo\" &&\n+\t(\n+\t\tcd repo &&\n+\n+\t\ttest_commit base &&\n+\t\tgit branch -M main &&\n+\n+\t\tgit checkout --orphan cruft.a &&\n+\t\tgit rm -fr . &&\n+\t\ttest_commit --no-tag cruft.a &&\n+\t\tcruft_a=\"$(git rev-parse HEAD)\" &&\n+\n+\t\tgit checkout --orphan cruft.b &&\n+\t\tgit rm -fr . &&\n+\t\ttest_commit --no-tag cruft.b &&\n+\t\tcruft_b=\"$(git rev-parse HEAD)\" &&\n+\n+\t\tgit checkout main &&\n+\t\tgit branch -D cruft.a cruft.b &&\n+\t\tgit reflog expire --all --expire=all &&\n+\n+\t\techo \"echo $cruft_a\" | write_script extra-tips.a &&\n+\t\techo \"echo $cruft_b\" | write_script extra-tips.b &&\n+\t\techo \"false\" | write_script extra-tips.c &&\n+\n+\t\tgit rev-list --objects --no-object-names $cruft_a $cruft_b \\\n+\t\t\t>cruft.raw &&\n+\t\tsort cruft.raw >cruft.expect &&\n+\n+\t\t# ensure that each extra cruft tip is saved by its\n+\t\t# respective hook\n+\t\tgit config --add gc.recentObjectsHook ./extra-tips.a &&\n+\t\tgit config --add gc.recentObjectsHook ./extra-tips.b &&\n+\t\tgit repack --cruft --cruft-expiration=now -d &&\n+\n+\t\tmtimes=\"$(ls .git/objects/pack/pack-*.mtimes)\" &&\n+\t\tgit show-index <${mtimes%.mtimes}.idx >cruft &&\n+\t\tcut -d\" \" -f2 cruft | sort >cruft.actual &&\n+\t\ttest_cmp cruft.expect cruft.actual &&\n+\n+\t\t# ensure that a dirty exit halts cruft pack generation\n+\t\tgit config --add gc.recentObjectsHook ./extra-tips.c &&\n+\t\ttest_must_fail git repack --cruft --cruft-expiration=now -d 2>err &&\n+\t\tgrep \"unable to enumerate additional recent objects\" err &&\n+\n+\t\t# and that the existing cruft pack is left alone\n+\t\ttest_path_is_file \"$mtimes\"\n+\t)\n+'\n+\n+test_expect_success 'additional cruft blobs via gc.recentObjectsHook' '\n+\tgit init repo &&\n+\ttest_when_finished \"rm -fr repo\" &&\n+\t(\n+\t\tcd repo &&\n+\n+\t\ttest_commit base &&\n+\n+\t\tblob=$(echo \"unreachable\" | git hash-object -w --stdin) &&\n+\n+\t\t# mark the unreachable blob we wrote above as having\n+\t\t# aged out of the retention period\n+\t\ttest-tool chmtime -2000 \"$objdir/$(test_oid_to_path $blob)\" &&\n+\n+\t\t# Write the script to list extra tips, which is just the\n+\t\t# extra blob as above.\n+\t\twrite_script extra-tips <<-EOF &&\n+\t\techo $blob\n+\t\tEOF\n+\t\tgit config gc.recentObjectsHook ./extra-tips &&\n+\n+\t\tgit repack --cruft --cruft-expiration=now -d &&\n+\n+\t\tmtimes=\"$(ls .git/objects/pack/pack-*.mtimes)\" &&\n+\t\tgit show-index <${mtimes%.mtimes}.idx >cruft &&\n+\t\tcut -d\" \" -f2 cruft >actual &&\n+\t\techo $blob >expect &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n test_done\ndiff --git a/t/t7701-repack-unpack-unreachable.sh b/t/t7701-repack-unpack-unreachable.sh\nindex ebb267855f..ba428c18a8 100755\n--- a/t/t7701-repack-unpack-unreachable.sh\n+++ b/t/t7701-repack-unpack-unreachable.sh\n@@ -113,6 +113,37 @@ test_expect_success 'do not bother loosening old objects' '\n \ttest_must_fail git cat-file -p $obj2\n '\n \n+test_expect_success 'gc.recentObjectsHook' '\n+\tobj1=$(echo one | git hash-object -w --stdin) &&\n+\tobj2=$(echo two | git hash-object -w --stdin) &&\n+\tobj3=$(echo three | git hash-object -w --stdin) &&\n+\tpack1=$(echo $obj1 | git pack-objects .git/objects/pack/pack) &&\n+\tpack2=$(echo $obj2 | git pack-objects .git/objects/pack/pack) &&\n+\tpack3=$(echo $obj3 | git pack-objects .git/objects/pack/pack) &&\n+\tgit prune-packed &&\n+\n+\tgit cat-file -p $obj1 &&\n+\tgit cat-file -p $obj2 &&\n+\tgit cat-file -p $obj3 &&\n+\n+\tgit tag -a -m tag obj2-tag $obj2 &&\n+\tobj2_tag=\"$(git rev-parse obj2-tag)\" &&\n+\n+\twrite_script precious-objects <<-EOF &&\n+\techo $obj2_tag\n+\tEOF\n+\tgit config gc.recentObjectsHook ./precious-objects &&\n+\n+\ttest-tool chmtime =-86400 .git/objects/pack/pack-$pack2.pack &&\n+\ttest-tool chmtime =-86400 .git/objects/pack/pack-$pack3.pack &&\n+\tgit repack -A -d --unpack-unreachable=1.hour.ago &&\n+\n+\tgit cat-file -p $obj1 &&\n+\tgit cat-file -p $obj2 &&\n+\tgit cat-file -p $obj2_tag &&\n+\ttest_must_fail git cat-file -p $obj3\n+'\n+\n test_expect_success 'keep packed objects found only in index' '\n \techo my-unique-content >file &&\n \tgit add file &&\n-- \n2.41.0.2.gaaae24b3a6.dirty\n"},{"id":"478187","messageId":"kl6lwn0cfbyh.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"59731","inReplyTo":"f661b54941a12c9bd8e226aebb4f0d53c10479c8.1686178684.git.me@ttaylorr.com","subject":"Re: [PATCH v5 2/2] gc: introduce `gc.recentObjectsHook`","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2023-06-09T23:33:10Z","receivedAt":"2023-06-09T23:36:08Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Taylor Blau writes:\n\n> When performing a garbage collection operation on a repository with\n> unreachable objects, Git makes its decision on what to do with those\n> object(s) bed on how recent the objects are or not. Generally speaking,\n\ns/bed/based/ ?\n\n> +gc.recentObjectsHook::\n> +\tWhen considering whether or not to remove an object (either when\n> +\tgenerating a cruft pack or storing unreachable objects as\n> +\tloose), use the shell to execute the specified command(s).\n> +\tInterpret their output as object IDs which Git will consider as\n> +\t\"recent\", regardless of their age. By treating their mtimes as\n> +\t\"now\", any objects (and their descendants) mentioned in the\n> +\toutput will be kept regardless of their true age.\n\nThanks! I think this version will be clear to prospective users.\n\n> +static int run_one_gc_recent_objects_hook(struct oidset *set,\n> +\t\t\t\t\t const char *args)\n> +{\n> +\tstruct child_process cmd = CHILD_PROCESS_INIT;\n> +\tstruct strbuf buf = STRBUF_INIT;\n> +\tFILE *out;\n> +\tint ret = 0;\n> +\n> +\tcmd.use_shell = 1;\n> +\tcmd.out = -1;\n> +\n> +\tstrvec_push(&cmd.args, args);\n> +\n> +\tif (start_command(&cmd))\n> +\t\treturn -1;\n> +\n> +\tout = xfdopen(cmd.out, \"r\");\n> +\twhile (strbuf_getline(&buf, out) != EOF) {\n> +\t\tstruct object_id oid;\n> +\t\tconst char *rest;\n> +\n> +\t\tif (parse_oid_hex(buf.buf, &oid, &rest) || *rest) {\n> +\t\t\tret = error(_(\"invalid extra cruft tip: '%s'\"), buf.buf);\n\nTo be consistent with the other error message, perhaps s/extra cruft\ntip/additional recent object/?\n\n> +static void load_gc_recent_objects(struct recent_data *data)\n> +{\n> +\tconst struct string_list *programs;\n> +\tint ret = 0;\n> +\tsize_t i;\n> +\n> +\tdata->extra_recent_oids_loaded = 1;\n> +\n> +\tif (git_config_get_string_multi(\"gc.recentobjectshook\", &programs))\n> +\t\treturn;\n> +\n> +\tfor (i = 0; i < programs->nr; i++) {\n> +\t\tret = run_one_gc_recent_objects_hook(&data->extra_recent_oids,\n> +\t\t\t\t\t\t programs->items[i].string);\n> +\t\tif (ret)\n> +\t\t\tdie(_(\"unable to enumerate additional recent objects\"));\n\nThis error message to be exact.\n\n> +test_expect_success 'gc.recentObjectsHook' '\n> +\tgit init repo &&\n> +\ttest_when_finished \"rm -fr repo\" &&\n> +\t(\n> +\t\tcd repo &&\n> +\n> +\t\t# Create a handful of objects.\n> +\t\t#\n> +\t\t# - one reachable commit, \"base\", designated for the reachable\n> +\t\t# pack\n> +\t\t# - one unreachable commit, \"cruft.discard\", which is marked\n> +\t\t# for deletion\n> +\t\t# - one unreachable commit, \"cruft.old\", which would be marked\n> +\t\t# for deletion, but is rescued as an extra cruft tip\n\nPerhaps we intended to drop \"cruft tips\" in the test comments too? e.g.\nhere and..\n\n> +test_expect_success 'multi-valued gc.recentObjectsHook' '\n> +\tgit init repo &&\n> +\ttest_when_finished \"rm -fr repo\" &&\n> +\t(\n> +\t\tcd repo &&\n> +\n> +\t\ttest_commit base &&\n> +\t\tgit branch -M main &&\n> +\n> +\t\tgit checkout --orphan cruft.a &&\n> +\t\tgit rm -fr . &&\n> +\t\ttest_commit --no-tag cruft.a &&\n> +\t\tcruft_a=\"$(git rev-parse HEAD)\" &&\n> +\n> +\t\tgit checkout --orphan cruft.b &&\n> +\t\tgit rm -fr . &&\n> +\t\ttest_commit --no-tag cruft.b &&\n> +\t\tcruft_b=\"$(git rev-parse HEAD)\" &&\n> +\n> +\t\tgit checkout main &&\n> +\t\tgit branch -D cruft.a cruft.b &&\n> +\t\tgit reflog expire --all --expire=all &&\n> +\n> +\t\techo \"echo $cruft_a\" | write_script extra-tips.a &&\n> +\t\techo \"echo $cruft_b\" | write_script extra-tips.b &&\n> +\t\techo \"false\" | write_script extra-tips.c &&\n> +\n> +\t\tgit rev-list --objects --no-object-names $cruft_a $cruft_b \\\n> +\t\t\t>cruft.raw &&\n> +\t\tsort cruft.raw >cruft.expect &&\n> +\n> +\t\t# ensure that each extra cruft tip is saved by its\n\nhere.\n\nAside from those trivial points, everything looks good.\n"},{"id":"478304","messageId":"xmqqv8fs8jsu.fsf@gitster.g","threadId":"59731","inReplyTo":"kl6lwn0cfbyh.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re: [PATCH v5 2/2] gc: introduce `gc.recentObjectsHook`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-06-12T21:14:41Z","receivedAt":"2023-06-12T21:21:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Glen Choo writes:\n\n> Taylor Blau writes:\n>\n>> When performing a garbage collection operation on a repository with\n>> unreachable objects, Git makes its decision on what to do with those\n>> object(s) bed on how recent the objects are or not. Generally speaking,\n>\n> s/bed/based/ ?\n\nIndeed. Also \"or not\" sounds a bit extraneous.\n\n>> +\t\tif (parse_oid_hex(buf.buf, &oid, &rest) || *rest) {\n>> +\t\t\tret = error(_(\"invalid extra cruft tip: '%s'\"), buf.buf);\n>\n> To be consistent with the other error message, perhaps s/extra cruft\n> tip/additional recent object/?\n\nSharp eyes.\n\n> Aside from those trivial points, everything looks good.\n\nThanks for reviewing (and writing).\n"}]}