Thread (59 messages) 59 messages, 5 authors, 2021-06-19

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

flat view

From: Jonathan Tan <hidden>
Date: 2021-06-05 02:16:19

quoted
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.)
quoted
                    !(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.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help