{"thread":{"id":"53583","subject":"[PATCH 0/8] CDN offloading update","startedAt":"2020-05-29T22:30:35Z","lastAt":"2020-12-01T12:49:45Z","messageCount":44,"participants":["Jonathan Tan","Junio C Hamano","Jonathan Nieder","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":8},"messages":[{"id":"398909","messageId":"cover.1590789428.git.jonathantanmy@google.com","threadId":"53583","inReplyTo":null,"subject":"[PATCH 0/8] CDN offloading update","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-05-29T22:30:12Z","receivedAt":"2020-05-29T22:30:35Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"For those of you who are interested in CDN offloading, we have been\ntrying it out at $DAYJOB and found a bug - since we use the URL as the\nname of a temporary file, if the URL is longer than the maximum filename\nsize (255 on Linux), it will not work.\n\nHere are the latest patches with the bug fix. Note that these are based\non the \"next\" branch.\n\nJonathan Tan (8):\n  http: use --stdin when getting dumb HTTP pack\n  http: improve documentation of http_pack_request\n  http-fetch: support fetching packfiles by URL\n  Documentation: order protocol v2 sections\n  Documentation: add Packfile URIs design doc\n  upload-pack: refactor reading of pack-objects out\n  fetch-pack: support more than one pack lockfile\n  upload-pack: send part of packfile response as uri\n\n Documentation/git-http-fetch.txt         |   8 +-\n Documentation/technical/packfile-uri.txt |  78 +++++++++++\n Documentation/technical/protocol-v2.txt  |  44 +++++--\n builtin/fetch-pack.c                     |  17 ++-\n builtin/pack-objects.c                   |  76 +++++++++++\n connected.c                              |   8 +-\n fetch-pack.c                             | 135 ++++++++++++++++---\n fetch-pack.h                             |   2 +-\n http-fetch.c                             |  64 +++++++--\n http.c                                   |  88 ++++++++-----\n http.h                                   |  32 ++++-\n t/t5550-http-fetch-dumb.sh               |  25 ++++\n t/t5702-protocol-v2.sh                   |  88 +++++++++++++\n transport-helper.c                       |   5 +-\n transport.c                              |  12 +-\n transport.h                              |   6 +-\n upload-pack.c                            | 157 +++++++++++++++++------\n 17 files changed, 711 insertions(+), 134 deletions(-)\n create mode 100644 Documentation/technical/packfile-uri.txt\n\n-- \n2.27.0.rc0.183.gde8f92d652-goog\n\n"},{"id":"398910","messageId":"4d17d560b87746acfd62ff785cc22c09600d4e65.1590789428.git.jonathantanmy@google.com","threadId":"53583","inReplyTo":"cover.1590789428.git.jonathantanmy@google.com","subject":"[PATCH 1/8] http: use --stdin when getting dumb HTTP pack","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-05-29T22:30:13Z","receivedAt":"2020-05-29T22:30:36Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"When Git fetches a pack using dumb HTTP, it reuses the server's name for\nthe packfile (which incorporates a hash), which is different from the\nbehavior of fetch-pack and receive-pack.\n\nA subsequent patch will allow downloading packs over HTTP(S) as part of\na fetch. These downloads will not necessarily be from a Git repository,\nand thus may not have a hash as part of its name.\n\nThus, teach http to pass --stdin to index-pack, so that we have no\nreliance on the server's name for the packfile.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\n http.c | 33 +++++++++++----------------------\n 1 file changed, 11 insertions(+), 22 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex 4882c9f5b2..130e9d6259 100644\n--- a/http.c\n+++ b/http.c\n@@ -2276,9 +2276,9 @@ int finish_http_pack_request(struct http_pack_request *preq)\n {\n \tstruct packed_git **lst;\n \tstruct packed_git *p = preq->target;\n-\tchar *tmp_idx;\n-\tsize_t len;\n \tstruct child_process ip = CHILD_PROCESS_INIT;\n+\tint tmpfile_fd;\n+\tint ret = 0;\n \n \tclose_pack_index(p);\n \n@@ -2290,35 +2290,24 @@ int finish_http_pack_request(struct http_pack_request *preq)\n \t\tlst = &((*lst)->next);\n \t*lst = (*lst)->next;\n \n-\tif (!strip_suffix(preq->tmpfile.buf, \".pack.temp\", &len))\n-\t\tBUG(\"pack tmpfile does not end in .pack.temp?\");\n-\ttmp_idx = xstrfmt(\"%.*s.idx.temp\", (int)len, preq->tmpfile.buf);\n+\ttmpfile_fd = xopen(preq->tmpfile.buf, O_RDONLY);\n \n \targv_array_push(&ip.args, \"index-pack\");\n-\targv_array_pushl(&ip.args, \"-o\", tmp_idx, NULL);\n-\targv_array_push(&ip.args, preq->tmpfile.buf);\n+\targv_array_push(&ip.args, \"--stdin\");\n \tip.git_cmd = 1;\n-\tip.no_stdin = 1;\n+\tip.in = tmpfile_fd;\n \tip.no_stdout = 1;\n \n \tif (run_command(&ip)) {\n-\t\tunlink(preq->tmpfile.buf);\n-\t\tunlink(tmp_idx);\n-\t\tfree(tmp_idx);\n-\t\treturn -1;\n-\t}\n-\n-\tunlink(sha1_pack_index_name(p->hash));\n-\n-\tif (finalize_object_file(preq->tmpfile.buf, sha1_pack_name(p->hash))\n-\t || finalize_object_file(tmp_idx, sha1_pack_index_name(p->hash))) {\n-\t\tfree(tmp_idx);\n-\t\treturn -1;\n+\t\tret = -1;\n+\t\tgoto cleanup;\n \t}\n \n \tinstall_packed_git(the_repository, p);\n-\tfree(tmp_idx);\n-\treturn 0;\n+cleanup:\n+\tclose(tmpfile_fd);\n+\tunlink(preq->tmpfile.buf);\n+\treturn ret;\n }\n \n struct http_pack_request *new_http_pack_request(\n-- \n2.27.0.rc0.183.gde8f92d652-goog\n\n"},{"id":"398911","messageId":"7c66ab1be0722eecf3a7be76921b4c1ef9bb50f2.1590789428.git.jonathantanmy@google.com","threadId":"53583","inReplyTo":"cover.1590789428.git.jonathantanmy@google.com","subject":"[PATCH 2/8] http: improve documentation of http_pack_request","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-05-29T22:30:14Z","receivedAt":"2020-05-29T22:30:38Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"struct http_pack_request and the functions that use it will be modified\nin a subsequent patch. Using it is complicated (to use, call the\ninitialization function, then set some but not all fields in the\nreturned struct), so add some documentation to help future users.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\n http.h | 19 ++++++++++++++++++-\n 1 file changed, 18 insertions(+), 1 deletion(-)\n\ndiff --git a/http.h b/http.h\nindex faf8cbb0d1..a5b082f3ae 100644\n--- a/http.h\n+++ b/http.h\n@@ -215,14 +215,31 @@ int http_get_info_packs(const char *base_url,\n \t\t\tstruct packed_git **packs_head);\n \n struct http_pack_request {\n+\t/*\n+\t * Initialized by new_http_pack_request().\n+\t */\n \tchar *url;\n \tstruct packed_git *target;\n+\tstruct active_request_slot *slot;\n+\n+\t/*\n+\t * After calling new_http_pack_request(), point lst to the head of the\n+\t * pack list that target is in. finish_http_pack_request() will remove\n+\t * target from lst and call install_packed_git() on target.\n+\t */\n \tstruct packed_git **lst;\n+\n+\t/*\n+\t * State managed by functions in http.c.\n+\t */\n \tFILE *packfile;\n \tstruct strbuf tmpfile;\n-\tstruct active_request_slot *slot;\n };\n \n+/*\n+ * target must be an element in a pack list obtained from\n+ * http_get_info_packs().\n+ */\n struct http_pack_request *new_http_pack_request(\n \tstruct packed_git *target, const char *base_url);\n int finish_http_pack_request(struct http_pack_request *preq);\n-- \n2.27.0.rc0.183.gde8f92d652-goog\n\n"},{"id":"398912","messageId":"7a2e9c3c5994fc155eb6a40f039cf2298957fa6c.1590789428.git.jonathantanmy@google.com","threadId":"53583","inReplyTo":"cover.1590789428.git.jonathantanmy@google.com","subject":"[PATCH 4/8] Documentation: order protocol v2 sections","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-05-29T22:30:16Z","receivedAt":"2020-05-29T22:30:39Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"The current C Git implementation expects Git servers to follow a\nspecific order of sections when transmitting protocol v2 responses, but\nthis is not explicit in the documentation. Make the order explicit.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\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 3996d70891..ef7514a3ee 100644\n--- a/Documentation/technical/protocol-v2.txt\n+++ b/Documentation/technical/protocol-v2.txt\n@@ -327,11 +327,11 @@ included in the client's request:\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@@ -353,9 +353,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@@ -406,9 +407,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.27.0.rc0.183.gde8f92d652-goog\n\n"},{"id":"398913","messageId":"6b3a628719e0593893e537de0220a5e0d5460232.1590789428.git.jonathantanmy@google.com","threadId":"53583","inReplyTo":"cover.1590789428.git.jonathantanmy@google.com","subject":"[PATCH 3/8] http-fetch: support fetching packfiles by URL","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-05-29T22:30:15Z","receivedAt":"2020-05-29T22:30:40Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Teach http-fetch the ability to download packfiles directly, given a\nURL, and to verify them.\n\nThe http_pack_request suite of functions have been modified to support a\nNULL target. When target is NULL, the given URL is downloaded directly\ninstead of being treated as the root of a repository.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\n Documentation/git-http-fetch.txt |  8 +++-\n http-fetch.c                     | 64 +++++++++++++++++++++++++-------\n http.c                           | 55 ++++++++++++++++++++-------\n http.h                           | 19 ++++++++--\n t/t5550-http-fetch-dumb.sh       | 25 +++++++++++++\n 5 files changed, 141 insertions(+), 30 deletions(-)\n\ndiff --git a/Documentation/git-http-fetch.txt b/Documentation/git-http-fetch.txt\nindex 666b042679..8357359a9b 100644\n--- a/Documentation/git-http-fetch.txt\n+++ b/Documentation/git-http-fetch.txt\n@@ -9,7 +9,7 @@ git-http-fetch - Download from a remote Git repository via HTTP\n SYNOPSIS\n --------\n [verse]\n-'git http-fetch' [-c] [-t] [-a] [-d] [-v] [-w filename] [--recover] [--stdin] <commit> <url>\n+'git http-fetch' [-c] [-t] [-a] [-d] [-v] [-w filename] [--recover] [--stdin | --packfile | <commit>] <url>\n \n DESCRIPTION\n -----------\n@@ -40,6 +40,12 @@ commit-id::\n \n \t\t<commit-id>['\\t'<filename-as-in--w>]\n \n+--packfile::\n+\tInstead of a commit id on the command line (which is not expected in\n+\tthis case), 'git http-fetch' fetches the packfile directly at the given\n+\tURL and uses index-pack to generate corresponding .idx and .keep files.\n+\tThe output of index-pack is printed to stdout.\n+\n --recover::\n \tVerify that everything reachable from target is fetched.  Used after\n \tan earlier fetch is interrupted.\ndiff --git a/http-fetch.c b/http-fetch.c\nindex a32ac118d9..a9764d6f96 100644\n--- a/http-fetch.c\n+++ b/http-fetch.c\n@@ -5,7 +5,7 @@\n #include \"walker.h\"\n \n static const char http_fetch_usage[] = \"git http-fetch \"\n-\"[-c] [-t] [-a] [-v] [--recover] [-w ref] [--stdin] commit-id url\";\n+\"[-c] [-t] [-a] [-v] [--recover] [-w ref] [--stdin | --packfile | commit-id] url\";\n \n int cmd_main(int argc, const char **argv)\n {\n@@ -19,6 +19,7 @@ int cmd_main(int argc, const char **argv)\n \tint rc = 0;\n \tint get_verbosely = 0;\n \tint get_recover = 0;\n+\tint packfile = 0;\n \n \twhile (arg < argc && argv[arg][0] == '-') {\n \t\tif (argv[arg][1] == 't') {\n@@ -35,43 +36,80 @@ int cmd_main(int argc, const char **argv)\n \t\t\tget_recover = 1;\n \t\t} else if (!strcmp(argv[arg], \"--stdin\")) {\n \t\t\tcommits_on_stdin = 1;\n+\t\t} else if (!strcmp(argv[arg], \"--packfile\")) {\n+\t\t\tpackfile = 1;\n \t\t}\n \t\targ++;\n \t}\n-\tif (argc != arg + 2 - commits_on_stdin)\n+\tif (argc != arg + 2 - (commits_on_stdin || packfile))\n \t\tusage(http_fetch_usage);\n \tif (commits_on_stdin) {\n \t\tcommits = walker_targets_stdin(&commit_id, &write_ref);\n+\t} else if (packfile) {\n+\t\t/* URL will be set later */\n \t} else {\n \t\tcommit_id = (char **) &argv[arg++];\n \t\tcommits = 1;\n \t}\n \n-\tif (argv[arg])\n-\t\tstr_end_url_with_slash(argv[arg], &url);\n+\tif (packfile) {\n+\t\turl = xstrdup(argv[arg]);\n+\t} else {\n+\t\tif (argv[arg])\n+\t\t\tstr_end_url_with_slash(argv[arg], &url);\n+\t}\n \n \tsetup_git_directory();\n \n \tgit_config(git_default_config, NULL);\n \n \thttp_init(NULL, url, 0);\n-\twalker = get_http_walker(url);\n-\twalker->get_verbosely = get_verbosely;\n-\twalker->get_recover = get_recover;\n \n-\trc = walker_fetch(walker, commits, commit_id, write_ref, url);\n+\tif (packfile) {\n+\t\tstruct http_pack_request *preq;\n+\t\tstruct slot_results results;\n+\t\tint ret;\n+\n+\t\tpreq = new_http_pack_request(NULL, url);\n+\t\tif (preq == NULL)\n+\t\t\tdie(\"couldn't create http pack request\");\n+\t\tpreq->slot->results = &results;\n+\t\tpreq->generate_keep = 1;\n+\n+\t\tif (start_active_slot(preq->slot)) {\n+\t\t\trun_active_slot(preq->slot);\n+\t\t\tif (results.curl_result != CURLE_OK) {\n+\t\t\t\tdie(\"Unable to get pack file %s\\n%s\", preq->url,\n+\t\t\t\t    curl_errorstr);\n+\t\t\t}\n+\t\t} else {\n+\t\t\tdie(\"Unable to start request\");\n+\t\t}\n+\n+\t\tif ((ret = finish_http_pack_request(preq)))\n+\t\t\tdie(\"finish_http_pack_request gave result %d\", ret);\n+\t\trelease_http_pack_request(preq);\n+\t\trc = 0;\n+\t} else {\n+\t\twalker = get_http_walker(url);\n+\t\twalker->get_verbosely = get_verbosely;\n+\t\twalker->get_recover = get_recover;\n+\n+\t\trc = walker_fetch(walker, commits, commit_id, write_ref, url);\n \n-\tif (commits_on_stdin)\n-\t\twalker_targets_free(commits, commit_id, write_ref);\n+\t\tif (commits_on_stdin)\n+\t\t\twalker_targets_free(commits, commit_id, write_ref);\n \n-\tif (walker->corrupt_object_found) {\n-\t\tfprintf(stderr,\n+\t\tif (walker->corrupt_object_found) {\n+\t\t\tfprintf(stderr,\n \"Some loose object were found to be corrupt, but they might be just\\n\"\n \"a false '404 Not Found' error message sent with incorrect HTTP\\n\"\n \"status code.  Suggest running 'git fsck'.\\n\");\n+\t\t}\n+\n+\t\twalker_free(walker);\n \t}\n \n-\twalker_free(walker);\n \thttp_cleanup();\n \n \tfree(url);\ndiff --git a/http.c b/http.c\nindex 130e9d6259..ac66215ee6 100644\n--- a/http.c\n+++ b/http.c\n@@ -2280,15 +2280,18 @@ int finish_http_pack_request(struct http_pack_request *preq)\n \tint tmpfile_fd;\n \tint ret = 0;\n \n-\tclose_pack_index(p);\n+\tif (p)\n+\t\tclose_pack_index(p);\n \n \tfclose(preq->packfile);\n \tpreq->packfile = NULL;\n \n-\tlst = preq->lst;\n-\twhile (*lst != p)\n-\t\tlst = &((*lst)->next);\n-\t*lst = (*lst)->next;\n+\tif (p) {\n+\t\tlst = preq->lst;\n+\t\twhile (*lst != p)\n+\t\t\tlst = &((*lst)->next);\n+\t\t*lst = (*lst)->next;\n+\t}\n \n \ttmpfile_fd = xopen(preq->tmpfile.buf, O_RDONLY);\n \n@@ -2296,14 +2299,21 @@ int finish_http_pack_request(struct http_pack_request *preq)\n \targv_array_push(&ip.args, \"--stdin\");\n \tip.git_cmd = 1;\n \tip.in = tmpfile_fd;\n-\tip.no_stdout = 1;\n+\tif (preq->generate_keep) {\n+\t\targv_array_pushf(&ip.args, \"--keep=git %\"PRIuMAX,\n+\t\t\t\t (uintmax_t)getpid());\n+\t\tip.out = 0;\n+\t} else {\n+\t\tip.no_stdout = 1;\n+\t}\n \n \tif (run_command(&ip)) {\n \t\tret = -1;\n \t\tgoto cleanup;\n \t}\n \n-\tinstall_packed_git(the_repository, p);\n+\tif (p)\n+\t\tinstall_packed_git(the_repository, p);\n cleanup:\n \tclose(tmpfile_fd);\n \tunlink(preq->tmpfile.buf);\n@@ -2321,12 +2331,31 @@ struct http_pack_request *new_http_pack_request(\n \tstrbuf_init(&preq->tmpfile, 0);\n \tpreq->target = target;\n \n-\tend_url_with_slash(&buf, base_url);\n-\tstrbuf_addf(&buf, \"objects/pack/pack-%s.pack\",\n-\t\thash_to_hex(target->hash));\n-\tpreq->url = strbuf_detach(&buf, NULL);\n+\tif (target) {\n+\t\tend_url_with_slash(&buf, base_url);\n+\t\tstrbuf_addf(&buf, \"objects/pack/pack-%s.pack\",\n+\t\t\thash_to_hex(target->hash));\n+\t\tpreq->url = strbuf_detach(&buf, NULL);\n+\t} else {\n+\t\tpreq->url = xstrdup(base_url);\n+\t}\n+\n+\tif (target) {\n+\t\tstrbuf_addf(&preq->tmpfile, \"%s.temp\",\n+\t\t\t    sha1_pack_name(target->hash));\n+\t} else {\n+\t\tconst char *shortened_url;\n+\t\tsize_t url_len = strlen(base_url);\n+\n+\t\tshortened_url = url_len <= 50\n+\t\t\t? base_url : base_url + (url_len - 50);\n+\t\tstrbuf_addf(&preq->tmpfile, \"%s/pack/pack-\",\n+\t\t\t    get_object_directory());\n+\t\tstrbuf_addstr_urlencode(&preq->tmpfile,\n+\t\t\t\t\tshortened_url, is_rfc3986_unreserved);\n+\t\tstrbuf_addstr(&preq->tmpfile, \".temp\");\n+\t}\n \n-\tstrbuf_addf(&preq->tmpfile, \"%s.temp\", sha1_pack_name(target->hash));\n \tpreq->packfile = fopen(preq->tmpfile.buf, \"a\");\n \tif (!preq->packfile) {\n \t\terror(\"Unable to open local file %s for pack\",\n@@ -2350,7 +2379,7 @@ struct http_pack_request *new_http_pack_request(\n \t\tif (http_is_verbose)\n \t\t\tfprintf(stderr,\n \t\t\t\t\"Resuming fetch of pack %s at byte %\"PRIuMAX\"\\n\",\n-\t\t\t\thash_to_hex(target->hash),\n+\t\t\t\ttarget ? hash_to_hex(target->hash) : base_url,\n \t\t\t\t(uintmax_t)prev_posn);\n \t\thttp_opt_request_remainder(preq->slot->curl, prev_posn);\n \t}\ndiff --git a/http.h b/http.h\nindex a5b082f3ae..709dfa4c19 100644\n--- a/http.h\n+++ b/http.h\n@@ -223,12 +223,21 @@ struct http_pack_request {\n \tstruct active_request_slot *slot;\n \n \t/*\n-\t * After calling new_http_pack_request(), point lst to the head of the\n+\t * After calling new_http_pack_request(), if fetching a pack that\n+\t * http_get_info_packs() told us about, point lst to the head of the\n \t * pack list that target is in. finish_http_pack_request() will remove\n \t * target from lst and call install_packed_git() on target.\n \t */\n \tstruct packed_git **lst;\n \n+\t/*\n+\t * If this is true, finish_http_pack_request() will pass \"--keep\" to\n+\t * index-pack, resulting in the creation of a keep file, and will not\n+\t * suppress its stdout (that is, the \"keep\\t<hash>\\n\" line will be\n+\t * printed to stdout).\n+\t */\n+\tunsigned generate_keep : 1;\n+\n \t/*\n \t * State managed by functions in http.c.\n \t */\n@@ -237,8 +246,12 @@ struct http_pack_request {\n };\n \n /*\n- * target must be an element in a pack list obtained from\n- * http_get_info_packs().\n+ * If fetching a pack that http_get_info_packs() told us about, set target to\n+ * an element in a pack list obtained from http_get_info_packs(). The actual\n+ * URL fetched will be base_url followed by a suffix with the hash of the pack.\n+ *\n+ * Otherwise, set target to NULL. The actual URL fetched will be base_url\n+ * itself.\n  */\n struct http_pack_request *new_http_pack_request(\n \tstruct packed_git *target, const char *base_url);\ndiff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh\nindex 50485300eb..53010efc49 100755\n--- a/t/t5550-http-fetch-dumb.sh\n+++ b/t/t5550-http-fetch-dumb.sh\n@@ -199,6 +199,23 @@ test_expect_success 'fetch packed objects' '\n \tgit clone $HTTPD_URL/dumb/repo_pack.git\n '\n \n+test_expect_success 'http-fetch --packfile' '\n+\tgit init packfileclient &&\n+\tp=$(cd \"$HTTPD_DOCUMENT_ROOT_PATH\"/repo_pack.git && ls objects/pack/pack-*.pack) &&\n+\tgit -C packfileclient http-fetch --packfile \"$HTTPD_URL\"/dumb/repo_pack.git/$p >out &&\n+\n+\t# Ensure that the expected files are generated\n+\tgrep \"^keep.[0-9a-f]\\{16,\\}$\" out &&\n+\tcut -c6- out >packhash &&\n+\ttest -e \"packfileclient/.git/objects/pack/pack-$(cat packhash).pack\" &&\n+\ttest -e \"packfileclient/.git/objects/pack/pack-$(cat packhash).idx\" &&\n+\ttest -e \"packfileclient/.git/objects/pack/pack-$(cat packhash).keep\" &&\n+\n+\t# Ensure that it has the HEAD of repo_pack, at least\n+\tHASH=$(git -C \"$HTTPD_DOCUMENT_ROOT_PATH\"/repo_pack.git rev-parse HEAD) &&\n+\tgit -C packfileclient cat-file -e \"$HASH\"\n+'\n+\n test_expect_success 'fetch notices corrupt pack' '\n \tcp -R \"$HTTPD_DOCUMENT_ROOT_PATH\"/repo_pack.git \"$HTTPD_DOCUMENT_ROOT_PATH\"/repo_bad1.git &&\n \t(cd \"$HTTPD_DOCUMENT_ROOT_PATH\"/repo_bad1.git &&\n@@ -214,6 +231,14 @@ test_expect_success 'fetch notices corrupt pack' '\n \t)\n '\n \n+test_expect_success 'http-fetch --packfile with corrupt pack' '\n+\trm -rf packfileclient &&\n+\tgit init packfileclient &&\n+\tp=$(cd \"$HTTPD_DOCUMENT_ROOT_PATH\"/repo_bad1.git && ls objects/pack/pack-*.pack) &&\n+\ttest_must_fail git -C packfileclient http-fetch --packfile \\\n+\t\t\"$HTTPD_URL\"/dumb/repo_bad1.git/$p\n+'\n+\n test_expect_success 'fetch notices corrupt idx' '\n \tcp -R \"$HTTPD_DOCUMENT_ROOT_PATH\"/repo_pack.git \"$HTTPD_DOCUMENT_ROOT_PATH\"/repo_bad2.git &&\n \t(cd \"$HTTPD_DOCUMENT_ROOT_PATH\"/repo_bad2.git &&\n-- \n2.27.0.rc0.183.gde8f92d652-goog\n\n"},{"id":"398914","messageId":"4eea9d927af1df11cdb0342e969b293a6e317d46.1590789428.git.jonathantanmy@google.com","threadId":"53583","inReplyTo":"cover.1590789428.git.jonathantanmy@google.com","subject":"[PATCH 5/8] Documentation: add Packfile URIs design doc","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-05-29T22:30:17Z","receivedAt":"2020-05-29T22:30:43Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Signed-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\n Documentation/technical/packfile-uri.txt | 78 ++++++++++++++++++++++++\n Documentation/technical/protocol-v2.txt  | 28 ++++++++-\n 2 files changed, 105 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..6a5a6440d5\n--- /dev/null\n+++ b/Documentation/technical/packfile-uri.txt\n@@ -0,0 +1,78 @@\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 then communicates which protocols (HTTPS, etc.) it supports with\n+a `packfile-uris` argument, the server MAY send a `packfile-uris` section\n+directly before the `packfile` section (right after `wanted-refs` if it is\n+sent) containing URIs of any of the given protocols. The URIs point to\n+packfiles that use only features that the client has declared that it supports\n+(e.g. ofs-delta and thin-pack). 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.\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=<sha1>\n+<uri>` 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-uris` parameter) by passing --no-packfile-uris 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)\ndiff --git a/Documentation/technical/protocol-v2.txt b/Documentation/technical/protocol-v2.txt\nindex ef7514a3ee..7e1b3a0bfe 100644\n--- a/Documentation/technical/protocol-v2.txt\n+++ b/Documentation/technical/protocol-v2.txt\n@@ -325,13 +325,26 @@ included in the client's request:\n \tindicating its sideband (1, 2, or 3), and the server may send \"0005\\2\"\n \t(a PKT-LINE of sideband 2 with no payload) as a keepalive packet.\n \n+If the 'packfile-uris' feature is advertised, the following argument\n+can be included in the client's request as well as the potential\n+addition of the 'packfile-uris' section in the server's response as\n+explained below.\n+\n+    packfile-uris <comma-separated list of protocols>\n+\tIndicates to the server that the client is willing to receive\n+\tURIs of any of the given protocols in place of objects in the\n+\tsent packfile. Before performing the connectivity check, the\n+\tclient should download from all given URIs. Currently, the\n+\tprotocols supported are \"http\" and \"https\".\n+\n The response of `fetch` is broken into a number of sections separated by\n delimiter packets (0001), with each section beginning with its section\n header. Most sections are sent only when the packfile is sent.\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@@ -349,6 +362,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(40*(HEXDIGIT) SP *%x20-ff LF)\n+\n     packfile = PKT-LINE(\"packfile\" LF)\n \t       *PKT-LINE(%x01-03 *%x00-ff)\n \n@@ -420,6 +436,16 @@ header. Most sections are sent only when the packfile is sent.\n \t* The server MUST NOT send any refs which were not requested\n \t  using 'want-ref' lines.\n \n+    packfile-uris section\n+\t* This section is only included if the client sent\n+\t  'packfile-uris' and the server has at least one such URI to\n+\t  send.\n+\n+\t* Always begins with the section header \"packfile-uris\".\n+\n+\t* For each URI the server sends, it sends a hash of the pack's\n+\t  contents (as output by git index-pack) followed by the URI.\n+\n     packfile section\n \t* This section is only included if the client has sent 'want'\n \t  lines in its request and either requested that no more\n-- \n2.27.0.rc0.183.gde8f92d652-goog\n\n"},{"id":"398915","messageId":"65db1a649d9b481b0122f981eee255907b7139bd.1590789428.git.jonathantanmy@google.com","threadId":"53583","inReplyTo":"cover.1590789428.git.jonathantanmy@google.com","subject":"[PATCH 6/8] upload-pack: refactor reading of pack-objects out","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-05-29T22:30:18Z","receivedAt":"2020-05-29T22:30:44Z","isPatch":true,"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 <jonathantanmy@google.com>\n---\n upload-pack.c | 81 ++++++++++++++++++++++++++++++---------------------\n 1 file changed, 47 insertions(+), 34 deletions(-)\n\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 0478bff3e7..13f6152560 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -102,15 +102,53 @@ 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 relay_pack_data(int pack_objects_out, struct output_state *os)\n+{\n+\t/*\n+\t * 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(pack_objects_out, 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 \t\t\t     struct list_objects_filter_options *filter_options)\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@@ -239,39 +277,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 = relay_pack_data(pack_objects.out,\n+\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@@ -296,9 +310,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.27.0.rc0.183.gde8f92d652-goog\n\n"},{"id":"398916","messageId":"2cfee363873736d9ff73cd38d96f3533bb49c904.1590789428.git.jonathantanmy@google.com","threadId":"53583","inReplyTo":"cover.1590789428.git.jonathantanmy@google.com","subject":"[PATCH 8/8] upload-pack: send part of packfile response as uri","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-05-29T22:30:20Z","receivedAt":"2020-05-29T22:30:47Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Teach upload-pack to send part of its packfile response as URIs.\n\nAn administrator may configure a repository with one or more\n\"uploadpack.blobpackfileuri\" lines, each line containing an OID, a pack\nhash, and a URI. A client may configure fetch.uriprotocols to be a\ncomma-separated list of protocols that it is willing to use to fetch\nadditional packfiles - this list will be sent to the server. Whenever an\nobject with one of those OIDs would appear in the packfile transmitted\nby upload-pack, the server may exclude that object, and instead send the\nURI. The client will then download the packs referred to by those URIs\nbefore performing the connectivity check.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\n builtin/pack-objects.c |  76 ++++++++++++++++++++++++++++\n fetch-pack.c           | 110 +++++++++++++++++++++++++++++++++++++++--\n t/t5702-protocol-v2.sh |  88 +++++++++++++++++++++++++++++++++\n upload-pack.c          |  80 +++++++++++++++++++++++++++---\n 4 files changed, 344 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex c5b433a23f..7016b28485 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -117,6 +117,8 @@ static unsigned long window_memory_limit = 0;\n \n static struct list_objects_filter_options filter_options;\n \n+static struct string_list uri_protocols = STRING_LIST_INIT_NODUP;\n+\n enum missing_action {\n \tMA_ERROR = 0,      /* fail if any missing objects are encountered */\n \tMA_ALLOW_ANY,      /* silently allow ALL missing objects */\n@@ -125,6 +127,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 *pack_hash_hex;\n+\tchar *uri;\n+};\n+static struct oidmap configured_exclusions;\n+\n+static struct oidset excluded_by_config;\n+\n /*\n  * stats\n  */\n@@ -969,6 +980,25 @@ static void write_reused_pack(struct hashfile *f)\n \tunuse_pack(&w_curs);\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->pack_hash_hex, strlen(ex->pack_hash_hex));\n+\t\twrite_in_full(1, \" \", 1);\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@@ -1266,6 +1296,25 @@ static int want_object_in_pack(const struct object_id *oid,\n \t\t}\n \t}\n \n+\tif (uri_protocols.nr) {\n+\t\tstruct configured_exclusion *ex =\n+\t\t\toidmap_get(&configured_exclusions, oid);\n+\t\tint i;\n+\t\tconst char *p;\n+\n+\t\tif (ex) {\n+\t\t\tfor (i = 0; i < uri_protocols.nr; i++) {\n+\t\t\t\tif (skip_prefix(ex->uri,\n+\t\t\t\t\t\turi_protocols.items[i].string,\n+\t\t\t\t\t\t&p) &&\n+\t\t\t\t    *p == ':') {\n+\t\t\t\t\toidset_insert(&excluded_by_config, oid);\n+\t\t\t\t\treturn 0;\n+\t\t\t\t}\n+\t\t\t}\n+\t\t}\n+\t}\n+\n \treturn 1;\n }\n \n@@ -2864,6 +2913,29 @@ 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 *oid_end, *pack_end;\n+\t\t/*\n+\t\t * Stores the pack hash. This is not a true object ID, but is\n+\t\t * of the same form.\n+\t\t */\n+\t\tstruct object_id pack_hash;\n+\n+\t\tif (parse_oid_hex(v, &ex->e.oid, &oid_end) ||\n+\t\t    *oid_end != ' ' ||\n+\t\t    parse_oid_hex(oid_end + 1, &pack_hash, &pack_end) ||\n+\t\t    *pack_end != ' ')\n+\t\t\tdie(_(\"value of uploadpack.blobpackfileuri must be \"\n+\t\t\t      \"of the form '<object-hash> <pack-hash> <uri>' (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->pack_hash_hex = xcalloc(1, pack_end - oid_end);\n+\t\tmemcpy(ex->pack_hash_hex, oid_end + 1, pack_end - oid_end - 1);\n+\t\tex->uri = xstrdup(pack_end + 1);\n+\t\toidmap_put(&configured_exclusions, ex);\n+\t}\n \treturn git_default_config(k, v, cb);\n }\n \n@@ -3462,6 +3534,9 @@ 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_STRING_LIST(0, \"uri-protocol\", &uri_protocols,\n+\t\t\t\tN_(\"protocol\"),\n+\t\t\t\tN_(\"exclude any configured uploadpack.blobpackfileuri with this protocol\")),\n \t\tOPT_END(),\n \t};\n \n@@ -3650,6 +3725,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \t}\n \n \ttrace2_region_enter(\"pack-objects\", \"write-pack-file\", the_repository);\n+\twrite_excluded_by_configs();\n \twrite_pack_file();\n \ttrace2_region_leave(\"pack-objects\", \"write-pack-file\", the_repository);\n \ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 0a9a82bc46..9668c0b66e 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -38,6 +38,7 @@ static int server_supports_filtering;\n static struct shallow_lock shallow_lock;\n static const char *alternate_shallow_file;\n static struct strbuf fsck_msg_types = STRBUF_INIT;\n+static struct string_list uri_protocols = STRING_LIST_INIT_DUP;\n \n /* Remember to update object flag allocation in object.h */\n #define COMPLETE\t(1U << 0)\n@@ -795,6 +796,7 @@ static void write_promisor_file(const char *keep_name,\n \n static int get_pack(struct fetch_pack_args *args,\n \t\t    int xd[2], struct string_list *pack_lockfiles,\n+\t\t    int only_packfile,\n \t\t    struct ref **sought, int nr_sought)\n {\n \tstruct async demux;\n@@ -855,8 +857,15 @@ static int get_pack(struct fetch_pack_args *args,\n \t\t\t\t\t\"--keep=fetch-pack %\"PRIuMAX \" on %s\",\n \t\t\t\t\t(uintmax_t)getpid(), hostname);\n \t\t}\n-\t\tif (args->check_self_contained_and_connected)\n+\t\tif (only_packfile && args->check_self_contained_and_connected)\n \t\t\targv_array_push(&cmd.args, \"--check-self-contained-and-connected\");\n+\t\telse\n+\t\t\t/*\n+\t\t\t * We cannot perform any connectivity checks because\n+\t\t\t * not all packs have been downloaded; let the caller\n+\t\t\t * have this responsibility.\n+\t\t\t */\n+\t\t\targs->check_self_contained_and_connected = 0;\n \t\t/*\n \t\t * If we're obtaining the filename of a lockfile, we'll use\n \t\t * that filename to write a .promisor file with more\n@@ -1068,7 +1077,7 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,\n \t\talternate_shallow_file = setup_temporary_shallow(si->shallow);\n \telse\n \t\talternate_shallow_file = NULL;\n-\tif (get_pack(args, fd, pack_lockfiles, sought, nr_sought))\n+\tif (get_pack(args, fd, pack_lockfiles, 1, sought, nr_sought))\n \t\tdie(_(\"git fetch-pack: fetch failed.\"));\n \n  all_done:\n@@ -1222,6 +1231,26 @@ static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,\n \t\twarning(\"filtering not recognized by server, ignoring\");\n \t}\n \n+\tif (server_supports_feature(\"fetch\", \"packfile-uris\", 0)) {\n+\t\tint i;\n+\t\tstruct strbuf to_send = STRBUF_INIT;\n+\n+\t\tfor (i = 0; i < uri_protocols.nr; i++) {\n+\t\t\tconst char *s = uri_protocols.items[i].string;\n+\n+\t\t\tif (!strcmp(s, \"https\") || !strcmp(s, \"http\")) {\n+\t\t\t\tif (to_send.len)\n+\t\t\t\t\tstrbuf_addch(&to_send, ',');\n+\t\t\t\tstrbuf_addstr(&to_send, s);\n+\t\t\t}\n+\t\t}\n+\t\tif (to_send.len) {\n+\t\t\tpacket_buf_write(&req_buf, \"packfile-uris %s\",\n+\t\t\t\t\t to_send.buf);\n+\t\t\tstrbuf_release(&to_send);\n+\t\t}\n+\t}\n+\n \t/* add wants */\n \tadd_wants(args->no_dependents, wants, &req_buf);\n \n@@ -1444,6 +1473,21 @@ static void receive_wanted_refs(struct packet_reader *reader,\n \t\tdie(_(\"error processing wanted refs: %d\"), reader->status);\n }\n \n+static void receive_packfile_uris(struct packet_reader *reader,\n+\t\t\t\t  struct string_list *uris)\n+{\n+\tprocess_section_header(reader, \"packfile-uris\", 0);\n+\twhile (packet_reader_read(reader) == PACKET_READ_NORMAL) {\n+\t\tif (reader->pktlen < the_hash_algo->hexsz ||\n+\t\t    reader->line[the_hash_algo->hexsz] != ' ')\n+\t\t\tdie(\"expected '<hash> <uri>', got: %s\\n\", reader->line);\n+\n+\t\tstring_list_append(uris, reader->line);\n+\t}\n+\tif (reader->status != PACKET_READ_DELIM)\n+\t\tdie(\"expected DELIM\");\n+}\n+\n enum fetch_state {\n \tFETCH_CHECK_LOCAL = 0,\n \tFETCH_SEND_REQUEST,\n@@ -1477,6 +1521,8 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \tstruct fetch_negotiator negotiator_alloc;\n \tstruct fetch_negotiator *negotiator;\n \tint seen_ack = 0;\n+\tstruct string_list packfile_uris = STRING_LIST_INIT_DUP;\n+\tint i;\n \n \tif (args->no_dependents) {\n \t\tnegotiator = NULL;\n@@ -1570,9 +1616,12 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \t\t\tif (process_section_header(&reader, \"wanted-refs\", 1))\n \t\t\t\treceive_wanted_refs(&reader, sought, nr_sought);\n \n-\t\t\t/* get the pack */\n+\t\t\t/* get the pack(s) */\n+\t\t\tif (process_section_header(&reader, \"packfile-uris\", 1))\n+\t\t\t\treceive_packfile_uris(&reader, &packfile_uris);\n \t\t\tprocess_section_header(&reader, \"packfile\", 0);\n-\t\t\tif (get_pack(args, fd, pack_lockfiles, sought, nr_sought))\n+\t\t\tif (get_pack(args, fd, pack_lockfiles,\n+\t\t\t\t     !packfile_uris.nr, sought, nr_sought))\n \t\t\t\tdie(_(\"git fetch-pack: fetch failed.\"));\n \t\t\tdo_check_stateless_delimiter(args, &reader);\n \n@@ -1583,8 +1632,53 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \t\t}\n \t}\n \n+\tfor (i = 0; i < packfile_uris.nr; i++) {\n+\t\tstruct child_process cmd = CHILD_PROCESS_INIT;\n+\t\tchar packname[GIT_MAX_HEXSZ + 1];\n+\t\tconst char *uri = packfile_uris.items[i].string +\n+\t\t\tthe_hash_algo->hexsz + 1;\n+\n+\t\targv_array_push(&cmd.args, \"http-fetch\");\n+\t\targv_array_push(&cmd.args, \"--packfile\");\n+\t\targv_array_push(&cmd.args, uri);\n+\t\tcmd.git_cmd = 1;\n+\t\tcmd.no_stdin = 1;\n+\t\tcmd.out = -1;\n+\t\tif (start_command(&cmd))\n+\t\t\tdie(\"fetch-pack: unable to spawn http-fetch\");\n+\n+\t\tif (read_in_full(cmd.out, packname, 5) < 0 ||\n+\t\t    memcmp(packname, \"keep\\t\", 5))\n+\t\t\tdie(\"fetch-pack: expected keep then TAB at start of http-fetch output\");\n+\n+\t\tif (read_in_full(cmd.out, packname,\n+\t\t\t\t the_hash_algo->hexsz + 1) < 0 ||\n+\t\t    packname[the_hash_algo->hexsz] != '\\n')\n+\t\t\tdie(\"fetch-pack: expected hash then LF at end of http-fetch output\");\n+\n+\t\tpackname[the_hash_algo->hexsz] = '\\0';\n+\n+\t\tclose(cmd.out);\n+\n+\t\tif (finish_command(&cmd))\n+\t\t\tdie(\"fetch-pack: unable to finish http-fetch\");\n+\n+\t\tif (memcmp(packfile_uris.items[i].string, packname,\n+\t\t\t   the_hash_algo->hexsz))\n+\t\t\tdie(\"fetch-pack: pack downloaded from %s does not match expected hash %.*s\",\n+\t\t\t    uri, (int) the_hash_algo->hexsz,\n+\t\t\t    packfile_uris.items[i].string);\n+\n+\t\tstring_list_append_nodup(pack_lockfiles,\n+\t\t\t\t\t xstrfmt(\"%s/pack/pack-%s.keep\",\n+\t\t\t\t\t\t get_object_directory(),\n+\t\t\t\t\t\t packname));\n+\t}\n+\tstring_list_clear(&packfile_uris, 0);\n+\n \tif (negotiator)\n \t\tnegotiator->release(negotiator);\n+\n \toidset_clear(&common);\n \treturn ref;\n }\n@@ -1621,6 +1715,14 @@ static void fetch_pack_config(void)\n \tgit_config_get_bool(\"repack.usedeltabaseoffset\", &prefer_ofs_delta);\n \tgit_config_get_bool(\"fetch.fsckobjects\", &fetch_fsck_objects);\n \tgit_config_get_bool(\"transfer.fsckobjects\", &transfer_fsck_objects);\n+\tif (!uri_protocols.nr) {\n+\t\tchar *str;\n+\n+\t\tif (!git_config_get_string(\"fetch.uriprotocols\", &str) && str) {\n+\t\t\tstring_list_split(&uri_protocols, str, ',', -1);\n+\t\t\tfree(str);\n+\t\t}\n+\t}\n \n \tgit_config(fetch_pack_config_cb, NULL);\n }\ndiff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh\nindex 8da65e60de..3ea9345e6f 100755\n--- a/t/t5702-protocol-v2.sh\n+++ b/t/t5702-protocol-v2.sh\n@@ -795,6 +795,94 @@ 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+configure_exclusion () {\n+\tgit -C \"$1\" hash-object \"$2\" >objh &&\n+\tgit -C \"$1\" pack-objects \"$HTTPD_DOCUMENT_ROOT_PATH/mypack\" <objh >packh &&\n+\tgit -C \"$1\" config --add \\\n+\t\t\"uploadpack.blobpackfileuri\" \\\n+\t\t\"$(cat objh) $(cat packh) $HTTPD_URL/dumb/mypack-$(cat packh).pack\" &&\n+\tcat objh\n+}\n+\n+test_expect_success 'part of packfile response provided as URI' '\n+\tP=\"$HTTPD_DOCUMENT_ROOT_PATH/http_parent\" &&\n+\trm -rf \"$P\" http_child log &&\n+\n+\tgit init \"$P\" &&\n+\tgit -C \"$P\" config \"uploadpack.allowsidebandall\" \"true\" &&\n+\n+\techo my-blob >\"$P/my-blob\" &&\n+\tgit -C \"$P\" add my-blob &&\n+\techo other-blob >\"$P/other-blob\" &&\n+\tgit -C \"$P\" add other-blob &&\n+\tgit -C \"$P\" commit -m x &&\n+\n+\tconfigure_exclusion \"$P\" my-blob >h &&\n+\tconfigure_exclusion \"$P\" other-blob >h2 &&\n+\n+\tGIT_TRACE=1 GIT_TRACE_PACKET=\"$(pwd)/log\" GIT_TEST_SIDEBAND_ALL=1 \\\n+\tgit -c protocol.version=2 \\\n+\t\t-c fetch.uriprotocols=http,https \\\n+\t\tclone \"$HTTPD_URL/smart/http_parent\" http_child &&\n+\n+\t# Ensure that my-blob and other-blob are in separate packfiles.\n+\tfor idx in http_child/.git/objects/pack/*.idx\n+\tdo\n+\t\tgit verify-pack --verbose $idx >out &&\n+\t\t{\n+\t\t\tgrep \"^[0-9a-f]\\{16,\\} \" out || :\n+\t\t} >out.objectlist &&\n+\t\tif test_line_count = 1 out.objectlist\n+\t\tthen\n+\t\t\tif grep $(cat h) out\n+\t\t\tthen\n+\t\t\t\t>hfound\n+\t\t\tfi &&\n+\t\t\tif grep $(cat h2) out\n+\t\t\tthen\n+\t\t\t\t>h2found\n+\t\t\tfi\n+\t\tfi\n+\tdone &&\n+\ttest -f hfound &&\n+\ttest -f h2found &&\n+\n+\t# Ensure that there are exactly 6 files (3 .pack and 3 .idx).\n+\tls http_child/.git/objects/pack/* >filelist &&\n+\ttest_line_count = 6 filelist\n+'\n+\n+test_expect_success 'fetching with valid packfile URI but invalid hash fails' '\n+\tP=\"$HTTPD_DOCUMENT_ROOT_PATH/http_parent\" &&\n+\trm -rf \"$P\" http_child log &&\n+\n+\tgit init \"$P\" &&\n+\tgit -C \"$P\" config \"uploadpack.allowsidebandall\" \"true\" &&\n+\n+\techo my-blob >\"$P/my-blob\" &&\n+\tgit -C \"$P\" add my-blob &&\n+\techo other-blob >\"$P/other-blob\" &&\n+\tgit -C \"$P\" add other-blob &&\n+\tgit -C \"$P\" commit -m x &&\n+\n+\tconfigure_exclusion \"$P\" my-blob >h &&\n+\t# Configure a URL for other-blob. Just reuse the hash of the object as\n+\t# the hash of the packfile, since the hash does not matter for this\n+\t# test as long as it is not the hash of the pack, and it is of the\n+\t# expected length.\n+\tgit -C \"$P\" hash-object other-blob >objh &&\n+\tgit -C \"$P\" pack-objects \"$HTTPD_DOCUMENT_ROOT_PATH/mypack\" <objh >packh &&\n+\tgit -C \"$P\" config --add \\\n+\t\t\"uploadpack.blobpackfileuri\" \\\n+\t\t\"$(cat objh) $(cat objh) $HTTPD_URL/dumb/mypack-$(cat packh).pack\" &&\n+\n+\tGIT_TEST_SIDEBAND_ALL=1 \\\n+\ttest_must_fail git -c protocol.version=2 \\\n+\t\t-c fetch.uriprotocols=http,https \\\n+\t\tclone \"$HTTPD_URL/smart/http_parent\" http_child 2>err &&\n+\ttest_i18ngrep \"pack downloaded from.*does not match expected hash\" err\n+'\n+\n # DO NOT add non-httpd-specific tests here, because the last part of this\n # test script is only executed when httpd is available and enabled.\n \ndiff --git a/upload-pack.c b/upload-pack.c\nindex 13f6152560..d928d1c0a8 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -105,9 +105,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_uris_started : 1;\n+\tunsigned packfile_started : 1;\n };\n \n-static int relay_pack_data(int pack_objects_out, struct output_state *os)\n+static int relay_pack_data(int pack_objects_out, struct output_state *os,\n+\t\t\t   int write_packfile_line)\n {\n \t/*\n \t * We keep the last byte to ourselves\n@@ -128,6 +131,37 @@ static int relay_pack_data(int pack_objects_out, struct output_state *os)\n \t}\n \tos->used += readsz;\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 (write_packfile_line) {\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, \"\\1packfile\\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 (!write_packfile_line)\n+\t\t\t\t\tBUG(\"packfile_uris requires sideband-all\");\n+\t\t\t\tpacket_write_fmt(1, \"\\1packfile-uris\\n\");\n+\t\t\t}\n+\t\t\t*p = '\\0';\n+\t\t\tpacket_write_fmt(1, \"\\1%s\\n\", os->buffer);\n+\n+\t\t\tos->used -= p - os->buffer + 1;\n+\t\t\tmemmove(os->buffer, p + 1, 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 \t\tsend_client_data(1, os->buffer, os->used - 1);\n \t\tos->buffer[0] = os->buffer[os->used - 1];\n@@ -142,7 +176,8 @@ static int relay_pack_data(int pack_objects_out, struct output_state *os)\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     struct list_objects_filter_options *filter_options)\n+\t\t\t     struct list_objects_filter_options *filter_options,\n+\t\t\t     const struct string_list *uri_protocols)\n {\n \tstruct child_process pack_objects = CHILD_PROCESS_INIT;\n \tstruct output_state output_state = { { 0 } };\n@@ -192,6 +227,11 @@ static void create_pack_file(const struct object_array *have_obj,\n \t\t\t\t\t spec);\n \t\t}\n \t}\n+\tif (uri_protocols) {\n+\t\tfor (i = 0; i < uri_protocols->nr; i++)\n+\t\t\targv_array_pushf(&pack_objects.args, \"--uri-protocol=%s\",\n+\t\t\t\t\t uri_protocols->items[0].string);\n+\t}\n \n \tpack_objects.in = -1;\n \tpack_objects.out = -1;\n@@ -278,7 +318,8 @@ 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 = relay_pack_data(pack_objects.out,\n-\t\t\t\t\t\t     &output_state);\n+\t\t\t\t\t\t     &output_state,\n+\t\t\t\t\t\t     !!uri_protocols);\n \n \t\t\tif (result == 0) {\n \t\t\t\tclose(pack_objects.out);\n@@ -1137,7 +1178,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(&reader, &have_obj, &want_obj);\n-\t\tcreate_pack_file(&have_obj, &want_obj, &filter_options);\n+\t\tcreate_pack_file(&have_obj, &want_obj, &filter_options, 0);\n \t}\n \n \tlist_objects_filter_release(&filter_options);\n@@ -1154,6 +1195,7 @@ struct upload_pack_data {\n \ttimestamp_t deepen_since;\n \tint deepen_rev_list;\n \tint deepen_relative;\n+\tstruct string_list uri_protocols;\n \n \tstruct list_objects_filter_options filter_options;\n \n@@ -1175,6 +1217,7 @@ static void upload_pack_data_init(struct upload_pack_data *data)\n \tstruct oid_array haves = OID_ARRAY_INIT;\n \tstruct object_array shallows = OBJECT_ARRAY_INIT;\n \tstruct string_list deepen_not = STRING_LIST_INIT_DUP;\n+\tstruct string_list uri_protocols = STRING_LIST_INIT_DUP;\n \n \tmemset(data, 0, sizeof(*data));\n \tdata->wants = wants;\n@@ -1182,6 +1225,7 @@ static void upload_pack_data_init(struct upload_pack_data *data)\n \tdata->haves = haves;\n \tdata->shallows = shallows;\n \tdata->deepen_not = deepen_not;\n+\tdata->uri_protocols = uri_protocols;\n \tpacket_writer_init(&data->writer, 1);\n }\n \n@@ -1342,10 +1386,18 @@ static void process_args(struct packet_reader *request,\n \t\t\tcontinue;\n \t\t}\n \n+\t\tif (skip_prefix(arg, \"packfile-uris \", &p)) {\n+\t\t\tstring_list_split(&data->uri_protocols, p, ',', -1);\n+\t\t\tcontinue;\n+\t\t}\n+\n \t\t/* ignore unknown lines maybe? */\n \t\tdie(\"unexpected line: '%s'\", arg);\n \t}\n \n+\tif (data->uri_protocols.nr && !data->writer.use_sideband)\n+\t\tstring_list_clear(&data->uri_protocols, 0);\n+\n \tif (request->status != PACKET_READ_FLUSH)\n \t\tdie(_(\"expected flush after fetch arguments\"));\n }\n@@ -1537,8 +1589,15 @@ 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_writer_write(&data.writer, \"packfile\\n\");\n-\t\t\tcreate_pack_file(&have_obj, &want_obj, &data.filter_options);\n+\t\t\tif (data.uri_protocols.nr) {\n+\t\t\t\tcreate_pack_file(&have_obj, &want_obj,\n+\t\t\t\t\t\t &data.filter_options,\n+\t\t\t\t\t\t &data.uri_protocols);\n+\t\t\t} else {\n+\t\t\t\tpacket_writer_write(&data.writer, \"packfile\\n\");\n+\t\t\t\tcreate_pack_file(&have_obj, &want_obj,\n+\t\t\t\t\t\t &data.filter_options, NULL);\n+\t\t\t}\n \t\t\tstate = FETCH_DONE;\n \t\t\tbreak;\n \t\tcase FETCH_DONE:\n@@ -1559,6 +1618,7 @@ int upload_pack_advertise(struct repository *r,\n \t\tint allow_filter_value;\n \t\tint allow_ref_in_want;\n \t\tint allow_sideband_all_value;\n+\t\tchar *str = NULL;\n \n \t\tstrbuf_addstr(value, \"shallow\");\n \n@@ -1580,6 +1640,14 @@ int upload_pack_advertise(struct repository *r,\n \t\t\t\t\t   &allow_sideband_all_value) &&\n \t\t     allow_sideband_all_value))\n \t\t\tstrbuf_addstr(value, \" sideband-all\");\n+\n+\t\tif (!repo_config_get_string(the_repository,\n+\t\t\t\t\t    \"uploadpack.blobpackfileuri\",\n+\t\t\t\t\t    &str) &&\n+\t\t    str) {\n+\t\t\tstrbuf_addstr(value, \" packfile-uris\");\n+\t\t\tfree(str);\n+\t\t}\n \t}\n \n \treturn 1;\n-- \n2.27.0.rc0.183.gde8f92d652-goog\n\n"},{"id":"398917","messageId":"4a34e5104a5e3aafc4efc81419fb18296d422577.1590789428.git.jonathantanmy@google.com","threadId":"53583","inReplyTo":"cover.1590789428.git.jonathantanmy@google.com","subject":"[PATCH 7/8] fetch-pack: support more than one pack lockfile","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-05-29T22:30:19Z","receivedAt":"2020-05-29T22:30:49Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Whenever a fetch results in a packfile being downloaded, a .keep file is\ngenerated, so that the packfile can be preserved (from, say, a running\n\"git repack\") until refs are written referring to the contents of the\npackfile.\n\nIn a subsequent patch, a successful fetch using protocol v2 may result\nin more than one .keep file being generated. Therefore, teach\nfetch_pack() and the transport mechanism to support multiple .keep\nfiles.\n\nImplementation notes:\n\n - builtin/fetch-pack.c normally does not generate .keep files, and thus\n   is unaffected by this or future changes. However, it has an\n   undocumented \"--lock-pack\" feature, used by remote-curl.c when\n   implementing the \"fetch\" remote helper command. In keeping with the\n   remote helper protocol, only one \"lock\" line will ever be written;\n   the rest will result in warnings to stderr. However, in practice,\n   warnings will never be written because the remote-curl.c \"fetch\" is\n   only used for protocol v0/v1 (which will not generate multiple .keep\n   files). (Protocol v2 uses the \"stateless-connect\" command, not the\n   \"fetch\" command.)\n\n - connected.c has an optimization in that connectivity checks on a ref\n   need not be done if the target object is in a pack known to be\n   self-contained and connected. If there are multiple packfiles, this\n   optimization can no longer be done.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\n builtin/fetch-pack.c | 17 +++++++++++------\n connected.c          |  8 +++++---\n fetch-pack.c         | 29 +++++++++++++++--------------\n fetch-pack.h         |  2 +-\n transport-helper.c   |  5 +++--\n transport.c          | 12 +++++++-----\n transport.h          |  6 +++---\n 7 files changed, 45 insertions(+), 34 deletions(-)\n\ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex 94b0c89b82..bbb5c96167 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -48,8 +48,8 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n \tstruct ref **sought = NULL;\n \tint nr_sought = 0, alloc_sought = 0;\n \tint fd[2];\n-\tchar *pack_lockfile = NULL;\n-\tchar **pack_lockfile_ptr = NULL;\n+\tstruct string_list pack_lockfiles = STRING_LIST_INIT_DUP;\n+\tstruct string_list *pack_lockfiles_ptr = NULL;\n \tstruct child_process *conn;\n \tstruct fetch_pack_args args;\n \tstruct oid_array shallow = OID_ARRAY_INIT;\n@@ -134,7 +134,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n \t\t}\n \t\tif (!strcmp(\"--lock-pack\", arg)) {\n \t\t\targs.lock_pack = 1;\n-\t\t\tpack_lockfile_ptr = &pack_lockfile;\n+\t\t\tpack_lockfiles_ptr = &pack_lockfiles;\n \t\t\tcontinue;\n \t\t}\n \t\tif (!strcmp(\"--check-self-contained-and-connected\", arg)) {\n@@ -235,10 +235,15 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n \t}\n \n \tref = fetch_pack(&args, fd, ref, sought, nr_sought,\n-\t\t\t &shallow, pack_lockfile_ptr, version);\n-\tif (pack_lockfile) {\n-\t\tprintf(\"lock %s\\n\", pack_lockfile);\n+\t\t\t &shallow, pack_lockfiles_ptr, version);\n+\tif (pack_lockfiles.nr) {\n+\t\tint i;\n+\n+\t\tprintf(\"lock %s\\n\", pack_lockfiles.items[0].string);\n \t\tfflush(stdout);\n+\t\tfor (i = 1; i < pack_lockfiles.nr; i++)\n+\t\t\twarning(_(\"Lockfile created but not reported: %s\"),\n+\t\t\t\tpack_lockfiles.items[i].string);\n \t}\n \tif (args.check_self_contained_and_connected &&\n \t    args.self_contained_and_connected) {\ndiff --git a/connected.c b/connected.c\nindex 3135b71e19..937b4bae38 100644\n--- a/connected.c\n+++ b/connected.c\n@@ -43,10 +43,12 @@ int check_connected(oid_iterate_fn fn, void *cb_data,\n \n \tif (transport && transport->smart_options &&\n \t    transport->smart_options->self_contained_and_connected &&\n-\t    transport->pack_lockfile &&\n-\t    strip_suffix(transport->pack_lockfile, \".keep\", &base_len)) {\n+\t    transport->pack_lockfiles.nr == 1 &&\n+\t    strip_suffix(transport->pack_lockfiles.items[0].string,\n+\t\t\t \".keep\", &base_len)) {\n \t\tstruct strbuf idx_file = STRBUF_INIT;\n-\t\tstrbuf_add(&idx_file, transport->pack_lockfile, base_len);\n+\t\tstrbuf_add(&idx_file, transport->pack_lockfiles.items[0].string,\n+\t\t\t   base_len);\n \t\tstrbuf_addstr(&idx_file, \".idx\");\n \t\tnew_pack = add_packed_git(idx_file.buf, idx_file.len, 1);\n \t\tstrbuf_release(&idx_file);\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex d8bbf45ee2..0a9a82bc46 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -794,7 +794,7 @@ static void write_promisor_file(const char *keep_name,\n }\n \n static int get_pack(struct fetch_pack_args *args,\n-\t\t    int xd[2], char **pack_lockfile,\n+\t\t    int xd[2], struct string_list *pack_lockfiles,\n \t\t    struct ref **sought, int nr_sought)\n {\n \tstruct async demux;\n@@ -838,7 +838,7 @@ static int get_pack(struct fetch_pack_args *args,\n \t}\n \n \tif (do_keep || args->from_promisor) {\n-\t\tif (pack_lockfile)\n+\t\tif (pack_lockfiles)\n \t\t\tcmd.out = -1;\n \t\tcmd_name = \"index-pack\";\n \t\targv_array_push(&cmd.args, cmd_name);\n@@ -863,7 +863,7 @@ static int get_pack(struct fetch_pack_args *args,\n \t\t * information below. If not, we need index-pack to do it for\n \t\t * us.\n \t\t */\n-\t\tif (!(do_keep && pack_lockfile) && args->from_promisor)\n+\t\tif (!(do_keep && pack_lockfiles) && args->from_promisor)\n \t\t\targv_array_push(&cmd.args, \"--promisor\");\n \t}\n \telse {\n@@ -899,8 +899,9 @@ static int get_pack(struct fetch_pack_args *args,\n \tcmd.git_cmd = 1;\n \tif (start_command(&cmd))\n \t\tdie(_(\"fetch-pack: unable to fork off %s\"), cmd_name);\n-\tif (do_keep && pack_lockfile) {\n-\t\t*pack_lockfile = index_pack_lockfile(cmd.out);\n+\tif (do_keep && pack_lockfiles) {\n+\t\tstring_list_append_nodup(pack_lockfiles,\n+\t\t\t\t\t index_pack_lockfile(cmd.out));\n \t\tclose(cmd.out);\n \t}\n \n@@ -922,8 +923,8 @@ static int get_pack(struct fetch_pack_args *args,\n \t * Now that index-pack has succeeded, write the promisor file using the\n \t * obtained .keep filename if necessary\n \t */\n-\tif (do_keep && pack_lockfile && args->from_promisor)\n-\t\twrite_promisor_file(*pack_lockfile, sought, nr_sought);\n+\tif (do_keep && pack_lockfiles && pack_lockfiles->nr && args->from_promisor)\n+\t\twrite_promisor_file(pack_lockfiles->items[0].string, sought, nr_sought);\n \n \treturn 0;\n }\n@@ -940,7 +941,7 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,\n \t\t\t\t const struct ref *orig_ref,\n \t\t\t\t struct ref **sought, int nr_sought,\n \t\t\t\t struct shallow_info *si,\n-\t\t\t\t char **pack_lockfile)\n+\t\t\t\t struct string_list *pack_lockfiles)\n {\n \tstruct repository *r = the_repository;\n \tstruct ref *ref = copy_ref_list(orig_ref);\n@@ -1067,7 +1068,7 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,\n \t\talternate_shallow_file = setup_temporary_shallow(si->shallow);\n \telse\n \t\talternate_shallow_file = NULL;\n-\tif (get_pack(args, fd, pack_lockfile, sought, nr_sought))\n+\tif (get_pack(args, fd, pack_lockfiles, sought, nr_sought))\n \t\tdie(_(\"git fetch-pack: fetch failed.\"));\n \n  all_done:\n@@ -1464,7 +1465,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \t\t\t\t    struct ref **sought, int nr_sought,\n \t\t\t\t    struct oid_array *shallows,\n \t\t\t\t    struct shallow_info *si,\n-\t\t\t\t    char **pack_lockfile)\n+\t\t\t\t    struct string_list *pack_lockfiles)\n {\n \tstruct repository *r = the_repository;\n \tstruct ref *ref = copy_ref_list(orig_ref);\n@@ -1571,7 +1572,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \n \t\t\t/* get the pack */\n \t\t\tprocess_section_header(&reader, \"packfile\", 0);\n-\t\t\tif (get_pack(args, fd, pack_lockfile, sought, nr_sought))\n+\t\t\tif (get_pack(args, fd, pack_lockfiles, sought, nr_sought))\n \t\t\t\tdie(_(\"git fetch-pack: fetch failed.\"));\n \t\t\tdo_check_stateless_delimiter(args, &reader);\n \n@@ -1772,7 +1773,7 @@ struct ref *fetch_pack(struct fetch_pack_args *args,\n \t\t       const struct ref *ref,\n \t\t       struct ref **sought, int nr_sought,\n \t\t       struct oid_array *shallow,\n-\t\t       char **pack_lockfile,\n+\t\t       struct string_list *pack_lockfiles,\n \t\t       enum protocol_version version)\n {\n \tstruct ref *ref_cpy;\n@@ -1807,11 +1808,11 @@ struct ref *fetch_pack(struct fetch_pack_args *args,\n \t\tmemset(&si, 0, sizeof(si));\n \t\tref_cpy = do_fetch_pack_v2(args, fd, ref, sought, nr_sought,\n \t\t\t\t\t   &shallows_scratch, &si,\n-\t\t\t\t\t   pack_lockfile);\n+\t\t\t\t\t   pack_lockfiles);\n \t} else {\n \t\tprepare_shallow_info(&si, shallow);\n \t\tref_cpy = do_fetch_pack(args, fd, ref, sought, nr_sought,\n-\t\t\t\t\t&si, pack_lockfile);\n+\t\t\t\t\t&si, pack_lockfiles);\n \t}\n \treprepare_packed_git(the_repository);\n \ndiff --git a/fetch-pack.h b/fetch-pack.h\nindex 67f684229a..85d1e39fe7 100644\n--- a/fetch-pack.h\n+++ b/fetch-pack.h\n@@ -83,7 +83,7 @@ struct ref *fetch_pack(struct fetch_pack_args *args,\n \t\t       struct ref **sought,\n \t\t       int nr_sought,\n \t\t       struct oid_array *shallow,\n-\t\t       char **pack_lockfile,\n+\t\t       struct string_list *pack_lockfiles,\n \t\t       enum protocol_version version);\n \n /*\ndiff --git a/transport-helper.c b/transport-helper.c\nindex a46afcb69d..93a6f50793 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -410,10 +410,11 @@ static int fetch_with_fetch(struct transport *transport,\n \t\t\texit(128);\n \n \t\tif (skip_prefix(buf.buf, \"lock \", &name)) {\n-\t\t\tif (transport->pack_lockfile)\n+\t\t\tif (transport->pack_lockfiles.nr)\n \t\t\t\twarning(_(\"%s also locked %s\"), data->name, name);\n \t\t\telse\n-\t\t\t\ttransport->pack_lockfile = xstrdup(name);\n+\t\t\t\tstring_list_append(&transport->pack_lockfiles,\n+\t\t\t\t\t\t   name);\n \t\t}\n \t\telse if (data->check_connectivity &&\n \t\t\t data->transport_options.check_self_contained_and_connected &&\ndiff --git a/transport.c b/transport.c\nindex 7d50c502ad..6ee6771f55 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -378,7 +378,7 @@ static int fetch_refs_via_pack(struct transport *transport,\n \trefs = fetch_pack(&args, data->fd,\n \t\t\t  refs_tmp ? refs_tmp : transport->remote_refs,\n \t\t\t  to_fetch, nr_heads, &data->shallow,\n-\t\t\t  &transport->pack_lockfile, data->version);\n+\t\t\t  &transport->pack_lockfiles, data->version);\n \n \tclose(data->fd[0]);\n \tclose(data->fd[1]);\n@@ -921,6 +921,7 @@ struct transport *transport_get(struct remote *remote, const char *url)\n \tstruct transport *ret = xcalloc(1, sizeof(*ret));\n \n \tret->progress = isatty(2);\n+\tstring_list_init(&ret->pack_lockfiles, 1);\n \n \tif (!remote)\n \t\tBUG(\"No remote provided to transport_get()\");\n@@ -1316,10 +1317,11 @@ int transport_fetch_refs(struct transport *transport, struct ref *refs)\n \n void transport_unlock_pack(struct transport *transport)\n {\n-\tif (transport->pack_lockfile) {\n-\t\tunlink_or_warn(transport->pack_lockfile);\n-\t\tFREE_AND_NULL(transport->pack_lockfile);\n-\t}\n+\tint i;\n+\n+\tfor (i = 0; i < transport->pack_lockfiles.nr; i++)\n+\t\tunlink_or_warn(transport->pack_lockfiles.items[i].string);\n+\tstring_list_clear(&transport->pack_lockfiles, 0);\n }\n \n int transport_connect(struct transport *transport, const char *name,\ndiff --git a/transport.h b/transport.h\nindex 4298c855be..05efa72db1 100644\n--- a/transport.h\n+++ b/transport.h\n@@ -5,8 +5,7 @@\n #include \"run-command.h\"\n #include \"remote.h\"\n #include \"list-objects-filter-options.h\"\n-\n-struct string_list;\n+#include \"string-list.h\"\n \n struct git_transport_options {\n \tunsigned thin : 1;\n@@ -98,7 +97,8 @@ struct transport {\n \t */\n \tconst struct string_list *server_options;\n \n-\tchar *pack_lockfile;\n+\tstruct string_list pack_lockfiles;\n+\n \tsigned verbose : 3;\n \t/**\n \t * Transports should not set this directly, and should use this\n-- \n2.27.0.rc0.183.gde8f92d652-goog\n\n"},{"id":"398919","messageId":"xmqqmu5qxs6e.fsf@gitster.c.googlers.com","threadId":"53583","inReplyTo":"4d17d560b87746acfd62ff785cc22c09600d4e65.1590789428.git.jonathantanmy@google.com","subject":"Re: [PATCH 1/8] http: use --stdin when getting dumb HTTP pack","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-29T23:00:41Z","receivedAt":"2020-05-29T23:00:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan <jonathantanmy@google.com> writes:\n\n> When Git fetches a pack using dumb HTTP, it reuses the server's name for\n> the packfile (which incorporates a hash), which is different from the\n> behavior of fetch-pack and receive-pack.\n\nMy first two reads of the above mistakenly thought that for some\nreason the packfile has the URL to the server encoded in its name,\nbut that is not what you meant by \"the server's name\".  You rather\nmeant \"the name the server stores the packfile under\", \"the name the\nserver gave the packfile\", \"it reuses the name the server uses for\nthe packfile\".  The last rephrase may be the easiest to understand.\n\n> A subsequent patch will allow downloading packs over HTTP(S) as part of\n> a fetch. These downloads will not necessarily be from a Git repository,\n> and thus may not have a hash as part of its name.\n\nA location that is not necessarily a Git repository can still honor\nthe naming convention, so I find this a bit weak argument.  After\nall, the procedure to prepare such a CDN backed file would be using\nGit and the (git) \"natural\" name for the resulting packfile is\neasily available to it, isn't it?\n\nI am not necessarily against loosening the limitation of the shape\nof the filename, but we may want to say the reason why we want to\nname the packfile on the CDN side differently from how Git would\nnaturally name them.  What benefit would we get out from geing able\nto do so?  Perhaps it makes arrangements such as \"you can fetch\n'pack-v1.0.pack' to become reasonably up-to-date if you at least\nhave the version v1.0 software\", \"if the last time you fetched from\nus was 2020-05-20 or after, you can fetch 'pack-2020-05-20.pack' and\nbe assured that you aren't missing anything\", etc.?  Such a \"why\nwould somebody want to name the packfile differently\" would make a\nmore convincing justification.\n\n> Thus, teach http to pass --stdin to index-pack, so that we have no\n> reliance on the server's name for the packfile.\n\nOK.  By definition, if we feed the packdata via --stdin, the\nindex-pack command would not even _know_ what the filename we use,\nor the name the other side had.  Makes sense.\n\n"},{"id":"398920","messageId":"xmqqeer2xr0f.fsf@gitster.c.googlers.com","threadId":"53583","inReplyTo":"6b3a628719e0593893e537de0220a5e0d5460232.1590789428.git.jonathantanmy@google.com","subject":"Re: [PATCH 3/8] http-fetch: support fetching packfiles by URL","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-29T23:25:52Z","receivedAt":"2020-05-29T23:26:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan <jonathantanmy@google.com> writes:\n\n> +'git http-fetch' [-c] [-t] [-a] [-d] [-v] [-w filename] [--recover] [--stdin | --packfile | <commit>] <url>\n> ...\n> +--packfile::\n> +\tInstead of a commit id on the command line (which is not expected in\n> +\tthis case), 'git http-fetch' fetches the packfile directly at the given\n> +\tURL and uses index-pack to generate corresponding .idx and .keep files.\n> +\tThe output of index-pack is printed to stdout.\n\nThis makes sense as an external interface, I guess.\n\nHow should this interact with --stdin option, which is more like\n\"instead of getting a single <dest filename, object name> pair from\nthe command line, handle many pairs read from the standard input\"\nbatch mode operation.  Would it be beneficial to allow unbounded\nnumber of packfiles, not just a single one, to be fetched and\nindexed by a single invocation of the command?  I suspect that given\nthe relatively large size of a single request for fetching a\npackfile, one invocation of the command per packfile won't be too\nheavy an overhead, so lack of such an orthogonality may only hurt\nconceptual cleanliness, but not performance.  OK.\n\n> -\tif (argc != arg + 2 - commits_on_stdin)\n> +\tif (argc != arg + 2 - (commits_on_stdin || packfile))\n>  \t\tusage(http_fetch_usage);\n>  \tif (commits_on_stdin) {\n>  \t\tcommits = walker_targets_stdin(&commit_id, &write_ref);\n> +\t} else if (packfile) {\n> +\t\t/* URL will be set later */\n\nPrefer to see an empty statement spelled more explicitly, like this:\n\n\t\t; /* URL will be set later */\n\nOtherwise reader would be left wondering if a line was (or lines\nwere) accidentally lost after this comment.\n\n>  \t} else {\n>  \t\tcommit_id = (char **) &argv[arg++];\n>  \t\tcommits = 1;\n>  \t}\n>  \n> +\tif (packfile) {\n> +\t\turl = xstrdup(argv[arg]);\n> +\t} else {\n> +\t\tif (argv[arg])\n> +\t\t\tstr_end_url_with_slash(argv[arg], &url);\n> +\t}\n>  \n>  \tsetup_git_directory();\n>  \n>  \tgit_config(git_default_config, NULL);\n>  \n>  \thttp_init(NULL, url, 0);\n> +\tif (packfile) {\n> +\t\tstruct http_pack_request *preq;\n> +\t\tstruct slot_results results;\n> +\t\tint ret;\n> +\n> +\t\tpreq = new_http_pack_request(NULL, url);\n> +\t\tif (preq == NULL)\n> +\t\t\tdie(\"couldn't create http pack request\");\n> +\t\tpreq->slot->results = &results;\n> +\t\tpreq->generate_keep = 1;\n> +\n> +\t\tif (start_active_slot(preq->slot)) {\n> +\t\t\trun_active_slot(preq->slot);\n> +\t\t\tif (results.curl_result != CURLE_OK) {\n> +\t\t\t\tdie(\"Unable to get pack file %s\\n%s\", preq->url,\n> +\t\t\t\t    curl_errorstr);\n> +\t\t\t}\n> +\t\t} else {\n> +\t\t\tdie(\"Unable to start request\");\n> +\t\t}\n> +\n> +\t\tif ((ret = finish_http_pack_request(preq)))\n> +\t\t\tdie(\"finish_http_pack_request gave result %d\", ret);\n> +\t\trelease_http_pack_request(preq);\n> +\t\trc = 0;\n\nThe above probably want to be a single helper function.\n\nThe other side of if/else may also become another helper function.\n\nThat way, the flow of control would become clearer.  After all,\nthese two branches do not share all that much.  Only http-init and\nhttp-cleanup and nothing else.\n\nFor that matter, even before introducing this new mode of operation,\nanother patch to make a preparatory move of the original logic in\nthis function to a helper function that would be called from the\n\"else\" side may make it easier to see what is going on.\n\n> diff --git a/http.c b/http.c\n> index 130e9d6259..ac66215ee6 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -2280,15 +2280,18 @@ int finish_http_pack_request(struct http_pack_request *preq)\n>  \tint tmpfile_fd;\n>  \tint ret = 0;\n>  \n> -\tclose_pack_index(p);\n> +\tif (p)\n> +\t\tclose_pack_index(p);\n>  \n>  \tfclose(preq->packfile);\n>  \tpreq->packfile = NULL;\n>  \n> -\tlst = preq->lst;\n> -\twhile (*lst != p)\n> -\t\tlst = &((*lst)->next);\n> -\t*lst = (*lst)->next;\n> +\tif (p) {\n> +\t\tlst = preq->lst;\n> +\t\twhile (*lst != p)\n> +\t\t\tlst = &((*lst)->next);\n> +\t\t*lst = (*lst)->next;\n> +\t}\n\nThis is quite ugly.  What is the original meaning of the target\nfield of the pack_request structure again?  A packed_git structure\nthat will be filled when we are done fetching the packfile from the\nother side and installed in our repository?  When we are (ab)using\nhttp_fetch code to fetch a single packfile, we do not install the\npackfile into the running process, because we are only (re)using the\nexisting machinery as a poor-man's \"curl | git index-pack --stdin\"?\n\nI do not think it is a bad idea to roll \"curl | git index-pack\n--stdin\" our own, but I do find this an ugly way to do so.  Perhaps\na set of lower-level helper functions can be isolated out of the\nexisting code before this new feature is added, and then a new \"just\nfetch and pipe it to the index-pack\" feature should be written using\nthese helpers but with a separate set of entry points?  Would it be\na good way to make the resulting code cleaner than this patch does?\nI dunno.\n\n> diff --git a/http.h b/http.h\n> index a5b082f3ae..709dfa4c19 100644\n> --- a/http.h\n> +++ b/http.h\n> @@ -223,12 +223,21 @@ struct http_pack_request {\n>  \tstruct active_request_slot *slot;\n>  \n>  \t/*\n> -\t * After calling new_http_pack_request(), point lst to the head of the\n> +\t * After calling new_http_pack_request(), if fetching a pack that\n> +\t * http_get_info_packs() told us about, point lst to the head of the\n>  \t * pack list that target is in. finish_http_pack_request() will remove\n>  \t * target from lst and call install_packed_git() on target.\n>  \t */\n>  \tstruct packed_git **lst;\n>  \n> +\t/*\n> +\t * If this is true, finish_http_pack_request() will pass \"--keep\" to\n> +\t * index-pack, resulting in the creation of a keep file, and will not\n> +\t * suppress its stdout (that is, the \"keep\\t<hash>\\n\" line will be\n> +\t * printed to stdout).\n> +\t */\n> +\tunsigned generate_keep : 1;\n> +\n\nI suspect that this is a sign that this single patch is trying to\ndo too many things at the same time.\n\n - Whether we are fetching a single packfile from a URL, or walking\n   to fetch all the packfiles in the repository at a given URL\n\n - Whether packfiles taken from outer space are marked with the\n   \"keep\" bit\n\n - Whether the obtained packfile(s) are internally \"installed\"\n   to the running process\n\nare conceptually independent choices, but somehow mixed up, it\nseems.\n\nThanks.\n\n"},{"id":"398921","messageId":"xmqqa71qxqoy.fsf@gitster.c.googlers.com","threadId":"53583","inReplyTo":"7a2e9c3c5994fc155eb6a40f039cf2298957fa6c.1590789428.git.jonathantanmy@google.com","subject":"Re: [PATCH 4/8] Documentation: order protocol v2 sections","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-29T23:32:45Z","receivedAt":"2020-05-29T23:32:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan <jonathantanmy@google.com> writes:\n\n> The current C Git implementation expects Git servers to follow a\n> specific order of sections when transmitting protocol v2 responses, but\n> this is not explicit in the documentation. Make the order explicit.\n\nThanks.  Are we breaking other people's implementation, or is this\nthe order all reimplementations of Git currently follow, so this is\npurely an preemptive safety measure?\n\n> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>\n> ---\n>  Documentation/technical/protocol-v2.txt | 18 ++++++++----------\n>  1 file changed, 8 insertions(+), 10 deletions(-)\n>\n> diff --git a/Documentation/technical/protocol-v2.txt b/Documentation/technical/protocol-v2.txt\n> index 3996d70891..ef7514a3ee 100644\n> --- a/Documentation/technical/protocol-v2.txt\n> +++ b/Documentation/technical/protocol-v2.txt\n> @@ -327,11 +327,11 @@ included in the client's request:\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> @@ -353,9 +353,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> @@ -406,9 +407,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"},{"id":"398922","messageId":"xmqq5zcexoqi.fsf@gitster.c.googlers.com","threadId":"53583","inReplyTo":"4eea9d927af1df11cdb0342e969b293a6e317d46.1590789428.git.jonathantanmy@google.com","subject":"Re: [PATCH 5/8] Documentation: add Packfile URIs design doc","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-30T00:15:01Z","receivedAt":"2020-05-30T00:15:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan <jonathantanmy@google.com> writes:\n\n> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>\n> ---\n>  Documentation/technical/packfile-uri.txt | 78 ++++++++++++++++++++++++\n>  Documentation/technical/protocol-v2.txt  | 28 ++++++++-\n>  2 files changed, 105 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..6a5a6440d5\n> --- /dev/null\n> +++ b/Documentation/technical/packfile-uri.txt\n> @@ -0,0 +1,78 @@\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\nYay.\n\n> +Protocol\n> +--------\n> +\n> +The server advertises `packfile-uris`.\n\nIs this a new \"protocol capability\"?  There are multiple things that\nare \"advertised\" over the wire (there is \"reference advertisement\")\nand readers would want to know if this is yet another kind of\nadvertisement or a new variety of the \"capability\".\n\n> +If the client then communicates which protocols (HTTPS, etc.) it supports with\n> +a `packfile-uris` argument, the server MAY send a `packfile-uris` section\n> +directly before the `packfile` section (right after `wanted-refs` if it is\n> +sent) containing URIs of any of the given protocols. The URIs point to\n> +packfiles that use only features that the client has declared that it supports\n> +(e.g. ofs-delta and thin-pack). 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\nI am guessing that this merely mean that the result of downloading\nand installing the packfile does not necessarily make the resulting\nrepository up-to-date with respect to the \"want\" the \"fetch\" command\nrequested.  But the above can easily be misread that the returned\npackfile is somewhat broken, corrupt or missing objects that it\nought to have (e.g. a deltified object lacks its base object in the\nfile). [#1]\n\n> +and that it needs to download all the given URIs before the fetch or clone is\n> +complete.\n\nSo if I say \"I want history leading to commit X\", and choose to use\nthe packfile-uris that told me to fetch two packfiles P and Q, does\nit mean that I need to only fetch P and Q, index-pack them and store\nthe resulting two packfiles and their idx files in my repository, do\nI have the history leading to commit X?\n\nI would have guessed that the resulting repository after fetching\nthese URIs can still be incomplete and the \"packfile\" section of the\nresponse needs to be processed before the fetch or clone is complete,\nbut the above does not say so.\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=<sha1>\n> +<uri>` 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\nLet me see if the above is understandable.\n\nDoes \"git cat-file blob <sha1>\" come back when we try to \"wget/curl\"\nthe <uri>?\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\nSame question again.  As URIs are allowed to be incomplete (point #1 above),\nit is still too early to declare success after all URIs have been downloaded\nas packfiles, isn't it?  Shouldn't the \"latest bits\" need to be extracted\nout of the usual \"packfile\" section of the protocol?\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-uris` parameter) by passing --no-packfile-uris to `git fetch`.\n\nBy default, as long as the other side advertises packfile-uris, the\nclient automatically attempts to use it and users need to explicitly\ndisable it?  \n\nIt's quite different from the way we introduce new features and I am\nwondering if it is a good approach.\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\nDid we discuss \"inclusion and exclusion sets\" whatever they are?\n\n> diff --git a/Documentation/technical/protocol-v2.txt b/Documentation/technical/protocol-v2.txt\n> index ef7514a3ee..7e1b3a0bfe 100644\n> --- a/Documentation/technical/protocol-v2.txt\n> +++ b/Documentation/technical/protocol-v2.txt\n> @@ -325,13 +325,26 @@ included in the client's request:\n>  \tindicating its sideband (1, 2, or 3), and the server may send \"0005\\2\"\n>  \t(a PKT-LINE of sideband 2 with no payload) as a keepalive packet.\n>  \n> +If the 'packfile-uris' feature is advertised, the following argument\n> +can be included in the client's request as well as the potential\n> +addition of the 'packfile-uris' section in the server's response as\n> +explained below.\n> +\n> +    packfile-uris <comma-separated list of protocols>\n> +\tIndicates to the server that the client is willing to receive\n> +\tURIs of any of the given protocols in place of objects in the\n> +\tsent packfile. Before performing the connectivity check, the\n> +\tclient should download from all given URIs. Currently, the\n> +\tprotocols supported are \"http\" and \"https\".\n\nAh, I think the puzzlement I repeatedly expressed above is starting\nto dissolve.  You are assuming that the receiving end would remember\nthe URIs but the in-protocol packfile data at the end is handled\nfirst, and then before the final connectivity check is done,\npackfiles are downloaded from the URIs.  If you spelled out that\nassumption early in the document, then I wouldn't have had to ask\nall those questions.  I was assuming a different order (i.e. CDN\npackfiles first to establish the baseline, and then in-protocol\npackfile to fill the \"latest bits\", the last mile to reach the tips\nof requested refs).\n\nIn practice, I suspect that these fetches would go in parallel with\nthe processing of the in-protocol packfile, but spelling it out as\nif these are done sequencially would help establishing the right\nmental model.  \n\n\"(1) Process in-protocol packfiles first, and then (2) fetch CDN\nURIs, and after all is done, (3) update the tips of refs\" would\nserve as a base to establish a good mental model.  It would be even\nbetter to throw in another step before all that: (0) record the\nwanted-refs and CDN URIs to the safe place.  If you get disconnected\nbefore finishing (1), you have to redo from the scratch, but once\nyou finished (0) and (1), then (2) and (3) can be done at your\nleisure using the information you saved in step (0), and (1) can be\nretried if your connection is lousy.\n\n>  The response of `fetch` is broken into a number of sections separated by\n>  delimiter packets (0001), with each section beginning with its section\n>  header. Most sections are sent only when the packfile is sent.\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> @@ -349,6 +362,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(40*(HEXDIGIT) SP *%x20-ff LF)\n\n40* \n\n>      packfile = PKT-LINE(\"packfile\" LF)\n>  \t       *PKT-LINE(%x01-03 *%x00-ff)\n>  \n> @@ -420,6 +436,16 @@ header. Most sections are sent only when the packfile is sent.\n>  \t* The server MUST NOT send any refs which were not requested\n>  \t  using 'want-ref' lines.\n>  \n> +    packfile-uris section\n> +\t* This section is only included if the client sent\n> +\t  'packfile-uris' and the server has at least one such URI to\n> +\t  send.\n> +\n> +\t* Always begins with the section header \"packfile-uris\".\n> +\n> +\t* For each URI the server sends, it sends a hash of the pack's\n> +\t  contents (as output by git index-pack) followed by the URI.\n\nOK.  This allows the URI that feeds us the packfile contents to have\nany name, and still lets us verify the contents match what the other\nend wanted to feed us.\n\nThanks.\n\n>      packfile section\n>  \t* This section is only included if the client has sent 'want'\n>  \t  lines in its request and either requested that no more\n"},{"id":"398923","messageId":"xmqq1rn2xoe8.fsf@gitster.c.googlers.com","threadId":"53583","inReplyTo":"xmqq5zcexoqi.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 5/8] Documentation: add Packfile URIs design doc","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-30T00:22:23Z","receivedAt":"2020-05-30T00:22:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> In practice, I suspect that these fetches would go in parallel with\n> the processing of the in-protocol packfile, but spelling it out as\n> if these are done sequencially would help establishing the right\n> mental model.  \n>\n> \"(1) Process in-protocol packfiles first, and then (2) fetch CDN\n> URIs, and after all is done, (3) update the tips of refs\" would\n> serve as a base to establish a good mental model.  It would be even\n> better to throw in another step before all that: (0) record the\n> wanted-refs and CDN URIs to the safe place.  If you get disconnected\n> before finishing (1), you have to redo from the scratch, but once\n> you finished (0) and (1), then (2) and (3) can be done at your\n> leisure using the information you saved in step (0), and (1) can be\n> retried if your connection is lousy.\n\nWe need to be a bit careful here.  After finishing (0) and (1), the\nmost recent history near the requested tips is not anchored by any\nref.  We of course cannot point these \"most recent\" objects with\nrefs because it is very likely that they are not connected to the\nparts of the history we already have in the receiving repository.\nThe huge gap exists to be filled by the bulk download from CDN.\n\nSo a GC that happens before (3) completes can discard object data\nobtained in step (1).  One way to protect it may be to use a .keep\nfile but then some procedure needs to be there to remove it once we\nare done.  Perhaps at the end of (1), the name of that .keep file is\nadded to the set of information we keep until (3) happens (the\nremainder of the set of information was obtained in step (0)).\n\n"},{"id":"398955","messageId":"xmqqa71ovy50.fsf@gitster.c.googlers.com","threadId":"53583","inReplyTo":"2cfee363873736d9ff73cd38d96f3533bb49c904.1590789428.git.jonathantanmy@google.com","subject":"Re: [PATCH 8/8] upload-pack: send part of packfile response as uri","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-31T16:59:23Z","receivedAt":"2020-05-31T16:59:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan <jonathantanmy@google.com> writes:\n\n> diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh\n> index 8da65e60de..3ea9345e6f 100755\n> + ...\n> +\tGIT_TEST_SIDEBAND_ALL=1 \\\n> +\ttest_must_fail git -c protocol.version=2 \\\n> +\t\t-c fetch.uriprotocols=http,https \\\n> +\t\tclone \"$HTTPD_URL/smart/http_parent\" http_child 2>err &&\n> +\ttest_i18ngrep \"pack downloaded from.*does not match expected hash\" err\n\nI haven't really read the later parts of this series, but I noticed\nthat this triggers a test-lint check.  I'll be adding the following\nfixup in the meantime before continuing today's integration run.\n\nThanks.\n\n-- >8 --\nSubject: [PATCH] fixup! upload-pack: send part of packfile response as uri\n\n---\n t/t5702-protocol-v2.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh\nindex 041e78a88f..53bda39fb7 100755\n--- a/t/t5702-protocol-v2.sh\n+++ b/t/t5702-protocol-v2.sh\n@@ -829,8 +829,8 @@ test_expect_success 'fetching with valid packfile URI but invalid hash fails' '\n \t\t\"uploadpack.blobpackfileuri\" \\\n \t\t\"$(cat objh) $(cat objh) $HTTPD_URL/dumb/mypack-$(cat packh).pack\" &&\n \n-\tGIT_TEST_SIDEBAND_ALL=1 \\\n-\ttest_must_fail git -c protocol.version=2 \\\n+\ttest_must_fail env GIT_TEST_SIDEBAND_ALL=1 \\\n+\t\tgit -c protocol.version=2 \\\n \t\t-c fetch.uriprotocols=http,https \\\n \t\tclone \"$HTTPD_URL/smart/http_parent\" http_child 2>err &&\n \ttest_i18ngrep \"pack downloaded from.*does not match expected hash\" err\n-- \n2.27.0-rc2-6-g1aa69c7357\n\n"},{"id":"398957","messageId":"xmqq367gvwgw.fsf@gitster.c.googlers.com","threadId":"53583","inReplyTo":"2cfee363873736d9ff73cd38d96f3533bb49c904.1590789428.git.jonathantanmy@google.com","subject":"Re: [PATCH 8/8] upload-pack: send part of packfile response as uri","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-31T17:35:27Z","receivedAt":"2020-05-31T17:35:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan <jonathantanmy@google.com> writes:\n\n>  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     struct list_objects_filter_options *filter_options)\n> +\t\t\t     struct list_objects_filter_options *filter_options,\n> +\t\t\t     const struct string_list *uri_protocols)\n>  {\n\nI wanted to see why you rebased on top of 'next' to see possible\ninteractions with topics in-flight, and I found out that this series\nwas trivial to rebase on 'master'.\n\nThe codebase however is moving in the direction to reduce the number\nof parameters this function takes, and the above change does not\nplay well with the cc/upload-pack-data-2 topic that cleans up the\ncode around this area.\n\nCan you help review cc/upload-pack-data and cc/upload-pack-data-2\ntopics, as you'd eventually be basing your topic on top of the\nresult of merging these two clean-up topics to 'master'?  The former\nis already in 'next' after Peff's review, and after finding nothing\nglaringly wrong in it, I am not so worried about it, but the latter\nmay benefit from an extra set of eyes.\n\nThanks.\n\n\n"},{"id":"398993","messageId":"20200601200001.GB98003@google.com","threadId":"53583","inReplyTo":"2cfee363873736d9ff73cd38d96f3533bb49c904.1590789428.git.jonathantanmy@google.com","subject":"Re: [PATCH 8/8] upload-pack: send part of packfile response as uri","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2020-06-01T20:00:01Z","receivedAt":"2020-06-01T20:00:07Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jonathan Tan wrote:\n\n> +++ b/builtin/pack-objects.c\n[...]\n> @@ -192,6 +227,11 @@ static void create_pack_file(const struct object_array *have_obj,\n>  \t\t\t\t\t spec);\n>  \t\t}\n>  \t}\n> +\tif (uri_protocols) {\n> +\t\tfor (i = 0; i < uri_protocols->nr; i++)\n> +\t\t\targv_array_pushf(&pack_objects.args, \"--uri-protocol=%s\",\n> +\t\t\t\t\t uri_protocols->items[0].string);\n\nShould this be items[i]?\n\nThanks,\nJonathan\n"},{"id":"398995","messageId":"20200601203710.222245-1-jonathantanmy@google.com","threadId":"53583","inReplyTo":"xmqqmu5qxs6e.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 1/8] http: use --stdin when getting dumb HTTP pack","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-06-01T20:37:10Z","receivedAt":"2020-06-01T20:37:16Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> Jonathan Tan <jonathantanmy@google.com> writes:\n> \n> > When Git fetches a pack using dumb HTTP, it reuses the server's name for\n> > the packfile (which incorporates a hash), which is different from the\n> > behavior of fetch-pack and receive-pack.\n> \n> My first two reads of the above mistakenly thought that for some\n> reason the packfile has the URL to the server encoded in its name,\n> but that is not what you meant by \"the server's name\".  You rather\n> meant \"the name the server stores the packfile under\", \"the name the\n> server gave the packfile\", \"it reuses the name the server uses for\n> the packfile\".  The last rephrase may be the easiest to understand.\n\nOK - I'll use that.\n\n> > A subsequent patch will allow downloading packs over HTTP(S) as part of\n> > a fetch. These downloads will not necessarily be from a Git repository,\n> > and thus may not have a hash as part of its name.\n> \n> A location that is not necessarily a Git repository can still honor\n> the naming convention, so I find this a bit weak argument.  After\n> all, the procedure to prepare such a CDN backed file would be using\n> Git and the (git) \"natural\" name for the resulting packfile is\n> easily available to it, isn't it?\n> \n> I am not necessarily against loosening the limitation of the shape\n> of the filename, but we may want to say the reason why we want to\n> name the packfile on the CDN side differently from how Git would\n> naturally name them.  What benefit would we get out from geing able\n> to do so?  Perhaps it makes arrangements such as \"you can fetch\n> 'pack-v1.0.pack' to become reasonably up-to-date if you at least\n> have the version v1.0 software\", \"if the last time you fetched from\n> us was 2020-05-20 or after, you can fetch 'pack-2020-05-20.pack' and\n> be assured that you aren't missing anything\", etc.?  Such a \"why\n> would somebody want to name the packfile differently\" would make a\n> more convincing justification.\n\nI didn't want to unnecessarily exclude features like signed URLs which\nmay change the way the URL is - for example, in Google Cloud Storage,\nthe signed part is a suffix [1]. I'll include this in the commit\nmessage.\n\nHaving said that, after rereading my patch:\n\n (1) I'm not sure anymore if the restriction is that there must be a\n     hash in the filename. It might be just that the filename\n     must end in \".pack.temp\". (Having said that, if the filename was\n     not named \"<hash>.pack.temp\", it would look different to the rest\n     of the contents of \"objects/pack/\", which may or may not be fine.)\n\n (2) The filename restriction in question is on the local filename, not\n     the URL. We could do any manipulation we want on the URL (e.g. by\n     appending \".pack.temp\").\n\nAnd one idea that came up at $DAYJOB is that if we're using a suffix of\nthe URL as the filename, there may be a clash of names anyway, so we\nmight as well use the hash instead (which is reported by the server).\nI'll take a further look - maybe this patch will no longer be needed.\n\n[1] https://cloud.google.com/storage/docs/access-control/signed-urls\n"},{"id":"398997","messageId":"20200601205452.229381-1-jonathantanmy@google.com","threadId":"53583","inReplyTo":"xmqqeer2xr0f.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 3/8] http-fetch: support fetching packfiles by URL","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-06-01T20:54:52Z","receivedAt":"2020-06-01T20:54:56Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> > -\tif (argc != arg + 2 - commits_on_stdin)\n> > +\tif (argc != arg + 2 - (commits_on_stdin || packfile))\n> >  \t\tusage(http_fetch_usage);\n> >  \tif (commits_on_stdin) {\n> >  \t\tcommits = walker_targets_stdin(&commit_id, &write_ref);\n> > +\t} else if (packfile) {\n> > +\t\t/* URL will be set later */\n> \n> Prefer to see an empty statement spelled more explicitly, like this:\n> \n> \t\t; /* URL will be set later */\n> \n> Otherwise reader would be left wondering if a line was (or lines\n> were) accidentally lost after this comment.\n\nOK.\n\n> The above probably want to be a single helper function.\n> \n> The other side of if/else may also become another helper function.\n> \n> That way, the flow of control would become clearer.  After all,\n> these two branches do not share all that much.  Only http-init and\n> http-cleanup and nothing else.\n> \n> For that matter, even before introducing this new mode of operation,\n> another patch to make a preparatory move of the original logic in\n> this function to a helper function that would be called from the\n> \"else\" side may make it easier to see what is going on.\n\nOK.\n\n> > diff --git a/http.c b/http.c\n> > index 130e9d6259..ac66215ee6 100644\n> > --- a/http.c\n> > +++ b/http.c\n> > @@ -2280,15 +2280,18 @@ int finish_http_pack_request(struct http_pack_request *preq)\n> >  \tint tmpfile_fd;\n> >  \tint ret = 0;\n> >  \n> > -\tclose_pack_index(p);\n> > +\tif (p)\n> > +\t\tclose_pack_index(p);\n> >  \n> >  \tfclose(preq->packfile);\n> >  \tpreq->packfile = NULL;\n> >  \n> > -\tlst = preq->lst;\n> > -\twhile (*lst != p)\n> > -\t\tlst = &((*lst)->next);\n> > -\t*lst = (*lst)->next;\n> > +\tif (p) {\n> > +\t\tlst = preq->lst;\n> > +\t\twhile (*lst != p)\n> > +\t\t\tlst = &((*lst)->next);\n> > +\t\t*lst = (*lst)->next;\n> > +\t}\n> \n> This is quite ugly.  What is the original meaning of the target\n> field of the pack_request structure again?  A packed_git structure\n> that will be filled when we are done fetching the packfile from the\n> other side and installed in our repository?  When we are (ab)using\n> http_fetch code to fetch a single packfile, we do not install the\n> packfile into the running process, because we are only (re)using the\n> existing machinery as a poor-man's \"curl | git index-pack --stdin\"?\n> \n> I do not think it is a bad idea to roll \"curl | git index-pack\n> --stdin\" our own, but I do find this an ugly way to do so.  Perhaps\n> a set of lower-level helper functions can be isolated out of the\n> existing code before this new feature is added, and then a new \"just\n> fetch and pipe it to the index-pack\" feature should be written using\n> these helpers but with a separate set of entry points?  Would it be\n> a good way to make the resulting code cleaner than this patch does?\n> I dunno.\n\nOK - I'll extract the functions as you suggested earlier in the email,\nand then it might be more obvious if it can be done more cleanly.\n\n> > diff --git a/http.h b/http.h\n> > index a5b082f3ae..709dfa4c19 100644\n> > --- a/http.h\n> > +++ b/http.h\n> > @@ -223,12 +223,21 @@ struct http_pack_request {\n> >  \tstruct active_request_slot *slot;\n> >  \n> >  \t/*\n> > -\t * After calling new_http_pack_request(), point lst to the head of the\n> > +\t * After calling new_http_pack_request(), if fetching a pack that\n> > +\t * http_get_info_packs() told us about, point lst to the head of the\n> >  \t * pack list that target is in. finish_http_pack_request() will remove\n> >  \t * target from lst and call install_packed_git() on target.\n> >  \t */\n> >  \tstruct packed_git **lst;\n> >  \n> > +\t/*\n> > +\t * If this is true, finish_http_pack_request() will pass \"--keep\" to\n> > +\t * index-pack, resulting in the creation of a keep file, and will not\n> > +\t * suppress its stdout (that is, the \"keep\\t<hash>\\n\" line will be\n> > +\t * printed to stdout).\n> > +\t */\n> > +\tunsigned generate_keep : 1;\n> > +\n> \n> I suspect that this is a sign that this single patch is trying to\n> do too many things at the same time.\n> \n>  - Whether we are fetching a single packfile from a URL, or walking\n>    to fetch all the packfiles in the repository at a given URL\n> \n>  - Whether packfiles taken from outer space are marked with the\n>    \"keep\" bit\n> \n>  - Whether the obtained packfile(s) are internally \"installed\"\n>    to the running process\n> \n> are conceptually independent choices, but somehow mixed up, it\n> seems.\n\nYou might be right...I needed another mode that does the opposite\nchoices in these 3 points and these points because options that could be\ntoggled independently. I'll see if there is a better way to do this.\n"},{"id":"398998","messageId":"20200601205730.230613-1-jonathantanmy@google.com","threadId":"53583","inReplyTo":"xmqqa71qxqoy.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 4/8] Documentation: order protocol v2 sections","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-06-01T20:57:30Z","receivedAt":"2020-06-01T20:57:35Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> Jonathan Tan <jonathantanmy@google.com> writes:\n> \n> > The current C Git implementation expects Git servers to follow a\n> > specific order of sections when transmitting protocol v2 responses, but\n> > this is not explicit in the documentation. Make the order explicit.\n> \n> Thanks.  Are we breaking other people's implementation, or is this\n> the order all reimplementations of Git currently follow, so this is\n> purely an preemptive safety measure?\n\nI only know of the JGit implementation, and it currently follows the\nspecific order of sections (if not it wouldn't be able to interoperate\nwith the C client). So it's a preemptive safety measure. (If this series\ndoesn't go in for whatever reason, it might be worth just merging this\npatch on its own.)\n"},{"id":"399003","messageId":"20200601230749.257056-1-jonathantanmy@google.com","threadId":"53583","inReplyTo":"xmqq5zcexoqi.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 5/8] Documentation: add Packfile URIs design doc","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-06-01T23:07:49Z","receivedAt":"2020-06-01T23:07:55Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> > +Protocol\n> > +--------\n> > +\n> > +The server advertises `packfile-uris`.\n> \n> Is this a new \"protocol capability\"?  There are multiple things that\n> are \"advertised\" over the wire (there is \"reference advertisement\")\n> and readers would want to know if this is yet another kind of\n> advertisement or a new variety of the \"capability\".\n\nYes, it's a new protocol capability. I'll update the text.\n\n> > +If the client then communicates which protocols (HTTPS, etc.) it supports with\n> > +a `packfile-uris` argument, the server MAY send a `packfile-uris` section\n> > +directly before the `packfile` section (right after `wanted-refs` if it is\n> > +sent) containing URIs of any of the given protocols. The URIs point to\n> > +packfiles that use only features that the client has declared that it supports\n> > +(e.g. ofs-delta and thin-pack). 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> \n> I am guessing that this merely mean that the result of downloading\n> and installing the packfile does not necessarily make the resulting\n> repository up-to-date with respect to the \"want\" the \"fetch\" command\n> requested.  But the above can easily be misread that the returned\n> packfile is somewhat broken, corrupt or missing objects that it\n> ought to have (e.g. a deltified object lacks its base object in the\n> file). [#1]\n\nMost of this is resolved below, but I'll try to write upfront what's\ngoing on so it will be clear from the beginning (and not just at the\nend).\n\nBut you bring up a good point here - can one of the linked-by-URI packs\nhave a delta against the inline packfile, or vice versa? I'll take a\nlook and clarify that.\n\n> > +and that it needs to download all the given URIs before the fetch or clone is\n> > +complete.\n> \n> So if I say \"I want history leading to commit X\", and choose to use\n> the packfile-uris that told me to fetch two packfiles P and Q, does\n> it mean that I need to only fetch P and Q, index-pack them and store\n> the resulting two packfiles and their idx files in my repository, do\n> I have the history leading to commit X?\n> \n> I would have guessed that the resulting repository after fetching\n> these URIs can still be incomplete and the \"packfile\" section of the\n> response needs to be processed before the fetch or clone is complete,\n> but the above does not say so.\n\nI'll clarify the statement.\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=<sha1>\n> > +<uri>` 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> Let me see if the above is understandable.\n> \n> Does \"git cat-file blob <sha1>\" come back when we try to \"wget/curl\"\n> the <uri>?\n\nNo - a packfile containing a single object will be returned, not the\nuncompressed and headerless object. I'll update the text to clarify\nthat.\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-uris` parameter) by passing --no-packfile-uris to `git fetch`.\n> \n> By default, as long as the other side advertises packfile-uris, the\n> client automatically attempts to use it and users need to explicitly\n> disable it?  \n> \n> It's quite different from the way we introduce new features and I am\n> wondering if it is a good approach.\n\nThe client has to opt-in first with the fetch.uriprotocols config (which\nsays what protocols it wants to support) but it's not written here in\nthis spec. I'll add it.\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> \n> Did we discuss \"inclusion and exclusion sets\" whatever they are?\n\nNo - this is future/speculative so I didn't want to spend too much time\nexplaining this. I was thinking of saying that this URI includes all\nobjects from commit A (inclusion) but not from commits B and C\n(exclusion). Maybe I should just leave it out.\n\n> > diff --git a/Documentation/technical/protocol-v2.txt b/Documentation/technical/protocol-v2.txt\n> > index ef7514a3ee..7e1b3a0bfe 100644\n> > --- a/Documentation/technical/protocol-v2.txt\n> > +++ b/Documentation/technical/protocol-v2.txt\n> > @@ -325,13 +325,26 @@ included in the client's request:\n> >  \tindicating its sideband (1, 2, or 3), and the server may send \"0005\\2\"\n> >  \t(a PKT-LINE of sideband 2 with no payload) as a keepalive packet.\n> >  \n> > +If the 'packfile-uris' feature is advertised, the following argument\n> > +can be included in the client's request as well as the potential\n> > +addition of the 'packfile-uris' section in the server's response as\n> > +explained below.\n> > +\n> > +    packfile-uris <comma-separated list of protocols>\n> > +\tIndicates to the server that the client is willing to receive\n> > +\tURIs of any of the given protocols in place of objects in the\n> > +\tsent packfile. Before performing the connectivity check, the\n> > +\tclient should download from all given URIs. Currently, the\n> > +\tprotocols supported are \"http\" and \"https\".\n> \n> Ah, I think the puzzlement I repeatedly expressed above is starting\n> to dissolve.  You are assuming that the receiving end would remember\n> the URIs but the in-protocol packfile data at the end is handled\n> first, and then before the final connectivity check is done,\n> packfiles are downloaded from the URIs.  If you spelled out that\n> assumption early in the document, then I wouldn't have had to ask\n> all those questions.  I was assuming a different order (i.e. CDN\n> packfiles first to establish the baseline, and then in-protocol\n> packfile to fill the \"latest bits\", the last mile to reach the tips\n> of requested refs).\n> \n> In practice, I suspect that these fetches would go in parallel with\n> the processing of the in-protocol packfile, but spelling it out as\n> if these are done sequencially would help establishing the right\n> mental model.  \n\nOK.\n\n> \"(1) Process in-protocol packfiles first, and then (2) fetch CDN\n> URIs, and after all is done, (3) update the tips of refs\" would\n> serve as a base to establish a good mental model.  It would be even\n> better to throw in another step before all that: (0) record the\n> wanted-refs and CDN URIs to the safe place.  If you get disconnected\n> before finishing (1), you have to redo from the scratch, but once\n> you finished (0) and (1), then (2) and (3) can be done at your\n> leisure using the information you saved in step (0), and (1) can be\n> retried if your connection is lousy.\n\nOK. This set of patches does not do (0) yet, and I think doing so would\nrequire more design - in particular, if we have fetch resumption, we\nwould need to somehow track that none of the previously fetched objects\nhave been deleted.\n\nThanks for all your comments.\n"},{"id":"399004","messageId":"20200601231040.258250-1-jonathantanmy@google.com","threadId":"53583","inReplyTo":"xmqq1rn2xoe8.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 5/8] Documentation: add Packfile URIs design doc","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-06-01T23:10:40Z","receivedAt":"2020-06-01T23:10:46Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> Junio C Hamano <gitster@pobox.com> writes:\n> \n> > In practice, I suspect that these fetches would go in parallel with\n> > the processing of the in-protocol packfile, but spelling it out as\n> > if these are done sequencially would help establishing the right\n> > mental model.  \n> >\n> > \"(1) Process in-protocol packfiles first, and then (2) fetch CDN\n> > URIs, and after all is done, (3) update the tips of refs\" would\n> > serve as a base to establish a good mental model.  It would be even\n> > better to throw in another step before all that: (0) record the\n> > wanted-refs and CDN URIs to the safe place.  If you get disconnected\n> > before finishing (1), you have to redo from the scratch, but once\n> > you finished (0) and (1), then (2) and (3) can be done at your\n> > leisure using the information you saved in step (0), and (1) can be\n> > retried if your connection is lousy.\n> \n> We need to be a bit careful here.  After finishing (0) and (1), the\n> most recent history near the requested tips is not anchored by any\n> ref.  We of course cannot point these \"most recent\" objects with\n> refs because it is very likely that they are not connected to the\n> parts of the history we already have in the receiving repository.\n> The huge gap exists to be filled by the bulk download from CDN.\n> \n> So a GC that happens before (3) completes can discard object data\n> obtained in step (1).  One way to protect it may be to use a .keep\n> file but then some procedure needs to be there to remove it once we\n> are done.  Perhaps at the end of (1), the name of that .keep file is\n> added to the set of information we keep until (3) happens (the\n> remainder of the set of information was obtained in step (0)).\n\nYes, this is precisely what we're doing - the packs obtained through the\npackfile URIs are all written with keep files, and the names of the keep\nfiles are added to a list. They are then deleted at the same time that\nthe regular keep file (the one generated during an ordinary fetch) is\ndeleted.\n\nI'll also add this information to the spec.\n"},{"id":"399005","messageId":"20200601232031.261207-1-jonathantanmy@google.com","threadId":"53583","inReplyTo":"xmqq367gvwgw.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 8/8] upload-pack: send part of packfile response as uri","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-06-01T23:20:31Z","receivedAt":"2020-06-01T23:20:35Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> Jonathan Tan <jonathantanmy@google.com> writes:\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     struct list_objects_filter_options *filter_options)\n> > +\t\t\t     struct list_objects_filter_options *filter_options,\n> > +\t\t\t     const struct string_list *uri_protocols)\n> >  {\n> \n> I wanted to see why you rebased on top of 'next' to see possible\n> interactions with topics in-flight, and I found out that this series\n> was trivial to rebase on 'master'.\n> \n> The codebase however is moving in the direction to reduce the number\n> of parameters this function takes, and the above change does not\n> play well with the cc/upload-pack-data-2 topic that cleans up the\n> code around this area.\n> \n> Can you help review cc/upload-pack-data and cc/upload-pack-data-2\n> topics, as you'd eventually be basing your topic on top of the\n> result of merging these two clean-up topics to 'master'?  The former\n> is already in 'next' after Peff's review, and after finding nothing\n> glaringly wrong in it, I am not so worried about it, but the latter\n> may benefit from an extra set of eyes.\n> \n> Thanks.\n\nOK - I'll take a look tomorrow.\n"},{"id":"399382","messageId":"20200610011406.178334-1-jonathantanmy@google.com","threadId":"53583","inReplyTo":"xmqq5zcexoqi.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 5/8] Documentation: add Packfile URIs design doc","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-06-10T01:14:06Z","receivedAt":"2020-06-10T01:14:16Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> > @@ -349,6 +362,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(40*(HEXDIGIT) SP *%x20-ff LF)\n> \n> 40* \n\nI'm almost ready to send out an updated version, but have one question:\nwhat do you mean by this? If you mean that I should use \"obj-id\"\ninstead, I didn't want to because it's not the hash of an object, but\nthe hash of a packfile.\n"},{"id":"399423","messageId":"xmqqeeqmg7ty.fsf@gitster.c.googlers.com","threadId":"53583","inReplyTo":"20200610011406.178334-1-jonathantanmy@google.com","subject":"Re: [PATCH 5/8] Documentation: add Packfile URIs design doc","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-06-10T17:16:09Z","receivedAt":"2020-06-10T17:16:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan <jonathantanmy@google.com> writes:\n\n>> > @@ -349,6 +362,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(40*(HEXDIGIT) SP *%x20-ff LF)\n>> \n>> 40* \n>\n> I'm almost ready to send out an updated version, but have one question:\n> what do you mean by this? If you mean that I should use \"obj-id\"\n> instead, I didn't want to because it's not the hash of an object, but\n> the hash of a packfile.\n\nIt clearly is not an object name, but it is a run of hexdigits whose\nlength is the same as (hexadecimal representation of) the object name.\n\nHow is \"obj-id\" we see above in the precontext of that hunk defined?\nDoes it use 40*(HEXDIGIT), too?  Do we plan to support non SHA-1 hashes\nin this design in the future, and if so how?\n\n\"We are only focused on SHA-1 hashes for now\" is a perfectly\nacceptable answer, and then 40* here makes 100% sense, but then we'd\nneed to say \"for now this design only assumes SHA-1 hash\" upfront, I\nwould think, to remind ourselves that we need to consider this part\nof the system when we upgrade to SHA-256.\n\nThanks.\n"},{"id":"399429","messageId":"20200610180401.71146-1-jonathantanmy@google.com","threadId":"53583","inReplyTo":"xmqqeeqmg7ty.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 5/8] Documentation: add Packfile URIs design doc","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-06-10T18:04:01Z","receivedAt":"2020-06-10T18:04:07Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> Jonathan Tan <jonathantanmy@google.com> writes:\n> \n> >> > @@ -349,6 +362,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(40*(HEXDIGIT) SP *%x20-ff LF)\n> >> \n> >> 40* \n> >\n> > I'm almost ready to send out an updated version, but have one question:\n> > what do you mean by this? If you mean that I should use \"obj-id\"\n> > instead, I didn't want to because it's not the hash of an object, but\n> > the hash of a packfile.\n> \n> It clearly is not an object name, but it is a run of hexdigits whose\n> length is the same as (hexadecimal representation of) the object name.\n> \n> How is \"obj-id\" we see above in the precontext of that hunk defined?\n> Does it use 40*(HEXDIGIT), too?  \n\nYes, it's defined as such in protocol-common.txt:\n\n  obj-id    =  40*(HEXDIGIT)\n\n> Do we plan to support non SHA-1 hashes\n> in this design in the future, and if so how?\n> \n> \"We are only focused on SHA-1 hashes for now\" is a perfectly\n> acceptable answer, and then 40* here makes 100% sense, but then we'd\n> need to say \"for now this design only assumes SHA-1 hash\" upfront, I\n> would think, to remind ourselves that we need to consider this part\n> of the system when we upgrade to SHA-256.\n\nThis will be whatever is output by index-pack after \"pack\\t\" or\n\"keep\\t\". I'll make a note in the version I'm about to send out. Yes,\nwe'll definitely need to remind ourselves about considering this part\nwhen we upgrade.\n"},{"id":"399440","messageId":"cover.1591821067.git.jonathantanmy@google.com","threadId":"53583","inReplyTo":"cover.1590789428.git.jonathantanmy@google.com","subject":"[PATCH v2 0/9] CDN offloading update","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-06-10T20:57:14Z","receivedAt":"2020-06-10T20:57:32Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Thanks, Junio, for taking a look at this.\n\nThis version is based on cc/upload-pack-data-2. I thought of retaining\nthe base from the original version, but trying out the rebase showed\nthat there were some code movements that would clutter the range-diff\nanyway, so I went ahead with the rebase.\n\nI tried to avoid the need for the first patch, but it turns out that\nindex-pack can operate with custom names for .pack and .idx\n(\".pack.temp\" and \".idx.temp\", as used by http-fetch) but not .keep, and\nwe need .keep, so changing to --stdin instead of using custom names is\nnecessary anyway. I've updated the commit message to use that\njustification.\n\nThe main change is Junio's suggestion [1] to refactor into smaller\nfunctions. Quoting from [1]:\n\n>  - Whether we are fetching a single packfile from a URL, or walking\n>    to fetch all the packfiles in the repository at a given URL\n> \n>  - Whether packfiles taken from outer space are marked with the\n>    \"keep\" bit\n> \n>  - Whether the obtained packfile(s) are internally \"installed\"\n>    to the running process\n\nTo which we can add:\n\n 4. Whether to close the index mmap\n 5. Whether to delete the packfile from the given list\n\nThe 3rd, 4th, and 5th concerns were refactored in patch 2, and the\nothers were refactored in patch 4.\n\nI also updated the documentation to hopefully make it clearer that the\nclient should download all packfiles, including the inline packfile,\nbefore performing the connectivity check.\n\n[1] https://lore.kernel.org/git/xmqqeer2xr0f.fsf@gitster.c.googlers.com/\n\nJonathan Tan (9):\n  http: use --stdin when indexing dumb HTTP pack\n  http: refactor finish_http_pack_request()\n  http-fetch: refactor into function\n  http-fetch: support fetching packfiles by URL\n  Documentation: order protocol v2 sections\n  Documentation: add Packfile URIs design doc\n  upload-pack: refactor reading of pack-objects out\n  fetch-pack: support more than one pack lockfile\n  upload-pack: send part of packfile response as uri\n\n Documentation/git-http-fetch.txt         |   9 +-\n Documentation/technical/packfile-uri.txt |  78 +++++++++++\n Documentation/technical/protocol-v2.txt  |  48 +++++--\n builtin/fetch-pack.c                     |  17 ++-\n builtin/pack-objects.c                   |  76 +++++++++++\n connected.c                              |   8 +-\n fetch-pack.c                             | 137 +++++++++++++++++---\n fetch-pack.h                             |   2 +-\n http-fetch.c                             | 126 +++++++++++++-----\n http-push.c                              |   8 +-\n http-walker.c                            |   5 +-\n http.c                                   |  82 ++++++------\n http.h                                   |  24 +++-\n t/t5550-http-fetch-dumb.sh               |  30 +++++\n t/t5702-protocol-v2.sh                   |  88 +++++++++++++\n transport-helper.c                       |   5 +-\n transport.c                              |  14 +-\n transport.h                              |   6 +-\n upload-pack.c                            | 157 +++++++++++++++++------\n 19 files changed, 752 insertions(+), 168 deletions(-)\n create mode 100644 Documentation/technical/packfile-uri.txt\n\n-- \n2.27.0.278.ge193c7cf3a9-goog\n\n"},{"id":"399441","messageId":"d3b27394cad951a9f1412a04572439513d7e9f4d.1591821067.git.jonathantanmy@google.com","threadId":"53583","inReplyTo":"cover.1591821067.git.jonathantanmy@google.com","subject":"[PATCH v2 1/9] http: use --stdin when indexing dumb HTTP pack","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-06-10T20:57:15Z","receivedAt":"2020-06-10T20:57:34Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"When Git fetches a pack using dumb HTTP, (among other things) it invokes\nindex-pack on a \".pack.temp\" packfile, specifying the filename as an\nargument.\n\nA future commit will require the aforementioned invocation of index-pack\nto also generate a \"keep\" file. To use this, we either have to use\nindex-pack's naming convention (because --keep requires the pack's\nfilename to end with \".pack\") or to pass the pack through stdin. Of the\ntwo, it is simpler to pass the pack through stdin.\n\nThus, teach http to pass --stdin to index-pack. As a bonus, the code is\nnow simpler.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\n http.c | 33 +++++++++++----------------------\n 1 file changed, 11 insertions(+), 22 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex 62aa995245..39cbd56702 100644\n--- a/http.c\n+++ b/http.c\n@@ -2270,9 +2270,9 @@ int finish_http_pack_request(struct http_pack_request *preq)\n {\n \tstruct packed_git **lst;\n \tstruct packed_git *p = preq->target;\n-\tchar *tmp_idx;\n-\tsize_t len;\n \tstruct child_process ip = CHILD_PROCESS_INIT;\n+\tint tmpfile_fd;\n+\tint ret = 0;\n \n \tclose_pack_index(p);\n \n@@ -2284,35 +2284,24 @@ int finish_http_pack_request(struct http_pack_request *preq)\n \t\tlst = &((*lst)->next);\n \t*lst = (*lst)->next;\n \n-\tif (!strip_suffix(preq->tmpfile.buf, \".pack.temp\", &len))\n-\t\tBUG(\"pack tmpfile does not end in .pack.temp?\");\n-\ttmp_idx = xstrfmt(\"%.*s.idx.temp\", (int)len, preq->tmpfile.buf);\n+\ttmpfile_fd = xopen(preq->tmpfile.buf, O_RDONLY);\n \n \targv_array_push(&ip.args, \"index-pack\");\n-\targv_array_pushl(&ip.args, \"-o\", tmp_idx, NULL);\n-\targv_array_push(&ip.args, preq->tmpfile.buf);\n+\targv_array_push(&ip.args, \"--stdin\");\n \tip.git_cmd = 1;\n-\tip.no_stdin = 1;\n+\tip.in = tmpfile_fd;\n \tip.no_stdout = 1;\n \n \tif (run_command(&ip)) {\n-\t\tunlink(preq->tmpfile.buf);\n-\t\tunlink(tmp_idx);\n-\t\tfree(tmp_idx);\n-\t\treturn -1;\n-\t}\n-\n-\tunlink(sha1_pack_index_name(p->hash));\n-\n-\tif (finalize_object_file(preq->tmpfile.buf, sha1_pack_name(p->hash))\n-\t || finalize_object_file(tmp_idx, sha1_pack_index_name(p->hash))) {\n-\t\tfree(tmp_idx);\n-\t\treturn -1;\n+\t\tret = -1;\n+\t\tgoto cleanup;\n \t}\n \n \tinstall_packed_git(the_repository, p);\n-\tfree(tmp_idx);\n-\treturn 0;\n+cleanup:\n+\tclose(tmpfile_fd);\n+\tunlink(preq->tmpfile.buf);\n+\treturn ret;\n }\n \n struct http_pack_request *new_http_pack_request(\n-- \n2.27.0.278.ge193c7cf3a9-goog\n\n"},{"id":"399442","messageId":"048f6845669570edbf7de3da7f496c4e96592aad.1591821067.git.jonathantanmy@google.com","threadId":"53583","inReplyTo":"cover.1591821067.git.jonathantanmy@google.com","subject":"[PATCH v2 2/9] http: refactor finish_http_pack_request()","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-06-10T20:57:16Z","receivedAt":"2020-06-10T20:57:35Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"finish_http_pack_request() does multiple tasks, including some\nhousekeeping on a struct packed_git - (1) closing its index, (2)\nremoving it from a list, and (3) installing it. These concerns are\nindependent of fetching a pack through HTTP: they are there only because\n(1) the calling code opens the pack's index before deciding to fetch it,\n(2) the calling code maintains a list of packfiles that can be fetched,\nand (3) the calling code fetches it in order to make use of its objects\nin the same process.\n\nIn preparation for a subsequent commit, which adds a feature that does\nnot need any of this housekeeping, remove (1), (2), and (3) from\nfinish_http_pack_request(). (2) and (3) are now done by a helper\nfunction, and (1) is the responsibility of the caller (in this patch,\ndone closer to the point where the pack index is opened).\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\n http-push.c   |  8 ++++++--\n http-walker.c |  5 +++--\n http.c        | 31 ++++++++++++++++---------------\n http.h        | 13 ++++++++++---\n 4 files changed, 35 insertions(+), 22 deletions(-)\n\ndiff --git a/http-push.c b/http-push.c\nindex 822f326599..ac7868ffee 100644\n--- a/http-push.c\n+++ b/http-push.c\n@@ -117,6 +117,7 @@ enum transfer_state {\n \n struct transfer_request {\n \tstruct object *obj;\n+\tstruct packed_git *target;\n \tchar *url;\n \tchar *dest;\n \tstruct remote_lock *lock;\n@@ -314,17 +315,18 @@ static void start_fetch_packed(struct transfer_request *request)\n \t\trelease_request(request);\n \t\treturn;\n \t}\n+\tclose_pack_index(target);\n+\trequest->target = target;\n \n \tfprintf(stderr,\t\"Fetching pack %s\\n\",\n \t\thash_to_hex(target->hash));\n \tfprintf(stderr, \" which contains %s\\n\", oid_to_hex(&request->obj->oid));\n \n-\tpreq = new_http_pack_request(target, repo->url);\n+\tpreq = new_http_pack_request(target->hash, repo->url);\n \tif (preq == NULL) {\n \t\trepo->can_update_info_refs = 0;\n \t\treturn;\n \t}\n-\tpreq->lst = &repo->packs;\n \n \t/* Make sure there isn't another open request for this pack */\n \twhile (check_request) {\n@@ -597,6 +599,8 @@ static void finish_request(struct transfer_request *request)\n \t\t}\n \t\tif (fail)\n \t\t\trepo->can_update_info_refs = 0;\n+\t\telse\n+\t\t\thttp_install_packfile(request->target, &repo->packs);\n \t\trelease_request(request);\n \t}\n }\ndiff --git a/http-walker.c b/http-walker.c\nindex fe15e325fa..4fb1235cd4 100644\n--- a/http-walker.c\n+++ b/http-walker.c\n@@ -439,6 +439,7 @@ static int http_fetch_pack(struct walker *walker, struct alt_base *repo, unsigne\n \ttarget = find_sha1_pack(sha1, repo->packs);\n \tif (!target)\n \t\treturn -1;\n+\tclose_pack_index(target);\n \n \tif (walker->get_verbosely) {\n \t\tfprintf(stderr, \"Getting pack %s\\n\",\n@@ -447,10 +448,9 @@ static int http_fetch_pack(struct walker *walker, struct alt_base *repo, unsigne\n \t\t\thash_to_hex(sha1));\n \t}\n \n-\tpreq = new_http_pack_request(target, repo->base);\n+\tpreq = new_http_pack_request(target->hash, repo->base);\n \tif (preq == NULL)\n \t\tgoto abort;\n-\tpreq->lst = &repo->packs;\n \tpreq->slot->results = &results;\n \n \tif (start_active_slot(preq->slot)) {\n@@ -469,6 +469,7 @@ static int http_fetch_pack(struct walker *walker, struct alt_base *repo, unsigne\n \trelease_http_pack_request(preq);\n \tif (ret)\n \t\treturn ret;\n+\thttp_install_packfile(target, &repo->packs);\n \n \treturn 0;\n \ndiff --git a/http.c b/http.c\nindex 39cbd56702..4f6e1fb018 100644\n--- a/http.c\n+++ b/http.c\n@@ -2268,22 +2268,13 @@ void release_http_pack_request(struct http_pack_request *preq)\n \n int finish_http_pack_request(struct http_pack_request *preq)\n {\n-\tstruct packed_git **lst;\n-\tstruct packed_git *p = preq->target;\n \tstruct child_process ip = CHILD_PROCESS_INIT;\n \tint tmpfile_fd;\n \tint ret = 0;\n \n-\tclose_pack_index(p);\n-\n \tfclose(preq->packfile);\n \tpreq->packfile = NULL;\n \n-\tlst = preq->lst;\n-\twhile (*lst != p)\n-\t\tlst = &((*lst)->next);\n-\t*lst = (*lst)->next;\n-\n \ttmpfile_fd = xopen(preq->tmpfile.buf, O_RDONLY);\n \n \targv_array_push(&ip.args, \"index-pack\");\n@@ -2297,15 +2288,26 @@ int finish_http_pack_request(struct http_pack_request *preq)\n \t\tgoto cleanup;\n \t}\n \n-\tinstall_packed_git(the_repository, p);\n cleanup:\n \tclose(tmpfile_fd);\n \tunlink(preq->tmpfile.buf);\n \treturn ret;\n }\n \n+void http_install_packfile(struct packed_git *p,\n+\t\t\t   struct packed_git **list_to_remove_from)\n+{\n+\tstruct packed_git **lst = list_to_remove_from;\n+\n+\twhile (*lst != p)\n+\t\tlst = &((*lst)->next);\n+\t*lst = (*lst)->next;\n+\n+\tinstall_packed_git(the_repository, p);\n+}\n+\n struct http_pack_request *new_http_pack_request(\n-\tstruct packed_git *target, const char *base_url)\n+\tconst unsigned char *packed_git_hash, const char *base_url)\n {\n \toff_t prev_posn = 0;\n \tstruct strbuf buf = STRBUF_INIT;\n@@ -2313,14 +2315,13 @@ struct http_pack_request *new_http_pack_request(\n \n \tpreq = xcalloc(1, sizeof(*preq));\n \tstrbuf_init(&preq->tmpfile, 0);\n-\tpreq->target = target;\n \n \tend_url_with_slash(&buf, base_url);\n \tstrbuf_addf(&buf, \"objects/pack/pack-%s.pack\",\n-\t\thash_to_hex(target->hash));\n+\t\thash_to_hex(packed_git_hash));\n \tpreq->url = strbuf_detach(&buf, NULL);\n \n-\tstrbuf_addf(&preq->tmpfile, \"%s.temp\", sha1_pack_name(target->hash));\n+\tstrbuf_addf(&preq->tmpfile, \"%s.temp\", sha1_pack_name(packed_git_hash));\n \tpreq->packfile = fopen(preq->tmpfile.buf, \"a\");\n \tif (!preq->packfile) {\n \t\terror(\"Unable to open local file %s for pack\",\n@@ -2344,7 +2345,7 @@ struct http_pack_request *new_http_pack_request(\n \t\tif (http_is_verbose)\n \t\t\tfprintf(stderr,\n \t\t\t\t\"Resuming fetch of pack %s at byte %\"PRIuMAX\"\\n\",\n-\t\t\t\thash_to_hex(target->hash),\n+\t\t\t\thash_to_hex(packed_git_hash),\n \t\t\t\t(uintmax_t)prev_posn);\n \t\thttp_opt_request_remainder(preq->slot->curl, prev_posn);\n \t}\ndiff --git a/http.h b/http.h\nindex 5e0ad724f9..bbc6b070f1 100644\n--- a/http.h\n+++ b/http.h\n@@ -216,18 +216,25 @@ int http_get_info_packs(const char *base_url,\n \n struct http_pack_request {\n \tchar *url;\n-\tstruct packed_git *target;\n-\tstruct packed_git **lst;\n \tFILE *packfile;\n \tstruct strbuf tmpfile;\n \tstruct active_request_slot *slot;\n };\n \n struct http_pack_request *new_http_pack_request(\n-\tstruct packed_git *target, const char *base_url);\n+\tconst unsigned char *packed_git_hash, const char *base_url);\n int finish_http_pack_request(struct http_pack_request *preq);\n void release_http_pack_request(struct http_pack_request *preq);\n \n+/*\n+ * Remove p from the given list, and invoke install_packed_git() on it.\n+ *\n+ * This is a convenience function for users that have obtained a list of packs\n+ * from http_get_info_packs() and have chosen a specific pack to fetch.\n+ */\n+void http_install_packfile(struct packed_git *p,\n+\t\t\t   struct packed_git **list_to_remove_from);\n+\n /* Helpers for fetching object */\n struct http_object_request {\n \tchar *url;\n-- \n2.27.0.278.ge193c7cf3a9-goog\n\n"},{"id":"399443","messageId":"7a9b454bd609aaa7d9909c70739603add5c038e5.1591821067.git.jonathantanmy@google.com","threadId":"53583","inReplyTo":"cover.1591821067.git.jonathantanmy@google.com","subject":"[PATCH v2 3/9] http-fetch: refactor into function","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-06-10T20:57:17Z","receivedAt":"2020-06-10T20:57:38Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"cmd_main() in http-fetch.c will grow in a future patch, so refactor the\nHTTP walking part into its own function.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\n http-fetch.c | 69 +++++++++++++++++++++++++++++++---------------------\n 1 file changed, 41 insertions(+), 28 deletions(-)\n\ndiff --git a/http-fetch.c b/http-fetch.c\nindex a32ac118d9..e538174bde 100644\n--- a/http-fetch.c\n+++ b/http-fetch.c\n@@ -7,16 +7,49 @@\n static const char http_fetch_usage[] = \"git http-fetch \"\n \"[-c] [-t] [-a] [-v] [--recover] [-w ref] [--stdin] commit-id url\";\n \n-int cmd_main(int argc, const char **argv)\n+static int fetch_using_walker(const char *raw_url, int get_verbosely,\n+\t\t\t      int get_recover, int commits, char **commit_id,\n+\t\t\t      const char **write_ref, int commits_on_stdin)\n {\n+\tchar *url = NULL;\n \tstruct walker *walker;\n+\tint rc;\n+\n+\tstr_end_url_with_slash(raw_url, &url);\n+\n+\thttp_init(NULL, url, 0);\n+\n+\twalker = get_http_walker(url);\n+\twalker->get_verbosely = get_verbosely;\n+\twalker->get_recover = get_recover;\n+\twalker->get_progress = 0;\n+\n+\trc = walker_fetch(walker, commits, commit_id, write_ref, url);\n+\n+\tif (commits_on_stdin)\n+\t\twalker_targets_free(commits, commit_id, write_ref);\n+\n+\tif (walker->corrupt_object_found) {\n+\t\tfprintf(stderr,\n+\"Some loose object were found to be corrupt, but they might be just\\n\"\n+\"a false '404 Not Found' error message sent with incorrect HTTP\\n\"\n+\"status code.  Suggest running 'git fsck'.\\n\");\n+\t}\n+\n+\twalker_free(walker);\n+\thttp_cleanup();\n+\tfree(url);\n+\n+\treturn rc;\n+}\n+\n+int cmd_main(int argc, const char **argv)\n+{\n \tint commits_on_stdin = 0;\n \tint commits;\n \tconst char **write_ref = NULL;\n \tchar **commit_id;\n-\tchar *url = NULL;\n \tint arg = 1;\n-\tint rc = 0;\n \tint get_verbosely = 0;\n \tint get_recover = 0;\n \n@@ -47,34 +80,14 @@ int cmd_main(int argc, const char **argv)\n \t\tcommits = 1;\n \t}\n \n-\tif (argv[arg])\n-\t\tstr_end_url_with_slash(argv[arg], &url);\n-\n \tsetup_git_directory();\n \n \tgit_config(git_default_config, NULL);\n \n-\thttp_init(NULL, url, 0);\n-\twalker = get_http_walker(url);\n-\twalker->get_verbosely = get_verbosely;\n-\twalker->get_recover = get_recover;\n-\n-\trc = walker_fetch(walker, commits, commit_id, write_ref, url);\n-\n-\tif (commits_on_stdin)\n-\t\twalker_targets_free(commits, commit_id, write_ref);\n+\tif (!argv[arg])\n+\t\tBUG(\"must have one arg remaining\");\n \n-\tif (walker->corrupt_object_found) {\n-\t\tfprintf(stderr,\n-\"Some loose object were found to be corrupt, but they might be just\\n\"\n-\"a false '404 Not Found' error message sent with incorrect HTTP\\n\"\n-\"status code.  Suggest running 'git fsck'.\\n\");\n-\t}\n-\n-\twalker_free(walker);\n-\thttp_cleanup();\n-\n-\tfree(url);\n-\n-\treturn rc;\n+\treturn fetch_using_walker(argv[arg], get_verbosely, get_recover,\n+\t\t\t\t  commits, commit_id, write_ref,\n+\t\t\t\t  commits_on_stdin);\n }\n-- \n2.27.0.278.ge193c7cf3a9-goog\n\n"},{"id":"399444","messageId":"6994b22b0fd36ece6cf6d9967f0d5c1f8f9481ac.1591821067.git.jonathantanmy@google.com","threadId":"53583","inReplyTo":"cover.1591821067.git.jonathantanmy@google.com","subject":"[PATCH v2 5/9] Documentation: order protocol v2 sections","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-06-10T20:57:19Z","receivedAt":"2020-06-10T20:57:40Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"The current C Git implementation expects Git servers to follow a\nspecific order of sections when transmitting protocol v2 responses, but\nthis is not explicit in the documentation. Make the order explicit.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\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 7e3766cafb..995f07481e 100644\n--- a/Documentation/technical/protocol-v2.txt\n+++ b/Documentation/technical/protocol-v2.txt\n@@ -325,11 +325,11 @@ included in the client's request:\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@@ -351,9 +351,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@@ -404,9 +405,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.27.0.278.ge193c7cf3a9-goog\n\n"},{"id":"399445","messageId":"a4a7ed512b251fb16f0b2400fac8c0c5cc14aa75.1591821067.git.jonathantanmy@google.com","threadId":"53583","inReplyTo":"cover.1591821067.git.jonathantanmy@google.com","subject":"[PATCH v2 4/9] http-fetch: support fetching packfiles by URL","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-06-10T20:57:18Z","receivedAt":"2020-06-10T20:57:41Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Teach http-fetch the ability to download packfiles directly, given a\nURL, and to verify them.\n\nThe http_pack_request suite has been augmented with a function that\ntakes a URL directly. With this function, the hash is only used to\ndetermine the name of the temporary file.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\n Documentation/git-http-fetch.txt |  9 ++++-\n http-fetch.c                     | 63 +++++++++++++++++++++++++++-----\n http.c                           | 28 ++++++++++----\n http.h                           | 11 ++++++\n t/t5550-http-fetch-dumb.sh       | 30 +++++++++++++++\n 5 files changed, 123 insertions(+), 18 deletions(-)\n\ndiff --git a/Documentation/git-http-fetch.txt b/Documentation/git-http-fetch.txt\nindex 666b042679..4deb4893f5 100644\n--- a/Documentation/git-http-fetch.txt\n+++ b/Documentation/git-http-fetch.txt\n@@ -9,7 +9,7 @@ git-http-fetch - Download from a remote Git repository via HTTP\n SYNOPSIS\n --------\n [verse]\n-'git http-fetch' [-c] [-t] [-a] [-d] [-v] [-w filename] [--recover] [--stdin] <commit> <url>\n+'git http-fetch' [-c] [-t] [-a] [-d] [-v] [-w filename] [--recover] [--stdin | --packfile=<hash> | <commit>] <url>\n \n DESCRIPTION\n -----------\n@@ -40,6 +40,13 @@ commit-id::\n \n \t\t<commit-id>['\\t'<filename-as-in--w>]\n \n+--packfile=<hash>::\n+\tInstead of a commit id on the command line (which is not expected in\n+\tthis case), 'git http-fetch' fetches the packfile directly at the given\n+\tURL and uses index-pack to generate corresponding .idx and .keep files.\n+\tThe hash is used to determine the name of the temporary file and is\n+\tarbitrary. The output of index-pack is printed to stdout.\n+\n --recover::\n \tVerify that everything reachable from target is fetched.  Used after\n \tan earlier fetch is interrupted.\ndiff --git a/http-fetch.c b/http-fetch.c\nindex e538174bde..1df376e745 100644\n--- a/http-fetch.c\n+++ b/http-fetch.c\n@@ -5,7 +5,7 @@\n #include \"walker.h\"\n \n static const char http_fetch_usage[] = \"git http-fetch \"\n-\"[-c] [-t] [-a] [-v] [--recover] [-w ref] [--stdin] commit-id url\";\n+\"[-c] [-t] [-a] [-v] [--recover] [-w ref] [--stdin | --packfile=hash | commit-id] url\";\n \n static int fetch_using_walker(const char *raw_url, int get_verbosely,\n \t\t\t      int get_recover, int commits, char **commit_id,\n@@ -43,6 +43,37 @@ static int fetch_using_walker(const char *raw_url, int get_verbosely,\n \treturn rc;\n }\n \n+static void fetch_single_packfile(struct object_id *packfile_hash,\n+\t\t\t\t  const char *url) {\n+\tstruct http_pack_request *preq;\n+\tstruct slot_results results;\n+\tint ret;\n+\n+\thttp_init(NULL, url, 0);\n+\n+\tpreq = new_direct_http_pack_request(packfile_hash->hash, xstrdup(url));\n+\tif (preq == NULL)\n+\t\tdie(\"couldn't create http pack request\");\n+\tpreq->slot->results = &results;\n+\tpreq->generate_keep = 1;\n+\n+\tif (start_active_slot(preq->slot)) {\n+\t\trun_active_slot(preq->slot);\n+\t\tif (results.curl_result != CURLE_OK) {\n+\t\t\tdie(\"Unable to get pack file %s\\n%s\", preq->url,\n+\t\t\t    curl_errorstr);\n+\t\t}\n+\t} else {\n+\t\tdie(\"Unable to start request\");\n+\t}\n+\n+\tif ((ret = finish_http_pack_request(preq)))\n+\t\tdie(\"finish_http_pack_request gave result %d\", ret);\n+\n+\trelease_http_pack_request(preq);\n+\thttp_cleanup();\n+}\n+\n int cmd_main(int argc, const char **argv)\n {\n \tint commits_on_stdin = 0;\n@@ -52,8 +83,12 @@ int cmd_main(int argc, const char **argv)\n \tint arg = 1;\n \tint get_verbosely = 0;\n \tint get_recover = 0;\n+\tint packfile = 0;\n+\tstruct object_id packfile_hash;\n \n \twhile (arg < argc && argv[arg][0] == '-') {\n+\t\tconst char *p;\n+\n \t\tif (argv[arg][1] == 't') {\n \t\t} else if (argv[arg][1] == 'c') {\n \t\t} else if (argv[arg][1] == 'a') {\n@@ -68,25 +103,33 @@ int cmd_main(int argc, const char **argv)\n \t\t\tget_recover = 1;\n \t\t} else if (!strcmp(argv[arg], \"--stdin\")) {\n \t\t\tcommits_on_stdin = 1;\n+\t\t} else if (skip_prefix(argv[arg], \"--packfile=\", &p)) {\n+\t\t\tconst char *end;\n+\n+\t\t\tpackfile = 1;\n+\t\t\tif (parse_oid_hex(p, &packfile_hash, &end) || *end)\n+\t\t\t\tdie(_(\"argument to --packfile must be a valid hash (got '%s')\"), p);\n \t\t}\n \t\targ++;\n \t}\n-\tif (argc != arg + 2 - commits_on_stdin)\n+\tif (argc != arg + 2 - (commits_on_stdin || packfile))\n \t\tusage(http_fetch_usage);\n-\tif (commits_on_stdin) {\n-\t\tcommits = walker_targets_stdin(&commit_id, &write_ref);\n-\t} else {\n-\t\tcommit_id = (char **) &argv[arg++];\n-\t\tcommits = 1;\n-\t}\n \n \tsetup_git_directory();\n \n \tgit_config(git_default_config, NULL);\n \n-\tif (!argv[arg])\n-\t\tBUG(\"must have one arg remaining\");\n+\tif (packfile) {\n+\t\tfetch_single_packfile(&packfile_hash, argv[arg]);\n+\t\treturn 0;\n+\t}\n \n+\tif (commits_on_stdin) {\n+\t\tcommits = walker_targets_stdin(&commit_id, &write_ref);\n+\t} else {\n+\t\tcommit_id = (char **) &argv[arg++];\n+\t\tcommits = 1;\n+\t}\n \treturn fetch_using_walker(argv[arg], get_verbosely, get_recover,\n \t\t\t\t  commits, commit_id, write_ref,\n \t\t\t\t  commits_on_stdin);\ndiff --git a/http.c b/http.c\nindex 4f6e1fb018..3aa0fa9fe6 100644\n--- a/http.c\n+++ b/http.c\n@@ -2281,7 +2281,13 @@ int finish_http_pack_request(struct http_pack_request *preq)\n \targv_array_push(&ip.args, \"--stdin\");\n \tip.git_cmd = 1;\n \tip.in = tmpfile_fd;\n-\tip.no_stdout = 1;\n+\tif (preq->generate_keep) {\n+\t\targv_array_pushf(&ip.args, \"--keep=git %\"PRIuMAX,\n+\t\t\t\t (uintmax_t)getpid());\n+\t\tip.out = 0;\n+\t} else {\n+\t\tip.no_stdout = 1;\n+\t}\n \n \tif (run_command(&ip)) {\n \t\tret = -1;\n@@ -2307,19 +2313,27 @@ void http_install_packfile(struct packed_git *p,\n }\n \n struct http_pack_request *new_http_pack_request(\n-\tconst unsigned char *packed_git_hash, const char *base_url)\n+\tconst unsigned char *packed_git_hash, const char *base_url) {\n+\n+\tstruct strbuf buf = STRBUF_INIT;\n+\n+\tend_url_with_slash(&buf, base_url);\n+\tstrbuf_addf(&buf, \"objects/pack/pack-%s.pack\",\n+\t\thash_to_hex(packed_git_hash));\n+\treturn new_direct_http_pack_request(packed_git_hash,\n+\t\t\t\t\t    strbuf_detach(&buf, NULL));\n+}\n+\n+struct http_pack_request *new_direct_http_pack_request(\n+\tconst unsigned char *packed_git_hash, char *url)\n {\n \toff_t prev_posn = 0;\n-\tstruct strbuf buf = STRBUF_INIT;\n \tstruct http_pack_request *preq;\n \n \tpreq = xcalloc(1, sizeof(*preq));\n \tstrbuf_init(&preq->tmpfile, 0);\n \n-\tend_url_with_slash(&buf, base_url);\n-\tstrbuf_addf(&buf, \"objects/pack/pack-%s.pack\",\n-\t\thash_to_hex(packed_git_hash));\n-\tpreq->url = strbuf_detach(&buf, NULL);\n+\tpreq->url = url;\n \n \tstrbuf_addf(&preq->tmpfile, \"%s.temp\", sha1_pack_name(packed_git_hash));\n \tpreq->packfile = fopen(preq->tmpfile.buf, \"a\");\ndiff --git a/http.h b/http.h\nindex bbc6b070f1..dc49c60165 100644\n--- a/http.h\n+++ b/http.h\n@@ -216,6 +216,15 @@ int http_get_info_packs(const char *base_url,\n \n struct http_pack_request {\n \tchar *url;\n+\n+\t/*\n+\t * If this is true, finish_http_pack_request() will pass \"--keep\" to\n+\t * index-pack, resulting in the creation of a keep file, and will not\n+\t * suppress its stdout (that is, the \"keep\\t<hash>\\n\" line will be\n+\t * printed to stdout).\n+\t */\n+\tunsigned generate_keep : 1;\n+\n \tFILE *packfile;\n \tstruct strbuf tmpfile;\n \tstruct active_request_slot *slot;\n@@ -223,6 +232,8 @@ struct http_pack_request {\n \n struct http_pack_request *new_http_pack_request(\n \tconst unsigned char *packed_git_hash, const char *base_url);\n+struct http_pack_request *new_direct_http_pack_request(\n+\tconst unsigned char *packed_git_hash, char *url);\n int finish_http_pack_request(struct http_pack_request *preq);\n void release_http_pack_request(struct http_pack_request *preq);\n \ndiff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh\nindex 50485300eb..ca2e8af022 100755\n--- a/t/t5550-http-fetch-dumb.sh\n+++ b/t/t5550-http-fetch-dumb.sh\n@@ -199,6 +199,28 @@ test_expect_success 'fetch packed objects' '\n \tgit clone $HTTPD_URL/dumb/repo_pack.git\n '\n \n+test_expect_success 'http-fetch --packfile' '\n+\t# Arbitrary hash. Use rev-parse so that we get one of the correct\n+\t# length.\n+\tARBITRARY=$(git -C \"$HTTPD_DOCUMENT_ROOT_PATH\"/repo_pack.git rev-parse HEAD) &&\n+\n+\tgit init packfileclient &&\n+\tp=$(cd \"$HTTPD_DOCUMENT_ROOT_PATH\"/repo_pack.git && ls objects/pack/pack-*.pack) &&\n+\tgit -C packfileclient http-fetch --packfile=$ARBITRARY \"$HTTPD_URL\"/dumb/repo_pack.git/$p >out &&\n+\n+\tgrep \"^keep.[0-9a-f]\\{16,\\}$\" out &&\n+\tcut -c6- out >packhash &&\n+\n+\t# Ensure that the expected files are generated\n+\ttest -e \"packfileclient/.git/objects/pack/pack-$(cat packhash).pack\" &&\n+\ttest -e \"packfileclient/.git/objects/pack/pack-$(cat packhash).idx\" &&\n+\ttest -e \"packfileclient/.git/objects/pack/pack-$(cat packhash).keep\" &&\n+\n+\t# Ensure that it has the HEAD of repo_pack, at least\n+\tHASH=$(git -C \"$HTTPD_DOCUMENT_ROOT_PATH\"/repo_pack.git rev-parse HEAD) &&\n+\tgit -C packfileclient cat-file -e \"$HASH\"\n+'\n+\n test_expect_success 'fetch notices corrupt pack' '\n \tcp -R \"$HTTPD_DOCUMENT_ROOT_PATH\"/repo_pack.git \"$HTTPD_DOCUMENT_ROOT_PATH\"/repo_bad1.git &&\n \t(cd \"$HTTPD_DOCUMENT_ROOT_PATH\"/repo_bad1.git &&\n@@ -214,6 +236,14 @@ test_expect_success 'fetch notices corrupt pack' '\n \t)\n '\n \n+test_expect_success 'http-fetch --packfile with corrupt pack' '\n+\trm -rf packfileclient &&\n+\tgit init packfileclient &&\n+\tp=$(cd \"$HTTPD_DOCUMENT_ROOT_PATH\"/repo_bad1.git && ls objects/pack/pack-*.pack) &&\n+\ttest_must_fail git -C packfileclient http-fetch --packfile \\\n+\t\t\"$HTTPD_URL\"/dumb/repo_bad1.git/$p\n+'\n+\n test_expect_success 'fetch notices corrupt idx' '\n \tcp -R \"$HTTPD_DOCUMENT_ROOT_PATH\"/repo_pack.git \"$HTTPD_DOCUMENT_ROOT_PATH\"/repo_bad2.git &&\n \t(cd \"$HTTPD_DOCUMENT_ROOT_PATH\"/repo_bad2.git &&\n-- \n2.27.0.278.ge193c7cf3a9-goog\n\n"},{"id":"399446","messageId":"6344c225897de1a2d8aa86d610e9eaf1c6ec82b4.1591821067.git.jonathantanmy@google.com","threadId":"53583","inReplyTo":"cover.1591821067.git.jonathantanmy@google.com","subject":"[PATCH v2 6/9] Documentation: add Packfile URIs design doc","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-06-10T20:57:20Z","receivedAt":"2020-06-10T20:57:42Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Signed-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\n Documentation/technical/packfile-uri.txt | 78 ++++++++++++++++++++++++\n Documentation/technical/protocol-v2.txt  | 32 +++++++++-\n 2 files changed, 109 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..318713abc3\n--- /dev/null\n+++ b/Documentation/technical/packfile-uri.txt\n@@ -0,0 +1,78 @@\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 the `packfile-uris` capability.\n+\n+If the client then communicates which protocols (HTTPS, etc.) it supports with\n+a `packfile-uris` argument, the server MAY send a `packfile-uris` section\n+directly before the `packfile` section (right after `wanted-refs` if it is\n+sent) containing URIs of any of the given protocols. The URIs point to\n+packfiles that use only features that the client has declared that it supports\n+(e.g. ofs-delta and thin-pack). See protocol-v2.txt for the documentation of\n+this section.\n+\n+Clients should then download and index all the given URIs (in addition to\n+downloading and indexing the packfile given in the `packfile` section of the\n+response) before performing the connectivity check.\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=<sha1>\n+<uri>` entries. Whenever the list of objects to be sent is assembled, all such\n+blobs are excluded, replaced with URIs. The client will download those URIs,\n+expecting them to each point to packfiles containing single blobs.\n+\n+Client design\n+-------------\n+\n+The client has a config variable `fetch.uriprotocols` that determines which\n+protocols the end user is willing to use. By default, this is empty.\n+\n+When the client downloads the given URIs, it should store them with \"keep\"\n+files, just like it does with the packfile in the `packfile` section. These\n+additional \"keep\" files can only be removed after the refs have been updated -\n+just like the \"keep\" file for the packfile in the `packfile` section.\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+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, more sophisticated means of excluding objects (e.g. by\n+   specifying a commit to represent that commit and all objects that it\n+   references).\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)\ndiff --git a/Documentation/technical/protocol-v2.txt b/Documentation/technical/protocol-v2.txt\nindex 995f07481e..f9f4e4ddd0 100644\n--- a/Documentation/technical/protocol-v2.txt\n+++ b/Documentation/technical/protocol-v2.txt\n@@ -323,13 +323,26 @@ included in the client's request:\n \tindicating its sideband (1, 2, or 3), and the server may send \"0005\\2\"\n \t(a PKT-LINE of sideband 2 with no payload) as a keepalive packet.\n \n+If the 'packfile-uris' feature is advertised, the following argument\n+can be included in the client's request as well as the potential\n+addition of the 'packfile-uris' section in the server's response as\n+explained below.\n+\n+    packfile-uris <comma-separated list of protocols>\n+\tIndicates to the server that the client is willing to receive\n+\tURIs of any of the given protocols in place of objects in the\n+\tsent packfile. Before performing the connectivity check, the\n+\tclient should download from all given URIs. Currently, the\n+\tprotocols supported are \"http\" and \"https\".\n+\n The response of `fetch` is broken into a number of sections separated by\n delimiter packets (0001), with each section beginning with its section\n header. Most sections are sent only when the packfile is sent.\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@@ -347,6 +360,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(40*(HEXDIGIT) SP *%x20-ff LF)\n+\n     packfile = PKT-LINE(\"packfile\" LF)\n \t       *PKT-LINE(%x01-03 *%x00-ff)\n \n@@ -418,6 +434,20 @@ header. Most sections are sent only when the packfile is sent.\n \t* The server MUST NOT send any refs which were not requested\n \t  using 'want-ref' lines.\n \n+    packfile-uris section\n+\t* This section is only included if the client sent\n+\t  'packfile-uris' and the server has at least one such URI to\n+\t  send.\n+\n+\t* Always begins with the section header \"packfile-uris\".\n+\n+\t* For each URI the server sends, it sends a hash of the pack's\n+\t  contents (as output by git index-pack) followed by the URI.\n+\n+\t* The hashes are 40 hex characters long. When Git upgrades to a new\n+\t  hash algorithm, this might need to be updated. (It should match\n+\t  whatever index-pack outputs after \"pack\\t\" or \"keep\\t\".\n+\n     packfile section\n \t* This section is only included if the client has sent 'want'\n \t  lines in its request and either requested that no more\n-- \n2.27.0.278.ge193c7cf3a9-goog\n\n"},{"id":"399447","messageId":"927ca4133cc0941bd3c16367347e9833feced0cd.1591821067.git.jonathantanmy@google.com","threadId":"53583","inReplyTo":"cover.1591821067.git.jonathantanmy@google.com","subject":"[PATCH v2 7/9] upload-pack: refactor reading of pack-objects out","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-06-10T20:57:21Z","receivedAt":"2020-06-10T20:57:45Z","isPatch":true,"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 <jonathantanmy@google.com>\n---\n upload-pack.c | 84 ++++++++++++++++++++++++++++++---------------------\n 1 file changed, 49 insertions(+), 35 deletions(-)\n\ndiff --git a/upload-pack.c b/upload-pack.c\nindex bc7e3ca19d..da1f749620 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -173,13 +173,52 @@ 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 relay_pack_data(int pack_objects_out, struct output_state *os,\n+\t\t\t   int use_sideband)\n+{\n+\t/*\n+\t * 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(pack_objects_out, 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, use_sideband);\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, use_sideband);\n+\t\tos->used = 0;\n+\t}\n+\n+\treturn readsz;\n+}\n+\n static void create_pack_file(struct upload_pack_data *pack_data)\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@@ -312,40 +351,16 @@ static void create_pack_file(struct upload_pack_data *pack_data)\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 = relay_pack_data(pack_objects.out,\n+\t\t\t\t\t\t     &output_state,\n+\t\t\t\t\t\t     pack_data->use_sideband);\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\t\t\t pack_data->use_sideband);\n \t\t}\n \n \t\t/*\n@@ -370,9 +385,8 @@ static void create_pack_file(struct upload_pack_data *pack_data)\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\t\t\t pack_data->use_sideband);\n \t\tfprintf(stderr, \"flushed.\\n\");\n \t}\n-- \n2.27.0.278.ge193c7cf3a9-goog\n\n"},{"id":"399448","messageId":"566419ae74348ad3f7b1e5d484cf986fea29af73.1591821067.git.jonathantanmy@google.com","threadId":"53583","inReplyTo":"cover.1591821067.git.jonathantanmy@google.com","subject":"[PATCH v2 8/9] fetch-pack: support more than one pack lockfile","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-06-10T20:57:22Z","receivedAt":"2020-06-10T20:57:47Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Whenever a fetch results in a packfile being downloaded, a .keep file is\ngenerated, so that the packfile can be preserved (from, say, a running\n\"git repack\") until refs are written referring to the contents of the\npackfile.\n\nIn a subsequent patch, a successful fetch using protocol v2 may result\nin more than one .keep file being generated. Therefore, teach\nfetch_pack() and the transport mechanism to support multiple .keep\nfiles.\n\nImplementation notes:\n\n - builtin/fetch-pack.c normally does not generate .keep files, and thus\n   is unaffected by this or future changes. However, it has an\n   undocumented \"--lock-pack\" feature, used by remote-curl.c when\n   implementing the \"fetch\" remote helper command. In keeping with the\n   remote helper protocol, only one \"lock\" line will ever be written;\n   the rest will result in warnings to stderr. However, in practice,\n   warnings will never be written because the remote-curl.c \"fetch\" is\n   only used for protocol v0/v1 (which will not generate multiple .keep\n   files). (Protocol v2 uses the \"stateless-connect\" command, not the\n   \"fetch\" command.)\n\n - connected.c has an optimization in that connectivity checks on a ref\n   need not be done if the target object is in a pack known to be\n   self-contained and connected. If there are multiple packfiles, this\n   optimization can no longer be done.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\n builtin/fetch-pack.c | 17 +++++++++++------\n connected.c          |  8 +++++---\n fetch-pack.c         | 29 +++++++++++++++--------------\n fetch-pack.h         |  2 +-\n transport-helper.c   |  5 +++--\n transport.c          | 14 ++++++++------\n transport.h          |  6 +++---\n 7 files changed, 46 insertions(+), 35 deletions(-)\n\ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex 4771100072..f66891b010 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -48,8 +48,8 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n \tstruct ref **sought = NULL;\n \tint nr_sought = 0, alloc_sought = 0;\n \tint fd[2];\n-\tchar *pack_lockfile = NULL;\n-\tchar **pack_lockfile_ptr = NULL;\n+\tstruct string_list pack_lockfiles = STRING_LIST_INIT_DUP;\n+\tstruct string_list *pack_lockfiles_ptr = NULL;\n \tstruct child_process *conn;\n \tstruct fetch_pack_args args;\n \tstruct oid_array shallow = OID_ARRAY_INIT;\n@@ -134,7 +134,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n \t\t}\n \t\tif (!strcmp(\"--lock-pack\", arg)) {\n \t\t\targs.lock_pack = 1;\n-\t\t\tpack_lockfile_ptr = &pack_lockfile;\n+\t\t\tpack_lockfiles_ptr = &pack_lockfiles;\n \t\t\tcontinue;\n \t\t}\n \t\tif (!strcmp(\"--check-self-contained-and-connected\", arg)) {\n@@ -235,10 +235,15 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n \t}\n \n \tref = fetch_pack(&args, fd, ref, sought, nr_sought,\n-\t\t\t &shallow, pack_lockfile_ptr, version);\n-\tif (pack_lockfile) {\n-\t\tprintf(\"lock %s\\n\", pack_lockfile);\n+\t\t\t &shallow, pack_lockfiles_ptr, version);\n+\tif (pack_lockfiles.nr) {\n+\t\tint i;\n+\n+\t\tprintf(\"lock %s\\n\", pack_lockfiles.items[0].string);\n \t\tfflush(stdout);\n+\t\tfor (i = 1; i < pack_lockfiles.nr; i++)\n+\t\t\twarning(_(\"Lockfile created but not reported: %s\"),\n+\t\t\t\tpack_lockfiles.items[i].string);\n \t}\n \tif (args.check_self_contained_and_connected &&\n \t    args.self_contained_and_connected) {\ndiff --git a/connected.c b/connected.c\nindex 3135b71e19..937b4bae38 100644\n--- a/connected.c\n+++ b/connected.c\n@@ -43,10 +43,12 @@ int check_connected(oid_iterate_fn fn, void *cb_data,\n \n \tif (transport && transport->smart_options &&\n \t    transport->smart_options->self_contained_and_connected &&\n-\t    transport->pack_lockfile &&\n-\t    strip_suffix(transport->pack_lockfile, \".keep\", &base_len)) {\n+\t    transport->pack_lockfiles.nr == 1 &&\n+\t    strip_suffix(transport->pack_lockfiles.items[0].string,\n+\t\t\t \".keep\", &base_len)) {\n \t\tstruct strbuf idx_file = STRBUF_INIT;\n-\t\tstrbuf_add(&idx_file, transport->pack_lockfile, base_len);\n+\t\tstrbuf_add(&idx_file, transport->pack_lockfiles.items[0].string,\n+\t\t\t   base_len);\n \t\tstrbuf_addstr(&idx_file, \".idx\");\n \t\tnew_pack = add_packed_git(idx_file.buf, idx_file.len, 1);\n \t\tstrbuf_release(&idx_file);\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 7eaa19d7c1..bc3af3c726 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -794,7 +794,7 @@ static void write_promisor_file(const char *keep_name,\n }\n \n static int get_pack(struct fetch_pack_args *args,\n-\t\t    int xd[2], char **pack_lockfile,\n+\t\t    int xd[2], struct string_list *pack_lockfiles,\n \t\t    struct ref **sought, int nr_sought)\n {\n \tstruct async demux;\n@@ -838,7 +838,7 @@ static int get_pack(struct fetch_pack_args *args,\n \t}\n \n \tif (do_keep || args->from_promisor) {\n-\t\tif (pack_lockfile)\n+\t\tif (pack_lockfiles)\n \t\t\tcmd.out = -1;\n \t\tcmd_name = \"index-pack\";\n \t\targv_array_push(&cmd.args, cmd_name);\n@@ -863,7 +863,7 @@ static int get_pack(struct fetch_pack_args *args,\n \t\t * information below. If not, we need index-pack to do it for\n \t\t * us.\n \t\t */\n-\t\tif (!(do_keep && pack_lockfile) && args->from_promisor)\n+\t\tif (!(do_keep && pack_lockfiles) && args->from_promisor)\n \t\t\targv_array_push(&cmd.args, \"--promisor\");\n \t}\n \telse {\n@@ -899,8 +899,9 @@ static int get_pack(struct fetch_pack_args *args,\n \tcmd.git_cmd = 1;\n \tif (start_command(&cmd))\n \t\tdie(_(\"fetch-pack: unable to fork off %s\"), cmd_name);\n-\tif (do_keep && pack_lockfile) {\n-\t\t*pack_lockfile = index_pack_lockfile(cmd.out);\n+\tif (do_keep && pack_lockfiles) {\n+\t\tstring_list_append_nodup(pack_lockfiles,\n+\t\t\t\t\t index_pack_lockfile(cmd.out));\n \t\tclose(cmd.out);\n \t}\n \n@@ -922,8 +923,8 @@ static int get_pack(struct fetch_pack_args *args,\n \t * Now that index-pack has succeeded, write the promisor file using the\n \t * obtained .keep filename if necessary\n \t */\n-\tif (do_keep && pack_lockfile && args->from_promisor)\n-\t\twrite_promisor_file(*pack_lockfile, sought, nr_sought);\n+\tif (do_keep && pack_lockfiles && pack_lockfiles->nr && args->from_promisor)\n+\t\twrite_promisor_file(pack_lockfiles->items[0].string, sought, nr_sought);\n \n \treturn 0;\n }\n@@ -940,7 +941,7 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,\n \t\t\t\t const struct ref *orig_ref,\n \t\t\t\t struct ref **sought, int nr_sought,\n \t\t\t\t struct shallow_info *si,\n-\t\t\t\t char **pack_lockfile)\n+\t\t\t\t struct string_list *pack_lockfiles)\n {\n \tstruct repository *r = the_repository;\n \tstruct ref *ref = copy_ref_list(orig_ref);\n@@ -1067,7 +1068,7 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,\n \t\talternate_shallow_file = setup_temporary_shallow(si->shallow);\n \telse\n \t\talternate_shallow_file = NULL;\n-\tif (get_pack(args, fd, pack_lockfile, sought, nr_sought))\n+\tif (get_pack(args, fd, pack_lockfiles, sought, nr_sought))\n \t\tdie(_(\"git fetch-pack: fetch failed.\"));\n \n  all_done:\n@@ -1457,7 +1458,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \t\t\t\t    struct ref **sought, int nr_sought,\n \t\t\t\t    struct oid_array *shallows,\n \t\t\t\t    struct shallow_info *si,\n-\t\t\t\t    char **pack_lockfile)\n+\t\t\t\t    struct string_list *pack_lockfiles)\n {\n \tstruct repository *r = the_repository;\n \tstruct ref *ref = copy_ref_list(orig_ref);\n@@ -1559,7 +1560,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \n \t\t\t/* get the pack */\n \t\t\tprocess_section_header(&reader, \"packfile\", 0);\n-\t\t\tif (get_pack(args, fd, pack_lockfile, sought, nr_sought))\n+\t\t\tif (get_pack(args, fd, pack_lockfiles, sought, nr_sought))\n \t\t\t\tdie(_(\"git fetch-pack: fetch failed.\"));\n \n \t\t\tstate = FETCH_DONE;\n@@ -1759,7 +1760,7 @@ struct ref *fetch_pack(struct fetch_pack_args *args,\n \t\t       const struct ref *ref,\n \t\t       struct ref **sought, int nr_sought,\n \t\t       struct oid_array *shallow,\n-\t\t       char **pack_lockfile,\n+\t\t       struct string_list *pack_lockfiles,\n \t\t       enum protocol_version version)\n {\n \tstruct ref *ref_cpy;\n@@ -1794,11 +1795,11 @@ struct ref *fetch_pack(struct fetch_pack_args *args,\n \t\tmemset(&si, 0, sizeof(si));\n \t\tref_cpy = do_fetch_pack_v2(args, fd, ref, sought, nr_sought,\n \t\t\t\t\t   &shallows_scratch, &si,\n-\t\t\t\t\t   pack_lockfile);\n+\t\t\t\t\t   pack_lockfiles);\n \t} else {\n \t\tprepare_shallow_info(&si, shallow);\n \t\tref_cpy = do_fetch_pack(args, fd, ref, sought, nr_sought,\n-\t\t\t\t\t&si, pack_lockfile);\n+\t\t\t\t\t&si, pack_lockfiles);\n \t}\n \treprepare_packed_git(the_repository);\n \ndiff --git a/fetch-pack.h b/fetch-pack.h\nindex 67f684229a..85d1e39fe7 100644\n--- a/fetch-pack.h\n+++ b/fetch-pack.h\n@@ -83,7 +83,7 @@ struct ref *fetch_pack(struct fetch_pack_args *args,\n \t\t       struct ref **sought,\n \t\t       int nr_sought,\n \t\t       struct oid_array *shallow,\n-\t\t       char **pack_lockfile,\n+\t\t       struct string_list *pack_lockfiles,\n \t\t       enum protocol_version version);\n \n /*\ndiff --git a/transport-helper.c b/transport-helper.c\nindex a46afcb69d..93a6f50793 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -410,10 +410,11 @@ static int fetch_with_fetch(struct transport *transport,\n \t\t\texit(128);\n \n \t\tif (skip_prefix(buf.buf, \"lock \", &name)) {\n-\t\t\tif (transport->pack_lockfile)\n+\t\t\tif (transport->pack_lockfiles.nr)\n \t\t\t\twarning(_(\"%s also locked %s\"), data->name, name);\n \t\t\telse\n-\t\t\t\ttransport->pack_lockfile = xstrdup(name);\n+\t\t\t\tstring_list_append(&transport->pack_lockfiles,\n+\t\t\t\t\t\t   name);\n \t\t}\n \t\telse if (data->check_connectivity &&\n \t\t\t data->transport_options.check_self_contained_and_connected &&\ndiff --git a/transport.c b/transport.c\nindex 15f5ba4e8f..a67e1990bf 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -374,7 +374,7 @@ static int fetch_refs_via_pack(struct transport *transport,\n \t\trefs = fetch_pack(&args, data->fd,\n \t\t\t\t  refs_tmp ? refs_tmp : transport->remote_refs,\n \t\t\t\t  to_fetch, nr_heads, &data->shallow,\n-\t\t\t\t  &transport->pack_lockfile, data->version);\n+\t\t\t\t  &transport->pack_lockfiles, data->version);\n \t\tbreak;\n \tcase protocol_v1:\n \tcase protocol_v0:\n@@ -382,7 +382,7 @@ static int fetch_refs_via_pack(struct transport *transport,\n \t\trefs = fetch_pack(&args, data->fd,\n \t\t\t\t  refs_tmp ? refs_tmp : transport->remote_refs,\n \t\t\t\t  to_fetch, nr_heads, &data->shallow,\n-\t\t\t\t  &transport->pack_lockfile, data->version);\n+\t\t\t\t  &transport->pack_lockfiles, data->version);\n \t\tbreak;\n \tcase protocol_unknown_version:\n \t\tBUG(\"unknown protocol version\");\n@@ -929,6 +929,7 @@ struct transport *transport_get(struct remote *remote, const char *url)\n \tstruct transport *ret = xcalloc(1, sizeof(*ret));\n \n \tret->progress = isatty(2);\n+\tstring_list_init(&ret->pack_lockfiles, 1);\n \n \tif (!remote)\n \t\tBUG(\"No remote provided to transport_get()\");\n@@ -1324,10 +1325,11 @@ int transport_fetch_refs(struct transport *transport, struct ref *refs)\n \n void transport_unlock_pack(struct transport *transport)\n {\n-\tif (transport->pack_lockfile) {\n-\t\tunlink_or_warn(transport->pack_lockfile);\n-\t\tFREE_AND_NULL(transport->pack_lockfile);\n-\t}\n+\tint i;\n+\n+\tfor (i = 0; i < transport->pack_lockfiles.nr; i++)\n+\t\tunlink_or_warn(transport->pack_lockfiles.items[i].string);\n+\tstring_list_clear(&transport->pack_lockfiles, 0);\n }\n \n int transport_connect(struct transport *transport, const char *name,\ndiff --git a/transport.h b/transport.h\nindex 4298c855be..05efa72db1 100644\n--- a/transport.h\n+++ b/transport.h\n@@ -5,8 +5,7 @@\n #include \"run-command.h\"\n #include \"remote.h\"\n #include \"list-objects-filter-options.h\"\n-\n-struct string_list;\n+#include \"string-list.h\"\n \n struct git_transport_options {\n \tunsigned thin : 1;\n@@ -98,7 +97,8 @@ struct transport {\n \t */\n \tconst struct string_list *server_options;\n \n-\tchar *pack_lockfile;\n+\tstruct string_list pack_lockfiles;\n+\n \tsigned verbose : 3;\n \t/**\n \t * Transports should not set this directly, and should use this\n-- \n2.27.0.278.ge193c7cf3a9-goog\n\n"},{"id":"399449","messageId":"b301ca539a7a5f0cefd33ee4bc6f4035157579a5.1591821067.git.jonathantanmy@google.com","threadId":"53583","inReplyTo":"cover.1591821067.git.jonathantanmy@google.com","subject":"[PATCH v2 9/9] upload-pack: send part of packfile response as uri","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-06-10T20:57:23Z","receivedAt":"2020-06-10T20:57:47Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Teach upload-pack to send part of its packfile response as URIs.\n\nAn administrator may configure a repository with one or more\n\"uploadpack.blobpackfileuri\" lines, each line containing an OID, a pack\nhash, and a URI. A client may configure fetch.uriprotocols to be a\ncomma-separated list of protocols that it is willing to use to fetch\nadditional packfiles - this list will be sent to the server. Whenever an\nobject with one of those OIDs would appear in the packfile transmitted\nby upload-pack, the server may exclude that object, and instead send the\nURI. The client will then download the packs referred to by those URIs\nbefore performing the connectivity check.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\n builtin/pack-objects.c |  76 ++++++++++++++++++++++++++++\n fetch-pack.c           | 112 +++++++++++++++++++++++++++++++++++++++--\n t/t5702-protocol-v2.sh |  88 ++++++++++++++++++++++++++++++++\n upload-pack.c          |  77 +++++++++++++++++++++++++---\n 4 files changed, 343 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex c5b433a23f..7016b28485 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -117,6 +117,8 @@ static unsigned long window_memory_limit = 0;\n \n static struct list_objects_filter_options filter_options;\n \n+static struct string_list uri_protocols = STRING_LIST_INIT_NODUP;\n+\n enum missing_action {\n \tMA_ERROR = 0,      /* fail if any missing objects are encountered */\n \tMA_ALLOW_ANY,      /* silently allow ALL missing objects */\n@@ -125,6 +127,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 *pack_hash_hex;\n+\tchar *uri;\n+};\n+static struct oidmap configured_exclusions;\n+\n+static struct oidset excluded_by_config;\n+\n /*\n  * stats\n  */\n@@ -969,6 +980,25 @@ static void write_reused_pack(struct hashfile *f)\n \tunuse_pack(&w_curs);\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->pack_hash_hex, strlen(ex->pack_hash_hex));\n+\t\twrite_in_full(1, \" \", 1);\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@@ -1266,6 +1296,25 @@ static int want_object_in_pack(const struct object_id *oid,\n \t\t}\n \t}\n \n+\tif (uri_protocols.nr) {\n+\t\tstruct configured_exclusion *ex =\n+\t\t\toidmap_get(&configured_exclusions, oid);\n+\t\tint i;\n+\t\tconst char *p;\n+\n+\t\tif (ex) {\n+\t\t\tfor (i = 0; i < uri_protocols.nr; i++) {\n+\t\t\t\tif (skip_prefix(ex->uri,\n+\t\t\t\t\t\turi_protocols.items[i].string,\n+\t\t\t\t\t\t&p) &&\n+\t\t\t\t    *p == ':') {\n+\t\t\t\t\toidset_insert(&excluded_by_config, oid);\n+\t\t\t\t\treturn 0;\n+\t\t\t\t}\n+\t\t\t}\n+\t\t}\n+\t}\n+\n \treturn 1;\n }\n \n@@ -2864,6 +2913,29 @@ 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 *oid_end, *pack_end;\n+\t\t/*\n+\t\t * Stores the pack hash. This is not a true object ID, but is\n+\t\t * of the same form.\n+\t\t */\n+\t\tstruct object_id pack_hash;\n+\n+\t\tif (parse_oid_hex(v, &ex->e.oid, &oid_end) ||\n+\t\t    *oid_end != ' ' ||\n+\t\t    parse_oid_hex(oid_end + 1, &pack_hash, &pack_end) ||\n+\t\t    *pack_end != ' ')\n+\t\t\tdie(_(\"value of uploadpack.blobpackfileuri must be \"\n+\t\t\t      \"of the form '<object-hash> <pack-hash> <uri>' (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->pack_hash_hex = xcalloc(1, pack_end - oid_end);\n+\t\tmemcpy(ex->pack_hash_hex, oid_end + 1, pack_end - oid_end - 1);\n+\t\tex->uri = xstrdup(pack_end + 1);\n+\t\toidmap_put(&configured_exclusions, ex);\n+\t}\n \treturn git_default_config(k, v, cb);\n }\n \n@@ -3462,6 +3534,9 @@ 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_STRING_LIST(0, \"uri-protocol\", &uri_protocols,\n+\t\t\t\tN_(\"protocol\"),\n+\t\t\t\tN_(\"exclude any configured uploadpack.blobpackfileuri with this protocol\")),\n \t\tOPT_END(),\n \t};\n \n@@ -3650,6 +3725,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \t}\n \n \ttrace2_region_enter(\"pack-objects\", \"write-pack-file\", the_repository);\n+\twrite_excluded_by_configs();\n \twrite_pack_file();\n \ttrace2_region_leave(\"pack-objects\", \"write-pack-file\", the_repository);\n \ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex bc3af3c726..ca2b101b8d 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -38,6 +38,7 @@ static int server_supports_filtering;\n static struct shallow_lock shallow_lock;\n static const char *alternate_shallow_file;\n static struct strbuf fsck_msg_types = STRBUF_INIT;\n+static struct string_list uri_protocols = STRING_LIST_INIT_DUP;\n \n /* Remember to update object flag allocation in object.h */\n #define COMPLETE\t(1U << 0)\n@@ -795,6 +796,7 @@ static void write_promisor_file(const char *keep_name,\n \n static int get_pack(struct fetch_pack_args *args,\n \t\t    int xd[2], struct string_list *pack_lockfiles,\n+\t\t    int only_packfile,\n \t\t    struct ref **sought, int nr_sought)\n {\n \tstruct async demux;\n@@ -855,8 +857,15 @@ static int get_pack(struct fetch_pack_args *args,\n \t\t\t\t\t\"--keep=fetch-pack %\"PRIuMAX \" on %s\",\n \t\t\t\t\t(uintmax_t)getpid(), hostname);\n \t\t}\n-\t\tif (args->check_self_contained_and_connected)\n+\t\tif (only_packfile && args->check_self_contained_and_connected)\n \t\t\targv_array_push(&cmd.args, \"--check-self-contained-and-connected\");\n+\t\telse\n+\t\t\t/*\n+\t\t\t * We cannot perform any connectivity checks because\n+\t\t\t * not all packs have been downloaded; let the caller\n+\t\t\t * have this responsibility.\n+\t\t\t */\n+\t\t\targs->check_self_contained_and_connected = 0;\n \t\t/*\n \t\t * If we're obtaining the filename of a lockfile, we'll use\n \t\t * that filename to write a .promisor file with more\n@@ -1068,7 +1077,7 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,\n \t\talternate_shallow_file = setup_temporary_shallow(si->shallow);\n \telse\n \t\talternate_shallow_file = NULL;\n-\tif (get_pack(args, fd, pack_lockfiles, sought, nr_sought))\n+\tif (get_pack(args, fd, pack_lockfiles, 1, sought, nr_sought))\n \t\tdie(_(\"git fetch-pack: fetch failed.\"));\n \n  all_done:\n@@ -1222,6 +1231,26 @@ static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,\n \t\twarning(\"filtering not recognized by server, ignoring\");\n \t}\n \n+\tif (server_supports_feature(\"fetch\", \"packfile-uris\", 0)) {\n+\t\tint i;\n+\t\tstruct strbuf to_send = STRBUF_INIT;\n+\n+\t\tfor (i = 0; i < uri_protocols.nr; i++) {\n+\t\t\tconst char *s = uri_protocols.items[i].string;\n+\n+\t\t\tif (!strcmp(s, \"https\") || !strcmp(s, \"http\")) {\n+\t\t\t\tif (to_send.len)\n+\t\t\t\t\tstrbuf_addch(&to_send, ',');\n+\t\t\t\tstrbuf_addstr(&to_send, s);\n+\t\t\t}\n+\t\t}\n+\t\tif (to_send.len) {\n+\t\t\tpacket_buf_write(&req_buf, \"packfile-uris %s\",\n+\t\t\t\t\t to_send.buf);\n+\t\t\tstrbuf_release(&to_send);\n+\t\t}\n+\t}\n+\n \t/* add wants */\n \tadd_wants(args->no_dependents, wants, &req_buf);\n \n@@ -1444,6 +1473,21 @@ static void receive_wanted_refs(struct packet_reader *reader,\n \t\tdie(_(\"error processing wanted refs: %d\"), reader->status);\n }\n \n+static void receive_packfile_uris(struct packet_reader *reader,\n+\t\t\t\t  struct string_list *uris)\n+{\n+\tprocess_section_header(reader, \"packfile-uris\", 0);\n+\twhile (packet_reader_read(reader) == PACKET_READ_NORMAL) {\n+\t\tif (reader->pktlen < the_hash_algo->hexsz ||\n+\t\t    reader->line[the_hash_algo->hexsz] != ' ')\n+\t\t\tdie(\"expected '<hash> <uri>', got: %s\\n\", reader->line);\n+\n+\t\tstring_list_append(uris, reader->line);\n+\t}\n+\tif (reader->status != PACKET_READ_DELIM)\n+\t\tdie(\"expected DELIM\");\n+}\n+\n enum fetch_state {\n \tFETCH_CHECK_LOCAL = 0,\n \tFETCH_SEND_REQUEST,\n@@ -1470,6 +1514,8 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \tstruct fetch_negotiator negotiator_alloc;\n \tstruct fetch_negotiator *negotiator;\n \tint seen_ack = 0;\n+\tstruct string_list packfile_uris = STRING_LIST_INIT_DUP;\n+\tint i;\n \n \tif (args->no_dependents) {\n \t\tnegotiator = NULL;\n@@ -1558,9 +1604,12 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \t\t\tif (process_section_header(&reader, \"wanted-refs\", 1))\n \t\t\t\treceive_wanted_refs(&reader, sought, nr_sought);\n \n-\t\t\t/* get the pack */\n+\t\t\t/* get the pack(s) */\n+\t\t\tif (process_section_header(&reader, \"packfile-uris\", 1))\n+\t\t\t\treceive_packfile_uris(&reader, &packfile_uris);\n \t\t\tprocess_section_header(&reader, \"packfile\", 0);\n-\t\t\tif (get_pack(args, fd, pack_lockfiles, sought, nr_sought))\n+\t\t\tif (get_pack(args, fd, pack_lockfiles,\n+\t\t\t\t     !packfile_uris.nr, sought, nr_sought))\n \t\t\t\tdie(_(\"git fetch-pack: fetch failed.\"));\n \n \t\t\tstate = FETCH_DONE;\n@@ -1570,8 +1619,55 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \t\t}\n \t}\n \n+\tfor (i = 0; i < packfile_uris.nr; i++) {\n+\t\tstruct child_process cmd = CHILD_PROCESS_INIT;\n+\t\tchar packname[GIT_MAX_HEXSZ + 1];\n+\t\tconst char *uri = packfile_uris.items[i].string +\n+\t\t\tthe_hash_algo->hexsz + 1;\n+\n+\t\targv_array_push(&cmd.args, \"http-fetch\");\n+\t\targv_array_pushf(&cmd.args, \"--packfile=%.*s\",\n+\t\t\t\t (int) the_hash_algo->hexsz,\n+\t\t\t\t packfile_uris.items[i].string);\n+\t\targv_array_push(&cmd.args, uri);\n+\t\tcmd.git_cmd = 1;\n+\t\tcmd.no_stdin = 1;\n+\t\tcmd.out = -1;\n+\t\tif (start_command(&cmd))\n+\t\t\tdie(\"fetch-pack: unable to spawn http-fetch\");\n+\n+\t\tif (read_in_full(cmd.out, packname, 5) < 0 ||\n+\t\t    memcmp(packname, \"keep\\t\", 5))\n+\t\t\tdie(\"fetch-pack: expected keep then TAB at start of http-fetch output\");\n+\n+\t\tif (read_in_full(cmd.out, packname,\n+\t\t\t\t the_hash_algo->hexsz + 1) < 0 ||\n+\t\t    packname[the_hash_algo->hexsz] != '\\n')\n+\t\t\tdie(\"fetch-pack: expected hash then LF at end of http-fetch output\");\n+\n+\t\tpackname[the_hash_algo->hexsz] = '\\0';\n+\n+\t\tclose(cmd.out);\n+\n+\t\tif (finish_command(&cmd))\n+\t\t\tdie(\"fetch-pack: unable to finish http-fetch\");\n+\n+\t\tif (memcmp(packfile_uris.items[i].string, packname,\n+\t\t\t   the_hash_algo->hexsz))\n+\t\t\tdie(\"fetch-pack: pack downloaded from %s does not match expected hash %.*s\",\n+\t\t\t    uri, (int) the_hash_algo->hexsz,\n+\t\t\t    packfile_uris.items[i].string);\n+\n+\t\tstring_list_append_nodup(pack_lockfiles,\n+\t\t\t\t\t xstrfmt(\"%s/pack/pack-%s.keep\",\n+\t\t\t\t\t\t get_object_directory(),\n+\t\t\t\t\t\t packname));\n+\t}\n+\tstring_list_clear(&packfile_uris, 0);\n+\n \tif (negotiator)\n \t\tnegotiator->release(negotiator);\n+\n \toidset_clear(&common);\n \treturn ref;\n }\n@@ -1608,6 +1704,14 @@ static void fetch_pack_config(void)\n \tgit_config_get_bool(\"repack.usedeltabaseoffset\", &prefer_ofs_delta);\n \tgit_config_get_bool(\"fetch.fsckobjects\", &fetch_fsck_objects);\n \tgit_config_get_bool(\"transfer.fsckobjects\", &transfer_fsck_objects);\n+\tif (!uri_protocols.nr) {\n+\t\tchar *str;\n+\n+\t\tif (!git_config_get_string(\"fetch.uriprotocols\", &str) && str) {\n+\t\t\tstring_list_split(&uri_protocols, str, ',', -1);\n+\t\t\tfree(str);\n+\t\t}\n+\t}\n \n \tgit_config(fetch_pack_config_cb, NULL);\n }\ndiff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh\nindex 5039e66dc4..53bda39fb7 100755\n--- a/t/t5702-protocol-v2.sh\n+++ b/t/t5702-protocol-v2.sh\n@@ -748,6 +748,94 @@ 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+configure_exclusion () {\n+\tgit -C \"$1\" hash-object \"$2\" >objh &&\n+\tgit -C \"$1\" pack-objects \"$HTTPD_DOCUMENT_ROOT_PATH/mypack\" <objh >packh &&\n+\tgit -C \"$1\" config --add \\\n+\t\t\"uploadpack.blobpackfileuri\" \\\n+\t\t\"$(cat objh) $(cat packh) $HTTPD_URL/dumb/mypack-$(cat packh).pack\" &&\n+\tcat objh\n+}\n+\n+test_expect_success 'part of packfile response provided as URI' '\n+\tP=\"$HTTPD_DOCUMENT_ROOT_PATH/http_parent\" &&\n+\trm -rf \"$P\" http_child log &&\n+\n+\tgit init \"$P\" &&\n+\tgit -C \"$P\" config \"uploadpack.allowsidebandall\" \"true\" &&\n+\n+\techo my-blob >\"$P/my-blob\" &&\n+\tgit -C \"$P\" add my-blob &&\n+\techo other-blob >\"$P/other-blob\" &&\n+\tgit -C \"$P\" add other-blob &&\n+\tgit -C \"$P\" commit -m x &&\n+\n+\tconfigure_exclusion \"$P\" my-blob >h &&\n+\tconfigure_exclusion \"$P\" other-blob >h2 &&\n+\n+\tGIT_TRACE=1 GIT_TRACE_PACKET=\"$(pwd)/log\" GIT_TEST_SIDEBAND_ALL=1 \\\n+\tgit -c protocol.version=2 \\\n+\t\t-c fetch.uriprotocols=http,https \\\n+\t\tclone \"$HTTPD_URL/smart/http_parent\" http_child &&\n+\n+\t# Ensure that my-blob and other-blob are in separate packfiles.\n+\tfor idx in http_child/.git/objects/pack/*.idx\n+\tdo\n+\t\tgit verify-pack --verbose $idx >out &&\n+\t\t{\n+\t\t\tgrep \"^[0-9a-f]\\{16,\\} \" out || :\n+\t\t} >out.objectlist &&\n+\t\tif test_line_count = 1 out.objectlist\n+\t\tthen\n+\t\t\tif grep $(cat h) out\n+\t\t\tthen\n+\t\t\t\t>hfound\n+\t\t\tfi &&\n+\t\t\tif grep $(cat h2) out\n+\t\t\tthen\n+\t\t\t\t>h2found\n+\t\t\tfi\n+\t\tfi\n+\tdone &&\n+\ttest -f hfound &&\n+\ttest -f h2found &&\n+\n+\t# Ensure that there are exactly 6 files (3 .pack and 3 .idx).\n+\tls http_child/.git/objects/pack/* >filelist &&\n+\ttest_line_count = 6 filelist\n+'\n+\n+test_expect_success 'fetching with valid packfile URI but invalid hash fails' '\n+\tP=\"$HTTPD_DOCUMENT_ROOT_PATH/http_parent\" &&\n+\trm -rf \"$P\" http_child log &&\n+\n+\tgit init \"$P\" &&\n+\tgit -C \"$P\" config \"uploadpack.allowsidebandall\" \"true\" &&\n+\n+\techo my-blob >\"$P/my-blob\" &&\n+\tgit -C \"$P\" add my-blob &&\n+\techo other-blob >\"$P/other-blob\" &&\n+\tgit -C \"$P\" add other-blob &&\n+\tgit -C \"$P\" commit -m x &&\n+\n+\tconfigure_exclusion \"$P\" my-blob >h &&\n+\t# Configure a URL for other-blob. Just reuse the hash of the object as\n+\t# the hash of the packfile, since the hash does not matter for this\n+\t# test as long as it is not the hash of the pack, and it is of the\n+\t# expected length.\n+\tgit -C \"$P\" hash-object other-blob >objh &&\n+\tgit -C \"$P\" pack-objects \"$HTTPD_DOCUMENT_ROOT_PATH/mypack\" <objh >packh &&\n+\tgit -C \"$P\" config --add \\\n+\t\t\"uploadpack.blobpackfileuri\" \\\n+\t\t\"$(cat objh) $(cat objh) $HTTPD_URL/dumb/mypack-$(cat packh).pack\" &&\n+\n+\ttest_must_fail env GIT_TEST_SIDEBAND_ALL=1 \\\n+\t\tgit -c protocol.version=2 \\\n+\t\t-c fetch.uriprotocols=http,https \\\n+\t\tclone \"$HTTPD_URL/smart/http_parent\" http_child 2>err &&\n+\ttest_i18ngrep \"pack downloaded from.*does not match expected hash\" err\n+'\n+\n # DO NOT add non-httpd-specific tests here, because the last part of this\n # test script is only executed when httpd is available and enabled.\n \ndiff --git a/upload-pack.c b/upload-pack.c\nindex da1f749620..764265ec40 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -83,6 +83,8 @@ struct upload_pack_data {\n \t/* 0 for no sideband, otherwise DEFAULT_PACKET_MAX or LARGE_PACKET_MAX */\n \tint use_sideband;\n \n+\tstruct string_list uri_protocols;\n+\n \tstruct list_objects_filter_options filter_options;\n \n \tstruct packet_writer writer;\n@@ -114,6 +116,7 @@ static void upload_pack_data_init(struct upload_pack_data *data)\n \tstruct oid_array haves = OID_ARRAY_INIT;\n \tstruct object_array shallows = OBJECT_ARRAY_INIT;\n \tstruct string_list deepen_not = STRING_LIST_INIT_DUP;\n+\tstruct string_list uri_protocols = STRING_LIST_INIT_DUP;\n \n \tmemset(data, 0, sizeof(*data));\n \tdata->symref = symref;\n@@ -123,6 +126,7 @@ static void upload_pack_data_init(struct upload_pack_data *data)\n \tdata->haves = haves;\n \tdata->shallows = shallows;\n \tdata->deepen_not = deepen_not;\n+\tdata->uri_protocols = uri_protocols;\n \tpacket_writer_init(&data->writer, 1);\n \n \tdata->keepalive = 5;\n@@ -176,10 +180,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_uris_started : 1;\n+\tunsigned packfile_started : 1;\n };\n \n static int relay_pack_data(int pack_objects_out, struct output_state *os,\n-\t\t\t   int use_sideband)\n+\t\t\t   int use_sideband, int write_packfile_line)\n {\n \t/*\n \t * We keep the last byte to ourselves\n@@ -200,6 +206,37 @@ static int relay_pack_data(int pack_objects_out, struct output_state *os,\n \t}\n \tos->used += readsz;\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 (write_packfile_line) {\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, \"\\1packfile\\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 (!write_packfile_line)\n+\t\t\t\t\tBUG(\"packfile_uris requires sideband-all\");\n+\t\t\t\tpacket_write_fmt(1, \"\\1packfile-uris\\n\");\n+\t\t\t}\n+\t\t\t*p = '\\0';\n+\t\t\tpacket_write_fmt(1, \"\\1%s\\n\", os->buffer);\n+\n+\t\t\tos->used -= p - os->buffer + 1;\n+\t\t\tmemmove(os->buffer, p + 1, 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 \t\tsend_client_data(1, os->buffer, os->used - 1, use_sideband);\n \t\tos->buffer[0] = os->buffer[os->used - 1];\n@@ -212,7 +249,8 @@ static int relay_pack_data(int pack_objects_out, struct output_state *os,\n \treturn readsz;\n }\n \n-static void create_pack_file(struct upload_pack_data *pack_data)\n+static void create_pack_file(struct upload_pack_data *pack_data,\n+\t\t\t     const struct string_list *uri_protocols)\n {\n \tstruct child_process pack_objects = CHILD_PROCESS_INIT;\n \tstruct output_state output_state = { { 0 } };\n@@ -262,6 +300,11 @@ static void create_pack_file(struct upload_pack_data *pack_data)\n \t\t\t\t\t spec);\n \t\t}\n \t}\n+\tif (uri_protocols) {\n+\t\tfor (i = 0; i < uri_protocols->nr; i++)\n+\t\t\targv_array_pushf(&pack_objects.args, \"--uri-protocol=%s\",\n+\t\t\t\t\t uri_protocols->items[i].string);\n+\t}\n \n \tpack_objects.in = -1;\n \tpack_objects.out = -1;\n@@ -353,7 +396,8 @@ static void create_pack_file(struct upload_pack_data *pack_data)\n \t\tif (0 <= pu && (pfd[pu].revents & (POLLIN|POLLHUP))) {\n \t\t\tint result = relay_pack_data(pack_objects.out,\n \t\t\t\t\t\t     &output_state,\n-\t\t\t\t\t\t     pack_data->use_sideband);\n+\t\t\t\t\t\t     pack_data->use_sideband,\n+\t\t\t\t\t\t     !!uri_protocols);\n \n \t\t\tif (result == 0) {\n \t\t\t\tclose(pack_objects.out);\n@@ -1210,7 +1254,7 @@ void upload_pack(struct upload_pack_options *options)\n \t\treceive_needs(&data, &reader);\n \t\tif (data.want_obj.nr) {\n \t\t\tget_common_commits(&data, &reader);\n-\t\t\tcreate_pack_file(&data);\n+\t\t\tcreate_pack_file(&data, 0);\n \t\t}\n \t}\n \n@@ -1363,10 +1407,18 @@ static void process_args(struct packet_reader *request,\n \t\t\tcontinue;\n \t\t}\n \n+\t\tif (skip_prefix(arg, \"packfile-uris \", &p)) {\n+\t\t\tstring_list_split(&data->uri_protocols, p, ',', -1);\n+\t\t\tcontinue;\n+\t\t}\n+\n \t\t/* ignore unknown lines maybe? */\n \t\tdie(\"unexpected line: '%s'\", arg);\n \t}\n \n+\tif (data->uri_protocols.nr && !data->writer.use_sideband)\n+\t\tstring_list_clear(&data->uri_protocols, 0);\n+\n \tif (request->status != PACKET_READ_FLUSH)\n \t\tdie(_(\"expected flush after fetch arguments\"));\n }\n@@ -1553,8 +1605,12 @@ 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);\n \n-\t\t\tpacket_writer_write(&data.writer, \"packfile\\n\");\n-\t\t\tcreate_pack_file(&data);\n+\t\t\tif (data.uri_protocols.nr) {\n+\t\t\t\tcreate_pack_file(&data, &data.uri_protocols);\n+\t\t\t} else {\n+\t\t\t\tpacket_writer_write(&data.writer, \"packfile\\n\");\n+\t\t\t\tcreate_pack_file(&data, NULL);\n+\t\t\t}\n \t\t\tstate = FETCH_DONE;\n \t\t\tbreak;\n \t\tcase FETCH_DONE:\n@@ -1573,6 +1629,7 @@ int upload_pack_advertise(struct repository *r,\n \t\tint allow_filter_value;\n \t\tint allow_ref_in_want;\n \t\tint allow_sideband_all_value;\n+\t\tchar *str = NULL;\n \n \t\tstrbuf_addstr(value, \"shallow\");\n \n@@ -1594,6 +1651,14 @@ int upload_pack_advertise(struct repository *r,\n \t\t\t\t\t   &allow_sideband_all_value) &&\n \t\t     allow_sideband_all_value))\n \t\t\tstrbuf_addstr(value, \" sideband-all\");\n+\n+\t\tif (!repo_config_get_string(the_repository,\n+\t\t\t\t\t    \"uploadpack.blobpackfileuri\",\n+\t\t\t\t\t    &str) &&\n+\t\t    str) {\n+\t\t\tstrbuf_addstr(value, \" packfile-uris\");\n+\t\t\tfree(str);\n+\t\t}\n \t}\n \n \treturn 1;\n-- \n2.27.0.278.ge193c7cf3a9-goog\n\n"},{"id":"399471","messageId":"xmqqftb2eckl.fsf@gitster.c.googlers.com","threadId":"53583","inReplyTo":"cover.1591821067.git.jonathantanmy@google.com","subject":"Re: [PATCH v2 0/9] CDN offloading update","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-06-10T23:16:42Z","receivedAt":"2020-06-10T23:16:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan <jonathantanmy@google.com> writes:\n\n> Thanks, Junio, for taking a look at this.\n>\n> This version is based on cc/upload-pack-data-2. I thought of retaining\n> the base from the original version, but trying out the rebase showed\n> that there were some code movements that would clutter the range-diff\n> anyway, so I went ahead with the rebase.\n\nI think rebasing is not just justifiable, but is the right thing to\ndo in this case, as the topics fairly heavily steps each other's toe.\n\nThanks.\n"},{"id":"399490","messageId":"xmqqsgf2csps.fsf@gitster.c.googlers.com","threadId":"53583","inReplyTo":"d3b27394cad951a9f1412a04572439513d7e9f4d.1591821067.git.jonathantanmy@google.com","subject":"Re: [PATCH v2 1/9] http: use --stdin when indexing dumb HTTP pack","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-06-11T01:10:55Z","receivedAt":"2020-06-11T01:11:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan <jonathantanmy@google.com> writes:\n\n> diff --git a/http.c b/http.c\n> index 62aa995245..39cbd56702 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -2270,9 +2270,9 @@ int finish_http_pack_request(struct http_pack_request *preq)\n>  {\n>  \tstruct packed_git **lst;\n>  \tstruct packed_git *p = preq->target;\n> -\tchar *tmp_idx;\n> -\tsize_t len;\n>  \tstruct child_process ip = CHILD_PROCESS_INIT;\n> +\tint tmpfile_fd;\n> +\tint ret = 0;\n>  \n>  \tclose_pack_index(p);\n>  \n> @@ -2284,35 +2284,24 @@ int finish_http_pack_request(struct http_pack_request *preq)\n>  \t\tlst = &((*lst)->next);\n>  \t*lst = (*lst)->next;\n>  \n> -\tif (!strip_suffix(preq->tmpfile.buf, \".pack.temp\", &len))\n> -\t\tBUG(\"pack tmpfile does not end in .pack.temp?\");\n> -\ttmp_idx = xstrfmt(\"%.*s.idx.temp\", (int)len, preq->tmpfile.buf);\n> +\ttmpfile_fd = xopen(preq->tmpfile.buf, O_RDONLY);\n\nShouldn't we catch the case where we cannot open the file for\nreading and return -1 to have the caller handle the error, just like\nthe case where other errors are detected in this function?\n\n>  \targv_array_push(&ip.args, \"index-pack\");\n> -\targv_array_pushl(&ip.args, \"-o\", tmp_idx, NULL);\n> -\targv_array_push(&ip.args, preq->tmpfile.buf);\n> +\targv_array_push(&ip.args, \"--stdin\");\n>  \tip.git_cmd = 1;\n> -\tip.no_stdin = 1;\n> +\tip.in = tmpfile_fd;\n>  \tip.no_stdout = 1;\n>  \n>  \tif (run_command(&ip)) {\n> -\t\tunlink(preq->tmpfile.buf);\n> -\t\tunlink(tmp_idx);\n> -\t\tfree(tmp_idx);\n> -\t\treturn -1;\n> -\t}\n> -\n> -\tunlink(sha1_pack_index_name(p->hash));\n> -\n> -\tif (finalize_object_file(preq->tmpfile.buf, sha1_pack_name(p->hash))\n> -\t || finalize_object_file(tmp_idx, sha1_pack_index_name(p->hash))) {\n> -\t\tfree(tmp_idx);\n> -\t\treturn -1;\n> +\t\tret = -1;\n> +\t\tgoto cleanup;\n>  \t}\n>  \n>  \tinstall_packed_git(the_repository, p);\n> -\tfree(tmp_idx);\n> -\treturn 0;\n> +cleanup:\n> +\tclose(tmpfile_fd);\n> +\tunlink(preq->tmpfile.buf);\n> +\treturn ret;\n>  }\n>  \n>  struct http_pack_request *new_http_pack_request(\n"},{"id":"399491","messageId":"xmqqo8pqcrtq.fsf@gitster.c.googlers.com","threadId":"53583","inReplyTo":"a4a7ed512b251fb16f0b2400fac8c0c5cc14aa75.1591821067.git.jonathantanmy@google.com","subject":"Re: [PATCH v2 4/9] http-fetch: support fetching packfiles by URL","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-06-11T01:30:09Z","receivedAt":"2020-06-11T01:30:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan <jonathantanmy@google.com> writes:\n\n> Teach http-fetch the ability to download packfiles directly, given a\n> URL, and to verify them.\n>\n> The http_pack_request suite has been augmented with a function that\n> takes a URL directly. With this function, the hash is only used to\n> determine the name of the temporary file.\n>\n> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>\n> ---\n>  Documentation/git-http-fetch.txt |  9 ++++-\n>  http-fetch.c                     | 63 +++++++++++++++++++++++++++-----\n>  http.c                           | 28 ++++++++++----\n>  http.h                           | 11 ++++++\n>  t/t5550-http-fetch-dumb.sh       | 30 +++++++++++++++\n>  5 files changed, 123 insertions(+), 18 deletions(-)\n\nThis step certainly got easier to read thanks to the introduction of 3/9\nbut ...\n\n> @@ -68,25 +103,33 @@ int cmd_main(int argc, const char **argv)\n>  \t\t\tget_recover = 1;\n>  \t\t} else if (!strcmp(argv[arg], \"--stdin\")) {\n>  \t\t\tcommits_on_stdin = 1;\n> +\t\t} else if (skip_prefix(argv[arg], \"--packfile=\", &p)) {\n> +\t\t\tconst char *end;\n> +\n> +\t\t\tpackfile = 1;\n> +\t\t\tif (parse_oid_hex(p, &packfile_hash, &end) || *end)\n> +\t\t\t\tdie(_(\"argument to --packfile must be a valid hash (got '%s')\"), p);\n>  \t\t}\n>  \t\targ++;\n>  \t}\n> -\tif (argc != arg + 2 - commits_on_stdin)\n> +\tif (argc != arg + 2 - (commits_on_stdin || packfile))\n>  \t\tusage(http_fetch_usage);\n> -\tif (commits_on_stdin) {\n> -\t\tcommits = walker_targets_stdin(&commit_id, &write_ref);\n> -\t} else {\n> -\t\tcommit_id = (char **) &argv[arg++];\n> -\t\tcommits = 1;\n> -\t}\n>  \n>  \tsetup_git_directory();\n>  \n>  \tgit_config(git_default_config, NULL);\n>  \n> -\tif (!argv[arg])\n> -\t\tBUG(\"must have one arg remaining\");\n> +\tif (packfile) {\n> +\t\tfetch_single_packfile(&packfile_hash, argv[arg]);\n> +\t\treturn 0;\n> +\t}\n>  \n> +\tif (commits_on_stdin) {\n> +\t\tcommits = walker_targets_stdin(&commit_id, &write_ref);\n> +\t} else {\n> +\t\tcommit_id = (char **) &argv[arg++];\n> +\t\tcommits = 1;\n> +\t}\n\n... it would have been even less taxing to the readers' minds if the\ncode movement to run setup-git-directory and git-config before the\ncomputation of the walker target done here is *not* part of this\nstep.  Perhaps that can be done between 2/9 and 3/9, or as part of\n3/9?  The point is to reduce the mental load required to understand\nthe step that does *new* things, like this step.\n\n> diff --git a/http.h b/http.h\n> index bbc6b070f1..dc49c60165 100644\n> --- a/http.h\n> +++ b/http.h\n> @@ -216,6 +216,15 @@ int http_get_info_packs(const char *base_url,\n>  \n>  struct http_pack_request {\n>  \tchar *url;\n> +\n> +\t/*\n> +\t * If this is true, finish_http_pack_request() will pass \"--keep\" to\n> +\t * index-pack, resulting in the creation of a keep file, and will not\n> +\t * suppress its stdout (that is, the \"keep\\t<hash>\\n\" line will be\n> +\t * printed to stdout).\n> +\t */\n> +\tunsigned generate_keep : 1;\n\nOK.\n\n"},{"id":"399492","messageId":"xmqqk10ecrb4.fsf@gitster.c.googlers.com","threadId":"53583","inReplyTo":"566419ae74348ad3f7b1e5d484cf986fea29af73.1591821067.git.jonathantanmy@google.com","subject":"Re: [PATCH v2 8/9] fetch-pack: support more than one pack lockfile","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-06-11T01:41:19Z","receivedAt":"2020-06-11T01:41:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan <jonathantanmy@google.com> writes:\n\n> diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\n> index 4771100072..f66891b010 100644\n> --- a/builtin/fetch-pack.c\n> +++ b/builtin/fetch-pack.c\n> @@ -48,8 +48,8 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n>  \tstruct ref **sought = NULL;\n>  \tint nr_sought = 0, alloc_sought = 0;\n>  \tint fd[2];\n> -\tchar *pack_lockfile = NULL;\n> -\tchar **pack_lockfile_ptr = NULL;\n> +\tstruct string_list pack_lockfiles = STRING_LIST_INIT_DUP;\n> +\tstruct string_list *pack_lockfiles_ptr = NULL;\n>  \tstruct child_process *conn;\n>  \tstruct fetch_pack_args args;\n>  \tstruct oid_array shallow = OID_ARRAY_INIT;\n> @@ -134,7 +134,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n>  \t\t}\n>  \t\tif (!strcmp(\"--lock-pack\", arg)) {\n>  \t\t\targs.lock_pack = 1;\n> +\t\t\tpack_lockfiles_ptr = &pack_lockfiles;\n>  \t\t\tcontinue;\n>  \t\t}\n>  \t\tif (!strcmp(\"--check-self-contained-and-connected\", arg)) {\n> @@ -235,10 +235,15 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n>  \t}\n>  \n>  \tref = fetch_pack(&args, fd, ref, sought, nr_sought,\n> +\t\t\t &shallow, pack_lockfiles_ptr, version);\n> +\tif (pack_lockfiles.nr) {\n\nIn other parts of this patch, \"is the pack_lockfiles pointer NULL?\"\nis used and \"does the pack_lockfiles actually have any element?\" is\nnot checked, which was a bit hard to reason about, but the idea here\nis that the presence of the list tells the callee to use lockfiles\nand accumulate them in the string list and then this caller checks\nif we ended up locking any, which makes sense.\n\n> +\t\tint i;\n> +\n> +\t\tprintf(\"lock %s\\n\", pack_lockfiles.items[0].string);\n>  \t\tfflush(stdout);\n> +\t\tfor (i = 1; i < pack_lockfiles.nr; i++)\n> +\t\t\twarning(_(\"Lockfile created but not reported: %s\"),\n> +\t\t\t\tpack_lockfiles.items[i].string);\n>  \t}\n>  \tif (args.check_self_contained_and_connected &&\n>  \t    args.self_contained_and_connected) {\n> diff --git a/connected.c b/connected.c\n> index 3135b71e19..937b4bae38 100644\n> --- a/connected.c\n> +++ b/connected.c\n> @@ -43,10 +43,12 @@ int check_connected(oid_iterate_fn fn, void *cb_data,\n>  \n>  \tif (transport && transport->smart_options &&\n>  \t    transport->smart_options->self_contained_and_connected &&\n> -\t    transport->pack_lockfile &&\n> -\t    strip_suffix(transport->pack_lockfile, \".keep\", &base_len)) {\n> +\t    transport->pack_lockfiles.nr == 1 &&\n\nHmph, I can understand why \"== 1\" case must behave differently from\n\"== 0\" case, but shouldn't the behavior in \"> 1\" cases be more similar\nto the \"== 1\" case than \"== 0\" case?\n\n> +\t    strip_suffix(transport->pack_lockfiles.items[0].string,\n> +\t\t\t \".keep\", &base_len)) {\n>  \t\tstruct strbuf idx_file = STRBUF_INIT;\n> -\t\tstrbuf_add(&idx_file, transport->pack_lockfile, base_len);\n> +\t\tstrbuf_add(&idx_file, transport->pack_lockfiles.items[0].string,\n> +\t\t\t   base_len);\n>  \t\tstrbuf_addstr(&idx_file, \".idx\");\n>  \t\tnew_pack = add_packed_git(idx_file.buf, idx_file.len, 1);\n>  \t\tstrbuf_release(&idx_file);\n"},{"id":"399493","messageId":"xmqqftb2cqmp.fsf@gitster.c.googlers.com","threadId":"53583","inReplyTo":"6344c225897de1a2d8aa86d610e9eaf1c6ec82b4.1591821067.git.jonathantanmy@google.com","subject":"Re: [PATCH v2 6/9] Documentation: add Packfile URIs design doc","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-06-11T01:55:58Z","receivedAt":"2020-06-11T01:56:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan <jonathantanmy@google.com> writes:\n\n> +The server advertises the `packfile-uris` capability.\n> +\n> +If the client then communicates which protocols (HTTPS, etc.) it supports with\n> +a `packfile-uris` argument, the server MAY send a `packfile-uris` section\n> +directly before the `packfile` section (right after `wanted-refs` if it is\n> +sent) containing URIs of any of the given protocols. The URIs point to\n> +packfiles that use only features that the client has declared that it supports\n> +(e.g. ofs-delta and thin-pack). See protocol-v2.txt for the documentation of\n> +this section.\n\nEven if we have some pre-packaged packfiles, if the requestor lacks\nsome feature (by the way, shouldn't we consistently use \"capability\"\ninstead of \"feature\" to call things like \"ofs-delta\"?)  to be able\nto understand them, we would already know that fact (i.e. the lack\nof capability on the other side) by the time we have to decide if we\ncan give packfile-uris section.  OK, this makes sense.\n\n> +Clients should then download and index all the given URIs (in addition to\n> +downloading and indexing the packfile given in the `packfile` section of the\n> +response) before performing the connectivity check.\n\nLooking good.\n\nIs there other requirement on the packfile that can be retrieved\nwith the URI listed in the packfile-uris section?  It would be clear\nthat it must, together with the contents in the dynamic 'packfile'\nsection, give fully connected set of objects to the other side that\nhas the object it claimed to have.  But are we allowed to include\nextra/unasked-for objects in such a packfile?  Or is it better to\nleave it unspecified?\n\nThanks.\n"},{"id":"410775","messageId":"87zh35okzy.fsf@evledraar.gmail.com","threadId":"53583","inReplyTo":"6344c225897de1a2d8aa86d610e9eaf1c6ec82b4.1591821067.git.jonathantanmy@google.com","subject":"Re: [PATCH v2 6/9] Documentation: add Packfile URIs design doc","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2020-11-25T09:15:29Z","receivedAt":"2020-11-25T09:15:35Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Jun 10 2020, Jonathan Tan wrote:\n\n> +This is the implementation: a feature, marked experimental, that allows the\n> +server to be configured by one or more `uploadpack.blobPackfileUri=<sha1>\n> +<uri>` entries. Whenever the list of objects to be sent is assembled, all such\n> +blobs are excluded, replaced with URIs. The client will download those URIs,\n> +expecting them to each point to packfiles containing single blobs.\n\nI was poking at this recently to see whether I could change it into the\nmore dumb method I noted in\nhttps://public-inbox.org/git/87k1hv6eel.fsf@evledraar.gmail.com/\n\nAs an aside on a protocol level could that be supported with this\ncurrent verb by having the client say \"packfile-uris=early\" or something\nlike that instead of \"packfile-uris\"? The server advertising the same,\nand the client then just requesting packfile-uris before ls-refs or\nwhatever? The server would need to be stateful about what's requested\nwhen and serve up something different than the current\none-blob-per-pack. Any pointers to where/how to implement that would be\nwelcome, I got lost in the non-linearity of the\nconnect.c/fetch-pack.c/upload-pack.c code yesterday.\n\nBut I'm mainly replying here to ask if it's intentional that clients are\ntolerant of the server sending whatever it pleases in the supposedly\n\"single blob\" packs. As demonstrated by the tests passing with this\npatch:\n    \n    diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh\n    index 7d5b17909bb..4fe2030f4c1 100755\n    --- a/t/t5702-protocol-v2.sh\n    +++ b/t/t5702-protocol-v2.sh\n    @@ -797,11 +797,12 @@ test_expect_success 'when server does not send \"ready\", expect FLUSH' '\n     \n     configure_exclusion () {\n     \tgit -C \"$1\" hash-object \"$2\" >objh &&\n    +\techo -n shattered | git -C \"$1\" hash-object --stdin -w >>objh &&\n     \tgit -C \"$1\" pack-objects \"$HTTPD_DOCUMENT_ROOT_PATH/mypack\" <objh >packh &&\n     \tgit -C \"$1\" config --add \\\n     \t\t\"uploadpack.blobpackfileuri\" \\\n    -\t\t\"$(cat objh) $(cat packh) $HTTPD_URL/dumb/mypack-$(cat packh).pack\" &&\n    -\tcat objh\n    +\t\t\"$(head -n 1 objh) $(cat packh) $HTTPD_URL/dumb/mypack-$(cat packh).pack\" &&\n    +\thead -n 1 objh\n     }\n     \n     test_expect_success 'part of packfile response provided as URI' '\n    @@ -820,10 +821,11 @@ test_expect_success 'part of packfile response provided as URI' '\n     \tconfigure_exclusion \"$P\" my-blob >h &&\n     \tconfigure_exclusion \"$P\" other-blob >h2 &&\n     \n    -\tGIT_TRACE=1 GIT_TRACE_PACKET=\"$(pwd)/log\" GIT_TEST_SIDEBAND_ALL=1 \\\n    -\tgit -c protocol.version=2 \\\n    +\tGIT_TRACE=1 GIT_TRACE2=\"$(pwd)/log\" GIT_TRACE_PACKET=\"$(pwd)/log\" GIT_TEST_SIDEBAND_ALL=1 \\\n    +\tCHECK_SHATTERED=1 git -c protocol.version=2 \\\n     \t\t-c fetch.uriprotocols=http,https \\\n     \t\tclone \"$HTTPD_URL/smart/http_parent\" http_child &&\n    +\tcp \"$(pwd)/log\" /tmp/clone.log &&\n     \n     \t# Ensure that my-blob and other-blob are in separate packfiles.\n     \tfor idx in http_child/.git/objects/pack/*.idx\n    @@ -832,7 +834,7 @@ test_expect_success 'part of packfile response provided as URI' '\n     \t\t{\n     \t\t\tgrep \"^[0-9a-f]\\{16,\\} \" out || :\n     \t\t} >out.objectlist &&\n    -\t\tif test_line_count = 1 out.objectlist\n    +\t\tif true\n     \t\tthen\n     \t\t\tif grep $(cat h) out\n     \t\t\tthen\n\nAs you may guess from the \"shattered\" I was trying to find if the\nparticulars around the partial fsck allowed me to exploit this somehow,\nI haven't found a way to do that, just be annoying by sending the client\nmore than they asked for, but I could also do that with the normal\ndialog. Just wondering if the client should be opening the pack and\nbarfing if it has more than one object, or not care.\n"},{"id":"410793","messageId":"20201125190957.1113461-1-jonathantanmy@google.com","threadId":"53583","inReplyTo":"87zh35okzy.fsf@evledraar.gmail.com","subject":"Re: [PATCH v2 6/9] Documentation: add Packfile URIs design doc","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-11-25T19:09:57Z","receivedAt":"2020-11-25T19:10:17Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> On Wed, Jun 10 2020, Jonathan Tan wrote:\n> \n> > +This is the implementation: a feature, marked experimental, that allows the\n> > +server to be configured by one or more `uploadpack.blobPackfileUri=<sha1>\n> > +<uri>` entries. Whenever the list of objects to be sent is assembled, all such\n> > +blobs are excluded, replaced with URIs. The client will download those URIs,\n> > +expecting them to each point to packfiles containing single blobs.\n> \n> I was poking at this recently to see whether I could change it into the\n> more dumb method I noted in\n> https://public-inbox.org/git/87k1hv6eel.fsf@evledraar.gmail.com/\n> \n> As an aside on a protocol level could that be supported with this\n> current verb by having the client say \"packfile-uris=early\" or something\n> like that instead of \"packfile-uris\"? \n\nHmm...would the advantage of this be that the client can subsequently\nreport any OIDs it finds as \"want\"s?\n\nI guess the protocol could be extended to support \"packfile-uris\" at any\nnegotiation step.\n\n> The server advertising the same,\n> and the client then just requesting packfile-uris before ls-refs or\n> whatever? The server would need to be stateful about what's requested\n> when and serve up something different than the current\n> one-blob-per-pack. \n\nStatefulness will be difficult. Right now, protocol v2 is stateless,\nand updating it to be stateful will be difficult, I believe - at least\nfor HTTP, the statelessness design has been long there and other\nimplementations of Git or systems that use Git might have already made\nthat assumption (it is stateless both for protocol v0 and v2).\n\nAs for serving more than one blob per pack, the current protocol and\nimplementation already allows this. You can see a demonstration by\ncloning the following repository, which supports it on the server side:\n\n  GIT_TRACE_PACKET=1 git -c fetch.uriprotocols=https clone \\\n    https://chromium.googlesource.com/chromium/src/base\n\n> Any pointers to where/how to implement that would be\n> welcome, I got lost in the non-linearity of the\n> connect.c/fetch-pack.c/upload-pack.c code yesterday.\n\nupload_pack_v2() in upload-pack.c and do_fetch_pack_v2() in fetch-pack.c\nhave the state machines of the server and client side respectively - I\nthink those would be the first places to look.\n\n> But I'm mainly replying here to ask if it's intentional that clients are\n> tolerant of the server sending whatever it pleases in the supposedly\n> \"single blob\" packs. As demonstrated by the tests passing with this\n> patch:\n\n[snip test]\n\nYes, it has the same tolerance w.r.t. the packfile URI packs as w.r.t.\nthe inline packfile that gets sent.\n\n> As you may guess from the \"shattered\" I was trying to find if the\n> particulars around the partial fsck allowed me to exploit this somehow,\n> I haven't found a way to do that, just be annoying by sending the client\n> more than they asked for, but I could also do that with the normal\n> dialog. Just wondering if the client should be opening the pack and\n> barfing if it has more than one object, or not care.\n\nAh yes, as you said, it's the same as with the normal dialog.\n"},{"id":"411067","messageId":"87tut5vghw.fsf@evledraar.gmail.com","threadId":"53583","inReplyTo":"20201125190957.1113461-1-jonathantanmy@google.com","subject":"Re: [PATCH v2 6/9] Documentation: add Packfile URIs design doc","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2020-12-01T12:48:59Z","receivedAt":"2020-12-01T12:49:45Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Nov 25 2020, Jonathan Tan wrote:\n\n>> On Wed, Jun 10 2020, Jonathan Tan wrote:\n>> \n>> > +This is the implementation: a feature, marked experimental, that allows the\n>> > +server to be configured by one or more `uploadpack.blobPackfileUri=<sha1>\n>> > +<uri>` entries. Whenever the list of objects to be sent is assembled, all such\n>> > +blobs are excluded, replaced with URIs. The client will download those URIs,\n>> > +expecting them to each point to packfiles containing single blobs.\n>> \n>> I was poking at this recently to see whether I could change it into the\n>> more dumb method I noted in\n>> https://public-inbox.org/git/87k1hv6eel.fsf@evledraar.gmail.com/\n>> \n>> As an aside on a protocol level could that be supported with this\n>> current verb by having the client say \"packfile-uris=early\" or something\n>> like that instead of \"packfile-uris\"? \n>\n> Hmm...would the advantage of this be that the client can subsequently\n> report any OIDs it finds as \"want\"s?\n\nYes, as a means-to-an-end of allowing the server CDN part to be dumb and\npossibly out-of-date.\n\nI.e. the current \"here's some blobs and then a complimentary pack\"\nrequires very close CDN/server coordination.\n\nWhereas if you can say early in the dialog \"here's a pack that should\nhave most of what you need, then do a want/fetch to get up-to-date\" you\ncan just make that pack in a cronjob, the pack could even be corrupted\netc. and we'd just optimistically fall back on a full clone.\n\nIt wouldn't be reporting any OID it finds (too noisy). I haven't\nexperimented, but one dumb way to do it is for the server to serve up a\npack with \"start negotiating with this SHA\", which would just be a\nrecent revision of HEAD guaranteed to be in the served pack.\n\nOr the client could scour the pack and e.g. find all commit tips in it\nby running the equivalent of commit-graph on it first. More expensive,\nbut expensive on the client-side, and allows e.g. re-using that codepath\nto clone on the basis of a GIT_ALTERNATE_OBJECT_DIRECTORIES containing\nobjects the <remote url> might have already.\n\n> I guess the protocol could be extended to support \"packfile-uris\" at any\n> negotiation step.\n\n*nod*\n\n>> The server advertising the same,\n>> and the client then just requesting packfile-uris before ls-refs or\n>> whatever? The server would need to be stateful about what's requested\n>> when and serve up something different than the current\n>> one-blob-per-pack. \n>\n> Statefulness will be difficult. Right now, protocol v2 is stateless,\n> and updating it to be stateful will be difficult, I believe - at least\n> for HTTP, the statelessness design has been long there and other\n> implementations of Git or systems that use Git might have already made\n> that assumption (it is stateless both for protocol v0 and v2).\n\nSorry, on second thought what I'm suggesting doesn't really require\nstate, just the server to get/read/understand the \"client supports this\"\nflags that it already does.\n\n> As for serving more than one blob per pack, the current protocol and\n> implementation already allows this. You can see a demonstration by\n> cloning the following repository, which supports it on the server side:\n>\n>   GIT_TRACE_PACKET=1 git -c fetch.uriprotocols=https clone \\\n>     https://chromium.googlesource.com/chromium/src/base\n\nSo cloning that produces two packs (the packfile-uri one, and the\nnegotiated one), the packfile-uri one being:\n    \n    $ git show-index <objects/pack/pack-d917d9803d3acb03da7ea9e8ebb8e643364ba051.idx\n    12 3ea853619c232488e683139217585f520ec636e0 (8e33883c)\n    9371 83d1358e4979bf9aff879cb4150276cd3894463a (0ea17153)\n    13640 d0f8c611c044b36b8ac57b7a3af18a8d88e4dde2 (af8be7a0)\n    572 da9ca8b36d5029bd9b18addc054ba9c0b016e6bc (d57fdbac)\n\nSo 3ea853619 is a commit with a tree da9ca8b36/ , that tree has pointer\nto win/ via 83d1358e4, which has a blob win/.clang-tidy at d0f8c611c.\n\nSo this is intended & you're already using it like that. So shouldn't\nthe docs be updated from:\n\n    [...]one or more `uploadpack.blobPackfileUri=<sha1> <uri>`\n    entries. Whenever the list of objects to be sent is assembled, all\n    such blobs are excluded, replaced with URIs. The client will\n    download those URIs, expecting them to each point to packfiles\n    containing single blobs.\n\nTo something like:\n\n    [...]one or more `uploadpack.postWantPackfileUri=<sha1> <uri>`\n    entries. When the server sends the subsequent full pack any objects\n    sent out-of-bound may be excluded. The client will download those\n    URIs, expecting them to each contain some amount of objects. The\n    union of the packs found at these URIs and the server's own returned\n    packfile is expected to yield a valid repository that'll pass an\n    fsck.\n\nAside from the hassle of renaming the \"uploadpack.blobPackfileUri\"\nvariable to some more accurate \"this clearly has nothing to do with\nblobs per-se, really\" name that re-done paragraph seems to more\naccurately reflect what this is doing & intended to do.\n\nAlso, why it it necessary to make it an error if the expected hash for\nthe packfile doesn't match, since we're about to do a full fsck\nconnectivity check anyway? The POC patch at the end of this mail[1]\nshows that we mostly support it not matching now.\n\nI suppose you might want to point to an evil CDN, but since it currently\nneeds to fsck the potential for that evilness seems rather limited. It's\nnot a hassle in the current closely coupled server/CDN implementation,\nbut to e.g. have a server dumbly pointing at\nhttps://some-cdn.example/REPONAME/some-big-recent-pack.pack it's a\nhassle, so having it at least optionally support the NULL_SHA would be\nideal for that. So I'm curious what you think it adds to the overall\nsecurity of the feature.\n\n>> Any pointers to where/how to implement that would be\n>> welcome, I got lost in the non-linearity of the\n>> connect.c/fetch-pack.c/upload-pack.c code yesterday.\n>\n> upload_pack_v2() in upload-pack.c and do_fetch_pack_v2() in fetch-pack.c\n> have the state machines of the server and client side respectively - I\n> think those would be the first places to look.\n\nThanks.\n\n>> But I'm mainly replying here to ask if it's intentional that clients are\n>> tolerant of the server sending whatever it pleases in the supposedly\n>> \"single blob\" packs. As demonstrated by the tests passing with this\n>> patch:\n>\n> [snip test]\n>\n> Yes, it has the same tolerance w.r.t. the packfile URI packs as w.r.t.\n> the inline packfile that gets sent.\n>\n>> As you may guess from the \"shattered\" I was trying to find if the\n>> particulars around the partial fsck allowed me to exploit this somehow,\n>> I haven't found a way to do that, just be annoying by sending the client\n>> more than they asked for, but I could also do that with the normal\n>> dialog. Just wondering if the client should be opening the pack and\n>> barfing if it has more than one object, or not care.\n>\n> Ah yes, as you said, it's the same as with the normal dialog.\n\n1.:\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex b10c432315..87d948b023 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -1661,9 +1661,9 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \n \t\tif (memcmp(packfile_uris.items[i].string, packname,\n \t\t\t   the_hash_algo->hexsz))\n-\t\t\tdie(\"fetch-pack: pack downloaded from %s does not match expected hash %.*s\",\n-\t\t\t    uri, (int) the_hash_algo->hexsz,\n-\t\t\t    packfile_uris.items[i].string);\n+\t\t\twarning(\"fetch-pack: pack downloaded from %s does not match expected hash %.*s\",\n+\t\t\t\turi, (int) the_hash_algo->hexsz,\n+\t\t\t\tpackfile_uris.items[i].string);\n \n \t\tstring_list_append_nodup(pack_lockfiles,\n \t\t\t\t\t xstrfmt(\"%s/pack/pack-%s.keep\",\ndiff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh\nindex 7d5b17909b..bc0fc4d8e3 100755\n--- a/t/t5702-protocol-v2.sh\n+++ b/t/t5702-protocol-v2.sh\n@@ -798,9 +798,13 @@ test_expect_success 'when server does not send \"ready\", expect FLUSH' '\n configure_exclusion () {\n \tgit -C \"$1\" hash-object \"$2\" >objh &&\n \tgit -C \"$1\" pack-objects \"$HTTPD_DOCUMENT_ROOT_PATH/mypack\" <objh >packh &&\n+\tfake_sha=$(git hash-object --stdin <packh) &&\n+\tmv \\\n+\t\t\"$HTTPD_DOCUMENT_ROOT_PATH/mypack-$(cat packh).pack\" \\\n+\t\t\"$HTTPD_DOCUMENT_ROOT_PATH/mypack-$fake_sha.pack\"\n \tgit -C \"$1\" config --add \\\n \t\t\"uploadpack.blobpackfileuri\" \\\n-\t\t\"$(cat objh) $(cat packh) $HTTPD_URL/dumb/mypack-$(cat packh).pack\" &&\n+\t\t\"$(cat objh) $fake_sha $HTTPD_URL/dumb/mypack-$fake_sha.pack\" &&\n \tcat objh\n }\n \n@@ -852,7 +856,7 @@ test_expect_success 'part of packfile response provided as URI' '\n \ttest_line_count = 6 filelist\n '\n \n-test_expect_success 'fetching with valid packfile URI but invalid hash fails' '\n+-test_expect_success 'fetching with valid packfile URI but invalid hash warns' '\n \tP=\"$HTTPD_DOCUMENT_ROOT_PATH/http_parent\" &&\n \trm -rf \"$P\" http_child log &&\n \n@@ -870,13 +874,12 @@ test_expect_success 'fetching with valid packfile URI but invalid hash fails' '\n \t# the hash of the packfile, since the hash does not matter for this\n \t# test as long as it is not the hash of the pack, and it is of the\n \t# expected length.\n-\tgit -C \"$P\" hash-object other-blob >objh &&\n \tgit -C \"$P\" pack-objects \"$HTTPD_DOCUMENT_ROOT_PATH/mypack\" <objh >packh &&\n \tgit -C \"$P\" config --add \\\n \t\t\"uploadpack.blobpackfileuri\" \\\n \t\t\"$(cat objh) $(cat objh) $HTTPD_URL/dumb/mypack-$(cat packh).pack\" &&\n \n-\ttest_must_fail env GIT_TEST_SIDEBAND_ALL=1 \\\n+\tenv GIT_TEST_SIDEBAND_ALL=1 \\\n \t\tgit -c protocol.version=2 \\\n \t\t-c fetch.uriprotocols=http,https \\\n \t\tclone \"$HTTPD_URL/smart/http_parent\" http_child 2>err &&\n"}]}