Re: [PATCH 4/4] promisor-remote: teach lazy-fetch in any repo

[Jonathan Tan <jonathantanmy@google.com>]

> > diff --git a/object-file.c b/object-file.c
> > index f233b440b2..ebf273e9e7 100644
> > --- a/object-file.c
> > +++ b/object-file.c
> > @@ -1570,15 +1570,12 @@ static int do_oid_object_info_extended(struct repository *r,
> >                 }
> >
> >                 /* Check if it is a missing object */
> > -               if (fetch_if_missing && has_promisor_remote() &&
> > -                   !already_retried && r == the_repository &&
> > +               if (fetch_if_missing && repo_has_promisor_remote(r) &&
> > +                   !already_retried &&
> 
> So here you removed the special check against the_repository while
> looking for promisor_remotes.  There are other such special checks in
> the code; I also see:
> 
> diff.c: if (options->repo == the_repository && has_promisor_remote() &&
> diffcore-break.c:       if (r == the_repository && has_promisor_remote()) {
> diffcore-rename.c:      if (r == the_repository && has_promisor_remote()) {
> 
> and a series I'm planning to submit soon will add another to merge.ort.c.
> 
> Do these need to all be fixed as part of the partial clone submodule
> support as well?  Do I need to change anything about my series?  I
> guess since I'm asking that, I should probably submit it first so you
> can actually see it and answer my question.  (And the timing may be
> good since the area is fresh in your memory...)

Thanks for raising this. Looking at the 3 you listed, they all have to
do with prefetching. This is fine both now and later. Now, since partial
clones in submodules still don't work, any fetching of any sort (pre- or
not) will not work. Later, since this prefetching is just an
optimization. (Of course, we should come back and add prefetching for
submodule partial clones, but that is an optimization, not a correctness
issue.)

> >                     !(flags & OBJECT_INFO_SKIP_FETCH_OBJECT)) {
> >                         /*
> >                          * TODO Investigate checking promisor_remote_get_direct()
> >                          * TODO return value and stopping on error here.
> > -                        * TODO Pass a repository struct through
> > -                        * promisor_remote_get_direct(), such that arbitrary
> > -                        * repositories work.
> 
> Odd, it appears that when this comment was added (in commit b14ed5adaf
> ("Use promisor_remote_get_direct() and has_promisor_remote()",
> 2019-06-25)), a repository was passed to promisor_remote_get_direct().
> Sure, it was just a transliteration of the comment that was there
> before when fetch_objects() was the function being called, but since
> the code was being changed and the comment being updated, it seems the
> TODO should have been removed back then.
> 
> Oh, well, good to update it now at least.

Yes - perhaps the comment was emphasizing the "such that arbitrary
repositories work" part. But anyway, yes, it is now removed.

[snip]

> Looks good to me.

Thanks for taking a look.