Re: [PATCH] Rework strbuf API and semantics.

Pierre Habouzit [off-list ref] writes:

Hmmmmm...  Didn't we recently had a patch that used va_copy()
which was not available somewhere and was shot down?

Instead of that nice inline strbuf_addf(), you may have to do
something like:

	strbuf_addf(..., fmt, ...) {
                va_list ap;

                va_start(ap, fmt);
                len = vsnprintf(...);
                va_end(ap);
                if (len >= sb_avail(sb)) {
                        strbuf_grow(sb, len);
                        va_start(ap, fmt);
                        len = vsnprintf(...);
                        va_end(ap);
                        if (len >= sb_avail(sb))
                                gaah();
                }
		sb->len += len;
                sb->buf[sb->len] = '\0';
	}

+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) {
+			sb->buf[sb->len = oldlen] = '\0';

Assignment inside array subscript is very hard to read.
Besides, what's the error semantics?  On error, this behaves as
if no bytes are read (i.e. partial read results in the initial
round is lost forever)?

On Thu, Sep 06, 2007 at 10:03:57AM +0000, Junio C Hamano wrote:

Pierre Habouzit [off-list ref] writes:

quoted

diff --git a/strbuf.c b/strbuf.c

...
+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->len = sb->len + len;
+	sb->buf[sb->len] = '\0';
 }

Hmmmmm...  Didn't we recently had a patch that used va_copy()
which was not available somewhere and was shot down?

Instead of that nice inline strbuf_addf(), you may have to do
something like:

	strbuf_addf(..., fmt, ...) {
                va_list ap;

                va_start(ap, fmt);
                len = vsnprintf(...);
                va_end(ap);
                if (len >= sb_avail(sb)) {
                        strbuf_grow(sb, len);
                        va_start(ap, fmt);
                        len = vsnprintf(...);
                        va_end(ap);
                        if (len >= sb_avail(sb))
                                gaah();
                }
		sb->len += len;
                sb->buf[sb->len] = '\0';
	}

  I'll do that.

quoted

+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) {
+			sb->buf[sb->len = oldlen] = '\0';

Assignment inside array subscript is very hard to read.
Besides, what's the error semantics?  On error, this behaves as
if no bytes are read (i.e. partial read results in the initial
round is lost forever)?

  Yes that is the semantics, because it's how it was used everywhere in
git: either we were able to load all the data from the file descriptor,
or weren't able to and the next thing we do is to die().

  Maybe I should call the function strbuf_xread() instead ?
-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org