[PATCH 0/5] replace strbuf_expand()

[PATCH 0/5] replace strbuf_expand()

[René Scharfe]

strbuf_expand() is used to interpolate placeholders that start with a
percent sign (%) in strings.  It invokes a callback function to handle
those placeholders, expands "%%" to "%" itself and copies unrecognized
placeholders verbatim.  Context info for the callback needs to be
passed in via a void pointer, e.g. to a custom struct.

There is a better, simpler way: Use a loop to iterate over placeholders
and access variables directly, without the need to take a detour through
a context struct.  This series converts the callers of strbuf_expand()
to use the new helper strbuf_expand_step(), removing the overhead that
comes with using a callback.

  pretty: factor out expand_separator()
  strbuf: factor out strbuf_expand_step()
  replace strbuf_expand_dict_cb() with strbuf_expand_step()
  replace strbuf_expand() with strbuf_expand_step()
  strbuf: simplify strbuf_expand_literal_cb()

 builtin/branch.c   |  13 +-----
 builtin/cat-file.c |  35 +++++++--------
 builtin/ls-files.c | 109 +++++++++++++++++++--------------------------
 builtin/ls-tree.c  | 107 +++++++++++++++++---------------------------
 convert.c          |  22 +++++----
 daemon.c           |  61 ++++++++-----------------
 ll-merge.c         |  32 +++++++------
 pretty.c           |  99 ++++++++++++++++++++++------------------
 strbuf.c           |  58 +++++-------------------
 strbuf.h           |  57 ++++--------------------
 10 files changed, 228 insertions(+), 365 deletions(-)

--
2.41.0

[PATCH 1/5] pretty: factor out expand_separator()

[René Scharfe]

Deduplicate the code for setting the options "separator" and
"key_value_separator" by moving it into a new helper function,
expand_separator().

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 pretty.c | 27 +++++++++++++--------------
 1 file changed, 13 insertions(+), 14 deletions(-)

diff --git a/pretty.c b/pretty.c
index 0bb938021b..d2df561a05 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1250,6 +1250,17 @@ static int format_trailer_match_cb(const struct strbuf *key, void *ud)
 	return 0;
 }

+static struct strbuf *expand_separator(struct strbuf *sb,
+				       const char *argval, size_t arglen)
+{
+	char *fmt = xstrndup(argval, arglen);
+
+	strbuf_reset(sb);
+	strbuf_expand(sb, fmt, strbuf_expand_literal_cb, NULL);
+	free(fmt);
+	return sb;
+}
+
 int format_set_trailers_options(struct process_trailer_options *opts,
 				struct string_list *filter_list,
 				struct strbuf *sepbuf,
@@ -1278,21 +1289,9 @@ int format_set_trailers_options(struct process_trailer_options *opts,
 			opts->filter_data = filter_list;
 			opts->only_trailers = 1;
 		} else if (match_placeholder_arg_value(*arg, "separator", arg, &argval, &arglen)) {
-			char *fmt;
-
-			strbuf_reset(sepbuf);
-			fmt = xstrndup(argval, arglen);
-			strbuf_expand(sepbuf, fmt, strbuf_expand_literal_cb, NULL);
-			free(fmt);
-			opts->separator = sepbuf;
+			opts->separator = expand_separator(sepbuf, argval, arglen);
 		} else if (match_placeholder_arg_value(*arg, "key_value_separator", arg, &argval, &arglen)) {
-			char *fmt;
-
-			strbuf_reset(kvsepbuf);
-			fmt = xstrndup(argval, arglen);
-			strbuf_expand(kvsepbuf, fmt, strbuf_expand_literal_cb, NULL);
-			free(fmt);
-			opts->key_value_separator = kvsepbuf;
+			opts->key_value_separator = expand_separator(kvsepbuf, argval, arglen);
 		} else if (!match_placeholder_bool_arg(*arg, "only", arg, &opts->only_trailers) &&
 			   !match_placeholder_bool_arg(*arg, "unfold", arg, &opts->unfold) &&
 			   !match_placeholder_bool_arg(*arg, "keyonly", arg, &opts->key_only) &&
--
2.41.0

[PATCH 2/5] strbuf: factor out strbuf_expand_step()

[René Scharfe]

Extract the part of strbuf_expand that finds the next placeholder into a
new function.  It allows to build parsers without callback functions and
the overhead imposed by them.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 builtin/branch.c | 13 ++-----------
 strbuf.c         | 28 ++++++++++++++--------------
 strbuf.h         |  8 ++++++++
 3 files changed, 24 insertions(+), 25 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index e6c2655af6..7c20e049a2 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -366,17 +366,8 @@ static const char *quote_literal_for_format(const char *s)
 	static struct strbuf buf = STRBUF_INIT;

 	strbuf_reset(&buf);
-	while (*s) {
-		const char *ep = strchrnul(s, '%');
-		if (s < ep)
-			strbuf_add(&buf, s, ep - s);
-		if (*ep == '%') {
-			strbuf_addstr(&buf, "%%");
-			s = ep + 1;
-		} else {
-			s = ep;
-		}
-	}
+	while (strbuf_expand_step(&buf, &s))
+		strbuf_addstr(&buf, "%%");
 	return buf.buf;
 }

diff --git a/strbuf.c b/strbuf.c
index 08eec8f1d8..a90b597da1 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -415,19 +415,24 @@ void strbuf_vaddf(struct strbuf *sb, const char *fmt, va_list ap)
 	strbuf_setlen(sb, sb->len + len);
 }

+int strbuf_expand_step(struct strbuf *sb, const char **formatp)
+{
+	const char *format = *formatp;
+	const char *percent = strchrnul(format, '%');
+
+	strbuf_add(sb, format, percent - format);
+	if (!*percent)
+		return 0;
+	*formatp = percent + 1;
+	return 1;
+}
+
 void strbuf_expand(struct strbuf *sb, const char *format, expand_fn_t fn,
 		   void *context)
 {
-	for (;;) {
-		const char *percent;
+	while (strbuf_expand_step(sb, &format)) {
 		size_t consumed;

-		percent = strchrnul(format, '%');
-		strbuf_add(sb, format, percent - format);
-		if (!*percent)
-			break;
-		format = percent + 1;
-
 		if (*format == '%') {
 			strbuf_addch(sb, '%');
 			format++;
@@ -1022,12 +1027,7 @@ void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm,
 	 * we want for %z, but the computation for %s has to convert to number
 	 * of seconds.
 	 */
-	for (;;) {
-		const char *percent = strchrnul(fmt, '%');
-		strbuf_add(&munged_fmt, fmt, percent - fmt);
-		if (!*percent)
-			break;
-		fmt = percent + 1;
+	while (strbuf_expand_step(&munged_fmt, &fmt)) {
 		switch (*fmt) {
 		case '%':
 			strbuf_addstr(&munged_fmt, "%%");
diff --git a/strbuf.h b/strbuf.h
index 3dfeadb44c..a189f12b84 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -371,6 +371,14 @@ size_t strbuf_expand_dict_cb(struct strbuf *sb,
 			     const char *placeholder,
 			     void *context);

+/**
+ * If the string pointed to by `formatp` contains a percent sign ("%"),
+ * advance it to point to the character following the next one and
+ * return 1, otherwise return 0.  Append the substring before that
+ * percent sign to `sb`, or the whole string if there is none.
+ */
+int strbuf_expand_step(struct strbuf *sb, const char **formatp);
+
 /**
  * Append the contents of one strbuf to another, quoting any
  * percent signs ("%") into double-percents ("%%") in the
--
2.41.0

Re: [PATCH 2/5] strbuf: factor out strbuf_expand_step()

[Taylor Blau]

On Sat, Jun 17, 2023 at 10:41:44PM +0200, René Scharfe wrote:
> diff --git a/strbuf.c b/strbuf.c
> index 08eec8f1d8..a90b597da1 100644
> --- a/strbuf.c
> +++ b/strbuf.c
> @@ -415,19 +415,24 @@ void strbuf_vaddf(struct strbuf *sb, const char *fmt, va_list ap)
>  	strbuf_setlen(sb, sb->len + len);
>  }
>
> +int strbuf_expand_step(struct strbuf *sb, const char **formatp)
> +{
> +	const char *format = *formatp;
> +	const char *percent = strchrnul(format, '%');

Can format be NULL here? Obviously formatp is never NULL since it is
`&s`, but we guarded the while loop in the pre-image with `while (*s)`
and I am not sure that **formatp is always non-NULL here.

Thanks,
Taylor

Re: [PATCH 2/5] strbuf: factor out strbuf_expand_step()

[René Scharfe]

Am 19.06.23 um 18:10 schrieb Taylor Blau:
> On Sat, Jun 17, 2023 at 10:41:44PM +0200, René Scharfe wrote:
>> diff --git a/strbuf.c b/strbuf.c
>> index 08eec8f1d8..a90b597da1 100644
>> --- a/strbuf.c
>> +++ b/strbuf.c
>> @@ -415,19 +415,24 @@ void strbuf_vaddf(struct strbuf *sb, const char *fmt, va_list ap)
>>  	strbuf_setlen(sb, sb->len + len);
>>  }
>>
>> +int strbuf_expand_step(struct strbuf *sb, const char **formatp)
>> +{
>> +	const char *format = *formatp;
>> +	const char *percent = strchrnul(format, '%');
>
> Can format be NULL here? Obviously formatp is never NULL since it is
> `&s`, but we guarded the while loop in the pre-image with `while (*s)`
> and I am not sure that **formatp is always non-NULL here.

The "*s" in builtin/branch.c::quote_literal_for_format() that you quote
dereferences "s", so we'd get a segfault if it was NULL.  A NULL (and
NUL) check would look like "while (s && *s)".  The old code in strbuf.c
passes the format to strchrnul(), which can't handle NULL either.  So
no, format must not be NULL here, and this is not a new requirement.

But now I noticed that builtin/branch.c::quote_literal_for_format()
only ever gets called with "" or "remotes/", none of which needs
quoting.  We could drop this function entirely, and add it back when
needed, if ever.  But that's out of the scope of this series.

René

Re: [PATCH 2/5] strbuf: factor out strbuf_expand_step()

[Taylor Blau]

On Tue, Jun 20, 2023 at 06:25:30PM +0200, René Scharfe wrote:
> Am 19.06.23 um 18:10 schrieb Taylor Blau:
> > On Sat, Jun 17, 2023 at 10:41:44PM +0200, René Scharfe wrote:
> >> diff --git a/strbuf.c b/strbuf.c
> >> index 08eec8f1d8..a90b597da1 100644
> >> --- a/strbuf.c
> >> +++ b/strbuf.c
> >> @@ -415,19 +415,24 @@ void strbuf_vaddf(struct strbuf *sb, const char *fmt, va_list ap)
> >>  	strbuf_setlen(sb, sb->len + len);
> >>  }
> >>
> >> +int strbuf_expand_step(struct strbuf *sb, const char **formatp)
> >> +{
> >> +	const char *format = *formatp;
> >> +	const char *percent = strchrnul(format, '%');
> >
> > Can format be NULL here? Obviously formatp is never NULL since it is
> > `&s`, but we guarded the while loop in the pre-image with `while (*s)`
> > and I am not sure that **formatp is always non-NULL here.
>
> The "*s" in builtin/branch.c::quote_literal_for_format() that you quote
> dereferences "s", so we'd get a segfault if it was NULL.  A NULL (and
> NUL) check would look like "while (s && *s)".  The old code in strbuf.c
> passes the format to strchrnul(), which can't handle NULL either.  So
> no, format must not be NULL here, and this is not a new requirement.

Ah, thanks for catching: I agree with your reasoning.

> But now I noticed that builtin/branch.c::quote_literal_for_format()
> only ever gets called with "" or "remotes/", none of which needs
> quoting.  We could drop this function entirely, and add it back when
> needed, if ever.  But that's out of the scope of this series.

Yes, agreed.

Thanks,
Taylor

Re: [PATCH 2/5] strbuf: factor out strbuf_expand_step()

[Jeff King]

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

> Extract the part of strbuf_expand that finds the next placeholder into a
> new function.  It allows to build parsers without callback functions and
> the overhead imposed by them.
> 
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
>  builtin/branch.c | 13 ++-----------
>  strbuf.c         | 28 ++++++++++++++--------------
>  strbuf.h         |  8 ++++++++
>  3 files changed, 24 insertions(+), 25 deletions(-)

I was a little surprised to see the change in branch.c here (as well as
the one in strbuf_addftime), since the commit message just says
"extract..." and doesn't mention them.

They do serve as examples of how it can be used, so I think it's OK. But
it made me wonder: is this all of the sites? Or are these just examples?

-Peff

Re: [PATCH 2/5] strbuf: factor out strbuf_expand_step()

[René Scharfe]

Am 27.06.23 um 10:26 schrieb Jeff King:
> On Sat, Jun 17, 2023 at 10:41:44PM +0200, René Scharfe wrote:
>
>> Extract the part of strbuf_expand that finds the next placeholder into a
>> new function.  It allows to build parsers without callback functions and
>> the overhead imposed by them.
>>
>> Signed-off-by: René Scharfe <l.s.r@web.de>
>> ---
>>  builtin/branch.c | 13 ++-----------
>>  strbuf.c         | 28 ++++++++++++++--------------
>>  strbuf.h         |  8 ++++++++
>>  3 files changed, 24 insertions(+), 25 deletions(-)
>
> I was a little surprised to see the change in branch.c here (as well as
> the one in strbuf_addftime), since the commit message just says
> "extract..." and doesn't mention them.

Fair point, the other extraction sources should have been mentioned as
well somehow.

> They do serve as examples of how it can be used, so I think it's OK. But
> it made me wonder: is this all of the sites? Or are these just examples?

These are all that I could find.

René

[PATCH 3/5] replace strbuf_expand_dict_cb() with strbuf_expand_step()

[René Scharfe]

Avoid the overhead of setting up a dictionary and passing it via
strbuf_expand() to strbuf_expand_dict_cb() by using strbuf_expand_step()
in a loop instead.  It requires explicit handling of %% and unrecognized
placeholders, but is more direct and simpler overall, and expands only
on demand.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 convert.c  | 22 ++++++++++------------
 ll-merge.c | 32 ++++++++++++++++++--------------
 strbuf.c   | 16 ----------------
 strbuf.h   | 14 --------------
 4 files changed, 28 insertions(+), 56 deletions(-)

diff --git a/convert.c b/convert.c
index 9ee79fe469..455d05cf6b 100644
--- a/convert.c
+++ b/convert.c
@@ -633,23 +633,21 @@ static int filter_buffer_or_fd(int in UNUSED, int out, void *data)
 	 */
 	struct child_process child_process = CHILD_PROCESS_INIT;
 	struct filter_params *params = (struct filter_params *)data;
+	const char *format = params->cmd;
 	int write_err, status;

 	/* apply % substitution to cmd */
 	struct strbuf cmd = STRBUF_INIT;
-	struct strbuf path = STRBUF_INIT;
-	struct strbuf_expand_dict_entry dict[] = {
-		{ "f", NULL, },
-		{ NULL, NULL, },
-	};
-
-	/* quote the path to preserve spaces, etc. */
-	sq_quote_buf(&path, params->path);
-	dict[0].value = path.buf;

-	/* expand all %f with the quoted path */
-	strbuf_expand(&cmd, params->cmd, strbuf_expand_dict_cb, &dict);
-	strbuf_release(&path);
+	/* expand all %f with the quoted path; quote to preserve space, etc. */
+	while (strbuf_expand_step(&cmd, &format)) {
+		if (skip_prefix(format, "%", &format))
+			strbuf_addch(&cmd, '%');
+		else if (skip_prefix(format, "f", &format))
+			sq_quote_buf(&cmd, params->path);
+		else
+			strbuf_addch(&cmd, '%');
+	}

 	strvec_push(&child_process.args, cmd.buf);
 	child_process.use_shell = 1;
diff --git a/ll-merge.c b/ll-merge.c
index 07ec16e8e5..b307ad293c 100644
--- a/ll-merge.c
+++ b/ll-merge.c
@@ -192,24 +192,15 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,
 			const struct ll_merge_options *opts,
 			int marker_size)
 {
-	char temp[4][50];
+	char temp[3][50];
 	struct strbuf cmd = STRBUF_INIT;
-	struct strbuf_expand_dict_entry dict[6];
-	struct strbuf path_sq = STRBUF_INIT;
+	const char *format = fn->cmdline;
 	struct child_process child = CHILD_PROCESS_INIT;
 	int status, fd, i;
 	struct stat st;
 	enum ll_merge_result ret;
 	assert(opts);

-	sq_quote_buf(&path_sq, path);
-	dict[0].placeholder = "O"; dict[0].value = temp[0];
-	dict[1].placeholder = "A"; dict[1].value = temp[1];
-	dict[2].placeholder = "B"; dict[2].value = temp[2];
-	dict[3].placeholder = "L"; dict[3].value = temp[3];
-	dict[4].placeholder = "P"; dict[4].value = path_sq.buf;
-	dict[5].placeholder = NULL; dict[5].value = NULL;
-
 	if (!fn->cmdline)
 		die("custom merge driver %s lacks command line.", fn->name);

@@ -218,9 +209,23 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,
 	create_temp(orig, temp[0], sizeof(temp[0]));
 	create_temp(src1, temp[1], sizeof(temp[1]));
 	create_temp(src2, temp[2], sizeof(temp[2]));
-	xsnprintf(temp[3], sizeof(temp[3]), "%d", marker_size);

-	strbuf_expand(&cmd, fn->cmdline, strbuf_expand_dict_cb, &dict);
+	while (strbuf_expand_step(&cmd, &format)) {
+		if (skip_prefix(format, "%", &format))
+			strbuf_addch(&cmd, '%');
+		else if (skip_prefix(format, "O", &format))
+			strbuf_addstr(&cmd, temp[0]);
+		else if (skip_prefix(format, "A", &format))
+			strbuf_addstr(&cmd, temp[1]);
+		else if (skip_prefix(format, "B", &format))
+			strbuf_addstr(&cmd, temp[2]);
+		else if (skip_prefix(format, "L", &format))
+			strbuf_addf(&cmd, "%d", marker_size);
+		else if (skip_prefix(format, "P", &format))
+			sq_quote_buf(&cmd, path);
+		else
+			strbuf_addch(&cmd, '%');
+	}

 	child.use_shell = 1;
 	strvec_push(&child.args, cmd.buf);
@@ -242,7 +247,6 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,
 	for (i = 0; i < 3; i++)
 		unlink_or_warn(temp[i]);
 	strbuf_release(&cmd);
-	strbuf_release(&path_sq);
 	ret = (status > 0) ? LL_MERGE_CONFLICT : status;
 	return ret;
 }
diff --git a/strbuf.c b/strbuf.c
index a90b597da1..972366b410 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -468,22 +468,6 @@ size_t strbuf_expand_literal_cb(struct strbuf *sb,
 	return 0;
 }

-size_t strbuf_expand_dict_cb(struct strbuf *sb, const char *placeholder,
-		void *context)
-{
-	struct strbuf_expand_dict_entry *e = context;
-	size_t len;
-
-	for (; e->placeholder && (len = strlen(e->placeholder)); e++) {
-		if (!strncmp(placeholder, e->placeholder, len)) {
-			if (e->value)
-				strbuf_addstr(sb, e->value);
-			return len;
-		}
-	}
-	return 0;
-}
-
 void strbuf_addbuf_percentquote(struct strbuf *dst, const struct strbuf *src)
 {
 	size_t i, len = src->len;
diff --git a/strbuf.h b/strbuf.h
index a189f12b84..e293117f07 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -357,20 +357,6 @@ size_t strbuf_expand_literal_cb(struct strbuf *sb,
 				const char *placeholder,
 				void *context);

-/**
- * Used as callback for `strbuf_expand()`, expects an array of
- * struct strbuf_expand_dict_entry as context, i.e. pairs of
- * placeholder and replacement string.  The array needs to be
- * terminated by an entry with placeholder set to NULL.
- */
-struct strbuf_expand_dict_entry {
-	const char *placeholder;
-	const char *value;
-};
-size_t strbuf_expand_dict_cb(struct strbuf *sb,
-			     const char *placeholder,
-			     void *context);
-
 /**
  * If the string pointed to by `formatp` contains a percent sign ("%"),
  * advance it to point to the character following the next one and
--
2.41.0

Re: [PATCH 3/5] replace strbuf_expand_dict_cb() with strbuf_expand_step()

[Jeff King]

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

> Avoid the overhead of setting up a dictionary and passing it via
> strbuf_expand() to strbuf_expand_dict_cb() by using strbuf_expand_step()
> in a loop instead.  It requires explicit handling of %% and unrecognized
> placeholders, but is more direct and simpler overall, and expands only
> on demand.

Great. I think even replacing the dictionary with regular
strbuf_expand() would be an improvement in terms of the on-demand
loading. But I am happy to see it go all the way to the iterative
version. :)

Your comment above does make me wonder if strbuf_expand_step() should be
quietly handling "%%" itself. But I guess strbuf_expand() doesn't, and
your branch.c quote-literal example probably would not want that
behavior.

-Peff

Re: [PATCH 3/5] replace strbuf_expand_dict_cb() with strbuf_expand_step()

[Jeff King]

On Tue, Jun 27, 2023 at 04:29:02AM -0400, Jeff King wrote:

> Your comment above does make me wonder if strbuf_expand_step() should be
> quietly handling "%%" itself. But I guess strbuf_expand() doesn't, and
> your branch.c quote-literal example probably would not want that
> behavior.

Er, nope, strbuf_expand() does handle "%%" itself. It really feels like
we'd want strbuf_expand_step() to do so, too, then. Even if we had two
variants (a "raw" one which doesn't handle "%%" so that branch.c could
use it, and then another that wrapped it to handle "%%").

-Peff

Re: [PATCH 3/5] replace strbuf_expand_dict_cb() with strbuf_expand_step()

[René Scharfe]

Am 27.06.23 um 10:35 schrieb Jeff King:
> On Tue, Jun 27, 2023 at 04:29:02AM -0400, Jeff King wrote:
>
>> Your comment above does make me wonder if strbuf_expand_step() should be
>> quietly handling "%%" itself. But I guess strbuf_expand() doesn't, and
>> your branch.c quote-literal example probably would not want that
>> behavior.
>
> Er, nope, strbuf_expand() does handle "%%" itself. It really feels like
> we'd want strbuf_expand_step() to do so, too, then. Even if we had two
> variants (a "raw" one which doesn't handle "%%" so that branch.c could
> use it, and then another that wrapped it to handle "%%").

strbuf_expand() handles %% and unrecognized placeholders.

We could do that, or move the %% handling to strbuf_expand_literal or
something else.  E.g. initially I had a strbuf_expand_default that
handled %% and unrecognized placeholders.

I think it's a good idea to first get used to the loop paradigm by
eschewing the sugary automatic handling and having everything spelled
out explicitly.  It's just one more branch.  Then we can come up with
a better factoring after a while.

René

[PATCH 4/5] replace strbuf_expand() with strbuf_expand_step()

[René Scharfe]

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.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 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(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 0bafc14e6c..424f39675b 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -309,10 +309,8 @@ static int is_atom(const char *atom, const char *s, int slen)
 }

 static void expand_atom(struct strbuf *sb, const char *atom, int len,
-			void *vdata)
+			struct expand_data *data)
 {
-	struct expand_data *data = vdata;
-
 	if (is_atom("objectname", atom, len)) {
 		if (!data->mark_query)
 			strbuf_addstr(sb, oid_to_hex(&data->oid));
@@ -346,19 +344,21 @@ static void expand_atom(struct strbuf *sb, const char *atom, int len,
 		die("unknown format element: %.*s", len, atom);
 }

-static size_t expand_format(struct strbuf *sb, const char *start, void *data)
+static void expand_format(struct strbuf *sb, const char *start,
+			  struct expand_data *data)
 {
-	const char *end;
-
-	if (*start != '(')
-		return 0;
-	end = strchr(start + 1, ')');
-	if (!end)
-		die("format element '%s' does not end in ')'", start);
-
-	expand_atom(sb, start + 1, end - start - 1, data);
-
-	return end - start + 1;
+	while (strbuf_expand_step(sb, &start)) {
+		const char *end;
+
+		if (skip_prefix(start, "%", &start) || *start != '(')
+			strbuf_addch(sb, '%');
+		else if (!(end = strchr(start + 1, ')')))
+			die("format element '%s' does not end in ')'", start);
+		else {
+			expand_atom(sb, start + 1, end - start - 1, data);
+			start = end + 1;
+		}
+	}
 }

 static void batch_write(struct batch_options *opt, const void *data, int len)
@@ -494,7 +494,7 @@ static void batch_object_write(const char *obj_name,
 	if (!opt->format) {
 		print_default_format(scratch, data);
 	} else {
-		strbuf_expand(scratch, opt->format, expand_format, data);
+		expand_format(scratch, opt->format, data);
 		strbuf_addch(scratch, '\n');
 	}

@@ -777,9 +777,8 @@ static int batch_objects(struct batch_options *opt)
 	 */
 	memset(&data, 0, sizeof(data));
 	data.mark_query = 1;
-	strbuf_expand(&output,
+	expand_format(&output,
 		      opt->format ? opt->format : DEFAULT_FORMAT,
-		      expand_format,
 		      &data);
 	data.mark_query = 0;
 	strbuf_release(&output);
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 72012c0f0f..03bf5771b4 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -262,74 +262,57 @@ static void expand_objectsize(struct strbuf *line, const struct object_id *oid,
 		strbuf_addstr(line, "-");
 	}
 }
-struct show_index_data {
-	const char *pathname;
-	struct index_state *istate;
-	const struct cache_entry *ce;
-};
-
-static size_t expand_show_index(struct strbuf *sb, const char *start,
-				void *context)
-{
-	struct show_index_data *data = context;
-	const char *end;
-	const char *p;
-	size_t len = strbuf_expand_literal_cb(sb, start, NULL);
-	struct stat st;
-
-	if (len)
-		return len;
-	if (*start != '(')
-		die(_("bad ls-files format: element '%s' "
-		      "does not start with '('"), start);
-
-	end = strchr(start + 1, ')');
-	if (!end)
-		die(_("bad ls-files format: element '%s' "
-		      "does not end in ')'"), start);
-
-	len = end - start + 1;
-	if (skip_prefix(start, "(objectmode)", &p))
-		strbuf_addf(sb, "%06o", data->ce->ce_mode);
-	else if (skip_prefix(start, "(objectname)", &p))
-		strbuf_add_unique_abbrev(sb, &data->ce->oid, abbrev);
-	else if (skip_prefix(start, "(objecttype)", &p))
-		strbuf_addstr(sb, type_name(object_type(data->ce->ce_mode)));
-	else if (skip_prefix(start, "(objectsize:padded)", &p))
-		expand_objectsize(sb, &data->ce->oid, object_type(data->ce->ce_mode), 1);
-	else if (skip_prefix(start, "(objectsize)", &p))
-		expand_objectsize(sb, &data->ce->oid, object_type(data->ce->ce_mode), 0);
-	else if (skip_prefix(start, "(stage)", &p))
-		strbuf_addf(sb, "%d", ce_stage(data->ce));
-	else if (skip_prefix(start, "(eolinfo:index)", &p))
-		strbuf_addstr(sb, S_ISREG(data->ce->ce_mode) ?
-			      get_cached_convert_stats_ascii(data->istate,
-			      data->ce->name) : "");
-	else if (skip_prefix(start, "(eolinfo:worktree)", &p))
-		strbuf_addstr(sb, !lstat(data->pathname, &st) &&
-			      S_ISREG(st.st_mode) ?
-			      get_wt_convert_stats_ascii(data->pathname) : "");
-	else if (skip_prefix(start, "(eolattr)", &p))
-		strbuf_addstr(sb, get_convert_attr_ascii(data->istate,
-			      data->pathname));
-	else if (skip_prefix(start, "(path)", &p))
-		write_name_to_buf(sb, data->pathname);
-	else
-		die(_("bad ls-files format: %%%.*s"), (int)len, start);
-
-	return len;
-}

 static void show_ce_fmt(struct repository *repo, const struct cache_entry *ce,
 			const char *format, const char *fullname) {
-	struct show_index_data data = {
-		.pathname = fullname,
-		.istate = repo->index,
-		.ce = ce,
-	};
 	struct strbuf sb = STRBUF_INIT;

-	strbuf_expand(&sb, format, expand_show_index, &data);
+	while (strbuf_expand_step(&sb, &format)) {
+		const char *end;
+		size_t len;
+		struct stat st;
+
+		if (skip_prefix(format, "%", &format))
+			strbuf_addch(&sb, '%');
+		else if ((len = strbuf_expand_literal_cb(&sb, format, NULL)))
+			format += len;
+		else if (*format != '(')
+			die(_("bad ls-files format: element '%s' "
+			      "does not start with '('"), format);
+		else if (!(end = strchr(format + 1, ')')))
+			die(_("bad ls-files format: element '%s' "
+			      "does not end in ')'"), format);
+		else if (skip_prefix(format, "(objectmode)", &format))
+			strbuf_addf(&sb, "%06o", ce->ce_mode);
+		else if (skip_prefix(format, "(objectname)", &format))
+			strbuf_add_unique_abbrev(&sb, &ce->oid, abbrev);
+		else if (skip_prefix(format, "(objecttype)", &format))
+			strbuf_addstr(&sb, type_name(object_type(ce->ce_mode)));
+		else if (skip_prefix(format, "(objectsize:padded)", &format))
+			expand_objectsize(&sb, &ce->oid,
+					  object_type(ce->ce_mode), 1);
+		else if (skip_prefix(format, "(objectsize)", &format))
+			expand_objectsize(&sb, &ce->oid,
+					  object_type(ce->ce_mode), 0);
+		else if (skip_prefix(format, "(stage)", &format))
+			strbuf_addf(&sb, "%d", ce_stage(ce));
+		else if (skip_prefix(format, "(eolinfo:index)", &format))
+			strbuf_addstr(&sb, S_ISREG(ce->ce_mode) ?
+				      get_cached_convert_stats_ascii(repo->index,
+				      ce->name) : "");
+		else if (skip_prefix(format, "(eolinfo:worktree)", &format))
+			strbuf_addstr(&sb, !lstat(fullname, &st) &&
+				      S_ISREG(st.st_mode) ?
+				      get_wt_convert_stats_ascii(fullname) : "");
+		else if (skip_prefix(format, "(eolattr)", &format))
+			strbuf_addstr(&sb, get_convert_attr_ascii(repo->index,
+								 fullname));
+		else if (skip_prefix(format, "(path)", &format))
+			write_name_to_buf(&sb, fullname);
+		else
+			die(_("bad ls-files format: %%%.*s"),
+			    (int)(end - format + 1), format);
+	}
 	strbuf_addch(&sb, line_terminator);
 	fwrite(sb.buf, sb.len, 1, stdout);
 	strbuf_release(&sb);
diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c
index 077977a461..8460d20257 100644
--- a/builtin/ls-tree.c
+++ b/builtin/ls-tree.c
@@ -55,63 +55,6 @@ struct ls_tree_options {
 	const char *format;
 };

-struct show_tree_data {
-	struct ls_tree_options *options;
-	unsigned mode;
-	enum object_type type;
-	const struct object_id *oid;
-	const char *pathname;
-	struct strbuf *base;
-};
-
-static size_t expand_show_tree(struct strbuf *sb, const char *start,
-			       void *context)
-{
-	struct show_tree_data *data = context;
-	struct ls_tree_options *options = data->options;
-	const char *end;
-	const char *p;
-	unsigned int errlen;
-	size_t len = strbuf_expand_literal_cb(sb, start, NULL);
-
-	if (len)
-		return len;
-	if (*start != '(')
-		die(_("bad ls-tree format: element '%s' does not start with '('"), start);
-
-	end = strchr(start + 1, ')');
-	if (!end)
-		die(_("bad ls-tree format: element '%s' does not end in ')'"), start);
-
-	len = end - start + 1;
-	if (skip_prefix(start, "(objectmode)", &p)) {
-		strbuf_addf(sb, "%06o", data->mode);
-	} else if (skip_prefix(start, "(objecttype)", &p)) {
-		strbuf_addstr(sb, type_name(data->type));
-	} else if (skip_prefix(start, "(objectsize:padded)", &p)) {
-		expand_objectsize(sb, data->oid, data->type, 1);
-	} else if (skip_prefix(start, "(objectsize)", &p)) {
-		expand_objectsize(sb, data->oid, data->type, 0);
-	} else if (skip_prefix(start, "(objectname)", &p)) {
-		strbuf_add_unique_abbrev(sb, data->oid, options->abbrev);
-	} else if (skip_prefix(start, "(path)", &p)) {
-		const char *name = data->base->buf;
-		const char *prefix = options->chomp_prefix ? options->ls_tree_prefix : NULL;
-		struct strbuf sbuf = STRBUF_INIT;
-		size_t baselen = data->base->len;
-
-		strbuf_addstr(data->base, data->pathname);
-		name = relative_path(data->base->buf, prefix, &sbuf);
-		quote_c_style(name, sb, NULL, 0);
-		strbuf_setlen(data->base, baselen);
-		strbuf_release(&sbuf);
-	} else {
-		errlen = (unsigned long)len;
-		die(_("bad ls-tree format: %%%.*s"), errlen, start);
-	}
-	return len;
-}
-
 static int show_recursive(struct ls_tree_options *options, const char *base,
 			  size_t baselen, const char *pathname)
 {
@@ -150,14 +93,7 @@ static int show_tree_fmt(const struct object_id *oid, struct strbuf *base,
 	int recurse = 0;
 	struct strbuf sb = STRBUF_INIT;
 	enum object_type type = object_type(mode);
-	struct show_tree_data cb_data = {
-		.options = options,
-		.mode = mode,
-		.type = type,
-		.oid = oid,
-		.pathname = pathname,
-		.base = base,
-	};
+	const char *format = options->format;

 	if (type == OBJ_TREE && show_recursive(options, base->buf, base->len, pathname))
 		recurse = READ_TREE_RECURSIVE;
@@ -166,7 +102,46 @@ static int show_tree_fmt(const struct object_id *oid, struct strbuf *base,
 	if (type == OBJ_BLOB && (options->ls_options & LS_TREE_ONLY))
 		return 0;

-	strbuf_expand(&sb, options->format, expand_show_tree, &cb_data);
+	while (strbuf_expand_step(&sb, &format)) {
+		const char *end;
+		size_t len;
+
+		if (skip_prefix(format, "%", &format))
+			strbuf_addch(&sb, '%');
+		else if ((len = strbuf_expand_literal_cb(&sb, format, NULL)))
+			format += len;
+		else if (*format != '(')
+			die(_("bad ls-tree format: element '%s' "
+			      "does not start with '('"), format);
+		else if (!(end = strchr(format + 1, ')')))
+			die(_("bad ls-tree format: element '%s' "
+			      "does not end in ')'"), format);
+		else if (skip_prefix(format, "(objectmode)", &format))
+			strbuf_addf(&sb, "%06o", mode);
+		else if (skip_prefix(format, "(objecttype)", &format))
+			strbuf_addstr(&sb, type_name(type));
+		else if (skip_prefix(format, "(objectsize:padded)", &format))
+			expand_objectsize(&sb, oid, type, 1);
+		else if (skip_prefix(format, "(objectsize)", &format))
+			expand_objectsize(&sb, oid, type, 0);
+		else if (skip_prefix(format, "(objectname)", &format))
+			strbuf_add_unique_abbrev(&sb, oid, options->abbrev);
+		else if (skip_prefix(format, "(path)", &format)) {
+			const char *name;
+			const char *prefix = options->chomp_prefix ?
+					     options->ls_tree_prefix : NULL;
+			struct strbuf sbuf = STRBUF_INIT;
+			size_t baselen = base->len;
+
+			strbuf_addstr(base, pathname);
+			name = relative_path(base->buf, prefix, &sbuf);
+			quote_c_style(name, &sb, NULL, 0);
+			strbuf_setlen(base, baselen);
+			strbuf_release(&sbuf);
+		} else
+			die(_("bad ls-tree format: %%%.*s"),
+			    (int)(end - format + 1), format);
+	}
 	strbuf_addch(&sb, options->null_termination ? '\0' : '\n');
 	fwrite(sb.buf, sb.len, 1, stdout);
 	strbuf_release(&sb);
diff --git a/daemon.c b/daemon.c
index 7139cc201d..3682bfdd08 100644
--- a/daemon.c
+++ b/daemon.c
@@ -144,42 +144,6 @@ static void NORETURN daemon_die(const char *err, va_list params)
 	exit(1);
 }

-struct expand_path_context {
-	const char *directory;
-	struct hostinfo *hostinfo;
-};
-
-static size_t expand_path(struct strbuf *sb, const char *placeholder, void *ctx)
-{
-	struct expand_path_context *context = ctx;
-	struct hostinfo *hi = context->hostinfo;
-
-	switch (placeholder[0]) {
-	case 'H':
-		strbuf_addbuf(sb, &hi->hostname);
-		return 1;
-	case 'C':
-		if (placeholder[1] == 'H') {
-			strbuf_addstr(sb, get_canon_hostname(hi));
-			return 2;
-		}
-		break;
-	case 'I':
-		if (placeholder[1] == 'P') {
-			strbuf_addstr(sb, get_ip_address(hi));
-			return 2;
-		}
-		break;
-	case 'P':
-		strbuf_addbuf(sb, &hi->tcp_port);
-		return 1;
-	case 'D':
-		strbuf_addstr(sb, context->directory);
-		return 1;
-	}
-	return 0;
-}
-
 static const char *path_ok(const char *directory, struct hostinfo *hi)
 {
 	static char rpath[PATH_MAX];
@@ -223,10 +187,7 @@ static const char *path_ok(const char *directory, struct hostinfo *hi)
 	}
 	else if (interpolated_path && hi->saw_extended_args) {
 		struct strbuf expanded_path = STRBUF_INIT;
-		struct expand_path_context context;
-
-		context.directory = directory;
-		context.hostinfo = hi;
+		const char *format = interpolated_path;

 		if (*dir != '/') {
 			/* Allow only absolute */
@@ -234,8 +195,24 @@ static const char *path_ok(const char *directory, struct hostinfo *hi)
 			return NULL;
 		}

-		strbuf_expand(&expanded_path, interpolated_path,
-			      expand_path, &context);
+		while (strbuf_expand_step(&expanded_path, &format)) {
+			if (skip_prefix(format, "%", &format))
+				strbuf_addch(&expanded_path, '%');
+			else if (skip_prefix(format, "H", &format))
+				strbuf_addbuf(&expanded_path, &hi->hostname);
+			else if (skip_prefix(format, "CH", &format))
+				strbuf_addstr(&expanded_path,
+					      get_canon_hostname(hi));
+			else if (skip_prefix(format, "IP", &format))
+				strbuf_addstr(&expanded_path,
+					      get_ip_address(hi));
+			else if (skip_prefix(format, "P", &format))
+				strbuf_addbuf(&expanded_path, &hi->tcp_port);
+			else if (skip_prefix(format, "D", &format))
+				strbuf_addstr(&expanded_path, directory);
+			else
+				strbuf_addch(&expanded_path, '%');
+		}

 		rlen = strlcpy(interp_path, expanded_path.buf,
 			       sizeof(interp_path));
diff --git a/pretty.c b/pretty.c
index d2df561a05..cffbf32987 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1254,9 +1254,19 @@ static struct strbuf *expand_separator(struct strbuf *sb,
 				       const char *argval, size_t arglen)
 {
 	char *fmt = xstrndup(argval, arglen);
+	const char *format = fmt;

 	strbuf_reset(sb);
-	strbuf_expand(sb, fmt, strbuf_expand_literal_cb, NULL);
+	while (strbuf_expand_step(sb, &format)) {
+		size_t len;
+
+		if (skip_prefix(format, "%", &format))
+			strbuf_addch(sb, '%');
+		else if ((len = strbuf_expand_literal_cb(sb, format, NULL)))
+			format += len;
+		else
+			strbuf_addch(sb, '%');
+	}
 	free(fmt);
 	return sb;
 }
@@ -1803,7 +1813,7 @@ static size_t format_and_pad_commit(struct strbuf *sb, /* in UTF-8 */

 static size_t format_commit_item(struct strbuf *sb, /* in UTF-8 */
 				 const char *placeholder,
-				 void *context)
+				 struct format_commit_context *context)
 {
 	size_t consumed, orig_len;
 	enum {
@@ -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);
@@ -1861,30 +1871,6 @@ static size_t format_commit_item(struct strbuf *sb, /* in UTF-8 */
 	return consumed + 1;
 }

-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;
-}
-
 void userformat_find_requirements(const char *fmt, struct userformat_want *w)
 {
 	struct strbuf dummy = STRBUF_INIT;
@@ -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);
 }

@@ -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, '%');
+	}
 	rewrap_message_tail(sb, &context, 0, 0, 0);

 	/*
diff --git a/strbuf.c b/strbuf.c
index 972366b410..c3d1cee616 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -427,26 +427,6 @@ int strbuf_expand_step(struct strbuf *sb, const char **formatp)
 	return 1;
 }

-void strbuf_expand(struct strbuf *sb, const char *format, expand_fn_t fn,
-		   void *context)
-{
-	while (strbuf_expand_step(sb, &format)) {
-		size_t consumed;
-
-		if (*format == '%') {
-			strbuf_addch(sb, '%');
-			format++;
-			continue;
-		}
-
-		consumed = fn(sb, format, context);
-		if (consumed)
-			format += consumed;
-		else
-			strbuf_addch(sb, '%');
-	}
-}
-
 size_t strbuf_expand_literal_cb(struct strbuf *sb,
 				const char *placeholder,
 				void *context UNUSED)
diff --git a/strbuf.h b/strbuf.h
index e293117f07..95e50e243e 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -318,40 +318,9 @@ const char *strbuf_join_argv(struct strbuf *buf, int argc,
 			     const char **argv, char delim);

 /**
- * This function can be used to expand a format string containing
- * placeholders. To that end, it parses the string and calls the specified
- * function for every percent sign found.
- *
- * The callback function is given a pointer to the character after the `%`
- * and a pointer to the struct strbuf.  It is expected to add the expanded
- * version of the placeholder to the strbuf, e.g. to add a newline
- * character if the letter `n` appears after a `%`.  The function returns
- * the length of the placeholder recognized and `strbuf_expand()` skips
- * over it.
- *
- * The format `%%` is automatically expanded to a single `%` as a quoting
- * mechanism; callers do not need to handle the `%` placeholder themselves,
- * and the callback function will not be invoked for this placeholder.
- *
- * All other characters (non-percent and not skipped ones) are copied
- * verbatim to the strbuf.  If the callback returned zero, meaning that the
- * placeholder is unknown, then the percent sign is copied, too.
- *
- * In order to facilitate caching and to make it possible to give
- * parameters to the callback, `strbuf_expand()` passes a context
- * pointer with any kind of data.
- */
-typedef size_t (*expand_fn_t) (struct strbuf *sb,
-			       const char *placeholder,
-			       void *context);
-void strbuf_expand(struct strbuf *sb,
-		   const char *format,
-		   expand_fn_t fn,
-		   void *context);
-
-/**
- * Used as callback for `strbuf_expand` to only expand literals
- * (i.e. %n and %xNN). The context argument is ignored.
+ * Used with `strbuf_expand_step` to expand the literals %n and %x
+ * followed by two hexadecimal digits. Returns the number of recognized
+ * characters. The context argument is ignored.
  */
 size_t strbuf_expand_literal_cb(struct strbuf *sb,
 				const char *placeholder,
--
2.41.0

Re: [PATCH 4/5] replace strbuf_expand() with strbuf_expand_step()

[Jeff King]

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

> 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. :)

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

>  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.

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

> -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. :)

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

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

-Peff

Re: [PATCH 4/5] replace strbuf_expand() with strbuf_expand_step()

[René Scharfe]

Am 27.06.23 um 10:54 schrieb Jeff King:
> On Sat, Jun 17, 2023 at 10:43:17PM +0200, René Scharfe wrote:
>
>> 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.

>>  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.

>> @@ -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:
>
>> -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.

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

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

Re: [PATCH 4/5] replace strbuf_expand() with strbuf_expand_step()

[Jeff King]

On Tue, Jun 27, 2023 at 06:31:55PM +0200, René Scharfe wrote:

> > 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.

Yeah, since you did the work to handle "%%" in each caller, I'm OK with
proceeding and letting a later refactor shrink it back down if we
choose.

-Peff

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

[René Scharfe]

Now that strbuf_expand_literal_cb() is no longer used as a callback,
drop its "_cb" name suffix and unused context parameter.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 builtin/ls-files.c | 2 +-
 builtin/ls-tree.c  | 2 +-
 pretty.c           | 4 ++--
 strbuf.c           | 4 +---
 strbuf.h           | 6 ++----
 5 files changed, 7 insertions(+), 11 deletions(-)

diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 03bf5771b4..0b00bd5d0f 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -274,7 +274,7 @@ static void show_ce_fmt(struct repository *repo, const struct cache_entry *ce,

 		if (skip_prefix(format, "%", &format))
 			strbuf_addch(&sb, '%');
-		else if ((len = strbuf_expand_literal_cb(&sb, format, NULL)))
+		else if ((len = strbuf_expand_literal(&sb, format)))
 			format += len;
 		else if (*format != '(')
 			die(_("bad ls-files format: element '%s' "
diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c
index 8460d20257..a90f3c81a0 100644
--- a/builtin/ls-tree.c
+++ b/builtin/ls-tree.c
@@ -108,7 +108,7 @@ static int show_tree_fmt(const struct object_id *oid, struct strbuf *base,

 		if (skip_prefix(format, "%", &format))
 			strbuf_addch(&sb, '%');
-		else if ((len = strbuf_expand_literal_cb(&sb, format, NULL)))
+		else if ((len = strbuf_expand_literal(&sb, format)))
 			format += len;
 		else if (*format != '(')
 			die(_("bad ls-tree format: element '%s' "
diff --git a/pretty.c b/pretty.c
index cffbf32987..4c08f9856b 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1262,7 +1262,7 @@ static struct strbuf *expand_separator(struct strbuf *sb,

 		if (skip_prefix(format, "%", &format))
 			strbuf_addch(sb, '%');
-		else if ((len = strbuf_expand_literal_cb(sb, format, NULL)))
+		else if ((len = strbuf_expand_literal(sb, format)))
 			format += len;
 		else
 			strbuf_addch(sb, '%');
@@ -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;

diff --git a/strbuf.c b/strbuf.c
index c3d1cee616..55a3cfa5cf 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -427,9 +427,7 @@ int strbuf_expand_step(struct strbuf *sb, const char **formatp)
 	return 1;
 }

-size_t strbuf_expand_literal_cb(struct strbuf *sb,
-				const char *placeholder,
-				void *context UNUSED)
+size_t strbuf_expand_literal(struct strbuf *sb, const char *placeholder)
 {
 	int ch;

diff --git a/strbuf.h b/strbuf.h
index 95e50e243e..b1eab015f0 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -320,11 +320,9 @@ const char *strbuf_join_argv(struct strbuf *buf, int argc,
 /**
  * Used with `strbuf_expand_step` to expand the literals %n and %x
  * followed by two hexadecimal digits. Returns the number of recognized
- * characters. The context argument is ignored.
+ * characters.
  */
-size_t strbuf_expand_literal_cb(struct strbuf *sb,
-				const char *placeholder,
-				void *context);
+size_t strbuf_expand_literal(struct strbuf *sb, const char *placeholder);

 /**
  * If the string pointed to by `formatp` contains a percent sign ("%"),
--
2.41.0

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

[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).

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).

-Peff

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

[René Scharfe]

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é

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

[Jeff King]

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