{"thread":{"id":"54560","subject":"[PATCH 0/4] \"Push\" protocol change proposal: user-specified base","startedAt":"2020-11-03T00:26:20Z","lastAt":"2020-11-09T22:47:37Z","messageCount":21,"participants":["Jonathan Tan","Junio C Hamano","Jonathan Nieder","SZEDER Gábor","Derrick Stolee","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"408963","messageId":"cover.1604362701.git.jonathantanmy@google.com","threadId":"54560","inReplyTo":null,"subject":"[PATCH 0/4] \"Push\" protocol change proposal: user-specified base","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-11-03T00:26:09Z","receivedAt":"2020-11-03T00:26:20Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"At $DAY_JOB, we've noticed some pushes being larger than necessary.\nHere's a first step towards solving that. In this protocol change, we're\nallowing the user to specify an ancestor believed to be common; if the\nserver agrees, the server sends that ancestor as the lone so-called ref\nbeing advertised instead of the full ref advertisement. Not only does\nthis save bandwidth for the ref advertisement, but this avoids the issue\nof the server's refs being ahead of the client (and thus the client\ncannot use them since it is unaware of those objects).\n\nMore information (including a design doc) can be found in the\ndocumentation patch.\n\nThis works for file://, ssh://, and http(s)://.\n\nOne last concern I had is the locations of the tests - I just bundled\nthe protocol v0 ones with their corresponding protocol v1 ones, as I\ncouldn't find good places to put them.\n\nJonathan Tan (4):\n connect: refactor building of Extra Parameters\n push: teach --base for ssh:// and file://\n remote-curl: teach --base for http(s)://\n Doc: push with --base\n\n Documentation/gitremote-helpers.txt | 8 ++\n Documentation/technical/pack-protocol.txt | 10 +-\n Documentation/technical/push-with-base.txt | 61 ++++++++++++\n builtin/fetch-pack.c | 2 +-\n builtin/push.c | 12 ++-\n builtin/receive-pack.c | 46 ++++++++-\n builtin/send-pack.c | 2 +-\n builtin/upload-pack.c | 2 +-\n connect.c | 39 +++++---\n connect.h | 4 +-\n http-backend.c | 2 +-\n protocol.c | 5 +-\n protocol.h | 5 +-\n remote-curl.c | 25 +++--\n t/t5700-protocol-v1.sh | 110 +++++++++++++++++++++\n transport-helper.c | 15 +++\n transport.c | 14 ++-\n transport.h | 9 ++\n 18 files changed, 332 insertions(+), 39 deletions(-)\n create mode 100644 Documentation/technical/push-with-base.txt\n\n-- \n2.29.1.341.ge80a0c044ae-goog\n\n"},{"id":"408964","messageId":"7893cfba05a663370f353cdf60aa1df24b386c04.1604362701.git.jonathantanmy@google.com","threadId":"54560","inReplyTo":"cover.1604362701.git.jonathantanmy@google.com","subject":"[PATCH 1/4] connect: refactor building of Extra Parameters","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-11-03T00:26:10Z","receivedAt":"2020-11-03T00:27:12Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Build the Extra Parameters string (\"version=%d\") in only one place\n(git_connect()) instead of two (git_connect() and push_ssh_options()).\nThis is in preparation for a subsequent patch, which will change how the\nExtra Parameters are built.\n\nSigned-off-by: Jonathan Tan \n---\n connect.c | 31 ++++++++++++++++++-------------\n 1 file changed, 18 insertions(+), 13 deletions(-)\n\ndiff --git a/connect.c b/connect.c\nindex 8b8f56cf6d..5221f1764c 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -1201,14 +1201,13 @@ static struct child_process *git_connect_git(int fd[2], char *hostandport,\n */\n static void push_ssh_options(struct strvec *args, struct strvec *env,\n \t\t\t enum ssh_variant variant, const char *port,\n-\t\t\t enum protocol_version version, int flags)\n+\t\t\t const char *extra_parameters, int flags)\n {\n-\tif (variant == VARIANT_SSH &&\n-\t version > 0) {\n+\tif (variant == VARIANT_SSH && extra_parameters) {\n \t\tstrvec_push(args, \"-o\");\n \t\tstrvec_push(args, \"SendEnv=\" GIT_PROTOCOL_ENVIRONMENT);\n-\t\tstrvec_pushf(env, GIT_PROTOCOL_ENVIRONMENT \"=version=%d\",\n-\t\t\t version);\n+\t\tstrvec_pushf(env, GIT_PROTOCOL_ENVIRONMENT \"=%s\",\n+\t\t\t extra_parameters);\n \t}\n \n \tif (flags & CONNECT_IPV4) {\n@@ -1261,7 +1260,7 @@ static void push_ssh_options(struct strvec *args, struct strvec *env,\n \n /* Prepare a child_process for use by Git's SSH-tunneled transport. */\n static void fill_ssh_args(struct child_process *conn, const char *ssh_host,\n-\t\t\t const char *port, enum protocol_version version,\n+\t\t\t const char *port, const char *extra_parameters,\n \t\t\t int flags)\n {\n \tconst char *ssh;\n@@ -1296,14 +1295,14 @@ static void fill_ssh_args(struct child_process *conn, const char *ssh_host,\n \t\tstrvec_push(&detect.args, ssh);\n \t\tstrvec_push(&detect.args, \"-G\");\n \t\tpush_ssh_options(&detect.args, &detect.env_array,\n-\t\t\t\t VARIANT_SSH, port, version, flags);\n+\t\t\t\t VARIANT_SSH, port, extra_parameters, flags);\n \t\tstrvec_push(&detect.args, ssh_host);\n \n \t\tvariant = run_command(&detect) ? VARIANT_SIMPLE : VARIANT_SSH;\n \t}\n \n \tstrvec_push(&conn->args, ssh);\n-\tpush_ssh_options(&conn->args, &conn->env_array, variant, port, version, flags);\n+\tpush_ssh_options(&conn->args, &conn->env_array, variant, port, extra_parameters, flags);\n \tstrvec_push(&conn->args, ssh_host);\n }\n \n@@ -1352,6 +1351,7 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t} else {\n \t\tstruct strbuf cmd = STRBUF_INIT;\n \t\tconst char *const *var;\n+\t\tstruct strbuf extra_parameters = STRBUF_INIT;\n \n \t\tconn = xmalloc(sizeof(*conn));\n \t\tchild_process_init(conn);\n@@ -1367,6 +1367,9 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t\tfor (var = local_repo_env; *var; var++)\n \t\t\tstrvec_push(&conn->env_array, *var);\n \n+\t\tif (version > 0)\n+\t\t\tstrbuf_addf(&extra_parameters, \"version=%d\", version);\n+\n \t\tconn->use_shell = 1;\n \t\tconn->in = conn->out = -1;\n \t\tif (protocol == PROTO_SSH) {\n@@ -1392,15 +1395,16 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t\t\t\treturn NULL;\n \t\t\t}\n \t\t\tconn->trace2_child_class = \"transport/ssh\";\n-\t\t\tfill_ssh_args(conn, ssh_host, port, version, flags);\n+\t\t\tfill_ssh_args(conn, ssh_host, port,\n+\t\t\t\t extra_parameters.len ? extra_parameters.buf : NULL,\n+\t\t\t\t flags);\n \t\t} else {\n \t\t\ttransport_check_allowed(\"file\");\n \t\t\tconn->trace2_child_class = \"transport/file\";\n-\t\t\tif (version > 0) {\n+\t\t\tif (extra_parameters.len)\n \t\t\t\tstrvec_pushf(&conn->env_array,\n-\t\t\t\t\t GIT_PROTOCOL_ENVIRONMENT \"=version=%d\",\n-\t\t\t\t\t version);\n-\t\t\t}\n+\t\t\t\t\t GIT_PROTOCOL_ENVIRONMENT \"=%s\",\n+\t\t\t\t\t extra_parameters.buf);\n \t\t}\n \t\tstrvec_push(&conn->args, cmd.buf);\n \n@@ -1409,6 +1413,7 @@ struct child_process *git_connect(int fd[2], const char *url,\n \n \t\tfd[0] = conn->out; /* read from child's stdout */\n \t\tfd[1] = conn->in; /* write to child's stdin */\n+\t\tstrbuf_release(&extra_parameters);\n \t\tstrbuf_release(&cmd);\n \t}\n \tfree(hostandport);\n-- \n2.29.1.341.ge80a0c044ae-goog\n\n"},{"id":"408965","messageId":"148e39960a2185d2355cdfe34f8856e708fb1b80.1604362701.git.jonathantanmy@google.com","threadId":"54560","inReplyTo":"cover.1604362701.git.jonathantanmy@google.com","subject":"[PATCH 2/4] push: teach --base for ssh:// and file://","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-11-03T00:26:11Z","receivedAt":"2020-11-03T00:27:14Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"For the ssh:// and file:// transports, teach push the \"--base\"\nparameter, which indicates an ancestor of the commits to be pushed that\nis believed to be known by the server.\n\nIf specified, it will be communicated to receive-pack through an Extra\nParameter. If the repository served by receive-pack indeed has that\nobject, receive-pack will send an abbreviated ref advertisement\ncontaining only that object instead of the usual ref advertisement,\nreducing the size of the ref advertisement and, potentially, the size of\nthe packfile to be sent.\n\nThis is supported both on protocol v0 and v1. (Protocol v1 is the same\nas protocol v0, except that the client sends \"version=1\" as an Extra\nParameter and the server responds with \"version 1\".)\n\nSupport for http(s):// will be added in a subsequent patch.\n\nSigned-off-by: Jonathan Tan \n---\n builtin/fetch-pack.c | 2 +-\n builtin/push.c | 12 +++++--\n builtin/receive-pack.c | 46 +++++++++++++++++++++---\n builtin/send-pack.c | 2 +-\n builtin/upload-pack.c | 2 +-\n connect.c | 8 ++++-\n connect.h | 4 ++-\n http-backend.c | 2 +-\n protocol.c | 5 ++-\n protocol.h | 5 ++-\n t/t5700-protocol-v1.sh | 80 ++++++++++++++++++++++++++++++++++++++++++\n transport.c | 14 ++++++--\n transport.h | 9 +++++\n 13 files changed, 175 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex 58b7c1fbdc..9452969319 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -207,7 +207,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n \t\tif (args.diag_url)\n \t\t\tflags |= CONNECT_DIAG_URL;\n \t\tconn = git_connect(fd, dest, args.uploadpack,\n-\t\t\t\t flags);\n+\t\t\t\t flags, NULL);\n \t\tif (!conn)\n \t\t\treturn args.diag_url ? 0 : 1;\n \t}\ndiff --git a/builtin/push.c b/builtin/push.c\nindex 6da3a8e5d3..a9f3278a24 100644\n--- a/builtin/push.c\n+++ b/builtin/push.c\n@@ -381,7 +381,8 @@ static int push_with_options(struct transport *transport, struct refspec *rs,\n \n static int do_push(int flags,\n \t\t const struct string_list *push_options,\n-\t\t struct remote *remote)\n+\t\t struct remote *remote,\n+\t\t const char *push_base)\n {\n \tint i, errs;\n \tconst char **url;\n@@ -405,6 +406,8 @@ static int do_push(int flags,\n \t\t\t\ttransport_get(remote, url[i]);\n \t\t\tif (flags & TRANSPORT_PUSH_OPTIONS)\n \t\t\t\ttransport->push_options = push_options;\n+\t\t\tif (push_base)\n+\t\t\t\ttransport_set_option(transport, TRANS_OPT_PUSH_BASE, push_base);\n \t\t\tif (push_with_options(transport, push_refspec, flags))\n \t\t\t\terrs++;\n \t\t}\n@@ -413,6 +416,8 @@ static int do_push(int flags,\n \t\t\ttransport_get(remote, NULL);\n \t\tif (flags & TRANSPORT_PUSH_OPTIONS)\n \t\t\ttransport->push_options = push_options;\n+\t\tif (push_base)\n+\t\t\ttransport_set_option(transport, TRANS_OPT_PUSH_BASE, push_base);\n \t\tif (push_with_options(transport, push_refspec, flags))\n \t\t\terrs++;\n \t}\n@@ -526,6 +531,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)\n \tstruct string_list *push_options;\n \tconst struct string_list_item *item;\n \tstruct remote *remote;\n+\tconst char *push_base = NULL;\n \n \tstruct option options[] = {\n \t\tOPT__VERBOSITY(&verbosity),\n@@ -562,6 +568,8 @@ int cmd_push(int argc, const char **argv, const char *prefix)\n \t\t\t\tTRANSPORT_FAMILY_IPV4),\n \t\tOPT_SET_INT('6', \"ipv6\", &family, N_(\"use IPv6 addresses only\"),\n \t\t\t\tTRANSPORT_FAMILY_IPV6),\n+\t\tOPT_STRING(0, \"base\", &push_base, N_(\"revision\"),\n+\t\t\t N_(\"ancestor of commits to be pushed that is believed to be known by the server\")),\n \t\tOPT_END()\n \t};\n \n@@ -629,7 +637,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)\n \t\tif (strchr(item->string, '\\n'))\n \t\t\tdie(_(\"push options must not have new line characters\"));\n \n-\trc = do_push(flags, push_options, remote);\n+\trc = do_push(flags, push_options, remote, push_base);\n \tstring_list_clear(&push_options_cmdline, 0);\n \tstring_list_clear(&push_options_config, 0);\n \tif (rc == -1)\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex bb9909c52e..274f39c944 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -313,13 +313,38 @@ static void show_one_alternate_ref(const struct object_id *oid,\n \tshow_ref(\".have\", oid);\n }\n \n-static void write_head_info(void)\n+struct check_connected_for_one_args {\n+\tstruct object_id *base;\n+\tunsigned already_emitted : 1;\n+};\n+static int check_connected_for_one(void *args, struct object_id *oid)\n+{\n+\tstruct check_connected_for_one_args *a = args;\n+\n+\tif (a->already_emitted)\n+\t\treturn -1;\n+\toidcpy(oid, a->base);\n+\ta->already_emitted = 1;\n+\treturn 0;\n+}\n+\n+static void write_head_info(struct object_id *base)\n {\n \tstatic struct oidset seen = OIDSET_INIT;\n \n+\tif (base) {\n+\t\tstruct check_connected_for_one_args args = {base};\n+\n+\t\tif (!check_connected(check_connected_for_one, &args, NULL)) {\n+\t\t\tshow_ref(\".have\", base);\n+\t\t\tgoto refs_shown;\n+\t\t}\n+\t}\n+\n \tfor_each_ref(show_ref_cb, &seen);\n \tfor_each_alternate_ref(show_one_alternate_ref, &seen);\n \toidset_clear(&seen);\n+refs_shown:\n \tif (!sent_capabilities)\n \t\tshow_ref(\"capabilities^{}\", &null_oid);\n \n@@ -2417,6 +2442,7 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)\n \tstruct oid_array ref = OID_ARRAY_INIT;\n \tstruct shallow_info si;\n \tstruct packet_reader reader;\n+\tstruct strbuf base_sb = STRBUF_INIT;\n \n \tstruct option options[] = {\n \t\tOPT__QUIET(&quiet, N_(\"quiet\")),\n@@ -2451,7 +2477,7 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)\n \telse if (0 <= receive_unpack_limit)\n \t\tunpack_limit = receive_unpack_limit;\n \n-\tswitch (determine_protocol_version_server()) {\n+\tswitch (determine_protocol_version_server(&base_sb)) {\n \tcase protocol_v2:\n \t\t/*\n \t\t * push support for protocol v2 has not been implemented yet,\n@@ -2474,10 +2500,21 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)\n \t}\n \n \tif (advertise_refs || !stateless_rpc) {\n-\t\twrite_head_info();\n+\t\tif (base_sb.len) {\n+\t\t\tstruct object_id oid;\n+\t\t\tconst char *p;\n+\n+\t\t\tif (parse_oid_hex(base_sb.buf, &oid, &p) || *p != '\\0')\n+\t\t\t\tdie(\"invalid base\");\n+\t\t\twrite_head_info(&oid);\n+\t\t} else {\n+\t\t\twrite_head_info(NULL);\n+\t\t}\n \t}\n-\tif (advertise_refs)\n+\tif (advertise_refs) {\n+\t\tstrbuf_release(&base_sb);\n \t\treturn 0;\n+\t}\n \n \tpacket_reader_init(&reader, 0, NULL, 0,\n \t\t\t PACKET_READ_CHOMP_NEWLINE |\n@@ -2540,6 +2577,7 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)\n \t}\n \tif (use_sideband)\n \t\tpacket_flush(1);\n+\tstrbuf_release(&base_sb);\n \toid_array_clear(&shallow);\n \toid_array_clear(&ref);\n \tfree((void *)push_cert_nonce);\ndiff --git a/builtin/send-pack.c b/builtin/send-pack.c\nindex 7af148d733..1557db7a42 100644\n--- a/builtin/send-pack.c\n+++ b/builtin/send-pack.c\n@@ -262,7 +262,7 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)\n \t\tfd[1] = 1;\n \t} else {\n \t\tconn = git_connect(fd, dest, receivepack,\n-\t\t\targs.verbose ? CONNECT_VERBOSE : 0);\n+\t\t\targs.verbose ? CONNECT_VERBOSE : 0, NULL);\n \t}\n \n \tpacket_reader_init(&reader, fd[0], NULL, 0,\ndiff --git a/builtin/upload-pack.c b/builtin/upload-pack.c\nindex 6da8fa2607..32aaa7742c 100644\n--- a/builtin/upload-pack.c\n+++ b/builtin/upload-pack.c\n@@ -48,7 +48,7 @@ int cmd_upload_pack(int argc, const char **argv, const char *prefix)\n \tif (!enter_repo(dir, strict))\n \t\tdie(\"'%s' does not appear to be a git repository\", dir);\n \n-\tswitch (determine_protocol_version_server()) {\n+\tswitch (determine_protocol_version_server(NULL)) {\n \tcase protocol_v2:\n \t\tserve_opts.advertise_capabilities = opts.advertise_refs;\n \t\tserve_opts.stateless_rpc = opts.stateless_rpc;\ndiff --git a/connect.c b/connect.c\nindex 5221f1764c..fa5b7ea886 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -1318,7 +1318,8 @@ static void fill_ssh_args(struct child_process *conn, const char *ssh_host,\n * the connection failed).\n */\n struct child_process *git_connect(int fd[2], const char *url,\n-\t\t\t\t const char *prog, int flags)\n+\t\t\t\t const char *prog, int flags,\n+\t\t\t\t const char *other_extra_parameters)\n {\n \tchar *hostandport, *path;\n \tstruct child_process *conn;\n@@ -1369,6 +1370,11 @@ struct child_process *git_connect(int fd[2], const char *url,\n \n \t\tif (version > 0)\n \t\t\tstrbuf_addf(&extra_parameters, \"version=%d\", version);\n+\t\tif (other_extra_parameters) {\n+\t\t\tif (extra_parameters.len)\n+\t\t\t\tstrbuf_addch(&extra_parameters, ':');\n+\t\t\tstrbuf_addstr(&extra_parameters, other_extra_parameters);\n+\t\t}\n \n \t\tconn->use_shell = 1;\n \t\tconn->in = conn->out = -1;\ndiff --git a/connect.h b/connect.h\nindex c53586e929..6d502d4224 100644\n--- a/connect.h\n+++ b/connect.h\n@@ -7,7 +7,9 @@\n #define CONNECT_DIAG_URL (1u << 1)\n #define CONNECT_IPV4 (1u << 2)\n #define CONNECT_IPV6 (1u << 3)\n-struct child_process *git_connect(int fd[2], const char *url, const char *prog, int flags);\n+struct child_process *git_connect(int fd[2], const char *url,\n+\t\t\t\t const char *prog, int flags,\n+\t\t\t\t const char *other_extra_parameters);\n int finish_connect(struct child_process *conn);\n int git_connection_is_socket(struct child_process *conn);\n int server_supports(const char *feature);\ndiff --git a/http-backend.c b/http-backend.c\nindex a03b4bae22..7cd684c8b5 100644\n--- a/http-backend.c\n+++ b/http-backend.c\n@@ -544,7 +544,7 @@ static void get_info_refs(struct strbuf *hdr, char *arg)\n \t\tend_headers(hdr);\n \n \n-\t\tif (determine_protocol_version_server() != protocol_v2) {\n+\t\tif (determine_protocol_version_server(NULL) != protocol_v2) {\n \t\t\tpacket_write_fmt(1, \"# service=git-%s\\n\", svc->name);\n \t\t\tpacket_flush(1);\n \t\t}\ndiff --git a/protocol.c b/protocol.c\nindex 052d7edbb9..ad653b7521 100644\n--- a/protocol.c\n+++ b/protocol.c\n@@ -42,7 +42,7 @@ enum protocol_version get_protocol_version_config(void)\n \treturn protocol_v2;\n }\n \n-enum protocol_version determine_protocol_version_server(void)\n+enum protocol_version determine_protocol_version_server(struct strbuf *base_sb)\n {\n \tconst char *git_protocol = getenv(GIT_PROTOCOL_ENVIRONMENT);\n \tenum protocol_version version = protocol_v0;\n@@ -67,6 +67,9 @@ enum protocol_version determine_protocol_version_server(void)\n \t\t\t\tv = parse_protocol_version(value);\n \t\t\t\tif (v > version)\n \t\t\t\t\tversion = v;\n+\t\t\t} else if (skip_prefix(item->string, \"base=\", &value)) {\n+\t\t\t\tif (base_sb)\n+\t\t\t\t\tstrbuf_addstr(base_sb, value);\n \t\t\t}\n \t\t}\n \ndiff --git a/protocol.h b/protocol.h\nindex cef1a4a01c..e418b92049 100644\n--- a/protocol.h\n+++ b/protocol.h\n@@ -22,8 +22,11 @@ enum protocol_version get_protocol_version_config(void);\n * by setting appropriate values for the key 'version'. If a client doesn't\n * request a particular protocol version, a default of 'protocol_v0' will be\n * used.\n+ *\n+ * If base_sb is not NULL and an extra parameter \"base\" is specified, this\n+ * function will append its value to base_sb.\n */\n-enum protocol_version determine_protocol_version_server(void);\n+enum protocol_version determine_protocol_version_server(struct strbuf *base_sb);\n \n /*\n * Used by a client to determine which protocol version the server is speaking\ndiff --git a/t/t5700-protocol-v1.sh b/t/t5700-protocol-v1.sh\nindex 022901b9eb..22459d37f5 100755\n--- a/t/t5700-protocol-v1.sh\n+++ b/t/t5700-protocol-v1.sh\n@@ -146,6 +146,56 @@ test_expect_success 'push with file:// using protocol v1' '\n \tgrep \"push< version 1\" log\n '\n \n+test_expect_success 'push with file:// using protocol v1 and --base' '\n+\ttest_commit -C file_child four &&\n+\tCOMMON_HASH=$(git -C file_child rev-parse three) &&\n+\n+\t# Push to another branch, as the target repository has the\n+\t# master branch checked out and we cannot push into it.\n+\tGIT_TRACE_PACKET=1 git -C file_child -c protocol.version=1 \\\n+\t\tpush --base=three origin HEAD:client_branch_four 2>log &&\n+\n+\t# Server responded using protocol v1\n+\tgrep \"push< version 1\" log &&\n+\t# Server advertised only the expected object\n+\tgrep \"$COMMON_HASH .have\" log\n+'\n+\n+test_expect_success 'push with file:// using protocol v0 and --base' '\n+\ttest_commit -C file_child five &&\n+\tCOMMON_HASH=$(git -C file_child rev-parse four) &&\n+\n+\t# Push to another branch, as the target repository has the\n+\t# master branch checked out and we cannot push into it.\n+\tGIT_TRACE_PACKET=1 git -C file_child -c protocol.version=0 \\\n+\t\tpush --base=four origin HEAD:client_branch_five 2>log &&\n+\n+\t# Server did not respond with any version\n+\t! grep \"push< version\" log &&\n+\t# Server advertised only the expected object\n+\tgrep \"$COMMON_HASH .have\" log\n+'\n+\n+test_expect_success 'push with invalid --base' '\n+\ttest_commit -C file_child six &&\n+\n+\t# Server does not have \"six\".\n+\ttest_must_fail git -C file_child -c protocol.version=0 \\\n+\t\tpush --base=an_invalid_object origin HEAD:client_branch_six 2>log &&\n+\tgrep \"is not a valid object\" log\n+'\n+\n+test_expect_success 'push with --base that does not exist on server' '\n+\tCOMMON_HASH=$(git -C file_child rev-parse six) &&\n+\n+\t# The push still succeeds.\n+\tGIT_TRACE_PACKET=1 git -C file_child -c protocol.version=0 \\\n+\t\tpush --base=six origin HEAD:client_branch_six 2>log &&\n+\n+\t# Server did not advertise \"six\", since it does not know it\n+\t! grep \"$COMMON_HASH .have\" log\n+'\n+\n # Test protocol v1 with 'ssh://' transport\n #\n test_expect_success 'setup ssh wrapper' '\n@@ -226,6 +276,36 @@ test_expect_success 'push with ssh:// using protocol v1' '\n \tgrep \"push< version 1\" log\n '\n \n+test_expect_success 'push with ssh:// using protocol v1 and --base' '\n+\ttest_commit -C ssh_child four &&\n+\tCOMMON_HASH=$(git -C ssh_child rev-parse three) &&\n+\n+\t# Push to another branch, as the target repository has the\n+\t# master branch checked out and we cannot push into it.\n+\tGIT_TRACE_PACKET=1 git -C ssh_child -c protocol.version=1 \\\n+\t\tpush --base=\"$COMMON_HASH\" origin HEAD:client_branch_four 2>log &&\n+\n+\t# Server responded using protocol v1\n+\tgrep \"push< version 1\" log &&\n+\t# Server advertised only the expected object\n+\tgrep \"$COMMON_HASH .have\" log\n+'\n+\n+test_expect_success 'push with ssh:// using protocol v0 and --base' '\n+\ttest_commit -C ssh_child five &&\n+\tCOMMON_HASH=$(git -C ssh_child rev-parse four) &&\n+\n+\t# Push to another branch, as the target repository has the\n+\t# master branch checked out and we cannot push into it.\n+\tGIT_TRACE_PACKET=1 git -C ssh_child -c protocol.version=0 \\\n+\t\tpush --base=\"$COMMON_HASH\" origin HEAD:client_branch_five 2>log &&\n+\n+\t# Server did not respond with any version\n+\t! grep \"push< version\" log &&\n+\t# Server advertised only the expected object\n+\tgrep \"$COMMON_HASH .have\" log\n+'\n+\n # Test protocol v1 with 'http://' transport\n #\n . \"$TEST_DIRECTORY\"/lib-httpd.sh\ndiff --git a/transport.c b/transport.c\nindex ffe2115845..531ca0a834 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -236,6 +236,10 @@ static int set_git_option(struct git_transport_options *opts,\n \t\tlist_objects_filter_die_if_populated(&opts->filter_options);\n \t\tparse_list_objects_filter(&opts->filter_options, value);\n \t\treturn 0;\n+\t} else if (!strcmp(name, TRANS_OPT_PUSH_BASE)) {\n+\t\tif (get_oid(value, &opts->push_base))\n+\t\t\tdie(_(\"transport: '%s' is not a valid object\"), value);\n+\t\treturn 0;\n \t}\n \treturn 1;\n }\n@@ -244,6 +248,7 @@ static int connect_setup(struct transport *transport, int for_push)\n {\n \tstruct git_transport_data *data = transport->data;\n \tint flags = transport->verbose > 0 ? CONNECT_VERBOSE : 0;\n+\tchar *extra_parameters = NULL;\n \n \tif (data->conn)\n \t\treturn 0;\n@@ -254,11 +259,16 @@ static int connect_setup(struct transport *transport, int for_push)\n \tcase TRANSPORT_FAMILY_IPV6: flags |= CONNECT_IPV6; break;\n \t}\n \n+\tif (!is_null_oid(&data->options.push_base))\n+\t\textra_parameters = xstrfmt(\"base=%s\",\n+\t\t\t\t\t oid_to_hex(&data->options.push_base));\n+\n \tdata->conn = git_connect(data->fd, transport->url,\n \t\t\t\t for_push ? data->options.receivepack :\n \t\t\t\t data->options.uploadpack,\n-\t\t\t\t flags);\n+\t\t\t\t flags, extra_parameters);\n \n+\tfree(extra_parameters);\n \treturn 0;\n }\n \n@@ -815,7 +825,7 @@ static int connect_git(struct transport *transport, const char *name,\n {\n \tstruct git_transport_data *data = transport->data;\n \tdata->conn = git_connect(data->fd, transport->url,\n-\t\t\t\t executable, 0);\n+\t\t\t\t executable, 0, NULL);\n \tfd[0] = data->fd[0];\n \tfd[1] = data->fd[1];\n \treturn 0;\ndiff --git a/transport.h b/transport.h\nindex ca409ea1e4..ce51edd673 100644\n--- a/transport.h\n+++ b/transport.h\n@@ -46,6 +46,12 @@ struct git_transport_options {\n \t * transport_set_option().\n \t */\n \tstruct oid_array *negotiation_tips;\n+\n+\t/*\n+\t * When pushing, if this is not a null OID, indicates an ancestor of\n+\t * the commits to be pushed that is believed to be known by the server.\n+\t */\n+\tstruct object_id push_base;\n };\n \n enum transport_family {\n@@ -208,6 +214,9 @@ void transport_check_allowed(const char *type);\n /* Request atomic (all-or-nothing) updates when pushing */\n #define TRANS_OPT_ATOMIC \"atomic\"\n \n+/* See \"push_base\" in struct git_transport_options */\n+#define TRANS_OPT_PUSH_BASE \"push-base\"\n+\n /**\n * Returns 0 if the option was used, non-zero otherwise. Prints a\n * message to stderr if the option is not used.\n-- \n2.29.1.341.ge80a0c044ae-goog\n\n"},{"id":"408966","messageId":"d0a996f77689a12a52b192599fd5e6d27192a648.1604362701.git.jonathantanmy@google.com","threadId":"54560","inReplyTo":"cover.1604362701.git.jonathantanmy@google.com","subject":"[PATCH 3/4] remote-curl: teach --base for http(s)://","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-11-03T00:26:12Z","receivedAt":"2020-11-03T00:27:16Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"For the http(s):// transports, teach push the \"--base\" parameter, which\nindicates an ancestor of the commits to be pushed that is believed to be\nknown by the server.\n\nThis is done through a new remote helper capability and option\n\"push-base\". If a remote helper advertises this capability, then it also\nsupports the option of the same name. Git can then use this option to\ninform the remote helper of a base that the user has specified.\n\nSee the commit message of this commit's parent for more information.\n\nSigned-off-by: Jonathan Tan \n---\n Documentation/gitremote-helpers.txt | 8 ++++++++\n remote-curl.c | 25 +++++++++++++++++-------\n t/t5700-protocol-v1.sh | 30 +++++++++++++++++++++++++++++\n transport-helper.c | 15 +++++++++++++++\n 4 files changed, 71 insertions(+), 7 deletions(-)\n\ndiff --git a/Documentation/gitremote-helpers.txt b/Documentation/gitremote-helpers.txt\nindex 6f1e269ae4..b4eff16a59 100644\n--- a/Documentation/gitremote-helpers.txt\n+++ b/Documentation/gitremote-helpers.txt\n@@ -116,6 +116,9 @@ Supported commands: 'stateless-connect'.\n +\n Supported commands: 'list for-push', 'push'.\n \n+'push-base'::\n+\tIndicates that this remote helper supports the 'push-base' option.\n+\n 'export'::\n \tCan discover remote refs and push specified objects from a\n \tfast-import stream to remote refs.\n@@ -541,6 +544,11 @@ set by Git if the remote helper has the 'option' capability.\n If set to an algorithm, indicate that the caller wants to interact with\n the remote side using that algorithm.\n \n+'option push-base' ::\n+\tOnly supported if this remote helper advertises the 'push-base'\n+\tcapability. Indicate that is an ancestor of the commits to be\n+\tpushed, and it is believed to be known by the server.\n+\n SEE ALSO\n --------\n linkgit:git-remote[1]\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 32cc4a0c55..7919e4ebcf 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -29,6 +29,7 @@ struct options {\n \tstruct string_list deepen_not;\n \tstruct string_list push_options;\n \tchar *filter;\n+\tstruct object_id push_base;\n \tunsigned progress : 1,\n \t\tcheck_self_contained_and_connected : 1,\n \t\tcloning : 1,\n@@ -205,6 +206,10 @@ static int set_option(const char *name, const char *value)\n \t\t\toptions.hash_algo = &hash_algos[algo];\n \t\t}\n \t\treturn 0;\n+\t} else if (!strcmp(name, \"push-base\")) {\n+\t\tif (get_oid(value, &options.push_base))\n+\t\t\tdie(_(\"%s is not a valid object\"), value);\n+\t\treturn 0;\n \t} else {\n \t\treturn 1 /* unsupported */;\n \t}\n@@ -364,16 +369,21 @@ static int show_http_message(struct strbuf *type, struct strbuf *charset,\n }\n \n static int get_protocol_http_header(enum protocol_version version,\n+\t\t\t\t const struct object_id *base,\n \t\t\t\t struct strbuf *header)\n {\n-\tif (version > 0) {\n+\tif (version > 0)\n \t\tstrbuf_addf(header, GIT_PROTOCOL_HEADER \": version=%d\",\n \t\t\t version);\n-\n-\t\treturn 1;\n+\tif (!is_null_oid(base)) {\n+\t\tif (version > 0)\n+\t\t\tstrbuf_addch(header, ':');\n+\t\telse\n+\t\t\tstrbuf_addstr(header, GIT_PROTOCOL_HEADER \": \");\n+\t\tstrbuf_addf(header, \"base=%s\", oid_to_hex(base));\n \t}\n \n-\treturn 0;\n+\treturn !!(version > 0 || !(is_null_oid(base)));\n }\n \n static void check_smart_http(struct discovery *d, const char *service,\n@@ -469,7 +479,7 @@ static struct discovery *discover_refs(const char *service, int for_push)\n \t\tversion = protocol_v0;\n \n \t/* Add the extra Git-Protocol header */\n-\tif (get_protocol_http_header(version, &protocol_header))\n+\tif (get_protocol_http_header(version, &options.push_base, &protocol_header))\n \t\tstring_list_append(&extra_headers, protocol_header.buf);\n \n \tmemset(&http_options, 0, sizeof(http_options));\n@@ -1074,7 +1084,7 @@ static int rpc_service(struct rpc_state *rpc, struct discovery *heads,\n \tstrbuf_addf(&buf, \"Accept: application/x-%s-result\", svc);\n \trpc->hdr_accept = strbuf_detach(&buf, NULL);\n \n-\tif (get_protocol_http_header(heads->version, &buf))\n+\tif (get_protocol_http_header(heads->version, &options.push_base, &buf))\n \t\trpc->protocol_header = strbuf_detach(&buf, NULL);\n \telse\n \t\trpc->protocol_header = NULL;\n@@ -1406,7 +1416,7 @@ static int stateless_connect(const char *service_name)\n \trpc.service_url = xstrfmt(\"%s%s\", url.buf, rpc.service_name);\n \trpc.hdr_content_type = xstrfmt(\"Content-Type: application/x-%s-request\", rpc.service_name);\n \trpc.hdr_accept = xstrfmt(\"Accept: application/x-%s-result\", rpc.service_name);\n-\tif (get_protocol_http_header(discover->version, &buf)) {\n+\tif (get_protocol_http_header(discover->version, &options.push_base, &buf)) {\n \t\trpc.protocol_header = strbuf_detach(&buf, NULL);\n \t} else {\n \t\trpc.protocol_header = NULL;\n@@ -1538,6 +1548,7 @@ int cmd_main(int argc, const char **argv)\n \t\t\tprintf(\"push\\n\");\n \t\t\tprintf(\"check-connectivity\\n\");\n \t\t\tprintf(\"object-format\\n\");\n+\t\t\tprintf(\"push-base\\n\");\n \t\t\tprintf(\"\\n\");\n \t\t\tfflush(stdout);\n \t\t} else if (skip_prefix(buf.buf, \"stateless-connect \", &arg)) {\ndiff --git a/t/t5700-protocol-v1.sh b/t/t5700-protocol-v1.sh\nindex 22459d37f5..4ee29c1be0 100755\n--- a/t/t5700-protocol-v1.sh\n+++ b/t/t5700-protocol-v1.sh\n@@ -373,6 +373,36 @@ test_expect_success 'push with http:// using protocol v1' '\n \tgrep \"git< version 1\" log\n '\n \n+test_expect_success 'push with http:// using protocol v1 and --base' '\n+\ttest_commit -C http_child four &&\n+\tCOMMON_HASH=$(git -C http_child rev-parse three) &&\n+\n+\t# Push to another branch, as the target repository has the\n+\t# master branch checked out and we cannot push into it.\n+\tGIT_TRACE_PACKET=1 test_might_fail git -C http_child -c protocol.version=1 \\\n+\t\tpush --base=three origin HEAD:client_branch_four 2>log &&\n+\n+\t# Server responded using protocol v1\n+\tgrep \"git< version 1\" log &&\n+\t# Server advertised only the expected object\n+\tgrep \"$COMMON_HASH .have\" log\n+'\n+\n+test_expect_success 'push with http:// using protocol v0 and --base' '\n+\ttest_commit -C http_child five &&\n+\tCOMMON_HASH=$(git -C http_child rev-parse four) &&\n+\n+\t# Push to another branch, as the target repository has the\n+\t# master branch checked out and we cannot push into it.\n+\tGIT_TRACE_PACKET=1 git -C http_child -c protocol.version=0 \\\n+\t\tpush --base=four origin HEAD:client_branch_five 2>log &&\n+\n+\t# Server did not respond with any version\n+\t! grep \"git< version\" log &&\n+\t# Server advertised only the expected object\n+\tgrep \"$COMMON_HASH .have\" log\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 b573b6c730..15d9527419 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -27,6 +27,7 @@ struct helper_data {\n \t\texport : 1,\n \t\toption : 1,\n \t\tpush : 1,\n+\t\tpush_base : 1,\n \t\tconnect : 1,\n \t\tstateless_connect : 1,\n \t\tsigned_tags : 1,\n@@ -186,6 +187,8 @@ static struct child_process *get_helper(struct transport *transport)\n \t\t\tdata->option = 1;\n \t\telse if (!strcmp(capname, \"push\"))\n \t\t\tdata->push = 1;\n+\t\telse if (!strcmp(capname, \"push-base\"))\n+\t\t\tdata->push_base = 1;\n \t\telse if (!strcmp(capname, \"import\"))\n \t\t\tdata->import = 1;\n \t\telse if (!strcmp(capname, \"bidi-import\"))\n@@ -1183,6 +1186,18 @@ static struct ref *get_refs_list_using_list(struct transport *transport,\n \t\t\texit(128);\n \t}\n \n+\tif (!is_null_oid(&data->transport_options.push_base)) {\n+\t\tif (data->push_base) {\n+\t\t\twrite_str_in_full(helper->in, \"option push-base \");\n+\t\t\twrite_str_in_full(helper->in, oid_to_hex(&data->transport_options.push_base));\n+\t\t\twrite_str_in_full(helper->in, \"\\n\");\n+\t\t\tif (recvline(data, &buf) || strcmp(buf.buf, \"ok\"))\n+\t\t\t\texit(128);\n+\t\t} else {\n+\t\t\twarning(_(\"transport does not support --base\"));\n+\t\t}\n+\t}\n+\n \tif (data->push && for_push)\n \t\twrite_str_in_full(helper->in, \"list for-push\\n\");\n \telse\n-- \n2.29.1.341.ge80a0c044ae-goog\n\n"},{"id":"408967","messageId":"6250c13897e3cc01f247d80c148cf8dc5e7f3ad0.1604362701.git.jonathantanmy@google.com","threadId":"54560","inReplyTo":"cover.1604362701.git.jonathantanmy@google.com","subject":"[PATCH 4/4] Doc: push with --base","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-11-03T00:26:13Z","receivedAt":"2020-11-03T00:27:17Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Includes protocol documentation and a design document.\n\nSigned-off-by: Jonathan Tan \n---\n Documentation/technical/pack-protocol.txt | 10 ++--\n Documentation/technical/push-with-base.txt | 61 ++++++++++++++++++++++\n 2 files changed, 68 insertions(+), 3 deletions(-)\n create mode 100644 Documentation/technical/push-with-base.txt\n\ndiff --git a/Documentation/technical/pack-protocol.txt b/Documentation/technical/pack-protocol.txt\nindex e13a2c064d..0485616701 100644\n--- a/Documentation/technical/pack-protocol.txt\n+++ b/Documentation/technical/pack-protocol.txt\n@@ -59,9 +59,9 @@ Parameters\", and are supported by the Git, SSH, and HTTP protocols.\n Each Extra Parameter takes the form of `=` or ``.\n \n Servers that receive any such Extra Parameters MUST ignore all\n-unrecognized keys. Currently, the only Extra Parameter recognized is\n-\"version\" with a value of '1' or '2'. See protocol-v2.txt for more\n-information on protocol version 2.\n+unrecognized keys. Currently, the only Extra Parameters recognized are\n+\"version\" with a value of '1' or '2' and, for push, \"base\" with an OID. See\n+protocol-v2.txt for more information on protocol version 2.\n \n Git Transport\n -------------\n@@ -506,6 +506,10 @@ real difference is that the capability listing is different - the only\n possible values are 'report-status', 'report-status-v2', 'delete-refs',\n 'ofs-delta', 'atomic' and 'push-options'.\n \n+If a \"base=\" Extra Parameter was sent by the client, and the\n+server recognizes that object, the server MAY send \" .have\" in\n+lieu of all the reference obj-ids and names.\n+\n Reference Update Request and Packfile Transfer\n ----------------------------------------------\n \ndiff --git a/Documentation/technical/push-with-base.txt b/Documentation/technical/push-with-base.txt\nnew file mode 100644\nindex 0000000000..d56aa7f900\n--- /dev/null\n+++ b/Documentation/technical/push-with-base.txt\n@@ -0,0 +1,61 @@\n+Push with base design notes\n+===========================\n+\n+This feature allows clients, when pushing, to indicate that a\n+certain object is an ancestor of all pushed commits and that they\n+believe that the server knows of this object. This in turn allows\n+servers to send an abbreviated ref advertisement containing only that\n+object.\n+\n+Besides bandwidth savings, this also ensures that the ref\n+advertisement contains information relevant to the client. For\n+example, at least one project (Gerrit [1]) have included workarounds\n+to send ancestors of refs that move often, even though the ref\n+advertisement is only meant to contain refs.\n+\n+[1] https://gerrit.googlesource.com/gerrit/+/refs/heads/master/java/com/google/gerrit/server/git/receive/HackPushNegotiateHook.java\n+\n+\n+Design overview\n+---------------\n+\n+The \"base\" being sent is sent as an Extra Parameter, supported in the\n+git://, ssh://, and http(s):// protocols. By sending it as an Extra\n+Parameter, the server is aware of this parameter before it generates\n+the ref advertisement, thus making it able to tailor the ref\n+advertisement accordingly. Sending it as an Extra Parameter also makes\n+this protocol backwards-compatible, as servers will ignore any Extra\n+Parameters they do not understand. (The push will then proceed as if\n+neither party had this feature.)\n+\n+The remote helper protocol has been extended to support the\n+\"push-base\" capability and an option of the same name. When a remote\n+helper advertises this capability, it thus indicates that it supports\n+this option. Git then will send \"option push-base\" if the user\n+specifies it when invoking \"git push\".\n+\n+The remote-curl remote helper bundled with Git has been updated to\n+support this capability and option.\n+\n+\n+Future work\n+-----------\n+\n+In the future, we might want a way to automatically determine the base\n+instead of always having the user specify it. However, this does not\n+make obsolete any of the current work - once the base is automatically\n+determined, we still need this protocol to communicate it to the\n+server, and allowing the user to specify the base manually is still\n+useful.\n+\n+\n+Alternatives\n+------------\n+\n+- Making a more substantial protocol change like \"fetch\" protocol v2.\n+ This would eliminate the need for some of the remote helper updates;\n+ as part of the protocol change, the protocol could be made to\n+ support \"stateless-connect\" and thus no remote helper updates (like\n+ \"push-base\") would be needed. For \"fetch\", the protocol change has\n+ enabled features like wanted-refs and packfile-uris, but I do not\n+ have any similar ideas in mind for \"push\".\n-- \n2.29.1.341.ge80a0c044ae-goog\n\n"},{"id":"408969","messageId":"xmqqh7q7qncm.fsf@gitster.c.googlers.com","threadId":"54560","inReplyTo":"cover.1604362701.git.jonathantanmy@google.com","subject":"Re: [PATCH 0/4] \"Push\" protocol change proposal: user-specified base","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-11-03T00:46:01Z","receivedAt":"2020-11-03T00:46:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> At $DAY_JOB, we've noticed some pushes being larger than necessary.\n> Here's a first step towards solving that. In this protocol change, we're\n> allowing the user to specify an ancestor believed to be common\n\nIs this designed to be a short-term kludge until we have a similar\n\"common discovery\" negotiation we have on the upload-pack side, or\ndo we know some fundamental reason why the \"push\" side cannot do so?\n\nJust being curious as to what the eventual and ideal future beyond\n\"we must have an immediate problem worked around in some way even\nwith an ugly hack before the end of this cycle\" would be.\n\nWill take a look but not today.\n\nThanks.\n"},{"id":"408975","messageId":"xmqqd00vp7ic.fsf@gitster.c.googlers.com","threadId":"54560","inReplyTo":"d0a996f77689a12a52b192599fd5e6d27192a648.1604362701.git.jonathantanmy@google.com","subject":"Re: [PATCH 3/4] remote-curl: teach --base for http(s)://","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-11-03T01:13:31Z","receivedAt":"2020-11-03T01:13:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> diff --git a/t/t5700-protocol-v1.sh b/t/t5700-protocol-v1.sh\n> index 22459d37f5..4ee29c1be0 100755\n> --- a/t/t5700-protocol-v1.sh\n> +++ b/t/t5700-protocol-v1.sh\n> ...\n> +\t# Push to another branch, as the target repository has the\n> +\t# master branch checked out and we cannot push into it.\n> +\tGIT_TRACE_PACKET=1 test_might_fail git -C http_child -c protocol.version=1 \\\n> +\t\tpush --base=three origin HEAD:client_branch_four 2>log &&\n\nThis attempt to one-shot export GIT_TRACE_PACKET is flagged as an\nerror by test-lint-shell-syntax.\n\n test_might_fail env GIT_TRACE_PACKET=1 \\\n\tgit -C ... push --base=three ... &&\n\nperhaps?\n"},{"id":"408982","messageId":"20201103053554.GC948115@google.com","threadId":"54560","inReplyTo":"6250c13897e3cc01f247d80c148cf8dc5e7f3ad0.1604362701.git.jonathantanmy@google.com","subject":"Re: [PATCH 4/4] Doc: push with --base","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2020-11-03T05:35:54Z","receivedAt":"2020-11-03T05:36:00Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"(+cc: Elijah Newren and Derrick Stolee because of [1])\nHi,\n\nJonathan Tan wrote:\n\n> Includes protocol documentation and a design document.\n\nThanks for writing this.\n\n> Signed-off-by: Jonathan Tan \n> ---\n> Documentation/technical/pack-protocol.txt | 10 ++--\n> Documentation/technical/push-with-base.txt | 61 ++++++++++++++++++++++\n> 2 files changed, 68 insertions(+), 3 deletions(-)\n> create mode 100644 Documentation/technical/push-with-base.txt\n> \n> diff --git a/Documentation/technical/pack-protocol.txt b/Documentation/technical/pack-protocol.txt\n> index e13a2c064d..0485616701 100644\n> --- a/Documentation/technical/pack-protocol.txt\n> +++ b/Documentation/technical/pack-protocol.txt\n> @@ -59,9 +59,9 @@ Parameters\", and are supported by the Git, SSH, and HTTP protocols.\n> Each Extra Parameter takes the form of `=` or ``.\n> \n> Servers that receive any such Extra Parameters MUST ignore all\n> -unrecognized keys. Currently, the only Extra Parameter recognized is\n> -\"version\" with a value of '1' or '2'. See protocol-v2.txt for more\n> -information on protocol version 2.\n> +unrecognized keys. Currently, the only Extra Parameters recognized are\n> +\"version\" with a value of '1' or '2' and, for push, \"base\" with an OID. See\n> +protocol-v2.txt for more information on protocol version 2.\n\nIt's nice to see an example of extra parameters being useful in\nprotocol v0 as well. :)\n\n> \n> Git Transport\n> -------------\n> @@ -506,6 +506,10 @@ real difference is that the capability listing is different - the only\n> possible values are 'report-status', 'report-status-v2', 'delete-refs',\n> 'ofs-delta', 'atomic' and 'push-options'.\n> \n> +If a \"base=\" Extra Parameter was sent by the client, and the\n> +server recognizes that object, the server MAY send \" .have\" in\n> +lieu of all the reference obj-ids and names.\n> +\n\nCan this only appear once, or is it permitted to pass multiple oids\nthis way?\n\nI'm also curious about what Junio asked about elsewhere in the thread:\nfor cases that benefit from more complex negotiation than the client\nproposing a particular oid, what comes next in this roadmap? I like\nthat this is an optional feature so we could clean up later by\nremoving support for it; do we expect to? If we do expect to, is\nthere anything we could do to minimize the impact of the feature later\n(e.g. using a less short-and-sweet key name than `base`, maybe)?\n\n> Reference Update Request and Packfile Transfer\n> ----------------------------------------------\n> \n> diff --git a/Documentation/technical/push-with-base.txt b/Documentation/technical/push-with-base.txt\n> new file mode 100644\n> index 0000000000..d56aa7f900\n> --- /dev/null\n> +++ b/Documentation/technical/push-with-base.txt\n> @@ -0,0 +1,61 @@\n> +Push with base design notes\n> +===========================\n> +\n> +This feature allows clients, when pushing, to indicate that a\n> +certain object is an ancestor of all pushed commits and that they\n> +believe that the server knows of this object. This in turn allows\n> +servers to send an abbreviated ref advertisement containing only that\n> +object.\n> +\n> +Besides bandwidth savings, this also ensures that the ref\n> +advertisement contains information relevant to the client. For\n> +example, at least one project (Gerrit [1]) have included workarounds\n> +to send ancestors of refs that move often, even though the ref\n> +advertisement is only meant to contain refs.\n> +\n> +[1] https://gerrit.googlesource.com/gerrit/+/refs/heads/master/java/com/google/gerrit/server/git/receive/HackPushNegotiateHook.java\n> +\n> +\n> +Design overview\n> +---------------\n> +\n> +The \"base\" being sent is sent as an Extra Parameter, supported in the\n> +git://, ssh://, and http(s):// protocols. By sending it as an Extra\n> +Parameter, the server is aware of this parameter before it generates\n> +the ref advertisement, thus making it able to tailor the ref\n> +advertisement accordingly. Sending it as an Extra Parameter also makes\n> +this protocol backwards-compatible, as servers will ignore any Extra\n> +Parameters they do not understand. (The push will then proceed as if\n> +neither party had this feature.)\n> +\n> +The remote helper protocol has been extended to support the\n> +\"push-base\" capability and an option of the same name. When a remote\n> +helper advertises this capability, it thus indicates that it supports\n> +this option. Git then will send \"option push-base\" if the user\n> +specifies it when invoking \"git push\".\n> +\n> +The remote-curl remote helper bundled with Git has been updated to\n> +support this capability and option.\n\nSince this is an optional remote helper capability, it could be\nremoved later. Good (but the capability and option names would still\nneed to be reserved).\n\n> +\n> +\n> +Future work\n> +-----------\n> +\n> +In the future, we might want a way to automatically determine the base\n> +instead of always having the user specify it. However, this does not\n> +make obsolete any of the current work - once the base is automatically\n> +determined, we still need this protocol to communicate it to the\n> +server, and allowing the user to specify the base manually is still\n> +useful.\n\nMakes sense. I think this isn't \"might\", but \"would\"; most users\nthat do not know why their push is slow wouldn't know to use this\nfeature.\n\n> +\n> +\n> +Alternatives\n> +------------\n> +\n> +- Making a more substantial protocol change like \"fetch\" protocol v2.\n> + This would eliminate the need for some of the remote helper updates;\n> + as part of the protocol change, the protocol could be made to\n> + support \"stateless-connect\" and thus no remote helper updates (like\n> + \"push-base\") would be needed. For \"fetch\", the protocol change has\n> + enabled features like wanted-refs and packfile-uris, but I do not\n> + have any similar ideas in mind for \"push\".\n\nI think you're saying that we don't need a \"push\" v2 because v0\nalready has what a user would want.\n\nGit protocol v2 for fetch brought two major changes:\n\n- it changed the response for the initial request, allowing\n abbreviating the ref advertisement at last\n\n- it defined a structure for requests and responses, simplifying the\n addition of later protocol improvements. In particular, because the\n initial response is a capability advertisement, it allows changing\n the ref advertisement format more in the future.\n\nBoth of those changes would be valuable for push. The ref\nadvertisements are large, and matching the structure of commands used\nby fetchv2 would make debugging easier.\n\nThere are some specific applications I'm interested in after that\n(e.g., pushing symrefs), but the fundamental extensibility improvement\nis larger than any particular application I could think of.\n\nThat said, I'm not against experimenting with extra parameters before\nwe go there, as a way of getting more information about what a\nworkable negotiation for push looks like. The push ref advertisement\nserves two roles: it advertises commits, to serve as negotiation, and\nit tells the client the current values of refs they may want to\nupdate, so they can send an appropriate compare-and-swap command for\na force-push. If we have a workable replacement negotiation system,\nthen the other role can be replaced with something simpler. For\nexample:\n\n- listing ref updates before the pack, to allow the server to bail\n out early if they would be rejected, and\n\n- allowing an explicit force update of a ref instead of requiring the\n client to compare-and-swap if they don't care about the old value\n (but taking care to still make the client \"fetch first\" when\n appropriate!)\n\nThanks,\nJonathan\n\n[1] https://lore.kernel.org/git/CABPp-BEswHLhymQ1_07g3qqu=7kFR3eQyAHR0qMgSvi6THy=zQ@mail.gmail.com/\n"},{"id":"408987","messageId":"20201103102307.GI24813@szeder.dev","threadId":"54560","inReplyTo":"148e39960a2185d2355cdfe34f8856e708fb1b80.1604362701.git.jonathantanmy@google.com","subject":"Re: [PATCH 2/4] push: teach --base for ssh:// and file://","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2020-11-03T10:23:07Z","receivedAt":"2020-11-03T10:23:12Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Mon, Nov 02, 2020 at 04:26:11PM -0800, Jonathan Tan wrote:\n> diff --git a/t/t5700-protocol-v1.sh b/t/t5700-protocol-v1.sh\n> index 022901b9eb..22459d37f5 100755\n> --- a/t/t5700-protocol-v1.sh\n> +++ b/t/t5700-protocol-v1.sh\n> @@ -146,6 +146,56 @@ test_expect_success 'push with file:// using protocol v1' '\n> \tgrep \"push< version 1\" log\n> '\n> \n> +test_expect_success 'push with file:// using protocol v1 and --base' '\n> +\ttest_commit -C file_child four &&\n> +\tCOMMON_HASH=$(git -C file_child rev-parse three) &&\n> +\n> +\t# Push to another branch, as the target repository has the\n> +\t# master branch checked out and we cannot push into it.\n> +\tGIT_TRACE_PACKET=1 git -C file_child -c protocol.version=1 \\\n> +\t\tpush --base=three origin HEAD:client_branch_four 2>log &&\n> +\n> +\t# Server responded using protocol v1\n> +\tgrep \"push< version 1\" log &&\n> +\t# Server advertised only the expected object\n> +\tgrep \"$COMMON_HASH .have\" log\n> +'\n> +\n> +test_expect_success 'push with file:// using protocol v0 and --base' '\n> +\ttest_commit -C file_child five &&\n> +\tCOMMON_HASH=$(git -C file_child rev-parse four) &&\n> +\n> +\t# Push to another branch, as the target repository has the\n> +\t# master branch checked out and we cannot push into it.\n> +\tGIT_TRACE_PACKET=1 git -C file_child -c protocol.version=0 \\\n> +\t\tpush --base=four origin HEAD:client_branch_five 2>log &&\n> +\n> +\t# Server did not respond with any version\n> +\t! grep \"push< version\" log &&\n> +\t# Server advertised only the expected object\n> +\tgrep \"$COMMON_HASH .have\" log\n> +'\n> +\n> +test_expect_success 'push with invalid --base' '\n> +\ttest_commit -C file_child six &&\n> +\n> +\t# Server does not have \"six\".\n> +\ttest_must_fail git -C file_child -c protocol.version=0 \\\n> +\t\tpush --base=an_invalid_object origin HEAD:client_branch_six 2>log &&\n> +\tgrep \"is not a valid object\" log\n\nThis should rather use 'test_i18ngrep' ...\n\n> +'\n> +\n> +test_expect_success 'push with --base that does not exist on server' '\n> +\tCOMMON_HASH=$(git -C file_child rev-parse six) &&\n> +\n> +\t# The push still succeeds.\n> +\tGIT_TRACE_PACKET=1 git -C file_child -c protocol.version=0 \\\n> +\t\tpush --base=six origin HEAD:client_branch_six 2>log &&\n> +\n> +\t# Server did not advertise \"six\", since it does not know it\n> +\t! grep \"$COMMON_HASH .have\" log\n> +'\n> +\n> # Test protocol v1 with 'ssh://' transport\n> #\n> test_expect_success 'setup ssh wrapper' '\n\n\n> diff --git a/transport.c b/transport.c\n> index ffe2115845..531ca0a834 100644\n> --- a/transport.c\n> +++ b/transport.c\n> @@ -236,6 +236,10 @@ static int set_git_option(struct git_transport_options *opts,\n> \t\tlist_objects_filter_die_if_populated(&opts->filter_options);\n> \t\tparse_list_objects_filter(&opts->filter_options, value);\n> \t\treturn 0;\n> +\t} else if (!strcmp(name, TRANS_OPT_PUSH_BASE)) {\n> +\t\tif (get_oid(value, &opts->push_base))\n> +\t\t\tdie(_(\"transport: '%s' is not a valid object\"), value);\n\n... because the error message here is translated.\n\n> +\t\treturn 0;\n> \t}\n> \treturn 1;\n> }\n"},{"id":"408993","messageId":"ee98ed69-dc49-af18-71e3-7c6168b5a464@gmail.com","threadId":"54560","inReplyTo":"6250c13897e3cc01f247d80c148cf8dc5e7f3ad0.1604362701.git.jonathantanmy@google.com","subject":"Re: [PATCH 4/4] Doc: push with --base","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-11-03T13:53:06Z","receivedAt":"2020-11-03T13:54:27Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 11/2/2020 7:26 PM, Jonathan Tan wrote:\n> Includes protocol documentation and a design document.\n\n> Servers that receive any such Extra Parameters MUST ignore all\n> -unrecognized keys. Currently, the only Extra Parameter recognized is\n> -\"version\" with a value of '1' or '2'. See protocol-v2.txt for more\n> -information on protocol version 2.\n> +unrecognized keys. Currently, the only Extra Parameters recognized are\n> +\"version\" with a value of '1' or '2' and, for push, \"base\" with an OID. See\n> +protocol-v2.txt for more information on protocol version 2.\n> \n> Git Transport\n> -------------\n> @@ -506,6 +506,10 @@ real difference is that the capability listing is different - the only\n> possible values are 'report-status', 'report-status-v2', 'delete-refs',\n> 'ofs-delta', 'atomic' and 'push-options'.\n> \n> +If a \"base=\" Extra Parameter was sent by the client, and the\n> +server recognizes that object, the server MAY send \" .have\" in\n> +lieu of all the reference obj-ids and names.\n> +\n\nnit: no comma before \"and the server recognizes\" as these are both\nconditions of the \"if\". My preference is also to include a \"then\"\nbefore the result, especially with compound conditions like this.\nPerhaps also make it more active voice while we are here?\n\n+ If the client sent a \"base=\" Extra Parameter and the server\n+ recognizes that object, then the server MAY send \" .have\" in\n+ lieu of all the reference object ids and names.\n\n> +Push with base design notes\n> +===========================\n\nPerhaps start with a brief problem statement?\n\n+ If a client wishes to push data to a server without first running\n+ `git fetch`, then they might not have local copies of the objects\n+ at the server's ref tips. The client cannot compute an adequate\n+ common base and will send more objects than necessary.\n\nThen,\n\n+ The \"push with base\" feature allows...\n\n> +This feature allows clients, when pushing, to indicate that a\n> +certain object is an ancestor of all pushed commits and that they\n> +believe that the server knows of this object. This in turn allows\n> +servers to send an abbreviated ref advertisement containing only that\n> +object.\n\nWhy only one base? Could there be value in sending multiple possible\nbases and the server can report the subset that they know about?\n\n> +Besides bandwidth savings,\n\nIt seems you are underselling the bandwidth savings here, because\nwe save in both the ref advertisement and the push pack size. It\nmight be good to call out both metrics so we can see that the ref\nimprovement works even for cases where the client recently fetched\nand can figure out the proper base on its own!\n\n> this also ensures that the ref\n> +advertisement contains information relevant to the client. For\n> +example, at least one project (Gerrit [1]) have included workarounds\n> +to send ancestors of refs that move often, even though the ref\n> +advertisement is only meant to contain refs.\n> +\n> +[1] https://gerrit.googlesource.com/gerrit/+/refs/heads/master/java/com/google/gerrit/server/git/receive/HackPushNegotiateHook.java\n> +\n> +\n> +Design overview\n> +---------------\n> +\n> +The \"base\" being sent is sent as an Extra Parameter, supported in the\n> +git://, ssh://, and http(s):// protocols. By sending it as an Extra\n> +Parameter, the server is aware of this parameter before it generates\n> +the ref advertisement, thus making it able to tailor the ref\n> +advertisement accordingly. Sending it as an Extra Parameter also makes\n> +this protocol backwards-compatible, as servers will ignore any Extra\n> +Parameters they do not understand. (The push will then proceed as if\n> +neither party had this feature.)\n\nI'm not familiar enough with the extra parameter logic to know if we\nallow multi-valued extra parameters. It would be good to relax the\n\"base\" parameter to either allow multiple instances of the parameter\nor to have something like a comma-separated list of OIDs be allowed.\n\nThis might be particularly important for multiple refs being pushed\nat the same time.\n\n> +The remote helper protocol has been extended to support the\n> +\"push-base\" capability and an option of the same name. When a remote\n> +helper advertises this capability, it thus indicates that it supports\n> +this option. Git then will send \"option push-base\" if the user\n> +specifies it when invoking \"git push\".\n> +\n> +The remote-curl remote helper bundled with Git has been updated to\n> +support this capability and option.> +\n> +\n> +Future work\n> +-----------\n> +\n> +In the future, we might want a way to automatically determine the base\n> +instead of always having the user specify it. However, this does not\n> +make obsolete any of the current work - once the base is automatically\n> +determined, we still need this protocol to communicate it to the\n> +server, and allowing the user to specify the base manually is still\n> +useful.\n\nIt is appropriate to separate the protocol ability from the automatic\ncomputation on the client. It would be nice to know how you expect that\nto work.\n\nFor the single-ref push case, my default instinct would be to find a\nmerge-base between that ref and all of the remote refs matching the\nremote we are pushing to. Bonus points if we find _all_ (maximal)\nmerge-bases!\n\nThe equivalent for the multi-ref push might be to find the boundary\ncommits from something like the following command:\n\n\tgit rev-list --boundary \\\n\t\t--not refs/remotes/origin/A \\\n\t\t--not refs/remotes/origin/B \\\n\t\t--not refs/remotes/origin/C \\\n\t\trefs/heads/toPush/1 \\\n\t\trefs/heads/toPush/2 \\\n\t\trefs/heads/toPush/3\n\nBut you might have something better in mind.\n\n> +\n> +\n> +Alternatives\n> +------------\n\nOdd to have a single bullet in a bulleted list.\n\n> +- Making a more substantial protocol change like \"fetch\" protocol v2.\n\nThis isn't a full sentence.\n\nPerhaps: \"One considered approach was to introduce a multi-phase\nnegotiation step similar to how 'git fetch' operates.\"\n\n> + This would eliminate the need for some of the remote helper updates;\n> + as part of the protocol change, the protocol could be made to\n> + support \"stateless-connect\" and thus no remote helper updates (like\n> + \"push-base\") would be needed. For \"fetch\", the protocol change has\n> + enabled features like wanted-refs and packfile-uris, but I do not\n> + have any similar ideas in mind for \"push\".\n\nI think that you should mention that a negotiation phase here would\nuse significantly more bandwidth to do the ref advertisements on both\nends with only a rare case that the negotiated bases are better than\nthe locally-computed bases. Everything is about tradeoffs here!\n\nI'm generally excited about the opportunities here. I'd love to see\nsome measurements for reduced ref advertisements and reduced object\ncounts in the pushed pack.\n\nThanks,\n-Stolee\n"},{"id":"408994","messageId":"1ccd47dc-0470-39de-b26e-5e2f896bb0fe@gmail.com","threadId":"54560","inReplyTo":"148e39960a2185d2355cdfe34f8856e708fb1b80.1604362701.git.jonathantanmy@google.com","subject":"Re: [PATCH 2/4] push: teach --base for ssh:// and file://","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-11-03T13:57:15Z","receivedAt":"2020-11-03T13:57:51Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 11/2/2020 7:26 PM, Jonathan Tan wrote:\n> For the ssh:// and file:// transports, teach push the \"--base\"\n> parameter, which indicates an ancestor of the commits to be pushed that\n> is believed to be known by the server.\n\nLooking at this patch, I don't see any reason why this couldn't be\na multi-valued parameter that populates a string_list on the client\nand the server translates into a list of object_ids. Converting from\nthe current single-base model to a multiple-base model would probably\nbe just as big of a patch as this one, so perhaps jump straight to\nthat capability instead of pausing at the single-base model here?\n\nThanks,\n-Stolee\n"},{"id":"409003","messageId":"20201103151859.GA444466@coredump.intra.peff.net","threadId":"54560","inReplyTo":"20201103053554.GC948115@google.com","subject":"Re: [PATCH 4/4] Doc: push with --base","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-11-03T15:18:59Z","receivedAt":"2020-11-03T15:20:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 02, 2020 at 09:35:54PM -0800, Jonathan Nieder wrote:\n\n> I think you're saying that we don't need a \"push\" v2 because v0\n> already has what a user would want.\n> \n> Git protocol v2 for fetch brought two major changes:\n> \n> - it changed the response for the initial request, allowing\n> abbreviating the ref advertisement at last\n> \n> - it defined a structure for requests and responses, simplifying the\n> addition of later protocol improvements. In particular, because the\n> initial response is a capability advertisement, it allows changing\n> the ref advertisement format more in the future.\n> \n> Both of those changes would be valuable for push. The ref\n> advertisements are large, and matching the structure of commands used\n> by fetchv2 would make debugging easier.\n> \n> There are some specific applications I'm interested in after that\n> (e.g., pushing symrefs), but the fundamental extensibility improvement\n> is larger than any particular application I could think of.\n\nYou pretty much summed up what I was going to respond. :)\n\nBut I'd go further here...\n\n> That said, I'm not against experimenting with extra parameters before\n> we go there, as a way of getting more information about what a\n> workable negotiation for push looks like.\n\nI'd prefer to avoid doing this as an extra parameter for a few reasons:\n\n - once it's in a released version, it's much harder for us to take it\n away\n\n - the extra parameters area is a hack that helped us bootstrap v2. We\n could probably use the same hack to bootstrap v3, etc. But it has\n limitations for stuffing in arbitrary data. An obvious one is size.\n We can transmit a single base, but would be limited if we wanted to\n be able to send multiple. We already ran into this once with the\n \"symref=foo:bar\" capability overflowing pkt-line limits. Here I'm\n not even sure what the limits might be (it's subject to things like\n how big an HTTP header a proxy will pass, or how large an\n environment variable an ssh implementation supports)\n\n - it potentially pushes more data/work outside of the git protocol\n itself. E.g., web servers have to translate Git-Protocol headers\n into the GIT_PROTOCOL environment for v2. I guess this new field\n works in our tests because we copy the header's value entirely in\n our apache.conf. But I wonder how many systems in the wild may only\n work if it contains \"version=2\".\n\n-Peff\n"},{"id":"409022","messageId":"xmqqft5qs5qs.fsf@gitster.c.googlers.com","threadId":"54560","inReplyTo":"20201103151859.GA444466@coredump.intra.peff.net","subject":"Re: [PATCH 4/4] Doc: push with --base","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-11-03T17:35:39Z","receivedAt":"2020-11-03T17:35:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Mon, Nov 02, 2020 at 09:35:54PM -0800, Jonathan Nieder wrote:\n>\n>> I think you're saying that we don't need a \"push\" v2 because v0\n>> already has what a user would want.\n>> \n>> Git protocol v2 for fetch brought two major changes:\n>> \n>> - it changed the response for the initial request, allowing\n>> abbreviating the ref advertisement at last\n>> \n>> - it defined a structure for requests and responses, simplifying the\n>> addition of later protocol improvements. In particular, because the\n>> initial response is a capability advertisement, it allows changing\n>> the ref advertisement format more in the future.\n>> \n>> Both of those changes would be valuable for push. The ref\n>> advertisements are large, and matching the structure of commands used\n>> by fetchv2 would make debugging easier.\n>> \n>> There are some specific applications I'm interested in after that\n>> (e.g., pushing symrefs), but the fundamental extensibility improvement\n>> is larger than any particular application I could think of.\n>\n> You pretty much summed up what I was going to respond. :)\n>\n> But I'd go further here...\n>\n>> That said, I'm not against experimenting with extra parameters before\n>> we go there, as a way of getting more information about what a\n>> workable negotiation for push looks like.\n>\n> I'd prefer to avoid doing this as an extra parameter for a few reasons:\n>\n> - once it's in a released version, it's much harder for us to take it\n> away\n>\n> - the extra parameters area is a hack that helped us bootstrap v2. We\n> could probably use the same hack to bootstrap v3, etc. But it has\n> limitations for stuffing in arbitrary data. An obvious one is size.\n> We can transmit a single base, but would be limited if we wanted to\n> be able to send multiple. We already ran into this once with the\n> \"symref=foo:bar\" capability overflowing pkt-line limits. Here I'm\n> not even sure what the limits might be (it's subject to things like\n> how big an HTTP header a proxy will pass, or how large an\n> environment variable an ssh implementation supports)\n>\n> - it potentially pushes more data/work outside of the git protocol\n> itself. E.g., web servers have to translate Git-Protocol headers\n> into the GIT_PROTOCOL environment for v2. I guess this new field\n> works in our tests because we copy the header's value entirely in\n> our apache.conf. But I wonder how many systems in the wild may only\n> work if it contains \"version=2\".\n\nI do not have much to add to what has been said so far, other than\noffering historical perspective.\n\nThe single biggest reason why \"fetch\" has common ancestor discovery\nnegotiation and \"push\" does not is because the design comes from the\nuse case the inventor of Git and those worked on the early protocol\nwanted to support---you are pushing into your own repository you\nalone push into, your work is disseminated to others who fetch from\nyour repository, and you get others' work by fetching from theirs.\n\nIn such a world without a central server where everybody pushes\ninto, by definition, a pusher knows all the objects that have ever\nbeen pushed into the receiving repository when running \"git push\".\nThey are all objects that passed through the repository you are\npushing from to the receiving repository in your previous pushes.\nThe advertised ref(s) are expected to be known to the repository you\nare pusing from anyway, and if that is not the case, you would first\nfetch from there before force-pushing.\n\nHence, when you push, there isn't much need to walk back from the\ntip of refs at the remote to discover common ancestor like we do for\nthe fetch side in pre-central-server world.\n\nOn the other hand, you expect that remote refs point at objects\nunknown to you when you fetch from your colleagues, so it is\nexpected that you have to perform the common ancestor discovery\nnegotiation.\n\nAfter 15 years, we live in a different world.\n\nPeople expect that a single repository at their hosting sites can be\nused as the central meeting point for the project, just like CVS/SVN\nservers were in older world. \"git push\" would need to accept that\nreality and start common ancestor discovery eventually.\n"},{"id":"409336","messageId":"xmqq4klzejdv.fsf@gitster.c.googlers.com","threadId":"54560","inReplyTo":"148e39960a2185d2355cdfe34f8856e708fb1b80.1604362701.git.jonathantanmy@google.com","subject":"Re: [PATCH 2/4] push: teach --base for ssh:// and file://","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-11-08T19:30:36Z","receivedAt":"2020-11-08T19:30:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> static int do_push(int flags,\n> \t\t const struct string_list *push_options,\n> -\t\t struct remote *remote)\n> +\t\t struct remote *remote,\n> +\t\t const char *push_base)\n> {\n> \tint i, errs;\n> \tconst char **url;\n> @@ -405,6 +406,8 @@ static int do_push(int flags,\n> \t\t\t\ttransport_get(remote, url[i]);\n> \t\t\tif (flags & TRANSPORT_PUSH_OPTIONS)\n> \t\t\t\ttransport->push_options = push_options;\n> +\t\t\tif (push_base)\n> +\t\t\t\ttransport_set_option(transport, TRANS_OPT_PUSH_BASE, push_base);\n> \t\t\tif (push_with_options(transport, push_refspec, flags))\n> \t\t\t\terrs++;\n> \t\t}\n> @@ -413,6 +416,8 @@ static int do_push(int flags,\n> \t\t\ttransport_get(remote, NULL);\n> \t\tif (flags & TRANSPORT_PUSH_OPTIONS)\n> \t\t\ttransport->push_options = push_options;\n> +\t\tif (push_base)\n> +\t\t\ttransport_set_option(transport, TRANS_OPT_PUSH_BASE, push_base);\n> \t\tif (push_with_options(transport, push_refspec, flags))\n> \t\t\terrs++;\n> \t}\n\nThese just send push_base as-is.\n\n> @@ -526,6 +531,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)\n> \tstruct string_list *push_options;\n> \tconst struct string_list_item *item;\n> \tstruct remote *remote;\n> +\tconst char *push_base = NULL;\n> \n> \tstruct option options[] = {\n> \t\tOPT__VERBOSITY(&verbosity),\n> @@ -562,6 +568,8 @@ int cmd_push(int argc, const char **argv, const char *prefix)\n> \t\t\t\tTRANSPORT_FAMILY_IPV4),\n> \t\tOPT_SET_INT('6', \"ipv6\", &family, N_(\"use IPv6 addresses only\"),\n> \t\t\t\tTRANSPORT_FAMILY_IPV6),\n> +\t\tOPT_STRING(0, \"base\", &push_base, N_(\"revision\"),\n> +\t\t\t N_(\"ancestor of commits to be pushed that is believed to be known by the server\")),\n> \t\tOPT_END()\n> \t};\n\nAnd this takes push_base as a string that is not even validated for\nany constraints.\n\n> @@ -629,7 +637,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)\n> \t\tif (strchr(item->string, '\\n'))\n> \t\t\tdie(_(\"push options must not have new line characters\"));\n> \n> -\trc = do_push(flags, push_options, remote);\n> +\trc = do_push(flags, push_options, remote, push_base);\n\n\nAnd passes that arbitrary cruft given by the user down to the\ntransport.\n\nAs the spirit of the \"base\" parameter is to tell the other side that\nit is what the receiving end believes to be common, shouldn't we\nmake sure we do have it on our side after getting it from the user\nwith OPT_STRING() before passing it down to the transport layer and\nhave the transport layer convert it to an object name? This patch\nassumes that running get_oid_hex() at the transport layer and\nassuming that the transport would keep working on the_repository\n(hence when we say \"We expect that the receiving end has 'master'\",\nthe transport somehow knows that is 'master' in our repository, not\nin a submodule repository, for example), but by converting it to\nfull object name early, we do not have to assume transport to stay\nthat way.\n"},{"id":"409337","messageId":"xmqqzh3rd4sd.fsf@gitster.c.googlers.com","threadId":"54560","inReplyTo":"20201103102307.GI24813@szeder.dev","subject":"Re: [PATCH 2/4] push: teach --base for ssh:// and file://","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-11-08T19:31:14Z","receivedAt":"2020-11-08T19:34:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor writes:\n\n>> +\t# Server does not have \"six\".\n>> +\ttest_must_fail git -C file_child -c protocol.version=0 \\\n>> +\t\tpush --base=an_invalid_object origin HEAD:client_branch_six 2>log &&\n>> +\tgrep \"is not a valid object\" log\n>\n> This should rather use 'test_i18ngrep' ...\n>\n>> +'\n>> +\n>> +test_expect_success 'push with --base that does not exist on server' '\n>> +\tCOMMON_HASH=$(git -C file_child rev-parse six) &&\n>> +\n>> +\t# The push still succeeds.\n>> +\tGIT_TRACE_PACKET=1 git -C file_child -c protocol.version=0 \\\n>> +\t\tpush --base=six origin HEAD:client_branch_six 2>log &&\n>> +\n>> +\t# Server did not advertise \"six\", since it does not know it\n>> +\t! grep \"$COMMON_HASH .have\" log\n>> +'\n>> +\n>> # Test protocol v1 with 'ssh://' transport\n>> #\n>> test_expect_success 'setup ssh wrapper' '\n>\n>\n>> diff --git a/transport.c b/transport.c\n>> index ffe2115845..531ca0a834 100644\n>> --- a/transport.c\n>> +++ b/transport.c\n>> @@ -236,6 +236,10 @@ static int set_git_option(struct git_transport_options *opts,\n>> \t\tlist_objects_filter_die_if_populated(&opts->filter_options);\n>> \t\tparse_list_objects_filter(&opts->filter_options, value);\n>> \t\treturn 0;\n>> +\t} else if (!strcmp(name, TRANS_OPT_PUSH_BASE)) {\n>> +\t\tif (get_oid(value, &opts->push_base))\n>> +\t\t\tdie(_(\"transport: '%s' is not a valid object\"), value);\n>\n> ... because the error message here is translated.\n>\n>> +\t\treturn 0;\n>> \t}\n>> \treturn 1;\n>> }\n\nYes, and we are getting CI failure ever since we queued this patch.\n\nLet's discard the topic for now.\n\nThanks.\n"},{"id":"409408","messageId":"20201109195630.2511149-1-jonathantanmy@google.com","threadId":"54560","inReplyTo":"xmqqft5qs5qs.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 4/4] Doc: push with --base","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-11-09T19:56:30Z","receivedAt":"2020-11-09T19:56:38Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> People expect that a single repository at their hosting sites can be\n> used as the central meeting point for the project, just like CVS/SVN\n> servers were in older world. \"git push\" would need to accept that\n> reality and start common ancestor discovery eventually.\n\nThanks for your reply (and everyone else's). I was thinking that a more\nrudimentary form of the feature would suffice, since I wasn't expecting\nmuch more need in the future, but looks like this isn't the case. I'll\nbe thinking of a more comprehensive idea.\n"},{"id":"409412","messageId":"d72de441-e7f4-347d-e37a-e8e530855821@gmail.com","threadId":"54560","inReplyTo":"20201109195630.2511149-1-jonathantanmy@google.com","subject":"Re: [PATCH 4/4] Doc: push with --base","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-11-09T21:00:55Z","receivedAt":"2020-11-09T21:01:01Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 11/9/20 2:56 PM, Jonathan Tan wrote:\n>> People expect that a single repository at their hosting sites can be\n>> used as the central meeting point for the project, just like CVS/SVN\n>> servers were in older world. \"git push\" would need to accept that\n>> reality and start common ancestor discovery eventually.\n> \n> Thanks for your reply (and everyone else's). I was thinking that a more\n> rudimentary form of the feature would suffice, since I wasn't expecting\n> much more need in the future, but looks like this isn't the case. I'll\n> be thinking of a more comprehensive idea.\n\nI think this \"half round negotiation\" idea you have has merit, and can\nget us 95% of the benefit that a multi-round negotiation would bring\nwithout those extra steps.\n\nMy concerns with the current series is that it isn't fully ready for\neven that case. In my mind, a protocol change like this would need:\n\n1. A top-to-bottom implementation that allows a user to opt-in to\n this new behavior with a config setting.\n\n2. A demonstration of situations where this algorithm out-performs\n the existing algorithm (i.e. client is far behind server, but\n topic is a small change based on something in server's history)\n\n3. A clear way to handle odd cases, such as multiple merge-bases.\n This leads to a change in how you are sending the data.\n\nPerhaps this \"send multiple OIDs in a payload\" is already half-way to\nimplementing a full negotiation, and we might as well go all the way\nin the writing. I expect that sending all maximal merge-bases will be\nsufficient for the vast majority of cases, and so any multi-round\nnegotiation process will almost always end after the client sends that\ndata.\n\nLooking forward to your next version.\n\nThanks,\n-Stolee\n"},{"id":"409414","messageId":"xmqq8sbab52p.fsf@gitster.c.googlers.com","threadId":"54560","inReplyTo":"20201109195630.2511149-1-jonathantanmy@google.com","subject":"Re: [PATCH 4/4] Doc: push with --base","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-11-09T21:20:14Z","receivedAt":"2020-11-09T21:20:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n>> People expect that a single repository at their hosting sites can be\n>> used as the central meeting point for the project, just like CVS/SVN\n>> servers were in older world. \"git push\" would need to accept that\n>> reality and start common ancestor discovery eventually.\n>\n> Thanks for your reply (and everyone else's). I was thinking that a more\n> rudimentary form of the feature would suffice, since I wasn't expecting\n> much more need in the future, but looks like this isn't the case. I'll\n> be thinking of a more comprehensive idea.\n\nI said \"eventually\", meaning that we may not have to solve it\nimmediately, but judging from the need for ad-hoc workarounds like\nsending older commits that are not necessarily at the tip of\nanything from the receiving end as if they are tips and then another\nad-hoc workaround like this one, it seems that the need is real.\n\nWould the earlier refactoring of the negotiation part into a\nseparate negotiator module help, or did the refactor not remove the\ndeep assumption that it is only about the fetch/upload-pack traffic\nand we need a design for push/receive-pack from scratch?\n\nThanks.\n"},{"id":"409415","messageId":"20201109214011.GA668551@coredump.intra.peff.net","threadId":"54560","inReplyTo":"xmqq8sbab52p.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 4/4] Doc: push with --base","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-11-09T21:40:11Z","receivedAt":"2020-11-09T21:40:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 09, 2020 at 01:20:14PM -0800, Junio C Hamano wrote:\n\n> Would the earlier refactoring of the negotiation part into a\n> separate negotiator module help, or did the refactor not remove the\n> deep assumption that it is only about the fetch/upload-pack traffic\n> and we need a design for push/receive-pack from scratch?\n\nI haven't thought too deeply about this problem area, but there is one\nchallenge I'd expect with reusing the existing negotiation protocol and\ncode: the stateless side is flipped for http.\n\nIn the fetch protocol, the client is stateful and makes a series of\nrequests to a stateless server, each time saying \"I want X, we agreed\npreviously on Y, and here are some more options Z; let me know if that's\nenough to generate a pack\". If we were to flip that around, the client\nmust remain the stateful party, but it's no longer the receiver of the\npack. For the receiver (i.e., the stateless server) to iterate through\nits history looking for a shard point, I think the message would have to\nbe more like \"last time you told me about X, and gave me cut-points Y to\nresume your traversal; tell me more about Y so that I can decide if\nwe're ready to send a pack\".\n\n-Peff\n"},{"id":"409424","messageId":"20201109222212.2544841-1-jonathantanmy@google.com","threadId":"54560","inReplyTo":"d72de441-e7f4-347d-e37a-e8e530855821@gmail.com","subject":"Re: [PATCH 4/4] Doc: push with --base","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-11-09T22:22:12Z","receivedAt":"2020-11-09T22:22:17Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> On 11/9/20 2:56 PM, Jonathan Tan wrote:\n> >> People expect that a single repository at their hosting sites can be\n> >> used as the central meeting point for the project, just like CVS/SVN\n> >> servers were in older world. \"git push\" would need to accept that\n> >> reality and start common ancestor discovery eventually.\n> > \n> > Thanks for your reply (and everyone else's). I was thinking that a more\n> > rudimentary form of the feature would suffice, since I wasn't expecting\n> > much more need in the future, but looks like this isn't the case. I'll\n> > be thinking of a more comprehensive idea.\n> \n> I think this \"half round negotiation\" idea you have has merit, and can\n> get us 95% of the benefit that a multi-round negotiation would bring\n> without those extra steps.\n\nThanks.\n\n> My concerns with the current series is that it isn't fully ready for\n> even that case. In my mind, a protocol change like this would need:\n> \n> 1. A top-to-bottom implementation that allows a user to opt-in to\n> this new behavior with a config setting.\n\nMakes sense. The client already needs to opt-in by specifying the base,\nbut yes, an option needs to be added for the server.\n\n> 2. A demonstration of situations where this algorithm out-performs\n> the existing algorithm (i.e. client is far behind server, but\n> topic is a small change based on something in server's history)\n\nNoted.\n\n> 3. A clear way to handle odd cases, such as multiple merge-bases.\n> This leads to a change in how you are sending the data.\n> \n> Perhaps this \"send multiple OIDs in a payload\" is already half-way to\n> implementing a full negotiation, and we might as well go all the way\n> in the writing. I expect that sending all maximal merge-bases will be\n> sufficient for the vast majority of cases, and so any multi-round\n> negotiation process will almost always end after the client sends that\n> data.\n\nI think the ability to send multiple bases would work in this scenario,\nbut this might run into concerns that other reviewers have brought up\n(specifically, the fact that we're using environment variables and HTTP\nheaders to pass this information - and among other things, line length\nlimits). So a more comprehensive solution might be needed, even if only\nfor this reason.\n\n> Looking forward to your next version.\n\nThanks.\n"},{"id":"409431","messageId":"20201109224732.2549561-1-jonathantanmy@google.com","threadId":"54560","inReplyTo":"xmqq8sbab52p.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 4/4] Doc: push with --base","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-11-09T22:47:32Z","receivedAt":"2020-11-09T22:47:37Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> Jonathan Tan writes:\n> \n> >> People expect that a single repository at their hosting sites can be\n> >> used as the central meeting point for the project, just like CVS/SVN\n> >> servers were in older world. \"git push\" would need to accept that\n> >> reality and start common ancestor discovery eventually.\n> >\n> > Thanks for your reply (and everyone else's). I was thinking that a more\n> > rudimentary form of the feature would suffice, since I wasn't expecting\n> > much more need in the future, but looks like this isn't the case. I'll\n> > be thinking of a more comprehensive idea.\n> \n> I said \"eventually\", meaning that we may not have to solve it\n> immediately, but judging from the need for ad-hoc workarounds like\n> sending older commits that are not necessarily at the tip of\n> anything from the receiving end as if they are tips and then another\n> ad-hoc workaround like this one, it seems that the need is real.\n\nThat's true. And if we need a more comprehensive solution than putting\nbases as Extra Parameters (and don't think that base-in-Extra-Parameter\nwill form a part of that comprehensive solution), then I do think that\nwe should go for the comprehensive solution instead of making something\nobsolete that we will still need to support later.\n\n> Would the earlier refactoring of the negotiation part into a\n> separate negotiator module help, or did the refactor not remove the\n> deep assumption that it is only about the fetch/upload-pack traffic\n> and we need a design for push/receive-pack from scratch?\n\nThe negotiator module might help - looking that the API, it takes a list\nof known commons (none in this case, because we want to skip the long\nref advertisement) and a list of tips to check (for this case, the tips\nwe want to push), and then the negotiation starts. So we might be able\nto use it in a push v2.\n\nHaving said that, it might be possible to reuse the existing fetch v0/v2\nprotocol to perform this negotiation (preferably v2, so that we can skip\nthe ref advertisement). We'll just need to add the opposite of the\n\"no-done\" capability to make sure that the server never sends a\npackfile.\n\nOnce we have found the base commit(s) either through push v2 or through\nfetch+opposite-of-no-done, we'll need to send the packfile somehow. My\nproposal here could do it, although then we might run into the problems\nPeff describes about Extra Parameters [1]. If we don't use Extra\nParameters, we would probably need a push v2, but we might then run into\nsimilar problems to what we had during the fetch v2 migration (e.g.\nunexpected subtly different behaviors).\n\n[1] https://lore.kernel.org/git/20201103151859.GA444466@coredump.intra.peff.net/\n"}]}