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

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 3, 2014, 22:40 UTC
Message-ID: <xmqq388wwe58.fsf@gitster.dls.corp.google.com>
In-Reply-To: <20141203215918.GF6527@google.com>

Jonathan Nieder <jrnieder@gmail.com> writes:

Show 28 quoted lines

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