Re: [PATCH] Rework strbuf API and semantics.

[Junio C Hamano <gitster@pobox.com>]

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.

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