On Tue, Jun 27, 2023 at 06:32:22PM +0200, René Scharfe wrote: > 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? Heh. It is a little funny to have an empty block. But at the same time, it aligns the conditional with all of the skip_prefix() calls, which are advancing "format" as a side effect of matching. So I could go either way (and we can always change it on top). I think based on your responses that there's nothing that would require a re-roll. The only thing left from my review is the extra parentheses in format_commit_item, which could possibly be fixed up while applying. -Peff
