Re: strbuf API

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

Re: strbuf API

From: Miles Bader <hidden>
Date: 2016-06-15 22:43:33

Pierre Habouzit [off-list ref] writes:
  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
Yeah, I got the same impression.  There's an odd "accumulated
haphazardly over time" feel to much of it.

[One thing I found especially weird is the way the "core" interface uses
lowercase names, but the "aux" interfaces all seem to use StUDlyCaPS...]

-Miles
-- 
Ich bin ein Virus. Mach' mit und kopiere mich in Deine .signature.

strbuf new semantics, let's give it a try

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

  Allright, here is a proposal for a new strbuf semantics like the one I
proposed before. Like said, with this proposal, strbuf's always have a
'\0' at the end of the buffer, not accounted in its length, so that any
C str* function can still be used.

  The second patch demonstrate that strbufs makes the code smaller,
simpler to read, while keeping the exact same efficiency (as may
mallocs, memcpys, and fread's in that case).


  I've tried to keep the amount of inlines low. Though, I wonder if
strbuf_add shouldn't be inlined (so that memcpys could be even more
inlined/optmized by gcc). And strbuf_addstr is inlined so that the
strlen call is optimized when the argument is an immediate string.
Meaning that calling strbuf_addstr(sb, "foo") is not sloppy at all.

  I don't really like the buffer growth scheme right now, I just stick
with the one that was used in git before. Though I really belive it's
inelegant as is, and should be simplified (strbuf_extend).

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

[PATCH] Add strbuf_fread, use it in fast-import.c.

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

  This is a good proof of concept that combined with strbuf_detach it often
helps improving readability a lot, while remaining as efficient as before.

Signed-off-by: Pierre Habouzit <redacted>
---
 fast-import.c |   26 ++++++++++----------------
 strbuf.c      |   12 ++++++++++++
 strbuf.h      |    3 +++
 3 files changed, 25 insertions(+), 16 deletions(-)
diff --git a/fast-import.c b/fast-import.c
index ab3927b..2d8daeb 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1638,20 +1638,15 @@ static void cmd_mark(void)
 
 static void *cmd_data (size_t *size)
 {
-	size_t length;
-	char *buffer;
+	struct strbuf buffer = STRBUF_INIT;
 
 	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;
 
-		/* XXX possible memory leak ? */
-		strbuf_detach(&command_buf);
 		for (;;) {
 			read_line(&command_buf, stdin, '\n');
 			if (command_buf.eof)
@@ -1659,19 +1654,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));
@@ -1680,8 +1674,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)
diff --git a/strbuf.c b/strbuf.c
index 86d5965..693fbcf 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -58,6 +58,18 @@ void strbuf_addvf(struct strbuf *sb, const char *fmt, va_list ap)
 	sb->buf[sb->len] = '\0';
 }
 
+size_t strbuf_fread(struct strbuf *sb, size_t size, FILE *f) {
+	size_t res;
+
+	strbuf_ensure(sb, size);
+	res = fread(sb->buf + sb->len, 1, size, f);
+	if (res > 0) {
+		sb->len += res;
+		sb->buf[sb->len] = '\0';
+	}
+	return res;
+}
+
 void read_line(struct strbuf *sb, FILE *fp, int term) {
 	int ch;
 	if (feof(fp)) {
diff --git a/strbuf.h b/strbuf.h
index ee72419..20c5255 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -42,6 +42,9 @@ void strbuf_addf(struct strbuf *sb, const char *fmt, ...) {
 	va_end(ap);
 }
 
+
+extern size_t strbuf_fread(struct strbuf *, size_t, FILE *);
+
 extern void read_line(struct strbuf *, FILE *, int);
 
 #endif /* STRBUF_H */
-- 
1.5.3

[PATCH] Rework strbuf API and semantics.

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

  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: buffer_ensure(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 |   65 +++++++++++++---------------------------------------
 fast-import.c |   24 +++++++++---------
 mktree.c      |    4 +--
 strbuf.c      |   71 +++++++++++++++++++++++++++++++++++++++++++++-----------
 strbuf.h      |   34 +++++++++++++++++++++++++++
 5 files changed, 120 insertions(+), 78 deletions(-)
diff --git a/archive-tar.c b/archive-tar.c
index 66fe3e3..ccdd7d5 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_ensure(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_wipe(&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_wipe(&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_ensure(&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 {
diff --git a/fast-import.c b/fast-import.c
index 078079d..ab3927b 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;
@@ -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;
@@ -1610,7 +1610,7 @@ static void read_next_command(void)
 				free(rc->buf);
 			}
 
-			rc->buf = command_buf.buf;
+			rc->buf  = command_buf.buf;
 			rc->prev = cmd_tail;
 			rc->next = cmd_hist.prev;
 			rc->prev->next = rc;
@@ -1649,7 +1649,9 @@ 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;
+
+		/* XXX possible memory leak ? */
+		strbuf_detach(&command_buf);
 		for (;;) {
 			read_line(&command_buf, stdin, '\n');
 			if (command_buf.eof)
@@ -1657,11 +1659,9 @@ 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);
-			memcpy(buffer + length,
-				command_buf.buf,
-				command_buf.len - 1);
-			length += command_buf.len - 1;
+			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';
 		}
 		free(term);
@@ -2101,7 +2101,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 +2256,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 +2273,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..86d5965 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -2,40 +2,83 @@
 #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_wipe(struct strbuf *sb) {
 	free(sb->buf);
-	strbuf_init(sb);
+	memset(sb, 0, sizeof(*sb));
 }
 
-static void inline strbuf_add(struct strbuf *sb, int ch) {
-	if (sb->alloc <= sb->len) {
-		sb->alloc = sb->alloc * 3 / 2 + 16;
+void strbuf_reset(struct strbuf *sb) {
+	if (sb->len)
+		sb->buf[sb->len = 0] = '\0';
+}
+
+char *strbuf_detach(struct strbuf *sb) {
+    char *res = sb->buf;
+    strbuf_init(sb);
+    return res;
+}
+
+void strbuf_ensure(struct strbuf *sb, int extra) {
+	if (sb->len + extra >= sb->alloc) {
+		while (sb->len + extra >= sb->alloc)
+			sb->alloc = sb->alloc * 3 / 2 + 16;
 		sb->buf = xrealloc(sb->buf, sb->alloc);
 	}
-	sb->buf[sb->len++] = ch;
 }
 
-static void strbuf_end(struct strbuf *sb) {
-	strbuf_add(sb, 0);
+void strbuf_add(struct strbuf *sb, const void *data, int len) {
+	strbuf_ensure(sb, len);
+	memcpy(sb->buf + sb->len, data, len);
+	sb->len += len;
+	sb->buf[sb->len] = '\0';
+}
+
+void strbuf_addvf(struct strbuf *sb, const char *fmt, va_list ap)
+{
+	int len;
+	va_list ap2;
+
+	va_copy(ap2, ap);
+
+	len = vsnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, ap);
+	if (len < 0) {
+		len = 0;
+	}
+	if (len >= sb->alloc - sb->len) {
+		strbuf_ensure(sb, len);
+		len = vsnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, ap2);
+		if (len >= sb->alloc - sb->len) {
+			len = sb->alloc - sb->len - 1;
+		}
+	}
+	sb->len = sb->len + len;
+	sb->buf[sb->len] = '\0';
 }
 
 void read_line(struct strbuf *sb, FILE *fp, int term) {
 	int ch;
-	strbuf_begin(sb);
 	if (feof(fp)) {
+		strbuf_wipe(sb);
 		sb->eof = 1;
 		return;
 	}
+
+	strbuf_reset(sb);
 	while ((ch = fgetc(fp)) != EOF) {
 		if (ch == term)
 			break;
-		strbuf_add(sb, ch);
+		strbuf_ensure(sb, 1);
+		sb->buf[sb->len++] = ch;
 	}
-	if (ch == EOF && sb->len == 0)
+	if (ch == EOF && sb->len == 0) {
+		strbuf_wipe(sb);
 		sb->eof = 1;
-	strbuf_end(sb);
+	}
+
+	strbuf_ensure(sb, 1);
+	sb->buf[sb->len] = '\0';
 }
+
diff --git a/strbuf.h b/strbuf.h
index 74cc012..ee72419 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -7,7 +7,41 @@ struct strbuf {
 	char *buf;
 };
 
+#define STRBUF_INIT  { 0, 0, 0, NULL }
+
+/* strbuf life cycle */
 extern void strbuf_init(struct strbuf *);
+extern void strbuf_wipe(struct strbuf *);
+extern void strbuf_reset(struct strbuf *);
+extern char *strbuf_detach(struct strbuf *);
+
+
+extern void strbuf_ensure(struct strbuf *, int);
+extern void strbuf_add(struct strbuf *, const void *data, int len);
+
+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);
+}
+static inline void strbuf_addch(struct strbuf *sb, int c) {
+	strbuf_ensure(sb, 1);
+	sb->buf[sb->len++] = c;
+	sb->buf[sb->len] = '\0';
+}
+
+__attribute__((format(printf,2,0)))
+extern void strbuf_addvf(struct strbuf *, const char *, va_list);
+
+static inline __attribute__((format(printf,2,3)))
+void strbuf_addf(struct strbuf *sb, const char *fmt, ...) {
+	va_list ap;
+	va_start(ap, fmt);
+	strbuf_addvf(sb, fmt, ap);
+	va_end(ap);
+}
+
 extern void read_line(struct strbuf *, FILE *, int);
 
 #endif /* STRBUF_H */
-- 
1.5.3

Re: [PATCH] Rework strbuf API and semantics.

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

Hi,

On Tue, 4 Sep 2007, Pierre Habouzit wrote:
  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: buffer_ensure(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.
I like the general idea of this!

However, some comments are due:

- IMHO strbuf_grow() would be more descriptive than buffer_ensure(),

- IMHO the same goes for strbuf_free() instead of strbuf_wipe(), and

- it would be nice to split this patch into

	- the API change (with _minimal_ changes to anything outside of 
	  strbuf.[ch]), and

	- the cleanups in the rest of the code.

Ciao,
Dscho

Re: [PATCH] Rework strbuf API and semantics.

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

On Tue, Sep 04, 2007 at 11:11:45AM +0000, Johannes Schindelin wrote:
Hi,

On Tue, 4 Sep 2007, Pierre Habouzit wrote:
quoted
  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: buffer_ensure(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.
I like the general idea of this!

However, some comments are due:

- IMHO strbuf_grow() would be more descriptive than buffer_ensure(),
  Yeah, strbuf_ensure (and not buffer_ensure sorry :P) isn't a clever
name. strbuf_grow looks better indeed.
- IMHO the same goes for strbuf_free() instead of strbuf_wipe(), and
  Well, I don't like strbuf_free, because it's opposed to
strbuf_alloc/new whatever, that would be functions that create and
release a struct strbuf * (meaning allocating the struct shell as well).
Here you only free the internal buffer, not the shell, so _free() would
be a very bad name.

  In my own coding rules, I have _new/_delete and _init/_wipe functions,
the former acts on pointers, the latter on the structs. Hence the
naming. Though, looking at git's code, it seems that the usual name for
this operation is _release. So would you go for strbuf_release ?

- it would be nice to split this patch into

	- the API change (with _minimal_ changes to anything outside of 
	  strbuf.[ch]), and

	- the cleanups in the rest of the code.
  I'll try to do that, but it's quite hard to achieve knowing that in
many places of the current state of master, there are embeded NUL's
(accounted in ->len). I'll try to see what I can split, but it's
unlikely I'll be able to have an intermediate _working_ state (I mean
that would pass the testsuite) with the new API/semantics _and_ without
touching a lot less of the rest of the code.

  But I really understand that it helps seeing what the new API
improves, whereas right now it may not be that obvious because of the
(not so) many new lines in strbuf.[hc].


  I'll try to roll a new patchset ASAP.

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

Re: [PATCH] Rework strbuf API and semantics.

From: Andreas Ericsson <hidden>
Date: 2016-06-15 22:43:33

Pierre Habouzit wrote:
On Tue, Sep 04, 2007 at 11:11:45AM +0000, Johannes Schindelin wrote:
quoted
Hi,

On Tue, 4 Sep 2007, Pierre Habouzit wrote:

- IMHO the same goes for strbuf_free() instead of strbuf_wipe(), and
  Well, I don't like strbuf_free, because it's opposed to
strbuf_alloc/new whatever, that would be functions that create and
release a struct strbuf * (meaning allocating the struct shell as well).
Here you only free the internal buffer, not the shell, so _free() would
be a very bad name.

  In my own coding rules, I have _new/_delete and _init/_wipe functions,
the former acts on pointers, the latter on the structs. Hence the
naming. Though, looking at git's code, it seems that the usual name for
this operation is _release. So would you go for strbuf_release ?
release is indeed better. To me, "wipe" means zeroing out (possibly after
an optional number of passes writing trash to the memory area) rather than
actually freeing any memory.
quoted
- it would be nice to split this patch into

	- the API change (with _minimal_ changes to anything outside of 
	  strbuf.[ch]), and

	- the cleanups in the rest of the code.
  I'll try to do that, but it's quite hard to achieve knowing that in
many places of the current state of master, there are embeded NUL's
(accounted in ->len). I'll try to see what I can split, but it's
unlikely I'll be able to have an intermediate _working_ state (I mean
that would pass the testsuite) with the new API/semantics _and_ without
touching a lot less of the rest of the code.
Always having a compilable/operational tree is pretty nice primarily due
to bisect. If the code turns from working to non-working in between,
make sure to jot it down in the commit message.

It might even be better to add the embedded NUL handling code in the
first patch, modify the callers in the second, and remove the embedded
NUL handling in a third.

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

[PATCH] Simplify strbuf uses in fast-import.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

[PATCH] 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 |   29 ++++++++++++-----------------
 1 files changed, 12 insertions(+), 17 deletions(-)
diff --git a/fast-import.c b/fast-import.c
index 2f7baf4..1a02481 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,15 @@ static void cmd_mark(void)
 
 static void *cmd_data (size_t *size)
 {
-	size_t length;
-	char *buffer;
+	struct strbuf buffer = STRBUF_INIT;
 
 	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 +1654,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 +1674,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

[PATCH] Rework strbuf API and semantics.

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

  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      |   85 +++++++++++++++++++++++++++++++++++++++++++++++---------
 strbuf.h      |   41 ++++++++++++++++++++++++++-
 5 files changed, 119 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..db19007 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -1,41 +1,98 @@
 #include "cache.h"
 #include "strbuf.h"
 
+#define STRBUF_GROW_STEP  (1 << 10)  /* must be a power of 2 */
+
 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)
+		sb->buf[sb->len = 0] = '\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 + STRBUF_GROW_STEP < sb->len)
+		die("you want to use way to much memory");
+
+	sb->alloc = ((sb->len + extra) + STRBUF_GROW_STEP) & ~(STRBUF_GROW_STEP - 1);
+	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);
+	sb->len += len;
+	sb->buf[sb->len] = '\0';
 }
 
-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_addvf(struct strbuf *sb, const char *fmt, va_list ap)
+{
+	size_t len;
+	va_list ap2;
+
+	va_copy(ap2, ap);
+
+	len = vsnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, ap);
+	if (len < 0) {
+		len = 0;
+	}
+	if (len >= sb->alloc - sb->len) {
+		strbuf_grow(sb, len);
+		len = vsnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, ap2);
+		if (len >= sb->alloc - sb->len) {
+			len = sb->alloc - sb->len - 1;
+		}
 	}
-	sb->buf[sb->len++] = ch;
+	sb->len = sb->len + len;
+	sb->buf[sb->len] = '\0';
 }
 
-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) {
+		sb->len += res;
+		sb->buf[sb->len] = '\0';
+	}
+	return res;
 }
 
 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..ec50eeb 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -1,13 +1,50 @@
 #ifndef STRBUF_H
 #define STRBUF_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 *);
+
+
+extern void strbuf_grow(struct strbuf *, size_t);
+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);
+}
+static inline void strbuf_addch(struct strbuf *sb, size_t c) {
+	strbuf_grow(sb, 1);
+	sb->buf[sb->len++] = c;
+	sb->buf[sb->len] = '\0';
+}
+
+__attribute__((format(printf,2,0)))
+extern void strbuf_addvf(struct strbuf *, const char *, va_list);
+
+static inline __attribute__((format(printf,2,3)))
+void strbuf_addf(struct strbuf *sb, const char *fmt, ...) {
+	va_list ap;
+	va_start(ap, fmt);
+	strbuf_addvf(sb, fmt, ap);
+	va_end(ap);
+}
+
+
+extern size_t strbuf_fread(struct strbuf *, size_t, FILE *);
+
 extern void read_line(struct strbuf *, FILE *, int);
 
 #endif /* STRBUF_H */
-- 
1.5.3

Re: [PATCH] Rework strbuf API and semantics.

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

Hi,

On Tue, 4 Sep 2007, Pierre Habouzit wrote:
+void strbuf_grow(struct strbuf *sb, size_t extra) {
+	if (sb->len + extra + STRBUF_GROW_STEP < sb->len)
+		die("you want to use way to much memory");
+
+	sb->alloc = ((sb->len + extra) + STRBUF_GROW_STEP) & ~(STRBUF_GROW_STEP - 1);
+	sb->buf   = xrealloc(sb->buf, sb->alloc);
+}
Why not use ALLOC_GROW()?  Seems to me more efficient than growing by 1kB 
blocks all the time, for big strings as for short strings.

Ciao,
Dscho

Re: [PATCH] Rework strbuf API and semantics.

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

On Tue, Sep 04, 2007 at 03:44:30PM +0000, Johannes Schindelin wrote:
Hi,

On Tue, 4 Sep 2007, Pierre Habouzit wrote:
quoted
+void strbuf_grow(struct strbuf *sb, size_t extra) {
+	if (sb->len + extra + STRBUF_GROW_STEP < sb->len)
+		die("you want to use way to much memory");
+
+	sb->alloc = ((sb->len + extra) + STRBUF_GROW_STEP) & ~(STRBUF_GROW_STEP - 1);
+	sb->buf   = xrealloc(sb->buf, sb->alloc);
+}
Why not use ALLOC_GROW()?  Seems to me more efficient than growing by 1kB 
blocks all the time, for big strings as for short strings.
  ooooh, now I'm guilty of not knowing all git APIs very well yet :) Indeed,
this should just be:

    void strbuf_grow(struct strbuf *sb, size_t extra) {
        if (sb->len + extra + 1 < sb->len)
            die("you want to use way to much memory");
        ALLOC_GROW(sb->buf, sb->len + extra + 1, sb->alloc);
    }

  This is definitely better on so many levels !

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

Re: [PATCH] Rework strbuf API and semantics.

From: Wincent Colaiuta <hidden>
Date: 2016-06-15 22:43:33

El 4/9/2007, a las 18:18, Pierre Habouzit escribió:
  ooooh, now I'm guilty of not knowing all git APIs very well  
yet :) Indeed,
this should just be:

    void strbuf_grow(struct strbuf *sb, size_t extra) {
        if (sb->len + extra + 1 < sb->len)
            die("you want to use way to much memory");
        ALLOC_GROW(sb->buf, sb->len + extra + 1, sb->alloc);
    }
"too much memory", not "to much memory"

Cheers,
Wincent

Re: [PATCH] Simplify strbuf uses in fast-import.c using the proper functions.

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

Pierre Habouzit schrieb:
  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(-)
Apart from the wrong subject I really like this patch. :-D

Thanks!
René

Re: [PATCH] Simplify strbuf uses in fast-import.c using the proper functions.

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

Pierre Habouzit schrieb:
  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(-)
Apart from the wrong subject line I really like this patch. :-D

Thanks!
René

Re: [PATCH] Simplify strbuf uses in fast-import.c using the proper functions.

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

On Tue, Sep 04, 2007 at 11:46:59PM +0000, René Scharfe wrote:
Pierre Habouzit schrieb:
quoted
  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(-)
Apart from the wrong subject line I really like this patch. :-D
  oh boy, yes I fixed that in my local patch collection. I'm waiting for
a few hours (days ?) to see if there will be some more comments, I've
integrated every single one done here already (and some I had on IRC
too), and I'll repost a new clean series that I intend to be a real
proposal for inclusion.

  And yes, this patch is a perfect example of the gain we have to share
a common buffer API. The code looks (at least to me) way nicer, and if
you look in the details, we perform as many memory allocations, copies,
and so on as in the previous version.

-- 
·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