{"thread":{"id":"9478","subject":"[PATCH] builtin-bundle.c - use stream buffered input for rev-list","startedAt":"2007-08-10T22:29:49Z","lastAt":"2007-08-11T00:39:24Z","messageCount":5,"participants":["Mark Levedahl","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"50445","messageId":"11867849893486-git-send-email-mdl123@verizon.net","threadId":"9478","inReplyTo":null,"subject":"[PATCH] builtin-bundle.c - use stream buffered input for rev-list","fromName":"Mark Levedahl","fromEmail":"mdl123@verizon.net","sentAt":"2007-08-10T22:29:49Z","receivedAt":"2007-08-10T22:29:49Z","isPatch":true,"sender":{"key":"mdl123@verizon.net","avatar":"https://avatars.githubusercontent.com/u/5302462?v=4"},"body":"git-bundle create on cygwin was nearly unusable due to 1 character\nat a time (unbuffered) reading from an exec'ed process. Fix by using\nfdopen to get a buffered stream.\n\nResults for \"time git bundle create test.bdl v1.0.3..v1.5.2\" are:\n\nbefore this patch:\n         cygwin         linux\nreal    1m38.828s      0m3.578s\nuser    0m12.122s      0m2.896s\nsys     1m28.215s      0m0.692s\n\nafter this patch:\nreal    0m3.688s       0m2.835s\nuser    0m3.075s       0m2.731s\nsys     0m1.075s       0m0.149s\n\nSigned-off-by: Mark Levedahl <mdl123@verizon.net>\n---\n builtin-bundle.c |    7 +++++--\n 1 files changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin-bundle.c b/builtin-bundle.c\nindex 2d0e106..b954213 100644\n--- a/builtin-bundle.c\n+++ b/builtin-bundle.c\n@@ -207,6 +207,7 @@ static int create_bundle(struct bundle_header *header, const char *path,\n \tchar buffer[1024];\n \tstruct rev_info revs;\n \tstruct child_process rls;\n+\tFILE *rls_fout;\n \n \t/*\n \t * NEEDSWORK: this should use something like lock-file\n@@ -236,10 +237,11 @@ static int create_bundle(struct bundle_header *header, const char *path,\n \trls.git_cmd = 1;\n \tif (start_command(&rls))\n \t\treturn -1;\n-\twhile ((i = read_string(rls.out, buffer, sizeof(buffer))) > 0) {\n+\trls_fout = fdopen(rls.out, \"r\");\n+\twhile (fgets(buffer, sizeof(buffer), rls_fout)) {\n \t\tunsigned char sha1[20];\n \t\tif (buffer[0] == '-') {\n-\t\t\twrite_or_die(bundle_fd, buffer, i);\n+\t\t\twrite_or_die(bundle_fd, buffer, strlen(buffer));\n \t\t\tif (!get_sha1_hex(buffer + 1, sha1)) {\n \t\t\t\tstruct object *object = parse_object(sha1);\n \t\t\t\tobject->flags |= UNINTERESTING;\n@@ -250,6 +252,7 @@ static int create_bundle(struct bundle_header *header, const char *path,\n \t\t\tobject->flags |= SHOWN;\n \t\t}\n \t}\n+\tfclose(rls_fout);\n \tif (finish_command(&rls))\n \t\treturn error(\"rev-list died\");\n \n-- \n1.5.3.rc4.53.g9fc90\n"},{"id":"50447","messageId":"7vvebnq9nv.fsf@assigned-by-dhcp.cox.net","threadId":"9478","inReplyTo":"11867849893486-git-send-email-mdl123@verizon.net","subject":"Re: [PATCH] builtin-bundle.c - use stream buffered input for rev-list","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-08-10T22:58:28Z","receivedAt":"2007-08-10T22:58:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mark Levedahl <mdl123@verizon.net> writes:\n\n> git-bundle create on cygwin was nearly unusable due to 1 character\n> at a time (unbuffered) reading from an exec'ed process.\n\nGaah, well spotted.  I was wondering what that 20x difference\nwas, but did not notice that read_string() thing.\n\nThat function is probably sometimes necessary if you are reading\nfrom a non-seekable fd and need to stop immediately after\nreading the header (so that you can hand the rest to pack\nhandling code), but it really should be the last-ditch thing to\nread one-byte-at-a-time.\n\nAlso, I even suspect that we are always reading from a seekable\nbundle file in this program and it should be able to use\nbuffered input with seeking back as needed after you read\nenough.\n"},{"id":"50450","messageId":"11867920441883-git-send-email-mdl123@verizon.net","threadId":"9478","inReplyTo":"7vvebnq9nv.fsf@assigned-by-dhcp.cox.net","subject":"[PATCH] builtin-bundle - use buffered reads for bundle header","fromName":"Mark Levedahl","fromEmail":"mdl123@verizon.net","sentAt":"2007-08-11T00:27:24Z","receivedAt":"2007-08-11T00:27:24Z","isPatch":true,"sender":{"key":"mdl123@verizon.net","avatar":"https://avatars.githubusercontent.com/u/5302462?v=4"},"body":"This eliminates all use of byte-at-a-time reading of data in this\nfunction: as Junio noted, a bundle file is seekable so we can\nreset the file position to the first part of the pack-file using lseek\nafter reading the header.\n\nSigned-off-by: Mark Levedahl <mdl123@verizon.net>\n---\n builtin-bundle.c |   37 +++++++++++++------------------------\n 1 files changed, 13 insertions(+), 24 deletions(-)\n\ndiff --git a/builtin-bundle.c b/builtin-bundle.c\nindex b954213..6651c51 100644\n--- a/builtin-bundle.c\n+++ b/builtin-bundle.c\n@@ -44,38 +44,21 @@ struct bundle_header {\n \tstruct ref_list references;\n };\n \n-/* this function returns the length of the string */\n-static int read_string(int fd, char *buffer, int size)\n-{\n-\tint i;\n-\tfor (i = 0; i < size - 1; i++) {\n-\t\tssize_t count = xread(fd, buffer + i, 1);\n-\t\tif (count < 0)\n-\t\t\treturn error(\"Read error: %s\", strerror(errno));\n-\t\tif (count == 0) {\n-\t\t\ti--;\n-\t\t\tbreak;\n-\t\t}\n-\t\tif (buffer[i] == '\\n')\n-\t\t\tbreak;\n-\t}\n-\tbuffer[i + 1] = '\\0';\n-\treturn i + 1;\n-}\n-\n /* returns an fd */\n static int read_header(const char *path, struct bundle_header *header) {\n \tchar buffer[1024];\n-\tint fd = open(path, O_RDONLY);\n+\tint fd;\n+    long fpos;\n+    FILE *ffd = fopen(path, \"r\");\n \n-\tif (fd < 0)\n+\tif (!ffd)\n \t\treturn error(\"could not open '%s'\", path);\n-\tif (read_string(fd, buffer, sizeof(buffer)) < 0 ||\n+    if (!fgets(buffer, sizeof(buffer), ffd) ||\n \t\t\tstrcmp(buffer, bundle_signature)) {\n-\t\tclose(fd);\n+        fclose(ffd);\n \t\treturn error(\"'%s' does not look like a v2 bundle file\", path);\n \t}\n-\twhile (read_string(fd, buffer, sizeof(buffer)) > 0\n+    while (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@@ -97,6 +80,12 @@ static int read_header(const char *path, struct bundle_header *header) {\n \t\tadd_to_ref_list(sha1, isspace(delim) ?\n \t\t\t\tbuffer + 41 + offset : \"\", list);\n \t}\n+    fpos = ftell(ffd);\n+    fclose(ffd);\n+    fd = open(path, O_RDONLY);\n+    if (fd < 0)\n+        return error(\"could not open '%s'\", path);\n+    lseek(fd, fpos, SEEK_SET);\n \treturn fd;\n }\n \n-- \n1.5.3.rc4.54.gbe00\n"},{"id":"50451","messageId":"7vk5s2rjxj.fsf@assigned-by-dhcp.cox.net","threadId":"9478","inReplyTo":"11867920441883-git-send-email-mdl123@verizon.net","subject":"Re: [PATCH] builtin-bundle - use buffered reads for bundle header","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-08-11T00:31:20Z","receivedAt":"2007-08-11T00:31:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Is this just me (I am writing this from an unfamiliar terminal),\nor is your patch somehow breaks all indentation?\n\n /* returns an fd */\n static int read_header(const char *path, struct bundle_header *header) {\n \tchar buffer[1024];\n-\tint fd = open(path, O_RDONLY);\n+\tint fd;\n+    long fpos;\n+    FILE *ffd = fopen(path, \"r\");\n"},{"id":"50452","messageId":"11867927642553-git-send-email-mdl123@verizon.net","threadId":"9478","inReplyTo":"7vvebnq9nv.fsf@assigned-by-dhcp.cox.net","subject":"[PATCH] builtin-bundle - use buffered reads for bundle header","fromName":"Mark Levedahl","fromEmail":"mdl123@verizon.net","sentAt":"2007-08-11T00:39:24Z","receivedAt":"2007-08-11T00:39:24Z","isPatch":true,"sender":{"key":"mdl123@verizon.net","avatar":"https://avatars.githubusercontent.com/u/5302462?v=4"},"body":"This eliminates all use of byte-at-a-time reading of data in this\nfunction: as Junio noted, a bundle file is seekable so we can\nreset the file position to the first part of the pack-file using lseek\nafter reading the header.\n\nSigned-off-by: Mark Levedahl <mdl123@verizon.net>\n---\n\n Try again - editor had its tab settings mangled..grrrr\n\n builtin-bundle.c |   37 +++++++++++++------------------------\n 1 files changed, 13 insertions(+), 24 deletions(-)\n\ndiff --git a/builtin-bundle.c b/builtin-bundle.c\nindex b954213..b8f8e78 100644\n--- a/builtin-bundle.c\n+++ b/builtin-bundle.c\n@@ -44,38 +44,21 @@ struct bundle_header {\n \tstruct ref_list references;\n };\n\n-/* this function returns the length of the string */\n-static int read_string(int fd, char *buffer, int size)\n-{\n-\tint i;\n-\tfor (i = 0; i < size - 1; i++) {\n-\t\tssize_t count = xread(fd, buffer + i, 1);\n-\t\tif (count < 0)\n-\t\t\treturn error(\"Read error: %s\", strerror(errno));\n-\t\tif (count == 0) {\n-\t\t\ti--;\n-\t\t\tbreak;\n-\t\t}\n-\t\tif (buffer[i] == '\\n')\n-\t\t\tbreak;\n-\t}\n-\tbuffer[i + 1] = '\\0';\n-\treturn i + 1;\n-}\n-\n /* returns an fd */\n static int read_header(const char *path, struct bundle_header *header) {\n \tchar buffer[1024];\n-\tint fd = open(path, O_RDONLY);\n+\tint fd;\n+\tlong fpos;\n+\tFILE *ffd = fopen(path, \"r\");\n\n-\tif (fd < 0)\n+\tif (!ffd)\n \t\treturn error(\"could not open '%s'\", path);\n-\tif (read_string(fd, buffer, sizeof(buffer)) < 0 ||\n+\tif (!fgets(buffer, sizeof(buffer), ffd) ||\n \t\t\tstrcmp(buffer, bundle_signature)) {\n-\t\tclose(fd);\n+        fclose(ffd);\n \t\treturn error(\"'%s' does not look like a v2 bundle file\", path);\n \t}\n-\twhile (read_string(fd, buffer, sizeof(buffer)) > 0\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@@ -97,6 +80,12 @@ static int read_header(const char *path, struct bundle_header *header) {\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 \treturn fd;\n }\n\n--\n1.5.3.rc4.54.gbe00\n"}]}