Re: [PATCH v5 04/16] promisor-remote: implement promisor_remote_get_direct()

[Derrick Stolee <stolee@gmail.com>]

On 4/9/2019 12:11 PM, Christian Couder wrote:
> From: Christian Couder <christian.couder@gmail.com>
> 
> This is implemented for now by calling fetch_objects(). It fetches
> from all the promisor remotes.

Hi Christian,

Sorry for jumping on the thread late, but I noticed some peculiarities
when looking at the test coverage report.

> +static int remove_fetched_oids(struct object_id **oids, int oid_nr, int to_free)

This method does not seem to be covered by the test suite at all.
Is this scenario difficult to set up for a test?

> +{
> +	int i, missing_nr = 0;
> +	int *missing = xcalloc(oid_nr, sizeof(*missing));
> +	struct object_id *old_oids = *oids;
> +	struct object_id *new_oids;
> +	int old_fetch_if_missing = fetch_if_missing;
> +
> +	fetch_if_missing = 0;

This global 'fetch_if_missing' swap seems very fragile. I'm guessing you are using
it to prevent a loop when calling oid_object_info_extended() below. Can you instead
pass a flag to the method that disables the fetch_if_missing behavior?

> +
> +	for (i = 0; i < oid_nr; i++)
> +		if (oid_object_info_extended(the_repository, &old_oids[i], NULL, 0)) {

A use of "the_repository" this deep in new code is asking for a refactor later to remove it.
Please try to pass a "struct repository *r" through your methods so we minimize references
to the_repository (and the amount of work required to remove them later).

> +			missing[i] = 1;
> +			missing_nr++;
> +		}
> +
> +	fetch_if_missing = old_fetch_if_missing;
> +
> +	if (missing_nr) {
> +		int j = 0;
> +		new_oids = xcalloc(missing_nr, sizeof(*new_oids));
> +		for (i = 0; i < oid_nr; i++)
> +			if (missing[i])
> +				oidcpy(&new_oids[j++], &old_oids[i]);
> +		*oids = new_oids;
> +		if (to_free)
> +			free(old_oids);
> +	}
> +
> +	free(missing);
> +
> +	return missing_nr;
> +}
> +
> +int promisor_remote_get_direct(const struct object_id *oids, int oid_nr)
> +{
> +	struct promisor_remote *r;
> +	struct object_id *missing_oids = (struct object_id *)oids;
> +	int missing_nr = oid_nr;

Note that for this method, "missing_nr" actually means "number of oids still in the list".

> +	int to_free = 0;
> +	int res = -1;
> +
> +	promisor_remote_init();
> +
> +	for (r = promisors; r; r = r->next) {
> +		if (fetch_objects(r->name, missing_oids, missing_nr) < 0) {

This block hits if we have any missing objects. This is not currently hit by the test
suite.

> +			if (missing_nr == 1)
> +				continue;

But we skip the call below if there is exactly one object in the list, as it must be the one
missing object. So, to be interesting we need to try fetching multiple objects.

> +			missing_nr = remove_fetched_oids(&missing_oids, missing_nr, to_free);

Here is the one call, and after this assignment "missing_nr" does mean the number of missing objects.
However, I do think this could be clarified by using remaining_nr and remaining_oids.

> +			if (missing_nr) {
> +				to_free = 1;
> +				continue;
> +			}

Now this block took a bit to grok. You use to_free in the if(to_free) free(missing_oids); below.
But it also changes the behavior of remove_fetched_oids(). This means that the first time
remove_fetched_oids() will preserve the list (because it is the input list) but all later
calls will free the newly-created intermediate list. This checks out.

What is confusing to me: is there any reason that missing_nr would be zero in this situation?
I guess if the fetch_objects() failed to find some objects, but we ended up having them locally
in a new call to oid_object_info_extended(). That's a fringe case that is worth guarding against
but I wouldn't worry about testing.

> +		}
> +		res = 0;
> +		break;
> +	}
> +
> +	if (to_free)
> +		free(missing_oids);
> +
> +	return res;
> +}

While the test coverage report brought this patch to my attention, it does seem correct.
I still think a test exposing this method would be good, especially one that requires
a fetch_objects() call to multiple remotes to really exercise the details of remove_fetched_oids().

Thanks,
-Stolee