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

From: Jonathan Nieder <[email-masked]>
Date: Wed, 3 Dec 2014 13:59:18 -0800

Junio C Hamano wrote:
> Jonathan Nieder <[email-masked]> writes:

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

I could do that.  It's less efficient but if the prevailing sentiment
is that it's worth it then I don't mind.

Would adding a comment to the implementation of strbuf_prefixf help?

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

Yes, that's the idea.

I'll do the tc_err thing, since I'm not getting a clear feeling that
I've offered enough motivation for the prefixf approach.

Jonathan