{"thread":{"id":"13542","subject":"git bug: rebase fatal failure","startedAt":"2008-05-15T07:27:51Z","lastAt":"2008-05-23T11:21:49Z","messageCount":21,"participants":["Tommy Thorn","Johannes Schindelin","Avery Pennarun","David Kastrup","Brian Foster","Stephen R. van den Berg","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"77103","messageId":"482BE5F7.2050108@thorn.ws","threadId":"13542","inReplyTo":null,"subject":"git bug: rebase fatal failure","fromName":"Tommy Thorn","fromEmail":"tommy-git@thorn.ws","sentAt":"2008-05-15T07:27:51Z","receivedAt":"2008-05-15T07:27:51Z","isPatch":false,"sender":{"key":"tommy-git@thorn.ws","avatar":null},"body":"Hi,\n\nI have large-ish repository (143 MB) where git rebase failed \nunexpectedly (see below). If anyone is interested in looking at this, I \nhave left a copy of the repository at \nhttp://thorn.ws/git-fatal-rebase-error.tar.gz\n\nTommy\nPS: Please add me in CC as I'm no longer on this list.\n\n\n$ git --version\ngit version 1.5.5.1.93.ge3f3e\n\n$ git rebase cacao my-cacao-build\nFirst, rewinding head to replay your work on top of it...\nApplying Import binutils-2.18\n.dotest/patch:16878: trailing whitespace.\n \n.dotest/patch:17923: trailing whitespace.\n 0. Additional Definitions.\n.dotest/patch:18024: trailing whitespace.\n Version.\n.dotest/patch:18094: trailing whitespace.\n//\n.dotest/patch:18099: trailing whitespace.\n//\nfatal: corrupt patch at line 260912\nPatch failed at 0001.\n\nWhen you have resolved this problem run \"git rebase --continue\".\nIf you would prefer to skip this patch, instead run \"git rebase --skip\".\nTo restore the original branch and stop rebasing run \"git rebase --abort\".\n"},{"id":"77112","messageId":"alpine.DEB.1.00.0805161139530.30431@racer","threadId":"13542","inReplyTo":"482BE5F7.2050108@thorn.ws","subject":"Re: git bug: rebase fatal failure","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-05-16T10:41:44Z","receivedAt":"2008-05-16T10:41:44Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 15 May 2008, Tommy Thorn wrote:\n\n> I have large-ish repository (143 MB) where git rebase failed \n> unexpectedly (see below). If anyone is interested in looking at this, I \n> have left a copy of the repository at \n> http://thorn.ws/git-fatal-rebase-error.tar.gz\n\nI can reproduce. It seems that our diff machinery generates a file that \nadds a file with 10668 lines, but says 10669 lines in the hunk header.\n\nIt is a .info file, so I suspect strange things going on with \nnon-printable ASCII characters.\n\nI'm on it,\nDscho\n"},{"id":"77115","messageId":"alpine.DEB.1.00.0805161148010.30431@racer","threadId":"13542","inReplyTo":"alpine.DEB.1.00.0805161139530.30431@racer","subject":"Re: git bug: rebase fatal failure","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-05-16T11:01:43Z","receivedAt":"2008-05-16T11:01:43Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 16 May 2008, Johannes Schindelin wrote:\n\n> It is a .info file, so I suspect strange things going on with \n> non-printable ASCII characters.\n\nAnd indeed, this is the culprit:\n\n-- snip --\nBFD Index\n*********\n\n^@^H[index^@^H]\n* Menu:\n\n-- snap --\n\nNote the NUL characters?\n\nNow, the thing is: git format-patch still outputs it correctly. But \ngit-rebase pipes the output to git-am, which in turn calls git-mailsplit, \nwhich suppresses that line.\n\nWill keep you posted,\nDscho\n"},{"id":"77125","messageId":"alpine.DEB.1.00.0805161403130.30431@racer","threadId":"13542","inReplyTo":"alpine.DEB.1.00.0805161148010.30431@racer","subject":"[PATCH] mailsplit and mailinfo: gracefully handle NUL characters","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-05-16T13:03:30Z","receivedAt":"2008-05-16T13:03:30Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"\nThe function fgets() has a big problem with NUL characters: it reads\nthem, but nobody will know if the NUL comes from the file stream, or\nwas appended at the end of the line.\n\nSo implement a custom read_line() function.\n\nNoticed by Tommy Thorn.\n\nSigned-off-by: Johannes Schindelin \n---\n\n\tSorry for the binary patch: the file t5100/nul contains NUL\n\tcharacters, obviously.\n\n\tBTW I do not know how much fgetc() instead of fgets() slows\n\tdown things, but I expect both to be equally fast because\n\tthey are both buffered, right?\n\n builtin-mailinfo.c | 24 +++++++++++++-----------\n builtin-mailsplit.c | 27 +++++++++++++++++++++++----\n builtin.h | 1 +\n t/t5100-mailinfo.sh | 9 +++++++++\n t/t5100/nul | Bin 0 -> 91 bytes\n 5 files changed, 46 insertions(+), 15 deletions(-)\n create mode 100644 t/t5100/nul\n\ndiff --git a/builtin-mailinfo.c b/builtin-mailinfo.c\nindex 11f154b..f0c4209 100644\n--- a/builtin-mailinfo.c\n+++ b/builtin-mailinfo.c\n@@ -641,7 +641,7 @@ static void decode_transfer_encoding(char *line, unsigned linesize)\n \t}\n }\n \n-static int handle_filter(char *line, unsigned linesize);\n+static int handle_filter(char *line, unsigned linesize, int linelen);\n \n static int find_boundary(void)\n {\n@@ -669,7 +669,7 @@ again:\n \t\t\t\t\t\"can't recover\\n\");\n \t\t\texit(1);\n \t\t}\n-\t\thandle_filter(newline, sizeof(newline));\n+\t\thandle_filter(newline, sizeof(newline), strlen(newline));\n \n \t\t/* skip to the next boundary */\n \t\tif (!find_boundary())\n@@ -759,14 +759,14 @@ static int handle_commit_msg(char *line, unsigned linesize)\n \treturn 0;\n }\n \n-static int handle_patch(char *line)\n+static int handle_patch(char *line, int len)\n {\n-\tfputs(line, patchfile);\n+\tfwrite(line, 1, len, patchfile);\n \tpatch_lines++;\n \treturn 0;\n }\n \n-static int handle_filter(char *line, unsigned linesize)\n+static int handle_filter(char *line, unsigned linesize, int linelen)\n {\n \tstatic int filter = 0;\n \n@@ -779,7 +779,7 @@ static int handle_filter(char *line, unsigned linesize)\n \t\t\tbreak;\n \t\tfilter++;\n \tcase 1:\n-\t\tif (!handle_patch(line))\n+\t\tif (!handle_patch(line, linelen))\n \t\t\tbreak;\n \t\tfilter++;\n \tdefault:\n@@ -794,6 +794,7 @@ static void handle_body(void)\n \tint rc = 0;\n \tstatic char newline[2000];\n \tstatic char *np = newline;\n+\tint len = strlen(line);\n \n \t/* Skip up to the first boundary */\n \tif (content_top->boundary) {\n@@ -807,7 +808,8 @@ static void handle_body(void)\n \t\t\t/* flush any leftover */\n \t\t\tif ((transfer_encoding == TE_BASE64) &&\n \t\t\t (np != newline)) {\n-\t\t\t\thandle_filter(newline, sizeof(newline));\n+\t\t\t\thandle_filter(newline, sizeof(newline),\n+\t\t\t\t\t\tstrlen(newline));\n \t\t\t}\n \t\t\tif (!handle_boundary())\n \t\t\t\treturn;\n@@ -824,7 +826,7 @@ static void handle_body(void)\n \n \t\t\t/* binary data most likely doesn't have newlines */\n \t\t\tif (message_type != TYPE_TEXT) {\n-\t\t\t\trc = handle_filter(line, sizeof(newline));\n+\t\t\t\trc = handle_filter(line, sizeof(line), len);\n \t\t\t\tbreak;\n \t\t\t}\n \n@@ -841,7 +843,7 @@ static void handle_body(void)\n \t\t\t\t\t/* should be sitting on a new line */\n \t\t\t\t\t*(++np) = 0;\n \t\t\t\t\top++;\n-\t\t\t\t\trc = handle_filter(newline, sizeof(newline));\n+\t\t\t\t\trc = handle_filter(newline, sizeof(newline), np - newline);\n \t\t\t\t\tnp = newline;\n \t\t\t\t}\n \t\t\t} while (*op != 0);\n@@ -851,12 +853,12 @@ static void handle_body(void)\n \t\t\tbreak;\n \t\t}\n \t\tdefault:\n-\t\t\trc = handle_filter(line, sizeof(newline));\n+\t\t\trc = handle_filter(line, sizeof(line), len);\n \t\t}\n \t\tif (rc)\n \t\t\t/* nothing left to filter */\n \t\t\tbreak;\n-\t} while (fgets(line, sizeof(line), fin));\n+\t} while ((len = read_line_with_nul(line, sizeof(line), fin)));\n \n \treturn;\n }\ndiff --git a/builtin-mailsplit.c b/builtin-mailsplit.c\nindex 46b27cd..021dc16 100644\n--- a/builtin-mailsplit.c\n+++ b/builtin-mailsplit.c\n@@ -45,6 +45,25 @@ static int is_from_line(const char *line, int len)\n /* Could be as small as 64, enough to hold a Unix \"From \" line. */\n static char buf[4096];\n \n+/* We cannot use fgets() because our lines can contain NULs */\n+int read_line_with_nul(char *buf, int size, FILE *in)\n+{\n+\tint len = 0, c;\n+\n+\tfor (;;) {\n+\t\tc = fgetc(in);\n+\t\tbuf[len++] = c;\n+\t\tif (c == EOF || c == '\\n' || len + 1 >= size)\n+\t\t\tbreak;\n+\t}\n+\n+\tif (c == EOF)\n+\t\tlen--;\n+\tbuf[len] = '\\0';\n+\n+\treturn len;\n+}\n+\n /* Called with the first line (potentially partial)\n * already in buf[] -- normally that should begin with\n * the Unix \"From \" line. Write it into the specified\n@@ -70,19 +89,19 @@ static int split_one(FILE *mbox, const char *name, int allow_bare)\n \t * \"From \" and having something that looks like a date format.\n \t */\n \tfor (;;) {\n-\t\tint is_partial = (buf[len-1] != '\\n');\n+\t\tint is_partial = len && buf[len-1] != '\\n';\n \n-\t\tif (fputs(buf, output) == EOF)\n+\t\tif (fwrite(buf, 1, len, output) != len)\n \t\t\tdie(\"cannot write output\");\n \n-\t\tif (fgets(buf, sizeof(buf), mbox) == NULL) {\n+\t\tlen = read_line_with_nul(buf, sizeof(buf), mbox);\n+\t\tif (len == 0) {\n \t\t\tif (feof(mbox)) {\n \t\t\t\tstatus = 1;\n \t\t\t\tbreak;\n \t\t\t}\n \t\t\tdie(\"cannot read mbox\");\n \t\t}\n-\t\tlen = strlen(buf);\n \t\tif (!is_partial && !is_bare && is_from_line(buf, len))\n \t\t\tbreak; /* done with one message */\n \t}\ndiff --git a/builtin.h b/builtin.h\nindex c630d5b..d0a0ead 100644\n--- a/builtin.h\n+++ b/builtin.h\n@@ -9,6 +9,7 @@ extern const char git_usage_string[];\n extern void list_common_cmds_help(void);\n extern void help_unknown_cmd(const char *cmd);\n extern void prune_packed_objects(int);\n+extern int read_line_with_nul(char *buf, int size, FILE *file);\n \n extern int cmd_add(int argc, const char **argv, const char *prefix);\n extern int cmd_annotate(int argc, const char **argv, const char *prefix);\ndiff --git a/t/t5100-mailinfo.sh b/t/t5100-mailinfo.sh\nindex d6c55c1..5a4610b 100755\n--- a/t/t5100-mailinfo.sh\n+++ b/t/t5100-mailinfo.sh\n@@ -25,4 +25,13 @@ do\n \t\tdiff ../t5100/info$mail info$mail\"\n done\n \n+test_expect_success 'respect NULs' '\n+\n+\tgit mailsplit -d3 -o. ../t5100/nul &&\n+\tcmp ../t5100/nul 001 &&\n+\t(cat 001 | git mailinfo msg patch) &&\n+\ttest 4 = $(wc -l < patch)\n+\n+'\n+\n test_done\ndiff --git a/t/t5100/nul b/t/t5100/nul\nnew file mode 100644\nindex 0000000000000000000000000000000000000000..3d40691787b855cc0133514a19052492eb853d21\nGIT binary patch\nliteral 91\nzcmW;6y$ygM5C%|6a#MT@Tm%~v2e7kZ0t`Q)fHOej_C}MJcXX*}a!Gh_N`s3x>;_}@\nmA68>55i?ULDS wrote:\n> BTW I do not know how much fgetc() instead of fgets() slows\n> down things, but I expect both to be equally fast because\n> they are both buffered, right?\n\nIn my experience, fgetc() is pretty fantastically slow because you\nhave a function call for every byte (and, I gather, modern libc does\nthread locking for every fgetc). It's usually much faster to fread()\ninto a buffer and then access the buffer. I don't know if that's\nappropriate (or matters) here, though.\n\nHave fun,\n\nAvery\n"},{"id":"77132","messageId":"861w42wdh6.fsf@lola.quinscape.zz","threadId":"13542","inReplyTo":"32541b130805160703r27a55b91xbad03eb1d107a176@mail.gmail.com","subject":"Re: [PATCH] mailsplit and mailinfo: gracefully handle NUL characters","fromName":"David Kastrup","fromEmail":"dak@gnu.org","sentAt":"2008-05-16T14:05:57Z","receivedAt":"2008-05-16T14:05:57Z","isPatch":true,"sender":{"key":"dak@gnu.org","avatar":"https://avatars.githubusercontent.com/u/52141349?v=4"},"body":"\"Avery Pennarun\" writes:\n\n> On 5/16/08, Johannes Schindelin wrote:\n>> BTW I do not know how much fgetc() instead of fgets() slows\n>> down things, but I expect both to be equally fast because\n>> they are both buffered, right?\n>\n> In my experience, fgetc() is pretty fantastically slow because you\n> have a function call for every byte (and, I gather, modern libc does\n> thread locking for every fgetc). It's usually much faster to fread()\n> into a buffer and then access the buffer. I don't know if that's\n> appropriate (or matters) here, though.\n\nIs getc an option?\n\n-- \nDavid Kastrup\n"},{"id":"77133","messageId":"a537dd660805160707y3830b164td0605a15e6ae05a5@mail.gmail.com","threadId":"13542","inReplyTo":"200805161539.29259.brian.foster@innova-card.com","subject":"Re: [PATCH] mailsplit and mailinfo: gracefully handle NUL characters","fromName":"Brian Foster","fromEmail":"brian.foster@innova-card.com","sentAt":"2008-05-16T14:07:32Z","receivedAt":"2008-05-16T14:07:32Z","isPatch":true,"sender":{"key":"brian.foster@innova-card.com","avatar":null},"body":"two quibbles of no great importance ...\n\nJohannes Schindelin suggested:\n> The function fgets() has a big problem with NUL characters: it reads\n> them, but nobody will know if the NUL comes from the file stream, or\n> was appended at the end of the line.\n>\n> So implement a custom read_line() function.\n ^^^^^^^^^^^\n read_line_with_nul()\nmeaning read part or all of one line which may contain NULs.\n\n>[ ... ]\n> diff --git a/builtin-mailsplit.c b/builtin-mailsplit.c\n> index 46b27cd..021dc16 100644\n> --- a/builtin-mailsplit.c\n> +++ b/builtin-mailsplit.c\n> @@ -45,6 +45,25 @@ static int is_from_line(const char *line, int len)\n> /* Could be as small as 64, enough to hold a Unix \"From \" line. */\n> static char buf[4096];\n>\n> +/* We cannot use fgets() because our lines can contain NULs */\n> +int read_line_with_nul(char *buf, int size, FILE *in)\n> +{\n> + int len = 0, c;\n> +\n> + for (;;) {\n> + c = fgetc(in);\n> + buf[len++] = c;\n> + if (c == EOF || c == '\\n' || len + 1 >= size)\n> + break;\n> + }\n> +\n> + if (c == EOF)\n> + len--;\n> + buf[len] = '\\0';\n> +\n> + return len;\n\n when fgetc(3) — why not use getc(3)? — returns EOF\n it is pointlessly stored in buf[] (as a 'char'!),\n len's advanced, and then the storage and advancing\n are undone. isn't that a bit silly? untested:\n\n\tassert(2 <= size);\n\tdo {\n\t\tif ((c = getc(in)) == EOF)\n\t\t\tbreak;\n\t} while (((buf[len++] = c) != '\\n' && len+1 < size);\n\tbuf[len] = '\\0'\n\n\treturn len;\n\n I'd tend to write this in terms of pointers,\n something along the lines (untested):\n\n\tchar\t*p, *endp;\n\n\tassert(1 <= size);\n\tp = buf;\n\tendp = p + (size-1);\n\twhile (p < endp) {\n\t\tif ((c = getc(in)) == EOF || (*p++ = c) == '\\n')\n\t\t\tbreak;\n\t}\n\t*p = '\\0';\n\n\treturn p - buf;\n\n> +\n> +}\n\n-- \n\"How many surrealists does it take to | Brian Foster\n change a lightbulb? Three. One calms | somewhere in south of France\n the warthog, and two fill the bathtub | Stop E$$o (ExxonMobil)!\n with brightly-coloured machine tools.\" | http://www.stopesso.com\n"},{"id":"77135","messageId":"86wsluuyht.fsf@lola.quinscape.zz","threadId":"13542","inReplyTo":"a537dd660805160707y3830b164td0605a15e6ae05a5@mail.gmail.com","subject":"Re: [PATCH] mailsplit and mailinfo: gracefully handle NUL characters","fromName":"David Kastrup","fromEmail":"dak@gnu.org","sentAt":"2008-05-16T14:14:54Z","receivedAt":"2008-05-16T14:14:54Z","isPatch":true,"sender":{"key":"dak@gnu.org","avatar":"https://avatars.githubusercontent.com/u/52141349?v=4"},"body":"\"Brian Foster\" writes:\n\n> I'd tend to write this in terms of pointers,\n> something along the lines (untested):\n>\n> \tchar\t*p, *endp;\n>\n> \tassert(1 <= size);\n> \tp = buf;\n> \tendp = p + (size-1);\n> \twhile (p < endp) {\n> \t\tif ((c = getc(in)) == EOF || (*p++ = c) == '\\n')\n> \t\t\tbreak;\n> \t}\n> \t*p = '\\0';\n>\n> \treturn p - buf;\n\nLeave optimization to the compiler. Using pointer arithmetic more often\nthan not screws up loop optimization and strength reduction.\n\n-- \nDavid Kastrup\n"},{"id":"77138","messageId":"alpine.DEB.1.00.0805161526120.30431@racer","threadId":"13542","inReplyTo":"a537dd660805160707y3830b164td0605a15e6ae05a5@mail.gmail.com","subject":"Re: [PATCH] mailsplit and mailinfo: gracefully handle NUL characters","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-05-16T14:29:26Z","receivedAt":"2008-05-16T14:29:26Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 16 May 2008, Brian Foster wrote:\n\n> Johannes Schindelin suggested:\n> > The function fgets() has a big problem with NUL characters: it reads\n> > them, but nobody will know if the NUL comes from the file stream, or\n> > was appended at the end of the line.\n> >\n> > So implement a custom read_line() function.\n> ^^^^^^^^^^^\n> read_line_with_nul()\n> meaning read part or all of one line which may contain NULs.\n\nRight.\n\n> >[ ... ]\n> > diff --git a/builtin-mailsplit.c b/builtin-mailsplit.c\n> > index 46b27cd..021dc16 100644\n> > --- a/builtin-mailsplit.c\n> > +++ b/builtin-mailsplit.c\n> > @@ -45,6 +45,25 @@ static int is_from_line(const char *line, int len)\n> > /* Could be as small as 64, enough to hold a Unix \"From \" line. */\n> > static char buf[4096];\n> >\n> > +/* We cannot use fgets() because our lines can contain NULs */\n> > +int read_line_with_nul(char *buf, int size, FILE *in)\n> > +{\n> > + int len = 0, c;\n> > +\n> > + for (;;) {\n> > + c = fgetc(in);\n> > + buf[len++] = c;\n> > + if (c == EOF || c == '\\n' || len + 1 >= size)\n> > + break;\n> > + }\n> > +\n> > + if (c == EOF)\n> > + len--;\n> > + buf[len] = '\\0';\n> > +\n> > + return len;\n> \n> when fgetc(3) — why not use getc(3)? -\n\nBecause mailsplit can read from a file, too.\n\n> returns EOF it is pointlessly stored in buf[] (as a 'char'!), len's \n> advanced, and then the storage and advancing are undone. isn't that a \n> bit silly?\n\nI left it at that, because it is a rare case, the buffer has to be \naccessed with the trailing NUL anyway, and I think it is worth to have \nthis function quite readable. I, for one, am pretty certain that I \nunderstand what this function does, and how, in 6 months from now, without \nany additional documentation.\n\n> untested:\n> \n> \tassert(2 <= size);\n> \tdo {\n> \t\tif ((c = getc(in)) == EOF)\n> \t\t\tbreak;\n> \t} while (((buf[len++] = c) != '\\n' && len+1 < size);\n> \tbuf[len] = '\\0'\n> \n> \treturn len;\n\n... except this is unreadable at best ;-)\n\n> I'd tend to write this in terms of pointers,\n> something along the lines (untested):\n> \n> \tchar\t*p, *endp;\n> \n> \tassert(1 <= size);\n> \tp = buf;\n> \tendp = p + (size-1);\n> \twhile (p < endp) {\n> \t\tif ((c = getc(in)) == EOF || (*p++ = c) == '\\n')\n> \t\t\tbreak;\n> \t}\n> \t*p = '\\0';\n> \n> \treturn p - buf;\n\nAgain, I think this is too cuddled. You have to think about every second \nline, and that makes for stupid mistakes with this developer.\n\nCiao,\nDscho\n"},{"id":"77139","messageId":"alpine.DEB.1.00.0805161529390.30431@racer","threadId":"13542","inReplyTo":"32541b130805160703r27a55b91xbad03eb1d107a176@mail.gmail.com","subject":"Re: [PATCH] mailsplit and mailinfo: gracefully handle NUL characters","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-05-16T14:32:20Z","receivedAt":"2008-05-16T14:32:20Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 16 May 2008, Avery Pennarun wrote:\n\n> On 5/16/08, Johannes Schindelin wrote:\n> > BTW I do not know how much fgetc() instead of fgets() slows\n> > down things, but I expect both to be equally fast because\n> > they are both buffered, right?\n> \n> In my experience, fgetc() is pretty fantastically slow because you\n> have a function call for every byte (and, I gather, modern libc does\n> thread locking for every fgetc). It's usually much faster to fread()\n> into a buffer and then access the buffer.\n\nHmpf. I hoped to get more definitive information here. Especially given \nthat fgetc() is nothing more than a glorified fread() into a buffer, and \nthen access the buffer.\n\nWell, at least you kind of pointed me to the _unlocked() function family.\n\nCiao,\nDscho\n"},{"id":"77140","messageId":"86skwiuxm1.fsf@lola.quinscape.zz","threadId":"13542","inReplyTo":"alpine.DEB.1.00.0805161526120.30431@racer","subject":"Re: [PATCH] mailsplit and mailinfo: gracefully handle NUL characters","fromName":"David Kastrup","fromEmail":"dak@gnu.org","sentAt":"2008-05-16T14:33:58Z","receivedAt":"2008-05-16T14:33:58Z","isPatch":true,"sender":{"key":"dak@gnu.org","avatar":"https://avatars.githubusercontent.com/u/52141349?v=4"},"body":"Johannes Schindelin writes:\n\n> On Fri, 16 May 2008, Brian Foster wrote:\n>> \n>> when fgetc(3) — why not use getc(3)? -\n>\n> Because mailsplit can read from a file, too.\n\nHuh? getc != getchar\n\n-- \nDavid Kastrup\n"},{"id":"77145","messageId":"32541b130805160756h5a8fc4d7x313f9bfde4760568@mail.gmail.com","threadId":"13542","inReplyTo":"alpine.DEB.1.00.0805161529390.30431@racer","subject":"Re: [PATCH] mailsplit and mailinfo: gracefully handle NUL characters","fromName":"Avery Pennarun","fromEmail":"apenwarr@gmail.com","sentAt":"2008-05-16T14:56:35Z","receivedAt":"2008-05-16T14:56:35Z","isPatch":true,"sender":{"key":"apenwarr@gmail.com","avatar":"https://avatars.githubusercontent.com/u/20592?v=4"},"body":"On 5/16/08, Johannes Schindelin wrote:\n> Hmpf. I hoped to get more definitive information here. Especially given\n> that fgetc() is nothing more than a glorified fread() into a buffer, and\n> then access the buffer.\n>\n> Well, at least you kind of pointed me to the _unlocked() function family.\n\nPoint taken.\n\n/tmp $ for d in test1 test2 test3 test3u; do echo -n \"$d: \";\n/usr/bin/time ./$d \n\nint main()\n{\n char buf[1024];\n int i;\n for (i = 0; i < 102400; i++)\n fread(buf, 1, sizeof(buf), stdin);\n}\n\n=== test2.c ===\n#include \n\nint main()\n{\n int i;\n for (i = 0; i < 1024*102400; i++)\n fgetc(stdin);\n}\n\n=== test3.c ===\n#include \n\nint main()\n{\n int i;\n for (i = 0; i < 1024*102400; i++)\n getc(stdin);\n}\n\n=== test3u.c ===\n#include \n\nint main()\n{\n int i;\n for (i = 0; i < 1024*102400; i++)\n getc_unlocked(stdin);\n}\n"},{"id":"77162","messageId":"alpine.DEB.1.00.0805170058160.30431@racer","threadId":"13542","inReplyTo":"32541b130805160756h5a8fc4d7x313f9bfde4760568@mail.gmail.com","subject":"Re: [PATCH] mailsplit and mailinfo: gracefully handle NUL characters","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-05-16T23:59:15Z","receivedAt":"2008-05-16T23:59:15Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 16 May 2008, Avery Pennarun wrote:\n\n> On 5/16/08, Johannes Schindelin wrote:\n> > Hmpf. I hoped to get more definitive information here. Especially given\n> > that fgetc() is nothing more than a glorified fread() into a buffer, and\n> > then access the buffer.\n> >\n> > Well, at least you kind of pointed me to the _unlocked() function family.\n> \n> Point taken.\n> \n> /tmp $ for d in test1 test2 test3 test3u; do echo -n \"$d: \";\n> /usr/bin/time ./$d test1: 0.09user 0.05system 0:00.14elapsed 94%CPU\n> test2: 2.50user 0.05system 0:02.54elapsed 100%CPU\n> test3: 2.48user 0.06system 0:02.53elapsed 100%CPU\n> test3u: 1.05user 0.05system 0:01.10elapsed 99%CPU\n> \n> fread is about 18x faster than fgetc(). getc() is the same speed as\n> fgetc(). getc_unlocked() is definitely faster than getc, but still at\n> least 7x slower than fread().\n\nWell, my question was more about fgetc() vs fgets().\n\nIf you feel like it, you might benchmark this patch:\n\n-- snipsnap --\ndiff --git a/builtin-mailsplit.c b/builtin-mailsplit.c\nindex 021dc16..5d8defd 100644\n--- a/builtin-mailsplit.c\n+++ b/builtin-mailsplit.c\n@@ -45,13 +45,32 @@ static int is_from_line(const char *line, int len)\n /* Could be as small as 64, enough to hold a Unix \"From \" line. */\n static char buf[4096];\n \n+/*\n+ * This is an ugly hack to avoid fgetc(), which is slow, as it is locking.\n+ * The argument \"in\" must be the same for all calls to this function!\n+ */\n+static int fast_fgetc(FILE *in)\n+{\n+\tstatic char buf[4096];\n+\tstatic int offset = 0, len = 0;\n+\n+\tif (offset >= len) {\n+\t\tlen = fread(buf, 1, sizeof(buf), in);\n+\t\toffset = 0;\n+\t\tif (!len && feof(in))\n+\t\t\treturn EOF;\n+\t}\n+\n+\treturn buf[offset++];\n+}\n+\n /* We cannot use fgets() because our lines can contain NULs */\n int read_line_with_nul(char *buf, int size, FILE *in)\n {\n \tint len = 0, c;\n \n \tfor (;;) {\n-\t\tc = fgetc(in);\n+\t\tc = fast_fgetc(in);\n \t\tbuf[len++] = c;\n \t\tif (c == EOF || c == '\\n' || len + 1 >= size)\n \t\t\tbreak;\n"},{"id":"77166","messageId":"482E2175.8030904@thorn.ws","threadId":"13542","inReplyTo":"alpine.DEB.1.00.0805170058160.30431@racer","subject":"Re: [PATCH] mailsplit and mailinfo: gracefully handle NUL characters","fromName":"Tommy Thorn","fromEmail":"tommy-git@thorn.ws","sentAt":"2008-05-17T00:06:13Z","receivedAt":"2008-05-17T00:06:13Z","isPatch":true,"sender":{"key":"tommy-git@thorn.ws","avatar":null},"body":"Johannes Schindelin wrote:\n> +/*\n> + * This is an ugly hack to avoid fgetc(), which is slow, as it is locking.\n> + * The argument \"in\" must be the same for all calls to this function!\n> + */\n> +static int fast_fgetc(FILE *in)\n> +{\n> \n\nLooks great to me, but shouldn't you add an \"inline\" for this one? Also, \nmaybe a double the buffer size.\n\nTommy\n"},{"id":"77169","messageId":"alpine.DEB.1.00.0805170120550.30431@racer","threadId":"13542","inReplyTo":"482E2175.8030904@thorn.ws","subject":"Re: [PATCH] mailsplit and mailinfo: gracefully handle NUL characters","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-05-17T00:26:26Z","receivedAt":"2008-05-17T00:26:26Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 16 May 2008, Tommy Thorn wrote:\n\n> Johannes Schindelin wrote:\n> > +/*\n> > + * This is an ugly hack to avoid fgetc(), which is slow, as it is locking.\n> > + * The argument \"in\" must be the same for all calls to this function!\n> > + */\n> > +static int fast_fgetc(FILE *in)\n> > +{\n> > \n> \n> Looks great to me, but shouldn't you add an \"inline\" for this one? Also, \n> maybe a double the buffer size.\n\nNo. This is an ugly hack, and not meant for application.\n\nIf that is substantially faster than the fgetc() version (and I want this \nbe tested in a _real-world_ scenario, i.e. not the fgetc() alone, but a \nreal mailsplit and a real mailinfo on a huge patch, with all three \nversions: fgets(), fgetc() and fast_fgetc())), then I would prefer having \nsomething like\n\n\tstruct line_reader {\n\t\tFILE *in;\n\t\tchar buffer[4096];\n\t\tint offset, int len;\n\t\tchar line[1024];\n\t\tint linelen;\n\t};\n\nand corresponding functions to read lines in that setting. Maybe it would \neven be better to have line be a strbuf, but I am not so sure on that.\n\nLet's see what the tests show. Would you do them, please? \n\"git format-patch --stdout bla..blub | /usr/bin/time git mailsplit -o.\" \nthree times in succession should give you a good hint on the runtime.\n\nCiao,\nDscho\n"},{"id":"77182","messageId":"20080517100726.GA24416@cuci.nl","threadId":"13542","inReplyTo":"alpine.DEB.1.00.0805170058160.30431@racer","subject":"Re: [PATCH] mailsplit and mailinfo: gracefully handle NUL characters","fromName":"Stephen R. van den Berg","fromEmail":"srb@cuci.nl","sentAt":"2008-05-17T10:07:26Z","receivedAt":"2008-05-17T10:07:26Z","isPatch":true,"sender":{"key":"srb@cuci.nl","avatar":"https://gravatar.com/avatar/f75389059e827634d38e9df2a9b6ecbd50028b5a454442efa1c7205b7ff29c6a?d=mp&s=160"},"body":"Johannes Schindelin wrote:\n>On Fri, 16 May 2008, Avery Pennarun wrote:\n>> fread is about 18x faster than fgetc(). getc() is the same speed as\n>> fgetc(). getc_unlocked() is definitely faster than getc, but still at\n>> least 7x slower than fread().\n\n>Well, my question was more about fgetc() vs fgets().\n>If you feel like it, you might benchmark this patch:\n\nWouldn't it be better to improve the implementation of getc()\nin glibc instead?\n\ngetc() is meant to be the fast version of fgetc(), and if it isn't\n(anymore), then the library needs fixing.\n-- \nSincerely, srb@cuci.nl\n Stephen R. van den Berg.\n"},{"id":"77184","messageId":"alpine.DEB.1.00.0805171117010.30431@racer","threadId":"13542","inReplyTo":"20080517100726.GA24416@cuci.nl","subject":"Re: [PATCH] mailsplit and mailinfo: gracefully handle NUL characters","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-05-17T10:18:17Z","receivedAt":"2008-05-17T10:18:17Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sat, 17 May 2008, Stephen R. van den Berg wrote:\n\n> Johannes Schindelin wrote:\n> >On Fri, 16 May 2008, Avery Pennarun wrote:\n> >> fread is about 18x faster than fgetc(). getc() is the same speed as\n> >> fgetc(). getc_unlocked() is definitely faster than getc, but still at\n> >> least 7x slower than fread().\n> \n> >Well, my question was more about fgetc() vs fgets().\n> >If you feel like it, you might benchmark this patch:\n> \n> Wouldn't it be better to improve the implementation of getc()\n> in glibc instead?\n\nSure, because glibc is used on Windows, AIX, Solaris, etc.\n\n> getc() is meant to be the fast version of fgetc(), and if it isn't \n> (anymore), then the library needs fixing.\n\nfgetc() has to work reliably in a threaded environment, too. So I guess \nthat it does all kinds of locks that slow it down, but it is not something \nto be \"fixed\".\n\nHth,\nDscho\n"},{"id":"77402","messageId":"7v8wy34jj3.fsf@gitster.siamese.dyndns.org","threadId":"13542","inReplyTo":"alpine.DEB.1.00.0805161403130.30431@racer","subject":"Re: [PATCH] mailsplit and mailinfo: gracefully handle NUL characters","fromName":"Junio C Hamano","fromEmail":"junio@pobox.com","sentAt":"2008-05-21T18:08:32Z","receivedAt":"2008-05-21T18:08:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> The function fgets() has a big problem with NUL characters: it reads\n> them, but nobody will know if the NUL comes from the file stream, or\n> was appended at the end of the line.\n>\n> So implement a custom read_line() function.\n\nLooking at what handle_body() does for TE_BASE64 and TE_QP cases, I have\nto wonder if this is enough. The loop seems to stop at (*op == NUL) which\nfollows an old assumption that each line is terminated with NUL, not the\nnew assumption you introduced that each line's length is kept in local\nvariable len.\n"},{"id":"77453","messageId":"alpine.DEB.1.00.0805221136230.30431@racer","threadId":"13542","inReplyTo":"7v8wy34jj3.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] mailsplit and mailinfo: gracefully handle NUL characters","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-05-22T10:38:34Z","receivedAt":"2008-05-22T10:38:34Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Wed, 21 May 2008, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> > The function fgets() has a big problem with NUL characters: it reads \n> > them, but nobody will know if the NUL comes from the file stream, or \n> > was appended at the end of the line.\n> >\n> > So implement a custom read_line() function.\n> \n> Looking at what handle_body() does for TE_BASE64 and TE_QP cases, I have \n> to wonder if this is enough. The loop seems to stop at (*op == NUL) \n> which follows an old assumption that each line is terminated with NUL, \n> not the new assumption you introduced that each line's length is kept in \n> local variable len.\n\nOf course! But does BASE64 and QP contain NULs? After all, even my \ncustom read_line_with_nul() function adds a NUL IIRC.\n\nWell, the biggest problem here is my lack of time. I thought I would give \nTommy a patch which kinda works, and he would actually hold through to \nbrush it up until it shines and gets into git.git, because it is not _my_ \nitch.\n\nHmmmmmmm.\n\nCiao,\nDscho\n"},{"id":"77482","messageId":"7v8wy2w7wg.fsf@gitster.siamese.dyndns.org","threadId":"13542","inReplyTo":"alpine.DEB.1.00.0805221136230.30431@racer","subject":"Re: [PATCH] mailsplit and mailinfo: gracefully handle NUL characters","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-05-22T17:44:31Z","receivedAt":"2008-05-22T17:44:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n>> Looking at what handle_body() does for TE_BASE64 and TE_QP cases, I have \n>> to wonder if this is enough. The loop seems to stop at (*op == NUL) \n>> which follows an old assumption that each line is terminated with NUL, \n>> not the new assumption you introduced that each line's length is kept in \n>> local variable len.\n>\n> Of course! But does BASE64 and QP contain NULs?\n\nThe loop in question iterates over bytes _after_ decoding these encoded\nlines, and a typical reason you would encode the payload is because it\ncontains something not safe over e-mail transfer, e.g. NUL.\n\nI think decode_transfer_encoding() also needs to become safe against NULs\nin the payload.\n"},{"id":"77535","messageId":"alpine.DEB.1.00.0805231221390.30431@racer","threadId":"13542","inReplyTo":"7v8wy2w7wg.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] mailsplit and mailinfo: gracefully handle NUL characters","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-05-23T11:21:49Z","receivedAt":"2008-05-23T11:21:49Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 22 May 2008, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> >> Looking at what handle_body() does for TE_BASE64 and TE_QP cases, I have \n> >> to wonder if this is enough. The loop seems to stop at (*op == NUL) \n> >> which follows an old assumption that each line is terminated with NUL, \n> >> not the new assumption you introduced that each line's length is kept in \n> >> local variable len.\n> >\n> > Of course! But does BASE64 and QP contain NULs?\n> \n> The loop in question iterates over bytes _after_ decoding these encoded\n> lines, and a typical reason you would encode the payload is because it\n> contains something not safe over e-mail transfer, e.g. NUL.\n> \n> I think decode_transfer_encoding() also needs to become safe against NULs\n> in the payload.\n\nOkay, I missed that.\n\nCiao,\nDscho\n"}]}