{"thread":{"id":"41602","subject":"[PATCH] index-pack: correct --keep[=]","startedAt":"2016-03-03T19:14:46Z","lastAt":"2016-03-04T15:51:52Z","messageCount":9,"participants":["Junio C Hamano","Jeff King","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"280142","messageId":"xmqq1t7r2x21.fsf@gitster.mtv.corp.google.com","threadId":"41602","inReplyTo":null,"subject":"[PATCH] index-pack: correct --keep[=]","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-03T19:14:46Z","receivedAt":"2016-03-03T19:14:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"When 592ce208 (index-pack: use strip_suffix to avoid magic numbers,\n2014-06-30) refactored the code to derive names of .idx and .keep\nfiles from the name of .pack file, a copy-and-paste typo crept in,\nmistakingly attempting to create and store the keep message file in\nthe .idx file we just created, instead of .keep file.\n\nAs we create the .keep file with O_CREAT|O_EXCL, and we do so after\nwe write the .idx file, we luckily do not clobber the .idx file, but\nbecause we deliberately ignored EEXIST when creating .keep file\n(which is justifiable because only the existence of .keep file\nmatters), nobody noticed this mistake so far.\n\nSigned-off-by: Junio C Hamano \n---\n\n * In a later patch, I'll be adding another file to the family of\n .pack/.idx/.keep files, and the body of these if() statements\n would be made into a helper function when it happens. This is to\n fix it directly on top of the problematic commit without such a\n helper, the result of which could be merged to 2.1.x and later\n maintenance series.\n\n builtin/index-pack.c | 2 +-\n t/t5300-pack-object.sh | 6 ++++++\n 2 files changed, 7 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex d4b77fd..3814731 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -1617,7 +1617,7 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)\n \t\t\tdie(_(\"packfile name '%s' does not end with '.pack'\"),\n \t\t\t pack_name);\n \t\tstrbuf_add(&keep_name_buf, pack_name, len);\n-\t\tstrbuf_addstr(&keep_name_buf, \".idx\");\n+\t\tstrbuf_addstr(&keep_name_buf, \".keep\");\n \t\tkeep_name = keep_name_buf.buf;\n \t}\n \tif (verify) {\ndiff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh\nindex 20c1961..0e3cadf 100755\n--- a/t/t5300-pack-object.sh\n+++ b/t/t5300-pack-object.sh\n@@ -284,6 +284,12 @@ test_expect_success \\\n git index-pack test-3.pack &&\n cmp test-3.idx test-3-${packname_3}.idx &&\n \n+ cat test-1-${packname_1}.pack >test-4.pack &&\n+ rm -f test-4.keep &&\n+ git index-pack --keep=why test-4.pack &&\n+ cmp test-1-${packname_1}.idx test-4.idx &&\n+ test -f test-4.keep &&\n+\n :'\n \n test_expect_success 'unpacking with --strict' '\n-- \nv2.0.1-6-g592ce20\n"},{"id":"280144","messageId":"20160303194727.GA26087@sigill.intra.peff.net","threadId":"41602","inReplyTo":"xmqq1t7r2x21.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] index-pack: correct --keep[=]","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-03-03T19:47:27Z","receivedAt":"2016-03-03T19:47:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 03, 2016 at 11:14:46AM -0800, Junio C Hamano wrote:\n\n> When 592ce208 (index-pack: use strip_suffix to avoid magic numbers,\n> 2014-06-30) refactored the code to derive names of .idx and .keep\n> files from the name of .pack file, a copy-and-paste typo crept in,\n> mistakingly attempting to create and store the keep message file in\n> the .idx file we just created, instead of .keep file.\n> \n> As we create the .keep file with O_CREAT|O_EXCL, and we do so after\n> we write the .idx file, we luckily do not clobber the .idx file, but\n> because we deliberately ignored EEXIST when creating .keep file\n> (which is justifiable because only the existence of .keep file\n> matters), nobody noticed this mistake so far.\n\nEek. Sorry about that. Your explanation and fix looks obviously correct.\n\n-Peff\n"},{"id":"280150","messageId":"xmqqsi071bw1.fsf@gitster.mtv.corp.google.com","threadId":"41602","inReplyTo":"xmqq1t7r2x21.fsf@gitster.mtv.corp.google.com","subject":"[PATCH] index-pack: add a helper function to derive .idx/.keep filename","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-03T21:37:18Z","receivedAt":"2016-03-03T21:37:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"These are automatically named by replacing .pack suffix in the\nname of the packfile. Add a small helper to do so, as I'll be\nadding another one soonish.\n\nSigned-off-by: Junio C Hamano \n---\n builtin/index-pack.c | 35 +++++++++++++++++------------------\n 1 file changed, 17 insertions(+), 18 deletions(-)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 285424f..a5588a2 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -1598,6 +1598,18 @@ static void show_pack_info(int stat_only)\n \t}\n }\n \n+static const char *derive_filename(const char *pack_name, const char *suffix,\n+\t\t\t\t struct strbuf *buf)\n+{\n+\tsize_t len;\n+\tif (!strip_suffix(pack_name, \".pack\", &len))\n+\t\tdie(_(\"packfile name '%s' does not end with '.pack'\"),\n+\t\t pack_name);\n+\tstrbuf_add(buf, pack_name, len);\n+\tstrbuf_addstr(buf, suffix);\n+\treturn buf->buf;\n+}\n+\n int cmd_index_pack(int argc, const char **argv, const char *prefix)\n {\n \tint i, fix_thin_pack = 0, verify = 0, stat_only = 0;\n@@ -1706,24 +1718,11 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)\n \t\tusage(index_pack_usage);\n \tif (fix_thin_pack && !from_stdin)\n \t\tdie(_(\"--fix-thin cannot be used without --stdin\"));\n-\tif (!index_name && pack_name) {\n-\t\tsize_t len;\n-\t\tif (!strip_suffix(pack_name, \".pack\", &len))\n-\t\t\tdie(_(\"packfile name '%s' does not end with '.pack'\"),\n-\t\t\t pack_name);\n-\t\tstrbuf_add(&index_name_buf, pack_name, len);\n-\t\tstrbuf_addstr(&index_name_buf, \".idx\");\n-\t\tindex_name = index_name_buf.buf;\n-\t}\n-\tif (keep_msg && !keep_name && pack_name) {\n-\t\tsize_t len;\n-\t\tif (!strip_suffix(pack_name, \".pack\", &len))\n-\t\t\tdie(_(\"packfile name '%s' does not end with '.pack'\"),\n-\t\t\t pack_name);\n-\t\tstrbuf_add(&keep_name_buf, pack_name, len);\n-\t\tstrbuf_addstr(&keep_name_buf, \".keep\");\n-\t\tkeep_name = keep_name_buf.buf;\n-\t}\n+\tif (!index_name && pack_name)\n+\t\tindex_name = derive_filename(pack_name, \".idx\", &index_name_buf);\n+\tif (keep_msg && !keep_name && pack_name)\n+\t\tkeep_name = derive_filename(pack_name, \".keep\", &keep_name_buf);\n+\n \tif (verify) {\n \t\tif (!index_name)\n \t\t\tdie(_(\"--verify with no packfile name given\"));\n-- \n2.8.0-rc0-116-g5beb0d5\n"},{"id":"280151","messageId":"CAPig+cTgJxtuVOEAes_T1jjdN-MqHnSXkmL8mQ80n7rRd=gqXg@mail.gmail.com","threadId":"41602","inReplyTo":"xmqq1t7r2x21.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] index-pack: correct --keep[=]","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-03-03T21:44:58Z","receivedAt":"2016-03-03T21:44:58Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Mar 3, 2016 at 2:14 PM, Junio C Hamano wrote:\n> When 592ce208 (index-pack: use strip_suffix to avoid magic numbers,\n> 2014-06-30) refactored the code to derive names of .idx and .keep\n> files from the name of .pack file, a copy-and-paste typo crept in,\n> mistakingly attempting to create and store the keep message file in\n\ns/mistakingly/mistakenly/\n\n> the .idx file we just created, instead of .keep file.\n>\n> As we create the .keep file with O_CREAT|O_EXCL, and we do so after\n> we write the .idx file, we luckily do not clobber the .idx file, but\n> because we deliberately ignored EEXIST when creating .keep file\n> (which is justifiable because only the existence of .keep file\n> matters), nobody noticed this mistake so far.\n>\n> Signed-off-by: Junio C Hamano \n"},{"id":"280157","messageId":"20160303222902.GB26712@sigill.intra.peff.net","threadId":"41602","inReplyTo":"xmqqsi071bw1.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] index-pack: add a helper function to derive .idx/.keep filename","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-03-03T22:29:03Z","receivedAt":"2016-03-03T22:29:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 03, 2016 at 01:37:18PM -0800, Junio C Hamano wrote:\n\n> These are automatically named by replacing .pack suffix in the\n> name of the packfile. Add a small helper to do so, as I'll be\n> adding another one soonish.\n> \n> Signed-off-by: Junio C Hamano \n> ---\n> builtin/index-pack.c | 35 +++++++++++++++++------------------\n> 1 file changed, 17 insertions(+), 18 deletions(-)\n\nEven without the upcoming \"another one\", this is a nice readability\nimprovement.\n\n-Peff\n"},{"id":"280159","messageId":"xmqqfuw7186z.fsf_-_@gitster.mtv.corp.google.com","threadId":"41602","inReplyTo":"20160303222902.GB26712@sigill.intra.peff.net","subject":"[PATCH] index-pack: --clone-bundle option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-03T22:57:08Z","receivedAt":"2016-03-03T22:57:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Teach a new option \"--clone-bundle\" to \"git index-pack\" to create a\nsplit bundle file that uses an existing packfile as its data part.\n\nThe expected \"typical\" preparation for helping initial clone would\nstart by preparing a packfile that contains most of the history and\nadd another packfile that contains the remainder (e.g. the objects\nthat are only reachable from reflog entries). The first pack can\nthen be used as the data part of a split bundle and these two files\ncan be served as static files to bootstrap the clients without\nincurring any more CPU cycles to the server side.\n\nAmong the objects in the packfile, the ones that are not referenced\nby no other objects are identified and recorded as the \"references\"\nin the resulting bundle. As the packfile does not record any ref\ninformation, however, the names of the \"references\" recorded in the\nbundle need to be synthesized; we arbitrarily choose to record the\nobject whose name is $SHA1 as refs/objects/$SHA1.\n\nNote that this name choice does not matter very much in the larger\npicture. As an initial clone that bootstraps from a clone-bundle is\nexpected to do a rough equivalent of:\n\n # create a new repository\n git init new-repository &&\n git remote add origin $URL &&\n\n # prime the object store and anchor the history to temporary\n # references\n git fetch $bundle 'refs/*:refs/temporary/*' &&\n\n # fetch the more recent history from the true origin\n git fetch origin &&\n git checkout -f &&\n\n # remove the temporary refs\n git for-each-ref -z --format=%(refname) refs/temporary/ |\n xargs -0 git update-ref -d\n\nthe names recorded in the bundle will not really matter to the end\nresult.\n\nSigned-off-by: Junio C Hamano \n---\n\n * The upcoming \"another one\" looks like this.\n\n Documentation/git-index-pack.txt | 10 ++++++-\n builtin/index-pack.c | 57 +++++++++++++++++++++++++++++++++++++---\n 2 files changed, 62 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/git-index-pack.txt b/Documentation/git-index-pack.txt\nindex 7a4e055..ade2812 100644\n--- a/Documentation/git-index-pack.txt\n+++ b/Documentation/git-index-pack.txt\n@@ -9,7 +9,7 @@ git-index-pack - Build pack index file for an existing packed archive\n SYNOPSIS\n --------\n [verse]\n-'git index-pack' [-v] [-o ] \n+'git index-pack' [-v] [-o ] [--clone-bundle] \n 'git index-pack' --stdin [--fix-thin] [--keep] [-v] [-o ]\n []\n \n@@ -35,6 +35,14 @@ OPTIONS\n \tfails if the name of packed archive does not end\n \twith .pack).\n \n+--clone-bundle::\n+\tWrite a split bundle file that uses the as its\n+\tdata. The must not contain any broken links, or\n+\tthe bundle file will not be written. The prerequisite list\n+\tof the resulting bundle will be empty. The reference list\n+\tof the resulting bundle points at tips of the history in the\n+\t.\n+\n --stdin::\n \tWhen this flag is provided, the pack is read from stdin\n \tinstead and a copy is then written to . If\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex a5588a2..8fc04b0 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -13,7 +13,7 @@\n #include \"thread-utils.h\"\n \n static const char index_pack_usage[] =\n-\"git index-pack [-v] [-o ] [--keep | --keep=] [--verify] [--strict] ( | --stdin [--fix-thin] [])\";\n+\"git index-pack [-v] [-o ] [--keep | --keep=] [--verify] [--strict] [--clone-bundle] ( | --stdin [--fix-thin] [])\";\n \n struct object_entry {\n \tstruct pack_idx_entry idx;\n@@ -1373,9 +1373,40 @@ static void fix_unresolved_deltas(struct sha1file *f)\n \tfree(sorted_by_pos);\n }\n \n+static void write_bundle_file(const char *filename, unsigned char *sha1,\n+\t\t\t const char *packname)\n+{\n+\tint fd = open(filename, O_CREAT|O_EXCL|O_WRONLY, 0600);\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tstruct stat st;\n+\tint i;\n+\n+\tif (stat(packname, &st))\n+\t\tdie(_(\"cannot stat %s\"), packname);\n+\n+\tstrbuf_addstr(&buf, \"# v3 git bundle\\n\");\n+\tstrbuf_addf(&buf, \"size: %lu\\n\", (unsigned long) st.st_size);\n+\tstrbuf_addf(&buf, \"sha1: %s\\n\", sha1_to_hex(sha1));\n+\tstrbuf_addf(&buf, \"data: %s\\n\\n\", packname);\n+\n+\tfor (i = 0; i < nr_objects; i++) {\n+\t\tstruct object *o = lookup_object(objects[i].idx.sha1);\n+\t\tif (!o || (o->flags & FLAG_LINK))\n+\t\t\tcontinue;\n+\t\tstrbuf_addf(&buf, \"%s refs/objects/%s\\n\",\n+\t\t\t sha1_to_hex(o->oid.hash),\n+\t\t\t sha1_to_hex(o->oid.hash));\n+\t}\n+\tif (write_in_full(fd, buf.buf, buf.len) != buf.len)\n+\t\tdie(_(\"cannot write bundle header for %s\"), packname);\n+\tclose(fd);\n+\tstrbuf_release(&buf);\n+}\n+\n static void final(const char *final_pack_name, const char *curr_pack_name,\n \t\t const char *final_index_name, const char *curr_index_name,\n \t\t const char *keep_name, const char *keep_msg,\n+\t\t const char *bundle_name, int foreign_nr,\n \t\t unsigned char *sha1)\n {\n \tconst char *report = \"pack\";\n@@ -1457,6 +1488,13 @@ static void final(const char *final_pack_name, const char *curr_pack_name,\n \t\t\tinput_offset += err;\n \t\t}\n \t}\n+\n+\tif (bundle_name) {\n+\t\tif (foreign_nr)\n+\t\t\twarning(_(\"not writing bundle for an incomplete pack\"));\n+\t\telse\n+\t\t\twrite_bundle_file(bundle_name, sha1, final_pack_name);\n+\t}\n }\n \n static int git_index_pack_config(const char *k, const char *v, void *cb)\n@@ -1612,12 +1650,14 @@ static const char *derive_filename(const char *pack_name, const char *suffix,\n \n int cmd_index_pack(int argc, const char **argv, const char *prefix)\n {\n-\tint i, fix_thin_pack = 0, verify = 0, stat_only = 0;\n+\tint i, fix_thin_pack = 0, verify = 0, stat_only = 0, clone_bundle = 0;\n \tconst char *curr_index;\n \tconst char *index_name = NULL, *pack_name = NULL;\n \tconst char *keep_name = NULL, *keep_msg = NULL;\n-\tstruct strbuf index_name_buf = STRBUF_INIT,\n-\t\t keep_name_buf = STRBUF_INIT;\n+\tconst char *bundle_name = NULL;\n+\tstruct strbuf index_name_buf = STRBUF_INIT;\n+\tstruct strbuf keep_name_buf = STRBUF_INIT;\n+\tstruct strbuf bundle_name_buf = STRBUF_INIT;\n \tstruct pack_idx_entry **idx_objects;\n \tstruct pack_idx_option opts;\n \tunsigned char pack_sha1[20];\n@@ -1661,6 +1701,10 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)\n \t\t\t\tverify = 1;\n \t\t\t\tshow_stat = 1;\n \t\t\t\tstat_only = 1;\n+\t\t\t} else if (!strcmp(arg, \"--clone-bundle\")) {\n+\t\t\t\tstrict = 1;\n+\t\t\t\tclone_bundle = 1;\n+\t\t\t\tcheck_self_contained_and_connected = 1;\n \t\t\t} else if (!strcmp(arg, \"--keep\")) {\n \t\t\t\tkeep_msg = \"\";\n \t\t\t} else if (starts_with(arg, \"--keep=\")) {\n@@ -1718,10 +1762,14 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)\n \t\tusage(index_pack_usage);\n \tif (fix_thin_pack && !from_stdin)\n \t\tdie(_(\"--fix-thin cannot be used without --stdin\"));\n+\tif (clone_bundle && from_stdin)\n+\t\tdie(_(\"--clone-bundle cannot be used with --stdin\"));\n \tif (!index_name && pack_name)\n \t\tindex_name = derive_filename(pack_name, \".idx\", &index_name_buf);\n \tif (keep_msg && !keep_name && pack_name)\n \t\tkeep_name = derive_filename(pack_name, \".keep\", &keep_name_buf);\n+\tif (clone_bundle)\n+\t\tbundle_name = derive_filename(pack_name, \".bndl\", &bundle_name_buf);\n \n \tif (verify) {\n \t\tif (!index_name)\n@@ -1768,6 +1816,7 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)\n \t\tfinal(pack_name, curr_pack,\n \t\t index_name, curr_index,\n \t\t keep_name, keep_msg,\n+\t\t bundle_name, foreign_nr,\n \t\t pack_sha1);\n \telse\n \t\tclose(input_fd);\n-- \n2.8.0-rc0-133-g6d295b5\n"},{"id":"280161","messageId":"xmqqbn6v174b.fsf@gitster.mtv.corp.google.com","threadId":"41602","inReplyTo":"xmqqfuw7186z.fsf_-_@gitster.mtv.corp.google.com","subject":"Re: [PATCH] index-pack: --clone-bundle option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-03T23:20:20Z","receivedAt":"2016-03-03T23:20:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Note that this name choice does not matter very much in the larger\n> picture. As an initial clone that bootstraps from a clone-bundle is\n> expected to do a rough equivalent of:\n>\n> # create a new repository\n> git init new-repository &&\n> git remote add origin $URL &&\n>\n> # prime the object store and anchor the history to temporary\n> # references\n> git fetch $bundle 'refs/*:refs/temporary/*' &&\n>\n> # fetch the more recent history from the true origin\n> git fetch origin &&\n> git checkout -f &&\n>\n> # remove the temporary refs\n> git for-each-ref -z --format=%(refname) refs/temporary/ |\n> xargs -0 git update-ref -d\n>\n> the names recorded in the bundle will not really matter to the end\n> result.\n\nActually, the real implementation of \"bootstrap with clone-bundle\"\nis more likely to go like this:\n\n * The client gets redirected to $name.bndl file, and obtains a\n fairly full $name.pack file by downloading them as static\n files;\n\n * The client initializes an empty repository;\n\n * The pack file is stored at .git/objects/pack/pack-$sha1.pack;\n\n * When the client does a \"git fetch origin\" to fill the more\n recent part, fetch-pack.c::find_common() would read from the\n \"git bundle list-heads $name.bndl\" to learn the \"reference\"\n objects. These are thrown at rev_list_insert_ref() and are\n advertised as \"have\"s, just like we advertise objects at the\n tip of refs in alternate repository.\n\nSo there will be no refs/temporary/* hierarchy we would need to\nworry about cleaning up.\n\nAnother possible variant is to redirect the client directly to\ndownload pack-$sha1.pack; \"index-pack\" needs to be run on the client\nside anyway to create pack-$sha1.idx, so at that time it could do\nthe equivalent of \"--clone-bundle\" processing (it is not strictly\nnecessary to create a split bundle) to find the tips of histories,\nand use that information when running \"git fetch origin\".\n\nSo, even though I started working from \"split bundle\", we may not\nhave to have such a feature after all to support CDN offloadable and\nresumable clone.\n"},{"id":"280195","messageId":"20160304153359.GA16300@sigill.intra.peff.net","threadId":"41602","inReplyTo":"xmqqfuw7186z.fsf_-_@gitster.mtv.corp.google.com","subject":"Re: [PATCH] index-pack: --clone-bundle option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-03-04T15:34:00Z","receivedAt":"2016-03-04T15:34:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 03, 2016 at 02:57:08PM -0800, Junio C Hamano wrote:\n\n> Teach a new option \"--clone-bundle\" to \"git index-pack\" to create a\n> split bundle file that uses an existing packfile as its data part.\n> \n> The expected \"typical\" preparation for helping initial clone would\n> start by preparing a packfile that contains most of the history and\n> add another packfile that contains the remainder (e.g. the objects\n> that are only reachable from reflog entries). The first pack can\n> then be used as the data part of a split bundle and these two files\n> can be served as static files to bootstrap the clients without\n> incurring any more CPU cycles to the server side.\n\nJust FYI, because I know our setup is anything but typical, but this\nprobably _won't_ be what we do at GitHub. We try to keep everything in a\nsingle packfile that contains many \"islands\", which are not allowed to\ndelta between each other[1]. Individual forks of a repo get their own\nislands, unreachable objects are in another one, and so forth. So we'd\nprobably never want to provide a whole packfile, as it has way too much\ndata. But we _would_ be able to take a bitmap over the set of packed\nobjects such that if you blindly spew out every object with its bit set,\nthe resulting pack has the desired objects.\n\nThat's not as turn-key for serving with a dumb http server, obviously.\nAnd I don't expect you to write that part. I just wanted to let you know\nwhere I foresee having to take this for GitHub to get it deployed.\n\n[1] It's a little more complicated than \"don't delta with each other\".\n Each object has its own bitmap of which islands it is in, and the\n rule is that you can use a delta base iff it is in a subset of your\n own islands. The point is that a clone of a particular island should\n never have to throw away on-disk deltas.\n\n Cleaning up and sharing that code upstream has been on my todo list\n for about 2 years, but somehow there's always new code to be\n written. :-/ I'm happy to share if people want to look at the messy\n state.\n\n> Among the objects in the packfile, the ones that are not referenced\n> by no other objects are identified and recorded as the \"references\"\n\ns/no/any/ (double negative)\n\n> in the resulting bundle. As the packfile does not record any ref\n> information, however, the names of the \"references\" recorded in the\n> bundle need to be synthesized; we arbitrarily choose to record the\n> object whose name is $SHA1 as refs/objects/$SHA1.\n\nMakes sense. In an \"island\" world I'd want to write one bitmap per\nisland, and then reconstruct that list on the fly by referencing the\nisland bitmap with the sha1 names in the pack revidx.\n\n> +static void write_bundle_file(const char *filename, unsigned char *sha1,\n> +\t\t\t const char *packname)\n> +{\n> +\tint fd = open(filename, O_CREAT|O_EXCL|O_WRONLY, 0600);\n> +\tstruct strbuf buf = STRBUF_INIT;\n> +\tstruct stat st;\n> +\tint i;\n> +\n\nWe should probably bail here if \"fd < 0\", though I guess technically we\nwill notice in write_in_full(). :)\n\n> +\tif (stat(packname, &st))\n> +\t\tdie(_(\"cannot stat %s\"), packname);\n> +\n> +\tstrbuf_addstr(&buf, \"# v3 git bundle\\n\");\n> +\tstrbuf_addf(&buf, \"size: %lu\\n\", (unsigned long) st.st_size);\n> +\tstrbuf_addf(&buf, \"sha1: %s\\n\", sha1_to_hex(sha1));\n> +\tstrbuf_addf(&buf, \"data: %s\\n\\n\", packname);\n\nThis \"sha1\" field is the sha1 of the packfile, right? If so, I think\nit's redundant with the pack trailer found in the pack at \"packname\".\nI'd prefer not to include that here, because it makes it harder to\ngenerate these v3 bundle headers dynamically (you have to actually\nchecksum the pack subset to come up with sha1 up front, as opposed to\nchecksumming as you stream the pack out).\n\nIt _could_ be used as an http etag, but I think it would make more sense\nto push implementations toward using a unique value for the \"packname\"\n(i.e., so that fetching \"https://example.com/foo/1234abcd.pack\" is\nbasically immutable). And I think that comes basically for free, because\nthe packname has that same hash in it already.\n\n-Peff\n"},{"id":"280196","messageId":"20160304155151.GA18482@sigill.intra.peff.net","threadId":"41602","inReplyTo":"xmqqbn6v174b.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] index-pack: --clone-bundle option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-03-04T15:51:52Z","receivedAt":"2016-03-04T15:51:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 03, 2016 at 03:20:20PM -0800, Junio C Hamano wrote:\n\n> Junio C Hamano writes:\n> \n> > Note that this name choice does not matter very much in the larger\n> > picture. As an initial clone that bootstraps from a clone-bundle is\n> > expected to do a rough equivalent of:\n> >\n> > # create a new repository\n> > git init new-repository &&\n> > git remote add origin $URL &&\n> >\n> > # prime the object store and anchor the history to temporary\n> > # references\n> > git fetch $bundle 'refs/*:refs/temporary/*' &&\n> >\n> > # fetch the more recent history from the true origin\n> > git fetch origin &&\n> > git checkout -f &&\n> >\n> > # remove the temporary refs\n> > git for-each-ref -z --format=%(refname) refs/temporary/ |\n> > xargs -0 git update-ref -d\n> >\n> > the names recorded in the bundle will not really matter to the end\n> > result.\n> \n> Actually, the real implementation of \"bootstrap with clone-bundle\"\n> is more likely to go like this:\n> \n> * The client gets redirected to $name.bndl file, and obtains a\n> fairly full $name.pack file by downloading them as static\n> files;\n> \n> * The client initializes an empty repository;\n> \n> * The pack file is stored at .git/objects/pack/pack-$sha1.pack;\n> \n> * When the client does a \"git fetch origin\" to fill the more\n> recent part, fetch-pack.c::find_common() would read from the\n> \"git bundle list-heads $name.bndl\" to learn the \"reference\"\n> objects. These are thrown at rev_list_insert_ref() and are\n> advertised as \"have\"s, just like we advertise objects at the\n> tip of refs in alternate repository.\n> \n> So there will be no refs/temporary/* hierarchy we would need to\n> worry about cleaning up.\n\nI don't think details like this matter much to the bundle-generation\nside, so this is pretty academic at this point. But I think unless we\nwant to do a lot of surgery to git-clone, we'll end up more with\nsomething like:\n\n 1. init empty repository\n\n 2. contact the other side; find out they can redirect us to an\n alternate url\n\n 3. fetch the alternate url; it turns out to be a split bundle. Grab\n the header, and then spool the data into a temp packfile. When it's\n all there, we can \"index-pack --fix-thin\" it in-place.\n\nThe reason I think we'll end up with this approach is that it keeps the\ndetails of split-bundle fetching inside remote-curl. That keeps clone\ncleaner, and also means we can grab a split-bundle for a fetch, too.\n\n> Another possible variant is to redirect the client directly to\n> download pack-$sha1.pack; \"index-pack\" needs to be run on the client\n> side anyway to create pack-$sha1.idx, so at that time it could do\n> the equivalent of \"--clone-bundle\" processing (it is not strictly\n> necessary to create a split bundle) to find the tips of histories,\n> and use that information when running \"git fetch origin\".\n> \n> So, even though I started working from \"split bundle\", we may not\n> have to have such a feature after all to support CDN offloadable and\n> resumable clone.\n\nYeah. And I think we'd support this in my step (3) by responding to what\nwe get at the URL. I.e., \"it turns out to be...\" can have many outcomes,\nand one of them is \"a packfile\".\n\n-Peff\n"}]}