{"thread":{"id":"55459","subject":"[PATCH 0/6] Push negotiation","startedAt":"2021-04-09T01:10:10Z","lastAt":"2021-05-06T02:12:19Z","messageCount":33,"participants":["Jonathan Tan","Junio C Hamano","Derrick Stolee"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"421317","messageId":"cover.1617929278.git.jonathantanmy@google.com","threadId":"55459","inReplyTo":null,"subject":"[PATCH 0/6] Push negotiation","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-04-09T01:09:57Z","receivedAt":"2021-04-09T01:10:10Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Here are patches for push negotiation. The basic idea is that we can\nreuse part of the fetch protocol (and code) to find out what is in\ncommon between the client and server, and then use that information to\nfurther narrow the objects sent in the packfile during a push.\n\nPatch 1 is a bug fix that probably should be merged even if the rest\naren't. Patches 2-4 are refactorings in preparation for the future\npatches. Patches 5-6 contain the actual logic and documentation.\n\nI have written more about it in my prior work [1], although the commit\nmessages and documentation in patches 5-6 should be enough to explain\nwhat's going on. (If they're not, feel free to make review comments.)\n\nThe main change from [1] is that the client-side code that used to be in\nbuiltin/fetch-pack.c is now in builtin/fetch.c, because I realized that\nbuiltin/fetch-pack.c does not support HTTP. Other than that, all the\n\"what hasn't been done yet\" items have been done except for statistics\nin the commit message.\n\n[1] https://lore.kernel.org/git/20210218012100.928957-1-jonathantanmy@google.com/\n\nJonathan Tan (6):\n fetch-pack: buffer object-format with other args\n fetch-pack: refactor process_acks()\n fetch-pack: refactor add_haves()\n fetch-pack: refactor command and capability write\n fetch: teach independent negotiation (no packfile)\n send-pack: support push negotiation\n\n Documentation/config/push.txt | 7 +\n Documentation/technical/protocol-v2.txt | 8 +\n builtin/fetch.c | 27 ++-\n fetch-pack.c | 224 +++++++++++++++---------\n fetch-pack.h | 11 ++\n object.h | 2 +-\n send-pack.c | 61 ++++++-\n t/t5516-fetch-push.sh | 35 ++++\n t/t5701-git-serve.sh | 2 +-\n t/t5702-protocol-v2.sh | 89 ++++++++++\n transport-helper.c | 10 ++\n transport.c | 30 +++-\n transport.h | 6 +\n upload-pack.c | 18 +-\n 14 files changed, 430 insertions(+), 100 deletions(-)\n\n-- \n2.31.1.295.g9ea45b61b8-goog\n\n"},{"id":"421318","messageId":"86e4ce0e8d9665b1c5a2b30173be4afffe0a0abd.1617929278.git.jonathantanmy@google.com","threadId":"55459","inReplyTo":"cover.1617929278.git.jonathantanmy@google.com","subject":"[PATCH 1/6] fetch-pack: buffer object-format with other args","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-04-09T01:09:58Z","receivedAt":"2021-04-09T01:10:11Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"In send_fetch_request(), \"object-format\" is written directly to the file\ndescriptor, as opposed to the other arguments, which are buffered.\nBuffer \"object-format\" as well. \"object-format\" must be buffered; in\nparticular, it must appear after \"command=fetch\" in the request.\n\nThis divergence was introduced in 4b831208bb (\"fetch-pack: parse and\nadvertise the object-format capability\", 2020-05-27), perhaps as an\noversight (the surrounding code at the point of this commit has already\nbeen using a request buffer.)\n\nSigned-off-by: Jonathan Tan \n---\n fetch-pack.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 6e68276aa3..2318ebe680 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -1251,7 +1251,7 @@ static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,\n \t\tif (hash_algo_by_ptr(the_hash_algo) != hash_algo)\n \t\t\tdie(_(\"mismatched algorithms: client %s; server %s\"),\n \t\t\t the_hash_algo->name, hash_name);\n-\t\tpacket_write_fmt(fd_out, \"object-format=%s\", the_hash_algo->name);\n+\t\tpacket_buf_write(&req_buf, \"object-format=%s\", the_hash_algo->name);\n \t} else if (hash_algo_by_ptr(the_hash_algo) != GIT_HASH_SHA1) {\n \t\tdie(_(\"the server does not support algorithm '%s'\"),\n \t\t the_hash_algo->name);\n-- \n2.31.1.295.g9ea45b61b8-goog\n\n"},{"id":"421319","messageId":"6a9f78df1a3513df04ac3275cd1feccfb6cf87b1.1617929278.git.jonathantanmy@google.com","threadId":"55459","inReplyTo":"cover.1617929278.git.jonathantanmy@google.com","subject":"[PATCH 2/6] fetch-pack: refactor process_acks()","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-04-09T01:09:59Z","receivedAt":"2021-04-09T01:10:15Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"A subsequent commit will need part, but not all, of the functionality in\nprocess_acks(), so move some of its functionality to its sole caller\ndo_fetch_pack_v2(). As a side effect, the resulting code is also\nshorter.\n\nSigned-off-by: Jonathan Tan \n---\n fetch-pack.c | 70 +++++++++++++++++-----------------------------------\n 1 file changed, 22 insertions(+), 48 deletions(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 2318ebe680..9f3901cdba 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -1351,35 +1351,11 @@ static int process_section_header(struct packet_reader *reader,\n \treturn ret;\n }\n \n-enum common_found {\n-\t/*\n-\t * No commit was found to be possessed by both the client and the\n-\t * server, and \"ready\" was not received.\n-\t */\n-\tNO_COMMON_FOUND,\n-\n-\t/*\n-\t * At least one commit was found to be possessed by both the client and\n-\t * the server, and \"ready\" was not received.\n-\t */\n-\tCOMMON_FOUND,\n-\n-\t/*\n-\t * \"ready\" was received, indicating that the server is ready to send\n-\t * the packfile without any further negotiation.\n-\t */\n-\tREADY\n-};\n-\n-static enum common_found process_acks(struct fetch_negotiator *negotiator,\n-\t\t\t\t struct packet_reader *reader,\n-\t\t\t\t struct oidset *common)\n+static int process_ack(struct fetch_negotiator *negotiator,\n+\t\t struct packet_reader *reader,\n+\t\t struct object_id *common_oid,\n+\t\t int *received_ready)\n {\n-\t/* received */\n-\tint received_ready = 0;\n-\tint received_ack = 0;\n-\n-\tprocess_section_header(reader, \"acknowledgments\", 0);\n \twhile (packet_reader_read(reader) == PACKET_READ_NORMAL) {\n \t\tconst char *arg;\n \n@@ -1387,20 +1363,17 @@ static enum common_found process_acks(struct fetch_negotiator *negotiator,\n \t\t\tcontinue;\n \n \t\tif (skip_prefix(reader->line, \"ACK \", &arg)) {\n-\t\t\tstruct object_id oid;\n-\t\t\treceived_ack = 1;\n-\t\t\tif (!get_oid_hex(arg, &oid)) {\n+\t\t\tif (!get_oid_hex(arg, common_oid)) {\n \t\t\t\tstruct commit *commit;\n-\t\t\t\toidset_insert(common, &oid);\n-\t\t\t\tcommit = lookup_commit(the_repository, &oid);\n+\t\t\t\tcommit = lookup_commit(the_repository, common_oid);\n \t\t\t\tif (negotiator)\n \t\t\t\t\tnegotiator->ack(negotiator, commit);\n \t\t\t}\n-\t\t\tcontinue;\n+\t\t\treturn 1;\n \t\t}\n \n \t\tif (!strcmp(reader->line, \"ready\")) {\n-\t\t\treceived_ready = 1;\n+\t\t\t*received_ready = 1;\n \t\t\tcontinue;\n \t\t}\n \n@@ -1418,13 +1391,12 @@ static enum common_found process_acks(struct fetch_negotiator *negotiator,\n \t * sent. Therefore, a DELIM is expected if \"ready\" is sent, and a FLUSH\n \t * otherwise.\n \t */\n-\tif (received_ready && reader->status != PACKET_READ_DELIM)\n+\tif (*received_ready && reader->status != PACKET_READ_DELIM)\n \t\tdie(_(\"expected packfile to be sent after 'ready'\"));\n-\tif (!received_ready && reader->status != PACKET_READ_FLUSH)\n+\tif (!*received_ready && reader->status != PACKET_READ_FLUSH)\n \t\tdie(_(\"expected no other sections to be sent after no 'ready'\"));\n \n-\treturn received_ready ? READY :\n-\t\t(received_ack ? COMMON_FOUND : NO_COMMON_FOUND);\n+\treturn 0;\n }\n \n static void receive_shallow_info(struct fetch_pack_args *args,\n@@ -1573,6 +1545,8 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \tstruct fetch_negotiator negotiator_alloc;\n \tstruct fetch_negotiator *negotiator;\n \tint seen_ack = 0;\n+\tstruct object_id common_oid;\n+\tint received_ready = 0;\n \tstruct string_list packfile_uris = STRING_LIST_INIT_DUP;\n \tint i;\n \tstruct strvec index_pack_args = STRVEC_INIT;\n@@ -1631,22 +1605,22 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \t\t\tbreak;\n \t\tcase FETCH_PROCESS_ACKS:\n \t\t\t/* Process ACKs/NAKs */\n-\t\t\tswitch (process_acks(negotiator, &reader, &common)) {\n-\t\t\tcase READY:\n+\t\t\tprocess_section_header(&reader, \"acknowledgments\", 0);\n+\t\t\twhile (process_ack(negotiator, &reader, &common_oid,\n+\t\t\t\t\t &received_ready)) {\n+\t\t\t\tin_vain = 0;\n+\t\t\t\tseen_ack = 1;\n+\t\t\t\toidset_insert(&common, &common_oid);\n+\t\t\t}\n+\t\t\tif (received_ready) {\n \t\t\t\t/*\n \t\t\t\t * Don't check for response delimiter; get_pack() will\n \t\t\t\t * read the rest of this response.\n \t\t\t\t */\n \t\t\t\tstate = FETCH_GET_PACK;\n-\t\t\t\tbreak;\n-\t\t\tcase COMMON_FOUND:\n-\t\t\t\tin_vain = 0;\n-\t\t\t\tseen_ack = 1;\n-\t\t\t\t/* fallthrough */\n-\t\t\tcase NO_COMMON_FOUND:\n+\t\t\t} else {\n \t\t\t\tdo_check_stateless_delimiter(args, &reader);\n \t\t\t\tstate = FETCH_SEND_REQUEST;\n-\t\t\t\tbreak;\n \t\t\t}\n \t\t\tbreak;\n \t\tcase FETCH_GET_PACK:\n-- \n2.31.1.295.g9ea45b61b8-goog\n\n"},{"id":"421320","messageId":"b07e52ec18f46fb85d49ba0e1e6e8ad22845af9e.1617929278.git.jonathantanmy@google.com","threadId":"55459","inReplyTo":"cover.1617929278.git.jonathantanmy@google.com","subject":"[PATCH 3/6] fetch-pack: refactor add_haves()","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-04-09T01:10:00Z","receivedAt":"2021-04-09T01:10:17Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"A subsequent commit will need part, but not all, of the functionality in\nadd_haves(), so move some of its functionality to its sole caller\nsend_fetch_request().\n\nSigned-off-by: Jonathan Tan \n---\n fetch-pack.c | 28 ++++++++++++----------------\n 1 file changed, 12 insertions(+), 16 deletions(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 9f3901cdba..128ad47d2a 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -1195,11 +1195,9 @@ static void add_common(struct strbuf *req_buf, struct oidset *common)\n }\n \n static int add_haves(struct fetch_negotiator *negotiator,\n-\t\t int seen_ack,\n \t\t struct strbuf *req_buf,\n-\t\t int *haves_to_send, int *in_vain)\n+\t\t int *haves_to_send)\n {\n-\tint ret = 0;\n \tint haves_added = 0;\n \tconst struct object_id *oid;\n \n@@ -1209,17 +1207,10 @@ static int add_haves(struct fetch_negotiator *negotiator,\n \t\t\tbreak;\n \t}\n \n-\t*in_vain += haves_added;\n-\tif (!haves_added || (seen_ack && *in_vain >= MAX_IN_VAIN)) {\n-\t\t/* Send Done */\n-\t\tpacket_buf_write(req_buf, \"done\\n\");\n-\t\tret = 1;\n-\t}\n-\n \t/* Increase haves to send on next round */\n \t*haves_to_send = next_flush(1, *haves_to_send);\n \n-\treturn ret;\n+\treturn haves_added;\n }\n \n static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,\n@@ -1228,7 +1219,8 @@ static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,\n \t\t\t int *haves_to_send, int *in_vain,\n \t\t\t int sideband_all, int seen_ack)\n {\n-\tint ret = 0;\n+\tint haves_added;\n+\tint done_sent = 0;\n \tconst char *hash_name;\n \tstruct strbuf req_buf = STRBUF_INIT;\n \n@@ -1312,9 +1304,13 @@ static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,\n \t/* Add all of the common commits we've found in previous rounds */\n \tadd_common(&req_buf, common);\n \n-\t/* Add initial haves */\n-\tret = add_haves(negotiator, seen_ack, &req_buf,\n-\t\t\thaves_to_send, in_vain);\n+\thaves_added = add_haves(negotiator, &req_buf, haves_to_send);\n+\t*in_vain += haves_added;\n+\tif (!haves_added || (seen_ack && *in_vain >= MAX_IN_VAIN)) {\n+\t\t/* Send Done */\n+\t\tpacket_buf_write(&req_buf, \"done\\n\");\n+\t\tdone_sent = 1;\n+\t}\n \n \t/* Send request */\n \tpacket_buf_flush(&req_buf);\n@@ -1322,7 +1318,7 @@ static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,\n \t\tdie_errno(_(\"unable to write request to remote\"));\n \n \tstrbuf_release(&req_buf);\n-\treturn ret;\n+\treturn done_sent;\n }\n \n /*\n-- \n2.31.1.295.g9ea45b61b8-goog\n\n"},{"id":"421321","messageId":"7988c106873332ac6e5c2f9dd143cfa1f50e067c.1617929278.git.jonathantanmy@google.com","threadId":"55459","inReplyTo":"cover.1617929278.git.jonathantanmy@google.com","subject":"[PATCH 4/6] fetch-pack: refactor command and capability write","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-04-09T01:10:01Z","receivedAt":"2021-04-09T01:10:19Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"A subsequent commit will need this functionality independent of the rest\nof send_fetch_request(), so put this into its own function.\n\nSigned-off-by: Jonathan Tan \n---\n fetch-pack.c | 41 ++++++++++++++++++++++++-----------------\n 1 file changed, 24 insertions(+), 17 deletions(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 128ad47d2a..512fe5450d 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -1213,29 +1213,23 @@ static int add_haves(struct fetch_negotiator *negotiator,\n \treturn haves_added;\n }\n \n-static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,\n-\t\t\t struct fetch_pack_args *args,\n-\t\t\t const struct ref *wants, struct oidset *common,\n-\t\t\t int *haves_to_send, int *in_vain,\n-\t\t\t int sideband_all, int seen_ack)\n+static void write_fetch_command_and_capabilities(struct strbuf *req_buf,\n+\t\t\t\t\t\t const struct string_list *server_options)\n {\n-\tint haves_added;\n-\tint done_sent = 0;\n \tconst char *hash_name;\n-\tstruct strbuf req_buf = STRBUF_INIT;\n \n \tif (server_supports_v2(\"fetch\", 1))\n-\t\tpacket_buf_write(&req_buf, \"command=fetch\");\n+\t\tpacket_buf_write(req_buf, \"command=fetch\");\n \tif (server_supports_v2(\"agent\", 0))\n-\t\tpacket_buf_write(&req_buf, \"agent=%s\", git_user_agent_sanitized());\n+\t\tpacket_buf_write(req_buf, \"agent=%s\", git_user_agent_sanitized());\n \tif (advertise_sid && server_supports_v2(\"session-id\", 0))\n-\t\tpacket_buf_write(&req_buf, \"session-id=%s\", trace2_session_id());\n-\tif (args->server_options && args->server_options->nr &&\n+\t\tpacket_buf_write(req_buf, \"session-id=%s\", trace2_session_id());\n+\tif (server_options && server_options->nr &&\n \t server_supports_v2(\"server-option\", 1)) {\n \t\tint i;\n-\t\tfor (i = 0; i < args->server_options->nr; i++)\n-\t\t\tpacket_buf_write(&req_buf, \"server-option=%s\",\n-\t\t\t\t\t args->server_options->items[i].string);\n+\t\tfor (i = 0; i < server_options->nr; i++)\n+\t\t\tpacket_buf_write(req_buf, \"server-option=%s\",\n+\t\t\t\t\t server_options->items[i].string);\n \t}\n \n \tif (server_feature_v2(\"object-format\", &hash_name)) {\n@@ -1243,13 +1237,26 @@ static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,\n \t\tif (hash_algo_by_ptr(the_hash_algo) != hash_algo)\n \t\t\tdie(_(\"mismatched algorithms: client %s; server %s\"),\n \t\t\t the_hash_algo->name, hash_name);\n-\t\tpacket_buf_write(&req_buf, \"object-format=%s\", the_hash_algo->name);\n+\t\tpacket_buf_write(req_buf, \"object-format=%s\", the_hash_algo->name);\n \t} else if (hash_algo_by_ptr(the_hash_algo) != GIT_HASH_SHA1) {\n \t\tdie(_(\"the server does not support algorithm '%s'\"),\n \t\t the_hash_algo->name);\n \t}\n+\tpacket_buf_delim(req_buf);\n+}\n+\n+static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,\n+\t\t\t struct fetch_pack_args *args,\n+\t\t\t const struct ref *wants, struct oidset *common,\n+\t\t\t int *haves_to_send, int *in_vain,\n+\t\t\t int sideband_all, int seen_ack)\n+{\n+\tint haves_added;\n+\tint done_sent = 0;\n+\tstruct strbuf req_buf = STRBUF_INIT;\n+\n+\twrite_fetch_command_and_capabilities(&req_buf, args->server_options);\n \n-\tpacket_buf_delim(&req_buf);\n \tif (args->use_thin_pack)\n \t\tpacket_buf_write(&req_buf, \"thin-pack\");\n \tif (args->no_progress)\n-- \n2.31.1.295.g9ea45b61b8-goog\n\n"},{"id":"421322","messageId":"4696c8e901808853d17af10d5a6d95cd4711c6d5.1617929278.git.jonathantanmy@google.com","threadId":"55459","inReplyTo":"cover.1617929278.git.jonathantanmy@google.com","subject":"[PATCH 5/6] fetch: teach independent negotiation (no packfile)","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-04-09T01:10:02Z","receivedAt":"2021-04-09T01:10:21Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Currently, the packfile negotiation step within a Git fetch cannot be\ndone independent of sending the packfile, even though there is at least\none application wherein this is useful. Therefore, make it possible for\nthis negotiation step to be done independently. A subsequent commit will\nuse this for one such application - push negotiation.\n\nThis feature is for protocol v2 only. (An implementation for protocol v0\nwould require a separate implementation in the fetch, transport, and\ntransport helper code.)\n\nIn the protocol, the main hindrance towards independent negotiation is\nthat the server can unilaterally decide to send the packfile. This is\nsolved by a \"wait-for-done\" argument: the server will then wait for the\nclient to say \"done\". In practice, the client will never say it; instead\nit will cease requests once it is satisfied.\n\nIn the client, the main change lies in the transport and transport\nhelper code. fetch_refs_via_pack() performs everything needed - protocol\nversion and capability checks, and the negotiation itself.\n\nThere are 2 code paths that do not go through fetch_refs_via_pack() that\nneeded to be individually excluded: the bundle transport (excluded\nthrough requiring smart_options, which the bundle transport doesn't\nsupport) and transport helpers that do not support takeover.\nFortunately, none of these support protocol v2.\n\nSigned-off-by: Jonathan Tan \n---\n Documentation/technical/protocol-v2.txt | 8 +++\n builtin/fetch.c | 27 +++++++-\n fetch-pack.c | 89 +++++++++++++++++++++++--\n fetch-pack.h | 11 +++\n object.h | 2 +-\n t/t5701-git-serve.sh | 2 +-\n t/t5702-protocol-v2.sh | 89 +++++++++++++++++++++++++\n transport-helper.c | 10 +++\n transport.c | 30 +++++++--\n transport.h | 6 ++\n upload-pack.c | 18 +++--\n 11 files changed, 275 insertions(+), 17 deletions(-)\n\ndiff --git a/Documentation/technical/protocol-v2.txt b/Documentation/technical/protocol-v2.txt\nindex a7c806a73e..0b371d8de4 100644\n--- a/Documentation/technical/protocol-v2.txt\n+++ b/Documentation/technical/protocol-v2.txt\n@@ -346,6 +346,14 @@ explained below.\n \tclient should download from all given URIs. Currently, the\n \tprotocols supported are \"http\" and \"https\".\n \n+If the 'wait-for-done' feature is advertised, the following argument\n+can be included in the client's request.\n+\n+ wait-for-done\n+\tIndicates to the server that it should never send \"ready\", but\n+\tshould wait for the client to say \"done\" before sending the\n+\tpackfile.\n+\n The response of `fetch` is broken into a number of sections separated by\n delimiter packets (0001), with each section beginning with its section\n header. Most sections are sent only when the packfile is sent.\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 0b90de87c7..597973848a 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -82,6 +82,7 @@ static struct string_list server_options = STRING_LIST_INIT_DUP;\n static struct string_list negotiation_tip = STRING_LIST_INIT_NODUP;\n static int fetch_write_commit_graph = -1;\n static int stdin_refspecs = 0;\n+static int negotiate_only;\n \n static int git_fetch_config(const char *k, const char *v, void *cb)\n {\n@@ -202,6 +203,8 @@ static struct option builtin_fetch_options[] = {\n \t\t\tTRANSPORT_FAMILY_IPV6),\n \tOPT_STRING_LIST(0, \"negotiation-tip\", &negotiation_tip, N_(\"revision\"),\n \t\t\tN_(\"report that we have only objects reachable from this object\")),\n+\tOPT_BOOL(0, \"negotiate-only\", &negotiate_only,\n+\t\t N_(\"do not fetch a packfile; instead, print ancestors of negotiation tips\")),\n \tOPT_PARSE_LIST_OBJECTS_FILTER(&filter_options),\n \tOPT_BOOL(0, \"auto-maintenance\", &enable_auto_gc,\n \t\t N_(\"run 'maintenance --auto' after fetching\")),\n@@ -1986,7 +1989,29 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\t}\n \t}\n \n-\tif (remote) {\n+\tif (negotiate_only) {\n+\t\tstruct oidset acked_commits = OIDSET_INIT;\n+\t\tstruct oidset_iter iter;\n+\t\tconst struct object_id *oid;\n+\n+\t\tif (!remote)\n+\t\t\tdie(_(\"Must supply remote when using --negotiate-only\"));\n+\t\tgtransport = prepare_transport(remote, 1);\n+\t\tif (gtransport->smart_options) {\n+\t\t\tgtransport->smart_options->acked_commits = &acked_commits;\n+\t\t} else {\n+\t\t\twarning(_(\"Protocol does not support --negotiate-only, exiting.\"));\n+\t\t\treturn 1;\n+\t\t}\n+\t\tif (server_options.nr)\n+\t\t\tgtransport->server_options = &server_options;\n+\t\tresult = transport_fetch_refs(gtransport, NULL);\n+\n+\t\toidset_iter_init(&acked_commits, &iter);\n+\t\twhile ((oid = oidset_iter_next(&iter)))\n+\t\t\tprintf(\"%s\\n\", oid_to_hex(oid));\n+\t\toidset_clear(&acked_commits);\n+\t} else if (remote) {\n \t\tif (filter_options.choice || has_promisor_remote())\n \t\t\tfetch_one_setup_partial(remote);\n \t\tresult = fetch_one(remote, argc, argv, prune_tags_ok, stdin_refspecs);\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 512fe5450d..63054e2fc8 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -23,6 +23,7 @@\n #include \"fetch-negotiator.h\"\n #include \"fsck.h\"\n #include \"shallow.h\"\n+#include \"commit-reach.h\"\n \n static int transfer_unpack_limit = -1;\n static int fetch_unpack_limit = -1;\n@@ -45,6 +46,8 @@ static struct string_list uri_protocols = STRING_LIST_INIT_DUP;\n /* Remember to update object flag allocation in object.h */\n #define COMPLETE\t(1U << 0)\n #define ALTERNATE\t(1U << 1)\n+#define COMMON\t\t(1U << 6)\n+#define REACH_SCRATCH\t(1U << 7)\n \n /*\n * After sending this many \"have\"s if we do not get any new ACK , we\n@@ -1523,10 +1526,10 @@ enum fetch_state {\n \tFETCH_DONE,\n };\n \n-static void do_check_stateless_delimiter(const struct fetch_pack_args *args,\n+static void do_check_stateless_delimiter(int stateless_rpc,\n \t\t\t\t\t struct packet_reader *reader)\n {\n-\tcheck_stateless_delimiter(args->stateless_rpc, reader,\n+\tcheck_stateless_delimiter(stateless_rpc, reader,\n \t\t\t\t _(\"git fetch-pack: expected response end packet\"));\n }\n \n@@ -1622,7 +1625,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \t\t\t\t */\n \t\t\t\tstate = FETCH_GET_PACK;\n \t\t\t} else {\n-\t\t\t\tdo_check_stateless_delimiter(args, &reader);\n+\t\t\t\tdo_check_stateless_delimiter(args->stateless_rpc, &reader);\n \t\t\t\tstate = FETCH_SEND_REQUEST;\n \t\t\t}\n \t\t\tbreak;\n@@ -1645,7 +1648,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \t\t\t\t packfile_uris.nr ? &index_pack_args : NULL,\n \t\t\t\t sought, nr_sought, &fsck_options.gitmodules_found))\n \t\t\t\tdie(_(\"git fetch-pack: fetch failed.\"));\n-\t\t\tdo_check_stateless_delimiter(args, &reader);\n+\t\t\tdo_check_stateless_delimiter(args->stateless_rpc, &reader);\n \n \t\t\tstate = FETCH_DONE;\n \t\t\tbreak;\n@@ -1962,6 +1965,84 @@ struct ref *fetch_pack(struct fetch_pack_args *args,\n \treturn ref_cpy;\n }\n \n+static int add_to_object_array(const struct object_id *oid, void *data)\n+{\n+\tstruct object_array *a = data;\n+\n+\tadd_object_array(parse_object(the_repository, oid), \"\", a);\n+\treturn 0;\n+}\n+\n+void negotiate_using_fetch(const struct oid_array *negotiation_tips,\n+\t\t\t const struct string_list *server_options,\n+\t\t\t int stateless_rpc,\n+\t\t\t int fd[],\n+\t\t\t struct oidset *acked_commits)\n+{\n+\tstruct fetch_negotiator negotiator;\n+\tstruct packet_reader reader;\n+\tstruct object_array nt_object_array = OBJECT_ARRAY_INIT;\n+\tstruct strbuf req_buf = STRBUF_INIT;\n+\tint haves_to_send = INITIAL_FLUSH;\n+\tint in_vain = 0;\n+\tint seen_ack = 0;\n+\tint last_iteration = 0;\n+\n+\tfetch_negotiator_init(the_repository, &negotiator);\n+\tmark_tips(&negotiator, negotiation_tips);\n+\n+\tpacket_reader_init(&reader, fd[0], NULL, 0,\n+\t\t\t PACKET_READ_CHOMP_NEWLINE |\n+\t\t\t PACKET_READ_DIE_ON_ERR_PACKET);\n+\n+\toid_array_for_each((struct oid_array *) negotiation_tips,\n+\t\t\t add_to_object_array,\n+\t\t\t &nt_object_array);\n+\n+\twhile (!last_iteration) {\n+\t\tint haves_added;\n+\t\tstruct object_id common_oid;\n+\t\tint received_ready = 0;\n+\n+\t\tstrbuf_reset(&req_buf);\n+\t\twrite_fetch_command_and_capabilities(&req_buf, server_options);\n+\n+\t\tpacket_buf_write(&req_buf, \"wait-for-done\");\n+\n+\t\thaves_added = add_haves(&negotiator, &req_buf, &haves_to_send);\n+\t\tin_vain += haves_added;\n+\t\tif (!haves_added || (seen_ack && in_vain >= MAX_IN_VAIN))\n+\t\t\tlast_iteration = 1;\n+\n+\t\t/* Send request */\n+\t\tpacket_buf_flush(&req_buf);\n+\t\tif (write_in_full(fd[1], req_buf.buf, req_buf.len) < 0)\n+\t\t\tdie_errno(_(\"unable to write request to remote\"));\n+\n+\t\t/* Process ACKs/NAKs */\n+\t\tprocess_section_header(&reader, \"acknowledgments\", 0);\n+\t\twhile (process_ack(&negotiator, &reader, &common_oid,\n+\t\t\t\t &received_ready)) {\n+\t\t\tstruct commit *commit = lookup_commit(the_repository,\n+\t\t\t\t\t\t\t &common_oid);\n+\t\t\tif (commit)\n+\t\t\t\tcommit->object.flags |= COMMON;\n+\t\t\tin_vain = 0;\n+\t\t\tseen_ack = 1;\n+\t\t\toidset_insert(acked_commits, &common_oid);\n+\t\t}\n+\t\tif (received_ready)\n+\t\t\tdie(_(\"unexpected 'ready' from remote\"));\n+\t\telse\n+\t\t\tdo_check_stateless_delimiter(stateless_rpc, &reader);\n+\t\tif (can_all_from_reach_with_flag(&nt_object_array, COMMON,\n+\t\t\t\t\t\t REACH_SCRATCH, 0,\n+\t\t\t\t\t\t GENERATION_NUMBER_ZERO))\n+\t\t\tlast_iteration = 1;\n+\t}\n+\tstrbuf_release(&req_buf);\n+}\n+\n int report_unmatched_refs(struct ref **sought, int nr_sought)\n {\n \tint i, ret = 0;\ndiff --git a/fetch-pack.h b/fetch-pack.h\nindex f114d72775..7c8f49aca7 100644\n--- a/fetch-pack.h\n+++ b/fetch-pack.h\n@@ -5,6 +5,7 @@\n #include \"run-command.h\"\n #include \"protocol.h\"\n #include \"list-objects-filter-options.h\"\n+#include \"oidset.h\"\n \n struct oid_array;\n \n@@ -81,6 +82,16 @@ struct ref *fetch_pack(struct fetch_pack_args *args,\n \t\t struct string_list *pack_lockfiles,\n \t\t enum protocol_version version);\n \n+/*\n+ * Execute the --negotiate-only mode of \"git fetch\", adding all known common\n+ * commits to acked_commits.\n+ */\n+void negotiate_using_fetch(const struct oid_array *negotiation_tips,\n+\t\t\t const struct string_list *server_options,\n+\t\t\t int stateless_rpc,\n+\t\t\t int fd[],\n+\t\t\t struct oidset *acked_commits);\n+\n /*\n * Print an appropriate error message for each sought ref that wasn't\n * matched. Return 0 if all sought refs were matched, otherwise 1.\ndiff --git a/object.h b/object.h\nindex 59daadce21..4806fa8e66 100644\n--- a/object.h\n+++ b/object.h\n@@ -60,7 +60,7 @@ struct object_array {\n /*\n * object flag allocation:\n * revision.h: 0---------10 15 23------26\n- * fetch-pack.c: 01\n+ * fetch-pack.c: 01 67\n * negotiator/default.c: 2--5\n * walker.c: 0-2\n * upload-pack.c: 4 11-----14 16-----19\ndiff --git a/t/t5701-git-serve.sh b/t/t5701-git-serve.sh\nindex 509f379d49..f03bb04803 100755\n--- a/t/t5701-git-serve.sh\n+++ b/t/t5701-git-serve.sh\n@@ -16,7 +16,7 @@ test_expect_success 'test capability advertisement' '\n \tversion 2\n \tagent=git/$(git version | cut -d\" \" -f3)\n \tls-refs=unborn\n-\tfetch=shallow\n+\tfetch=shallow wait-for-done\n \tserver-option\n \tobject-format=$(test_oid algo)\n \t0000\ndiff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh\nindex 2e1243ca40..5d91f067d0 100755\n--- a/t/t5702-protocol-v2.sh\n+++ b/t/t5702-protocol-v2.sh\n@@ -585,6 +585,49 @@ test_expect_success 'deepen-relative' '\n \ttest_cmp expected actual\n '\n \n+setup_negotiate_only () {\n+\tSERVER=\"$1\"\n+\tURI=\"$2\"\n+\n+\trm -rf \"$SERVER\" client\n+\n+\tgit init \"$SERVER\"\n+\ttest_commit -C \"$SERVER\" one\n+\ttest_commit -C \"$SERVER\" two\n+\n+\tgit clone \"$URI\" client\n+\ttest_commit -C client three\n+}\n+\n+test_expect_success 'file:// --negotiate-only' '\n+\tSERVER=\"server\" &&\n+\tURI=\"file://$(pwd)/server\" &&\n+\n+\tsetup_negotiate_only \"$SERVER\" \"$URI\" &&\n+\n+\tgit -C client fetch \\\n+\t\t--no-tags \\\n+\t\t--negotiate-only \\\n+\t\t--negotiation-tip=$(git -C client rev-parse HEAD) \\\n+\t\torigin >out &&\n+\tCOMMON=$(git -C \"$SERVER\" rev-parse two) &&\n+\tgrep \"$COMMON\" out\n+'\n+\n+test_expect_success 'file:// --negotiate-only with protocol v0' '\n+\tSERVER=\"server\" &&\n+\tURI=\"file://$(pwd)/server\" &&\n+\n+\tsetup_negotiate_only \"$SERVER\" \"$URI\" &&\n+\n+\ttest_must_fail git -c protocol.version=0 -C client fetch \\\n+\t\t--no-tags \\\n+\t\t--negotiate-only \\\n+\t\t--negotiation-tip=$(git -C client rev-parse HEAD) \\\n+\t\torigin 2>err &&\n+\ttest_i18ngrep \"negotiate-only requires protocol v2\" err\n+'\n+\n # Test protocol v2 with 'http://' transport\n #\n . \"$TEST_DIRECTORY\"/lib-httpd.sh\n@@ -1035,6 +1078,52 @@ test_expect_success 'packfile-uri with transfer.fsckobjects fails when .gitmodul\n \ttest_i18ngrep \"disallowed submodule name\" err\n '\n \n+test_expect_success 'http:// --negotiate-only' '\n+\tSERVER=\"$HTTPD_DOCUMENT_ROOT_PATH/server\" &&\n+\tURI=\"$HTTPD_URL/smart/server\" &&\n+\n+\tsetup_negotiate_only \"$SERVER\" \"$URI\" &&\n+\n+\tgit -C client fetch \\\n+\t\t--no-tags \\\n+\t\t--negotiate-only \\\n+\t\t--negotiation-tip=$(git -C client rev-parse HEAD) \\\n+\t\torigin >out &&\n+\tCOMMON=$(git -C \"$SERVER\" rev-parse two) &&\n+\tgrep \"$COMMON\" out\n+'\n+\n+test_expect_success 'http:// --negotiate-only without wait-for-done support' '\n+\tSERVER=\"server\" &&\n+\tURI=\"$HTTPD_URL/one_time_perl/server\" &&\n+\n+\tsetup_negotiate_only \"$SERVER\" \"$URI\" &&\n+\n+\techo \"s/ wait-for-done/ xxxx-xxx-xxxx/\" \\\n+\t\t>\"$HTTPD_ROOT_PATH/one-time-perl\" &&\n+\n+\ttest_must_fail git -C client fetch \\\n+\t\t--no-tags \\\n+\t\t--negotiate-only \\\n+\t\t--negotiation-tip=$(git -C client rev-parse HEAD) \\\n+\t\torigin 2>err &&\n+\ttest_i18ngrep \"server does not support wait-for-done\" err\n+'\n+\n+test_expect_success 'http:// --negotiate-only with protocol v0' '\n+\tSERVER=\"$HTTPD_DOCUMENT_ROOT_PATH/server\" &&\n+\tURI=\"$HTTPD_URL/smart/server\" &&\n+\n+\tsetup_negotiate_only \"$SERVER\" \"$URI\" &&\n+\n+\ttest_must_fail git -c protocol.version=0 -C client fetch \\\n+\t\t--no-tags \\\n+\t\t--negotiate-only \\\n+\t\t--negotiation-tip=$(git -C client rev-parse HEAD) \\\n+\t\torigin 2>err &&\n+\ttest_i18ngrep \"negotiate-only requires protocol v2\" err\n+'\n+\n # DO NOT add non-httpd-specific tests here, because the last part of this\n # test script is only executed when httpd is available and enabled.\n \ndiff --git a/transport-helper.c b/transport-helper.c\nindex 4cd76366fa..4be035edb8 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -684,6 +684,16 @@ static int fetch(struct transport *transport,\n \t\treturn transport->vtable->fetch(transport, nr_heads, to_fetch);\n \t}\n \n+\t/*\n+\t * If we reach here, then the server, the client, and/or the transport\n+\t * helper does not support protocol v2. --negotiate-only requires\n+\t * protocol v2.\n+\t */\n+\tif (data->transport_options.acked_commits) {\n+\t\twarning(_(\"--negotiate-only requires protocol v2\"));\n+\t\treturn -1;\n+\t}\n+\n \tif (!data->get_refs_list_called)\n \t\tget_refs_list_using_list(transport, 0);\n \ndiff --git a/transport.c b/transport.c\nindex ef66e73090..80caeaa72f 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -392,16 +392,29 @@ static int fetch_refs_via_pack(struct transport *transport,\n \telse if (data->version <= protocol_v1)\n \t\tdie_if_server_options(transport);\n \n+\tif (data->options.acked_commits) {\n+\t\tif (data->version < protocol_v2) {\n+\t\t\twarning(_(\"--negotiate-only requires protocol v2\"));\n+\t\t\tret = -1;\n+\t\t} else if (!server_supports_feature(\"fetch\", \"wait-for-done\", 0)) {\n+\t\t\twarning(_(\"server does not support wait-for-done\"));\n+\t\t\tret = -1;\n+\t\t} else {\n+\t\t\tnegotiate_using_fetch(data->options.negotiation_tips,\n+\t\t\t\t\t transport->server_options,\n+\t\t\t\t\t transport->stateless_rpc,\n+\t\t\t\t\t data->fd,\n+\t\t\t\t\t data->options.acked_commits);\n+\t\t\tret = 0;\n+\t\t}\n+\t\tgoto cleanup;\n+\t}\n+\n \trefs = fetch_pack(&args, data->fd,\n \t\t\t refs_tmp ? refs_tmp : transport->remote_refs,\n \t\t\t to_fetch, nr_heads, &data->shallow,\n \t\t\t &transport->pack_lockfiles, data->version);\n \n-\tclose(data->fd[0]);\n-\tclose(data->fd[1]);\n-\tif (finish_connect(data->conn))\n-\t\tret = -1;\n-\tdata->conn = NULL;\n \tdata->got_remote_heads = 0;\n \tdata->options.self_contained_and_connected =\n \t\targs.self_contained_and_connected;\n@@ -412,6 +425,13 @@ static int fetch_refs_via_pack(struct transport *transport,\n \tif (report_unmatched_refs(to_fetch, nr_heads))\n \t\tret = -1;\n \n+cleanup:\n+\tclose(data->fd[0]);\n+\tclose(data->fd[1]);\n+\tif (finish_connect(data->conn))\n+\t\tret = -1;\n+\tdata->conn = NULL;\n+\n \tfree_refs(refs_tmp);\n \tfree_refs(refs);\n \treturn ret;\ndiff --git a/transport.h b/transport.h\nindex 4d5db0a7f2..b8b3d42e62 100644\n--- a/transport.h\n+++ b/transport.h\n@@ -47,6 +47,12 @@ struct git_transport_options {\n \t * transport_set_option().\n \t */\n \tstruct oid_array *negotiation_tips;\n+\n+\t/*\n+\t * If set, whenever transport_fetch_refs() is called, add known common\n+\t * commits to this oidset instead of fetching any packfiles.\n+\t */\n+\tstruct oidset *acked_commits;\n };\n \n enum transport_family {\ndiff --git a/upload-pack.c b/upload-pack.c\nindex e19583ae0f..b432ef0119 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -103,6 +103,7 @@ struct upload_pack_data {\n \tunsigned use_ofs_delta : 1;\n \tunsigned no_progress : 1;\n \tunsigned use_include_tag : 1;\n+\tunsigned wait_for_done : 1;\n \tunsigned allow_filter : 1;\n \tunsigned allow_filter_fallback : 1;\n \tunsigned long tree_filter_max_depth;\n@@ -1496,6 +1497,10 @@ static void process_args(struct packet_reader *request,\n \t\t\tdata->done = 1;\n \t\t\tcontinue;\n \t\t}\n+\t\tif (!strcmp(arg, \"wait-for-done\")) {\n+\t\t\tdata->wait_for_done = 1;\n+\t\t\tcontinue;\n+\t\t}\n \n \t\t/* Shallow related arguments */\n \t\tif (process_shallow(arg, &data->shallows))\n@@ -1578,7 +1583,7 @@ static int send_acks(struct upload_pack_data *data, struct oid_array *acks)\n \t\t\t\t oid_to_hex(&acks->oid[i]));\n \t}\n \n-\tif (ok_to_give_up(data)) {\n+\tif (!data->wait_for_done && ok_to_give_up(data)) {\n \t\t/* Send Ready */\n \t\tpacket_writer_write(&data->writer, \"ready\\n\");\n \t\treturn 1;\n@@ -1668,10 +1673,13 @@ int upload_pack_v2(struct repository *r, struct strvec *keys,\n \t\tcase FETCH_PROCESS_ARGS:\n \t\t\tprocess_args(request, &data);\n \n-\t\t\tif (!data.want_obj.nr) {\n+\t\t\tif (!data.want_obj.nr && !data.wait_for_done) {\n \t\t\t\t/*\n-\t\t\t\t * Request didn't contain any 'want' lines,\n-\t\t\t\t * guess they didn't want anything.\n+\t\t\t\t * Request didn't contain any 'want' lines (and\n+\t\t\t\t * the request does not contain\n+\t\t\t\t * \"wait-for-done\", in which it is reasonable\n+\t\t\t\t * to just send 'have's without 'want's); guess\n+\t\t\t\t * they didn't want anything.\n \t\t\t\t */\n \t\t\t\tstate = FETCH_DONE;\n \t\t\t} else if (data.haves.nr) {\n@@ -1723,7 +1731,7 @@ int upload_pack_advertise(struct repository *r,\n \t\tint allow_sideband_all_value;\n \t\tchar *str = NULL;\n \n-\t\tstrbuf_addstr(value, \"shallow\");\n+\t\tstrbuf_addstr(value, \"shallow wait-for-done\");\n \n \t\tif (!repo_config_get_bool(the_repository,\n \t\t\t\t\t \"uploadpack.allowfilter\",\n-- \n2.31.1.295.g9ea45b61b8-goog\n\n"},{"id":"421323","messageId":"a2daa1022c41820b2109d9572069d12684470cb8.1617929278.git.jonathantanmy@google.com","threadId":"55459","inReplyTo":"cover.1617929278.git.jonathantanmy@google.com","subject":"[PATCH 6/6] send-pack: support push negotiation","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-04-09T01:10:03Z","receivedAt":"2021-04-09T01:10:22Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Teach Git the push.negotiate config variable.\n\nSigned-off-by: Jonathan Tan \n---\n Documentation/config/push.txt | 7 ++++\n send-pack.c | 61 ++++++++++++++++++++++++++++++++---\n t/t5516-fetch-push.sh | 35 ++++++++++++++++++++\n 3 files changed, 99 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/config/push.txt b/Documentation/config/push.txt\nindex 21b256e0a4..f2667b2689 100644\n--- a/Documentation/config/push.txt\n+++ b/Documentation/config/push.txt\n@@ -120,3 +120,10 @@ push.useForceIfIncludes::\n \t`--force-if-includes` as an option to linkgit:git-push[1]\n \tin the command line. Adding `--no-force-if-includes` at the\n \ttime of push overrides this configuration setting.\n+\n+push.negotiate::\n+\tIf set to \"true\", attempt to reduce the size of the packfile\n+\tsent by rounds of negotiation in which the client and the\n+\tserver attempt to find commits in common. If \"false\", Git will\n+\trely solely on the server's ref advertisement to find commits\n+\tin common.\ndiff --git a/send-pack.c b/send-pack.c\nindex 5f215b13c7..9cb9f71650 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -56,7 +56,9 @@ static void feed_object(const struct object_id *oid, FILE *fh, int negative)\n /*\n * Make a pack stream and spit it out into file descriptor fd\n */\n-static int pack_objects(int fd, struct ref *refs, struct oid_array *extra, struct send_pack_args *args)\n+static int pack_objects(int fd, struct ref *refs, struct oid_array *advertised,\n+\t\t\tstruct oid_array *negotiated,\n+\t\t\tstruct send_pack_args *args)\n {\n \t/*\n \t * The child becomes pack-objects --revs; we feed\n@@ -94,8 +96,10 @@ static int pack_objects(int fd, struct ref *refs, struct oid_array *extra, struc\n \t * parameters by writing to the pipe.\n \t */\n \tpo_in = xfdopen(po.in, \"w\");\n-\tfor (i = 0; i < extra->nr; i++)\n-\t\tfeed_object(&extra->oid[i], po_in, 1);\n+\tfor (i = 0; i < advertised->nr; i++)\n+\t\tfeed_object(&advertised->oid[i], po_in, 1);\n+\tfor (i = 0; i < negotiated->nr; i++)\n+\t\tfeed_object(&negotiated->oid[i], po_in, 1);\n \n \twhile (refs) {\n \t\tif (!is_null_oid(&refs->old_oid))\n@@ -409,11 +413,55 @@ static void reject_invalid_nonce(const char *nonce, int len)\n \t}\n }\n \n+static void get_commons_through_negotiation(const char *url,\n+\t\t\t\t\t const struct ref *remote_refs,\n+\t\t\t\t\t struct oid_array *commons)\n+{\n+\tstruct child_process child = CHILD_PROCESS_INIT;\n+\tconst struct ref *ref;\n+\tint len = the_hash_algo->hexsz + 1; /* hash + NL */\n+\n+\tchild.git_cmd = 1;\n+\tchild.no_stdin = 1;\n+\tchild.out = -1;\n+\tstrvec_pushl(&child.args, \"fetch\", \"--negotiate-only\", NULL);\n+\tfor (ref = remote_refs; ref; ref = ref->next)\n+\t\tstrvec_pushf(&child.args, \"--negotiation-tip=%s\", oid_to_hex(&ref->new_oid));\n+\tstrvec_push(&child.args, url);\n+\n+\tif (start_command(&child))\n+\t\tdie(_(\"send-pack: unable to fork off fetch subprocess\"));\n+\n+\tdo {\n+\t\tchar hex_hash[GIT_MAX_HEXSZ + 1];\n+\t\tint read_len = read_in_full(child.out, hex_hash, len);\n+\t\tstruct object_id oid;\n+\t\tconst char *end;\n+\n+\t\tif (!read_len)\n+\t\t\tbreak;\n+\t\tif (read_len != len)\n+\t\t\tdie(\"invalid length read %d\", read_len);\n+\t\tif (parse_oid_hex(hex_hash, &oid, &end) || *end != '\\n')\n+\t\t\tdie(\"invalid hash\");\n+\t\toid_array_append(commons, &oid);\n+\t} while (1);\n+\n+\tif (finish_command(&child)) {\n+\t\t/*\n+\t\t * The information that push negotiation provides is useful but\n+\t\t * not mandatory.\n+\t\t */\n+\t\twarning(_(\"push negotiation failed; proceeding anyway with push\"));\n+\t}\n+}\n+\n int send_pack(struct send_pack_args *args,\n \t int fd[], struct child_process *conn,\n \t struct ref *remote_refs,\n \t struct oid_array *extra_have)\n {\n+\tstruct oid_array commons = OID_ARRAY_INIT;\n \tint in = fd[0];\n \tint out = fd[1];\n \tstruct strbuf req_buf = STRBUF_INIT;\n@@ -426,6 +474,7 @@ int send_pack(struct send_pack_args *args,\n \tint quiet_supported = 0;\n \tint agent_supported = 0;\n \tint advertise_sid = 0;\n+\tint push_negotiate = 0;\n \tint use_atomic = 0;\n \tint atomic_supported = 0;\n \tint use_push_options = 0;\n@@ -437,6 +486,10 @@ int send_pack(struct send_pack_args *args,\n \tconst char *push_cert_nonce = NULL;\n \tstruct packet_reader reader;\n \n+\tgit_config_get_bool(\"push.negotiate\", &push_negotiate);\n+\tif (push_negotiate)\n+\t\tget_commons_through_negotiation(args->url, remote_refs, &commons);\n+\n \tgit_config_get_bool(\"transfer.advertisesid\", &advertise_sid);\n \n \t/* Does the other end support the reporting? */\n@@ -625,7 +678,7 @@ int send_pack(struct send_pack_args *args,\n \t\t\t PACKET_READ_DIE_ON_ERR_PACKET);\n \n \tif (need_pack_data && cmds_sent) {\n-\t\tif (pack_objects(out, remote_refs, extra_have, args) < 0) {\n+\t\tif (pack_objects(out, remote_refs, extra_have, &commons, args) < 0) {\n \t\t\tif (args->stateless_rpc)\n \t\t\t\tclose(out);\n \t\t\tif (git_connection_is_socket(conn))\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex f11742ed59..62fb9074a2 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -191,6 +191,41 @@ test_expect_success 'fetch with pushInsteadOf (should not rewrite)' '\n \t)\n '\n \n+grep_wrote () {\n+\tobject_count=$1\n+\tfile_name=$2\n+\tgrep 'write_pack_file/wrote.*\"value\":\"'$1'\"' $2\n+}\n+\n+test_expect_success 'push with negotiation' '\n+\t# Without negotiation\n+\tmk_empty testrepo &&\n+\tgit push testrepo $the_first_commit:refs/remotes/origin/first_commit &&\n+\tgit -C testrepo config receive.hideRefs refs/remotes/origin/first_commit &&\n+\techo now pushing without negotiation &&\n+\tGIT_TRACE2_EVENT=\"$(pwd)/event\" git push testrepo refs/heads/main:refs/remotes/origin/main &&\n+\tgrep_wrote 5 event && # 2 commits, 2 trees, 1 blob\n+\n+\t# Same commands, but with negotiation\n+\trm event &&\n+\tmk_empty testrepo &&\n+\tgit push testrepo $the_first_commit:refs/remotes/origin/first_commit &&\n+\tgit -C testrepo config receive.hideRefs refs/remotes/origin/first_commit &&\n+\tGIT_TRACE2_EVENT=\"$(pwd)/event\" git -c push.negotiate=1 push testrepo refs/heads/main:refs/remotes/origin/main &&\n+\tgrep_wrote 2 event # 1 commit, 1 tree\n+'\n+\n+test_expect_success 'push with negotiation proceeds anyway even if negotiation fails' '\n+\trm event &&\n+\tmk_empty testrepo &&\n+\tgit push testrepo $the_first_commit:refs/remotes/origin/first_commit &&\n+\tgit -C testrepo config receive.hideRefs refs/remotes/origin/first_commit &&\n+\tGIT_TRACE_PACKET=\"$(pwd)/trace\" GIT_TEST_PROTOCOL_VERSION=0 GIT_TRACE2_EVENT=\"$(pwd)/event\" \\\n+\t\tgit -c push.negotiate=1 push testrepo refs/heads/main:refs/remotes/origin/main 2>err &&\n+\tgrep_wrote 5 event && # 2 commits, 2 trees, 1 blob\n+\ttest_i18ngrep \"push negotiation failed\" err\n+'\n+\n test_expect_success 'push without wildcard' '\n \tmk_empty testrepo &&\n \n-- \n2.31.1.295.g9ea45b61b8-goog\n\n"},{"id":"421339","messageId":"xmqqy2dsnjuf.fsf@gitster.g","threadId":"55459","inReplyTo":"86e4ce0e8d9665b1c5a2b30173be4afffe0a0abd.1617929278.git.jonathantanmy@google.com","subject":"Re: [PATCH 1/6] fetch-pack: buffer object-format with other args","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-04-09T04:49:12Z","receivedAt":"2021-04-09T04:49:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> In send_fetch_request(), \"object-format\" is written directly to the file\n> descriptor, as opposed to the other arguments, which are buffered.\n> Buffer \"object-format\" as well. \"object-format\" must be buffered; in\n> particular, it must appear after \"command=fetch\" in the request.\n>\n> This divergence was introduced in 4b831208bb (\"fetch-pack: parse and\n> advertise the object-format capability\", 2020-05-27), perhaps as an\n> oversight (the surrounding code at the point of this commit has already\n> been using a request buffer.)\n\nGood find. Did this actually resulted in a data corruption and that\nwas how you discovered it?\n\n\n\n>\n> Signed-off-by: Jonathan Tan \n> ---\n> fetch-pack.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/fetch-pack.c b/fetch-pack.c\n> index 6e68276aa3..2318ebe680 100644\n> --- a/fetch-pack.c\n> +++ b/fetch-pack.c\n> @@ -1251,7 +1251,7 @@ static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,\n> \t\tif (hash_algo_by_ptr(the_hash_algo) != hash_algo)\n> \t\t\tdie(_(\"mismatched algorithms: client %s; server %s\"),\n> \t\t\t the_hash_algo->name, hash_name);\n> -\t\tpacket_write_fmt(fd_out, \"object-format=%s\", the_hash_algo->name);\n> +\t\tpacket_buf_write(&req_buf, \"object-format=%s\", the_hash_algo->name);\n> \t} else if (hash_algo_by_ptr(the_hash_algo) != GIT_HASH_SHA1) {\n> \t\tdie(_(\"the server does not support algorithm '%s'\"),\n> \t\t the_hash_algo->name);\n"},{"id":"421340","messageId":"xmqqsg40niyo.fsf@gitster.g","threadId":"55459","inReplyTo":"6a9f78df1a3513df04ac3275cd1feccfb6cf87b1.1617929278.git.jonathantanmy@google.com","subject":"Re: [PATCH 2/6] fetch-pack: refactor process_acks()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-04-09T05:08:15Z","receivedAt":"2021-04-09T05:09:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> A subsequent commit will need part, but not all, of the functionality in\n> process_acks(), so move some of its functionality to its sole caller\n> do_fetch_pack_v2(). As a side effect, the resulting code is also\n> shorter.\n>\n> Signed-off-by: Jonathan Tan \n> ---\n> fetch-pack.c | 70 +++++++++++++++++-----------------------------------\n> 1 file changed, 22 insertions(+), 48 deletions(-)\n\nNicely done.\n\nI first found the patch hard to follow, but that wasn't due to this\npatch, but because I wasn't all that familiar with the original\nanymore (even though I do recall futzing with the codepath that\ninvolves the in_vain stuff myself at some point in the past).\n\nAnd indeed the resulting code is easy to see what is going on (I'd\nneed a lot more concentration than I can muster this late in the\nnight to be able to tell if the behaviour does not change with the\npatch, though).\n\n> diff --git a/fetch-pack.c b/fetch-pack.c\n> index 2318ebe680..9f3901cdba 100644\n> --- a/fetch-pack.c\n> +++ b/fetch-pack.c\n> @@ -1351,35 +1351,11 @@ static int process_section_header(struct packet_reader *reader,\n> \treturn ret;\n> }\n> \n> -enum common_found {\n> -\t/*\n> -\t * No commit was found to be possessed by both the client and the\n> -\t * server, and \"ready\" was not received.\n> -\t */\n> -\tNO_COMMON_FOUND,\n> -\n> -\t/*\n> -\t * At least one commit was found to be possessed by both the client and\n> -\t * the server, and \"ready\" was not received.\n> -\t */\n> -\tCOMMON_FOUND,\n> -\n> -\t/*\n> -\t * \"ready\" was received, indicating that the server is ready to send\n> -\t * the packfile without any further negotiation.\n> -\t */\n> -\tREADY\n> -};\n> -\n> -static enum common_found process_acks(struct fetch_negotiator *negotiator,\n> -\t\t\t\t struct packet_reader *reader,\n> -\t\t\t\t struct oidset *common)\n> +static int process_ack(struct fetch_negotiator *negotiator,\n> +\t\t struct packet_reader *reader,\n> +\t\t struct object_id *common_oid,\n> +\t\t int *received_ready)\n> {\n> -\t/* received */\n> -\tint received_ready = 0;\n> -\tint received_ack = 0;\n> -\n> -\tprocess_section_header(reader, \"acknowledgments\", 0);\n> \twhile (packet_reader_read(reader) == PACKET_READ_NORMAL) {\n> \t\tconst char *arg;\n> \n> @@ -1387,20 +1363,17 @@ static enum common_found process_acks(struct fetch_negotiator *negotiator,\n> \t\t\tcontinue;\n> \n> \t\tif (skip_prefix(reader->line, \"ACK \", &arg)) {\n> -\t\t\tstruct object_id oid;\n> -\t\t\treceived_ack = 1;\n> -\t\t\tif (!get_oid_hex(arg, &oid)) {\n> +\t\t\tif (!get_oid_hex(arg, common_oid)) {\n> \t\t\t\tstruct commit *commit;\n> -\t\t\t\toidset_insert(common, &oid);\n> -\t\t\t\tcommit = lookup_commit(the_repository, &oid);\n> +\t\t\t\tcommit = lookup_commit(the_repository, common_oid);\n> \t\t\t\tif (negotiator)\n> \t\t\t\t\tnegotiator->ack(negotiator, commit);\n> \t\t\t}\n> -\t\t\tcontinue;\n> +\t\t\treturn 1;\n> \t\t}\n> \n> \t\tif (!strcmp(reader->line, \"ready\")) {\n> -\t\t\treceived_ready = 1;\n> +\t\t\t*received_ready = 1;\n> \t\t\tcontinue;\n> \t\t}\n> \n> @@ -1418,13 +1391,12 @@ static enum common_found process_acks(struct fetch_negotiator *negotiator,\n> \t * sent. Therefore, a DELIM is expected if \"ready\" is sent, and a FLUSH\n> \t * otherwise.\n> \t */\n> -\tif (received_ready && reader->status != PACKET_READ_DELIM)\n> +\tif (*received_ready && reader->status != PACKET_READ_DELIM)\n> \t\tdie(_(\"expected packfile to be sent after 'ready'\"));\n> -\tif (!received_ready && reader->status != PACKET_READ_FLUSH)\n> +\tif (!*received_ready && reader->status != PACKET_READ_FLUSH)\n> \t\tdie(_(\"expected no other sections to be sent after no 'ready'\"));\n> \n> -\treturn received_ready ? READY :\n> -\t\t(received_ack ? COMMON_FOUND : NO_COMMON_FOUND);\n> +\treturn 0;\n> }\n> \n> static void receive_shallow_info(struct fetch_pack_args *args,\n> @@ -1573,6 +1545,8 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n> \tstruct fetch_negotiator negotiator_alloc;\n> \tstruct fetch_negotiator *negotiator;\n> \tint seen_ack = 0;\n> +\tstruct object_id common_oid;\n> +\tint received_ready = 0;\n> \tstruct string_list packfile_uris = STRING_LIST_INIT_DUP;\n> \tint i;\n> \tstruct strvec index_pack_args = STRVEC_INIT;\n> @@ -1631,22 +1605,22 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n> \t\t\tbreak;\n> \t\tcase FETCH_PROCESS_ACKS:\n> \t\t\t/* Process ACKs/NAKs */\n> -\t\t\tswitch (process_acks(negotiator, &reader, &common)) {\n> -\t\t\tcase READY:\n> +\t\t\tprocess_section_header(&reader, \"acknowledgments\", 0);\n> +\t\t\twhile (process_ack(negotiator, &reader, &common_oid,\n> +\t\t\t\t\t &received_ready)) {\n> +\t\t\t\tin_vain = 0;\n> +\t\t\t\tseen_ack = 1;\n> +\t\t\t\toidset_insert(&common, &common_oid);\n> +\t\t\t}\n> +\t\t\tif (received_ready) {\n> \t\t\t\t/*\n> \t\t\t\t * Don't check for response delimiter; get_pack() will\n> \t\t\t\t * read the rest of this response.\n> \t\t\t\t */\n> \t\t\t\tstate = FETCH_GET_PACK;\n> -\t\t\t\tbreak;\n> -\t\t\tcase COMMON_FOUND:\n> -\t\t\t\tin_vain = 0;\n> -\t\t\t\tseen_ack = 1;\n> -\t\t\t\t/* fallthrough */\n> -\t\t\tcase NO_COMMON_FOUND:\n> +\t\t\t} else {\n> \t\t\t\tdo_check_stateless_delimiter(args, &reader);\n> \t\t\t\tstate = FETCH_SEND_REQUEST;\n> -\t\t\t\tbreak;\n> \t\t\t}\n> \t\t\tbreak;\n> \t\tcase FETCH_GET_PACK:\n"},{"id":"421341","messageId":"xmqqlf9sniec.fsf@gitster.g","threadId":"55459","inReplyTo":"b07e52ec18f46fb85d49ba0e1e6e8ad22845af9e.1617929278.git.jonathantanmy@google.com","subject":"Re: [PATCH 3/6] fetch-pack: refactor add_haves()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-04-09T05:20:27Z","receivedAt":"2021-04-09T05:20:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> A subsequent commit will need part, but not all, of the functionality in\n> add_haves(), so move some of its functionality to its sole caller\n> send_fetch_request().\n>\n> Signed-off-by: Jonathan Tan \n> ---\n> fetch-pack.c | 28 ++++++++++++----------------\n> 1 file changed, 12 insertions(+), 16 deletions(-)\n>\n> diff --git a/fetch-pack.c b/fetch-pack.c\n> index 9f3901cdba..128ad47d2a 100644\n> --- a/fetch-pack.c\n> +++ b/fetch-pack.c\n> @@ -1195,11 +1195,9 @@ static void add_common(struct strbuf *req_buf, struct oidset *common)\n> }\n> \n> static int add_haves(struct fetch_negotiator *negotiator,\n> -\t\t int seen_ack,\n> \t\t struct strbuf *req_buf,\n> -\t\t int *haves_to_send, int *in_vain)\n> +\t\t int *haves_to_send)\n> {\n> -\tint ret = 0;\n> \tint haves_added = 0;\n> \tconst struct object_id *oid;\n> \n> @@ -1209,17 +1207,10 @@ static int add_haves(struct fetch_negotiator *negotiator,\n> \t\t\tbreak;\n> \t}\n> \n> -\t*in_vain += haves_added;\n> -\tif (!haves_added || (seen_ack && *in_vain >= MAX_IN_VAIN)) {\n> -\t\t/* Send Done */\n> -\t\tpacket_buf_write(req_buf, \"done\\n\");\n> -\t\tret = 1;\n> -\t}\n> -\n> \t/* Increase haves to send on next round */\n> \t*haves_to_send = next_flush(1, *haves_to_send);\n> \n> -\treturn ret;\n> +\treturn haves_added;\n> }\n\nOK, so the helper used to\n\n - send out have's\n - increment *in_vain with the number of have's sent out\n - decide when to send done and send it\n - increase haves-to-send for the next round.\n\nBut the new version skips the second and third step, and let the\ncaller be responsible for in_vain management. The only piece of\ninformation the caller needs to do so from this function is the\nnumber of have's we send out, so that is what we return from the\nfunction.\n\n> -\t/* Add initial haves */\n> -\tret = add_haves(negotiator, seen_ack, &req_buf,\n> -\t\t\thaves_to_send, in_vain);\n> +\thaves_added = add_haves(negotiator, &req_buf, haves_to_send);\n> +\t*in_vain += haves_added;\n> +\tif (!haves_added || (seen_ack && *in_vain >= MAX_IN_VAIN)) {\n> +\t\t/* Send Done */\n> +\t\tpacket_buf_write(&req_buf, \"done\\n\");\n> +\t\tdone_sent = 1;\n> +\t}\n\nAnd indeed, this caller does the *in_vain management add_haves()\nused to do and give \"done\" as needed.\n\n> @@ -1322,7 +1318,7 @@ static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,\n> \t\tdie_errno(_(\"unable to write request to remote\"));\n> \n> \tstrbuf_release(&req_buf);\n> -\treturn ret;\n> +\treturn done_sent;\n\nAnd of course, the value returned by this function is if we sent\n\"done\", which is far easier to see with the new variable name.\n\nAgain, very nicely done.\n\n"},{"id":"421342","messageId":"xmqqh7kgni2q.fsf@gitster.g","threadId":"55459","inReplyTo":"7988c106873332ac6e5c2f9dd143cfa1f50e067c.1617929278.git.jonathantanmy@google.com","subject":"Re: [PATCH 4/6] fetch-pack: refactor command and capability write","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-04-09T05:27:25Z","receivedAt":"2021-04-09T05:27:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> A subsequent commit will need this functionality independent of the rest\n> of send_fetch_request(), so put this into its own function.\n\nOK. send_fetch_request() used to do the \"command=fetch\", capability\nresponse, and concluded them with a delim packet, which is now done\nwith the new helper write_fetch_command_and_capabilities().\n\nMostly code movement, without any behaviour change.\n\nMakes sense.\n"},{"id":"421343","messageId":"xmqqczv4nhem.fsf@gitster.g","threadId":"55459","inReplyTo":"4696c8e901808853d17af10d5a6d95cd4711c6d5.1617929278.git.jonathantanmy@google.com","subject":"Re: [PATCH 5/6] fetch: teach independent negotiation (no packfile)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-04-09T05:41:53Z","receivedAt":"2021-04-09T05:42:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> There are 2 code paths that do not go through fetch_refs_via_pack() that\n> needed to be individually excluded: the bundle transport (excluded\n> through requiring smart_options, which the bundle transport doesn't\n> support) and transport helpers that do not support takeover.\n> Fortunately, none of these support protocol v2.\n\nI am a bit puzzled by this mention of \"Fortunately\". If one says\n\"this shiny new feature only works with protocol v2\" and \"transport\nX does not support protocol v2\", doesn't it imply that the shiny new\nfeature cannot be used with the transport X, which is unfortunate?\n\nI can understand \"while interacting with the bundle transport, you\ncannot do independent negotiation, but there is nothing to negotiate\nwith a static file that is a bundle anyway, so nothing is lost\" as\nan explanation, though.\n\n> Documentation/technical/protocol-v2.txt | 8 +++\n> builtin/fetch.c | 27 +++++++-\n> fetch-pack.c | 89 +++++++++++++++++++++++--\n> fetch-pack.h | 11 +++\n> object.h | 2 +-\n> t/t5701-git-serve.sh | 2 +-\n> t/t5702-protocol-v2.sh | 89 +++++++++++++++++++++++++\n> transport-helper.c | 10 +++\n> transport.c | 30 +++++++--\n> transport.h | 6 ++\n> upload-pack.c | 18 +++--\n> 11 files changed, 275 insertions(+), 17 deletions(-)\n\nIt is a bit surprising that there isn't much code removed, as I\nexpected that we'd be factoring out and reusing existing code used\nin negotiation for fetching into a new helper function (hence the\nexisting codepath would lose a lot of code to be replaced by a call\nto a new helper function), but that is apparently not what is going\non.\n\nI'll have to revisit this step and the next step tomorrow.\n\nThanks.\n"},{"id":"421417","messageId":"20210409162417.2558134-1-jonathantanmy@google.com","threadId":"55459","inReplyTo":"xmqqy2dsnjuf.fsf@gitster.g","subject":"Re: [PATCH 1/6] fetch-pack: buffer object-format with other args","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-04-09T16:24:17Z","receivedAt":"2021-04-09T16:24:27Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> Jonathan Tan writes:\n> \n> > In send_fetch_request(), \"object-format\" is written directly to the file\n> > descriptor, as opposed to the other arguments, which are buffered.\n> > Buffer \"object-format\" as well. \"object-format\" must be buffered; in\n> > particular, it must appear after \"command=fetch\" in the request.\n> >\n> > This divergence was introduced in 4b831208bb (\"fetch-pack: parse and\n> > advertise the object-format capability\", 2020-05-27), perhaps as an\n> > oversight (the surrounding code at the point of this commit has already\n> > been using a request buffer.)\n> \n> Good find. Did this actually resulted in a data corruption and that\n> was how you discovered it?\n\nNo, I did not see any data corruption. I discovered it because I was\nlooking at this function, thinking that I needed to refactor it.\n"},{"id":"421418","messageId":"20210409163835.2560428-1-jonathantanmy@google.com","threadId":"55459","inReplyTo":"xmqqczv4nhem.fsf@gitster.g","subject":"Re: [PATCH 5/6] fetch: teach independent negotiation (no packfile)","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-04-09T16:38:35Z","receivedAt":"2021-04-09T16:38:40Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> Jonathan Tan writes:\n> \n> > There are 2 code paths that do not go through fetch_refs_via_pack() that\n> > needed to be individually excluded: the bundle transport (excluded\n> > through requiring smart_options, which the bundle transport doesn't\n> > support) and transport helpers that do not support takeover.\n> > Fortunately, none of these support protocol v2.\n> \n> I am a bit puzzled by this mention of \"Fortunately\". If one says\n> \"this shiny new feature only works with protocol v2\" and \"transport\n> X does not support protocol v2\", doesn't it imply that the shiny new\n> feature cannot be used with the transport X, which is unfortunate?\n\nI meant to say that we don't have to do much about these cases because\nthey are out of scope for the support that we planned (protocol v2), but\nperhaps \"fortunately\" is the wrong way to say it. Perhaps a better way\nof explaining it is that most code paths go through\nfetch_refs_via_pack(), and for the ones that don't (bundle transport and\nnon-takeover transport helpers), we need to address them separately. But\nin this case, we are not planning to support either, so we just check if\nwe have requested independent negotiation and if yes, report failure.\n\n> I can understand \"while interacting with the bundle transport, you\n> cannot do independent negotiation, but there is nothing to negotiate\n> with a static file that is a bundle anyway, so nothing is lost\" as\n> an explanation, though.\n> \n> > Documentation/technical/protocol-v2.txt | 8 +++\n> > builtin/fetch.c | 27 +++++++-\n> > fetch-pack.c | 89 +++++++++++++++++++++++--\n> > fetch-pack.h | 11 +++\n> > object.h | 2 +-\n> > t/t5701-git-serve.sh | 2 +-\n> > t/t5702-protocol-v2.sh | 89 +++++++++++++++++++++++++\n> > transport-helper.c | 10 +++\n> > transport.c | 30 +++++++--\n> > transport.h | 6 ++\n> > upload-pack.c | 18 +++--\n> > 11 files changed, 275 insertions(+), 17 deletions(-)\n> \n> It is a bit surprising that there isn't much code removed, as I\n> expected that we'd be factoring out and reusing existing code used\n> in negotiation for fetching into a new helper function (hence the\n> existing codepath would lose a lot of code to be replaced by a call\n> to a new helper function), but that is apparently not what is going\n> on.\n\nThat's what I was trying to do (hence the earlier patches), but the main\nthing I ran into is that a lot of the fetch code assumes that you're\nfetching at least one ref, so I couldn't use a lot of it without more\ncode updating. Having said that, I may have missed more opportunities\nfor reuse.\n\n> I'll have to revisit this step and the next step tomorrow.\n> \n> Thanks.\n\nThanks for taking a look.\n"},{"id":"423320","messageId":"xmqqtunocoqf.fsf@gitster.g","threadId":"55459","inReplyTo":"cover.1617929278.git.jonathantanmy@google.com","subject":"Re: [PATCH 0/6] Push negotiation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-04-30T05:42:32Z","receivedAt":"2021-04-30T05:42:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> Here are patches for push negotiation. The basic idea is that we can\n> reuse part of the fetch protocol (and code) to find out what is in\n> common between the client and server, and then use that information to\n> further narrow the objects sent in the packfile during a push.\n>\n> Patch 1 is a bug fix that probably should be merged even if the rest\n> aren't. Patches 2-4 are refactorings in preparation for the future\n> patches. Patches 5-6 contain the actual logic and documentation.\n>\n> I have written more about it in my prior work [1], although the commit\n> messages and documentation in patches 5-6 should be enough to explain\n> what's going on. (If they're not, feel free to make review comments.)\n>\n> The main change from [1] is that the client-side code that used to be in\n> builtin/fetch-pack.c is now in builtin/fetch.c, because I realized that\n> builtin/fetch-pack.c does not support HTTP. Other than that, all the\n> \"what hasn't been done yet\" items have been done except for statistics\n> in the commit message.\n>\n> [1] https://lore.kernel.org/git/20210218012100.928957-1-jonathantanmy@google.com/\n\nSo... anybody else wants to review this and give it a-OK?\n\n> Jonathan Tan (6):\n> fetch-pack: buffer object-format with other args\n> fetch-pack: refactor process_acks()\n> fetch-pack: refactor add_haves()\n> fetch-pack: refactor command and capability write\n> fetch: teach independent negotiation (no packfile)\n> send-pack: support push negotiation\n>\n> Documentation/config/push.txt | 7 +\n> Documentation/technical/protocol-v2.txt | 8 +\n> builtin/fetch.c | 27 ++-\n> fetch-pack.c | 224 +++++++++++++++---------\n> fetch-pack.h | 11 ++\n> object.h | 2 +-\n> send-pack.c | 61 ++++++-\n> t/t5516-fetch-push.sh | 35 ++++\n> t/t5701-git-serve.sh | 2 +-\n> t/t5702-protocol-v2.sh | 89 ++++++++++\n> transport-helper.c | 10 ++\n> transport.c | 30 +++-\n> transport.h | 6 +\n> upload-pack.c | 18 +-\n> 14 files changed, 430 insertions(+), 100 deletions(-)\n"},{"id":"423362","messageId":"e8d20651-43fb-8e1c-3047-7a0bfa5010e6@gmail.com","threadId":"55459","inReplyTo":"xmqqtunocoqf.fsf@gitster.g","subject":"Re: [PATCH 0/6] Push negotiation","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-04-30T17:33:40Z","receivedAt":"2021-04-30T17:33:44Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 4/30/2021 1:42 AM, Junio C Hamano wrote:\n> Jonathan Tan writes:\n> \n>> Here are patches for push negotiation. The basic idea is that we can\n>> reuse part of the fetch protocol (and code) to find out what is in\n>> common between the client and server, and then use that information to\n>> further narrow the objects sent in the packfile during a push.\n>>\n>> Patch 1 is a bug fix that probably should be merged even if the rest\n>> aren't. Patches 2-4 are refactorings in preparation for the future\n>> patches. Patches 5-6 contain the actual logic and documentation.\n>>\n>> I have written more about it in my prior work [1], although the commit\n>> messages and documentation in patches 5-6 should be enough to explain\n>> what's going on. (If they're not, feel free to make review comments.)\n>>\n>> The main change from [1] is that the client-side code that used to be in\n>> builtin/fetch-pack.c is now in builtin/fetch.c, because I realized that\n>> builtin/fetch-pack.c does not support HTTP. Other than that, all the\n>> \"what hasn't been done yet\" items have been done except for statistics\n>> in the commit message.\n>>\n>> [1] https://lore.kernel.org/git/20210218012100.928957-1-jonathantanmy@google.com/\n> \n> So... anybody else wants to review this and give it a-OK?\n\nI put this on my calendar for Monday morning. I hope that's not too\nlong to wait.\n\nThanks,\n-Stolee\n"},{"id":"423500","messageId":"cb441f5b-33db-90a7-1492-e86c4ec1ed1a@gmail.com","threadId":"55459","inReplyTo":"4696c8e901808853d17af10d5a6d95cd4711c6d5.1617929278.git.jonathantanmy@google.com","subject":"Re: [PATCH 5/6] fetch: teach independent negotiation (no packfile)","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-05-03T15:25:56Z","receivedAt":"2021-05-03T15:26:03Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 4/8/21 9:10 PM, Jonathan Tan wrote:\n> Currently, the packfile negotiation step within a Git fetch cannot be\n> done independent of sending the packfile, even though there is at least\n> one application wherein this is useful. Therefore, make it possible for\n> this negotiation step to be done independently. A subsequent commit will\n> use this for one such application - push negotiation.\n...\n> --- a/Documentation/technical/protocol-v2.txt\n> +++ b/Documentation/technical/protocol-v2.txt\n> @@ -346,6 +346,14 @@ explained below.\n> \tclient should download from all given URIs. Currently, the\n> \tprotocols supported are \"http\" and \"https\".\n> \n> +If the 'wait-for-done' feature is advertised, the following argument\n> +can be included in the client's request.\n> +\n> + wait-for-done\n> +\tIndicates to the server that it should never send \"ready\", but\n> +\tshould wait for the client to say \"done\" before sending the\n> +\tpackfile.\n> +\n\nOk, this is a good way to handle the compatibility case. I'll check\nin the next patch how this feature is checked before sending a pack-file\nin 'git push'.\n\n> @@ -202,6 +203,8 @@ static struct option builtin_fetch_options[] = {\n> \t\t\tTRANSPORT_FAMILY_IPV6),\n> \tOPT_STRING_LIST(0, \"negotiation-tip\", &negotiation_tip, N_(\"revision\"),\n> \t\t\tN_(\"report that we have only objects reachable from this object\")),\n> +\tOPT_BOOL(0, \"negotiate-only\", &negotiate_only,\n> +\t\t N_(\"do not fetch a packfile; instead, print ancestors of negotiation tips\")),\n\nIt seems a little strange for this step, which is mostly used for tests,\nto be so visible. I think it would be too much work to create a test\nhelper with an equivalent implementation, so maybe describe this option\nas only for tests in Documentation/git-fetch.txt.\n \n> -\tif (remote) {\n> +\tif (negotiate_only) {\n> +\t\tstruct oidset acked_commits = OIDSET_INIT;\n> +\t\tstruct oidset_iter iter;\n> +\t\tconst struct object_id *oid;\n> +\n> +\t\tif (!remote)\n> +\t\t\tdie(_(\"Must supply remote when using --negotiate-only\"));\n\nPlease use lowercase letters to start your error messages.\n\n> +\t\tgtransport = prepare_transport(remote, 1);\n> +\t\tif (gtransport->smart_options) {\n> +\t\t\tgtransport->smart_options->acked_commits = &acked_commits;\n> +\t\t} else {\n> +\t\t\twarning(_(\"Protocol does not support --negotiate-only, exiting.\"));\n> +\t\t\treturn 1;\n> +\t\t}\n> +\t\tif (server_options.nr)\n> +\t\t\tgtransport->server_options = &server_options;\n> +\t\tresult = transport_fetch_refs(gtransport, NULL);\n> +\n> +\t\toidset_iter_init(&acked_commits, &iter);\n> +\t\twhile ((oid = oidset_iter_next(&iter)))\n> +\t\t\tprintf(\"%s\\n\", oid_to_hex(oid));\n> +\t\toidset_clear(&acked_commits);\n> +\t} else if (remote) {\n\n...\n\n> +static int add_to_object_array(const struct object_id *oid, void *data)\n> +{\n> +\tstruct object_array *a = data;\n> +\n> +\tadd_object_array(parse_object(the_repository, oid), \"\", a);\n> +\treturn 0;\n> +}\n\nWhy parse_object() over lookup_object()?\n\n> +void negotiate_using_fetch(const struct oid_array *negotiation_tips,\n> +\t\t\t const struct string_list *server_options,\n> +\t\t\t int stateless_rpc,\n> +\t\t\t int fd[],\n> +\t\t\t struct oidset *acked_commits)\n\nIt might be worth a careful doc comment to say that we _must_\nhave already verified the wait-for-done capability.\n\n> +{\n> +\tstruct fetch_negotiator negotiator;\n> +\tstruct packet_reader reader;\n> +\tstruct object_array nt_object_array = OBJECT_ARRAY_INIT;\n> +\tstruct strbuf req_buf = STRBUF_INIT;\n> +\tint haves_to_send = INITIAL_FLUSH;\n> +\tint in_vain = 0;\n> +\tint seen_ack = 0;\n> +\tint last_iteration = 0;\n\nAs will become clear later, create:\n\n\ttimestamp_t min_generation = GENERATION_NUMBER_INFINITY;\n\n> +\n> +\tfetch_negotiator_init(the_repository, &negotiator);\n> +\tmark_tips(&negotiator, negotiation_tips);\n> +\n> +\tpacket_reader_init(&reader, fd[0], NULL, 0,\n> +\t\t\t PACKET_READ_CHOMP_NEWLINE |\n> +\t\t\t PACKET_READ_DIE_ON_ERR_PACKET);\n> +\n> +\toid_array_for_each((struct oid_array *) negotiation_tips,\n> +\t\t\t add_to_object_array,\n> +\t\t\t &nt_object_array);\n> +\n> +\twhile (!last_iteration) {\n> +\t\tint haves_added;\n> +\t\tstruct object_id common_oid;\n> +\t\tint received_ready = 0;\n> +\n> +\t\tstrbuf_reset(&req_buf);\n> +\t\twrite_fetch_command_and_capabilities(&req_buf, server_options);\n> +\n> +\t\tpacket_buf_write(&req_buf, \"wait-for-done\");\n> +\n> +\t\thaves_added = add_haves(&negotiator, &req_buf, &haves_to_send);\n> +\t\tin_vain += haves_added;\n> +\t\tif (!haves_added || (seen_ack && in_vain >= MAX_IN_VAIN))\n> +\t\t\tlast_iteration = 1;\n> +\n> +\t\t/* Send request */\n> +\t\tpacket_buf_flush(&req_buf);\n> +\t\tif (write_in_full(fd[1], req_buf.buf, req_buf.len) < 0)\n> +\t\t\tdie_errno(_(\"unable to write request to remote\"));\n> +\n> +\t\t/* Process ACKs/NAKs */\n> +\t\tprocess_section_header(&reader, \"acknowledgments\", 0);\n> +\t\twhile (process_ack(&negotiator, &reader, &common_oid,\n> +\t\t\t\t &received_ready)) {\n> +\t\t\tstruct commit *commit = lookup_commit(the_repository,\n> +\t\t\t\t\t\t\t &common_oid);\n> +\t\t\tif (commit)\n> +\t\t\t\tcommit->object.flags |= COMMON;\n\nMaking note of this portion of the code: you can update a min_generation\nvalue here, decreasing it if we see a smaller generation.\n\n> +\t\t\tin_vain = 0;\n> +\t\t\tseen_ack = 1;\n> +\t\t\toidset_insert(acked_commits, &common_oid);\n> +\t\t}\n> +\t\tif (received_ready)\n> +\t\t\tdie(_(\"unexpected 'ready' from remote\"));\n> +\t\telse\n> +\t\t\tdo_check_stateless_delimiter(stateless_rpc, &reader);\n> +\t\tif (can_all_from_reach_with_flag(&nt_object_array, COMMON,\n> +\t\t\t\t\t\t REACH_SCRATCH, 0,\n> +\t\t\t\t\t\t GENERATION_NUMBER_ZERO))\n\nAnd finally here you can use min_generation. Otherwise, you will suffer\nperformance problems when there is a deep history but this returns false.\n\nAs long as min_generation is the minimum generation of a commit with the\nCOMMON flag, you will be correct to use that limit.\n\n> +\t\t\tlast_iteration = 1;\n> +\t}\n> +\tstrbuf_release(&req_buf);\n\nDon't forget to clear the COMMON flag before returning.\n\n> +}\n> +\n\n> +/*\n> + * Execute the --negotiate-only mode of \"git fetch\", adding all known common\n> + * commits to acked_commits.\n> + */> +void negotiate_using_fetch(const struct oid_array *negotiation_tips,\n> +\t\t\t const struct string_list *server_options,\n> +\t\t\t int stateless_rpc,\n> +\t\t\t int fd[],\n> +\t\t\t struct oidset *acked_commits);\n\nHere is a good place to specifically mention the wait-for-done capability,\nor else this method will die() when seeing the \"ready\" from the server.\n\n> +setup_negotiate_only () {\n> +\tSERVER=\"$1\"\n> +\tURI=\"$2\"\n> +\n> +\trm -rf \"$SERVER\" client\n> +\n> +\tgit init \"$SERVER\"\n> +\ttest_commit -C \"$SERVER\" one\n> +\ttest_commit -C \"$SERVER\" two\n> +\n> +\tgit clone \"$URI\" client\n> +\ttest_commit -C client three\n> +}\n> +\n> +test_expect_success 'file:// --negotiate-only' '\n> +\tSERVER=\"server\" &&\n> +\tURI=\"file://$(pwd)/server\" &&\n> +\n> +\tsetup_negotiate_only \"$SERVER\" \"$URI\" &&\n> +\n> +\tgit -C client fetch \\\n> +\t\t--no-tags \\\n> +\t\t--negotiate-only \\\n> +\t\t--negotiation-tip=$(git -C client rev-parse HEAD) \\\n> +\t\torigin >out &&\n> +\tCOMMON=$(git -C \"$SERVER\" rev-parse two) &&\n> +\tgrep \"$COMMON\" out\n> +'\n> +\n> +test_expect_success 'file:// --negotiate-only with protocol v0' '\n> +\tSERVER=\"server\" &&\n> +\tURI=\"file://$(pwd)/server\" &&\n> +\n> +\tsetup_negotiate_only \"$SERVER\" \"$URI\" &&\n> +\n> +\ttest_must_fail git -c protocol.version=0 -C client fetch \\\n> +\t\t--no-tags \\\n> +\t\t--negotiate-only \\\n> +\t\t--negotiation-tip=$(git -C client rev-parse HEAD) \\\n> +\t\torigin 2>err &&\n> +\ttest_i18ngrep \"negotiate-only requires protocol v2\" err\n> +'\n> +\n> # Test protocol v2 with 'http://' transport\n> #\n> . \"$TEST_DIRECTORY\"/lib-httpd.sh\n> @@ -1035,6 +1078,52 @@ test_expect_success 'packfile-uri with transfer.fsckobjects fails when .gitmodul\n> \ttest_i18ngrep \"disallowed submodule name\" err\n> '\n> \n> +test_expect_success 'http:// --negotiate-only' '\n> +\tSERVER=\"$HTTPD_DOCUMENT_ROOT_PATH/server\" &&\n> +\tURI=\"$HTTPD_URL/smart/server\" &&\n> +\n> +\tsetup_negotiate_only \"$SERVER\" \"$URI\" &&\n> +\n> +\tgit -C client fetch \\\n> +\t\t--no-tags \\\n> +\t\t--negotiate-only \\\n> +\t\t--negotiation-tip=$(git -C client rev-parse HEAD) \\\n> +\t\torigin >out &&\n> +\tCOMMON=$(git -C \"$SERVER\" rev-parse two) &&\n> +\tgrep \"$COMMON\" out\n> +'\n> +\n> +test_expect_success 'http:// --negotiate-only without wait-for-done support' '\n> +\tSERVER=\"server\" &&\n> +\tURI=\"$HTTPD_URL/one_time_perl/server\" &&\n> +\n> +\tsetup_negotiate_only \"$SERVER\" \"$URI\" &&\n> +\n> +\techo \"s/ wait-for-done/ xxxx-xxx-xxxx/\" \\\n> +\t\t>\"$HTTPD_ROOT_PATH/one-time-perl\" &&\n> +\n> +\ttest_must_fail git -C client fetch \\\n> +\t\t--no-tags \\\n> +\t\t--negotiate-only \\\n> +\t\t--negotiation-tip=$(git -C client rev-parse HEAD) \\\n> +\t\torigin 2>err &&\n> +\ttest_i18ngrep \"server does not support wait-for-done\" err\n> +'\n> +\n> +test_expect_success 'http:// --negotiate-only with protocol v0' '\n> +\tSERVER=\"$HTTPD_DOCUMENT_ROOT_PATH/server\" &&\n> +\tURI=\"$HTTPD_URL/smart/server\" &&\n> +\n> +\tsetup_negotiate_only \"$SERVER\" \"$URI\" &&\n> +\n> +\ttest_must_fail git -c protocol.version=0 -C client fetch \\\n> +\t\t--no-tags \\\n> +\t\t--negotiate-only \\\n> +\t\t--negotiation-tip=$(git -C client rev-parse HEAD) \\\n> +\t\torigin 2>err &&\n> +\ttest_i18ngrep \"negotiate-only requires protocol v2\" err\n> +'\n> +\n> # DO NOT add non-httpd-specific tests here, because the last part of this\n> # test script is only executed when httpd is available and enabled.\n> \n> diff --git a/transport-helper.c b/transport-helper.c\n> index 4cd76366fa..4be035edb8 100644\n> --- a/transport-helper.c\n> +++ b/transport-helper.c\n> @@ -684,6 +684,16 @@ static int fetch(struct transport *transport,\n> \t\treturn transport->vtable->fetch(transport, nr_heads, to_fetch);\n> \t}\n> \n> +\t/*\n> +\t * If we reach here, then the server, the client, and/or the transport\n> +\t * helper does not support protocol v2. --negotiate-only requires\n> +\t * protocol v2.\n> +\t */\n> +\tif (data->transport_options.acked_commits) {\n> +\t\twarning(_(\"--negotiate-only requires protocol v2\"));\n> +\t\treturn -1;\n> +\t}\n> +\n\nThis method continues to do a lot that doesn't seem specific to\n--negotiate-only. The warning message seems incorrect to me, but\nis also seems like this would break several cases when using\nprotocol v0. It is equally possible that I'm misunderstanding\nwhat is going on here.\n\n> +\tif (data->options.acked_commits) {\n> +\t\tif (data->version < protocol_v2) {\n> +\t\t\twarning(_(\"--negotiate-only requires protocol v2\"));\n> +\t\t\tret = -1;\n> +\t\t} else if (!server_supports_feature(\"fetch\", \"wait-for-done\", 0)) {\n> +\t\t\twarning(_(\"server does not support wait-for-done\"));\n> +\t\t\tret = -1;\n> +\t\t} else {\n> +\t\t\tnegotiate_using_fetch(data->options.negotiation_tips,\n> +\t\t\t\t\t transport->server_options,\n> +\t\t\t\t\t transport->stateless_rpc,\n> +\t\t\t\t\t data->fd,\n> +\t\t\t\t\t data->options.acked_commits);\n> +\t\t\tret = 0;\n> +\t\t}\n> +\t\tgoto cleanup;\n> +\t}\n\nThese prereqs look right. This makes it look rather strange that\nnegotiate_using_fetch() returns void (and has several die() calls inside)\nand could instead return an 'int' that is consumed here (and use\nerror() calls instead of die()).\n\n> \trefs = fetch_pack(&args, data->fd,\n> \t\t\t refs_tmp ? refs_tmp : transport->remote_refs,\n> \t\t\t to_fetch, nr_heads, &data->shallow,\n> \t\t\t &transport->pack_lockfiles, data->version);\n> \n> -\tclose(data->fd[0]);\n> -\tclose(data->fd[1]);\n> -\tif (finish_connect(data->conn))\n> -\t\tret = -1;\n> -\tdata->conn = NULL;\n...\n\n> +cleanup:\n> +\tclose(data->fd[0]);\n> +\tclose(data->fd[1]);\n> +\tif (finish_connect(data->conn))\n> +\t\tret = -1;\n> +\tdata->conn = NULL;\n> +\n\nGood to keep these after skipping a big chunk of the fetch logic.\n\n> +\n> +\t/*\n> +\t * If set, whenever transport_fetch_refs() is called, add known common\n> +\t * commits to this oidset instead of fetching any packfiles.\n> +\t */\n> +\tstruct oidset *acked_commits;\n\nnit: s/If set/If allocated/\n\n> @@ -1668,10 +1673,13 @@ int upload_pack_v2(struct repository *r, struct strvec *keys,\n> \t\tcase FETCH_PROCESS_ARGS:\n> \t\t\tprocess_args(request, &data);\n> \n> -\t\t\tif (!data.want_obj.nr) {\n> +\t\t\tif (!data.want_obj.nr && !data.wait_for_done) {\n> \t\t\t\t/*\n> -\t\t\t\t * Request didn't contain any 'want' lines,\n> -\t\t\t\t * guess they didn't want anything.\n> +\t\t\t\t * Request didn't contain any 'want' lines (and\n> +\t\t\t\t * the request does not contain\n> +\t\t\t\t * \"wait-for-done\", in which it is reasonable\n> +\t\t\t\t * to just send 'have's without 'want's); guess\n> +\t\t\t\t * they didn't want anything.\n> \t\t\t\t */\n> \t\t\t\tstate = FETCH_DONE;\n> \t\t\t} else if (data.haves.nr) {\n> @@ -1723,7 +1731,7 @@ int upload_pack_advertise(struct repository *r,\n> \t\tint allow_sideband_all_value;\n> \t\tchar *str = NULL;\n> \n> -\t\tstrbuf_addstr(value, \"shallow\");\n> +\t\tstrbuf_addstr(value, \"shallow wait-for-done\");\n\nIt might be nice to have this wait-for-done capability configurable.\nI'd likely want this on by default, but in case there is an issue it\nis easier to change config values than ship new binaries.\n\nThis is a pretty big patch. It might have been nice to split the\nserver-side stuff out as its own patch with a follow-up for the client\ncode and the tests.\n\nThanks,\n-Stolee\n"},{"id":"423501","messageId":"c2780570-81ec-bede-2f4e-75748b788bbb@gmail.com","threadId":"55459","inReplyTo":"a2daa1022c41820b2109d9572069d12684470cb8.1617929278.git.jonathantanmy@google.com","subject":"Re: [PATCH 6/6] send-pack: support push negotiation","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-05-03T15:35:02Z","receivedAt":"2021-05-03T15:35:11Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 4/8/21 9:10 PM, Jonathan Tan wrote:\n> Teach Git the push.negotiate config variable.\n...\n> +push.negotiate::\n> +\tIf set to \"true\", attempt to reduce the size of the packfile\n> +\tsent by rounds of negotiation in which the client and the\n> +\tserver attempt to find commits in common. If \"false\", Git will\n> +\trely solely on the server's ref advertisement to find commits\n> +\tin common.\n\nWorks for me.\n\n> diff --git a/send-pack.c b/send-pack.c\n> index 5f215b13c7..9cb9f71650 100644\n> --- a/send-pack.c\n> +++ b/send-pack.c\n> @@ -56,7 +56,9 @@ static void feed_object(const struct object_id *oid, FILE *fh, int negative)\n> /*\n> * Make a pack stream and spit it out into file descriptor fd\n> */\n> -static int pack_objects(int fd, struct ref *refs, struct oid_array *extra, struct send_pack_args *args)\n> +static int pack_objects(int fd, struct ref *refs, struct oid_array *advertised,\n> +\t\t\tstruct oid_array *negotiated,\n> +\t\t\tstruct send_pack_args *args)\n\nAt the moment, I don't see why we need two oid_arrays here.\nInstead, this 'extra' could instead be renamed to something\nlike 'server_objects' or 'base_objects' to make it clear\nthat we don't want those objects, and can even use them and\ntheir reachable objects as delta bases, when appropriate.\n\nOr, just don't touch it.\n\n> +static void get_commons_through_negotiation(const char *url,\n> +\t\t\t\t\t const struct ref *remote_refs,\n> +\t\t\t\t\t struct oid_array *commons)\n> +{\n> +\tstruct child_process child = CHILD_PROCESS_INIT;\n> +\tconst struct ref *ref;\n> +\tint len = the_hash_algo->hexsz + 1; /* hash + NL */\n> +\n> +\tchild.git_cmd = 1;\n> +\tchild.no_stdin = 1;\n> +\tchild.out = -1;\n> +\tstrvec_pushl(&child.args, \"fetch\", \"--negotiate-only\", NULL);\n> +\tfor (ref = remote_refs; ref; ref = ref->next)\n> +\t\tstrvec_pushf(&child.args, \"--negotiation-tip=%s\", oid_to_hex(&ref->new_oid));\n> +\tstrvec_push(&child.args, url);\n\nOh! We are using a 'git fetch --negotiate-only' subprocess here. You\ncan ignore my previous message about updating the docs for this to be\nused only by tests.\n\n> +\n> +\tif (start_command(&child))\n> +\t\tdie(_(\"send-pack: unable to fork off fetch subprocess\"));\n> +\n> +\tdo {\n> +\t\tchar hex_hash[GIT_MAX_HEXSZ + 1];\n> +\t\tint read_len = read_in_full(child.out, hex_hash, len);\n> +\t\tstruct object_id oid;\n> +\t\tconst char *end;\n> +\n> +\t\tif (!read_len)\n> +\t\t\tbreak;\n> +\t\tif (read_len != len)\n> +\t\t\tdie(\"invalid length read %d\", read_len);\n> +\t\tif (parse_oid_hex(hex_hash, &oid, &end) || *end != '\\n')\n> +\t\t\tdie(\"invalid hash\");\n> +\t\toid_array_append(commons, &oid);\nThis appends, so there is no reason why it needs to be empty before\nthe method. Is there a way we could feed the extra_have set when\ncalling this method? Or is it happening at a strange time?\n\n> +\t} while (1);\n> +\n> +\tif (finish_command(&child)) {\n> +\t\t/*\n> +\t\t * The information that push negotiation provides is useful but\n> +\t\t * not mandatory.\n> +\t\t */\n> +\t\twarning(_(\"push negotiation failed; proceeding anyway with push\"));\n> +\t}\n> +}\n> +\n> int send_pack(struct send_pack_args *args,\n> \t int fd[], struct child_process *conn,\n> \t struct ref *remote_refs,\n> \t struct oid_array *extra_have)\n> {\n> +\tstruct oid_array commons = OID_ARRAY_INIT;\n> \tint in = fd[0];\n> \tint out = fd[1];\n> \tstruct strbuf req_buf = STRBUF_INIT;\n> @@ -426,6 +474,7 @@ int send_pack(struct send_pack_args *args,\n> \tint quiet_supported = 0;\n> \tint agent_supported = 0;\n> \tint advertise_sid = 0;\n> +\tint push_negotiate = 0;\n> \tint use_atomic = 0;\n> \tint atomic_supported = 0;\n> \tint use_push_options = 0;\n> @@ -437,6 +486,10 @@ int send_pack(struct send_pack_args *args,\n> \tconst char *push_cert_nonce = NULL;\n> \tstruct packet_reader reader;\n> \n> +\tgit_config_get_bool(\"push.negotiate\", &push_negotiate);\n> +\tif (push_negotiate)\n> +\t\tget_commons_through_negotiation(args->url, remote_refs, &commons);\n> +\n> \tgit_config_get_bool(\"transfer.advertisesid\", &advertise_sid);\n> \n> \t/* Does the other end support the reporting? */\n> @@ -625,7 +678,7 @@ int send_pack(struct send_pack_args *args,\n> \t\t\t PACKET_READ_DIE_ON_ERR_PACKET);\n> \n> \tif (need_pack_data && cmds_sent) {\n> -\t\tif (pack_objects(out, remote_refs, extra_have, args) < 0) {\n> +\t\tif (pack_objects(out, remote_refs, extra_have, &commons, args) < 0) {\n\nHere, it would be nice if extra_have and commons were merged before calling\npack_objects(). I mentioned a way to perhaps make that easier above, but\nthe context might not make that be super-simple. Running a loop here to\nscan 'commons' and append them to 'extra_have' might be a sufficient\napproach.\n\nGenerally, this approach seems like it would work. I have not done any\nlocal testing, yet.\n\nThanks,\n-Stolee\n\n"},{"id":"423503","messageId":"d30876b3-e94a-3a80-db0d-c20ec510e92a@gmail.com","threadId":"55459","inReplyTo":"cb441f5b-33db-90a7-1492-e86c4ec1ed1a@gmail.com","subject":"Re: [PATCH 5/6] fetch: teach independent negotiation (no packfile)","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-05-03T15:40:10Z","receivedAt":"2021-05-03T15:40:16Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 5/3/21 11:25 AM, Derrick Stolee wrote:\n> On 4/8/21 9:10 PM, Jonathan Tan wrote:\n>> Currently, the packfile negotiation step within a Git fetch cannot be\n>> done independent of sending the packfile, even though there is at least\n>> one application wherein this is useful. Therefore, make it possible for\n>> this negotiation step to be done independently. A subsequent commit will\n>> use this for one such application - push negotiation.\n> ...\n>> diff --git a/transport-helper.c b/transport-helper.c\n>> index 4cd76366fa..4be035edb8 100644\n>> --- a/transport-helper.c\n>> +++ b/transport-helper.c\n>> @@ -684,6 +684,16 @@ static int fetch(struct transport *transport,\n>> \t\treturn transport->vtable->fetch(transport, nr_heads, to_fetch);\n>> \t}\n>> \n>> +\t/*\n>> +\t * If we reach here, then the server, the client, and/or the transport\n>> +\t * helper does not support protocol v2. --negotiate-only requires\n>> +\t * protocol v2.\n>> +\t */\n>> +\tif (data->transport_options.acked_commits) {\n>> +\t\twarning(_(\"--negotiate-only requires protocol v2\"));\n>> +\t\treturn -1;\n>> +\t}\n>> +\n> \n> This method continues to do a lot that doesn't seem specific to\n> --negotiate-only. The warning message seems incorrect to me, but\n> is also seems like this would break several cases when using\n> protocol v0. It is equally possible that I'm misunderstanding\n> what is going on here.\n\nI didn't see this cause any failures when running the test suite\nwith GIT_TEST_PROTOCOL_VERSION=0, but I _do_ see failures in your\ntests added in PATCH 6 with that environment variable set. You\nmight want to set it manually in the important test cases.\n\nThanks,\n-Stolee\n\n"},{"id":"423509","messageId":"15b4fb49-6254-bd6f-4c34-f04b55adced8@gmail.com","threadId":"55459","inReplyTo":"6a9f78df1a3513df04ac3275cd1feccfb6cf87b1.1617929278.git.jonathantanmy@google.com","subject":"Re: [PATCH 2/6] fetch-pack: refactor process_acks()","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-05-03T16:30:04Z","receivedAt":"2021-05-03T16:30:10Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 4/8/21 9:09 PM, Jonathan Tan wrote:\n> A subsequent commit will need part, but not all, of the functionality in\n> process_acks(), so move some of its functionality to its sole caller\n> do_fetch_pack_v2(). As a side effect, the resulting code is also\n> shorter.\n> \t\tcase FETCH_PROCESS_ACKS:\n> \t\t\t/* Process ACKs/NAKs */\n> -\t\t\tswitch (process_acks(negotiator, &reader, &common)) {\n\nGood riddance to this nested switch!\n\nI am unfamiliar with this code, but I believe these changes to\nbe correct.\n\nThanks,\n-Stolee\n"},{"id":"423540","messageId":"20210503215239.2877489-1-jonathantanmy@google.com","threadId":"55459","inReplyTo":"cb441f5b-33db-90a7-1492-e86c4ec1ed1a@gmail.com","subject":"Re: [PATCH 5/6] fetch: teach independent negotiation (no packfile)","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-05-03T21:52:39Z","receivedAt":"2021-05-03T21:52:46Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> On 4/8/21 9:10 PM, Jonathan Tan wrote:\n> > Currently, the packfile negotiation step within a Git fetch cannot be\n> > done independent of sending the packfile, even though there is at least\n> > one application wherein this is useful. Therefore, make it possible for\n> > this negotiation step to be done independently. A subsequent commit will\n> > use this for one such application - push negotiation.\n> ...\n> > --- a/Documentation/technical/protocol-v2.txt\n> > +++ b/Documentation/technical/protocol-v2.txt\n> > @@ -346,6 +346,14 @@ explained below.\n> > \tclient should download from all given URIs. Currently, the\n> > \tprotocols supported are \"http\" and \"https\".\n> > \n> > +If the 'wait-for-done' feature is advertised, the following argument\n> > +can be included in the client's request.\n> > +\n> > + wait-for-done\n> > +\tIndicates to the server that it should never send \"ready\", but\n> > +\tshould wait for the client to say \"done\" before sending the\n> > +\tpackfile.\n> > +\n> \n> Ok, this is a good way to handle the compatibility case. I'll check\n> in the next patch how this feature is checked before sending a pack-file\n> in 'git push'.\n\nThanks.\n\n> > @@ -202,6 +203,8 @@ static struct option builtin_fetch_options[] = {\n> > \t\t\tTRANSPORT_FAMILY_IPV6),\n> > \tOPT_STRING_LIST(0, \"negotiation-tip\", &negotiation_tip, N_(\"revision\"),\n> > \t\t\tN_(\"report that we have only objects reachable from this object\")),\n> > +\tOPT_BOOL(0, \"negotiate-only\", &negotiate_only,\n> > +\t\t N_(\"do not fetch a packfile; instead, print ancestors of negotiation tips\")),\n> \n> It seems a little strange for this step, which is mostly used for tests,\n> to be so visible. I think it would be too much work to create a test\n> helper with an equivalent implementation, so maybe describe this option\n> as only for tests in Documentation/git-fetch.txt.\n\nAs you mention in your review of the subsequent patch [1], this is\nbecause we're calling \"fetch\" as a subprocess from \"push\", so it needs\nto accept command-line arguments specifying that we're not interested in\nthe packfile.\n\n[1] https://lore.kernel.org/git/c2780570-81ec-bede-2f4e-75748b788bbb@gmail.com/\n\n> > -\tif (remote) {\n> > +\tif (negotiate_only) {\n> > +\t\tstruct oidset acked_commits = OIDSET_INIT;\n> > +\t\tstruct oidset_iter iter;\n> > +\t\tconst struct object_id *oid;\n> > +\n> > +\t\tif (!remote)\n> > +\t\t\tdie(_(\"Must supply remote when using --negotiate-only\"));\n> \n> Please use lowercase letters to start your error messages.\n\nThanks - done.\n\n> > +static int add_to_object_array(const struct object_id *oid, void *data)\n> > +{\n> > +\tstruct object_array *a = data;\n> > +\n> > +\tadd_object_array(parse_object(the_repository, oid), \"\", a);\n> > +\treturn 0;\n> > +}\n> \n> Why parse_object() over lookup_object()?\n\nSwitched to lookup_object(). I don't think it matters here because we'll\nneed to parse all commits when doing negotiation anyway (to find their\nparents), but I changed it to avoid confusion.\n\n> > +void negotiate_using_fetch(const struct oid_array *negotiation_tips,\n> > +\t\t\t const struct string_list *server_options,\n> > +\t\t\t int stateless_rpc,\n> > +\t\t\t int fd[],\n> > +\t\t\t struct oidset *acked_commits)\n> \n> It might be worth a careful doc comment to say that we _must_\n> have already verified the wait-for-done capability.\n\nOK - written in the .h file.\n\n[snip min_generation explanation]\n\n> And finally here you can use min_generation. Otherwise, you will suffer\n> performance problems when there is a deep history but this returns false.\n> \n> As long as min_generation is the minimum generation of a commit with the\n> COMMON flag, you will be correct to use that limit.\n\nThanks for the precise guidance of how to use generation numbers to\nspeed things up. I've implemented your suggestions.\n\n> \n> > +\t\t\tlast_iteration = 1;\n> > +\t}\n> > +\tstrbuf_release(&req_buf);\n> \n> Don't forget to clear the COMMON flag before returning.\n\nOK - done. (I don't think it matters here, but I guess it's clearer if\nwe do as you suggest. The loop will take some time to run, but it's\nsmall compared to the network time needed anyway.)\n\n> > +/*\n> > + * Execute the --negotiate-only mode of \"git fetch\", adding all known common\n> > + * commits to acked_commits.\n> > + */> +void negotiate_using_fetch(const struct oid_array *negotiation_tips,\n> > +\t\t\t const struct string_list *server_options,\n> > +\t\t\t int stateless_rpc,\n> > +\t\t\t int fd[],\n> > +\t\t\t struct oidset *acked_commits);\n> \n> Here is a good place to specifically mention the wait-for-done capability,\n> or else this method will die() when seeing the \"ready\" from the server.\n\nAcknowledged.\n\n> > diff --git a/transport-helper.c b/transport-helper.c\n> > index 4cd76366fa..4be035edb8 100644\n> > --- a/transport-helper.c\n> > +++ b/transport-helper.c\n> > @@ -684,6 +684,16 @@ static int fetch(struct transport *transport,\n> > \t\treturn transport->vtable->fetch(transport, nr_heads, to_fetch);\n> > \t}\n> > \n> > +\t/*\n> > +\t * If we reach here, then the server, the client, and/or the transport\n> > +\t * helper does not support protocol v2. --negotiate-only requires\n> > +\t * protocol v2.\n> > +\t */\n> > +\tif (data->transport_options.acked_commits) {\n> > +\t\twarning(_(\"--negotiate-only requires protocol v2\"));\n> > +\t\treturn -1;\n> > +\t}\n> > +\n> \n> This method continues to do a lot that doesn't seem specific to\n> --negotiate-only. The warning message seems incorrect to me, but\n> is also seems like this would break several cases when using\n> protocol v0. It is equally possible that I'm misunderstanding\n> what is going on here.\n\nIt's specific to --negotiate-only in that acked_commits is set if and\nonly if we use --negotiate-only. What cases are you think of that break\nwhen using protocol v0?\n\n> > +\tif (data->options.acked_commits) {\n> > +\t\tif (data->version < protocol_v2) {\n> > +\t\t\twarning(_(\"--negotiate-only requires protocol v2\"));\n> > +\t\t\tret = -1;\n> > +\t\t} else if (!server_supports_feature(\"fetch\", \"wait-for-done\", 0)) {\n> > +\t\t\twarning(_(\"server does not support wait-for-done\"));\n> > +\t\t\tret = -1;\n> > +\t\t} else {\n> > +\t\t\tnegotiate_using_fetch(data->options.negotiation_tips,\n> > +\t\t\t\t\t transport->server_options,\n> > +\t\t\t\t\t transport->stateless_rpc,\n> > +\t\t\t\t\t data->fd,\n> > +\t\t\t\t\t data->options.acked_commits);\n> > +\t\t\tret = 0;\n> > +\t\t}\n> > +\t\tgoto cleanup;\n> > +\t}\n> \n> These prereqs look right. This makes it look rather strange that\n> negotiate_using_fetch() returns void (and has several die() calls inside)\n> and could instead return an 'int' that is consumed here (and use\n> error() calls instead of die()).\n\nWhen negotiate_using_fetch() is used, we are already past the capability\nadvertisement step, so we need to believe here that the wait-for-done\nprerequisite has been met. (Admittedly, said prerequisite wasn't\ndocumented, but I've documented it following your suggestion above.) So\nI think it would be too late to return an \"int\" after several\nnegotiation steps have occurred and the packfile downloaded.\n\n> > +\n> > +\t/*\n> > +\t * If set, whenever transport_fetch_refs() is called, add known common\n> > +\t * commits to this oidset instead of fetching any packfiles.\n> > +\t */\n> > +\tstruct oidset *acked_commits;\n> \n> nit: s/If set/If allocated/\n\nChanged.\n\n> > @@ -1668,10 +1673,13 @@ int upload_pack_v2(struct repository *r, struct strvec *keys,\n> > \t\tcase FETCH_PROCESS_ARGS:\n> > \t\t\tprocess_args(request, &data);\n> > \n> > -\t\t\tif (!data.want_obj.nr) {\n> > +\t\t\tif (!data.want_obj.nr && !data.wait_for_done) {\n> > \t\t\t\t/*\n> > -\t\t\t\t * Request didn't contain any 'want' lines,\n> > -\t\t\t\t * guess they didn't want anything.\n> > +\t\t\t\t * Request didn't contain any 'want' lines (and\n> > +\t\t\t\t * the request does not contain\n> > +\t\t\t\t * \"wait-for-done\", in which it is reasonable\n> > +\t\t\t\t * to just send 'have's without 'want's); guess\n> > +\t\t\t\t * they didn't want anything.\n> > \t\t\t\t */\n> > \t\t\t\tstate = FETCH_DONE;\n> > \t\t\t} else if (data.haves.nr) {\n> > @@ -1723,7 +1731,7 @@ int upload_pack_advertise(struct repository *r,\n> > \t\tint allow_sideband_all_value;\n> > \t\tchar *str = NULL;\n> > \n> > -\t\tstrbuf_addstr(value, \"shallow\");\n> > +\t\tstrbuf_addstr(value, \"shallow wait-for-done\");\n> \n> It might be nice to have this wait-for-done capability configurable.\n> I'd likely want this on by default, but in case there is an issue it\n> is easier to change config values than ship new binaries.\n\nI felt that we didn't need this knob on the server side as this doesn't\nexpose any more information than a regular fetch, but I'm open to adding\nit if neeed be.\n\n> This is a pretty big patch. It might have been nice to split the\n> server-side stuff out as its own patch with a follow-up for the client\n> code and the tests.\n> \n> Thanks,\n> -Stolee\n\nPersonally I prefer that such changes (server and client) be together,\nso that I can see on both sides how things work. There is (in theory)\nalready a clear divide between both sides, even within a patch, since\nthe server and client parts are usually in different parts. But if the\nmajority prefers that such changes be separate, I'm OK with doing that\ntoo.\n\nAlso, thanks for your review.\n"},{"id":"423542","messageId":"20210503220244.2878522-1-jonathantanmy@google.com","threadId":"55459","inReplyTo":"c2780570-81ec-bede-2f4e-75748b788bbb@gmail.com","subject":"Re: [PATCH 6/6] send-pack: support push negotiation","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-05-03T22:02:44Z","receivedAt":"2021-05-03T22:02:49Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"[snip asking about whether extra_have (a.k.a. advertised) and commons (a.k.a.\nnegotiated) can be merged]\n\n> Here, it would be nice if extra_have and commons were merged before calling\n> pack_objects(). I mentioned a way to perhaps make that easier above, but\n> the context might not make that be super-simple. Running a loop here to\n> scan 'commons' and append them to 'extra_have' might be a sufficient\n> approach.\n> \n> Generally, this approach seems like it would work. I have not done any\n> local testing, yet.\n> \n> Thanks,\n> -Stolee\n\nI was reluctant to merge them because that would involve either (1)\nadding commons to \"extra_have\" (as you suggest) or (2) iterating through\n\"extra_have\" in order to add it to the \"commons\" set. For (1), this\nwould modify \"extra_have\", which is passed in from the outside. Looking\nat its callers, the main one in git_transport_push() in transport.c\ncalls send_pack() with a set that has traversed the transport API, so I\nthink it would be confusing if such a set suddenly changed. For (2), the\nextra loop seems more troublesome than having two parameters with\nclearer names indicating where they come from. I don't mind changing to\n(2), though, if people want it.\n"},{"id":"423600","messageId":"70ea6b85-bc73-3f39-3449-9c08f6738a73@gmail.com","threadId":"55459","inReplyTo":"20210503220244.2878522-1-jonathantanmy@google.com","subject":"Re: [PATCH 6/6] send-pack: support push negotiation","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-05-04T17:26:29Z","receivedAt":"2021-05-04T17:26:35Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 5/3/2021 6:02 PM, Jonathan Tan wrote:\n> [snip asking about whether extra_have (a.k.a. advertised) and commons (a.k.a.\n> negotiated) can be merged]\n> \n>> Here, it would be nice if extra_have and commons were merged before calling\n>> pack_objects(). I mentioned a way to perhaps make that easier above, but\n>> the context might not make that be super-simple. Running a loop here to\n>> scan 'commons' and append them to 'extra_have' might be a sufficient\n>> approach.\n>>\n>> Generally, this approach seems like it would work. I have not done any\n>> local testing, yet.\n>>\n>> Thanks,\n>> -Stolee\n> \n> I was reluctant to merge them because that would involve either (1)\n> adding commons to \"extra_have\" (as you suggest) or (2) iterating through\n> \"extra_have\" in order to add it to the \"commons\" set. For (1), this\n> would modify \"extra_have\", which is passed in from the outside. Looking\n> at its callers, the main one in git_transport_push() in transport.c\n> calls send_pack() with a set that has traversed the transport API, so I\n> think it would be confusing if such a set suddenly changed. For (2), the\n> extra loop seems more troublesome than having two parameters with\n> clearer names indicating where they come from. I don't mind changing to\n> (2), though, if people want it.\n \nI suppose this concern about \"ownership\" is valid and worth having the\ntwo parameters in the helper function for extra safety.\n\nThanks,\n-Stolee\n"},{"id":"423612","messageId":"cover.1620162764.git.jonathantanmy@google.com","threadId":"55459","inReplyTo":"cover.1617929278.git.jonathantanmy@google.com","subject":"[PATCH v2 0/5] Push negotiation","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-05-04T21:15:57Z","receivedAt":"2021-05-04T21:16:11Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Thanks for the review, Junio and Stolee.\n\nThe first patch in version 1 has been merged to master, I believe, so\nhere are the remaining 5 patches.\n\nAs Stolee pointed out, some tests in version 1 fail when\nGIT_TEST_PROTOCOL_VERSION is set to 0. I have corrected that, but there\nis one test that still fails in t5601. That seems to also fail in\n\"master\". I'll see if I can fix it and send a separate patch for that.\n\nJonathan Tan (5):\n fetch-pack: refactor process_acks()\n fetch-pack: refactor add_haves()\n fetch-pack: refactor command and capability write\n fetch: teach independent negotiation (no packfile)\n send-pack: support push negotiation\n\n Documentation/config/push.txt | 7 +\n Documentation/technical/protocol-v2.txt | 8 +\n builtin/fetch.c | 27 ++-\n fetch-pack.c | 246 ++++++++++++++++--------\n fetch-pack.h | 14 ++\n object.h | 2 +-\n send-pack.c | 61 +++++-\n t/t5516-fetch-push.sh | 35 ++++\n t/t5701-git-serve.sh | 2 +-\n t/t5702-protocol-v2.sh | 89 +++++++++\n transport-helper.c | 10 +\n transport.c | 30 ++-\n transport.h | 6 +\n upload-pack.c | 18 +-\n 14 files changed, 455 insertions(+), 100 deletions(-)\n\nRange-diff against v1:\n-: ---------- > 1: 8102570374 fetch-pack: refactor process_acks()\n-: ---------- > 2: 57c3451b2e fetch-pack: refactor add_haves()\n-: ---------- > 3: 6871d0cec6 fetch-pack: refactor command and capability write\n1: 3ebe4ada28 ! 4: 1de34a6dce fetch: teach independent negotiation (no packfile)\n @@ Commit message\n There are 2 code paths that do not go through fetch_refs_via_pack() that\n needed to be individually excluded: the bundle transport (excluded\n through requiring smart_options, which the bundle transport doesn't\n - support) and transport helpers that do not support takeover.\n - Fortunately, none of these support protocol v2.\n + support) and transport helpers that do not support takeover. If or when\n + we support independent negotiation for protocol v0, we will need to\n + modify these 2 code paths to support it. But for now, report failure if\n + independent negotiation is requested in these cases.\n \n Signed-off-by: Jonathan Tan \n Signed-off-by: Junio C Hamano \n @@ builtin/fetch.c: int cmd_fetch(int argc, const char **argv, const char *prefix)\n +\t\tconst struct object_id *oid;\n +\n +\t\tif (!remote)\n -+\t\t\tdie(_(\"Must supply remote when using --negotiate-only\"));\n ++\t\t\tdie(_(\"must supply remote when using --negotiate-only\"));\n +\t\tgtransport = prepare_transport(remote, 1);\n +\t\tif (gtransport->smart_options) {\n +\t\t\tgtransport->smart_options->acked_commits = &acked_commits;\n @@ fetch-pack.c\n #include \"fsck.h\"\n #include \"shallow.h\"\n +#include \"commit-reach.h\"\n ++#include \"commit-graph.h\"\n \n static int transfer_unpack_limit = -1;\n static int fetch_unpack_limit = -1;\n @@ fetch-pack.c: struct ref *fetch_pack(struct fetch_pack_args *args,\n +{\n +\tstruct object_array *a = data;\n +\n -+\tadd_object_array(parse_object(the_repository, oid), \"\", a);\n ++\tadd_object_array(lookup_object(the_repository, oid), \"\", a);\n +\treturn 0;\n +}\n +\n ++static void clear_common_flag(struct oidset *s)\n ++{\n ++\tstruct oidset_iter iter;\n ++\tconst struct object_id *oid;\n ++\toidset_iter_init(s, &iter);\n ++\n ++\twhile ((oid = oidset_iter_next(&iter))) {\n ++\t\tstruct object *obj = lookup_object(the_repository, oid);\n ++\t\tobj->flags &= ~COMMON;\n ++\t}\n ++}\n ++\n +void negotiate_using_fetch(const struct oid_array *negotiation_tips,\n +\t\t\t const struct string_list *server_options,\n +\t\t\t int stateless_rpc,\n @@ fetch-pack.c: struct ref *fetch_pack(struct fetch_pack_args *args,\n +\tint in_vain = 0;\n +\tint seen_ack = 0;\n +\tint last_iteration = 0;\n ++\ttimestamp_t min_generation = GENERATION_NUMBER_INFINITY;\n +\n +\tfetch_negotiator_init(the_repository, &negotiator);\n +\tmark_tips(&negotiator, negotiation_tips);\n @@ fetch-pack.c: struct ref *fetch_pack(struct fetch_pack_args *args,\n +\t\t\t\t &received_ready)) {\n +\t\t\tstruct commit *commit = lookup_commit(the_repository,\n +\t\t\t\t\t\t\t &common_oid);\n -+\t\t\tif (commit)\n ++\t\t\tif (commit) {\n ++\t\t\t\ttimestamp_t generation;\n ++\n ++\t\t\t\tparse_commit_or_die(commit);\n +\t\t\t\tcommit->object.flags |= COMMON;\n ++\t\t\t\tgeneration = commit_graph_generation(commit);\n ++\t\t\t\tif (generation < min_generation)\n ++\t\t\t\t\tmin_generation = generation;\n ++\t\t\t}\n +\t\t\tin_vain = 0;\n +\t\t\tseen_ack = 1;\n +\t\t\toidset_insert(acked_commits, &common_oid);\n @@ fetch-pack.c: struct ref *fetch_pack(struct fetch_pack_args *args,\n +\t\t\tdo_check_stateless_delimiter(stateless_rpc, &reader);\n +\t\tif (can_all_from_reach_with_flag(&nt_object_array, COMMON,\n +\t\t\t\t\t\t REACH_SCRATCH, 0,\n -+\t\t\t\t\t\t GENERATION_NUMBER_ZERO))\n ++\t\t\t\t\t\t min_generation))\n +\t\t\tlast_iteration = 1;\n +\t}\n ++\tclear_common_flag(acked_commits);\n +\tstrbuf_release(&req_buf);\n +}\n +\n @@ fetch-pack.h: struct ref *fetch_pack(struct fetch_pack_args *args,\n +/*\n + * Execute the --negotiate-only mode of \"git fetch\", adding all known common\n + * commits to acked_commits.\n ++ *\n ++ * In the capability advertisement that has happened prior to invoking this\n ++ * function, the \"wait-for-done\" capability must be present.\n + */\n +void negotiate_using_fetch(const struct oid_array *negotiation_tips,\n +\t\t\t const struct string_list *server_options,\n @@ t/t5702-protocol-v2.sh: test_expect_success 'deepen-relative' '\n +\n +\tsetup_negotiate_only \"$SERVER\" \"$URI\" &&\n +\n -+\tgit -C client fetch \\\n ++\tgit -c protocol.version=2 -C client fetch \\\n +\t\t--no-tags \\\n +\t\t--negotiate-only \\\n +\t\t--negotiation-tip=$(git -C client rev-parse HEAD) \\\n @@ t/t5702-protocol-v2.sh: test_expect_success 'packfile-uri with transfer.fsckobje\n +\n +\tsetup_negotiate_only \"$SERVER\" \"$URI\" &&\n +\n -+\tgit -C client fetch \\\n ++\tgit -c protocol.version=2 -C client fetch \\\n +\t\t--no-tags \\\n +\t\t--negotiate-only \\\n +\t\t--negotiation-tip=$(git -C client rev-parse HEAD) \\\n @@ t/t5702-protocol-v2.sh: test_expect_success 'packfile-uri with transfer.fsckobje\n +\techo \"s/ wait-for-done/ xxxx-xxx-xxxx/\" \\\n +\t\t>\"$HTTPD_ROOT_PATH/one-time-perl\" &&\n +\n -+\ttest_must_fail git -C client fetch \\\n ++\ttest_must_fail git -c protocol.version=2 -C client fetch \\\n +\t\t--no-tags \\\n +\t\t--negotiate-only \\\n +\t\t--negotiation-tip=$(git -C client rev-parse HEAD) \\\n @@ transport.h: struct git_transport_options {\n \tstruct oid_array *negotiation_tips;\n +\n +\t/*\n -+\t * If set, whenever transport_fetch_refs() is called, add known common\n -+\t * commits to this oidset instead of fetching any packfiles.\n ++\t * If allocated, whenever transport_fetch_refs() is called, add known\n ++\t * common commits to this oidset instead of fetching any packfiles.\n +\t */\n +\tstruct oidset *acked_commits;\n };\n2: bd55d6ba36 ! 5: d38a8b7d66 send-pack: support push negotiation\n @@ t/t5516-fetch-push.sh: test_expect_success 'fetch with pushInsteadOf (should not\n +\tgit push testrepo $the_first_commit:refs/remotes/origin/first_commit &&\n +\tgit -C testrepo config receive.hideRefs refs/remotes/origin/first_commit &&\n +\techo now pushing without negotiation &&\n -+\tGIT_TRACE2_EVENT=\"$(pwd)/event\" git push testrepo refs/heads/main:refs/remotes/origin/main &&\n ++\tGIT_TRACE2_EVENT=\"$(pwd)/event\" git -c protocol.version=2 push testrepo refs/heads/main:refs/remotes/origin/main &&\n +\tgrep_wrote 5 event && # 2 commits, 2 trees, 1 blob\n +\n +\t# Same commands, but with negotiation\n @@ t/t5516-fetch-push.sh: test_expect_success 'fetch with pushInsteadOf (should not\n +\tmk_empty testrepo &&\n +\tgit push testrepo $the_first_commit:refs/remotes/origin/first_commit &&\n +\tgit -C testrepo config receive.hideRefs refs/remotes/origin/first_commit &&\n -+\tGIT_TRACE2_EVENT=\"$(pwd)/event\" git -c push.negotiate=1 push testrepo refs/heads/main:refs/remotes/origin/main &&\n ++\tGIT_TRACE2_EVENT=\"$(pwd)/event\" git -c protocol.version=2 -c push.negotiate=1 push testrepo refs/heads/main:refs/remotes/origin/main &&\n +\tgrep_wrote 2 event # 1 commit, 1 tree\n +'\n +\n @@ t/t5516-fetch-push.sh: test_expect_success 'fetch with pushInsteadOf (should not\n +\tmk_empty testrepo &&\n +\tgit push testrepo $the_first_commit:refs/remotes/origin/first_commit &&\n +\tgit -C testrepo config receive.hideRefs refs/remotes/origin/first_commit &&\n -+\tGIT_TRACE_PACKET=\"$(pwd)/trace\" GIT_TEST_PROTOCOL_VERSION=0 GIT_TRACE2_EVENT=\"$(pwd)/event\" \\\n ++\tGIT_TEST_PROTOCOL_VERSION=0 GIT_TRACE2_EVENT=\"$(pwd)/event\" \\\n +\t\tgit -c push.negotiate=1 push testrepo refs/heads/main:refs/remotes/origin/main 2>err &&\n +\tgrep_wrote 5 event && # 2 commits, 2 trees, 1 blob\n +\ttest_i18ngrep \"push negotiation failed\" err\n-- \n2.31.1.527.g47e6f16901-goog\n\n"},{"id":"423613","messageId":"8102570374a87c032bff2d7114c67390f55f9a1b.1620162764.git.jonathantanmy@google.com","threadId":"55459","inReplyTo":"cover.1620162764.git.jonathantanmy@google.com","subject":"[PATCH v2 1/5] fetch-pack: refactor process_acks()","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-05-04T21:15:58Z","receivedAt":"2021-05-04T21:16:12Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"A subsequent commit will need part, but not all, of the functionality in\nprocess_acks(), so move some of its functionality to its sole caller\ndo_fetch_pack_v2(). As a side effect, the resulting code is also\nshorter.\n\nSigned-off-by: Jonathan Tan \nSigned-off-by: Junio C Hamano \n---\n fetch-pack.c | 70 +++++++++++++++++-----------------------------------\n 1 file changed, 22 insertions(+), 48 deletions(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 2318ebe680..9f3901cdba 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -1351,35 +1351,11 @@ static int process_section_header(struct packet_reader *reader,\n \treturn ret;\n }\n \n-enum common_found {\n-\t/*\n-\t * No commit was found to be possessed by both the client and the\n-\t * server, and \"ready\" was not received.\n-\t */\n-\tNO_COMMON_FOUND,\n-\n-\t/*\n-\t * At least one commit was found to be possessed by both the client and\n-\t * the server, and \"ready\" was not received.\n-\t */\n-\tCOMMON_FOUND,\n-\n-\t/*\n-\t * \"ready\" was received, indicating that the server is ready to send\n-\t * the packfile without any further negotiation.\n-\t */\n-\tREADY\n-};\n-\n-static enum common_found process_acks(struct fetch_negotiator *negotiator,\n-\t\t\t\t struct packet_reader *reader,\n-\t\t\t\t struct oidset *common)\n+static int process_ack(struct fetch_negotiator *negotiator,\n+\t\t struct packet_reader *reader,\n+\t\t struct object_id *common_oid,\n+\t\t int *received_ready)\n {\n-\t/* received */\n-\tint received_ready = 0;\n-\tint received_ack = 0;\n-\n-\tprocess_section_header(reader, \"acknowledgments\", 0);\n \twhile (packet_reader_read(reader) == PACKET_READ_NORMAL) {\n \t\tconst char *arg;\n \n@@ -1387,20 +1363,17 @@ static enum common_found process_acks(struct fetch_negotiator *negotiator,\n \t\t\tcontinue;\n \n \t\tif (skip_prefix(reader->line, \"ACK \", &arg)) {\n-\t\t\tstruct object_id oid;\n-\t\t\treceived_ack = 1;\n-\t\t\tif (!get_oid_hex(arg, &oid)) {\n+\t\t\tif (!get_oid_hex(arg, common_oid)) {\n \t\t\t\tstruct commit *commit;\n-\t\t\t\toidset_insert(common, &oid);\n-\t\t\t\tcommit = lookup_commit(the_repository, &oid);\n+\t\t\t\tcommit = lookup_commit(the_repository, common_oid);\n \t\t\t\tif (negotiator)\n \t\t\t\t\tnegotiator->ack(negotiator, commit);\n \t\t\t}\n-\t\t\tcontinue;\n+\t\t\treturn 1;\n \t\t}\n \n \t\tif (!strcmp(reader->line, \"ready\")) {\n-\t\t\treceived_ready = 1;\n+\t\t\t*received_ready = 1;\n \t\t\tcontinue;\n \t\t}\n \n@@ -1418,13 +1391,12 @@ static enum common_found process_acks(struct fetch_negotiator *negotiator,\n \t * sent. Therefore, a DELIM is expected if \"ready\" is sent, and a FLUSH\n \t * otherwise.\n \t */\n-\tif (received_ready && reader->status != PACKET_READ_DELIM)\n+\tif (*received_ready && reader->status != PACKET_READ_DELIM)\n \t\tdie(_(\"expected packfile to be sent after 'ready'\"));\n-\tif (!received_ready && reader->status != PACKET_READ_FLUSH)\n+\tif (!*received_ready && reader->status != PACKET_READ_FLUSH)\n \t\tdie(_(\"expected no other sections to be sent after no 'ready'\"));\n \n-\treturn received_ready ? READY :\n-\t\t(received_ack ? COMMON_FOUND : NO_COMMON_FOUND);\n+\treturn 0;\n }\n \n static void receive_shallow_info(struct fetch_pack_args *args,\n@@ -1573,6 +1545,8 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \tstruct fetch_negotiator negotiator_alloc;\n \tstruct fetch_negotiator *negotiator;\n \tint seen_ack = 0;\n+\tstruct object_id common_oid;\n+\tint received_ready = 0;\n \tstruct string_list packfile_uris = STRING_LIST_INIT_DUP;\n \tint i;\n \tstruct strvec index_pack_args = STRVEC_INIT;\n@@ -1631,22 +1605,22 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \t\t\tbreak;\n \t\tcase FETCH_PROCESS_ACKS:\n \t\t\t/* Process ACKs/NAKs */\n-\t\t\tswitch (process_acks(negotiator, &reader, &common)) {\n-\t\t\tcase READY:\n+\t\t\tprocess_section_header(&reader, \"acknowledgments\", 0);\n+\t\t\twhile (process_ack(negotiator, &reader, &common_oid,\n+\t\t\t\t\t &received_ready)) {\n+\t\t\t\tin_vain = 0;\n+\t\t\t\tseen_ack = 1;\n+\t\t\t\toidset_insert(&common, &common_oid);\n+\t\t\t}\n+\t\t\tif (received_ready) {\n \t\t\t\t/*\n \t\t\t\t * Don't check for response delimiter; get_pack() will\n \t\t\t\t * read the rest of this response.\n \t\t\t\t */\n \t\t\t\tstate = FETCH_GET_PACK;\n-\t\t\t\tbreak;\n-\t\t\tcase COMMON_FOUND:\n-\t\t\t\tin_vain = 0;\n-\t\t\t\tseen_ack = 1;\n-\t\t\t\t/* fallthrough */\n-\t\t\tcase NO_COMMON_FOUND:\n+\t\t\t} else {\n \t\t\t\tdo_check_stateless_delimiter(args, &reader);\n \t\t\t\tstate = FETCH_SEND_REQUEST;\n-\t\t\t\tbreak;\n \t\t\t}\n \t\t\tbreak;\n \t\tcase FETCH_GET_PACK:\n-- \n2.31.1.527.g47e6f16901-goog\n\n"},{"id":"423614","messageId":"57c3451b2e36d003aa7fc398c644ce71ab668b5e.1620162764.git.jonathantanmy@google.com","threadId":"55459","inReplyTo":"cover.1620162764.git.jonathantanmy@google.com","subject":"[PATCH v2 2/5] fetch-pack: refactor add_haves()","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-05-04T21:15:59Z","receivedAt":"2021-05-04T21:16:15Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"A subsequent commit will need part, but not all, of the functionality in\nadd_haves(), so move some of its functionality to its sole caller\nsend_fetch_request().\n\nSigned-off-by: Jonathan Tan \nSigned-off-by: Junio C Hamano \n---\n fetch-pack.c | 28 ++++++++++++----------------\n 1 file changed, 12 insertions(+), 16 deletions(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 9f3901cdba..128ad47d2a 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -1195,11 +1195,9 @@ static void add_common(struct strbuf *req_buf, struct oidset *common)\n }\n \n static int add_haves(struct fetch_negotiator *negotiator,\n-\t\t int seen_ack,\n \t\t struct strbuf *req_buf,\n-\t\t int *haves_to_send, int *in_vain)\n+\t\t int *haves_to_send)\n {\n-\tint ret = 0;\n \tint haves_added = 0;\n \tconst struct object_id *oid;\n \n@@ -1209,17 +1207,10 @@ static int add_haves(struct fetch_negotiator *negotiator,\n \t\t\tbreak;\n \t}\n \n-\t*in_vain += haves_added;\n-\tif (!haves_added || (seen_ack && *in_vain >= MAX_IN_VAIN)) {\n-\t\t/* Send Done */\n-\t\tpacket_buf_write(req_buf, \"done\\n\");\n-\t\tret = 1;\n-\t}\n-\n \t/* Increase haves to send on next round */\n \t*haves_to_send = next_flush(1, *haves_to_send);\n \n-\treturn ret;\n+\treturn haves_added;\n }\n \n static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,\n@@ -1228,7 +1219,8 @@ static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,\n \t\t\t int *haves_to_send, int *in_vain,\n \t\t\t int sideband_all, int seen_ack)\n {\n-\tint ret = 0;\n+\tint haves_added;\n+\tint done_sent = 0;\n \tconst char *hash_name;\n \tstruct strbuf req_buf = STRBUF_INIT;\n \n@@ -1312,9 +1304,13 @@ static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,\n \t/* Add all of the common commits we've found in previous rounds */\n \tadd_common(&req_buf, common);\n \n-\t/* Add initial haves */\n-\tret = add_haves(negotiator, seen_ack, &req_buf,\n-\t\t\thaves_to_send, in_vain);\n+\thaves_added = add_haves(negotiator, &req_buf, haves_to_send);\n+\t*in_vain += haves_added;\n+\tif (!haves_added || (seen_ack && *in_vain >= MAX_IN_VAIN)) {\n+\t\t/* Send Done */\n+\t\tpacket_buf_write(&req_buf, \"done\\n\");\n+\t\tdone_sent = 1;\n+\t}\n \n \t/* Send request */\n \tpacket_buf_flush(&req_buf);\n@@ -1322,7 +1318,7 @@ static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,\n \t\tdie_errno(_(\"unable to write request to remote\"));\n \n \tstrbuf_release(&req_buf);\n-\treturn ret;\n+\treturn done_sent;\n }\n \n /*\n-- \n2.31.1.527.g47e6f16901-goog\n\n"},{"id":"423615","messageId":"6871d0cec62dc12d0c5f7390eee8a80614919578.1620162764.git.jonathantanmy@google.com","threadId":"55459","inReplyTo":"cover.1620162764.git.jonathantanmy@google.com","subject":"[PATCH v2 3/5] fetch-pack: refactor command and capability write","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-05-04T21:16:00Z","receivedAt":"2021-05-04T21:16:16Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"A subsequent commit will need this functionality independent of the rest\nof send_fetch_request(), so put this into its own function.\n\nSigned-off-by: Jonathan Tan \nSigned-off-by: Junio C Hamano \n---\n fetch-pack.c | 41 ++++++++++++++++++++++++-----------------\n 1 file changed, 24 insertions(+), 17 deletions(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 128ad47d2a..512fe5450d 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -1213,29 +1213,23 @@ static int add_haves(struct fetch_negotiator *negotiator,\n \treturn haves_added;\n }\n \n-static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,\n-\t\t\t struct fetch_pack_args *args,\n-\t\t\t const struct ref *wants, struct oidset *common,\n-\t\t\t int *haves_to_send, int *in_vain,\n-\t\t\t int sideband_all, int seen_ack)\n+static void write_fetch_command_and_capabilities(struct strbuf *req_buf,\n+\t\t\t\t\t\t const struct string_list *server_options)\n {\n-\tint haves_added;\n-\tint done_sent = 0;\n \tconst char *hash_name;\n-\tstruct strbuf req_buf = STRBUF_INIT;\n \n \tif (server_supports_v2(\"fetch\", 1))\n-\t\tpacket_buf_write(&req_buf, \"command=fetch\");\n+\t\tpacket_buf_write(req_buf, \"command=fetch\");\n \tif (server_supports_v2(\"agent\", 0))\n-\t\tpacket_buf_write(&req_buf, \"agent=%s\", git_user_agent_sanitized());\n+\t\tpacket_buf_write(req_buf, \"agent=%s\", git_user_agent_sanitized());\n \tif (advertise_sid && server_supports_v2(\"session-id\", 0))\n-\t\tpacket_buf_write(&req_buf, \"session-id=%s\", trace2_session_id());\n-\tif (args->server_options && args->server_options->nr &&\n+\t\tpacket_buf_write(req_buf, \"session-id=%s\", trace2_session_id());\n+\tif (server_options && server_options->nr &&\n \t server_supports_v2(\"server-option\", 1)) {\n \t\tint i;\n-\t\tfor (i = 0; i < args->server_options->nr; i++)\n-\t\t\tpacket_buf_write(&req_buf, \"server-option=%s\",\n-\t\t\t\t\t args->server_options->items[i].string);\n+\t\tfor (i = 0; i < server_options->nr; i++)\n+\t\t\tpacket_buf_write(req_buf, \"server-option=%s\",\n+\t\t\t\t\t server_options->items[i].string);\n \t}\n \n \tif (server_feature_v2(\"object-format\", &hash_name)) {\n@@ -1243,13 +1237,26 @@ static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,\n \t\tif (hash_algo_by_ptr(the_hash_algo) != hash_algo)\n \t\t\tdie(_(\"mismatched algorithms: client %s; server %s\"),\n \t\t\t the_hash_algo->name, hash_name);\n-\t\tpacket_buf_write(&req_buf, \"object-format=%s\", the_hash_algo->name);\n+\t\tpacket_buf_write(req_buf, \"object-format=%s\", the_hash_algo->name);\n \t} else if (hash_algo_by_ptr(the_hash_algo) != GIT_HASH_SHA1) {\n \t\tdie(_(\"the server does not support algorithm '%s'\"),\n \t\t the_hash_algo->name);\n \t}\n+\tpacket_buf_delim(req_buf);\n+}\n+\n+static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,\n+\t\t\t struct fetch_pack_args *args,\n+\t\t\t const struct ref *wants, struct oidset *common,\n+\t\t\t int *haves_to_send, int *in_vain,\n+\t\t\t int sideband_all, int seen_ack)\n+{\n+\tint haves_added;\n+\tint done_sent = 0;\n+\tstruct strbuf req_buf = STRBUF_INIT;\n+\n+\twrite_fetch_command_and_capabilities(&req_buf, args->server_options);\n \n-\tpacket_buf_delim(&req_buf);\n \tif (args->use_thin_pack)\n \t\tpacket_buf_write(&req_buf, \"thin-pack\");\n \tif (args->no_progress)\n-- \n2.31.1.527.g47e6f16901-goog\n\n"},{"id":"423616","messageId":"1de34a6dced3f5477162dd675615342bc2dec05f.1620162764.git.jonathantanmy@google.com","threadId":"55459","inReplyTo":"cover.1620162764.git.jonathantanmy@google.com","subject":"[PATCH v2 4/5] fetch: teach independent negotiation (no packfile)","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-05-04T21:16:01Z","receivedAt":"2021-05-04T21:16:18Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Currently, the packfile negotiation step within a Git fetch cannot be\ndone independent of sending the packfile, even though there is at least\none application wherein this is useful. Therefore, make it possible for\nthis negotiation step to be done independently. A subsequent commit will\nuse this for one such application - push negotiation.\n\nThis feature is for protocol v2 only. (An implementation for protocol v0\nwould require a separate implementation in the fetch, transport, and\ntransport helper code.)\n\nIn the protocol, the main hindrance towards independent negotiation is\nthat the server can unilaterally decide to send the packfile. This is\nsolved by a \"wait-for-done\" argument: the server will then wait for the\nclient to say \"done\". In practice, the client will never say it; instead\nit will cease requests once it is satisfied.\n\nIn the client, the main change lies in the transport and transport\nhelper code. fetch_refs_via_pack() performs everything needed - protocol\nversion and capability checks, and the negotiation itself.\n\nThere are 2 code paths that do not go through fetch_refs_via_pack() that\nneeded to be individually excluded: the bundle transport (excluded\nthrough requiring smart_options, which the bundle transport doesn't\nsupport) and transport helpers that do not support takeover. If or when\nwe support independent negotiation for protocol v0, we will need to\nmodify these 2 code paths to support it. But for now, report failure if\nindependent negotiation is requested in these cases.\n\nSigned-off-by: Jonathan Tan \nSigned-off-by: Junio C Hamano \n---\n Documentation/technical/protocol-v2.txt | 8 ++\n builtin/fetch.c | 27 +++++-\n fetch-pack.c | 111 +++++++++++++++++++++++-\n fetch-pack.h | 14 +++\n object.h | 2 +-\n t/t5701-git-serve.sh | 2 +-\n t/t5702-protocol-v2.sh | 89 +++++++++++++++++++\n transport-helper.c | 10 +++\n transport.c | 30 +++++--\n transport.h | 6 ++\n upload-pack.c | 18 ++--\n 11 files changed, 300 insertions(+), 17 deletions(-)\n\ndiff --git a/Documentation/technical/protocol-v2.txt b/Documentation/technical/protocol-v2.txt\nindex a7c806a73e..0b371d8de4 100644\n--- a/Documentation/technical/protocol-v2.txt\n+++ b/Documentation/technical/protocol-v2.txt\n@@ -346,6 +346,14 @@ explained below.\n \tclient should download from all given URIs. Currently, the\n \tprotocols supported are \"http\" and \"https\".\n \n+If the 'wait-for-done' feature is advertised, the following argument\n+can be included in the client's request.\n+\n+ wait-for-done\n+\tIndicates to the server that it should never send \"ready\", but\n+\tshould wait for the client to say \"done\" before sending the\n+\tpackfile.\n+\n The response of `fetch` is broken into a number of sections separated by\n delimiter packets (0001), with each section beginning with its section\n header. Most sections are sent only when the packfile is sent.\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 0b90de87c7..a732295a6b 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -82,6 +82,7 @@ static struct string_list server_options = STRING_LIST_INIT_DUP;\n static struct string_list negotiation_tip = STRING_LIST_INIT_NODUP;\n static int fetch_write_commit_graph = -1;\n static int stdin_refspecs = 0;\n+static int negotiate_only;\n \n static int git_fetch_config(const char *k, const char *v, void *cb)\n {\n@@ -202,6 +203,8 @@ static struct option builtin_fetch_options[] = {\n \t\t\tTRANSPORT_FAMILY_IPV6),\n \tOPT_STRING_LIST(0, \"negotiation-tip\", &negotiation_tip, N_(\"revision\"),\n \t\t\tN_(\"report that we have only objects reachable from this object\")),\n+\tOPT_BOOL(0, \"negotiate-only\", &negotiate_only,\n+\t\t N_(\"do not fetch a packfile; instead, print ancestors of negotiation tips\")),\n \tOPT_PARSE_LIST_OBJECTS_FILTER(&filter_options),\n \tOPT_BOOL(0, \"auto-maintenance\", &enable_auto_gc,\n \t\t N_(\"run 'maintenance --auto' after fetching\")),\n@@ -1986,7 +1989,29 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\t}\n \t}\n \n-\tif (remote) {\n+\tif (negotiate_only) {\n+\t\tstruct oidset acked_commits = OIDSET_INIT;\n+\t\tstruct oidset_iter iter;\n+\t\tconst struct object_id *oid;\n+\n+\t\tif (!remote)\n+\t\t\tdie(_(\"must supply remote when using --negotiate-only\"));\n+\t\tgtransport = prepare_transport(remote, 1);\n+\t\tif (gtransport->smart_options) {\n+\t\t\tgtransport->smart_options->acked_commits = &acked_commits;\n+\t\t} else {\n+\t\t\twarning(_(\"Protocol does not support --negotiate-only, exiting.\"));\n+\t\t\treturn 1;\n+\t\t}\n+\t\tif (server_options.nr)\n+\t\t\tgtransport->server_options = &server_options;\n+\t\tresult = transport_fetch_refs(gtransport, NULL);\n+\n+\t\toidset_iter_init(&acked_commits, &iter);\n+\t\twhile ((oid = oidset_iter_next(&iter)))\n+\t\t\tprintf(\"%s\\n\", oid_to_hex(oid));\n+\t\toidset_clear(&acked_commits);\n+\t} else if (remote) {\n \t\tif (filter_options.choice || has_promisor_remote())\n \t\t\tfetch_one_setup_partial(remote);\n \t\tresult = fetch_one(remote, argc, argv, prune_tags_ok, stdin_refspecs);\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 512fe5450d..c135635e34 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -23,6 +23,8 @@\n #include \"fetch-negotiator.h\"\n #include \"fsck.h\"\n #include \"shallow.h\"\n+#include \"commit-reach.h\"\n+#include \"commit-graph.h\"\n \n static int transfer_unpack_limit = -1;\n static int fetch_unpack_limit = -1;\n@@ -45,6 +47,8 @@ static struct string_list uri_protocols = STRING_LIST_INIT_DUP;\n /* Remember to update object flag allocation in object.h */\n #define COMPLETE\t(1U << 0)\n #define ALTERNATE\t(1U << 1)\n+#define COMMON\t\t(1U << 6)\n+#define REACH_SCRATCH\t(1U << 7)\n \n /*\n * After sending this many \"have\"s if we do not get any new ACK , we\n@@ -1523,10 +1527,10 @@ enum fetch_state {\n \tFETCH_DONE,\n };\n \n-static void do_check_stateless_delimiter(const struct fetch_pack_args *args,\n+static void do_check_stateless_delimiter(int stateless_rpc,\n \t\t\t\t\t struct packet_reader *reader)\n {\n-\tcheck_stateless_delimiter(args->stateless_rpc, reader,\n+\tcheck_stateless_delimiter(stateless_rpc, reader,\n \t\t\t\t _(\"git fetch-pack: expected response end packet\"));\n }\n \n@@ -1622,7 +1626,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \t\t\t\t */\n \t\t\t\tstate = FETCH_GET_PACK;\n \t\t\t} else {\n-\t\t\t\tdo_check_stateless_delimiter(args, &reader);\n+\t\t\t\tdo_check_stateless_delimiter(args->stateless_rpc, &reader);\n \t\t\t\tstate = FETCH_SEND_REQUEST;\n \t\t\t}\n \t\t\tbreak;\n@@ -1645,7 +1649,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \t\t\t\t packfile_uris.nr ? &index_pack_args : NULL,\n \t\t\t\t sought, nr_sought, &fsck_options.gitmodules_found))\n \t\t\t\tdie(_(\"git fetch-pack: fetch failed.\"));\n-\t\t\tdo_check_stateless_delimiter(args, &reader);\n+\t\t\tdo_check_stateless_delimiter(args->stateless_rpc, &reader);\n \n \t\t\tstate = FETCH_DONE;\n \t\t\tbreak;\n@@ -1962,6 +1966,105 @@ struct ref *fetch_pack(struct fetch_pack_args *args,\n \treturn ref_cpy;\n }\n \n+static int add_to_object_array(const struct object_id *oid, void *data)\n+{\n+\tstruct object_array *a = data;\n+\n+\tadd_object_array(lookup_object(the_repository, oid), \"\", a);\n+\treturn 0;\n+}\n+\n+static void clear_common_flag(struct oidset *s)\n+{\n+\tstruct oidset_iter iter;\n+\tconst struct object_id *oid;\n+\toidset_iter_init(s, &iter);\n+\n+\twhile ((oid = oidset_iter_next(&iter))) {\n+\t\tstruct object *obj = lookup_object(the_repository, oid);\n+\t\tobj->flags &= ~COMMON;\n+\t}\n+}\n+\n+void negotiate_using_fetch(const struct oid_array *negotiation_tips,\n+\t\t\t const struct string_list *server_options,\n+\t\t\t int stateless_rpc,\n+\t\t\t int fd[],\n+\t\t\t struct oidset *acked_commits)\n+{\n+\tstruct fetch_negotiator negotiator;\n+\tstruct packet_reader reader;\n+\tstruct object_array nt_object_array = OBJECT_ARRAY_INIT;\n+\tstruct strbuf req_buf = STRBUF_INIT;\n+\tint haves_to_send = INITIAL_FLUSH;\n+\tint in_vain = 0;\n+\tint seen_ack = 0;\n+\tint last_iteration = 0;\n+\ttimestamp_t min_generation = GENERATION_NUMBER_INFINITY;\n+\n+\tfetch_negotiator_init(the_repository, &negotiator);\n+\tmark_tips(&negotiator, negotiation_tips);\n+\n+\tpacket_reader_init(&reader, fd[0], NULL, 0,\n+\t\t\t PACKET_READ_CHOMP_NEWLINE |\n+\t\t\t PACKET_READ_DIE_ON_ERR_PACKET);\n+\n+\toid_array_for_each((struct oid_array *) negotiation_tips,\n+\t\t\t add_to_object_array,\n+\t\t\t &nt_object_array);\n+\n+\twhile (!last_iteration) {\n+\t\tint haves_added;\n+\t\tstruct object_id common_oid;\n+\t\tint received_ready = 0;\n+\n+\t\tstrbuf_reset(&req_buf);\n+\t\twrite_fetch_command_and_capabilities(&req_buf, server_options);\n+\n+\t\tpacket_buf_write(&req_buf, \"wait-for-done\");\n+\n+\t\thaves_added = add_haves(&negotiator, &req_buf, &haves_to_send);\n+\t\tin_vain += haves_added;\n+\t\tif (!haves_added || (seen_ack && in_vain >= MAX_IN_VAIN))\n+\t\t\tlast_iteration = 1;\n+\n+\t\t/* Send request */\n+\t\tpacket_buf_flush(&req_buf);\n+\t\tif (write_in_full(fd[1], req_buf.buf, req_buf.len) < 0)\n+\t\t\tdie_errno(_(\"unable to write request to remote\"));\n+\n+\t\t/* Process ACKs/NAKs */\n+\t\tprocess_section_header(&reader, \"acknowledgments\", 0);\n+\t\twhile (process_ack(&negotiator, &reader, &common_oid,\n+\t\t\t\t &received_ready)) {\n+\t\t\tstruct commit *commit = lookup_commit(the_repository,\n+\t\t\t\t\t\t\t &common_oid);\n+\t\t\tif (commit) {\n+\t\t\t\ttimestamp_t generation;\n+\n+\t\t\t\tparse_commit_or_die(commit);\n+\t\t\t\tcommit->object.flags |= COMMON;\n+\t\t\t\tgeneration = commit_graph_generation(commit);\n+\t\t\t\tif (generation < min_generation)\n+\t\t\t\t\tmin_generation = generation;\n+\t\t\t}\n+\t\t\tin_vain = 0;\n+\t\t\tseen_ack = 1;\n+\t\t\toidset_insert(acked_commits, &common_oid);\n+\t\t}\n+\t\tif (received_ready)\n+\t\t\tdie(_(\"unexpected 'ready' from remote\"));\n+\t\telse\n+\t\t\tdo_check_stateless_delimiter(stateless_rpc, &reader);\n+\t\tif (can_all_from_reach_with_flag(&nt_object_array, COMMON,\n+\t\t\t\t\t\t REACH_SCRATCH, 0,\n+\t\t\t\t\t\t min_generation))\n+\t\t\tlast_iteration = 1;\n+\t}\n+\tclear_common_flag(acked_commits);\n+\tstrbuf_release(&req_buf);\n+}\n+\n int report_unmatched_refs(struct ref **sought, int nr_sought)\n {\n \tint i, ret = 0;\ndiff --git a/fetch-pack.h b/fetch-pack.h\nindex f114d72775..7f94a2a583 100644\n--- a/fetch-pack.h\n+++ b/fetch-pack.h\n@@ -5,6 +5,7 @@\n #include \"run-command.h\"\n #include \"protocol.h\"\n #include \"list-objects-filter-options.h\"\n+#include \"oidset.h\"\n \n struct oid_array;\n \n@@ -81,6 +82,19 @@ struct ref *fetch_pack(struct fetch_pack_args *args,\n \t\t struct string_list *pack_lockfiles,\n \t\t enum protocol_version version);\n \n+/*\n+ * Execute the --negotiate-only mode of \"git fetch\", adding all known common\n+ * commits to acked_commits.\n+ *\n+ * In the capability advertisement that has happened prior to invoking this\n+ * function, the \"wait-for-done\" capability must be present.\n+ */\n+void negotiate_using_fetch(const struct oid_array *negotiation_tips,\n+\t\t\t const struct string_list *server_options,\n+\t\t\t int stateless_rpc,\n+\t\t\t int fd[],\n+\t\t\t struct oidset *acked_commits);\n+\n /*\n * Print an appropriate error message for each sought ref that wasn't\n * matched. Return 0 if all sought refs were matched, otherwise 1.\ndiff --git a/object.h b/object.h\nindex 59daadce21..4806fa8e66 100644\n--- a/object.h\n+++ b/object.h\n@@ -60,7 +60,7 @@ struct object_array {\n /*\n * object flag allocation:\n * revision.h: 0---------10 15 23------26\n- * fetch-pack.c: 01\n+ * fetch-pack.c: 01 67\n * negotiator/default.c: 2--5\n * walker.c: 0-2\n * upload-pack.c: 4 11-----14 16-----19\ndiff --git a/t/t5701-git-serve.sh b/t/t5701-git-serve.sh\nindex 509f379d49..f03bb04803 100755\n--- a/t/t5701-git-serve.sh\n+++ b/t/t5701-git-serve.sh\n@@ -16,7 +16,7 @@ test_expect_success 'test capability advertisement' '\n \tversion 2\n \tagent=git/$(git version | cut -d\" \" -f3)\n \tls-refs=unborn\n-\tfetch=shallow\n+\tfetch=shallow wait-for-done\n \tserver-option\n \tobject-format=$(test_oid algo)\n \t0000\ndiff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh\nindex 2e1243ca40..66af411057 100755\n--- a/t/t5702-protocol-v2.sh\n+++ b/t/t5702-protocol-v2.sh\n@@ -585,6 +585,49 @@ test_expect_success 'deepen-relative' '\n \ttest_cmp expected actual\n '\n \n+setup_negotiate_only () {\n+\tSERVER=\"$1\"\n+\tURI=\"$2\"\n+\n+\trm -rf \"$SERVER\" client\n+\n+\tgit init \"$SERVER\"\n+\ttest_commit -C \"$SERVER\" one\n+\ttest_commit -C \"$SERVER\" two\n+\n+\tgit clone \"$URI\" client\n+\ttest_commit -C client three\n+}\n+\n+test_expect_success 'file:// --negotiate-only' '\n+\tSERVER=\"server\" &&\n+\tURI=\"file://$(pwd)/server\" &&\n+\n+\tsetup_negotiate_only \"$SERVER\" \"$URI\" &&\n+\n+\tgit -c protocol.version=2 -C client fetch \\\n+\t\t--no-tags \\\n+\t\t--negotiate-only \\\n+\t\t--negotiation-tip=$(git -C client rev-parse HEAD) \\\n+\t\torigin >out &&\n+\tCOMMON=$(git -C \"$SERVER\" rev-parse two) &&\n+\tgrep \"$COMMON\" out\n+'\n+\n+test_expect_success 'file:// --negotiate-only with protocol v0' '\n+\tSERVER=\"server\" &&\n+\tURI=\"file://$(pwd)/server\" &&\n+\n+\tsetup_negotiate_only \"$SERVER\" \"$URI\" &&\n+\n+\ttest_must_fail git -c protocol.version=0 -C client fetch \\\n+\t\t--no-tags \\\n+\t\t--negotiate-only \\\n+\t\t--negotiation-tip=$(git -C client rev-parse HEAD) \\\n+\t\torigin 2>err &&\n+\ttest_i18ngrep \"negotiate-only requires protocol v2\" err\n+'\n+\n # Test protocol v2 with 'http://' transport\n #\n . \"$TEST_DIRECTORY\"/lib-httpd.sh\n@@ -1035,6 +1078,52 @@ test_expect_success 'packfile-uri with transfer.fsckobjects fails when .gitmodul\n \ttest_i18ngrep \"disallowed submodule name\" err\n '\n \n+test_expect_success 'http:// --negotiate-only' '\n+\tSERVER=\"$HTTPD_DOCUMENT_ROOT_PATH/server\" &&\n+\tURI=\"$HTTPD_URL/smart/server\" &&\n+\n+\tsetup_negotiate_only \"$SERVER\" \"$URI\" &&\n+\n+\tgit -c protocol.version=2 -C client fetch \\\n+\t\t--no-tags \\\n+\t\t--negotiate-only \\\n+\t\t--negotiation-tip=$(git -C client rev-parse HEAD) \\\n+\t\torigin >out &&\n+\tCOMMON=$(git -C \"$SERVER\" rev-parse two) &&\n+\tgrep \"$COMMON\" out\n+'\n+\n+test_expect_success 'http:// --negotiate-only without wait-for-done support' '\n+\tSERVER=\"server\" &&\n+\tURI=\"$HTTPD_URL/one_time_perl/server\" &&\n+\n+\tsetup_negotiate_only \"$SERVER\" \"$URI\" &&\n+\n+\techo \"s/ wait-for-done/ xxxx-xxx-xxxx/\" \\\n+\t\t>\"$HTTPD_ROOT_PATH/one-time-perl\" &&\n+\n+\ttest_must_fail git -c protocol.version=2 -C client fetch \\\n+\t\t--no-tags \\\n+\t\t--negotiate-only \\\n+\t\t--negotiation-tip=$(git -C client rev-parse HEAD) \\\n+\t\torigin 2>err &&\n+\ttest_i18ngrep \"server does not support wait-for-done\" err\n+'\n+\n+test_expect_success 'http:// --negotiate-only with protocol v0' '\n+\tSERVER=\"$HTTPD_DOCUMENT_ROOT_PATH/server\" &&\n+\tURI=\"$HTTPD_URL/smart/server\" &&\n+\n+\tsetup_negotiate_only \"$SERVER\" \"$URI\" &&\n+\n+\ttest_must_fail git -c protocol.version=0 -C client fetch \\\n+\t\t--no-tags \\\n+\t\t--negotiate-only \\\n+\t\t--negotiation-tip=$(git -C client rev-parse HEAD) \\\n+\t\torigin 2>err &&\n+\ttest_i18ngrep \"negotiate-only requires protocol v2\" err\n+'\n+\n # DO NOT add non-httpd-specific tests here, because the last part of this\n # test script is only executed when httpd is available and enabled.\n \ndiff --git a/transport-helper.c b/transport-helper.c\nindex 4cd76366fa..4be035edb8 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -684,6 +684,16 @@ static int fetch(struct transport *transport,\n \t\treturn transport->vtable->fetch(transport, nr_heads, to_fetch);\n \t}\n \n+\t/*\n+\t * If we reach here, then the server, the client, and/or the transport\n+\t * helper does not support protocol v2. --negotiate-only requires\n+\t * protocol v2.\n+\t */\n+\tif (data->transport_options.acked_commits) {\n+\t\twarning(_(\"--negotiate-only requires protocol v2\"));\n+\t\treturn -1;\n+\t}\n+\n \tif (!data->get_refs_list_called)\n \t\tget_refs_list_using_list(transport, 0);\n \ndiff --git a/transport.c b/transport.c\nindex ef66e73090..80caeaa72f 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -392,16 +392,29 @@ static int fetch_refs_via_pack(struct transport *transport,\n \telse if (data->version <= protocol_v1)\n \t\tdie_if_server_options(transport);\n \n+\tif (data->options.acked_commits) {\n+\t\tif (data->version < protocol_v2) {\n+\t\t\twarning(_(\"--negotiate-only requires protocol v2\"));\n+\t\t\tret = -1;\n+\t\t} else if (!server_supports_feature(\"fetch\", \"wait-for-done\", 0)) {\n+\t\t\twarning(_(\"server does not support wait-for-done\"));\n+\t\t\tret = -1;\n+\t\t} else {\n+\t\t\tnegotiate_using_fetch(data->options.negotiation_tips,\n+\t\t\t\t\t transport->server_options,\n+\t\t\t\t\t transport->stateless_rpc,\n+\t\t\t\t\t data->fd,\n+\t\t\t\t\t data->options.acked_commits);\n+\t\t\tret = 0;\n+\t\t}\n+\t\tgoto cleanup;\n+\t}\n+\n \trefs = fetch_pack(&args, data->fd,\n \t\t\t refs_tmp ? refs_tmp : transport->remote_refs,\n \t\t\t to_fetch, nr_heads, &data->shallow,\n \t\t\t &transport->pack_lockfiles, data->version);\n \n-\tclose(data->fd[0]);\n-\tclose(data->fd[1]);\n-\tif (finish_connect(data->conn))\n-\t\tret = -1;\n-\tdata->conn = NULL;\n \tdata->got_remote_heads = 0;\n \tdata->options.self_contained_and_connected =\n \t\targs.self_contained_and_connected;\n@@ -412,6 +425,13 @@ static int fetch_refs_via_pack(struct transport *transport,\n \tif (report_unmatched_refs(to_fetch, nr_heads))\n \t\tret = -1;\n \n+cleanup:\n+\tclose(data->fd[0]);\n+\tclose(data->fd[1]);\n+\tif (finish_connect(data->conn))\n+\t\tret = -1;\n+\tdata->conn = NULL;\n+\n \tfree_refs(refs_tmp);\n \tfree_refs(refs);\n \treturn ret;\ndiff --git a/transport.h b/transport.h\nindex 4d5db0a7f2..1cbab11373 100644\n--- a/transport.h\n+++ b/transport.h\n@@ -47,6 +47,12 @@ struct git_transport_options {\n \t * transport_set_option().\n \t */\n \tstruct oid_array *negotiation_tips;\n+\n+\t/*\n+\t * If allocated, whenever transport_fetch_refs() is called, add known\n+\t * common commits to this oidset instead of fetching any packfiles.\n+\t */\n+\tstruct oidset *acked_commits;\n };\n \n enum transport_family {\ndiff --git a/upload-pack.c b/upload-pack.c\nindex e19583ae0f..b432ef0119 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -103,6 +103,7 @@ struct upload_pack_data {\n \tunsigned use_ofs_delta : 1;\n \tunsigned no_progress : 1;\n \tunsigned use_include_tag : 1;\n+\tunsigned wait_for_done : 1;\n \tunsigned allow_filter : 1;\n \tunsigned allow_filter_fallback : 1;\n \tunsigned long tree_filter_max_depth;\n@@ -1496,6 +1497,10 @@ static void process_args(struct packet_reader *request,\n \t\t\tdata->done = 1;\n \t\t\tcontinue;\n \t\t}\n+\t\tif (!strcmp(arg, \"wait-for-done\")) {\n+\t\t\tdata->wait_for_done = 1;\n+\t\t\tcontinue;\n+\t\t}\n \n \t\t/* Shallow related arguments */\n \t\tif (process_shallow(arg, &data->shallows))\n@@ -1578,7 +1583,7 @@ static int send_acks(struct upload_pack_data *data, struct oid_array *acks)\n \t\t\t\t oid_to_hex(&acks->oid[i]));\n \t}\n \n-\tif (ok_to_give_up(data)) {\n+\tif (!data->wait_for_done && ok_to_give_up(data)) {\n \t\t/* Send Ready */\n \t\tpacket_writer_write(&data->writer, \"ready\\n\");\n \t\treturn 1;\n@@ -1668,10 +1673,13 @@ int upload_pack_v2(struct repository *r, struct strvec *keys,\n \t\tcase FETCH_PROCESS_ARGS:\n \t\t\tprocess_args(request, &data);\n \n-\t\t\tif (!data.want_obj.nr) {\n+\t\t\tif (!data.want_obj.nr && !data.wait_for_done) {\n \t\t\t\t/*\n-\t\t\t\t * Request didn't contain any 'want' lines,\n-\t\t\t\t * guess they didn't want anything.\n+\t\t\t\t * Request didn't contain any 'want' lines (and\n+\t\t\t\t * the request does not contain\n+\t\t\t\t * \"wait-for-done\", in which it is reasonable\n+\t\t\t\t * to just send 'have's without 'want's); guess\n+\t\t\t\t * they didn't want anything.\n \t\t\t\t */\n \t\t\t\tstate = FETCH_DONE;\n \t\t\t} else if (data.haves.nr) {\n@@ -1723,7 +1731,7 @@ int upload_pack_advertise(struct repository *r,\n \t\tint allow_sideband_all_value;\n \t\tchar *str = NULL;\n \n-\t\tstrbuf_addstr(value, \"shallow\");\n+\t\tstrbuf_addstr(value, \"shallow wait-for-done\");\n \n \t\tif (!repo_config_get_bool(the_repository,\n \t\t\t\t\t \"uploadpack.allowfilter\",\n-- \n2.31.1.527.g47e6f16901-goog\n\n"},{"id":"423617","messageId":"d38a8b7d66efee74e327c4a7f4322e2c02a7495d.1620162764.git.jonathantanmy@google.com","threadId":"55459","inReplyTo":"cover.1620162764.git.jonathantanmy@google.com","subject":"[PATCH v2 5/5] send-pack: support push negotiation","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-05-04T21:16:02Z","receivedAt":"2021-05-04T21:16:21Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Teach Git the push.negotiate config variable.\n\nSigned-off-by: Jonathan Tan \nSigned-off-by: Junio C Hamano \n---\n Documentation/config/push.txt | 7 ++++\n send-pack.c | 61 ++++++++++++++++++++++++++++++++---\n t/t5516-fetch-push.sh | 35 ++++++++++++++++++++\n 3 files changed, 99 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/config/push.txt b/Documentation/config/push.txt\nindex 21b256e0a4..f2667b2689 100644\n--- a/Documentation/config/push.txt\n+++ b/Documentation/config/push.txt\n@@ -120,3 +120,10 @@ push.useForceIfIncludes::\n \t`--force-if-includes` as an option to linkgit:git-push[1]\n \tin the command line. Adding `--no-force-if-includes` at the\n \ttime of push overrides this configuration setting.\n+\n+push.negotiate::\n+\tIf set to \"true\", attempt to reduce the size of the packfile\n+\tsent by rounds of negotiation in which the client and the\n+\tserver attempt to find commits in common. If \"false\", Git will\n+\trely solely on the server's ref advertisement to find commits\n+\tin common.\ndiff --git a/send-pack.c b/send-pack.c\nindex 5f215b13c7..9cb9f71650 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -56,7 +56,9 @@ static void feed_object(const struct object_id *oid, FILE *fh, int negative)\n /*\n * Make a pack stream and spit it out into file descriptor fd\n */\n-static int pack_objects(int fd, struct ref *refs, struct oid_array *extra, struct send_pack_args *args)\n+static int pack_objects(int fd, struct ref *refs, struct oid_array *advertised,\n+\t\t\tstruct oid_array *negotiated,\n+\t\t\tstruct send_pack_args *args)\n {\n \t/*\n \t * The child becomes pack-objects --revs; we feed\n@@ -94,8 +96,10 @@ static int pack_objects(int fd, struct ref *refs, struct oid_array *extra, struc\n \t * parameters by writing to the pipe.\n \t */\n \tpo_in = xfdopen(po.in, \"w\");\n-\tfor (i = 0; i < extra->nr; i++)\n-\t\tfeed_object(&extra->oid[i], po_in, 1);\n+\tfor (i = 0; i < advertised->nr; i++)\n+\t\tfeed_object(&advertised->oid[i], po_in, 1);\n+\tfor (i = 0; i < negotiated->nr; i++)\n+\t\tfeed_object(&negotiated->oid[i], po_in, 1);\n \n \twhile (refs) {\n \t\tif (!is_null_oid(&refs->old_oid))\n@@ -409,11 +413,55 @@ static void reject_invalid_nonce(const char *nonce, int len)\n \t}\n }\n \n+static void get_commons_through_negotiation(const char *url,\n+\t\t\t\t\t const struct ref *remote_refs,\n+\t\t\t\t\t struct oid_array *commons)\n+{\n+\tstruct child_process child = CHILD_PROCESS_INIT;\n+\tconst struct ref *ref;\n+\tint len = the_hash_algo->hexsz + 1; /* hash + NL */\n+\n+\tchild.git_cmd = 1;\n+\tchild.no_stdin = 1;\n+\tchild.out = -1;\n+\tstrvec_pushl(&child.args, \"fetch\", \"--negotiate-only\", NULL);\n+\tfor (ref = remote_refs; ref; ref = ref->next)\n+\t\tstrvec_pushf(&child.args, \"--negotiation-tip=%s\", oid_to_hex(&ref->new_oid));\n+\tstrvec_push(&child.args, url);\n+\n+\tif (start_command(&child))\n+\t\tdie(_(\"send-pack: unable to fork off fetch subprocess\"));\n+\n+\tdo {\n+\t\tchar hex_hash[GIT_MAX_HEXSZ + 1];\n+\t\tint read_len = read_in_full(child.out, hex_hash, len);\n+\t\tstruct object_id oid;\n+\t\tconst char *end;\n+\n+\t\tif (!read_len)\n+\t\t\tbreak;\n+\t\tif (read_len != len)\n+\t\t\tdie(\"invalid length read %d\", read_len);\n+\t\tif (parse_oid_hex(hex_hash, &oid, &end) || *end != '\\n')\n+\t\t\tdie(\"invalid hash\");\n+\t\toid_array_append(commons, &oid);\n+\t} while (1);\n+\n+\tif (finish_command(&child)) {\n+\t\t/*\n+\t\t * The information that push negotiation provides is useful but\n+\t\t * not mandatory.\n+\t\t */\n+\t\twarning(_(\"push negotiation failed; proceeding anyway with push\"));\n+\t}\n+}\n+\n int send_pack(struct send_pack_args *args,\n \t int fd[], struct child_process *conn,\n \t struct ref *remote_refs,\n \t struct oid_array *extra_have)\n {\n+\tstruct oid_array commons = OID_ARRAY_INIT;\n \tint in = fd[0];\n \tint out = fd[1];\n \tstruct strbuf req_buf = STRBUF_INIT;\n@@ -426,6 +474,7 @@ int send_pack(struct send_pack_args *args,\n \tint quiet_supported = 0;\n \tint agent_supported = 0;\n \tint advertise_sid = 0;\n+\tint push_negotiate = 0;\n \tint use_atomic = 0;\n \tint atomic_supported = 0;\n \tint use_push_options = 0;\n@@ -437,6 +486,10 @@ int send_pack(struct send_pack_args *args,\n \tconst char *push_cert_nonce = NULL;\n \tstruct packet_reader reader;\n \n+\tgit_config_get_bool(\"push.negotiate\", &push_negotiate);\n+\tif (push_negotiate)\n+\t\tget_commons_through_negotiation(args->url, remote_refs, &commons);\n+\n \tgit_config_get_bool(\"transfer.advertisesid\", &advertise_sid);\n \n \t/* Does the other end support the reporting? */\n@@ -625,7 +678,7 @@ int send_pack(struct send_pack_args *args,\n \t\t\t PACKET_READ_DIE_ON_ERR_PACKET);\n \n \tif (need_pack_data && cmds_sent) {\n-\t\tif (pack_objects(out, remote_refs, extra_have, args) < 0) {\n+\t\tif (pack_objects(out, remote_refs, extra_have, &commons, args) < 0) {\n \t\t\tif (args->stateless_rpc)\n \t\t\t\tclose(out);\n \t\t\tif (git_connection_is_socket(conn))\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex f11742ed59..0916f76302 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -191,6 +191,41 @@ test_expect_success 'fetch with pushInsteadOf (should not rewrite)' '\n \t)\n '\n \n+grep_wrote () {\n+\tobject_count=$1\n+\tfile_name=$2\n+\tgrep 'write_pack_file/wrote.*\"value\":\"'$1'\"' $2\n+}\n+\n+test_expect_success 'push with negotiation' '\n+\t# Without negotiation\n+\tmk_empty testrepo &&\n+\tgit push testrepo $the_first_commit:refs/remotes/origin/first_commit &&\n+\tgit -C testrepo config receive.hideRefs refs/remotes/origin/first_commit &&\n+\techo now pushing without negotiation &&\n+\tGIT_TRACE2_EVENT=\"$(pwd)/event\" git -c protocol.version=2 push testrepo refs/heads/main:refs/remotes/origin/main &&\n+\tgrep_wrote 5 event && # 2 commits, 2 trees, 1 blob\n+\n+\t# Same commands, but with negotiation\n+\trm event &&\n+\tmk_empty testrepo &&\n+\tgit push testrepo $the_first_commit:refs/remotes/origin/first_commit &&\n+\tgit -C testrepo config receive.hideRefs refs/remotes/origin/first_commit &&\n+\tGIT_TRACE2_EVENT=\"$(pwd)/event\" git -c protocol.version=2 -c push.negotiate=1 push testrepo refs/heads/main:refs/remotes/origin/main &&\n+\tgrep_wrote 2 event # 1 commit, 1 tree\n+'\n+\n+test_expect_success 'push with negotiation proceeds anyway even if negotiation fails' '\n+\trm event &&\n+\tmk_empty testrepo &&\n+\tgit push testrepo $the_first_commit:refs/remotes/origin/first_commit &&\n+\tgit -C testrepo config receive.hideRefs refs/remotes/origin/first_commit &&\n+\tGIT_TEST_PROTOCOL_VERSION=0 GIT_TRACE2_EVENT=\"$(pwd)/event\" \\\n+\t\tgit -c push.negotiate=1 push testrepo refs/heads/main:refs/remotes/origin/main 2>err &&\n+\tgrep_wrote 5 event && # 2 commits, 2 trees, 1 blob\n+\ttest_i18ngrep \"push negotiation failed\" err\n+'\n+\n test_expect_success 'push without wildcard' '\n \tmk_empty testrepo &&\n \n-- \n2.31.1.527.g47e6f16901-goog\n\n"},{"id":"423632","messageId":"xmqq4kfi54kb.fsf@gitster.g","threadId":"55459","inReplyTo":"1de34a6dced3f5477162dd675615342bc2dec05f.1620162764.git.jonathantanmy@google.com","subject":"Re: [PATCH v2 4/5] fetch: teach independent negotiation (no packfile)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-05-05T01:53:40Z","receivedAt":"2021-05-05T01:53:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> +static int add_to_object_array(const struct object_id *oid, void *data)\n> +{\n> +\tstruct object_array *a = data;\n> +\n> +\tadd_object_array(lookup_object(the_repository, oid), \"\", a);\n\nMoving to lookup_object() made me look around, but at this point the\nobject in question (which comes from the negotiation_tips) has been\ninstantiated, so it is OK.\n\n Side note. The big difference between lookup and parse is what\n happens when this process hasn't seen the object yet---the\n former will just return NULL instead of instantiating the\n in-core copy; for that reason, it is easier on the readers to\n use parse_object() if we know we want an in-core object *and*\n when we use it we want to see it parsed already).\n\n> +static void clear_common_flag(struct oidset *s)\n> +{\n> +\tstruct oidset_iter iter;\n> +\tconst struct object_id *oid;\n> +\toidset_iter_init(s, &iter);\n> +\n> +\twhile ((oid = oidset_iter_next(&iter))) {\n> +\t\tstruct object *obj = lookup_object(the_repository, oid);\n\nThis one obviously is OK ;-) The fact we are clearing by definition\nmeans we already do have in-core objects.\n\n> +\t\tobj->flags &= ~COMMON;\n> +\t}\n> +}\n\nThanks.\n"},{"id":"423697","messageId":"9f650594-c8a6-4d89-2686-49de48aabb62@gmail.com","threadId":"55459","inReplyTo":"xmqq4kfi54kb.fsf@gitster.g","subject":"Re: [PATCH v2 4/5] fetch: teach independent negotiation (no packfile)","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-05-05T16:42:09Z","receivedAt":"2021-05-05T17:27:49Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 5/4/2021 9:53 PM, Junio C Hamano wrote:\n> Jonathan Tan writes:\n> \n>> +static int add_to_object_array(const struct object_id *oid, void *data)\n>> +{\n>> +\tstruct object_array *a = data;\n>> +\n>> +\tadd_object_array(lookup_object(the_repository, oid), \"\", a);\n> \n> Moving to lookup_object() made me look around, but at this point the\n> object in question (which comes from the negotiation_tips) has been\n> instantiated, so it is OK.\n> \n> Side note. The big difference between lookup and parse is what\n> happens when this process hasn't seen the object yet---the\n> former will just return NULL instead of instantiating the\n> in-core copy; for that reason, it is easier on the readers to\n> use parse_object() if we know we want an in-core object *and*\n> when we use it we want to see it parsed already).\n\nPlease forgive my incorrect recommendation here. I was expecting\nlookup_object() to behave like lookup_commit(), which creates the\nobject if it is not already in the cached set.\n\n>> +static void clear_common_flag(struct oidset *s)\n>> +{\n>> +\tstruct oidset_iter iter;\n>> +\tconst struct object_id *oid;\n>> +\toidset_iter_init(s, &iter);\n>> +\n>> +\twhile ((oid = oidset_iter_next(&iter))) {\n>> +\t\tstruct object *obj = lookup_object(the_repository, oid);\n> \n> This one obviously is OK ;-) The fact we are clearing by definition\n> means we already do have in-core objects.\n\nThanks for your careful eye here.\n\n-Stolee\n"},{"id":"423698","messageId":"f2acf884-8930-a3fd-ec4d-601652b2ec24@gmail.com","threadId":"55459","inReplyTo":"1de34a6dced3f5477162dd675615342bc2dec05f.1620162764.git.jonathantanmy@google.com","subject":"Re: [PATCH v2 4/5] fetch: teach independent negotiation (no packfile)","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-05-05T16:44:42Z","receivedAt":"2021-05-05T17:27:50Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 5/4/2021 5:16 PM, Jonathan Tan wrote:\n\n> +void negotiate_using_fetch(const struct oid_array *negotiation_tips,\n> +\t\t\t const struct string_list *server_options,\n> +\t\t\t int stateless_rpc,\n> +\t\t\t int fd[],\n> +\t\t\t struct oidset *acked_commits)\n> +{\n> +\tstruct fetch_negotiator negotiator;\n> +\tstruct packet_reader reader;\n> +\tstruct object_array nt_object_array = OBJECT_ARRAY_INIT;\n> +\tstruct strbuf req_buf = STRBUF_INIT;\n> +\tint haves_to_send = INITIAL_FLUSH;\n> +\tint in_vain = 0;\n> +\tint seen_ack = 0;\n> +\tint last_iteration = 0;\n> +\ttimestamp_t min_generation = GENERATION_NUMBER_INFINITY;\n\n...\n\n> +\t\t/* Process ACKs/NAKs */\n> +\t\tprocess_section_header(&reader, \"acknowledgments\", 0);\n> +\t\twhile (process_ack(&negotiator, &reader, &common_oid,\n> +\t\t\t\t &received_ready)) {\n> +\t\t\tstruct commit *commit = lookup_commit(the_repository,\n> +\t\t\t\t\t\t\t &common_oid);\n> +\t\t\tif (commit) {\n> +\t\t\t\ttimestamp_t generation;\n> +\n> +\t\t\t\tparse_commit_or_die(commit);\n> +\t\t\t\tcommit->object.flags |= COMMON;\n> +\t\t\t\tgeneration = commit_graph_generation(commit);\n> +\t\t\t\tif (generation < min_generation)\n> +\t\t\t\t\tmin_generation = generation;\n> +\t\t\t}\n> +\t\t\tin_vain = 0;\n> +\t\t\tseen_ack = 1;\n> +\t\t\toidset_insert(acked_commits, &common_oid);\n> +\t\t}\n> +\t\tif (received_ready)\n> +\t\t\tdie(_(\"unexpected 'ready' from remote\"));\n> +\t\telse\n> +\t\t\tdo_check_stateless_delimiter(stateless_rpc, &reader);\n> +\t\tif (can_all_from_reach_with_flag(&nt_object_array, COMMON,\n> +\t\t\t\t\t\t REACH_SCRATCH, 0,\n> +\t\t\t\t\t\t min_generation))\n> +\t\t\tlast_iteration = 1;\n\nI'm just chiming in to confirm that this use of min_generation is\ncorrect. Thanks!\n\n-Stolee\n"},{"id":"423732","messageId":"xmqqbl9o1ugy.fsf@gitster.g","threadId":"55459","inReplyTo":"9f650594-c8a6-4d89-2686-49de48aabb62@gmail.com","subject":"Re: [PATCH v2 4/5] fetch: teach independent negotiation (no packfile)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-05-06T02:12:13Z","receivedAt":"2021-05-06T02:12:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee writes:\n\n>> Moving to lookup_object() made me look around, but at this point the\n>> object in question (which comes from the negotiation_tips) has been\n>> instantiated, so it is OK.\n>> \n>> Side note. The big difference between lookup and parse is what\n>> happens when this process hasn't seen the object yet---the\n>> former will just return NULL instead of instantiating the\n>> in-core copy; for that reason, it is easier on the readers to\n>> use parse_object() if we know we want an in-core object *and*\n>> when we use it we want to see it parsed already).\n>\n> Please forgive my incorrect recommendation here. I was expecting\n> lookup_object() to behave like lookup_commit(), which creates the\n> object if it is not already in the cached set.\n\nIt is not wrong per-se to recommend or use lookup_object() in this\ncase, as long as who recommends or uses the function is certain that\nthe object should already exist in-core.\n\nBut it is just that the lookup_object() is often not what the caller\nwants to use; it is a very rare use case for a caller to be able to\nsay \"I am OK for you to return NULL to me for this object, even if\nit is in our object store, when we do not yet have it in-core\". The\nonly exception is the implementation of lookup_() helpers,\nwhere they handle \"not loaded yet\" answer from lookup_object() with\ncreate_object() and always return usable in-core object to the\ncaller.\n\nSo the sole practical use case for lookup_object() outside the\nlookup_() implementation becomes \"I am 100% sure that an\nobject with this object name has already been loaded in-core, so\nplease let me access it\", which may be optimized for a wrong case\n(because most of the API pass in-core objects, not object names,\naround). I am not sure if this particular API strangeness can be\nremedied, though.\n\nThanks.\n"}]}