{"thread":{"id":"49581","subject":"[PATCH] upload-pack: clear flags before each v2 request","startedAt":"2018-10-16T21:58:57Z","lastAt":"2018-10-19T03:19:28Z","messageCount":8,"participants":["Jonathan Tan","Arturas Moskvinas","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"360675","messageId":"20181016215850.47821-1-jonathantanmy@google.com","threadId":"49581","inReplyTo":null,"subject":"[PATCH] upload-pack: clear flags before each v2 request","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-10-16T21:58:50Z","receivedAt":"2018-10-16T21:58:57Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Suppose a server has the following commit graph:\n\n A   B\n  \\ /\n   O\n\nWe create a client by cloning A from the server with depth 1, and add\nmany commits to it (so that future fetches span multiple requests due to\nlengthy negotiation). If it then fetches B using protocol v2, the fetch\nspanning multiple requests, the resulting packfile does not contain O\neven though the client did report that A is shallow.\n\nThis is because upload_pack_v2() can be called multiple times while\nprocessing the same session. During the 2nd and all subsequent\ninvocations, some object flags remain from the previous invocations. In\nparticular, CLIENT_SHALLOW remains, preventing process_shallow() from\nadding client-reported shallows to the \"shallows\" array, and hence\npack-objects not knowing about these client-reported shallows.\n\nTherefore, teach upload_pack_v2() to clear object flags at the start of\neach invocation.\n\n(One alternative is to reduce or eliminate usage of object flags in\nprotocol v2, but that doesn't seem feasible because almost all 8 flags\nare used pervasively in v2 code.)\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\nThis was noticed by Arturas Moskvinas <arturas@uber.com> in [1]. The\nreproduction steps given were to repeat a shallow fetch twice in\nsuccession, but I found it easier to write a more understandable test if\nI made the 2nd fetch an ordinary fetch. In any case, I also reran the\noriginal reproduction steps, and the fetch completes without error.\n\nThis patch doesn't cover the negotiation issue that I mentioned in my\nprevious reply [2].\n\n[1] https://public-inbox.org/git/CAGY-PBgsG-T3JY=awszwGMpFx+jDx-a1FcV0s6vR067bSqgZaA@mail.gmail.com/\n[2] https://public-inbox.org/git/20181013004356.257709-1-jonathantanmy@google.com/\n---\n t/t5702-protocol-v2.sh | 25 +++++++++++++++++++++++++\n upload-pack.c          |  5 +++++\n 2 files changed, 30 insertions(+)\n\ndiff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh\nindex 88a886975d..70b88385ba 100755\n--- a/t/t5702-protocol-v2.sh\n+++ b/t/t5702-protocol-v2.sh\n@@ -429,6 +429,31 @@ test_expect_success 'fetch supports include-tag and tag following' '\n \tgit -C client cat-file -e $(git -C client rev-parse annotated_tag)\n '\n \n+test_expect_success 'upload-pack respects client shallows' '\n+\trm -rf server client trace &&\n+\n+\tgit init server &&\n+\ttest_commit -C server base &&\n+\ttest_commit -C server client_has &&\n+\n+\tgit clone --depth=1 \"file://$(pwd)/server\" client &&\n+\n+\t# Add extra commits to the client so that the whole fetch takes more\n+\t# than 1 request (due to negotiation)\n+\tfor i in $(seq 1 32)\n+\tdo\n+\t\ttest_commit -C client c$i\n+\tdone &&\n+\n+\tgit -C server checkout -b newbranch base &&\n+\ttest_commit -C server client_wants &&\n+\n+\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client -c protocol.version=2 \\\n+\t\tfetch origin newbranch &&\n+\t# Ensure that protocol v2 is used\n+\tgrep \"git< version 2\" trace\n+'\n+\n # Test protocol v2 with 'http://' transport\n #\n . \"$TEST_DIRECTORY\"/lib-httpd.sh\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 62a1000f44..de7de1de38 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -37,6 +37,9 @@\n #define CLIENT_SHALLOW\t(1u << 18)\n #define HIDDEN_REF\t(1u << 19)\n \n+#define ALL_FLAGS (THEY_HAVE | OUR_REF | WANTED | COMMON_KNOWN | SHALLOW | \\\n+\t\tNOT_SHALLOW | CLIENT_SHALLOW | HIDDEN_REF)\n+\n static timestamp_t oldest_have;\n \n static int deepen_relative;\n@@ -1393,6 +1396,8 @@ int upload_pack_v2(struct repository *r, struct argv_array *keys,\n \tenum fetch_state state = FETCH_PROCESS_ARGS;\n \tstruct upload_pack_data data;\n \n+\tclear_object_flags(ALL_FLAGS);\n+\n \tgit_config(upload_pack_config, NULL);\n \n \tupload_pack_data_init(&data);\n-- \n2.19.0.271.gfe8321ec05.dirty\n\n"},{"id":"360740","messageId":"CAGY-PBiH1fKebjgCfrcCTpC9Xue7guqYoE51UpBQJ30zJoxGeQ@mail.gmail.com","threadId":"49581","inReplyTo":"20181016215850.47821-1-jonathantanmy@google.com","subject":"Re: [PATCH] upload-pack: clear flags before each v2 request","fromName":"Arturas Moskvinas","fromEmail":"arturas@uber.com","sentAt":"2018-10-17T12:05:17Z","receivedAt":"2018-10-17T12:05:33Z","isPatch":true,"sender":{"key":"arturas@uber.com","avatar":null},"body":"Hello,\n\nI can confirm that shallow clones are no longer failing.\n\n--\nArturas Moskvinas\n\nOn Wed, Oct 17, 2018 at 12:58 AM Jonathan Tan <jonathantanmy@google.com> wrote:\n>\n> Suppose a server has the following commit graph:\n>\n>  A   B\n>   \\ /\n>    O\n>\n> We create a client by cloning A from the server with depth 1, and add\n> many commits to it (so that future fetches span multiple requests due to\n> lengthy negotiation). If it then fetches B using protocol v2, the fetch\n> spanning multiple requests, the resulting packfile does not contain O\n> even though the client did report that A is shallow.\n>\n> This is because upload_pack_v2() can be called multiple times while\n> processing the same session. During the 2nd and all subsequent\n> invocations, some object flags remain from the previous invocations. In\n> particular, CLIENT_SHALLOW remains, preventing process_shallow() from\n> adding client-reported shallows to the \"shallows\" array, and hence\n> pack-objects not knowing about these client-reported shallows.\n>\n> Therefore, teach upload_pack_v2() to clear object flags at the start of\n> each invocation.\n>\n> (One alternative is to reduce or eliminate usage of object flags in\n> protocol v2, but that doesn't seem feasible because almost all 8 flags\n> are used pervasively in v2 code.)\n>\n> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>\n> ---\n> This was noticed by Arturas Moskvinas <arturas@uber.com> in [1]. The\n> reproduction steps given were to repeat a shallow fetch twice in\n> succession, but I found it easier to write a more understandable test if\n> I made the 2nd fetch an ordinary fetch. In any case, I also reran the\n> original reproduction steps, and the fetch completes without error.\n>\n> This patch doesn't cover the negotiation issue that I mentioned in my\n> previous reply [2].\n>\n> [1] https://public-inbox.org/git/CAGY-PBgsG-T3JY=awszwGMpFx+jDx-a1FcV0s6vR067bSqgZaA@mail.gmail.com/\n> [2] https://public-inbox.org/git/20181013004356.257709-1-jonathantanmy@google.com/\n> ---\n>  t/t5702-protocol-v2.sh | 25 +++++++++++++++++++++++++\n>  upload-pack.c          |  5 +++++\n>  2 files changed, 30 insertions(+)\n>\n> diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh\n> index 88a886975d..70b88385ba 100755\n> --- a/t/t5702-protocol-v2.sh\n> +++ b/t/t5702-protocol-v2.sh\n> @@ -429,6 +429,31 @@ test_expect_success 'fetch supports include-tag and tag following' '\n>         git -C client cat-file -e $(git -C client rev-parse annotated_tag)\n>  '\n>\n> +test_expect_success 'upload-pack respects client shallows' '\n> +       rm -rf server client trace &&\n> +\n> +       git init server &&\n> +       test_commit -C server base &&\n> +       test_commit -C server client_has &&\n> +\n> +       git clone --depth=1 \"file://$(pwd)/server\" client &&\n> +\n> +       # Add extra commits to the client so that the whole fetch takes more\n> +       # than 1 request (due to negotiation)\n> +       for i in $(seq 1 32)\n> +       do\n> +               test_commit -C client c$i\n> +       done &&\n> +\n> +       git -C server checkout -b newbranch base &&\n> +       test_commit -C server client_wants &&\n> +\n> +       GIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client -c protocol.version=2 \\\n> +               fetch origin newbranch &&\n> +       # Ensure that protocol v2 is used\n> +       grep \"git< version 2\" trace\n> +'\n> +\n>  # Test protocol v2 with 'http://' transport\n>  #\n>  . \"$TEST_DIRECTORY\"/lib-httpd.sh\n> diff --git a/upload-pack.c b/upload-pack.c\n> index 62a1000f44..de7de1de38 100644\n> --- a/upload-pack.c\n> +++ b/upload-pack.c\n> @@ -37,6 +37,9 @@\n>  #define CLIENT_SHALLOW (1u << 18)\n>  #define HIDDEN_REF     (1u << 19)\n>\n> +#define ALL_FLAGS (THEY_HAVE | OUR_REF | WANTED | COMMON_KNOWN | SHALLOW | \\\n> +               NOT_SHALLOW | CLIENT_SHALLOW | HIDDEN_REF)\n> +\n>  static timestamp_t oldest_have;\n>\n>  static int deepen_relative;\n> @@ -1393,6 +1396,8 @@ int upload_pack_v2(struct repository *r, struct argv_array *keys,\n>         enum fetch_state state = FETCH_PROCESS_ARGS;\n>         struct upload_pack_data data;\n>\n> +       clear_object_flags(ALL_FLAGS);\n> +\n>         git_config(upload_pack_config, NULL);\n>\n>         upload_pack_data_init(&data);\n> --\n> 2.19.0.271.gfe8321ec05.dirty\n>\n"},{"id":"360814","messageId":"xmqq5zxzvnq1.fsf@gitster-ct.c.googlers.com","threadId":"49581","inReplyTo":"20181016215850.47821-1-jonathantanmy@google.com","subject":"Re: [PATCH] upload-pack: clear flags before each v2 request","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-18T05:16:22Z","receivedAt":"2018-10-18T05:16:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan <jonathantanmy@google.com> writes:\n\n> Suppose a server has the following commit graph:\n>\n>  A   B\n>   \\ /\n>    O\n>\n> We create a client by cloning A from the server with depth 1, and add\n> many commits to it (so that future fetches span multiple requests due to\n> lengthy negotiation). If it then fetches B using protocol v2, the fetch\n> spanning multiple requests, the resulting packfile does not contain O\n> even though the client did report that A is shallow.\n\nHmph, what if commit O had a long history behind it?  \n\nShould fetching of B result in fetching the whole history?  Would we\nnotice that now all of A's parents are available locally and declare\nthat the repository is no longer shallow?\n\nI am trying to figure out if \"does not contain O when we fetch B,\neven though we earlier fetched A shallowly, excluding its parents\"\nis unconditionally a bad thing.\n\nThe change to the code itself sort-of makes sense (I say sort-of\nbecause I didn't carefully look at the callers to see if they mind\ngetting all these flags cleared, but the ones that are cleared are\nthe ones that are involved mostly in the negotiation and shold be\nOK).\n\n> diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh\n> index 88a886975d..70b88385ba 100755\n> --- a/t/t5702-protocol-v2.sh\n> +++ b/t/t5702-protocol-v2.sh\n> @@ -429,6 +429,31 @@ test_expect_success 'fetch supports include-tag and tag following' '\n>  \tgit -C client cat-file -e $(git -C client rev-parse annotated_tag)\n>  '\n>  \n> +test_expect_success 'upload-pack respects client shallows' '\n> +\trm -rf server client trace &&\n> +\n> +\tgit init server &&\n> +\ttest_commit -C server base &&\n> +\ttest_commit -C server client_has &&\n> +\n> +\tgit clone --depth=1 \"file://$(pwd)/server\" client &&\n> +\n> +\t# Add extra commits to the client so that the whole fetch takes more\n> +\t# than 1 request (due to negotiation)\n> +\tfor i in $(seq 1 32)\n\nUse test_seq instead, or you'll get hit by test-lint?\n\nApplied on 'master' or 'maint', this new test does not pass even\nwith s/seq/test_&/, so there may be something else wrong with it,\nthough.\n"},{"id":"360873","messageId":"cover.1539893192.git.jonathantanmy@google.com","threadId":"49581","inReplyTo":"20181016215850.47821-1-jonathantanmy@google.com","subject":"[PATCH v2 0/3] Clear flags before each v2 request","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-10-18T20:43:26Z","receivedAt":"2018-10-18T20:43:35Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"To explain the differences in this version of the patch set, I'll quote\nan email [1] from Junio:\n\n[1] https://public-inbox.org/git/xmqq5zxzvnq1.fsf@gitster-ct.c.googlers.com/\n\n> The change to the code itself sort-of makes sense (I say sort-of\n> because I didn't carefully look at the callers to see if they mind\n> getting all these flags cleared, but the ones that are cleared are\n> the ones that are involved mostly in the negotiation and shold be\n> OK).\n\nI have included 2 additional patches for these reasons:\n\n - After reading the section of Junio's email quoted above, I took\n   another look at the flags, and found that not only is state stored in\n   the flags between invocations of upload_pack_v2(), state is also\n   stored in the want_obj and have_obj global variables. The additional\n   patches help clean those up.\n\n - To help reviewers who want to see if the callers mind getting all 8\n   flags cleared, I have included a discussion of all 8 flags in the\n   commit message of patch 3. The additional patches made the discussion\n   easier.\n\nResponses to other points:\n\n> Hmph, what if commit O had a long history behind it?  \n> \n> Should fetching of B result in fetching the whole history?\n\nI think so - when we fetch without --depth or any similar arguments, I\nthink it's reasonable to have all objects referenced by the fetched\ntips.\n\n> Would we\n> notice that now all of A's parents are available locally and declare\n> that the repository is no longer shallow?\n\nWe could, but I think this is outside the scope of this patch set.\n\n> Use test_seq instead, or you'll get hit by test-lint?\n\nThanks for the pointer to test-lint. I've used test_seq, and checked\nthat test-lint doesn't print any errors.\n\n> Applied on 'master' or 'maint', this new test does not pass even\n> with s/seq/test_&/, so there may be something else wrong with it,\n> though.\n\nThanks - there was a copy-and-paste error (should have grepped for\n\"fetch< version 2\", not \"git< version 2\").\n\nJonathan Tan (3):\n  upload-pack: make have_obj not global\n  upload-pack: make want_obj not global\n  upload-pack: clear flags before each v2 request\n\n t/t5702-protocol-v2.sh |  25 +++++++\n upload-pack.c          | 153 ++++++++++++++++++++++++-----------------\n 2 files changed, 115 insertions(+), 63 deletions(-)\n\n-- \n2.19.0.271.gfe8321ec05.dirty\n\n"},{"id":"360874","messageId":"8dc7bfb2e06e5fbb6bf4e0fb50173e7b291a7763.1539893192.git.jonathantanmy@google.com","threadId":"49581","inReplyTo":"cover.1539893192.git.jonathantanmy@google.com","subject":"[PATCH v2 1/3] upload-pack: make have_obj not global","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-10-18T20:43:27Z","receivedAt":"2018-10-18T20:43:38Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Because upload_pack_v2() can be invoked multiple times in the same\nprocess, the static variable have_obj may not be empty when it is\ninvoked. To make further analysis of this situation easier, make the\nvariable local; analysis will be done in a subsequent patch.\n\nThe new local variable in upload_pack_v2() is static to preserve\nexisting behavior; this is not necessary in upload_pack() because\nupload_pack() is only invoked once per process.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\n upload-pack.c | 58 ++++++++++++++++++++++++++++-----------------------\n 1 file changed, 32 insertions(+), 26 deletions(-)\n\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 62a1000f44..cb2401f90d 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -52,7 +52,6 @@ static int no_progress, daemon_mode;\n #define ALLOW_ANY_SHA1\t07\n static unsigned int allow_unadvertised_object_request;\n static int shallow_nr;\n-static struct object_array have_obj;\n static struct object_array want_obj;\n static struct object_array extra_edge_obj;\n static unsigned int timeout;\n@@ -99,7 +98,7 @@ static int write_one_shallow(const struct commit_graft *graft, void *cb_data)\n \treturn 0;\n }\n \n-static void create_pack_file(void)\n+static void create_pack_file(const struct object_array *have_obj)\n {\n \tstruct child_process pack_objects = CHILD_PROCESS_INIT;\n \tchar data[8193], progress[128];\n@@ -164,9 +163,9 @@ static void create_pack_file(void)\n \t\tfprintf(pipe_fd, \"%s\\n\",\n \t\t\toid_to_hex(&want_obj.objects[i].item->oid));\n \tfprintf(pipe_fd, \"--not\\n\");\n-\tfor (i = 0; i < have_obj.nr; i++)\n+\tfor (i = 0; i < have_obj->nr; i++)\n \t\tfprintf(pipe_fd, \"%s\\n\",\n-\t\t\toid_to_hex(&have_obj.objects[i].item->oid));\n+\t\t\toid_to_hex(&have_obj->objects[i].item->oid));\n \tfor (i = 0; i < extra_edge_obj.nr; i++)\n \t\tfprintf(pipe_fd, \"%s\\n\",\n \t\t\toid_to_hex(&extra_edge_obj.objects[i].item->oid));\n@@ -303,7 +302,8 @@ static void create_pack_file(void)\n \tdie(\"git upload-pack: %s\", abort_msg);\n }\n \n-static int got_oid(const char *hex, struct object_id *oid)\n+static int got_oid(const char *hex, struct object_id *oid,\n+\t\t   struct object_array *have_obj)\n {\n \tstruct object *o;\n \tint we_knew_they_have = 0;\n@@ -331,17 +331,17 @@ static int got_oid(const char *hex, struct object_id *oid)\n \t\t\tparents->item->object.flags |= THEY_HAVE;\n \t}\n \tif (!we_knew_they_have) {\n-\t\tadd_object_array(o, NULL, &have_obj);\n+\t\tadd_object_array(o, NULL, have_obj);\n \t\treturn 1;\n \t}\n \treturn 0;\n }\n \n-static int ok_to_give_up(void)\n+static int ok_to_give_up(const struct object_array *have_obj)\n {\n \tuint32_t min_generation = GENERATION_NUMBER_ZERO;\n \n-\tif (!have_obj.nr)\n+\tif (!have_obj->nr)\n \t\treturn 0;\n \n \treturn can_all_from_reach_with_flag(&want_obj, THEY_HAVE,\n@@ -349,7 +349,7 @@ static int ok_to_give_up(void)\n \t\t\t\t\t    min_generation);\n }\n \n-static int get_common_commits(void)\n+static int get_common_commits(struct object_array *have_obj)\n {\n \tstruct object_id oid;\n \tchar last_hex[GIT_MAX_HEXSZ + 1];\n@@ -367,11 +367,11 @@ static int get_common_commits(void)\n \n \t\tif (!line) {\n \t\t\tif (multi_ack == 2 && got_common\n-\t\t\t    && !got_other && ok_to_give_up()) {\n+\t\t\t    && !got_other && ok_to_give_up(have_obj)) {\n \t\t\t\tsent_ready = 1;\n \t\t\t\tpacket_write_fmt(1, \"ACK %s ready\\n\", last_hex);\n \t\t\t}\n-\t\t\tif (have_obj.nr == 0 || multi_ack)\n+\t\t\tif (have_obj->nr == 0 || multi_ack)\n \t\t\t\tpacket_write_fmt(1, \"NAK\\n\");\n \n \t\t\tif (no_done && sent_ready) {\n@@ -385,10 +385,10 @@ static int get_common_commits(void)\n \t\t\tcontinue;\n \t\t}\n \t\tif (skip_prefix(line, \"have \", &arg)) {\n-\t\t\tswitch (got_oid(arg, &oid)) {\n+\t\t\tswitch (got_oid(arg, &oid, have_obj)) {\n \t\t\tcase -1: /* they have what we do not */\n \t\t\t\tgot_other = 1;\n-\t\t\t\tif (multi_ack && ok_to_give_up()) {\n+\t\t\t\tif (multi_ack && ok_to_give_up(have_obj)) {\n \t\t\t\t\tconst char *hex = oid_to_hex(&oid);\n \t\t\t\t\tif (multi_ack == 2) {\n \t\t\t\t\t\tsent_ready = 1;\n@@ -404,14 +404,14 @@ static int get_common_commits(void)\n \t\t\t\t\tpacket_write_fmt(1, \"ACK %s common\\n\", last_hex);\n \t\t\t\telse if (multi_ack)\n \t\t\t\t\tpacket_write_fmt(1, \"ACK %s continue\\n\", last_hex);\n-\t\t\t\telse if (have_obj.nr == 1)\n+\t\t\t\telse if (have_obj->nr == 1)\n \t\t\t\t\tpacket_write_fmt(1, \"ACK %s\\n\", last_hex);\n \t\t\t\tbreak;\n \t\t\t}\n \t\t\tcontinue;\n \t\t}\n \t\tif (!strcmp(line, \"done\")) {\n-\t\t\tif (have_obj.nr > 0) {\n+\t\t\tif (have_obj->nr > 0) {\n \t\t\t\tif (multi_ack)\n \t\t\t\t\tpacket_write_fmt(1, \"ACK %s\\n\", last_hex);\n \t\t\t\treturn 0;\n@@ -1065,8 +1065,9 @@ void upload_pack(struct upload_pack_options *options)\n \n \treceive_needs();\n \tif (want_obj.nr) {\n-\t\tget_common_commits();\n-\t\tcreate_pack_file();\n+\t\tstruct object_array have_obj = OBJECT_ARRAY_INIT;\n+\t\tget_common_commits(&have_obj);\n+\t\tcreate_pack_file(&have_obj);\n \t}\n }\n \n@@ -1254,7 +1255,8 @@ static void process_args(struct packet_reader *request,\n \t}\n }\n \n-static int process_haves(struct oid_array *haves, struct oid_array *common)\n+static int process_haves(struct oid_array *haves, struct oid_array *common,\n+\t\t\t struct object_array *have_obj)\n {\n \tint i;\n \n@@ -1287,13 +1289,14 @@ static int process_haves(struct oid_array *haves, struct oid_array *common)\n \t\t\t\tparents->item->object.flags |= THEY_HAVE;\n \t\t}\n \t\tif (!we_knew_they_have)\n-\t\t\tadd_object_array(o, NULL, &have_obj);\n+\t\t\tadd_object_array(o, NULL, have_obj);\n \t}\n \n \treturn 0;\n }\n \n-static int send_acks(struct oid_array *acks, struct strbuf *response)\n+static int send_acks(struct oid_array *acks, struct strbuf *response,\n+\t\t     const struct object_array *have_obj)\n {\n \tint i;\n \n@@ -1308,7 +1311,7 @@ static int send_acks(struct oid_array *acks, struct strbuf *response)\n \t\t\t\t oid_to_hex(&acks->oid[i]));\n \t}\n \n-\tif (ok_to_give_up()) {\n+\tif (ok_to_give_up(have_obj)) {\n \t\t/* Send Ready */\n \t\tpacket_buf_write(response, \"ready\\n\");\n \t\treturn 1;\n@@ -1317,16 +1320,17 @@ static int send_acks(struct oid_array *acks, struct strbuf *response)\n \treturn 0;\n }\n \n-static int process_haves_and_send_acks(struct upload_pack_data *data)\n+static int process_haves_and_send_acks(struct upload_pack_data *data,\n+\t\t\t\t       struct object_array *have_obj)\n {\n \tstruct oid_array common = OID_ARRAY_INIT;\n \tstruct strbuf response = STRBUF_INIT;\n \tint ret = 0;\n \n-\tprocess_haves(&data->haves, &common);\n+\tprocess_haves(&data->haves, &common, have_obj);\n \tif (data->done) {\n \t\tret = 1;\n-\t} else if (send_acks(&common, &response)) {\n+\t} else if (send_acks(&common, &response, have_obj)) {\n \t\tpacket_buf_delim(&response);\n \t\tret = 1;\n \t} else {\n@@ -1392,6 +1396,8 @@ int upload_pack_v2(struct repository *r, struct argv_array *keys,\n {\n \tenum fetch_state state = FETCH_PROCESS_ARGS;\n \tstruct upload_pack_data data;\n+\t/* NEEDSWORK: make this non-static */\n+\tstatic struct object_array have_obj;\n \n \tgit_config(upload_pack_config, NULL);\n \n@@ -1423,7 +1429,7 @@ int upload_pack_v2(struct repository *r, struct argv_array *keys,\n \t\t\t}\n \t\t\tbreak;\n \t\tcase FETCH_SEND_ACKS:\n-\t\t\tif (process_haves_and_send_acks(&data))\n+\t\t\tif (process_haves_and_send_acks(&data, &have_obj))\n \t\t\t\tstate = FETCH_SEND_PACK;\n \t\t\telse\n \t\t\t\tstate = FETCH_DONE;\n@@ -1433,7 +1439,7 @@ int upload_pack_v2(struct repository *r, struct argv_array *keys,\n \t\t\tsend_shallow_info(&data);\n \n \t\t\tpacket_write_fmt(1, \"packfile\\n\");\n-\t\t\tcreate_pack_file();\n+\t\t\tcreate_pack_file(&have_obj);\n \t\t\tstate = FETCH_DONE;\n \t\t\tbreak;\n \t\tcase FETCH_DONE:\n-- \n2.19.0.271.gfe8321ec05.dirty\n\n"},{"id":"360875","messageId":"3853c006257c418ee5179aa7c35c9e9e31eeef1f.1539893192.git.jonathantanmy@google.com","threadId":"49581","inReplyTo":"cover.1539893192.git.jonathantanmy@google.com","subject":"[PATCH v2 2/3] upload-pack: make want_obj not global","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-10-18T20:43:28Z","receivedAt":"2018-10-18T20:43:40Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Because upload_pack_v2() can be invoked multiple times in the same\nprocess, the static variable want_obj may not be empty when it is\ninvoked. To make further analysis of this situation easier, make the\nvariable local; analysis will be done in a subsequent patch.\n\nThe new local variable in upload_pack_v2() is static to preserve\nexisting behavior; this is not necessary in upload_pack() because\nupload_pack() is only invoked once per process.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\n upload-pack.c | 116 ++++++++++++++++++++++++++++----------------------\n 1 file changed, 66 insertions(+), 50 deletions(-)\n\ndiff --git a/upload-pack.c b/upload-pack.c\nindex cb2401f90d..451bf47e7f 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -52,7 +52,6 @@ static int no_progress, daemon_mode;\n #define ALLOW_ANY_SHA1\t07\n static unsigned int allow_unadvertised_object_request;\n static int shallow_nr;\n-static struct object_array want_obj;\n static struct object_array extra_edge_obj;\n static unsigned int timeout;\n static int keepalive = 5;\n@@ -98,7 +97,8 @@ static int write_one_shallow(const struct commit_graft *graft, void *cb_data)\n \treturn 0;\n }\n \n-static void create_pack_file(const struct object_array *have_obj)\n+static void create_pack_file(const struct object_array *have_obj,\n+\t\t\t     const struct object_array *want_obj)\n {\n \tstruct child_process pack_objects = CHILD_PROCESS_INIT;\n \tchar data[8193], progress[128];\n@@ -159,9 +159,9 @@ static void create_pack_file(const struct object_array *have_obj)\n \tif (shallow_nr)\n \t\tfor_each_commit_graft(write_one_shallow, pipe_fd);\n \n-\tfor (i = 0; i < want_obj.nr; i++)\n+\tfor (i = 0; i < want_obj->nr; i++)\n \t\tfprintf(pipe_fd, \"%s\\n\",\n-\t\t\toid_to_hex(&want_obj.objects[i].item->oid));\n+\t\t\toid_to_hex(&want_obj->objects[i].item->oid));\n \tfprintf(pipe_fd, \"--not\\n\");\n \tfor (i = 0; i < have_obj->nr; i++)\n \t\tfprintf(pipe_fd, \"%s\\n\",\n@@ -337,19 +337,21 @@ static int got_oid(const char *hex, struct object_id *oid,\n \treturn 0;\n }\n \n-static int ok_to_give_up(const struct object_array *have_obj)\n+static int ok_to_give_up(const struct object_array *have_obj,\n+\t\t\t struct object_array *want_obj)\n {\n \tuint32_t min_generation = GENERATION_NUMBER_ZERO;\n \n \tif (!have_obj->nr)\n \t\treturn 0;\n \n-\treturn can_all_from_reach_with_flag(&want_obj, THEY_HAVE,\n+\treturn can_all_from_reach_with_flag(want_obj, THEY_HAVE,\n \t\t\t\t\t    COMMON_KNOWN, oldest_have,\n \t\t\t\t\t    min_generation);\n }\n \n-static int get_common_commits(struct object_array *have_obj)\n+static int get_common_commits(struct object_array *have_obj,\n+\t\t\t      struct object_array *want_obj)\n {\n \tstruct object_id oid;\n \tchar last_hex[GIT_MAX_HEXSZ + 1];\n@@ -367,7 +369,7 @@ static int get_common_commits(struct object_array *have_obj)\n \n \t\tif (!line) {\n \t\t\tif (multi_ack == 2 && got_common\n-\t\t\t    && !got_other && ok_to_give_up(have_obj)) {\n+\t\t\t    && !got_other && ok_to_give_up(have_obj, want_obj)) {\n \t\t\t\tsent_ready = 1;\n \t\t\t\tpacket_write_fmt(1, \"ACK %s ready\\n\", last_hex);\n \t\t\t}\n@@ -388,7 +390,7 @@ static int get_common_commits(struct object_array *have_obj)\n \t\t\tswitch (got_oid(arg, &oid, have_obj)) {\n \t\t\tcase -1: /* they have what we do not */\n \t\t\t\tgot_other = 1;\n-\t\t\t\tif (multi_ack && ok_to_give_up(have_obj)) {\n+\t\t\t\tif (multi_ack && ok_to_give_up(have_obj, want_obj)) {\n \t\t\t\t\tconst char *hex = oid_to_hex(&oid);\n \t\t\t\t\tif (multi_ack == 2) {\n \t\t\t\t\t\tsent_ready = 1;\n@@ -581,7 +583,7 @@ static int has_unreachable(struct object_array *src)\n \treturn 1;\n }\n \n-static void check_non_tip(void)\n+static void check_non_tip(struct object_array *want_obj)\n {\n \tint i;\n \n@@ -592,14 +594,14 @@ static void check_non_tip(void)\n \t */\n \tif (!stateless_rpc && !(allow_unadvertised_object_request & ALLOW_REACHABLE_SHA1))\n \t\tgoto error;\n-\tif (!has_unreachable(&want_obj))\n+\tif (!has_unreachable(want_obj))\n \t\t/* All the non-tip ones are ancestors of what we advertised */\n \t\treturn;\n \n error:\n \t/* Pick one of them (we know there at least is one) */\n-\tfor (i = 0; i < want_obj.nr; i++) {\n-\t\tstruct object *o = want_obj.objects[i].item;\n+\tfor (i = 0; i < want_obj->nr; i++) {\n+\t\tstruct object *o = want_obj->objects[i].item;\n \t\tif (!is_our_ref(o))\n \t\t\tdie(\"git upload-pack: not our ref %s\",\n \t\t\t    oid_to_hex(&o->oid));\n@@ -620,7 +622,8 @@ static void send_shallow(struct commit_list *result)\n \t}\n }\n \n-static void send_unshallow(const struct object_array *shallows)\n+static void send_unshallow(const struct object_array *shallows,\n+\t\t\t   struct object_array *want_obj)\n {\n \tint i;\n \n@@ -644,7 +647,7 @@ static void send_unshallow(const struct object_array *shallows)\n \t\t\tparents = ((struct commit *)object)->parents;\n \t\t\twhile (parents) {\n \t\t\t\tadd_object_array(&parents->item->object,\n-\t\t\t\t\t\t NULL, &want_obj);\n+\t\t\t\t\t\t NULL, want_obj);\n \t\t\t\tparents = parents->next;\n \t\t\t}\n \t\t\tadd_object_array(object, NULL, &extra_edge_obj);\n@@ -655,7 +658,7 @@ static void send_unshallow(const struct object_array *shallows)\n }\n \n static void deepen(int depth, int deepen_relative,\n-\t\t   struct object_array *shallows)\n+\t\t   struct object_array *shallows, struct object_array *want_obj)\n {\n \tif (depth == INFINITE_DEPTH && !is_repository_shallow(the_repository)) {\n \t\tint i;\n@@ -678,38 +681,40 @@ static void deepen(int depth, int deepen_relative,\n \t} else {\n \t\tstruct commit_list *result;\n \n-\t\tresult = get_shallow_commits(&want_obj, depth,\n+\t\tresult = get_shallow_commits(want_obj, depth,\n \t\t\t\t\t     SHALLOW, NOT_SHALLOW);\n \t\tsend_shallow(result);\n \t\tfree_commit_list(result);\n \t}\n \n-\tsend_unshallow(shallows);\n+\tsend_unshallow(shallows, want_obj);\n }\n \n static void deepen_by_rev_list(int ac, const char **av,\n-\t\t\t       struct object_array *shallows)\n+\t\t\t       struct object_array *shallows,\n+\t\t\t       struct object_array *want_obj)\n {\n \tstruct commit_list *result;\n \n \tresult = get_shallow_commits_by_rev_list(ac, av, SHALLOW, NOT_SHALLOW);\n \tsend_shallow(result);\n \tfree_commit_list(result);\n-\tsend_unshallow(shallows);\n+\tsend_unshallow(shallows, want_obj);\n }\n \n /* Returns 1 if a shallow list is sent or 0 otherwise */\n static int send_shallow_list(int depth, int deepen_rev_list,\n \t\t\t     timestamp_t deepen_since,\n \t\t\t     struct string_list *deepen_not,\n-\t\t\t     struct object_array *shallows)\n+\t\t\t     struct object_array *shallows,\n+\t\t\t     struct object_array *want_obj)\n {\n \tint ret = 0;\n \n \tif (depth > 0 && deepen_rev_list)\n \t\tdie(\"git upload-pack: deepen and deepen-since (or deepen-not) cannot be used together\");\n \tif (depth > 0) {\n-\t\tdeepen(depth, deepen_relative, shallows);\n+\t\tdeepen(depth, deepen_relative, shallows, want_obj);\n \t\tret = 1;\n \t} else if (deepen_rev_list) {\n \t\tstruct argv_array av = ARGV_ARRAY_INIT;\n@@ -726,11 +731,11 @@ static int send_shallow_list(int depth, int deepen_rev_list,\n \t\t\t}\n \t\t\targv_array_push(&av, \"--not\");\n \t\t}\n-\t\tfor (i = 0; i < want_obj.nr; i++) {\n-\t\t\tstruct object *o = want_obj.objects[i].item;\n+\t\tfor (i = 0; i < want_obj->nr; i++) {\n+\t\t\tstruct object *o = want_obj->objects[i].item;\n \t\t\targv_array_push(&av, oid_to_hex(&o->oid));\n \t\t}\n-\t\tdeepen_by_rev_list(av.argc, av.argv, shallows);\n+\t\tdeepen_by_rev_list(av.argc, av.argv, shallows, want_obj);\n \t\targv_array_clear(&av);\n \t\tret = 1;\n \t} else {\n@@ -815,7 +820,7 @@ static int process_deepen_not(const char *line, struct string_list *deepen_not,\n \treturn 0;\n }\n \n-static void receive_needs(void)\n+static void receive_needs(struct object_array *want_obj)\n {\n \tstruct object_array shallows = OBJECT_ARRAY_INIT;\n \tstruct string_list deepen_not = STRING_LIST_INIT_DUP;\n@@ -893,7 +898,7 @@ static void receive_needs(void)\n \t\t\tif (!((allow_unadvertised_object_request & ALLOW_ANY_SHA1) == ALLOW_ANY_SHA1\n \t\t\t      || is_our_ref(o)))\n \t\t\t\thas_non_tip = 1;\n-\t\t\tadd_object_array(o, NULL, &want_obj);\n+\t\t\tadd_object_array(o, NULL, want_obj);\n \t\t}\n \t}\n \n@@ -905,7 +910,7 @@ static void receive_needs(void)\n \t * by another process that handled the initial request.\n \t */\n \tif (has_non_tip)\n-\t\tcheck_non_tip();\n+\t\tcheck_non_tip(want_obj);\n \n \tif (!use_sideband && daemon_mode)\n \t\tno_progress = 1;\n@@ -914,7 +919,7 @@ static void receive_needs(void)\n \t\treturn;\n \n \tif (send_shallow_list(depth, deepen_rev_list, deepen_since,\n-\t\t\t      &deepen_not, &shallows))\n+\t\t\t      &deepen_not, &shallows, want_obj))\n \t\tpacket_flush(1);\n \tobject_array_clear(&shallows);\n }\n@@ -1040,6 +1045,7 @@ static int upload_pack_config(const char *var, const char *value, void *unused)\n void upload_pack(struct upload_pack_options *options)\n {\n \tstruct string_list symref = STRING_LIST_INIT_DUP;\n+\tstruct object_array want_obj = OBJECT_ARRAY_INIT;\n \n \tstateless_rpc = options->stateless_rpc;\n \ttimeout = options->timeout;\n@@ -1063,11 +1069,11 @@ void upload_pack(struct upload_pack_options *options)\n \tif (options->advertise_refs)\n \t\treturn;\n \n-\treceive_needs();\n+\treceive_needs(&want_obj);\n \tif (want_obj.nr) {\n \t\tstruct object_array have_obj = OBJECT_ARRAY_INIT;\n-\t\tget_common_commits(&have_obj);\n-\t\tcreate_pack_file(&have_obj);\n+\t\tget_common_commits(&have_obj, &want_obj);\n+\t\tcreate_pack_file(&have_obj, &want_obj);\n \t}\n }\n \n@@ -1117,7 +1123,7 @@ static void upload_pack_data_clear(struct upload_pack_data *data)\n \tstring_list_clear(&data->deepen_not, 0);\n }\n \n-static int parse_want(const char *line)\n+static int parse_want(const char *line, struct object_array *want_obj)\n {\n \tconst char *arg;\n \tif (skip_prefix(line, \"want \", &arg)) {\n@@ -1139,7 +1145,7 @@ static int parse_want(const char *line)\n \n \t\tif (!(o->flags & WANTED)) {\n \t\t\to->flags |= WANTED;\n-\t\t\tadd_object_array(o, NULL, &want_obj);\n+\t\t\tadd_object_array(o, NULL, want_obj);\n \t\t}\n \n \t\treturn 1;\n@@ -1148,7 +1154,8 @@ static int parse_want(const char *line)\n \treturn 0;\n }\n \n-static int parse_want_ref(const char *line, struct string_list *wanted_refs)\n+static int parse_want_ref(const char *line, struct string_list *wanted_refs,\n+\t\t\t  struct object_array *want_obj)\n {\n \tconst char *arg;\n \tif (skip_prefix(line, \"want-ref \", &arg)) {\n@@ -1167,7 +1174,7 @@ static int parse_want_ref(const char *line, struct string_list *wanted_refs)\n \t\to = parse_object_or_die(&oid, arg);\n \t\tif (!(o->flags & WANTED)) {\n \t\t\to->flags |= WANTED;\n-\t\t\tadd_object_array(o, NULL, &want_obj);\n+\t\t\tadd_object_array(o, NULL, want_obj);\n \t\t}\n \n \t\treturn 1;\n@@ -1192,16 +1199,18 @@ static int parse_have(const char *line, struct oid_array *haves)\n }\n \n static void process_args(struct packet_reader *request,\n-\t\t\t struct upload_pack_data *data)\n+\t\t\t struct upload_pack_data *data,\n+\t\t\t struct object_array *want_obj)\n {\n \twhile (packet_reader_read(request) != PACKET_READ_FLUSH) {\n \t\tconst char *arg = request->line;\n \t\tconst char *p;\n \n \t\t/* process want */\n-\t\tif (parse_want(arg))\n+\t\tif (parse_want(arg, want_obj))\n \t\t\tcontinue;\n-\t\tif (allow_ref_in_want && parse_want_ref(arg, &data->wanted_refs))\n+\t\tif (allow_ref_in_want &&\n+\t\t    parse_want_ref(arg, &data->wanted_refs, want_obj))\n \t\t\tcontinue;\n \t\t/* process have line */\n \t\tif (parse_have(arg, &data->haves))\n@@ -1296,7 +1305,8 @@ static int process_haves(struct oid_array *haves, struct oid_array *common,\n }\n \n static int send_acks(struct oid_array *acks, struct strbuf *response,\n-\t\t     const struct object_array *have_obj)\n+\t\t     const struct object_array *have_obj,\n+\t\t     struct object_array *want_obj)\n {\n \tint i;\n \n@@ -1311,7 +1321,7 @@ static int send_acks(struct oid_array *acks, struct strbuf *response,\n \t\t\t\t oid_to_hex(&acks->oid[i]));\n \t}\n \n-\tif (ok_to_give_up(have_obj)) {\n+\tif (ok_to_give_up(have_obj, want_obj)) {\n \t\t/* Send Ready */\n \t\tpacket_buf_write(response, \"ready\\n\");\n \t\treturn 1;\n@@ -1321,7 +1331,8 @@ static int send_acks(struct oid_array *acks, struct strbuf *response,\n }\n \n static int process_haves_and_send_acks(struct upload_pack_data *data,\n-\t\t\t\t       struct object_array *have_obj)\n+\t\t\t\t       struct object_array *have_obj,\n+\t\t\t\t       struct object_array *want_obj)\n {\n \tstruct oid_array common = OID_ARRAY_INIT;\n \tstruct strbuf response = STRBUF_INIT;\n@@ -1330,7 +1341,7 @@ static int process_haves_and_send_acks(struct upload_pack_data *data,\n \tprocess_haves(&data->haves, &common, have_obj);\n \tif (data->done) {\n \t\tret = 1;\n-\t} else if (send_acks(&common, &response, have_obj)) {\n+\t} else if (send_acks(&common, &response, have_obj, want_obj)) {\n \t\tpacket_buf_delim(&response);\n \t\tret = 1;\n \t} else {\n@@ -1366,7 +1377,8 @@ static void send_wanted_ref_info(struct upload_pack_data *data)\n \tpacket_delim(1);\n }\n \n-static void send_shallow_info(struct upload_pack_data *data)\n+static void send_shallow_info(struct upload_pack_data *data,\n+\t\t\t      struct object_array *want_obj)\n {\n \t/* No shallow info needs to be sent */\n \tif (!data->depth && !data->deepen_rev_list && !data->shallows.nr &&\n@@ -1377,9 +1389,10 @@ static void send_shallow_info(struct upload_pack_data *data)\n \n \tif (!send_shallow_list(data->depth, data->deepen_rev_list,\n \t\t\t       data->deepen_since, &data->deepen_not,\n-\t\t\t       &data->shallows) &&\n+\t\t\t       &data->shallows, want_obj) &&\n \t    is_repository_shallow(the_repository))\n-\t\tdeepen(INFINITE_DEPTH, data->deepen_relative, &data->shallows);\n+\t\tdeepen(INFINITE_DEPTH, data->deepen_relative, &data->shallows,\n+\t\t       want_obj);\n \n \tpacket_delim(1);\n }\n@@ -1398,6 +1411,8 @@ int upload_pack_v2(struct repository *r, struct argv_array *keys,\n \tstruct upload_pack_data data;\n \t/* NEEDSWORK: make this non-static */\n \tstatic struct object_array have_obj;\n+\t/* NEEDSWORK: make this non-static */\n+\tstatic struct object_array want_obj;\n \n \tgit_config(upload_pack_config, NULL);\n \n@@ -1407,7 +1422,7 @@ int upload_pack_v2(struct repository *r, struct argv_array *keys,\n \twhile (state != FETCH_DONE) {\n \t\tswitch (state) {\n \t\tcase FETCH_PROCESS_ARGS:\n-\t\t\tprocess_args(request, &data);\n+\t\t\tprocess_args(request, &data, &want_obj);\n \n \t\t\tif (!want_obj.nr) {\n \t\t\t\t/*\n@@ -1429,17 +1444,18 @@ int upload_pack_v2(struct repository *r, struct argv_array *keys,\n \t\t\t}\n \t\t\tbreak;\n \t\tcase FETCH_SEND_ACKS:\n-\t\t\tif (process_haves_and_send_acks(&data, &have_obj))\n+\t\t\tif (process_haves_and_send_acks(&data, &have_obj,\n+\t\t\t\t\t\t\t&want_obj))\n \t\t\t\tstate = FETCH_SEND_PACK;\n \t\t\telse\n \t\t\t\tstate = FETCH_DONE;\n \t\t\tbreak;\n \t\tcase FETCH_SEND_PACK:\n \t\t\tsend_wanted_ref_info(&data);\n-\t\t\tsend_shallow_info(&data);\n+\t\t\tsend_shallow_info(&data, &want_obj);\n \n \t\t\tpacket_write_fmt(1, \"packfile\\n\");\n-\t\t\tcreate_pack_file(&have_obj);\n+\t\t\tcreate_pack_file(&have_obj, &want_obj);\n \t\t\tstate = FETCH_DONE;\n \t\t\tbreak;\n \t\tcase FETCH_DONE:\n-- \n2.19.0.271.gfe8321ec05.dirty\n\n"},{"id":"360876","messageId":"d50b8bc81f4380ee4dea87e9131d67aeddfffe48.1539893192.git.jonathantanmy@google.com","threadId":"49581","inReplyTo":"cover.1539893192.git.jonathantanmy@google.com","subject":"[PATCH v2 3/3] upload-pack: clear flags before each v2 request","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-10-18T20:43:29Z","receivedAt":"2018-10-18T20:43:42Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Suppose a server has the following commit graph:\n\n A   B\n  \\ /\n   O\n\nWe create a client by cloning A from the server with depth 1, and add\nmany commits to it (so that future fetches span multiple requests due to\nlengthy negotiation). If it then fetches B using protocol v2, the fetch\nspanning multiple requests, the resulting packfile does not contain O\neven though the client did report that A is shallow.\n\nThis is because upload_pack_v2() can be called multiple times while\nprocessing the same session. During the 2nd and all subsequent\ninvocations, some object flags remain from the previous invocations. In\nparticular, CLIENT_SHALLOW remains, preventing process_shallow() from\nadding client-reported shallows to the \"shallows\" array, and hence\npack-objects not knowing about these client-reported shallows.\n\nTherefore, teach upload_pack_v2() to clear object flags at the start of\neach invocation. This has some other results:\n\n - THEY_HAVE gates addition of objects to have_obj in process_haves().\n   Previously in upload_pack_v2(), have_obj needed to be static because\n   once an object is added to have_obj, it is never readded and thus we\n   needed to retain the contents of have_obj between invocations. Now\n   that flags are cleared, this is no longer necessary. This patch does\n   not change the behavior of ok_to_give_up() (THEY_HAVE is still set on\n   each \"have\") and got_oid() (used only in non-v2)); THEY_HAVE is not\n   used in any other function.\n\n - WANTED gates addition of objects to want_obj in parse_want() and\n   parse_want_ref(). It is also used in receive_needs(), but that is\n   only used in non-v2. For the same reasons as THEY_HAVE, want_obj no\n   longer needs to be static in upload_pack_v2().\n\n - CLIENT_SHALLOW is changed as discussed above.\n\nClearing of the other 5 flags does not affect functionality in v2. (Note\nthat in non-v2, upload_pack() is only called once per process, so each\ninvocation starts with blank flags anyway.)\n\n - OUR_REF is only used in non-v2.\n\n - COMMON_KNOWN is only used as a scratch flag in ok_to_give_up().\n\n - SHALLOW is passed to invocations in deepen() and\n   deepen_by_rev_list(), but upload-pack doesn't use it.\n\n - NOT_SHALLOW is used by send_shallow() and send_unshallow(), but\n   invocations of those functions are always preceded by code that sets\n   NOT_SHALLOW on the appropriate objects.\n\n - HIDDEN_REF is only used in non-v2.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\n t/t5702-protocol-v2.sh | 25 +++++++++++++++++++++++++\n upload-pack.c          | 13 +++++++++----\n 2 files changed, 34 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh\nindex 88a886975d..4adc4b00e3 100755\n--- a/t/t5702-protocol-v2.sh\n+++ b/t/t5702-protocol-v2.sh\n@@ -429,6 +429,31 @@ test_expect_success 'fetch supports include-tag and tag following' '\n \tgit -C client cat-file -e $(git -C client rev-parse annotated_tag)\n '\n \n+test_expect_success 'upload-pack respects client shallows' '\n+\trm -rf server client trace &&\n+\n+\tgit init server &&\n+\ttest_commit -C server base &&\n+\ttest_commit -C server client_has &&\n+\n+\tgit clone --depth=1 \"file://$(pwd)/server\" client &&\n+\n+\t# Add extra commits to the client so that the whole fetch takes more\n+\t# than 1 request (due to negotiation)\n+\tfor i in $(test_seq 1 32)\n+\tdo\n+\t\ttest_commit -C client c$i\n+\tdone &&\n+\n+\tgit -C server checkout -b newbranch base &&\n+\ttest_commit -C server client_wants &&\n+\n+\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client -c protocol.version=2 \\\n+\t\tfetch origin newbranch &&\n+\t# Ensure that protocol v2 is used\n+\tgrep \"fetch< version 2\" trace\n+'\n+\n # Test protocol v2 with 'http://' transport\n #\n . \"$TEST_DIRECTORY\"/lib-httpd.sh\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 451bf47e7f..42c5ec44cf 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -37,6 +37,9 @@\n #define CLIENT_SHALLOW\t(1u << 18)\n #define HIDDEN_REF\t(1u << 19)\n \n+#define ALL_FLAGS (THEY_HAVE | OUR_REF | WANTED | COMMON_KNOWN | SHALLOW | \\\n+\t\tNOT_SHALLOW | CLIENT_SHALLOW | HIDDEN_REF)\n+\n static timestamp_t oldest_have;\n \n static int deepen_relative;\n@@ -1409,10 +1412,10 @@ int upload_pack_v2(struct repository *r, struct argv_array *keys,\n {\n \tenum fetch_state state = FETCH_PROCESS_ARGS;\n \tstruct upload_pack_data data;\n-\t/* NEEDSWORK: make this non-static */\n-\tstatic struct object_array have_obj;\n-\t/* NEEDSWORK: make this non-static */\n-\tstatic struct object_array want_obj;\n+\tstruct object_array have_obj = OBJECT_ARRAY_INIT;\n+\tstruct object_array want_obj = OBJECT_ARRAY_INIT;\n+\n+\tclear_object_flags(ALL_FLAGS);\n \n \tgit_config(upload_pack_config, NULL);\n \n@@ -1464,6 +1467,8 @@ int upload_pack_v2(struct repository *r, struct argv_array *keys,\n \t}\n \n \tupload_pack_data_clear(&data);\n+\tobject_array_clear(&have_obj);\n+\tobject_array_clear(&want_obj);\n \treturn 0;\n }\n \n-- \n2.19.0.271.gfe8321ec05.dirty\n\n"},{"id":"360896","messageId":"xmqq8t2u39ok.fsf@gitster-ct.c.googlers.com","threadId":"49581","inReplyTo":"cover.1539893192.git.jonathantanmy@google.com","subject":"Re: [PATCH v2 0/3] Clear flags before each v2 request","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-19T03:19:23Z","receivedAt":"2018-10-19T03:19:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan <jonathantanmy@google.com> writes:\n\n> Jonathan Tan (3):\n>   upload-pack: make have_obj not global\n>   upload-pack: make want_obj not global\n>   upload-pack: clear flags before each v2 request\n\nIt took a bit of time why 2/3 did not apply cleanly but it turns out\nthis is based on a slightly older tip of 'master' (but still ahead\nof 'maint'), that lacks 829a3215 (\"commit-graph: close_commit_graph\nbefore shallow walk\", 2018-08-20).  Applying and merging it to make\nit up-to-date with the tip of 'master' went smoothly once I figured\nit out.\n\nThe first two clean-up patches are probably overdue and worth doing\nregardless of the bugfix.  Nicely done.\n\nThe first two steps use static objects local to a transport method\nto \"preserve the existing behaviour\", and because this codepath\nhappens to want a clean slate every time it gets called, the third\nstep manages to lose it, which is a nice progression.  But it makes\nme wonder if it also hints that there may be a need to invent a\nstate object that is passed around from the transport layer across\nrequests, if we want to fulfill a request by calling multiple\ntransport methods in the future.  In any case, there is no immediate\nneed to address this comment ;-).\n\nWill replace.  Thanks.\n\n"}]}