Am 27.06.23 um 10:54 schrieb Jeff King:

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

quoted

Avoid the overhead of passing context to a callback function of
strbuf_expand() by using strbuf_expand_step() in a loop instead.  It
requires explicit handling of %% and unrecognized placeholders, but is
simpler, more direct and avoids void pointers.

I like this. I don't care that much about the runtime overhead of
passing the context around, but if you meant by "overhead" the
programmer time and confusion in stuffing everything into context
structs, then I agree this is much better. :)

Indeed, I meant the burden of being forced to define a struct and
filling in all necessary context.  Bureaucratic overhead.

It does still feel like we should be handling "%%" on behalf of the
callers.

I feel the same, but restrained myself from doing that, so that we
can see all the pieces for now.  It allows us to recombine them in
better ways than before.

quoted

 builtin/cat-file.c |  35 +++++++--------
 builtin/ls-files.c | 109 +++++++++++++++++++--------------------------
 builtin/ls-tree.c  | 107 +++++++++++++++++---------------------------
 daemon.c           |  61 ++++++++-----------------
 pretty.c           |  72 ++++++++++++++++++------------
 strbuf.c           |  20 ---------
 strbuf.h           |  37 ++-------------
 7 files changed, 169 insertions(+), 272 deletions(-)

The changes mostly looked OK to me (and the diffstat is certainly
pleasing). The old callbacks returned a "consumed" length, and we need
each "step" caller to now do "format += consumed" themselves. At first
glance I thought there were cases where you didn't, but then I realized
that you are relying on skip_prefix() to do that incrementing. Which
makes sense and the resulting code looks nice, but it took me a minute
to realize what was going on.

*nod*  The "returns consumed length" style is still used by
strbuf_expand_literal, though, so we have a bit of a mix.  Which
works, but a uniform convention might be better.

quoted

@@ -1894,7 +1880,26 @@ void userformat_find_requirements(const char *fmt, struct userformat_want *w)
 			return;
 		fmt = user_format;
 	}
-	strbuf_expand(&dummy, fmt, userformat_want_item, w);
+	while (strbuf_expand_step(&dummy, &fmt)) {
+		if (skip_prefix(fmt, "%", &fmt))
+			continue;
+
+		if (*fmt == '+' || *fmt == '-' || *fmt == ' ')
+			fmt++;
+
+		switch (*fmt) {
+		case 'N':
+			w->notes = 1;
+			break;
+		case 'S':
+			w->source = 1;
+			break;
+		case 'd':
+		case 'D':
+			w->decorate = 1;
+			break;
+		}
+	}
 	strbuf_release(&dummy);
 }

This one actually doesn't increment the format (so we restart the
expansion on "N" or whatever). But neither did the original! It always
returned 0:

quoted

-static size_t userformat_want_item(struct strbuf *sb UNUSED,
-				   const char *placeholder,
-				   void *context)
-{
-	struct userformat_want *w = context;
-
-	if (*placeholder == '+' || *placeholder == '-' || *placeholder == ' ')
-		placeholder++;
-
-	switch (*placeholder) {
-	case 'N':
-		w->notes = 1;
-		break;
-	case 'S':
-		w->source = 1;
-		break;
-	case 'd':
-	case 'D':
-		w->decorate = 1;
-		break;
-	}
-	return 0;
-}

So probably OK, though a little funny.

It also feels like this whole function would be just as happy using
"strchr()", since it throws away the expanded result anyway. But that
can be for another time. :)

Good idea!  And the conversion to a loop brings us halfway there.

quoted

@@ -1912,7 +1917,16 @@ void repo_format_commit_message(struct repository *r,
 	const char *output_enc = pretty_ctx->output_encoding;
 	const char *utf8 = "UTF-8";

-	strbuf_expand(sb, format, format_commit_item, &context);
+	while (strbuf_expand_step(sb, &format)) {
+		size_t len;
+
+		if (skip_prefix(format, "%", &format))
+			strbuf_addch(sb, '%');
+		else if ((len = format_commit_item(sb, format, &context)))
+			format += len;
+		else
+			strbuf_addch(sb, '%');
+	}

This one doesn't advance the format for a not-understood placeholder.
But that's OK, because we know it isn't "%", so starting the search from
there again is correct.

Right.  This is copied from strbuf_expand.

quoted

@@ -1842,7 +1852,7 @@ static size_t format_commit_item(struct strbuf *sb, /* in UTF-8 */
 	}

 	orig_len = sb->len;
-	if (((struct format_commit_context *)context)->flush_type != no_flush)
+	if ((context)->flush_type != no_flush)
 		consumed = format_and_pad_commit(sb, placeholder, context);
 	else
 		consumed = format_commit_one(sb, placeholder, context);

Since we're no longer casting, the extra parentheses seem redundant now.
I.e., this can just be context->flush_type.

Indeed.

René