{"thread":{"id":"28668","subject":"[RFC/PATCH 1/2] bundle: allowing to read from an unseekable fd","startedAt":"2011-10-13T22:32:43Z","lastAt":"2011-10-14T18:14:55Z","messageCount":5,"participants":["Junio C Hamano","Phil Hord"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"177596","messageId":"7vpqi034l0.fsf@alter.siamese.dyndns.org","threadId":"28668","inReplyTo":null,"subject":"[RFC/PATCH 1/2] bundle: allowing to read from an unseekable fd","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-10-13T22:32:43Z","receivedAt":"2011-10-13T22:32:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The current code opens a given file with fopen(), reads it until the end\nof the header and runs ftell(), and reopens the same file with open() and\nseeks to skip the header. This structure makes it hard to retarget the\ncode to read from input that is not seekable, such as a network socket.\n\nThis patch by itself does not reach that goal yet, but I think it is a\nright step in that direction.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\n * It would be nice if we can avoid byte-by-byte reading from the file\n   descriptor by over-reading into the strbuf and pass the remainder to\n   the caller of read_bundle_header(), but I suspect that it would require\n   us to carry the \"here is the remainder from the previous read\" buffer\n   around throughout the transport layer. Parsing of the header wouldn't\n   be performance critical compared to the computation cost of actually\n   reading the rest of the bundle, hopefully, so...\n\n bundle.c |   99 ++++++++++++++++++++++++++++++++++++++++----------------------\n 1 files changed, 64 insertions(+), 35 deletions(-)\n\ndiff --git a/bundle.c b/bundle.c\nindex f48fd7d..3aa715c 100644\n--- a/bundle.c\n+++ b/bundle.c\n@@ -23,49 +23,78 @@ static void add_to_ref_list(const unsigned char *sha1, const char *name,\n \tlist->nr++;\n }\n \n-/* returns an fd */\n+/* Eventually this should go to strbuf.[ch] */\n+static int strbuf_readline_fd(struct strbuf *sb, int fd)\n+{\n+\tstrbuf_reset(sb);\n+\n+\twhile (1) {\n+\t\tchar ch;\n+\t\tssize_t len = xread(fd, &ch, 1);\n+\t\tif (len < 0)\n+\t\t\treturn -1;\n+\t\tstrbuf_addch(sb, ch);\n+\t\tif (ch == '\\n')\n+\t\t\tbreak;\n+\t}\n+\treturn 0;\n+}\n+\n int read_bundle_header(const char *path, struct bundle_header *header)\n {\n-\tchar buffer[1024];\n-\tint fd;\n-\tlong fpos;\n-\tFILE *ffd = fopen(path, \"rb\");\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tint fd = open(path, O_RDONLY);\n+\tint status = 0;\n \n-\tif (!ffd)\n+\tif (fd < 0)\n \t\treturn error(\"could not open '%s'\", path);\n-\tif (!fgets(buffer, sizeof(buffer), ffd) ||\n-\t\t\tstrcmp(buffer, bundle_signature)) {\n-\t\tfclose(ffd);\n-\t\treturn error(\"'%s' does not look like a v2 bundle file\", path);\n+\n+\t/* The bundle header begins with the signature */\n+\tif (strbuf_readline_fd(&buf, fd) ||\n+\t    strcmp(buf.buf, bundle_signature)) {\n+\t\terror(\"'%s' does not look like a v2 bundle file\", path);\n+\t\tstatus = -1;\n+\t\tgoto abort;\n \t}\n-\twhile (fgets(buffer, sizeof(buffer), ffd)\n-\t\t\t&& buffer[0] != '\\n') {\n-\t\tint is_prereq = buffer[0] == '-';\n-\t\tint offset = is_prereq ? 1 : 0;\n-\t\tint len = strlen(buffer);\n+\n+\t/* The bundle header ends with an empty line */\n+\twhile (!strbuf_readline_fd(&buf, fd) &&\n+\t       buf.len && buf.buf[0] != '\\n') {\n \t\tunsigned char sha1[20];\n-\t\tstruct ref_list *list = is_prereq ? &header->prerequisites\n-\t\t\t: &header->references;\n-\t\tchar delim;\n-\n-\t\tif (len && buffer[len - 1] == '\\n')\n-\t\t\tbuffer[len - 1] = '\\0';\n-\t\tif (get_sha1_hex(buffer + offset, sha1)) {\n-\t\t\twarning(\"unrecognized header: %s\", buffer);\n-\t\t\tcontinue;\n+\t\tint is_prereq = 0;\n+\n+\t\tif (*buf.buf == '-') {\n+\t\t\tis_prereq = 1;\n+\t\t\tstrbuf_remove(&buf, 0, 1);\n+\t\t}\n+\t\tstrbuf_rtrim(&buf);\n+\n+\t\t/*\n+\t\t * Tip lines have object name, SP, and refname.\n+\t\t * Prerequisites have object name that is optionally\n+\t\t * followed by SP and subject line.\n+\t\t */\n+\t\tif (get_sha1_hex(buf.buf, sha1) ||\n+\t\t    (40 <= buf.len && !isspace(buf.buf[40])) ||\n+\t\t    (!is_prereq && buf.len <= 40)) {\n+\t\t\terror(\"unrecognized header: %s%s (%d)\",\n+\t\t\t      (is_prereq ? \"-\" : \"\"), buf.buf, (int)buf.len);\n+\t\t\tstatus = -1;\n+\t\t\tbreak;\n+\t\t} else {\n+\t\t\tif (is_prereq)\n+\t\t\t\tadd_to_ref_list(sha1, \"\", &header->prerequisites);\n+\t\t\telse\n+\t\t\t\tadd_to_ref_list(sha1, buf.buf + 41, &header->references);\n \t\t}\n-\t\tdelim = buffer[40 + offset];\n-\t\tif (!isspace(delim) && (delim != '\\0' || !is_prereq))\n-\t\t\tdie (\"invalid header: %s\", buffer);\n-\t\tadd_to_ref_list(sha1, isspace(delim) ?\n-\t\t\t\tbuffer + 41 + offset : \"\", list);\n \t}\n-\tfpos = ftell(ffd);\n-\tfclose(ffd);\n-\tfd = open(path, O_RDONLY);\n-\tif (fd < 0)\n-\t\treturn error(\"could not open '%s'\", path);\n-\tlseek(fd, fpos, SEEK_SET);\n+\n+ abort:\n+\tif (status) {\n+\t\tclose(fd);\n+\t\tfd = -1;\n+\t}\n+\tstrbuf_release(&buf);\n \treturn fd;\n }\n \n-- \n1.7.7.289.gd0d4bb\n"},{"id":"177597","messageId":"7vliso34gc.fsf@alter.siamese.dyndns.org","threadId":"28668","inReplyTo":"7vpqi034l0.fsf@alter.siamese.dyndns.org","subject":"[PATCH 2/2] bundle: add parse_bundle_header() helper function","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-10-13T22:35:31Z","receivedAt":"2011-10-13T22:35:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Move most of the code from read_bundle_header() to parse_bundle_header()\nthat takes a file descriptor that is already opened for reading, and make\nthe former responsible only for opening the file and noticing errors.\n\nAs a logical consequence of this, is_bundle() helper function can be\nimplemented as a non-complaining variant of read_bundle_header() that\ndoes not return an open file descriptor, and can be used to tighten\nthe check used to decide the use of bundle transport in transport.c\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\n * It generally is a bad practice to base a non-RFC patch on an RFC one,\n   but in any case here is how I would do the is_bundle() helper.\n\n bundle.c    |   39 +++++++++++++++++++++++++++++++--------\n bundle.h    |    1 +\n transport.c |    2 +-\n 3 files changed, 33 insertions(+), 9 deletions(-)\n\ndiff --git a/bundle.c b/bundle.c\nindex 3aa715c..105b005 100644\n--- a/bundle.c\n+++ b/bundle.c\n@@ -40,19 +40,18 @@ static int strbuf_readline_fd(struct strbuf *sb, int fd)\n \treturn 0;\n }\n \n-int read_bundle_header(const char *path, struct bundle_header *header)\n+static int parse_bundle_header(int fd, struct bundle_header *header,\n+\t\t\t       const char *report_path)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n-\tint fd = open(path, O_RDONLY);\n \tint status = 0;\n \n-\tif (fd < 0)\n-\t\treturn error(\"could not open '%s'\", path);\n-\n \t/* The bundle header begins with the signature */\n \tif (strbuf_readline_fd(&buf, fd) ||\n \t    strcmp(buf.buf, bundle_signature)) {\n-\t\terror(\"'%s' does not look like a v2 bundle file\", path);\n+\t\tif (report_path)\n+\t\t\terror(\"'%s' does not look like a v2 bundle file\",\n+\t\t\t      report_path);\n \t\tstatus = -1;\n \t\tgoto abort;\n \t}\n@@ -77,8 +76,9 @@ int read_bundle_header(const char *path, struct bundle_header *header)\n \t\tif (get_sha1_hex(buf.buf, sha1) ||\n \t\t    (40 <= buf.len && !isspace(buf.buf[40])) ||\n \t\t    (!is_prereq && buf.len <= 40)) {\n-\t\t\terror(\"unrecognized header: %s%s (%d)\",\n-\t\t\t      (is_prereq ? \"-\" : \"\"), buf.buf, (int)buf.len);\n+\t\t\tif (report_path)\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 \t\t\tbreak;\n \t\t} else {\n@@ -98,6 +98,29 @@ int read_bundle_header(const char *path, struct bundle_header *header)\n \treturn fd;\n }\n \n+int read_bundle_header(const char *path, struct bundle_header *header)\n+{\n+\tint fd = open(path, 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+}\n+\n+int is_bundle(const char *path, int quiet)\n+{\n+\tstruct bundle_header header;\n+\tint fd = open(path, O_RDONLY);\n+\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+\tif (fd >= 0)\n+\t\tclose(fd);\n+\treturn (fd >= 0);\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 e2aedd6..c6228e2 100644\n--- a/bundle.h\n+++ b/bundle.h\n@@ -14,6 +14,7 @@ struct bundle_header {\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 create_bundle(struct bundle_header *header, const char *path,\n \t\tint argc, const char **argv);\ndiff --git a/transport.c b/transport.c\nindex c9c8056..84d6480 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -913,7 +913,7 @@ struct transport *transport_get(struct remote *remote, const char *url)\n \t\tret->fetch = fetch_objs_via_rsync;\n \t\tret->push = rsync_transport_push;\n \t\tret->smart_options = NULL;\n-\t} else if (is_local(url) && is_file(url)) {\n+\t} else if (is_local(url) && is_file(url) && is_bundle(url, 1)) {\n \t\tstruct bundle_transport_data *data = xcalloc(1, sizeof(*data));\n \t\tret->data = data;\n \t\tret->get_refs_list = get_refs_from_bundle;\n-- \n1.7.7.289.gd0d4bb\n"},{"id":"177638","messageId":"CABURp0qnhNQ06pNHw_6_PkT2+WNPdQ1WzUHF8p6q3Hx3JkoC8A@mail.gmail.com","threadId":"28668","inReplyTo":"7vpqi034l0.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC/PATCH 1/2] bundle: allowing to read from an unseekable fd","fromName":"Phil Hord","fromEmail":"phil.hord@gmail.com","sentAt":"2011-10-14T12:31:25Z","receivedAt":"2011-10-14T12:31:25Z","isPatch":true,"sender":{"key":"phil.hord@gmail.com","avatar":"https://avatars.githubusercontent.com/u/123908?v=4"},"body":"On Thu, Oct 13, 2011 at 6:32 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> The current code opens a given file with fopen(), reads it until the end\n> of the header and runs ftell(), and reopens the same file with open() and\n> seeks to skip the header. This structure makes it hard to retarget the\n> code to read from input that is not seekable, such as a network socket.\n>\n> This patch by itself does not reach that goal yet, but I think it is a\n> right step in that direction.\n>\n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> ---\n>\n>  * It would be nice if we can avoid byte-by-byte reading from the file\n>   descriptor by over-reading into the strbuf and pass the remainder to\n>   the caller of read_bundle_header(), but I suspect that it would require\n>   us to carry the \"here is the remainder from the previous read\" buffer\n>   around throughout the transport layer. Parsing of the header wouldn't\n>   be performance critical compared to the computation cost of actually\n>   reading the rest of the bundle, hopefully, so...\n>\n>  bundle.c |   99 ++++++++++++++++++++++++++++++++++++++++----------------------\n>  1 files changed, 64 insertions(+), 35 deletions(-)\n>\n> diff --git a/bundle.c b/bundle.c\n> index f48fd7d..3aa715c 100644\n> --- a/bundle.c\n> +++ b/bundle.c\n> @@ -23,49 +23,78 @@ static void add_to_ref_list(const unsigned char *sha1, const char *name,\n>        list->nr++;\n>  }\n>\n> -/* returns an fd */\n> +/* Eventually this should go to strbuf.[ch] */\n> +static int strbuf_readline_fd(struct strbuf *sb, int fd)\n\nA size limiter would be useful here.  Since it's readline, maybe the\nlimit can even be hardcoded.\n\nWithout a limit, calling this with something stupid like \"/dev/zero\"\nwill consume all memory and never return.\n\n\nPhil\n"},{"id":"177639","messageId":"CABURp0otqxeKjLe-Rk3htZRa0M7+rerfrbVrXx-7Dr1tK3tTTg@mail.gmail.com","threadId":"28668","inReplyTo":"7vliso34gc.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] bundle: add parse_bundle_header() helper function","fromName":"Phil Hord","fromEmail":"phil.hord@gmail.com","sentAt":"2011-10-14T12:59:20Z","receivedAt":"2011-10-14T12:59:20Z","isPatch":true,"sender":{"key":"phil.hord@gmail.com","avatar":"https://avatars.githubusercontent.com/u/123908?v=4"},"body":"On Thu, Oct 13, 2011 at 6:35 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Move most of the code from read_bundle_header() to parse_bundle_header()\n> that takes a file descriptor that is already opened for reading, and make\n> the former responsible only for opening the file and noticing errors.\n...\n\n>  * It generally is a bad practice to base a non-RFC patch on an RFC one,\n>   but in any case here is how I would do the is_bundle() helper.\n\nI didn't label it RFC, but I did pose a question in the message\nsection.  Is there a rule for what marks something as RFC (if not the\nRFC label in the subject line)?\n\nYour implementation looks fine to me.\n\nPhil\n"},{"id":"177662","messageId":"7vsjmvzbhc.fsf@alter.siamese.dyndns.org","threadId":"28668","inReplyTo":"CABURp0otqxeKjLe-Rk3htZRa0M7+rerfrbVrXx-7Dr1tK3tTTg@mail.gmail.com","subject":"Re: [PATCH 2/2] bundle: add parse_bundle_header() helper function","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-10-14T18:14:55Z","receivedAt":"2011-10-14T18:14:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phil Hord <phil.hord@gmail.com> writes:\n\n> On Thu, Oct 13, 2011 at 6:35 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>> Move most of the code from read_bundle_header() to parse_bundle_header()\n>> that takes a file descriptor that is already opened for reading, and make\n>> the former responsible only for opening the file and noticing errors.\n> ...\n>\n>>  * It generally is a bad practice to base a non-RFC patch on an RFC one,\n>>   but in any case here is how I would do the is_bundle() helper.\n>\n> I didn't label it RFC,...\n\nI was referring to the fact that _my_ 2/2 you are responding to was meant\nto be applied on top of _my_ 1/2 that was marked as RFC.\n"}]}