{"thread":{"id":"49942","subject":"[WIP RFC 0/5] Design for offloading part of packfile response to CDN","startedAt":"2018-12-03T23:37:46Z","lastAt":"2019-02-22T11:35:15Z","messageCount":30,"participants":["Jonathan Tan","Stefan Beller","brian m. carlson","Junio C Hamano","Christian Couder","Ævar Arnfjörð Bjarmason"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"364503","messageId":"cover.1543879256.git.jonathantanmy@google.com","threadId":"49942","inReplyTo":null,"subject":"[WIP RFC 0/5] Design for offloading part of packfile response to CDN","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-12-03T23:37:33Z","receivedAt":"2018-12-03T23:37:46Z","isPatch":false,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Some of us have been working on a design to improve the scalability of\nGit servers by allowing them to offload part of the packfile response to\nCDNs in this way: returning HTTP(S) URIs in fetch responses in addition\nto packfiles.\n\nThis can reduce the load on individual Git servers and improves\nproximity (by having data served from closer to the user).\n\nI have included here a design document (patch 2) and a rough\nimplementation of the server (patch 5). Currently, the implementation\nonly allows replacing single blobs with URIs, but the protocol\nimprovement is designed in such a way as to allow independent\nimprovement of Git server implementations.\n\nThere is a potential issue: a server which produces both the URIs and\nthe packfile at roughly the same time (like the implementation in this\npatch set) will not have sideband access until it has concluded sending\nthe URIs. Among other things, this means that the server cannot send\nkeepalive packets until quite late in the response. One solution to this\nmight be to add a feature that allows the server to use a sideband\nthroughout the whole response - and this has other benefits too like\nallowing servers to inform the client throughout the whole fetch, not\njust at the end.\n\nJonathan Tan (5):\n Documentation: order protocol v2 sections\n Documentation: add Packfile URIs design doc\n upload-pack: refactor reading of pack-objects out\n upload-pack: refactor writing of \"packfile\" line\n upload-pack: send part of packfile response as uri\n\n Documentation/technical/packfile-uri.txt | 83 +++++++++++++\n Documentation/technical/protocol-v2.txt | 22 ++--\n builtin/pack-objects.c | 48 ++++++++\n fetch-pack.c | 9 ++\n t/t5702-protocol-v2.sh | 25 ++++\n upload-pack.c | 150 ++++++++++++++++-------\n 6 files changed, 285 insertions(+), 52 deletions(-)\n create mode 100644 Documentation/technical/packfile-uri.txt\n\n-- \n2.19.0.271.gfe8321ec05.dirty\n\n"},{"id":"364504","messageId":"061dacf51763bd2f72b95c382e08f44655e247a7.1543879256.git.jonathantanmy@google.com","threadId":"49942","inReplyTo":"cover.1543879256.git.jonathantanmy@google.com","subject":"[WIP RFC 1/5] Documentation: order protocol v2 sections","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-12-03T23:37:34Z","receivedAt":"2018-12-03T23:37:48Z","isPatch":false,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"The git command line expects Git servers to follow a specific order of\nsections when transmitting protocol v2 responses, but this is not\nexplicit in the documentation. Make the order explicit.\n\nSigned-off-by: Jonathan Tan \n---\n Documentation/technical/protocol-v2.txt | 18 ++++++++----------\n 1 file changed, 8 insertions(+), 10 deletions(-)\n\ndiff --git a/Documentation/technical/protocol-v2.txt b/Documentation/technical/protocol-v2.txt\nindex 09e4e0273f..345c00e08c 100644\n--- a/Documentation/technical/protocol-v2.txt\n+++ b/Documentation/technical/protocol-v2.txt\n@@ -309,11 +309,11 @@ the 'wanted-refs' section in the server's response as explained below.\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.\n+header. Most sections are sent only when the packfile is sent.\n \n- output = *section\n- section = (acknowledgments | shallow-info | wanted-refs | packfile)\n-\t (flush-pkt | delim-pkt)\n+ output = acknowledgements flush-pkt |\n+\t [acknowledgments delim-pkt] [shallow-info delim-pkt]\n+\t [wanted-refs delim-pkt] packfile flush-pkt\n \n acknowledgments = PKT-LINE(\"acknowledgments\" LF)\n \t\t (nak | *ack)\n@@ -335,9 +335,10 @@ header.\n \t *PKT-LINE(%x01-03 *%x00-ff)\n \n acknowledgments section\n-\t* If the client determines that it is finished with negotiations\n-\t by sending a \"done\" line, the acknowledgments sections MUST be\n-\t omitted from the server's response.\n+\t* If the client determines that it is finished with negotiations by\n+\t sending a \"done\" line (thus requiring the server to send a packfile),\n+\t the acknowledgments sections MUST be omitted from the server's\n+\t response.\n \n \t* Always begins with the section header \"acknowledgments\"\n \n@@ -388,9 +389,6 @@ header.\n \t which the client has not indicated was shallow as a part of\n \t its request.\n \n-\t* This section is only included if a packfile section is also\n-\t included in the response.\n-\n wanted-refs section\n \t* This section is only included if the client has requested a\n \t ref using a 'want-ref' line and if a packfile section is also\n-- \n2.19.0.271.gfe8321ec05.dirty\n\n"},{"id":"364505","messageId":"0461b362569362c6d0e73951469c547a03a1b59d.1543879256.git.jonathantanmy@google.com","threadId":"49942","inReplyTo":"cover.1543879256.git.jonathantanmy@google.com","subject":"[WIP RFC 2/5] Documentation: add Packfile URIs design doc","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-12-03T23:37:35Z","receivedAt":"2018-12-03T23:37:51Z","isPatch":false,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Signed-off-by: Jonathan Tan \n---\n Documentation/technical/packfile-uri.txt | 83 ++++++++++++++++++++++++\n Documentation/technical/protocol-v2.txt | 6 +-\n 2 files changed, 88 insertions(+), 1 deletion(-)\n create mode 100644 Documentation/technical/packfile-uri.txt\n\ndiff --git a/Documentation/technical/packfile-uri.txt b/Documentation/technical/packfile-uri.txt\nnew file mode 100644\nindex 0000000000..6535801486\n--- /dev/null\n+++ b/Documentation/technical/packfile-uri.txt\n@@ -0,0 +1,83 @@\n+Packfile URIs\n+=============\n+\n+This feature allows servers to serve part of their packfile response as URIs.\n+This allows server designs that improve scalability in bandwidth and CPU usage\n+(for example, by serving some data through a CDN), and (in the future) provides\n+some measure of resumability to clients.\n+\n+This feature is available only in protocol version 2.\n+\n+Protocol\n+--------\n+\n+The server advertises `packfile-uris`.\n+\n+If the client replies with the following arguments:\n+\n+ * packfile-uris\n+ * thin-pack\n+ * ofs-delta\n+\n+when the server sends the packfile, it MAY send a `packfile-uris` section\n+directly before the `packfile` section (right after `wanted-refs` if it is\n+sent) containing HTTP(S) URIs. See protocol-v2.txt for the documentation of\n+this section.\n+\n+Clients then should understand that the returned packfile could be incomplete,\n+and that it needs to download all the given URIs before the fetch or clone is\n+complete. Each URI should point to a Git packfile (which may be a thin pack and\n+which may contain offset deltas).\n+\n+Server design\n+-------------\n+\n+The server can be trivially made compatible with the proposed protocol by\n+having it advertise `packfile-uris`, tolerating the client sending\n+`packfile-uris`, and never sending any `packfile-uris` section. But we should\n+include some sort of non-trivial implementation in the Minimum Viable Product,\n+at least so that we can test the client.\n+\n+This is the implementation: a feature, marked experimental, that allows the\n+server to be configured by one or more `uploadpack.blobPackfileUri=\n+` entries. Whenever the list of objects to be sent is assembled, a blob\n+with the given sha1 can be replaced by the given URI. This allows, for example,\n+servers to delegate serving of large blobs to CDNs.\n+\n+Client design\n+-------------\n+\n+While fetching, the client needs to remember the list of URIs and cannot\n+declare that the fetch is complete until all URIs have been downloaded as\n+packfiles.\n+\n+The division of work (initial fetch + additional URIs) introduces convenient\n+points for resumption of an interrupted clone - such resumption can be done\n+after the Minimum Viable Product (see \"Future work\").\n+\n+The client can inhibit this feature (i.e. refrain from sending the\n+`packfile-urls` parameter) by passing --no-packfile-urls to `git fetch`.\n+\n+Future work\n+-----------\n+\n+The protocol design allows some evolution of the server and client without any\n+need for protocol changes, so only a small-scoped design is included here to\n+form the MVP. For example, the following can be done:\n+\n+ * On the server, a long-running process that takes in entire requests and\n+ outputs a list of URIs and the corresponding inclusion and exclusion sets of\n+ objects. This allows, e.g., signed URIs to be used and packfiles for common\n+ requests to be cached.\n+ * On the client, resumption of clone. If a clone is interrupted, information\n+ could be recorded in the repository's config and a \"clone-resume\" command\n+ can resume the clone in progress. (Resumption of subsequent fetches is more\n+ difficult because that must deal with the user wanting to use the repository\n+ even after the fetch was interrupted.)\n+\n+There are some possible features that will require a change in protocol:\n+\n+ * Additional HTTP headers (e.g. authentication)\n+ * Byte range support\n+ * Different file formats referenced by URIs (e.g. raw object)\n+\ndiff --git a/Documentation/technical/protocol-v2.txt b/Documentation/technical/protocol-v2.txt\nindex 345c00e08c..2cb1c41742 100644\n--- a/Documentation/technical/protocol-v2.txt\n+++ b/Documentation/technical/protocol-v2.txt\n@@ -313,7 +313,8 @@ header. Most sections are sent only when the packfile is sent.\n \n output = acknowledgements flush-pkt |\n \t [acknowledgments delim-pkt] [shallow-info delim-pkt]\n-\t [wanted-refs delim-pkt] packfile flush-pkt\n+\t [wanted-refs delim-pkt] [packfile-uris delim-pkt]\n+\t packfile flush-pkt\n \n acknowledgments = PKT-LINE(\"acknowledgments\" LF)\n \t\t (nak | *ack)\n@@ -331,6 +332,9 @@ header. Most sections are sent only when the packfile is sent.\n \t\t *PKT-LINE(wanted-ref LF)\n wanted-ref = obj-id SP refname\n \n+ packfile-uris = PKT-LINE(\"packfile-uris\" LF) *packfile-uri\n+ packfile-uri = PKT-LINE(\"uri\" SP *%x20-ff LF)\n+\n packfile = PKT-LINE(\"packfile\" LF)\n \t *PKT-LINE(%x01-03 *%x00-ff)\n \n-- \n2.19.0.271.gfe8321ec05.dirty\n\n"},{"id":"364506","messageId":"e19f294df9ff999d30a47339a7848c7104bfae7d.1543879256.git.jonathantanmy@google.com","threadId":"49942","inReplyTo":"cover.1543879256.git.jonathantanmy@google.com","subject":"[WIP RFC 3/5] upload-pack: refactor reading of pack-objects out","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-12-03T23:37:36Z","receivedAt":"2018-12-03T23:37:53Z","isPatch":false,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Subsequent patches will change how the output of pack-objects is\nprocessed, so extract that processing into its own function.\n\nCurrently, at most 1 character can be buffered (in the \"buffered\" local\nvariable). One of those patches will require a larger buffer, so replace\nthat \"buffered\" local variable with a buffer array.\n\nSigned-off-by: Jonathan Tan \n---\n upload-pack.c | 80 +++++++++++++++++++++++++++++----------------------\n 1 file changed, 46 insertions(+), 34 deletions(-)\n\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 5e81f1ff24..cec43e0a46 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -101,14 +101,51 @@ static int write_one_shallow(const struct commit_graft *graft, void *cb_data)\n \treturn 0;\n }\n \n+struct output_state {\n+\tchar buffer[8193];\n+\tint used;\n+};\n+\n+static int read_pack_objects_stdout(int outfd, struct output_state *os)\n+{\n+\t/* Data ready; we keep the last byte to ourselves\n+\t * in case we detect broken rev-list, so that we\n+\t * can leave the stream corrupted. This is\n+\t * unfortunate -- unpack-objects would happily\n+\t * accept a valid packdata with trailing garbage,\n+\t * so appending garbage after we pass all the\n+\t * pack data is not good enough to signal\n+\t * breakage to downstream.\n+\t */\n+\tssize_t readsz;\n+\n+\treadsz = xread(outfd, os->buffer + os->used,\n+\t\t sizeof(os->buffer) - os->used);\n+\tif (readsz < 0) {\n+\t\treturn readsz;\n+\t}\n+\tos->used += readsz;\n+\n+\tif (os->used > 1) {\n+\t\tsend_client_data(1, os->buffer, os->used - 1);\n+\t\tos->buffer[0] = os->buffer[os->used - 1];\n+\t\tos->used = 1;\n+\t} else {\n+\t\tsend_client_data(1, os->buffer, os->used);\n+\t\tos->used = 0;\n+\t}\n+\n+\treturn readsz;\n+}\n+\n static void create_pack_file(const struct object_array *have_obj,\n \t\t\t const struct object_array *want_obj)\n {\n \tstruct child_process pack_objects = CHILD_PROCESS_INIT;\n-\tchar data[8193], progress[128];\n+\tstruct output_state output_state = {0};\n+\tchar progress[128];\n \tchar abort_msg[] = \"aborting due to possible repository \"\n \t\t\"corruption on the remote side.\";\n-\tint buffered = -1;\n \tssize_t sz;\n \tint i;\n \tFILE *pipe_fd;\n@@ -235,39 +272,15 @@ static void create_pack_file(const struct object_array *have_obj,\n \t\t\tcontinue;\n \t\t}\n \t\tif (0 <= pu && (pfd[pu].revents & (POLLIN|POLLHUP))) {\n-\t\t\t/* Data ready; we keep the last byte to ourselves\n-\t\t\t * in case we detect broken rev-list, so that we\n-\t\t\t * can leave the stream corrupted. This is\n-\t\t\t * unfortunate -- unpack-objects would happily\n-\t\t\t * accept a valid packdata with trailing garbage,\n-\t\t\t * so appending garbage after we pass all the\n-\t\t\t * pack data is not good enough to signal\n-\t\t\t * breakage to downstream.\n-\t\t\t */\n-\t\t\tchar *cp = data;\n-\t\t\tssize_t outsz = 0;\n-\t\t\tif (0 <= buffered) {\n-\t\t\t\t*cp++ = buffered;\n-\t\t\t\toutsz++;\n-\t\t\t}\n-\t\t\tsz = xread(pack_objects.out, cp,\n-\t\t\t\t sizeof(data) - outsz);\n-\t\t\tif (0 < sz)\n-\t\t\t\t;\n-\t\t\telse if (sz == 0) {\n+\t\t\tint result = read_pack_objects_stdout(pack_objects.out,\n+\t\t\t\t\t\t\t &output_state);\n+\n+\t\t\tif (result == 0) {\n \t\t\t\tclose(pack_objects.out);\n \t\t\t\tpack_objects.out = -1;\n-\t\t\t}\n-\t\t\telse\n+\t\t\t} else if (result < 0) {\n \t\t\t\tgoto fail;\n-\t\t\tsz += outsz;\n-\t\t\tif (1 < sz) {\n-\t\t\t\tbuffered = data[sz-1] & 0xFF;\n-\t\t\t\tsz--;\n \t\t\t}\n-\t\t\telse\n-\t\t\t\tbuffered = -1;\n-\t\t\tsend_client_data(1, data, sz);\n \t\t}\n \n \t\t/*\n@@ -292,9 +305,8 @@ static void create_pack_file(const struct object_array *have_obj,\n \t}\n \n \t/* flush the data */\n-\tif (0 <= buffered) {\n-\t\tdata[0] = buffered;\n-\t\tsend_client_data(1, data, 1);\n+\tif (output_state.used > 0) {\n+\t\tsend_client_data(1, output_state.buffer, output_state.used);\n \t\tfprintf(stderr, \"flushed.\\n\");\n \t}\n \tif (use_sideband)\n-- \n2.19.0.271.gfe8321ec05.dirty\n\n"},{"id":"364507","messageId":"1d678a78a63b7e58988b52c8c0efab38c34a6848.1543879256.git.jonathantanmy@google.com","threadId":"49942","inReplyTo":"cover.1543879256.git.jonathantanmy@google.com","subject":"[WIP RFC 4/5] upload-pack: refactor writing of \"packfile\" line","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-12-03T23:37:37Z","receivedAt":"2018-12-03T23:37:55Z","isPatch":false,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"A subsequent patch allows pack-objects to output additional information\n(in addition to the packfile that it currently outputs). This means that\nwe must hold off on writing the \"packfile\" section header to the client\nbefore we process the output of pack-objects, so move the writing of\nthe \"packfile\" section header to read_pack_objects_stdout().\n\nUnfortunately, this also means that we cannot send keepalive packets\nuntil pack-objects starts sending out the packfile, since the sideband\nis only established when the \"packfile\" section header is sent.\n\nSigned-off-by: Jonathan Tan \n---\n upload-pack.c | 47 ++++++++++++++++++++++++++++++++++++-----------\n 1 file changed, 36 insertions(+), 11 deletions(-)\n\ndiff --git a/upload-pack.c b/upload-pack.c\nindex cec43e0a46..aa2589b858 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -104,9 +104,12 @@ static int write_one_shallow(const struct commit_graft *graft, void *cb_data)\n struct output_state {\n \tchar buffer[8193];\n \tint used;\n+\tunsigned packfile_started : 1;\n+\tstruct strbuf progress_buf;\n };\n \n-static int read_pack_objects_stdout(int outfd, struct output_state *os)\n+static int read_pack_objects_stdout(int outfd, struct output_state *os,\n+\t\t\t\t int use_protocol_v2)\n {\n \t/* Data ready; we keep the last byte to ourselves\n \t * in case we detect broken rev-list, so that we\n@@ -126,6 +129,12 @@ static int read_pack_objects_stdout(int outfd, struct output_state *os)\n \t}\n \tos->used += readsz;\n \n+\tif (!os->packfile_started) {\n+\t\tos->packfile_started = 1;\n+\t\tif (use_protocol_v2)\n+\t\t\tpacket_write_fmt(1, \"packfile\\n\");\n+\t}\n+\n \tif (os->used > 1) {\n \t\tsend_client_data(1, os->buffer, os->used - 1);\n \t\tos->buffer[0] = os->buffer[os->used - 1];\n@@ -138,8 +147,17 @@ static int read_pack_objects_stdout(int outfd, struct output_state *os)\n \treturn readsz;\n }\n \n+static void flush_progress_buf(struct strbuf *progress_buf)\n+{\n+\tif (!progress_buf->len)\n+\t\treturn;\n+\tsend_client_data(2, progress_buf->buf, progress_buf->len);\n+\tstrbuf_reset(progress_buf);\n+}\n+\n static void create_pack_file(const struct object_array *have_obj,\n-\t\t\t const struct object_array *want_obj)\n+\t\t\t const struct object_array *want_obj,\n+\t\t\t int use_protocol_v2)\n {\n \tstruct child_process pack_objects = CHILD_PROCESS_INIT;\n \tstruct output_state output_state = {0};\n@@ -260,9 +278,13 @@ static void create_pack_file(const struct object_array *have_obj,\n \t\t\t */\n \t\t\tsz = xread(pack_objects.err, progress,\n \t\t\t\t sizeof(progress));\n-\t\t\tif (0 < sz)\n-\t\t\t\tsend_client_data(2, progress, sz);\n-\t\t\telse if (sz == 0) {\n+\t\t\tif (0 < sz) {\n+\t\t\t\tif (output_state.packfile_started)\n+\t\t\t\t\tsend_client_data(2, progress, sz);\n+\t\t\t\telse\n+\t\t\t\t\tstrbuf_add(&output_state.progress_buf,\n+\t\t\t\t\t\t progress, sz);\n+\t\t\t} else if (sz == 0) {\n \t\t\t\tclose(pack_objects.err);\n \t\t\t\tpack_objects.err = -1;\n \t\t\t}\n@@ -273,8 +295,10 @@ static void create_pack_file(const struct object_array *have_obj,\n \t\t}\n \t\tif (0 <= pu && (pfd[pu].revents & (POLLIN|POLLHUP))) {\n \t\t\tint result = read_pack_objects_stdout(pack_objects.out,\n-\t\t\t\t\t\t\t &output_state);\n-\n+\t\t\t\t\t\t\t &output_state,\n+\t\t\t\t\t\t\t use_protocol_v2);\n+\t\t\tif (output_state.packfile_started)\n+\t\t\t\tflush_progress_buf(&output_state.progress_buf);\n \t\t\tif (result == 0) {\n \t\t\t\tclose(pack_objects.out);\n \t\t\t\tpack_objects.out = -1;\n@@ -293,12 +317,14 @@ static void create_pack_file(const struct object_array *have_obj,\n \t\t * protocol to say anything, so those clients are just out of\n \t\t * luck.\n \t\t */\n-\t\tif (!ret && use_sideband) {\n+\t\tif (!ret && use_sideband && output_state.packfile_started) {\n \t\t\tstatic const char buf[] = \"0005\\1\";\n \t\t\twrite_or_die(1, buf, 5);\n \t\t}\n \t}\n \n+\tflush_progress_buf(&output_state.progress_buf);\n+\n \tif (finish_command(&pack_objects)) {\n \t\terror(\"git upload-pack: git-pack-objects died with error.\");\n \t\tgoto fail;\n@@ -1094,7 +1120,7 @@ void upload_pack(struct upload_pack_options *options)\n \tif (want_obj.nr) {\n \t\tstruct object_array have_obj = OBJECT_ARRAY_INIT;\n \t\tget_common_commits(&have_obj, &want_obj);\n-\t\tcreate_pack_file(&have_obj, &want_obj);\n+\t\tcreate_pack_file(&have_obj, &want_obj, 0);\n \t}\n }\n \n@@ -1475,8 +1501,7 @@ int upload_pack_v2(struct repository *r, struct argv_array *keys,\n \t\t\tsend_wanted_ref_info(&data);\n \t\t\tsend_shallow_info(&data, &want_obj);\n \n-\t\t\tpacket_write_fmt(1, \"packfile\\n\");\n-\t\t\tcreate_pack_file(&have_obj, &want_obj);\n+\t\t\tcreate_pack_file(&have_obj, &want_obj, 1);\n \t\t\tstate = FETCH_DONE;\n \t\t\tbreak;\n \t\tcase FETCH_DONE:\n-- \n2.19.0.271.gfe8321ec05.dirty\n\n"},{"id":"364508","messageId":"707a8181a0e8cd2e21989fa6da59cee38d9fadde.1543879256.git.jonathantanmy@google.com","threadId":"49942","inReplyTo":"cover.1543879256.git.jonathantanmy@google.com","subject":"[WIP RFC 5/5] upload-pack: send part of packfile response as uri","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-12-03T23:37:38Z","receivedAt":"2018-12-03T23:38:02Z","isPatch":false,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"This is a partial implementation of upload-pack sending part of its\npackfile response as URIs.\n\nThe client is not fully implemented - it knows to ignore the\n\"packfile-uris\" section, but because it does not actually fetch those\nURIs, the returned packfile is incomplete. A test is included to show\nthat the appropriate URI is indeed transmitted, and that the returned\npackfile is lacking exactly the expected object.\n\nSigned-off-by: Jonathan Tan \n---\n builtin/pack-objects.c | 48 ++++++++++++++++++++++++++++++++++++++++++\n fetch-pack.c | 9 ++++++++\n t/t5702-protocol-v2.sh | 25 ++++++++++++++++++++++\n upload-pack.c | 37 ++++++++++++++++++++++++++++----\n 4 files changed, 115 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex e7ea206c08..2abbddd3cb 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -117,6 +117,15 @@ enum missing_action {\n static enum missing_action arg_missing_action;\n static show_object_fn fn_show_object;\n \n+struct configured_exclusion {\n+\tstruct oidmap_entry e;\n+\tchar *uri;\n+};\n+static struct oidmap configured_exclusions;\n+\n+static int exclude_configured_blobs;\n+static struct oidset excluded_by_config;\n+\n /*\n * stats\n */\n@@ -831,6 +840,23 @@ static off_t write_reused_pack(struct hashfile *f)\n \treturn reuse_packfile_offset - sizeof(struct pack_header);\n }\n \n+static void write_excluded_by_configs(void)\n+{\n+\tstruct oidset_iter iter;\n+\tconst struct object_id *oid;\n+\n+\toidset_iter_init(&excluded_by_config, &iter);\n+\twhile ((oid = oidset_iter_next(&iter))) {\n+\t\tstruct configured_exclusion *ex =\n+\t\t\toidmap_get(&configured_exclusions, oid);\n+\n+\t\tif (!ex)\n+\t\t\tBUG(\"configured exclusion wasn't configured\");\n+\t\twrite_in_full(1, ex->uri, strlen(ex->uri));\n+\t\twrite_in_full(1, \"\\n\", 1);\n+\t}\n+}\n+\n static const char no_split_warning[] = N_(\n \"disabling bitmap writing, packs are split due to pack.packSizeLimit\"\n );\n@@ -1124,6 +1150,12 @@ static int want_object_in_pack(const struct object_id *oid,\n \t\t}\n \t}\n \n+\tif (exclude_configured_blobs &&\n+\t oidmap_get(&configured_exclusions, oid)) {\n+\t\toidset_insert(&excluded_by_config, oid);\n+\t\treturn 0;\n+\t}\n+\n \treturn 1;\n }\n \n@@ -2728,6 +2760,19 @@ static int git_pack_config(const char *k, const char *v, void *cb)\n \t\t\t pack_idx_opts.version);\n \t\treturn 0;\n \t}\n+\tif (!strcmp(k, \"uploadpack.blobpackfileuri\")) {\n+\t\tstruct configured_exclusion *ex = xmalloc(sizeof(*ex));\n+\t\tconst char *end;\n+\n+\t\tif (parse_oid_hex(v, &ex->e.oid, &end) || *end != ' ')\n+\t\t\tdie(_(\"value of uploadpack.blobpackfileuri must be \"\n+\t\t\t \"of the form ' ' (got '%s')\"), v);\n+\t\tif (oidmap_get(&configured_exclusions, &ex->e.oid))\n+\t\t\tdie(_(\"object already configured in another \"\n+\t\t\t \"uploadpack.blobpackfileuri (got '%s')\"), v);\n+\t\tex->uri = xstrdup(end + 1);\n+\t\toidmap_put(&configured_exclusions, ex);\n+\t}\n \treturn git_default_config(k, v, cb);\n }\n \n@@ -3314,6 +3359,8 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \t\t\t N_(\"do not pack objects in promisor packfiles\")),\n \t\tOPT_BOOL(0, \"delta-islands\", &use_delta_islands,\n \t\t\t N_(\"respect islands during delta compression\")),\n+\t\tOPT_BOOL(0, \"exclude-configured-blobs\", &exclude_configured_blobs,\n+\t\t\t N_(\"respect uploadpack.blobpackfileuri\")),\n \t\tOPT_END(),\n \t};\n \n@@ -3487,6 +3534,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \t\treturn 0;\n \tif (nr_result)\n \t\tprepare_pack(window, depth);\n+\twrite_excluded_by_configs();\n \twrite_pack_file();\n \tif (progress)\n \t\tfprintf_ln(stderr,\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 9691046e64..6e1985ab55 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -1413,6 +1413,15 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \t\t\t\treceive_wanted_refs(&reader, sought, nr_sought);\n \n \t\t\t/* get the pack */\n+\t\t\tif (process_section_header(&reader, \"packfile-uris\", 1)) {\n+\t\t\t\t/* skip the whole section */\n+\t\t\t\tprocess_section_header(&reader, \"packfile-uris\", 0);\n+\t\t\t\twhile (packet_reader_read(&reader) == PACKET_READ_NORMAL) {\n+\t\t\t\t\t/* do nothing */\n+\t\t\t\t}\n+\t\t\t\tif (reader.status != PACKET_READ_DELIM)\n+\t\t\t\t\tdie(\"expected DELIM\");\n+\t\t\t}\n \t\t\tprocess_section_header(&reader, \"packfile\", 0);\n \t\t\tif (get_pack(args, fd, pack_lockfile))\n \t\t\t\tdie(_(\"git fetch-pack: fetch failed.\"));\ndiff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh\nindex 0f2b09ebb8..ccb1fc510e 100755\n--- a/t/t5702-protocol-v2.sh\n+++ b/t/t5702-protocol-v2.sh\n@@ -588,6 +588,31 @@ test_expect_success 'when server does not send \"ready\", expect FLUSH' '\n \ttest_i18ngrep \"expected no other sections to be sent after no .ready.\" err\n '\n \n+test_expect_success 'part of packfile response provided as URI' '\n+\trm -rf \"$HTTPD_DOCUMENT_ROOT_PATH/http_parent\" http_child log &&\n+\n+\tgit init \"$HTTPD_DOCUMENT_ROOT_PATH/http_parent\" &&\n+\techo my-blob >\"$HTTPD_DOCUMENT_ROOT_PATH/http_parent/my-blob\" &&\n+\tgit -C \"$HTTPD_DOCUMENT_ROOT_PATH/http_parent/\" add my-blob &&\n+\tgit -C \"$HTTPD_DOCUMENT_ROOT_PATH/http_parent/\" commit -m x &&\n+\n+\tgit -C \"$HTTPD_DOCUMENT_ROOT_PATH/http_parent/\" hash-object my-blob >h &&\n+\tgit -C \"$HTTPD_DOCUMENT_ROOT_PATH/http_parent/\" config \\\n+\t\t\"uploadpack.blobpackfileuri\" \\\n+\t\t\"$(cat h) https://example.com/a-uri\" &&\n+\n+\t# NEEDSWORK: \"git clone\" fails here because it ignores the URI provided\n+\t# instead of fetching it.\n+\ttest_must_fail env GIT_TRACE_PACKET=\"$(pwd)/log\" \\\n+\t\tgit -c protocol.version=2 clone \\\n+\t\t\"$HTTPD_URL/smart/http_parent\" http_child 2>err &&\n+\t# Although \"git clone\" fails, we can still check that the server\n+\t# provided the URI we requested and that the error message pinpoints\n+\t# the object that is missing.\n+\tgrep \"clone< uri https://example.com/a-uri\" log &&\n+\ttest_i18ngrep \"did not receive expected object $(cat h)\" err\n+'\n+\n stop_httpd\n \n test_done\ndiff --git a/upload-pack.c b/upload-pack.c\nindex aa2589b858..a8eef697ec 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -104,6 +104,7 @@ static int write_one_shallow(const struct commit_graft *graft, void *cb_data)\n struct output_state {\n \tchar buffer[8193];\n \tint used;\n+\tunsigned packfile_uris_started : 1;\n \tunsigned packfile_started : 1;\n \tstruct strbuf progress_buf;\n };\n@@ -129,10 +130,35 @@ static int read_pack_objects_stdout(int outfd, struct output_state *os,\n \t}\n \tos->used += readsz;\n \n-\tif (!os->packfile_started) {\n-\t\tos->packfile_started = 1;\n-\t\tif (use_protocol_v2)\n-\t\t\tpacket_write_fmt(1, \"packfile\\n\");\n+\twhile (!os->packfile_started) {\n+\t\tchar *p;\n+\t\tif (os->used >= 4 && !memcmp(os->buffer, \"PACK\", 4)) {\n+\t\t\tos->packfile_started = 1;\n+\t\t\tif (use_protocol_v2) {\n+\t\t\t\tif (os->packfile_uris_started)\n+\t\t\t\t\tpacket_delim(1);\n+\t\t\t\tpacket_write_fmt(1, \"packfile\\n\");\n+\t\t\t}\n+\t\t\tbreak;\n+\t\t}\n+\t\tif ((p = memchr(os->buffer, '\\n', os->used))) {\n+\t\t\tif (!os->packfile_uris_started) {\n+\t\t\t\tos->packfile_uris_started = 1;\n+\t\t\t\tif (!use_protocol_v2)\n+\t\t\t\t\tBUG(\"packfile_uris requires protocol v2\");\n+\t\t\t\tpacket_write_fmt(1, \"packfile-uris\\n\");\n+\t\t\t}\n+\t\t\t*p = '\\0';\n+\t\t\tpacket_write_fmt(1, \"uri %s\\n\", os->buffer);\n+\n+\t\t\tos->used -= p - os->buffer + 1;\n+\t\t\tmemmove(os->buffer, p, os->used);\n+\t\t} else {\n+\t\t\t/*\n+\t\t\t * Incomplete line.\n+\t\t\t */\n+\t\t\treturn readsz;\n+\t\t}\n \t}\n \n \tif (os->used > 1) {\n@@ -205,6 +231,9 @@ static void create_pack_file(const struct object_array *have_obj,\n \t\t\t\t\t filter_options.filter_spec);\n \t\t}\n \t}\n+\tif (use_protocol_v2)\n+\t\targv_array_push(&pack_objects.args,\n+\t\t\t\t\"--exclude-configured-blobs\");\n \n \tpack_objects.in = -1;\n \tpack_objects.out = -1;\n-- \n2.19.0.271.gfe8321ec05.dirty\n\n"},{"id":"364511","messageId":"CAGZ79kajSr1jzs0r9bK+Pt6arz5XRq04cWaaPGU=BibDw1PHvA@mail.gmail.com","threadId":"49942","inReplyTo":"cover.1543879256.git.jonathantanmy@google.com","subject":"Re: [WIP RFC 0/5] Design for offloading part of packfile response to CDN","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-12-04T00:01:16Z","receivedAt":"2018-12-04T00:01:32Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Dec 3, 2018 at 3:37 PM Jonathan Tan wrote:\n\n>\n> There is a potential issue: a server which produces both the URIs and\n> the packfile at roughly the same time (like the implementation in this\n> patch set) will not have sideband access until it has concluded sending\n> the URIs. Among other things, this means that the server cannot send\n> keepalive packets until quite late in the response. One solution to this\n> might be to add a feature that allows the server to use a sideband\n> throughout the whole response - and this has other benefits too like\n> allowing servers to inform the client throughout the whole fetch, not\n> just at the end.\n\nWhile side band sounds like the right thing to do, we could also\nsending (NULL)-URIs within this feature.\n"},{"id":"364512","messageId":"CAGZ79kafqUcT96zzkEmhpUaUj7v83sdrGboSk-hrA8r-nC=ggQ@mail.gmail.com","threadId":"49942","inReplyTo":"0461b362569362c6d0e73951469c547a03a1b59d.1543879256.git.jonathantanmy@google.com","subject":"Re: [WIP RFC 2/5] Documentation: add Packfile URIs design doc","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-12-04T00:21:47Z","receivedAt":"2018-12-04T00:22:02Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Thanks for bringing this design to the list!\n\n> diff --git a/Documentation/technical/protocol-v2.txt b/Documentation/technical/protocol-v2.txt\n> index 345c00e08c..2cb1c41742 100644\n> --- a/Documentation/technical/protocol-v2.txt\n> +++ b/Documentation/technical/protocol-v2.txt\n> @@ -313,7 +313,8 @@ header. Most sections are sent only when the packfile is sent.\n>\n> output = acknowledgements flush-pkt |\n> [acknowledgments delim-pkt] [shallow-info delim-pkt]\n> - [wanted-refs delim-pkt] packfile flush-pkt\n> + [wanted-refs delim-pkt] [packfile-uris delim-pkt]\n> + packfile flush-pkt\n\nWhile this is an RFC and incomplete, we'd need to remember to\nadd packfile-uris to the capabilities list above, stating that it requires\nthin-pack and ofs-delta to be sent, and what to expect from it.\n\nThe mention of --no-packfile-urls in the Client design above\nseems to imply we'd want to turn it on by default, which I thought\nwas not the usual stance how we introduce new things.\n\nAn odd way of disabling it would be --no-thin-pack, hoping the\nclient side implementation abides by the implied requirements.\n\n> acknowledgments = PKT-LINE(\"acknowledgments\" LF)\n> (nak | *ack)\n> @@ -331,6 +332,9 @@ header. Most sections are sent only when the packfile is sent.\n> *PKT-LINE(wanted-ref LF)\n> wanted-ref = obj-id SP refname\n>\n> + packfile-uris = PKT-LINE(\"packfile-uris\" LF) *packfile-uri\n> + packfile-uri = PKT-LINE(\"uri\" SP *%x20-ff LF)\n\nIs the *%x20-ff a fancy way of saying obj-id?\n\nWhile the server is configured with pairs of (oid URL),\nwe would not need to send the exact oid to the client\nas that is what the client can figure out on its own by reading\nthe downloaded pack.\n\nInstead we could send an integrity hash (i.e. the packfile\ndownloaded from \"uri\" is expected to hash to $oid here)\n\nThanks,\nStefan\n"},{"id":"364513","messageId":"CAGZ79kZ3W3n7Y80ZLP5-eC2WSBBmC-ztx2z1zgzegvKXfD3r2g@mail.gmail.com","threadId":"49942","inReplyTo":"e19f294df9ff999d30a47339a7848c7104bfae7d.1543879256.git.jonathantanmy@google.com","subject":"Re: [WIP RFC 3/5] upload-pack: refactor reading of pack-objects out","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-12-04T00:30:46Z","receivedAt":"2018-12-04T00:31:01Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Dec 3, 2018 at 3:37 PM Jonathan Tan wrote:\n>\n> Subsequent patches will change how the output of pack-objects is\n> processed, so extract that processing into its own function.\n>\n> Currently, at most 1 character can be buffered (in the \"buffered\" local\n> variable). One of those patches will require a larger buffer, so replace\n> that \"buffered\" local variable with a buffer array.\n\nThis buffering sounds oddly similar to the pkt reader which can buffer\nat most one pkt, the difference being that we'd buffer bytes\ninstead of pkts.\n"},{"id":"364517","messageId":"20181204015446.GX890086@genre.crustytoothpaste.net","threadId":"49942","inReplyTo":"0461b362569362c6d0e73951469c547a03a1b59d.1543879256.git.jonathantanmy@google.com","subject":"Re: [WIP RFC 2/5] Documentation: add Packfile URIs design doc","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2018-12-04T01:54:46Z","receivedAt":"2018-12-04T01:54:55Z","isPatch":false,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Mon, Dec 03, 2018 at 03:37:35PM -0800, Jonathan Tan wrote:\n> Signed-off-by: Jonathan Tan \n> ---\n> Documentation/technical/packfile-uri.txt | 83 ++++++++++++++++++++++++\n> Documentation/technical/protocol-v2.txt | 6 +-\n> 2 files changed, 88 insertions(+), 1 deletion(-)\n> create mode 100644 Documentation/technical/packfile-uri.txt\n> \n> diff --git a/Documentation/technical/packfile-uri.txt b/Documentation/technical/packfile-uri.txt\n> new file mode 100644\n> index 0000000000..6535801486\n> --- /dev/null\n> +++ b/Documentation/technical/packfile-uri.txt\n> @@ -0,0 +1,83 @@\n> +Packfile URIs\n> +=============\n> +\n> +This feature allows servers to serve part of their packfile response as URIs.\n> +This allows server designs that improve scalability in bandwidth and CPU usage\n> +(for example, by serving some data through a CDN), and (in the future) provides\n> +some measure of resumability to clients.\n> +\n> +This feature is available only in protocol version 2.\n> +\n> +Protocol\n> +--------\n> +\n> +The server advertises `packfile-uris`.\n> +\n> +If the client replies with the following arguments:\n> +\n> + * packfile-uris\n> + * thin-pack\n> + * ofs-delta\n> +\n> +when the server sends the packfile, it MAY send a `packfile-uris` section\n> +directly before the `packfile` section (right after `wanted-refs` if it is\n> +sent) containing HTTP(S) URIs. See protocol-v2.txt for the documentation of\n> +this section.\n> +\n> +Clients then should understand that the returned packfile could be incomplete,\n> +and that it needs to download all the given URIs before the fetch or clone is\n> +complete. Each URI should point to a Git packfile (which may be a thin pack and\n> +which may contain offset deltas).\n\n\nSome thoughts here:\n\nFirst, I'd like to see a section (and a bit in the implementation)\nrequiring HTTPS if the original protocol is secure (SSH or HTTPS).\nAllowing the server to downgrade to HTTP, even by accident, would be a\nsecurity problem.\n\nSecond, this feature likely should be opt-in for SSH. One issue I've\nseen repeatedly is that people don't want to use HTTPS to fetch things\nwhen they're using SSH for Git. Many people in corporate environments\nhave proxies that break HTTP for non-browser use cases[0], and using SSH\nis the only way that they can make a functional Git connection.\n\nThird, I think the server needs to be required to both support Range\nheaders and never change the content of a URI, so that we can have\nresumable clone implicit in this design. There are some places in the\nworld where connections are poor and fetching even the initial packfile\nat once might be a problem. (I've seen such questions on Stack\nOverflow, for example.)\n\nHaving said that, I think overall this is a good idea and I'm glad to\nsee a proposal for it.\n\n[0] For example, a naughty-word filter may corrupt or block certain byte\nsequences that occur incidentally in the pack stream.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"364574","messageId":"20181204192904.40409-1-jonathantanmy@google.com","threadId":"49942","inReplyTo":"20181204015446.GX890086@genre.crustytoothpaste.net","subject":"Re: [WIP RFC 2/5] Documentation: add Packfile URIs design doc","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-12-04T19:29:04Z","receivedAt":"2018-12-04T19:29:10Z","isPatch":false,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> Some thoughts here:\n> \n> First, I'd like to see a section (and a bit in the implementation)\n> requiring HTTPS if the original protocol is secure (SSH or HTTPS).\n> Allowing the server to downgrade to HTTP, even by accident, would be a\n> security problem.\n> \n> Second, this feature likely should be opt-in for SSH. One issue I've\n> seen repeatedly is that people don't want to use HTTPS to fetch things\n> when they're using SSH for Git. Many people in corporate environments\n> have proxies that break HTTP for non-browser use cases[0], and using SSH\n> is the only way that they can make a functional Git connection.\n\nGood points about SSH support and the client needing to control which\nprotocols the server will send URIs for. I'll include a line in the\nclient request in which the client can specify which protocols it is OK\nwith.\n\n> Third, I think the server needs to be required to both support Range\n> headers and never change the content of a URI, so that we can have\n> resumable clone implicit in this design. There are some places in the\n> world where connections are poor and fetching even the initial packfile\n> at once might be a problem. (I've seen such questions on Stack\n> Overflow, for example.)\n\nGood points. I'll add these in the next revision.\n\n> Having said that, I think overall this is a good idea and I'm glad to\n> see a proposal for it.\n\nThanks, and thanks for your comments too.\n"},{"id":"364576","messageId":"CAGZ79kYhemdUjxQcOZ6ryofh6XddfFZB6zG29grQwstC=VeV-g@mail.gmail.com","threadId":"49942","inReplyTo":"707a8181a0e8cd2e21989fa6da59cee38d9fadde.1543879256.git.jonathantanmy@google.com","subject":"Re: [WIP RFC 5/5] upload-pack: send part of packfile response as uri","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-12-04T20:09:37Z","receivedAt":"2018-12-04T20:09:53Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Dec 3, 2018 at 3:38 PM Jonathan Tan wrote:\n>\n> This is a partial implementation of upload-pack sending part of its\n> packfile response as URIs.\n\nIt does so by implementing a new flag `--exclude-configured-blobs`\nin pack-objects, which would change the output of pack-objects to\noutput a list of URLs (of the excluded blobs) followed by the\npack to be asked for.\n\nThis design seems easy to implement as then upload-pack\ncan just parse the output and only needs to insert\n\"packfile\" and \"packfile-uris\\n\" at the appropriate places\nof the stream, otherwise it just passes through the information\nobtained from pack-objects.\n\nThe design as-is would make for hard documentation of\npack-objects (its output is not just a pack anymore when that\nflag is given, but a highly optimized byte stream).\n\nInitially I did not anticipate this to be one of the major design problems\nas I assumed we'd want to use this pack feature over broadly (e.g.\neventually by offloading most of the objects into a base pack that\nis just always included as the likelihood for any object in there is\nvery high on initial clone), but it makes total sense to only\noutput the URIs that we actually need.\n\nAn alternative that comes very close to the current situation\nwould be to either pass a file path or file descriptor (that upload-pack\nlistens to in parallel) to pack-objects as an argument of the new flag.\nThen we would not need to splice the protocol sections into the single\noutput stream, but we could announce the sections, then flush\nthe URIs and then flush the pack afterwards.\n\nI looked at this quickly, but that would need either extensions in\nrun-command.c for setting up the new fd for us, as there we already\nhave OS specific code for these setups, or we'd have to duplicate\nsome of the logic here, which doesn't enthuse me either.\n\nSo maybe we'd create a temp file via mkstemp and pass\nthe file name to pack-objects for writing the URIs and then\nwe'd just need to stream that file?\n\n> + # NEEDSWORK: \"git clone\" fails here because it ignores the URI provided\n> + # instead of fetching it.\n> + test_must_fail env GIT_TRACE_PACKET=\"$(pwd)/log\" \\\n> + git -c protocol.version=2 clone \\\n> + \"$HTTPD_URL/smart/http_parent\" http_child 2>err &&\n> + # Although \"git clone\" fails, we can still check that the server\n> + # provided the URI we requested and that the error message pinpoints\n> + # the object that is missing.\n> + grep \"clone< uri https://example.com/a-uri\" log &&\n> + test_i18ngrep \"did not receive expected object $(cat h)\" err\n\nThat is a nice test!\n"},{"id":"364598","messageId":"xmqqin08d36k.fsf@gitster-ct.c.googlers.com","threadId":"49942","inReplyTo":"061dacf51763bd2f72b95c382e08f44655e247a7.1543879256.git.jonathantanmy@google.com","subject":"Re: [WIP RFC 1/5] Documentation: order protocol v2 sections","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-12-05T04:10:11Z","receivedAt":"2018-12-05T04:10:17Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> The git command line expects Git servers to follow a specific order of\n\n\"Command line\"? It sounds like you are talking about the order of\ncommand line arguments and options, but apparently that is not what\nyou are doing. Is it \"The git over-the-wire protocol\"?\n\n> + output = acknowledgements flush-pkt |\n> +\t [acknowledgments delim-pkt] [shallow-info delim-pkt]\n> +\t [wanted-refs delim-pkt] packfile flush-pkt\n\nSo the output can be either \n\n - acks followed by flush (and nothing else) or\n\n - (possibly) acks, followed by (possibly) shallow, followed by\n (possibly) wanted-refs, followed by the pack stream and flush at\n the end.\n\n> @@ -335,9 +335,10 @@ header.\n> \t *PKT-LINE(%x01-03 *%x00-ff)\n> \n> acknowledgments section\n> -\t* If the client determines that it is finished with negotiations\n> -\t by sending a \"done\" line, the acknowledgments sections MUST be\n> -\t omitted from the server's response.\n> +\t* If the client determines that it is finished with negotiations by\n> +\t sending a \"done\" line (thus requiring the server to send a packfile),\n> +\t the acknowledgments sections MUST be omitted from the server's\n> +\t response.\n\nOK. \n\n> \t* Always begins with the section header \"acknowledgments\"\n> \n> @@ -388,9 +389,6 @@ header.\n> \t which the client has not indicated was shallow as a part of\n> \t its request.\n> \n> -\t* This section is only included if a packfile section is also\n> -\t included in the response.\n> -\n\nEarlier, we said that shallow-info is not given when packfile is not\nthere. That is captured in the updated EBNF above. We don't have a\ncorresponding removal of a bullet point for wanted-refs section below\nbut probably that is because the original did not have corresponding\nbullet point to begin with.\n\n> wanted-refs section\n> \t* This section is only included if the client has requested a\n> \t ref using a 'want-ref' line and if a packfile section is also\n"},{"id":"364604","messageId":"xmqqbm60d0s1.fsf@gitster-ct.c.googlers.com","threadId":"49942","inReplyTo":"0461b362569362c6d0e73951469c547a03a1b59d.1543879256.git.jonathantanmy@google.com","subject":"Re: [WIP RFC 2/5] Documentation: add Packfile URIs design doc","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-12-05T05:02:06Z","receivedAt":"2018-12-05T05:02:11Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> +This feature allows servers to serve part of their packfile response as URIs.\n> +This allows server designs that improve scalability in bandwidth and CPU usage\n> +(for example, by serving some data through a CDN), and (in the future) provides\n> +some measure of resumability to clients.\n\nWithout reading the remainder, this makes readers anticipate a few\ngood things ;-)\n\n - \"part of\", so pre-generated constant material can be given from\n CDN and then followed-up by \"filling the gaps\" small packfile,\n perhaps?\n\n - The \"part of\" transmission may not bring the repository up to\n date wrt to the \"want\" objects; would this feature involve \"you\n asked history up to these commits, but with this pack-uri, you'll\n be getting history up to these (somewhat stale) commits\"?\n\nAnyway, let's read on.\n\n> +This feature is available only in protocol version 2.\n> +\n> +Protocol\n> +--------\n> +\n> +The server advertises `packfile-uris`.\n> +\n> +If the client replies with the following arguments:\n> +\n> + * packfile-uris\n> + * thin-pack\n> + * ofs-delta\n\n\"with the following\" meaning \"with all of the following\", or \"with\nany of the following\"? Is there a reason why the server side must\nrequire that the client understands and is willing to accept a\nthin-pack when wanting to use packfile-uris? The same question for\nthe ofs-delta.\n\nWhen the pregenerated constant material the server plans to hand out\nthe uris for was prepared by using ofs-delta encoding, the server\ncannot give the uri to it when the client does not want ofs-delta\nencoded packfile, but it feels somewhat strange that we require the\nmost capable client at the protocol level. After all, the server\nside could prepare one with ofs-delta and another without ofs-delta\nand depending on what the client is capable of, hand out different\nURIs, if it wanted to.\n\nThe reason why I care is because thin and ofs will *NOT* stay\nforever be the only optional features of the pack format. We may\ninvent yet another such optional 'frotz' feature, which may greatly\nhelp the efficiency of the packfile encoding, hence it may be\npreferrable to always generate a CDN packfile with that feature, in\naddition to thin and ofs. Would we add 'frotz' to the above list in\nthe documentation, then? What would happen to existing servers and\nclients written before that time then?\n\nMy recommendation is to drop the mention of \"thin\" and \"ofs\" from\nthe above list, and also from the following paragraph. The \"it MAY\nsend\" will serve as a practical escape clause to allow a server/CDN\nimplementation that *ALWAYS* prepares pregenerated material that can\nonly be digested by clients that supports thin and ofs. Such a server\ncan send packfile-URIs only when all of the three are given by the\nclient and be compliant. And such an update to the proposed document\nwould allow a more diskful server to prepare both thin and non-thin\npregenerated packs and choose which one to give to the client depending\non the capability.\n\n> +when the server sends the packfile, it MAY send a `packfile-uris` section\n> +directly before the `packfile` section (right after `wanted-refs` if it is\n> +sent) containing HTTP(S) URIs. See protocol-v2.txt for the documentation of\n> +this section.\n\nSo, this is OK, but\n\n> +Clients then should understand that the returned packfile could be incomplete,\n> +and that it needs to download all the given URIs before the fetch or clone is\n> +complete. Each URI should point to a Git packfile (which may be a thin pack and\n> +which may contain offset deltas).\n\nweaken or remove the (parenthetical comment) in the last sentence,\nand replace the beginning of the section with something like\n\n\tIf the client replies with 'packfile-uris', when the server\n\tsends the packfile, it MAY send a `packfile-uris` section...\n\nYou may steal what I wrote in the above response to help the\nserver-side folks to decide how to actually implement the \"it MAY\nsend a packfile-uris\" part in the document.\n\n> +Server design\n> +-------------\n> +\n> +The server can be trivially made compatible with the proposed protocol by\n> +having it advertise `packfile-uris`, tolerating the client sending\n> +`packfile-uris`, and never sending any `packfile-uris` section. But we should\n> +include some sort of non-trivial implementation in the Minimum Viable Product,\n> +at least so that we can test the client.\n> +\n> +This is the implementation: a feature, marked experimental, that allows the\n> +server to be configured by one or more `uploadpack.blobPackfileUri=\n> +` entries. Whenever the list of objects to be sent is assembled, a blob\n> +with the given sha1 can be replaced by the given URI. This allows, for example,\n> +servers to delegate serving of large blobs to CDNs.\n\n;-)\n\n> +Client design\n> +-------------\n> +\n> +While fetching, the client needs to remember the list of URIs and cannot\n> +declare that the fetch is complete until all URIs have been downloaded as\n> +packfiles.\n> +\n> +The division of work (initial fetch + additional URIs) introduces convenient\n> +points for resumption of an interrupted clone - such resumption can be done\n> +after the Minimum Viable Product (see \"Future work\").\n> +\n> +The client can inhibit this feature (i.e. refrain from sending the\n> +`packfile-urls` parameter) by passing --no-packfile-urls to `git fetch`.\n\nOK, this comes back to what I alluded to at the beginning. We could\nrespond to a full-clone request by feeding a series of packfile-uris\nand some ref information, perhaps like this:\n\n\t* Grab this packfile and update your remote-tracking refs\n and tags to these values; you'd be as if you cloned the\n project when it was at v1.0.\n\n\t* When you are done with the above, grab this packfile and\n update your remote-tracking refs and tags to these values;\n you'd be as if you cloned the project when it was at v2.0.\n\n\t* When you are done with the above, grab this packfile and\n update your remote-tracking refs and tags to these values;\n you'd be as if you cloned the project when it was at v3.0.\n\n\t...\n\n\t* When you are done with the above, here is the remaining\n packdata to bring you fully up to date with your original\n \"want\"s.\n\nand before fully reading the proposal, I anticipated that it was\nwhat you were going to describe. The major difference is \"up to the\npackdata given to you so far, you'd be as if you fetched these\" ref\ninformation, which would allow you to be interrupted and then simply\nresume, without having to remember the set of packfile-uris yet to\nbe processed across a fetch/clone failure. If you sucessfully fetch\npackfile for ..v1.0, you can update the remote-tracking refs to\nmatch as if you fetched back when that was the most recent state of\nthe project, and then if you failed while transferring packfile for\nv1.0..v2.0, the resuming would just reissue \"git fetch\" internally.\n\nI think what you proposed, i.e. without the \"with the data up to\nthis packfile, you have history to these objects\", would also work,\neven though it requires us to remember more of what we learned\nduring the initial attempt throughout retrying failed transfers.\n\n> +Future work\n> +-----------\n> +\n> +The protocol design allows some evolution of the server and client without any\n> +need for protocol changes, so only a small-scoped design is included here to\n> +form the MVP. For example, the following can be done:\n> +\n> + * On the server, a long-running process that takes in entire requests and\n> + outputs a list of URIs and the corresponding inclusion and exclusion sets of\n> + objects. This allows, e.g., signed URIs to be used and packfiles for common\n> + requests to be cached.\n> + * On the client, resumption of clone. If a clone is interrupted, information\n> + could be recorded in the repository's config and a \"clone-resume\" command\n> + can resume the clone in progress. (Resumption of subsequent fetches is more\n> + difficult because that must deal with the user wanting to use the repository\n> + even after the fetch was interrupted.)\n> +\n> +There are some possible features that will require a change in protocol:\n> +\n> + * Additional HTTP headers (e.g. authentication)\n> + * Byte range support\n> + * Different file formats referenced by URIs (e.g. raw object)\n> +\n> diff --git a/Documentation/technical/protocol-v2.txt b/Documentation/technical/protocol-v2.txt\n> index 345c00e08c..2cb1c41742 100644\n> --- a/Documentation/technical/protocol-v2.txt\n> +++ b/Documentation/technical/protocol-v2.txt\n> @@ -313,7 +313,8 @@ header. Most sections are sent only when the packfile is sent.\n> \n> output = acknowledgements flush-pkt |\n> \t [acknowledgments delim-pkt] [shallow-info delim-pkt]\n> -\t [wanted-refs delim-pkt] packfile flush-pkt\n> +\t [wanted-refs delim-pkt] [packfile-uris delim-pkt]\n> +\t packfile flush-pkt\n> \n> acknowledgments = PKT-LINE(\"acknowledgments\" LF)\n> \t\t (nak | *ack)\n> @@ -331,6 +332,9 @@ header. Most sections are sent only when the packfile is sent.\n> \t\t *PKT-LINE(wanted-ref LF)\n> wanted-ref = obj-id SP refname\n> \n> + packfile-uris = PKT-LINE(\"packfile-uris\" LF) *packfile-uri\n> + packfile-uri = PKT-LINE(\"uri\" SP *%x20-ff LF)\n> +\n> packfile = PKT-LINE(\"packfile\" LF)\n> \t *PKT-LINE(%x01-03 *%x00-ff)\n"},{"id":"364610","messageId":"xmqq7egocybv.fsf@gitster-ct.c.googlers.com","threadId":"49942","inReplyTo":"xmqqbm60d0s1.fsf@gitster-ct.c.googlers.com","subject":"Re: [WIP RFC 2/5] Documentation: add Packfile URIs design doc","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-12-05T05:55:00Z","receivedAt":"2018-12-05T05:55:08Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> So, this is OK, but\n>\n>> +Clients then should understand that the returned packfile could be incomplete,\n>> +and that it needs to download all the given URIs before the fetch or clone is\n>> +complete. Each URI should point to a Git packfile (which may be a thin pack and\n>> +which may contain offset deltas).\n>\n> weaken or remove the (parenthetical comment) in the last sentence,\n> and replace the beginning of the section with something like\n>\n> \tIf the client replies with 'packfile-uris', when the server\n> \tsends the packfile, it MAY send a `packfile-uris` section...\n>\n> You may steal what I wrote in the above response to help the\n> server-side folks to decide how to actually implement the \"it MAY\n> send a packfile-uris\" part in the document.\n\nBy the way, I do agree with the practical consideration the design\nyou described makes. For a pregenerated pack that brings you from\nv1.0 to v2.0, \"thin\" would roughly save the transfer of one full\ncheckout (compressed, of course), and \"ofs\" would also save several\nbytes per object. Compared to a single pack that delivers everything\nthe fetcher wants, concatenation of packs without \"thin\" to transfer\nthe same set of objects would cost quite a lot more.\n\nAnd I do not think we should care too much about fetchers that lack\neither thin or ofs, so I'd imagine that any client that ask for\npackfile-uris would also advertise thin and ofs as well, so in\npractice, a request with packfile-uris that lack thin or ofs would\nbe pretty rare and requiring all three and requiring only one would\nnot make much practical difference. It's just that I think singling\nout these two capabilities as hard requirements at the protocol\nlevel is wrong.\n"},{"id":"364614","messageId":"xmqqzhtkbi50.fsf@gitster-ct.c.googlers.com","threadId":"49942","inReplyTo":"e19f294df9ff999d30a47339a7848c7104bfae7d.1543879256.git.jonathantanmy@google.com","subject":"Re: [WIP RFC 3/5] upload-pack: refactor reading of pack-objects out","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-12-05T06:30:03Z","receivedAt":"2018-12-05T06:30:16Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> +struct output_state {\n> +\tchar buffer[8193];\n> +\tint used;\n> +};\n> +\n> +static int read_pack_objects_stdout(int outfd, struct output_state *os)\n> +{\n\nThe naming feels quite odd. We are downstream of pack-objects and\nreading its standard output stream, so \"read stdout-of-pack-objects\"\nis not wrong per-se, but it may be just me. Stepping back and what\nthe function is really about often helps us to understand what we\nare doing.\n\nI think the function you extracted from the existing logic is\nresponsible for relaying the data read from pack-objects to the\nfetch-pack client on the other side of the wire. So from that point\nof view, singling out \"read\" as if it is a primary thing the\nfunction does is already suboptimal. Perhaps\n\n\tstatic int relay_pack_data(int in, struct output_state *os)\n\nbecause the fd is what we \"read\" from (hence, 'in', not 'out'), and\nrelaying is what we do and reading is only one half of doing so?\n\n> +\t/* Data ready; we keep the last byte to ourselves\n> +\t * in case we detect broken rev-list, so that we\n> +\t * can leave the stream corrupted. This is\n> +\t * unfortunate -- unpack-objects would happily\n> +\t * accept a valid packdata with trailing garbage,\n> +\t * so appending garbage after we pass all the\n> +\t * pack data is not good enough to signal\n> +\t * breakage to downstream.\n> +\t */\n\nYeah, I recall writing this funky and unfortunate logic in 2006.\nPerhaps we want to update the comment style to make it a bit more\nmodern?\n\nThe \"Data ready;\" part of the comment applies more to what the\ncaller of this logic does (i.e. poll returned and revents indicates\nwe can read, and that is why we are calling into this logic). The\nremainder is the comment that is relevat to this logic.\n\n> +\tssize_t readsz;\n> +\n> +\treadsz = xread(outfd, os->buffer + os->used,\n> +\t\t sizeof(os->buffer) - os->used);\n\nSo we read what we could read in the remaining buffer.\n\nI notice that you alloated 8k+1 bytes; would this code end up\nreading that 8k+1 bytes in full under the right condition? I am\nmainly wondering where the need for +1 comes from.\n\n> +\tif (readsz < 0) {\n> +\t\treturn readsz;\n> +\t}\n\nOK, report an error to the caller by returning negative. I am\nguessing that you'd have more code in this block that currently have\nonly one statement in the future steps, perhaps.\n\n> +\tos->used += readsz;\n> +\n> +\tif (os->used > 1) {\n> +\t\tsend_client_data(1, os->buffer, os->used - 1);\n> +\t\tos->buffer[0] = os->buffer[os->used - 1];\n\nOK, this corresponds to the \"*cp++ = buffered\" in the original just\nbefore xread().\n\n> +\t\tos->used = 1;\n> +\t} else {\n> +\t\tsend_client_data(1, os->buffer, os->used);\n> +\t\tos->used = 0;\n\nI am not sure if the code is correct when os->used happens to be 1\n(shouldn't we hold the byte, skip the call to send_client_data(),\nand go back to poll() to expect more data?), but this is a faithful\ncode movement and rewrite of the original.\n\nI've read the caller (below, snipped) and the conversion looked\nsensible there as well. The final flushing of the byte(s) held back\nin *os also looks good.\n"},{"id":"364672","messageId":"xmqqzhtj88nw.fsf@gitster-ct.c.googlers.com","threadId":"49942","inReplyTo":"1d678a78a63b7e58988b52c8c0efab38c34a6848.1543879256.git.jonathantanmy@google.com","subject":"Re: [WIP RFC 4/5] upload-pack: refactor writing of \"packfile\" line","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-12-06T06:35:15Z","receivedAt":"2018-12-06T06:35:20Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> @@ -126,6 +129,12 @@ static int read_pack_objects_stdout(int outfd, struct output_state *os)\n> \t}\n> \tos->used += readsz;\n> \n> +\tif (!os->packfile_started) {\n> +\t\tos->packfile_started = 1;\n> +\t\tif (use_protocol_v2)\n> +\t\t\tpacket_write_fmt(1, \"packfile\\n\");\n\nIf we fix this function so that the only byte in the buffer is held\nback without emitted when os->used == 1 as I alluded to, this may\nhave to be done a bit later, as with such a change, it is no longer\nguaranteed that send_client_data() will be called after this point.\n\n> +\t}\n> +\n> \tif (os->used > 1) {\n> \t\tsend_client_data(1, os->buffer, os->used - 1);\n> \t\tos->buffer[0] = os->buffer[os->used - 1];\n\n> +static void flush_progress_buf(struct strbuf *progress_buf)\n> +{\n> +\tif (!progress_buf->len)\n> +\t\treturn;\n> +\tsend_client_data(2, progress_buf->buf, progress_buf->len);\n> +\tstrbuf_reset(progress_buf);\n> +}\n\nInteresting.\n\n> static void create_pack_file(const struct object_array *have_obj,\n> -\t\t\t const struct object_array *want_obj)\n> +\t\t\t const struct object_array *want_obj,\n> +\t\t\t int use_protocol_v2)\n> {\n> \tstruct child_process pack_objects = CHILD_PROCESS_INIT;\n> \tstruct output_state output_state = {0};\n> @@ -260,9 +278,13 @@ static void create_pack_file(const struct object_array *have_obj,\n> \t\t\t */\n> \t\t\tsz = xread(pack_objects.err, progress,\n> \t\t\t\t sizeof(progress));\n> -\t\t\tif (0 < sz)\n> -\t\t\t\tsend_client_data(2, progress, sz);\n> -\t\t\telse if (sz == 0) {\n> +\t\t\tif (0 < sz) {\n> +\t\t\t\tif (output_state.packfile_started)\n> +\t\t\t\t\tsend_client_data(2, progress, sz);\n> +\t\t\t\telse\n> +\t\t\t\t\tstrbuf_add(&output_state.progress_buf,\n> +\t\t\t\t\t\t progress, sz);\n\nIsn't progress output that goes to the channel #2 pretty much\nindependent from the payload stream that carries the pkt-line \ncommand like \"packfile\" plus the raw pack stream? It somehow\nfeels like an oxymoron to _buffer_ progress indicator, as it\ndefeats the whole point of progress report to buffer it.\n"},{"id":"364716","messageId":"20181206225431.135449-1-jonathantanmy@google.com","threadId":"49942","inReplyTo":"xmqqin08d36k.fsf@gitster-ct.c.googlers.com","subject":"Re: [WIP RFC 1/5] Documentation: order protocol v2 sections","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-12-06T22:54:31Z","receivedAt":"2018-12-06T22:54:36Z","isPatch":false,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> > The git command line expects Git servers to follow a specific order of\n> \n> \"Command line\"? It sounds like you are talking about the order of\n> command line arguments and options, but apparently that is not what\n> you are doing. Is it \"The git over-the-wire protocol\"?\n\nI meant to say the current Git implementation, as opposed to what is\nwritten in the specification. I'll replace it with \"The current C Git\nimplementation\".\n\n> Earlier, we said that shallow-info is not given when packfile is not\n> there. That is captured in the updated EBNF above. We don't have a\n> corresponding removal of a bullet point for wanted-refs section below\n> but probably that is because the original did not have corresponding\n> bullet point to begin with.\n\nThat's because the corresponding bullet point had other information.\nQuoted in full below:\n\n> \t* This section is only included if the client has requested a\n> \t ref using a 'want-ref' line and if a packfile section is also\n> \t included in the response.\n\nI could reword it to \"If a packfile section is included in the response,\nthis section is only included if the client has requested a ref using a\n'want-ref' line\", but I don't think that is significantly clearer.\n"},{"id":"364722","messageId":"20181206231624.139000-1-jonathantanmy@google.com","threadId":"49942","inReplyTo":"xmqqbm60d0s1.fsf@gitster-ct.c.googlers.com","subject":"Re: [WIP RFC 2/5] Documentation: add Packfile URIs design doc","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-12-06T23:16:24Z","receivedAt":"2018-12-06T23:16:31Z","isPatch":false,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> > +This feature allows servers to serve part of their packfile response as URIs.\n> > +This allows server designs that improve scalability in bandwidth and CPU usage\n> > +(for example, by serving some data through a CDN), and (in the future) provides\n> > +some measure of resumability to clients.\n> \n> Without reading the remainder, this makes readers anticipate a few\n> good things ;-)\n> \n> - \"part of\", so pre-generated constant material can be given from\n> CDN and then followed-up by \"filling the gaps\" small packfile,\n> perhaps?\n\nYes :-)\n\n> - The \"part of\" transmission may not bring the repository up to\n> date wrt to the \"want\" objects; would this feature involve \"you\n> asked history up to these commits, but with this pack-uri, you'll\n> be getting history up to these (somewhat stale) commits\"?\n\nIt could be, but not necessarily. In my current WIP implementation, for\nexample, pack URIs don't give you any commits at all (and thus, no\nhistory) - only blobs. Quite a few people first think of the \"stale\nclone then top-up\" case, though - I wonder if it would be a good idea to\ngive the blob example in this paragraph in order to put people in the\nright frame of mind.\n\n> > +If the client replies with the following arguments:\n> > +\n> > + * packfile-uris\n> > + * thin-pack\n> > + * ofs-delta\n> \n> \"with the following\" meaning \"with all of the following\", or \"with\n> any of the following\"? Is there a reason why the server side must\n> require that the client understands and is willing to accept a\n> thin-pack when wanting to use packfile-uris? The same question for\n> the ofs-delta.\n\n\"All of the following\", but from your later comments, we probably don't\nneed this section anyway.\n\n> My recommendation is to drop the mention of \"thin\" and \"ofs\" from\n> the above list, and also from the following paragraph. The \"it MAY\n> send\" will serve as a practical escape clause to allow a server/CDN\n> implementation that *ALWAYS* prepares pregenerated material that can\n> only be digested by clients that supports thin and ofs. Such a server\n> can send packfile-URIs only when all of the three are given by the\n> client and be compliant. And such an update to the proposed document\n> would allow a more diskful server to prepare both thin and non-thin\n> pregenerated packs and choose which one to give to the client depending\n> on the capability.\n\nThat is true - we can just let the server decide. I'll update the patch\naccordingly, and state that the URIs should point to packs with features\nlike thin-pack and ofs-delta only if the client has declared that it\nsupports them.\n\n> > +Clients then should understand that the returned packfile could be incomplete,\n> > +and that it needs to download all the given URIs before the fetch or clone is\n> > +complete. Each URI should point to a Git packfile (which may be a thin pack and\n> > +which may contain offset deltas).\n> \n> weaken or remove the (parenthetical comment) in the last sentence,\n> and replace the beginning of the section with something like\n> \n> \tIf the client replies with 'packfile-uris', when the server\n> \tsends the packfile, it MAY send a `packfile-uris` section...\n> \n> You may steal what I wrote in the above response to help the\n> server-side folks to decide how to actually implement the \"it MAY\n> send a packfile-uris\" part in the document.\n\nOK, will do.\n\n> OK, this comes back to what I alluded to at the beginning. We could\n> respond to a full-clone request by feeding a series of packfile-uris\n> and some ref information, perhaps like this:\n> \n> \t* Grab this packfile and update your remote-tracking refs\n> and tags to these values; you'd be as if you cloned the\n> project when it was at v1.0.\n> \n> \t* When you are done with the above, grab this packfile and\n> update your remote-tracking refs and tags to these values;\n> you'd be as if you cloned the project when it was at v2.0.\n> \n> \t* When you are done with the above, grab this packfile and\n> update your remote-tracking refs and tags to these values;\n> you'd be as if you cloned the project when it was at v3.0.\n> \n> \t...\n> \n> \t* When you are done with the above, here is the remaining\n> packdata to bring you fully up to date with your original\n> \"want\"s.\n> \n> and before fully reading the proposal, I anticipated that it was\n> what you were going to describe. The major difference is \"up to the\n> packdata given to you so far, you'd be as if you fetched these\" ref\n> information, which would allow you to be interrupted and then simply\n> resume, without having to remember the set of packfile-uris yet to\n> be processed across a fetch/clone failure. If you sucessfully fetch\n> packfile for ..v1.0, you can update the remote-tracking refs to\n> match as if you fetched back when that was the most recent state of\n> the project, and then if you failed while transferring packfile for\n> v1.0..v2.0, the resuming would just reissue \"git fetch\" internally.\n\nThe \"up to\" would work if we had the stale clone + periodic \"upgrades\"\narrangement you describe, but not when, for example, we just want to\nseparate large blobs out. If we were to insist on attaching ref\ninformation to each packfile URI (or turn the packfiles into bundles),\nit is true that we can have resumable fetch, although I haven't fully\nthought out the implications of letting the user modify the repository\nwhile a fetch is in progress. (What happens if the user wipes out their\nobject store in between fetching one packfile and the next, for\nexample?) That is why I only talked about resumable clone, not resumable\nfetch.\n"},{"id":"364723","messageId":"20181206232538.141378-1-jonathantanmy@google.com","threadId":"49942","inReplyTo":"xmqqzhtj88nw.fsf@gitster-ct.c.googlers.com","subject":"Re: [WIP RFC 4/5] upload-pack: refactor writing of \"packfile\" line","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-12-06T23:25:38Z","receivedAt":"2018-12-06T23:25:48Z","isPatch":false,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> Jonathan Tan writes:\n> \n> > @@ -126,6 +129,12 @@ static int read_pack_objects_stdout(int outfd, struct output_state *os)\n> > \t}\n> > \tos->used += readsz;\n> > \n> > +\tif (!os->packfile_started) {\n> > +\t\tos->packfile_started = 1;\n> > +\t\tif (use_protocol_v2)\n> > +\t\t\tpacket_write_fmt(1, \"packfile\\n\");\n> \n> If we fix this function so that the only byte in the buffer is held\n> back without emitted when os->used == 1 as I alluded to, this may\n> have to be done a bit later, as with such a change, it is no longer\n> guaranteed that send_client_data() will be called after this point.\n\nI'm not sure what you mean about there being no guarantee that\nsend_client_data() is not called - in create_pack_file(), there is an\n\"if (output_state.used > 0)\" line (previously \"if (0 <= buffered)\") that\noutputs anything remaining.\n\n> Isn't progress output that goes to the channel #2 pretty much\n> independent from the payload stream that carries the pkt-line \n> command like \"packfile\" plus the raw pack stream? It somehow\n> feels like an oxymoron to _buffer_ progress indicator, as it\n> defeats the whole point of progress report to buffer it.\n\nYes, it is - I don't fully like this part of the design. I alluded to a\nsimilar issue (keepalive) in the toplevel email [1] and that it might be\nbetter if the server can send sideband throughout the whole response -\nperhaps that should be investigated first. If we had sideband throughout\nthe whole response, we wouldn't need to buffer the progress indicator.\n\n[1] https://public-inbox.org/git/cover.1543879256.git.jonathantanmy@google.com/\n"},{"id":"364727","messageId":"xmqqmupi89ub.fsf@gitster-ct.c.googlers.com","threadId":"49942","inReplyTo":"20181206232538.141378-1-jonathantanmy@google.com","subject":"Re: [WIP RFC 4/5] upload-pack: refactor writing of \"packfile\" line","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-12-07T00:22:04Z","receivedAt":"2018-12-07T00:22:16Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n>> Jonathan Tan writes:\n>> \n>> > @@ -126,6 +129,12 @@ static int read_pack_objects_stdout(int outfd, struct output_state *os)\n>> > \t}\n>> > \tos->used += readsz;\n>> > \n>> > +\tif (!os->packfile_started) {\n>> > +\t\tos->packfile_started = 1;\n>> > +\t\tif (use_protocol_v2)\n>> > +\t\t\tpacket_write_fmt(1, \"packfile\\n\");\n>> \n>> If we fix this function so that the only byte in the buffer is held\n>> back without emitted when os->used == 1 as I alluded to, this may\n>> have to be done a bit later, as with such a change, it is no longer\n>> guaranteed that send_client_data() will be called after this point.\n>\n> I'm not sure what you mean about there being no guarantee that\n> send_client_data() is not called - in create_pack_file(), there is an\n> \"if (output_state.used > 0)\" line (previously \"if (0 <= buffered)\") that\n> outputs anything remaining.\n\nI was referring to this part of the review on the previous step,\nwhich you may not yet have read.\n\n OK, this corresponds to the \"*cp++ = buffered\" in the original just\n before xread().\n\n > +\t\tos->used = 1;\n > +\t} else {\n > +\t\tsend_client_data(1, os->buffer, os->used);\n > +\t\tos->used = 0;\n\n I am not sure if the code is correct when os->used happens to be 1\n (shouldn't we hold the byte, skip the call to send_client_data(),\n and go back to poll() to expect more data?), but this is a faithful\n code movement and rewrite of the original.\n\nThe point of this logic is to make sure we always hold back some\nbytes and do not feed *all* the bytes to the other side by calling\n\"send-client-data\" until we made sure the upstream of what we are\nrelaying (pack-objects?) successfully exited, but it looks to me\nthat the \"else\" clause above ends up flushing everything when\nos->used is 1, which goes against the whole purpose of the code.\n\nAnd the \"fix\" I was alluding to was to update that \"else\" clause to\nmake it a no-op that keeps os->used non-zero, which would not call\nsend-client-data.\n\nWhen that fix happens, the part that early in the function this\npatch added \"now we know we will call send-client-data, so let's say\n'here comes packdata' unless we have already said that\" is making\nthe decision too early. Depending on the value of os->used when we\nenter the code and the number of bytes xread() reads from the\nupstream, we might not call send-client-data yet (namely, when we\nhave no buffered data and we happened to get only one byte).\n\n> ... it might be\n> better if the server can send sideband throughout the whole response -\n> perhaps that should be investigated first.\n\nYup. It just looked quite crazy, and it is even more crazy to\nbuffer keepalives ;-)\n"},{"id":"364824","messageId":"xmqqftv77dy0.fsf@gitster-ct.c.googlers.com","threadId":"49942","inReplyTo":"20181206225431.135449-1-jonathantanmy@google.com","subject":"Re: [WIP RFC 1/5] Documentation: order protocol v2 sections","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-12-09T00:15:35Z","receivedAt":"2018-12-09T00:15:42Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n>> > The git command line expects Git servers to follow a specific order of\n>> \n>> \"Command line\"? It sounds like you are talking about the order of\n>> command line arguments and options, but apparently that is not what\n>> you are doing. Is it \"The git over-the-wire protocol\"?\n>\n> I meant to say the current Git implementation, as opposed to what is\n> written in the specification. I'll replace it with \"The current C Git\n> implementation\".\n\nYeah, that would avoid confusing future readers; sounds good.\n\n>> Earlier, we said that shallow-info is not given when packfile is not\n>> there. That is captured in the updated EBNF above. We don't have a\n>> corresponding removal of a bullet point for wanted-refs section below\n>> but probably that is because the original did not have corresponding\n>> bullet point to begin with.\n>\n> That's because the corresponding bullet point had other information.\n> Quoted in full below:\n>\n>> \t* This section is only included if the client has requested a\n>> \t ref using a 'want-ref' line and if a packfile section is also\n>> \t included in the response.\n>\n> I could reword it to \"If a packfile section is included in the response,\n> this section is only included if the client has requested a ref using a\n> 'want-ref' line\", but I don't think that is significantly clearer.\n\nI don't either. I didn't mean to suggest to change anything in this\npart. I was just giving an observation---two parallel things do not\nget updates in tandem, and that is because they were not described\nthe same way to begin with, which was a good enough explanation.\n"},{"id":"369632","messageId":"CAP8UFD0VPZEhBTUb--n5RHECUkO81aPcGkTYGi0fqd35rnKr3Q@mail.gmail.com","threadId":"49942","inReplyTo":"20181204192904.40409-1-jonathantanmy@google.com","subject":"Re: [WIP RFC 2/5] Documentation: add Packfile URIs design doc","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2019-02-19T13:22:29Z","receivedAt":"2019-02-19T13:22:45Z","isPatch":false,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Tue, Dec 4, 2018 at 8:31 PM Jonathan Tan wrote:\n>\n> > Some thoughts here:\n> >\n> > First, I'd like to see a section (and a bit in the implementation)\n> > requiring HTTPS if the original protocol is secure (SSH or HTTPS).\n> > Allowing the server to downgrade to HTTP, even by accident, would be a\n> > security problem.\n> >\n> > Second, this feature likely should be opt-in for SSH. One issue I've\n> > seen repeatedly is that people don't want to use HTTPS to fetch things\n> > when they're using SSH for Git. Many people in corporate environments\n> > have proxies that break HTTP for non-browser use cases[0], and using SSH\n> > is the only way that they can make a functional Git connection.\n>\n> Good points about SSH support and the client needing to control which\n> protocols the server will send URIs for. I'll include a line in the\n> client request in which the client can specify which protocols it is OK\n> with.\n\nWhat if a client is ok to fetch from some servers but not others (for\nexample github.com and gitlab.com but nothing else)?\n\nOr what if a client is ok to fetch using SSH from some servers and\nHTTPS from other servers but nothing else?\n\nI also wonder in general how this would interact with promisor/partial\nclone remotes.\n\nWhen we discussed promisor/partial clone remotes in the thread\nfollowing this email:\n\nhttps://public-inbox.org/git/20181016174304.GA221682@aiede.svl.corp.google.com/\n\nit looked like you were ok with having many promisor remotes, which I\nthink could fill the same use cases especially related to large\nobjects.\n\nAs clients would configure promisor remotes explicitly, there would be\nno issues about which protocol and servers are allowed or not.\n\nIf the issue is that you want the server to decide which promisor\nremotes would be used without the client having to do anything, maybe\nthat could be something added on top of the possibility to have many\npromisor remotes.\n"},{"id":"369634","messageId":"87lg2b6gg0.fsf@evledraar.gmail.com","threadId":"49942","inReplyTo":"20181204015446.GX890086@genre.crustytoothpaste.net","subject":"Re: [WIP RFC 2/5] Documentation: add Packfile URIs design doc","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-02-19T13:44:31Z","receivedAt":"2019-02-19T13:44:37Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Dec 04 2018, brian m. carlson wrote:\n\n> On Mon, Dec 03, 2018 at 03:37:35PM -0800, Jonathan Tan wrote:\n>> Signed-off-by: Jonathan Tan \n>> ---\n>> Documentation/technical/packfile-uri.txt | 83 ++++++++++++++++++++++++\n>> Documentation/technical/protocol-v2.txt | 6 +-\n>> 2 files changed, 88 insertions(+), 1 deletion(-)\n>> create mode 100644 Documentation/technical/packfile-uri.txt\n>>\n>> diff --git a/Documentation/technical/packfile-uri.txt b/Documentation/technical/packfile-uri.txt\n>> new file mode 100644\n>> index 0000000000..6535801486\n>> --- /dev/null\n>> +++ b/Documentation/technical/packfile-uri.txt\n>> @@ -0,0 +1,83 @@\n>> +Packfile URIs\n>> +=============\n>> +\n>> +This feature allows servers to serve part of their packfile response as URIs.\n>> +This allows server designs that improve scalability in bandwidth and CPU usage\n>> +(for example, by serving some data through a CDN), and (in the future) provides\n>> +some measure of resumability to clients.\n>> +\n>> +This feature is available only in protocol version 2.\n>> +\n>> +Protocol\n>> +--------\n>> +\n>> +The server advertises `packfile-uris`.\n>> +\n>> +If the client replies with the following arguments:\n>> +\n>> + * packfile-uris\n>> + * thin-pack\n>> + * ofs-delta\n>> +\n>> +when the server sends the packfile, it MAY send a `packfile-uris` section\n>> +directly before the `packfile` section (right after `wanted-refs` if it is\n>> +sent) containing HTTP(S) URIs. See protocol-v2.txt for the documentation of\n>> +this section.\n>> +\n>> +Clients then should understand that the returned packfile could be incomplete,\n>> +and that it needs to download all the given URIs before the fetch or clone is\n>> +complete. Each URI should point to a Git packfile (which may be a thin pack and\n>> +which may contain offset deltas).\n>\n>\n> First, I'd like to see a section (and a bit in the implementation)\n> requiring HTTPS if the original protocol is secure (SSH or HTTPS).\n> Allowing the server to downgrade to HTTP, even by accident, would be a\n> security problem.\n\nMaybe I've misunderstood the design (I'm writing some other follow-up\nE-Mails in this thread which might clarify things for me), but I don't\nsee why.\n\nWe get the ref advertisement from the server. We don't need to trust the\nCDN server or the transport layer. We just download whatever we get from\nthere, validate the packfile with SHA-1 (and in the future SHA-256). It\ndoesn't matter if the CDN transport is insecure.\n\nYou can do this offline with git today, you don't need to trust me to\ntrust that my copy of git.git I give you on a sketchy USB stick is\ngenuine. Just unpack it, then compare the SHA-1s you get with:\n\n git ls-remote https://github.com/git/git.git\n\nSo this is a case similar to Debian's where they distribute packages\nover http, but manifests over https: https://whydoesaptnotusehttps.com\n\n> Second, this feature likely should be opt-in for SSH. One issue I've\n> seen repeatedly is that people don't want to use HTTPS to fetch things\n> when they're using SSH for Git. Many people in corporate environments\n> have proxies that break HTTP for non-browser use cases[0], and using SSH\n> is the only way that they can make a functional Git connection.\n\nYeah, there should definitely be accommodations for such clients, per my\nreading clients can always ignore the CDN and proceed with a normal\nnegotiation. Isn't that enough, or is something extra needed?\n\n> Third, I think the server needs to be required to both support Range\n> headers and never change the content of a URI, so that we can have\n> resumable clone implicit in this design. There are some places in the\n> world where connections are poor and fetching even the initial packfile\n> at once might be a problem. (I've seen such questions on Stack\n> Overflow, for example.)\n\nI think this should be a MAY not a MUST in RFC 2119 terms. There's still\nmany users who might want to offload things to a very dumb CDN, such as\nDebian where they don't control their own mirrors, but might want to\noffload a 1GB packfile download to some random university's Debian\nmirror.\n\nSuch a download (over http) will work most of the time. If it's not\nresumable it still sucks less than no CDN at all, and client can always\nfall back if the CDN breaks, which they should be doing anyway in case\nof other sorts of issues.\n\n> Having said that, I think overall this is a good idea and I'm glad to\n> see a proposal for it.\n>\n> [0] For example, a naughty-word filter may corrupt or block certain byte\n> sequences that occur incidentally in the pack stream.\n"},{"id":"369643","messageId":"87k1hv6eel.fsf@evledraar.gmail.com","threadId":"49942","inReplyTo":"0461b362569362c6d0e73951469c547a03a1b59d.1543879256.git.jonathantanmy@google.com","subject":"Re: [WIP RFC 2/5] Documentation: add Packfile URIs design doc","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-02-19T14:28:34Z","receivedAt":"2019-02-19T14:28:39Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Dec 04 2018, Jonathan Tan wrote:\n\nI meant to follow-up after Git Merge, but didn't remember until this\nthread was bumped.\n\nBut some things I'd like to clarify / am concerned about...\n\n> +when the server sends the packfile, it MAY send a `packfile-uris` section\n> +directly before the `packfile` section (right after `wanted-refs` if it is\n> +sent) containing HTTP(S) URIs. See protocol-v2.txt for the documentation of\n> +this section.\n> +\n> +Clients then should understand that the returned packfile could be incomplete,\n> +and that it needs to download all the given URIs before the fetch or clone is\n> +complete. Each URI should point to a Git packfile (which may be a thin pack and\n> +which may contain offset deltas).\n> [...]\n> +This is the implementation: a feature, marked experimental, that allows the\n> +server to be configured by one or more `uploadpack.blobPackfileUri=\n> +` entries. Whenever the list of objects to be sent is assembled, a blob\n> +with the given sha1 can be replaced by the given URI. This allows, for example,\n> +servers to delegate serving of large blobs to CDNs.\n\nOkey, so the server advertisement is not just \"\" but \npairs. More on this later...\n\n> +While fetching, the client needs to remember the list of URIs and cannot\n> +declare that the fetch is complete until all URIs have been downloaded as\n> +packfiles.\n\nAnd this. I don't quite understand this well enough, but maybe it helps\nif I talk about what I'd expect out of CDN offloading. It comes down to\nthree things:\n\n * The server should be able to point to some \"seed\" packfiles *without*\n necessarily knowing what OIDs are in it, or have to tell the client.\n\n * The client should be able to just blindly get this data (\"I guess\n this is where most of it is\"), unpack it, see what OIDs it has, and\n *then* without initiating a new connection continue a want/have\n dialog.\n\n This effectively \"bootstraps\" a \"clone\" mid way into an arbitrary\n \"fetch\".\n\n * There should be no requirement that a client successfully downloads\n the advertised CDNs, for fault handling (also discussed in\n https://public-inbox.org/git/87lg2b6gg0.fsf@evledraar.gmail.com/)\n\nMore concretely, I'd like to have a setup where a server can just dumbly\npoint to some URL that probably has most of the data, without having any\nidea what OIDs are in it. So that e.g. some machine entirely\ndisconnected from the server (and with just a regular clone) can\ncontinually generating an up-to-date-enough packfile.\n\nI don't see how this is compatible with the server needing to send a\nbunch of \" \" lines, or why a client \"cannot declare that the\nfetch is complete until all URIs have been downloaded as\npackfiles\". Can't it fall back on the normal dialog?\n\nOther thoughts:\n\n * If there isn't such a close coordination between git server & CDN, is\n there a case for having pack *.idx files on the CDN, so clients can\n inspect them to see if they'd like to download the full referenced\n pack?\n\n * Without the server needing to know enough about the packs to\n advertise \" \" is there a way to e.g. advertise 4x packs to\n clients:\n\n big.pack, last-month.pack, last-week.pack, last-day.pack\n\n Or some other optimistic negotiation where clients, even ones just\n doing regular fetches, can seek to get more up-to-date with one of\n the more recent packs before doing the first fetch in 3 days?\n\n In the past I'd toyed with creating a similar \"not quite CDN\" setup\n using git-bundle.\n"},{"id":"369664","messageId":"20190219201022.237430-1-jonathantanmy@google.com","threadId":"49942","inReplyTo":"CAP8UFD0VPZEhBTUb--n5RHECUkO81aPcGkTYGi0fqd35rnKr3Q@mail.gmail.com","subject":"Re: [WIP RFC 2/5] Documentation: add Packfile URIs design doc","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-02-19T20:10:22Z","receivedAt":"2019-02-19T20:10:28Z","isPatch":false,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> > Good points about SSH support and the client needing to control which\n> > protocols the server will send URIs for. I'll include a line in the\n> > client request in which the client can specify which protocols it is OK\n> > with.\n> \n> What if a client is ok to fetch from some servers but not others (for\n> example github.com and gitlab.com but nothing else)?\n> \n> Or what if a client is ok to fetch using SSH from some servers and\n> HTTPS from other servers but nothing else?\n\nThe objects received from the various CDNs are still rehashed by the\nclient (so they are identified with the correct name), and if the client\nis fetching from a server, presumably it can trust the URLs it receives\n(just like it trusts ref names, and so on). Do you know of a specific\ncase in which a client wants to fetch from some servers but not others?\n(In any case, if this happens, the client can just disable the CDN\nsupport.)\n\n> I also wonder in general how this would interact with promisor/partial\n> clone remotes.\n> \n> When we discussed promisor/partial clone remotes in the thread\n> following this email:\n> \n> https://public-inbox.org/git/20181016174304.GA221682@aiede.svl.corp.google.com/\n> \n> it looked like you were ok with having many promisor remotes, which I\n> think could fill the same use cases especially related to large\n> objects.\n>\n> As clients would configure promisor remotes explicitly, there would be\n> no issues about which protocol and servers are allowed or not.\n> \n> If the issue is that you want the server to decide which promisor\n> remotes would be used without the client having to do anything, maybe\n> that could be something added on top of the possibility to have many\n> promisor remotes.\n\nIt's true that there is a slight overlap with respect to large objects,\nbut this protocol can also handle large sets of objects being offloaded\nto CDN, not only single ones. (The included implementation only handles\nsingle objects, as a minimum viable product, but it is conceivable that\nthe server implementation is later expanded to allow offloading of sets\nof objects.)\n\nAnd this protocol is meant to be able to use CDNs to help serve objects,\nwhether single objects or sets of objects. In the case of promisor\nremotes, the thing we fetch from has to be a Git server. (We could use\ndumb HTTP from a CDN, but that defeats the purpose in at least one way -\nwith dumb HTTP, we have to fetch objects individually, but with URL\nsupport, we can fetch objects as sets too.)\n"},{"id":"369670","messageId":"20190219220605.8905-1-jonathantanmy@google.com","threadId":"49942","inReplyTo":"87k1hv6eel.fsf@evledraar.gmail.com","subject":"Re: [WIP RFC 2/5] Documentation: add Packfile URIs design doc","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-02-19T22:06:05Z","receivedAt":"2019-02-19T22:06:13Z","isPatch":false,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> > +when the server sends the packfile, it MAY send a `packfile-uris` section\n> > +directly before the `packfile` section (right after `wanted-refs` if it is\n> > +sent) containing HTTP(S) URIs. See protocol-v2.txt for the documentation of\n> > +this section.\n> > +\n> > +Clients then should understand that the returned packfile could be incomplete,\n> > +and that it needs to download all the given URIs before the fetch or clone is\n> > +complete. Each URI should point to a Git packfile (which may be a thin pack and\n> > +which may contain offset deltas).\n> > [...]\n> > +This is the implementation: a feature, marked experimental, that allows the\n> > +server to be configured by one or more `uploadpack.blobPackfileUri=\n> > +` entries. Whenever the list of objects to be sent is assembled, a blob\n> > +with the given sha1 can be replaced by the given URI. This allows, for example,\n> > +servers to delegate serving of large blobs to CDNs.\n> \n> Okey, so the server advertisement is not just \"\" but \n> pairs. More on this later...\n\nActually, the server advertisement is just \"\". (The OID is there\nto tell the server which object to omit if it sends the URL.) But I see\nthat the rest of your comments still stand.\n\n> More concretely, I'd like to have a setup where a server can just dumbly\n> point to some URL that probably has most of the data, without having any\n> idea what OIDs are in it. So that e.g. some machine entirely\n> disconnected from the server (and with just a regular clone) can\n> continually generating an up-to-date-enough packfile.\n\nThanks for the concrete use case. Server ignorance would work in this\ncase, since the client can concisely communicate to the server what\nobjects it obtained from the CDN (in this case, through \"have\" lines),\nbut it does not seem to work in the general case (e.g. offloading large\nblobs; or the CDN serving a pack suitable for a shallow clone -\ncontaining all objects referenced by the last few commits, whether\nchanged in that commit or not).\n\nIn this case, maybe the batch job can also inform the server which\ncommit the CDN is prepared to serve.\n\n> I don't see how this is compatible with the server needing to send a\n> bunch of \" \" lines, or why a client \"cannot declare that the\n> fetch is complete until all URIs have been downloaded as\n> packfiles\". Can't it fall back on the normal dialog?\n\nAs stated above, the server advertisement is just \"\", but you're\nright that the server still needs to know their corresponding OIDs (or\nhave some knowledge like \"this pack contains all objects in between this\ncommit and that commit\").\n\nI was thinking that there is no normal dialog to be had with this\nprotocol, since (as above) in the general case, the client cannot\nconcisely communicate what objects it obtained from the CDN.\n\n> Other thoughts:\n> \n> * If there isn't such a close coordination between git server & CDN, is\n> there a case for having pack *.idx files on the CDN, so clients can\n> inspect them to see if they'd like to download the full referenced\n> pack?\n\nI'm not sure if I understand this fully, but off the top of my head, the\n.idx file doesn't contain relations between objects, so I don't think\nthe client has enough information to decide if it wants to download the\ncorresponding packfile.\n\n> * Without the server needing to know enough about the packs to\n> advertise \" \" is there a way to e.g. advertise 4x packs to\n> clients:\n> \n> big.pack, last-month.pack, last-week.pack, last-day.pack\n> \n> Or some other optimistic negotiation where clients, even ones just\n> doing regular fetches, can seek to get more up-to-date with one of\n> the more recent packs before doing the first fetch in 3 days?\n> \n> In the past I'd toyed with creating a similar \"not quite CDN\" setup\n> using git-bundle.\n\nI think such optimistic downloading of packs during a regular fetch\nwould only work in a partial clone where \"holes\" are tolerated. (That\ndoes bring up the possibility of having a fetch mode in which we\ndownload potentially incomplete packfiles into a partial repo and then\n\"completing\" the repo through a not-yet-implemented process, but I\nhaven't thought through this.)\n"},{"id":"369754","messageId":"20190221010909.GA488342@genre.crustytoothpaste.net","threadId":"49942","inReplyTo":"87lg2b6gg0.fsf@evledraar.gmail.com","subject":"Re: [WIP RFC 2/5] Documentation: add Packfile URIs design doc","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2019-02-21T01:09:09Z","receivedAt":"2019-02-21T01:09:22Z","isPatch":false,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Tue, Feb 19, 2019 at 02:44:31PM +0100, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Tue, Dec 04 2018, brian m. carlson wrote:\n> > First, I'd like to see a section (and a bit in the implementation)\n> > requiring HTTPS if the original protocol is secure (SSH or HTTPS).\n> > Allowing the server to downgrade to HTTP, even by accident, would be a\n> > security problem.\n> \n> Maybe I've misunderstood the design (I'm writing some other follow-up\n> E-Mails in this thread which might clarify things for me), but I don't\n> see why.\n> \n> We get the ref advertisement from the server. We don't need to trust the\n> CDN server or the transport layer. We just download whatever we get from\n> there, validate the packfile with SHA-1 (and in the future SHA-256). It\n> doesn't matter if the CDN transport is insecure.\n> \n> You can do this offline with git today, you don't need to trust me to\n> trust that my copy of git.git I give you on a sketchy USB stick is\n> genuine. Just unpack it, then compare the SHA-1s you get with:\n> \n> git ls-remote https://github.com/git/git.git\n> \n> So this is a case similar to Debian's where they distribute packages\n> over http, but manifests over https: https://whydoesaptnotusehttps.com\n\nThis assumes that integrity of the data is the only reason you'd want to\nuse HTTPS. There's also confidentiality. Perhaps a user is downloading\ndata that will help them circumvent the Great Firewall of China. A\ndowngrade to HTTP could result in a long prison sentence.\n\nFurthermore, some ISPs tamper with headers to allow tracking, and some\nenvironments (e.g. schools and libraries) perform opportunistic\nfiltering on HTTP connections to filter certain content (and a lot of\nthis filtering is really simplistic).\n\nMoreover, Google is planning on using this and filters in place of Git\nLFS for large objects. I expect that if this approach becomes viable, it\nmay actually grow authentication functionality, or, depending on how the\nseries uses the existing code, it may already have it. In such a case,\nwe should not allow authentication to go over a plaintext connection\nwhen the user thinks that the connection they're using is encrypted\n(since they used an SSH or HTTPS URL to clone or fetch).\n\nDowngrades from HTTPS to HTTP are generally considered CVE-worthy. We\nneed to make sure that we refuse to allow a downgrade on the client\nside, even if the server ignores our request for a secure protocol.\n\n> > Second, this feature likely should be opt-in for SSH. One issue I've\n> > seen repeatedly is that people don't want to use HTTPS to fetch things\n> > when they're using SSH for Git. Many people in corporate environments\n> > have proxies that break HTTP for non-browser use cases[0], and using SSH\n> > is the only way that they can make a functional Git connection.\n> \n> Yeah, there should definitely be accommodations for such clients, per my\n> reading clients can always ignore the CDN and proceed with a normal\n> negotiation. Isn't that enough, or is something extra needed?\n\nI think at least a config option and a command line flag are needed to\nbe able to turn CDN usage off. There needs to be an easy way for people\nin broken environments to circumvent the breakage.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"369895","messageId":"877eds5fpl.fsf@evledraar.gmail.com","threadId":"49942","inReplyTo":"20190221010909.GA488342@genre.crustytoothpaste.net","subject":"Re: [WIP RFC 2/5] Documentation: add Packfile URIs design doc","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-02-22T09:34:46Z","receivedAt":"2019-02-22T09:34:51Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Feb 21 2019, brian m. carlson wrote:\n\n> On Tue, Feb 19, 2019 at 02:44:31PM +0100, Ævar Arnfjörð Bjarmason wrote:\n>>\n>> On Tue, Dec 04 2018, brian m. carlson wrote:\n>> > First, I'd like to see a section (and a bit in the implementation)\n>> > requiring HTTPS if the original protocol is secure (SSH or HTTPS).\n>> > Allowing the server to downgrade to HTTP, even by accident, would be a\n>> > security problem.\n>>\n>> Maybe I've misunderstood the design (I'm writing some other follow-up\n>> E-Mails in this thread which might clarify things for me), but I don't\n>> see why.\n>>\n>> We get the ref advertisement from the server. We don't need to trust the\n>> CDN server or the transport layer. We just download whatever we get from\n>> there, validate the packfile with SHA-1 (and in the future SHA-256). It\n>> doesn't matter if the CDN transport is insecure.\n>>\n>> You can do this offline with git today, you don't need to trust me to\n>> trust that my copy of git.git I give you on a sketchy USB stick is\n>> genuine. Just unpack it, then compare the SHA-1s you get with:\n>>\n>> git ls-remote https://github.com/git/git.git\n>>\n>> So this is a case similar to Debian's where they distribute packages\n>> over http, but manifests over https: https://whydoesaptnotusehttps.com\n>\n> This assumes that integrity of the data is the only reason you'd want to\n> use HTTPS. There's also confidentiality. Perhaps a user is downloading\n> data that will help them circumvent the Great Firewall of China. A\n> downgrade to HTTP could result in a long prison sentence.\n>\n> Furthermore, some ISPs tamper with headers to allow tracking, and some\n> environments (e.g. schools and libraries) perform opportunistic\n> filtering on HTTP connections to filter certain content (and a lot of\n> this filtering is really simplistic).\n>\n> Moreover, Google is planning on using this and filters in place of Git\n> LFS for large objects. I expect that if this approach becomes viable, it\n> may actually grow authentication functionality, or, depending on how the\n> series uses the existing code, it may already have it. In such a case,\n> we should not allow authentication to go over a plaintext connection\n> when the user thinks that the connection they're using is encrypted\n> (since they used an SSH or HTTPS URL to clone or fetch).\n>\n> Downgrades from HTTPS to HTTP are generally considered CVE-worthy. We\n> need to make sure that we refuse to allow a downgrade on the client\n> side, even if the server ignores our request for a secure protocol.\n\nAll good points, I definitely agree we shouldn't do downgrading by\ndefault for the reasons you've outlined, and e.g. make this an opt-in.\n\nI'm just mindful that git's used as infrastructure in a lot of unusual\ncase, e.g. something like what apt's doing (after carefully weighing\nhttp v.s. https for their use-case).\n\nSo I think providing some optional escape hatch is still a good idea.\n\n>> > Second, this feature likely should be opt-in for SSH. One issue I've\n>> > seen repeatedly is that people don't want to use HTTPS to fetch things\n>> > when they're using SSH for Git. Many people in corporate environments\n>> > have proxies that break HTTP for non-browser use cases[0], and using SSH\n>> > is the only way that they can make a functional Git connection.\n>>\n>> Yeah, there should definitely be accommodations for such clients, per my\n>> reading clients can always ignore the CDN and proceed with a normal\n>> negotiation. Isn't that enough, or is something extra needed?\n>\n> I think at least a config option and a command line flag are needed to\n> be able to turn CDN usage off. There needs to be an easy way for people\n> in broken environments to circumvent the breakage.\n\nYeah, but let's try hard to make it Just Work. I.e. if in the middle of\nthe dialog the CDN connection is broken can we retry then, and if that\nfails just continue with negotiation against the server?\n\nAs opposed to erroring by default, and the user needing to retry with\nsome config option...\n"},{"id":"369905","messageId":"CAP8UFD16fvtu_dg3S_J9BjGpxAYvgp8SXscdh=TJB5jvAbzi4A@mail.gmail.com","threadId":"49942","inReplyTo":"20190219201022.237430-1-jonathantanmy@google.com","subject":"Re: [WIP RFC 2/5] Documentation: add Packfile URIs design doc","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2019-02-22T11:35:00Z","receivedAt":"2019-02-22T11:35:15Z","isPatch":false,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Tue, Feb 19, 2019 at 9:10 PM Jonathan Tan wrote:\n>\n> > > Good points about SSH support and the client needing to control which\n> > > protocols the server will send URIs for. I'll include a line in the\n> > > client request in which the client can specify which protocols it is OK\n> > > with.\n> >\n> > What if a client is ok to fetch from some servers but not others (for\n> > example github.com and gitlab.com but nothing else)?\n> >\n> > Or what if a client is ok to fetch using SSH from some servers and\n> > HTTPS from other servers but nothing else?\n>\n> The objects received from the various CDNs are still rehashed by the\n> client (so they are identified with the correct name), and if the client\n> is fetching from a server, presumably it can trust the URLs it receives\n> (just like it trusts ref names, and so on). Do you know of a specific\n> case in which a client wants to fetch from some servers but not others?\n\nFor example I think the Great Firewall of China lets people in China\nuse GitHub.com but not Google.com. So if people start configuring\ntheir repos on GitHub so that they send packs that contain Google.com\nCDN URLs (or actually anything that the Firewall blocks), it might\ncreate many problems for users in China if they don't have a way to\nopt out of receiving packs with those kind of URLs.\n\n> (In any case, if this happens, the client can just disable the CDN\n> support.)\n\nWould this mean that people in China will not be able to use the\nfeature at all, because too many of their clones could be blocked? Or\nthat they will have to create forks to mirror any interesting repo and\n reconfigure those forks to work well from China?\n\n> > I also wonder in general how this would interact with promisor/partial\n> > clone remotes.\n> >\n> > When we discussed promisor/partial clone remotes in the thread\n> > following this email:\n> >\n> > https://public-inbox.org/git/20181016174304.GA221682@aiede.svl.corp.google.com/\n> >\n> > it looked like you were ok with having many promisor remotes, which I\n> > think could fill the same use cases especially related to large\n> > objects.\n> >\n> > As clients would configure promisor remotes explicitly, there would be\n> > no issues about which protocol and servers are allowed or not.\n> >\n> > If the issue is that you want the server to decide which promisor\n> > remotes would be used without the client having to do anything, maybe\n> > that could be something added on top of the possibility to have many\n> > promisor remotes.\n>\n> It's true that there is a slight overlap with respect to large objects,\n> but this protocol can also handle large sets of objects being offloaded\n> to CDN, not only single ones.\n\nIsn't partial clone also designed to handle large sets of objects?\n\n> (The included implementation only handles\n> single objects, as a minimum viable product, but it is conceivable that\n> the server implementation is later expanded to allow offloading of sets\n> of objects.)\n>\n> And this protocol is meant to be able to use CDNs to help serve objects,\n> whether single objects or sets of objects. In the case of promisor\n> remotes, the thing we fetch from has to be a Git server.\n\nWhen we discussed the plan for many promisor remotes, Jonathan Nieder\n(in the email linked above) suggested:\n\n 2. Simplifying the protocol for fetching missing objects so that it\n can be satisfied by a lighter weight object storage system than\n a full Git server. The ODB helpers introduced in this series are\n meant to speak such a simpler protocol since they are only used\n for one-off requests of a collection of missing objects instead of\n needing to understand refs, Git's negotiation, etc.\n\nand I agreed with that point.\n\nIs there something that you don't like in many promisor remotes?\n\n> (We could use\n> dumb HTTP from a CDN, but that defeats the purpose in at least one way -\n> with dumb HTTP, we have to fetch objects individually, but with URL\n> support, we can fetch objects as sets too.)\n"}]}