diff mbox series

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

Message ID 150df0b0-0548-f291-2b68-960841dd1c97@web.de (mailing list archive)
State New, archived
Headers show
Series replace strbuf_expand() | expand

Commit Message

René Scharfe June 17, 2023, 8:43 p.m. UTC 
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(-)

--
2.41.0

Comments

Jeff King June 27, 2023, 8:54 a.m. UTC | #1 
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
René Scharfe June 27, 2023, 4:31 p.m. UTC | #2 
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é
Jeff King June 27, 2023, 8:19 p.m. UTC | #3 
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
diff mbox series

Patch

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,