Jonathan Nieder <jrnieder@xxxxxxxxx> writes:
> Junio C Hamano wrote:
>> Jonathan Nieder <jrnieder@xxxxxxxxx> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html