{"thread":{"id":"59878","subject":"[PATCH 0/5] replace strbuf_expand()","startedAt":"2023-06-17T20:37:43Z","lastAt":"2023-06-27T20:22:09Z","messageCount":20,"participants":["René Scharfe","Taylor Blau","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"478514","messageId":"767baa64-20a6-daf2-d34b-d81f72363749@web.de","threadId":"59878","inReplyTo":null,"subject":"[PATCH 0/5] replace strbuf_expand()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-06-17T20:37:30Z","receivedAt":"2023-06-17T20:37:43Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"strbuf_expand() is used to interpolate placeholders that start with a\npercent sign (%) in strings.  It invokes a callback function to handle\nthose placeholders, expands \"%%\" to \"%\" itself and copies unrecognized\nplaceholders verbatim.  Context info for the callback needs to be\npassed in via a void pointer, e.g. to a custom struct.\n\nThere is a better, simpler way: Use a loop to iterate over placeholders\nand access variables directly, without the need to take a detour through\na context struct.  This series converts the callers of strbuf_expand()\nto use the new helper strbuf_expand_step(), removing the overhead that\ncomes with using a callback.\n\n  pretty: factor out expand_separator()\n  strbuf: factor out strbuf_expand_step()\n  replace strbuf_expand_dict_cb() with strbuf_expand_step()\n  replace strbuf_expand() with strbuf_expand_step()\n  strbuf: simplify strbuf_expand_literal_cb()\n\n builtin/branch.c   |  13 +-----\n builtin/cat-file.c |  35 +++++++--------\n builtin/ls-files.c | 109 +++++++++++++++++++--------------------------\n builtin/ls-tree.c  | 107 +++++++++++++++++---------------------------\n convert.c          |  22 +++++----\n daemon.c           |  61 ++++++++-----------------\n ll-merge.c         |  32 +++++++------\n pretty.c           |  99 ++++++++++++++++++++++------------------\n strbuf.c           |  58 +++++-------------------\n strbuf.h           |  57 ++++--------------------\n 10 files changed, 228 insertions(+), 365 deletions(-)\n\n--\n2.41.0\n"},{"id":"478515","messageId":"9aee9e14-5459-b62b-6427-23fc0d532b5b@web.de","threadId":"59878","inReplyTo":"767baa64-20a6-daf2-d34b-d81f72363749@web.de","subject":"[PATCH 1/5] pretty: factor out expand_separator()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-06-17T20:40:57Z","receivedAt":"2023-06-17T20:41:14Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Deduplicate the code for setting the options \"separator\" and\n\"key_value_separator\" by moving it into a new helper function,\nexpand_separator().\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n pretty.c | 27 +++++++++++++--------------\n 1 file changed, 13 insertions(+), 14 deletions(-)\n\ndiff --git a/pretty.c b/pretty.c\nindex 0bb938021b..d2df561a05 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1250,6 +1250,17 @@ static int format_trailer_match_cb(const struct strbuf *key, void *ud)\n \treturn 0;\n }\n\n+static struct strbuf *expand_separator(struct strbuf *sb,\n+\t\t\t\t       const char *argval, size_t arglen)\n+{\n+\tchar *fmt = xstrndup(argval, arglen);\n+\n+\tstrbuf_reset(sb);\n+\tstrbuf_expand(sb, fmt, strbuf_expand_literal_cb, NULL);\n+\tfree(fmt);\n+\treturn sb;\n+}\n+\n int format_set_trailers_options(struct process_trailer_options *opts,\n \t\t\t\tstruct string_list *filter_list,\n \t\t\t\tstruct strbuf *sepbuf,\n@@ -1278,21 +1289,9 @@ int format_set_trailers_options(struct process_trailer_options *opts,\n \t\t\topts->filter_data = filter_list;\n \t\t\topts->only_trailers = 1;\n \t\t} else if (match_placeholder_arg_value(*arg, \"separator\", arg, &argval, &arglen)) {\n-\t\t\tchar *fmt;\n-\n-\t\t\tstrbuf_reset(sepbuf);\n-\t\t\tfmt = xstrndup(argval, arglen);\n-\t\t\tstrbuf_expand(sepbuf, fmt, strbuf_expand_literal_cb, NULL);\n-\t\t\tfree(fmt);\n-\t\t\topts->separator = sepbuf;\n+\t\t\topts->separator = expand_separator(sepbuf, argval, arglen);\n \t\t} else if (match_placeholder_arg_value(*arg, \"key_value_separator\", arg, &argval, &arglen)) {\n-\t\t\tchar *fmt;\n-\n-\t\t\tstrbuf_reset(kvsepbuf);\n-\t\t\tfmt = xstrndup(argval, arglen);\n-\t\t\tstrbuf_expand(kvsepbuf, fmt, strbuf_expand_literal_cb, NULL);\n-\t\t\tfree(fmt);\n-\t\t\topts->key_value_separator = kvsepbuf;\n+\t\t\topts->key_value_separator = expand_separator(kvsepbuf, argval, arglen);\n \t\t} else if (!match_placeholder_bool_arg(*arg, \"only\", arg, &opts->only_trailers) &&\n \t\t\t   !match_placeholder_bool_arg(*arg, \"unfold\", arg, &opts->unfold) &&\n \t\t\t   !match_placeholder_bool_arg(*arg, \"keyonly\", arg, &opts->key_only) &&\n--\n2.41.0\n"},{"id":"478516","messageId":"caf8e100-1308-cb4e-d61a-4e15ee3f47f7@web.de","threadId":"59878","inReplyTo":"767baa64-20a6-daf2-d34b-d81f72363749@web.de","subject":"[PATCH 2/5] strbuf: factor out strbuf_expand_step()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-06-17T20:41:44Z","receivedAt":"2023-06-17T20:42:11Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Extract the part of strbuf_expand that finds the next placeholder into a\nnew function.  It allows to build parsers without callback functions and\nthe overhead imposed by them.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n builtin/branch.c | 13 ++-----------\n strbuf.c         | 28 ++++++++++++++--------------\n strbuf.h         |  8 ++++++++\n 3 files changed, 24 insertions(+), 25 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex e6c2655af6..7c20e049a2 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -366,17 +366,8 @@ static const char *quote_literal_for_format(const char *s)\n \tstatic struct strbuf buf = STRBUF_INIT;\n\n \tstrbuf_reset(&buf);\n-\twhile (*s) {\n-\t\tconst char *ep = strchrnul(s, '%');\n-\t\tif (s < ep)\n-\t\t\tstrbuf_add(&buf, s, ep - s);\n-\t\tif (*ep == '%') {\n-\t\t\tstrbuf_addstr(&buf, \"%%\");\n-\t\t\ts = ep + 1;\n-\t\t} else {\n-\t\t\ts = ep;\n-\t\t}\n-\t}\n+\twhile (strbuf_expand_step(&buf, &s))\n+\t\tstrbuf_addstr(&buf, \"%%\");\n \treturn buf.buf;\n }\n\ndiff --git a/strbuf.c b/strbuf.c\nindex 08eec8f1d8..a90b597da1 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -415,19 +415,24 @@ void strbuf_vaddf(struct strbuf *sb, const char *fmt, va_list ap)\n \tstrbuf_setlen(sb, sb->len + len);\n }\n\n+int strbuf_expand_step(struct strbuf *sb, const char **formatp)\n+{\n+\tconst char *format = *formatp;\n+\tconst char *percent = strchrnul(format, '%');\n+\n+\tstrbuf_add(sb, format, percent - format);\n+\tif (!*percent)\n+\t\treturn 0;\n+\t*formatp = percent + 1;\n+\treturn 1;\n+}\n+\n void strbuf_expand(struct strbuf *sb, const char *format, expand_fn_t fn,\n \t\t   void *context)\n {\n-\tfor (;;) {\n-\t\tconst char *percent;\n+\twhile (strbuf_expand_step(sb, &format)) {\n \t\tsize_t consumed;\n\n-\t\tpercent = strchrnul(format, '%');\n-\t\tstrbuf_add(sb, format, percent - format);\n-\t\tif (!*percent)\n-\t\t\tbreak;\n-\t\tformat = percent + 1;\n-\n \t\tif (*format == '%') {\n \t\t\tstrbuf_addch(sb, '%');\n \t\t\tformat++;\n@@ -1022,12 +1027,7 @@ void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm,\n \t * we want for %z, but the computation for %s has to convert to number\n \t * of seconds.\n \t */\n-\tfor (;;) {\n-\t\tconst char *percent = strchrnul(fmt, '%');\n-\t\tstrbuf_add(&munged_fmt, fmt, percent - fmt);\n-\t\tif (!*percent)\n-\t\t\tbreak;\n-\t\tfmt = percent + 1;\n+\twhile (strbuf_expand_step(&munged_fmt, &fmt)) {\n \t\tswitch (*fmt) {\n \t\tcase '%':\n \t\t\tstrbuf_addstr(&munged_fmt, \"%%\");\ndiff --git a/strbuf.h b/strbuf.h\nindex 3dfeadb44c..a189f12b84 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -371,6 +371,14 @@ size_t strbuf_expand_dict_cb(struct strbuf *sb,\n \t\t\t     const char *placeholder,\n \t\t\t     void *context);\n\n+/**\n+ * If the string pointed to by `formatp` contains a percent sign (\"%\"),\n+ * advance it to point to the character following the next one and\n+ * return 1, otherwise return 0.  Append the substring before that\n+ * percent sign to `sb`, or the whole string if there is none.\n+ */\n+int strbuf_expand_step(struct strbuf *sb, const char **formatp);\n+\n /**\n  * Append the contents of one strbuf to another, quoting any\n  * percent signs (\"%\") into double-percents (\"%%\") in the\n--\n2.41.0\n"},{"id":"478517","messageId":"5ce9513b-d463-6f62-db4e-f9f60a7c3e9f@web.de","threadId":"59878","inReplyTo":"767baa64-20a6-daf2-d34b-d81f72363749@web.de","subject":"[PATCH 3/5] replace strbuf_expand_dict_cb() with strbuf_expand_step()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-06-17T20:42:26Z","receivedAt":"2023-06-17T20:42:40Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Avoid the overhead of setting up a dictionary and passing it via\nstrbuf_expand() to strbuf_expand_dict_cb() by using strbuf_expand_step()\nin a loop instead.  It requires explicit handling of %% and unrecognized\nplaceholders, but is more direct and simpler overall, and expands only\non demand.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n convert.c  | 22 ++++++++++------------\n ll-merge.c | 32 ++++++++++++++++++--------------\n strbuf.c   | 16 ----------------\n strbuf.h   | 14 --------------\n 4 files changed, 28 insertions(+), 56 deletions(-)\n\ndiff --git a/convert.c b/convert.c\nindex 9ee79fe469..455d05cf6b 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -633,23 +633,21 @@ static int filter_buffer_or_fd(int in UNUSED, int out, void *data)\n \t */\n \tstruct child_process child_process = CHILD_PROCESS_INIT;\n \tstruct filter_params *params = (struct filter_params *)data;\n+\tconst char *format = params->cmd;\n \tint write_err, status;\n\n \t/* apply % substitution to cmd */\n \tstruct strbuf cmd = STRBUF_INIT;\n-\tstruct strbuf path = STRBUF_INIT;\n-\tstruct strbuf_expand_dict_entry dict[] = {\n-\t\t{ \"f\", NULL, },\n-\t\t{ NULL, NULL, },\n-\t};\n-\n-\t/* quote the path to preserve spaces, etc. */\n-\tsq_quote_buf(&path, params->path);\n-\tdict[0].value = path.buf;\n\n-\t/* expand all %f with the quoted path */\n-\tstrbuf_expand(&cmd, params->cmd, strbuf_expand_dict_cb, &dict);\n-\tstrbuf_release(&path);\n+\t/* expand all %f with the quoted path; quote to preserve space, etc. */\n+\twhile (strbuf_expand_step(&cmd, &format)) {\n+\t\tif (skip_prefix(format, \"%\", &format))\n+\t\t\tstrbuf_addch(&cmd, '%');\n+\t\telse if (skip_prefix(format, \"f\", &format))\n+\t\t\tsq_quote_buf(&cmd, params->path);\n+\t\telse\n+\t\t\tstrbuf_addch(&cmd, '%');\n+\t}\n\n \tstrvec_push(&child_process.args, cmd.buf);\n \tchild_process.use_shell = 1;\ndiff --git a/ll-merge.c b/ll-merge.c\nindex 07ec16e8e5..b307ad293c 100644\n--- a/ll-merge.c\n+++ b/ll-merge.c\n@@ -192,24 +192,15 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,\n \t\t\tconst struct ll_merge_options *opts,\n \t\t\tint marker_size)\n {\n-\tchar temp[4][50];\n+\tchar temp[3][50];\n \tstruct strbuf cmd = STRBUF_INIT;\n-\tstruct strbuf_expand_dict_entry dict[6];\n-\tstruct strbuf path_sq = STRBUF_INIT;\n+\tconst char *format = fn->cmdline;\n \tstruct child_process child = CHILD_PROCESS_INIT;\n \tint status, fd, i;\n \tstruct stat st;\n \tenum ll_merge_result ret;\n \tassert(opts);\n\n-\tsq_quote_buf(&path_sq, path);\n-\tdict[0].placeholder = \"O\"; dict[0].value = temp[0];\n-\tdict[1].placeholder = \"A\"; dict[1].value = temp[1];\n-\tdict[2].placeholder = \"B\"; dict[2].value = temp[2];\n-\tdict[3].placeholder = \"L\"; dict[3].value = temp[3];\n-\tdict[4].placeholder = \"P\"; dict[4].value = path_sq.buf;\n-\tdict[5].placeholder = NULL; dict[5].value = NULL;\n-\n \tif (!fn->cmdline)\n \t\tdie(\"custom merge driver %s lacks command line.\", fn->name);\n\n@@ -218,9 +209,23 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,\n \tcreate_temp(orig, temp[0], sizeof(temp[0]));\n \tcreate_temp(src1, temp[1], sizeof(temp[1]));\n \tcreate_temp(src2, temp[2], sizeof(temp[2]));\n-\txsnprintf(temp[3], sizeof(temp[3]), \"%d\", marker_size);\n\n-\tstrbuf_expand(&cmd, fn->cmdline, strbuf_expand_dict_cb, &dict);\n+\twhile (strbuf_expand_step(&cmd, &format)) {\n+\t\tif (skip_prefix(format, \"%\", &format))\n+\t\t\tstrbuf_addch(&cmd, '%');\n+\t\telse if (skip_prefix(format, \"O\", &format))\n+\t\t\tstrbuf_addstr(&cmd, temp[0]);\n+\t\telse if (skip_prefix(format, \"A\", &format))\n+\t\t\tstrbuf_addstr(&cmd, temp[1]);\n+\t\telse if (skip_prefix(format, \"B\", &format))\n+\t\t\tstrbuf_addstr(&cmd, temp[2]);\n+\t\telse if (skip_prefix(format, \"L\", &format))\n+\t\t\tstrbuf_addf(&cmd, \"%d\", marker_size);\n+\t\telse if (skip_prefix(format, \"P\", &format))\n+\t\t\tsq_quote_buf(&cmd, path);\n+\t\telse\n+\t\t\tstrbuf_addch(&cmd, '%');\n+\t}\n\n \tchild.use_shell = 1;\n \tstrvec_push(&child.args, cmd.buf);\n@@ -242,7 +247,6 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,\n \tfor (i = 0; i < 3; i++)\n \t\tunlink_or_warn(temp[i]);\n \tstrbuf_release(&cmd);\n-\tstrbuf_release(&path_sq);\n \tret = (status > 0) ? LL_MERGE_CONFLICT : status;\n \treturn ret;\n }\ndiff --git a/strbuf.c b/strbuf.c\nindex a90b597da1..972366b410 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -468,22 +468,6 @@ size_t strbuf_expand_literal_cb(struct strbuf *sb,\n \treturn 0;\n }\n\n-size_t strbuf_expand_dict_cb(struct strbuf *sb, const char *placeholder,\n-\t\tvoid *context)\n-{\n-\tstruct strbuf_expand_dict_entry *e = context;\n-\tsize_t len;\n-\n-\tfor (; e->placeholder && (len = strlen(e->placeholder)); e++) {\n-\t\tif (!strncmp(placeholder, e->placeholder, len)) {\n-\t\t\tif (e->value)\n-\t\t\t\tstrbuf_addstr(sb, e->value);\n-\t\t\treturn len;\n-\t\t}\n-\t}\n-\treturn 0;\n-}\n-\n void strbuf_addbuf_percentquote(struct strbuf *dst, const struct strbuf *src)\n {\n \tsize_t i, len = src->len;\ndiff --git a/strbuf.h b/strbuf.h\nindex a189f12b84..e293117f07 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -357,20 +357,6 @@ size_t strbuf_expand_literal_cb(struct strbuf *sb,\n \t\t\t\tconst char *placeholder,\n \t\t\t\tvoid *context);\n\n-/**\n- * Used as callback for `strbuf_expand()`, expects an array of\n- * struct strbuf_expand_dict_entry as context, i.e. pairs of\n- * placeholder and replacement string.  The array needs to be\n- * terminated by an entry with placeholder set to NULL.\n- */\n-struct strbuf_expand_dict_entry {\n-\tconst char *placeholder;\n-\tconst char *value;\n-};\n-size_t strbuf_expand_dict_cb(struct strbuf *sb,\n-\t\t\t     const char *placeholder,\n-\t\t\t     void *context);\n-\n /**\n  * If the string pointed to by `formatp` contains a percent sign (\"%\"),\n  * advance it to point to the character following the next one and\n--\n2.41.0\n"},{"id":"478518","messageId":"150df0b0-0548-f291-2b68-960841dd1c97@web.de","threadId":"59878","inReplyTo":"767baa64-20a6-daf2-d34b-d81f72363749@web.de","subject":"[PATCH 4/5] replace strbuf_expand() with strbuf_expand_step()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-06-17T20:43:17Z","receivedAt":"2023-06-17T20:43:24Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Avoid the overhead of passing context to a callback function of\nstrbuf_expand() by using strbuf_expand_step() in a loop instead.  It\nrequires explicit handling of %% and unrecognized placeholders, but is\nsimpler, more direct and avoids void pointers.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n builtin/cat-file.c |  35 +++++++--------\n builtin/ls-files.c | 109 +++++++++++++++++++--------------------------\n builtin/ls-tree.c  | 107 +++++++++++++++++---------------------------\n daemon.c           |  61 ++++++++-----------------\n pretty.c           |  72 ++++++++++++++++++------------\n strbuf.c           |  20 ---------\n strbuf.h           |  37 ++-------------\n 7 files changed, 169 insertions(+), 272 deletions(-)\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex 0bafc14e6c..424f39675b 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -309,10 +309,8 @@ static int is_atom(const char *atom, const char *s, int slen)\n }\n\n static void expand_atom(struct strbuf *sb, const char *atom, int len,\n-\t\t\tvoid *vdata)\n+\t\t\tstruct expand_data *data)\n {\n-\tstruct expand_data *data = vdata;\n-\n \tif (is_atom(\"objectname\", atom, len)) {\n \t\tif (!data->mark_query)\n \t\t\tstrbuf_addstr(sb, oid_to_hex(&data->oid));\n@@ -346,19 +344,21 @@ static void expand_atom(struct strbuf *sb, const char *atom, int len,\n \t\tdie(\"unknown format element: %.*s\", len, atom);\n }\n\n-static size_t expand_format(struct strbuf *sb, const char *start, void *data)\n+static void expand_format(struct strbuf *sb, const char *start,\n+\t\t\t  struct expand_data *data)\n {\n-\tconst char *end;\n-\n-\tif (*start != '(')\n-\t\treturn 0;\n-\tend = strchr(start + 1, ')');\n-\tif (!end)\n-\t\tdie(\"format element '%s' does not end in ')'\", start);\n-\n-\texpand_atom(sb, start + 1, end - start - 1, data);\n-\n-\treturn end - start + 1;\n+\twhile (strbuf_expand_step(sb, &start)) {\n+\t\tconst char *end;\n+\n+\t\tif (skip_prefix(start, \"%\", &start) || *start != '(')\n+\t\t\tstrbuf_addch(sb, '%');\n+\t\telse if (!(end = strchr(start + 1, ')')))\n+\t\t\tdie(\"format element '%s' does not end in ')'\", start);\n+\t\telse {\n+\t\t\texpand_atom(sb, start + 1, end - start - 1, data);\n+\t\t\tstart = end + 1;\n+\t\t}\n+\t}\n }\n\n static void batch_write(struct batch_options *opt, const void *data, int len)\n@@ -494,7 +494,7 @@ static void batch_object_write(const char *obj_name,\n \tif (!opt->format) {\n \t\tprint_default_format(scratch, data);\n \t} else {\n-\t\tstrbuf_expand(scratch, opt->format, expand_format, data);\n+\t\texpand_format(scratch, opt->format, data);\n \t\tstrbuf_addch(scratch, '\\n');\n \t}\n\n@@ -777,9 +777,8 @@ static int batch_objects(struct batch_options *opt)\n \t */\n \tmemset(&data, 0, sizeof(data));\n \tdata.mark_query = 1;\n-\tstrbuf_expand(&output,\n+\texpand_format(&output,\n \t\t      opt->format ? opt->format : DEFAULT_FORMAT,\n-\t\t      expand_format,\n \t\t      &data);\n \tdata.mark_query = 0;\n \tstrbuf_release(&output);\ndiff --git a/builtin/ls-files.c b/builtin/ls-files.c\nindex 72012c0f0f..03bf5771b4 100644\n--- a/builtin/ls-files.c\n+++ b/builtin/ls-files.c\n@@ -262,74 +262,57 @@ static void expand_objectsize(struct strbuf *line, const struct object_id *oid,\n \t\tstrbuf_addstr(line, \"-\");\n \t}\n }\n-struct show_index_data {\n-\tconst char *pathname;\n-\tstruct index_state *istate;\n-\tconst struct cache_entry *ce;\n-};\n-\n-static size_t expand_show_index(struct strbuf *sb, const char *start,\n-\t\t\t\tvoid *context)\n-{\n-\tstruct show_index_data *data = context;\n-\tconst char *end;\n-\tconst char *p;\n-\tsize_t len = strbuf_expand_literal_cb(sb, start, NULL);\n-\tstruct stat st;\n-\n-\tif (len)\n-\t\treturn len;\n-\tif (*start != '(')\n-\t\tdie(_(\"bad ls-files format: element '%s' \"\n-\t\t      \"does not start with '('\"), start);\n-\n-\tend = strchr(start + 1, ')');\n-\tif (!end)\n-\t\tdie(_(\"bad ls-files format: element '%s' \"\n-\t\t      \"does not end in ')'\"), start);\n-\n-\tlen = end - start + 1;\n-\tif (skip_prefix(start, \"(objectmode)\", &p))\n-\t\tstrbuf_addf(sb, \"%06o\", data->ce->ce_mode);\n-\telse if (skip_prefix(start, \"(objectname)\", &p))\n-\t\tstrbuf_add_unique_abbrev(sb, &data->ce->oid, abbrev);\n-\telse if (skip_prefix(start, \"(objecttype)\", &p))\n-\t\tstrbuf_addstr(sb, type_name(object_type(data->ce->ce_mode)));\n-\telse if (skip_prefix(start, \"(objectsize:padded)\", &p))\n-\t\texpand_objectsize(sb, &data->ce->oid, object_type(data->ce->ce_mode), 1);\n-\telse if (skip_prefix(start, \"(objectsize)\", &p))\n-\t\texpand_objectsize(sb, &data->ce->oid, object_type(data->ce->ce_mode), 0);\n-\telse if (skip_prefix(start, \"(stage)\", &p))\n-\t\tstrbuf_addf(sb, \"%d\", ce_stage(data->ce));\n-\telse if (skip_prefix(start, \"(eolinfo:index)\", &p))\n-\t\tstrbuf_addstr(sb, S_ISREG(data->ce->ce_mode) ?\n-\t\t\t      get_cached_convert_stats_ascii(data->istate,\n-\t\t\t      data->ce->name) : \"\");\n-\telse if (skip_prefix(start, \"(eolinfo:worktree)\", &p))\n-\t\tstrbuf_addstr(sb, !lstat(data->pathname, &st) &&\n-\t\t\t      S_ISREG(st.st_mode) ?\n-\t\t\t      get_wt_convert_stats_ascii(data->pathname) : \"\");\n-\telse if (skip_prefix(start, \"(eolattr)\", &p))\n-\t\tstrbuf_addstr(sb, get_convert_attr_ascii(data->istate,\n-\t\t\t      data->pathname));\n-\telse if (skip_prefix(start, \"(path)\", &p))\n-\t\twrite_name_to_buf(sb, data->pathname);\n-\telse\n-\t\tdie(_(\"bad ls-files format: %%%.*s\"), (int)len, start);\n-\n-\treturn len;\n-}\n\n static void show_ce_fmt(struct repository *repo, const struct cache_entry *ce,\n \t\t\tconst char *format, const char *fullname) {\n-\tstruct show_index_data data = {\n-\t\t.pathname = fullname,\n-\t\t.istate = repo->index,\n-\t\t.ce = ce,\n-\t};\n \tstruct strbuf sb = STRBUF_INIT;\n\n-\tstrbuf_expand(&sb, format, expand_show_index, &data);\n+\twhile (strbuf_expand_step(&sb, &format)) {\n+\t\tconst char *end;\n+\t\tsize_t len;\n+\t\tstruct stat st;\n+\n+\t\tif (skip_prefix(format, \"%\", &format))\n+\t\t\tstrbuf_addch(&sb, '%');\n+\t\telse if ((len = strbuf_expand_literal_cb(&sb, format, NULL)))\n+\t\t\tformat += len;\n+\t\telse if (*format != '(')\n+\t\t\tdie(_(\"bad ls-files format: element '%s' \"\n+\t\t\t      \"does not start with '('\"), format);\n+\t\telse if (!(end = strchr(format + 1, ')')))\n+\t\t\tdie(_(\"bad ls-files format: element '%s' \"\n+\t\t\t      \"does not end in ')'\"), format);\n+\t\telse if (skip_prefix(format, \"(objectmode)\", &format))\n+\t\t\tstrbuf_addf(&sb, \"%06o\", ce->ce_mode);\n+\t\telse if (skip_prefix(format, \"(objectname)\", &format))\n+\t\t\tstrbuf_add_unique_abbrev(&sb, &ce->oid, abbrev);\n+\t\telse if (skip_prefix(format, \"(objecttype)\", &format))\n+\t\t\tstrbuf_addstr(&sb, type_name(object_type(ce->ce_mode)));\n+\t\telse if (skip_prefix(format, \"(objectsize:padded)\", &format))\n+\t\t\texpand_objectsize(&sb, &ce->oid,\n+\t\t\t\t\t  object_type(ce->ce_mode), 1);\n+\t\telse if (skip_prefix(format, \"(objectsize)\", &format))\n+\t\t\texpand_objectsize(&sb, &ce->oid,\n+\t\t\t\t\t  object_type(ce->ce_mode), 0);\n+\t\telse if (skip_prefix(format, \"(stage)\", &format))\n+\t\t\tstrbuf_addf(&sb, \"%d\", ce_stage(ce));\n+\t\telse if (skip_prefix(format, \"(eolinfo:index)\", &format))\n+\t\t\tstrbuf_addstr(&sb, S_ISREG(ce->ce_mode) ?\n+\t\t\t\t      get_cached_convert_stats_ascii(repo->index,\n+\t\t\t\t      ce->name) : \"\");\n+\t\telse if (skip_prefix(format, \"(eolinfo:worktree)\", &format))\n+\t\t\tstrbuf_addstr(&sb, !lstat(fullname, &st) &&\n+\t\t\t\t      S_ISREG(st.st_mode) ?\n+\t\t\t\t      get_wt_convert_stats_ascii(fullname) : \"\");\n+\t\telse if (skip_prefix(format, \"(eolattr)\", &format))\n+\t\t\tstrbuf_addstr(&sb, get_convert_attr_ascii(repo->index,\n+\t\t\t\t\t\t\t\t fullname));\n+\t\telse if (skip_prefix(format, \"(path)\", &format))\n+\t\t\twrite_name_to_buf(&sb, fullname);\n+\t\telse\n+\t\t\tdie(_(\"bad ls-files format: %%%.*s\"),\n+\t\t\t    (int)(end - format + 1), format);\n+\t}\n \tstrbuf_addch(&sb, line_terminator);\n \tfwrite(sb.buf, sb.len, 1, stdout);\n \tstrbuf_release(&sb);\ndiff --git a/builtin/ls-tree.c b/builtin/ls-tree.c\nindex 077977a461..8460d20257 100644\n--- a/builtin/ls-tree.c\n+++ b/builtin/ls-tree.c\n@@ -55,63 +55,6 @@ struct ls_tree_options {\n \tconst char *format;\n };\n\n-struct show_tree_data {\n-\tstruct ls_tree_options *options;\n-\tunsigned mode;\n-\tenum object_type type;\n-\tconst struct object_id *oid;\n-\tconst char *pathname;\n-\tstruct strbuf *base;\n-};\n-\n-static size_t expand_show_tree(struct strbuf *sb, const char *start,\n-\t\t\t       void *context)\n-{\n-\tstruct show_tree_data *data = context;\n-\tstruct ls_tree_options *options = data->options;\n-\tconst char *end;\n-\tconst char *p;\n-\tunsigned int errlen;\n-\tsize_t len = strbuf_expand_literal_cb(sb, start, NULL);\n-\n-\tif (len)\n-\t\treturn len;\n-\tif (*start != '(')\n-\t\tdie(_(\"bad ls-tree format: element '%s' does not start with '('\"), start);\n-\n-\tend = strchr(start + 1, ')');\n-\tif (!end)\n-\t\tdie(_(\"bad ls-tree format: element '%s' does not end in ')'\"), start);\n-\n-\tlen = end - start + 1;\n-\tif (skip_prefix(start, \"(objectmode)\", &p)) {\n-\t\tstrbuf_addf(sb, \"%06o\", data->mode);\n-\t} else if (skip_prefix(start, \"(objecttype)\", &p)) {\n-\t\tstrbuf_addstr(sb, type_name(data->type));\n-\t} else if (skip_prefix(start, \"(objectsize:padded)\", &p)) {\n-\t\texpand_objectsize(sb, data->oid, data->type, 1);\n-\t} else if (skip_prefix(start, \"(objectsize)\", &p)) {\n-\t\texpand_objectsize(sb, data->oid, data->type, 0);\n-\t} else if (skip_prefix(start, \"(objectname)\", &p)) {\n-\t\tstrbuf_add_unique_abbrev(sb, data->oid, options->abbrev);\n-\t} else if (skip_prefix(start, \"(path)\", &p)) {\n-\t\tconst char *name = data->base->buf;\n-\t\tconst char *prefix = options->chomp_prefix ? options->ls_tree_prefix : NULL;\n-\t\tstruct strbuf sbuf = STRBUF_INIT;\n-\t\tsize_t baselen = data->base->len;\n-\n-\t\tstrbuf_addstr(data->base, data->pathname);\n-\t\tname = relative_path(data->base->buf, prefix, &sbuf);\n-\t\tquote_c_style(name, sb, NULL, 0);\n-\t\tstrbuf_setlen(data->base, baselen);\n-\t\tstrbuf_release(&sbuf);\n-\t} else {\n-\t\terrlen = (unsigned long)len;\n-\t\tdie(_(\"bad ls-tree format: %%%.*s\"), errlen, start);\n-\t}\n-\treturn len;\n-}\n-\n static int show_recursive(struct ls_tree_options *options, const char *base,\n \t\t\t  size_t baselen, const char *pathname)\n {\n@@ -150,14 +93,7 @@ static int show_tree_fmt(const struct object_id *oid, struct strbuf *base,\n \tint recurse = 0;\n \tstruct strbuf sb = STRBUF_INIT;\n \tenum object_type type = object_type(mode);\n-\tstruct show_tree_data cb_data = {\n-\t\t.options = options,\n-\t\t.mode = mode,\n-\t\t.type = type,\n-\t\t.oid = oid,\n-\t\t.pathname = pathname,\n-\t\t.base = base,\n-\t};\n+\tconst char *format = options->format;\n\n \tif (type == OBJ_TREE && show_recursive(options, base->buf, base->len, pathname))\n \t\trecurse = READ_TREE_RECURSIVE;\n@@ -166,7 +102,46 @@ static int show_tree_fmt(const struct object_id *oid, struct strbuf *base,\n \tif (type == OBJ_BLOB && (options->ls_options & LS_TREE_ONLY))\n \t\treturn 0;\n\n-\tstrbuf_expand(&sb, options->format, expand_show_tree, &cb_data);\n+\twhile (strbuf_expand_step(&sb, &format)) {\n+\t\tconst char *end;\n+\t\tsize_t len;\n+\n+\t\tif (skip_prefix(format, \"%\", &format))\n+\t\t\tstrbuf_addch(&sb, '%');\n+\t\telse if ((len = strbuf_expand_literal_cb(&sb, format, NULL)))\n+\t\t\tformat += len;\n+\t\telse if (*format != '(')\n+\t\t\tdie(_(\"bad ls-tree format: element '%s' \"\n+\t\t\t      \"does not start with '('\"), format);\n+\t\telse if (!(end = strchr(format + 1, ')')))\n+\t\t\tdie(_(\"bad ls-tree format: element '%s' \"\n+\t\t\t      \"does not end in ')'\"), format);\n+\t\telse if (skip_prefix(format, \"(objectmode)\", &format))\n+\t\t\tstrbuf_addf(&sb, \"%06o\", mode);\n+\t\telse if (skip_prefix(format, \"(objecttype)\", &format))\n+\t\t\tstrbuf_addstr(&sb, type_name(type));\n+\t\telse if (skip_prefix(format, \"(objectsize:padded)\", &format))\n+\t\t\texpand_objectsize(&sb, oid, type, 1);\n+\t\telse if (skip_prefix(format, \"(objectsize)\", &format))\n+\t\t\texpand_objectsize(&sb, oid, type, 0);\n+\t\telse if (skip_prefix(format, \"(objectname)\", &format))\n+\t\t\tstrbuf_add_unique_abbrev(&sb, oid, options->abbrev);\n+\t\telse if (skip_prefix(format, \"(path)\", &format)) {\n+\t\t\tconst char *name;\n+\t\t\tconst char *prefix = options->chomp_prefix ?\n+\t\t\t\t\t     options->ls_tree_prefix : NULL;\n+\t\t\tstruct strbuf sbuf = STRBUF_INIT;\n+\t\t\tsize_t baselen = base->len;\n+\n+\t\t\tstrbuf_addstr(base, pathname);\n+\t\t\tname = relative_path(base->buf, prefix, &sbuf);\n+\t\t\tquote_c_style(name, &sb, NULL, 0);\n+\t\t\tstrbuf_setlen(base, baselen);\n+\t\t\tstrbuf_release(&sbuf);\n+\t\t} else\n+\t\t\tdie(_(\"bad ls-tree format: %%%.*s\"),\n+\t\t\t    (int)(end - format + 1), format);\n+\t}\n \tstrbuf_addch(&sb, options->null_termination ? '\\0' : '\\n');\n \tfwrite(sb.buf, sb.len, 1, stdout);\n \tstrbuf_release(&sb);\ndiff --git a/daemon.c b/daemon.c\nindex 7139cc201d..3682bfdd08 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -144,42 +144,6 @@ static void NORETURN daemon_die(const char *err, va_list params)\n \texit(1);\n }\n\n-struct expand_path_context {\n-\tconst char *directory;\n-\tstruct hostinfo *hostinfo;\n-};\n-\n-static size_t expand_path(struct strbuf *sb, const char *placeholder, void *ctx)\n-{\n-\tstruct expand_path_context *context = ctx;\n-\tstruct hostinfo *hi = context->hostinfo;\n-\n-\tswitch (placeholder[0]) {\n-\tcase 'H':\n-\t\tstrbuf_addbuf(sb, &hi->hostname);\n-\t\treturn 1;\n-\tcase 'C':\n-\t\tif (placeholder[1] == 'H') {\n-\t\t\tstrbuf_addstr(sb, get_canon_hostname(hi));\n-\t\t\treturn 2;\n-\t\t}\n-\t\tbreak;\n-\tcase 'I':\n-\t\tif (placeholder[1] == 'P') {\n-\t\t\tstrbuf_addstr(sb, get_ip_address(hi));\n-\t\t\treturn 2;\n-\t\t}\n-\t\tbreak;\n-\tcase 'P':\n-\t\tstrbuf_addbuf(sb, &hi->tcp_port);\n-\t\treturn 1;\n-\tcase 'D':\n-\t\tstrbuf_addstr(sb, context->directory);\n-\t\treturn 1;\n-\t}\n-\treturn 0;\n-}\n-\n static const char *path_ok(const char *directory, struct hostinfo *hi)\n {\n \tstatic char rpath[PATH_MAX];\n@@ -223,10 +187,7 @@ static const char *path_ok(const char *directory, struct hostinfo *hi)\n \t}\n \telse if (interpolated_path && hi->saw_extended_args) {\n \t\tstruct strbuf expanded_path = STRBUF_INIT;\n-\t\tstruct expand_path_context context;\n-\n-\t\tcontext.directory = directory;\n-\t\tcontext.hostinfo = hi;\n+\t\tconst char *format = interpolated_path;\n\n \t\tif (*dir != '/') {\n \t\t\t/* Allow only absolute */\n@@ -234,8 +195,24 @@ static const char *path_ok(const char *directory, struct hostinfo *hi)\n \t\t\treturn NULL;\n \t\t}\n\n-\t\tstrbuf_expand(&expanded_path, interpolated_path,\n-\t\t\t      expand_path, &context);\n+\t\twhile (strbuf_expand_step(&expanded_path, &format)) {\n+\t\t\tif (skip_prefix(format, \"%\", &format))\n+\t\t\t\tstrbuf_addch(&expanded_path, '%');\n+\t\t\telse if (skip_prefix(format, \"H\", &format))\n+\t\t\t\tstrbuf_addbuf(&expanded_path, &hi->hostname);\n+\t\t\telse if (skip_prefix(format, \"CH\", &format))\n+\t\t\t\tstrbuf_addstr(&expanded_path,\n+\t\t\t\t\t      get_canon_hostname(hi));\n+\t\t\telse if (skip_prefix(format, \"IP\", &format))\n+\t\t\t\tstrbuf_addstr(&expanded_path,\n+\t\t\t\t\t      get_ip_address(hi));\n+\t\t\telse if (skip_prefix(format, \"P\", &format))\n+\t\t\t\tstrbuf_addbuf(&expanded_path, &hi->tcp_port);\n+\t\t\telse if (skip_prefix(format, \"D\", &format))\n+\t\t\t\tstrbuf_addstr(&expanded_path, directory);\n+\t\t\telse\n+\t\t\t\tstrbuf_addch(&expanded_path, '%');\n+\t\t}\n\n \t\trlen = strlcpy(interp_path, expanded_path.buf,\n \t\t\t       sizeof(interp_path));\ndiff --git a/pretty.c b/pretty.c\nindex d2df561a05..cffbf32987 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1254,9 +1254,19 @@ static struct strbuf *expand_separator(struct strbuf *sb,\n \t\t\t\t       const char *argval, size_t arglen)\n {\n \tchar *fmt = xstrndup(argval, arglen);\n+\tconst char *format = fmt;\n\n \tstrbuf_reset(sb);\n-\tstrbuf_expand(sb, fmt, strbuf_expand_literal_cb, NULL);\n+\twhile (strbuf_expand_step(sb, &format)) {\n+\t\tsize_t len;\n+\n+\t\tif (skip_prefix(format, \"%\", &format))\n+\t\t\tstrbuf_addch(sb, '%');\n+\t\telse if ((len = strbuf_expand_literal_cb(sb, format, NULL)))\n+\t\t\tformat += len;\n+\t\telse\n+\t\t\tstrbuf_addch(sb, '%');\n+\t}\n \tfree(fmt);\n \treturn sb;\n }\n@@ -1803,7 +1813,7 @@ static size_t format_and_pad_commit(struct strbuf *sb, /* in UTF-8 */\n\n static size_t format_commit_item(struct strbuf *sb, /* in UTF-8 */\n \t\t\t\t const char *placeholder,\n-\t\t\t\t void *context)\n+\t\t\t\t struct format_commit_context *context)\n {\n \tsize_t consumed, orig_len;\n \tenum {\n@@ -1842,7 +1852,7 @@ static size_t format_commit_item(struct strbuf *sb, /* in UTF-8 */\n \t}\n\n \torig_len = sb->len;\n-\tif (((struct format_commit_context *)context)->flush_type != no_flush)\n+\tif ((context)->flush_type != no_flush)\n \t\tconsumed = format_and_pad_commit(sb, placeholder, context);\n \telse\n \t\tconsumed = format_commit_one(sb, placeholder, context);\n@@ -1861,30 +1871,6 @@ static size_t format_commit_item(struct strbuf *sb, /* in UTF-8 */\n \treturn consumed + 1;\n }\n\n-static size_t userformat_want_item(struct strbuf *sb UNUSED,\n-\t\t\t\t   const char *placeholder,\n-\t\t\t\t   void *context)\n-{\n-\tstruct userformat_want *w = context;\n-\n-\tif (*placeholder == '+' || *placeholder == '-' || *placeholder == ' ')\n-\t\tplaceholder++;\n-\n-\tswitch (*placeholder) {\n-\tcase 'N':\n-\t\tw->notes = 1;\n-\t\tbreak;\n-\tcase 'S':\n-\t\tw->source = 1;\n-\t\tbreak;\n-\tcase 'd':\n-\tcase 'D':\n-\t\tw->decorate = 1;\n-\t\tbreak;\n-\t}\n-\treturn 0;\n-}\n-\n void userformat_find_requirements(const char *fmt, struct userformat_want *w)\n {\n \tstruct strbuf dummy = STRBUF_INIT;\n@@ -1894,7 +1880,26 @@ void userformat_find_requirements(const char *fmt, struct userformat_want *w)\n \t\t\treturn;\n \t\tfmt = user_format;\n \t}\n-\tstrbuf_expand(&dummy, fmt, userformat_want_item, w);\n+\twhile (strbuf_expand_step(&dummy, &fmt)) {\n+\t\tif (skip_prefix(fmt, \"%\", &fmt))\n+\t\t\tcontinue;\n+\n+\t\tif (*fmt == '+' || *fmt == '-' || *fmt == ' ')\n+\t\t\tfmt++;\n+\n+\t\tswitch (*fmt) {\n+\t\tcase 'N':\n+\t\t\tw->notes = 1;\n+\t\t\tbreak;\n+\t\tcase 'S':\n+\t\t\tw->source = 1;\n+\t\t\tbreak;\n+\t\tcase 'd':\n+\t\tcase 'D':\n+\t\t\tw->decorate = 1;\n+\t\t\tbreak;\n+\t\t}\n+\t}\n \tstrbuf_release(&dummy);\n }\n\n@@ -1912,7 +1917,16 @@ void repo_format_commit_message(struct repository *r,\n \tconst char *output_enc = pretty_ctx->output_encoding;\n \tconst char *utf8 = \"UTF-8\";\n\n-\tstrbuf_expand(sb, format, format_commit_item, &context);\n+\twhile (strbuf_expand_step(sb, &format)) {\n+\t\tsize_t len;\n+\n+\t\tif (skip_prefix(format, \"%\", &format))\n+\t\t\tstrbuf_addch(sb, '%');\n+\t\telse if ((len = format_commit_item(sb, format, &context)))\n+\t\t\tformat += len;\n+\t\telse\n+\t\t\tstrbuf_addch(sb, '%');\n+\t}\n \trewrap_message_tail(sb, &context, 0, 0, 0);\n\n \t/*\ndiff --git a/strbuf.c b/strbuf.c\nindex 972366b410..c3d1cee616 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -427,26 +427,6 @@ int strbuf_expand_step(struct strbuf *sb, const char **formatp)\n \treturn 1;\n }\n\n-void strbuf_expand(struct strbuf *sb, const char *format, expand_fn_t fn,\n-\t\t   void *context)\n-{\n-\twhile (strbuf_expand_step(sb, &format)) {\n-\t\tsize_t consumed;\n-\n-\t\tif (*format == '%') {\n-\t\t\tstrbuf_addch(sb, '%');\n-\t\t\tformat++;\n-\t\t\tcontinue;\n-\t\t}\n-\n-\t\tconsumed = fn(sb, format, context);\n-\t\tif (consumed)\n-\t\t\tformat += consumed;\n-\t\telse\n-\t\t\tstrbuf_addch(sb, '%');\n-\t}\n-}\n-\n size_t strbuf_expand_literal_cb(struct strbuf *sb,\n \t\t\t\tconst char *placeholder,\n \t\t\t\tvoid *context UNUSED)\ndiff --git a/strbuf.h b/strbuf.h\nindex e293117f07..95e50e243e 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -318,40 +318,9 @@ const char *strbuf_join_argv(struct strbuf *buf, int argc,\n \t\t\t     const char **argv, char delim);\n\n /**\n- * This function can be used to expand a format string containing\n- * placeholders. To that end, it parses the string and calls the specified\n- * function for every percent sign found.\n- *\n- * The callback function is given a pointer to the character after the `%`\n- * and a pointer to the struct strbuf.  It is expected to add the expanded\n- * version of the placeholder to the strbuf, e.g. to add a newline\n- * character if the letter `n` appears after a `%`.  The function returns\n- * the length of the placeholder recognized and `strbuf_expand()` skips\n- * over it.\n- *\n- * The format `%%` is automatically expanded to a single `%` as a quoting\n- * mechanism; callers do not need to handle the `%` placeholder themselves,\n- * and the callback function will not be invoked for this placeholder.\n- *\n- * All other characters (non-percent and not skipped ones) are copied\n- * verbatim to the strbuf.  If the callback returned zero, meaning that the\n- * placeholder is unknown, then the percent sign is copied, too.\n- *\n- * In order to facilitate caching and to make it possible to give\n- * parameters to the callback, `strbuf_expand()` passes a context\n- * pointer with any kind of data.\n- */\n-typedef size_t (*expand_fn_t) (struct strbuf *sb,\n-\t\t\t       const char *placeholder,\n-\t\t\t       void *context);\n-void strbuf_expand(struct strbuf *sb,\n-\t\t   const char *format,\n-\t\t   expand_fn_t fn,\n-\t\t   void *context);\n-\n-/**\n- * Used as callback for `strbuf_expand` to only expand literals\n- * (i.e. %n and %xNN). The context argument is ignored.\n+ * Used with `strbuf_expand_step` to expand the literals %n and %x\n+ * followed by two hexadecimal digits. Returns the number of recognized\n+ * characters. The context argument is ignored.\n  */\n size_t strbuf_expand_literal_cb(struct strbuf *sb,\n \t\t\t\tconst char *placeholder,\n--\n2.41.0\n"},{"id":"478519","messageId":"ded69969-158d-b05f-fdd4-91b26e9b502b@web.de","threadId":"59878","inReplyTo":"767baa64-20a6-daf2-d34b-d81f72363749@web.de","subject":"[PATCH 5/5] strbuf: simplify strbuf_expand_literal_cb()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-06-17T20:44:00Z","receivedAt":"2023-06-17T20:44:06Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Now that strbuf_expand_literal_cb() is no longer used as a callback,\ndrop its \"_cb\" name suffix and unused context parameter.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n builtin/ls-files.c | 2 +-\n builtin/ls-tree.c  | 2 +-\n pretty.c           | 4 ++--\n strbuf.c           | 4 +---\n strbuf.h           | 6 ++----\n 5 files changed, 7 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/ls-files.c b/builtin/ls-files.c\nindex 03bf5771b4..0b00bd5d0f 100644\n--- a/builtin/ls-files.c\n+++ b/builtin/ls-files.c\n@@ -274,7 +274,7 @@ static void show_ce_fmt(struct repository *repo, const struct cache_entry *ce,\n\n \t\tif (skip_prefix(format, \"%\", &format))\n \t\t\tstrbuf_addch(&sb, '%');\n-\t\telse if ((len = strbuf_expand_literal_cb(&sb, format, NULL)))\n+\t\telse if ((len = strbuf_expand_literal(&sb, format)))\n \t\t\tformat += len;\n \t\telse if (*format != '(')\n \t\t\tdie(_(\"bad ls-files format: element '%s' \"\ndiff --git a/builtin/ls-tree.c b/builtin/ls-tree.c\nindex 8460d20257..a90f3c81a0 100644\n--- a/builtin/ls-tree.c\n+++ b/builtin/ls-tree.c\n@@ -108,7 +108,7 @@ static int show_tree_fmt(const struct object_id *oid, struct strbuf *base,\n\n \t\tif (skip_prefix(format, \"%\", &format))\n \t\t\tstrbuf_addch(&sb, '%');\n-\t\telse if ((len = strbuf_expand_literal_cb(&sb, format, NULL)))\n+\t\telse if ((len = strbuf_expand_literal(&sb, format)))\n \t\t\tformat += len;\n \t\telse if (*format != '(')\n \t\t\tdie(_(\"bad ls-tree format: element '%s' \"\ndiff --git a/pretty.c b/pretty.c\nindex cffbf32987..4c08f9856b 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1262,7 +1262,7 @@ static struct strbuf *expand_separator(struct strbuf *sb,\n\n \t\tif (skip_prefix(format, \"%\", &format))\n \t\t\tstrbuf_addch(sb, '%');\n-\t\telse if ((len = strbuf_expand_literal_cb(sb, format, NULL)))\n+\t\telse if ((len = strbuf_expand_literal(sb, format)))\n \t\t\tformat += len;\n \t\telse\n \t\t\tstrbuf_addch(sb, '%');\n@@ -1395,7 +1395,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n \tchar **slot;\n\n \t/* these are independent of the commit */\n-\tres = strbuf_expand_literal_cb(sb, placeholder, NULL);\n+\tres = strbuf_expand_literal(sb, placeholder);\n \tif (res)\n \t\treturn res;\n\ndiff --git a/strbuf.c b/strbuf.c\nindex c3d1cee616..55a3cfa5cf 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -427,9 +427,7 @@ int strbuf_expand_step(struct strbuf *sb, const char **formatp)\n \treturn 1;\n }\n\n-size_t strbuf_expand_literal_cb(struct strbuf *sb,\n-\t\t\t\tconst char *placeholder,\n-\t\t\t\tvoid *context UNUSED)\n+size_t strbuf_expand_literal(struct strbuf *sb, const char *placeholder)\n {\n \tint ch;\n\ndiff --git a/strbuf.h b/strbuf.h\nindex 95e50e243e..b1eab015f0 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -320,11 +320,9 @@ const char *strbuf_join_argv(struct strbuf *buf, int argc,\n /**\n  * Used with `strbuf_expand_step` to expand the literals %n and %x\n  * followed by two hexadecimal digits. Returns the number of recognized\n- * characters. The context argument is ignored.\n+ * characters.\n  */\n-size_t strbuf_expand_literal_cb(struct strbuf *sb,\n-\t\t\t\tconst char *placeholder,\n-\t\t\t\tvoid *context);\n+size_t strbuf_expand_literal(struct strbuf *sb, const char *placeholder);\n\n /**\n  * If the string pointed to by `formatp` contains a percent sign (\"%\"),\n--\n2.41.0\n"},{"id":"478529","messageId":"ZJB98Q6NHbK6D1uB@nand.local","threadId":"59878","inReplyTo":"caf8e100-1308-cb4e-d61a-4e15ee3f47f7@web.de","subject":"Re: [PATCH 2/5] strbuf: factor out strbuf_expand_step()","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-06-19T16:10:25Z","receivedAt":"2023-06-19T16:10:31Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Sat, Jun 17, 2023 at 10:41:44PM +0200, René Scharfe wrote:\n> diff --git a/strbuf.c b/strbuf.c\n> index 08eec8f1d8..a90b597da1 100644\n> --- a/strbuf.c\n> +++ b/strbuf.c\n> @@ -415,19 +415,24 @@ void strbuf_vaddf(struct strbuf *sb, const char *fmt, va_list ap)\n>  \tstrbuf_setlen(sb, sb->len + len);\n>  }\n>\n> +int strbuf_expand_step(struct strbuf *sb, const char **formatp)\n> +{\n> +\tconst char *format = *formatp;\n> +\tconst char *percent = strchrnul(format, '%');\n\nCan format be NULL here? Obviously formatp is never NULL since it is\n`&s`, but we guarded the while loop in the pre-image with `while (*s)`\nand I am not sure that **formatp is always non-NULL here.\n\nThanks,\nTaylor\n"},{"id":"478569","messageId":"fe0db8cc-4d1b-3108-1d36-939b01a95244@web.de","threadId":"59878","inReplyTo":"ZJB98Q6NHbK6D1uB@nand.local","subject":"Re: [PATCH 2/5] strbuf: factor out strbuf_expand_step()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-06-20T16:25:30Z","receivedAt":"2023-06-20T16:26:21Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 19.06.23 um 18:10 schrieb Taylor Blau:\n> On Sat, Jun 17, 2023 at 10:41:44PM +0200, René Scharfe wrote:\n>> diff --git a/strbuf.c b/strbuf.c\n>> index 08eec8f1d8..a90b597da1 100644\n>> --- a/strbuf.c\n>> +++ b/strbuf.c\n>> @@ -415,19 +415,24 @@ void strbuf_vaddf(struct strbuf *sb, const char *fmt, va_list ap)\n>>  \tstrbuf_setlen(sb, sb->len + len);\n>>  }\n>>\n>> +int strbuf_expand_step(struct strbuf *sb, const char **formatp)\n>> +{\n>> +\tconst char *format = *formatp;\n>> +\tconst char *percent = strchrnul(format, '%');\n>\n> Can format be NULL here? Obviously formatp is never NULL since it is\n> `&s`, but we guarded the while loop in the pre-image with `while (*s)`\n> and I am not sure that **formatp is always non-NULL here.\n\nThe \"*s\" in builtin/branch.c::quote_literal_for_format() that you quote\ndereferences \"s\", so we'd get a segfault if it was NULL.  A NULL (and\nNUL) check would look like \"while (s && *s)\".  The old code in strbuf.c\npasses the format to strchrnul(), which can't handle NULL either.  So\nno, format must not be NULL here, and this is not a new requirement.\n\nBut now I noticed that builtin/branch.c::quote_literal_for_format()\nonly ever gets called with \"\" or \"remotes/\", none of which needs\nquoting.  We could drop this function entirely, and add it back when\nneeded, if ever.  But that's out of the scope of this series.\n\nRené\n"},{"id":"478641","messageId":"ZJLsdZK+nT82zIAu@nand.local","threadId":"59878","inReplyTo":"fe0db8cc-4d1b-3108-1d36-939b01a95244@web.de","subject":"Re: [PATCH 2/5] strbuf: factor out strbuf_expand_step()","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-06-21T12:26:29Z","receivedAt":"2023-06-21T12:26:39Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Jun 20, 2023 at 06:25:30PM +0200, René Scharfe wrote:\n> Am 19.06.23 um 18:10 schrieb Taylor Blau:\n> > On Sat, Jun 17, 2023 at 10:41:44PM +0200, René Scharfe wrote:\n> >> diff --git a/strbuf.c b/strbuf.c\n> >> index 08eec8f1d8..a90b597da1 100644\n> >> --- a/strbuf.c\n> >> +++ b/strbuf.c\n> >> @@ -415,19 +415,24 @@ void strbuf_vaddf(struct strbuf *sb, const char *fmt, va_list ap)\n> >>  \tstrbuf_setlen(sb, sb->len + len);\n> >>  }\n> >>\n> >> +int strbuf_expand_step(struct strbuf *sb, const char **formatp)\n> >> +{\n> >> +\tconst char *format = *formatp;\n> >> +\tconst char *percent = strchrnul(format, '%');\n> >\n> > Can format be NULL here? Obviously formatp is never NULL since it is\n> > `&s`, but we guarded the while loop in the pre-image with `while (*s)`\n> > and I am not sure that **formatp is always non-NULL here.\n>\n> The \"*s\" in builtin/branch.c::quote_literal_for_format() that you quote\n> dereferences \"s\", so we'd get a segfault if it was NULL.  A NULL (and\n> NUL) check would look like \"while (s && *s)\".  The old code in strbuf.c\n> passes the format to strchrnul(), which can't handle NULL either.  So\n> no, format must not be NULL here, and this is not a new requirement.\n\nAh, thanks for catching: I agree with your reasoning.\n\n> But now I noticed that builtin/branch.c::quote_literal_for_format()\n> only ever gets called with \"\" or \"remotes/\", none of which needs\n> quoting.  We could drop this function entirely, and add it back when\n> needed, if ever.  But that's out of the scope of this series.\n\nYes, agreed.\n\nThanks,\nTaylor\n"},{"id":"478872","messageId":"20230627082622.GH1226768@coredump.intra.peff.net","threadId":"59878","inReplyTo":"caf8e100-1308-cb4e-d61a-4e15ee3f47f7@web.de","subject":"Re: [PATCH 2/5] strbuf: factor out strbuf_expand_step()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-06-27T08:26:22Z","receivedAt":"2023-06-27T08:26:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jun 17, 2023 at 10:41:44PM +0200, René Scharfe wrote:\n\n> Extract the part of strbuf_expand that finds the next placeholder into a\n> new function.  It allows to build parsers without callback functions and\n> the overhead imposed by them.\n> \n> Signed-off-by: René Scharfe <l.s.r@web.de>\n> ---\n>  builtin/branch.c | 13 ++-----------\n>  strbuf.c         | 28 ++++++++++++++--------------\n>  strbuf.h         |  8 ++++++++\n>  3 files changed, 24 insertions(+), 25 deletions(-)\n\nI was a little surprised to see the change in branch.c here (as well as\nthe one in strbuf_addftime), since the commit message just says\n\"extract...\" and doesn't mention them.\n\nThey do serve as examples of how it can be used, so I think it's OK. But\nit made me wonder: is this all of the sites? Or are these just examples?\n\n-Peff\n"},{"id":"478873","messageId":"20230627082902.GI1226768@coredump.intra.peff.net","threadId":"59878","inReplyTo":"5ce9513b-d463-6f62-db4e-f9f60a7c3e9f@web.de","subject":"Re: [PATCH 3/5] replace strbuf_expand_dict_cb() with strbuf_expand_step()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-06-27T08:29:02Z","receivedAt":"2023-06-27T08:31:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jun 17, 2023 at 10:42:26PM +0200, René Scharfe wrote:\n\n> Avoid the overhead of setting up a dictionary and passing it via\n> strbuf_expand() to strbuf_expand_dict_cb() by using strbuf_expand_step()\n> in a loop instead.  It requires explicit handling of %% and unrecognized\n> placeholders, but is more direct and simpler overall, and expands only\n> on demand.\n\nGreat. I think even replacing the dictionary with regular\nstrbuf_expand() would be an improvement in terms of the on-demand\nloading. But I am happy to see it go all the way to the iterative\nversion. :)\n\nYour comment above does make me wonder if strbuf_expand_step() should be\nquietly handling \"%%\" itself. But I guess strbuf_expand() doesn't, and\nyour branch.c quote-literal example probably would not want that\nbehavior.\n\n-Peff\n"},{"id":"478874","messageId":"20230627083526.GA1273865@coredump.intra.peff.net","threadId":"59878","inReplyTo":"20230627082902.GI1226768@coredump.intra.peff.net","subject":"Re: [PATCH 3/5] replace strbuf_expand_dict_cb() with strbuf_expand_step()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-06-27T08:35:26Z","receivedAt":"2023-06-27T08:35:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 27, 2023 at 04:29:02AM -0400, Jeff King wrote:\n\n> Your comment above does make me wonder if strbuf_expand_step() should be\n> quietly handling \"%%\" itself. But I guess strbuf_expand() doesn't, and\n> your branch.c quote-literal example probably would not want that\n> behavior.\n\nEr, nope, strbuf_expand() does handle \"%%\" itself. It really feels like\nwe'd want strbuf_expand_step() to do so, too, then. Even if we had two\nvariants (a \"raw\" one which doesn't handle \"%%\" so that branch.c could\nuse it, and then another that wrapped it to handle \"%%\").\n\n-Peff\n"},{"id":"478875","messageId":"20230627085422.GJ1226768@coredump.intra.peff.net","threadId":"59878","inReplyTo":"150df0b0-0548-f291-2b68-960841dd1c97@web.de","subject":"Re: [PATCH 4/5] replace strbuf_expand() with strbuf_expand_step()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-06-27T08:54:22Z","receivedAt":"2023-06-27T08:54:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jun 17, 2023 at 10:43:17PM +0200, René Scharfe wrote:\n\n> Avoid the overhead of passing context to a callback function of\n> strbuf_expand() by using strbuf_expand_step() in a loop instead.  It\n> requires explicit handling of %% and unrecognized placeholders, but is\n> simpler, more direct and avoids void pointers.\n\nI like this. I don't care that much about the runtime overhead of\npassing the context around, but if you meant by \"overhead\" the\nprogrammer time and confusion in stuffing everything into context\nstructs, then I agree this is much better. :)\n\nIt does still feel like we should be handling \"%%\" on behalf of the\ncallers.\n\n>  builtin/cat-file.c |  35 +++++++--------\n>  builtin/ls-files.c | 109 +++++++++++++++++++--------------------------\n>  builtin/ls-tree.c  | 107 +++++++++++++++++---------------------------\n>  daemon.c           |  61 ++++++++-----------------\n>  pretty.c           |  72 ++++++++++++++++++------------\n>  strbuf.c           |  20 ---------\n>  strbuf.h           |  37 ++-------------\n>  7 files changed, 169 insertions(+), 272 deletions(-)\n\nThe changes mostly looked OK to me (and the diffstat is certainly\npleasing). The old callbacks returned a \"consumed\" length, and we need\neach \"step\" caller to now do \"format += consumed\" themselves. At first\nglance I thought there were cases where you didn't, but then I realized\nthat you are relying on skip_prefix() to do that incrementing. Which\nmakes sense and the resulting code looks nice, but it took me a minute\nto realize what was going on.\n\n> @@ -1894,7 +1880,26 @@ void userformat_find_requirements(const char *fmt, struct userformat_want *w)\n>  \t\t\treturn;\n>  \t\tfmt = user_format;\n>  \t}\n> -\tstrbuf_expand(&dummy, fmt, userformat_want_item, w);\n> +\twhile (strbuf_expand_step(&dummy, &fmt)) {\n> +\t\tif (skip_prefix(fmt, \"%\", &fmt))\n> +\t\t\tcontinue;\n> +\n> +\t\tif (*fmt == '+' || *fmt == '-' || *fmt == ' ')\n> +\t\t\tfmt++;\n> +\n> +\t\tswitch (*fmt) {\n> +\t\tcase 'N':\n> +\t\t\tw->notes = 1;\n> +\t\t\tbreak;\n> +\t\tcase 'S':\n> +\t\t\tw->source = 1;\n> +\t\t\tbreak;\n> +\t\tcase 'd':\n> +\t\tcase 'D':\n> +\t\t\tw->decorate = 1;\n> +\t\t\tbreak;\n> +\t\t}\n> +\t}\n>  \tstrbuf_release(&dummy);\n>  }\n\nThis one actually doesn't increment the format (so we restart the\nexpansion on \"N\" or whatever). But neither did the original! It always\nreturned 0:\n\n> -static size_t userformat_want_item(struct strbuf *sb UNUSED,\n> -\t\t\t\t   const char *placeholder,\n> -\t\t\t\t   void *context)\n> -{\n> -\tstruct userformat_want *w = context;\n> -\n> -\tif (*placeholder == '+' || *placeholder == '-' || *placeholder == ' ')\n> -\t\tplaceholder++;\n> -\n> -\tswitch (*placeholder) {\n> -\tcase 'N':\n> -\t\tw->notes = 1;\n> -\t\tbreak;\n> -\tcase 'S':\n> -\t\tw->source = 1;\n> -\t\tbreak;\n> -\tcase 'd':\n> -\tcase 'D':\n> -\t\tw->decorate = 1;\n> -\t\tbreak;\n> -\t}\n> -\treturn 0;\n> -}\n\nSo probably OK, though a little funny.\n\nIt also feels like this whole function would be just as happy using\n\"strchr()\", since it throws away the expanded result anyway. But that\ncan be for another time. :)\n\n> @@ -1912,7 +1917,16 @@ void repo_format_commit_message(struct repository *r,\n>  \tconst char *output_enc = pretty_ctx->output_encoding;\n>  \tconst char *utf8 = \"UTF-8\";\n> \n> -\tstrbuf_expand(sb, format, format_commit_item, &context);\n> +\twhile (strbuf_expand_step(sb, &format)) {\n> +\t\tsize_t len;\n> +\n> +\t\tif (skip_prefix(format, \"%\", &format))\n> +\t\t\tstrbuf_addch(sb, '%');\n> +\t\telse if ((len = format_commit_item(sb, format, &context)))\n> +\t\t\tformat += len;\n> +\t\telse\n> +\t\t\tstrbuf_addch(sb, '%');\n> +\t}\n\nThis one doesn't advance the format for a not-understood placeholder.\nBut that's OK, because we know it isn't \"%\", so starting the search from\nthere again is correct.\n\n> @@ -1842,7 +1852,7 @@ static size_t format_commit_item(struct strbuf *sb, /* in UTF-8 */\n>  \t}\n> \n>  \torig_len = sb->len;\n> -\tif (((struct format_commit_context *)context)->flush_type != no_flush)\n> +\tif ((context)->flush_type != no_flush)\n>  \t\tconsumed = format_and_pad_commit(sb, placeholder, context);\n>  \telse\n>  \t\tconsumed = format_commit_one(sb, placeholder, context);\n\nSince we're no longer casting, the extra parentheses seem redundant now.\nI.e., this can just be context->flush_type.\n\n-Peff\n"},{"id":"478876","messageId":"20230627085701.GK1226768@coredump.intra.peff.net","threadId":"59878","inReplyTo":"ded69969-158d-b05f-fdd4-91b26e9b502b@web.de","subject":"Re: [PATCH 5/5] strbuf: simplify strbuf_expand_literal_cb()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-06-27T08:57:01Z","receivedAt":"2023-06-27T08:57:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jun 17, 2023 at 10:44:00PM +0200, René Scharfe wrote:\n\n> Now that strbuf_expand_literal_cb() is no longer used as a callback,\n> drop its \"_cb\" name suffix and unused context parameter.\n\nMakes sense.\n\nSince most callers just call \"format += len\", it kind of feels like the\nappropriate interface might be more like:\n\n  strbuf_expand_literal(&sb, &format);\n\nto auto-advance the format. But I guess that gets weird with this\ncaller:\n\n> @@ -1395,7 +1395,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n>  \tchar **slot;\n> \n>  \t/* these are independent of the commit */\n> -\tres = strbuf_expand_literal_cb(sb, placeholder, NULL);\n> +\tres = strbuf_expand_literal(sb, placeholder);\n>  \tif (res)\n>  \t\treturn res;\n\nwhich is still in the \"return the length\" mentality (OTOH, if it\nadvanced the local copy of the placeholder pointer nobody would mind).\n\nI dunno. It is a little thing, and I am OK with it either way (I would\nnot even think of changing it if you were not already touching the\nfunction).\n\n-Peff\n"},{"id":"478908","messageId":"6fc5092d-2095-5b44-cc26-1e7208a342f0@web.de","threadId":"59878","inReplyTo":"20230627082622.GH1226768@coredump.intra.peff.net","subject":"Re: [PATCH 2/5] strbuf: factor out strbuf_expand_step()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-06-27T16:21:57Z","receivedAt":"2023-06-27T16:22:18Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 27.06.23 um 10:26 schrieb Jeff King:\n> On Sat, Jun 17, 2023 at 10:41:44PM +0200, René Scharfe wrote:\n>\n>> Extract the part of strbuf_expand that finds the next placeholder into a\n>> new function.  It allows to build parsers without callback functions and\n>> the overhead imposed by them.\n>>\n>> Signed-off-by: René Scharfe <l.s.r@web.de>\n>> ---\n>>  builtin/branch.c | 13 ++-----------\n>>  strbuf.c         | 28 ++++++++++++++--------------\n>>  strbuf.h         |  8 ++++++++\n>>  3 files changed, 24 insertions(+), 25 deletions(-)\n>\n> I was a little surprised to see the change in branch.c here (as well as\n> the one in strbuf_addftime), since the commit message just says\n> \"extract...\" and doesn't mention them.\n\nFair point, the other extraction sources should have been mentioned as\nwell somehow.\n\n> They do serve as examples of how it can be used, so I think it's OK. But\n> it made me wonder: is this all of the sites? Or are these just examples?\n\nThese are all that I could find.\n\nRené\n"},{"id":"478909","messageId":"68593128-21e8-a728-719c-632fccf740d6@web.de","threadId":"59878","inReplyTo":"20230627083526.GA1273865@coredump.intra.peff.net","subject":"Re: [PATCH 3/5] replace strbuf_expand_dict_cb() with strbuf_expand_step()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-06-27T16:24:12Z","receivedAt":"2023-06-27T16:24:25Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 27.06.23 um 10:35 schrieb Jeff King:\n> On Tue, Jun 27, 2023 at 04:29:02AM -0400, Jeff King wrote:\n>\n>> Your comment above does make me wonder if strbuf_expand_step() should be\n>> quietly handling \"%%\" itself. But I guess strbuf_expand() doesn't, and\n>> your branch.c quote-literal example probably would not want that\n>> behavior.\n>\n> Er, nope, strbuf_expand() does handle \"%%\" itself. It really feels like\n> we'd want strbuf_expand_step() to do so, too, then. Even if we had two\n> variants (a \"raw\" one which doesn't handle \"%%\" so that branch.c could\n> use it, and then another that wrapped it to handle \"%%\").\n\nstrbuf_expand() handles %% and unrecognized placeholders.\n\nWe could do that, or move the %% handling to strbuf_expand_literal or\nsomething else.  E.g. initially I had a strbuf_expand_default that\nhandled %% and unrecognized placeholders.\n\nI think it's a good idea to first get used to the loop paradigm by\neschewing the sugary automatic handling and having everything spelled\nout explicitly.  It's just one more branch.  Then we can come up with\na better factoring after a while.\n\nRené\n"},{"id":"478910","messageId":"8d2223b8-ab33-be5a-20ea-ad60f6cbcc75@web.de","threadId":"59878","inReplyTo":"20230627085422.GJ1226768@coredump.intra.peff.net","subject":"Re: [PATCH 4/5] replace strbuf_expand() with strbuf_expand_step()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-06-27T16:31:55Z","receivedAt":"2023-06-27T16:32:50Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 27.06.23 um 10:54 schrieb Jeff King:\n> On Sat, Jun 17, 2023 at 10:43:17PM +0200, René Scharfe wrote:\n>\n>> Avoid the overhead of passing context to a callback function of\n>> strbuf_expand() by using strbuf_expand_step() in a loop instead.  It\n>> requires explicit handling of %% and unrecognized placeholders, but is\n>> simpler, more direct and avoids void pointers.\n>\n> I like this. I don't care that much about the runtime overhead of\n> passing the context around, but if you meant by \"overhead\" the\n> programmer time and confusion in stuffing everything into context\n> structs, then I agree this is much better. :)\n\nIndeed, I meant the burden of being forced to define a struct and\nfilling in all necessary context.  Bureaucratic overhead.\n\n> It does still feel like we should be handling \"%%\" on behalf of the\n> callers.\n\nI feel the same, but restrained myself from doing that, so that we\ncan see all the pieces for now.  It allows us to recombine them in\nbetter ways than before.\n\n>>  builtin/cat-file.c |  35 +++++++--------\n>>  builtin/ls-files.c | 109 +++++++++++++++++++--------------------------\n>>  builtin/ls-tree.c  | 107 +++++++++++++++++---------------------------\n>>  daemon.c           |  61 ++++++++-----------------\n>>  pretty.c           |  72 ++++++++++++++++++------------\n>>  strbuf.c           |  20 ---------\n>>  strbuf.h           |  37 ++-------------\n>>  7 files changed, 169 insertions(+), 272 deletions(-)\n>\n> The changes mostly looked OK to me (and the diffstat is certainly\n> pleasing). The old callbacks returned a \"consumed\" length, and we need\n> each \"step\" caller to now do \"format += consumed\" themselves. At first\n> glance I thought there were cases where you didn't, but then I realized\n> that you are relying on skip_prefix() to do that incrementing. Which\n> makes sense and the resulting code looks nice, but it took me a minute\n> to realize what was going on.\n\n*nod*  The \"returns consumed length\" style is still used by\nstrbuf_expand_literal, though, so we have a bit of a mix.  Which\nworks, but a uniform convention might be better.\n\n>> @@ -1894,7 +1880,26 @@ void userformat_find_requirements(const char *fmt, struct userformat_want *w)\n>>  \t\t\treturn;\n>>  \t\tfmt = user_format;\n>>  \t}\n>> -\tstrbuf_expand(&dummy, fmt, userformat_want_item, w);\n>> +\twhile (strbuf_expand_step(&dummy, &fmt)) {\n>> +\t\tif (skip_prefix(fmt, \"%\", &fmt))\n>> +\t\t\tcontinue;\n>> +\n>> +\t\tif (*fmt == '+' || *fmt == '-' || *fmt == ' ')\n>> +\t\t\tfmt++;\n>> +\n>> +\t\tswitch (*fmt) {\n>> +\t\tcase 'N':\n>> +\t\t\tw->notes = 1;\n>> +\t\t\tbreak;\n>> +\t\tcase 'S':\n>> +\t\t\tw->source = 1;\n>> +\t\t\tbreak;\n>> +\t\tcase 'd':\n>> +\t\tcase 'D':\n>> +\t\t\tw->decorate = 1;\n>> +\t\t\tbreak;\n>> +\t\t}\n>> +\t}\n>>  \tstrbuf_release(&dummy);\n>>  }\n>\n> This one actually doesn't increment the format (so we restart the\n> expansion on \"N\" or whatever). But neither did the original! It always\n> returned 0:\n>\n>> -static size_t userformat_want_item(struct strbuf *sb UNUSED,\n>> -\t\t\t\t   const char *placeholder,\n>> -\t\t\t\t   void *context)\n>> -{\n>> -\tstruct userformat_want *w = context;\n>> -\n>> -\tif (*placeholder == '+' || *placeholder == '-' || *placeholder == ' ')\n>> -\t\tplaceholder++;\n>> -\n>> -\tswitch (*placeholder) {\n>> -\tcase 'N':\n>> -\t\tw->notes = 1;\n>> -\t\tbreak;\n>> -\tcase 'S':\n>> -\t\tw->source = 1;\n>> -\t\tbreak;\n>> -\tcase 'd':\n>> -\tcase 'D':\n>> -\t\tw->decorate = 1;\n>> -\t\tbreak;\n>> -\t}\n>> -\treturn 0;\n>> -}\n>\n> So probably OK, though a little funny.\n>\n> It also feels like this whole function would be just as happy using\n> \"strchr()\", since it throws away the expanded result anyway. But that\n> can be for another time. :)\n\nGood idea!  And the conversion to a loop brings us halfway there.\n\n>\n>> @@ -1912,7 +1917,16 @@ void repo_format_commit_message(struct repository *r,\n>>  \tconst char *output_enc = pretty_ctx->output_encoding;\n>>  \tconst char *utf8 = \"UTF-8\";\n>>\n>> -\tstrbuf_expand(sb, format, format_commit_item, &context);\n>> +\twhile (strbuf_expand_step(sb, &format)) {\n>> +\t\tsize_t len;\n>> +\n>> +\t\tif (skip_prefix(format, \"%\", &format))\n>> +\t\t\tstrbuf_addch(sb, '%');\n>> +\t\telse if ((len = format_commit_item(sb, format, &context)))\n>> +\t\t\tformat += len;\n>> +\t\telse\n>> +\t\t\tstrbuf_addch(sb, '%');\n>> +\t}\n>\n> This one doesn't advance the format for a not-understood placeholder.\n> But that's OK, because we know it isn't \"%\", so starting the search from\n> there again is correct.\n\nRight.  This is copied from strbuf_expand.\n\n>\n>> @@ -1842,7 +1852,7 @@ static size_t format_commit_item(struct strbuf *sb, /* in UTF-8 */\n>>  \t}\n>>\n>>  \torig_len = sb->len;\n>> -\tif (((struct format_commit_context *)context)->flush_type != no_flush)\n>> +\tif ((context)->flush_type != no_flush)\n>>  \t\tconsumed = format_and_pad_commit(sb, placeholder, context);\n>>  \telse\n>>  \t\tconsumed = format_commit_one(sb, placeholder, context);\n>\n> Since we're no longer casting, the extra parentheses seem redundant now.\n> I.e., this can just be context->flush_type.\n\nIndeed.\n\nRené\n"},{"id":"478911","messageId":"3654f95d-5709-28f6-eeb2-ca62a1ee789c@web.de","threadId":"59878","inReplyTo":"20230627085701.GK1226768@coredump.intra.peff.net","subject":"Re: [PATCH 5/5] strbuf: simplify strbuf_expand_literal_cb()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-06-27T16:32:22Z","receivedAt":"2023-06-27T16:33:11Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 27.06.23 um 10:57 schrieb Jeff King:\n> On Sat, Jun 17, 2023 at 10:44:00PM +0200, René Scharfe wrote:\n>\n>> Now that strbuf_expand_literal_cb() is no longer used as a callback,\n>> drop its \"_cb\" name suffix and unused context parameter.\n>\n> Makes sense.\n>\n> Since most callers just call \"format += len\", it kind of feels like the\n> appropriate interface might be more like:\n>\n>   strbuf_expand_literal(&sb, &format);\n>\n> to auto-advance the format. But I guess that gets weird with this\n> caller:\n>\n>> @@ -1395,7 +1395,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n>>  \tchar **slot;\n>>\n>>  \t/* these are independent of the commit */\n>> -\tres = strbuf_expand_literal_cb(sb, placeholder, NULL);\n>> +\tres = strbuf_expand_literal(sb, placeholder);\n>>  \tif (res)\n>>  \t\treturn res;\n>\n> which is still in the \"return the length\" mentality (OTOH, if it\n> advanced the local copy of the placeholder pointer nobody would mind).\n\nYes, I only grabbed the two low-hanging fruits here.\n\nA format-advancing version would also look a bit weird in an if/else\ncascade:\n\n\telse if (strbuf_expand_literal(&sb, &format))\n\t\t; /* nothing */\n\telse ...\n\n> I dunno. It is a little thing, and I am OK with it either way (I would\n> not even think of changing it if you were not already touching the\n> function).\n\nUnsure.  Should I overcome my horror vacui and let the /* nothing */ in?\n\nRené\n"},{"id":"478931","messageId":"20230627201923.GB1358527@coredump.intra.peff.net","threadId":"59878","inReplyTo":"8d2223b8-ab33-be5a-20ea-ad60f6cbcc75@web.de","subject":"Re: [PATCH 4/5] replace strbuf_expand() with strbuf_expand_step()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-06-27T20:19:23Z","receivedAt":"2023-06-27T20:20:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 27, 2023 at 06:31:55PM +0200, René Scharfe wrote:\n\n> > It does still feel like we should be handling \"%%\" on behalf of the\n> > callers.\n> \n> I feel the same, but restrained myself from doing that, so that we\n> can see all the pieces for now.  It allows us to recombine them in\n> better ways than before.\n\nYeah, since you did the work to handle \"%%\" in each caller, I'm OK with\nproceeding and letting a later refactor shrink it back down if we\nchoose.\n\n-Peff\n"},{"id":"478932","messageId":"20230627202159.GC1358527@coredump.intra.peff.net","threadId":"59878","inReplyTo":"3654f95d-5709-28f6-eeb2-ca62a1ee789c@web.de","subject":"Re: [PATCH 5/5] strbuf: simplify strbuf_expand_literal_cb()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-06-27T20:21:59Z","receivedAt":"2023-06-27T20:22:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 27, 2023 at 06:32:22PM +0200, René Scharfe wrote:\n\n> A format-advancing version would also look a bit weird in an if/else\n> cascade:\n> \n> \telse if (strbuf_expand_literal(&sb, &format))\n> \t\t; /* nothing */\n> \telse ...\n> \n> > I dunno. It is a little thing, and I am OK with it either way (I would\n> > not even think of changing it if you were not already touching the\n> > function).\n> \n> Unsure.  Should I overcome my horror vacui and let the /* nothing */ in?\n\nHeh. It is a little funny to have an empty block. But at the same time,\nit aligns the conditional with all of the skip_prefix() calls, which are\nadvancing \"format\" as a side effect of matching.\n\nSo I could go either way (and we can always change it on top).\n\nI think based on your responses that there's nothing that would require\na re-roll. The only thing left from my review is the extra parentheses\nin format_commit_item, which could possibly be fixed up while applying.\n\n-Peff\n"}]}