{"thread":{"id":"66265","subject":"[PATCH] pack-objects: prefetch in the order objects are checked","startedAt":"2026-09-03T12:55:32Z","lastAt":"2026-09-03T12:55:32Z","messageCount":1,"participants":["Aleksei Sviridkin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"551863","messageId":"20260903125529.67971-1-f@lex.la","threadId":"66265","inReplyTo":null,"subject":"[PATCH] pack-objects: prefetch in the order objects are checked","fromName":"Aleksei Sviridkin","fromEmail":"f@lex.la","sentAt":"2026-09-03T12:55:29Z","receivedAt":"2026-09-03T12:55:32Z","isPatch":true,"body":"get_object_details() sorts a pointer array by pack and offset and\nhands each object's position in that array down to prefetch_to_pack(),\nwhich reads it as a position in to_pack.objects. That second array is\nin the order the traversal added entries, so the batch of objects to\nfetch starts at an unrelated entry.\n\nAn object missing from a partial clone sits in no pack, and the offset\nsort places such entries ahead of packed ones. The index is small\nenough that the batch still covers every missing object in a freshly\ncloned repository. Loose objects held locally sort by object name\namong the missing ones, and enough of them move the index past entries\nthat are themselves missing. Those never enter the batch, end up with\nno recorded type, and are fetched one at a time as the pack is\nwritten. Packing a list of 2 missing and 16 locally present loose\nobjects takes two fetches rather than one.\n\nIndex the sorted array in prefetch_to_pack() so the batch holds what\nthe caller has not reached yet.\n\nThis was introduced by e00549aa9b (pack-objects: prefetch objects to\nbe packed, 2020-07-20).\n\nAssisted-by: LLM\nSigned-off-by: Aleksei Sviridkin <f@lex.la>\n---\n\nNotes:\n    Measured on a synthetic repository of 5000 commits, each adding one\n    file, cloned with --filter=blob:none over file:// so that all 5000\n    blobs are absent.  Fetch invocations counted from GIT_TRACE2_EVENT\n    child_start records, objects per fetch read from the --pack_header\n    argument index-pack is given.\n    \n      input to pack-objects              before        after\n      fresh blobless clone, --revs       1 fetch/5000  1 fetch/5000\n      8 absent blobs, then 400 loose     8 fetches/1   1 fetch/8\n    \n    The first row is the ordinary case and does not move.\n    pack_offset_sort compares the in_pack pointer before anything else,\n    and an object that is not present has none, so every absent entry\n    lands ahead of every packed entry.  On a fresh clone the first absent\n    object is therefore at sorted index 0, and reading 0 as a\n    traversal-order index selects the whole array by accident.\n    \n    The second row is the shape the fix is for.  Loose objects have no\n    in_pack pointer either and sort by object name among the absent ones,\n    which moves the first absent object to a nonzero sorted index.  The\n    scan then starts that far into the traversal-order array.  In this\n    input it collects nothing at all, promisor_remote_get_direct()\n    returns early on an empty list, type -1 is recorded, and each object\n    is fetched by itself while the pack is written.\n    \n    Alternatives considered:\n    \n      Drop the start index and scan all of to_pack.objects.  Obviously\n      correct, and it removes a parameter rather than adding one.\n      Rejected because it repeats the lookups for entries the caller has\n      already checked, object_index_start of them on every call.\n    \n      Pass the unsorted index instead, entry - to_pack.objects.  One line\n      at the call site and no new parameter.  Rejected because it only\n      guarantees that the triggering object is in its own batch.  The scan\n      would still cover an arbitrary suffix of the traversal order and\n      leave out absent objects the caller has not reached.\n    \n    Left alone on purpose:\n    \n      prefetch_to_pack() keeps its opening brace on the declaration\n      line.  CodingGuidelines says nothing about function braces, but\n      .clang-format carries BreakBeforeBraces: Linux, so this is a rule\n      rather than a habit.  It arrived with the commit named above and\n      is one of 9 such definitions under builtin/, 2 of them in this\n      file, the other being is_not_in_promisor_pack().  This patch\n      rewrites that declaration, so moving the brace would cost one\n      character.  Left out because it is not what the patch is about,\n      and it is the same edit wherever it lands.\n    \n      check_object() takes entry, sorted_by_offset and object_index, and\n      entry == sorted_by_offset[object_index] holds without being\n      written down.  Deriving entry in the callee would make the\n      mismatch this patch fixes impossible to express again.  Kept as it\n      is because a function that checks one object reads better taking\n      that object, and the file has one caller.\n    \n    Noticed nearby, not touched: pack_offset_sort() orders entries by\n    comparing struct packed_git pointers that do not point into one\n    array, which C99 6.5.8p5 leaves undefined.  The second paragraph of\n    the message rests on that comparison sorting NULL first.  It holds on\n    every platform git runs on and predates this patch by years.\n    \n    The test asserts one fetch round for a packing request of 2 absent and\n    16 loose objects, and counts two without the code change, under both\n    SHA-1 and SHA-256.  Which objects sort where depends on the hash, so\n    the test checks its own precondition, that at least one loose object\n    sorts before the first wanted one, instead of assuming it.  A hash\n    change turns the test into a failure rather than into one that passes\n    for the wrong reason.\n\n builtin/pack-objects.c | 13 +++++++-----\n t/t5300-pack-object.sh | 45 ++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 53 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 27048bbb4d..448ddb99eb 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -2227,12 +2227,13 @@ static int can_reuse_delta(const struct object_id *base_oid,\n \treturn 0;\n }\n \n-static void prefetch_to_pack(uint32_t object_index_start) {\n+static void prefetch_to_pack(struct object_entry **sorted_by_offset,\n+\t\t\t     uint32_t object_index_start) {\n \tstruct oid_array to_fetch = OID_ARRAY_INIT;\n \tuint32_t i;\n \n \tfor (i = object_index_start; i < to_pack.nr_objects; i++) {\n-\t\tstruct object_entry *entry = to_pack.objects + i;\n+\t\tstruct object_entry *entry = sorted_by_offset[i];\n \n \t\tif (!odb_read_object_info_extended(the_repository->objects,\n \t\t\t\t\t\t   &entry->idx.oid,\n@@ -2246,7 +2247,9 @@ static void prefetch_to_pack(uint32_t object_index_start) {\n \toid_array_clear(&to_fetch);\n }\n \n-static void check_object(struct object_entry *entry, uint32_t object_index)\n+static void check_object(struct object_entry *entry,\n+\t\t\t struct object_entry **sorted_by_offset,\n+\t\t\t uint32_t object_index)\n {\n \tsize_t canonical_size;\n \tenum object_type type;\n@@ -2389,7 +2392,7 @@ static void check_object(struct object_entry *entry, uint32_t object_index)\n \tif (odb_read_object_info_extended(the_repository->objects, &entry->idx.oid, &oi,\n \t\t\t\t\t  OBJECT_INFO_SKIP_FETCH_OBJECT | OBJECT_INFO_LOOKUP_REPLACE) < 0) {\n \t\tif (repo_has_promisor_remote(the_repository)) {\n-\t\t\tprefetch_to_pack(object_index);\n+\t\t\tprefetch_to_pack(sorted_by_offset, object_index);\n \t\t\tif (odb_read_object_info_extended(the_repository->objects, &entry->idx.oid, &oi,\n \t\t\t\t\t\t\t  OBJECT_INFO_SKIP_FETCH_OBJECT | OBJECT_INFO_LOOKUP_REPLACE) < 0)\n \t\t\t\ttype = -1;\n@@ -2619,7 +2622,7 @@ static void get_object_details(void)\n \n \tfor (i = 0; i < to_pack.nr_objects; i++) {\n \t\tstruct object_entry *entry = sorted_by_offset[i];\n-\t\tcheck_object(entry, i);\n+\t\tcheck_object(entry, sorted_by_offset, i);\n \t\tif (entry->type_valid &&\n \t\t    oe_size_greater_than(&to_pack, entry,\n \t\t\t\t\t repo_settings_get_big_file_threshold(the_repository)))\ndiff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh\nindex 73445782e7..d94e3d0630 100755\n--- a/t/t5300-pack-object.sh\n+++ b/t/t5300-pack-object.sh\n@@ -650,6 +650,51 @@ test_expect_success 'prefetch objects' '\n \ttest_line_count = 1 donelines\n '\n \n+test_expect_success 'prefetch objects that sort after locally present ones' '\n+\ttest_when_finished \"rm -rf batch_server batch_client\" &&\n+\n+\tgit init batch_server &&\n+\ttest_config -C batch_server uploadpack.allowanysha1inwant 1 &&\n+\ttest_config -C batch_server uploadpack.allowfilter 1 &&\n+\ttest_config -C batch_server protocol.version 2 &&\n+\n+\tfor i in $(test_seq 1 8)\n+\tdo\n+\t\techo \"content $i\" >batch_server/file$i || return 1\n+\tdone &&\n+\tgit -C batch_server add . &&\n+\tgit -C batch_server commit -m initial &&\n+\n+\tgit clone --filter=blob:none --no-checkout \\\n+\t\t\"file://$(pwd)/batch_server\" batch_client &&\n+\ttest_config -C batch_client protocol.version 2 &&\n+\n+\tgit -C batch_client rev-list --objects --all --missing=print >objects &&\n+\tsed -n \"s/^?//p\" objects | sort >absent &&\n+\ttail -n 2 absent >wanted &&\n+\n+\t>loose_names &&\n+\tfor i in $(test_seq 1 16)\n+\tdo\n+\t\techo \"loose $i\" |\n+\t\tgit -C batch_client hash-object -w --stdin >>loose_names ||\n+\t\t\treturn 1\n+\tdone &&\n+\tsort loose_names >loose &&\n+\n+\t# Objects that are in no pack are visited first, in object name\n+\t# order, so at least one locally present object has to be visited\n+\t# before the first wanted one for this to test anything.\n+\tawk -v limit=\"$(head -n 1 wanted)\" \"\\$0 \\\"\\\" < limit \\\"\\\"\" loose >earlier &&\n+\ttest_file_not_empty earlier &&\n+\n+\tcat wanted loose >to_pack &&\n+\tGIT_TRACE_PACKET=$(pwd)/trace_batch \\\n+\t\tgit -C batch_client pack-objects --stdout <to_pack >/dev/null &&\n+\tgrep \"fetch> done\" trace_batch >donelines_batch &&\n+\ttest_line_count = 1 donelines_batch\n+'\n+\n for hash in sha1 sha256\n do\n \ttest_expect_success \"verify-pack with $hash packfile\" '\n\nbase-commit: e9019fcafe0040228b8631c30f97ae1adb61bcdc\n-- \n2.55.0\n\n"}]}