{"thread":{"id":"41589","subject":"[PATCH 1/2] bundle: plug resource leak","startedAt":"2016-03-01T23:35:34Z","lastAt":"2016-06-16T02:19:50Z","messageCount":37,"participants":["Junio C Hamano","Jeff King","Duy Nguyen","Christian Couder","Mike Hommey"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"280020","messageId":"xmqqvb556abd.fsf@gitster.mtv.corp.google.com","threadId":"41589","inReplyTo":null,"subject":"[PATCH 1/2] bundle: plug resource leak","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-01T23:35:34Z","receivedAt":"2016-03-01T23:35:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The bundle header structure holds two lists of refs and object\nnames, which should be released when the user is done with it.\n\nSigned-off-by: Junio C Hamano \n---\n bundle.c | 12 ++++++++++++\n bundle.h | 1 +\n transport.c | 1 +\n 3 files changed, 14 insertions(+)\n\ndiff --git a/bundle.c b/bundle.c\nindex 506ac49..9c5a6f0 100644\n--- a/bundle.c\n+++ b/bundle.c\n@@ -102,6 +102,18 @@ int is_bundle(const char *path, int quiet)\n \treturn (fd >= 0);\n }\n \n+void release_bundle_header(struct bundle_header *header)\n+{\n+\tint i;\n+\n+\tfor (i = 0; i < header->prerequisites.nr; i++)\n+\t\tfree(header->prerequisites.list[i].name);\n+\tfree(header->prerequisites.list);\n+\tfor (i = 0; i < header->references.nr; i++)\n+\t\tfree(header->references.list[i].name);\n+\tfree(header->references.list);\n+}\n+\n static int list_refs(struct ref_list *r, int argc, const char **argv)\n {\n \tint i;\ndiff --git a/bundle.h b/bundle.h\nindex 1584e4d..f7ce23b 100644\n--- a/bundle.h\n+++ b/bundle.h\n@@ -23,5 +23,6 @@ int verify_bundle(struct bundle_header *header, int verbose);\n int unbundle(struct bundle_header *header, int bundle_fd, int flags);\n int list_bundle_refs(struct bundle_header *header,\n \t\tint argc, const char **argv);\n+void release_bundle_header(struct bundle_header *);\n \n #endif\ndiff --git a/transport.c b/transport.c\nindex ca3cfa4..08e15c5 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -107,6 +107,7 @@ static int close_bundle(struct transport *transport)\n \tstruct bundle_transport_data *data = transport->data;\n \tif (data->fd > 0)\n \t\tclose(data->fd);\n+\trelease_bundle_header(&data->header);\n \tfree(data);\n \treturn 0;\n }\n-- \n2.8.0-rc0-114-g0b3e5e5\n"},{"id":"280021","messageId":"xmqqr3ft6a9x.fsf@gitster.mtv.corp.google.com","threadId":"41589","inReplyTo":"xmqqvb556abd.fsf@gitster.mtv.corp.google.com","subject":"[PATCH 2/2] bundle: keep a copy of bundle file name in the in-core bundle header","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-01T23:36:26Z","receivedAt":"2016-03-01T23:36:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"This will be necessary when we start reading from a split bundle\nwhere the header and the thin-pack data live in different files.\n\nThe in-core bundle header will read from a file that has the header,\nand will record the path to that file. We would find the name of\nthe file that hosts the thin-pack data from the header, and we would\ntake that name as relative to the file we read the header from.\n\nSigned-off-by: Junio C Hamano \n---\n builtin/bundle.c | 5 +++--\n bundle.c | 21 +++++++++++----------\n bundle.h | 3 ++-\n transport.c | 3 ++-\n 4 files changed, 18 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/bundle.c b/builtin/bundle.c\nindex 4883a43..e63388d 100644\n--- a/builtin/bundle.c\n+++ b/builtin/bundle.c\n@@ -36,8 +36,9 @@ int cmd_bundle(int argc, const char **argv, const char *prefix)\n \t}\n \n \tmemset(&header, 0, sizeof(header));\n-\tif (strcmp(cmd, \"create\") && (bundle_fd =\n-\t\t\t\tread_bundle_header(bundle_file, &header)) < 0)\n+\theader.bundle_file = bundle_file;\n+\tif (strcmp(cmd, \"create\") &&\n+\t (bundle_fd = read_bundle_header(&header)) < 0)\n \t\treturn 1;\n \n \tif (!strcmp(cmd, \"verify\")) {\ndiff --git a/bundle.c b/bundle.c\nindex 9c5a6f0..efe19e0 100644\n--- a/bundle.c\n+++ b/bundle.c\n@@ -21,8 +21,7 @@ static void add_to_ref_list(const unsigned char *sha1, const char *name,\n \tlist->nr++;\n }\n \n-static int parse_bundle_header(int fd, struct bundle_header *header,\n-\t\t\t const char *report_path)\n+static int parse_bundle_header(int fd, struct bundle_header *header, int quiet)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n \tint status = 0;\n@@ -30,9 +29,9 @@ static int parse_bundle_header(int fd, struct bundle_header *header,\n \t/* The bundle header begins with the signature */\n \tif (strbuf_getwholeline_fd(&buf, fd, '\\n') ||\n \t strcmp(buf.buf, bundle_signature)) {\n-\t\tif (report_path)\n+\t\tif (!quiet)\n \t\t\terror(_(\"'%s' does not look like a v2 bundle file\"),\n-\t\t\t report_path);\n+\t\t\t header->bundle_file);\n \t\tstatus = -1;\n \t\tgoto abort;\n \t}\n@@ -57,7 +56,7 @@ static int parse_bundle_header(int fd, struct bundle_header *header,\n \t\tif (get_sha1_hex(buf.buf, sha1) ||\n \t\t (buf.len > 40 && !isspace(buf.buf[40])) ||\n \t\t (!is_prereq && buf.len <= 40)) {\n-\t\t\tif (report_path)\n+\t\t\tif (!quiet)\n \t\t\t\terror(_(\"unrecognized header: %s%s (%d)\"),\n \t\t\t\t (is_prereq ? \"-\" : \"\"), buf.buf, (int)buf.len);\n \t\t\tstatus = -1;\n@@ -79,13 +78,13 @@ static int parse_bundle_header(int fd, struct bundle_header *header,\n \treturn fd;\n }\n \n-int read_bundle_header(const char *path, struct bundle_header *header)\n+int read_bundle_header(struct bundle_header *header)\n {\n-\tint fd = open(path, O_RDONLY);\n+\tint fd = open(header->bundle_file, O_RDONLY);\n \n \tif (fd < 0)\n-\t\treturn error(_(\"could not open '%s'\"), path);\n-\treturn parse_bundle_header(fd, header, path);\n+\t\treturn error(_(\"could not open '%s'\"), header->bundle_file);\n+\treturn parse_bundle_header(fd, header, 0);\n }\n \n int is_bundle(const char *path, int quiet)\n@@ -96,7 +95,7 @@ int is_bundle(const char *path, int quiet)\n \tif (fd < 0)\n \t\treturn 0;\n \tmemset(&header, 0, sizeof(header));\n-\tfd = parse_bundle_header(fd, &header, quiet ? NULL : path);\n+\tfd = parse_bundle_header(fd, &header, quiet);\n \tif (fd >= 0)\n \t\tclose(fd);\n \treturn (fd >= 0);\n@@ -112,6 +111,8 @@ void release_bundle_header(struct bundle_header *header)\n \tfor (i = 0; i < header->references.nr; i++)\n \t\tfree(header->references.list[i].name);\n \tfree(header->references.list);\n+\n+\tfree((void *)header->bundle_file);\n }\n \n static int list_refs(struct ref_list *r, int argc, const char **argv)\ndiff --git a/bundle.h b/bundle.h\nindex f7ce23b..cf771df 100644\n--- a/bundle.h\n+++ b/bundle.h\n@@ -10,12 +10,13 @@ struct ref_list {\n };\n \n struct bundle_header {\n+\tconst char *bundle_file;\n \tstruct ref_list prerequisites;\n \tstruct ref_list references;\n };\n \n int is_bundle(const char *path, int quiet);\n-int read_bundle_header(const char *path, struct bundle_header *header);\n+int read_bundle_header(struct bundle_header *header);\n int create_bundle(struct bundle_header *header, const char *path,\n \t\tint argc, const char **argv);\n int verify_bundle(struct bundle_header *header, int verbose);\ndiff --git a/transport.c b/transport.c\nindex 08e15c5..03ce149 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -81,7 +81,8 @@ static struct ref *get_refs_from_bundle(struct transport *transport, int for_pus\n \n \tif (data->fd > 0)\n \t\tclose(data->fd);\n-\tdata->fd = read_bundle_header(transport->url, &data->header);\n+\tdata->header.bundle_file = xstrdup(transport->url);\n+\tdata->fd = read_bundle_header(&data->header);\n \tif (data->fd < 0)\n \t\tdie (\"Could not read bundle '%s'.\", transport->url);\n \tfor (i = 0; i < data->header.references.nr; i++) {\n-- \n2.8.0-rc0-114-g0b3e5e5\n"},{"id":"280049","messageId":"20160302085432.GB30295@sigill.intra.peff.net","threadId":"41589","inReplyTo":"xmqqvb556abd.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/2] bundle: plug resource leak","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-03-02T08:54:32Z","receivedAt":"2016-03-02T08:54:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 01, 2016 at 03:35:34PM -0800, Junio C Hamano wrote:\n\n> The bundle header structure holds two lists of refs and object\n> names, which should be released when the user is done with it.\n> \n> Signed-off-by: Junio C Hamano \n> ---\n> bundle.c | 12 ++++++++++++\n> bundle.h | 1 +\n> transport.c | 1 +\n> 3 files changed, 14 insertions(+)\n> \n> diff --git a/bundle.c b/bundle.c\n> index 506ac49..9c5a6f0 100644\n> --- a/bundle.c\n> +++ b/bundle.c\n> @@ -102,6 +102,18 @@ int is_bundle(const char *path, int quiet)\n> \treturn (fd >= 0);\n> }\n> \n> +void release_bundle_header(struct bundle_header *header)\n> +{\n> +\tint i;\n> +\n> +\tfor (i = 0; i < header->prerequisites.nr; i++)\n> +\t\tfree(header->prerequisites.list[i].name);\n> +\tfree(header->prerequisites.list);\n> +\tfor (i = 0; i < header->references.nr; i++)\n> +\t\tfree(header->references.list[i].name);\n> +\tfree(header->references.list);\n> +}\n\nLooks good. It's probably not worth adding a release_ref_list() to\nhandle the repeated data structures.\n\nI do find it hard to believe that the bundle code had to invent its own\nref storage data structure, and couldn't just use \"struct ref\" like all\nof the other code. It doesn't look like we ever sort it or do\nnon-sequential access. The linked-list \"struct ref\" probably would have\nbeen fine.\n\nNot a problem you are introducing, of course, but if you are touching\nthis code a lot, it might be worth seeing how painful it is.\n\n-Peff\n"},{"id":"280050","messageId":"xmqqoaax45l5.fsf@gitster.mtv.corp.google.com","threadId":"41589","inReplyTo":"20160302085432.GB30295@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] bundle: plug resource leak","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-02T09:00:38Z","receivedAt":"2016-03-02T09:00:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I do find it hard to believe that the bundle code had to invent its own\n> ref storage data structure, and couldn't just use \"struct ref\" like all\n> of the other code. It doesn't look like we ever sort it or do\n> non-sequential access. The linked-list \"struct ref\" probably would have\n> been fine.\n>\n> Not a problem you are introducing, of course, but if you are touching\n> this code a lot, it might be worth seeing how painful it is.\n\nThe bundle code being fairly old, I actually wouldn't be surprised\nif it predated the wide use of \"struct ref\" ;-)\n\nIt is not performance critical to add entries to the list of\nprerequisites or references (i.e. it is OK to have these as linear\nlists, not linked lists of \"struct ref\"), and these lists do not\nhave to be ultra-efficient in their storage use (i.e. it is OK to\nreplace these with \"struct ref\" linked lists), so we could go either\nway. It's not like we would be using a lot of helper functions we\nalready have for \"struct ref\" in this code, so I'm inclined to give\na very low priority to the task of rethinking this data structure.\n"},{"id":"280051","messageId":"20160302090129.GC30295@sigill.intra.peff.net","threadId":"41589","inReplyTo":"xmqqr3ft6a9x.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/2] bundle: keep a copy of bundle file name in the in-core bundle header","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-03-02T09:01:29Z","receivedAt":"2016-03-02T09:01:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 01, 2016 at 03:36:26PM -0800, Junio C Hamano wrote:\n\n> This will be necessary when we start reading from a split bundle\n> where the header and the thin-pack data live in different files.\n> \n> The in-core bundle header will read from a file that has the header,\n> and will record the path to that file. We would find the name of\n> the file that hosts the thin-pack data from the header, and we would\n> take that name as relative to the file we read the header from.\n\nNeat. I'm hoping this means you're working on split bundles. :)\n\n> diff --git a/builtin/bundle.c b/builtin/bundle.c\n> index 4883a43..e63388d 100644\n> --- a/builtin/bundle.c\n> +++ b/builtin/bundle.c\n> @@ -36,8 +36,9 @@ int cmd_bundle(int argc, const char **argv, const char *prefix)\n> \t}\n> \n> \tmemset(&header, 0, sizeof(header));\n> -\tif (strcmp(cmd, \"create\") && (bundle_fd =\n> -\t\t\t\tread_bundle_header(bundle_file, &header)) < 0)\n> +\theader.bundle_file = bundle_file;\n\nWhat are the memory ownership rules for header.bundle_file?\n\nHere you assign from either an argv parameter or a stack buffer, and\nhere...\n\n> @@ -112,6 +111,8 @@ void release_bundle_header(struct bundle_header *header)\n> \tfor (i = 0; i < header->references.nr; i++)\n> \t\tfree(header->references.list[i].name);\n> \tfree(header->references.list);\n> +\n> +\tfree((void *)header->bundle_file);\n> }\n\nYou free it.\n\nThe call in get_refs_from_bundle does do an xstrdup().\n\nShould we have:\n\n void init_bundle_header(struct bundle_header *header, const char *file)\n {\n\tmemset(header, 0, sizeof(*header));\n\theader.bundle_file = xstrdup(file);\n }\n\nto abstract the whole procedure?\n\n-Peff\n"},{"id":"280052","messageId":"20160302090228.GD30295@sigill.intra.peff.net","threadId":"41589","inReplyTo":"xmqqoaax45l5.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/2] bundle: plug resource leak","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-03-02T09:02:28Z","receivedAt":"2016-03-02T09:02:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 02, 2016 at 01:00:38AM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > I do find it hard to believe that the bundle code had to invent its own\n> > ref storage data structure, and couldn't just use \"struct ref\" like all\n> > of the other code. It doesn't look like we ever sort it or do\n> > non-sequential access. The linked-list \"struct ref\" probably would have\n> > been fine.\n> >\n> > Not a problem you are introducing, of course, but if you are touching\n> > this code a lot, it might be worth seeing how painful it is.\n> \n> The bundle code being fairly old, I actually wouldn't be surprised\n> if it predated the wide use of \"struct ref\" ;-)\n> \n> It is not performance critical to add entries to the list of\n> prerequisites or references (i.e. it is OK to have these as linear\n> lists, not linked lists of \"struct ref\"), and these lists do not\n> have to be ultra-efficient in their storage use (i.e. it is OK to\n> replace these with \"struct ref\" linked lists), so we could go either\n> way. It's not like we would be using a lot of helper functions we\n> already have for \"struct ref\" in this code, so I'm inclined to give\n> a very low priority to the task of rethinking this data structure.\n\nSure, I agree it's low priority by itself. It was more something to\nconsider if you find that you are touching the bundle code a lot.\n\n-Peff\n"},{"id":"280078","messageId":"xmqqfuw84uhb.fsf@gitster.mtv.corp.google.com","threadId":"41589","inReplyTo":"20160302090129.GC30295@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] bundle: keep a copy of bundle file name in the in-core bundle header","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-02T18:15:12Z","receivedAt":"2016-03-02T18:15:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, Mar 01, 2016 at 03:36:26PM -0800, Junio C Hamano wrote:\n>\n>> This will be necessary when we start reading from a split bundle\n>> where the header and the thin-pack data live in different files.\n>> \n>> The in-core bundle header will read from a file that has the header,\n>> and will record the path to that file. We would find the name of\n>> the file that hosts the thin-pack data from the header, and we would\n>> take that name as relative to the file we read the header from.\n>\n> Neat. I'm hoping this means you're working on split bundles. :)\n\nLet's just say that during the -rc freeze period, because I can stop\nlooking at or queuing completely new topics to encourage people who\nare responsible for topics in the upcoming release to focus more on\nresponding to regressions and follow-up fixes necessary, I have a\nbetter chance of having some leftover time to look into things\nmyself, at least enough to figure out what needs to be done, ;-)\n\n>> -\tif (strcmp(cmd, \"create\") && (bundle_fd =\n>> -\t\t\t\tread_bundle_header(bundle_file, &header)) < 0)\n>> +\theader.bundle_file = bundle_file;\n>\n> What are the memory ownership rules for header.bundle_file?\n>\n> Here you assign from either an argv parameter or a stack buffer, and\n> here...\n>\n>> @@ -112,6 +111,8 @@ void release_bundle_header(struct bundle_header *header)\n>> \tfor (i = 0; i < header->references.nr; i++)\n>> \t\tfree(header->references.list[i].name);\n>> \tfree(header->references.list);\n>> +\n>> +\tfree((void *)header->bundle_file);\n>> }\n>\n> You free it.\n>\n> The call in get_refs_from_bundle does do an xstrdup().\n\nGood eyes.\n\n>\n> Should we have:\n>\n> void init_bundle_header(struct bundle_header *header, const char *file)\n> {\n> \tmemset(header, 0, sizeof(*header));\n> \theader.bundle_file = xstrdup(file);\n> }\n>\n> to abstract the whole procedure?\n\nMaybe, maybe not. I'll decide after adding the bundle_version field\nto the structure (which will be read from an existing bundle, but\nwhich will have to be set for a bundle being created).\n\nThanks.\n"},{"id":"280088","messageId":"1456950761-19759-1-git-send-email-gitster@pobox.com","threadId":"41589","inReplyTo":"xmqqfuw84uhb.fsf@gitster.mtv.corp.google.com","subject":"[PATCH v2 0/4] \"split bundle\" preview","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-02T20:32:37Z","receivedAt":"2016-03-02T20:32:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Here is a preview of the \"split bundle\" stuff that may serve as one\nof the enabling technology to offload \"git clone\" traffic off of the\nserver core network to CDN.\n\nChanges:\n\n - The \"checksum\" bit about the in-bundle packdata, which was\n incorrect, was dropped from the proposed log message of 1/4.\n\n - \"init_bundle_header()\" helper has been added to 3/4; the name of\n the new field in bundle_header structure is now \"filename\", not\n \"bundle_file\". It is silly to name a field with \"bundle\" in it\n when the structure is about a bundle already.\n\n - 4/4 is new, and implements the unbundling part, i.e. running\n either \"git clone\" or \"git bundle unbundle\" on the two files\n after a split bundle is made available locally.\n\nJunio C Hamano (4):\n bundle doc: 'verify' is not about verifying the bundle\n bundle: plug resource leak\n bundle: keep a copy of bundle file name in the in-core bundle header\n bundle v3: the beginning\n\n Documentation/git-bundle.txt | 9 ++-\n builtin/bundle.c | 6 +-\n bundle.c | 142 +++++++++++++++++++++++++++++++++++++------\n bundle.h | 8 ++-\n t/t5704-bundle.sh | 64 +++++++++++++++++++\n transport.c | 4 +-\n 6 files changed, 204 insertions(+), 29 deletions(-)\n\n-- \n2.8.0-rc0-114-g0b3e5e5\n"},{"id":"280087","messageId":"1456950761-19759-2-git-send-email-gitster@pobox.com","threadId":"41589","inReplyTo":"1456950761-19759-1-git-send-email-gitster@pobox.com","subject":"[PATCH v2 1/4] bundle doc: 'verify' is not about verifying the bundle","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-02T20:32:38Z","receivedAt":"2016-03-02T20:32:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Even though the command does read the bundle header and checks to\nsee if it looks reasonable, the thin-pack data stream that follows\nthe header in the bundle file is not checked.\n\nThe documentation gives an incorrect impression that the data\ncontained in the bundle is validated, but the command is to validate\nthat the receiving repository is ready to accept the bundle, not to\ncheck the validity of a bundle file itself.\n\nRephrase the paragraph to clarify this.\n\nSigned-off-by: Junio C Hamano \n---\n Documentation/git-bundle.txt | 9 ++++-----\n 1 file changed, 4 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/git-bundle.txt b/Documentation/git-bundle.txt\nindex 3a8120c..c0113a8 100644\n--- a/Documentation/git-bundle.txt\n+++ b/Documentation/git-bundle.txt\n@@ -38,11 +38,10 @@ create ::\n \t'git-rev-list-args' arguments to define the bundle contents.\n \n verify ::\n-\tUsed to check that a bundle file is valid and will apply\n-\tcleanly to the current repository. This includes checks on the\n-\tbundle format itself as well as checking that the prerequisite\n-\tcommits exist and are fully linked in the current repository.\n-\t'git bundle' prints a list of missing commits, if any, and exits\n+\tVerifies that the given 'file' has a valid-looking bundle\n+\theader, and that your repository has all prerequisite\n+\tobjects necessary to unpack the file as a bundle. The\n+\tcommand prints a list of missing commits, if any, and exits\n \twith a non-zero status.\n \n list-heads ::\n-- \n2.8.0-rc0-114-g0b3e5e5\n"},{"id":"280090","messageId":"1456950761-19759-3-git-send-email-gitster@pobox.com","threadId":"41589","inReplyTo":"1456950761-19759-1-git-send-email-gitster@pobox.com","subject":"[PATCH v2 2/4] bundle: plug resource leak","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-02T20:32:39Z","receivedAt":"2016-03-02T20:32:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The bundle header structure holds two lists of refs and object\nnames, which should be released when the user is done with it.\n\nSigned-off-by: Junio C Hamano \n---\n bundle.c | 12 ++++++++++++\n bundle.h | 1 +\n transport.c | 1 +\n 3 files changed, 14 insertions(+)\n\ndiff --git a/bundle.c b/bundle.c\nindex 506ac49..9c5a6f0 100644\n--- a/bundle.c\n+++ b/bundle.c\n@@ -102,6 +102,18 @@ int is_bundle(const char *path, int quiet)\n \treturn (fd >= 0);\n }\n \n+void release_bundle_header(struct bundle_header *header)\n+{\n+\tint i;\n+\n+\tfor (i = 0; i < header->prerequisites.nr; i++)\n+\t\tfree(header->prerequisites.list[i].name);\n+\tfree(header->prerequisites.list);\n+\tfor (i = 0; i < header->references.nr; i++)\n+\t\tfree(header->references.list[i].name);\n+\tfree(header->references.list);\n+}\n+\n static int list_refs(struct ref_list *r, int argc, const char **argv)\n {\n \tint i;\ndiff --git a/bundle.h b/bundle.h\nindex 1584e4d..f7ce23b 100644\n--- a/bundle.h\n+++ b/bundle.h\n@@ -23,5 +23,6 @@ int verify_bundle(struct bundle_header *header, int verbose);\n int unbundle(struct bundle_header *header, int bundle_fd, int flags);\n int list_bundle_refs(struct bundle_header *header,\n \t\tint argc, const char **argv);\n+void release_bundle_header(struct bundle_header *);\n \n #endif\ndiff --git a/transport.c b/transport.c\nindex ca3cfa4..08e15c5 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -107,6 +107,7 @@ static int close_bundle(struct transport *transport)\n \tstruct bundle_transport_data *data = transport->data;\n \tif (data->fd > 0)\n \t\tclose(data->fd);\n+\trelease_bundle_header(&data->header);\n \tfree(data);\n \treturn 0;\n }\n-- \n2.8.0-rc0-114-g0b3e5e5\n"},{"id":"280091","messageId":"1456950761-19759-4-git-send-email-gitster@pobox.com","threadId":"41589","inReplyTo":"1456950761-19759-1-git-send-email-gitster@pobox.com","subject":"[PATCH v2 3/4] bundle: keep a copy of bundle file name in the in-core bundle header","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-02T20:32:40Z","receivedAt":"2016-03-02T20:32:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"This will be necessary when we start reading from a split bundle\nwhere the header and the thin-pack data live in different files.\n\nThe in-core bundle header will read from a file that has the header,\nand will record the path to that file. We would find the name of\nthe file that hosts the thin-pack data from the header, and we would\ntake that name as relative to the file we read the header from.\n\nHelped-by: Jeff King \nSigned-off-by: Junio C Hamano \n---\n builtin/bundle.c | 6 +++---\n bundle.c | 27 +++++++++++++++++----------\n bundle.h | 4 +++-\n transport.c | 3 ++-\n 4 files changed, 25 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/bundle.c b/builtin/bundle.c\nindex 4883a43..c9ede65 100644\n--- a/builtin/bundle.c\n+++ b/builtin/bundle.c\n@@ -35,9 +35,9 @@ int cmd_bundle(int argc, const char **argv, const char *prefix)\n \t\tbundle_file = buffer;\n \t}\n \n-\tmemset(&header, 0, sizeof(header));\n-\tif (strcmp(cmd, \"create\") && (bundle_fd =\n-\t\t\t\tread_bundle_header(bundle_file, &header)) < 0)\n+\tinit_bundle_header(&header, bundle_file);\n+\tif (strcmp(cmd, \"create\") &&\n+\t (bundle_fd = read_bundle_header(&header)) < 0)\n \t\treturn 1;\n \n \tif (!strcmp(cmd, \"verify\")) {\ndiff --git a/bundle.c b/bundle.c\nindex 9c5a6f0..32bdb01 100644\n--- a/bundle.c\n+++ b/bundle.c\n@@ -21,8 +21,13 @@ static void add_to_ref_list(const unsigned char *sha1, const char *name,\n \tlist->nr++;\n }\n \n-static int parse_bundle_header(int fd, struct bundle_header *header,\n-\t\t\t const char *report_path)\n+void init_bundle_header(struct bundle_header *header, const char *name)\n+{\n+\tmemset(header, '\\0', sizeof(*header));\n+\theader->filename = xstrdup(name);\n+}\n+\n+static int parse_bundle_header(int fd, struct bundle_header *header, int quiet)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n \tint status = 0;\n@@ -30,9 +35,9 @@ static int parse_bundle_header(int fd, struct bundle_header *header,\n \t/* The bundle header begins with the signature */\n \tif (strbuf_getwholeline_fd(&buf, fd, '\\n') ||\n \t strcmp(buf.buf, bundle_signature)) {\n-\t\tif (report_path)\n+\t\tif (!quiet)\n \t\t\terror(_(\"'%s' does not look like a v2 bundle file\"),\n-\t\t\t report_path);\n+\t\t\t header->filename);\n \t\tstatus = -1;\n \t\tgoto abort;\n \t}\n@@ -57,7 +62,7 @@ static int parse_bundle_header(int fd, struct bundle_header *header,\n \t\tif (get_sha1_hex(buf.buf, sha1) ||\n \t\t (buf.len > 40 && !isspace(buf.buf[40])) ||\n \t\t (!is_prereq && buf.len <= 40)) {\n-\t\t\tif (report_path)\n+\t\t\tif (!quiet)\n \t\t\t\terror(_(\"unrecognized header: %s%s (%d)\"),\n \t\t\t\t (is_prereq ? \"-\" : \"\"), buf.buf, (int)buf.len);\n \t\t\tstatus = -1;\n@@ -79,13 +84,13 @@ static int parse_bundle_header(int fd, struct bundle_header *header,\n \treturn fd;\n }\n \n-int read_bundle_header(const char *path, struct bundle_header *header)\n+int read_bundle_header(struct bundle_header *header)\n {\n-\tint fd = open(path, O_RDONLY);\n+\tint fd = open(header->filename, O_RDONLY);\n \n \tif (fd < 0)\n-\t\treturn error(_(\"could not open '%s'\"), path);\n-\treturn parse_bundle_header(fd, header, path);\n+\t\treturn error(_(\"could not open '%s'\"), header->filename);\n+\treturn parse_bundle_header(fd, header, 0);\n }\n \n int is_bundle(const char *path, int quiet)\n@@ -96,7 +101,7 @@ int is_bundle(const char *path, int quiet)\n \tif (fd < 0)\n \t\treturn 0;\n \tmemset(&header, 0, sizeof(header));\n-\tfd = parse_bundle_header(fd, &header, quiet ? NULL : path);\n+\tfd = parse_bundle_header(fd, &header, quiet);\n \tif (fd >= 0)\n \t\tclose(fd);\n \treturn (fd >= 0);\n@@ -112,6 +117,8 @@ void release_bundle_header(struct bundle_header *header)\n \tfor (i = 0; i < header->references.nr; i++)\n \t\tfree(header->references.list[i].name);\n \tfree(header->references.list);\n+\n+\tfree((void *)header->filename);\n }\n \n static int list_refs(struct ref_list *r, int argc, const char **argv)\ndiff --git a/bundle.h b/bundle.h\nindex f7ce23b..e059ccf 100644\n--- a/bundle.h\n+++ b/bundle.h\n@@ -10,12 +10,14 @@ struct ref_list {\n };\n \n struct bundle_header {\n+\tconst char *filename;\n \tstruct ref_list prerequisites;\n \tstruct ref_list references;\n };\n \n int is_bundle(const char *path, int quiet);\n-int read_bundle_header(const char *path, struct bundle_header *header);\n+void init_bundle_header(struct bundle_header *, const char *filename);\n+int read_bundle_header(struct bundle_header *header);\n int create_bundle(struct bundle_header *header, const char *path,\n \t\tint argc, const char **argv);\n int verify_bundle(struct bundle_header *header, int verbose);\ndiff --git a/transport.c b/transport.c\nindex 08e15c5..7bd3206 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -81,7 +81,8 @@ static struct ref *get_refs_from_bundle(struct transport *transport, int for_pus\n \n \tif (data->fd > 0)\n \t\tclose(data->fd);\n-\tdata->fd = read_bundle_header(transport->url, &data->header);\n+\tinit_bundle_header(&data->header, transport->url);\n+\tdata->fd = read_bundle_header(&data->header);\n \tif (data->fd < 0)\n \t\tdie (\"Could not read bundle '%s'.\", transport->url);\n \tfor (i = 0; i < data->header.references.nr; i++) {\n-- \n2.8.0-rc0-114-g0b3e5e5\n"},{"id":"280089","messageId":"1456950761-19759-5-git-send-email-gitster@pobox.com","threadId":"41589","inReplyTo":"1456950761-19759-1-git-send-email-gitster@pobox.com","subject":"[PATCH v2 4/4] bundle v3: the beginning","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-02T20:32:41Z","receivedAt":"2016-03-02T20:32:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The bundle v3 format introduces an ability to have the bundle header\n(which describes what references in the bundled history can be\nfetched, and what objects the receiving repository must have in\norder to unbundle it successfully) in one file, and the bundled pack\nstream data in a separate file.\n\nA v3 bundle file begins with a line with \"# v3 git bundle\", followed\nby zero or more \"extended header\" lines, and an empty line, finally\nfollowed by the list of prerequisites and references in the same\nformat as v2 bundle. If it uses the \"split bundle\" feature, there\nis a \"data: $URL\" extended header line, and nothing follows the list\nof prerequisites and references. Also, \"sha1: \" extended header\nline may exist to help validating that the pack stream data matches\nthe bundle header.\n\nA typical expected use of a split bundle is to help initial clone\nthat involves a huge data transfer, and would go like this:\n\n - Any repository people would clone and fetch from would regularly\n be repacked, and it is expected that there would be a packfile\n without prerequisites that holds all (or at least most) of the\n history of it (call it pack-$name.pack).\n\n - After arranging that packfile to be downloadable over popular\n transfer methods used for serving static files (such as HTTP or\n HTTPS) that are easily resumable as $URL/pack-$name.pack, a v3\n bundle file (call it $name.bndl) can be prepared with an extended\n header \"data: $URL/pack-$name.pack\" to point at the download\n location for the packfile, and be served at \"$URL/$name.bndl\".\n\n - An updated Git client, when trying to \"git clone\" from such a\n repository, may be redirected to $URL/$name.bndl\", which would be\n a tiny text file (when split bundle feature is used).\n\n - The client would then inspect the downloaded $name.bndl, learn\n that the corresponding packfile exists at $URL/pack-$name.pack,\n and downloads it as pack-$name.pack, until the download succeeds.\n This can easily be done with \"wget --continue\" equivalent over an\n unreliable link. The checksum recorded on the \"sha1: \" header\n line is expected to be used by this downloader (not written yet).\n\n - After fully downloading $name.bndl and pack-$name.pack and\n storing them next to each other, the client would clone from the\n $name.bndl; this would populate the newly created repository with\n reasonably recent history.\n\n - Then the client can issue \"git fetch\" against the original\n repository to obtain the most recent part of the history created\n since the bundle was made.\n\nSigned-off-by: Junio C Hamano \n---\n bundle.c | 103 +++++++++++++++++++++++++++++++++++++++++++++++++-----\n bundle.h | 3 ++\n t/t5704-bundle.sh | 64 +++++++++++++++++++++++++++++++++\n 3 files changed, 161 insertions(+), 9 deletions(-)\n\ndiff --git a/bundle.c b/bundle.c\nindex 32bdb01..480630d 100644\n--- a/bundle.c\n+++ b/bundle.c\n@@ -10,7 +10,8 @@\n #include \"refs.h\"\n #include \"argv-array.h\"\n \n-static const char bundle_signature[] = \"# v2 git bundle\\n\";\n+static const char bundle_signature_v2[] = \"# v2 git bundle\\n\";\n+static const char bundle_signature_v3[] = \"# v3 git bundle\\n\";\n \n static void add_to_ref_list(const unsigned char *sha1, const char *name,\n \t\tstruct ref_list *list)\n@@ -33,16 +34,55 @@ static int parse_bundle_header(int fd, struct bundle_header *header, int quiet)\n \tint status = 0;\n \n \t/* The bundle header begins with the signature */\n-\tif (strbuf_getwholeline_fd(&buf, fd, '\\n') ||\n-\t strcmp(buf.buf, bundle_signature)) {\n+\tif (strbuf_getwholeline_fd(&buf, fd, '\\n')) {\n+\tbad_bundle:\n \t\tif (!quiet)\n-\t\t\terror(_(\"'%s' does not look like a v2 bundle file\"),\n+\t\t\terror(_(\"'%s' does not look like a supported bundle file\"),\n \t\t\t header->filename);\n \t\tstatus = -1;\n \t\tgoto abort;\n \t}\n \n-\t/* The bundle header ends with an empty line */\n+\tif (!strcmp(buf.buf, bundle_signature_v2))\n+\t\theader->bundle_version = 2;\n+\telse if (!strcmp(buf.buf, bundle_signature_v3))\n+\t\theader->bundle_version = 3;\n+\telse\n+\t\tgoto bad_bundle;\n+\n+\tif (header->bundle_version == 3) {\n+\t\t/*\n+\t\t * bundle version v3 has extended headers before the\n+\t\t * list of prerequisites and references. The extended\n+\t\t * headers end with an empty line.\n+\t\t */\n+\t\twhile (!strbuf_getwholeline_fd(&buf, fd, '\\n')) {\n+\t\t\tconst char *cp;\n+\t\t\tif (buf.len && buf.buf[buf.len - 1] == '\\n')\n+\t\t\t\tbuf.buf[--buf.len] = '\\0';\n+\t\t\tif (!buf.len)\n+\t\t\t\tbreak;\n+\t\t\tif (skip_prefix(buf.buf, \"data: \", &cp)) {\n+\t\t\t\theader->datafile = xstrdup(cp);\n+\t\t\t\tcontinue;\n+\t\t\t}\n+\t\t\tif (skip_prefix(buf.buf, \"sha1: \", &cp)) {\n+\t\t\t\tunsigned char sha1[GIT_SHA1_RAWSZ];\n+\t\t\t\tif (get_sha1_hex(cp, sha1) ||\n+\t\t\t\t cp[GIT_SHA1_HEXSZ])\n+\t\t\t\t\tgoto bad_bundle;\n+\t\t\t\thashcpy(header->csum, sha1);\n+\t\t\t\tcontinue;\n+\t\t\t}\n+\n+\t\t\tgoto bad_bundle;\n+\t\t}\n+\t}\n+\n+\t/*\n+\t * The bundle header lists prerequisites and\n+\t * references, and the list ends with an empty line.\n+\t */\n \twhile (!strbuf_getwholeline_fd(&buf, fd, '\\n') &&\n \t buf.len && buf.buf[0] != '\\n') {\n \t\tunsigned char sha1[20];\n@@ -77,7 +117,8 @@ static int parse_bundle_header(int fd, struct bundle_header *header, int quiet)\n \n abort:\n \tif (status) {\n-\t\tclose(fd);\n+\t\tif (0 <= fd)\n+\t\t\tclose(fd);\n \t\tfd = -1;\n \t}\n \tstrbuf_release(&buf);\n@@ -426,6 +467,7 @@ int create_bundle(struct bundle_header *header, const char *path,\n \tint bundle_to_stdout;\n \tint ref_count = 0;\n \tstruct rev_info revs;\n+\tconst char *bundle_signature = bundle_signature_v2;\n \n \tbundle_to_stdout = !strcmp(path, \"-\");\n \tif (bundle_to_stdout)\n@@ -480,22 +522,65 @@ int create_bundle(struct bundle_header *header, const char *path,\n \treturn 0;\n }\n \n+/*\n+ * v3 \"split bundle\" allows a separate packfile to be named\n+ * as \"data: $URL/$name_of_the_packfile\". This file is expected\n+ * to be downloaded next to the bundle header file when the\n+ * bundle is used. Hence we find the path to the directory\n+ * that contains the bundle header file, and append the basename\n+ * part of the bundle_data_file to it, to form the name of the\n+ * file that holds the pack data stream.\n+ */\n+static int open_bundle_data(struct bundle_header *header)\n+{\n+\tconst char *cp;\n+\tstruct strbuf filename = STRBUF_INIT;\n+\tint fd;\n+\n+\tassert(header->datafile);\n+\n+\tcp = find_last_dir_sep(header->filename);\n+\tif (cp)\n+\t\tstrbuf_add(&filename, header->filename,\n+\t\t\t (cp - header->filename) + 1);\n+\tcp = find_last_dir_sep(header->datafile);\n+\tif (!cp)\n+\t\tcp = header->datafile;\n+\tstrbuf_addstr(&filename, cp);\n+\n+\tfd = open(filename.buf, O_RDONLY);\n+\tstrbuf_release(&filename);\n+\treturn fd;\n+}\n+\n int unbundle(struct bundle_header *header, int bundle_fd, int flags)\n {\n \tconst char *argv_index_pack[] = {\"index-pack\",\n \t\t\t\t\t \"--fix-thin\", \"--stdin\", NULL, NULL};\n \tstruct child_process ip = CHILD_PROCESS_INIT;\n+\tint status = 0, data_fd = -1;\n \n \tif (flags & BUNDLE_VERBOSE)\n \t\targv_index_pack[3] = \"-v\";\n \n \tif (verify_bundle(header, 0))\n \t\treturn -1;\n+\n+\tif (header->datafile) {\n+\t\tdata_fd = open_bundle_data(header);\n+\t\tif (data_fd < 0)\n+\t\t\treturn error(_(\"bundle data not found\"));\n+\t\tip.in = data_fd;\n+\t} else {\n+\t\tip.in = bundle_fd;\n+\t}\n+\n \tip.argv = argv_index_pack;\n-\tip.in = bundle_fd;\n \tip.no_stdout = 1;\n \tip.git_cmd = 1;\n \tif (run_command(&ip))\n-\t\treturn error(_(\"index-pack died\"));\n-\treturn 0;\n+\t\tstatus = error(_(\"index-pack died\"));\n+\tif (0 <= data_fd)\n+\t\tclose(data_fd);\n+\treturn status;\n }\ndiff --git a/bundle.h b/bundle.h\nindex e059ccf..db55dc7 100644\n--- a/bundle.h\n+++ b/bundle.h\n@@ -10,7 +10,10 @@ struct ref_list {\n };\n \n struct bundle_header {\n+\tint bundle_version;\n \tconst char *filename;\n+\tconst char *datafile;\n+\tunsigned char csum[GIT_SHA1_RAWSZ];\n \tstruct ref_list prerequisites;\n \tstruct ref_list references;\n };\ndiff --git a/t/t5704-bundle.sh b/t/t5704-bundle.sh\nindex 348d9b3..e68523c 100755\n--- a/t/t5704-bundle.sh\n+++ b/t/t5704-bundle.sh\n@@ -71,4 +71,68 @@ test_expect_success 'prerequisites with an empty commit message' '\n \tgit bundle verify bundle\n '\n \n+# bundle v3 (experimental)\n+test_expect_success 'clone from v3' '\n+\n+\t# as \"bundle create\" does not exist yet for v3\n+\t# prepare it by hand here\n+\thead=$(git rev-parse HEAD) &&\n+\tname=$(echo $head | git pack-objects --revs v3) &&\n+\ttest_when_finished \"rm v3-$name.pack v3-$name.idx\" &&\n+\tcat >v3.bndl <<-EOF &&\n+\t# v3 git bundle\n+\tdata: v3-$name.pack\n+\n+\t$head HEAD\n+\t$head refs/heads/master\n+\tEOF\n+\n+\tgit bundle verify v3.bndl &&\n+\tgit bundle list-heads v3.bndl >actual &&\n+\tcat >expect <<-EOF &&\n+\t$head HEAD\n+\t$head refs/heads/master\n+\tEOF\n+\ttest_cmp expect actual &&\n+\n+\tgit clone v3.bndl v3dst &&\n+\tgit -C v3dst for-each-ref --format=\"%(objectname) %(refname)\" >actual &&\n+\tcat >expect <<-EOF &&\n+\t$head refs/heads/master\n+\t$head refs/remotes/origin/HEAD\n+\t$head refs/remotes/origin/master\n+\tEOF\n+\ttest_cmp expect actual &&\n+\tgit -C v3dst fsck &&\n+\n+\t# an \"inline\" v3 is still possible.\n+\tcat >v3i.bndl <<-EOF &&\n+\t# v3 git bundle\n+\n+\t$head HEAD\n+\t$head refs/heads/master\n+\n+\tEOF\n+\tcat v3-$name.pack >>v3i.bndl &&\n+\ttest_when_finished \"rm v3i.bndl\" &&\n+\n+\tgit bundle verify v3i.bndl &&\n+\tgit bundle list-heads v3i.bndl >actual &&\n+\tcat >expect <<-EOF &&\n+\t$head HEAD\n+\t$head refs/heads/master\n+\tEOF\n+\ttest_cmp expect actual &&\n+\n+\tgit clone v3i.bndl v3idst &&\n+\tgit -C v3idst for-each-ref --format=\"%(objectname) %(refname)\" >actual &&\n+\tcat >expect <<-EOF &&\n+\t$head refs/heads/master\n+\t$head refs/remotes/origin/HEAD\n+\t$head refs/remotes/origin/master\n+\tEOF\n+\ttest_cmp expect actual &&\n+\tgit -C v3idst fsck\n+'\n+\n test_done\n-- \n2.8.0-rc0-114-g0b3e5e5\n"},{"id":"280093","messageId":"20160302204947.GA6608@sigill.intra.peff.net","threadId":"41589","inReplyTo":"1456950761-19759-4-git-send-email-gitster@pobox.com","subject":"Re: [PATCH v2 3/4] bundle: keep a copy of bundle file name in the in-core bundle header","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-03-02T20:49:47Z","receivedAt":"2016-03-02T20:49:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 02, 2016 at 12:32:40PM -0800, Junio C Hamano wrote:\n\n> +\tfree((void *)header->filename);\n\nThis cast is necessary, because...\n\n> diff --git a/bundle.h b/bundle.h\n> index f7ce23b..e059ccf 100644\n> --- a/bundle.h\n> +++ b/bundle.h\n> @@ -10,12 +10,14 @@ struct ref_list {\n> };\n> \n> struct bundle_header {\n> +\tconst char *filename;\n> \tstruct ref_list prerequisites;\n> \tstruct ref_list references;\n> };\n\n...this is const, even though we know it is allocated on the heap.\n\nI am OK if we want to keep it \"conceptually const\" so that anybody\nlooking at the struct knows they should not touch it. But I am also OK\nwith just making it \"char *\".\n\n-Peff\n"},{"id":"280099","messageId":"CACsJy8AzZsC3_7mWQutko8W=X1Lch3bMZRjoHtjVnWwtbMBt7A@mail.gmail.com","threadId":"41589","inReplyTo":"1456950761-19759-5-git-send-email-gitster@pobox.com","subject":"Re: [PATCH v2 4/4] bundle v3: the beginning","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-03-03T01:36:40Z","receivedAt":"2016-03-03T01:36:40Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Mar 3, 2016 at 3:32 AM, Junio C Hamano wrote:\n> - After arranging that packfile to be downloadable over popular\n> transfer methods used for serving static files (such as HTTP or\n> HTTPS) that are easily resumable as $URL/pack-$name.pack, a v3\n> bundle file (call it $name.bndl) can be prepared with an extended\n> header \"data: $URL/pack-$name.pack\" to point at the download\n> location for the packfile, and be served at \"$URL/$name.bndl\".\n\nExtra setup to offload things to CDN is great and all. But would it be\nok if we introduced a minimal resumable download service via\ngit-daemon to enable this feature with very little setup? Like\ngit-shell, you can only download certain packfiles for this use case\nand nothing else with this service.\n-- \nDuy\n"},{"id":"280100","messageId":"xmqqwppk2rqo.fsf@gitster.mtv.corp.google.com","threadId":"41589","inReplyTo":"CACsJy8AzZsC3_7mWQutko8W=X1Lch3bMZRjoHtjVnWwtbMBt7A@mail.gmail.com","subject":"Re: [PATCH v2 4/4] bundle v3: the beginning","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-03T02:57:19Z","receivedAt":"2016-03-03T02:57:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n> would it be\n> ok if we introduced a minimal resumable download service via\n> git-daemon to enable this feature with very little setup? Like\n> git-shell, you can only download certain packfiles for this use case\n> and nothing else with this service.\n\nI think it is a matter of priorities.\n\nA minimalistic site that offers only git-daemon traffic without a\nworking HTTP server would certainly benefit from such a thing, but\nserving static files efficiently over the web is commodity service\nthese days. Wouldn't it be sufficient to just recommend having a\nnormal HTTP server serving static files, which should be \"very\nlittle setup\" in today's world?\n\nSuch a \"minimal resumable download service\" over the git-daemon\ntransport still has to reinvent what is already done well by the\nHTTP servers and clients (e.g. support of ETag equivalent to make\nsure that the client can notice that the underlying data has changed\nfor a given resource, headers to communicate the total length,\nmaking a range request and responding to it, etc. etc.).\n\nIn addition,, by going the custom protocol route, you wouldn't\nbenefit from caching HTTP proxies available to the clients.\n\nSo I am not sure if the benefit outweighs the cost.\n\nI wouldn't stop you if you really want to do it, but again, it is a\nmatter of priorities. I personally feel that it would be a waste of\nengineering talent, and it certainly would be a waste of review\nbandwidth, if you gave priority to this over other more widely\nuseful parts of the system. The procedure to repack should be\nupdated to produce such a base pack with the separate bundle header\non the server side, the protocol needs to be updated to allow\nredirection for \"clone\" traffic, the logic to decide when to\nredirect must be designed (e.g. \"single branch\" clone should not\nchoose a pack/bundle that represents the full repository, but a pack\nfor the branch that was asked), etc. There are still tons of things\nthat need to be done, and it would be a distraction to invent a\ncustom download service nobody other than git-daemon talks before\nall of the above is done.\n"},{"id":"280103","messageId":"CACsJy8BONz-q7kW4zsOyq4O+YJOmDoNF1tjp5-kDBbPnx7mC-w@mail.gmail.com","threadId":"41589","inReplyTo":"xmqqwppk2rqo.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 4/4] bundle v3: the beginning","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-03-03T05:15:07Z","receivedAt":"2016-03-03T05:15:07Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Mar 3, 2016 at 9:57 AM, Junio C Hamano wrote:\n> Duy Nguyen writes:\n>\n>> would it be\n>> ok if we introduced a minimal resumable download service via\n>> git-daemon to enable this feature with very little setup? Like\n>> git-shell, you can only download certain packfiles for this use case\n>> and nothing else with this service.\n>\n> I think it is a matter of priorities.\n>\n> A minimalistic site that offers only git-daemon traffic without a\n> working HTTP server would certainly benefit from such a thing, but\n> serving static files efficiently over the web is commodity service\n> these days. Wouldn't it be sufficient to just recommend having a\n> normal HTTP server serving static files, which should be \"very\n> little setup\" in today's world?\n>\n> Such a \"minimal resumable download service\" over the git-daemon\n> transport still has to reinvent what is already done well by the\n> HTTP servers and clients (e.g. support of ETag equivalent to make\n> sure that the client can notice that the underlying data has changed\n> for a given resource, headers to communicate the total length,\n> making a range request and responding to it, etc. etc.).\n>\n> In addition,, by going the custom protocol route, you wouldn't\n> benefit from caching HTTP proxies available to the clients.\n>\n> So I am not sure if the benefit outweighs the cost.\n\nWhat I had in mind was individuals who just want to publish their work\nover git://. Right now it's just a matter of running git-daemon and\nconfiguring it a bit. If it was me, I wouldn't expect all the bells\nand whistles that come with http. But I agree that this is low\npriority, \"scratch your own itch\" kind of thing. Let's have resumable\nclone with standard download protocols first, then we'll see.\n-- \nDuy\n"},{"id":"287081","messageId":"CAP8UFD1xqRMFE2Wzntu=XevCyj+acGLEO-cTq1fqn+NMe3x0vg@mail.gmail.com","threadId":"41589","inReplyTo":"1456950761-19759-5-git-send-email-gitster@pobox.com","subject":"Re: [PATCH v2 4/4] bundle v3: the beginning","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2016-05-20T12:39:06Z","receivedAt":"2016-05-20T12:39:06Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"I am responding to this 2+ month old email because I am investigating\nadding an alternate object store at the same level as loose and packed\nobjects. This alternate object store could be used for large files. I\nam working on this for GitLab. (Yeah, I am working, as a freelance,\nfor both Booking.com and GitLab these days.)\n\nOn Wed, Mar 2, 2016 at 9:32 PM, Junio C Hamano wrote:\n> The bundle v3 format introduces an ability to have the bundle header\n> (which describes what references in the bundled history can be\n> fetched, and what objects the receiving repository must have in\n> order to unbundle it successfully) in one file, and the bundled pack\n> stream data in a separate file.\n>\n> A v3 bundle file begins with a line with \"# v3 git bundle\", followed\n> by zero or more \"extended header\" lines, and an empty line, finally\n> followed by the list of prerequisites and references in the same\n> format as v2 bundle. If it uses the \"split bundle\" feature, there\n> is a \"data: $URL\" extended header line, and nothing follows the list\n> of prerequisites and references. Also, \"sha1: \" extended header\n> line may exist to help validating that the pack stream data matches\n> the bundle header.\n>\n> A typical expected use of a split bundle is to help initial clone\n> that involves a huge data transfer, and would go like this:\n>\n> - Any repository people would clone and fetch from would regularly\n> be repacked, and it is expected that there would be a packfile\n> without prerequisites that holds all (or at least most) of the\n> history of it (call it pack-$name.pack).\n>\n> - After arranging that packfile to be downloadable over popular\n> transfer methods used for serving static files (such as HTTP or\n> HTTPS) that are easily resumable as $URL/pack-$name.pack, a v3\n> bundle file (call it $name.bndl) can be prepared with an extended\n> header \"data: $URL/pack-$name.pack\" to point at the download\n> location for the packfile, and be served at \"$URL/$name.bndl\".\n>\n> - An updated Git client, when trying to \"git clone\" from such a\n> repository, may be redirected to $URL/$name.bndl\", which would be\n> a tiny text file (when split bundle feature is used).\n>\n> - The client would then inspect the downloaded $name.bndl, learn\n> that the corresponding packfile exists at $URL/pack-$name.pack,\n> and downloads it as pack-$name.pack, until the download succeeds.\n> This can easily be done with \"wget --continue\" equivalent over an\n> unreliable link. The checksum recorded on the \"sha1: \" header\n> line is expected to be used by this downloader (not written yet).\n\nI wonder if this mechanism could also be used or extended to clone and\nfetch an alternate object database.\n\nIn [1], [2] and [3], and this was also discussed during the\nContributor Summit last month, Peff says that he started working on\nalternate object database support a long time ago, and that the hard\npart is a protocol extension to tell remotes that you can access some\nobjects in a different way.\n\nIf a Git client would download a \"$name.bndl\" v3 bundle file that\nwould have a \"data: $URL/alt-odb-$name.odb\" extended header, the Git\nclient would just need to download \"$URL/alt-odb-$name.odb\" and use\nthe alternate object database support on this file.\n\nThis way it would know all it has to know to access the objects in the\nalternate database. The alternate object database may not contain the\nreal objects, if they are too big for example, but just files that\ndescribe how to get the real objects.\n\n> - After fully downloading $name.bndl and pack-$name.pack and\n> storing them next to each other, the client would clone from the\n> $name.bndl; this would populate the newly created repository with\n> reasonably recent history.\n>\n> - Then the client can issue \"git fetch\" against the original\n> repository to obtain the most recent part of the history created\n> since the bundle was made.\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/206886/focus=207040\n[2] http://thread.gmane.org/gmane.comp.version-control.git/247171\n[3] http://thread.gmane.org/gmane.comp.version-control.git/202902/focus=203020\n"},{"id":"287895","messageId":"CACsJy8Dr_Z886Jb-O8gbAv_vzBLicNH6bPPpKwb9HWZTKQ9muw@mail.gmail.com","threadId":"41589","inReplyTo":"CAP8UFD1xqRMFE2Wzntu=XevCyj+acGLEO-cTq1fqn+NMe3x0vg@mail.gmail.com","subject":"Re: [PATCH v2 4/4] bundle v3: the beginning","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-05-31T12:43:27Z","receivedAt":"2016-05-31T12:43:27Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, May 20, 2016 at 7:39 PM, Christian Couder\n wrote:\n> I am responding to this 2+ month old email because I am investigating\n> adding an alternate object store at the same level as loose and packed\n> objects. This alternate object store could be used for large files. I\n> am working on this for GitLab. (Yeah, I am working, as a freelance,\n> for both Booking.com and GitLab these days.)\n\nI'm also interested in this from a different angle, narrow clone that\npotentially allows to skip download some large blobs (likely old ones\nfrom the past that nobody will bother).\n\n> On Wed, Mar 2, 2016 at 9:32 PM, Junio C Hamano wrote:\n>> The bundle v3 format introduces an ability to have the bundle header\n>> (which describes what references in the bundled history can be\n>> fetched, and what objects the receiving repository must have in\n>> order to unbundle it successfully) in one file, and the bundled pack\n>> stream data in a separate file.\n>>\n>> A v3 bundle file begins with a line with \"# v3 git bundle\", followed\n>> by zero or more \"extended header\" lines, and an empty line, finally\n>> followed by the list of prerequisites and references in the same\n>> format as v2 bundle. If it uses the \"split bundle\" feature, there\n>> is a \"data: $URL\" extended header line, and nothing follows the list\n>> of prerequisites and references. Also, \"sha1: \" extended header\n>> line may exist to help validating that the pack stream data matches\n>> the bundle header.\n>>\n>> A typical expected use of a split bundle is to help initial clone\n>> that involves a huge data transfer, and would go like this:\n>>\n>> - Any repository people would clone and fetch from would regularly\n>> be repacked, and it is expected that there would be a packfile\n>> without prerequisites that holds all (or at least most) of the\n>> history of it (call it pack-$name.pack).\n>>\n>> - After arranging that packfile to be downloadable over popular\n>> transfer methods used for serving static files (such as HTTP or\n>> HTTPS) that are easily resumable as $URL/pack-$name.pack, a v3\n>> bundle file (call it $name.bndl) can be prepared with an extended\n>> header \"data: $URL/pack-$name.pack\" to point at the download\n>> location for the packfile, and be served at \"$URL/$name.bndl\".\n>>\n>> - An updated Git client, when trying to \"git clone\" from such a\n>> repository, may be redirected to $URL/$name.bndl\", which would be\n>> a tiny text file (when split bundle feature is used).\n>>\n>> - The client would then inspect the downloaded $name.bndl, learn\n>> that the corresponding packfile exists at $URL/pack-$name.pack,\n>> and downloads it as pack-$name.pack, until the download succeeds.\n>> This can easily be done with \"wget --continue\" equivalent over an\n>> unreliable link. The checksum recorded on the \"sha1: \" header\n>> line is expected to be used by this downloader (not written yet).\n>\n> I wonder if this mechanism could also be used or extended to clone and\n> fetch an alternate object database.\n>\n> In [1], [2] and [3], and this was also discussed during the\n> Contributor Summit last month, Peff says that he started working on\n> alternate object database support a long time ago, and that the hard\n> part is a protocol extension to tell remotes that you can access some\n> objects in a different way.\n>\n> If a Git client would download a \"$name.bndl\" v3 bundle file that\n> would have a \"data: $URL/alt-odb-$name.odb\" extended header, the Git\n> client would just need to download \"$URL/alt-odb-$name.odb\" and use\n> the alternate object database support on this file.\n\nWhat does this file contain exactly? A list of SHA-1 that can be\nretrieved from this remote/alternate odb? I wonder if we could just\ngit-replace for this marking. The replaced content could contain the\nuri pointing to the alt odb. We could optionally contact alt odb to\nretrieve real content, or just show the replaced/fake data when alt\nodb is out of reach. Transferring git-replace is basically ref\nexchange, which may be fine if you don't have a lot of objects in this\nalt odb. If you do, well, we need to deal with lots of refs anyway.\nThis may benefit from it too.\n\n> [3] http://thread.gmane.org/gmane.comp.version-control.git/202902/focus=203020\n\nThis points to https://github.com/peff/git/commits/jk/external-odb\nwhich is dead. Jeff, do you still have it somewhere, or is it not\nworth looking at anymore?\n-- \nDuy\n"},{"id":"287896","messageId":"CAP8UFD3jPQFk2deSk5JXC3PTz5yWcvXJ4=Qjam5Qw6P9SrLzFQ@mail.gmail.com","threadId":"41589","inReplyTo":"CACsJy8Dr_Z886Jb-O8gbAv_vzBLicNH6bPPpKwb9HWZTKQ9muw@mail.gmail.com","subject":"Re: [PATCH v2 4/4] bundle v3: the beginning","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2016-05-31T13:18:42Z","receivedAt":"2016-05-31T13:18:42Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Tue, May 31, 2016 at 2:43 PM, Duy Nguyen wrote:\n> On Fri, May 20, 2016 at 7:39 PM, Christian Couder\n> wrote:\n>> I am responding to this 2+ month old email because I am investigating\n>> adding an alternate object store at the same level as loose and packed\n>> objects. This alternate object store could be used for large files. I\n>> am working on this for GitLab. (Yeah, I am working, as a freelance,\n>> for both Booking.com and GitLab these days.)\n>\n> I'm also interested in this from a different angle, narrow clone that\n> potentially allows to skip download some large blobs (likely old ones\n> from the past that nobody will bother).\n\nInteresting!\n\n[...]\n\n>> I wonder if this mechanism could also be used or extended to clone and\n>> fetch an alternate object database.\n>>\n>> In [1], [2] and [3], and this was also discussed during the\n>> Contributor Summit last month, Peff says that he started working on\n>> alternate object database support a long time ago, and that the hard\n>> part is a protocol extension to tell remotes that you can access some\n>> objects in a different way.\n>>\n>> If a Git client would download a \"$name.bndl\" v3 bundle file that\n>> would have a \"data: $URL/alt-odb-$name.odb\" extended header, the Git\n>> client would just need to download \"$URL/alt-odb-$name.odb\" and use\n>> the alternate object database support on this file.\n>\n> What does this file contain exactly? A list of SHA-1 that can be\n> retrieved from this remote/alternate odb?\n\nIt would depend on the external odb. Git could support different\nexternal odb that have different trade-offs.\n\n> I wonder if we could just\n> git-replace for this marking. The replaced content could contain the\n> uri pointing to the alt odb.\n\nYeah, interesting!\nThat's indeed another possibility that might not need the transfer of\nany external odb.\n\nBut in this case it might be cleaner to just have a separate ref hierarchy like:\n\nrefs/external-odbs/my-ext-odb/\n\ninstead of using the replace one.\n\nOr maybe:\n\nrefs/replace/external-odbs/my-ext-odb/\n\nif we really want to use the replace hierarchy.\n\n> We could optionally contact alt odb to\n> retrieve real content, or just show the replaced/fake data when alt\n> odb is out of reach.\n\nYeah, I wonder if that really needs the replace mechanism.\n\n> Transferring git-replace is basically ref\n> exchange, which may be fine if you don't have a lot of objects in this\n> alt odb.\n\nYeah sure, great idea!\n\nBy the way this makes me wonder if we could implement resumable clone\nusing some kind of replace ref.\n\nThe client while cloning nearly as usual would download one or more\nspecial replace refs that would points to objects with links to\ndownload bundles using standard protocols.\nJust after the clone, the client would read these objects and download\nthe bundles from these objects.\nAnd then it would clone from these bundles.\n\n> If you do, well, we need to deal with lots of refs anyway.\n> This may benefit from it too.\n>\n>> [3] http://thread.gmane.org/gmane.comp.version-control.git/202902/focus=203020\n>\n> This points to https://github.com/peff/git/commits/jk/external-odb\n> which is dead. Jeff, do you still have it somewhere, or is it not\n> worth looking at anymore?\n\nI have rebased, fixed and improved it a bit. I added write support for\nblobs. But the result is not very clean right now.\nI was going to send a RFC patch series after cleaning the result, but\nas you ask, here are some links to some branches:\n\n- https://github.com/chriscool/git/commits/gl-external-odb3 (the\nupdated patches from Peff, plus 2 small patches from me)\n- https://github.com/chriscool/git/commits/gl-external-odb7 (the same\nas above, plus a number of WIP patches to add blob write support)\n\nThanks,\nChristian.\n"},{"id":"287944","messageId":"20160531222322.GC3824@sigill.intra.peff.net","threadId":"41589","inReplyTo":"CACsJy8Dr_Z886Jb-O8gbAv_vzBLicNH6bPPpKwb9HWZTKQ9muw@mail.gmail.com","subject":"Re: [PATCH v2 4/4] bundle v3: the beginning","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-05-31T22:23:22Z","receivedAt":"2016-05-31T22:23:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 31, 2016 at 07:43:27PM +0700, Duy Nguyen wrote:\n\n> > [3] http://thread.gmane.org/gmane.comp.version-control.git/202902/focus=203020\n> \n> This points to https://github.com/peff/git/commits/jk/external-odb\n> which is dead. Jeff, do you still have it somewhere, or is it not\n> worth looking at anymore?\n\nIt's now \"jk/external-odb-wip\" at the same repo. I wouldn't be surprised\nif it doesn't even compile, though. I basically rebase my topics daily\nagainst Junio's \"master\", so it may be carried forward, but things\nmarked \"-wip\" aren't part of my daily git build, and generally don't\neven get compile-tested (usually if the rebase looks too hairy or awful,\nI'll drop it completely, though, and I haven't done that here).\n\nYou're probably better off looking whatever Christian produces. :)\n\n-Peff\n"},{"id":"287945","messageId":"20160531223118.GD3824@sigill.intra.peff.net","threadId":"41589","inReplyTo":"CAP8UFD1xqRMFE2Wzntu=XevCyj+acGLEO-cTq1fqn+NMe3x0vg@mail.gmail.com","subject":"Re: [PATCH v2 4/4] bundle v3: the beginning","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-05-31T22:31:18Z","receivedAt":"2016-05-31T22:31:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 20, 2016 at 02:39:06PM +0200, Christian Couder wrote:\n\n> I wonder if this mechanism could also be used or extended to clone and\n> fetch an alternate object database.\n> \n> In [1], [2] and [3], and this was also discussed during the\n> Contributor Summit last month, Peff says that he started working on\n> alternate object database support a long time ago, and that the hard\n> part is a protocol extension to tell remotes that you can access some\n> objects in a different way.\n> \n> If a Git client would download a \"$name.bndl\" v3 bundle file that\n> would have a \"data: $URL/alt-odb-$name.odb\" extended header, the Git\n> client would just need to download \"$URL/alt-odb-$name.odb\" and use\n> the alternate object database support on this file.\n> \n> This way it would know all it has to know to access the objects in the\n> alternate database. The alternate object database may not contain the\n> real objects, if they are too big for example, but just files that\n> describe how to get the real objects.\n\nI'm not sure about this strategy. I see two complications:\n\n 1. I don't think bundles need to be a part of this \"external odb\"\n strategy at all. If I understand correctly, I think you want to use\n it as a place to stuff metadata that the server tells the client,\n like \"by the way, go here if you want another way to access some\n objects\".\n\n But there are lots of cases where the server might want to tell\n the client that don't involve bundles at all.\n\n 2. A server pointing the client to another object store is actually\n the least interesting bit of the protocol.\n\n The more interesting cases (to me) are:\n\n a. The receiving side of a connection (e.g., a fetch client)\n somehow has out-of-band access to some objects. How does it\n\t tell the other side \"do not bother sending me these objects; I\n\t can get them in another way\"?\n\n b. The receiving side of a connection has out-of-band access to\n some objects. Some of these will be expensive to get (e.g.,\n\t requiring a large download), and some may be fast (e.g.,\n\t they've already been fetched to a local cache). How do we tell\n\t the sending side not to assume we have cheap access to these\n\t objects (e.g., for use as a delta base)?\n\nSo I don't think you want to tie this into bundles due to (1), and I\nthink that bundles would be insufficient anyway because of (2).\n\nOr maybe I'm misunderstanding what you propose.\n\n-Peff\n"},{"id":"288006","messageId":"CACsJy8AxPKuDUZzWdaWg8tfcWBhqJBADc=ia-Y8cM+zyJ1NFTQ@mail.gmail.com","threadId":"41589","inReplyTo":"CAP8UFD3jPQFk2deSk5JXC3PTz5yWcvXJ4=Qjam5Qw6P9SrLzFQ@mail.gmail.com","subject":"Re: [PATCH v2 4/4] bundle v3: the beginning","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-06-01T13:37:32Z","receivedAt":"2016-06-01T13:37:32Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, May 31, 2016 at 8:18 PM, Christian Couder\n wrote:\n>>> I wonder if this mechanism could also be used or extended to clone and\n>>> fetch an alternate object database.\n>>>\n>>> In [1], [2] and [3], and this was also discussed during the\n>>> Contributor Summit last month, Peff says that he started working on\n>>> alternate object database support a long time ago, and that the hard\n>>> part is a protocol extension to tell remotes that you can access some\n>>> objects in a different way.\n>>>\n>>> If a Git client would download a \"$name.bndl\" v3 bundle file that\n>>> would have a \"data: $URL/alt-odb-$name.odb\" extended header, the Git\n>>> client would just need to download \"$URL/alt-odb-$name.odb\" and use\n>>> the alternate object database support on this file.\n>>\n>> What does this file contain exactly? A list of SHA-1 that can be\n>> retrieved from this remote/alternate odb?\n>\n> It would depend on the external odb. Git could support different\n> external odb that have different trade-offs.\n>\n>> I wonder if we could just\n>> git-replace for this marking. The replaced content could contain the\n>> uri pointing to the alt odb.\n>\n> Yeah, interesting!\n> That's indeed another possibility that might not need the transfer of\n> any external odb.\n>\n> But in this case it might be cleaner to just have a separate ref hierarchy like:\n>\n> refs/external-odbs/my-ext-odb/\n>\n> instead of using the replace one.\n>\n> Or maybe:\n>\n> refs/replace/external-odbs/my-ext-odb/\n>\n> if we really want to use the replace hierarchy.\n\nYep. replace hierarchy crossed my mind. But then I thought about\nperformance degradation when there are more than one pack (we have to\nsearch through them all for every SHA-1) and discarded it because we\nwould need to do the same linear search here. I guess we will most\nlikely have one or two name spaces so it probably won't matter.\n\n>> We could optionally contact alt odb to\n>> retrieve real content, or just show the replaced/fake data when alt\n>> odb is out of reach.\n>\n> Yeah, I wonder if that really needs the replace mechanism.\n\nReplace mechanism provides good hook point. But it really depends how\ninvasive this remote odb is. If a fake content is enough to avoid\nbreakages up high, git-replace is enough. If you really need to pass\nremote odb info up so higher levels can do something more fancy, then\nit's insufficient.\n\n> By the way this makes me wonder if we could implement resumable clone\n> using some kind of replace ref.\n>\n> The client while cloning nearly as usual would download one or more\n> special replace refs that would points to objects with links to\n> download bundles using standard protocols.\n> Just after the clone, the client would read these objects and download\n> the bundles from these objects.\n> And then it would clone from these bundles.\n\nI thought we have settled on resumable clone, just waiting for an\nimplementation :) Doing it your way, you would need to download these\nspecial objects too (in a pack?) and come back download some more\nbundles. It would be more efficient to show the bundle uri early and\ngo download the bundle on the side while you go on to get the\naddition/smaller pack that contains the rest.\n-- \nDuy\n"},{"id":"288007","messageId":"CACsJy8DB_17DZ7REBzicyA_GZCnvNkoYEzftjfyM72QVmEb_Vg@mail.gmail.com","threadId":"41589","inReplyTo":"CAP8UFD3jPQFk2deSk5JXC3PTz5yWcvXJ4=Qjam5Qw6P9SrLzFQ@mail.gmail.com","subject":"Re: [PATCH v2 4/4] bundle v3: the beginning","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-06-01T14:00:25Z","receivedAt":"2016-06-01T14:00:25Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, May 31, 2016 at 8:18 PM, Christian Couder\n wrote:\n>>> [3] http://thread.gmane.org/gmane.comp.version-control.git/202902/focus=203020\n>>\n>> This points to https://github.com/peff/git/commits/jk/external-odb\n>> which is dead. Jeff, do you still have it somewhere, or is it not\n>> worth looking at anymore?\n>\n> I have rebased, fixed and improved it a bit. I added write support for\n> blobs. But the result is not very clean right now.\n> I was going to send a RFC patch series after cleaning the result, but\n> as you ask, here are some links to some branches:\n>\n> - https://github.com/chriscool/git/commits/gl-external-odb3 (the\n> updated patches from Peff, plus 2 small patches from me)\n> - https://github.com/chriscool/git/commits/gl-external-odb7 (the same\n> as above, plus a number of WIP patches to add blob write support)\n\nThanks. I had a super quick look. It would be nice if you could give a\nhigh level overview on this (if you're going to spend a lot more time on it).\n\nOne random thought, maybe it's better to have a daemon for external\nodb right from the start (one for all odbs, or one per odb, I don't\nknow). It could do fancy stuff like object caching if necessary, and\nit can avoid high cost handshake (e.g. via tls) every time a git\nprocess runs and gets one object. Reducing process spawn would\ndefinitely receive a big cheer from Windows crowd.\n\nAny thought on object streaming support? It could be a big deal (might\naffect some design decisions). I would also think about how pack v4\nfits in this (e.g. how a tree walker can still walk fast, a big\npromise of pack v4; I suppose if you still maintain \"pack\" concept\nover external odb then it might work). Not that it really matters.\nPack v4 is the future, but the future can never be \"today\" :)\n-- \nDuy\n"},{"id":"288538","messageId":"CAP8UFD2t=2wJ=1U1ctMYNuMSejBYLh2yeLU7ZfP5Q6KLxUApjQ@mail.gmail.com","threadId":"41589","inReplyTo":"CACsJy8DB_17DZ7REBzicyA_GZCnvNkoYEzftjfyM72QVmEb_Vg@mail.gmail.com","subject":"Re: [PATCH v2 4/4] bundle v3: the beginning","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2016-06-07T08:46:07Z","receivedAt":"2016-06-07T08:46:07Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Wed, Jun 1, 2016 at 4:00 PM, Duy Nguyen wrote:\n> On Tue, May 31, 2016 at 8:18 PM, Christian Couder\n> wrote:\n>>>> [3] http://thread.gmane.org/gmane.comp.version-control.git/202902/focus=203020\n>>>\n>>> This points to https://github.com/peff/git/commits/jk/external-odb\n>>> which is dead. Jeff, do you still have it somewhere, or is it not\n>>> worth looking at anymore?\n>>\n>> I have rebased, fixed and improved it a bit. I added write support for\n>> blobs. But the result is not very clean right now.\n>> I was going to send a RFC patch series after cleaning the result, but\n>> as you ask, here are some links to some branches:\n>>\n>> - https://github.com/chriscool/git/commits/gl-external-odb3 (the\n>> updated patches from Peff, plus 2 small patches from me)\n>> - https://github.com/chriscool/git/commits/gl-external-odb7 (the same\n>> as above, plus a number of WIP patches to add blob write support)\n>\n> Thanks. I had a super quick look. It would be nice if you could give a\n> high level overview on this (if you're going to spend a lot more time on it).\n\nSorry about the late answer.\n\nHere is a new series after some cleanup:\n\nhttps://github.com/chriscool/git/commits/gl-external-odb12\n\nThe high level overview of the patch series I would like to send\nreally soon now could go like this:\n\n---\nGit can store its objects only in the form of loose objects in\nseparate files or packed objects in a pack file.\nTo be able to better handle some kind of objects, for example big\nblobs, it would be nice if Git could store its objects in other object\ndatabases (ODB).\n\nTo do that, this patch series makes it possible to register commands,\nusing \"odb..command\" config variables, to access external\nODBs. Each specified command will then be called the following ways:\n\n - \" have\": the command should output the sha1, size and\ntype of all the objects the external ODB contains, one object per\nline.\n - \" get \": the command should then read from the\nexternal ODB the content of the object corresponding to and\noutput it on stdout.\n - \" put \": the command should then read\nfrom stdin an object and store it in the external ODB.\n\nThis RFC patch series does not address the following important parts\nof a complete solution:\n\n - There is no way to transfer external ODB content using Git.\n - No real external ODB has been interfaced with Git. The tests use\nanother git repo in a separate directory for this purpose which is\nprobably useless in the real world.\n---\n\n> One random thought, maybe it's better to have a daemon for external\n> odb right from the start (one for all odbs, or one per odb, I don't\n> know). It could do fancy stuff like object caching if necessary, and\n> it can avoid high cost handshake (e.g. via tls) every time a git\n> process runs and gets one object. Reducing process spawn would\n> definitely receive a big cheer from Windows crowd.\n\nThe caching could be done inside Git and I am not sure it's worth\noptimizing this now.\nIt could also make it more difficult to write support for an external\nODB if we required a daemon.\nMaybe later we can add support for \"odb..daemon\" if we think\nthat this is worth it.\n\n> Any thought on object streaming support?\n\nNo I didn't think about this. In fact I am not sure what this means.\n\n> It could be a big deal (might\n> affect some design decisions).\n\nCould you elaborate on this?\n\n> I would also think about how pack v4\n> fits in this (e.g. how a tree walker can still walk fast, a big\n> promise of pack v4; I suppose if you still maintain \"pack\" concept\n> over external odb then it might work). Not that it really matters.\n> Pack v4 is the future, but the future can never be \"today\" :)\n\nSorry I haven't really followed pack v4 and I forgot what it is about.\n\nThanks,\nChristian.\n"},{"id":"288539","messageId":"20160607085340.GA21631@glandium.org","threadId":"41589","inReplyTo":"CAP8UFD2t=2wJ=1U1ctMYNuMSejBYLh2yeLU7ZfP5Q6KLxUApjQ@mail.gmail.com","subject":"Re: [PATCH v2 4/4] bundle v3: the beginning","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-06-07T08:53:40Z","receivedAt":"2016-06-07T08:53:40Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"On Tue, Jun 07, 2016 at 10:46:07AM +0200, Christian Couder wrote:\n> The high level overview of the patch series I would like to send\n> really soon now could go like this:\n> \n> ---\n> Git can store its objects only in the form of loose objects in\n> separate files or packed objects in a pack file.\n> To be able to better handle some kind of objects, for example big\n> blobs, it would be nice if Git could store its objects in other object\n> databases (ODB).\n> \n> To do that, this patch series makes it possible to register commands,\n> using \"odb..command\" config variables, to access external\n> ODBs. Each specified command will then be called the following ways:\n> \n> - \" have\": the command should output the sha1, size and\n> type of all the objects the external ODB contains, one object per\n> line.\n> - \" get \": the command should then read from the\n> external ODB the content of the object corresponding to and\n> output it on stdout.\n> - \" put \": the command should then read\n> from stdin an object and store it in the external ODB.\n\n(disclaimer: I didn't look at the patch series)\n\nDoes this mean you're going to fork/exec() a new for each of\nthese? It would probably be better if it was \"batched\", where the\nexecutable is invoked once and the commands are passed to its stdin.\n\nMike\n"},{"id":"288544","messageId":"CACsJy8DSvS6DxBM-RUknOEe5DquF-qcRr-MK5FUZcRXYDpSgXg@mail.gmail.com","threadId":"41589","inReplyTo":"CAP8UFD2t=2wJ=1U1ctMYNuMSejBYLh2yeLU7ZfP5Q6KLxUApjQ@mail.gmail.com","subject":"Re: [PATCH v2 4/4] bundle v3: the beginning","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-06-07T10:22:22Z","receivedAt":"2016-06-07T10:22:22Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Jun 7, 2016 at 3:46 PM, Christian Couder\n wrote:\n>> Any thought on object streaming support?\n>\n> No I didn't think about this. In fact I am not sure what this means.\n>\n>> It could be a big deal (might\n>> affect some design decisions).\n>\n> Could you elaborate on this?\n\nObject streaming api is in streaming.h. Normally objects are small and\nwe can inflate the whole thing in memory before doing anything with\nthem. For really large objects (which I guess is one of the reasons\nfor remote odb) we don't want to do that. It takes lots of memory and\nyou could have objects larger than your physical memory. In some cases\nwhen can ignore those objects (e.g. mark them binary and choose not to\ndiff). In some other cases (e.g. checkout), we use streaming interface\nto process an object while we're inflating it to keep memory usage\ndown. It's easy to add a new streaming backend, once you settle on how\nremote odb streams stuff.\n\n>> I would also think about how pack v4\n>> fits in this (e.g. how a tree walker can still walk fast, a big\n>> promise of pack v4; I suppose if you still maintain \"pack\" concept\n>> over external odb then it might work). Not that it really matters.\n>> Pack v4 is the future, but the future can never be \"today\" :)\n>\n> Sorry I haven't really followed pack v4 and I forgot what it is about.\n\nIt's a new pack format (and practically vaporware at this point) that\npromises much faster access when you need to walk through trees and\ncommits (think rev-list --objects --all, or git-blame). Because we are\n(or I am) still not sure if pack v4 will ever get to the state where\nit can be merged to git.git, I think it's ok for you to ignore it too\nif you want. You can read more about the format here [1] and go even\nfurther back to [2] when Nicolas teased us with the pack size\n(smaller, which is a nice side effect). The potential issue with pack\nv4 is, the tree walker (struct tree_desc and related funcs in\nwalk-tree.h) needs to know about pack v4 in order to walk fast.\nCurrent tree walker does not care if an object is packed (using what\nformat) at all. Remote odb for pack v4 must have some way that allows\nto read pack data directly, something close to \"mmap\", it's not just\nabout an api to \"get me the canonical content of this object\".\n\n[1] http://article.gmane.org/gmane.comp.version-control.git/234012\n[2] http://article.gmane.org/gmane.comp.version-control.git/233038\n-- \nDuy\n"},{"id":"288599","messageId":"CAP8UFD3mGKTONzh1fxCJAJBrmc=OcCHeRqBQi-xTzHvcnAJ_sw@mail.gmail.com","threadId":"41589","inReplyTo":"20160531223118.GD3824@sigill.intra.peff.net","subject":"Re: [PATCH v2 4/4] bundle v3: the beginning","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2016-06-07T13:19:46Z","receivedAt":"2016-06-07T13:19:46Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Wed, Jun 1, 2016 at 12:31 AM, Jeff King wrote:\n> On Fri, May 20, 2016 at 02:39:06PM +0200, Christian Couder wrote:\n>\n>> I wonder if this mechanism could also be used or extended to clone and\n>> fetch an alternate object database.\n>>\n>> In [1], [2] and [3], and this was also discussed during the\n>> Contributor Summit last month, Peff says that he started working on\n>> alternate object database support a long time ago, and that the hard\n>> part is a protocol extension to tell remotes that you can access some\n>> objects in a different way.\n>>\n>> If a Git client would download a \"$name.bndl\" v3 bundle file that\n>> would have a \"data: $URL/alt-odb-$name.odb\" extended header, the Git\n>> client would just need to download \"$URL/alt-odb-$name.odb\" and use\n>> the alternate object database support on this file.\n>>\n>> This way it would know all it has to know to access the objects in the\n>> alternate database. The alternate object database may not contain the\n>> real objects, if they are too big for example, but just files that\n>> describe how to get the real objects.\n>\n> I'm not sure about this strategy.\n\nI am also not sure that this is the best strategy, but I think it's\nworth discussing.\n\n> I see two complications:\n>\n> 1. I don't think bundles need to be a part of this \"external odb\"\n> strategy at all. If I understand correctly, I think you want to use\n> it as a place to stuff metadata that the server tells the client,\n> like \"by the way, go here if you want another way to access some\n> objects\".\n\nYeah, basically I think it might be possible to use the bundle\nmechanism to transfer what an external ODB on the client would need to\nbe initialized or updated.\n\n> But there are lots of cases where the server might want to tell\n> the client that don't involve bundles at all.\n\nThe idea is also that anytime the server needs to send external ODB\ndata to the client, it would ask its own external ODB to prepare a\nkind of bundle with that data and use the bundle v3 mechanism to send\nit.\nThat may need the bundle v3 mechanism to be extended, but I don't see\nin which cases it would not work.\n\n> 2. A server pointing the client to another object store is actually\n> the least interesting bit of the protocol.\n>\n> The more interesting cases (to me) are:\n>\n> a. The receiving side of a connection (e.g., a fetch client)\n> somehow has out-of-band access to some objects. How does it\n> tell the other side \"do not bother sending me these objects; I\n> can get them in another way\"?\n\nI don't see a difference with regular objects that the fetch client\nalready has. If it already has some regular objects, a way to tell the\nserver \"don't bother sending me these objects\" is useful already and\nit should be possible to use it to tell the server that there is no\nneed to send some objects stored in the external ODB too.\n\nAlso something like this is needed for shallow clones and narrow clones anyway.\n\n> b. The receiving side of a connection has out-of-band access to\n> some objects. Some of these will be expensive to get (e.g.,\n> requiring a large download), and some may be fast (e.g.,\n> they've already been fetched to a local cache). How do we tell\n> the sending side not to assume we have cheap access to these\n> objects (e.g., for use as a delta base)?\n\nI don't think we need to tell the sending side we have cheap access or\nnot to some objects.\nIf the objects are managed by the external ODB, it's the external ODB\non the server and on the client that will manage these objects. They\nshould not be used as delta bases.\nPerhaps there is no mechanism to say that some objects (basically all\nexternal ODB managed objects) should not be used as delta bases, but\nthat could be added.\n\nThanks,\nChristian.\n"},{"id":"288608","messageId":"CAP8UFD0+0EYUNy6sQWjHAqfTU8yx9-GmOVV+JtqzWkbO1S1G2g@mail.gmail.com","threadId":"41589","inReplyTo":"CACsJy8AxPKuDUZzWdaWg8tfcWBhqJBADc=ia-Y8cM+zyJ1NFTQ@mail.gmail.com","subject":"Re: [PATCH v2 4/4] bundle v3: the beginning","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2016-06-07T14:49:05Z","receivedAt":"2016-06-07T14:49:05Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Wed, Jun 1, 2016 at 3:37 PM, Duy Nguyen wrote:\n> On Tue, May 31, 2016 at 8:18 PM, Christian Couder\n> wrote:\n>>>> I wonder if this mechanism could also be used or extended to clone and\n>>>> fetch an alternate object database.\n>>>>\n>>>> In [1], [2] and [3], and this was also discussed during the\n>>>> Contributor Summit last month, Peff says that he started working on\n>>>> alternate object database support a long time ago, and that the hard\n>>>> part is a protocol extension to tell remotes that you can access some\n>>>> objects in a different way.\n>>>>\n>>>> If a Git client would download a \"$name.bndl\" v3 bundle file that\n>>>> would have a \"data: $URL/alt-odb-$name.odb\" extended header, the Git\n>>>> client would just need to download \"$URL/alt-odb-$name.odb\" and use\n>>>> the alternate object database support on this file.\n>>>\n>>> What does this file contain exactly? A list of SHA-1 that can be\n>>> retrieved from this remote/alternate odb?\n>>\n>> It would depend on the external odb. Git could support different\n>> external odb that have different trade-offs.\n>>\n>>> I wonder if we could just\n>>> git-replace for this marking. The replaced content could contain the\n>>> uri pointing to the alt odb.\n>>\n>> Yeah, interesting!\n>> That's indeed another possibility that might not need the transfer of\n>> any external odb.\n>>\n>> But in this case it might be cleaner to just have a separate ref hierarchy like:\n>>\n>> refs/external-odbs/my-ext-odb/\n>>\n>> instead of using the replace one.\n>>\n>> Or maybe:\n>>\n>> refs/replace/external-odbs/my-ext-odb/\n>>\n>> if we really want to use the replace hierarchy.\n>\n> Yep. replace hierarchy crossed my mind. But then I thought about\n> performance degradation when there are more than one pack (we have to\n> search through them all for every SHA-1) and discarded it because we\n> would need to do the same linear search here. I guess we will most\n> likely have one or two name spaces so it probably won't matter.\n\nYeah.\n\n>>> We could optionally contact alt odb to\n>>> retrieve real content, or just show the replaced/fake data when alt\n>>> odb is out of reach.\n>>\n>> Yeah, I wonder if that really needs the replace mechanism.\n>\n> Replace mechanism provides good hook point. But it really depends how\n> invasive this remote odb is. If a fake content is enough to avoid\n> breakages up high, git-replace is enough. If you really need to pass\n> remote odb info up so higher levels can do something more fancy, then\n> it's insufficient.\n>\n>> By the way this makes me wonder if we could implement resumable clone\n>> using some kind of replace ref.\n>>\n>> The client while cloning nearly as usual would download one or more\n>> special replace refs that would points to objects with links to\n>> download bundles using standard protocols.\n>> Just after the clone, the client would read these objects and download\n>> the bundles from these objects.\n>> And then it would clone from these bundles.\n>\n> I thought we have settled on resumable clone, just waiting for an\n> implementation :) Doing it your way, you would need to download these\n> special objects too (in a pack?) and come back download some more\n> bundles. It would be more efficient to show the bundle uri early and\n> go download the bundle on the side while you go on to get the\n> addition/smaller pack that contains the rest.\n\nYeah, something like the bundle v3 mechanism is probably more efficient.\n\nThanks,\nChristian.\n"},{"id":"288632","messageId":"xmqqinxkpzur.fsf@gitster.mtv.corp.google.com","threadId":"41589","inReplyTo":"CAP8UFD2t=2wJ=1U1ctMYNuMSejBYLh2yeLU7ZfP5Q6KLxUApjQ@mail.gmail.com","subject":"Re: [PATCH v2 4/4] bundle v3: the beginning","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-07T19:23:40Z","receivedAt":"2016-06-07T19:23:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> Git can store its objects only in the form of loose objects in\n> separate files or packed objects in a pack file.\n> To be able to better handle some kind of objects, for example big\n> blobs, it would be nice if Git could store its objects in other object\n> databases (ODB).\n>\n> To do that, this patch series makes it possible to register commands,\n> using \"odb..command\" config variables, to access external\n> ODBs. Each specified command will then be called the following ways:\n\nHopefully it is done via a cheap RPC instead of forking/execing the\ncommand for each and every object lookup.\n\n> - \" have\": the command should output the sha1, size and\n> type of all the objects the external ODB contains, one object per\n> line.\n\nWhy size and type at this point is needed by the clients? That is\nmore expensive to compute than just a bare list of object names.\n\n> - \" get \": the command should then read from the\n> external ODB the content of the object corresponding to and\n> output it on stdout.\n\nThe type and size should be given at this point.\n\n> - \" put \": the command should then read\n> from stdin an object and store it in the external ODB.\n\nIs ODB required to sanity check that matches what the data\nhashes down to?\n\nIf this thing is primarily to offload large blobs, you might also\nwant not \"get\" but \"checkout \" to bypass Git entirely,\nbut I haven't thought it through.\n"},{"id":"288636","messageId":"20160607202351.GA5726@sigill.intra.peff.net","threadId":"41589","inReplyTo":"xmqqinxkpzur.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 4/4] bundle v3: the beginning","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-07T20:23:51Z","receivedAt":"2016-06-07T20:23:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 07, 2016 at 12:23:40PM -0700, Junio C Hamano wrote:\n\n> Christian Couder writes:\n> \n> > Git can store its objects only in the form of loose objects in\n> > separate files or packed objects in a pack file.\n> > To be able to better handle some kind of objects, for example big\n> > blobs, it would be nice if Git could store its objects in other object\n> > databases (ODB).\n> >\n> > To do that, this patch series makes it possible to register commands,\n> > using \"odb..command\" config variables, to access external\n> > ODBs. Each specified command will then be called the following ways:\n> \n> Hopefully it is done via a cheap RPC instead of forking/execing the\n> command for each and every object lookup.\n\nThis interface comes from my earlier patches, so I'll try to shed a\nlittle light on the decisions I made there.\n\nBecause this \"external odb\" essentially acts as a git alternate, we\nwould hit it only when we couldn't find an object through regular means.\nGit would then make the object available in the usual on-disk format\n(probably as a loose object).\n\nSo in most processes, we would not need to consult the odb command at\nall. And when we do, the first thing would be to get its \"have\" list,\nwhich would at most run once per process.\n\nSo the per-object cost is really calling \"get\", and my assumption there\nwas that the cost of actually retrieving the object over the network\nwould dwarf the fork/exec cost.\n\nI also waffled on having git cache the output of \" have\" in\nsome fast-lookup format to save even the single fork/exec. But I figured\nthat was something that could be added later if needed.\n\nYou'll note that this is sort of a \"fault-in\" model. Another model would\nbe to treat external odb updates similar to fetches. I.e., we touch the\nnetwork only during a special update operation, and then try to work\nlocally with whatever the external odb has. IMHO this policy could\nactually be up to the external odb itself (i.e., its \"have\" command\ncould serve from a local cache if it likes).\n\n> > - \" have\": the command should output the sha1, size and\n> > type of all the objects the external ODB contains, one object per\n> > line.\n> \n> Why size and type at this point is needed by the clients? That is\n> more expensive to compute than just a bare list of object names.\n\nYes, but it lets get avoid doing a lot of \"get\" operations. For example,\nin a regular diff without binary-diffs enabled, we can automatically\ndetermine that a diff will be considered binary based purely on the size\nof the objects (related to core.bigfilethreshold). So if we know the\nsizes, we can run \"git log -p\" without faulting-in each of the objects\njust to say \"woah, that looks binary\".\n\nOne can accomplish this with .gitattributes, too, of course, but the\nsize thing just works out of the box.\n\nThere are other places where it will come in handy, too. E.g., fscking a\ntree object you have, you want to make sure that the object referred to\nwith mode 100644 is actually a blob.\n\nI also don't think the cost to compute size and type on the server is\nall that important. Yes, if you're backing your external odb with a git\nrepository that runs \"git cat-file\" on the fly, it is more expensive.\nBut in practice, I'd expect the server side to create a static manifest\nand serve it over HTTP (this also gives the benefit of things like\nETags).\n\n> > - \" get \": the command should then read from the\n> > external ODB the content of the object corresponding to and\n> > output it on stdout.\n> \n> The type and size should be given at this point.\n\nI don't think there's a reason not to; I didn't here because it would be\nredundant with what Git already knows from the \"have\" manifest it\nreceives above.\n\n> > - \" put \": the command should then read\n> > from stdin an object and store it in the external ODB.\n> \n> Is ODB required to sanity check that matches what the data\n> hashes down to?\n\nI think that would be up to the ODB, but it does seem like a good idea.\n\nLikewise, I'm not sure if \"get\" should be allowed to return contents\nthat don't match the sha1. That would be fine for things like \"diff\",\nbut would probably make \"fsck\" unhappy.\n\n> If this thing is primarily to offload large blobs, you might also\n> want not \"get\" but \"checkout \" to bypass Git entirely,\n> but I haven't thought it through.\n\nMy mental model is that the external odb gets the object into the local\nodb, and then you can use the regular streaming-checkout code path. And\nthe local odb serves as your cache.\n\nThat does mean you might have two copies of each object (one in the odb,\nand one in the working tree), as opposed to a true cacheless system,\nwhich can get away with one.\n\nI think you could do that cacheless thing with the interface here,\nthough; the \"get\" operation can stream, and you can stream directly to\nthe working tree.\n\n-Peff\n"},{"id":"288637","messageId":"20160607203557.GB5726@sigill.intra.peff.net","threadId":"41589","inReplyTo":"CAP8UFD3mGKTONzh1fxCJAJBrmc=OcCHeRqBQi-xTzHvcnAJ_sw@mail.gmail.com","subject":"Re: [PATCH v2 4/4] bundle v3: the beginning","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-07T20:35:57Z","receivedAt":"2016-06-07T20:35:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 07, 2016 at 03:19:46PM +0200, Christian Couder wrote:\n\n> > But there are lots of cases where the server might want to tell\n> > the client that don't involve bundles at all.\n> \n> The idea is also that anytime the server needs to send external ODB\n> data to the client, it would ask its own external ODB to prepare a\n> kind of bundle with that data and use the bundle v3 mechanism to send\n> it.\n> That may need the bundle v3 mechanism to be extended, but I don't see\n> in which cases it would not work.\n\nAh, I see we do not have the same underlying mental model.\n\nI think the external odb is purely the _client's_ business. The server\ndoes not have to have an external odb at all, and does not need to know\nabout the client's. The client is responsible for telling the server\nduring the git protocol anything it would need to know (like \"do not\nbother sending objects over 50MB; I can get them elsewhere\").\n\nThis makes the problem much more complicated, but it is more flexible\nand decentralized.\n\n> > a. The receiving side of a connection (e.g., a fetch client)\n> > somehow has out-of-band access to some objects. How does it\n> > tell the other side \"do not bother sending me these objects; I\n> > can get them in another way\"?\n> \n> I don't see a difference with regular objects that the fetch client\n> already has. If it already has some regular objects, a way to tell the\n> server \"don't bother sending me these objects\" is useful already and\n> it should be possible to use it to tell the server that there is no\n> need to send some objects stored in the external ODB too.\n\nThe way to do that with normal objects is by finding shared commit tips,\nand assuming the normal git repository property of \"if you have X, you\nhave all of the objects reachable from X\".\n\nThis whole idea is essentially creating \"holes\" in that property. You\ncan enumerate all of the holes, but I am not sure that scales well. We\nget a lot of efficiency by communicating only ref tips during the\nnegotiation, and not individual object names.\n\n> Also something like this is needed for shallow clones and narrow\n> clones anyway.\n\nYes, and I don't think it scales well there, either. A single shallow\ncutoff works OK. But if you repeatedly shallow-fetch into a repository,\nyou end up with a patchwork of disconnected \"islands\" of history. The\nCPU required on the server side to serve those fetch requests is much\ngreater than what would normally be needed. You can't use things like\nreachability bitmaps, and you have to open up the trees for each island\nto see which objects the other side actually has.\n\n> > b. The receiving side of a connection has out-of-band access to\n> > some objects. Some of these will be expensive to get (e.g.,\n> > requiring a large download), and some may be fast (e.g.,\n> > they've already been fetched to a local cache). How do we tell\n> > the sending side not to assume we have cheap access to these\n> > objects (e.g., for use as a delta base)?\n> \n> I don't think we need to tell the sending side we have cheap access or\n> not to some objects.\n> If the objects are managed by the external ODB, it's the external ODB\n> on the server and on the client that will manage these objects. They\n> should not be used as delta bases.\n> Perhaps there is no mechanism to say that some objects (basically all\n> external ODB managed objects) should not be used as delta bases, but\n> that could be added.\n\nYes, I agree that _if_ the server can access the list of objects\navailable in the external odb, this becomes much easier. I'm just not\nconvinced that level of coupling is a good idea.\n\nNote that the server would also want to take this into account during\nrepacking, as otherwise you end up with fetches that are very expensive\nto serve (you want to send X which is a delta based on Y, but you know\nthat Y is available via the external odb, and therefore should not be\nused as a base. So you have to throw out the delta for X and either send\nit whole or compute a new one. That's much more expensive than blitting\nthe delta from disk, which is what a normal clone would do).\n\n-Peff\n"},{"id":"288703","messageId":"CACsJy8CtsFFJPssDDBuL8TLoxi1f=734mjAjCUOr8Y63aD3xUA@mail.gmail.com","threadId":"41589","inReplyTo":"20160607202351.GA5726@sigill.intra.peff.net","subject":"Re: [PATCH v2 4/4] bundle v3: the beginning","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-06-08T10:44:06Z","receivedAt":"2016-06-08T10:44:06Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Jun 8, 2016 at 3:23 AM, Jeff King wrote:\n> Because this \"external odb\" essentially acts as a git alternate, we\n> would hit it only when we couldn't find an object through regular means.\n> Git would then make the object available in the usual on-disk format\n> (probably as a loose object).\n\nThis means git-gc (and all things that do rev-list --objects --all)\nwould download at least all trees and commits? Or will we have special\ntreatment for those commands?\n-- \nDuy\n"},{"id":"288729","messageId":"20160608161958.GA30876@sigill.intra.peff.net","threadId":"41589","inReplyTo":"CACsJy8CtsFFJPssDDBuL8TLoxi1f=734mjAjCUOr8Y63aD3xUA@mail.gmail.com","subject":"Re: [PATCH v2 4/4] bundle v3: the beginning","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-08T16:19:58Z","receivedAt":"2016-06-08T16:19:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 08, 2016 at 05:44:06PM +0700, Duy Nguyen wrote:\n\n> On Wed, Jun 8, 2016 at 3:23 AM, Jeff King wrote:\n> > Because this \"external odb\" essentially acts as a git alternate, we\n> > would hit it only when we couldn't find an object through regular means.\n> > Git would then make the object available in the usual on-disk format\n> > (probably as a loose object).\n> \n> This means git-gc (and all things that do rev-list --objects --all)\n> would download at least all trees and commits? Or will we have special\n> treatment for those commands?\n\nYes. To me, this was always about punting large blobs from the clones.\nBasically the way git-lfs and other tools work, but without munging your\nhistory permanently.\n\nI don't know if Christian had other cases in mind (like the many-files\ncase, which I think is better served by something like narrow clones).\n\n-Peff\n"},{"id":"288741","messageId":"xmqq4m93mu8v.fsf@gitster.mtv.corp.google.com","threadId":"41589","inReplyTo":"20160607202351.GA5726@sigill.intra.peff.net","subject":"Re: [PATCH v2 4/4] bundle v3: the beginning","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-08T18:05:20Z","receivedAt":"2016-06-08T18:05:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> This interface comes from my earlier patches, so I'll try to shed a\n> little light on the decisions I made there.\n>\n> Because this \"external odb\" essentially acts as a git alternate, we\n> would hit it only when we couldn't find an object through regular means.\n> Git would then make the object available in the usual on-disk format\n> (probably as a loose object).\n>\n> So in most processes, we would not need to consult the odb command at\n> all. And when we do, the first thing would be to get its \"have\" list,\n> which would at most run once per process.\n>\n> So the per-object cost is really calling \"get\", and my assumption there\n> was that the cost of actually retrieving the object over the network\n> would dwarf the fork/exec cost.\n\nOK, presented that way, the design makes sense (I do not know if\nChristian's (revised) design and implementation does or not, though,\nas I haven't seen it).\n\nAs \"check for non-existence\" is important and costly, grabbing\n\"have\" once is a good strategy, just like we open the .idx files of\navailable packfiles.\n\n>> > - \" have\": the command should output the sha1, size and\n>> > type of all the objects the external ODB contains, one object per\n>> > line.\n>> \n>> Why size and type at this point is needed by the clients? That is\n>> more expensive to compute than just a bare list of object names.\n>\n> Yes, but it lets get avoid doing a lot of \"get\" operations.\n\nOK, so it is more like having richer information in pack-v4 index ;-)\n\n>> > - \" put \": the command should then read\n>> > from stdin an object and store it in the external ODB.\n>> \n>> Is ODB required to sanity check that matches what the data\n>> hashes down to?\n>\n> I think that would be up to the ODB, but it does seem like a good idea.\n>\n> Likewise, I'm not sure if \"get\" should be allowed to return contents\n> that don't match the sha1.\n\nYes, this is what I was getting at. It would be ideal to come up\nwith a way to do the large-blob offload without resorting to hacks\n(like LFS and annex where \"the same object contents will always\nresult in the same object name\" is deliberately broken), and \"object\nname must match what the data hashes down to\" is a basic requirement\nfor that.\n\nThanks.\n"},{"id":"288754","messageId":"20160608190043.GA19572@sigill.intra.peff.net","threadId":"41589","inReplyTo":"xmqq4m93mu8v.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 4/4] bundle v3: the beginning","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-08T19:00:43Z","receivedAt":"2016-06-08T19:00:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 08, 2016 at 11:05:20AM -0700, Junio C Hamano wrote:\n\n> > Likewise, I'm not sure if \"get\" should be allowed to return contents\n> > that don't match the sha1.\n> \n> Yes, this is what I was getting at. It would be ideal to come up\n> with a way to do the large-blob offload without resorting to hacks\n> (like LFS and annex where \"the same object contents will always\n> result in the same object name\" is deliberately broken), and \"object\n> name must match what the data hashes down to\" is a basic requirement\n> for that.\n\nI meant to elaborate here more, but it looks like I didn't.\n\nOne thing that an external odb command might want to be able to do is\nsay \"I _do_ have that object, but it would be expensive or impossible to\nget right now, so I will give you a placeholder\" (e.g., you are just\ntrying to run \"git log\" while on an airplane, and you would not want to\ndie() because you cannot fetch some blob).\n\nBut the right way is not to have \"get\" send content that does not match\nthe requested sha1. It needs to make git aware that the object is a\nplaceholder, so git does not do stupid things like write the bogus\ncontent into a loose object.\n\nThe right way may be as simple as the external odb returning a non-zero\nexit code, and git fills in the placeholder data itself (or dies,\npossibly, depending on what the user asks it to do).\n\nOne of the reasons I worked up that initial external-odb patch was\nbecause I knew that before we settled on a definite interface, we would\nhave to try it out and see what odd corner cases came up. E.g., when do\nwe fault in objects in a way that's annoying to the user? Which code\npaths need to handle \"we do have this object available, but you can't\nsee it right now, so what kind of fallback can we do?\". Etc.\n\nUnfortunately I never actually did any of that testing. ;)\n\n-Peff\n"},{"id":"288797","messageId":"CACsJy8BGV-AWwap9c2hOjXWMYxBLUirX8i20rS_=Vmci_SG_rw@mail.gmail.com","threadId":"41589","inReplyTo":"20160608161958.GA30876@sigill.intra.peff.net","subject":"Re: [PATCH v2 4/4] bundle v3: the beginning","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-06-09T08:53:26Z","receivedAt":"2016-06-16T02:19:49Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Jun 8, 2016 at 11:19 PM, Jeff King wrote:\n> On Wed, Jun 08, 2016 at 05:44:06PM +0700, Duy Nguyen wrote:\n>\n>> On Wed, Jun 8, 2016 at 3:23 AM, Jeff King wrote:\n>> > Because this \"external odb\" essentially acts as a git alternate, we\n>> > would hit it only when we couldn't find an object through regular means.\n>> > Git would then make the object available in the usual on-disk format\n>> > (probably as a loose object).\n>>\n>> This means git-gc (and all things that do rev-list --objects --all)\n>> would download at least all trees and commits? Or will we have special\n>> treatment for those commands?\n>\n> Yes. To me, this was always about punting large blobs from the clones.\n> Basically the way git-lfs and other tools work, but without munging your\n> history permanently.\n\nMakes sense. If we keep all trees and commits locally, pack v4 still\nhas a chance to rise!\n\n> I don't know if Christian had other cases in mind (like the many-files\n> case, which I think is better served by something like narrow clones).\n\nAlthough for git-gc or git-fsck, I guess we need special support\nanyway not to download large blobs unnecessarily. Not sure if git-gc\ncan already do that now. All I remember is git-repack can still be\nused to make a repo independent from odb alternates. We probably want\nto avoid that. git-fsck definitely should verify that large remote\nblobs are good without downloading them (a new \"fsck\" command to\nexternal odb, maybe).\n-- \nDuy\n"},{"id":"288829","messageId":"20160609172352.GC9016@sigill.intra.peff.net","threadId":"41589","inReplyTo":"CACsJy8BGV-AWwap9c2hOjXWMYxBLUirX8i20rS_=Vmci_SG_rw@mail.gmail.com","subject":"Re: [PATCH v2 4/4] bundle v3: the beginning","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-09T17:23:52Z","receivedAt":"2016-06-16T02:19:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 09, 2016 at 03:53:26PM +0700, Duy Nguyen wrote:\n\n> > Yes. To me, this was always about punting large blobs from the clones.\n> > Basically the way git-lfs and other tools work, but without munging your\n> > history permanently.\n> \n> Makes sense. If we keep all trees and commits locally, pack v4 still\n> has a chance to rise!\n\nYeah, I don't think anything here precludes pack v4.\n\n> > I don't know if Christian had other cases in mind (like the many-files\n> > case, which I think is better served by something like narrow clones).\n> \n> Although for git-gc or git-fsck, I guess we need special support\n> anyway not to download large blobs unnecessarily. Not sure if git-gc\n> can already do that now. All I remember is git-repack can still be\n> used to make a repo independent from odb alternates. We probably want\n> to avoid that. git-fsck definitely should verify that large remote\n> blobs are good without downloading them (a new \"fsck\" command to\n> external odb, maybe).\n\nI think git-gc should work out of the box; you'd want to use \"repack\n-l\", which git-gc passes already.\n\nFsck would be OK as long as you didn't actually load blobs. We have\n--connectivity-only for that, but of course it isn't the default. You'd\nprobably want the default mode to fsck local blobs, but _not_ to fault\nin external blobs (but have an option to fault them all in if you really\nwanted to be sure you have everything).\n\n-Peff\n"}]}