{"thread":{"id":"65851","subject":"[PATCH 0/3] connected: search promisor objects generically","startedAt":"2026-06-22T08:49:37Z","lastAt":"2026-06-26T06:55:39Z","messageCount":25,"participants":["Patrick Steinhardt","Junio C Hamano","Christian Couder"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"546145","messageId":"20260622-pks-connected-generic-promisor-checks-v1-0-25eba2698202@pks.im","threadId":"65851","inReplyTo":null,"subject":"[PATCH 0/3] connected: search promisor objects generically","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-06-22T08:49:26Z","receivedAt":"2026-06-22T08:49:37Z","isPatch":true,"body":"Hi,\n\nthis patch series refactors \"connected.c\" so that we search for promisor\nobjects in a generic way instead of reaching into internal of the object\ndatabase. As a result, the connectivity checks will work properly in\nrepos that don't use packfiles in the first place.\n\nThe series is built on top of 8d96f09e92 (Merge branch\n'js/objects-larger-than-4gb-on-windows', 2026-06-19) with\nps/odb-source-packed at 1bba3c035d (odb/source-packed: drop pointer to\n\"files\" parent source, 2026-06-17) merged into it.\n\nThanks!\n\nPatrick\n\n---\nPatrick Steinhardt (3):\n odb/source-packed: extract logic to skip certain packs\n odb/source-packed: support flags when iterating an object prefix\n connected: search promisor objects generically\n\n connected.c | 39 +++++++++++++++++++++++++--------------\n odb/source-packed.c | 50 +++++++++++++++++++++++++++++++++++++-------------\n 2 files changed, 62 insertions(+), 27 deletions(-)\n\n\n---\nbase-commit: 4a8e7a446f41435e157131162dfe901eca9250fe\nchange-id: 20260612-pks-connected-generic-promisor-checks-2933bff3028d\n\n"},{"id":"546146","messageId":"20260622-pks-connected-generic-promisor-checks-v1-1-25eba2698202@pks.im","threadId":"65851","inReplyTo":"20260622-pks-connected-generic-promisor-checks-v1-0-25eba2698202@pks.im","subject":"[PATCH 1/3] odb/source-packed: extract logic to skip certain packs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-06-22T08:49:27Z","receivedAt":"2026-06-22T08:49:38Z","isPatch":true,"body":"The caller can pass flags that allow them to filter out specific kinds\nof objects when iterating objects via `odb_for_each_object()`. This only\nworks for \"normal\" iteration though, as we `BUG()` when the user passes\nflags and specifies an object prefix.\n\nThis limitation will be lifted in the next commit. Prepare for this by\nextracting the logic that skips certain kinds of packs so that we can\neasily reuse it.\n\nSigned-off-by: Patrick Steinhardt \n---\n odb/source-packed.c | 28 ++++++++++++++++++----------\n 1 file changed, 18 insertions(+), 10 deletions(-)\n\ndiff --git a/odb/source-packed.c b/odb/source-packed.c\nindex 42c28fba0e..3afc4bf01f 100644\n--- a/odb/source-packed.c\n+++ b/odb/source-packed.c\n@@ -126,6 +126,22 @@ static int match_hash(unsigned len, const unsigned char *a, const unsigned char\n \treturn 1;\n }\n \n+static bool should_exclude_pack(struct packed_git *p, enum odb_for_each_object_flags flags)\n+{\n+\tif ((flags & ODB_FOR_EACH_OBJECT_LOCAL_ONLY) && !p->pack_local)\n+\t\treturn true;\n+\tif ((flags & ODB_FOR_EACH_OBJECT_PROMISOR_ONLY) &&\n+\t !p->pack_promisor)\n+\t\treturn true;\n+\tif ((flags & ODB_FOR_EACH_OBJECT_SKIP_IN_CORE_KEPT_PACKS) &&\n+\t p->pack_keep_in_core)\n+\t\treturn true;\n+\tif ((flags & ODB_FOR_EACH_OBJECT_SKIP_ON_DISK_KEPT_PACKS) &&\n+\t p->pack_keep)\n+\t\treturn true;\n+\treturn false;\n+}\n+\n static int for_each_prefixed_object_in_midx(\n \tstruct odb_source_packed *store,\n \tstruct multi_pack_index *m,\n@@ -306,17 +322,9 @@ static int odb_source_packed_for_each_object(struct odb_source *source,\n \tfor (e = packfile_store_get_packs(packed); e; e = e->next) {\n \t\tstruct packed_git *p = e->pack;\n \n-\t\tif ((opts->flags & ODB_FOR_EACH_OBJECT_LOCAL_ONLY) && !p->pack_local)\n-\t\t\tcontinue;\n-\t\tif ((opts->flags & ODB_FOR_EACH_OBJECT_PROMISOR_ONLY) &&\n-\t\t !p->pack_promisor)\n-\t\t\tcontinue;\n-\t\tif ((opts->flags & ODB_FOR_EACH_OBJECT_SKIP_IN_CORE_KEPT_PACKS) &&\n-\t\t p->pack_keep_in_core)\n-\t\t\tcontinue;\n-\t\tif ((opts->flags & ODB_FOR_EACH_OBJECT_SKIP_ON_DISK_KEPT_PACKS) &&\n-\t\t p->pack_keep)\n+\t\tif (should_exclude_pack(p, opts->flags))\n \t\t\tcontinue;\n+\n \t\tif (open_pack_index(p)) {\n \t\t\tpack_errors = 1;\n \t\t\tcontinue;\n\n-- \n2.55.0.rc1.745.g43192e7977.dirty\n\n"},{"id":"546147","messageId":"20260622-pks-connected-generic-promisor-checks-v1-2-25eba2698202@pks.im","threadId":"65851","inReplyTo":"20260622-pks-connected-generic-promisor-checks-v1-0-25eba2698202@pks.im","subject":"[PATCH 2/3] odb/source-packed: support flags when iterating an object prefix","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-06-22T08:49:28Z","receivedAt":"2026-06-22T08:49:41Z","isPatch":true,"body":"Callers of `odb_for_each_object()` can specify an optional object name\nprefix so that we only yield objects that match it. This is incompatible\nthough with passing flags at the same time, as we don't yet know to\nhandle them.\n\nLoosen this restriction by calling `should_exclude_pack()`.\n\nSigned-off-by: Patrick Steinhardt \n---\n odb/source-packed.c | 22 +++++++++++++++++++---\n 1 file changed, 19 insertions(+), 3 deletions(-)\n\ndiff --git a/odb/source-packed.c b/odb/source-packed.c\nindex 3afc4bf01f..6f31f0ff94 100644\n--- a/odb/source-packed.c\n+++ b/odb/source-packed.c\n@@ -148,6 +148,7 @@ static int for_each_prefixed_object_in_midx(\n \tconst struct odb_for_each_object_options *opts,\n \tstruct odb_source_packed_for_each_object_wrapper_data *data)\n {\n+\tbool pack_errors = false;\n \tint ret;\n \n \tfor (; m; m = m->base_midx) {\n@@ -171,6 +172,20 @@ static int for_each_prefixed_object_in_midx(\n \t\t\tconst struct object_id *current = NULL;\n \t\t\tstruct object_id oid;\n \n+\t\t\tif (opts->flags) {\n+\t\t\t\tuint32_t pack_id = nth_midxed_pack_int_id(m, i);\n+\t\t\t\tstruct packed_git *pack;\n+\n+\t\t\t\tif (prepare_midx_pack(m, pack_id)) {\n+\t\t\t\t\tpack_errors = true;\n+\t\t\t\t\tcontinue;\n+\t\t\t\t}\n+\n+\t\t\t\tpack = nth_midxed_pack(m, pack_id);\n+\t\t\t\tif (should_exclude_pack(pack, opts->flags))\n+\t\t\t\t\tcontinue;\n+\t\t\t}\n+\n \t\t\tcurrent = nth_midxed_object_oid(&oid, m, i);\n \n \t\t\tif (!match_hash(len, opts->prefix->hash, current->hash))\n@@ -198,6 +213,8 @@ static int for_each_prefixed_object_in_midx(\n \tret = 0;\n \n out:\n+\tif (!ret && pack_errors)\n+\t\tret = -1;\n \treturn ret;\n }\n \n@@ -260,9 +277,6 @@ static int odb_source_packed_for_each_prefixed_object(\n \tbool pack_errors = false;\n \tint ret;\n \n-\tif (opts->flags)\n-\t\tBUG(\"flags unsupported\");\n-\n \tstore->skip_mru_updates = true;\n \n \tm = get_multi_pack_index(store);\n@@ -275,6 +289,8 @@ static int odb_source_packed_for_each_prefixed_object(\n \tfor (e = packfile_store_get_packs(store); e; e = e->next) {\n \t\tif (e->pack->multi_pack_index)\n \t\t\tcontinue;\n+\t\tif (should_exclude_pack(e->pack, opts->flags))\n+\t\t\tcontinue;\n \n \t\tif (open_pack_index(e->pack)) {\n \t\t\tpack_errors = true;\n\n-- \n2.55.0.rc1.745.g43192e7977.dirty\n\n"},{"id":"546148","messageId":"20260622-pks-connected-generic-promisor-checks-v1-3-25eba2698202@pks.im","threadId":"65851","inReplyTo":"20260622-pks-connected-generic-promisor-checks-v1-0-25eba2698202@pks.im","subject":"[PATCH 3/3] connected: search promisor objects generically","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-06-22T08:49:29Z","receivedAt":"2026-06-22T08:49:43Z","isPatch":true,"body":"When performing connectivity checks we have to figure out whether any of\nthe new objects are promisor objects, as we cannot assume full\nconnectivity if so.\n\nThis check is performed by iterating through all packfiles in the\nrepository and searching each of them for the given object. Of course,\nthis mechanism is quite specific to implementation details of the object\ndatabase, as we assume that it uses packfiles in the first place.\n\nRefactor the logic so that we instead use `odb_for_each_object_ext()`\nwith an object prefix filter and the `ODB_FOR_EACH_OBJECT_PROMISOR_ONLY`\nflag. This will yield all objects that have the exact object name and\nthat are part of a promisor pack in a generic way.\n\nSigned-off-by: Patrick Steinhardt \n---\n connected.c | 39 +++++++++++++++++++++++++--------------\n 1 file changed, 25 insertions(+), 14 deletions(-)\n\ndiff --git a/connected.c b/connected.c\nindex 7e26976832..9a666f0cdf 100644\n--- a/connected.c\n+++ b/connected.c\n@@ -11,6 +11,13 @@\n #include \"packfile.h\"\n #include \"promisor-remote.h\"\n \n+static int promised_object_cb(const struct object_id *oid UNUSED,\n+\t\t\t struct object_info *oi UNUSED,\n+\t\t\t void *payload UNUSED)\n+{\n+\treturn 1;\n+}\n+\n /*\n * If we feed all the commits we want to verify to this command\n *\n@@ -46,6 +53,11 @@ int check_connected(oid_iterate_fn fn, void *cb_data,\n \t}\n \n \tif (repo_has_promisor_remote(the_repository)) {\n+\t\tstruct odb_for_each_object_options opts = {\n+\t\t\t.flags = ODB_FOR_EACH_OBJECT_PROMISOR_ONLY,\n+\t\t\t.prefix_hex_len = the_repository->hash_algo->hexsz,\n+\t\t};\n+\n \t\t/*\n \t\t * For partial clones, we don't want to have to do a regular\n \t\t * connectivity check because we have to enumerate and exclude\n@@ -54,31 +66,30 @@ int check_connected(oid_iterate_fn fn, void *cb_data,\n \t\t * object is a promisor object. Instead, just make sure we\n \t\t * received, in a promisor packfile, the objects pointed to by\n \t\t * each wanted ref.\n-\t\t *\n-\t\t * Before checking for promisor packs, be sure we have the\n-\t\t * latest pack-files loaded into memory.\n \t\t */\n-\t\todb_reprepare(the_repository->objects);\n \t\tdo {\n-\t\t\tstruct packed_git *p;\n-\n-\t\t\trepo_for_each_pack(the_repository, p) {\n-\t\t\t\tif (!p->pack_promisor)\n-\t\t\t\t\tcontinue;\n-\t\t\t\tif (find_pack_entry_one(oid, p))\n-\t\t\t\t\tgoto promisor_pack_found;\n+\t\t\topts.prefix = oid;\n+\n+\t\t\terr = odb_for_each_object_ext(the_repository->objects,\n+\t\t\t\t\t\t NULL, promised_object_cb,\n+\t\t\t\t\t\t NULL, &opts);\n+\t\t\tif (err < 0)\n+\t\t\t\tbreak;\n+\t\t\tif (err > 0) {\n+\t\t\t\terr = 0;\n+\t\t\t\tcontinue;\n \t\t\t}\n+\n \t\t\t/*\n \t\t\t * Fallback to rev-list with oid and the rest of the\n \t\t\t * object IDs provided by fn.\n \t\t\t */\n \t\t\tgoto no_promisor_pack_found;\n-promisor_pack_found:\n-\t\t\t;\n \t\t} while ((oid = fn(cb_data)) != NULL);\n+\n \t\tif (opt->err_fd)\n \t\t\tclose(opt->err_fd);\n-\t\treturn 0;\n+\t\treturn err;\n \t}\n \n no_promisor_pack_found:\n\n-- \n2.55.0.rc1.745.g43192e7977.dirty\n\n"},{"id":"546196","messageId":"xmqqa4sm1n19.fsf@gitster.g","threadId":"65851","inReplyTo":"20260622-pks-connected-generic-promisor-checks-v1-1-25eba2698202@pks.im","subject":"Re: [PATCH 1/3] odb/source-packed: extract logic to skip certain packs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-06-22T17:51:30Z","receivedAt":"2026-06-22T17:51:33Z","isPatch":true,"body":"Patrick Steinhardt writes:\n\n> The caller can pass flags that allow them to filter out specific kinds\n> of objects when iterating objects via `odb_for_each_object()`. This only\n> works for \"normal\" iteration though, as we `BUG()` when the user passes\n> flags and specifies an object prefix.\n>\n> This limitation will be lifted in the next commit. Prepare for this by\n> extracting the logic that skips certain kinds of packs so that we can\n> easily reuse it.\n>\n> Signed-off-by: Patrick Steinhardt \n> ---\n> odb/source-packed.c | 28 ++++++++++++++++++----------\n> 1 file changed, 18 insertions(+), 10 deletions(-)\n\nQuite straight-forward creation of a simple helper function.\n\n> diff --git a/odb/source-packed.c b/odb/source-packed.c\n> index 42c28fba0e..3afc4bf01f 100644\n> --- a/odb/source-packed.c\n> +++ b/odb/source-packed.c\n> @@ -126,6 +126,22 @@ static int match_hash(unsigned len, const unsigned char *a, const unsigned char\n> \treturn 1;\n> }\n> \n> +static bool should_exclude_pack(struct packed_git *p, enum odb_for_each_object_flags flags)\n> +{\n> +\tif ((flags & ODB_FOR_EACH_OBJECT_LOCAL_ONLY) && !p->pack_local)\n> +\t\treturn true;\n> +\tif ((flags & ODB_FOR_EACH_OBJECT_PROMISOR_ONLY) &&\n> +\t !p->pack_promisor)\n> +\t\treturn true;\n> +\tif ((flags & ODB_FOR_EACH_OBJECT_SKIP_IN_CORE_KEPT_PACKS) &&\n> +\t p->pack_keep_in_core)\n> +\t\treturn true;\n> +\tif ((flags & ODB_FOR_EACH_OBJECT_SKIP_ON_DISK_KEPT_PACKS) &&\n> +\t p->pack_keep)\n> +\t\treturn true;\n> +\treturn false;\n> +}\n> +\n> static int for_each_prefixed_object_in_midx(\n> \tstruct odb_source_packed *store,\n> \tstruct multi_pack_index *m,\n> @@ -306,17 +322,9 @@ static int odb_source_packed_for_each_object(struct odb_source *source,\n> \tfor (e = packfile_store_get_packs(packed); e; e = e->next) {\n> \t\tstruct packed_git *p = e->pack;\n> \n> -\t\tif ((opts->flags & ODB_FOR_EACH_OBJECT_LOCAL_ONLY) && !p->pack_local)\n> -\t\t\tcontinue;\n> -\t\tif ((opts->flags & ODB_FOR_EACH_OBJECT_PROMISOR_ONLY) &&\n> -\t\t !p->pack_promisor)\n> -\t\t\tcontinue;\n> -\t\tif ((opts->flags & ODB_FOR_EACH_OBJECT_SKIP_IN_CORE_KEPT_PACKS) &&\n> -\t\t p->pack_keep_in_core)\n> -\t\t\tcontinue;\n> -\t\tif ((opts->flags & ODB_FOR_EACH_OBJECT_SKIP_ON_DISK_KEPT_PACKS) &&\n> -\t\t p->pack_keep)\n> +\t\tif (should_exclude_pack(p, opts->flags))\n> \t\t\tcontinue;\n> +\n> \t\tif (open_pack_index(p)) {\n> \t\t\tpack_errors = 1;\n> \t\t\tcontinue;\n"},{"id":"546197","messageId":"xmqq4iiu1mrt.fsf@gitster.g","threadId":"65851","inReplyTo":"20260622-pks-connected-generic-promisor-checks-v1-3-25eba2698202@pks.im","subject":"Re: [PATCH 3/3] connected: search promisor objects generically","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-06-22T17:57:10Z","receivedAt":"2026-06-22T17:57:13Z","isPatch":true,"body":"Patrick Steinhardt writes:\n\n> When performing connectivity checks we have to figure out whether any of\n> the new objects are promisor objects, as we cannot assume full\n> connectivity if so.\n>\n> This check is performed by iterating through all packfiles in the\n> repository and searching each of them for the given object. Of course,\n> this mechanism is quite specific to implementation details of the object\n> database, as we assume that it uses packfiles in the first place.\n>\n> Refactor the logic so that we instead use `odb_for_each_object_ext()`\n> with an object prefix filter and the `ODB_FOR_EACH_OBJECT_PROMISOR_ONLY`\n> flag. This will yield all objects that have the exact object name and\n> that are part of a promisor pack in a generic way.\n> ...\n\n\n> -\t\t *\n> -\t\t * Before checking for promisor packs, be sure we have the\n> -\t\t * latest pack-files loaded into memory.\n> \t\t */\n> -\t\todb_reprepare(the_repository->objects);\n\nHmph?\n\n> \t\tdo {\n> -\t\t\tstruct packed_git *p;\n> -\n> -\t\t\trepo_for_each_pack(the_repository, p) {\n> -\t\t\t\tif (!p->pack_promisor)\n> -\t\t\t\t\tcontinue;\n> -\t\t\t\tif (find_pack_entry_one(oid, p))\n> -\t\t\t\t\tgoto promisor_pack_found;\n> +\t\t\topts.prefix = oid;\n> +\n> +\t\t\terr = odb_for_each_object_ext(the_repository->objects,\n> +\t\t\t\t\t\t NULL, promised_object_cb,\n> +\t\t\t\t\t\t NULL, &opts);\n> +\t\t\tif (err < 0)\n> +\t\t\t\tbreak;\n> +\t\t\tif (err > 0) {\n> +\t\t\t\terr = 0;\n> +\t\t\t\tcontinue;\n> \t\t\t}\n\nSo we used to manually iterate and stop when we have a matching pack\nentry, but now \"stop when we find\" is done by promisor_object_cb\ncallback that returns 1.\n\nWhat is the reason why we no longer odb_(re)prepare() upfront before\ngoing into the loop? Would it make us miss a newly added promisor\npacks? We will fall back to rev-list for correctness, so it may not\nmatter, though.\n\n> +\n> \t\t\t/*\n> \t\t\t * Fallback to rev-list with oid and the rest of the\n> \t\t\t * object IDs provided by fn.\n> \t\t\t */\n> \t\t\tgoto no_promisor_pack_found;\n> -promisor_pack_found:\n> -\t\t\t;\n> \t\t} while ((oid = fn(cb_data)) != NULL);\n> +\n> \t\tif (opt->err_fd)\n> \t\t\tclose(opt->err_fd);\n> -\t\treturn 0;\n> +\t\treturn err;\n> \t}\n> \n> no_promisor_pack_found:\n"},{"id":"546224","messageId":"CAP8UFD1tqBBRiJV18xBMcDDT4Q7xCkqOLrtJGAO7o4oA=-Vr=w@mail.gmail.com","threadId":"65851","inReplyTo":"20260622-pks-connected-generic-promisor-checks-v1-3-25eba2698202@pks.im","subject":"Re: [PATCH 3/3] connected: search promisor objects generically","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2026-06-23T07:45:44Z","receivedAt":"2026-06-23T07:45:57Z","isPatch":true,"body":"On Mon, Jun 22, 2026 at 10:50 AM Patrick Steinhardt wrote:\n>\n> When performing connectivity checks we have to figure out whether any of\n> the new objects are promisor objects, as we cannot assume full\n> connectivity if so.\n>\n> This check is performed by iterating through all packfiles in the\n> repository and searching each of them for the given object. Of course,\n> this mechanism is quite specific to implementation details of the object\n> database, as we assume that it uses packfiles in the first place.\n>\n> Refactor the logic so that we instead use `odb_for_each_object_ext()`\n> with an object prefix filter and the `ODB_FOR_EACH_OBJECT_PROMISOR_ONLY`\n> flag. This will yield all objects that have the exact object name and\n> that are part of a promisor pack in a generic way.\n>\n> Signed-off-by: Patrick Steinhardt \n> ---\n> connected.c | 39 +++++++++++++++++++++++++--------------\n> 1 file changed, 25 insertions(+), 14 deletions(-)\n>\n> diff --git a/connected.c b/connected.c\n> index 7e26976832..9a666f0cdf 100644\n> --- a/connected.c\n> +++ b/connected.c\n> @@ -11,6 +11,13 @@\n> #include \"packfile.h\"\n> #include \"promisor-remote.h\"\n>\n> +static int promised_object_cb(const struct object_id *oid UNUSED,\n> + struct object_info *oi UNUSED,\n> + void *payload UNUSED)\n> +{\n> + return 1;\n> +}\n> +\n> /*\n> * If we feed all the commits we want to verify to this command\n> *\n> @@ -46,6 +53,11 @@ int check_connected(oid_iterate_fn fn, void *cb_data,\n> }\n>\n> if (repo_has_promisor_remote(the_repository)) {\n> + struct odb_for_each_object_options opts = {\n> + .flags = ODB_FOR_EACH_OBJECT_PROMISOR_ONLY,\n> + .prefix_hex_len = the_repository->hash_algo->hexsz,\n> + };\n> +\n> /*\n> * For partial clones, we don't want to have to do a regular\n> * connectivity check because we have to enumerate and exclude\n> @@ -54,31 +66,30 @@ int check_connected(oid_iterate_fn fn, void *cb_data,\n> * object is a promisor object. Instead, just make sure we\n> * received, in a promisor packfile, the objects pointed to by\n> * each wanted ref.\n> - *\n> - * Before checking for promisor packs, be sure we have the\n> - * latest pack-files loaded into memory.\n> */\n> - odb_reprepare(the_repository->objects);\n\nLike Junio, I am not sure it's correct to remove the\n`odb_reprepare(the_repository->objects)` call.\n\nI think it was added for good reasons in b739d971 (connected.c:\nreprepare packs for corner cases, 2020-03-13) and I am not sure\nodb_for_each_object_ext() is performing something similar.\n\nAt least the commit message should mention this change and explain a\nbit why the reasons the call was added are not valid anymore.\n\n> do {\n> - struct packed_git *p;\n> -\n> - repo_for_each_pack(the_repository, p) {\n> - if (!p->pack_promisor)\n> - continue;\n> - if (find_pack_entry_one(oid, p))\n> - goto promisor_pack_found;\n> + opts.prefix = oid;\n> +\n> + err = odb_for_each_object_ext(the_repository->objects,\n> + NULL, promised_object_cb,\n> + NULL, &opts);\n> + if (err < 0)\n> + break;\n> + if (err > 0) {\n> + err = 0;\n> + continue;\n> }\n> +\n> /*\n> * Fallback to rev-list with oid and the rest of the\n> * object IDs provided by fn.\n> */\n> goto no_promisor_pack_found;\n> -promisor_pack_found:\n> - ;\n> } while ((oid = fn(cb_data)) != NULL);\n> +\n> if (opt->err_fd)\n> close(opt->err_fd);\n> - return 0;\n> + return err;\n> }\n>\n> no_promisor_pack_found:\n\nThese changes are difficult to understand as there are a number of\n`goto`, `break`, `return`, etc involved.\n\nI think it comes in the first place from check_connected() doing too\nmany things, and adding a preparatory commit to refactor it would\nhelp.\n\nFor example the preparatory commit could move a lot of code from\ncheck_connected() to the following new functions:\n\n/*\n * Returns:\n * 1 = all wanted OIDs found in promisor packs: connected, done.\n * 0 = at least one OID not found: caller must fall back to rev-list.\n * <0 = error.\n * On the fallback (0) return, *oid is left pointing at the first\n * not-found OID so the rev-list path can resume the iteration.\n */\nstatic int check_connected_promisor(oid_iterate_fn fn, void *cb_data,\n const struct object_id **oid);\n\n/*\n * In a non-promisor repo, pass the first OID as `oid`.\n * Otherwise pass the first not-found OID resumed from\n * check_connected_promisor() as `oid`.\n */\nstatic int check_connected_rev_list(oid_iterate_fn fn, void *cb_data,\n struct check_connected_options *opt,\n const struct object_id *oid);\n"},{"id":"546277","messageId":"ajukWExVvpz8NFEc@pks.im","threadId":"65851","inReplyTo":"xmqq4iiu1mrt.fsf@gitster.g","subject":"Re: [PATCH 3/3] connected: search promisor objects generically","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-06-24T09:33:12Z","receivedAt":"2026-06-24T09:33:25Z","isPatch":true,"body":"On Mon, Jun 22, 2026 at 10:57:10AM -0700, Junio C Hamano wrote:\n> Patrick Steinhardt writes:\n[snip]\n> > -\t\t *\n> > -\t\t * Before checking for promisor packs, be sure we have the\n> > -\t\t * latest pack-files loaded into memory.\n> > \t\t */\n> > -\t\todb_reprepare(the_repository->objects);\n> \n> Hmph?\n\nOh, I think I completely misread this as `odb_alternates_prepare()`,\nwhich is something you typically see before loops like this. By using a\nhelper like `odb_for_each_object_ext()` we of course wouldn't have to\ncall that function anymore.\n\nBut this here is of course different, as this call would also cause us\nto reload packfiles and loose objects.\n\n> > \t\tdo {\n> > -\t\t\tstruct packed_git *p;\n> > -\n> > -\t\t\trepo_for_each_pack(the_repository, p) {\n> > -\t\t\t\tif (!p->pack_promisor)\n> > -\t\t\t\t\tcontinue;\n> > -\t\t\t\tif (find_pack_entry_one(oid, p))\n> > -\t\t\t\t\tgoto promisor_pack_found;\n> > +\t\t\topts.prefix = oid;\n> > +\n> > +\t\t\terr = odb_for_each_object_ext(the_repository->objects,\n> > +\t\t\t\t\t\t NULL, promised_object_cb,\n> > +\t\t\t\t\t\t NULL, &opts);\n> > +\t\t\tif (err < 0)\n> > +\t\t\t\tbreak;\n> > +\t\t\tif (err > 0) {\n> > +\t\t\t\terr = 0;\n> > +\t\t\t\tcontinue;\n> > \t\t\t}\n> \n> So we used to manually iterate and stop when we have a matching pack\n> entry, but now \"stop when we find\" is done by promisor_object_cb\n> callback that returns 1.\n> \n> What is the reason why we no longer odb_(re)prepare() upfront before\n> going into the loop? Would it make us miss a newly added promisor\n> packs? We will fall back to rev-list for correctness, so it may not\n> matter, though.\n\nSo yes, this is a bug.\n\nThanks!\n\nPatrick\n"},{"id":"546278","messageId":"ajukZGjCzg8E2U7E@pks.im","threadId":"65851","inReplyTo":"CAP8UFD1tqBBRiJV18xBMcDDT4Q7xCkqOLrtJGAO7o4oA=-Vr=w@mail.gmail.com","subject":"Re: [PATCH 3/3] connected: search promisor objects generically","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-06-24T09:33:24Z","receivedAt":"2026-06-24T09:33:30Z","isPatch":true,"body":"On Tue, Jun 23, 2026 at 09:45:44AM +0200, Christian Couder wrote:\n> On Mon, Jun 22, 2026 at 10:50 AM Patrick Steinhardt wrote:\n> > diff --git a/connected.c b/connected.c\n> > index 7e26976832..9a666f0cdf 100644\n> > --- a/connected.c\n> > +++ b/connected.c\n> > @@ -54,31 +66,30 @@ int check_connected(oid_iterate_fn fn, void *cb_data,\n> > * object is a promisor object. Instead, just make sure we\n> > * received, in a promisor packfile, the objects pointed to by\n> > * each wanted ref.\n> > - *\n> > - * Before checking for promisor packs, be sure we have the\n> > - * latest pack-files loaded into memory.\n> > */\n> > - odb_reprepare(the_repository->objects);\n> \n> Like Junio, I am not sure it's correct to remove the\n> `odb_reprepare(the_repository->objects)` call.\n> \n> I think it was added for good reasons in b739d971 (connected.c:\n> reprepare packs for corner cases, 2020-03-13) and I am not sure\n> odb_for_each_object_ext() is performing something similar.\n> \n> At least the commit message should mention this change and explain a\n> bit why the reasons the call was added are not valid anymore.\n\nYeah, I think you're both correct. The only explanation I have is that I\nmight have repeatedly misread this as `odb_prepare_alternates()`, which\nis something we often call before suck loops.\n\n> > do {\n> > - struct packed_git *p;\n> > -\n> > - repo_for_each_pack(the_repository, p) {\n> > - if (!p->pack_promisor)\n> > - continue;\n> > - if (find_pack_entry_one(oid, p))\n> > - goto promisor_pack_found;\n> > + opts.prefix = oid;\n> > +\n> > + err = odb_for_each_object_ext(the_repository->objects,\n> > + NULL, promised_object_cb,\n> > + NULL, &opts);\n> > + if (err < 0)\n> > + break;\n> > + if (err > 0) {\n> > + err = 0;\n> > + continue;\n> > }\n> > +\n> > /*\n> > * Fallback to rev-list with oid and the rest of the\n> > * object IDs provided by fn.\n> > */\n> > goto no_promisor_pack_found;\n> > -promisor_pack_found:\n> > - ;\n> > } while ((oid = fn(cb_data)) != NULL);\n> > +\n> > if (opt->err_fd)\n> > close(opt->err_fd);\n> > - return 0;\n> > + return err;\n> > }\n> >\n> > no_promisor_pack_found:\n> \n> These changes are difficult to understand as there are a number of\n> `goto`, `break`, `return`, etc involved.\n\nYeah, agreed. I had my issues understanding this logic, too.\n\n> I think it comes in the first place from check_connected() doing too\n> many things, and adding a preparatory commit to refactor it would\n> help.\n> \n> For example the preparatory commit could move a lot of code from\n> check_connected() to the following new functions:\n\nI'll give that a try, thanks!\n\nPatrick\n"},{"id":"546283","messageId":"20260624-pks-connected-generic-promisor-checks-v2-0-132d73ee47b9@pks.im","threadId":"65851","inReplyTo":"20260622-pks-connected-generic-promisor-checks-v1-0-25eba2698202@pks.im","subject":"[PATCH v2 0/4] connected: search promisor objects generically","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-06-24T10:37:02Z","receivedAt":"2026-06-24T10:37:12Z","isPatch":true,"body":"Hi,\n\nthis patch series refactors \"connected.c\" so that we search for promisor\nobjects in a generic way instead of reaching into internal of the object\ndatabase. As a result, the connectivity checks will work properly in\nrepos that don't use packfiles in the first place.\n\nThe series is built on top of 8d96f09e92 (Merge branch\n'js/objects-larger-than-4gb-on-windows', 2026-06-19) with\nps/odb-source-packed at 1bba3c035d (odb/source-packed: drop pointer to\n\"files\" parent source, 2026-06-17) merged into it.\n\nChanges in v2:\n - Fix the accidentally-dropped call to `odb_reprepare()`.\n - Add a preparatory commit that splits out `check_connected_promisor()`.\n I think also splitting out `check_connected_rev_list()` would only\n have diminishing returns, so I skipped that part.\n - Link to v1: https://patch.msgid.link/20260622-pks-connected-generic-promisor-checks-v1-0-25eba2698202@pks.im\n\nThanks!\n\nPatrick\n\n---\nPatrick Steinhardt (4):\n odb/source-packed: extract logic to skip certain packs\n odb/source-packed: support flags when iterating an object prefix\n connected: split out promisor-based connectivity check\n connected: search promisor objects generically\n\n connected.c | 95 ++++++++++++++++++++++++++++++++++-------------------\n odb/source-packed.c | 50 ++++++++++++++++++++--------\n 2 files changed, 98 insertions(+), 47 deletions(-)\n\nRange-diff versus v1:\n\n1: 6ff1fc8d89 = 1: a1a1af0fc6 odb/source-packed: extract logic to skip certain packs\n2: 1022a1fdcc = 2: bd81a9e478 odb/source-packed: support flags when iterating an object prefix\n3: 102fab7df2 < -: ---------- connected: search promisor objects generically\n-: ---------- > 3: f39ef68c3e connected: split out promisor-based connectivity check\n-: ---------- > 4: 558f30a6f2 connected: search promisor objects generically\n\n---\nbase-commit: 4a8e7a446f41435e157131162dfe901eca9250fe\nchange-id: 20260612-pks-connected-generic-promisor-checks-2933bff3028d\n\n"},{"id":"546284","messageId":"20260624-pks-connected-generic-promisor-checks-v2-1-132d73ee47b9@pks.im","threadId":"65851","inReplyTo":"20260624-pks-connected-generic-promisor-checks-v2-0-132d73ee47b9@pks.im","subject":"[PATCH v2 1/4] odb/source-packed: extract logic to skip certain packs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-06-24T10:37:03Z","receivedAt":"2026-06-24T10:37:13Z","isPatch":true,"body":"The caller can pass flags that allow them to filter out specific kinds\nof objects when iterating objects via `odb_for_each_object()`. This only\nworks for \"normal\" iteration though, as we `BUG()` when the user passes\nflags and specifies an object prefix.\n\nThis limitation will be lifted in the next commit. Prepare for this by\nextracting the logic that skips certain kinds of packs so that we can\neasily reuse it.\n\nSigned-off-by: Patrick Steinhardt \n---\n odb/source-packed.c | 28 ++++++++++++++++++----------\n 1 file changed, 18 insertions(+), 10 deletions(-)\n\ndiff --git a/odb/source-packed.c b/odb/source-packed.c\nindex 42c28fba0e..3afc4bf01f 100644\n--- a/odb/source-packed.c\n+++ b/odb/source-packed.c\n@@ -126,6 +126,22 @@ static int match_hash(unsigned len, const unsigned char *a, const unsigned char\n \treturn 1;\n }\n \n+static bool should_exclude_pack(struct packed_git *p, enum odb_for_each_object_flags flags)\n+{\n+\tif ((flags & ODB_FOR_EACH_OBJECT_LOCAL_ONLY) && !p->pack_local)\n+\t\treturn true;\n+\tif ((flags & ODB_FOR_EACH_OBJECT_PROMISOR_ONLY) &&\n+\t !p->pack_promisor)\n+\t\treturn true;\n+\tif ((flags & ODB_FOR_EACH_OBJECT_SKIP_IN_CORE_KEPT_PACKS) &&\n+\t p->pack_keep_in_core)\n+\t\treturn true;\n+\tif ((flags & ODB_FOR_EACH_OBJECT_SKIP_ON_DISK_KEPT_PACKS) &&\n+\t p->pack_keep)\n+\t\treturn true;\n+\treturn false;\n+}\n+\n static int for_each_prefixed_object_in_midx(\n \tstruct odb_source_packed *store,\n \tstruct multi_pack_index *m,\n@@ -306,17 +322,9 @@ static int odb_source_packed_for_each_object(struct odb_source *source,\n \tfor (e = packfile_store_get_packs(packed); e; e = e->next) {\n \t\tstruct packed_git *p = e->pack;\n \n-\t\tif ((opts->flags & ODB_FOR_EACH_OBJECT_LOCAL_ONLY) && !p->pack_local)\n-\t\t\tcontinue;\n-\t\tif ((opts->flags & ODB_FOR_EACH_OBJECT_PROMISOR_ONLY) &&\n-\t\t !p->pack_promisor)\n-\t\t\tcontinue;\n-\t\tif ((opts->flags & ODB_FOR_EACH_OBJECT_SKIP_IN_CORE_KEPT_PACKS) &&\n-\t\t p->pack_keep_in_core)\n-\t\t\tcontinue;\n-\t\tif ((opts->flags & ODB_FOR_EACH_OBJECT_SKIP_ON_DISK_KEPT_PACKS) &&\n-\t\t p->pack_keep)\n+\t\tif (should_exclude_pack(p, opts->flags))\n \t\t\tcontinue;\n+\n \t\tif (open_pack_index(p)) {\n \t\t\tpack_errors = 1;\n \t\t\tcontinue;\n\n-- \n2.55.0.rc1.745.g43192e7977.dirty\n\n"},{"id":"546285","messageId":"20260624-pks-connected-generic-promisor-checks-v2-2-132d73ee47b9@pks.im","threadId":"65851","inReplyTo":"20260624-pks-connected-generic-promisor-checks-v2-0-132d73ee47b9@pks.im","subject":"[PATCH v2 2/4] odb/source-packed: support flags when iterating an object prefix","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-06-24T10:37:04Z","receivedAt":"2026-06-24T10:37:15Z","isPatch":true,"body":"Callers of `odb_for_each_object()` can specify an optional object name\nprefix so that we only yield objects that match it. This is incompatible\nthough with passing flags at the same time, as we don't yet know to\nhandle them.\n\nLoosen this restriction by calling `should_exclude_pack()`.\n\nSigned-off-by: Patrick Steinhardt \n---\n odb/source-packed.c | 22 +++++++++++++++++++---\n 1 file changed, 19 insertions(+), 3 deletions(-)\n\ndiff --git a/odb/source-packed.c b/odb/source-packed.c\nindex 3afc4bf01f..6f31f0ff94 100644\n--- a/odb/source-packed.c\n+++ b/odb/source-packed.c\n@@ -148,6 +148,7 @@ static int for_each_prefixed_object_in_midx(\n \tconst struct odb_for_each_object_options *opts,\n \tstruct odb_source_packed_for_each_object_wrapper_data *data)\n {\n+\tbool pack_errors = false;\n \tint ret;\n \n \tfor (; m; m = m->base_midx) {\n@@ -171,6 +172,20 @@ static int for_each_prefixed_object_in_midx(\n \t\t\tconst struct object_id *current = NULL;\n \t\t\tstruct object_id oid;\n \n+\t\t\tif (opts->flags) {\n+\t\t\t\tuint32_t pack_id = nth_midxed_pack_int_id(m, i);\n+\t\t\t\tstruct packed_git *pack;\n+\n+\t\t\t\tif (prepare_midx_pack(m, pack_id)) {\n+\t\t\t\t\tpack_errors = true;\n+\t\t\t\t\tcontinue;\n+\t\t\t\t}\n+\n+\t\t\t\tpack = nth_midxed_pack(m, pack_id);\n+\t\t\t\tif (should_exclude_pack(pack, opts->flags))\n+\t\t\t\t\tcontinue;\n+\t\t\t}\n+\n \t\t\tcurrent = nth_midxed_object_oid(&oid, m, i);\n \n \t\t\tif (!match_hash(len, opts->prefix->hash, current->hash))\n@@ -198,6 +213,8 @@ static int for_each_prefixed_object_in_midx(\n \tret = 0;\n \n out:\n+\tif (!ret && pack_errors)\n+\t\tret = -1;\n \treturn ret;\n }\n \n@@ -260,9 +277,6 @@ static int odb_source_packed_for_each_prefixed_object(\n \tbool pack_errors = false;\n \tint ret;\n \n-\tif (opts->flags)\n-\t\tBUG(\"flags unsupported\");\n-\n \tstore->skip_mru_updates = true;\n \n \tm = get_multi_pack_index(store);\n@@ -275,6 +289,8 @@ static int odb_source_packed_for_each_prefixed_object(\n \tfor (e = packfile_store_get_packs(store); e; e = e->next) {\n \t\tif (e->pack->multi_pack_index)\n \t\t\tcontinue;\n+\t\tif (should_exclude_pack(e->pack, opts->flags))\n+\t\t\tcontinue;\n \n \t\tif (open_pack_index(e->pack)) {\n \t\t\tpack_errors = true;\n\n-- \n2.55.0.rc1.745.g43192e7977.dirty\n\n"},{"id":"546286","messageId":"20260624-pks-connected-generic-promisor-checks-v2-3-132d73ee47b9@pks.im","threadId":"65851","inReplyTo":"20260624-pks-connected-generic-promisor-checks-v2-0-132d73ee47b9@pks.im","subject":"[PATCH v2 3/4] connected: split out promisor-based connectivity check","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-06-24T10:37:05Z","receivedAt":"2026-06-24T10:37:18Z","isPatch":true,"body":"When performing a connectivity check in a partial clone we try to avoid\ndoing the connectivity check by checking whether all new tips are part\nof a promisor pack. This makes use of the fact that we don't expect full\nconnectivity for promised objects anyway, so it's basically fine if\nthose objects are not fully connected.\n\nThe logic that handles this promisor-based check is somewhat hard to\nread though as it uses nested loops and gotos. Pull it out into a\nstandalone function, which makes it a bit easier to reason about.\n\nWe'll also further simplify the function in the next commit.\n\nSuggested-by: Christian Couder \nSigned-off-by: Patrick Steinhardt \n---\n connected.c | 85 ++++++++++++++++++++++++++++++++++++-------------------------\n 1 file changed, 51 insertions(+), 34 deletions(-)\n\ndiff --git a/connected.c b/connected.c\nindex 7e26976832..d2b334173f 100644\n--- a/connected.c\n+++ b/connected.c\n@@ -11,6 +11,49 @@\n #include \"packfile.h\"\n #include \"promisor-remote.h\"\n \n+/*\n+ * For partial clones, we don't want to have to do a regular connectivity check\n+ * because we have to enumerate and exclude all promisor objects (slow), and\n+ * then the connectivity check itself becomes a no-op because in a partial\n+ * clone every object is a promisor object. Instead, just make sure we\n+ * received, in a promisor packfile, the objects pointed to by each wanted ref.\n+ *\n+ * Before checking for promisor packs, be sure we have the latest pack-files\n+ * loaded into memory.\n+ *\n+ * Returns 1 when all object IDs have been found in promisor packs, in which\n+ * case we're fully connected and thus done. Returns 0 when we have found\n+ * objects in non-promisor packs, in which case we'll have to fall back to the\n+ * rev-list-based connectivity checks. Returns a negative error code on error.\n+ */\n+static int check_connected_promisor(oid_iterate_fn fn,\n+\t\t\t\t void *cb_data,\n+\t\t\t\t const struct object_id **oid)\n+{\n+\todb_reprepare(the_repository->objects);\n+\tdo {\n+\t\tstruct packed_git *p;\n+\n+\t\trepo_for_each_pack(the_repository, p) {\n+\t\t\tif (!p->pack_promisor)\n+\t\t\t\tcontinue;\n+\t\t\tif (find_pack_entry_one(*oid, p))\n+\t\t\t\tgoto promisor_pack_found;\n+\t\t}\n+\n+\t\t/*\n+\t\t * We have found an object that is not part of a promisor pack,\n+\t\t * and thus we cannot skip the full connectivity check.\n+\t\t */\n+\t\treturn 0;\n+\n+promisor_pack_found:\n+\t\t;\n+\t} while ((*oid = fn(cb_data)) != NULL);\n+\n+\treturn 1;\n+}\n+\n /*\n * If we feed all the commits we want to verify to this command\n *\n@@ -46,42 +89,16 @@ int check_connected(oid_iterate_fn fn, void *cb_data,\n \t}\n \n \tif (repo_has_promisor_remote(the_repository)) {\n-\t\t/*\n-\t\t * For partial clones, we don't want to have to do a regular\n-\t\t * connectivity check because we have to enumerate and exclude\n-\t\t * all promisor objects (slow), and then the connectivity check\n-\t\t * itself becomes a no-op because in a partial clone every\n-\t\t * object is a promisor object. Instead, just make sure we\n-\t\t * received, in a promisor packfile, the objects pointed to by\n-\t\t * each wanted ref.\n-\t\t *\n-\t\t * Before checking for promisor packs, be sure we have the\n-\t\t * latest pack-files loaded into memory.\n-\t\t */\n-\t\todb_reprepare(the_repository->objects);\n-\t\tdo {\n-\t\t\tstruct packed_git *p;\n-\n-\t\t\trepo_for_each_pack(the_repository, p) {\n-\t\t\t\tif (!p->pack_promisor)\n-\t\t\t\t\tcontinue;\n-\t\t\t\tif (find_pack_entry_one(oid, p))\n-\t\t\t\t\tgoto promisor_pack_found;\n-\t\t\t}\n-\t\t\t/*\n-\t\t\t * Fallback to rev-list with oid and the rest of the\n-\t\t\t * object IDs provided by fn.\n-\t\t\t */\n-\t\t\tgoto no_promisor_pack_found;\n-promisor_pack_found:\n-\t\t\t;\n-\t\t} while ((oid = fn(cb_data)) != NULL);\n-\t\tif (opt->err_fd)\n-\t\t\tclose(opt->err_fd);\n-\t\treturn 0;\n+\t\terr = check_connected_promisor(fn, cb_data, &oid);\n+\t\tif (err) {\n+\t\t\tif (opt->err_fd)\n+\t\t\t\tclose(opt->err_fd);\n+\t\t\tif (err > 0)\n+\t\t\t\terr = 0;\n+\t\t\treturn err;\n+\t\t}\n \t}\n \n-no_promisor_pack_found:\n \tif (opt->shallow_file) {\n \t\tstrvec_push(&rev_list.args, \"--shallow-file\");\n \t\tstrvec_push(&rev_list.args, opt->shallow_file);\n\n-- \n2.55.0.rc1.745.g43192e7977.dirty\n\n"},{"id":"546287","messageId":"20260624-pks-connected-generic-promisor-checks-v2-4-132d73ee47b9@pks.im","threadId":"65851","inReplyTo":"20260624-pks-connected-generic-promisor-checks-v2-0-132d73ee47b9@pks.im","subject":"[PATCH v2 4/4] connected: search promisor objects generically","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-06-24T10:37:06Z","receivedAt":"2026-06-24T10:37:20Z","isPatch":true,"body":"When performing connectivity checks we have to figure out whether any of\nthe new objects are promisor objects, as we cannot assume full\nconnectivity if so.\n\nThis check is performed by iterating through all packfiles in the\nrepository and searching each of them for the given object. Of course,\nthis mechanism is quite specific to implementation details of the object\ndatabase, as we assume that it uses packfiles in the first place.\n\nRefactor the logic so that we instead use `odb_for_each_object_ext()`\nwith an object prefix filter and the `ODB_FOR_EACH_OBJECT_PROMISOR_ONLY`\nflag. This will yield all objects that have the exact object name and\nthat are part of a promisor pack in a generic way.\n\nSigned-off-by: Patrick Steinhardt \n---\n connected.c | 32 +++++++++++++++++++++-----------\n 1 file changed, 21 insertions(+), 11 deletions(-)\n\ndiff --git a/connected.c b/connected.c\nindex d2b334173f..b557ff5db9 100644\n--- a/connected.c\n+++ b/connected.c\n@@ -11,6 +11,13 @@\n #include \"packfile.h\"\n #include \"promisor-remote.h\"\n \n+static int promised_object_cb(const struct object_id *oid UNUSED,\n+\t\t\t struct object_info *oi UNUSED,\n+\t\t\t void *payload UNUSED)\n+{\n+\treturn 1;\n+}\n+\n /*\n * For partial clones, we don't want to have to do a regular connectivity check\n * because we have to enumerate and exclude all promisor objects (slow), and\n@@ -30,25 +37,28 @@ static int check_connected_promisor(oid_iterate_fn fn,\n \t\t\t\t void *cb_data,\n \t\t\t\t const struct object_id **oid)\n {\n+\tstruct odb_for_each_object_options opts = {\n+\t\t.flags = ODB_FOR_EACH_OBJECT_PROMISOR_ONLY,\n+\t\t.prefix_hex_len = the_repository->hash_algo->hexsz,\n+\t};\n+\tint err;\n+\n \todb_reprepare(the_repository->objects);\n \tdo {\n-\t\tstruct packed_git *p;\n+\t\topts.prefix = *oid;\n \n-\t\trepo_for_each_pack(the_repository, p) {\n-\t\t\tif (!p->pack_promisor)\n-\t\t\t\tcontinue;\n-\t\t\tif (find_pack_entry_one(*oid, p))\n-\t\t\t\tgoto promisor_pack_found;\n-\t\t}\n+\t\terr = odb_for_each_object_ext(the_repository->objects,\n+\t\t\t\t\t NULL, promised_object_cb,\n+\t\t\t\t\t NULL, &opts);\n+\t\tif (err < 0)\n+\t\t\treturn err;\n \n \t\t/*\n \t\t * We have found an object that is not part of a promisor pack,\n \t\t * and thus we cannot skip the full connectivity check.\n \t\t */\n-\t\treturn 0;\n-\n-promisor_pack_found:\n-\t\t;\n+\t\tif (err > 0)\n+\t\t\treturn 0;\n \t} while ((*oid = fn(cb_data)) != NULL);\n \n \treturn 1;\n\n-- \n2.55.0.rc1.745.g43192e7977.dirty\n\n"},{"id":"546334","messageId":"xmqqjyrnkinn.fsf@gitster.g","threadId":"65851","inReplyTo":"20260624-pks-connected-generic-promisor-checks-v2-4-132d73ee47b9@pks.im","subject":"Re: [PATCH v2 4/4] connected: search promisor objects generically","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-06-24T16:27:56Z","receivedAt":"2026-06-24T16:27:59Z","isPatch":true,"body":"Patrick Steinhardt writes:\n\n> When performing connectivity checks we have to figure out whether any of\n> the new objects are promisor objects, as we cannot assume full\n> connectivity if so.\n>\n> This check is performed by iterating through all packfiles in the\n> repository and searching each of them for the given object. Of course,\n> this mechanism is quite specific to implementation details of the object\n> database, as we assume that it uses packfiles in the first place.\n>\n> Refactor the logic so that we instead use `odb_for_each_object_ext()`\n> with an object prefix filter and the `ODB_FOR_EACH_OBJECT_PROMISOR_ONLY`\n> flag. This will yield all objects that have the exact object name and\n> that are part of a promisor pack in a generic way.\n>\n> Signed-off-by: Patrick Steinhardt \n> ---\n> connected.c | 32 +++++++++++++++++++++-----------\n> 1 file changed, 21 insertions(+), 11 deletions(-)\n>\n> diff --git a/connected.c b/connected.c\n> index d2b334173f..b557ff5db9 100644\n> --- a/connected.c\n> +++ b/connected.c\n> @@ -11,6 +11,13 @@\n> #include \"packfile.h\"\n> #include \"promisor-remote.h\"\n> \n> +static int promised_object_cb(const struct object_id *oid UNUSED,\n> +\t\t\t struct object_info *oi UNUSED,\n> +\t\t\t void *payload UNUSED)\n> +{\n> +\treturn 1;\n> +}\n> +\n> /*\n> * For partial clones, we don't want to have to do a regular connectivity check\n> * because we have to enumerate and exclude all promisor objects (slow), and\n> @@ -30,25 +37,28 @@ static int check_connected_promisor(oid_iterate_fn fn,\n> \t\t\t\t void *cb_data,\n> \t\t\t\t const struct object_id **oid)\n> {\n> +\tstruct odb_for_each_object_options opts = {\n> +\t\t.flags = ODB_FOR_EACH_OBJECT_PROMISOR_ONLY,\n> +\t\t.prefix_hex_len = the_repository->hash_algo->hexsz,\n> +\t};\n> +\tint err;\n> +\n> \todb_reprepare(the_repository->objects);\n> \tdo {\n> -\t\tstruct packed_git *p;\n> +\t\topts.prefix = *oid;\n> \n> -\t\trepo_for_each_pack(the_repository, p) {\n> -\t\t\tif (!p->pack_promisor)\n> -\t\t\t\tcontinue;\n> -\t\t\tif (find_pack_entry_one(*oid, p))\n> -\t\t\t\tgoto promisor_pack_found;\n> -\t\t}\n> +\t\terr = odb_for_each_object_ext(the_repository->objects,\n> +\t\t\t\t\t NULL, promised_object_cb,\n> +\t\t\t\t\t NULL, &opts);\n\npromised_object_cb() returns 1 without any computation since we are\nonly interested in learning ODB_FOR_EACH_OBJECT_PROMISOR_ONLY finds\nany such object.\n\nodb_for_each_object_ext() returns 0 (if it iterates all the sources\nto the end), but if its call to odb_source_for_each_object() yields\nnon-zero value, the returned value comes back as \"err\" here,\nterminating the for-each iteration immediately.\n\nodb_source_for_each_object() is implemented differently per the\nsource backend, but taking an example of \"packfile\" backend,\npackfile_loose_for_each_object() ends up calling cb (wrapped in\npackfile_store_for_each_object_wrapper_data) via\nfor_each_object_in_pack(), which stops immediately when cb returns\nnon-zero and the value returned from there is the value given by cb,\ni.e., 1. So we will have err==1 when we find any object.\n\n> +\t\tif (err < 0)\n> +\t\t\treturn err;\n\nAnd err presumably is 1 in such a case, so this does not trigger.\n\n> \t\t/*\n> \t\t * We have found an object that is not part of a promisor pack,\n> \t\t * and thus we cannot skip the full connectivity check.\n> \t\t */\n> -\t\treturn 0;\n> -\n> -promisor_pack_found:\n> -\t\t;\n> +\t\tif (err > 0)\n> +\t\t\treturn 0;\n\nAnd this does.\n\nI may be misreading the patch, but as we return 0 from here, do we\ncause the caller to fall back to full connectivity check? The\ncaller, check_connected(), sees a zero returned from here.\n\n> \t} while ((*oid = fn(cb_data)) != NULL);\n> \n> \treturn 1;\n"},{"id":"546336","messageId":"CAP8UFD1sJNJbAAu9ZUanB8gJV-Vb64pLVkNULm3onSFZirdKxA@mail.gmail.com","threadId":"65851","inReplyTo":"20260624-pks-connected-generic-promisor-checks-v2-2-132d73ee47b9@pks.im","subject":"Re: [PATCH v2 2/4] odb/source-packed: support flags when iterating an object prefix","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2026-06-24T17:02:48Z","receivedAt":"2026-06-24T17:03:00Z","isPatch":true,"body":"On Wed, Jun 24, 2026 at 12:37 PM Patrick Steinhardt wrote:\n>\n> Callers of `odb_for_each_object()` can specify an optional object name\n> prefix so that we only yield objects that match it. This is incompatible\n> though with passing flags at the same time, as we don't yet know to\n> handle them.\n>\n> Loosen this restriction by calling `should_exclude_pack()`.\n>\n> Signed-off-by: Patrick Steinhardt \n> ---\n> odb/source-packed.c | 22 +++++++++++++++++++---\n> 1 file changed, 19 insertions(+), 3 deletions(-)\n>\n> diff --git a/odb/source-packed.c b/odb/source-packed.c\n> index 3afc4bf01f..6f31f0ff94 100644\n> --- a/odb/source-packed.c\n> +++ b/odb/source-packed.c\n> @@ -148,6 +148,7 @@ static int for_each_prefixed_object_in_midx(\n> const struct odb_for_each_object_options *opts,\n> struct odb_source_packed_for_each_object_wrapper_data *data)\n> {\n> + bool pack_errors = false;\n> int ret;\n>\n> for (; m; m = m->base_midx) {\n> @@ -171,6 +172,20 @@ static int for_each_prefixed_object_in_midx(\n> const struct object_id *current = NULL;\n> struct object_id oid;\n>\n> + if (opts->flags) {\n> + uint32_t pack_id = nth_midxed_pack_int_id(m, i);\n> + struct packed_git *pack;\n> +\n> + if (prepare_midx_pack(m, pack_id)) {\n> + pack_errors = true;\n> + continue;\n> + }\n> +\n> + pack = nth_midxed_pack(m, pack_id);\n> + if (should_exclude_pack(pack, opts->flags))\n> + continue;\n> + }\n> +\n> current = nth_midxed_object_oid(&oid, m, i);\n>\n> if (!match_hash(len, opts->prefix->hash, current->hash))\n\nIt looks like this is:\n\n if (!match_hash(len, opts->prefix->hash, current->hash))\n break;\n\nand I wonder if the `if (opts->flags) { ... }` block would be better\nafter that prefix check rather than before it.\n\nPutting it after the prefix check would make sure we don't continue\nwhen the prefix doesn't match.\n"},{"id":"546373","messageId":"ajzCDpviaL6EillJ@pks.im","threadId":"65851","inReplyTo":"CAP8UFD1sJNJbAAu9ZUanB8gJV-Vb64pLVkNULm3onSFZirdKxA@mail.gmail.com","subject":"Re: [PATCH v2 2/4] odb/source-packed: support flags when iterating an object prefix","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-06-25T05:52:14Z","receivedAt":"2026-06-25T05:52:26Z","isPatch":true,"body":"On Wed, Jun 24, 2026 at 07:02:48PM +0200, Christian Couder wrote:\n> On Wed, Jun 24, 2026 at 12:37 PM Patrick Steinhardt wrote:\n> > diff --git a/odb/source-packed.c b/odb/source-packed.c\n> > index 3afc4bf01f..6f31f0ff94 100644\n> > --- a/odb/source-packed.c\n> > +++ b/odb/source-packed.c\n> > @@ -171,6 +172,20 @@ static int for_each_prefixed_object_in_midx(\n> > const struct object_id *current = NULL;\n> > struct object_id oid;\n> >\n> > + if (opts->flags) {\n> > + uint32_t pack_id = nth_midxed_pack_int_id(m, i);\n> > + struct packed_git *pack;\n> > +\n> > + if (prepare_midx_pack(m, pack_id)) {\n> > + pack_errors = true;\n> > + continue;\n> > + }\n> > +\n> > + pack = nth_midxed_pack(m, pack_id);\n> > + if (should_exclude_pack(pack, opts->flags))\n> > + continue;\n> > + }\n> > +\n> > current = nth_midxed_object_oid(&oid, m, i);\n> >\n> > if (!match_hash(len, opts->prefix->hash, current->hash))\n> \n> It looks like this is:\n> \n> if (!match_hash(len, opts->prefix->hash, current->hash))\n> break;\n> \n> and I wonder if the `if (opts->flags) { ... }` block would be better\n> after that prefix check rather than before it.\n> \n> Putting it after the prefix check would make sure we don't continue\n> when the prefix doesn't match.\n\nHm, that's a good point indeed. Will adapt, thanks!\n\nPatrick\n"},{"id":"546374","messageId":"ajzCjgLJ5pzBph2Z@pks.im","threadId":"65851","inReplyTo":"xmqqjyrnkinn.fsf@gitster.g","subject":"Re: [PATCH v2 4/4] connected: search promisor objects generically","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-06-25T05:54:22Z","receivedAt":"2026-06-25T05:54:27Z","isPatch":true,"body":"On Wed, Jun 24, 2026 at 09:27:56AM -0700, Junio C Hamano wrote:\n> Patrick Steinhardt writes:\n> > diff --git a/connected.c b/connected.c\n> > index d2b334173f..b557ff5db9 100644\n> > --- a/connected.c\n> > +++ b/connected.c\n> > @@ -11,6 +11,13 @@\n> > #include \"packfile.h\"\n> > #include \"promisor-remote.h\"\n> > \n> > +static int promised_object_cb(const struct object_id *oid UNUSED,\n> > +\t\t\t struct object_info *oi UNUSED,\n> > +\t\t\t void *payload UNUSED)\n> > +{\n> > +\treturn 1;\n> > +}\n> > +\n> > /*\n> > * For partial clones, we don't want to have to do a regular connectivity check\n> > * because we have to enumerate and exclude all promisor objects (slow), and\n> > @@ -30,25 +37,28 @@ static int check_connected_promisor(oid_iterate_fn fn,\n> > \t\t\t\t void *cb_data,\n> > \t\t\t\t const struct object_id **oid)\n> > {\n> > +\tstruct odb_for_each_object_options opts = {\n> > +\t\t.flags = ODB_FOR_EACH_OBJECT_PROMISOR_ONLY,\n> > +\t\t.prefix_hex_len = the_repository->hash_algo->hexsz,\n> > +\t};\n> > +\tint err;\n> > +\n> > \todb_reprepare(the_repository->objects);\n> > \tdo {\n> > -\t\tstruct packed_git *p;\n> > +\t\topts.prefix = *oid;\n> > \n> > -\t\trepo_for_each_pack(the_repository, p) {\n> > -\t\t\tif (!p->pack_promisor)\n> > -\t\t\t\tcontinue;\n> > -\t\t\tif (find_pack_entry_one(*oid, p))\n> > -\t\t\t\tgoto promisor_pack_found;\n> > -\t\t}\n> > +\t\terr = odb_for_each_object_ext(the_repository->objects,\n> > +\t\t\t\t\t NULL, promised_object_cb,\n> > +\t\t\t\t\t NULL, &opts);\n> \n> promised_object_cb() returns 1 without any computation since we are\n> only interested in learning ODB_FOR_EACH_OBJECT_PROMISOR_ONLY finds\n> any such object.\n> \n> odb_for_each_object_ext() returns 0 (if it iterates all the sources\n> to the end), but if its call to odb_source_for_each_object() yields\n> non-zero value, the returned value comes back as \"err\" here,\n> terminating the for-each iteration immediately.\n> \n> odb_source_for_each_object() is implemented differently per the\n> source backend, but taking an example of \"packfile\" backend,\n> packfile_loose_for_each_object() ends up calling cb (wrapped in\n> packfile_store_for_each_object_wrapper_data) via\n> for_each_object_in_pack(), which stops immediately when cb returns\n> non-zero and the value returned from there is the value given by cb,\n> i.e., 1. So we will have err==1 when we find any object.\n> \n> > +\t\tif (err < 0)\n> > +\t\t\treturn err;\n> \n> And err presumably is 1 in such a case, so this does not trigger.\n> \n> > \t\t/*\n> > \t\t * We have found an object that is not part of a promisor pack,\n> > \t\t * and thus we cannot skip the full connectivity check.\n> > \t\t */\n> > -\t\treturn 0;\n> > -\n> > -promisor_pack_found:\n> > -\t\t;\n> > +\t\tif (err > 0)\n> > +\t\t\treturn 0;\n> \n> And this does.\n> \n> I may be misreading the patch, but as we return 0 from here, do we\n> cause the caller to fall back to full connectivity check? The\n> caller, check_connected(), sees a zero returned from here.\n\nYou're right, this is a result of the refactor. Previously we had it\nlike this:\n\n err = odb_for_each_object_ext(the_repository->objects,\n NULL, promised_object_cb,\n NULL, &opts);\n if (err < 0)\n break;\n if (err > 0) {\n err = 0;\n continue;\n }\n\nBut that made us correctly skip to the next object. Now though we have\nto check for `if (!err) return 0;` in the refactored code. Makes me\nwonder whether the logic would be easier to follow like this:\n\ndiff --git a/connected.c b/connected.c\nindex b557ff5db9..b5a9b0543d 100644\n--- a/connected.c\n+++ b/connected.c\n@@ -13,8 +13,10 @@\n \n static int promised_object_cb(const struct object_id *oid UNUSED,\n \t\t\t struct object_info *oi UNUSED,\n-\t\t\t void *payload UNUSED)\n+\t\t\t void *payload)\n {\n+\tbool *found = payload;\n+\t*found = true;\n \treturn 1;\n }\n \n@@ -45,11 +47,13 @@ static int check_connected_promisor(oid_iterate_fn fn,\n \n \todb_reprepare(the_repository->objects);\n \tdo {\n+\t\tbool found = false;\n+\n \t\topts.prefix = *oid;\n \n \t\terr = odb_for_each_object_ext(the_repository->objects,\n \t\t\t\t\t NULL, promised_object_cb,\n-\t\t\t\t\t NULL, &opts);\n+\t\t\t\t\t &found, &opts);\n \t\tif (err < 0)\n \t\t\treturn err;\n \n@@ -57,7 +61,7 @@ static int check_connected_promisor(oid_iterate_fn fn,\n \t\t * We have found an object that is not part of a promisor pack,\n \t\t * and thus we cannot skip the full connectivity check.\n \t\t */\n-\t\tif (err > 0)\n+\t\tif (!found)\n \t\t\treturn 0;\n \t} while ((*oid = fn(cb_data)) != NULL);\n \n\nIt's also a bit concerning that this doesn't cause any tests to fail.\nI'll try to figure out whether I can add one.\n\nThanks!\n\nPatrick\n"},{"id":"546397","messageId":"20260625-pks-connected-generic-promisor-checks-v3-0-7308f3b9dc44@pks.im","threadId":"65851","inReplyTo":"20260622-pks-connected-generic-promisor-checks-v1-0-25eba2698202@pks.im","subject":"[PATCH v3 0/4] connected: search promisor objects generically","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-06-25T09:57:38Z","receivedAt":"2026-06-25T09:57:50Z","isPatch":true,"body":"Hi,\n\nthis patch series refactors \"connected.c\" so that we search for promisor\nobjects in a generic way instead of reaching into internal of the object\ndatabase. As a result, the connectivity checks will work properly in\nrepos that don't use packfiles in the first place.\n\nThe series is built on top of 8d96f09e92 (Merge branch\n'js/objects-larger-than-4gb-on-windows', 2026-06-19) with\nps/odb-source-packed at 1bba3c035d (odb/source-packed: drop pointer to\n\"files\" parent source, 2026-06-17) merged into it.\n\nChanges in v3:\n - Fix reversed logic for whether the promised object was found, which\n broke in v2.\n - Add a test that verifies that we indeed use the optimized check.\n - Match the hash before computing the flags so that we break out of\n the loop more eagerly.\n - Link to v2: https://patch.msgid.link/20260624-pks-connected-generic-promisor-checks-v2-0-132d73ee47b9@pks.im\n\nChanges in v2:\n - Fix the accidentally-dropped call to `odb_reprepare()`.\n - Add a preparatory commit that splits out `check_connected_promisor()`.\n I think also splitting out `check_connected_rev_list()` would only\n have diminishing returns, so I skipped that part.\n - Link to v1: https://patch.msgid.link/20260622-pks-connected-generic-promisor-checks-v1-0-25eba2698202@pks.im\n\nThanks!\n\nPatrick\n\n---\nPatrick Steinhardt (4):\n odb/source-packed: extract logic to skip certain packs\n odb/source-packed: support flags when iterating an object prefix\n connected: split out promisor-based connectivity check\n connected: search promisor objects generically\n\n connected.c | 98 +++++++++++++++++++++++++++++++-----------------\n odb/source-packed.c | 50 +++++++++++++++++-------\n t/t5616-partial-clone.sh | 24 ++++++++++++\n 3 files changed, 125 insertions(+), 47 deletions(-)\n\nRange-diff versus v2:\n\n1: 74d1d04183 = 1: 93b7b3b4cb odb/source-packed: extract logic to skip certain packs\n2: 02aa39bf1e ! 2: 3fd0885b85 odb/source-packed: support flags when iterating an object prefix\n @@ odb/source-packed.c: static int for_each_prefixed_object_in_midx(\n \n \tfor (; m; m = m->base_midx) {\n @@ odb/source-packed.c: static int for_each_prefixed_object_in_midx(\n - \t\t\tconst struct object_id *current = NULL;\n - \t\t\tstruct object_id oid;\n + \t\t\tif (!match_hash(len, opts->prefix->hash, current->hash))\n + \t\t\t\tbreak;\n \n +\t\t\tif (opts->flags) {\n +\t\t\t\tuint32_t pack_id = nth_midxed_pack_int_id(m, i);\n @@ odb/source-packed.c: static int for_each_prefixed_object_in_midx(\n +\t\t\t\t\tcontinue;\n +\t\t\t}\n +\n - \t\t\tcurrent = nth_midxed_object_oid(&oid, m, i);\n + \t\t\tif (data->request) {\n + \t\t\t\tstruct object_info oi = *data->request;\n \n - \t\t\tif (!match_hash(len, opts->prefix->hash, current->hash))\n @@ odb/source-packed.c: static int for_each_prefixed_object_in_midx(\n \tret = 0;\n \n3: ff9df84f65 = 3: 47a4732daf connected: split out promisor-based connectivity check\n4: a10d2e6a1e ! 4: 239abf2731 connected: search promisor objects generically\n @@ Commit message\n flag. This will yield all objects that have the exact object name and\n that are part of a promisor pack in a generic way.\n \n + Add a test to verify that we indeed use the optimization.\n +\n Signed-off-by: Patrick Steinhardt \n \n ## connected.c ##\n @@ connected.c\n \n +static int promised_object_cb(const struct object_id *oid UNUSED,\n +\t\t\t struct object_info *oi UNUSED,\n -+\t\t\t void *payload UNUSED)\n ++\t\t\t void *payload)\n +{\n ++\tbool *found = payload;\n ++\t*found = true;\n +\treturn 1;\n +}\n +\n @@ connected.c: static int check_connected_promisor(oid_iterate_fn fn,\n \todb_reprepare(the_repository->objects);\n \tdo {\n -\t\tstruct packed_git *p;\n -+\t\topts.prefix = *oid;\n ++\t\tbool found = false;\n \n -\t\trepo_for_each_pack(the_repository, p) {\n -\t\t\tif (!p->pack_promisor)\n @@ connected.c: static int check_connected_promisor(oid_iterate_fn fn,\n -\t\t\tif (find_pack_entry_one(*oid, p))\n -\t\t\t\tgoto promisor_pack_found;\n -\t\t}\n -+\t\terr = odb_for_each_object_ext(the_repository->objects,\n -+\t\t\t\t\t NULL, promised_object_cb,\n -+\t\t\t\t\t NULL, &opts);\n ++\t\topts.prefix = *oid;\n ++\n ++\t\terr = odb_for_each_object_ext(the_repository->objects, NULL,\n ++\t\t\t\t\t promised_object_cb, &found, &opts);\n +\t\tif (err < 0)\n +\t\t\treturn err;\n \n @@ connected.c: static int check_connected_promisor(oid_iterate_fn fn,\n -\n -promisor_pack_found:\n -\t\t;\n -+\t\tif (err > 0)\n ++\t\tif (!found)\n +\t\t\treturn 0;\n \t} while ((*oid = fn(cb_data)) != NULL);\n \n \treturn 1;\n +\n + ## t/t5616-partial-clone.sh ##\n +@@ t/t5616-partial-clone.sh: test_expect_success 'partial fetch inherits filter settings' '\n + \ttest_line_count = 5 observed\n + '\n + \n ++test_expect_success 'partial fetch does not spawn rev-list connectivity check' '\n ++\ttest_when_finished \"rm -rf connectivity-remote connectivity-client\" &&\n ++\tgit init connectivity-remote &&\n ++\ttest_commit -C connectivity-remote one &&\n ++\tgit -C connectivity-remote config uploadpack.allowfilter 1 &&\n ++\tgit -C connectivity-remote config uploadpack.allowanysha1inwant 1 &&\n ++\n ++\tgit clone --no-checkout --filter=blob:none \\\n ++\t\t\"file://$(pwd)/connectivity-remote\" connectivity-client &&\n ++\n ++\t# When doing a partial fetch where all tips are part of a promisor pack\n ++\t# we want to skip the connectivity check, as these objects are allowed\n ++\t# to not be fully connected.\n ++\ttest_commit -C connectivity-remote two &&\n ++\tGIT_TRACE2_EVENT=\"$(pwd)/partial.trace\" git -C connectivity-client fetch origin &&\n ++\ttest_subcommand_flex ! git rev-list --objects --stdin \n---\n odb/source-packed.c | 28 ++++++++++++++++++----------\n 1 file changed, 18 insertions(+), 10 deletions(-)\n\ndiff --git a/odb/source-packed.c b/odb/source-packed.c\nindex 42c28fba0e..3afc4bf01f 100644\n--- a/odb/source-packed.c\n+++ b/odb/source-packed.c\n@@ -126,6 +126,22 @@ static int match_hash(unsigned len, const unsigned char *a, const unsigned char\n \treturn 1;\n }\n \n+static bool should_exclude_pack(struct packed_git *p, enum odb_for_each_object_flags flags)\n+{\n+\tif ((flags & ODB_FOR_EACH_OBJECT_LOCAL_ONLY) && !p->pack_local)\n+\t\treturn true;\n+\tif ((flags & ODB_FOR_EACH_OBJECT_PROMISOR_ONLY) &&\n+\t !p->pack_promisor)\n+\t\treturn true;\n+\tif ((flags & ODB_FOR_EACH_OBJECT_SKIP_IN_CORE_KEPT_PACKS) &&\n+\t p->pack_keep_in_core)\n+\t\treturn true;\n+\tif ((flags & ODB_FOR_EACH_OBJECT_SKIP_ON_DISK_KEPT_PACKS) &&\n+\t p->pack_keep)\n+\t\treturn true;\n+\treturn false;\n+}\n+\n static int for_each_prefixed_object_in_midx(\n \tstruct odb_source_packed *store,\n \tstruct multi_pack_index *m,\n@@ -306,17 +322,9 @@ static int odb_source_packed_for_each_object(struct odb_source *source,\n \tfor (e = packfile_store_get_packs(packed); e; e = e->next) {\n \t\tstruct packed_git *p = e->pack;\n \n-\t\tif ((opts->flags & ODB_FOR_EACH_OBJECT_LOCAL_ONLY) && !p->pack_local)\n-\t\t\tcontinue;\n-\t\tif ((opts->flags & ODB_FOR_EACH_OBJECT_PROMISOR_ONLY) &&\n-\t\t !p->pack_promisor)\n-\t\t\tcontinue;\n-\t\tif ((opts->flags & ODB_FOR_EACH_OBJECT_SKIP_IN_CORE_KEPT_PACKS) &&\n-\t\t p->pack_keep_in_core)\n-\t\t\tcontinue;\n-\t\tif ((opts->flags & ODB_FOR_EACH_OBJECT_SKIP_ON_DISK_KEPT_PACKS) &&\n-\t\t p->pack_keep)\n+\t\tif (should_exclude_pack(p, opts->flags))\n \t\t\tcontinue;\n+\n \t\tif (open_pack_index(p)) {\n \t\t\tpack_errors = 1;\n \t\t\tcontinue;\n\n-- \n2.55.0.rc1.745.g43192e7977.dirty\n\n"},{"id":"546399","messageId":"20260625-pks-connected-generic-promisor-checks-v3-2-7308f3b9dc44@pks.im","threadId":"65851","inReplyTo":"20260625-pks-connected-generic-promisor-checks-v3-0-7308f3b9dc44@pks.im","subject":"[PATCH v3 2/4] odb/source-packed: support flags when iterating an object prefix","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-06-25T09:57:40Z","receivedAt":"2026-06-25T09:57:54Z","isPatch":true,"body":"Callers of `odb_for_each_object()` can specify an optional object name\nprefix so that we only yield objects that match it. This is incompatible\nthough with passing flags at the same time, as we don't yet know to\nhandle them.\n\nLoosen this restriction by calling `should_exclude_pack()`.\n\nSigned-off-by: Patrick Steinhardt \n---\n odb/source-packed.c | 22 +++++++++++++++++++---\n 1 file changed, 19 insertions(+), 3 deletions(-)\n\ndiff --git a/odb/source-packed.c b/odb/source-packed.c\nindex 3afc4bf01f..96fc436770 100644\n--- a/odb/source-packed.c\n+++ b/odb/source-packed.c\n@@ -148,6 +148,7 @@ static int for_each_prefixed_object_in_midx(\n \tconst struct odb_for_each_object_options *opts,\n \tstruct odb_source_packed_for_each_object_wrapper_data *data)\n {\n+\tbool pack_errors = false;\n \tint ret;\n \n \tfor (; m; m = m->base_midx) {\n@@ -176,6 +177,20 @@ static int for_each_prefixed_object_in_midx(\n \t\t\tif (!match_hash(len, opts->prefix->hash, current->hash))\n \t\t\t\tbreak;\n \n+\t\t\tif (opts->flags) {\n+\t\t\t\tuint32_t pack_id = nth_midxed_pack_int_id(m, i);\n+\t\t\t\tstruct packed_git *pack;\n+\n+\t\t\t\tif (prepare_midx_pack(m, pack_id)) {\n+\t\t\t\t\tpack_errors = true;\n+\t\t\t\t\tcontinue;\n+\t\t\t\t}\n+\n+\t\t\t\tpack = nth_midxed_pack(m, pack_id);\n+\t\t\t\tif (should_exclude_pack(pack, opts->flags))\n+\t\t\t\t\tcontinue;\n+\t\t\t}\n+\n \t\t\tif (data->request) {\n \t\t\t\tstruct object_info oi = *data->request;\n \n@@ -198,6 +213,8 @@ static int for_each_prefixed_object_in_midx(\n \tret = 0;\n \n out:\n+\tif (!ret && pack_errors)\n+\t\tret = -1;\n \treturn ret;\n }\n \n@@ -260,9 +277,6 @@ static int odb_source_packed_for_each_prefixed_object(\n \tbool pack_errors = false;\n \tint ret;\n \n-\tif (opts->flags)\n-\t\tBUG(\"flags unsupported\");\n-\n \tstore->skip_mru_updates = true;\n \n \tm = get_multi_pack_index(store);\n@@ -275,6 +289,8 @@ static int odb_source_packed_for_each_prefixed_object(\n \tfor (e = packfile_store_get_packs(store); e; e = e->next) {\n \t\tif (e->pack->multi_pack_index)\n \t\t\tcontinue;\n+\t\tif (should_exclude_pack(e->pack, opts->flags))\n+\t\t\tcontinue;\n \n \t\tif (open_pack_index(e->pack)) {\n \t\t\tpack_errors = true;\n\n-- \n2.55.0.rc1.745.g43192e7977.dirty\n\n"},{"id":"546400","messageId":"20260625-pks-connected-generic-promisor-checks-v3-3-7308f3b9dc44@pks.im","threadId":"65851","inReplyTo":"20260625-pks-connected-generic-promisor-checks-v3-0-7308f3b9dc44@pks.im","subject":"[PATCH v3 3/4] connected: split out promisor-based connectivity check","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-06-25T09:57:41Z","receivedAt":"2026-06-25T09:57:57Z","isPatch":true,"body":"When performing a connectivity check in a partial clone we try to avoid\ndoing the connectivity check by checking whether all new tips are part\nof a promisor pack. This makes use of the fact that we don't expect full\nconnectivity for promised objects anyway, so it's basically fine if\nthose objects are not fully connected.\n\nThe logic that handles this promisor-based check is somewhat hard to\nread though as it uses nested loops and gotos. Pull it out into a\nstandalone function, which makes it a bit easier to reason about.\n\nWe'll also further simplify the function in the next commit.\n\nSuggested-by: Christian Couder \nSigned-off-by: Patrick Steinhardt \n---\n connected.c | 85 ++++++++++++++++++++++++++++++++++++-------------------------\n 1 file changed, 51 insertions(+), 34 deletions(-)\n\ndiff --git a/connected.c b/connected.c\nindex 7e26976832..d2b334173f 100644\n--- a/connected.c\n+++ b/connected.c\n@@ -11,6 +11,49 @@\n #include \"packfile.h\"\n #include \"promisor-remote.h\"\n \n+/*\n+ * For partial clones, we don't want to have to do a regular connectivity check\n+ * because we have to enumerate and exclude all promisor objects (slow), and\n+ * then the connectivity check itself becomes a no-op because in a partial\n+ * clone every object is a promisor object. Instead, just make sure we\n+ * received, in a promisor packfile, the objects pointed to by each wanted ref.\n+ *\n+ * Before checking for promisor packs, be sure we have the latest pack-files\n+ * loaded into memory.\n+ *\n+ * Returns 1 when all object IDs have been found in promisor packs, in which\n+ * case we're fully connected and thus done. Returns 0 when we have found\n+ * objects in non-promisor packs, in which case we'll have to fall back to the\n+ * rev-list-based connectivity checks. Returns a negative error code on error.\n+ */\n+static int check_connected_promisor(oid_iterate_fn fn,\n+\t\t\t\t void *cb_data,\n+\t\t\t\t const struct object_id **oid)\n+{\n+\todb_reprepare(the_repository->objects);\n+\tdo {\n+\t\tstruct packed_git *p;\n+\n+\t\trepo_for_each_pack(the_repository, p) {\n+\t\t\tif (!p->pack_promisor)\n+\t\t\t\tcontinue;\n+\t\t\tif (find_pack_entry_one(*oid, p))\n+\t\t\t\tgoto promisor_pack_found;\n+\t\t}\n+\n+\t\t/*\n+\t\t * We have found an object that is not part of a promisor pack,\n+\t\t * and thus we cannot skip the full connectivity check.\n+\t\t */\n+\t\treturn 0;\n+\n+promisor_pack_found:\n+\t\t;\n+\t} while ((*oid = fn(cb_data)) != NULL);\n+\n+\treturn 1;\n+}\n+\n /*\n * If we feed all the commits we want to verify to this command\n *\n@@ -46,42 +89,16 @@ int check_connected(oid_iterate_fn fn, void *cb_data,\n \t}\n \n \tif (repo_has_promisor_remote(the_repository)) {\n-\t\t/*\n-\t\t * For partial clones, we don't want to have to do a regular\n-\t\t * connectivity check because we have to enumerate and exclude\n-\t\t * all promisor objects (slow), and then the connectivity check\n-\t\t * itself becomes a no-op because in a partial clone every\n-\t\t * object is a promisor object. Instead, just make sure we\n-\t\t * received, in a promisor packfile, the objects pointed to by\n-\t\t * each wanted ref.\n-\t\t *\n-\t\t * Before checking for promisor packs, be sure we have the\n-\t\t * latest pack-files loaded into memory.\n-\t\t */\n-\t\todb_reprepare(the_repository->objects);\n-\t\tdo {\n-\t\t\tstruct packed_git *p;\n-\n-\t\t\trepo_for_each_pack(the_repository, p) {\n-\t\t\t\tif (!p->pack_promisor)\n-\t\t\t\t\tcontinue;\n-\t\t\t\tif (find_pack_entry_one(oid, p))\n-\t\t\t\t\tgoto promisor_pack_found;\n-\t\t\t}\n-\t\t\t/*\n-\t\t\t * Fallback to rev-list with oid and the rest of the\n-\t\t\t * object IDs provided by fn.\n-\t\t\t */\n-\t\t\tgoto no_promisor_pack_found;\n-promisor_pack_found:\n-\t\t\t;\n-\t\t} while ((oid = fn(cb_data)) != NULL);\n-\t\tif (opt->err_fd)\n-\t\t\tclose(opt->err_fd);\n-\t\treturn 0;\n+\t\terr = check_connected_promisor(fn, cb_data, &oid);\n+\t\tif (err) {\n+\t\t\tif (opt->err_fd)\n+\t\t\t\tclose(opt->err_fd);\n+\t\t\tif (err > 0)\n+\t\t\t\terr = 0;\n+\t\t\treturn err;\n+\t\t}\n \t}\n \n-no_promisor_pack_found:\n \tif (opt->shallow_file) {\n \t\tstrvec_push(&rev_list.args, \"--shallow-file\");\n \t\tstrvec_push(&rev_list.args, opt->shallow_file);\n\n-- \n2.55.0.rc1.745.g43192e7977.dirty\n\n"},{"id":"546401","messageId":"20260625-pks-connected-generic-promisor-checks-v3-4-7308f3b9dc44@pks.im","threadId":"65851","inReplyTo":"20260625-pks-connected-generic-promisor-checks-v3-0-7308f3b9dc44@pks.im","subject":"[PATCH v3 4/4] connected: search promisor objects generically","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-06-25T09:57:42Z","receivedAt":"2026-06-25T09:57:59Z","isPatch":true,"body":"When performing connectivity checks we have to figure out whether any of\nthe new objects are promisor objects, as we cannot assume full\nconnectivity if so.\n\nThis check is performed by iterating through all packfiles in the\nrepository and searching each of them for the given object. Of course,\nthis mechanism is quite specific to implementation details of the object\ndatabase, as we assume that it uses packfiles in the first place.\n\nRefactor the logic so that we instead use `odb_for_each_object_ext()`\nwith an object prefix filter and the `ODB_FOR_EACH_OBJECT_PROMISOR_ONLY`\nflag. This will yield all objects that have the exact object name and\nthat are part of a promisor pack in a generic way.\n\nAdd a test to verify that we indeed use the optimization.\n\nSigned-off-by: Patrick Steinhardt \n---\n connected.c | 35 ++++++++++++++++++++++++-----------\n t/t5616-partial-clone.sh | 24 ++++++++++++++++++++++++\n 2 files changed, 48 insertions(+), 11 deletions(-)\n\ndiff --git a/connected.c b/connected.c\nindex d2b334173f..929b9bd28d 100644\n--- a/connected.c\n+++ b/connected.c\n@@ -11,6 +11,15 @@\n #include \"packfile.h\"\n #include \"promisor-remote.h\"\n \n+static int promised_object_cb(const struct object_id *oid UNUSED,\n+\t\t\t struct object_info *oi UNUSED,\n+\t\t\t void *payload)\n+{\n+\tbool *found = payload;\n+\t*found = true;\n+\treturn 1;\n+}\n+\n /*\n * For partial clones, we don't want to have to do a regular connectivity check\n * because we have to enumerate and exclude all promisor objects (slow), and\n@@ -30,25 +39,29 @@ static int check_connected_promisor(oid_iterate_fn fn,\n \t\t\t\t void *cb_data,\n \t\t\t\t const struct object_id **oid)\n {\n+\tstruct odb_for_each_object_options opts = {\n+\t\t.flags = ODB_FOR_EACH_OBJECT_PROMISOR_ONLY,\n+\t\t.prefix_hex_len = the_repository->hash_algo->hexsz,\n+\t};\n+\tint err;\n+\n \todb_reprepare(the_repository->objects);\n \tdo {\n-\t\tstruct packed_git *p;\n+\t\tbool found = false;\n \n-\t\trepo_for_each_pack(the_repository, p) {\n-\t\t\tif (!p->pack_promisor)\n-\t\t\t\tcontinue;\n-\t\t\tif (find_pack_entry_one(*oid, p))\n-\t\t\t\tgoto promisor_pack_found;\n-\t\t}\n+\t\topts.prefix = *oid;\n+\n+\t\terr = odb_for_each_object_ext(the_repository->objects, NULL,\n+\t\t\t\t\t promised_object_cb, &found, &opts);\n+\t\tif (err < 0)\n+\t\t\treturn err;\n \n \t\t/*\n \t\t * We have found an object that is not part of a promisor pack,\n \t\t * and thus we cannot skip the full connectivity check.\n \t\t */\n-\t\treturn 0;\n-\n-promisor_pack_found:\n-\t\t;\n+\t\tif (!found)\n+\t\t\treturn 0;\n \t} while ((*oid = fn(cb_data)) != NULL);\n \n \treturn 1;\ndiff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh\nindex 1c2805acca..905052072d 100755\n--- a/t/t5616-partial-clone.sh\n+++ b/t/t5616-partial-clone.sh\n@@ -97,6 +97,30 @@ test_expect_success 'partial fetch inherits filter settings' '\n \ttest_line_count = 5 observed\n '\n \n+test_expect_success 'partial fetch does not spawn rev-list connectivity check' '\n+\ttest_when_finished \"rm -rf connectivity-remote connectivity-client\" &&\n+\tgit init connectivity-remote &&\n+\ttest_commit -C connectivity-remote one &&\n+\tgit -C connectivity-remote config uploadpack.allowfilter 1 &&\n+\tgit -C connectivity-remote config uploadpack.allowanysha1inwant 1 &&\n+\n+\tgit clone --no-checkout --filter=blob:none \\\n+\t\t\"file://$(pwd)/connectivity-remote\" connectivity-client &&\n+\n+\t# When doing a partial fetch where all tips are part of a promisor pack\n+\t# we want to skip the connectivity check, as these objects are allowed\n+\t# to not be fully connected.\n+\ttest_commit -C connectivity-remote two &&\n+\tGIT_TRACE2_EVENT=\"$(pwd)/partial.trace\" git -C connectivity-client fetch origin &&\n+\ttest_subcommand_flex ! git rev-list --objects --stdin writes:\n\n> When performing connectivity checks we have to figure out whether any of\n> the new objects are promisor objects, as we cannot assume full\n> connectivity if so.\n>\n> This check is performed by iterating through all packfiles in the\n> repository and searching each of them for the given object. Of course,\n> this mechanism is quite specific to implementation details of the object\n> database, as we assume that it uses packfiles in the first place.\n>\n> Refactor the logic so that we instead use `odb_for_each_object_ext()`\n> with an object prefix filter and the `ODB_FOR_EACH_OBJECT_PROMISOR_ONLY`\n> flag. This will yield all objects that have the exact object name and\n> that are part of a promisor pack in a generic way.\n>\n> Add a test to verify that we indeed use the optimization.\n\nOK. The new test is a good way to catch the issue we noticed in the\nprevious round, I guess. Looking good.\n\nThanks.\n"},{"id":"546446","messageId":"CAP8UFD07AzNtP3rRj4btYfFfakX0kkLXKpO9T=a3Mds3YWEsXw@mail.gmail.com","threadId":"65851","inReplyTo":"xmqq4iiqfk0l.fsf@gitster.g","subject":"Re: [PATCH v3 4/4] connected: search promisor objects generically","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2026-06-26T06:55:27Z","receivedAt":"2026-06-26T06:55:39Z","isPatch":true,"body":"On Thu, Jun 25, 2026 at 10:22 PM Junio C Hamano wrote:\n>\n> Patrick Steinhardt writes:\n>\n> > When performing connectivity checks we have to figure out whether any of\n> > the new objects are promisor objects, as we cannot assume full\n> > connectivity if so.\n> >\n> > This check is performed by iterating through all packfiles in the\n> > repository and searching each of them for the given object. Of course,\n> > this mechanism is quite specific to implementation details of the object\n> > database, as we assume that it uses packfiles in the first place.\n> >\n> > Refactor the logic so that we instead use `odb_for_each_object_ext()`\n> > with an object prefix filter and the `ODB_FOR_EACH_OBJECT_PROMISOR_ONLY`\n> > flag. This will yield all objects that have the exact object name and\n> > that are part of a promisor pack in a generic way.\n> >\n> > Add a test to verify that we indeed use the optimization.\n>\n> OK. The new test is a good way to catch the issue we noticed in the\n> previous round, I guess. Looking good.\n\nYeah, it looks ready to me too.\n\nThanks.\n"}]}