Am 27.06.23 um 10:57 schrieb Jeff King: > 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é
