From d45f956f2022a90d39b1ce82aea668cce73f1d75 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Thu, 25 Jun 2026 11:57:39 +0200 Subject: [PATCH 1/4] odb/source-packed: extract logic to skip certain packs The caller can pass flags that allow them to filter out specific kinds of objects when iterating objects via `odb_for_each_object()`. This only works for "normal" iteration though, as we `BUG()` when the user passes flags and specifies an object prefix. This limitation will be lifted in the next commit. Prepare for this by extracting the logic that skips certain kinds of packs so that we can easily reuse it. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- odb/source-packed.c | 28 ++++++++++++++++++---------- 1 file changed, 18 insertions(+), 10 deletions(-) diff --git a/odb/source-packed.c b/odb/source-packed.c index 42c28fba0e..3afc4bf01f 100644 --- a/odb/source-packed.c +++ b/odb/source-packed.c @@ -126,6 +126,22 @@ static int match_hash(unsigned len, const unsigned char *a, const unsigned char return 1; } +static bool should_exclude_pack(struct packed_git *p, enum odb_for_each_object_flags flags) +{ + if ((flags & ODB_FOR_EACH_OBJECT_LOCAL_ONLY) && !p->pack_local) + return true; + if ((flags & ODB_FOR_EACH_OBJECT_PROMISOR_ONLY) && + !p->pack_promisor) + return true; + if ((flags & ODB_FOR_EACH_OBJECT_SKIP_IN_CORE_KEPT_PACKS) && + p->pack_keep_in_core) + return true; + if ((flags & ODB_FOR_EACH_OBJECT_SKIP_ON_DISK_KEPT_PACKS) && + p->pack_keep) + return true; + return false; +} + static int for_each_prefixed_object_in_midx( struct odb_source_packed *store, struct multi_pack_index *m, @@ -306,17 +322,9 @@ static int odb_source_packed_for_each_object(struct odb_source *source, for (e = packfile_store_get_packs(packed); e; e = e->next) { struct packed_git *p = e->pack; - if ((opts->flags & ODB_FOR_EACH_OBJECT_LOCAL_ONLY) && !p->pack_local) - continue; - if ((opts->flags & ODB_FOR_EACH_OBJECT_PROMISOR_ONLY) && - !p->pack_promisor) - continue; - if ((opts->flags & ODB_FOR_EACH_OBJECT_SKIP_IN_CORE_KEPT_PACKS) && - p->pack_keep_in_core) - continue; - if ((opts->flags & ODB_FOR_EACH_OBJECT_SKIP_ON_DISK_KEPT_PACKS) && - p->pack_keep) + if (should_exclude_pack(p, opts->flags)) continue; + if (open_pack_index(p)) { pack_errors = 1; continue; From 8ed957112de135f449698e9408f2582eabb9401e Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Thu, 25 Jun 2026 11:57:40 +0200 Subject: [PATCH 2/4] odb/source-packed: support flags when iterating an object prefix Callers of `odb_for_each_object()` can specify an optional object name prefix so that we only yield objects that match it. This is incompatible though with passing flags at the same time, as we don't yet know to handle them. Loosen this restriction by calling `should_exclude_pack()`. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- odb/source-packed.c | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/odb/source-packed.c b/odb/source-packed.c index 3afc4bf01f..96fc436770 100644 --- a/odb/source-packed.c +++ b/odb/source-packed.c @@ -148,6 +148,7 @@ static int for_each_prefixed_object_in_midx( const struct odb_for_each_object_options *opts, struct odb_source_packed_for_each_object_wrapper_data *data) { + bool pack_errors = false; int ret; for (; m; m = m->base_midx) { @@ -176,6 +177,20 @@ static int for_each_prefixed_object_in_midx( if (!match_hash(len, opts->prefix->hash, current->hash)) break; + if (opts->flags) { + uint32_t pack_id = nth_midxed_pack_int_id(m, i); + struct packed_git *pack; + + if (prepare_midx_pack(m, pack_id)) { + pack_errors = true; + continue; + } + + pack = nth_midxed_pack(m, pack_id); + if (should_exclude_pack(pack, opts->flags)) + continue; + } + if (data->request) { struct object_info oi = *data->request; @@ -198,6 +213,8 @@ static int for_each_prefixed_object_in_midx( ret = 0; out: + if (!ret && pack_errors) + ret = -1; return ret; } @@ -260,9 +277,6 @@ static int odb_source_packed_for_each_prefixed_object( bool pack_errors = false; int ret; - if (opts->flags) - BUG("flags unsupported"); - store->skip_mru_updates = true; m = get_multi_pack_index(store); @@ -275,6 +289,8 @@ static int odb_source_packed_for_each_prefixed_object( for (e = packfile_store_get_packs(store); e; e = e->next) { if (e->pack->multi_pack_index) continue; + if (should_exclude_pack(e->pack, opts->flags)) + continue; if (open_pack_index(e->pack)) { pack_errors = true; From 0a7f3389f4c16da05a1113ef0829a352af62dcbe Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Thu, 25 Jun 2026 11:57:41 +0200 Subject: [PATCH 3/4] connected: split out promisor-based connectivity check When performing a connectivity check in a partial clone we try to avoid doing the connectivity check by checking whether all new tips are part of a promisor pack. This makes use of the fact that we don't expect full connectivity for promised objects anyway, so it's basically fine if those objects are not fully connected. The logic that handles this promisor-based check is somewhat hard to read though as it uses nested loops and gotos. Pull it out into a standalone function, which makes it a bit easier to reason about. We'll also further simplify the function in the next commit. Suggested-by: Christian Couder Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- connected.c | 85 ++++++++++++++++++++++++++++++++--------------------- 1 file changed, 51 insertions(+), 34 deletions(-) diff --git a/connected.c b/connected.c index 7e26976832..d2b334173f 100644 --- a/connected.c +++ b/connected.c @@ -11,6 +11,49 @@ #include "packfile.h" #include "promisor-remote.h" +/* + * For partial clones, we don't want to have to do a regular connectivity check + * because we have to enumerate and exclude all promisor objects (slow), and + * then the connectivity check itself becomes a no-op because in a partial + * clone every object is a promisor object. Instead, just make sure we + * received, in a promisor packfile, the objects pointed to by each wanted ref. + * + * Before checking for promisor packs, be sure we have the latest pack-files + * loaded into memory. + * + * Returns 1 when all object IDs have been found in promisor packs, in which + * case we're fully connected and thus done. Returns 0 when we have found + * objects in non-promisor packs, in which case we'll have to fall back to the + * rev-list-based connectivity checks. Returns a negative error code on error. + */ +static int check_connected_promisor(oid_iterate_fn fn, + void *cb_data, + const struct object_id **oid) +{ + odb_reprepare(the_repository->objects); + do { + struct packed_git *p; + + repo_for_each_pack(the_repository, p) { + if (!p->pack_promisor) + continue; + if (find_pack_entry_one(*oid, p)) + goto promisor_pack_found; + } + + /* + * We have found an object that is not part of a promisor pack, + * and thus we cannot skip the full connectivity check. + */ + return 0; + +promisor_pack_found: + ; + } while ((*oid = fn(cb_data)) != NULL); + + return 1; +} + /* * If we feed all the commits we want to verify to this command * @@ -46,42 +89,16 @@ int check_connected(oid_iterate_fn fn, void *cb_data, } if (repo_has_promisor_remote(the_repository)) { - /* - * For partial clones, we don't want to have to do a regular - * connectivity check because we have to enumerate and exclude - * all promisor objects (slow), and then the connectivity check - * itself becomes a no-op because in a partial clone every - * object is a promisor object. Instead, just make sure we - * received, in a promisor packfile, the objects pointed to by - * each wanted ref. - * - * Before checking for promisor packs, be sure we have the - * latest pack-files loaded into memory. - */ - odb_reprepare(the_repository->objects); - do { - struct packed_git *p; - - repo_for_each_pack(the_repository, p) { - if (!p->pack_promisor) - continue; - if (find_pack_entry_one(oid, p)) - goto promisor_pack_found; - } - /* - * Fallback to rev-list with oid and the rest of the - * object IDs provided by fn. - */ - goto no_promisor_pack_found; -promisor_pack_found: - ; - } while ((oid = fn(cb_data)) != NULL); - if (opt->err_fd) - close(opt->err_fd); - return 0; + err = check_connected_promisor(fn, cb_data, &oid); + if (err) { + if (opt->err_fd) + close(opt->err_fd); + if (err > 0) + err = 0; + return err; + } } -no_promisor_pack_found: if (opt->shallow_file) { strvec_push(&rev_list.args, "--shallow-file"); strvec_push(&rev_list.args, opt->shallow_file); From 66ee9cb93086b27721a36e2ba877921dcf177d91 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Thu, 25 Jun 2026 11:57:42 +0200 Subject: [PATCH 4/4] connected: search promisor objects generically When performing connectivity checks we have to figure out whether any of the new objects are promisor objects, as we cannot assume full connectivity if so. This check is performed by iterating through all packfiles in the repository and searching each of them for the given object. Of course, this mechanism is quite specific to implementation details of the object database, as we assume that it uses packfiles in the first place. Refactor the logic so that we instead use `odb_for_each_object_ext()` with an object prefix filter and the `ODB_FOR_EACH_OBJECT_PROMISOR_ONLY` flag. This will yield all objects that have the exact object name and that are part of a promisor pack in a generic way. Add a test to verify that we indeed use the optimization. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- connected.c | 35 ++++++++++++++++++++++++----------- t/t5616-partial-clone.sh | 24 ++++++++++++++++++++++++ 2 files changed, 48 insertions(+), 11 deletions(-) diff --git a/connected.c b/connected.c index d2b334173f..929b9bd28d 100644 --- a/connected.c +++ b/connected.c @@ -11,6 +11,15 @@ #include "packfile.h" #include "promisor-remote.h" +static int promised_object_cb(const struct object_id *oid UNUSED, + struct object_info *oi UNUSED, + void *payload) +{ + bool *found = payload; + *found = true; + return 1; +} + /* * For partial clones, we don't want to have to do a regular connectivity check * because we have to enumerate and exclude all promisor objects (slow), and @@ -30,25 +39,29 @@ static int check_connected_promisor(oid_iterate_fn fn, void *cb_data, const struct object_id **oid) { + struct odb_for_each_object_options opts = { + .flags = ODB_FOR_EACH_OBJECT_PROMISOR_ONLY, + .prefix_hex_len = the_repository->hash_algo->hexsz, + }; + int err; + odb_reprepare(the_repository->objects); do { - struct packed_git *p; + bool found = false; - repo_for_each_pack(the_repository, p) { - if (!p->pack_promisor) - continue; - if (find_pack_entry_one(*oid, p)) - goto promisor_pack_found; - } + opts.prefix = *oid; + + err = odb_for_each_object_ext(the_repository->objects, NULL, + promised_object_cb, &found, &opts); + if (err < 0) + return err; /* * We have found an object that is not part of a promisor pack, * and thus we cannot skip the full connectivity check. */ - return 0; - -promisor_pack_found: - ; + if (!found) + return 0; } while ((*oid = fn(cb_data)) != NULL); return 1; diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh index 1c2805acca..905052072d 100755 --- a/t/t5616-partial-clone.sh +++ b/t/t5616-partial-clone.sh @@ -97,6 +97,30 @@ test_expect_success 'partial fetch inherits filter settings' ' test_line_count = 5 observed ' +test_expect_success 'partial fetch does not spawn rev-list connectivity check' ' + test_when_finished "rm -rf connectivity-remote connectivity-client" && + git init connectivity-remote && + test_commit -C connectivity-remote one && + git -C connectivity-remote config uploadpack.allowfilter 1 && + git -C connectivity-remote config uploadpack.allowanysha1inwant 1 && + + git clone --no-checkout --filter=blob:none \ + "file://$(pwd)/connectivity-remote" connectivity-client && + + # When doing a partial fetch where all tips are part of a promisor pack + # we want to skip the connectivity check, as these objects are allowed + # to not be fully connected. + test_commit -C connectivity-remote two && + GIT_TRACE2_EVENT="$(pwd)/partial.trace" git -C connectivity-client fetch origin && + test_subcommand_flex ! git rev-list --objects --stdin