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

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

On Fri, May 31, 2019 at 7:10 AM Christian Couder
<christian.couder@gmail.com> wrote:

> 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:

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

In the V6 patch series I just sent, the new
OBJECT_INFO_SKIP_FETCH_OBJECT flag that you introduced is used.

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

A "struct repository *r" is passed in V6. I forgot to mention that in
the cover letter.

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

Done in V6 too.

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

Actually missing_nr, or now remaining_nr, would be 0 if all the
promised objects have been fetched.

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

I haven't improved test coverage or looked at how we could better
handle a partial fetch. I plan to look at that soon.

Thanks,
Christian.