Re: [PATCH v5 04/16] promisor-remote: implement promisor_remote_get_direct()
- From
Derrick Stolee <stolee@gmail.com>
- Date
- May 30, 2019, 17:21 UTC
- Message-ID
- <b4d69d2b-dc0d-fffb-2909-c54060fe9cd1@gmail.com>
- In-Reply-To
- <20190409161116.30256-5-chriscool@tuxfamily.org>
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?
Show 8 quoted lines
> +{
> + 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).
Show 27 quoted lines
> + 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".
Show 7 quoted lines
> + 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.
Show 10 quoted lines
> + } > + 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