{"thread":{"id":"8989","subject":"[PATCH 1/2] Function stripspace now gets a buffer instead file descriptors.","startedAt":"2007-07-11T18:50:34Z","lastAt":"2007-07-12T00:14:53Z","messageCount":7,"participants":["Carlos Rica","Johannes Schindelin","Junio C Hamano","Bill Lear"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"47059","messageId":"4695267A.7080202@gmail.com","threadId":"8989","inReplyTo":null,"subject":"[PATCH 1/2] Function stripspace now gets a buffer instead file descriptors.","fromName":"Carlos Rica","fromEmail":"jasampler@gmail.com","sentAt":"2007-07-11T18:50:34Z","receivedAt":"2007-07-11T18:50:34Z","isPatch":true,"sender":{"key":"jasampler@gmail.com","avatar":null},"body":"An implementation easier to call from builtins. It is designed\nto be used from the upcoming builtin-tag.c and builtin-commit.c,\nbecause both need to remove unwanted spaces from messages.\n\nSigned-off-by: Carlos Rica <jasampler@gmail.com>\n---\n\n  This new implementation is a response to some Junio's comments\n  on the version of builtin-tag.c which Kristian released:\n  http://article.gmane.org/gmane.comp.version-control.git/49601\n\n  The aim on not making this function internal yet\n  is to be able to test it easily using the extensive\n  test suite designed for git-stripspace. Later,\n  this function could be in the file named \"editor.c\",\n  along with the \"launch_editor\" function called when the\n  program asks the user for a message not given from\n  command-line.\n\n builtin-stripspace.c |   61 ++++++++++++++++++++++++++++++-------------------\n builtin.h            |    2 +-\n 2 files changed, 38 insertions(+), 25 deletions(-)\n\ndiff --git a/builtin-stripspace.c b/builtin-stripspace.c\nindex d8358e2..949b640 100644\n--- a/builtin-stripspace.c\n+++ b/builtin-stripspace.c\n@@ -2,12 +2,11 @@\n #include \"cache.h\"\n\n /*\n- * Remove trailing spaces from a line.\n+ * Returns the length of a line removing trailing spaces.\n  *\n  * If the line ends with newline, it will be removed too.\n- * Returns the new length of the string.\n  */\n-static int cleanup(char *line, int len)\n+static size_t cleanup(char *line, size_t len)\n {\n \tif (len) {\n \t\tif (line[len - 1] == '\\n')\n@@ -19,7 +18,6 @@ static int cleanup(char *line, int len)\n \t\t\t\tbreak;\n \t\t\tlen--;\n \t\t}\n-\t\tline[len] = 0;\n \t}\n \treturn len;\n }\n@@ -28,52 +26,67 @@ static int cleanup(char *line, int len)\n  * Remove empty lines from the beginning and end\n  * and also trailing spaces from every line.\n  *\n+ * Note that the buffer will not be null-terminated.\n+ *\n  * Turn multiple consecutive empty lines between paragraphs\n  * into just one empty line.\n  *\n  * If the input has only empty lines and spaces,\n  * no output will be produced.\n  *\n+ * If last line has a newline at the end, it will be removed.\n+ *\n  * Enable skip_comments to skip every line starting with \"#\".\n  */\n-void stripspace(FILE *in, FILE *out, int skip_comments)\n+size_t stripspace(char *buffer, size_t length, int skip_comments)\n {\n \tint empties = -1;\n-\tint alloc = 1024;\n-\tchar *line = xmalloc(alloc);\n+\tsize_t i, j, len, newlen;\n+\tchar *eol;\n\n-\twhile (fgets(line, alloc, in)) {\n-\t\tint len = strlen(line);\n+\tfor (i = j = 0; i < length; i += len, j += newlen) {\n+\t\teol = memchr(buffer + i, '\\n', length - i);\n+\t\tlen = eol ? eol - (buffer + i) + 1 : length - i;\n\n-\t\twhile (len == alloc - 1 && line[len - 1] != '\\n') {\n-\t\t\talloc = alloc_nr(alloc);\n-\t\t\tline = xrealloc(line, alloc);\n-\t\t\tfgets(line + len, alloc - len, in);\n-\t\t\tlen += strlen(line + len);\n-\t\t}\n-\n-\t\tif (skip_comments && line[0] == '#')\n+\t\tif (skip_comments && len && buffer[i] == '#') {\n+\t\t\tnewlen = 0;\n \t\t\tcontinue;\n-\t\tlen = cleanup(line, len);\n+\t\t}\n+\t\tnewlen = cleanup(buffer + i, len);\n\n \t\t/* Not just an empty line? */\n-\t\tif (len) {\n+\t\tif (newlen) {\n+\t\t\tif (empties != -1)\n+\t\t\t\tbuffer[j++] = '\\n';\n \t\t\tif (empties > 0)\n-\t\t\t\tfputc('\\n', out);\n+\t\t\t\tbuffer[j++] = '\\n';\n \t\t\tempties = 0;\n-\t\t\tfputs(line, out);\n-\t\t\tfputc('\\n', out);\n+\t\t\tmemmove(buffer + j, buffer + i, newlen);\n \t\t\tcontinue;\n \t\t}\n \t\tif (empties < 0)\n \t\t\tcontinue;\n \t\tempties++;\n \t}\n-\tfree(line);\n+\n+\treturn j;\n }\n\n int cmd_stripspace(int argc, const char **argv, const char *prefix)\n {\n-\tstripspace(stdin, stdout, 0);\n+\tchar *buffer;\n+\tunsigned long size;\n+\n+\tsize = 1024;\n+\tbuffer = xmalloc(size);\n+\tif (read_pipe(0, &buffer, &size))\n+\t\tdie(\"could not read the input\");\n+\n+\tsize = stripspace(buffer, size, 0);\n+\twrite_or_die(1, buffer, size);\n+\tif (size)\n+\t\tputc('\\n', stdout);\n+\n+\tfree(buffer);\n \treturn 0;\n }\ndiff --git a/builtin.h b/builtin.h\nindex 661a92f..4cc228d 100644\n--- a/builtin.h\n+++ b/builtin.h\n@@ -7,7 +7,7 @@ extern const char git_version_string[];\n extern const char git_usage_string[];\n\n extern void help_unknown_cmd(const char *cmd);\n-extern void stripspace(FILE *in, FILE *out, int skip_comments);\n+extern size_t stripspace(char *buffer, size_t length, int skip_comments);\n extern int write_tree(unsigned char *sha1, int missing_ok, const char *prefix);\n extern void prune_packed_objects(int);\n\n-- \n1.5.0\n"},{"id":"47062","messageId":"Pine.LNX.4.64.0707112010560.4516@racer.site","threadId":"8989","inReplyTo":"4695267A.7080202@gmail.com","subject":"Re: [PATCH 1/2] Function stripspace now gets a buffer instead file descriptors.","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-07-11T19:17:06Z","receivedAt":"2007-07-11T19:17:06Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Wed, 11 Jul 2007, Carlos Rica wrote:\n\n> @@ -28,52 +26,67 @@ static int cleanup(char *line, int len)\n>   * Remove empty lines from the beginning and end\n>   * and also trailing spaces from every line.\n>   *\n> + * Note that the buffer will not be null-terminated.\n> + *\n>   * Turn multiple consecutive empty lines between paragraphs\n>   * into just one empty line.\n>   *\n>   * If the input has only empty lines and spaces,\n>   * no output will be produced.\n>   *\n> + * If last line has a newline at the end, it will be removed.\n> + *\n\nPlease let me comment about the rationale for both changes: The \nstripspace() function (which this hunk is about) is more useful if it does \nnot allocate a new buffer, but works in-place.\n\nAnd since it knows the new length already, it can just as well return the \nlength, and _not_ NUL terminate (which would mean that we have to \nreallocate if we used read_pipe() to get the buffer).\n\nThe reason for the missing newline at the end is the same: since we accept \nbuffers with a missing newline at the end, we would have to reallocate in \nthat case.\n\nSo for the sake of simplicity, we neither NUL-terminate, nor \\n terminate \nthe buffer, and leave that to the callers.\n\nCiao,\nDscho\n"},{"id":"47091","messageId":"7vd4yy1svw.fsf@assigned-by-dhcp.cox.net","threadId":"8989","inReplyTo":"4695267A.7080202@gmail.com","subject":"Re: [PATCH 1/2] Function stripspace now gets a buffer instead file descriptors.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-07-11T22:24:03Z","receivedAt":"2007-07-11T22:24:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlos Rica <jasampler@gmail.com> writes:\n\n> diff --git a/builtin-stripspace.c b/builtin-stripspace.c\n> index d8358e2..949b640 100644\n> --- a/builtin-stripspace.c\n> +++ b/builtin-stripspace.c\n> @@ -2,12 +2,11 @@\n>  #include \"cache.h\"\n>\n>  /*\n> - * Remove trailing spaces from a line.\n> + * Returns the length of a line removing trailing spaces.\n\nThis did not parse well for me; perhaps a comma before\n\"removing\" would make it easier to read?\n\n> @@ -28,52 +26,67 @@ static int cleanup(char *line, int len)\n>   * Remove empty lines from the beginning and end\n>   * and also trailing spaces from every line.\n>   *\n> + * Note that the buffer will not be null-terminated.\n> + *\n\nThe name of the sentinel character '\\0' is NUL, not null (which\nis a different word, used to call a pointer that points\nnowhere).  The buffer will not be \"NUL-terminated\".\n\n> -void stripspace(FILE *in, FILE *out, int skip_comments)\n> +size_t stripspace(char *buffer, size_t length, int skip_comments)\n>  {\n> +\t\tnewlen = cleanup(buffer + i, len);\n>\n>  \t\t/* Not just an empty line? */\n> +\t\tif (newlen) {\n> +\t\t\tif (empties != -1)\n> +\t\t\t\tbuffer[j++] = '\\n';\n>  \t\t\tif (empties > 0)\n> +\t\t\t\tbuffer[j++] = '\\n';\n>  \t\t\tempties = 0;\n> +\t\t\tmemmove(buffer + j, buffer + i, newlen);\n>  \t\t\tcontinue;\n>  \t\t}\n\nIt somehow strikes me odd that, given this:\n\n\tbuffer       j        i\n        **********************texttext  \\n.......\n\nyou would first do this with cleanup():\n\n\tbuffer       j        i\n        **********************texttext\\n.......\n\nand then do this with this memmove():\n\n\tbuffer       j        i\n        *************texttext\\n.......\n\nWould it become simpler if cleanup() knew where the final text\ngoes (i.e. buffer+j)?\n\n>  int cmd_stripspace(int argc, const char **argv, const char *prefix)\n>  {\n> -\tstripspace(stdin, stdout, 0);\n> +\tchar *buffer;\n> +\tunsigned long size;\n> +\n> +\tsize = 1024;\n> +\tbuffer = xmalloc(size);\n> +\tif (read_pipe(0, &buffer, &size))\n> +\t\tdie(\"could not read the input\");\n\nThe command used to be capable of streaming and filtering a few\nhundred gigabytes of text on a machine with small address space,\nas it operated one line at a time, but now it cannot as it has\nto hold everything in core before starting.\n\nI do not think we miss that loss of capability too much, but I\nwonder if we can be a bit more clever about it, perhaps feeding\na chunk at a time.  Not a very strong request, but just\nwondering if it is an easy change.\n"},{"id":"47094","messageId":"18069.26029.224024.66576@lisa.zopyra.com","threadId":"8989","inReplyTo":"7vd4yy1svw.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH 1/2] Function stripspace now gets a buffer instead file descriptors.","fromName":"Bill Lear","fromEmail":"rael@zopyra.com","sentAt":"2007-07-11T23:20:13Z","receivedAt":"2007-07-11T23:20:13Z","isPatch":true,"sender":{"key":"rael@zopyra.com","avatar":"https://gravatar.com/avatar/c4f2d2790ca3828d3b4e7dfebabf61d2fe94fd82fa49cdac2a5295dd2d46a874?d=mp&s=160"},"body":"On Wednesday, July 11, 2007 at 15:24:03 (-0700) Junio C Hamano writes:\n>Carlos Rica <jasampler@gmail.com> writes:\n>...\n>> + * Returns the length of a line removing trailing spaces.\n>\n>This did not parse well for me; perhaps a comma before\n>\"removing\" would make it easier to read?\n\nOr:\n\nReturns the length of a line, after removing trailing spaces.\n\n\nBill\n"},{"id":"47096","messageId":"1b46aba20707111641y615ff82dpf2da4afab2b17d58@mail.gmail.com","threadId":"8989","inReplyTo":"18069.26029.224024.66576@lisa.zopyra.com","subject":"Re: [PATCH 1/2] Function stripspace now gets a buffer instead file descriptors.","fromName":"Carlos Rica","fromEmail":"jasampler@gmail.com","sentAt":"2007-07-11T23:41:38Z","receivedAt":"2007-07-11T23:41:38Z","isPatch":true,"sender":{"key":"jasampler@gmail.com","avatar":null},"body":"Now the function cleanup is not removing spaces at all, but only\ncounting where the line of text ends.\n\nBefore, in the previous version, the function cleanup was taking a\nstring as argument, and then it needed to modify that string.  Now, it\njust returns the new length, \"not counting\" the spaces and newline at\nthe end of the buffer passed. Its name and comments then could be\ndifferent, but I didn't know which ones.\n"},{"id":"47097","messageId":"7v8x9m1oac.fsf@assigned-by-dhcp.cox.net","threadId":"8989","inReplyTo":"1b46aba20707111641y615ff82dpf2da4afab2b17d58@mail.gmail.com","subject":"Re: [PATCH 1/2] Function stripspace now gets a buffer instead file descriptors.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-07-12T00:03:23Z","receivedAt":"2007-07-12T00:03:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Carlos Rica\" <jasampler@gmail.com> writes:\n\n> Now the function cleanup is not removing spaces at all, but only\n> counting where the line of text ends.\n>\n> Before, in the previous version, the function cleanup was taking a\n> string as argument, and then it needed to modify that string.  Now, it\n> just returns the new length, \"not counting\" the spaces and newline at\n> the end of the buffer passed. Its name and comments then could be\n> different, but I didn't know which ones.\n\nAh, you are right.  I misread the patch --- cleanup() does not\neven touch the buffer.\n"},{"id":"47098","messageId":"1b46aba20707111714v63bf921dh15000e2629bcf260@mail.gmail.com","threadId":"8989","inReplyTo":"7vd4yy1svw.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH 1/2] Function stripspace now gets a buffer instead file descriptors.","fromName":"Carlos Rica","fromEmail":"jasampler@gmail.com","sentAt":"2007-07-12T00:14:53Z","receivedAt":"2007-07-12T00:14:53Z","isPatch":true,"sender":{"key":"jasampler@gmail.com","avatar":null},"body":"2007/7/12, Junio C Hamano <gitster@pobox.com>:\n> Carlos Rica <jasampler@gmail.com> writes:\n> > @@ -28,52 +26,67 @@ static int cleanup(char *line, int len)\n> >   * Remove empty lines from the beginning and end\n> >   * and also trailing spaces from every line.\n> >   *\n> > + * Note that the buffer will not be null-terminated.\n> > + *\n>\n> The name of the sentinel character '\\0' is NUL, not null (which\n> is a different word, used to call a pointer that points\n> nowhere).  The buffer will not be \"NUL-terminated\".\n\nThank you Junio, I will use it on the future.\n\n> >  int cmd_stripspace(int argc, const char **argv, const char *prefix)\n> >  {\n> > -     stripspace(stdin, stdout, 0);\n> > +     char *buffer;\n> > +     unsigned long size;\n> > +\n> > +     size = 1024;\n> > +     buffer = xmalloc(size);\n> > +     if (read_pipe(0, &buffer, &size))\n> > +             die(\"could not read the input\");\n>\n> The command used to be capable of streaming and filtering a few\n> hundred gigabytes of text on a machine with small address space,\n> as it operated one line at a time, but now it cannot as it has\n> to hold everything in core before starting.\n>\n> I do not think we miss that loss of capability too much, but I\n> wonder if we can be a bit more clever about it, perhaps feeding\n> a chunk at a time.  Not a very strong request, but just\n> wondering if it is an easy change.\n\nI did those changes because I was needing those tests that\nI had written before in order to develop the function. After that,\nwe now can restore the previous function with file descriptors to\nmake it capable of filter a few hundred gigabytes of text, provided\nthat the text does not have long long lines on it.\n\nIndeed, the implementation for composing a tag (header, cleaned\nmessage and optional signature) in \"builtin-tag.c\", now pass it to\nthe function write_sha1_file as a buffer on memory, so it won't support\nsizes bigger than memory available on the system. Messages should\nnot be so big, but I don't know how to limit those.\n"}]}