threads / patch / 8989

patch, 2 partsFunction stripspace now gets a buffer instead file descriptors.

Subject: [PATCH 1/2] Function stripspace now gets a buffer instead file descriptors.

## tl;dr

7 messages between Jul 11, 2007 and Jul 12, 2007. Diffs are folded; open one to read it.

replies: 6people: 4as markdown or json

Carlos Rica· Jul 11, 2007, 18:50 UTC · lore

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.

Signed-off-by: Carlos Rica <jasampler@gmail.com>
---
  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(-)
Show changes to 2 files +38 −25

builtin-stripspace.c, builtin.h

diff --git a/builtin-stripspace.c b/builtin-stripspace.c
index d8358e2..949b640 100644
--- a/builtin-stripspace.c
+++ b/builtin-stripspace.c
@@ -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;
 }
diff --git a/builtin.h b/builtin.h
index 661a92f..4cc228d 100644
--- a/builtin.h
+++ b/builtin.h
@@ -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);

-- 
1.5.0
Johannes Schindelin· Jul 11, 2007, 19:17 UTC · re: Carlos Rica · lore

Re: [PATCH 1/2] Function stripspace now gets a buffer instead file descriptors.

Hi,
On Wed, 11 Jul 2007, Carlos Rica wrote:
Show 14 quoted lines
> @@ -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.
> + *

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

Junio C Hamano· Jul 11, 2007, 22:24 UTC · re: Carlos Rica · lore

Re: [PATCH 1/2] Function stripspace now gets a buffer instead file descriptors.

Carlos Rica <jasampler@gmail.com> writes:
Show 10 quoted lines
> diff --git a/builtin-stripspace.c b/builtin-stripspace.c
> index d8358e2..949b640 100644
> --- a/builtin-stripspace.c
> +++ b/builtin-stripspace.c
> @@ -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?

Show 6 quoted lines
> @@ -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".

Show 15 quoted lines
> -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);
>
>  		/* 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)?

Show 10 quoted lines
>  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");

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.

Bill Lear· Jul 11, 2007, 23:20 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/2] Function stripspace now gets a buffer instead file descriptors.

On Wednesday, July 11, 2007 at 15:24:03 (-0700) Junio C Hamano writes:
Show 6 quoted lines
>Carlos Rica <jasampler@gmail.com> writes:
>...
>> + * 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
Carlos Rica· Jul 11, 2007, 23:41 UTC · re: Bill Lear · lore

Re: [PATCH 1/2] Function stripspace now gets a buffer instead file descriptors.

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.

Junio C Hamano· Jul 12, 2007, 00:03 UTC · re: Carlos Rica · lore

Re: [PATCH 1/2] Function stripspace now gets a buffer instead file descriptors.

"Carlos Rica" <jasampler@gmail.com> writes:
Show 8 quoted lines
> 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.

Carlos Rica· Jul 12, 2007, 00:14 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/2] Function stripspace now gets a buffer instead file descriptors.

2007/7/12, Junio C Hamano <gitster@pobox.com>:
Show 11 quoted lines
> Carlos Rica <jasampler@gmail.com> writes:
> > @@ -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.
Show 20 quoted lines
> >  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");
>
> 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.

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.

← back to recent threads