Re: [PATCH 5/5] strbuf: simplify strbuf_expand_literal_cb()

From: René Scharfe <l.s.r@web.de>
Date: Jun 27, 2023, 16:32 UTC
Message-ID: <3654f95d-5709-28f6-eeb2-ca62a1ee789c@web.de>
In-Reply-To: <20230627085701.GK1226768@coredump.intra.peff.net>

Am 27.06.23 um 10:57 schrieb Jeff King:

Show 26 quoted lines

> On Sat, Jun 17, 2023 at 10:44:00PM +0200, René Scharfe wrote:
>
>> Now that strbuf_expand_literal_cb() is no longer used as a callback,
>> drop its "_cb" name suffix and unused context parameter.
>
> Makes sense.
>
> Since most callers just call "format += len", it kind of feels like the
> appropriate interface might be more like:
>
>   strbuf_expand_literal(&sb, &format);
>
> to auto-advance the format. But I guess that gets weird with this
> caller:
>
>> @@ -1395,7 +1395,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
>>  	char **slot;
>>
>>  	/* these are independent of the commit */
>> -	res = strbuf_expand_literal_cb(sb, placeholder, NULL);
>> +	res = strbuf_expand_literal(sb, placeholder);
>>  	if (res)
>>  		return res;
>
> which is still in the "return the length" mentality (OTOH, if it
> advanced the local copy of the placeholder pointer nobody would mind).

Yes, I only grabbed the two low-hanging fruits here.

A format-advancing version would also look a bit weird in an if/else cascade:

	else if (strbuf_expand_literal(&sb, &format))
		; /* nothing */
	else ...

> I dunno. It is a little thing, and I am OK with it either way (I would
> not even think of changing it if you were not already touching the
> function).

Unsure.  Should I overcome my horror vacui and let the /* nothing */ in?

René