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

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

Jonathan Nieder <jrnieder@gmail.com> writes:

> Junio C Hamano wrote:
>> Jonathan Nieder <jrnieder@gmail.com> 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?

Perhaps.

The reason why it felt strange to me was primarily because this was
a short-hand way of writing something like this in the caller:

	if (transaction_commit(&t, err)) {
		struct strbuf scratch = STRBUF_INIT;
		strbuf_addf(&scratch, "cannot fetch '%s': ", remotename);
		strbuf_splice(err, 0, 0, sctach.buf, scratch.len);
                strbuf_reset(&scratch);
	}

Coming from that point of view, it looked strange not to be using a
separate scratch area; that's all.