strbuf API

36 messages, 7 authors, 2016-06-15 · open the first message on its own page

strbuf API

From: Pierre Habouzit <hidden>
Date: 2016-06-15 22:43:32

  I read the recent thread Timo Sirainen raised about string APIs in
git. ANd I went read the strbuf.[hc] module.

  I believe that the choice made in that module are wrong and could be
made better. I actually use to work with a string buffer API (that
interested readers can look at on [0]), that work almost the same
(except for the eof flag, but it's trivial to keep), but have
two significant differences:

  First, and that's the most important one: the buffer is always NUL
terminated, after its official "len". That means, in terms of strbuf
API, that "alloc" is always greater or equal to len+1 to be able to
store the ending NUL[1]. The advantages are obvious: you can pass the
buffer to any legacy C string function without any fear of read
overflow.  strtol comes to mind, and atm, git has to explicitely use
strbuf_end to put that ending nul to be able to call legacy
applications. But once done, the NUL is accounted into the string (aka
it's in "len") which makes it a non C-string (I mean you cannot append
any more data in it anymore). So current implementations tries to
workaround an issue (the non systematical NUL-termination) but IMHO the
wrong way.

  The other shortcoming is that you cannot tell the buffer "Hey, it's
very likely that you'll end up being _that_ long. That's why, in some
parts of the code (see write_tar_entry in archive-tar.c e.g.) the
programmer actually messes with the buffer allocation, outside from the
strbuf module, which makes it well, useless. In my API, I have a
"buffer_ensure" call, that is supposed to do that: "please ensure that
this buffer still has _this_ amount of free and allocated space to put
more data".


  So my question is, do people think I raise a valid point, and would
patches that would refactor the strbuf module to have those functions,
and would fix the code that uses strbuf's to interact properly, be
accepted ?

  Also, the efficiency of the buffer module API I use has a lot to do
with the fact that copying functions, and length tests are inlined in
the .h, so that the compiler can optimize the ones it already tested 10
calls before. I'm not sure if this is frowned upon or if it makes sense.


  [0] http://git.madism.org/?p=pfixtools.git;a=blob;f=buffer.h;hb=HEAD
      http://git.madism.org/?p=pfixtools.git;a=blob;f=buffer.c;hb=HEAD

  [1] Of course, ensuring the NUL-termination has a cost, though it's
      often benign, and for performance-critical places where characters
      are copied one by one, it's always possible to use an "unsafe"
      addch (that would not maintain the invariant), and then call an
      equivalent of strbuf_end (that would not append a \0 like it does
      now, but just would fix the invariant that for any strbuf,
      buf->buf[buf->len] == '\0') explicitely. For places where the
      invariant generate negligible cost (like concatenating two paths
      parts with a middle '/' e.g.) then we gain safety without even
      having to think about it.

-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

Re: strbuf API

From: Johan Herland <hidden>
Date: 2016-06-15 22:43:32

On Monday 03 September 2007, Pierre Habouzit wrote:
  I read the recent thread Timo Sirainen raised about string APIs in
git. ANd I went read the strbuf.[hc] module.

  I believe that the choice made in that module are wrong and could be
made better. I actually use to work with a string buffer API (that
interested readers can look at on [0]), that work almost the same
(except for the eof flag, but it's trivial to keep), but have
two significant differences:

  First, and that's the most important one: the buffer is always NUL
terminated, after its official "len". That means, in terms of strbuf
API, that "alloc" is always greater or equal to len+1 to be able to
store the ending NUL[1]. The advantages are obvious: you can pass the
buffer to any legacy C string function without any fear of read
overflow.  strtol comes to mind, and atm, git has to explicitely use
strbuf_end to put that ending nul to be able to call legacy
applications. But once done, the NUL is accounted into the string (aka
it's in "len") which makes it a non C-string (I mean you cannot append
any more data in it anymore). So current implementations tries to
workaround an issue (the non systematical NUL-termination) but IMHO the
wrong way.

  The other shortcoming is that you cannot tell the buffer "Hey, it's
very likely that you'll end up being _that_ long. That's why, in some
parts of the code (see write_tar_entry in archive-tar.c e.g.) the
programmer actually messes with the buffer allocation, outside from the
strbuf module, which makes it well, useless. In my API, I have a
"buffer_ensure" call, that is supposed to do that: "please ensure that
this buffer still has _this_ amount of free and allocated space to put
more data".


  So my question is, do people think I raise a valid point
Yes.
  , and would 
patches that would refactor the strbuf module to have those functions,
and would fix the code that uses strbuf's to interact properly, be
accepted ?
I don't know. Keep in mind that there is a parallel process (cf. the 
continuation of the "Buffer overflows" thread) to evaluate the Bstring 
library ( http://bstring.sourceforge.net/ ), and possibly substitute that 
for the strbuf "module".

I wouldn't want the work done by you or others to be wasted (depending on 
which solution wins out in the end), so I suggest you take a look at 
bstring and offer up arguments why your solution would be better for git.
  Also, the efficiency of the buffer module API I use has a lot to do
with the fact that copying functions, and length tests are inlined in
the .h, so that the compiler can optimize the ones it already tested 10
calls before. I'm not sure if this is frowned upon or if it makes sense.


  [0] http://git.madism.org/?p=pfixtools.git;a=blob;f=buffer.h;hb=HEAD
      http://git.madism.org/?p=pfixtools.git;a=blob;f=buffer.c;hb=HEAD

  [1] Of course, ensuring the NUL-termination has a cost, though it's
      often benign, and for performance-critical places where characters
      are copied one by one, it's always possible to use an "unsafe"
      addch (that would not maintain the invariant), and then call an
      equivalent of strbuf_end (that would not append a \0 like it does
      now, but just would fix the invariant that for any strbuf,
      buf->buf[buf->len] == '\0') explicitely. For places where the
      invariant generate negligible cost (like concatenating two paths
      parts with a middle '/' e.g.) then we gain safety without even
      having to think about it.
All the points you raise are valid, but please have in mind that all 
programmers write their own string library at some point. (Writing a string 
library is right up there with "discovering why 'goto' is bad" and "2nd 
system syndrome" on the top list of rites of passage all programmers (need 
to) go through. I'm guessing 99% of the people on this list have written 
their own string library. Why not 100%? Well, Linus have obviously not, 
else we'd be using his already... ;)

I'd like to know how your solution compare with the alternatives (in this 
case bstring): What's the performance compared to bstring? How much thought 
has been given to the usability of the API? What functionality is covered 
(compared to bstring)? In short: Why is your alternative the best for git?


...Johan

-- 
Johan Herland, [off-list ref]
www.herland.net

Re: strbuf API

From: Pierre Habouzit <hidden>
Date: 2016-06-15 22:43:32

On Mon, Sep 03, 2007 at 05:43:44AM +0000, Johan Herland wrote:
On Monday 03 September 2007, Pierre Habouzit wrote:
quoted
  , and would 
patches that would refactor the strbuf module to have those functions,
and would fix the code that uses strbuf's to interact properly, be
accepted ?
I don't know. Keep in mind that there is a parallel process (cf. the 
continuation of the "Buffer overflows" thread) to evaluate the Bstring 
library ( http://bstring.sourceforge.net/ ), and possibly substitute that 
for the strbuf "module".

I wouldn't want the work done by you or others to be wasted (depending on 
which solution wins out in the end), so I suggest you take a look at 
bstring and offer up arguments why your solution would be better for git.
  Oh I completely missed that the thread was still alive.
I'd like to know how your solution compare with the alternatives (in this 
case bstring): What's the performance compared to bstring? How much thought 
has been given to the usability of the API? What functionality is covered 
(compared to bstring)? In short: Why is your alternative the best for git?
  Well, technically, I don't mind, I wasn't really pushing this or this
implementation. I had a look at bstring, I dislike the code that looks
pretty ugly, but they almost took the same decisions, so if the
consensus is that bstring gives flexibility, so be it.

  For the record, I dislike the fact that bstring pullutes the b*
namespace, whereas it should use bstr as a prefix everywhere, and some
functions (like bstrcpy, that should definitely be named bstrdup) are
really named the _wrong_ way: you can dislike legacy C string APIs all
the way you want, if you name a function from your foostr library
foostrbar, it better behave the way strbar does in the legacy C API. In
the legacy C, strcpy clobbers the destination, here there is no
destination as bstrcpy is definitely a strdup operation. I only had a
quick look, and maybe it was the sole of the kind, but ... well.

  I also dislike the fact that there is no inline version of the
function that appends C strings, because it means that when you append a
constant string into a bstr, you'll need to write:

  bstrcat(bstr, "foo", sizeof("foo") - 1) or bstrcat(bstr, "foo", 3),
both suck a lot, and with an inline function using strlen, most
compilers will optimize the strlen away, and the code is way more
readable. But again, this is trivial to implement as an extension as
well, so I really don't mind that much either.

-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

strbuf new API, take 2 for inclusion

From: Pierre Habouzit <hidden>
Date: 2016-06-15 22:43:33

  Here is a new patchset (with no wrong patchs and numbering this time)
with Junio's remarks integrated (the va_copy stuff, better documentation
and patch comments and so on).

  strbuf_read remains a "I can read all the content of the file or the
caller is supposed to die()" operation for now, as it's how it's used
right now, this can be changed easily in the future if needs for it
exists.

  I've also stripped as many STRBUF_INIT uses as possible, some people
didn't liked it. I've kept its use for "static" strbufs where it's way
more convenient that a function call.

[PATCH 4/7] Simplify write_tree using strbuf's.

From: Pierre Habouzit <hidden>
Date: 2016-06-15 22:43:33

Signed-off-by: Pierre Habouzit <redacted>
---
 mktree.c |   23 ++++++++---------------
 1 files changed, 8 insertions(+), 15 deletions(-)
diff --git a/mktree.c b/mktree.c
index 86de5eb..2e84889 100644
--- a/mktree.c
+++ b/mktree.c
@@ -44,30 +44,23 @@ static int ent_compare(const void *a_, const void *b_)
 
 static void write_tree(unsigned char *sha1)
 {
-	char *buffer;
-	unsigned long size, offset;
+	struct strbuf buf;
+	size_t size;
 	int i;
 
 	qsort(entries, used, sizeof(*entries), ent_compare);
 	for (size = i = 0; i < used; i++)
 		size += 32 + entries[i]->len;
-	buffer = xmalloc(size);
-	offset = 0;
+	strbuf_init(&buf);
+	strbuf_grow(&buf, size);
 
 	for (i = 0; i < used; i++) {
 		struct treeent *ent = entries[i];
-
-		if (offset + ent->len + 100 < size) {
-			size = alloc_nr(offset + ent->len + 100);
-			buffer = xrealloc(buffer, size);
-		}
-		offset += sprintf(buffer + offset, "%o ", ent->mode);
-		offset += sprintf(buffer + offset, "%s", ent->name);
-		buffer[offset++] = 0;
-		hashcpy((unsigned char*)buffer + offset, ent->sha1);
-		offset += 20;
+		strbuf_addf(&buf, "%o %s%c", ent->mode, ent->name, '\0');
+		strbuf_add(&buf, ent->sha1, 20);
 	}
-	write_sha1_file(buffer, offset, tree_type, sha1);
+
+	write_sha1_file(buf.buf, buf.len, tree_type, sha1);
 }
 
 static const char mktree_usage[] = "git-mktree [-z]";
-- 
1.5.3.1

[PATCH 2/7] Simplify strbuf uses in archive-tar.c using the proper functions.

From: Pierre Habouzit <hidden>
Date: 2016-06-15 22:43:33

  This is just cleaner way to deal with strbufs, using its API rather than
reinventing it in the module (e.g. strbuf_append_string is just the plain
strbuf_addstr function, and it was used to perform what strbuf_addch does
anyways).
---
 archive-tar.c |   65 ++++++++++++++-------------------------------------------
 1 files changed, 16 insertions(+), 49 deletions(-)
diff --git a/archive-tar.c b/archive-tar.c
index a0763c5..c84d7c0 100644
--- a/archive-tar.c
+++ b/archive-tar.c
@@ -78,19 +78,6 @@ static void write_trailer(void)
 	}
 }
 
-static void strbuf_append_string(struct strbuf *sb, const char *s)
-{
-	int slen = strlen(s);
-	int total = sb->len + slen;
-	if (total + 1 > sb->alloc) {
-		sb->buf = xrealloc(sb->buf, total + 1);
-		sb->alloc = total + 1;
-	}
-	memcpy(sb->buf + sb->len, s, slen);
-	sb->len = total;
-	sb->buf[total] = '\0';
-}
-
 /*
  * pax extended header records have the format "%u %s=%s\n".  %u contains
  * the size of the whole string (including the %u), the first %s is the
@@ -100,26 +87,17 @@ static void strbuf_append_string(struct strbuf *sb, const char *s)
 static void strbuf_append_ext_header(struct strbuf *sb, const char *keyword,
                                      const char *value, unsigned int valuelen)
 {
-	char *p;
-	int len, total, tmp;
+	int len, tmp;
 
 	/* "%u %s=%s\n" */
 	len = 1 + 1 + strlen(keyword) + 1 + valuelen + 1;
 	for (tmp = len; tmp > 9; tmp /= 10)
 		len++;
 
-	total = sb->len + len;
-	if (total > sb->alloc) {
-		sb->buf = xrealloc(sb->buf, total);
-		sb->alloc = total;
-	}
-
-	p = sb->buf;
-	p += sprintf(p, "%u %s=", len, keyword);
-	memcpy(p, value, valuelen);
-	p += valuelen;
-	*p = '\n';
-	sb->len = total;
+	strbuf_grow(sb, len);
+	strbuf_addf(sb, "%u %s=", len, keyword);
+	strbuf_add(sb, value, valuelen);
+	strbuf_addch(sb, '\n');
 }
 
 static unsigned int ustar_header_chksum(const struct ustar_header *header)
@@ -153,8 +131,7 @@ static void write_entry(const unsigned char *sha1, struct strbuf *path,
 	struct strbuf ext_header;
 
 	memset(&header, 0, sizeof(header));
-	ext_header.buf = NULL;
-	ext_header.len = ext_header.alloc = 0;
+	strbuf_init(&ext_header);
 
 	if (!sha1) {
 		*header.typeflag = TYPEFLAG_GLOBAL_HEADER;
@@ -225,8 +202,8 @@ static void write_entry(const unsigned char *sha1, struct strbuf *path,
 
 	if (ext_header.len > 0) {
 		write_entry(sha1, NULL, 0, ext_header.buf, ext_header.len);
-		free(ext_header.buf);
 	}
+	strbuf_release(&ext_header);
 	write_blocked(&header, sizeof(header));
 	if (S_ISREG(mode) && buffer && size > 0)
 		write_blocked(buffer, size);
@@ -235,11 +212,11 @@ static void write_entry(const unsigned char *sha1, struct strbuf *path,
 static void write_global_extended_header(const unsigned char *sha1)
 {
 	struct strbuf ext_header;
-	ext_header.buf = NULL;
-	ext_header.len = ext_header.alloc = 0;
+
+	strbuf_init(&ext_header);
 	strbuf_append_ext_header(&ext_header, "comment", sha1_to_hex(sha1), 40);
 	write_entry(NULL, NULL, 0, ext_header.buf, ext_header.len);
-	free(ext_header.buf);
+	strbuf_release(&ext_header);
 }
 
 static int git_tar_config(const char *var, const char *value)
@@ -260,28 +237,18 @@ static int write_tar_entry(const unsigned char *sha1,
                            const char *base, int baselen,
                            const char *filename, unsigned mode, int stage)
 {
-	static struct strbuf path;
+	static struct strbuf path = STRBUF_INIT;
 	int filenamelen = strlen(filename);
 	void *buffer;
 	enum object_type type;
 	unsigned long size;
 
-	if (!path.alloc) {
-		path.buf = xmalloc(PATH_MAX);
-		path.alloc = PATH_MAX;
-		path.len = path.eof = 0;
-	}
-	if (path.alloc < baselen + filenamelen + 1) {
-		free(path.buf);
-		path.buf = xmalloc(baselen + filenamelen + 1);
-		path.alloc = baselen + filenamelen + 1;
-	}
-	memcpy(path.buf, base, baselen);
-	memcpy(path.buf + baselen, filename, filenamelen);
-	path.len = baselen + filenamelen;
-	path.buf[path.len] = '\0';
+	strbuf_grow(&path, MAX(PATH_MAX, baselen + filenamelen + 1));
+	strbuf_reset(&path);
+	strbuf_add(&path, base, baselen);
+	strbuf_add(&path, filename, filenamelen);
 	if (S_ISDIR(mode) || S_ISGITLINK(mode)) {
-		strbuf_append_string(&path, "/");
+		strbuf_addch(&path, '/');
 		buffer = NULL;
 		size = 0;
 	} else {
-- 
1.5.3.1

[PATCH 3/7] Use proper strbuf API, and also simplify cmd_data code.

From: Pierre Habouzit <hidden>
Date: 2016-06-15 22:43:33

  This patch features the use of strbuf_detach, and prevent the programmer
to mess with allocation directly. The code is as efficent as before, just
more concise and more straightforward.
---
 fast-import.c |   30 +++++++++++++-----------------
 1 files changed, 13 insertions(+), 17 deletions(-)
diff --git a/fast-import.c b/fast-import.c
index 2f7baf4..74ff0fd 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -340,7 +340,7 @@ static struct tag *last_tag;
 
 /* Input stream parsing */
 static whenspec_type whenspec = WHENSPEC_RAW;
-static struct strbuf command_buf;
+static struct strbuf command_buf = STRBUF_INIT;
 static int unread_command_buf;
 static struct recent_command cmd_hist = {&cmd_hist, &cmd_hist, NULL};
 static struct recent_command *cmd_tail = &cmd_hist;
@@ -1638,17 +1638,16 @@ static void cmd_mark(void)
 
 static void *cmd_data (size_t *size)
 {
-	size_t length;
-	char *buffer;
+	struct strbuf buffer;
 
+	strbuf_init(&buffer);
 	if (prefixcmp(command_buf.buf, "data "))
 		die("Expected 'data n' command, found: %s", command_buf.buf);
 
 	if (!prefixcmp(command_buf.buf + 5, "<<")) {
 		char *term = xstrdup(command_buf.buf + 5 + 2);
-		size_t sz = 8192, term_len = command_buf.len - 5 - 2;
-		length = 0;
-		buffer = xmalloc(sz);
+		size_t term_len = command_buf.len - 5 - 2;
+
 		for (;;) {
 			read_line(&command_buf, stdin, '\n');
 			if (command_buf.eof)
@@ -1656,21 +1655,18 @@ static void *cmd_data (size_t *size)
 			if (term_len == command_buf.len
 				&& !strcmp(term, command_buf.buf))
 				break;
-			ALLOC_GROW(buffer, length + command_buf.len + 1, sz);
-			memcpy(buffer + length,
-				command_buf.buf,
-				command_buf.len);
-			length += command_buf.len;
-			buffer[length++] = '\n';
+			strbuf_addbuf(&buffer, &command_buf);
+			strbuf_addch(&buffer, '\n');
 		}
 		free(term);
 	}
 	else {
-		size_t n = 0;
+		size_t n = 0, length;
+
 		length = strtoul(command_buf.buf + 5, NULL, 10);
-		buffer = xmalloc(length);
+
 		while (n < length) {
-			size_t s = fread(buffer + n, 1, length - n, stdin);
+			size_t s = strbuf_fread(&buffer, length - n, stdin);
 			if (!s && feof(stdin))
 				die("EOF in data (%lu bytes remaining)",
 					(unsigned long)(length - n));
@@ -1679,8 +1675,8 @@ static void *cmd_data (size_t *size)
 	}
 
 	skip_optional_lf();
-	*size = length;
-	return buffer;
+	*size = buffer.len;
+	return strbuf_detach(&buffer);
 }
 
 static int validate_raw_date(const char *src, char *result, int maxlen)
-- 
1.5.3.1

[PATCH 6/7] Eradicate yet-another-buffer implementation in buitin-rerere.c

From: Pierre Habouzit <hidden>
Date: 2016-06-15 22:43:33

Signed-off-by: Pierre Habouzit <redacted>
---
 builtin-rerere.c |   56 +++++++++++++++++------------------------------------
 1 files changed, 18 insertions(+), 38 deletions(-)
diff --git a/builtin-rerere.c b/builtin-rerere.c
index 29d057c..7ebf6f1 100644
--- a/builtin-rerere.c
+++ b/builtin-rerere.c
@@ -1,6 +1,7 @@
 #include "builtin.h"
 #include "cache.h"
 #include "path-list.h"
+#include "strbuf.h"
 #include "xdiff/xdiff.h"
 #include "xdiff-interface.h"
 
@@ -66,41 +67,20 @@ static int write_rr(struct path_list *rr, int out_fd)
 	return commit_lock_file(&write_lock);
 }
 
-struct buffer {
-	char *ptr;
-	int nr, alloc;
-};
-
-static void append_line(struct buffer *buffer, const char *line)
-{
-	int len = strlen(line);
-
-	if (buffer->nr + len > buffer->alloc) {
-		buffer->alloc = alloc_nr(buffer->nr + len);
-		buffer->ptr = xrealloc(buffer->ptr, buffer->alloc);
-	}
-	memcpy(buffer->ptr + buffer->nr, line, len);
-	buffer->nr += len;
-}
-
-static void clear_buffer(struct buffer *buffer)
-{
-	free(buffer->ptr);
-	buffer->ptr = NULL;
-	buffer->nr = buffer->alloc = 0;
-}
-
 static int handle_file(const char *path,
 	 unsigned char *sha1, const char *output)
 {
 	SHA_CTX ctx;
 	char buf[1024];
 	int hunk = 0, hunk_no = 0;
-	struct buffer minus = { NULL, 0, 0 }, plus = { NULL, 0, 0 };
-	struct buffer *one = &minus, *two = &plus;
+	struct strbuf minus, plus;
+	struct strbuf *one = &minus, *two = &plus;
 	FILE *f = fopen(path, "r");
 	FILE *out;
 
+        strbuf_init(&minus);
+        strbuf_init(&plus);
+
 	if (!f)
 		return error("Could not open %s", path);
 
@@ -122,36 +102,36 @@ static int handle_file(const char *path,
 		else if (!prefixcmp(buf, "======="))
 			hunk = 2;
 		else if (!prefixcmp(buf, ">>>>>>> ")) {
-			int one_is_longer = (one->nr > two->nr);
-			int common_len = one_is_longer ? two->nr : one->nr;
-			int cmp = memcmp(one->ptr, two->ptr, common_len);
+			int one_is_longer = (one->len > two->len);
+			int common_len = one_is_longer ? two->len : one->len;
+			int cmp = memcmp(one->buf, two->buf, common_len);
 
 			hunk_no++;
 			hunk = 0;
 			if ((cmp > 0) || ((cmp == 0) && one_is_longer)) {
-				struct buffer *swap = one;
+				struct strbuf *swap = one;
 				one = two;
 				two = swap;
 			}
 			if (out) {
 				fputs("<<<<<<<\n", out);
-				fwrite(one->ptr, one->nr, 1, out);
+				fwrite(one->buf, one->len, 1, out);
 				fputs("=======\n", out);
-				fwrite(two->ptr, two->nr, 1, out);
+				fwrite(two->buf, two->len, 1, out);
 				fputs(">>>>>>>\n", out);
 			}
 			if (sha1) {
-				SHA1_Update(&ctx, one->ptr, one->nr);
+				SHA1_Update(&ctx, one->buf, one->len);
 				SHA1_Update(&ctx, "\0", 1);
-				SHA1_Update(&ctx, two->ptr, two->nr);
+				SHA1_Update(&ctx, two->buf, two->len);
 				SHA1_Update(&ctx, "\0", 1);
 			}
-			clear_buffer(one);
-			clear_buffer(two);
+			strbuf_release(one);
+			strbuf_release(two);
 		} else if (hunk == 1)
-			append_line(one, buf);
+			strbuf_addstr(one, buf);
 		else if (hunk == 2)
-			append_line(two, buf);
+			strbuf_addstr(two, buf);
 		else if (out)
 			fputs(buf, out);
 	}
-- 
1.5.3.1

[PATCH 1/7] Rework strbuf API and semantics.

From: Pierre Habouzit <hidden>
Date: 2016-06-15 22:43:33

  The gory details are explained in strbuf.h. The change of semantics this
patch enforces is that the embeded buffer has always a '\0' character after
its last byte, to always make it a C-string. The offs-by-one changes are all
related to that very change.

  A strbuf can be used to store byte arrays, or as an extended string
library. The `buf' member can be passed to any C legacy string function,
because strbuf operations always ensure there is a terminating \0 at the end
of the buffer, not accounted in the `len' field of the structure.

  A strbuf can be used to generate a string/buffer whose final size is not
really known, and then "strbuf_detach" can be used to get the built buffer,
and keep the wrapping "strbuf" structure usable for further work again.

  Other interesting feature: strbuf_grow(sb, size) ensure that there is
enough allocated space in `sb' to put `size' new octets of data in the
buffer. It helps avoiding reallocating data for nothing when the problem the
strbuf helps to solve has a known typical size.

Signed-off-by: Pierre Habouzit <redacted>
---
 archive-tar.c |    2 +-
 fast-import.c |   15 ++++----
 mktree.c      |    4 +--
 strbuf.c      |  102 +++++++++++++++++++++++++++++++++++++++++++++++++--------
 strbuf.h      |   86 +++++++++++++++++++++++++++++++++++++++++++++++-
 5 files changed, 181 insertions(+), 28 deletions(-)
diff --git a/archive-tar.c b/archive-tar.c
index 66fe3e3..a0763c5 100644
--- a/archive-tar.c
+++ b/archive-tar.c
@@ -166,7 +166,7 @@ static void write_entry(const unsigned char *sha1, struct strbuf *path,
 		sprintf(header.name, "%s.paxheader", sha1_to_hex(sha1));
 	} else {
 		if (verbose)
-			fprintf(stderr, "%.*s\n", path->len, path->buf);
+			fprintf(stderr, "%.*s\n", (int)path->len, path->buf);
 		if (S_ISDIR(mode) || S_ISGITLINK(mode)) {
 			*header.typeflag = TYPEFLAG_DIR;
 			mode = (mode | 0777) & ~tar_umask;
diff --git a/fast-import.c b/fast-import.c
index 078079d..2f7baf4 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1595,7 +1595,7 @@ static void read_next_command(void)
 		} else {
 			struct recent_command *rc;
 
-			command_buf.buf = NULL;
+			strbuf_detach(&command_buf);
 			read_line(&command_buf, stdin, '\n');
 			if (command_buf.eof)
 				return;
@@ -1649,7 +1649,6 @@ static void *cmd_data (size_t *size)
 		size_t sz = 8192, term_len = command_buf.len - 5 - 2;
 		length = 0;
 		buffer = xmalloc(sz);
-		command_buf.buf = NULL;
 		for (;;) {
 			read_line(&command_buf, stdin, '\n');
 			if (command_buf.eof)
@@ -1657,11 +1656,11 @@ static void *cmd_data (size_t *size)
 			if (term_len == command_buf.len
 				&& !strcmp(term, command_buf.buf))
 				break;
-			ALLOC_GROW(buffer, length + command_buf.len, sz);
+			ALLOC_GROW(buffer, length + command_buf.len + 1, sz);
 			memcpy(buffer + length,
 				command_buf.buf,
-				command_buf.len - 1);
-			length += command_buf.len - 1;
+				command_buf.len);
+			length += command_buf.len;
 			buffer[length++] = '\n';
 		}
 		free(term);
@@ -2101,7 +2100,7 @@ static void cmd_new_commit(void)
 	}
 
 	/* file_change* */
-	while (!command_buf.eof && command_buf.len > 1) {
+	while (!command_buf.eof && command_buf.len > 0) {
 		if (!prefixcmp(command_buf.buf, "M "))
 			file_change_m(b);
 		else if (!prefixcmp(command_buf.buf, "D "))
@@ -2256,7 +2255,7 @@ static void cmd_reset_branch(void)
 	else
 		b = new_branch(sp);
 	read_next_command();
-	if (!cmd_from(b) && command_buf.len > 1)
+	if (!cmd_from(b) && command_buf.len > 0)
 		unread_command_buf = 1;
 }
 
@@ -2273,7 +2272,7 @@ static void cmd_checkpoint(void)
 
 static void cmd_progress(void)
 {
-	fwrite(command_buf.buf, 1, command_buf.len - 1, stdout);
+	fwrite(command_buf.buf, 1, command_buf.len, stdout);
 	fputc('\n', stdout);
 	fflush(stdout);
 	skip_optional_lf();
diff --git a/mktree.c b/mktree.c
index d86dde8..86de5eb 100644
--- a/mktree.c
+++ b/mktree.c
@@ -92,7 +92,6 @@ int main(int ac, char **av)
 
 	strbuf_init(&sb);
 	while (1) {
-		int len;
 		char *ptr, *ntr;
 		unsigned mode;
 		enum object_type type;
@@ -101,7 +100,6 @@ int main(int ac, char **av)
 		read_line(&sb, stdin, line_termination);
 		if (sb.eof)
 			break;
-		len = sb.len;
 		ptr = sb.buf;
 		/* Input is non-recursive ls-tree output format
 		 * mode SP type SP sha1 TAB name
@@ -111,7 +109,7 @@ int main(int ac, char **av)
 			die("input format error: %s", sb.buf);
 		ptr = ntr + 1; /* type */
 		ntr = strchr(ptr, ' ');
-		if (!ntr || sb.buf + len <= ntr + 41 ||
+		if (!ntr || sb.buf + sb.len <= ntr + 40 ||
 		    ntr[41] != '\t' ||
 		    get_sha1_hex(ntr + 1, sha1))
 			die("input format error: %s", sb.buf);
diff --git a/strbuf.c b/strbuf.c
index e33d06b..acc7fc8 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -2,40 +2,114 @@
 #include "strbuf.h"
 
 void strbuf_init(struct strbuf *sb) {
-	sb->buf = NULL;
-	sb->eof = sb->alloc = sb->len = 0;
+	memset(sb, 0, sizeof(*sb));
 }
 
-static void strbuf_begin(struct strbuf *sb) {
+void strbuf_release(struct strbuf *sb) {
 	free(sb->buf);
+	memset(sb, 0, sizeof(*sb));
+}
+
+void strbuf_reset(struct strbuf *sb) {
+	if (sb->len)
+		strbuf_setlen(sb, 0);
+	sb->eof = 0;
+}
+
+char *strbuf_detach(struct strbuf *sb) {
+	char *res = sb->buf;
 	strbuf_init(sb);
+	return res;
+}
+
+void strbuf_grow(struct strbuf *sb, size_t extra) {
+	if (sb->len + extra + 1 <= sb->len)
+		die("you want to use way too much memory");
+	ALLOC_GROW(sb->buf, sb->len + extra + 1, sb->alloc);
 }
 
-static void inline strbuf_add(struct strbuf *sb, int ch) {
-	if (sb->alloc <= sb->len) {
-		sb->alloc = sb->alloc * 3 / 2 + 16;
-		sb->buf = xrealloc(sb->buf, sb->alloc);
+void strbuf_add(struct strbuf *sb, const void *data, size_t len) {
+	strbuf_grow(sb, len);
+	memcpy(sb->buf + sb->len, data, len);
+	strbuf_setlen(sb, sb->len + len);
+}
+
+void strbuf_addf(struct strbuf *sb, const char *fmt, ...) {
+	int len;
+	va_list ap;
+
+	va_start(ap, fmt);
+	len = vsnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, ap);
+	va_end(ap);
+	if (len < 0) {
+		len = 0;
 	}
-	sb->buf[sb->len++] = ch;
+	if (len >= strbuf_avail(sb)) {
+		strbuf_grow(sb, len);
+		va_start(ap, fmt);
+		len = vsnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, ap);
+		va_end(ap);
+		if (len >= strbuf_avail(sb)) {
+			die("this should not happen, your snprintf is broken");
+		}
+	}
+	strbuf_setlen(sb, sb->len + len);
 }
 
-static void strbuf_end(struct strbuf *sb) {
-	strbuf_add(sb, 0);
+size_t strbuf_fread(struct strbuf *sb, size_t size, FILE *f) {
+	size_t res;
+
+	strbuf_grow(sb, size);
+	res = fread(sb->buf + sb->len, 1, size, f);
+	if (res > 0) {
+		strbuf_setlen(sb, sb->len + res);
+	}
+	return res;
+}
+
+ssize_t strbuf_read(struct strbuf *sb, int fd)
+{
+	size_t oldlen = sb->len;
+
+	for (;;) {
+		ssize_t cnt;
+
+		strbuf_grow(sb, 8192);
+		cnt = xread(fd, sb->buf + sb->len, sb->alloc - sb->len - 1);
+		if (cnt < 0) {
+			strbuf_setlen(sb, oldlen);
+			return -1;
+		}
+		if (!cnt)
+			break;
+		sb->len += cnt;
+	}
+
+	sb->buf[sb->len] = '\0';
+	return sb->len - oldlen;
 }
 
 void read_line(struct strbuf *sb, FILE *fp, int term) {
 	int ch;
-	strbuf_begin(sb);
 	if (feof(fp)) {
+		strbuf_release(sb);
 		sb->eof = 1;
 		return;
 	}
+
+	strbuf_reset(sb);
 	while ((ch = fgetc(fp)) != EOF) {
 		if (ch == term)
 			break;
-		strbuf_add(sb, ch);
+		strbuf_grow(sb, 1);
+		sb->buf[sb->len++] = ch;
 	}
-	if (ch == EOF && sb->len == 0)
+	if (ch == EOF && sb->len == 0) {
+		strbuf_release(sb);
 		sb->eof = 1;
-	strbuf_end(sb);
+	}
+
+	strbuf_grow(sb, 1);
+	sb->buf[sb->len] = '\0';
 }
+
diff --git a/strbuf.h b/strbuf.h
index 74cc012..b40dc99 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -1,13 +1,95 @@
 #ifndef STRBUF_H
 #define STRBUF_H
+
+/*
+ * Strbuf's can be use in many ways: as a byte array, or to store arbitrary
+ * long, overflow safe strings.
+ *
+ * Strbufs has some invariants that are very important to keep in mind:
+ *
+ * 1. the ->buf member is always malloc-ed, hence strbuf's can be used to
+ *    build complex strings/buffers whose final size isn't easily known.
+ *
+ *    It is legal to copy the ->buf pointer away. Though if you want to reuse
+ *    the strbuf after that, setting ->buf to NULL isn't legal.
+ *    `strbuf_detach' is the operation that detachs a buffer from its shell
+ *    while keeping the shell valid wrt its invariants.
+ *
+ * 2. the ->buf member is a byte array that has at least ->len + 1 bytes
+ *    allocated. The extra byte is used to store a '\0', allowing the ->buf
+ *    member to be a valid C-string. Every strbuf function ensure this
+ *    invariant is preserved.
+ *
+ *    Note that it is OK to "play" with the buffer directly if you work it
+ *    that way:
+ *
+ *    strbuf_grow(sb, SOME_SIZE);
+ *    // ... here the memory areay starting at sb->buf, and of length
+ *    // sb_avail(sb) is all yours, and you are sure that sb_avail(sb) is at
+ *    // least SOME_SIZE
+ *    strbuf_setlen(sb, sb->len + SOME_OTHER_SIZE);
+ *
+ *    Of course, SOME_OTHER_SIZE must be smaller or equal to sb_avail(sb).
+ *
+ *    Doing so is safe, though if it has to be done in many places, adding the
+ *    missing API to the strbuf module is the way to go.
+ *
+ *    XXX: do _not_ assume that the area that is yours is of size ->alloc - 1
+ *         even if it's true in the current implementation. Alloc is somehow a
+ *         "private" member that should not be messed with.
+ */
+
+#include <assert.h>
+
 struct strbuf {
-	int alloc;
-	int len;
+	size_t alloc;
+	size_t len;
 	int eof;
 	char *buf;
 };
 
+#define STRBUF_INIT  { 0, 0, 0, NULL }
+
+/*----- strbuf life cycle -----*/
 extern void strbuf_init(struct strbuf *);
+extern void strbuf_release(struct strbuf *);
+extern void strbuf_reset(struct strbuf *);
+extern char *strbuf_detach(struct strbuf *);
+
+/*----- strbuf size related -----*/
+static inline size_t strbuf_avail(struct strbuf *sb) {
+    return sb->alloc ? sb->alloc - sb->len - 1 : 0;
+}
+static inline void strbuf_setlen(struct strbuf *sb, size_t len) {
+    assert (len < sb->alloc);
+    sb->len = len;
+    sb->buf[len] = '\0';
+}
+
+extern void strbuf_grow(struct strbuf *, size_t);
+
+/*----- add data in your buffer -----*/
+static inline void strbuf_addch(struct strbuf *sb, int c) {
+	strbuf_grow(sb, 1);
+	sb->buf[sb->len++] = c;
+	sb->buf[sb->len] = '\0';
+}
+
+extern void strbuf_add(struct strbuf *, const void *, size_t);
+static inline void strbuf_addstr(struct strbuf *sb, const char *s) {
+	strbuf_add(sb, s, strlen(s));
+}
+static inline void strbuf_addbuf(struct strbuf *sb, struct strbuf *sb2) {
+	strbuf_add(sb, sb2->buf, sb2->len);
+}
+
+__attribute__((format(printf,2,3)))
+extern void strbuf_addf(struct strbuf *sb, const char *fmt, ...);
+
+extern size_t strbuf_fread(struct strbuf *, size_t, FILE *);
+/* XXX: if read fails, any partial read is undone */
+extern ssize_t strbuf_read(struct strbuf *, int fd);
+
 extern void read_line(struct strbuf *, FILE *, int);
 
 #endif /* STRBUF_H */
-- 
1.5.3.1

[PATCH 7/7] More strbuf uses in cache-tree.c.

From: Pierre Habouzit <hidden>
Date: 2016-06-15 22:43:33

  Should even be marginally faster.

Signed-off-by: Pierre Habouzit <redacted>
---
 cache-tree.c |   59 +++++++++++++++++++++------------------------------------
 1 files changed, 22 insertions(+), 37 deletions(-)
diff --git a/cache-tree.c b/cache-tree.c
index 077f034..76af6f5 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -1,4 +1,5 @@
 #include "cache.h"
+#include "strbuf.h"
 #include "tree.h"
 #include "cache-tree.h"
 
@@ -235,8 +236,7 @@ static int update_one(struct cache_tree *it,
 		      int missing_ok,
 		      int dryrun)
 {
-	unsigned long size, offset;
-	char *buffer;
+	struct strbuf buffer;
 	int i;
 
 	if (0 <= it->entry_count && has_sha1_file(it->sha1))
@@ -293,9 +293,8 @@ static int update_one(struct cache_tree *it,
 	/*
 	 * Then write out the tree object for this level.
 	 */
-	size = 8192;
-	buffer = xmalloc(size);
-	offset = 0;
+	strbuf_init(&buffer);
+	strbuf_grow(&buffer, 8192);
 
 	for (i = 0; i < entries; i++) {
 		struct cache_entry *ce = cache[i];
@@ -332,15 +331,9 @@ static int update_one(struct cache_tree *it,
 		if (!ce->ce_mode)
 			continue; /* entry being removed */
 
-		if (size < offset + entlen + 100) {
-			size = alloc_nr(offset + entlen + 100);
-			buffer = xrealloc(buffer, size);
-		}
-		offset += sprintf(buffer + offset,
-				  "%o %.*s", mode, entlen, path + baselen);
-		buffer[offset++] = 0;
-		hashcpy((unsigned char*)buffer + offset, sha1);
-		offset += 20;
+		strbuf_grow(&buffer, entlen + 100);
+		strbuf_addf(&buffer, "%o %.*s%c", mode, entlen, path + baselen, '\0');
+		strbuf_add(&buffer, sha1, 20);
 
 #if DEBUG
 		fprintf(stderr, "cache-tree update-one %o %.*s\n",
@@ -349,10 +342,10 @@ static int update_one(struct cache_tree *it,
 	}
 
 	if (dryrun)
-		hash_sha1_file(buffer, offset, tree_type, it->sha1);
+		hash_sha1_file(buffer.buf, buffer.len, tree_type, it->sha1);
 	else
-		write_sha1_file(buffer, offset, tree_type, it->sha1);
-	free(buffer);
+		write_sha1_file(buffer.buf, buffer.len, tree_type, it->sha1);
+	strbuf_release(&buffer);
 	it->entry_count = i;
 #if DEBUG
 	fprintf(stderr, "cache-tree update-one (%d ent, %d subtree) %s\n",
@@ -378,12 +371,10 @@ int cache_tree_update(struct cache_tree *it,
 	return 0;
 }
 
-static void *write_one(struct cache_tree *it,
+static void write_one(struct cache_tree *it,
 		       char *path,
 		       int pathlen,
-		       char *buffer,
-		       unsigned long *size,
-		       unsigned long *offset)
+			   struct strbuf *buffer)
 {
 	int i;
 
@@ -393,13 +384,9 @@ static void *write_one(struct cache_tree *it,
 	 * tree-sha1 (missing if invalid)
 	 * subtree_nr "cache-tree" entries for subtrees.
 	 */
-	if (*size < *offset + pathlen + 100) {
-		*size = alloc_nr(*offset + pathlen + 100);
-		buffer = xrealloc(buffer, *size);
-	}
-	*offset += sprintf(buffer + *offset, "%.*s%c%d %d\n",
-			   pathlen, path, 0,
-			   it->entry_count, it->subtree_nr);
+	strbuf_grow(buffer, pathlen + 100);
+	strbuf_add(buffer, path, pathlen);
+	strbuf_addf(buffer, "%c%d %d\n", 0, it->entry_count, it->subtree_nr);
 
 #if DEBUG
 	if (0 <= it->entry_count)
@@ -412,8 +399,7 @@ static void *write_one(struct cache_tree *it,
 #endif
 
 	if (0 <= it->entry_count) {
-		hashcpy((unsigned char*)buffer + *offset, it->sha1);
-		*offset += 20;
+		strbuf_add(buffer, it->sha1, 20);
 	}
 	for (i = 0; i < it->subtree_nr; i++) {
 		struct cache_tree_sub *down = it->down[i];
@@ -423,21 +409,20 @@ static void *write_one(struct cache_tree *it,
 					     prev->name, prev->namelen) <= 0)
 				die("fatal - unsorted cache subtree");
 		}
-		buffer = write_one(down->cache_tree, down->name, down->namelen,
-				   buffer, size, offset);
+		write_one(down->cache_tree, down->name, down->namelen, buffer);
 	}
-	return buffer;
 }
 
 void *cache_tree_write(struct cache_tree *root, unsigned long *size_p)
 {
 	char path[PATH_MAX];
-	unsigned long size = 8192;
-	char *buffer = xmalloc(size);
+	struct strbuf buffer;
 
-	*size_p = 0;
 	path[0] = 0;
-	return write_one(root, path, 0, buffer, &size, size_p);
+	strbuf_init(&buffer);
+	write_one(root, path, 0, &buffer);
+	*size_p = buffer.len;
+	return strbuf_detach(&buffer);
 }
 
 static struct cache_tree *read_one(const char **buffer, unsigned long *size_p)
-- 
1.5.3.1

[PATCH 5/7] Further strbuf re-engineering.

From: Pierre Habouzit <hidden>
Date: 2016-06-15 22:43:33

Signed-off-by: Pierre Habouzit <redacted>
---
 builtin-apply.c       |   30 +++++++-----------------
 builtin-blame.c       |   35 +++++++++------------------
 builtin-commit-tree.c |   60 +++++++++++-------------------------------------
 diff.c                |   27 +++++++--------------
 4 files changed, 44 insertions(+), 108 deletions(-)
diff --git a/builtin-apply.c b/builtin-apply.c
index 25b1447..d70c6cf 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -12,6 +12,7 @@
 #include "blob.h"
 #include "delta.h"
 #include "builtin.h"
+#include "strbuf.h"
 
 /*
  *  --check turns on checking that the working tree matches the
@@ -181,34 +182,21 @@ static void say_patch_name(FILE *output, const char *pre, struct patch *patch, c
 
 static void *read_patch_file(int fd, unsigned long *sizep)
 {
-	unsigned long size = 0, alloc = CHUNKSIZE;
-	void *buffer = xmalloc(alloc);
+	struct strbuf buf;
 
-	for (;;) {
-		ssize_t nr = alloc - size;
-		if (nr < 1024) {
-			alloc += CHUNKSIZE;
-			buffer = xrealloc(buffer, alloc);
-			nr = alloc - size;
-		}
-		nr = xread(fd, (char *) buffer + size, nr);
-		if (!nr)
-			break;
-		if (nr < 0)
-			die("git-apply: read returned %s", strerror(errno));
-		size += nr;
-	}
-	*sizep = size;
+        strbuf_init(&buf);
+	if (strbuf_read(&buf, fd) < 0)
+		die("git-apply: read returned %s", strerror(errno));
+	*sizep = buf.len;
 
 	/*
 	 * Make sure that we have some slop in the buffer
 	 * so that we can do speculative "memcmp" etc, and
 	 * see to it that it is NUL-filled.
 	 */
-	if (alloc < size + SLOP)
-		buffer = xrealloc(buffer, size + SLOP);
-	memset((char *) buffer + size, 0, SLOP);
-	return buffer;
+	strbuf_grow(&buf, SLOP);
+	memset(buf.buf + buf.len, 0, SLOP);
+	return strbuf_detach(&buf);
 }
 
 static unsigned long linelen(const char *buffer, unsigned long size)
diff --git a/builtin-blame.c b/builtin-blame.c
index dc88a95..1b1e6da 100644
--- a/builtin-blame.c
+++ b/builtin-blame.c
@@ -18,6 +18,7 @@
 #include "cache-tree.h"
 #include "path-list.h"
 #include "mailmap.h"
+#include "strbuf.h"
 
 static char blame_usage[] =
 "git-blame [-c] [-b] [-l] [--root] [-t] [-f] [-n] [-s] [-p] [-w] [-L n,m] [-S <revs-file>] [-M] [-C] [-C] [--contents <filename>] [--incremental] [commit] [--] file\n"
@@ -2001,11 +2002,10 @@ static struct commit *fake_working_tree_commit(const char *path, const char *con
 	struct commit *commit;
 	struct origin *origin;
 	unsigned char head_sha1[20];
-	char *buf;
+	struct strbuf buf;
 	const char *ident;
 	int fd;
 	time_t now;
-	unsigned long fin_size;
 	int size, len;
 	struct cache_entry *ce;
 	unsigned mode;
@@ -2023,9 +2023,11 @@ static struct commit *fake_working_tree_commit(const char *path, const char *con
 
 	origin = make_origin(commit, path);
 
+	strbuf_init(&buf);
 	if (!contents_from || strcmp("-", contents_from)) {
 		struct stat st;
 		const char *read_from;
+		unsigned long fin_size;
 
 		if (contents_from) {
 			if (stat(contents_from, &st) < 0)
@@ -2038,19 +2040,19 @@ static struct commit *fake_working_tree_commit(const char *path, const char *con
 			read_from = path;
 		}
 		fin_size = xsize_t(st.st_size);
-		buf = xmalloc(fin_size+1);
 		mode = canon_mode(st.st_mode);
 		switch (st.st_mode & S_IFMT) {
 		case S_IFREG:
 			fd = open(read_from, O_RDONLY);
 			if (fd < 0)
 				die("cannot open %s", read_from);
-			if (read_in_full(fd, buf, fin_size) != fin_size)
+			if (strbuf_read(&buf, fd) != xsize_t(st.st_size))
 				die("cannot read %s", read_from);
 			break;
 		case S_IFLNK:
-			if (readlink(read_from, buf, fin_size+1) != fin_size)
+			if (readlink(read_from, buf.buf, buf.alloc) != fin_size)
 				die("cannot readlink %s", read_from);
+			buf.len = fin_size;
 			break;
 		default:
 			die("unsupported file type %s", read_from);
@@ -2059,26 +2061,13 @@ static struct commit *fake_working_tree_commit(const char *path, const char *con
 	else {
 		/* Reading from stdin */
 		contents_from = "standard input";
-		buf = NULL;
-		fin_size = 0;
 		mode = 0;
-		while (1) {
-			ssize_t cnt = 8192;
-			buf = xrealloc(buf, fin_size + cnt);
-			cnt = xread(0, buf + fin_size, cnt);
-			if (cnt < 0)
-				die("read error %s from stdin",
-				    strerror(errno));
-			if (!cnt)
-				break;
-			fin_size += cnt;
-		}
-		buf = xrealloc(buf, fin_size + 1);
+		if (strbuf_read(&buf, 0) < 0)
+			die("read error %s from stdin", strerror(errno));
 	}
-	buf[fin_size] = 0;
-	origin->file.ptr = buf;
-	origin->file.size = fin_size;
-	pretend_sha1_file(buf, fin_size, OBJ_BLOB, origin->blob_sha1);
+	origin->file.ptr = buf.buf;
+	origin->file.size = buf.len;
+	pretend_sha1_file(buf.buf, buf.len, OBJ_BLOB, origin->blob_sha1);
 	commit->util = origin;
 
 	/*
diff --git a/builtin-commit-tree.c b/builtin-commit-tree.c
index ccbcbe3..bc9502c 100644
--- a/builtin-commit-tree.c
+++ b/builtin-commit-tree.c
@@ -8,42 +8,13 @@
 #include "tree.h"
 #include "builtin.h"
 #include "utf8.h"
+#include "strbuf.h"
 
 #define BLOCKING (1ul << 14)
 
 /*
  * FIXME! Share the code with "write-tree.c"
  */
-static void init_buffer(char **bufp, unsigned int *sizep)
-{
-	*bufp = xmalloc(BLOCKING);
-	*sizep = 0;
-}
-
-static void add_buffer(char **bufp, unsigned int *sizep, const char *fmt, ...)
-{
-	char one_line[2048];
-	va_list args;
-	int len;
-	unsigned long alloc, size, newsize;
-	char *buf;
-
-	va_start(args, fmt);
-	len = vsnprintf(one_line, sizeof(one_line), fmt, args);
-	va_end(args);
-	size = *sizep;
-	newsize = size + len + 1;
-	alloc = (size + 32767) & ~32767;
-	buf = *bufp;
-	if (newsize > alloc) {
-		alloc = (newsize + 32767) & ~32767;
-		buf = xrealloc(buf, alloc);
-		*bufp = buf;
-	}
-	*sizep = newsize - 1;
-	memcpy(buf + size, one_line, len);
-}
-
 static void check_valid(unsigned char *sha1, enum object_type expect)
 {
 	enum object_type type = sha1_object_info(sha1, NULL);
@@ -87,9 +58,7 @@ int cmd_commit_tree(int argc, const char **argv, const char *prefix)
 	int parents = 0;
 	unsigned char tree_sha1[20];
 	unsigned char commit_sha1[20];
-	char comment[1000];
-	char *buffer;
-	unsigned int size;
+	struct strbuf buffer;
 	int encoding_is_utf8;
 
 	git_config(git_default_config);
@@ -118,8 +87,9 @@ int cmd_commit_tree(int argc, const char **argv, const char *prefix)
 	/* Not having i18n.commitencoding is the same as having utf-8 */
 	encoding_is_utf8 = is_encoding_utf8(git_commit_encoding);
 
-	init_buffer(&buffer, &size);
-	add_buffer(&buffer, &size, "tree %s\n", sha1_to_hex(tree_sha1));
+	strbuf_init(&buffer);
+	strbuf_grow(&buffer, 8192); /* should avoid reallocs for the headers */
+	strbuf_addf(&buffer, "tree %s\n", sha1_to_hex(tree_sha1));
 
 	/*
 	 * NOTE! This ordering means that the same exact tree merged with a
@@ -127,26 +97,24 @@ int cmd_commit_tree(int argc, const char **argv, const char *prefix)
 	 * if everything else stays the same.
 	 */
 	for (i = 0; i < parents; i++)
-		add_buffer(&buffer, &size, "parent %s\n", sha1_to_hex(parent_sha1[i]));
+		strbuf_addf(&buffer, "parent %s\n", sha1_to_hex(parent_sha1[i]));
 
 	/* Person/date information */
-	add_buffer(&buffer, &size, "author %s\n", git_author_info(1));
-	add_buffer(&buffer, &size, "committer %s\n", git_committer_info(1));
+	strbuf_addf(&buffer, "author %s\n", git_author_info(1));
+	strbuf_addf(&buffer, "committer %s\n", git_committer_info(1));
 	if (!encoding_is_utf8)
-		add_buffer(&buffer, &size,
-				"encoding %s\n", git_commit_encoding);
-	add_buffer(&buffer, &size, "\n");
+		strbuf_addf(&buffer, "encoding %s\n", git_commit_encoding);
+	strbuf_addch(&buffer, '\n');
 
 	/* And add the comment */
-	while (fgets(comment, sizeof(comment), stdin) != NULL)
-		add_buffer(&buffer, &size, "%s", comment);
+	if (strbuf_read(&buffer, 0) < 0)
+		die("git-commit-tree: read returned %s", strerror(errno));
 
 	/* And check the encoding */
-	buffer[size] = '\0';
-	if (encoding_is_utf8 && !is_utf8(buffer))
+	if (encoding_is_utf8 && !is_utf8(buffer.buf))
 		fprintf(stderr, commit_utf8_warn);
 
-	if (!write_sha1_file(buffer, size, commit_type, commit_sha1)) {
+	if (!write_sha1_file(buffer.buf, buffer.len, commit_type, commit_sha1)) {
 		printf("%s\n", sha1_to_hex(commit_sha1));
 		return 0;
 	}
diff --git a/diff.c b/diff.c
index 0d30d05..c054b23 100644
--- a/diff.c
+++ b/diff.c
@@ -9,6 +9,7 @@
 #include "xdiff-interface.h"
 #include "color.h"
 #include "attr.h"
+#include "strbuf.h"
 
 #ifdef NO_FAST_WORKING_DIRECTORY
 #define FAST_WORKING_DIRECTORY 0
@@ -1545,26 +1546,16 @@ static int reuse_worktree_file(const char *name, const unsigned char *sha1, int
 
 static int populate_from_stdin(struct diff_filespec *s)
 {
-#define INCREMENT 1024
-	char *buf;
-	unsigned long size;
-	ssize_t got;
-
-	size = 0;
-	buf = NULL;
-	while (1) {
-		buf = xrealloc(buf, size + INCREMENT);
-		got = xread(0, buf + size, INCREMENT);
-		if (!got)
-			break; /* EOF */
-		if (got < 0)
-			return error("error while reading from stdin %s",
+	struct strbuf buf;
+
+	strbuf_init(&buf);
+	if (strbuf_read(&buf, 0) < 0)
+		return error("error while reading from stdin %s",
 				     strerror(errno));
-		size += got;
-	}
+
 	s->should_munmap = 0;
-	s->data = buf;
-	s->size = size;
+	s->size = buf.len;
+	s->data = strbuf_detach(&buf);
 	s->should_free = 1;
 	return 0;
 }
-- 
1.5.3.1

Re: strbuf new API, take 2 for inclusion

From: Jeff King <hidden>
Date: 2016-06-15 22:43:33

On Thu, Sep 06, 2007 at 01:20:04PM +0200, Pierre Habouzit wrote:
  I've also stripped as many STRBUF_INIT uses as possible, some people
didn't liked it. I've kept its use for "static" strbufs where it's way
more convenient that a function call.
The STRBUF_INIT initializer just sets everything to '0' or NULL. Static
objects already have this done automagically by the compiler, so there's
no need to use STRBUF_INIT at all there.

-Peff

Re: [PATCH 6/7] Eradicate yet-another-buffer implementation in buitin-rerere.c

From: Johannes Schindelin <hidden>
Date: 2016-06-15 22:43:33

Hi,

On Thu, 6 Sep 2007, Pierre Habouzit wrote:
Signed-off-by: Pierre Habouzit <redacted>
---
 builtin-rerere.c |   56 +++++++++++++++++------------------------------------
 1 files changed, 18 insertions(+), 38 deletions(-)
I like that one very much, but ...
 	FILE *f = fopen(path, "r");
 	FILE *out;
 
+        strbuf_init(&minus);
+        strbuf_init(&plus);
+
You used spaces instead of tabs here.

Ciao,
Dscho

Re: [PATCH 1/7] Rework strbuf API and semantics.

From: Johannes Schindelin <hidden>
Date: 2016-06-15 22:43:33

Hi,

let me thank you for this very nicely done patch series.  Except for 5/7, 
they look pretty much obvious changes to me.  I'll review that in detail 
later.

On Thu, 6 Sep 2007, Pierre Habouzit wrote:
+#define STRBUF_INIT  { 0, 0, 0, NULL }
Would not "struct strbuf sb = { 0 };" have the same effect?  (I am not so 
standards-keen as other people, who I have no doubt will gladly answer 
this one.)

Ciao,
Dscho

Re: [PATCH 1/7] Rework strbuf API and semantics.

From: Jeff King <hidden>
Date: 2016-06-15 22:43:33

On Thu, Sep 06, 2007 at 03:09:28PM +0100, Johannes Schindelin wrote:
let me thank you for this very nicely done patch series.  Except for 5/7, 
they look pretty much obvious changes to me.  I'll review that in detail 
later.
I second that; I am glad somebody is taking an interest in this area
(though I haven't closely reviewed the patches yet).
quoted
+#define STRBUF_INIT  { 0, 0, 0, NULL }
Would not "struct strbuf sb = { 0 };" have the same effect?  (I am not so 
standards-keen as other people, who I have no doubt will gladly answer 
this one.)
Yes, it would, according to the standard.

-Peff

Re: strbuf new API, take 2 for inclusion

From: Pierre Habouzit <hidden>
Date: 2016-06-15 22:43:33

On Thu, Sep 06, 2007 at 12:58:11PM +0000, Jeff King wrote:
On Thu, Sep 06, 2007 at 01:20:04PM +0200, Pierre Habouzit wrote:
quoted
  I've also stripped as many STRBUF_INIT uses as possible, some people
didn't liked it. I've kept its use for "static" strbufs where it's way
more convenient that a function call.
The STRBUF_INIT initializer just sets everything to '0' or NULL. Static
objects already have this done automagically by the compiler, so there's
no need to use STRBUF_INIT at all there.
  Yes, Junio already did that remark. The reason is that it's forward
compatible: if we ever change strbuf's intitial value for some reason,
we would just have to rebuild the code. As junio disliked it (and I'm
not sure I love it either) I've used it where using the _init() function
was impractical.


  And yes { 0 } would have worked the same as per C standard. It's just
that I setup my compiler to flag missing C89 initializers (because it
often detects real errors).

  Here are the whys'

-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

Re: strbuf new API, take 2 for inclusion

From: Jeff King <hidden>
Date: 2016-06-15 22:43:33

On Thu, Sep 06, 2007 at 07:15:02PM +0200, Pierre Habouzit wrote:
  Yes, Junio already did that remark. The reason is that it's forward
compatible: if we ever change strbuf's intitial value for some reason,
we would just have to rebuild the code. As junio disliked it (and I'm
not sure I love it either) I've used it where using the _init() function
was impractical.
OK, I missed that discussion. Thanks for the explanation.

-Peff

Re: [PATCH 6/7] Eradicate yet-another-buffer implementation in buitin-rerere.c

From: Pierre Habouzit <hidden>
Date: 2016-06-15 22:43:33

On Thu, Sep 06, 2007 at 02:05:36PM +0000, Johannes Schindelin wrote:
Hi,

On Thu, 6 Sep 2007, Pierre Habouzit wrote:
quoted
Signed-off-by: Pierre Habouzit <redacted>
---
 builtin-rerere.c |   56 +++++++++++++++++------------------------------------
 1 files changed, 18 insertions(+), 38 deletions(-)
I like that one very much, but ...
quoted
 	FILE *f = fopen(path, "r");
 	FILE *out;
 
+        strbuf_init(&minus);
+        strbuf_init(&plus);
+
You used spaces instead of tabs here.
  crap, and I did that in the 5th patch as well. well, I'll maybe send
privately a "fixed" version of the patch to junio then, to avoid
flooding the list with spacing issues.

  And I'll also set my vim to use tabs when I'm hacking on git.
-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

Re: strbuf new API, take 2 for inclusion

From: Pierre Habouzit <hidden>
Date: 2016-06-15 22:43:33

On Thu, Sep 06, 2007 at 05:16:21PM +0000, Jeff King wrote:
On Thu, Sep 06, 2007 at 07:15:02PM +0200, Pierre Habouzit wrote:
quoted
  Yes, Junio already did that remark. The reason is that it's forward
compatible: if we ever change strbuf's intitial value for some reason,
we would just have to rebuild the code. As junio disliked it (and I'm
not sure I love it either) I've used it where using the _init() function
was impractical.
OK, I missed that discussion. Thanks for the explanation.
  That was on IRC, maybe that's the why :)

-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

Re: [PATCH 1/7] Rework strbuf API and semantics.

From: Kristian Høgsberg <hidden>
Date: 2016-06-15 22:43:33

On Thu, 2007-09-06 at 13:20 +0200, Pierre Habouzit wrote:
The gory details are explained in strbuf.h. The change of semantics this
patch enforces is that the embeded buffer has always a '\0' character after
its last byte, to always make it a C-string. The offs-by-one changes are all
related to that very change.

  A strbuf can be used to store byte arrays, or as an extended string
library. The `buf' member can be passed to any C legacy string function,
because strbuf operations always ensure there is a terminating \0 at the end
of the buffer, not accounted in the `len' field of the structure.

  A strbuf can be used to generate a string/buffer whose final size is not
really known, and then "strbuf_detach" can be used to get the built buffer,
and keep the wrapping "strbuf" structure usable for further work again.

  Other interesting feature: strbuf_grow(sb, size) ensure that there is
enough allocated space in `sb' to put `size' new octets of data in the
buffer. It helps avoiding reallocating data for nothing when the problem the
strbuf helps to solve has a known typical size.
This looks good, should be very useful.

Kristian

Re: [PATCH 2/7] Simplify strbuf uses in archive-tar.c using the proper functions.

From: Kristian Høgsberg <hidden>
Date: 2016-06-15 22:43:33

On Thu, 2007-09-06 at 13:20 +0200, Pierre Habouzit wrote:
quoted hunk
This is just cleaner way to deal with strbufs, using its API rather than
reinventing it in the module (e.g. strbuf_append_string is just the plain
strbuf_addstr function, and it was used to perform what strbuf_addch does
anyways).
---
 archive-tar.c |   65 ++++++++++++++-------------------------------------------
 1 files changed, 16 insertions(+), 49 deletions(-)
diff --git a/archive-tar.c b/archive-tar.c
index a0763c5..c84d7c0 100644
--- a/archive-tar.c
+++ b/archive-tar.c
@@ -78,19 +78,6 @@ static void write_trailer(void)
 	}
 }
 
-static void strbuf_append_string(struct strbuf *sb, const char *s)
-{
-	int slen = strlen(s);
-	int total = sb->len + slen;
-	if (total + 1 > sb->alloc) {
-		sb->buf = xrealloc(sb->buf, total + 1);
-		sb->alloc = total + 1;
-	}
-	memcpy(sb->buf + sb->len, s, slen);
-	sb->len = total;
-	sb->buf[total] = '\0';
-}
-
 /*
  * pax extended header records have the format "%u %s=%s\n".  %u contains
  * the size of the whole string (including the %u), the first %s is the
@@ -100,26 +87,17 @@ static void strbuf_append_string(struct strbuf *sb, const char *s)
 static void strbuf_append_ext_header(struct strbuf *sb, const char *keyword,
                                      const char *value, unsigned int valuelen)
 {
-	char *p;
-	int len, total, tmp;
+	int len, tmp;
 
 	/* "%u %s=%s\n" */
 	len = 1 + 1 + strlen(keyword) + 1 + valuelen + 1;
 	for (tmp = len; tmp > 9; tmp /= 10)
 		len++;
 
-	total = sb->len + len;
-	if (total > sb->alloc) {
-		sb->buf = xrealloc(sb->buf, total);
-		sb->alloc = total;
-	}
-
-	p = sb->buf;
-	p += sprintf(p, "%u %s=", len, keyword);
-	memcpy(p, value, valuelen);
-	p += valuelen;
-	*p = '\n';
-	sb->len = total;
+	strbuf_grow(sb, len);
+	strbuf_addf(sb, "%u %s=", len, keyword);
+	strbuf_add(sb, value, valuelen);
+	strbuf_addch(sb, '\n');
 }
This entire function can be collapsed to just:

	strbuf_addf(sb, "%u %s=%.*s\n", len, keyword, valuelen, value);
quoted hunk
 
 static unsigned int ustar_header_chksum(const struct ustar_header *header)
@@ -153,8 +131,7 @@ static void write_entry(const unsigned char *sha1, struct strbuf *path,
 	struct strbuf ext_header;
 
 	memset(&header, 0, sizeof(header));
-	ext_header.buf = NULL;
-	ext_header.len = ext_header.alloc = 0;
+	strbuf_init(&ext_header);
Just use your STRBUF_INIT macro?
quoted hunk
 	if (!sha1) {
 		*header.typeflag = TYPEFLAG_GLOBAL_HEADER;
@@ -225,8 +202,8 @@ static void write_entry(const unsigned char *sha1, struct strbuf *path,
 
 	if (ext_header.len > 0) {
 		write_entry(sha1, NULL, 0, ext_header.buf, ext_header.len);
-		free(ext_header.buf);
 	}
Remove excess braces?
quoted hunk
+	strbuf_release(&ext_header);
 	write_blocked(&header, sizeof(header));
 	if (S_ISREG(mode) && buffer && size > 0)
 		write_blocked(buffer, size);
@@ -235,11 +212,11 @@ static void write_entry(const unsigned char *sha1, struct strbuf *path,
 static void write_global_extended_header(const unsigned char *sha1)
 {
 	struct strbuf ext_header;
-	ext_header.buf = NULL;
-	ext_header.len = ext_header.alloc = 0;
+
+	strbuf_init(&ext_header);
STRBUF_INIT macro?
quoted hunk
 	strbuf_append_ext_header(&ext_header, "comment", sha1_to_hex(sha1), 40);
 	write_entry(NULL, NULL, 0, ext_header.buf, ext_header.len);
-	free(ext_header.buf);
+	strbuf_release(&ext_header);
 }
 
 static int git_tar_config(const char *var, const char *value)
@@ -260,28 +237,18 @@ static int write_tar_entry(const unsigned char *sha1,
                            const char *base, int baselen,
                            const char *filename, unsigned mode, int stage)
 {
-	static struct strbuf path;
+	static struct strbuf path = STRBUF_INIT;
 	int filenamelen = strlen(filename);
 	void *buffer;
 	enum object_type type;
 	unsigned long size;
 
-	if (!path.alloc) {
-		path.buf = xmalloc(PATH_MAX);
-		path.alloc = PATH_MAX;
-		path.len = path.eof = 0;
-	}
-	if (path.alloc < baselen + filenamelen + 1) {
-		free(path.buf);
-		path.buf = xmalloc(baselen + filenamelen + 1);
-		path.alloc = baselen + filenamelen + 1;
-	}
-	memcpy(path.buf, base, baselen);
-	memcpy(path.buf + baselen, filename, filenamelen);
-	path.len = baselen + filenamelen;
-	path.buf[path.len] = '\0';
+	strbuf_grow(&path, MAX(PATH_MAX, baselen + filenamelen + 1));
+	strbuf_reset(&path);
Does strbuf_reset() do anything here?
+	strbuf_add(&path, base, baselen);
+	strbuf_add(&path, filename, filenamelen);
 	if (S_ISDIR(mode) || S_ISGITLINK(mode)) {
-		strbuf_append_string(&path, "/");
+		strbuf_addch(&path, '/');
 		buffer = NULL;
 		size = 0;
 	} else {

Re: [PATCH 2/7] Simplify strbuf uses in archive-tar.c using the proper functions.

From: Pierre Habouzit <hidden>
Date: 2016-06-15 22:43:33

On Thu, Sep 06, 2007 at 05:59:29PM +0000, Kristian Høgsberg wrote:
On Thu, 2007-09-06 at 13:20 +0200, Pierre Habouzit wrote:
quoted
+	strbuf_grow(sb, len);
+	strbuf_addf(sb, "%u %s=", len, keyword);
+	strbuf_add(sb, value, valuelen);
+	strbuf_addch(sb, '\n');
 }
This entire function can be collapsed to just:

	strbuf_addf(sb, "%u %s=%.*s\n", len, keyword, valuelen, value);
  yes, but it's less efficient, because %.*s says that sprintf must copy
at most valuelen bytes from value, but it still has to stop if it finds
a \0 before. And the strbuf_grow has sense because the extend policy at
snprintf time is optimistic: we try to write, and if it didn't fit, we
try again. So there is a huge benefit if we have a clue of the final
size.

  I would not change a thing.
quoted
+	strbuf_init(&ext_header);
Just use your STRBUF_INIT macro?
  Many people dilike it, I'm not sure it's a good idea either, and the
performance hit should be negligible, if it's not, then we can still
make the _init() function an inline.
quoted
 	if (ext_header.len > 0) {
 		write_entry(sha1, NULL, 0, ext_header.buf, ext_header.len);
-		free(ext_header.buf);
 	}
Remove excess braces?
  bah, I don't like to strip braces so I won't do that, else you end up
with stupidities like:

  if (foo)
    // bar();

  do_some_very_important_stuff();

  Call me paranoid but well, it saved me so many times ...
quoted
-	memcpy(path.buf, base, baselen);
-	memcpy(path.buf + baselen, filename, filenamelen);
-	path.len = baselen + filenamelen;
-	path.buf[path.len] = '\0';
+	strbuf_grow(&path, MAX(PATH_MAX, baselen + filenamelen + 1));
+	strbuf_reset(&path);
Does strbuf_reset() do anything here?
quoted
+	strbuf_add(&path, base, baselen);
  Yes _reset() sets length to 0. so the add here will write at the start
of the buffer again. It definitely is important !

-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

Re: [PATCH 2/7] Simplify strbuf uses in archive-tar.c using the proper functions.

From: Kristian Høgsberg <hidden>
Date: 2016-06-15 22:43:34

On Thu, 2007-09-06 at 20:08 +0200, Pierre Habouzit wrote:
On Thu, Sep 06, 2007 at 05:59:29PM +0000, Kristian Høgsberg wrote:
quoted
On Thu, 2007-09-06 at 13:20 +0200, Pierre Habouzit wrote:
quoted
+	strbuf_grow(sb, len);
+	strbuf_addf(sb, "%u %s=", len, keyword);
+	strbuf_add(sb, value, valuelen);
+	strbuf_addch(sb, '\n');
 }
This entire function can be collapsed to just:

	strbuf_addf(sb, "%u %s=%.*s\n", len, keyword, valuelen, value);
  yes, but it's less efficient, because %.*s says that sprintf must copy
at most valuelen bytes from value, but it still has to stop if it finds
a \0 before. And the strbuf_grow has sense because the extend policy at
snprintf time is optimistic: we try to write, and if it didn't fit, we
try again. So there is a huge benefit if we have a clue of the final
size.
Yeah, my strbuf_addf() suggestion doesn't work, since the format seems
to indicate the embedded NULs indeed is allowed (the %u is the length of
the value), so *.% could truncate data.
quoted
quoted
-	memcpy(path.buf, base, baselen);
-	memcpy(path.buf + baselen, filename, filenamelen);
-	path.len = baselen + filenamelen;
-	path.buf[path.len] = '\0';
+	strbuf_grow(&path, MAX(PATH_MAX, baselen + filenamelen + 1));
+	strbuf_reset(&path);
Does strbuf_reset() do anything here?
quoted
+	strbuf_add(&path, base, baselen);
  Yes _reset() sets length to 0. so the add here will write at the start
of the buffer again. It definitely is important !
But where was length set to non-zero?  path is initialized on entry to
the function, and strbuf_grow() just increases the allocation, not
length, right?

Kristian

Re: [PATCH 2/7] Simplify strbuf uses in archive-tar.c using the proper functions.

From: Pierre Habouzit <hidden>
Date: 2016-06-15 22:43:34

On Thu, Sep 06, 2007 at 06:18:34PM +0000, Kristian Høgsberg wrote:
On Thu, 2007-09-06 at 20:08 +0200, Pierre Habouzit wrote:
quoted
On Thu, Sep 06, 2007 at 05:59:29PM +0000, Kristian Høgsberg wrote:
quoted
On Thu, 2007-09-06 at 13:20 +0200, Pierre Habouzit wrote:
quoted
-	memcpy(path.buf, base, baselen);
-	memcpy(path.buf + baselen, filename, filenamelen);
-	path.len = baselen + filenamelen;
-	path.buf[path.len] = '\0';
+	strbuf_grow(&path, MAX(PATH_MAX, baselen + filenamelen + 1));
+	strbuf_reset(&path);
Does strbuf_reset() do anything here?
quoted
+	strbuf_add(&path, base, baselen);
  Yes _reset() sets length to 0. so the add here will write at the start
of the buffer again. It definitely is important !
But where was length set to non-zero?  path is initialized on entry to
the function, and strbuf_grow() just increases the allocation, not
length, right?
  The path is static, hence when you reenter the function you have the
last value in it. The fact that it's static may be questionable, but it
was like it before, I kept it, I've supposed it was for performance
reasons.

-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

Re: [PATCH 2/7] Simplify strbuf uses in archive-tar.c using the proper functions.

From: René Scharfe <hidden>
Date: 2016-06-15 22:43:34

Pierre Habouzit schrieb:
On Thu, Sep 06, 2007 at 06:18:34PM +0000, Kristian Høgsberg wrote:
quoted
On Thu, 2007-09-06 at 20:08 +0200, Pierre Habouzit wrote:
quoted
On Thu, Sep 06, 2007 at 05:59:29PM +0000, Kristian Høgsberg wrote:
quoted
On Thu, 2007-09-06 at 13:20 +0200, Pierre Habouzit wrote:
quoted
-	memcpy(path.buf, base, baselen);
-	memcpy(path.buf + baselen, filename, filenamelen);
-	path.len = baselen + filenamelen;
-	path.buf[path.len] = '\0';
+	strbuf_grow(&path, MAX(PATH_MAX, baselen + filenamelen + 1));
+	strbuf_reset(&path);
Does strbuf_reset() do anything here?
quoted
+	strbuf_add(&path, base, baselen);
  Yes _reset() sets length to 0. so the add here will write at the start
of the buffer again. It definitely is important !
But where was length set to non-zero?  path is initialized on entry to
the function, and strbuf_grow() just increases the allocation, not
length, right?
  The path is static, hence when you reenter the function you have the
last value in it. The fact that it's static may be questionable, but it
was like it before, I kept it, I've supposed it was for performance
reasons.
You're right, I've made it static to get the number of mallocs from
number-of-files-in-archive down to one.  It was a ~10% speedup in the
warm cache case, so I could not resist. :)

René

Re: [PATCH 6/7] Eradicate yet-another-buffer implementation in buitin-rerere.c

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:43:34

Pierre Habouzit [off-list ref] writes:
  crap, and I did that in the 5th patch as well. well, I'll maybe send
privately a "fixed" version of the patch to junio then, to avoid
flooding the list with spacing issues.
Heh, no need.  I can fix them up locally here.

For me personally, your use of Mail-Followup-To: header is very
much more irritating.  When I want to talk to you, I want my MUA
to put YOUR name on To: header, not Johannes.  I consider it
rude and selfish to use M-F-T to avoid getting duplicates from a
mailing list you subscribe to.

I have said this often enough in the past, and have been trying
not to say this unless necessary.  It is sad and happy at the
same time that I still occasionary need to bring this up.

Sad because I have to get irritated enough before I say this,
and happy because it means I found another person who is worth
communicating with and expect to communicate again with on the
list.

Please, don't.

Re: [PATCH 6/7] Eradicate yet-another-buffer implementation in buitin-rerere.c

From: Pierre Habouzit <hidden>
Date: 2016-06-15 22:43:34

On Fri, Sep 07, 2007 at 08:03:28AM +0000, Junio C Hamano wrote:
Pierre Habouzit [off-list ref] writes:
quoted
  crap, and I did that in the 5th patch as well. well, I'll maybe send
privately a "fixed" version of the patch to junio then, to avoid
flooding the list with spacing issues.
Heh, no need.  I can fix them up locally here.
  okay, perfect.
For me personally, your use of Mail-Followup-To: header is very
much more irritating.  When I want to talk to you, I want my MUA
to put YOUR name on To: header, not Johannes.  I consider it
rude and selfish to use M-F-T to avoid getting duplicates from a
mailing list you subscribe to.

I have said this often enough in the past, and have been trying
not to say this unless necessary.  It is sad and happy at the
same time that I still occasionary need to bring this up.

Sad because I have to get irritated enough before I say this,
  I'm sorry, I'll remove it (for the next mail though, I'm too lazy to
change that right now). It's a policy on Debian lists to force M-F-T,
and it's indeed not very practical on lists like the lkml or git. I
could have guessed that myself.
and happy because it means I found another person who is worth
communicating with and expect to communicate again with on the
list.
  heh, thanks :)
Please, don't.
-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

[PATCH 3/3] Rework pretty_print_commit to use strbufs instead of custom buffers.

From: Pierre Habouzit <hidden>
Date: 2016-06-15 22:43:34

  Also remove the "len" parameter, as:
  (1) it was used as a max boundary, and every caller used ~0u
  (2) we check for final NUL no matter what, so it doesn't help for speed.

  As a result most of the pp_* function takes 3 arguments less, and we need
a lot less local variables, this makes the code way more readable, and
easier to extend if needed.

  This patch also fixes some spacing and cosmetic issues.

Signed-off-by: Pierre Habouzit <redacted>
---
 builtin-branch.c      |   15 +--
 builtin-log.c         |   12 +-
 builtin-rev-list.c    |   13 +-
 builtin-show-branch.c |   13 +-
 commit.c              |  330 ++++++++++++++++++-------------------------------
 commit.h              |    6 +-
 log-tree.c            |   56 +++------
 7 files changed, 171 insertions(+), 274 deletions(-)
diff --git a/builtin-branch.c b/builtin-branch.c
index 5f5c182..68cd5f9 100644
--- a/builtin-branch.c
+++ b/builtin-branch.c
@@ -268,23 +268,22 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
 	}
 
 	if (verbose) {
-		char *subject = NULL;
-		unsigned long subject_len = 0;
+		struct strbuf subject;
 		const char *sub = " **** invalid ref ****";
 
+		strbuf_init(&subject);
+
 		commit = lookup_commit(item->sha1);
 		if (commit && !parse_commit(commit)) {
-			pretty_print_commit(CMIT_FMT_ONELINE, commit, ~0,
-					    &subject, &subject_len, 0,
-					    NULL, NULL, 0);
-			sub = subject;
+			pretty_print_commit(CMIT_FMT_ONELINE, commit,
+					    &subject, 0, NULL, NULL, 0);
+			sub = subject.buf;
 		}
 		printf("%c %s%-*s%s %s %s\n", c, branch_get_color(color),
 		       maxwidth, item->name,
 		       branch_get_color(COLOR_BRANCH_RESET),
 		       find_unique_abbrev(item->sha1, abbrev), sub);
-		if (subject)
-			free(subject);
+		strbuf_release(&subject);
 	} else {
 		printf("%c %s%s%s\n", c, branch_get_color(color), item->name,
 		       branch_get_color(COLOR_BRANCH_RESET));
diff --git a/builtin-log.c b/builtin-log.c
index fa81c25..21b35a1 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -763,13 +763,13 @@ int cmd_cherry(int argc, const char **argv, const char *prefix)
 			sign = '-';
 
 		if (verbose) {
-			char *buf = NULL;
-			unsigned long buflen = 0;
-			pretty_print_commit(CMIT_FMT_ONELINE, commit, ~0,
-			                    &buf, &buflen, 0, NULL, NULL, 0);
+			struct strbuf buf;
+			strbuf_init(&buf);
+			pretty_print_commit(CMIT_FMT_ONELINE, commit,
+			                    &buf, 0, NULL, NULL, 0);
 			printf("%c %s %s\n", sign,
-			       sha1_to_hex(commit->object.sha1), buf);
-			free(buf);
+			       sha1_to_hex(commit->object.sha1), buf.buf);
+			strbuf_release(&buf);
 		}
 		else {
 			printf("%c %s\n", sign,
diff --git a/builtin-rev-list.c b/builtin-rev-list.c
index ac551d5..7967f86 100644
--- a/builtin-rev-list.c
+++ b/builtin-rev-list.c
@@ -80,13 +80,12 @@ static void show_commit(struct commit *commit)
 		putchar('\n');
 
 	if (revs.verbose_header) {
-		char *buf = NULL;
-		unsigned long buflen = 0;
-		pretty_print_commit(revs.commit_format, commit, ~0,
-				    &buf, &buflen,
-				    revs.abbrev, NULL, NULL, revs.date_mode);
-		printf("%s%c", buf, hdr_termination);
-		free(buf);
+		struct strbuf buf;
+		strbuf_init(&buf);
+		pretty_print_commit(revs.commit_format, commit,
+					&buf, revs.abbrev, NULL, NULL, revs.date_mode);
+		printf("%s%c", buf.buf, hdr_termination);
+		strbuf_release(&buf);
 	}
 	maybe_flush_or_die(stdout, "stdout");
 	if (commit->parents) {
diff --git a/builtin-show-branch.c b/builtin-show-branch.c
index 4fa87f6..020116f 100644
--- a/builtin-show-branch.c
+++ b/builtin-show-branch.c
@@ -259,16 +259,15 @@ static void join_revs(struct commit_list **list_p,
 
 static void show_one_commit(struct commit *commit, int no_name)
 {
-	char *pretty = NULL;
+	struct strbuf pretty;
 	const char *pretty_str = "(unavailable)";
-	unsigned long pretty_len = 0;
 	struct commit_name *name = commit->util;
 
+	strbuf_init(&pretty);
 	if (commit->object.parsed) {
-		pretty_print_commit(CMIT_FMT_ONELINE, commit, ~0,
-				    &pretty, &pretty_len,
-				    0, NULL, NULL, 0);
-		pretty_str = pretty;
+		pretty_print_commit(CMIT_FMT_ONELINE, commit,
+					&pretty, 0, NULL, NULL, 0);
+		pretty_str = pretty.buf;
 	}
 	if (!prefixcmp(pretty_str, "[PATCH] "))
 		pretty_str += 8;
@@ -289,7 +288,7 @@ static void show_one_commit(struct commit *commit, int no_name)
 			       find_unique_abbrev(commit->object.sha1, 7));
 	}
 	puts(pretty_str);
-	free(pretty);
+	strbuf_release(&pretty);
 }
 
 static char *ref_name[MAX_REVS + 1];
diff --git a/commit.c b/commit.c
index 30db9f4..dafa821 100644
--- a/commit.c
+++ b/commit.c
@@ -458,11 +458,11 @@ void clear_commit_marks(struct commit *commit, unsigned int mark)
 /*
  * Generic support for pretty-printing the header
  */
-static int get_one_line(const char *msg, unsigned long len)
+static int get_one_line(const char *msg)
 {
 	int ret = 0;
 
-	while (len--) {
+	for (;;) {
 		char c = *msg++;
 		if (!c)
 			break;
@@ -485,31 +485,24 @@ static int is_rfc2047_special(char ch)
 	return (non_ascii(ch) || (ch == '=') || (ch == '?') || (ch == '_'));
 }
 
-static int add_rfc2047(char *buf, const char *line, int len,
+static void add_rfc2047(struct strbuf *sb, const char *line, int len,
 		       const char *encoding)
 {
-	char *bp = buf;
-	int i, needquote;
-	char q_encoding[128];
-	const char *q_encoding_fmt = "=?%s?q?";
+	int i, last;
 
-	for (i = needquote = 0; !needquote && i < len; i++) {
+	for (i = 0; i < len; i++) {
 		int ch = line[i];
 		if (non_ascii(ch))
-			needquote++;
-		if ((i + 1 < len) &&
-		    (ch == '=' && line[i+1] == '?'))
-			needquote++;
+			goto needquote;
+		if ((i + 1 < len) && (ch == '=' && line[i+1] == '?'))
+			goto needquote;
 	}
-	if (!needquote)
-		return sprintf(buf, "%.*s", len, line);
-
-	i = snprintf(q_encoding, sizeof(q_encoding), q_encoding_fmt, encoding);
-	if (sizeof(q_encoding) < i)
-		die("Insanely long encoding name %s", encoding);
-	memcpy(bp, q_encoding, i);
-	bp += i;
-	for (i = 0; i < len; i++) {
+	strbuf_add(sb, line, len);
+	return;
+
+needquote:
+	strbuf_addf(sb, "=?%s?q?", encoding);
+	for (i = last = 0; i < len; i++) {
 		unsigned ch = line[i] & 0xFF;
 		/*
 		 * We encode ' ' using '=20' even though rfc2047
@@ -518,15 +511,13 @@ static int add_rfc2047(char *buf, const char *line, int len,
 		 * leave the underscore in place.
 		 */
 		if (is_rfc2047_special(ch) || ch == ' ') {
-			sprintf(bp, "=%02X", ch);
-			bp += 3;
+			strbuf_add(sb, line + last, i - last);
+			strbuf_addf(sb, "=%02X", ch);
+			last = i + 1;
 		}
-		else
-			*bp++ = ch;
 	}
-	memcpy(bp, "?=", 2);
-	bp += 2;
-	return bp - buf;
+	strbuf_add(sb, line + last, len - last);
+	strbuf_addstr(sb, "?=");
 }
 
 static unsigned long bound_rfc2047(unsigned long len, const char *encoding)
@@ -537,21 +528,21 @@ static unsigned long bound_rfc2047(unsigned long len, const char *encoding)
 	return len * 3 + elen + 100;
 }
 
-static int add_user_info(const char *what, enum cmit_fmt fmt, char *buf,
+static void add_user_info(const char *what, enum cmit_fmt fmt, struct strbuf *sb,
 			 const char *line, enum date_mode dmode,
 			 const char *encoding)
 {
 	char *date;
 	int namelen;
 	unsigned long time;
-	int tz, ret;
+	int tz;
 	const char *filler = "    ";
 
 	if (fmt == CMIT_FMT_ONELINE)
-		return 0;
+		return;
 	date = strchr(line, '>');
 	if (!date)
-		return 0;
+		return;
 	namelen = ++date - line;
 	time = strtoul(date, &date, 10);
 	tz = strtol(date, NULL, 10);
@@ -560,42 +551,35 @@ static int add_user_info(const char *what, enum cmit_fmt fmt, char *buf,
 		char *name_tail = strchr(line, '<');
 		int display_name_length;
 		if (!name_tail)
-			return 0;
+			return;
 		while (line < name_tail && isspace(name_tail[-1]))
 			name_tail--;
 		display_name_length = name_tail - line;
 		filler = "";
-		strcpy(buf, "From: ");
-		ret = strlen(buf);
-		ret += add_rfc2047(buf + ret, line, display_name_length,
-				   encoding);
-		memcpy(buf + ret, name_tail, namelen - display_name_length);
-		ret += namelen - display_name_length;
-		buf[ret++] = '\n';
+		strbuf_addstr(sb, "From: ");
+		add_rfc2047(sb, line, display_name_length, encoding);
+		strbuf_add(sb, name_tail, namelen - display_name_length);
+		strbuf_addch(sb, '\n');
 	}
 	else {
-		ret = sprintf(buf, "%s: %.*s%.*s\n", what,
+		strbuf_addf(sb, "%s: %.*s%.*s\n", what,
 			      (fmt == CMIT_FMT_FULLER) ? 4 : 0,
 			      filler, namelen, line);
 	}
 	switch (fmt) {
 	case CMIT_FMT_MEDIUM:
-		ret += sprintf(buf + ret, "Date:   %s\n",
-			       show_date(time, tz, dmode));
+		strbuf_addf(sb, "Date:   %s\n", show_date(time, tz, dmode));
 		break;
 	case CMIT_FMT_EMAIL:
-		ret += sprintf(buf + ret, "Date: %s\n",
-			       show_date(time, tz, DATE_RFC2822));
+		strbuf_addf(sb, "Date: %s\n", show_date(time, tz, DATE_RFC2822));
 		break;
 	case CMIT_FMT_FULLER:
-		ret += sprintf(buf + ret, "%sDate: %s\n", what,
-			       show_date(time, tz, dmode));
+		strbuf_addf(sb, "%sDate: %s\n", what, show_date(time, tz, dmode));
 		break;
 	default:
 		/* notin' */
 		break;
 	}
-	return ret;
 }
 
 static int is_empty_line(const char *line, int *len_p)
@@ -607,16 +591,16 @@ static int is_empty_line(const char *line, int *len_p)
 	return !len;
 }
 
-static int add_merge_info(enum cmit_fmt fmt, char *buf, const struct commit *commit, int abbrev)
+static void add_merge_info(enum cmit_fmt fmt, struct strbuf *sb,
+			const struct commit *commit, int abbrev)
 {
 	struct commit_list *parent = commit->parents;
-	int offset;
 
 	if ((fmt == CMIT_FMT_ONELINE) || (fmt == CMIT_FMT_EMAIL) ||
 	    !parent || !parent->next)
-		return 0;
+		return;
 
-	offset = sprintf(buf, "Merge:");
+	strbuf_addstr(sb, "Merge:");
 
 	while (parent) {
 		struct commit *p = parent->item;
@@ -629,10 +613,9 @@ static int add_merge_info(enum cmit_fmt fmt, char *buf, const struct commit *com
 		dots = (abbrev && strlen(hex) != 40) ?  "..." : "";
 		parent = parent->next;
 
-		offset += sprintf(buf + offset, " %s%s", hex, dots);
+		strbuf_addf(sb, " %s%s", hex, dots);
 	}
-	buf[offset++] = '\n';
-	return offset;
+	strbuf_addch(sb, '\n');
 }
 
 static char *get_header(const struct commit *commit, const char *key)
@@ -787,8 +770,8 @@ static void fill_person(struct interp *table, const char *msg, int len)
 	interp_set_entry(table, 6, show_date(date, tz, DATE_ISO8601));
 }
 
-static long format_commit_message(const struct commit *commit,
-		const char *msg, char **buf_p, unsigned long *space_p)
+static void format_commit_message(const struct commit *commit,
+		const char *msg, struct strbuf *sb)
 {
 	struct interp table[] = {
 		{ "%H" },	/* commit hash */
@@ -841,6 +824,7 @@ static long format_commit_message(const struct commit *commit,
 	};
 	struct commit_list *p;
 	char parents[1024];
+	unsigned long len;
 	int i;
 	enum { HEADER, SUBJECT, BODY } state;
 
@@ -920,20 +904,15 @@ static long format_commit_message(const struct commit *commit,
 		if (!table[i].value)
 			interp_set_entry(table, i, "<unknown>");
 
-	do {
-		char *buf = *buf_p;
-		unsigned long len;
-
-		len = interpolate(buf, *space_p, user_format,
-				    table, ARRAY_SIZE(table));
-		if (len < *space_p)
-			break;
-		ALLOC_GROW(buf, len + 1, *space_p);
-		*buf_p = buf;
-	} while (1);
+	len = interpolate(sb->buf + sb->len, strbuf_avail(sb),
+				user_format, table, ARRAY_SIZE(table));
+	if (len > strbuf_avail(sb)) {
+		strbuf_grow(sb, len);
+		interpolate(sb->buf + sb->len, strbuf_avail(sb) + 1,
+					user_format, table, ARRAY_SIZE(table));
+	}
+	strbuf_setlen(sb, sb->len + len);
 	interp_clear_table(table, ARRAY_SIZE(table));
-
-	return strlen(*buf_p);
 }
 
 static void pp_header(enum cmit_fmt fmt,
@@ -942,34 +921,24 @@ static void pp_header(enum cmit_fmt fmt,
 		      const char *encoding,
 		      const struct commit *commit,
 		      const char **msg_p,
-		      unsigned long *len_p,
-		      unsigned long *ofs_p,
-		      char **buf_p,
-		      unsigned long *space_p)
+		      struct strbuf *sb)
 {
 	int parents_shown = 0;
 
 	for (;;) {
 		const char *line = *msg_p;
-		char *dst;
-		int linelen = get_one_line(*msg_p, *len_p);
-		unsigned long len;
+		int linelen = get_one_line(*msg_p);
 
 		if (!linelen)
 			return;
 		*msg_p += linelen;
-		*len_p -= linelen;
 
 		if (linelen == 1)
 			/* End of header */
 			return;
 
-		ALLOC_GROW(*buf_p, linelen + *ofs_p + 20, *space_p);
-		dst = *buf_p + *ofs_p;
-
 		if (fmt == CMIT_FMT_RAW) {
-			memcpy(dst, line, linelen);
-			*ofs_p += linelen;
+			strbuf_add(sb, line, linelen);
 			continue;
 		}
 
@@ -987,10 +956,8 @@ static void pp_header(enum cmit_fmt fmt,
 			     parent = parent->next, num++)
 				;
 			/* with enough slop */
-			num = *ofs_p + num * 50 + 20;
-			ALLOC_GROW(*buf_p, num, *space_p);
-			dst = *buf_p + *ofs_p;
-			*ofs_p += add_merge_info(fmt, dst, commit, abbrev);
+			strbuf_grow(sb, num * 50 + 20);
+			add_merge_info(fmt, sb, commit, abbrev);
 			parents_shown = 1;
 		}
 
@@ -1000,129 +967,100 @@ static void pp_header(enum cmit_fmt fmt,
 		 * FULLER shows both authors and dates.
 		 */
 		if (!memcmp(line, "author ", 7)) {
-			len = linelen;
+			unsigned long len = linelen;
 			if (fmt == CMIT_FMT_EMAIL)
 				len = bound_rfc2047(linelen, encoding);
-			ALLOC_GROW(*buf_p, *ofs_p + len + 80, *space_p);
-			dst = *buf_p + *ofs_p;
-			*ofs_p += add_user_info("Author", fmt, dst,
-						line + 7, dmode, encoding);
+			strbuf_grow(sb, len + 80);
+			add_user_info("Author", fmt, sb, line + 7, dmode, encoding);
 		}
 
 		if (!memcmp(line, "committer ", 10) &&
 		    (fmt == CMIT_FMT_FULL || fmt == CMIT_FMT_FULLER)) {
-			len = linelen;
+			unsigned long len = linelen;
 			if (fmt == CMIT_FMT_EMAIL)
 				len = bound_rfc2047(linelen, encoding);
-			ALLOC_GROW(*buf_p, *ofs_p + len + 80, *space_p);
-			dst = *buf_p + *ofs_p;
-			*ofs_p += add_user_info("Commit", fmt, dst,
-						line + 10, dmode, encoding);
+			strbuf_grow(sb, len + 80);
+			add_user_info("Commit", fmt, sb, line + 10, dmode, encoding);
 		}
 	}
 }
 
 static void pp_title_line(enum cmit_fmt fmt,
 			  const char **msg_p,
-			  unsigned long *len_p,
-			  unsigned long *ofs_p,
-			  char **buf_p,
-			  unsigned long *space_p,
-			  int indent,
+			  struct strbuf *sb,
 			  const char *subject,
 			  const char *after_subject,
 			  const char *encoding,
 			  int plain_non_ascii)
 {
-	char *title;
-	unsigned long title_alloc, title_len;
+	struct strbuf title;
 	unsigned long len;
 
-	title_len = 0;
-	title_alloc = 80;
-	title = xmalloc(title_alloc);
+	strbuf_init(&title);
+	strbuf_grow(&title, 80);
+
 	for (;;) {
 		const char *line = *msg_p;
-		int linelen = get_one_line(line, *len_p);
-		*msg_p += linelen;
-		*len_p -= linelen;
+		int linelen = get_one_line(line);
 
+		*msg_p += linelen;
 		if (!linelen || is_empty_line(line, &linelen))
 			break;
 
-		if (title_alloc <= title_len + linelen + 2) {
-			title_alloc = title_len + linelen + 80;
-			title = xrealloc(title, title_alloc);
-		}
-		len = 0;
-		if (title_len) {
+		strbuf_grow(&title, linelen + 2);
+		if (title.len) {
 			if (fmt == CMIT_FMT_EMAIL) {
-				len++;
-				title[title_len++] = '\n';
+				strbuf_addch(&title, '\n');
 			}
-			len++;
-			title[title_len++] = ' ';
+			strbuf_addch(&title, ' ');
 		}
-		memcpy(title + title_len, line, linelen);
-		title_len += linelen;
+		strbuf_add(&title, line, linelen);
 	}
 
 	/* Enough slop for the MIME header and rfc2047 */
-	len = bound_rfc2047(title_len, encoding)+ 1000;
+	len = bound_rfc2047(title.len, encoding) + 1000;
 	if (subject)
 		len += strlen(subject);
 	if (after_subject)
 		len += strlen(after_subject);
 	if (encoding)
 		len += strlen(encoding);
-	ALLOC_GROW(*buf_p, title_len + *ofs_p + len, *space_p);
 
+	strbuf_grow(sb, title.len + len);
 	if (subject) {
-		len = strlen(subject);
-		memcpy(*buf_p + *ofs_p, subject, len);
-		*ofs_p += len;
-		*ofs_p += add_rfc2047(*buf_p + *ofs_p,
-				      title, title_len, encoding);
+		strbuf_addstr(sb, subject);
+		add_rfc2047(sb, title.buf, title.len, encoding);
 	} else {
-		memcpy(*buf_p + *ofs_p, title, title_len);
-		*ofs_p += title_len;
+		strbuf_addbuf(sb, &title);
 	}
-	(*buf_p)[(*ofs_p)++] = '\n';
+	strbuf_addch(sb, '\n');
+
 	if (plain_non_ascii) {
 		const char *header_fmt =
 			"MIME-Version: 1.0\n"
 			"Content-Type: text/plain; charset=%s\n"
 			"Content-Transfer-Encoding: 8bit\n";
-		*ofs_p += snprintf(*buf_p + *ofs_p,
-				   *space_p - *ofs_p,
-				   header_fmt, encoding);
+		strbuf_addf(sb, header_fmt, encoding);
 	}
 	if (after_subject) {
-		len = strlen(after_subject);
-		memcpy(*buf_p + *ofs_p, after_subject, len);
-		*ofs_p += len;
+		strbuf_addstr(sb, after_subject);
 	}
-	free(title);
 	if (fmt == CMIT_FMT_EMAIL) {
-		ALLOC_GROW(*buf_p, *ofs_p + 20, *space_p);
-		(*buf_p)[(*ofs_p)++] = '\n';
+		strbuf_addch(sb, '\n');
 	}
+	strbuf_release(&title);
 }
 
 static void pp_remainder(enum cmit_fmt fmt,
 			 const char **msg_p,
-			 unsigned long *len_p,
-			 unsigned long *ofs_p,
-			 char **buf_p,
-			 unsigned long *space_p,
+			 struct strbuf *sb,
 			 int indent)
 {
 	int first = 1;
 	for (;;) {
 		const char *line = *msg_p;
-		int linelen = get_one_line(line, *len_p);
+		int linelen = get_one_line(line);
 		*msg_p += linelen;
-		*len_p -= linelen;
 
 		if (!linelen)
 			break;
@@ -1135,36 +1073,30 @@ static void pp_remainder(enum cmit_fmt fmt,
 		}
 		first = 0;
 
-		ALLOC_GROW(*buf_p, *ofs_p + linelen + indent + 20, *space_p);
+		strbuf_grow(sb, linelen + indent + 20);
 		if (indent) {
-			memset(*buf_p + *ofs_p, ' ', indent);
-			*ofs_p += indent;
+			memset(sb->buf + sb->len, ' ', indent);
+			strbuf_setlen(sb, sb->len + indent);
 		}
-		memcpy(*buf_p + *ofs_p, line, linelen);
-		*ofs_p += linelen;
-		(*buf_p)[(*ofs_p)++] = '\n';
+		strbuf_add(sb, line, linelen);
+		strbuf_addch(sb, '\n');
 	}
 }
 
-unsigned long pretty_print_commit(enum cmit_fmt fmt,
-				  const struct commit *commit,
-				  unsigned long len,
-				  char **buf_p, unsigned long *space_p,
-				  int abbrev, const char *subject,
-				  const char *after_subject,
+void pretty_print_commit(enum cmit_fmt fmt, const struct commit *commit,
+				  struct strbuf *sb, int abbrev,
+				  const char *subject, const char *after_subject,
 				  enum date_mode dmode)
 {
-	unsigned long offset = 0;
 	unsigned long beginning_of_body;
 	int indent = 4;
 	const char *msg = commit->buffer;
 	int plain_non_ascii = 0;
 	char *reencoded;
 	const char *encoding;
-	char *buf;
 
 	if (fmt == CMIT_FMT_USERFORMAT)
-		return format_commit_message(commit, msg, buf_p, space_p);
+		return format_commit_message(commit, msg, sb);
 
 	encoding = (git_log_output_encoding
 		    ? git_log_output_encoding
@@ -1174,7 +1106,6 @@ unsigned long pretty_print_commit(enum cmit_fmt fmt,
 	reencoded = logmsg_reencode(commit, encoding);
 	if (reencoded) {
 		msg = reencoded;
-		len = strlen(reencoded);
 	}
 
 	if (fmt == CMIT_FMT_ONELINE || fmt == CMIT_FMT_EMAIL)
@@ -1189,14 +1120,13 @@ unsigned long pretty_print_commit(enum cmit_fmt fmt,
 	if (fmt == CMIT_FMT_EMAIL && !after_subject) {
 		int i, ch, in_body;
 
-		for (in_body = i = 0; (ch = msg[i]) && i < len; i++) {
+		for (in_body = i = 0; (ch = msg[i]); i++) {
 			if (!in_body) {
 				/* author could be non 7-bit ASCII but
 				 * the log may be so; skip over the
 				 * header part first.
 				 */
-				if (ch == '\n' &&
-				    i + 1 < len && msg[i+1] == '\n')
+				if (ch == '\n' && msg[i+1] == '\n')
 					in_body = 1;
 			}
 			else if (non_ascii(ch)) {
@@ -1206,59 +1136,44 @@ unsigned long pretty_print_commit(enum cmit_fmt fmt,
 		}
 	}
 
-	pp_header(fmt, abbrev, dmode, encoding,
-		  commit, &msg, &len,
-		  &offset, buf_p, space_p);
+	pp_header(fmt, abbrev, dmode, encoding, commit, &msg, sb);
 	if (fmt != CMIT_FMT_ONELINE && !subject) {
-		ALLOC_GROW(*buf_p, offset + 20, *space_p);
-		(*buf_p)[offset++] = '\n';
+		strbuf_addch(sb, '\n');
 	}
 
 	/* Skip excess blank lines at the beginning of body, if any... */
 	for (;;) {
-		int linelen = get_one_line(msg, len);
+		int linelen = get_one_line(msg);
 		int ll = linelen;
 		if (!linelen)
 			break;
 		if (!is_empty_line(msg, &ll))
 			break;
 		msg += linelen;
-		len -= linelen;
 	}
 
 	/* These formats treat the title line specially. */
-	if (fmt == CMIT_FMT_ONELINE
-	    || fmt == CMIT_FMT_EMAIL)
-		pp_title_line(fmt, &msg, &len, &offset,
-			      buf_p, space_p, indent,
-			      subject, after_subject, encoding,
-			      plain_non_ascii);
-
-	beginning_of_body = offset;
-	if (fmt != CMIT_FMT_ONELINE)
-		pp_remainder(fmt, &msg, &len, &offset,
-			     buf_p, space_p, indent);
-
-	while (offset && isspace((*buf_p)[offset-1]))
-		offset--;
+	if (fmt == CMIT_FMT_ONELINE || fmt == CMIT_FMT_EMAIL)
+		pp_title_line(fmt, &msg, sb, subject,
+			      after_subject, encoding, plain_non_ascii);
 
-	ALLOC_GROW(*buf_p, offset + 20, *space_p);
-	buf = *buf_p;
+	beginning_of_body = sb->len;
+	if (fmt != CMIT_FMT_ONELINE)
+		pp_remainder(fmt, &msg, sb, indent);
+	strbuf_rtrim(sb);
 
 	/* Make sure there is an EOLN for the non-oneline case */
 	if (fmt != CMIT_FMT_ONELINE)
-		buf[offset++] = '\n';
+		strbuf_addch(sb, '\n');
 
 	/*
 	 * The caller may append additional body text in e-mail
 	 * format.  Make sure we did not strip the blank line
 	 * between the header and the body.
 	 */
-	if (fmt == CMIT_FMT_EMAIL && offset <= beginning_of_body)
-		buf[offset++] = '\n';
-	buf[offset] = '\0';
+	if (fmt == CMIT_FMT_EMAIL && sb->len <= beginning_of_body)
+		strbuf_addch(sb, '\n');
 	free(reencoded);
-	return offset;
 }
 
 struct commit *pop_commit(struct commit_list **stack)
@@ -1337,12 +1252,12 @@ void sort_in_topological_order_fn(struct commit_list ** list, int lifo,
 		next=next->next;
 	}
 	/*
-         * find the tips
-         *
-         * tips are nodes not reachable from any other node in the list
-         *
-         * the tips serve as a starting set for the work queue.
-         */
+	 * find the tips
+	 *
+	 * tips are nodes not reachable from any other node in the list
+	 *
+	 * the tips serve as a starting set for the work queue.
+	 */
 	next=*list;
 	insert = &work;
 	while (next) {
@@ -1369,9 +1284,9 @@ void sort_in_topological_order_fn(struct commit_list ** list, int lifo,
 			if (pn) {
 				/*
 				 * parents are only enqueued for emission
-                                 * when all their children have been emitted thereby
-                                 * guaranteeing topological order.
-                                 */
+				 * when all their children have been emitted thereby
+				 * guaranteeing topological order.
+				 */
 				pn->indegree--;
 				if (!pn->indegree) {
 					if (!lifo)
@@ -1383,9 +1298,9 @@ void sort_in_topological_order_fn(struct commit_list ** list, int lifo,
 			parents=parents->next;
 		}
 		/*
-                 * work_item is a commit all of whose children
-                 * have already been emitted. we can emit it now.
-                 */
+		 * work_item is a commit all of whose children
+		 * have already been emitted. we can emit it now.
+		 */
 		*pptr = work_node->list_item;
 		pptr = &(*pptr)->next;
 		*pptr = NULL;
@@ -1481,8 +1396,7 @@ static struct commit_list *merge_bases(struct commit *one, struct commit *two)
 }
 
 struct commit_list *get_merge_bases(struct commit *one,
-				    struct commit *two,
-                                    int cleanup)
+					struct commit *two, int cleanup)
 {
 	struct commit_list *list;
 	struct commit **rslt;
diff --git a/commit.h b/commit.h
index 467872e..745f538 100644
--- a/commit.h
+++ b/commit.h
@@ -3,6 +3,7 @@
 
 #include "object.h"
 #include "tree.h"
+#include "strbuf.h"
 #include "decorate.h"
 
 struct commit_list {
@@ -61,7 +62,10 @@ enum cmit_fmt {
 };
 
 extern enum cmit_fmt get_commit_format(const char *arg);
-extern unsigned long pretty_print_commit(enum cmit_fmt fmt, const struct commit *, unsigned long len, char **buf_p, unsigned long *space_p, int abbrev, const char *subject, const char *after_subject, enum date_mode dmode);
+extern void pretty_print_commit(enum cmit_fmt fmt, const struct commit*,
+                                struct strbuf *,
+                                int abbrev, const char *subject,
+                                const char *after_subject, enum date_mode);
 
 /** Removes the first commit from a list sorted by date, and adds all
  * of its parents.
diff --git a/log-tree.c b/log-tree.c
index a642371..5186939 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -79,25 +79,14 @@ static int detect_any_signoff(char *letter, int size)
 	return seen_head && seen_name;
 }
 
-static unsigned long append_signoff(char **buf_p, unsigned long *buf_sz_p,
-				    unsigned long at, const char *signoff)
+static void append_signoff(struct strbuf *sb, const char *signoff)
 {
 	static const char signed_off_by[] = "Signed-off-by: ";
 	size_t signoff_len = strlen(signoff);
 	int has_signoff = 0;
 	char *cp;
-	char *buf;
-	unsigned long buf_sz;
-
-	buf = *buf_p;
-	buf_sz = *buf_sz_p;
-	if (buf_sz <= at + strlen(signed_off_by) + signoff_len + 3) {
-		buf_sz += strlen(signed_off_by) + signoff_len + 3;
-		buf = xrealloc(buf, buf_sz);
-		*buf_p = buf;
-		*buf_sz_p = buf_sz;
-	}
-	cp = buf;
+
+	cp = sb->buf;
 
 	/* First see if we already have the sign-off by the signer */
 	while ((cp = strstr(cp, signed_off_by))) {
@@ -105,29 +94,25 @@ static unsigned long append_signoff(char **buf_p, unsigned long *buf_sz_p,
 		has_signoff = 1;
 
 		cp += strlen(signed_off_by);
-		if (cp + signoff_len >= buf + at)
+		if (cp + signoff_len >= sb->buf + sb->len)
 			break;
 		if (strncmp(cp, signoff, signoff_len))
 			continue;
 		if (!isspace(cp[signoff_len]))
 			continue;
 		/* we already have him */
-		return at;
+		return;
 	}
 
 	if (!has_signoff)
-		has_signoff = detect_any_signoff(buf, at);
+		has_signoff = detect_any_signoff(sb->buf, sb->len);
 
 	if (!has_signoff)
-		buf[at++] = '\n';
-
-	strcpy(buf + at, signed_off_by);
-	at += strlen(signed_off_by);
-	strcpy(buf + at, signoff);
-	at += signoff_len;
-	buf[at++] = '\n';
-	buf[at] = 0;
-	return at;
+		strbuf_addch(sb, '\n');
+
+	strbuf_addstr(sb, signed_off_by);
+	strbuf_add(sb, signoff, signoff_len);
+	strbuf_addch(sb, '\n');
 }
 
 static unsigned int digits_in_number(unsigned int number)
@@ -142,14 +127,12 @@ static unsigned int digits_in_number(unsigned int number)
 
 void show_log(struct rev_info *opt, const char *sep)
 {
-	char *msgbuf = NULL;
-	unsigned long msgbuf_len = 0;
+	struct strbuf msgbuf;
 	struct log_info *log = opt->loginfo;
 	struct commit *commit = log->commit, *parent = log->parent;
 	int abbrev = opt->diffopt.abbrev;
 	int abbrev_commit = opt->abbrev_commit ? opt->abbrev : 40;
 	const char *extra;
-	int len;
 	const char *subject = NULL, *extra_headers = opt->extra_headers;
 
 	opt->loginfo = NULL;
@@ -288,18 +271,17 @@ void show_log(struct rev_info *opt, const char *sep)
 	/*
 	 * And then the pretty-printed message itself
 	 */
-	len = pretty_print_commit(opt->commit_format, commit, ~0u,
-				  &msgbuf, &msgbuf_len, abbrev, subject,
-				  extra_headers, opt->date_mode);
+	strbuf_init(&msgbuf);
+	pretty_print_commit(opt->commit_format, commit, &msgbuf,
+				  abbrev, subject, extra_headers, opt->date_mode);
 
 	if (opt->add_signoff)
-		len = append_signoff(&msgbuf, &msgbuf_len, len,
-				     opt->add_signoff);
+		append_signoff(&msgbuf, opt->add_signoff);
 	if (opt->show_log_size)
-		printf("log size %i\n", len);
+		printf("log size %i\n", (int)msgbuf.len);
 
-	printf("%s%s%s", msgbuf, extra, sep);
-	free(msgbuf);
+	printf("%s%s%s", msgbuf.buf, extra, sep);
+	strbuf_release(&msgbuf);
 }
 
 int log_tree_diff_flush(struct rev_info *opt)
-- 
1.5.3.1

[PATCH 2/3] Change semantics of interpolate to work like snprintf.

From: Pierre Habouzit <hidden>
Date: 2016-06-15 22:43:34

  Also fix many off-by-ones and a useless memset.

Signed-off-by: Pierre Habouzit <redacted>
---
 commit.c      |    9 ++++-----
 interpolate.c |   20 ++++++++------------
 2 files changed, 12 insertions(+), 17 deletions(-)
diff --git a/commit.c b/commit.c
index dc5a064..30db9f4 100644
--- a/commit.c
+++ b/commit.c
@@ -922,15 +922,14 @@ static long format_commit_message(const struct commit *commit,
 
 	do {
 		char *buf = *buf_p;
-		unsigned long space = *space_p;
+		unsigned long len;
 
-		space = interpolate(buf, space, user_format,
+		len = interpolate(buf, *space_p, user_format,
 				    table, ARRAY_SIZE(table));
-		if (!space)
+		if (len < *space_p)
 			break;
-		buf = xrealloc(buf, space);
+		ALLOC_GROW(buf, len + 1, *space_p);
 		*buf_p = buf;
-		*space_p = space;
 	} while (1);
 	interp_clear_table(table, ARRAY_SIZE(table));
 
diff --git a/interpolate.c b/interpolate.c
index 0082677..ff4fb10 100644
--- a/interpolate.c
+++ b/interpolate.c
@@ -44,9 +44,8 @@ void interp_clear_table(struct interp *table, int ninterps)
  *        { "%%", "%"},
  *    }
  *
- * Returns 0 on a successful substitution pass that fits in result,
- * Returns a number of bytes needed to hold the full substituted
- * string otherwise.
+ * Returns the length of the substituted string (not including the final \0).
+ * Like with snprintf, if the result is >= reslen, then it overflowed.
  */
 
 unsigned long interpolate(char *result, unsigned long reslen,
@@ -61,8 +60,6 @@ unsigned long interpolate(char *result, unsigned long reslen,
 	int i;
 	char c;
 
-        memset(result, 0, reslen);
-
 	while ((c = *src)) {
 		if (c == '%') {
 			/* Try to match an interpolation string. */
@@ -78,9 +75,9 @@ unsigned long interpolate(char *result, unsigned long reslen,
 				value = interps[i].value;
 				valuelen = strlen(value);
 
-				if (newlen + valuelen + 1 < reslen) {
+				if (newlen + valuelen < reslen) {
 					/* Substitute. */
-					strncpy(dest, value, valuelen);
+					memcpy(dest, value, valuelen);
 					dest += valuelen;
 				}
 				newlen += valuelen;
@@ -89,14 +86,13 @@ unsigned long interpolate(char *result, unsigned long reslen,
 			}
 		}
 		/* Straight copy one non-interpolation character. */
-		if (newlen + 1 < reslen)
+		if (newlen < reslen)
 			*dest++ = *src;
 		src++;
 		newlen++;
 	}
 
-	if (newlen + 1 < reslen)
-		return 0;
-	else
-		return newlen + 2;
+	if (reslen > 0)
+		*dest = '\0';
+	return newlen;
 }
-- 
1.5.3.1

[PATCH 1/3] Add strbuf_rtrim (to remove trailing spaces).

From: Pierre Habouzit <hidden>
Date: 2016-06-15 22:43:34

Signed-off-by: Pierre Habouzit <redacted>
---
 strbuf.c |    7 +++++++
 strbuf.h |    3 +++
 2 files changed, 10 insertions(+), 0 deletions(-)
diff --git a/strbuf.c b/strbuf.c
index acc7fc8..565c343 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -28,6 +28,13 @@ void strbuf_grow(struct strbuf *sb, size_t extra) {
 	ALLOC_GROW(sb->buf, sb->len + extra + 1, sb->alloc);
 }
 
+void strbuf_rtrim(struct strbuf *sb)
+{
+    while (sb->len > 0 && isspace((unsigned char)sb->buf[sb->len - 1]))
+        sb->len--;
+    sb->buf[sb->len] = '\0';
+}
+
 void strbuf_add(struct strbuf *sb, const void *data, size_t len) {
 	strbuf_grow(sb, len);
 	memcpy(sb->buf + sb->len, data, len);
diff --git a/strbuf.h b/strbuf.h
index b40dc99..9b708cc 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -68,6 +68,9 @@ static inline void strbuf_setlen(struct strbuf *sb, size_t len) {
 
 extern void strbuf_grow(struct strbuf *, size_t);
 
+/*----- content related -----*/
+extern void strbuf_rtrim(struct strbuf *);
+
 /*----- add data in your buffer -----*/
 static inline void strbuf_addch(struct strbuf *sb, int c) {
 	strbuf_grow(sb, 1);
-- 
1.5.3.1

Use strbufs in commit.c (pretty printing)

From: Pierre Habouzit <hidden>
Date: 2016-06-15 22:43:34

  Here is a 3 patch series to use strbufs for the pretty printing
functions in commit.c.

  * The first one adds a needed useful function to strbufs (rtrim).
  * The second one changes the semantics of interpolate (as the third
    will need that) and fixes the rest of the code accordingly.
  * Do the change.

  Testsuite passes after step 2 and 3.

Re: [PATCH 1/3] Add strbuf_rtrim (to remove trailing spaces).

From: René Scharfe <hidden>
Date: 2016-06-15 22:43:34

Pierre Habouzit schrieb:
quoted hunk
diff --git a/strbuf.c b/strbuf.c
index acc7fc8..565c343 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -28,6 +28,13 @@ void strbuf_grow(struct strbuf *sb, size_t extra) {
 	ALLOC_GROW(sb->buf, sb->len + extra + 1, sb->alloc);
 }
 
+void strbuf_rtrim(struct strbuf *sb)
+{
+    while (sb->len > 0 && isspace((unsigned char)sb->buf[sb->len - 1]))
+        sb->len--;
+    sb->buf[sb->len] = '\0';
+}
Please use tabs instead of spaces to indent, just like you did in your
other patches. :)

René

Re: [PATCH 3/3] Rework pretty_print_commit to use strbufs instead of custom buffers.

From: René Scharfe <hidden>
Date: 2016-06-15 22:43:34

Pierre Habouzit schrieb:
  Also remove the "len" parameter, as:
  (1) it was used as a max boundary, and every caller used ~0u
  (2) we check for final NUL no matter what, so it doesn't help for speed.

  As a result most of the pp_* function takes 3 arguments less, and we need
a lot less local variables, this makes the code way more readable, and
easier to extend if needed.

  This patch also fixes some spacing and cosmetic issues.

Signed-off-by: Pierre Habouzit <redacted>
---
 builtin-branch.c      |   15 +--
 builtin-log.c         |   12 +-
 builtin-rev-list.c    |   13 +-
 builtin-show-branch.c |   13 +-
 commit.c              |  330 ++++++++++++++++++-------------------------------
 commit.h              |    6 +-
 log-tree.c            |   56 +++------
 7 files changed, 171 insertions(+), 274 deletions(-)
Nice!  I wonder if we should #include strbuf.h from git-compat-util.h
(just like e.g. string.h) instead of from commit.h, in order to have
strbuf available everywhere in git.

Please be aware of the changes to commit.c already in next which your
patch conflicts with: format_commit_message() has been exported and is
used in builtin-archive.c there.

René

Re: [PATCH 3/3] Rework pretty_print_commit to use strbufs instead of custom buffers.

From: Pierre Habouzit <hidden>
Date: 2016-06-15 22:43:34

On Sat, Sep 08, 2007 at 06:40:08PM +0000, René Scharfe wrote:
Pierre Habouzit schrieb:
quoted
  Also remove the "len" parameter, as:
  (1) it was used as a max boundary, and every caller used ~0u
  (2) we check for final NUL no matter what, so it doesn't help for speed.

  As a result most of the pp_* function takes 3 arguments less, and we need
a lot less local variables, this makes the code way more readable, and
easier to extend if needed.

  This patch also fixes some spacing and cosmetic issues.

Signed-off-by: Pierre Habouzit <redacted>
---
 builtin-branch.c      |   15 +--
 builtin-log.c         |   12 +-
 builtin-rev-list.c    |   13 +-
 builtin-show-branch.c |   13 +-
 commit.c              |  330 ++++++++++++++++++-------------------------------
 commit.h              |    6 +-
 log-tree.c            |   56 +++------
 7 files changed, 171 insertions(+), 274 deletions(-)
Nice!  I wonder if we should #include strbuf.h from git-compat-util.h
(just like e.g. string.h) instead of from commit.h, in order to have
strbuf available everywhere in git.

Please be aware of the changes to commit.c already in next which your
patch conflicts with: format_commit_message() has been exported and is
used in builtin-archive.c there.
  Okay, then I'll hope than Junio will merge the first series and then
rebase this one on next. If I'm going to propose more of those patches,
should I write them rather on next or master (like I do now) ?

-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

Re: [PATCH 1/3] Add strbuf_rtrim (to remove trailing spaces).

From: Pierre Habouzit <hidden>
Date: 2016-06-15 22:43:34

On sam, sep 08, 2007 at 04:18:20 +0000, René Scharfe wrote:
Pierre Habouzit schrieb:
quoted
diff --git a/strbuf.c b/strbuf.c
index acc7fc8..565c343 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -28,6 +28,13 @@ void strbuf_grow(struct strbuf *sb, size_t extra) {
 	ALLOC_GROW(sb->buf, sb->len + extra + 1, sb->alloc);
 }
 
+void strbuf_rtrim(struct strbuf *sb)
+{
+    while (sb->len > 0 && isspace((unsigned char)sb->buf[sb->len - 1]))
+        sb->len--;
+    sb->buf[sb->len] = '\0';
+}
Please use tabs instead of spaces to indent, just like you did in your
other patches. :)
  yeah, good catch.
  I've added some more patches tonight, and have added another function
in strbufs, and thanks to the awsome git rebase -i, this patch
integrates this new function as well, so this patch series is obsolete
already. I wont repost it until I know if I should rebase it on next
rather than master though.

Cheers,
-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

Re: [PATCH 1/3] Add strbuf_rtrim (to remove trailing spaces).

From: Pierre Habouzit <hidden>
Date: 2016-06-15 22:43:34

On Sat, Sep 08, 2007 at 10:53:53PM +0000, Pierre Habouzit wrote:
On sam, sep 08, 2007 at 04:18:20 +0000, René Scharfe wrote:
quoted
Pierre Habouzit schrieb:
quoted
diff --git a/strbuf.c b/strbuf.c
index acc7fc8..565c343 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -28,6 +28,13 @@ void strbuf_grow(struct strbuf *sb, size_t extra) {
 	ALLOC_GROW(sb->buf, sb->len + extra + 1, sb->alloc);
 }
 
+void strbuf_rtrim(struct strbuf *sb)
+{
+    while (sb->len > 0 && isspace((unsigned char)sb->buf[sb->len - 1]))
+        sb->len--;
+    sb->buf[sb->len] = '\0';
+}
Please use tabs instead of spaces to indent, just like you did in your
other patches. :)
  yeah, good catch.
  I've added some more patches tonight, and have added another function
in strbufs, and thanks to the awsome git rebase -i, this patch
integrates this new function as well, so this patch series is obsolete
already. I wont repost it until I know if I should rebase it on next
rather than master though.
  Okay I'm stupid, Junio already merged my patches in next, doh.

  I've seen the conflict btw, and there is a memory leak (on the format)
in builtin-archive.c now... but I'm going to fix it (hopefuly) :)


-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help