Re: [PATCH] Rework strbuf API and semantics.

[Pierre Habouzit <madcoder@debian.org>]

[-- Attachment #1: Type: text/plain, Size: 2576 bytes --]

On Thu, Sep 06, 2007 at 10:03:57AM +0000, Junio C Hamano wrote:
> Pierre Habouzit <madcoder@debian.org> writes:
> 
> > 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.

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

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]