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

[Christian Couder <christian.couder@gmail.com>]

Hi Stolee,

On Thu, May 30, 2019 at 7:21 PM Derrick Stolee <stolee@gmail.com> wrote:
>
> 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.
>
> Sorry for jumping on the thread late, but I noticed some peculiarities
> when looking at the test coverage report.

You are welcome. It needs review according to Junio, so it's
definitely a good thing that you take a look at it.

> > +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?

I think so. If I remember correctly, I added this following a review
by Junio because it could be possible that a promisor/partial clone
remote only sends parts of the promisor objects it is asked. In this
case the objects that have been fetched should be removed from the
list of objects we try to fetch from the next promisor/partial clone
remote.

The issue is that now if a promisor/partial clone remote can send only
parts of the promisor objects it is asked, it should fail, as far as I
understand, which means that we will not actually get the objects it
should send. That's why I think it's not easy, or perhaps even not
possible, to test this.

> > +{
> > +     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?

If such a flag existed when I wrote the function I would certainly
have used it, as I also dislike this kind of messing with a global
(and globals in general).

I will see if I can do something about it according to what you
suggest later in this thread.

> > +     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).

Ok, I will take a look at that.

> > +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.

Ok, I will take a look at 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 don't think so but I will check again, and maybe add a comment.

> 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().

Yeah, I would like to actually test it. I will take another look at
what can be done to test this. Perhaps I will look at what can be done
to still get some objects when fetching from a promisor/partial clone
remote even when it doesn't have all of the objects we request.

Thanks for the review,
Christian.