upload-pack: clear flags before each v2 request

Suppose a server has the following commit graph: A B \ / O We create a client by cloning A from the server with depth 1, and add many commits to it (so that future fetches span multiple requests due to lengthy negotiation). If it then fetches B using protocol v2, the fetch spanning multiple requests, the resulting packfile does not contain O even though the client did report that A is shallow. This is because upload_pack_v2() can be called multiple times while processing the same session. During the 2nd and all subsequent invocations, some object flags remain from the previous invocations. In particular, CLIENT_SHALLOW remains, preventing process_shallow() from adding client-reported shallows to the "shallows" array, and hence pack-objects not knowing about these client-reported shallows. Therefore, teach upload_pack_v2() to clear object flags at the start of each invocation. This has some other results: - THEY_HAVE gates addition of objects to have_obj in process_haves(). Previously in upload_pack_v2(), have_obj needed to be static because once an object is added to have_obj, it is never readded and thus we needed to retain the contents of have_obj between invocations. Now that flags are cleared, this is no longer necessary. This patch does not change the behavior of ok_to_give_up() (THEY_HAVE is still set on each "have") and got_oid() (used only in non-v2)); THEY_HAVE is not used in any other function. - WANTED gates addition of objects to want_obj in parse_want() and parse_want_ref(). It is also used in receive_needs(), but that is only used in non-v2. For the same reasons as THEY_HAVE, want_obj no longer needs to be static in upload_pack_v2(). - CLIENT_SHALLOW is changed as discussed above. Clearing of the other 5 flags does not affect functionality in v2. (Note that in non-v2, upload_pack() is only called once per process, so each invocation starts with blank flags anyway.) - OUR_REF is only used in non-v2. - COMMON_KNOWN is only used as a scratch flag in ok_to_give_up(). - SHALLOW is passed to invocations in deepen() and deepen_by_rev_list(), but upload-pack doesn't use it. - NOT_SHALLOW is used by send_shallow() and send_unshallow(), but invocations of those functions are always preceded by code that sets NOT_SHALLOW on the appropriate objects. - HIDDEN_REF is only used in non-v2. Signed-off-by: Jonathan Tan <jonathantanmy@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Jonathan Tan committed Oct 18, 2018 at 13:43 UTC d1035cac09602590578f70fb0294e97daf03769c
2 files changed +34 -4
t/t5702-protocol-v2.sh
+25
@@ -429,6 +429,31 @@ test_expect_success 'fetch supports include-tag and tag following' '
429 git -C client cat-file -e $(git -C client rev-parse annotated_tag)
430 '
431
432 +test_expect_success 'upload-pack respects client shallows' '
433 + rm -rf server client trace &&
434 +
435 + git init server &&
436 + test_commit -C server base &&
437 + test_commit -C server client_has &&
438 +
439 + git clone --depth=1 "file://$(pwd)/server" client &&
440 +
441 + # Add extra commits to the client so that the whole fetch takes more
442 + # than 1 request (due to negotiation)
443 + for i in $(test_seq 1 32)
444 + do
445 + test_commit -C client c$i
446 + done &&
447 +
448 + git -C server checkout -b newbranch base &&
449 + test_commit -C server client_wants &&
450 +
451 + GIT_TRACE_PACKET="$(pwd)/trace" git -C client -c protocol.version=2 \
452 + fetch origin newbranch &&
453 + # Ensure that protocol v2 is used
454 + grep "fetch< version 2" trace
455 +'
456 +
457 # Test protocol v2 with 'http://' transport
458 #
459 . "$TEST_DIRECTORY"/lib-httpd.sh
upload-pack.c
+9 -4
@@ -38,6 +38,9 @@
38 #define CLIENT_SHALLOW (1u << 18)
39 #define HIDDEN_REF (1u << 19)
40
41 +#define ALL_FLAGS (THEY_HAVE | OUR_REF | WANTED | COMMON_KNOWN | SHALLOW | \
42 + NOT_SHALLOW | CLIENT_SHALLOW | HIDDEN_REF)
43 +
44 static timestamp_t oldest_have;
45
46 static int deepen_relative;
@@ -1411,10 +1414,10 @@ int upload_pack_v2(struct repository *r, struct argv_array *keys,
1414 {
1415 enum fetch_state state = FETCH_PROCESS_ARGS;
1416 struct upload_pack_data data;
1414 - /* NEEDSWORK: make this non-static */
1415 - static struct object_array have_obj;
1416 - /* NEEDSWORK: make this non-static */
1417 - static struct object_array want_obj;
1417 + struct object_array have_obj = OBJECT_ARRAY_INIT;
1418 + struct object_array want_obj = OBJECT_ARRAY_INIT;
1419 +
1420 + clear_object_flags(ALL_FLAGS);
1421
1422 git_config(upload_pack_config, NULL);
1423
@@ -1466,6 +1469,8 @@ int upload_pack_v2(struct repository *r, struct argv_array *keys,
1469 }
1470
1471 upload_pack_data_clear(&data);
1472 + object_array_clear(&have_obj);
1473 + object_array_clear(&want_obj);
1474 return 0;
1475 }
1476