Am 27.06.23 um 10:57 schrieb Jeff King:

On Sat, Jun 17, 2023 at 10:44:00PM +0200, René Scharfe wrote:

quoted

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:

quoted

@@ -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é