Re: [PATCH v2 4/4] connected: search promisor objects generically

[Junio C Hamano <gitster@pobox.com>]

Patrick Steinhardt <ps@pks.im> writes:

> 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.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  connected.c | 32 +++++++++++++++++++++-----------
>  1 file changed, 21 insertions(+), 11 deletions(-)
>
> diff --git a/connected.c b/connected.c
> index d2b334173f..b557ff5db9 100644
> --- a/connected.c
> +++ b/connected.c
> @@ -11,6 +11,13 @@
>  #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 UNUSED)
> +{
> +	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 +37,28 @@ 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;
> +		opts.prefix = *oid;
>  
> -		repo_for_each_pack(the_repository, p) {
> -			if (!p->pack_promisor)
> -				continue;
> -			if (find_pack_entry_one(*oid, p))
> -				goto promisor_pack_found;
> -		}
> +		err = odb_for_each_object_ext(the_repository->objects,
> +					      NULL, promised_object_cb,
> +					      NULL, &opts);

promised_object_cb() returns 1 without any computation since we are
only interested in learning ODB_FOR_EACH_OBJECT_PROMISOR_ONLY finds
any such object.

odb_for_each_object_ext() returns 0 (if it iterates all the sources
to the end), but if its call to odb_source_for_each_object() yields
non-zero value, the returned value comes back as "err" here,
terminating the for-each iteration immediately.

odb_source_for_each_object() is implemented differently per the
source backend, but taking an example of "packfile" backend,
packfile_loose_for_each_object() ends up calling cb (wrapped in
packfile_store_for_each_object_wrapper_data) via
for_each_object_in_pack(), which stops immediately when cb returns
non-zero and the value returned from there is the value given by cb,
i.e., 1.  So we will have err==1 when we find any object.

> +		if (err < 0)
> +			return err;

And err presumably is 1 in such a case, so this does not trigger.

>  		/*
>  		 * 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 (err > 0)
> +			return 0;

And this does.

I may be misreading the patch, but as we return 0 from here, do we
cause the caller to fall back to full connectivity check?  The
caller, check_connected(), sees a zero returned from here.

>  	} while ((*oid = fn(cb_data)) != NULL);
>  
>  	return 1;