On Thu, Sep 06, 2007 at 09:31:37AM +0000, Junio C Hamano wrote:
> Pierre Habouzit <madcoder@debian.org> writes:
> 
> >   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.
> 
> "Rework API semantics" needs to be accompanied with an API
> description, perhaps at the beginning of each externally
> visible function.
> 
> Also the commit log message needs to explain what the old
> semantics was and what the improved one is, to highlight the
> changes needed to the callers.  Especially...
> 
> > @@ -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);
> 
> .... it is not all obvious why these off-by-one changes are
> needed without such a description.  The other hunks in this
> patch to this file are all such changes.

  Yes, as I suppose you know, but I state it here again so that
everybody understands, before strbuf's were merely a byte array, not
necessarily NUL-terminated. Hence many parts of the code that wanted to
pass the buffer to str* functions had to manually insert a NUL, hence it
was accounted in the length of the buffer.

  Now, we always have a NUL after the "official" end of the buffer, so
it's not needed anymore. The off-by-ones are just that. The hunk you
quote is one where git's code was messing with strbufs internals
directly, so the ALLOC_GROW has to take the 1 octed needed to maintain
the internal invariant. Though, the patch after this one rewrites the
hunk to use strbuf's API's.

> 
> > -static void inline strbuf_add(struct strbuf *sb, int ch) {
> 
> > +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';
> > +}
> 
> You certainly did not mean size_t wide characters.

  Oh boy, now I've been red-handed of :%s/\<int\>/\<size_t\>/ :)

  I'll repost a _clean_ patch series soon with those things fixed, and
the wrong overflow test (that should be a >= and not a >) as well, as we
discussed it on IRC before.

  Oh and FWIW I believe the details of the NUL always after the buffer
array has to be in strbuf.h and not in the commit comment, maybe I'll
put it in both to make everybody happy.

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