{"thread":{"id":"56399","subject":"[PATCH 0/3] pack-objects: simplify add_objects_in_unpacked_packs()","startedAt":"2021-08-30T02:48:53Z","lastAt":"2021-09-03T02:33:23Z","messageCount":9,"participants":["Taylor Blau","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"434046","messageId":"cover.1630291682.git.me@ttaylorr.com","threadId":"56399","inReplyTo":null,"subject":"[PATCH 0/3] pack-objects: simplify add_objects_in_unpacked_packs()","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-08-30T02:48:43Z","receivedAt":"2021-08-30T02:48:53Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"This short series is extracted from mine and Peff's work on cruft packs. These\nthree patches focus on cleaning up add_objects_in_unpacked_packs(), which is\nused to implement `git repack -k`.\n\nThe pay-off for this clean-up is significant, though: we net -50 lines of code,\nand the result is much more readable, at least in my opinion.\n\nThe changes are described in detail in the patch messages, but essentially we\nare replacing a loop over get_all_packs() with for_each_packed_object() after\nadding a couple of new flags necessary to make the switch. And once we are done\nwith that, the third patch removes a bit from the object flag allocation table.\n\nThanks in advance for your review.\n\nTaylor Blau (3):\n  object-store.h: teach for_each_packed_object to ignore kept packs\n  builtin/pack-objects.c: simplify add_objects_in_unpacked_packs()\n  builtin/pack-objects.c: remove duplicate hash lookup\n\n builtin/pack-objects.c | 85 ++++++------------------------------------\n object-store.h         |  6 +++\n object.h               |  1 -\n packfile.c             |  6 +++\n 4 files changed, 24 insertions(+), 74 deletions(-)\n\n-- \n2.33.0.96.g73915697e6\n"},{"id":"434047","messageId":"30df4374db583796389c64461c93730eb0a5aa89.1630291682.git.me@ttaylorr.com","threadId":"56399","inReplyTo":"cover.1630291682.git.me@ttaylorr.com","subject":"[PATCH 1/3] object-store.h: teach for_each_packed_object to ignore kept packs","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-08-30T02:48:52Z","receivedAt":"2021-08-30T02:48:57Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"The next patch will reimplement a function that wants to iterate over\npacked objects while ignoring packs which are marked as kept (either\nin-core or on-disk).\n\nTeach for_each_packed_object() to ignore all objects from those packs by\nadding a new flag for each of the \"kept\" states that a pack can be in.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n object-store.h | 6 ++++++\n packfile.c     | 6 ++++++\n 2 files changed, 12 insertions(+)\n\ndiff --git a/object-store.h b/object-store.h\nindex d24915ced1..b4dc6668aa 100644\n--- a/object-store.h\n+++ b/object-store.h\n@@ -455,6 +455,12 @@ enum for_each_object_flags {\n \t * Visit objects within a pack in packfile order rather than .idx order\n \t */\n \tFOR_EACH_OBJECT_PACK_ORDER = (1<<2),\n+\n+\t/* Only iterate over packs that are not marked as kept in-core. */\n+\tFOR_EACH_OBJECT_SKIP_IN_CORE_KEPT_PACKS = (1<<3),\n+\n+\t/* Only iterate over packs that do not have .keep files. */\n+\tFOR_EACH_OBJECT_SKIP_ON_DISK_KEPT_PACKS = (1<<4),\n };\n \n /*\ndiff --git a/packfile.c b/packfile.c\nindex 9ef6d98292..4d0d625238 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -2205,6 +2205,12 @@ int for_each_packed_object(each_packed_object_fn cb, void *data,\n \t\tif ((flags & FOR_EACH_OBJECT_PROMISOR_ONLY) &&\n \t\t    !p->pack_promisor)\n \t\t\tcontinue;\n+\t\tif ((flags & FOR_EACH_OBJECT_SKIP_IN_CORE_KEPT_PACKS) &&\n+\t\t    p->pack_keep_in_core)\n+\t\t\tcontinue;\n+\t\tif ((flags & FOR_EACH_OBJECT_SKIP_ON_DISK_KEPT_PACKS) &&\n+\t\t    p->pack_keep)\n+\t\t\tcontinue;\n \t\tif (open_pack_index(p)) {\n \t\t\tpack_errors = 1;\n \t\t\tcontinue;\n-- \n2.33.0.96.g73915697e6\n\n"},{"id":"434048","messageId":"c857e12a032f197626cd6a5eb0eafc66afbb5bed.1630291682.git.me@ttaylorr.com","threadId":"56399","inReplyTo":"cover.1630291682.git.me@ttaylorr.com","subject":"[PATCH 2/3] builtin/pack-objects.c: simplify add_objects_in_unpacked_packs()","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-08-30T02:48:54Z","receivedAt":"2021-08-30T02:49:01Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"This function is used to implement `pack-objects`'s `--keep-unreachable`\noption, but can be simplified in a couple of ways:\n\n  - add_objects_in_unpacked_packs() iterates over all packs (and then\n    all packed objects) itself, but could use for_each_packed_object()\n    instead since the missing flags necessary were added in the previous\n    commit\n\n  - objects are added to an in_pack array which store (off_t, object)\n    tuples, and then sorted in offset order when we could iterate\n    objects in offset order.\n\n    There is a slight behavior change here: before we would have added\n    objects in sorted offset order among _all_ packs. Handing objects to\n    create_object_entry() in pack order for each pack (instead of\n    feeding objects from all packs simultaneously their offset relative\n    to different packs) is much more reasonable, if different than how\n    the code currently works.\n\n  - objects in a single pack are iterated in index order and searched\n    for in order to discover their offsets, which is much less efficient\n    than using the on-disk reverse index\n\nSimplify the function by addressing each of the above and moving the\ncore of the loop into a callback function that we then pass to\nfor_each_packed_object() instead of open-coding the latter function\nourselves.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n builtin/pack-objects.c | 84 ++++++++----------------------------------\n 1 file changed, 16 insertions(+), 68 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex df49f656b9..87ddbd5553 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -3505,79 +3505,27 @@ static void show_edge(struct commit *commit)\n \tadd_preferred_base(&commit->object.oid);\n }\n \n-struct in_pack_object {\n-\toff_t offset;\n-\tstruct object *object;\n-};\n-\n-struct in_pack {\n-\tunsigned int alloc;\n-\tunsigned int nr;\n-\tstruct in_pack_object *array;\n-};\n-\n-static void mark_in_pack_object(struct object *object, struct packed_git *p, struct in_pack *in_pack)\n+static int add_object_in_unpacked_pack(const struct object_id *oid,\n+\t\t\t\t       struct packed_git *pack,\n+\t\t\t\t       uint32_t pos,\n+\t\t\t\t       void *_data)\n {\n-\tin_pack->array[in_pack->nr].offset = find_pack_entry_one(object->oid.hash, p);\n-\tin_pack->array[in_pack->nr].object = object;\n-\tin_pack->nr++;\n-}\n-\n-/*\n- * Compare the objects in the offset order, in order to emulate the\n- * \"git rev-list --objects\" output that produced the pack originally.\n- */\n-static int ofscmp(const void *a_, const void *b_)\n-{\n-\tstruct in_pack_object *a = (struct in_pack_object *)a_;\n-\tstruct in_pack_object *b = (struct in_pack_object *)b_;\n-\n-\tif (a->offset < b->offset)\n-\t\treturn -1;\n-\telse if (a->offset > b->offset)\n-\t\treturn 1;\n-\telse\n-\t\treturn oidcmp(&a->object->oid, &b->object->oid);\n+\tstruct object *obj = lookup_unknown_object(the_repository, oid);\n+\tif (obj->flags & OBJECT_ADDED)\n+\t\treturn 0;\n+\tadd_object_entry(oid, obj->type, \"\", 0);\n+\tobj->flags |= OBJECT_ADDED;\n+\treturn 0;\n }\n \n static void add_objects_in_unpacked_packs(void)\n {\n-\tstruct packed_git *p;\n-\tstruct in_pack in_pack;\n-\tuint32_t i;\n-\n-\tmemset(&in_pack, 0, sizeof(in_pack));\n-\n-\tfor (p = get_all_packs(the_repository); p; p = p->next) {\n-\t\tstruct object_id oid;\n-\t\tstruct object *o;\n-\n-\t\tif (!p->pack_local || p->pack_keep || p->pack_keep_in_core)\n-\t\t\tcontinue;\n-\t\tif (open_pack_index(p))\n-\t\t\tdie(_(\"cannot open pack index\"));\n-\n-\t\tALLOC_GROW(in_pack.array,\n-\t\t\t   in_pack.nr + p->num_objects,\n-\t\t\t   in_pack.alloc);\n-\n-\t\tfor (i = 0; i < p->num_objects; i++) {\n-\t\t\tnth_packed_object_id(&oid, p, i);\n-\t\t\to = lookup_unknown_object(the_repository, &oid);\n-\t\t\tif (!(o->flags & OBJECT_ADDED))\n-\t\t\t\tmark_in_pack_object(o, p, &in_pack);\n-\t\t\to->flags |= OBJECT_ADDED;\n-\t\t}\n-\t}\n-\n-\tif (in_pack.nr) {\n-\t\tQSORT(in_pack.array, in_pack.nr, ofscmp);\n-\t\tfor (i = 0; i < in_pack.nr; i++) {\n-\t\t\tstruct object *o = in_pack.array[i].object;\n-\t\t\tadd_object_entry(&o->oid, o->type, \"\", 0);\n-\t\t}\n-\t}\n-\tfree(in_pack.array);\n+\tif (for_each_packed_object(add_object_in_unpacked_pack, NULL,\n+\t\t\t\t   FOR_EACH_OBJECT_PACK_ORDER |\n+\t\t\t\t   FOR_EACH_OBJECT_LOCAL_ONLY |\n+\t\t\t\t   FOR_EACH_OBJECT_SKIP_IN_CORE_KEPT_PACKS |\n+\t\t\t\t   FOR_EACH_OBJECT_SKIP_ON_DISK_KEPT_PACKS))\n+\t\tdie(_(\"cannot open pack index\"));\n }\n \n static int add_loose_object(const struct object_id *oid, const char *path,\n-- \n2.33.0.96.g73915697e6\n\n"},{"id":"434049","messageId":"5f7f88ac1dc022de2ae5108828a1f61f7150686b.1630291682.git.me@ttaylorr.com","threadId":"56399","inReplyTo":"cover.1630291682.git.me@ttaylorr.com","subject":"[PATCH 3/3] builtin/pack-objects.c: remove duplicate hash lookup","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-08-30T02:48:57Z","receivedAt":"2021-08-30T02:49:05Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In the original code from 08cdfb1337 (pack-objects --keep-unreachable,\n2007-09-16), we add each object to the packing list with type\n`obj->type`, where `obj` comes from `lookup_unknown_object()`. Unless we\nhad already looked up and parsed the object, this will be `OBJ_NONE`.\nThat's fine, since oe_set_type() sets the type_valid bit to '0', and we\ndetermine the real type later on.\n\nSo the only thing we need from the object lookup is access to the\n`flags` field so that we can mark that we've added the object with\n`OBJECT_ADDED` to avoid adding it again (we can just pass `OBJ_NONE`\ndirectly instead of grabbing it from the object).\n\nBut add_object_entry() already rejects duplicates! This has been the\nbehavior since 7a979d99ba (Thin pack - create packfile with missing\ndelta base., 2006-02-19), but 08cdfb1337 didn't take advantage of it.\nMoreover, to do the OBJECT_ADDED check, we have to do a hash lookup in\n`obj_hash`.\n\nSo we can drop the lookup_unknown_object() call completely, *and* the\nOBJECT_ADDED flag, too, since the spot we're touching here is the only\nlocation that checks it.\n\nIn the end, we perform the same number of hash lookups, but with the\nadded bonus that we don't waste memory allocating an OBJ_NONE object (if\nwe were traversing, we'd need it eventually, but the whole point of this\ncode path is not to traverse).\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n builtin/pack-objects.c | 11 +----------\n object.h               |  1 -\n 2 files changed, 1 insertion(+), 11 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 87ddbd5553..ec8503563a 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -3405,13 +3405,9 @@ static void read_object_list_from_stdin(void)\n \t}\n }\n \n-/* Remember to update object flag allocation in object.h */\n-#define OBJECT_ADDED (1u<<20)\n-\n static void show_commit(struct commit *commit, void *data)\n {\n \tadd_object_entry(&commit->object.oid, OBJ_COMMIT, NULL, 0);\n-\tcommit->object.flags |= OBJECT_ADDED;\n \n \tif (write_bitmap_index)\n \t\tindex_commit_for_bitmap(commit);\n@@ -3424,7 +3420,6 @@ static void show_object(struct object *obj, const char *name, void *data)\n {\n \tadd_preferred_base_object(name);\n \tadd_object_entry(&obj->oid, obj->type, name, 0);\n-\tobj->flags |= OBJECT_ADDED;\n \n \tif (use_delta_islands) {\n \t\tconst char *p;\n@@ -3510,11 +3505,7 @@ static int add_object_in_unpacked_pack(const struct object_id *oid,\n \t\t\t\t       uint32_t pos,\n \t\t\t\t       void *_data)\n {\n-\tstruct object *obj = lookup_unknown_object(the_repository, oid);\n-\tif (obj->flags & OBJECT_ADDED)\n-\t\treturn 0;\n-\tadd_object_entry(oid, obj->type, \"\", 0);\n-\tobj->flags |= OBJECT_ADDED;\n+\tadd_object_entry(oid, OBJ_NONE, \"\", 0);\n \treturn 0;\n }\n \ndiff --git a/object.h b/object.h\nindex 3b38c9cc98..549f2d256b 100644\n--- a/object.h\n+++ b/object.h\n@@ -75,7 +75,6 @@ struct object_array {\n  * builtin/fsck.c:           0--3\n  * builtin/gc.c:             0\n  * builtin/index-pack.c:                                     2021\n- * builtin/pack-objects.c:                                   20\n  * builtin/reflog.c:                   10--12\n  * builtin/show-branch.c:    0-------------------------------------------26\n  * builtin/unpack-objects.c:                                 2021\n-- \n2.33.0.96.g73915697e6\n"},{"id":"434050","messageId":"xmqqy28j5tct.fsf@gitster.g","threadId":"56399","inReplyTo":"cover.1630291682.git.me@ttaylorr.com","subject":"Re: [PATCH 0/3] pack-objects: simplify add_objects_in_unpacked_packs()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-30T06:39:30Z","receivedAt":"2021-08-30T06:39:34Z","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 short series is extracted from mine and Peff's work on cruft packs. These\n> three patches focus on cleaning up add_objects_in_unpacked_packs(), which is\n> used to implement `git repack -k`.\n>\n> The pay-off for this clean-up is significant, though: we net -50 lines of code,\n> and the result is much more readable, at least in my opinion.\n>\n> The changes are described in detail in the patch messages, but essentially we\n> are replacing a loop over get_all_packs() with for_each_packed_object() after\n> adding a couple of new flags necessary to make the switch. And once we are done\n> with that, the third patch removes a bit from the object flag allocation table.\n\nA quite pleasant read.  \n\nEach step looked nicely done and well explained.\n\nWill queue.\n\nThanks.\n\n>\n> Thanks in advance for your review.\n>\n> Taylor Blau (3):\n>   object-store.h: teach for_each_packed_object to ignore kept packs\n>   builtin/pack-objects.c: simplify add_objects_in_unpacked_packs()\n>   builtin/pack-objects.c: remove duplicate hash lookup\n>\n>  builtin/pack-objects.c | 85 ++++++------------------------------------\n>  object-store.h         |  6 +++\n>  object.h               |  1 -\n>  packfile.c             |  6 +++\n>  4 files changed, 24 insertions(+), 74 deletions(-)\n"},{"id":"434168","messageId":"YS1GemF3aKTe//Tn@coredump.intra.peff.net","threadId":"56399","inReplyTo":"cover.1630291682.git.me@ttaylorr.com","subject":"Re: [PATCH 0/3] pack-objects: simplify add_objects_in_unpacked_packs()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-08-30T20:58:34Z","receivedAt":"2021-08-30T20:58:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Aug 29, 2021 at 10:48:43PM -0400, Taylor Blau wrote:\n\n> This short series is extracted from mine and Peff's work on cruft packs. These\n> three patches focus on cleaning up add_objects_in_unpacked_packs(), which is\n> used to implement `git repack -k`.\n> \n> The pay-off for this clean-up is significant, though: we net -50 lines of code,\n> and the result is much more readable, at least in my opinion.\n> \n> The changes are described in detail in the patch messages, but essentially we\n> are replacing a loop over get_all_packs() with for_each_packed_object() after\n> adding a couple of new flags necessary to make the switch. And once we are done\n> with that, the third patch removes a bit from the object flag allocation table.\n\nThanks for extracting these from that other work. I gave them all an\nextra read-through and didn't find anything wrong.\n\n>  builtin/pack-objects.c | 85 ++++++------------------------------------\n>  object-store.h         |  6 +++\n>  object.h               |  1 -\n>  packfile.c             |  6 +++\n>  4 files changed, 24 insertions(+), 74 deletions(-)\n\nOf course I love this diffstat, and I _really_ love dropping the one-off\nbit from the flag allocation table. But I wanted to also give some\nnumbers for the lookup_unknown_object() part of the final patch.\n\nThose unknown objects cost 72 bytes of heap each (the same size as a\ncommit, since it's the biggest struct and the unknown object is a\nunion). We've seen some real-world cases where there are 40M+\nunreachable objects. So that's almost 3GB of wasted RAM during\npack-objects just to store those \"did I see it already\" bits. :)\n\nOf course, we are already spending 96 bytes per object in \"struct\nobject_entry\", but at least that's doing more useful stuff. :)\n\n(And the more obvious question is: why not delete those objects. The\nanswer thus far has been: because git's pruning is racy, and thus we\ndon't run it as part of our automatic maintenance).\n\n-Peff\n"},{"id":"434180","messageId":"YS1N2moWMnd5nq83@nand.local","threadId":"56399","inReplyTo":"YS1GemF3aKTe//Tn@coredump.intra.peff.net","subject":"Re: [PATCH 0/3] pack-objects: simplify add_objects_in_unpacked_packs()","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-08-30T21:30:02Z","receivedAt":"2021-08-30T21:30:07Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Aug 30, 2021 at 04:58:34PM -0400, Jeff King wrote:\n> Those unknown objects cost 72 bytes of heap each (the same size as a\n> commit, since it's the biggest struct and the unknown object is a\n> union). We've seen some real-world cases where there are 40M+\n> unreachable objects. So that's almost 3GB of wasted RAM during\n> pack-objects just to store those \"did I see it already\" bits. :)\n\nI wondered if you could see the drop-off represented in GitHub's overall\n\"total heap allocated by Git\" metrics, but they get drowned out in the\nnoise of other processes we're running.\n\nBut not wasting memory there is obviously good, even if it isn't\nnoticeable in the overall metrics.\n\nThanks,\nTaylor\n"},{"id":"434611","messageId":"YTFYhG+nK49/jR/v@coredump.intra.peff.net","threadId":"56399","inReplyTo":"c857e12a032f197626cd6a5eb0eafc66afbb5bed.1630291682.git.me@ttaylorr.com","subject":"Re: [PATCH 2/3] builtin/pack-objects.c: simplify add_objects_in_unpacked_packs()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-02T23:04:36Z","receivedAt":"2021-09-02T23:04:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Aug 29, 2021 at 10:48:54PM -0400, Taylor Blau wrote:\n\n> +static int add_object_in_unpacked_pack(const struct object_id *oid,\n> +\t\t\t\t       struct packed_git *pack,\n> +\t\t\t\t       uint32_t pos,\n> +\t\t\t\t       void *_data)\n>  {\n> [...]\n> +\tstruct object *obj = lookup_unknown_object(the_repository, oid);\n> +\tif (obj->flags & OBJECT_ADDED)\n> +\t\treturn 0;\n> +\tadd_object_entry(oid, obj->type, \"\", 0);\n> +\tobj->flags |= OBJECT_ADDED;\n> +\treturn 0;\n>  }\n\nThis is not new in your patch series, but while merging this with\nanother topic I had, I noticed another opportunity for optimization\nhere. We already know the pack/pos in which we found the object. But\nwhen we call add_object_entry(), it will do another lookup, via\nwant_object_in_pack().\n\nWe could shortcut that by providing it with the extra information, the\nway add_object_entry_from_bitmap() does. The original code before your\nseries could have done the same optimization, but it became much more\nobvious after your series, since -Wunused-parameters notices that we do\nnot look at the \"pack\" or \"pos\" parameters at all. :)\n\nIt may not be that exciting an optimization, though. Pack lookups aren't\n_that_ expensive, and the pack-mru code would mean we always find it in\nthe first pack (since by definition we're iterating through the objects\nin whole packs, our locality is perfect).\n\nIt would also probably involve some slight refactoring of\nadd_object_entry() to avoid duplication (though possibly the result\ncould reduce similar duplication with the bitmap variant). Hmm, actually\nlooking further, we already have add_object_entry_from_pack() for\n--stdin-packs.\n\nSo I offer it mainly as an observation, in case somebody wants to look\ninto it further (both for the optimization and the possibility of\nsimplifying the code).\n\n-Peff\n"},{"id":"434619","messageId":"YTGJbYmq7O2OvqIJ@nand.local","threadId":"56399","inReplyTo":"YTFYhG+nK49/jR/v@coredump.intra.peff.net","subject":"Re: [PATCH 2/3] builtin/pack-objects.c: simplify add_objects_in_unpacked_packs()","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-03T02:33:17Z","receivedAt":"2021-09-03T02:33:23Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Sep 02, 2021 at 07:04:36PM -0400, Jeff King wrote:\n> On Sun, Aug 29, 2021 at 10:48:54PM -0400, Taylor Blau wrote:\n>\n> > +static int add_object_in_unpacked_pack(const struct object_id *oid,\n> > +\t\t\t\t       struct packed_git *pack,\n> > +\t\t\t\t       uint32_t pos,\n> > +\t\t\t\t       void *_data)\n> >  {\n> > [...]\n> > +\tstruct object *obj = lookup_unknown_object(the_repository, oid);\n> > +\tif (obj->flags & OBJECT_ADDED)\n> > +\t\treturn 0;\n> > +\tadd_object_entry(oid, obj->type, \"\", 0);\n> > +\tobj->flags |= OBJECT_ADDED;\n> > +\treturn 0;\n> >  }\n>\n> This is not new in your patch series, but while merging this with\n> another topic I had, I noticed another opportunity for optimization\n> here. We already know the pack/pos in which we found the object. But\n> when we call add_object_entry(), it will do another lookup, via\n> want_object_in_pack().\n>\n> [...]\n>\n> It would also probably involve some slight refactoring of\n> add_object_entry() to avoid duplication (though possibly the result\n> could reduce similar duplication with the bitmap variant). Hmm, actually\n> looking further, we already have add_object_entry_from_pack() for\n> --stdin-packs.\n\nI was trying to remember how I handled this for cruft packs (which want\nto take a slightly different path to add_object_entry() in order to\nrecord the object mtimes along the way). Indeed, in that case there is a\nspecial add_cruft_object_entry() which uses pack and offset directly.\n\nHaving looked at all of these functions which are layered on top of\nadd_object_entry() recently, I tend to agree that there is some room\nfor clean-up there.\n\nI plan on sending the cruft pack series soon, after I untangle all of\nthe `repack` + multi-pack bitmaps stuff, so perhaps that will be a good\ntime to pursue this.\n\nAnyway, just to say that I probably agree that the pack mru cache is\nhelping us enough to make this negligible, but that it is on my mind and\nwe'll have a chance to look at it soon.\n\nThanks,\nTaylor\n"}]}