On 4/9/2019 12:11 PM, Christian Couder wrote:
> From: Christian Couder <christian.cou...@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
