Function stripspace now gets a buffer instead file descriptors.
PATCH 1/2 Function stripspace now gets a buffer instead file descriptors.
An implementation easier to call from builtins. It is designed to be used from the upcoming builtin-tag.c and builtin-commit.c, because both need to remove unwanted spaces from messages.
---
This new implementation is a response to some Junio's comments on the version of builtin-tag.c which Kristian released:
http://article.gmane.org/gmane.comp.version-control.git/49601
The aim on not making this function internal yet is to be able to test it easily using the extensive test suite designed for git-stripspace. Later, this function could be in the file named "editor.c", along with the "launch_editor" function called when the program asks the user for a message not given from command-line.
builtin-stripspace.c | 61 ++++++++++++++++++++++++++++++------------------- builtin.h | 2 +- 2 files changed, 38 insertions(+), 25 deletions(-)
builtin-stripspace.c +37 -24
@@ -2,12 +2,11 @@#include "cache.h" /* -* Remove trailing spaces from a line. +* Returns the length of a line removing trailing spaces. * * If the line ends with newline, it will be removed too. -* Returns the new length of the string. */ -static int cleanup (char *line,int len) +static size_t cleanup (char *line,size_t len) {if (len) {if (line[len -1 ] =='\n' )
@@ -19,7 +18,6 @@ static int cleanup(char *line, int len)break ; len--; } - line[len] =0 ; }return len; }
@@ -28,52 +26,67 @@ static int cleanup(char *line, int len)* Remove empty lines from the beginning and end * and also trailing spaces from every line. * +* Note that the buffer will not be null-terminated. +* * Turn multiple consecutive empty lines between paragraphs * into just one empty line. * * If the input has only empty lines and spaces, * no output will be produced. * +* If last line has a newline at the end, it will be removed. +* * Enable skip_comments to skip every line starting with "#". */ -void stripspace (FILE *in,FILE *out,int skip_comments) +size_t stripspace (char *buffer,size_t length,int skip_comments) {int empties =-1 ; -int alloc =1024 ; -char *line =xmalloc (alloc); +size_t i, j, len, newlen; +char *eol; -while (fgets (line, alloc, in)) { -int len =strlen (line); +for (i = j =0 ; i < length; i += len, j += newlen) { + eol =memchr (buffer + i,'\n' , length - i); + len = eol ? eol - (buffer + i) +1 : length - i; -while (len == alloc -1 && line[len -1 ] !='\n' ) { - alloc =alloc_nr (alloc); - line =xrealloc (line, alloc); -fgets (line + len, alloc - len, in); - len +=strlen (line + len); - } - -if (skip_comments && line[0 ] =='#' ) +if (skip_comments && len && buffer[i] =='#' ) { + newlen =0 ;continue ; - len =cleanup (line, len); + } + newlen =cleanup (buffer + i, len);/* Not just an empty line? */ -if (len) { +if (newlen) { +if (empties !=-1 ) + buffer[j++] ='\n' ;if (empties >0 ) -fputc ('\n' , out); + buffer[j++] ='\n' ; empties =0 ; -fputs (line, out); -fputc ('\n' , out); +memmove (buffer + j, buffer + i, newlen);continue ; }if (empties <0 )continue ; empties++; } -free (line); + +return j; }int cmd_stripspace (int argc,const char **argv,const char *prefix) { -stripspace (stdin, stdout,0 ); +char *buffer; +unsigned long size; + + size =1024 ; + buffer =xmalloc (size); +if (read_pipe (0 , &buffer, &size)) +die ("could not read the input" ); + + size =stripspace (buffer, size,0 ); +write_or_die (1 , buffer, size); +if (size) +putc ('\n' , stdout); + +free (buffer);return 0 ; }
builtin.h +1 -1
@@ -7,7 +7,7 @@ extern const char git_version_string[];extern const char git_usage_string[];extern void help_unknown_cmd (const char *cmd); -extern void stripspace (FILE *in,FILE *out,int skip_comments); +extern size_t stripspace (char *buffer,size_t length,int skip_comments);extern int write_tree (unsigned char *sha1,int missing_ok,const char *prefix);extern void prune_packed_objects (int );
Signature · 1 line
1.5.0
builtin-stripspace.c line 5
Quoted 10 lines · Carlos Rica
builtin-stripspace.c +1
-1@@ -2,12 +2,11 @@#include "cache.h" /* -* Remove trailing spaces from a line. +* Returns the length of a line removing trailing spaces.
This did not parse well for me; perhaps a comma before "removing" would make it easier to read?
builtin-stripspace.c static int cleanup(char *line, int len) · line 30
Quoted 6 lines
builtin-stripspace.c +2
-0@@ -28,52 +26,67 @@ static int cleanup(char *line, int len)* Remove empty lines from the beginning and end * and also trailing spaces from every line. * +* Note that the buffer will not be null-terminated. +*
The name of the sentinel character '\0' is NUL, not null (which is a different word, used to call a pointer that points nowhere). The buffer will not be "NUL-terminated".
Quoted 5 lines
builtin-stripspace.c +2
-1-void stripspace (FILE *in,FILE *out,int skip_comments) +size_t stripspace (char *buffer,size_t length,int skip_comments) { + newlen =cleanup (buffer + i, len);
builtin-stripspace.c static int cleanup(char *line, int len) · line 66
Quoted 10 lines
builtin-stripspace.c +5
-0/* Not just an empty line? */ +if (newlen) { +if (empties !=-1 ) + buffer[j++] ='\n' ;if (empties >0 ) + buffer[j++] ='\n' ; empties =0 ; +memmove (buffer + j, buffer + i, newlen);continue ; }
It somehow strikes me odd that, given this:
buffer j i
**********************texttext \n.......you would first do this with cleanup():
buffer j i
**********************texttext\n.......and then do this with this memmove():
buffer j i
*************texttext\n.......Would it become simpler if cleanup() knew where the final text goes (i.e. buffer+j)?
builtin-stripspace.c static int cleanup(char *line, int len) · line 83
Quoted 10 lines
builtin-stripspace.c +7
-1int cmd_stripspace (int argc,const char **argv,const char *prefix) { -stripspace (stdin, stdout,0 ); +char *buffer; +unsigned long size; + + size =1024 ; + buffer =xmalloc (size); +if (read_pipe (0 , &buffer, &size)) +die ("could not read the input" );
The command used to be capable of streaming and filtering a few hundred gigabytes of text on a machine with small address space, as it operated one line at a time, but now it cannot as it has to hold everything in core before starting.
I do not think we miss that loss of capability too much, but I wonder if we can be a bit more clever about it, perhaps feeding a chunk at a time. Not a very strong request, but just wondering if it is an easy change.
Quoted 6 lines · On Wednesday, July 11, 2007 at 15:24:03 (-0700) Junio C Hamano writes:
Carlos Rica <[email protected]> writes:
...Quoted 1 line
+ * Returns the length of a line removing trailing spaces.
This did not parse well for me; perhaps a comma before "removing" would make it easier to read?
Or:
Returns the length of a line, after removing trailing spaces.
Bill
Now the function cleanup is not removing spaces at all, but only counting where the line of text ends.
Before, in the previous version, the function cleanup was taking a string as argument, and then it needed to modify that string. Now, it just returns the new length, "not counting" the spaces and newline at the end of the buffer passed. Its name and comments then could be different, but I didn't know which ones.
Quoted 8 lines · Carlos Rica
Now the function cleanup is not removing spaces at all, but only counting where the line of text ends.
Before, in the previous version, the function cleanup was taking a string as argument, and then it needed to modify that string. Now, it just returns the new length, "not counting" the spaces and newline at the end of the buffer passed. Its name and comments then could be different, but I didn't know which ones.
Ah, you are right. I misread the patch --- cleanup() does not even touch the buffer.
2007/7/12, Junio C Hamano <[email protected]>:
Quoted 11 lines
builtin-stripspace.c static int cleanup(char *line, int len) · line 30
Quoted 6 lines · Carlos Rica
builtin-stripspace.c +2
-0@@ -28,52 +26,67 @@ static int cleanup(char *line, int len)* Remove empty lines from the beginning and end * and also trailing spaces from every line. * +* Note that the buffer will not be null-terminated. +* The name of the sentinel character '\0' is NUL, not null (which is a different word, used to call a pointer that points nowhere). The buffer will not be "NUL-terminated".
Thank you Junio, I will use it on the future.
Quoted 16 lines
builtin-stripspace.c static int cleanup(char *line, int len) · line 83
Quoted 10 lines
builtin-stripspace.c +7
-1int cmd_stripspace (int argc,const char **argv,const char *prefix) { -stripspace (stdin, stdout,0 ); +char *buffer; +unsigned long size; + + size =1024 ; + buffer =xmalloc (size); +if (read_pipe (0 , &buffer, &size)) +die ("could not read the input" );The command used to be capable of streaming and filtering a few hundred gigabytes of text on a machine with small address space, as it operated one line at a time, but now it cannot as it has to hold everything in core before starting.
Quoted 4 lines
I do not think we miss that loss of capability too much, but I wonder if we can be a bit more clever about it, perhaps feeding a chunk at a time. Not a very strong request, but just wondering if it is an easy change.
I did those changes because I was needing those tests that I had written before in order to develop the function. After that, we now can restore the previous function with file descriptors to make it capable of filter a few hundred gigabytes of text, provided that the text does not have long long lines on it.
Indeed, the implementation for composing a tag (header, cleaned message and optional signature) in "builtin-tag.c", now pass it to the function write_sha1_file as a buffer on memory, so it won't support sizes bigger than memory available on the system. Messages should not be so big, but I don't know how to limit those.
Hi,
Quoted 4 lines · Carlos Rica
builtin-stripspace.c +0
-0@@ -28,52 +26,67 @@ static int cleanup(char *line, int len)* Remove empty lines from the beginning and end * and also trailing spaces from every line. *
builtin-stripspace.c static int cleanup(char *line, int len) · line 38
Quoted 10 lines
builtin-stripspace.c +4
-0+* Note that the buffer will not be null-terminated. +* * Turn multiple consecutive empty lines between paragraphs * into just one empty line. * * If the input has only empty lines and spaces, * no output will be produced. * +* If last line has a newline at the end, it will be removed. +*
Please let me comment about the rationale for both changes: The stripspace() function (which this hunk is about) is more useful if it does not allocate a new buffer, but works in-place.
And since it knows the new length already, it can just as well return the length, and _not_ NUL terminate (which would mean that we have to reallocate if we used read_pipe() to get the buffer).
The reason for the missing newline at the end is the same: since we accept buffers with a missing newline at the end, we would have to reallocate in that case.
So for the sake of simplicity, we neither NUL-terminate, nor \n terminate the buffer, and leave that to the callers.
Ciao,
Dscho