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>

diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
index 98fbf39da325ab574f9717a03c6a6588b15f0ffa..8e3b773e3f5b96b8429ec75ba7d5e233276f34ba 100755 (executable)
--- a/t/t5702-protocol-v2.sh
+++ b/t/t5702-protocol-v2.sh
@@ -429,6 +429,31 @@ test_expect_success 'fetch supports include-tag and tag following' '
        git -C client cat-file -e $(git -C client rev-parse annotated_tag)
 '
 
+test_expect_success 'upload-pack respects client shallows' '
+       rm -rf server client trace &&
+
+       git init server &&
+       test_commit -C server base &&
+       test_commit -C server client_has &&
+
+       git clone --depth=1 "file://$(pwd)/server" client &&
+
+       # Add extra commits to the client so that the whole fetch takes more
+       # than 1 request (due to negotiation)
+       for i in $(test_seq 1 32)
+       do
+               test_commit -C client c$i
+       done &&
+
+       git -C server checkout -b newbranch base &&
+       test_commit -C server client_wants &&
+
+       GIT_TRACE_PACKET="$(pwd)/trace" git -C client -c protocol.version=2 \
+               fetch origin newbranch &&
+       # Ensure that protocol v2 is used
+       grep "fetch< version 2" trace
+'
+
 # Test protocol v2 with 'http://' transport
 #
 . "$TEST_DIRECTORY"/lib-httpd.sh

diff --git a/upload-pack.c b/upload-pack.c
index 1d1deae18f9e0e0f350c74540b085ff674f2385b..14e42526ce4c002ecba9d572d8fc4ff12611d33b 100644 (file)
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -38,6 +38,9 @@
 #define CLIENT_SHALLOW (1u << 18)
 #define HIDDEN_REF     (1u << 19)
 
+#define ALL_FLAGS (THEY_HAVE | OUR_REF | WANTED | COMMON_KNOWN | SHALLOW | \
+               NOT_SHALLOW | CLIENT_SHALLOW | HIDDEN_REF)
+
 static timestamp_t oldest_have;
 
 static int deepen_relative;
@@ -1411,10 +1414,10 @@ int upload_pack_v2(struct repository *r, struct argv_array *keys,
 {
        enum fetch_state state = FETCH_PROCESS_ARGS;
        struct upload_pack_data data;
-       /* NEEDSWORK: make this non-static */
-       static struct object_array have_obj;
-       /* NEEDSWORK: make this non-static */
-       static struct object_array want_obj;
+       struct object_array have_obj = OBJECT_ARRAY_INIT;
+       struct object_array want_obj = OBJECT_ARRAY_INIT;
+
+       clear_object_flags(ALL_FLAGS);
 
        git_config(upload_pack_config, NULL);
 
@@ -1466,6 +1469,8 @@ int upload_pack_v2(struct repository *r, struct argv_array *keys,
        }
 
        upload_pack_data_clear(&data);
+       object_array_clear(&have_obj);
+       object_array_clear(&want_obj);
        return 0;
 }