fetch-pack: avoid object flags if no_dependents

When fetch_pack() is invoked as part of another Git command (due to a lazy fetch from a partial clone, for example), it uses object flags that may already be used by the outer Git command. The commit that introduced the lazy fetch feature (88e2f9ed8e ("introduce fetch-object: fetch one promisor object", 2017-12-05)) tried to avoid this overlap, but it did not avoid it totally. It was successful in avoiding writing COMPLETE, but did not avoid reading COMPLETE, and did not avoid writing and reading ALTERNATE. Ensure that no flags are written or read by fetch_pack() in the case where it is used to perform a lazy fetch. To do this, it is sufficient to avoid checking completeness of wanted refs (unnecessary in the case of lazy fetches), and to avoid negotiation-related work (in the current implementation, already, no negotiation is performed). After that was done, the lack of overlap was verified by checking all direct and indirect usages of COMPLETE and ALTERNATE - that they are read or written only if no_dependents is false. There are other possible solutions to this issue: (1) Split fetch-pack.{c,h} into a flag-using part and a non-flag-using part, and whenever no_dependents is set, only use the non-flag-using part. (2) Make fetch_pack() be able to be used with arbitrary repository objects. fetch_pack() should then create its own repository object based on the given repository object, with its own object hashtable, so that the flags do not conflict. (1) is possible but invasive - some functions would need to be split; and such invasiveness would potentially be unnecessary if we ever were to need (2) anyway. (2) would be useful if we were to support, say, submodules that were partial clones themselves, but I don't know when or if the Git project plans to support those. Signed-off-by: Jonathan Tan <jonathantanmy@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Jonathan Tan committed Oct 3, 2018 at 16:04 UTC 12f19a9825686e3641803580bda4e2ab2abd44ed
1 file changed +59 -42
fetch-pack.c
+59 -42
@@ -253,8 +253,10 @@ static int find_common(struct fetch_negotiator *negotiator,
253 if (args->stateless_rpc && multi_ack == 1)
254 die(_("--stateless-rpc requires multi_ack_detailed"));
255
256 - mark_tips(negotiator, args->negotiation_tips);
257 - for_each_cached_alternate(negotiator, insert_one_alternate_object);
256 + if (!args->no_dependents) {
257 + mark_tips(negotiator, args->negotiation_tips);
258 + for_each_cached_alternate(negotiator, insert_one_alternate_object);
259 + }
260
261 fetching = 0;
262 for ( ; refs ; refs = refs->next) {
@@ -271,8 +273,12 @@ static int find_common(struct fetch_negotiator *negotiator,
273 * We use lookup_object here because we are only
274 * interested in the case we *know* the object is
275 * reachable and we have already scanned it.
276 + *
277 + * Do this only if args->no_dependents is false (if it is true,
278 + * we cannot trust the object flags).
279 */
275 - if (((o = lookup_object(the_repository, remote->hash)) != NULL) &&
280 + if (!args->no_dependents &&
281 + ((o = lookup_object(the_repository, remote->hash)) != NULL) &&
282 (o->flags & COMPLETE)) {
283 continue;
284 }
@@ -710,31 +716,29 @@ static void mark_complete_and_common_ref(struct fetch_negotiator *negotiator,
716
717 oidset_clear(&loose_oid_set);
718
713 - if (!args->no_dependents) {
714 - if (!args->deepen) {
715 - for_each_ref(mark_complete_oid, NULL);
716 - for_each_cached_alternate(NULL, mark_alternate_complete);
717 - commit_list_sort_by_date(&complete);
718 - if (cutoff)
719 - mark_recent_complete_commits(args, cutoff);
720 - }
719 + if (!args->deepen) {
720 + for_each_ref(mark_complete_oid, NULL);
721 + for_each_cached_alternate(NULL, mark_alternate_complete);
722 + commit_list_sort_by_date(&complete);
723 + if (cutoff)
724 + mark_recent_complete_commits(args, cutoff);
725 + }
726
722 - /*
723 - * Mark all complete remote refs as common refs.
724 - * Don't mark them common yet; the server has to be told so first.
725 - */
726 - for (ref = *refs; ref; ref = ref->next) {
727 - struct object *o = deref_tag(the_repository,
728 - lookup_object(the_repository,
729 - ref->old_oid.hash),
730 - NULL, 0);
727 + /*
728 + * Mark all complete remote refs as common refs.
729 + * Don't mark them common yet; the server has to be told so first.
730 + */
731 + for (ref = *refs; ref; ref = ref->next) {
732 + struct object *o = deref_tag(the_repository,
733 + lookup_object(the_repository,
734 + ref->old_oid.hash),
735 + NULL, 0);
736
732 - if (!o || o->type != OBJ_COMMIT || !(o->flags & COMPLETE))
733 - continue;
737 + if (!o || o->type != OBJ_COMMIT || !(o->flags & COMPLETE))
738 + continue;
739
735 - negotiator->known_common(negotiator,
736 - (struct commit *)o);
737 - }
740 + negotiator->known_common(negotiator,
741 + (struct commit *)o);
742 }
743
744 save_commit_buffer = old_save_commit_buffer;
@@ -990,11 +994,15 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,
994 if (!server_supports("deepen-relative") && args->deepen_relative)
995 die(_("Server does not support --deepen"));
996
993 - mark_complete_and_common_ref(&negotiator, args, &ref);
994 - filter_refs(args, &ref, sought, nr_sought);
995 - if (everything_local(args, &ref)) {
996 - packet_flush(fd[1]);
997 - goto all_done;
997 + if (!args->no_dependents) {
998 + mark_complete_and_common_ref(&negotiator, args, &ref);
999 + filter_refs(args, &ref, sought, nr_sought);
1000 + if (everything_local(args, &ref)) {
1001 + packet_flush(fd[1]);
1002 + goto all_done;
1003 + }
1004 + } else {
1005 + filter_refs(args, &ref, sought, nr_sought);
1006 }
1007 if (find_common(&negotiator, args, fd, &oid, ref) < 0)
1008 if (!args->keep_pack)
@@ -1040,7 +1048,7 @@ static void add_shallow_requests(struct strbuf *req_buf,
1048 }
1049 }
1050
1043 -static void add_wants(const struct ref *wants, struct strbuf *req_buf)
1051 +static void add_wants(int no_dependents, const struct ref *wants, struct strbuf *req_buf)
1052 {
1053 int use_ref_in_want = server_supports_feature("fetch", "ref-in-want", 0);
1054
@@ -1057,8 +1065,12 @@ static void add_wants(const struct ref *wants, struct strbuf *req_buf)
1065 * We use lookup_object here because we are only
1066 * interested in the case we *know* the object is
1067 * reachable and we have already scanned it.
1068 + *
1069 + * Do this only if args->no_dependents is false (if it is true,
1070 + * we cannot trust the object flags).
1071 */
1061 - if (((o = lookup_object(the_repository, remote->hash)) != NULL) &&
1072 + if (!no_dependents &&
1073 + ((o = lookup_object(the_repository, remote->hash)) != NULL) &&
1074 (o->flags & COMPLETE)) {
1075 continue;
1076 }
@@ -1155,7 +1167,7 @@ static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,
1167 }
1168
1169 /* add wants */
1158 - add_wants(wants, &req_buf);
1170 + add_wants(args->no_dependents, wants, &req_buf);
1171
1172 if (args->no_dependents) {
1173 packet_buf_write(&req_buf, "done");
@@ -1346,16 +1358,21 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
1358 args->deepen = 1;
1359
1360 /* Filter 'ref' by 'sought' and those that aren't local */
1349 - mark_complete_and_common_ref(&negotiator, args, &ref);
1350 - filter_refs(args, &ref, sought, nr_sought);
1351 - if (everything_local(args, &ref))
1352 - state = FETCH_DONE;
1353 - else
1361 + if (!args->no_dependents) {
1362 + mark_complete_and_common_ref(&negotiator, args, &ref);
1363 + filter_refs(args, &ref, sought, nr_sought);
1364 + if (everything_local(args, &ref))
1365 + state = FETCH_DONE;
1366 + else
1367 + state = FETCH_SEND_REQUEST;
1368 +
1369 + mark_tips(&negotiator, args->negotiation_tips);
1370 + for_each_cached_alternate(&negotiator,
1371 + insert_one_alternate_object);
1372 + } else {
1373 + filter_refs(args, &ref, sought, nr_sought);
1374 state = FETCH_SEND_REQUEST;
1355 -
1356 - mark_tips(&negotiator, args->negotiation_tips);
1357 - for_each_cached_alternate(&negotiator,
1358 - insert_one_alternate_object);
1375 + }
1376 break;
1377 case FETCH_SEND_REQUEST:
1378 if (send_fetch_request(&negotiator, fd[1], args, ref,