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

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 3, 2014, 21:45 UTC
Message-ID: <xmqqbnnkwgpg.fsf@gitster.dls.corp.google.com>
In-Reply-To: <20141203051016.GK6527@google.com>

Jonathan Nieder <jrnieder@gmail.com> writes:

Show 7 quoted lines

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

Show 15 quoted lines

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