{"thread":{"id":"59624","subject":"[PATCH] builtin/pack-objects.c: introduce `pack.extraCruftTips`","startedAt":"2023-04-20T17:27:18Z","lastAt":"2023-05-06T02:13:01Z","messageCount":31,"participants":["Taylor Blau","Junio C Hamano","Chris Torek","Derrick Stolee","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"475748","messageId":"8af478ebe34539b68ffb9b353bbb1372dfca3871.1682011600.git.me@ttaylorr.com","threadId":"59624","inReplyTo":null,"subject":"[PATCH] builtin/pack-objects.c: introduce `pack.extraCruftTips`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-20T17:27:06Z","receivedAt":"2023-04-20T17:27:18Z","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.extraCruftTips` as an alternative means to mark certain objects in\nthe cruft pack as rescued, even if they would otherwise be pruned.\n\nTraditionally, cruft packs are generated in one of two ways:\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\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\nThis works well for repositories which have all of their \"interesting\"\nobjects worth keeping around reachable via at least one reference.\n\nHowever, there is 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  - to extend the grace period, which may keep around other objects that\n    the caller *does* want to discard,\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.extraCruftTips` which\nallows the caller to specify a program (or set of programs) whose output\nis treated as a set of objects to treat as \"kept\", even if they are\nunreachable and have aged out of the retention period.\n\nThe implementation is straightforward: after determining the set of\nobjects to pack into the cruft pack (either by calling\n`enumerate_cruft_objects()` or `enumerate_and_traverse_cruft_objects()`)\nwe figure out if there are any program(s) we need to consult in order to\ndetermine if there are any objects which we need to \"rescue\". We then\nadd those as tips to another reachability traversal, marking every\nobject along the way as cruft (and thus retaining it in the cruft pack).\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-tips` that does the\nsame thing as above, adding the visited set of objects along the\ntraversal 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 tips\" programs is not closed, then the\nresulting pack won't be either. This makes it impossible in the general\ncase to write out reachability bitmaps for that pack, since closure is a\nrequirement there.\n\nInstead, keep these unreachable objects in the cruft pack instead, to\nensure that we can continue to have a pack containing just reachable\nobjects, which is always safe to write a bitmap over.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n Documentation/config/pack.txt |  9 ++++\n builtin/pack-objects.c        | 95 +++++++++++++++++++++++++++++++++\n t/t5329-pack-objects-cruft.sh | 98 +++++++++++++++++++++++++++++++++++\n 3 files changed, 202 insertions(+)\n\ndiff --git a/Documentation/config/pack.txt b/Documentation/config/pack.txt\nindex 53093d9996..ab6aadd4b0 100644\n--- a/Documentation/config/pack.txt\n+++ b/Documentation/config/pack.txt\n@@ -67,6 +67,15 @@ 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++\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+\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 77d88f85b0..3b575c79dc 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -43,6 +43,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.\n@@ -3586,6 +3587,98 @@ static void enumerate_and_traverse_cruft_objects(struct string_list *fresh_packs\n \tstop_progress(&progress_state);\n }\n \n+static int enumerate_extra_cruft_tips_1(struct rev_info *revs, const char *args)\n+{\n+\tstruct child_process cmd = CHILD_PROCESS_INIT;\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tFILE *out = NULL;\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+\n+\tout = xfdopen(cmd.out, \"r\");\n+\twhile (strbuf_getline_lf(&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+\t\t\tret = error(_(\"invalid extra cruft tip: '%s'\"), buf.buf);\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}\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 int enumerate_extra_cruft_tips(void)\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+\n+\trepo_init_revisions(the_repository, &revs, NULL);\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\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+\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 {\n \tstruct strbuf buf = STRBUF_INIT;\n@@ -3641,6 +3734,8 @@ static void read_cruft_objects(void)\n \telse\n \t\tenumerate_cruft_objects();\n \n+\tenumerate_extra_cruft_tips();\n+\n \tstrbuf_release(&buf);\n \tstring_list_clear(&discard_packs, 0);\n \tstring_list_clear(&fresh_packs, 0);\ndiff --git a/t/t5329-pack-objects-cruft.sh b/t/t5329-pack-objects-cruft.sh\nindex 303f7a5d84..1260e11d41 100755\n--- a/t/t5329-pack-objects-cruft.sh\n+++ b/t/t5329-pack-objects-cruft.sh\n@@ -739,4 +739,102 @@ 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 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.extraCruftTips ./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+\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.extraCruftTips ./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\n-- \n2.40.0.353.g8af478ebe3\n"},{"id":"475752","messageId":"xmqqzg72zalp.fsf@gitster.g","threadId":"59624","inReplyTo":"8af478ebe34539b68ffb9b353bbb1372dfca3871.1682011600.git.me@ttaylorr.com","subject":"Re: [PATCH] builtin/pack-objects.c: introduce `pack.extraCruftTips`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-20T18:12:34Z","receivedAt":"2023-04-20T18:12:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau <me@ttaylorr.com> writes:\n\n> 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\nI can see why this would make the implementation simpler, simply\nbecause no existing codepaths that determines if an object is still\nlive knows about such a program hence all the codepaths by default\nkeep considering them expungeable crufts except for a very limited\nset of codepaths that will pay attention to the program.  But it\nmakes me wonder if it would make the life of end-users simpler if we\nreserve a special ref hierarchy, say \"refs/crufts/*\", than having to\nwrite a program for doing something like this.\n\nThanks.\n"},{"id":"475756","messageId":"ZEGSvfEkE5PhokKv@nand.local","threadId":"59624","inReplyTo":"xmqqzg72zalp.fsf@gitster.g","subject":"Re: [PATCH] builtin/pack-objects.c: introduce `pack.extraCruftTips`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-20T19:30:05Z","receivedAt":"2023-04-20T19:30:11Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Apr 20, 2023 at 11:12:34AM -0700, Junio C Hamano wrote:\n> Taylor Blau <me@ttaylorr.com> writes:\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>\n> I can see why this would make the implementation simpler, simply\n> because no existing codepaths that determines if an object is still\n> live knows about such a program hence all the codepaths by default\n> keep considering them expungeable crufts except for a very limited\n> set of codepaths that will pay attention to the program.\n\nRight, this is about band-aiding a situation where there are objects\nthat we want to save for whatever reason but which do not (need to) be\nreachable via any reference(s).\n\n> But it makes me wonder if it would make the life of end-users simpler\n> if we reserve a special ref hierarchy, say \"refs/crufts/*\", than\n> having to write a program for doing something like this.\n\nIdeally, yes. But I think there are certain instances where there are\nfar too many (disconnected) objects that creating a reference for each\npart of the unreachable object graph that we want to keep is infeasible.\n\nAnother way to think about pack.extraCruftTips is that the program\ninvocation is acting like the refs/crufts hierarchy would if it existed,\nbut without actually having to write all of those references down.\n\nThanks,\nTaylor\n"},{"id":"475758","messageId":"xmqq1qkez5yg.fsf@gitster.g","threadId":"59624","inReplyTo":"ZEGSvfEkE5PhokKv@nand.local","subject":"Re: [PATCH] builtin/pack-objects.c: introduce `pack.extraCruftTips`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-20T19:52:55Z","receivedAt":"2023-04-20T19:53:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau <me@ttaylorr.com> writes:\n\n>> But it makes me wonder if it would make the life of end-users simpler\n>> if we reserve a special ref hierarchy, say \"refs/crufts/*\", than\n>> having to write a program for doing something like this.\n>\n> Ideally, yes. But I think there are certain instances where there are\n> far too many (disconnected) objects that creating a reference for each\n> part of the unreachable object graph that we want to keep is infeasible.\n>\n> Another way to think about pack.extraCruftTips is that the program\n> invocation is acting like the refs/crufts hierarchy would if it existed,\n> but without actually having to write all of those references down.\n\nI do not need another way, actually.\n\nIf it is infeasible to keep track of what we want to keep from\nexpiration because they are too numerous and disconnected, I have to\nwonder how a programatic way to enumerate them is feasible,\nespecially the enumeration is for doing something useful.  \n\nYou can write a program that enumerates all the loose objects and\nexisting object names that appear in .idx files and such a program\nwill cover \"everything is too disconnected\" case, but that does not\ncount as a useful use case---disabling cruft expiration would\nachieve the same thing.  Is there a less hand-wavy use case you have\nin mind?\n\nTHanks.\n\n"},{"id":"475761","messageId":"ZEGlJx4ibYSp6qmD@nand.local","threadId":"59624","inReplyTo":"xmqq1qkez5yg.fsf@gitster.g","subject":"Re: [PATCH] builtin/pack-objects.c: introduce `pack.extraCruftTips`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-20T20:48:39Z","receivedAt":"2023-04-20T20:48:50Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Apr 20, 2023 at 12:52:55PM -0700, Junio C Hamano wrote:\n> Taylor Blau <me@ttaylorr.com> writes:\n>\n> >> But it makes me wonder if it would make the life of end-users simpler\n> >> if we reserve a special ref hierarchy, say \"refs/crufts/*\", than\n> >> having to write a program for doing something like this.\n> >\n> > Ideally, yes. But I think there are certain instances where there are\n> > far too many (disconnected) objects that creating a reference for each\n> > part of the unreachable object graph that we want to keep is infeasible.\n> >\n> > Another way to think about pack.extraCruftTips is that the program\n> > invocation is acting like the refs/crufts hierarchy would if it existed,\n> > but without actually having to write all of those references down.\n>\n> [...] Is there a less hand-wavy use case you have in mind?\n\nSure. The use-case I have in mind directly is keeping certain entries\nfrom GitHub's `audit_log` file (see a description from Peff in [1])\nwhile excluding others.\n\nWe use the audit_log to track every single reference change, like the\nreflog but with the reference name prepended to each entry and some\noptional metadata attached to the end of each entry.\n\nOur goal is to be able to prune the test-merge objects that GitHub\ncreates without (usually) pruning any objects that was pushed by a user.\nE.g., even if a user force-pushes their branch from A to B (where B is\nnot strictly ahead of A) we want to keep the objects from A around, even\nthough a reference is no longer pointing at it.\n\nThis already works with reflogs, which are considered as reachable\nobjects when generating a cruft pack (that is, they go in the \"big\"\npack with the rest of the reachable objects. This is extending that\nmechanism to work with GitHub's custom format, but doing so in a way\nthat is not tied to that format whatsoever.\n\nThe hope is that others may find it useful in other special\ncircumstances like above.\n\nThanks,\nTaylor\n\n[1]: https://lore.kernel.org/git/20150624094919.GC5436@peff.net/\n"},{"id":"475772","messageId":"CAPx1GvdmzDVbiZGeguVVjWe+eAhj6=yG18Rd6wLhZVrnd4jiBg@mail.gmail.com","threadId":"59624","inReplyTo":"8af478ebe34539b68ffb9b353bbb1372dfca3871.1682011600.git.me@ttaylorr.com","subject":"Re: [PATCH] builtin/pack-objects.c: introduce `pack.extraCruftTips`","fromName":"Chris Torek","fromEmail":"chris.torek@gmail.com","sentAt":"2023-04-21T00:10:31Z","receivedAt":"2023-04-21T00:10:48Z","isPatch":true,"sender":{"key":"chris.torek@gmail.com","avatar":"https://avatars.githubusercontent.com/u/16826774?v=4"},"body":"On Thu, Apr 20, 2023 at 10:35 AM Taylor Blau <me@ttaylorr.com> wrote:\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\nThe first part sounds good, as a rigid output format can always be\nrelaxed later if necessary. The second I'm less sure about: should\nthere perhaps be a --{no-,}missing-objects option to control it?\n\nIn particular I'd be a bit concerned if I always wanted to keep object\n$H and ran a command that is supposed to keep $H, but it was\nsilently dropped (accidentally or not) and is then gone.\n\nOne could always run `gitt rev-parse` or `git cat-file -t` on the hash\nafterward to double check, I suppose...\n\nChris\n"},{"id":"475776","messageId":"ZEHxctCveqt06g4a@nand.local","threadId":"59624","inReplyTo":"CAPx1GvdmzDVbiZGeguVVjWe+eAhj6=yG18Rd6wLhZVrnd4jiBg@mail.gmail.com","subject":"Re: [PATCH] builtin/pack-objects.c: introduce `pack.extraCruftTips`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-21T02:14:10Z","receivedAt":"2023-04-21T02:14:20Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Apr 20, 2023 at 05:10:31PM -0700, Chris Torek wrote:\n> On Thu, Apr 20, 2023 at 10:35 AM Taylor Blau <me@ttaylorr.com> wrote:\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>\n> The first part sounds good, as a rigid output format can always be\n> relaxed later if necessary. The second I'm less sure about: should\n> there perhaps be a --{no-,}missing-objects option to control it?\n\nMaybe, although specifying it might be a little tricky.\n\nI think you'd have to tie that setting into the configuration instead of\npassing it through as a command-line argument. So things get interesting\nwhen you have multiple `pack.extraCruftTips` commands: which of them are\nallowed (or not allowed) to specify missing objects?\n\nI think you could do something like:\n\n    [packExtraCruft \"foo\"]\n        exec = /path/to/script\n        allowMissing = true\n\nand specify multiple pack.extraCruftTips-esque programs, that each allow\n(or not) passing missing objects.\n\nI dunno. It might be overkill, but I could also see a compelling\nargument towards doing something like that. It may equally be OK to set\n`pack.allowMissingExtraCruftTips` once and have it apply to all values\nof `pack.extraCruftTips`.\n\nThat might be a reasonable compromise, and it doesn't feel like overkill\nto me. I can't easily imagine a circumstance where you'd want to allow\nsome programs to specify missing objects and not others.\n\nThanks,\nTaylor\n"},{"id":"476066","messageId":"4ba5eec2-dfbb-b918-147a-d6e03c2748b2@github.com","threadId":"59624","inReplyTo":"8af478ebe34539b68ffb9b353bbb1372dfca3871.1682011600.git.me@ttaylorr.com","subject":"Re: [PATCH] builtin/pack-objects.c: introduce `pack.extraCruftTips`","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2023-04-25T19:42:51Z","receivedAt":"2023-04-25T19:43:01Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 4/20/2023 1:27 PM, Taylor Blau wrote:\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\n> However, there is 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>   - to extend the grace period, which may keep around other objects that\n>     the caller *does* want to discard,\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\nOne way to think about this is that we want to store a second set of\nreferences to objects we do not want to prune, but those references\nare not useful for _any other Git operation_, so polluting the ref\nspace would be too expensive. And reflogs are insufficient because\ndeleted references also delete their reflogs.\n\nThese \"forever\" references can be kept in a completely different format\nif we use a hook interface. This helps us in particular because we\nalready have such a format on our systems. This may be a common\nscenario in other systems, too.\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\nI initially wondered why we would need a second walk, and I _think_\none reason is that we don't want to update the mtimes for these\nobjects as if they were reachable. The next point about the reachable\npack is also critical.\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> \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\nThis caught me off guard at first, but the important thing is that\nGit allows objects to be missing if they are only reachable from\nobjects that are not reachable from references. This \"the repository\nis not necessarily closed under reachability when including\nunreachable objects\" is always a confusing state to keep in mind.\n\nThere's another reason to keep them in the cruft pack, even if they\nwere closed under reachability: their presence muddies the reachable\npack(s) so their deltas are worse and their bitmaps a less efficient.\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\nMakes sense. Also: don't update their mtime so they could be removed\nimmediately if the external pointers change.\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> ++\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> +\n\nThis only indicates it is multi-valued by the word \"command(s)\". It\nalso doesn't specify that we execute them all or stop when one\ncompletes with success. (see code)\n\n> +static int enumerate_extra_cruft_tips_1(struct rev_info *revs, const char *args)\n> +{\n> +\tstruct child_process cmd = CHILD_PROCESS_INIT;\n> +\tstruct strbuf buf = STRBUF_INIT;\n> +\tFILE *out = NULL;\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\nI'm surprised this is necessary to include here. However, I see\none other use like this is for core.alternateRefsCommand, which\nserves a similar purpose.\n\n> +\n> +\tif (start_command(&cmd)) {\n> +\t\tret = -1;\n> +\t\tgoto done;\n> +\t}\n> +\n> +\tout = xfdopen(cmd.out, \"r\");\n> +\twhile (strbuf_getline_lf(&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> +\t\t\tret = error(_(\"invalid extra cruft tip: '%s'\"), buf.buf);\n> +\t\t\tgoto done;\n> +\t\t}\n\nReporting an error on malformed output from hook...\n\n> +\t\tobj = parse_object(the_repository, &oid);\n> +\t\tif (!obj)\n> +\t\t\tcontinue;\n\n...but skipping over missing objects. Good.\n\n> +\n> +\t\tdisplay_progress(progress_state, ++nr_seen);\n> +\t\tadd_pending_object(revs, obj, \"\");\n> +\t}\n> +\n> +done:\n> +\tif (out)\n> +\t\tfclose(out);\n> +\tstrbuf_release(&buf);\n> +\tchild_process_clear(&cmd);\n\nThis cleanup looks sufficient from my reading.\n\n> +\treturn ret;\n> +}\n> +\n> +static int enumerate_extra_cruft_tips(void)\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> +\n> +\trepo_init_revisions(the_repository, &revs, NULL);\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\nShould we be including two progress counts? or would it be sufficient\nto say \"traversing extra cruft objects\" and include both of these steps\nin that one progress mode?\n\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\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\nIf we are going to die() here, we might as well do it in the loop\ninstead of the break. But wait... this seems backwards.\n\nI suppose what this is saying is: try each possible hook until we\nget a success (in which case we stop trying). If we have a failure\nin all attempts, then we die().\n\nI was expecting this to be a cumulative operation: we execute each\nhook in order and include all of their output. This implementation\ndoesn't seem to match that (or I'm reading something wrong).\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> +\ttraverse_commit_list(&revs, show_cruft_commit, show_cruft_object, NULL);\n\nHm. I'm trying to look at these helpers and figure out what they\nare doing to prevent these objects from being pruned. This could\nuse a comment or something, as I'm unable to grok it at present.\n\n> +\tstop_progress(&progress_state);\n> +\n> +\treturn ret;\n> +}\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\nCould we store this commit to make sure it is removed from the\nrepository later?\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 old new &&\n\nDo you need to delete the 'discard' branch here?\n\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\nOk, here you list the exact object set you are expecting post-prune.\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.extraCruftTips ./extra-tips &&\n> +\n> +\t\tgit repack --cruft --cruft-expiration=now -d &&\n\nSo, at this point, it seems we are not including the 'discard'\nobjects in the cruft pack because they are still reachable. Or,\nam I reading that incorrectly?\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\nOne thing that would be good, as a safety check, is to remove\nthe pack.extraCruftTips config and re-run the repack and be\nsure that we are pruning the objects previously saved by the\nhook.\n\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.extraCruftTips ./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\nIt would be helpful to include a test for the multi-value case where\nmultiple scripts are executed and maybe include an \"exit 1\" in some\nof them?\n\nThanks,\n-Stolee\n"},{"id":"476073","messageId":"ZEhFScCoAgGGM/th@nand.local","threadId":"59624","inReplyTo":"4ba5eec2-dfbb-b918-147a-d6e03c2748b2@github.com","subject":"Re: [PATCH] builtin/pack-objects.c: introduce `pack.extraCruftTips`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-25T21:25:29Z","receivedAt":"2023-04-25T21:25:36Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Apr 25, 2023 at 03:42:51PM -0400, Derrick Stolee wrote:\n> On 4/20/2023 1:27 PM, Taylor Blau wrote:\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>\n> > However, there is 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> >   - to extend the grace period, which may keep around other objects that\n> >     the caller *does* want to discard,\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>\n> One way to think about this is that we want to store a second set of\n> references to objects we do not want to prune, but those references\n> are not useful for _any other Git operation_, so polluting the ref\n> space would be too expensive. And reflogs are insufficient because\n> deleted references also delete their reflogs.\n\nWell put. I think that reflogs only stick around as long as their\nreferences do is the key point and is under-emphasized in my original patch.\n\nAt the risk of repeating what you already said: yes, we (a) want to keep\naround a second set of objects, but (b) don't want to point references\nat them, since they are useless to all other operations as well as\nexpensive to track, and (c) don't want to use reflogs since their\nlifetime is tied to the reference they're tracking.\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>\n> I initially wondered why we would need a second walk, and I _think_\n> one reason is that we don't want to update the mtimes for these\n> objects as if they were reachable. The next point about the reachable\n> pack is also critical.\n\nTheir mtimes might be updated during that second walk, but the key point\nis that we want to rescue any objects that the extra tips might reach\n(but aren't listed themselves in the output of one of the hooks).\n\nThe idea there is similar to the rescuing pass we do for pruning GC's,\nwhere we'll save any objects which would have been pruned, but are\nreachable from another object which won't yet be pruned.\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>\n> Makes sense. Also: don't update their mtime so they could be removed\n> immediately if the external pointers change.\n\nI don't think I'm quite following why we would want to leave their\nmtimes alone here. I think we'd want their mtimes to be accurate as of\nthe last time we updated the *.mtimes file so that if they (a) no longer\nappear in the output of one of the extra-cruft hooks, and (b) they have\nbeen written recently, that they would not be pruned immediately.\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> > ++\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> > +\n>\n> This only indicates it is multi-valued by the word \"command(s)\". It\n> also doesn't specify that we execute them all or stop when one\n> completes with success. (see code)\n\n> > +\treturn ret;\n> > +}\n> > +\n> > +static int enumerate_extra_cruft_tips(void)\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> > +\n> > +\trepo_init_revisions(the_repository, &revs, NULL);\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>\n> Should we be including two progress counts? or would it be sufficient\n> to say \"traversing extra cruft objects\" and include both of these steps\n> in that one progress mode?\n\nI may be missing something, but there are already two progress meters\nhere: one (above) for enumerating the list of extra cruft tips, which\nincrements each time we read a new line that refers to an extant object,\nand another (below) for the number of objects that we visit along the\nsecond walk.\n\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\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> If we are going to die() here, we might as well do it in the loop\n> instead of the break. But wait... this seems backwards.\n>\n> I suppose what this is saying is: try each possible hook until we\n> get a success (in which case we stop trying). If we have a failure\n> in all attempts, then we die().\n>\n> I was expecting this to be a cumulative operation: we execute each\n> hook in order and include all of their output. This implementation\n> doesn't seem to match that (or I'm reading something wrong).\n\nYeah, this is definitely a bug, and the if-statement should read \"if\n(ret)\" not \"if (!ret)\". We should ideally consult all of the hooks here,\nbut if any one of them fails, we should stop and refuse to generate a\ncruft pack.\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> > +\ttraverse_commit_list(&revs, show_cruft_commit, show_cruft_object, NULL);\n>\n> Hm. I'm trying to look at these helpers and figure out what they\n> are doing to prevent these objects from being pruned. This could\n> use a comment or something, as I'm unable to grok it at present.\n\nVery fair, the show_cruft_object() callback calls\nadd_cruft_object_entry(), which adds the visited object to the cruft\npack regardless of its mtime with respect to pruning.\n\nA comment here might look like:\n\n    /* retain all objects reachable from extra tips via show_cruft_object() */\n\nor something.\n\n> Could we store this commit to make sure it is removed from the\n> repository later?\n\nPossibly, though I think we already have good coverage of this in other\ntests. So I'd be equally happy to assert on the exact contents of the\ncruft pack (which wouldn't include the above commit), but I'm happy to\nassert on it more directly if you feel strongly.\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 old new &&\n>\n> Do you need to delete the 'discard' branch here?\n\nGreat catch. I didn't notice it here because even though it was\nreachable (and not mentioned as an extra tip), so didn't appear in the\ncruft pack.\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.extraCruftTips ./extra-tips &&\n> > +\n> > +\t\tgit repack --cruft --cruft-expiration=now -d &&\n>\n> So, at this point, it seems we are not including the 'discard'\n> objects in the cruft pack because they are still reachable. Or,\n> am I reading that incorrectly?\n\nMy mistake, see above.\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> One thing that would be good, as a safety check, is to remove\n> the pack.extraCruftTips config and re-run the repack and be\n> sure that we are pruning the objects previously saved by the\n> hook.\n\nGood call. I think this amounts to:\n\n--- 8< ---\ndiff --git a/t/t5329-pack-objects-cruft.sh b/t/t5329-pack-objects-cruft.sh\nindex 1260e11d41..44852e6925 100755\n--- a/t/t5329-pack-objects-cruft.sh\n+++ b/t/t5329-pack-objects-cruft.sh\n@@ -773,7 +773,7 @@ test_expect_success 'additional cruft tips may be specified via pack.extraCruftT\n \t\tcruft_new=\"$(git rev-parse HEAD)\" &&\n\n \t\tgit checkout main &&\n-\t\tgit branch -D old new &&\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@@ -802,6 +802,19 @@ test_expect_success 'additional cruft tips may be specified via pack.extraCruftT\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.extraCruftTips &&\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\tsort cruft.raw >cruft.expect &&\n \t\ttest_cmp cruft.expect cruft.actual\n \t)\n '\n--- >8 ---\n\n> It would be helpful to include a test for the multi-value case where\n> multiple scripts are executed and maybe include an \"exit 1\" in some\n> of them?\n\nDefinitely, that is a great suggestion (especially because it would have\ncaught the \"if (!ret)\" bug that you pointed out above).\n\nThanks for a thorough review. I'll wait for other feedback before\nre-rolling, or do so in a couple of days if I haven't heard anything.\n\nThanks,\nTaylor\n"},{"id":"476096","messageId":"97d84abc-9b25-9d43-503e-264b33c223d4@github.com","threadId":"59624","inReplyTo":"ZEhFScCoAgGGM/th@nand.local","subject":"Re: [PATCH] builtin/pack-objects.c: introduce `pack.extraCruftTips`","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2023-04-26T10:52:26Z","receivedAt":"2023-04-26T10:53:46Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 4/25/2023 5:25 PM, Taylor Blau wrote:\n> On Tue, Apr 25, 2023 at 03:42:51PM -0400, Derrick Stolee wrote:\n>> On 4/20/2023 1:27 PM, Taylor Blau wrote:\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>>\n>> I initially wondered why we would need a second walk, and I _think_\n>> one reason is that we don't want to update the mtimes for these\n>> objects as if they were reachable. The next point about the reachable\n>> pack is also critical.\n> \n> Their mtimes might be updated during that second walk, but the key point\n> is that we want to rescue any objects that the extra tips might reach\n> (but aren't listed themselves in the output of one of the hooks).\n> \n> The idea there is similar to the rescuing pass we do for pruning GC's,\n> where we'll save any objects which would have been pruned, but are\n> reachable from another object which won't yet be pruned.\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>>\n>> Makes sense. Also: don't update their mtime so they could be removed\n>> immediately if the external pointers change.\n> \n> I don't think I'm quite following why we would want to leave their\n> mtimes alone here. I think we'd want their mtimes to be accurate as of\n> the last time we updated the *.mtimes file so that if they (a) no longer\n> appear in the output of one of the extra-cruft hooks, and (b) they have\n> been written recently, that they would not be pruned immediately.\n\nYou corrected me that we _are_ updating mtimes, so my understanding was\nincorrect and this follow-up is incorrect. Thanks.\n\nIs it possible to demonstrate this mtime-updating in a test?\n\n>>> +\tif (progress)\n>>> +\t\tprogress_state = start_progress(_(\"Enumerating extra cruft tips\"), 0);\n>>\n>> Should we be including two progress counts? or would it be sufficient\n>> to say \"traversing extra cruft objects\" and include both of these steps\n>> in that one progress mode?\n> \n> I may be missing something, but there are already two progress meters\n> here: one (above) for enumerating the list of extra cruft tips, which\n> increments each time we read a new line that refers to an extant object,\n> and another (below) for the number of objects that we visit along the\n> second walk.\n\nI meant: why two counts? Should we instead only have one?\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>>> +\ttraverse_commit_list(&revs, show_cruft_commit, show_cruft_object, NULL);\n>>\n>> Hm. I'm trying to look at these helpers and figure out what they\n>> are doing to prevent these objects from being pruned. This could\n>> use a comment or something, as I'm unable to grok it at present.\n> \n> Very fair, the show_cruft_object() callback calls\n> add_cruft_object_entry(), which adds the visited object to the cruft\n> pack regardless of its mtime with respect to pruning.\n\nAh, thanks. That helps a lot.\n \n> A comment here might look like:\n> \n>     /* retain all objects reachable from extra tips via show_cruft_object() */\n> \n> or something.\n\nSounds good.\n\n>> Could we store this commit to make sure it is removed from the\n>> repository later?\n> \n> Possibly, though I think we already have good coverage of this in other\n> tests. So I'd be equally happy to assert on the exact contents of the\n> cruft pack (which wouldn't include the above commit), but I'm happy to\n> assert on it more directly if you feel strongly.\n\nThis is a case of me thinking \"can we test the test?\" to make sure\nthe test is doing everything we think it is doing...\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 old new &&\n>>\n>> Do you need to delete the 'discard' branch here?\n> \n> Great catch. I didn't notice it here because even though it was\n> reachable (and not mentioned as an extra tip), so didn't appear in the\n> cruft pack.\n\n...so we catch things like this automatically.\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>> One thing that would be good, as a safety check, is to remove\n>> the pack.extraCruftTips config and re-run the repack and be\n>> sure that we are pruning the objects previously saved by the\n>> hook.\n> \n> Good call. I think this amounts to:\n> \n> --- 8< ---\n> diff --git a/t/t5329-pack-objects-cruft.sh b/t/t5329-pack-objects-cruft.sh\n> index 1260e11d41..44852e6925 100755\n> --- a/t/t5329-pack-objects-cruft.sh\n> +++ b/t/t5329-pack-objects-cruft.sh\n> @@ -773,7 +773,7 @@ test_expect_success 'additional cruft tips may be specified via pack.extraCruftT\n>  \t\tcruft_new=\"$(git rev-parse HEAD)\" &&\n> \n>  \t\tgit checkout main &&\n> -\t\tgit branch -D old new &&\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> @@ -802,6 +802,19 @@ test_expect_success 'additional cruft tips may be specified via pack.extraCruftT\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\nIf we store \"discard=$(git rev-parse discard)\" earlier, we can also\ncheck\n\n\t\ttest_must_fail git show $discard &&\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 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\nI like how $cruft_new is still kept because its mtime is still\n\"later than now\".\n\n> +\t\tsort cruft.raw >cruft.expect &&\n>  \t\ttest_cmp cruft.expect cruft.actual\n>  \t)\n>  '\n> --- >8 ---\n> \n>> It would be helpful to include a test for the multi-value case where\n>> multiple scripts are executed and maybe include an \"exit 1\" in some\n>> of them?\n> \n> Definitely, that is a great suggestion (especially because it would have\n> caught the \"if (!ret)\" bug that you pointed out above).\n> \n> Thanks for a thorough review. I'll wait for other feedback before\n> re-rolling, or do so in a couple of days if I haven't heard anything.\n\nThanks,\n-Stolee\n"},{"id":"476478","messageId":"ZFGlnB5foNSRFmmw@nand.local","threadId":"59624","inReplyTo":"97d84abc-9b25-9d43-503e-264b33c223d4@github.com","subject":"Re: [PATCH] builtin/pack-objects.c: introduce `pack.extraCruftTips`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-05-03T00:06:52Z","receivedAt":"2023-05-03T00:06:58Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Apr 26, 2023 at 06:52:26AM -0400, Derrick Stolee wrote:\n> You corrected me that we _are_ updating mtimes, so my understanding was\n> incorrect and this follow-up is incorrect. Thanks.\n>\n> Is it possible to demonstrate this mtime-updating in a test?\n\nIt is, though I think that this might be out of scope for the\n`pack.extraCruftTips` feature, and instead is more directly related to\nthe core machinery behind cruft packs. We have fairly thorough tests\nthroughout t5329 that cover this behavior implicitly, but let me know if\nyou think there is anything missing there.\n\n> >>> +\tif (progress)\n> >>> +\t\tprogress_state = start_progress(_(\"Enumerating extra cruft tips\"), 0);\n> >>\n> >> Should we be including two progress counts? or would it be sufficient\n> >> to say \"traversing extra cruft objects\" and include both of these steps\n> >> in that one progress mode?\n> >\n> > I may be missing something, but there are already two progress meters\n> > here: one (above) for enumerating the list of extra cruft tips, which\n> > increments each time we read a new line that refers to an extant object,\n> > and another (below) for the number of objects that we visit along the\n> > second walk.\n>\n> I meant: why two counts? Should we instead only have one?\n\nAh, thanks for helping my understanding. We have two counts to match the\nbehavior when generating cruft packs with pruning, where one counter\ntracks the number of tips we add to the traversal, and the second\ncounter tracks the number of objects that we actually visit along the\ntraversal (and thus end up in the cruft pack).\n\nSince we have to do an identical traversal here to ensure reachability\nclosure (where possible) over the extra cruft tips, I added a second\ncounter to track that progress.\n\n> >> Could we store this commit to make sure it is removed from the\n> >> repository later?\n> >\n> > Possibly, though I think we already have good coverage of this in other\n> > tests. So I'd be equally happy to assert on the exact contents of the\n> > cruft pack (which wouldn't include the above commit), but I'm happy to\n> > assert on it more directly if you feel strongly.\n>\n> This is a case of me thinking \"can we test the test?\" to make sure\n> the test is doing everything we think it is doing...\n\nGood call, and doing so isn't too bad, either:\n\n--- 8< ---\ndiff --git a/t/t5329-pack-objects-cruft.sh b/t/t5329-pack-objects-cruft.sh\nindex 0ac2f515b8..3ad16a9fca 100755\n--- a/t/t5329-pack-objects-cruft.sh\n+++ b/t/t5329-pack-objects-cruft.sh\n@@ -814,8 +814,16 @@ test_expect_success 'additional cruft tips may be specified via pack.extraCruftT\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+\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--- >8 ---\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 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>\n> I like how $cruft_new is still kept because its mtime is still\n> \"later than now\".\n\nThanks. I added this object to make sure that we weren't testing trivial\nbehavior, such as removing all objects regardless of whether or not they\nappear as extra cruft tips.\n\nThanks,\nTaylor\n"},{"id":"476480","messageId":"73ad7b90e1fe6c15f41ff828651f7ab06076ffd8.1683072587.git.me@ttaylorr.com","threadId":"59624","inReplyTo":"8af478ebe34539b68ffb9b353bbb1372dfca3871.1682011600.git.me@ttaylorr.com","subject":"[PATCH v2] builtin/pack-objects.c: introduce `pack.extraCruftTips`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-05-03T00:09:47Z","receivedAt":"2023-05-03T00:09:57Z","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.extraCruftTips` as an alternative means to mark certain objects in\nthe cruft pack as rescued, even if they would otherwise be pruned.\n\nTraditionally, cruft packs are generated in one of two ways:\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\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\nThis works well for repositories which have all of their \"interesting\"\nobjects worth keeping around reachable via at least one reference.\n\nHowever, there is 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  - 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  - to extend the grace period, which may keep around other objects that\n    the caller *does* want to discard,\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.extraCruftTips` which\nallows the caller to specify a program (or set of programs) whose output\nis treated as a set of objects to treat as \"kept\", even if they are\nunreachable and have aged out of the retention period.\n\nThe implementation is straightforward: after determining the set of\nobjects to pack into the cruft pack (either by calling\n`enumerate_cruft_objects()` or `enumerate_and_traverse_cruft_objects()`)\nwe figure out if there are any program(s) we need to consult in order to\ndetermine if there are any objects which we need to \"rescue\". We then\nadd those as tips to another reachability traversal, marking every\nobject along the way as cruft (and thus retaining it in the cruft pack).\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-tips` that does the\nsame thing as above, adding the visited set of objects along the\ntraversal 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 tips\" programs is not closed, then the\nresulting pack won't be either. This makes it impossible in the general\ncase to write out reachability bitmaps for that pack, since closure is a\nrequirement there.\n\nInstead, keep these unreachable objects in the cruft pack instead, to\nensure that we can continue to have a pack containing just reachable\nobjects, which is always safe to write a bitmap over.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\nRange-diff against v1:\n1:  8af478ebe3 ! 1:  73ad7b90e1 builtin/pack-objects.c: introduce `pack.extraCruftTips`\n    @@ Commit message\n     \n           - to point a reference at them, which may be undesirable or\n             infeasible,\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           - to extend the grace period, which may keep around other objects that\n             the caller *does* want to discard,\n           - or, to force the caller to construct the pack of objects they want\n    @@ Documentation/config/pack.txt: pack.deltaCacheLimit::\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     +\n      pack.threads::\n      \tSpecifies the number of threads to spawn when searching for best\n    @@ builtin/pack-objects.c: static void enumerate_and_traverse_cruft_objects(struct\n     +\t\tadd_pending_object(revs, obj, \"\");\n     +\t}\n     +\n    ++\tret = finish_command(&cmd);\n    ++\n     +done:\n     +\tif (out)\n     +\t\tfclose(out);\n    @@ builtin/pack-objects.c: static void enumerate_and_traverse_cruft_objects(struct\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\tif (!ret)\n    ++\t\tif (ret)\n     +\t\t\tbreak;\n     +\t}\n     +\tstop_progress(&progress_state);\n    @@ builtin/pack-objects.c: static void enumerate_and_traverse_cruft_objects(struct\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    @@ t/t5329-pack-objects-cruft.sh: test_expect_success 'cruft objects are freshend v\n     +\t\tcruft_new=\"$(git rev-parse HEAD)\" &&\n     +\n     +\t\tgit checkout main &&\n    -+\t\tgit branch -D old new &&\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/t5329-pack-objects-cruft.sh: test_expect_success 'cruft objects are freshend v\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    ++\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.extraCruftTips &&\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.extraCruftTips ./extra-tips.a &&\n    ++\t\tgit config --add pack.extraCruftTips ./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.extraCruftTips ./extra-tips.c &&\n    ++\t\ttest_must_fail git repack --cruft -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\n Documentation/config/pack.txt |  11 +++\n builtin/pack-objects.c        | 103 ++++++++++++++++++++\n t/t5329-pack-objects-cruft.sh | 171 ++++++++++++++++++++++++++++++++++\n 3 files changed, 285 insertions(+)\n\ndiff --git a/Documentation/config/pack.txt b/Documentation/config/pack.txt\nindex d4c7c9d4e4..0b79bd1751 100644\n--- a/Documentation/config/pack.txt\n+++ b/Documentation/config/pack.txt\n@@ -67,6 +67,17 @@ 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++\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+\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..79a29796fe 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.\n@@ -3587,6 +3588,106 @@ static void enumerate_and_traverse_cruft_objects(struct string_list *fresh_packs\n \tstop_progress(&progress_state);\n }\n \n+static int enumerate_extra_cruft_tips_1(struct rev_info *revs, const char *args)\n+{\n+\tstruct child_process cmd = CHILD_PROCESS_INIT;\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tFILE *out = NULL;\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+\n+\tout = xfdopen(cmd.out, \"r\");\n+\twhile (strbuf_getline_lf(&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+\t\t\tret = error(_(\"invalid extra cruft tip: '%s'\"), buf.buf);\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}\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 int enumerate_extra_cruft_tips(void)\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+\n+\trepo_init_revisions(the_repository, &revs, NULL);\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\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 {\n \tstruct strbuf buf = STRBUF_INIT;\n@@ -3642,6 +3743,8 @@ static void read_cruft_objects(void)\n \telse\n \t\tenumerate_cruft_objects();\n \n+\tenumerate_extra_cruft_tips();\n+\n \tstrbuf_release(&buf);\n \tstring_list_clear(&discard_packs, 0);\n \tstring_list_clear(&fresh_packs, 0);\ndiff --git a/t/t5329-pack-objects-cruft.sh b/t/t5329-pack-objects-cruft.sh\nindex 303f7a5d84..3ad16a9fca 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.extraCruftTips ./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.extraCruftTips &&\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.extraCruftTips ./extra-tips.a &&\n+\t\tgit config --add pack.extraCruftTips ./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.extraCruftTips ./extra-tips.c &&\n+\t\ttest_must_fail git repack --cruft -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.extraCruftTips ./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\n-- \n2.40.1.477.g73ad7b90e1\n"},{"id":"476510","messageId":"b29ff03e-5504-af04-72c6-f98abde1442f@github.com","threadId":"59624","inReplyTo":"73ad7b90e1fe6c15f41ff828651f7ab06076ffd8.1683072587.git.me@ttaylorr.com","subject":"Re: [PATCH v2] builtin/pack-objects.c: introduce `pack.extraCruftTips`","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2023-05-03T14:01:51Z","receivedAt":"2023-05-03T14:01:58Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 5/2/2023 8:09 PM, Taylor Blau wrote:\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\n> Range-diff against v1:\n> 1:  8af478ebe3 ! 1:  73ad7b90e1 builtin/pack-objects.c: introduce `pack.extraCruftTips`\n>     @@ Commit message\n>      \n>            - to point a reference at them, which may be undesirable or\n>              infeasible,\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>            - to extend the grace period, which may keep around other objects that\n>              the caller *does* want to discard,\n>            - or, to force the caller to construct the pack of objects they want\n>     @@ Documentation/config/pack.txt: pack.deltaCacheLimit::\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\nThanks for these updates.\n\n>       pack.threads::\n>       \tSpecifies the number of threads to spawn when searching for best\n>     @@ builtin/pack-objects.c: static void enumerate_and_traverse_cruft_objects(struct\n>      +\t\tadd_pending_object(revs, obj, \"\");\n>      +\t}\n>      +\n>     ++\tret = finish_command(&cmd);\n>     ++\n>      +done:\n>      +\tif (out)\n>      +\t\tfclose(out);\n>     @@ builtin/pack-objects.c: static void enumerate_and_traverse_cruft_objects(struct\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\tif (!ret)\n>     ++\t\tif (ret)\n>      +\t\t\tbreak;\n>      +\t}\n\nIf we have a failure, then we stop immediately and report a failure\n(from this bit outside the range-diff's context):\n\n> +\tif (ret)\n> +\t\tdie(_(\"unable to enumerate additional cruft tips\"));\n\n>     @@ t/t5329-pack-objects-cruft.sh: test_expect_success 'cruft objects are freshend v\n>      +\t\tcruft_new=\"$(git rev-parse HEAD)\" &&\n>      +\n>      +\t\tgit checkout main &&\n>     -+\t\tgit branch -D old new &&\n>     ++\t\tgit branch -D discard old new &&\n\nGood update.\n\n>      +\t\tgit reflog expire --all --expire=all &&\n>      +\n>      +\t\t# mark cruft.old with an mtime that is many minutes\n>     @@ t/t5329-pack-objects-cruft.sh: test_expect_success 'cruft objects are freshend v\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>     ++\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.extraCruftTips &&\n>     ++\t\tgit repack --cruft --cruft-expiration=now -d &&\n\nDouble-checking that removing the tips correctly removes the objects\nat this time is good, but...\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\n...it would be good to do this check before the removal of the hook\nto be sure the objects from 'discard' are removed as part of the step\nwith the hook. I can imagine a world where the hook-based approach\nerroneously keeps the 'discard' objects in the non-cruft pack only\nfor them to be deleted by the repack where hooks are disabled, and\nhaving a test would help guarantee we don't live in that hypothetical\nworld.\n\nI would also be satisfied with checking just the commits that were\npreviously referenced by 'discard' and 'old', but this more\nthorough check is also good.\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.extraCruftTips ./extra-tips.a &&\n>     ++\t\tgit config --add pack.extraCruftTips ./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.extraCruftTips ./extra-tips.c &&\n>     ++\t\ttest_must_fail git repack --cruft -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\nThis new test looks good to me.\n\nThanks,\n-Stolee\n"},{"id":"476557","messageId":"20230503195906.GA1796816@coredump.intra.peff.net","threadId":"59624","inReplyTo":"73ad7b90e1fe6c15f41ff828651f7ab06076ffd8.1683072587.git.me@ttaylorr.com","subject":"Re: [PATCH v2] builtin/pack-objects.c: introduce `pack.extraCruftTips`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-05-03T19:59:06Z","receivedAt":"2023-05-03T19:59:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 02, 2023 at 08:09:47PM -0400, Taylor Blau wrote:\n\n> However, there is 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>   - 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>   - to extend the grace period, which may keep around other objects that\n>     the caller *does* want to discard,\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\nOK, I understand the use case you're trying to support, and your\napproach mostly makes sense. But there are two things I was surprised by\nin the implementation:\n\n  1. Does this need to be tied to cruft packs? The same logic would\n     apply to \"repack -A\" which turns objects loose (of course you\n     probably don't want to do that in the long term for performance\n     reasons, but it's conceptually the same thing; see below).\n\n  2. Why is there a separate walk distinct from the one that rescues\n     recent-but-unreachable objects?\n\nConceptually it seems to me that the simplest and most flexible way to\nthink of this new feature is: pretend these objects are recent enough to\nbe kept in the grace period, even though their mtimes do not qualify\".\n\nAnd then everything else would just fall out naturally. Am I missing\nsomething?\n\nIn a pre-cruft-pack world, I'd have just expected the patch to look like\nthis:\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex bd6ad016d6..1d655dc758 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -4077,6 +4077,7 @@ static void get_object_list(struct rev_info *revs, int ac, const char **av)\n \t\tif (add_unseen_recent_objects_to_traversal(revs,\n \t\t\t\tunpack_unreachable_expiration, NULL, 0))\n \t\t\tdie(_(\"unable to add recent objects\"));\n+\t\tadd_tips_from_program(revs);\n \t\tif (prepare_revision_walk(revs))\n \t\t\tdie(_(\"revision walk setup failed\"));\n \t\ttraverse_commit_list(revs, record_recent_commit,\n\nSadly the cruft-pack feature doesn't really share this code, but I think\nit does something similar, and could just consider those synthetic tips\nas \"recent\" for this run.\n\n> +static int enumerate_extra_cruft_tips_1(struct rev_info *revs, const char *args)\n> +{\n> +\tstruct child_process cmd = CHILD_PROCESS_INIT;\n> +\tstruct strbuf buf = STRBUF_INIT;\n> +\tFILE *out = NULL;\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\nWhy does this clear the environment of local-repo variables? That seems\nunlike most other hooks we have, and would cause confusion if $GIT_DIR\nor various config variables are important (e.g., should \"git -c foo.bar\ngc\" persist foo.bar when running the hook? It usually does).\n\nI know that some hooks that try to change repositories by changing\ndirectories have the opposite confusion ($GIT_DIR is set, but they did\nnot want it). But it makes more sense to me to remain consistent with\nhow other hooks behave.\n\n> +\tif (start_command(&cmd)) {\n> +\t\tret = -1;\n> +\t\tgoto done;\n> +\t}\n\nThis may be a matter of taste, but you can \"return -1\" directly here, as\nnothing has been allocated (and a failed start_command() will call\nchild_process_clear() for you). This would mean \"out\" is always set in\nthe \"done:\" label, so it wouldn't need a NULL-check (nor an\ninitialization).\n\n> +\n> +\tout = xfdopen(cmd.out, \"r\");\n> +\twhile (strbuf_getline_lf(&buf, out) != EOF) {\n\nis there any reason to be a stickler for LF versus CRLF here? I.e.,\nwouldn't strbuf_getline() be more friendly, with little chance that we\nmisinterpret the result?\n\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> +\t\t\tret = error(_(\"invalid extra cruft tip: '%s'\"), buf.buf);\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\nThis parse_object() can be pretty expensive, especially if you are\nrescuing a lot of objects (or if your program directly references large\nblobs). Can we use oid_object_info() here in combination with\nlookup_object_by_type()?\n\n-Peff\n"},{"id":"476565","messageId":"ZFLQfMtIGcZHNERE@nand.local","threadId":"59624","inReplyTo":"20230503195906.GA1796816@coredump.intra.peff.net","subject":"Re: [PATCH v2] builtin/pack-objects.c: introduce `pack.extraCruftTips`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-05-03T21:22:04Z","receivedAt":"2023-05-03T21:22:17Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, May 03, 2023 at 03:59:06PM -0400, Jeff King wrote:\n> On Tue, May 02, 2023 at 08:09:47PM -0400, Taylor Blau wrote:\n>\n> > However, there is 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> >   - 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> >   - to extend the grace period, which may keep around other objects that\n> >     the caller *does* want to discard,\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> OK, I understand the use case you're trying to support, and your\n> approach mostly makes sense. But there are two things I was surprised by\n> in the implementation:\n>\n>   1. Does this need to be tied to cruft packs? The same logic would\n>      apply to \"repack -A\" which turns objects loose (of course you\n>      probably don't want to do that in the long term for performance\n>      reasons, but it's conceptually the same thing; see below).\n>\n>   2. Why is there a separate walk distinct from the one that rescues\n>      recent-but-unreachable objects?\n\n> Conceptually it seems to me that the simplest and most flexible way to\n> think of this new feature is: pretend these objects are recent enough to\n> be kept in the grace period, even though their mtimes do not qualify\".\n>\n> And then everything else would just fall out naturally. Am I missing\n> something?\n\nI originally shied away from it, thinking that I wouldn't want to do an\nexpensive walk with all of the recent objects during a non-pruning\nrepack.\n\nThe two code paths are quite different in practice. In the pruning case,\nwe add only new objects from the kept packs and then start our traversal\nthere. In the non-pruning case, we just do\n`add_objects_in_unpacked_packs()` which is really just a call to\n`for_each_packed_object()`.\n\nSo it gets tricky when you have a pack.extraCruftTips program and want\nto invoke it in a non-pruning case. You could do something like:\n\n  - call enumerate_and_traverse_cruft_objects() *always*, either because\n    we were doing a pruning GC, or calling it after\n    `enumerate_cruft_objects()` (in the non-pruning case)\n\n  - ensure that enumerate_and_traverse_cruft_objects() is a noop when\n    (a) cruft_expiration is set to zero, and (b) there are no\n    pack.extraCruftTips programs specified\n\n> > +static int enumerate_extra_cruft_tips_1(struct rev_info *revs, const char *args)\n> > +{\n> > +\tstruct child_process cmd = CHILD_PROCESS_INIT;\n> > +\tstruct strbuf buf = STRBUF_INIT;\n> > +\tFILE *out = NULL;\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> Why does this clear the environment of local-repo variables? That seems\n> unlike most other hooks we have, and would cause confusion if $GIT_DIR\n> or various config variables are important (e.g., should \"git -c foo.bar\n> gc\" persist foo.bar when running the hook? It usually does).\n>\n> I know that some hooks that try to change repositories by changing\n> directories have the opposite confusion ($GIT_DIR is set, but they did\n> not want it). But it makes more sense to me to remain consistent with\n> how other hooks behave.\n\nCopy-and-pasted from the core.alternateRefsCommand stuff, there is no\nneed for this here.\n\n> > +\tif (start_command(&cmd)) {\n> > +\t\tret = -1;\n> > +\t\tgoto done;\n> > +\t}\n>\n> This may be a matter of taste, but you can \"return -1\" directly here, as\n> nothing has been allocated (and a failed start_command() will call\n> child_process_clear() for you). This would mean \"out\" is always set in\n> the \"done:\" label, so it wouldn't need a NULL-check (nor an\n> initialization).\n\nVery nice, thanks for the suggestion.\n\n> > +\n> > +\tout = xfdopen(cmd.out, \"r\");\n> > +\twhile (strbuf_getline_lf(&buf, out) != EOF) {\n>\n> is there any reason to be a stickler for LF versus CRLF here? I.e.,\n> wouldn't strbuf_getline() be more friendly, with little chance that we\n> misinterpret the result?\n\nNo reason, other than I happened to type this one out first ;-). We\nshould definitely be more permissive here, I agree that strbuf_getline()\nis the friendlier choice.\n\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> > +\t\t\tret = error(_(\"invalid extra cruft tip: '%s'\"), buf.buf);\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> This parse_object() can be pretty expensive, especially if you are\n> rescuing a lot of objects (or if your program directly references large\n> blobs). Can we use oid_object_info() here in combination with\n> lookup_object_by_type()?\n\nYep, definitely. Nice catch ;-).\n\nThanks,\nTaylor\n"},{"id":"476567","messageId":"ZFLSDS89y8P2+g/2@nand.local","threadId":"59624","inReplyTo":"20230503195906.GA1796816@coredump.intra.peff.net","subject":"Re: [PATCH v2] builtin/pack-objects.c: introduce `pack.extraCruftTips`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-05-03T21:28:45Z","receivedAt":"2023-05-03T21:28:50Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, May 03, 2023 at 03:59:06PM -0400, Jeff King wrote:\n> On Tue, May 02, 2023 at 08:09:47PM -0400, Taylor Blau wrote:\n>\n> > However, there is 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> >   - 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> >   - to extend the grace period, which may keep around other objects that\n> >     the caller *does* want to discard,\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> OK, I understand the use case you're trying to support, and your\n> approach mostly makes sense. But there are two things I was surprised by\n> in the implementation:\n>\n>   1. Does this need to be tied to cruft packs? The same logic would\n>      apply to \"repack -A\" which turns objects loose (of course you\n>      probably don't want to do that in the long term for performance\n>      reasons, but it's conceptually the same thing; see below).\n\nI agree that you wouldn't want to do it for performance reasons, but I'm\ncomfortable with the asymmetry here, since this is `pack.extraCruftTips`\n(emphasis on \"cruft\"), so it's not clear that it has to be related to\n\"repack -A\".\n\nHappy to change things up if you feel strongly, though.\n\nThanks,\nTaylor\n"},{"id":"476576","messageId":"27a7f16aab35b5cac391d9831aadb0f2e2146313.1683151485.git.me@ttaylorr.com","threadId":"59624","inReplyTo":"8af478ebe34539b68ffb9b353bbb1372dfca3871.1682011600.git.me@ttaylorr.com","subject":"[PATCH v3] builtin/pack-objects.c: introduce `pack.extraCruftTips`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-05-03T22:05:45Z","receivedAt":"2023-05-03T22:05:54Z","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.extraCruftTips` as an alternative means to mark certain objects in\nthe cruft pack as rescued, even if they would otherwise be pruned.\n\nTraditionally, cruft packs are generated in one of two ways:\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\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\nThis works well for repositories which have all of their \"interesting\"\nobjects worth keeping around reachable via at least one reference.\n\nHowever, there is 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  - 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  - to extend the grace period, which may keep around other objects that\n    the caller *does* want to discard,\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.extraCruftTips` which\nallows the caller to specify a program (or set of programs) whose output\nis treated as a set of objects to treat as \"kept\", even if they are\nunreachable and have aged out of the retention period.\n\nThe implementation is straightforward, and pretends as if any objects\nlisted by one of the `pack.extraCruftTips` hooks is recent-enough to\nwarrant rescuing (even for non-pruning GCs).\n\nWe then add those as tips to another reachability traversal (along with\nany recent objects, if pruning), marking every object along the way as\ncruft (and thus retaining it in the cruft pack).\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-tips` that does the\nsame thing as above, adding the visited set of objects along the\ntraversal 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 tips\" programs is not closed, then the\nresulting pack won't be either. This makes it impossible in the general\ncase to write out reachability bitmaps for that pack, since closure is a\nrequirement there.\n\nInstead, keep these unreachable objects in the cruft pack instead, to\nensure that we can continue to have a pack containing just reachable\nobjects, which is always safe to write a bitmap over.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\nHere is a reworked version of the pack.extraCruftTips feature to reuse\nparts of the \"rescue old/unreachable, but reachable-from-recent\" traversal.\n\nIt is somewhat more complicated than the original, and I would\ndefinitely appreciate a close review from a second (or third) set of\neyes ;-).\n\nRange-diff against v2:\n1:  da1711b13b ! 1:  27a7f16aab builtin/pack-objects.c: introduce `pack.extraCruftTips`\n    @@ Commit message\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\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, and pretends as if any objects\n    +    listed by one of the `pack.extraCruftTips` hooks is recent-enough to\n    +    warrant rescuing (even for non-pruning GCs).\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 as\n    +    cruft (and thus retaining it in the cruft pack).\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    @@ 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    +@@ builtin/pack-objects.c: static void enumerate_cruft_objects(void)\n      \tstop_progress(&progress_state);\n      }\n\n    @@ builtin/pack-objects.c: static void enumerate_and_traverse_cruft_objects(struct\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\tint type;\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\ttype = oid_object_info(the_repository, &oid, NULL);\n    ++\t\tif (type < 0)\n    ++\t\t\tcontinue;\n    ++\n    ++\t\tobj = lookup_object_by_type(the_repository, &oid, type);\n     +\t\tif (!obj)\n     +\t\t\tcontinue;\n     +\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 add_extra_cruft_tips_to_traversal(struct rev_info *revs)\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    -+\n    -+\trepo_init_revisions(the_repository, &revs, NULL);\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    ++\t\treturn;\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\tret = enumerate_extra_cruft_tips_1(revs,\n     +\t\t\t\t\t\t   programs->items[i].string);\n     +\t\tif (ret)\n     +\t\t\tbreak;\n    @@ builtin/pack-objects.c: static void enumerate_and_traverse_cruft_objects(struct\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 void enumerate_and_traverse_cruft_objects(struct string_list *fresh_packs)\n      {\n    - \tstruct strbuf buf = STRBUF_INIT;\n    + \tstruct packed_git *p;\n    + \tstruct rev_info revs;\n    +-\tint ret;\n    ++\tint ret = 0;\n    +\n    + \trepo_init_revisions(the_repository, &revs, NULL);\n    +\n    +@@ builtin/pack-objects.c: static void enumerate_and_traverse_cruft_objects(struct string_list *fresh_packs\n    +\n    + \trevs.ignore_missing_links = 1;\n    +\n    +-\tif (progress)\n    +-\t\tprogress_state = start_progress(_(\"Enumerating cruft objects\"), 0);\n    +-\tret = add_unseen_recent_objects_to_traversal(&revs, cruft_expiration,\n    +-\t\t\t\t\t\t     set_cruft_mtime, 1);\n    +-\tstop_progress(&progress_state);\n    ++\tif (cruft_expiration) {\n    ++\t\tif (progress)\n    ++\t\t\tprogress_state = start_progress(_(\"Enumerating cruft objects\"), 0);\n    ++\t\tret = add_unseen_recent_objects_to_traversal(&revs, cruft_expiration,\n    ++\t\t\t\t\t\t\t     set_cruft_mtime, 1);\n    ++\t\tstop_progress(&progress_state);\n    ++\t}\n    ++\n    ++\tadd_extra_cruft_tips_to_traversal(&revs);\n    +\n    + \tif (ret)\n    + \t\tdie(_(\"unable to add cruft objects\"));\n     @@ builtin/pack-objects.c: static void read_cruft_objects(void)\n    - \telse\n    +\n    + \tif (cruft_expiration)\n    + \t\tenumerate_and_traverse_cruft_objects(&fresh_packs);\n    +-\telse\n    ++\telse {\n    ++\t\t/*\n    ++\t\t * call both enumerate_cruft_objects() and\n    ++\t\t * enumerate_and_traverse_cruft_objects(), since:\n    ++\t\t *\n    ++\t\t *   - the former is required to implement non-pruning GCs\n    ++\t\t *   - the latter is a noop for \"cruft_expiration == 0\", but\n    ++\t\t *     picks up any additional tips mentioned via\n    ++\t\t *     `pack.extraCruftTips`.\n    ++\t\t */\n      \t\tenumerate_cruft_objects();\n    ++\t\tenumerate_and_traverse_cruft_objects(&fresh_packs);\n    ++\t}\n\n    -+\tenumerate_extra_cruft_tips();\n    -+\n      \tstrbuf_release(&buf);\n      \tstring_list_clear(&discard_packs, 0);\n    - \tstring_list_clear(&fresh_packs, 0);\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\n Documentation/config/pack.txt |  11 +++\n builtin/pack-objects.c        | 104 +++++++++++++++++++--\n t/t5329-pack-objects-cruft.sh | 171 ++++++++++++++++++++++++++++++++++\n 3 files changed, 279 insertions(+), 7 deletions(-)\n\ndiff --git a/Documentation/config/pack.txt b/Documentation/config/pack.txt\nindex d4c7c9d4e4..0b79bd1751 100644\n--- a/Documentation/config/pack.txt\n+++ b/Documentation/config/pack.txt\n@@ -67,6 +67,17 @@ 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++\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+\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..6f6e7872cd 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.\n@@ -3543,11 +3544,85 @@ static void enumerate_cruft_objects(void)\n \tstop_progress(&progress_state);\n }\n\n+static int enumerate_extra_cruft_tips_1(struct rev_info *revs, 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\tstruct object *obj;\n+\t\tint type;\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\ttype = oid_object_info(the_repository, &oid, NULL);\n+\t\tif (type < 0)\n+\t\t\tcontinue;\n+\n+\t\tobj = lookup_object_by_type(the_repository, &oid, type);\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}\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 add_extra_cruft_tips_to_traversal(struct rev_info *revs)\n+{\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;\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\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+\n static void enumerate_and_traverse_cruft_objects(struct string_list *fresh_packs)\n {\n \tstruct packed_git *p;\n \tstruct rev_info revs;\n-\tint ret;\n+\tint ret = 0;\n\n \trepo_init_revisions(the_repository, &revs, NULL);\n\n@@ -3560,11 +3635,15 @@ static void enumerate_and_traverse_cruft_objects(struct string_list *fresh_packs\n\n \trevs.ignore_missing_links = 1;\n\n-\tif (progress)\n-\t\tprogress_state = start_progress(_(\"Enumerating cruft objects\"), 0);\n-\tret = add_unseen_recent_objects_to_traversal(&revs, cruft_expiration,\n-\t\t\t\t\t\t     set_cruft_mtime, 1);\n-\tstop_progress(&progress_state);\n+\tif (cruft_expiration) {\n+\t\tif (progress)\n+\t\t\tprogress_state = start_progress(_(\"Enumerating cruft objects\"), 0);\n+\t\tret = add_unseen_recent_objects_to_traversal(&revs, cruft_expiration,\n+\t\t\t\t\t\t\t     set_cruft_mtime, 1);\n+\t\tstop_progress(&progress_state);\n+\t}\n+\n+\tadd_extra_cruft_tips_to_traversal(&revs);\n\n \tif (ret)\n \t\tdie(_(\"unable to add cruft objects\"));\n@@ -3639,8 +3718,19 @@ static void read_cruft_objects(void)\n\n \tif (cruft_expiration)\n \t\tenumerate_and_traverse_cruft_objects(&fresh_packs);\n-\telse\n+\telse {\n+\t\t/*\n+\t\t * call both enumerate_cruft_objects() and\n+\t\t * enumerate_and_traverse_cruft_objects(), since:\n+\t\t *\n+\t\t *   - the former is required to implement non-pruning GCs\n+\t\t *   - the latter is a noop for \"cruft_expiration == 0\", but\n+\t\t *     picks up any additional tips mentioned via\n+\t\t *     `pack.extraCruftTips`.\n+\t\t */\n \t\tenumerate_cruft_objects();\n+\t\tenumerate_and_traverse_cruft_objects(&fresh_packs);\n+\t}\n\n \tstrbuf_release(&buf);\n \tstring_list_clear(&discard_packs, 0);\ndiff --git a/t/t5329-pack-objects-cruft.sh b/t/t5329-pack-objects-cruft.sh\nindex 303f7a5d84..3ad16a9fca 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.extraCruftTips ./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.extraCruftTips &&\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.extraCruftTips ./extra-tips.a &&\n+\t\tgit config --add pack.extraCruftTips ./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.extraCruftTips ./extra-tips.c &&\n+\t\ttest_must_fail git repack --cruft -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.extraCruftTips ./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\n--\n2.40.1.477.g73ad7b90e1.dirty\n"},{"id":"476579","messageId":"xmqqv8h9m2az.fsf@gitster.g","threadId":"59624","inReplyTo":"27a7f16aab35b5cac391d9831aadb0f2e2146313.1683151485.git.me@ttaylorr.com","subject":"Re: [PATCH v3] builtin/pack-objects.c: introduce `pack.extraCruftTips`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-05-03T23:18:44Z","receivedAt":"2023-05-03T23:18:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau <me@ttaylorr.com> writes:\n\n>   - When 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\n\"wil remain\" -> \"will remain\"\n\nBut more importantly, it is not clear to me whose \"stdin\" we are\ntalking about and what is sent \"over stdin\" here.  Somebody pipes\nthe output of \"printf %s --cruft\" to \"pack-objects\"???\n\n> However, there is 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>   - 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>   - to extend the grace period, which may keep around other objects that\n>     the caller *does* want to discard,\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\nIf you do not want to point objects you want to keep with a ref,\nthen you are making a problem for yourself, as gc decisions are\ndesigned around the notion that anything worthwhile to keep are\nsupposed to be reachable from some anchoring points (like the refs,\nthe reflogs, the index and its various extensions, etc.).\n\n> Here is a reworked version of the pack.extraCruftTips feature to reuse\n> parts of the \"rescue old/unreachable, but reachable-from-recent\" traversal.\n\nThat sounds like a sensible way to go, but as I said earlier, it\nstill is not clear, at least to me, why the pack.extraCruftTips\nprogram can compute them cheaply enough when you cannot have these\ntips pointed at with refs in some reserved namespaces of your own.\n\n> +pack.extraCruftTips::\n\nIs this consistent with the way we name a variable whose value\nspecifies a command to run?  At first, I thought this was a\nmulti-valued configuration variable, each of the value is an object\nname.  extraCruftTipsCmd?  extraCruftTipsHook?\n\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\nWhat is a \"tip of an object\"?  The first byte ;-)?\n\nA \"tip of history\" would only imply commit objects, but presumably\nyou would want to specify a tree and protect all the blobs and trees\nit recursively contains, so that is not a good name for it.\n\n> +\tage.\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\nAre objects that cannot be found the same as objects that are\nmissing?  How do we determine if a given object name refers to an\nobject which cannot be found?  Would the lazy fetching from promisor\nremotes come into play?\n\n> +Multiple hooks are supported, but all must exit successfully, else no\n> +cruft pack will be generated.\n> +\n\nNow, we are told this variable refers to \"hooks\".  If that is the\nname we want to call them, we'd better use the term from the\nbeginning.\n\n> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n> index a5b466839b..6f6e7872cd 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.\n> @@ -3543,11 +3544,85 @@ static void enumerate_cruft_objects(void)\n>  \tstop_progress(&progress_state);\n>  }\n>\n> +static int enumerate_extra_cruft_tips_1(struct rev_info *revs, const char *args)\n\nLet's lose the _1 suffix unless you have\nenumerate_extra_cruft_tips() that farms out bulk of its inner\nworkings to this one.\n\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\nIt somehow feels funny to be reading from \"out\", which is usually\nwhere we write things to.\n\n> +\twhile (strbuf_getline(&buf, out) != EOF) {\n\nDitto.\n\n> +\t\tstruct object_id oid;\n> +\t\tstruct object *obj;\n> +\t\tint type;\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\ttype = oid_object_info(the_repository, &oid, NULL);\n> +\t\tif (type < 0)\n> +\t\t\tcontinue;\n> +\n> +\t\tobj = lookup_object_by_type(the_repository, &oid, type);\n> +\t\tif (!obj)\n> +\t\t\tcontinue;\n\nHmph, we may want to have an interface that lets us avoid looking up\nthe same oid twice in the same set of tables.  Given an object\nunseen so far, oid_object_info() should have done most of the work\nnecessary for lookup_object_by_type() to get to and start parsing\nthe data of the object in the good case (i.e. object exists and in a\npack---just we haven't needed it yet), but in the above sequence\nthere is not enough information passed between two calls to take\nadvantage of it.\n\n> +\t\tdisplay_progress(progress_state, ++nr_seen);\n> +\t\tadd_pending_object(revs, obj, \"\");\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 add_extra_cruft_tips_to_traversal(struct rev_info *revs)\n> +{\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;\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\tif (ret)\n> +\t\t\tbreak;\n> +\t}\n> +\tstop_progress(&progress_state);\n\nWe iterate over the value list internal to the repo_config, but we\nare only borrowing them so there is no need to do any clean-up upon\nleaving.  OK.\n\n> +\tif (ret)\n> +\t\tdie(_(\"unable to enumerate additional cruft tips\"));\n> +}\n> +\n>  static void enumerate_and_traverse_cruft_objects(struct string_list *fresh_packs)\n>  {\n>  \tstruct packed_git *p;\n>  \tstruct rev_info revs;\n> -\tint ret;\n> +\tint ret = 0;\n>\n>  \trepo_init_revisions(the_repository, &revs, NULL);\n>\n> @@ -3560,11 +3635,15 @@ static void enumerate_and_traverse_cruft_objects(struct string_list *fresh_packs\n>\n>  \trevs.ignore_missing_links = 1;\n>\n> -\tif (progress)\n> -\t\tprogress_state = start_progress(_(\"Enumerating cruft objects\"), 0);\n> -\tret = add_unseen_recent_objects_to_traversal(&revs, cruft_expiration,\n> -\t\t\t\t\t\t     set_cruft_mtime, 1);\n> -\tstop_progress(&progress_state);\n> +\tif (cruft_expiration) {\n> +\t\tif (progress)\n> +\t\t\tprogress_state = start_progress(_(\"Enumerating cruft objects\"), 0);\n> +\t\tret = add_unseen_recent_objects_to_traversal(&revs, cruft_expiration,\n> +\t\t\t\t\t\t\t     set_cruft_mtime, 1);\n> +\t\tstop_progress(&progress_state);\n> +\t}\n> +\n> +\tadd_extra_cruft_tips_to_traversal(&revs);\n\nHmph, is this hunk doing two unrelated things at the same time?\nWhat was the reason why we didn't have to be conditional on\ncruft_expiration in the original code, and why does it start\nmattering when we teach the code to call out to hooks?\n\n> @@ -3639,8 +3718,19 @@ static void read_cruft_objects(void)\n>\n>  \tif (cruft_expiration)\n>  \t\tenumerate_and_traverse_cruft_objects(&fresh_packs);\n> -\telse\n> +\telse {\n> +\t\t/*\n> +\t\t * call both enumerate_cruft_objects() and\n> +\t\t * enumerate_and_traverse_cruft_objects(), since:\n> +\t\t *\n> +\t\t *   - the former is required to implement non-pruning GCs\n> +\t\t *   - the latter is a noop for \"cruft_expiration == 0\", but\n> +\t\t *     picks up any additional tips mentioned via\n> +\t\t *     `pack.extraCruftTips`.\n> +\t\t */\n>  \t\tenumerate_cruft_objects();\n> +\t\tenumerate_and_traverse_cruft_objects(&fresh_packs);\n> +\t}\n"},{"id":"476582","messageId":"xmqqjzxpm172.fsf@gitster.g","threadId":"59624","inReplyTo":"xmqqv8h9m2az.fsf@gitster.g","subject":"Re: [PATCH v3] builtin/pack-objects.c: introduce `pack.extraCruftTips`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-05-03T23:42:41Z","receivedAt":"2023-05-03T23:42:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n>> +Multiple hooks are supported, but all must exit successfully, else no\n>> +cruft pack will be generated.\n>> +\n>\n> Now, we are told this variable refers to \"hooks\".  If that is the\n> name we want to call them, we'd better use the term from the\n> beginning.\n\nI was updating the \"What's cooking\" draft and here is my tentative\nsummary of the topic.\n\n    * tb/pack-extra-cruft-tips (2023-05-03) 1 commit\n     - builtin/pack-objects.c: introduce `pack.extraCruftTips`\n\n     \"git pack-objects\" learned to invoke a new hook program that\n     enumerates extra objects to be used as anchoring points to keep\n     otherwise unreachable objects in cruft packs.\n\n     source: <27a7f16aab35b5cac391d9831aadb0f2e2146313.1683151485.git.me@ttaylorr.com>\n\nI think I earlier saw review comments on a side thread that\nmentioned \"repack -A\"; when I read it I didn't quite get the point\nof the discussion, but after summarizing the gist of the topic as\nabove, I _think_ I tend to agree.  This should not be about \"should\nwe have these extra objects in cruft pack?\" but is about \"should we\nkeep these extra objects alive and exempt from pruning\".  What these\nextra tips tell us is that the part of the reachability graph rooted\nat these extra objects should be treated as if they are reachable.\n\nStoring them inside cruft packs may be a reasonable choice to make\ntoday, in the sense that among various object storage mechansim, the\ncruft pack mechanism may be the best fit in today's system, but it\ndoes not have to stay that way.  Naming the variable to specify the\nhooks with name \"cruft\" in them would make it hard to explain once\nwe find an even better storage mechanism to store such a \"not really\nused but want to keep\" objects.\n"},{"id":"476583","messageId":"ZFLyv0s/o8gr6IZH@nand.local","threadId":"59624","inReplyTo":"xmqqjzxpm172.fsf@gitster.g","subject":"Re: [PATCH v3] builtin/pack-objects.c: introduce `pack.extraCruftTips`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-05-03T23:48:15Z","receivedAt":"2023-05-03T23:50:49Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, May 03, 2023 at 04:42:41PM -0700, Junio C Hamano wrote:\n> Storing them inside cruft packs may be a reasonable choice to make\n> today, in the sense that among various object storage mechansim, the\n> cruft pack mechanism may be the best fit in today's system, but it\n> does not have to stay that way.  Naming the variable to specify the\n> hooks with name \"cruft\" in them would make it hard to explain once\n> we find an even better storage mechanism to store such a \"not really\n> used but want to keep\" objects.\n\nI dunno. I thought about this too, and I get your argument, but I am not\nconvinced that a future mechanism would lend itself well to keeping\naround additional sets of objects in the same way cruft packs do. In\nthat case, we would prefer having called this `pack.extraCruftTips` and\nrelegating it to the cruft pack system.\n\nWe could make this more generic, and extend support to the legacy\nprune-via-loose mechanism. But like I said to Peff, I have a hard time\nimagining anybody using it.\n\nSo, I'm torn. I see what you're saying, but I think I still tend to fall\non the side of leaving it as-is.\n\nThanks,\nTaylor\n"},{"id":"476584","messageId":"ZFLzC1slvOAD6udn@nand.local","threadId":"59624","inReplyTo":"xmqqjzxpm172.fsf@gitster.g","subject":"Re: [PATCH v3] builtin/pack-objects.c: introduce `pack.extraCruftTips`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-05-03T23:50:30Z","receivedAt":"2023-05-03T23:50:56Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, May 03, 2023 at 04:42:41PM -0700, Junio C Hamano wrote:\n> Storing them inside cruft packs may be a reasonable choice to make\n> today, in the sense that among various object storage mechansim, the\n> cruft pack mechanism may be the best fit in today's system, but it\n> does not have to stay that way.  Naming the variable to specify the\n> hooks with name \"cruft\" in them would make it hard to explain once\n> we find an even better storage mechanism to store such a \"not really\n> used but want to keep\" objects.\n\nI dunno. I thought about this too, and I get your argument, but I am not\nconvinced that a future mechanism would lend itself well to keeping\naround additional sets of objects in the same way cruft packs do. In\nthat case, we would prefer having called this `pack.extraCruftTips` and\nrelegating it to the cruft pack system.\n\nWe could make this more generic, and extend support to the legacy\nprune-via-loose mechanism. But like I said to Peff, I have a hard time\nimagining anybody using it.\n\nSo, I'm torn. I see what you're saying, but I think I still tend to fall\non the side of leaving it as-is.\n\nThanks,\nTaylor\n"},{"id":"476652","messageId":"20230505212322.GA3321533@coredump.intra.peff.net","threadId":"59624","inReplyTo":"ZFLQfMtIGcZHNERE@nand.local","subject":"Re: [PATCH v2] builtin/pack-objects.c: introduce `pack.extraCruftTips`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-05-05T21:23:22Z","receivedAt":"2023-05-05T21:23:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 03, 2023 at 05:22:04PM -0400, Taylor Blau wrote:\n\n> > OK, I understand the use case you're trying to support, and your\n> > approach mostly makes sense. But there are two things I was surprised by\n> > in the implementation:\n> >\n> >   1. Does this need to be tied to cruft packs? The same logic would\n> >      apply to \"repack -A\" which turns objects loose (of course you\n> >      probably don't want to do that in the long term for performance\n> >      reasons, but it's conceptually the same thing; see below).\n> >\n> >   2. Why is there a separate walk distinct from the one that rescues\n> >      recent-but-unreachable objects?\n> \n> > Conceptually it seems to me that the simplest and most flexible way to\n> > think of this new feature is: pretend these objects are recent enough to\n> > be kept in the grace period, even though their mtimes do not qualify\".\n> >\n> > And then everything else would just fall out naturally. Am I missing\n> > something?\n> \n> I originally shied away from it, thinking that I wouldn't want to do an\n> expensive walk with all of the recent objects during a non-pruning\n> repack.\n>\n> The two code paths are quite different in practice. In the pruning case,\n> we add only new objects from the kept packs and then start our traversal\n> there. In the non-pruning case, we just do\n> `add_objects_in_unpacked_packs()` which is really just a call to\n> `for_each_packed_object()`.\n\nRight, that's what I'd expect. I think you are describing a cruft-pack\nworld here (because you say \"kept packs\"), but the traditional \"repack\n-k\" versus \"repack -A\" is similar (if we are not doing something special\nwith recent objects, then there is no need to figure out what they\nreach; we can just add them all).\n\n> So it gets tricky when you have a pack.extraCruftTips program and want\n> to invoke it in a non-pruning case. You could do something like:\n> \n>   - call enumerate_and_traverse_cruft_objects() *always*, either because\n>     we were doing a pruning GC, or calling it after\n>     `enumerate_cruft_objects()` (in the non-pruning case)\n> \n>   - ensure that enumerate_and_traverse_cruft_objects() is a noop when\n>     (a) cruft_expiration is set to zero, and (b) there are no\n>     pack.extraCruftTips programs specified\n\nI'm not sure why you'd need to traverse, though. If we are in \"-k\" mode,\nwe are keeping everything anyway (so I don't even see the point of\nasking the helper about extra tips). And all of those objects that are\nnot reachable from the regular traversal are by definition \"cruft\" and\ngo into the cruft pack.\n\nMaybe I don't understand what you mean by \"non-pruning\" here.\n\n-Peff\n"},{"id":"476653","messageId":"20230505212631.GB3321533@coredump.intra.peff.net","threadId":"59624","inReplyTo":"ZFLSDS89y8P2+g/2@nand.local","subject":"Re: [PATCH v2] builtin/pack-objects.c: introduce `pack.extraCruftTips`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-05-05T21:26:31Z","receivedAt":"2023-05-05T21:26:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 03, 2023 at 05:28:45PM -0400, Taylor Blau wrote:\n\n> > OK, I understand the use case you're trying to support, and your\n> > approach mostly makes sense. But there are two things I was surprised by\n> > in the implementation:\n> >\n> >   1. Does this need to be tied to cruft packs? The same logic would\n> >      apply to \"repack -A\" which turns objects loose (of course you\n> >      probably don't want to do that in the long term for performance\n> >      reasons, but it's conceptually the same thing; see below).\n> \n> I agree that you wouldn't want to do it for performance reasons, but I'm\n> comfortable with the asymmetry here, since this is `pack.extraCruftTips`\n> (emphasis on \"cruft\"), so it's not clear that it has to be related to\n> \"repack -A\".\n> \n> Happy to change things up if you feel strongly, though.\n\nI don't feel strongly. I certainly have no intent to run \"git repack -A\"\nwith extra tips specified. Mostly I just thought that it would be\nsimpler to just apply it everywhere, both conceptually to the user and\nwithin the implementation.\n\nBut since the cruft-pack implementation diverges quite a bit from the\nregular \"-A\" handling, I guess it makes the code more complex rather\nthan less. The asymmetry feels like a wart to me, but I guess in the\nlong run we'd hope that the \"turn unreachable loose\" strategy is\ndeprecated anyway.\n\n-Peff\n"},{"id":"476654","messageId":"20230505213946.GC3321533@coredump.intra.peff.net","threadId":"59624","inReplyTo":"xmqqv8h9m2az.fsf@gitster.g","subject":"Re: [PATCH v3] builtin/pack-objects.c: introduce `pack.extraCruftTips`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-05-05T21:39:46Z","receivedAt":"2023-05-05T21:39:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 03, 2023 at 04:18:44PM -0700, Junio C Hamano wrote:\n\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> \n> What is a \"tip of an object\"?  The first byte ;-)?\n> \n> A \"tip of history\" would only imply commit objects, but presumably\n> you would want to specify a tree and protect all the blobs and trees\n> it recursively contains, so that is not a good name for it.\n\n\"tips of the object graph\" perhaps?\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\ttype = oid_object_info(the_repository, &oid, NULL);\n> > +\t\tif (type < 0)\n> > +\t\t\tcontinue;\n> > +\n> > +\t\tobj = lookup_object_by_type(the_repository, &oid, type);\n> > +\t\tif (!obj)\n> > +\t\t\tcontinue;\n> \n> Hmph, we may want to have an interface that lets us avoid looking up\n> the same oid twice in the same set of tables.  Given an object\n> unseen so far, oid_object_info() should have done most of the work\n> necessary for lookup_object_by_type() to get to and start parsing\n> the data of the object in the good case (i.e. object exists and in a\n> pack---just we haven't needed it yet), but in the above sequence\n> there is not enough information passed between two calls to take\n> advantage of it.\n\nThis code was my suggestion, but it may have actually been a bad\ndirection.\n\nI don't think communicating between oid_object_info() and\nlookup_object_by_type() is important. The latter is only doing a lookup\nin the internal hash with lookup_object(), and then auto-vivifying using\nthe type if necessary (which we provide to it).\n\nThe bigger inefficiency is that we call oid_object_info() before seeing\nif we have already instantiated an object struct via lookup_object().\n\nObviously we could do that first. But let's take a step back. My\noriginal suggestion was thinking that we don't want to call\nparse_object() because it's expensive, especially for a blob. But in the\nlong run, most of these objects (except blobs!) will end up parsed\nanyway, because we are going to see which other objects they reach.\n\nSo it's OK to parse anything except blobs. And indeed, we have a better\ntool for that these days:\n\n  obj = parse_object_with_flags(r, oid, PARSE_OBJECT_SKIP_HASH_CHECK);\n\nThat does exactly what we want. If we already saw and parsed the object,\nit's quick noop after a hash lookup. If we didn't, then it already has\noptimizations to avoid reading object contents if possible (checking the\ncommit graph, checking the type for blobs).\n\nSkipping the hash check might seem like a bad idea for a repack, but\nit's what we already do for blobs found via traversing. A disk repack\nuses the much cheaper pack idx crc for exactly this purpose: to avoid\nexpanding objects unnecessarily.\n\n-Peff\n"},{"id":"476656","messageId":"20230505221357.GD3321533@coredump.intra.peff.net","threadId":"59624","inReplyTo":"20230505212631.GB3321533@coredump.intra.peff.net","subject":"Re: [PATCH v2] builtin/pack-objects.c: introduce `pack.extraCruftTips`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-05-05T22:13:57Z","receivedAt":"2023-05-05T22:14:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 05, 2023 at 05:26:31PM -0400, Jeff King wrote:\n\n> But since the cruft-pack implementation diverges quite a bit from the\n> regular \"-A\" handling, I guess it makes the code more complex rather\n> than less. The asymmetry feels like a wart to me, but I guess in the\n> long run we'd hope that the \"turn unreachable loose\" strategy is\n> deprecated anyway.\n\nOne thing that could make this a lot simpler is if the code was added to\n\"are we recent\" code paths in the first place.\n\nSomething like this:\n\ndiff --git a/reachable.c b/reachable.c\nindex 55bb114353..86bb5e021e 100644\n--- a/reachable.c\n+++ b/reachable.c\n@@ -16,6 +16,9 @@\n #include \"object-store.h\"\n #include \"pack-bitmap.h\"\n #include \"pack-mtimes.h\"\n+#include \"oidset.h\"\n+#include \"run-command.h\"\n+#include \"config.h\"\n \n struct connectivity_progress {\n \tstruct progress *progress;\n@@ -67,8 +70,71 @@ struct recent_data {\n \ttimestamp_t timestamp;\n \treport_recent_object_fn *cb;\n \tint ignore_in_core_kept_packs;\n+\tstruct oidset fake_recent_oids;\n+\tint fake_recent_oids_loaded;\n };\n \n+static int run_one_recent_cmd(struct oidset *set, 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\tdie(_(\"invalid extra cruft tip: '%s'\"), buf.buf);\n+\n+\t\toidset_insert(set, &oid);\n+\t}\n+\n+\tif (finish_command(&cmd))\n+\t\tdie(_(\"cruft tip hook returned error\"));\n+\n+\tfclose(out);\n+\tstrbuf_release(&buf);\n+\n+\treturn ret;\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+\tif (mtime > data->timestamp)\n+\t\treturn 1;\n+\n+\tif (!data->fake_recent_oids_loaded) {\n+\t\tconst struct string_list *programs;\n+\n+\t\tif (!git_config_get_string_multi(\"pack.extracrufttips\", &programs)) {\n+\t\t\tsize_t i;\n+\t\t\tfor (i = 0; i < programs->nr; i++) {\n+\t\t\t\tif (run_one_recent_cmd(&data->fake_recent_oids,\n+\t\t\t\t\t\t       programs->items[i].string) < 0)\n+\t\t\t\t\tdie(_(\"unable to enumerate additional cruft tips\"));\n+\t\t\t}\n+\t\t}\n+\t\tdata->fake_recent_oids_loaded = 1;\n+\t}\n+\n+\tif (oidset_contains(&data->fake_recent_oids, oid))\n+\t\treturn 1;\n+\n+\treturn 0;\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 +144,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@@ -193,6 +259,10 @@ 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+\t/* XXX must free before exiting function */\n+\toidset_init(&data.fake_recent_oids, 0);\n+\tdata.fake_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\nThat would affect both \"repack -A\" and \"repack --cruft\". It would also\naffect \"git prune\", but that seems like a good thing to me.\n\nIt would not affect a straight \"repack -ad\", since there we do not ask\nabout recency at all. I'm not sure if that use case is important or not.\nIf it is, we could easily trigger the \"is it recent\" code in that case\nonly if a hook is configured (we'd just set the cutoff to \"0\" so that\neverything looks \"too old\").\n\nNote that this inverts the loop logic from what you have. In your patch,\nyou ask the hook to enumerate all faux-recent objects, and then you\ntreat them separately from objects we found on disk. Here we enumerate\nthe objects on disk as usual, and just adjust our idea of \"recent\" based\non the hook. I think this is a bit simpler because objects we don't even\nbother to ask it about objects we've already handled, are already marked\nas recent, or are not present in the repository.\n\n-Peff\n"},{"id":"476657","messageId":"20230505221921.GE3321533@coredump.intra.peff.net","threadId":"59624","inReplyTo":"27a7f16aab35b5cac391d9831aadb0f2e2146313.1683151485.git.me@ttaylorr.com","subject":"Re: [PATCH v3] builtin/pack-objects.c: introduce `pack.extraCruftTips`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-05-05T22:19:21Z","receivedAt":"2023-05-05T22:19:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 03, 2023 at 06:05:45PM -0400, Taylor Blau wrote:\n\n> +\tout = xfdopen(cmd.out, \"r\");\n> +\twhile (strbuf_getline(&buf, out) != EOF) {\n> +\t\tstruct object_id oid;\n> +\t\tstruct object *obj;\n> +\t\tint type;\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\nWhile adapting this for my \"how about this\" patch elsewhere in the\nthread, I noticed the error handling here is a little funny.\n\nThe other side sent us unexpected output, so we stopped parsing. But we\nnever reap the child process. We just jump to \"done\", which clears the\nmemory used by the process struct, but never call finish_command().\n\nI think you want to break out of this loop and run finish_command. But\nyou have to make sure to fclose(out) first, so that it knows to exit\n(otherwise it may fill up the pipe buffer and block waiting for us to\nread, creating a deadlock).\n\nAnd then\n\nSo something like:\n\n  while (strbuf_getline(...)) {\n\tif (some_error) {\n\t\tret = error(...);\n\t\tbreak;\n  }\n\n  fclose(out); /* no need for \"if (out)\" here; it's always open */\n  ret |= finish_command(&cmd);\n\n  /* no need to child_process_clear(); already done by finish_command() */\n  strbuf_release(&buf);\n  return ret;\n\n-Peff\n"},{"id":"476662","messageId":"ZFWZ9gQ4c0dLzoss@nand.local","threadId":"59624","inReplyTo":"20230505212322.GA3321533@coredump.intra.peff.net","subject":"Re: [PATCH v2] builtin/pack-objects.c: introduce `pack.extraCruftTips`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-05-06T00:06:14Z","receivedAt":"2023-05-06T00:06:22Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, May 05, 2023 at 05:23:22PM -0400, Jeff King wrote:\n> On Wed, May 03, 2023 at 05:22:04PM -0400, Taylor Blau wrote:\n>\n> > So it gets tricky when you have a pack.extraCruftTips program and want\n> > to invoke it in a non-pruning case. You could do something like:\n> >\n> >   - call enumerate_and_traverse_cruft_objects() *always*, either because\n> >     we were doing a pruning GC, or calling it after\n> >     `enumerate_cruft_objects()` (in the non-pruning case)\n> >\n> >   - ensure that enumerate_and_traverse_cruft_objects() is a noop when\n> >     (a) cruft_expiration is set to zero, and (b) there are no\n> >     pack.extraCruftTips programs specified\n>\n> I'm not sure why you'd need to traverse, though. If we are in \"-k\" mode,\n> we are keeping everything anyway (so I don't even see the point of\n> asking the helper about extra tips). And all of those objects that are\n> not reachable from the regular traversal are by definition \"cruft\" and\n> go into the cruft pack.\n>\n> Maybe I don't understand what you mean by \"non-pruning\" here.\n\nBy non-pruning, I meant something like \"git gc --prune=never\", which\nwould run the equivalent of `git repack -A` to generate the pack\ncontaining just reachable objects, and then invoke `git pack-objects\n--cruft` to generate the cruft pack.\n\nThanks,\nTaylor\n"},{"id":"476663","messageId":"ZFWbuf34pVxJyRiM@nand.local","threadId":"59624","inReplyTo":"20230505221357.GD3321533@coredump.intra.peff.net","subject":"Re: [PATCH v2] builtin/pack-objects.c: introduce `pack.extraCruftTips`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-05-06T00:13:45Z","receivedAt":"2023-05-06T00:13:50Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, May 05, 2023 at 06:13:57PM -0400, Jeff King wrote:\n> One thing that could make this a lot simpler is if the code was added to\n> \"are we recent\" code paths in the first place.\n>\n> Something like this:\n\nThis is quite nice, and I think that it's a good direction to push this\n~series~ patch in before queuing. That said, I wasn't sure about...\n\n> @@ -78,7 +144,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\n...this hunk. That only kicks in if you have other recent object(s),\nsince the hooks are consulted as a side-effect of calling your new\n`obj_is_recent()` function.\n\nI think in most cases that's fine, but if you had no otherwise-recent\nobjects around, then this code wouldn't kick in in the first place.\n\nI wonder if it might make more sense to call the hooks directly in\nadd_unseen_recent_objects_to_traversal().\n\n> That would affect both \"repack -A\" and \"repack --cruft\". It would also\n> affect \"git prune\", but that seems like a good thing to me.\n\nI was going to say, I'm not sure this would work since we don't use any\nof this code from enumerate_cruft_objects() when the cruft_expiration is\nset to \"never\", but that's fine since we're keeping all of those objects\nanyway.\n\nOK, my bad, I think that was the point you were trying to make in your\nprevious email, but I didn't quite grok it at the time. Yes, I agree,\nthis code only needs to kick in when pruning.\n\nThanks,\nTaylor\n"},{"id":"476664","messageId":"ZFWb5V2sBYVrOEdF@nand.local","threadId":"59624","inReplyTo":"ZFWZ9gQ4c0dLzoss@nand.local","subject":"Re: [PATCH v2] builtin/pack-objects.c: introduce `pack.extraCruftTips`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-05-06T00:14:29Z","receivedAt":"2023-05-06T00:16:44Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, May 05, 2023 at 08:06:14PM -0400, Taylor Blau wrote:\n> On Fri, May 05, 2023 at 05:23:22PM -0400, Jeff King wrote:\n> > On Wed, May 03, 2023 at 05:22:04PM -0400, Taylor Blau wrote:\n> >\n> > > So it gets tricky when you have a pack.extraCruftTips program and want\n> > > to invoke it in a non-pruning case. You could do something like:\n> > >\n> > >   - call enumerate_and_traverse_cruft_objects() *always*, either because\n> > >     we were doing a pruning GC, or calling it after\n> > >     `enumerate_cruft_objects()` (in the non-pruning case)\n> > >\n> > >   - ensure that enumerate_and_traverse_cruft_objects() is a noop when\n> > >     (a) cruft_expiration is set to zero, and (b) there are no\n> > >     pack.extraCruftTips programs specified\n> >\n> > I'm not sure why you'd need to traverse, though. If we are in \"-k\" mode,\n> > we are keeping everything anyway (so I don't even see the point of\n> > asking the helper about extra tips). And all of those objects that are\n> > not reachable from the regular traversal are by definition \"cruft\" and\n> > go into the cruft pack.\n> >\n> > Maybe I don't understand what you mean by \"non-pruning\" here.\n>\n> By non-pruning, I meant something like \"git gc --prune=never\", which\n> would run the equivalent of `git repack -A` to generate the pack\n> containing just reachable objects, and then invoke `git pack-objects\n> --cruft` to generate the cruft pack.\n\nOops, my misunderstanding. I see what you're saying: in a `git gc\n--prune=never`, it does not matter whether we ask for extra cruft tips,\nsince we're keeping all of those objects anyway. Duh.\n\nThanks,\nTaylor\n"},{"id":"476665","messageId":"ZFWdRoUOQJfP3db+@nand.local","threadId":"59624","inReplyTo":"ZFWbuf34pVxJyRiM@nand.local","subject":"Re: [PATCH v2] builtin/pack-objects.c: introduce `pack.extraCruftTips`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-05-06T00:20:22Z","receivedAt":"2023-05-06T00:20:31Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, May 05, 2023 at 08:13:45PM -0400, Taylor Blau wrote:\n> On Fri, May 05, 2023 at 06:13:57PM -0400, Jeff King wrote:\n> > One thing that could make this a lot simpler is if the code was added to\n> > \"are we recent\" code paths in the first place.\n> >\n> > Something like this:\n>\n> This is quite nice, and I think that it's a good direction to push this\n> ~series~ patch in before queuing. That said, I wasn't sure about...\n>\n> > @@ -78,7 +144,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>\n> ...this hunk. That only kicks in if you have other recent object(s),\n> since the hooks are consulted as a side-effect of calling your new\n> `obj_is_recent()` function.\n>\n> I think in most cases that's fine, but if you had no otherwise-recent\n> objects around, then this code wouldn't kick in in the first place.\n>\n> I wonder if it might make more sense to call the hooks directly in\n> add_unseen_recent_objects_to_traversal().\n\nOK, no, this is fine, but we'd need to make sure that\nwant_recent_object() also understood the fake recent set here, since\nadd_recent_loose() and add_recent_packed() both bail early when\nwant_recent_object() returns 0.\n\nThanks,\nTaylor\n"},{"id":"476666","messageId":"20230506021255.GA3798242@coredump.intra.peff.net","threadId":"59624","inReplyTo":"ZFWbuf34pVxJyRiM@nand.local","subject":"Re: [PATCH v2] builtin/pack-objects.c: introduce `pack.extraCruftTips`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-05-06T02:12:55Z","receivedAt":"2023-05-06T02:13:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 05, 2023 at 08:13:45PM -0400, Taylor Blau wrote:\n\n> > @@ -78,7 +144,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> \n> ...this hunk. That only kicks in if you have other recent object(s),\n> since the hooks are consulted as a side-effect of calling your new\n> `obj_is_recent()` function.\n\nI think we'll evaluate each object in the repo for recent-ness, via\nfor_each_loose_object and for_each_packed_object. So if an object exists\nin the repo it will be evaluated here, as long as we are checking for\nrecent objects at all. And if it doesn't exist, then having the hook\ntell us about it won't help; there is nothing for us to save (nor even\nto use as a source of reachability, since we don't know what's in it).\n\nModulo the want_recent_object() thing you mentioned, of course, which is\nevaluated first. And I could see that yeah, that might need to let the\nhook override it, which shouldn't be too hard to do.\n\nI'm not super familiar with this aspect of the cruft pack code. That\nfunction is trying to avoid looking at objects that are in in-core kept\npacks, which implies repack feeding those via stdin. Looking at\nrepack.c, we are feeding the existing packs we just wrote (so all of the\nreachable stuff). So I can see why we might skip those packs, but at the\nsame time, we know we are keeping them (they were just written!) so they\nare not really cruft candidates anyway. If an extraCruftTips hook told\nus about them, would it matter? They are already being kept as\nnon-cruft.\n\nBut that is just an analysis based on 5 minutes of poking at the code. I\nwon't be surprised if I'm misunderstanding it or missing subtle cases.\n\n-Peff\n"}]}