promisor-remote: remove fetch_if_missing=0

Commit 6462d5eb9a ("fetch: remove fetch_if_missing=0", 2019-11-08)
strove to remove the need for fetch_if_missing=0 from the fetching
mechanism, so it is plausible to attempt removing fetch_if_missing=0
from the lazy-fetching mechanism in promisor-remote as well.

But doing so reveals a bug - when the server does not send an object
pointed to by a tag object, an infinite loop occurs: Git attempts to
fetch the missing object, which causes a deferencing of all refs (for
negotiation), which causes a lazy fetch of that missing object, and so
on. This bug is because of unnecessary use of the fetch negotiator
during lazy fetching - it is not used after initialization, but it is
still initialized (which causes the dereferencing of all refs).

Thus, when the negotiator is not used during fetching, refrain from
initializing it. Then, remove fetch_if_missing from promisor-remote.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/fetch-pack.c b/fetch-pack.c
index 37178e2d3400599e5ca793d98a0ab1fe969ec1fd..490b1118224d8cbe1877dd680960fdc059152ee4 100644 (file)
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -896,8 +896,15 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,
        struct object_id oid;
        const char *agent_feature;
        int agent_len;
-       struct fetch_negotiator negotiator;
-       fetch_negotiator_init(r, &negotiator);
+       struct fetch_negotiator negotiator_alloc;
+       struct fetch_negotiator *negotiator;
+
+       if (args->no_dependents) {
+               negotiator = NULL;
+       } else {
+               negotiator = &negotiator_alloc;
+               fetch_negotiator_init(r, negotiator);
+       }
 
        sort_ref_list(&ref, ref_compare_name);
        QSORT(sought, nr_sought, cmp_ref_by_name);
@@ -984,7 +991,7 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,
                die(_("Server does not support --deepen"));
 
        if (!args->no_dependents) {
-               mark_complete_and_common_ref(&negotiator, args, &ref);
+               mark_complete_and_common_ref(negotiator, args, &ref);
                filter_refs(args, &ref, sought, nr_sought);
                if (everything_local(args, &ref)) {
                        packet_flush(fd[1]);
@@ -993,7 +1000,7 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,
        } else {
                filter_refs(args, &ref, sought, nr_sought);
        }
-       if (find_common(&negotiator, args, fd, &oid, ref) < 0)
+       if (find_common(negotiator, args, fd, &oid, ref) < 0)
                if (!args->keep_pack)
                        /* When cloning, it is not unusual to have
                         * no common commit.
@@ -1013,7 +1020,8 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,
                die(_("git fetch-pack: fetch failed."));
 
  all_done:
-       negotiator.release(&negotiator);
+       if (negotiator)
+               negotiator->release(negotiator);
        return ref;
 }
 
@@ -1231,7 +1239,8 @@ static int process_acks(struct fetch_negotiator *negotiator,
                                struct commit *commit;
                                oidset_insert(common, &oid);
                                commit = lookup_commit(the_repository, &oid);
-                               negotiator->ack(negotiator, commit);
+                               if (negotiator)
+                                       negotiator->ack(negotiator, commit);
                        }
                        continue;
                }
@@ -1383,8 +1392,16 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
        struct packet_reader reader;
        int in_vain = 0, negotiation_started = 0;
        int haves_to_send = INITIAL_FLUSH;
-       struct fetch_negotiator negotiator;
-       fetch_negotiator_init(r, &negotiator);
+       struct fetch_negotiator negotiator_alloc;
+       struct fetch_negotiator *negotiator;
+
+       if (args->no_dependents) {
+               negotiator = NULL;
+       } else {
+               negotiator = &negotiator_alloc;
+               fetch_negotiator_init(r, negotiator);
+       }
+
        packet_reader_init(&reader, fd[0], NULL, 0,
                           PACKET_READ_CHOMP_NEWLINE |
                           PACKET_READ_DIE_ON_ERR_PACKET);
@@ -1408,15 +1425,15 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
 
                        /* Filter 'ref' by 'sought' and those that aren't local */
                        if (!args->no_dependents) {
-                               mark_complete_and_common_ref(&negotiator, args, &ref);
+                               mark_complete_and_common_ref(negotiator, args, &ref);
                                filter_refs(args, &ref, sought, nr_sought);
                                if (everything_local(args, &ref))
                                        state = FETCH_DONE;
                                else
                                        state = FETCH_SEND_REQUEST;
 
-                               mark_tips(&negotiator, args->negotiation_tips);
-                               for_each_cached_alternate(&negotiator,
+                               mark_tips(negotiator, args->negotiation_tips);
+                               for_each_cached_alternate(negotiator,
                                                          insert_one_alternate_object);
                        } else {
                                filter_refs(args, &ref, sought, nr_sought);
@@ -1430,7 +1447,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
                                                    "negotiation_v2",
                                                    the_repository);
                        }
-                       if (send_fetch_request(&negotiator, fd[1], args, ref,
+                       if (send_fetch_request(negotiator, fd[1], args, ref,
                                               &common,
                                               &haves_to_send, &in_vain,
                                               reader.use_sideband))
@@ -1440,7 +1457,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
                        break;
                case FETCH_PROCESS_ACKS:
                        /* Process ACKs/NAKs */
-                       switch (process_acks(&negotiator, &reader, &common)) {
+                       switch (process_acks(negotiator, &reader, &common)) {
                        case 2:
                                state = FETCH_GET_PACK;
                                break;
@@ -1475,7 +1492,8 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
                }
        }
 
-       negotiator.release(&negotiator);
+       if (negotiator)
+               negotiator->release(negotiator);
        oidset_clear(&common);
        return ref;
 }

diff --git a/promisor-remote.c b/promisor-remote.c
index 9bd5b79d59446dede45ea075fc8d7307fcb98648..9f338c945faf997a11d2a55065ddbc14d8be6cdc 100644 (file)
--- a/promisor-remote.c
+++ b/promisor-remote.c
@@ -16,10 +16,8 @@ static int fetch_refs(const char *remote_name, struct ref *ref)
 {
        struct remote *remote;
        struct transport *transport;
-       int original_fetch_if_missing = fetch_if_missing;
        int res;
 
-       fetch_if_missing = 0;
        remote = remote_get(remote_name);
        if (!remote->url[0])
                die(_("Remote with no URL"));
@@ -28,7 +26,6 @@ static int fetch_refs(const char *remote_name, struct ref *ref)
        transport_set_option(transport, TRANS_OPT_FROM_PROMISOR, "1");
        transport_set_option(transport, TRANS_OPT_NO_DEPENDENTS, "1");
        res = transport_fetch_refs(transport, ref);
-       fetch_if_missing = original_fetch_if_missing;
 
        return res;
 }