Subject: Re: [PATCH 01/14] strbuf: introduce strbuf_prefixf()

From: Junio C Hamano <[email-masked]>
Date: Wed, 03 Dec 2014 13:45:15 -0800

Jonathan Nieder <[email-masked]> writes:

> When preparing an error message in a strbuf, it can be convenient
> to add a formatted string to the beginning:
>
> 	if (transaction_commit(&t, err)) {
> 		strbuf_prefixf(err, "cannot fetch '%s': ", remotename);
> 		return -1;
> 	}

I am somewhat unhappy with this justification, as it is not very
clear why "cannot fetch '%s': " must come at the beginning without
knowing what kind of string transaction_commit() is expected to add
to err when it is called.  It could be a sign that the convention
for transaction_commit() to report its errors is screwed up, and
not a sign that prefixf is necessary (not that I think that is the
case---there is not enough explanation here to decide).

> +void strbuf_prefixf(struct strbuf *sb, const char *fmt, ...)
> +{
> +	va_list ap;
> +	size_t pos, len;
> +
> +	pos = sb->len;
> +
> +	va_start(ap, fmt);
> +	strbuf_vaddf(sb, fmt, ap);
> +	va_end(ap);
> +
> +	len = sb->len - pos;
> +	strbuf_insert(sb, 0, sb->buf + pos, len);
> +	strbuf_remove(sb, pos + len, len);
> +}

This indeed is strange to read; it would be more straightforward to
use a second strbuf for temporary storage you need to do this,
instead of using the tail-end of the original strbuf and shuffling
bytes around.

In any case, instead of this:

	struct strbuf tc_err = STRBUF_INIT;
        if (transaction_commit(&t, &tc_err)) {
		strbuf_addf(err, "cannot fetch '%s': %s", remotename, 
			tc_err.buf);        
		strbuf_release(&tc_err);
		return -1;
	}	                

you can use the four-line version you cited above, which might be an
improvement.  I dunno if it is such a big deal, though.