{"thread":{"id":"63162","subject":"[PATCH 0/8] pretty: minor bugfixing, some refactorings","startedAt":"2025-03-19T07:24:56Z","lastAt":"2025-03-24T10:10:11Z","messageCount":22,"participants":["Martin Ågren","Patrick Steinhardt","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":8},"messages":[{"id":"514606","messageId":"cover.1742367347.git.martin.agren@gmail.com","threadId":"63162","inReplyTo":null,"subject":"[PATCH 0/8] pretty: minor bugfixing, some refactorings","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2025-03-19T07:23:33Z","receivedAt":"2025-03-19T07:24:56Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Hi,\n\nI've been going through pretty.c with an eye to pulling apart parsing of\nplaceholders and formatting of strings. These are some initial cleanups\nI've accumulated in the process. I think they should be valuable on\ntheir own, even if I eventually abandon the bigger project.\n\nMartin Ågren (8):\n  pretty: tighten function signature to not take `void *`\n  pretty: simplify if-else to reduce code duplication\n\nTwo more or less simple cleanups.\n\n  pretty: collect padding-related fields in separate struct\n  pretty: fix parsing of half-valid \"%<\" and \"%>\" placeholders\n  pretty: after padding, reset padding info\n\nFix a bug or two in parsing and handling of padding placeholders.\n\n  pretty: refactor parsing of line-wrapping \"%w\" placeholder\n  pretty: refactor parsing of magic\n  pretty: refactor parsing of decoration options\n\nFurther splitting without any intended changes in behavior. These\nrefactorings aren't followed by any patch that actually benefits from\nthem. Still, I do think these end up making the code a tiny bit easier\nto understand by way of having smaller functions and collecting related\npieces of data into smaller structs.\n\nUnless the original authors are (TTBOMK) no longer around, I'm cc-ing\nthem on the individual patches and on this cover letter.\n\nMartin\n\n pretty.c                      | 300 +++++++++++++++++++++-------------\n t/t4205-log-pretty-formats.sh |  15 ++\n 2 files changed, 197 insertions(+), 118 deletions(-)\n\n-- \n2.49.0.472.ge94155a9ec\n\n"},{"id":"514607","messageId":"192fc78dd869f28cb6ae91f3a26a05eb6b6a4bbf.1742367347.git.martin.agren@gmail.com","threadId":"63162","inReplyTo":"cover.1742367347.git.martin.agren@gmail.com","subject":"[PATCH 1/8] pretty: tighten function signature to not take `void *`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2025-03-19T07:23:34Z","receivedAt":"2025-03-19T07:25:08Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"We take a `void *` and immediately cast it. Both callers already have\nthis pointer as the right type, so tighten the interface and stop\ncasting.\n\nSigned-off-by: Martin Ågren <martin.agren@gmail.com>\n---\n pretty.c | 3 +--\n 1 file changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/pretty.c b/pretty.c\nindex 0bc8ad8a9a..a4e5fc5c50 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1437,9 +1437,8 @@ static void free_decoration_options(const struct decoration_options *opts)\n \n static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n \t\t\t\tconst char *placeholder,\n-\t\t\t\tvoid *context)\n+\t\t\t\tstruct format_commit_context *c)\n {\n-\tstruct format_commit_context *c = context;\n \tconst struct commit *commit = c->commit;\n \tconst char *msg = c->message;\n \tstruct commit_list *p;\n-- \n2.49.0.472.ge94155a9ec\n\n"},{"id":"514608","messageId":"5f787ddac2d80391feadb8cf6be379fc8e58652f.1742367347.git.martin.agren@gmail.com","threadId":"63162","inReplyTo":"cover.1742367347.git.martin.agren@gmail.com","subject":"[PATCH 2/8] pretty: simplify if-else to reduce code duplication","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2025-03-19T07:23:35Z","receivedAt":"2025-03-19T07:25:14Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"First we look for \"auto,\", then we try \"always,\", then we fall back to\nthe default, which is to do exactly the same thing as we do for \"auto,\".\nThe amount of code duplication isn't huge, but still: reading this code\ncarefully requires spending at least *some* time on making sure the two\nblocks of code are indeed identical.\n\nRearrange the checks so that we end with the default case,\nopportunistically consuming the \"auto,\" which may or may not be there.\n\nIn the \"always,\" case, we don't actually *do* anything, so if we were\ninto golfing, we'd just write the whole thing as a single\n\n  if (!skip_prefix(begin, \"always,\", &begin)) {\n    ...\n  }\n\nIf we ever learn something new besides \"always,\" and \"auto,\" we'd need\nto pull things apart again. Plus we still need somewhere to place the\ncomment. Let's focus on code de-duplication rather than golfing for now.\n\nSigned-off-by: Martin Ågren <martin.agren@gmail.com>\n---\n pretty.c | 6 ++----\n 1 file changed, 2 insertions(+), 4 deletions(-)\n\ndiff --git a/pretty.c b/pretty.c\nindex a4e5fc5c50..6a4264dd01 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1076,13 +1076,11 @@ static size_t parse_color(struct strbuf *sb, /* in UTF-8 */\n \t\tif (!end)\n \t\t\treturn 0;\n \n-\t\tif (skip_prefix(begin, \"auto,\", &begin)) {\n-\t\t\tif (!want_color(c->pretty_ctx->color))\n-\t\t\t\treturn end - placeholder + 1;\n-\t\t} else if (skip_prefix(begin, \"always,\", &begin)) {\n+\t\tif (skip_prefix(begin, \"always,\", &begin)) {\n \t\t\t/* nothing to do; we do not respect want_color at all */\n \t\t} else {\n \t\t\t/* the default is the same as \"auto\" */\n+\t\t\tskip_prefix(begin, \"auto,\", &begin);\n \t\t\tif (!want_color(c->pretty_ctx->color))\n \t\t\t\treturn end - placeholder + 1;\n \t\t}\n-- \n2.49.0.472.ge94155a9ec\n\n"},{"id":"514609","messageId":"1adaa171fb2de74aef811bb5e410a08da72718cf.1742367347.git.martin.agren@gmail.com","threadId":"63162","inReplyTo":"cover.1742367347.git.martin.agren@gmail.com","subject":"[PATCH 3/8] pretty: collect padding-related fields in separate struct","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2025-03-19T07:23:36Z","receivedAt":"2025-03-19T07:25:16Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Padding (\"%<\" and \"%>\") involves three fields of `struct\nformat_commit_context`. This goes all the way back to commits a57523428b\n(pretty: support padding placeholders, %< %> and %><, 2013-04-19) and\n1640632b4f (pretty: support %>> that steal trailing spaces, 2013-04-19).\nThese fields are not used for anything else.\n\nMake that clearer by collecting them into their own little struct. Let\nour parser populate just that struct to make it obvious that the rest of\nthe big struct does not influence the parsing.\n\nSigned-off-by: Martin Ågren <martin.agren@gmail.com>\n---\n pretty.c | 42 +++++++++++++++++++++++-------------------\n 1 file changed, 23 insertions(+), 19 deletions(-)\n\ndiff --git a/pretty.c b/pretty.c\nindex 6a4264dd01..e5e8ef24fa 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -887,6 +887,12 @@ enum trunc_type {\n \ttrunc_right\n };\n \n+struct padding_args {\n+\tenum flush_type flush_type;\n+\tenum trunc_type truncate;\n+\tint padding;\n+};\n+\n struct format_commit_context {\n \tstruct repository *repository;\n \tconst struct commit *commit;\n@@ -894,13 +900,11 @@ struct format_commit_context {\n \tunsigned commit_header_parsed:1;\n \tunsigned commit_message_parsed:1;\n \tstruct signature_check signature_check;\n-\tenum flush_type flush_type;\n-\tenum trunc_type truncate;\n \tconst char *message;\n \tchar *commit_encoding;\n \tsize_t width, indent1, indent2;\n \tint auto_color;\n-\tint padding;\n+\tstruct padding_args pad;\n \n \t/* These offsets are relative to the start of the commit message. */\n \tstruct chunk author;\n@@ -1112,7 +1116,7 @@ static size_t parse_color(struct strbuf *sb, /* in UTF-8 */\n }\n \n static size_t parse_padding_placeholder(const char *placeholder,\n-\t\t\t\t\tstruct format_commit_context *c)\n+\t\t\t\t\tstruct padding_args *p)\n {\n \tconst char *ch = placeholder;\n \tenum flush_type flush_type;\n@@ -1167,8 +1171,8 @@ static size_t parse_padding_placeholder(const char *placeholder,\n \t\t\tif (width < 0)\n \t\t\t\treturn 0;\n \t\t}\n-\t\tc->padding = to_column ? -width : width;\n-\t\tc->flush_type = flush_type;\n+\t\tp->padding = to_column ? -width : width;\n+\t\tp->flush_type = flush_type;\n \n \t\tif (*end == ',') {\n \t\t\tstart = end + 1;\n@@ -1176,15 +1180,15 @@ static size_t parse_padding_placeholder(const char *placeholder,\n \t\t\tif (!end || end == start)\n \t\t\t\treturn 0;\n \t\t\tif (starts_with(start, \"trunc)\"))\n-\t\t\t\tc->truncate = trunc_right;\n+\t\t\t\tp->truncate = trunc_right;\n \t\t\telse if (starts_with(start, \"ltrunc)\"))\n-\t\t\t\tc->truncate = trunc_left;\n+\t\t\t\tp->truncate = trunc_left;\n \t\t\telse if (starts_with(start, \"mtrunc)\"))\n-\t\t\t\tc->truncate = trunc_middle;\n+\t\t\t\tp->truncate = trunc_middle;\n \t\t\telse\n \t\t\t\treturn 0;\n \t\t} else\n-\t\t\tc->truncate = trunc_none;\n+\t\t\tp->truncate = trunc_none;\n \n \t\treturn end - placeholder + 1;\n \t}\n@@ -1504,7 +1508,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n \n \tcase '<':\n \tcase '>':\n-\t\treturn parse_padding_placeholder(placeholder, c);\n+\t\treturn parse_padding_placeholder(placeholder, &c->pad);\n \t}\n \n \tif (skip_prefix(placeholder, \"(describe\", &arg)) {\n@@ -1788,7 +1792,7 @@ static size_t format_and_pad_commit(struct strbuf *sb, /* in UTF-8 */\n {\n \tstruct strbuf local_sb = STRBUF_INIT;\n \tsize_t total_consumed = 0;\n-\tint len, padding = c->padding;\n+\tint len, padding = c->pad.padding;\n \n \tif (padding < 0) {\n \t\tconst char *start = strrchr(sb->buf, '\\n');\n@@ -1815,7 +1819,7 @@ static size_t format_and_pad_commit(struct strbuf *sb, /* in UTF-8 */\n \t}\n \tlen = utf8_strnwidth(local_sb.buf, local_sb.len, 1);\n \n-\tif (c->flush_type == flush_left_and_steal) {\n+\tif (c->pad.flush_type == flush_left_and_steal) {\n \t\tconst char *ch = sb->buf + sb->len - 1;\n \t\twhile (len > padding && ch > sb->buf) {\n \t\t\tconst char *p;\n@@ -1841,11 +1845,11 @@ static size_t format_and_pad_commit(struct strbuf *sb, /* in UTF-8 */\n \t\t\tch = p - 1;\n \t\t}\n \t\tstrbuf_setlen(sb, ch + 1 - sb->buf);\n-\t\tc->flush_type = flush_left;\n+\t\tc->pad.flush_type = flush_left;\n \t}\n \n \tif (len > padding) {\n-\t\tswitch (c->truncate) {\n+\t\tswitch (c->pad.truncate) {\n \t\tcase trunc_left:\n \t\t\tstrbuf_utf8_replace(&local_sb,\n \t\t\t\t\t    0, len - (padding - 2),\n@@ -1868,9 +1872,9 @@ static size_t format_and_pad_commit(struct strbuf *sb, /* in UTF-8 */\n \t\tstrbuf_addbuf(sb, &local_sb);\n \t} else {\n \t\tsize_t sb_len = sb->len, offset = 0;\n-\t\tif (c->flush_type == flush_left)\n+\t\tif (c->pad.flush_type == flush_left)\n \t\t\toffset = padding - len;\n-\t\telse if (c->flush_type == flush_both)\n+\t\telse if (c->pad.flush_type == flush_both)\n \t\t\toffset = (padding - len) / 2;\n \t\t/*\n \t\t * we calculate padding in columns, now\n@@ -1882,7 +1886,7 @@ static size_t format_and_pad_commit(struct strbuf *sb, /* in UTF-8 */\n \t\t       local_sb.len);\n \t}\n \tstrbuf_release(&local_sb);\n-\tc->flush_type = no_flush;\n+\tc->pad.flush_type = no_flush;\n \treturn total_consumed;\n }\n \n@@ -1927,7 +1931,7 @@ static size_t format_commit_item(struct strbuf *sb, /* in UTF-8 */\n \t}\n \n \torig_len = sb->len;\n-\tif (context->flush_type == no_flush)\n+\tif (context->pad.flush_type == no_flush)\n \t\tconsumed = format_commit_one(sb, placeholder, context);\n \telse\n \t\tconsumed = format_and_pad_commit(sb, placeholder, context);\n-- \n2.49.0.472.ge94155a9ec\n\n"},{"id":"514610","messageId":"7d6b62006ecaf7db159e8db0c85455ed58027ce6.1742367347.git.martin.agren@gmail.com","threadId":"63162","inReplyTo":"cover.1742367347.git.martin.agren@gmail.com","subject":"[PATCH 4/8] pretty: fix parsing of half-valid \"%<\" and \"%>\" placeholders","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2025-03-19T07:23:37Z","receivedAt":"2025-03-19T07:25:18Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"When we parse a padding directive (\"%<\" or \"%>\"), we might populate a\nfew of the struct's fields before bailing. This can result in such\nhalf-parsed information being used to actually introduce some\npadding/truncation.\n\nWhen parsing a \"%<\" or \"%>\", only store the parsed data after parsing\nsuccessfully. The added test would have failed before this commit. It\nalso shows how the existing behavior is hardly something someone can\nrely on since the non-consumed modifier (\"%<(10,bad)\") shows up verbatim\nin the pretty output.\n\nWe could let the caller use a temporary struct and only copy the data on\nsuccess. Let's instead make our parsing function easy to use correctly\nby letting it only touch the output struct in the success case.\n\nWhile setting up a temporary struct for parsing into, we might as well\ninitialize it to a well-defined state. It's unnecessary for the current\nimplementation since it always writes to all three fields in a\nsuccessful case, but some future-proofing shouldn't hurt.\n\nNote that the test relies on first using a correct placeholder\n\"%<(4,trunc)\" where \"trunc\" (`trunc_right`) lingers in our struct until\nit's then used instead of the invalid \"bad\". The next commit will teach\nus to clean up any remnants of \"%<(4,trunc)\" after handling it.\n\nSigned-off-by: Martin Ågren <martin.agren@gmail.com>\n---\n pretty.c                      | 18 ++++++++++++------\n t/t4205-log-pretty-formats.sh |  6 ++++++\n 2 files changed, 18 insertions(+), 6 deletions(-)\n\ndiff --git a/pretty.c b/pretty.c\nindex e5e8ef24fa..a4fa052f8b 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1121,6 +1121,11 @@ static size_t parse_padding_placeholder(const char *placeholder,\n \tconst char *ch = placeholder;\n \tenum flush_type flush_type;\n \tint to_column = 0;\n+\tstruct padding_args ans = {\n+\t\t.flush_type = no_flush,\n+\t\t.truncate = trunc_none,\n+\t\t.padding = 0,\n+\t};\n \n \tswitch (*ch++) {\n \tcase '<':\n@@ -1171,8 +1176,8 @@ static size_t parse_padding_placeholder(const char *placeholder,\n \t\t\tif (width < 0)\n \t\t\t\treturn 0;\n \t\t}\n-\t\tp->padding = to_column ? -width : width;\n-\t\tp->flush_type = flush_type;\n+\t\tans.padding = to_column ? -width : width;\n+\t\tans.flush_type = flush_type;\n \n \t\tif (*end == ',') {\n \t\t\tstart = end + 1;\n@@ -1180,16 +1185,17 @@ static size_t parse_padding_placeholder(const char *placeholder,\n \t\t\tif (!end || end == start)\n \t\t\t\treturn 0;\n \t\t\tif (starts_with(start, \"trunc)\"))\n-\t\t\t\tp->truncate = trunc_right;\n+\t\t\t\tans.truncate = trunc_right;\n \t\t\telse if (starts_with(start, \"ltrunc)\"))\n-\t\t\t\tp->truncate = trunc_left;\n+\t\t\t\tans.truncate = trunc_left;\n \t\t\telse if (starts_with(start, \"mtrunc)\"))\n-\t\t\t\tp->truncate = trunc_middle;\n+\t\t\t\tans.truncate = trunc_middle;\n \t\t\telse\n \t\t\t\treturn 0;\n \t\t} else\n-\t\t\tp->truncate = trunc_none;\n+\t\t\tans.truncate = trunc_none;\n \n+\t\t*p = ans;\n \t\treturn end - placeholder + 1;\n \t}\n \treturn 0;\ndiff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh\nindex f81e42a84d..26987ecd77 100755\n--- a/t/t4205-log-pretty-formats.sh\n+++ b/t/t4205-log-pretty-formats.sh\n@@ -1130,6 +1130,12 @@ test_expect_success 'log --pretty with invalid padding format' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'semi-parseable padding format does not get semi-applied' '\n+\tgit log -1 --pretty=\"format:%<(4,trunc)%H%%<(10,bad)%H\" >expect &&\n+\tgit log -1 --pretty=\"format:%<(4,trunc)%H%<(10,bad)%H\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'log --pretty with magical wrapping directives' '\n \tcommit_id=$(git commit-tree HEAD^{tree} -m \"describe me\") &&\n \tgit tag describe-me $commit_id &&\n-- \n2.49.0.472.ge94155a9ec\n\n"},{"id":"514611","messageId":"e34ae37982e76179aee780c70b48aaaf959a307b.1742367347.git.martin.agren@gmail.com","threadId":"63162","inReplyTo":"cover.1742367347.git.martin.agren@gmail.com","subject":"[PATCH 5/8] pretty: after padding, reset padding info","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2025-03-19T07:23:38Z","receivedAt":"2025-03-19T07:25:20Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"After handling a padding directive (\"%<\" or \"%>\"), we leave the `struct\npadding_args` in a halfway state. We modify it a bit as we apply the\npadding/truncation so that by the time we're done, it can't be in quite\nas many states as when we started. Still, we don't fully restore it to\nits default, no-action state.\n\n\"%<\" and \"%>\" should only affect the next placeholder, but leaving a bit\nof state around doesn't make it obvious that we don't spill any of it\ninto our handling of later placeholders. The previous commit closed off\na way of populating only half the `struct padding_args`, thereby fixing\na bug that *also* relied on then having the other half contain this kind\nof lingering data.\n\nAfter that fix, I haven't figured out a way to provoke a bug using just\nthis here half of the issue. Still, after handling padding, let's drop\nall remnants of the previous \"%<\" or \"%>\".\n\nUnlike the bug fixed in the previous commit, this could have some\nrealistic chance of regressing something for someone if they've actually\nbeen using such state leftovers (knowingly or not). Still, it seems\nworthwhile to try to tighten this.\n\nThis change to pretty.c would have been sufficient to make the test\nadded in the previous commit pass. Belt and suspenders.\n\nSigned-off-by: Martin Ågren <martin.agren@gmail.com>\n---\n pretty.c                      | 2 ++\n t/t4205-log-pretty-formats.sh | 9 +++++++++\n 2 files changed, 11 insertions(+)\n\ndiff --git a/pretty.c b/pretty.c\nindex a4fa052f8b..f53e77ed86 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1893,6 +1893,8 @@ static size_t format_and_pad_commit(struct strbuf *sb, /* in UTF-8 */\n \t}\n \tstrbuf_release(&local_sb);\n \tc->pad.flush_type = no_flush;\n+\tc->pad.truncate = trunc_none;\n+\tc->pad.padding = 0;\n \treturn total_consumed;\n }\n \ndiff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh\nindex 26987ecd77..d34a7cec09 100755\n--- a/t/t4205-log-pretty-formats.sh\n+++ b/t/t4205-log-pretty-formats.sh\n@@ -1124,6 +1124,15 @@ test_expect_success 'log --pretty with space stealing' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'only the next placeholder gets truncated' '\n+\t{\n+\t\tgit log -1 --pretty=\"format:%<(4,trunc)%H\" &&\n+\t\tprintf \"$(git rev-parse HEAD)\"\n+\t} >expect &&\n+\tgit log -1 --pretty=\"format:%<(4,trunc)%H%H\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'log --pretty with invalid padding format' '\n \tprintf \"%s%%<(20\" \"$(git rev-parse HEAD)\" >expect &&\n \tgit log -1 --pretty=\"format:%H%<(20\" >actual &&\n-- \n2.49.0.472.ge94155a9ec\n\n"},{"id":"514612","messageId":"465c91155eb30197b5eac00d294dc6e7ea2dd310.1742367347.git.martin.agren@gmail.com","threadId":"63162","inReplyTo":"cover.1742367347.git.martin.agren@gmail.com","subject":"[PATCH 6/8] pretty: refactor parsing of line-wrapping \"%w\" placeholder","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2025-03-19T07:23:39Z","receivedAt":"2025-03-19T07:25:23Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Our parsing of a \"%w\" placeholder is quite a big chunk of code in\nthe middle of our switch for handling a few different placeholders. We\nparse into three different variables, then use them to compare to and\nupdate existing values in the big `struct format_commit_context`.\n\nPull out a helper function for parsing such a \"%w\" placeholder. Define a\nstruct for collecting the three variables.\n\nUnlike recent commits, parsing and subsequent use are already a bit more\nseparated in the sense that we don't parse directly into the big context\nstruct. Thus, unlike the preceding commits, this does not fix any bugs\nthat I'm aware of. There's still value in separating parsing and usage\nmore clearly and simplifying `format_commit_one()`.\n\nNote that we use two different types for these values, `unsigned long`\nwhen parsing, `size_t` when eventually applying. Let's go for `size_t`\nin our struct. I don't know if there are platforms where assigning an\n`unsigned long` to a `size_t` could truncate the value, but since we\nalready verify the values to be at most 16 KiB, we should be able to fit\nthem into any sane `size_t`s.\n\nSigned-off-by: Martin Ågren <martin.agren@gmail.com>\n---\n pretty.c | 120 +++++++++++++++++++++++++++++++++++--------------------\n 1 file changed, 76 insertions(+), 44 deletions(-)\n\ndiff --git a/pretty.c b/pretty.c\nindex f53e77ed86..c44ff87481 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -893,6 +893,10 @@ struct padding_args {\n \tint padding;\n };\n \n+struct rewrap_args {\n+\tsize_t width, indent1, indent2;\n+};\n+\n struct format_commit_context {\n \tstruct repository *repository;\n \tconst struct commit *commit;\n@@ -902,7 +906,7 @@ struct format_commit_context {\n \tstruct signature_check signature_check;\n \tconst char *message;\n \tchar *commit_encoding;\n-\tsize_t width, indent1, indent2;\n+\tstruct rewrap_args rewrap;\n \tint auto_color;\n \tstruct padding_args pad;\n \n@@ -1034,18 +1038,21 @@ static void strbuf_wrap(struct strbuf *sb, size_t pos,\n \n static void rewrap_message_tail(struct strbuf *sb,\n \t\t\t\tstruct format_commit_context *c,\n-\t\t\t\tsize_t new_width, size_t new_indent1,\n-\t\t\t\tsize_t new_indent2)\n+\t\t\t\tconst struct rewrap_args *new_rewrap)\n {\n-\tif (c->width == new_width && c->indent1 == new_indent1 &&\n-\t    c->indent2 == new_indent2)\n+\tconst struct rewrap_args *old_rewrap = &c->rewrap;\n+\n+\tif (old_rewrap->width == new_rewrap->width &&\n+\t    old_rewrap->indent1 == new_rewrap->indent1 &&\n+\t    old_rewrap->indent2 == new_rewrap->indent2)\n \t\treturn;\n+\n \tif (c->wrap_start < sb->len)\n-\t\tstrbuf_wrap(sb, c->wrap_start, c->width, c->indent1, c->indent2);\n+\t\tstrbuf_wrap(sb, c->wrap_start, old_rewrap->width,\n+\t\t\t    old_rewrap->indent1, old_rewrap->indent2);\n+\n \tc->wrap_start = sb->len;\n-\tc->width = new_width;\n-\tc->indent1 = new_indent1;\n-\tc->indent2 = new_indent2;\n+\tc->rewrap = *new_rewrap;\n }\n \n static int format_reflog_person(struct strbuf *sb,\n@@ -1443,6 +1450,57 @@ static void free_decoration_options(const struct decoration_options *opts)\n \tfree(opts->tag);\n }\n \n+static size_t parse_rewrap(const char *placeholder, struct rewrap_args *rewrap)\n+{\n+\tunsigned long width = 0, indent1 = 0, indent2 = 0;\n+\tchar *next;\n+\tconst char *start;\n+\tconst char *end;\n+\n+\tmemset(rewrap, 0, sizeof(*rewrap));\n+\n+\tif (placeholder[1] != '(')\n+\t\treturn 0;\n+\n+\tstart = placeholder + 2;\n+\tend = strchr(start, ')');\n+\n+\tif (!end)\n+\t\treturn 0;\n+\tif (end > start) {\n+\t\twidth = strtoul(start, &next, 10);\n+\t\tif (*next == ',') {\n+\t\t\tindent1 = strtoul(next + 1, &next, 10);\n+\t\t\tif (*next == ',') {\n+\t\t\t\tindent2 = strtoul(next + 1,\n+\t\t\t\t\t\t &next, 10);\n+\t\t\t}\n+\t\t}\n+\t\tif (*next != ')')\n+\t\t\treturn 0;\n+\t}\n+\n+\t/*\n+\t * We need to limit the format here as it allows the\n+\t * user to prepend arbitrarily many bytes to the buffer\n+\t * when rewrapping.\n+\t */\n+\tif (width > FORMATTING_LIMIT ||\n+\t    indent1 > FORMATTING_LIMIT ||\n+\t    indent2 > FORMATTING_LIMIT)\n+\t\treturn 0;\n+\n+\t/*\n+\t * These values are small enough to fit in any\n+\t * real-world size_t.\n+\t */\n+\trewrap->width = width;\n+\trewrap->indent1 = indent1;\n+\trewrap->indent2 = indent2;\n+\n+\treturn end - placeholder + 1;\n+}\n+\n static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n \t\t\t\tconst char *placeholder,\n \t\t\t\tstruct format_commit_context *c)\n@@ -1478,40 +1536,13 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n \t\t\treturn ret;\n \t\t}\n \tcase 'w':\n-\t\tif (placeholder[1] == '(') {\n-\t\t\tunsigned long width = 0, indent1 = 0, indent2 = 0;\n-\t\t\tchar *next;\n-\t\t\tconst char *start = placeholder + 2;\n-\t\t\tconst char *end = strchr(start, ')');\n-\t\t\tif (!end)\n-\t\t\t\treturn 0;\n-\t\t\tif (end > start) {\n-\t\t\t\twidth = strtoul(start, &next, 10);\n-\t\t\t\tif (*next == ',') {\n-\t\t\t\t\tindent1 = strtoul(next + 1, &next, 10);\n-\t\t\t\t\tif (*next == ',') {\n-\t\t\t\t\t\tindent2 = strtoul(next + 1,\n-\t\t\t\t\t\t\t\t &next, 10);\n-\t\t\t\t\t}\n-\t\t\t\t}\n-\t\t\t\tif (*next != ')')\n-\t\t\t\t\treturn 0;\n-\t\t\t}\n-\n-\t\t\t/*\n-\t\t\t * We need to limit the format here as it allows the\n-\t\t\t * user to prepend arbitrarily many bytes to the buffer\n-\t\t\t * when rewrapping.\n-\t\t\t */\n-\t\t\tif (width > FORMATTING_LIMIT ||\n-\t\t\t    indent1 > FORMATTING_LIMIT ||\n-\t\t\t    indent2 > FORMATTING_LIMIT)\n-\t\t\t\treturn 0;\n-\t\t\trewrap_message_tail(sb, c, width, indent1, indent2);\n-\t\t\treturn end - placeholder + 1;\n-\t\t} else\n-\t\t\treturn 0;\n-\n+\t\t{\n+\t\t\tstruct rewrap_args rewrap;\n+\t\t\tres = parse_rewrap(placeholder, &rewrap);\n+\t\t\tif (res)\n+\t\t\t\trewrap_message_tail(sb, c, &rewrap);\n+\t\t\treturn res;\n+\t\t}\n \tcase '<':\n \tcase '>':\n \t\treturn parse_padding_placeholder(placeholder, &c->pad);\n@@ -2005,6 +2036,7 @@ void repo_format_commit_message(struct repository *r,\n \t};\n \tconst char *output_enc = pretty_ctx->output_encoding;\n \tconst char *utf8 = \"UTF-8\";\n+\tconst struct rewrap_args rewrap_reset = { 0 };\n \n \twhile (strbuf_expand_step(sb, &format)) {\n \t\tsize_t len;\n@@ -2016,7 +2048,7 @@ void repo_format_commit_message(struct repository *r,\n \t\telse\n \t\t\tstrbuf_addch(sb, '%');\n \t}\n-\trewrap_message_tail(sb, &context, 0, 0, 0);\n+\trewrap_message_tail(sb, &context, &rewrap_reset);\n \n \t/*\n \t * Convert output to an actual output encoding; note that\n-- \n2.49.0.472.ge94155a9ec\n\n"},{"id":"514613","messageId":"7c96899bb520ab945a650205982f54d65461d5bd.1742367347.git.martin.agren@gmail.com","threadId":"63162","inReplyTo":"cover.1742367347.git.martin.agren@gmail.com","subject":"[PATCH 7/8] pretty: refactor parsing of magic","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2025-03-19T07:23:40Z","receivedAt":"2025-03-19T07:25:26Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Similar to the previous commit, pull out our parsing of initial\nplaceholder magic into a separate function. This helps make it a bit\neasier to get an overview of `format_commit_item()`. It also represents\nanother small step towards separating the parsing of placeholders from\nsubsequent usage of the parsed information.\n\nThis diff might be a bit easier to read with `-w`.\n\nSigned-off-by: Martin Ågren <martin.agren@gmail.com>\n---\n pretty.c | 69 ++++++++++++++++++++++++++++++++++----------------------\n 1 file changed, 42 insertions(+), 27 deletions(-)\n\ndiff --git a/pretty.c b/pretty.c\nindex c44ff87481..ddc7fd6aab 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1929,17 +1929,17 @@ static size_t format_and_pad_commit(struct strbuf *sb, /* in UTF-8 */\n \treturn total_consumed;\n }\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 struct format_commit_context *context)\n+enum magic {\n+\tNO_MAGIC,\n+\tADD_LF_BEFORE_NON_EMPTY,\n+\tDEL_LF_BEFORE_EMPTY,\n+\tADD_SP_BEFORE_NON_EMPTY\n+};\n+\n+/* 2 for 'bad magic', otherwise whether we consumed 0 or 1 chars. */\n+static size_t parse_magic(const char *placeholder, enum magic *ret)\n {\n-\tsize_t consumed, orig_len;\n-\tenum {\n-\t\tNO_MAGIC,\n-\t\tADD_LF_BEFORE_NON_EMPTY,\n-\t\tDEL_LF_BEFORE_EMPTY,\n-\t\tADD_SP_BEFORE_NON_EMPTY\n-\t} magic = NO_MAGIC;\n+\tenum magic magic;\n \n \tswitch (placeholder[0]) {\n \tcase '-':\n@@ -1952,28 +1952,43 @@ static size_t format_commit_item(struct strbuf *sb, /* in UTF-8 */\n \t\tmagic = ADD_SP_BEFORE_NON_EMPTY;\n \t\tbreak;\n \tdefault:\n-\t\tbreak;\n+\t\t*ret = NO_MAGIC;\n+\t\treturn 0;\n \t}\n-\tif (magic != NO_MAGIC) {\n-\t\tplaceholder++;\n \n-\t\tswitch (placeholder[0]) {\n-\t\tcase 'w':\n-\t\t\t/*\n-\t\t\t * `%+w()` cannot ever expand to a non-empty string,\n-\t\t\t * and it potentially changes the layout of preceding\n-\t\t\t * contents. We're thus not able to handle the magic in\n-\t\t\t * this combination and refuse the pattern.\n-\t\t\t */\n-\t\t\treturn 0;\n-\t\t};\n-\t}\n+\tswitch (placeholder[1]) {\n+\tcase 'w':\n+\t\t/*\n+\t\t * `%+w()` cannot ever expand to a non-empty string,\n+\t\t * and it potentially changes the layout of preceding\n+\t\t * contents. We're thus not able to handle the magic in\n+\t\t * this combination and refuse the pattern.\n+\t\t */\n+\t\t*ret = NO_MAGIC;\n+\t\treturn 2;\n+\t};\n+\n+\t*ret = magic;\n+\treturn 1;\n+}\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 struct format_commit_context *context)\n+{\n+\tsize_t consumed, orig_len;\n+\tenum magic magic;\n+\n+\tconsumed = parse_magic(placeholder, &magic);\n+\tif (consumed > 1)\n+\t\treturn 0;\n+\tplaceholder += consumed;\n \n \torig_len = sb->len;\n \tif (context->pad.flush_type == no_flush)\n-\t\tconsumed = format_commit_one(sb, placeholder, context);\n+\t\tconsumed += format_commit_one(sb, placeholder, context);\n \telse\n-\t\tconsumed = format_and_pad_commit(sb, placeholder, context);\n+\t\tconsumed += format_and_pad_commit(sb, placeholder, context);\n \tif (magic == NO_MAGIC)\n \t\treturn consumed;\n \n@@ -1986,7 +2001,7 @@ static size_t format_commit_item(struct strbuf *sb, /* in UTF-8 */\n \t\telse if (magic == ADD_SP_BEFORE_NON_EMPTY)\n \t\t\tstrbuf_insertstr(sb, orig_len, \" \");\n \t}\n-\treturn consumed + 1;\n+\treturn consumed;\n }\n \n void userformat_find_requirements(const char *fmt, struct userformat_want *w)\n-- \n2.49.0.472.ge94155a9ec\n\n"},{"id":"514614","messageId":"f4d0d5c00ab7d314d19d82335d7381959ee6fb41.1742367347.git.martin.agren@gmail.com","threadId":"63162","inReplyTo":"cover.1742367347.git.martin.agren@gmail.com","subject":"[PATCH 8/8] pretty: refactor parsing of decoration options","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2025-03-19T07:23:41Z","receivedAt":"2025-03-19T07:25:27Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"After having spotted \"%(decorate\", we see if there's a ':' and, if so,\nreach out to `parse_decoration_options()`. We then verify there's a\nclosing ')' before actually considering the placeholder valid. Pull the\nhandling of ':' and ')' into `parse_decoration_options()` so that it's\nmore of a one-stop shop for handling everything after \"%(decorate\". Let\nthis include freeing up resources in the error path to make it really\neasy to use this function.\n\nSigned-off-by: Martin Ågren <martin.agren@gmail.com>\n---\n pretty.c | 52 ++++++++++++++++++++++++++++++----------------------\n 1 file changed, 30 insertions(+), 22 deletions(-)\n\ndiff --git a/pretty.c b/pretty.c\nindex ddc7fd6aab..d5a8ceb7ef 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1430,17 +1430,6 @@ static int parse_decoration_option(const char **arg,\n \treturn 0;\n }\n \n-static void parse_decoration_options(const char **arg,\n-\t\t\t\t     struct decoration_options *opts)\n-{\n-\twhile (parse_decoration_option(arg, \"prefix\", &opts->prefix) ||\n-\t       parse_decoration_option(arg, \"suffix\", &opts->suffix) ||\n-\t       parse_decoration_option(arg, \"separator\", &opts->separator) ||\n-\t       parse_decoration_option(arg, \"pointer\", &opts->pointer) ||\n-\t       parse_decoration_option(arg, \"tag\", &opts->tag))\n-\t\t;\n-}\n-\n static void free_decoration_options(const struct decoration_options *opts)\n {\n \tfree(opts->prefix);\n@@ -1450,6 +1439,30 @@ static void free_decoration_options(const struct decoration_options *opts)\n \tfree(opts->tag);\n }\n \n+static int parse_decoration_options(const char **arg,\n+\t\t\t\t    struct decoration_options *opts)\n+{\n+\tmemset(opts, 0, sizeof(*opts));\n+\n+\tif (**arg == ':') {\n+\t\t(*arg)++;\n+\t\twhile (parse_decoration_option(arg, \"prefix\", &opts->prefix) ||\n+\t\t       parse_decoration_option(arg, \"suffix\", &opts->suffix) ||\n+\t\t       parse_decoration_option(arg, \"separator\", &opts->separator) ||\n+\t\t       parse_decoration_option(arg, \"pointer\", &opts->pointer) ||\n+\t\t       parse_decoration_option(arg, \"tag\", &opts->tag))\n+\t\t\t;\n+\t}\n+\n+\tif (**arg != ')') {\n+\t\tfree_decoration_options(opts);\n+\t\treturn -1;\n+\t}\n+\t(*arg)++;\n+\n+\treturn 0;\n+}\n+\n static size_t parse_rewrap(const char *placeholder, struct rewrap_args *rewrap)\n {\n \tunsigned long width = 0, indent1 = 0, indent2 = 0;\n@@ -1735,20 +1748,15 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n \t}\n \n \tif (skip_prefix(placeholder, \"(decorate\", &arg)) {\n-\t\tstruct decoration_options opts = { NULL };\n-\t\tsize_t ret = 0;\n+\t\tstruct decoration_options opts;\n \n-\t\tif (*arg == ':') {\n-\t\t\targ++;\n-\t\t\tparse_decoration_options(&arg, &opts);\n-\t\t}\n-\t\tif (*arg == ')') {\n-\t\t\tformat_decorations(sb, commit, c->auto_color, &opts);\n-\t\t\tret = arg - placeholder + 1;\n-\t\t}\n+\t\tif (parse_decoration_options(&arg, &opts) < 0)\n+\t\t\treturn 0;\n+\n+\t\tformat_decorations(sb, commit, c->auto_color, &opts);\n \n \t\tfree_decoration_options(&opts);\n-\t\treturn ret;\n+\t\treturn arg - placeholder;\n \t}\n \n \t/* For the rest we have to parse the commit header. */\n-- \n2.49.0.472.ge94155a9ec\n\n"},{"id":"514724","messageId":"Z9vdQPtmUiuobOP6@pks.im","threadId":"63162","inReplyTo":"5f787ddac2d80391feadb8cf6be379fc8e58652f.1742367347.git.martin.agren@gmail.com","subject":"Re: [PATCH 2/8] pretty: simplify if-else to reduce code duplication","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-03-20T09:17:52Z","receivedAt":"2025-03-20T09:18:00Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Mar 19, 2025 at 08:23:35AM +0100, Martin Ågren wrote:\n> First we look for \"auto,\", then we try \"always,\", then we fall back to\n\nNit: we typically have the body carry enough context so that it makes\nsense even without reading the commit subject.\n\n> the default, which is to do exactly the same thing as we do for \"auto,\".\n> The amount of code duplication isn't huge, but still: reading this code\n> carefully requires spending at least *some* time on making sure the two\n> blocks of code are indeed identical.\n> \n> Rearrange the checks so that we end with the default case,\n> opportunistically consuming the \"auto,\" which may or may not be there.\n> \n> In the \"always,\" case, we don't actually *do* anything, so if we were\n> into golfing, we'd just write the whole thing as a single\n> \n>   if (!skip_prefix(begin, \"always,\", &begin)) {\n>     ...\n>   }\n> \n> If we ever learn something new besides \"always,\" and \"auto,\" we'd need\n> to pull things apart again. Plus we still need somewhere to place the\n> comment. Let's focus on code de-duplication rather than golfing for now.\n> \n> Signed-off-by: Martin Ågren <martin.agren@gmail.com>\n> ---\n>  pretty.c | 6 ++----\n>  1 file changed, 2 insertions(+), 4 deletions(-)\n> \n> diff --git a/pretty.c b/pretty.c\n> index a4e5fc5c50..6a4264dd01 100644\n> --- a/pretty.c\n> +++ b/pretty.c\n> @@ -1076,13 +1076,11 @@ static size_t parse_color(struct strbuf *sb, /* in UTF-8 */\n>  \t\tif (!end)\n>  \t\t\treturn 0;\n>  \n> -\t\tif (skip_prefix(begin, \"auto,\", &begin)) {\n> -\t\t\tif (!want_color(c->pretty_ctx->color))\n> -\t\t\t\treturn end - placeholder + 1;\n> -\t\t} else if (skip_prefix(begin, \"always,\", &begin)) {\n> +\t\tif (skip_prefix(begin, \"always,\", &begin)) {\n>  \t\t\t/* nothing to do; we do not respect want_color at all */\n>  \t\t} else {\n>  \t\t\t/* the default is the same as \"auto\" */\n> +\t\t\tskip_prefix(begin, \"auto,\", &begin);\n>  \t\t\tif (!want_color(c->pretty_ctx->color))\n>  \t\t\t\treturn end - placeholder + 1;\n>  \t\t}\n\nOkay, this change should lead to the same results as before indeed. As\nyou mention it does require us to be more careful if we ever were to\nintroduce another option here. But I still think it's fine to simplify\nthe code like this.\n\nPatrick\n"},{"id":"514725","messageId":"Z9vdSBDfB0MHP-iD@pks.im","threadId":"63162","inReplyTo":"192fc78dd869f28cb6ae91f3a26a05eb6b6a4bbf.1742367347.git.martin.agren@gmail.com","subject":"Re: [PATCH 1/8] pretty: tighten function signature to not take `void *`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-03-20T09:18:00Z","receivedAt":"2025-03-20T09:18:03Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Mar 19, 2025 at 08:23:34AM +0100, Martin Ågren wrote:\n> We take a `void *` and immediately cast it. Both callers already have\n> this pointer as the right type, so tighten the interface and stop\n> casting.\n> \n> Signed-off-by: Martin Ågren <martin.agren@gmail.com>\n> ---\n>  pretty.c | 3 +--\n>  1 file changed, 1 insertion(+), 2 deletions(-)\n> \n> diff --git a/pretty.c b/pretty.c\n> index 0bc8ad8a9a..a4e5fc5c50 100644\n> --- a/pretty.c\n> +++ b/pretty.c\n> @@ -1437,9 +1437,8 @@ static void free_decoration_options(const struct decoration_options *opts)\n>  \n>  static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n>  \t\t\t\tconst char *placeholder,\n> -\t\t\t\tvoid *context)\n> +\t\t\t\tstruct format_commit_context *c)\n>  {\n> -\tstruct format_commit_context *c = context;\n>  \tconst struct commit *commit = c->commit;\n>  \tconst char *msg = c->message;\n>  \tstruct commit_list *p;\n\nMakes sense. The function has been introduced all the way back in\n9fa708dab1c (Pretty-format: %[+-]x to tweak inter-item newlines,\n2009-10-04), and at that point in time the callers only had `void *`\ncontexts available. That has changed eventually, so I agree that it is\nnice to adapt accordingly now.\n\nPatrick\n"},{"id":"514726","messageId":"Z9vdS4bxY6spILsc@pks.im","threadId":"63162","inReplyTo":"7d6b62006ecaf7db159e8db0c85455ed58027ce6.1742367347.git.martin.agren@gmail.com","subject":"Re: [PATCH 4/8] pretty: fix parsing of half-valid \"%<\" and \"%>\" placeholders","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-03-20T09:18:03Z","receivedAt":"2025-03-20T09:18:06Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Mar 19, 2025 at 08:23:37AM +0100, Martin Ågren wrote:\n> When we parse a padding directive (\"%<\" or \"%>\"), we might populate a\n> few of the struct's fields before bailing. This can result in such\n> half-parsed information being used to actually introduce some\n> padding/truncation.\n> \n> When parsing a \"%<\" or \"%>\", only store the parsed data after parsing\n> successfully. The added test would have failed before this commit. It\n> also shows how the existing behavior is hardly something someone can\n> rely on since the non-consumed modifier (\"%<(10,bad)\") shows up verbatim\n> in the pretty output.\n\nIdeally I'd expect us to die when seeing misformatted placeholders like\nthis. This is way less confusing to the user as otherwise things _look_\nlike they work, but we silently do the wrong thing.\n\nThat being said, I have no idea whether we can do such a change now\nwithout breaking existing usecases. As you rightfully argue the result\nalready is wrong, but with my proposal we'd completely refuse to do\nanything. Which I'd argue is a good thing in the end.\n\n> We could let the caller use a temporary struct and only copy the data on\n> success. Let's instead make our parsing function easy to use correctly\n> by letting it only touch the output struct in the success case.\n\ns/success/&ful/\n\n> While setting up a temporary struct for parsing into, we might as well\n> initialize it to a well-defined state. It's unnecessary for the current\n> implementation since it always writes to all three fields in a\n> successful case, but some future-proofing shouldn't hurt.\n> \n> Note that the test relies on first using a correct placeholder\n> \"%<(4,trunc)\" where \"trunc\" (`trunc_right`) lingers in our struct until\n> it's then used instead of the invalid \"bad\". The next commit will teach\n> us to clean up any remnants of \"%<(4,trunc)\" after handling it.\n> \n> Signed-off-by: Martin Ågren <martin.agren@gmail.com>\n> ---\n>  pretty.c                      | 18 ++++++++++++------\n>  t/t4205-log-pretty-formats.sh |  6 ++++++\n>  2 files changed, 18 insertions(+), 6 deletions(-)\n> \n> diff --git a/pretty.c b/pretty.c\n> index e5e8ef24fa..a4fa052f8b 100644\n> --- a/pretty.c\n> +++ b/pretty.c\n> @@ -1121,6 +1121,11 @@ static size_t parse_padding_placeholder(const char *placeholder,\n>  \tconst char *ch = placeholder;\n>  \tenum flush_type flush_type;\n>  \tint to_column = 0;\n> +\tstruct padding_args ans = {\n> +\t\t.flush_type = no_flush,\n> +\t\t.truncate = trunc_none,\n> +\t\t.padding = 0,\n> +\t};\n>  \n>  \tswitch (*ch++) {\n>  \tcase '<':\n\nI honestly have no idea what `ans` stands for. You could call it\n`result` to signify that it's what we'll ultimately bubble up to the\ncaller in the successful case.\n\nPatrick\n"},{"id":"514727","messageId":"Z9vdTmVbIJLa9PGO@pks.im","threadId":"63162","inReplyTo":"e34ae37982e76179aee780c70b48aaaf959a307b.1742367347.git.martin.agren@gmail.com","subject":"Re: [PATCH 5/8] pretty: after padding, reset padding info","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-03-20T09:18:06Z","receivedAt":"2025-03-20T09:18:09Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Mar 19, 2025 at 08:23:38AM +0100, Martin Ågren wrote:\n> After handling a padding directive (\"%<\" or \"%>\"), we leave the `struct\n> padding_args` in a halfway state. We modify it a bit as we apply the\n> padding/truncation so that by the time we're done, it can't be in quite\n> as many states as when we started. Still, we don't fully restore it to\n> its default, no-action state.\n> \n> \"%<\" and \"%>\" should only affect the next placeholder, but leaving a bit\n> of state around doesn't make it obvious that we don't spill any of it\n> into our handling of later placeholders. The previous commit closed off\n> a way of populating only half the `struct padding_args`, thereby fixing\n> a bug that *also* relied on then having the other half contain this kind\n> of lingering data.\n> \n> After that fix, I haven't figured out a way to provoke a bug using just\n> this here half of the issue. Still, after handling padding, let's drop\n> all remnants of the previous \"%<\" or \"%>\".\n> \n> Unlike the bug fixed in the previous commit, this could have some\n> realistic chance of regressing something for someone if they've actually\n> been using such state leftovers (knowingly or not). Still, it seems\n> worthwhile to try to tighten this.\n\nYeah, I agree. It's very surprising that we retain only a subset of\nstate, and that does feel like a bug to me.\n\n> This change to pretty.c would have been sufficient to make the test\n> added in the previous commit pass. Belt and suspenders.\n> \n> Signed-off-by: Martin Ågren <martin.agren@gmail.com>\n> ---\n>  pretty.c                      | 2 ++\n>  t/t4205-log-pretty-formats.sh | 9 +++++++++\n>  2 files changed, 11 insertions(+)\n> \n> diff --git a/pretty.c b/pretty.c\n> index a4fa052f8b..f53e77ed86 100644\n> --- a/pretty.c\n> +++ b/pretty.c\n> @@ -1893,6 +1893,8 @@ static size_t format_and_pad_commit(struct strbuf *sb, /* in UTF-8 */\n>  \t}\n>  \tstrbuf_release(&local_sb);\n>  \tc->pad.flush_type = no_flush;\n> +\tc->pad.truncate = trunc_none;\n> +\tc->pad.padding = 0;\n>  \treturn total_consumed;\n>  }\n\nThis is using the same default values now as you started to use in the\npreceding commit. It might make sense to introduce a macro or function\nto initialize the structure so that we don't duplicate initialization.\n\nPatrick\n"},{"id":"514728","messageId":"Z9vdUTQTnctm3965@pks.im","threadId":"63162","inReplyTo":"465c91155eb30197b5eac00d294dc6e7ea2dd310.1742367347.git.martin.agren@gmail.com","subject":"Re: [PATCH 6/8] pretty: refactor parsing of line-wrapping \"%w\" placeholder","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-03-20T09:18:09Z","receivedAt":"2025-03-20T09:18:12Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Mar 19, 2025 at 08:23:39AM +0100, Martin Ågren wrote:\n> Our parsing of a \"%w\" placeholder is quite a big chunk of code in\n> the middle of our switch for handling a few different placeholders. We\n> parse into three different variables, then use them to compare to and\n> update existing values in the big `struct format_commit_context`.\n> \n> Pull out a helper function for parsing such a \"%w\" placeholder. Define a\n> struct for collecting the three variables.\n> \n> Unlike recent commits, parsing and subsequent use are already a bit more\n> separated in the sense that we don't parse directly into the big context\n> struct. Thus, unlike the preceding commits, this does not fix any bugs\n> that I'm aware of. There's still value in separating parsing and usage\n> more clearly and simplifying `format_commit_one()`.\n> \n> Note that we use two different types for these values, `unsigned long`\n> when parsing, `size_t` when eventually applying. Let's go for `size_t`\n> in our struct. I don't know if there are platforms where assigning an\n> `unsigned long` to a `size_t` could truncate the value, but since we\n> already verify the values to be at most 16 KiB, we should be able to fit\n> them into any sane `size_t`s.\n> \n> Signed-off-by: Martin Ågren <martin.agren@gmail.com>\n> ---\n>  pretty.c | 120 +++++++++++++++++++++++++++++++++++--------------------\n>  1 file changed, 76 insertions(+), 44 deletions(-)\n> \n> diff --git a/pretty.c b/pretty.c\n> index f53e77ed86..c44ff87481 100644\n> --- a/pretty.c\n> +++ b/pretty.c\n> @@ -893,6 +893,10 @@ struct padding_args {\n>  \tint padding;\n>  };\n>  \n> +struct rewrap_args {\n> +\tsize_t width, indent1, indent2;\n> +};\n> +\n>  struct format_commit_context {\n>  \tstruct repository *repository;\n>  \tconst struct commit *commit;\n> @@ -902,7 +906,7 @@ struct format_commit_context {\n>  \tstruct signature_check signature_check;\n>  \tconst char *message;\n>  \tchar *commit_encoding;\n> -\tsize_t width, indent1, indent2;\n> +\tstruct rewrap_args rewrap;\n>  \tint auto_color;\n>  \tstruct padding_args pad;\n>  \n> @@ -1034,18 +1038,21 @@ static void strbuf_wrap(struct strbuf *sb, size_t pos,\n>  \n>  static void rewrap_message_tail(struct strbuf *sb,\n>  \t\t\t\tstruct format_commit_context *c,\n> -\t\t\t\tsize_t new_width, size_t new_indent1,\n> -\t\t\t\tsize_t new_indent2)\n> +\t\t\t\tconst struct rewrap_args *new_rewrap)\n>  {\n> -\tif (c->width == new_width && c->indent1 == new_indent1 &&\n> -\t    c->indent2 == new_indent2)\n> +\tconst struct rewrap_args *old_rewrap = &c->rewrap;\n> +\n> +\tif (old_rewrap->width == new_rewrap->width &&\n> +\t    old_rewrap->indent1 == new_rewrap->indent1 &&\n> +\t    old_rewrap->indent2 == new_rewrap->indent2)\n>  \t\treturn;\n> +\n>  \tif (c->wrap_start < sb->len)\n> -\t\tstrbuf_wrap(sb, c->wrap_start, c->width, c->indent1, c->indent2);\n> +\t\tstrbuf_wrap(sb, c->wrap_start, old_rewrap->width,\n> +\t\t\t    old_rewrap->indent1, old_rewrap->indent2);\n> +\n>  \tc->wrap_start = sb->len;\n> -\tc->width = new_width;\n> -\tc->indent1 = new_indent1;\n> -\tc->indent2 = new_indent2;\n> +\tc->rewrap = *new_rewrap;\n>  }\n>  \n>  static int format_reflog_person(struct strbuf *sb,\n> @@ -1443,6 +1450,57 @@ static void free_decoration_options(const struct decoration_options *opts)\n>  \tfree(opts->tag);\n>  }\n>  \n> +static size_t parse_rewrap(const char *placeholder, struct rewrap_args *rewrap)\n> +{\n> +\tunsigned long width = 0, indent1 = 0, indent2 = 0;\n> +\tchar *next;\n> +\tconst char *start;\n> +\tconst char *end;\n> +\n> +\tmemset(rewrap, 0, sizeof(*rewrap));\n\nThe `memset()` feels rather unnecessary as we only use the result at our\nsingle caller in case we return successfully. And if we do, we know to\ninitialize all struct fields.\n\n> +\tif (placeholder[1] != '(')\n> +\t\treturn 0;\n\nThis matches the `else` branch. It's nice that it's converted into an\nearly return.\n\n> +\tstart = placeholder + 2;\n> +\tend = strchr(start, ')');\n> +\n> +\tif (!end)\n> +\t\treturn 0;\n> +\tif (end > start) {\n> +\t\twidth = strtoul(start, &next, 10);\n> +\t\tif (*next == ',') {\n> +\t\t\tindent1 = strtoul(next + 1, &next, 10);\n> +\t\t\tif (*next == ',') {\n> +\t\t\t\tindent2 = strtoul(next + 1,\n> +\t\t\t\t\t\t &next, 10);\n> +\t\t\t}\n> +\t\t}\n> +\t\tif (*next != ')')\n> +\t\t\treturn 0;\n> +\t}\n> +\n> +\t/*\n> +\t * We need to limit the format here as it allows the\n> +\t * user to prepend arbitrarily many bytes to the buffer\n> +\t * when rewrapping.\n> +\t */\n> +\tif (width > FORMATTING_LIMIT ||\n> +\t    indent1 > FORMATTING_LIMIT ||\n> +\t    indent2 > FORMATTING_LIMIT)\n> +\t\treturn 0;\n\nAnd all of the above matches the `if` branch, except that we don't\nperform the rewrap itself.\n\nLooks good.\n\nPatrick\n"},{"id":"514729","messageId":"Z9vdVP4edeaRawsz@pks.im","threadId":"63162","inReplyTo":"7c96899bb520ab945a650205982f54d65461d5bd.1742367347.git.martin.agren@gmail.com","subject":"Re: [PATCH 7/8] pretty: refactor parsing of magic","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-03-20T09:18:12Z","receivedAt":"2025-03-20T09:18:15Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Mar 19, 2025 at 08:23:40AM +0100, Martin Ågren wrote:\n> Similar to the previous commit, pull out our parsing of initial\n> placeholder magic into a separate function. This helps make it a bit\n> easier to get an overview of `format_commit_item()`. It also represents\n> another small step towards separating the parsing of placeholders from\n> subsequent usage of the parsed information.\n> \n> This diff might be a bit easier to read with `-w`.\n> \n> Signed-off-by: Martin Ågren <martin.agren@gmail.com>\n> ---\n>  pretty.c | 69 ++++++++++++++++++++++++++++++++++----------------------\n>  1 file changed, 42 insertions(+), 27 deletions(-)\n> \n> diff --git a/pretty.c b/pretty.c\n> index c44ff87481..ddc7fd6aab 100644\n> --- a/pretty.c\n> +++ b/pretty.c\n> @@ -1929,17 +1929,17 @@ static size_t format_and_pad_commit(struct strbuf *sb, /* in UTF-8 */\n>  \treturn total_consumed;\n>  }\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 struct format_commit_context *context)\n> +enum magic {\n> +\tNO_MAGIC,\n> +\tADD_LF_BEFORE_NON_EMPTY,\n> +\tDEL_LF_BEFORE_EMPTY,\n> +\tADD_SP_BEFORE_NON_EMPTY\n> +};\n> +\n\nIt would be nice to give all of these enums a common prefix, e.g.:\n\n    enum magic {\n            MAGIC_NONE,\n            MAGIC_ADD_LF_BEFORE_NON_EMPTY,\n            MAGIC_DEL_LF_BEFORE_EMPTY,\n            MAGIC_ADD_SP_BEFORE_NON_EMPTY\n    };\n\nMakes it easier to see that things belong together and it provides\nproper namespacing.\n\n> +/* 2 for 'bad magic', otherwise whether we consumed 0 or 1 chars. */\n> +static size_t parse_magic(const char *placeholder, enum magic *ret)\n>  {\n> -\tsize_t consumed, orig_len;\n> -\tenum {\n> -\t\tNO_MAGIC,\n> -\t\tADD_LF_BEFORE_NON_EMPTY,\n> -\t\tDEL_LF_BEFORE_EMPTY,\n> -\t\tADD_SP_BEFORE_NON_EMPTY\n> -\t} magic = NO_MAGIC;\n> +\tenum magic magic;\n>  \n>  \tswitch (placeholder[0]) {\n>  \tcase '-':\n\nOn the other hand you simply retain existing names. I don't insist on\nthe refactoring, but still thing it would be nice as the enum has wider\nscope now.\n\n> @@ -1952,28 +1952,43 @@ static size_t format_commit_item(struct strbuf *sb, /* in UTF-8 */\n>  \t\tmagic = ADD_SP_BEFORE_NON_EMPTY;\n>  \t\tbreak;\n>  \tdefault:\n> -\t\tbreak;\n> +\t\t*ret = NO_MAGIC;\n> +\t\treturn 0;\n>  \t}\n> -\tif (magic != NO_MAGIC) {\n> -\t\tplaceholder++;\n>  \n> -\t\tswitch (placeholder[0]) {\n> -\t\tcase 'w':\n> -\t\t\t/*\n> -\t\t\t * `%+w()` cannot ever expand to a non-empty string,\n> -\t\t\t * and it potentially changes the layout of preceding\n> -\t\t\t * contents. We're thus not able to handle the magic in\n> -\t\t\t * this combination and refuse the pattern.\n> -\t\t\t */\n> -\t\t\treturn 0;\n> -\t\t};\n> -\t}\n> +\tswitch (placeholder[1]) {\n> +\tcase 'w':\n> +\t\t/*\n> +\t\t * `%+w()` cannot ever expand to a non-empty string,\n> +\t\t * and it potentially changes the layout of preceding\n> +\t\t * contents. We're thus not able to handle the magic in\n> +\t\t * this combination and refuse the pattern.\n> +\t\t */\n> +\t\t*ret = NO_MAGIC;\n> +\t\treturn 2;\n> +\t};\n> +\n> +\t*ret = magic;\n> +\treturn 1;\n> +}\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 struct format_commit_context *context)\n> +{\n> +\tsize_t consumed, orig_len;\n> +\tenum magic magic;\n> +\n> +\tconsumed = parse_magic(placeholder, &magic);\n> +\tif (consumed > 1)\n> +\t\treturn 0;\n> +\tplaceholder += consumed;\n>  \n>  \torig_len = sb->len;\n>  \tif (context->pad.flush_type == no_flush)\n> -\t\tconsumed = format_commit_one(sb, placeholder, context);\n> +\t\tconsumed += format_commit_one(sb, placeholder, context);\n>  \telse\n> -\t\tconsumed = format_and_pad_commit(sb, placeholder, context);\n> +\t\tconsumed += format_and_pad_commit(sb, placeholder, context);\n>  \tif (magic == NO_MAGIC)\n>  \t\treturn consumed;\n>  \n> @@ -1986,7 +2001,7 @@ static size_t format_commit_item(struct strbuf *sb, /* in UTF-8 */\n>  \t\telse if (magic == ADD_SP_BEFORE_NON_EMPTY)\n>  \t\t\tstrbuf_insertstr(sb, orig_len, \" \");\n>  \t}\n> -\treturn consumed + 1;\n> +\treturn consumed;\n\nIt took me a bit to figure out why this is equivalent to what we had\nbefore. But:\n\n  - If `parse_magic()` returns bigger than 1 we'd have exited early, so\n    this return here is never hit.\n\n  - If it returns `0` we have hit `NO_MAGIC`, and we have another early\n    return for this case.\n\nSo we only end up here in case `consumed = parse_magic(...)` is 1, and\nthen we add the result from `format_and_pad_commit()` to that value.\nWhich means that the refactoring is true to the original spirit.\n\nPatrick\n"},{"id":"514774","messageId":"CAN0heSpHZo=+ZwM5wJXQtFVD5jsMJGu8+KmRVcMDUT_ipgtMRw@mail.gmail.com","threadId":"63162","inReplyTo":"Z9vdQPtmUiuobOP6@pks.im","subject":"Re: [PATCH 2/8] pretty: simplify if-else to reduce code duplication","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2025-03-20T16:10:50Z","receivedAt":"2025-03-20T16:11:04Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Hi Patrick,\n\nThanks for reviewing!\n\nOn Thu, 20 Mar 2025 at 10:18, Patrick Steinhardt <ps@pks.im> wrote:\n>\n> On Wed, Mar 19, 2025 at 08:23:35AM +0100, Martin Ågren wrote:\n> > First we look for \"auto,\", then we try \"always,\", then we fall back to\n>\n> Nit: we typically have the body carry enough context so that it makes\n> sense even without reading the commit subject.\n\nThanks. I'll add some context: \"After spotting \"%C\", we first ...\"\n\n\nMartin\n"},{"id":"514775","messageId":"CAN0heSpN-k886+RsZ0+djLd974Mq57B4quZK1yKXRMxCnOvzZw@mail.gmail.com","threadId":"63162","inReplyTo":"Z9vdS4bxY6spILsc@pks.im","subject":"Re: [PATCH 4/8] pretty: fix parsing of half-valid \"%<\" and \"%>\" placeholders","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2025-03-20T16:11:09Z","receivedAt":"2025-03-20T16:11:23Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Thu, 20 Mar 2025 at 10:18, Patrick Steinhardt <ps@pks.im> wrote:\n>\n> On Wed, Mar 19, 2025 at 08:23:37AM +0100, Martin Ågren wrote:\n> > When parsing a \"%<\" or \"%>\", only store the parsed data after parsing\n> > successfully. The added test would have failed before this commit. It\n> > also shows how the existing behavior is hardly something someone can\n> > rely on since the non-consumed modifier (\"%<(10,bad)\") shows up verbatim\n> > in the pretty output.\n>\n> Ideally I'd expect us to die when seeing misformatted placeholders like\n> this. This is way less confusing to the user as otherwise things _look_\n> like they work, but we silently do the wrong thing.\n\nRight. I can see how it makes some kind of sense to print what we don't\nunderstand when it's something short and simple like \"%X\". But for more\ncomplex \"%X(first,second)\" it's kind of obvious that a misspelled\n\"X(fist,second)\" isn't something you want in the output. The whole \"if\nwe can't parse, return zero as the number of consumed characters so that\nwe can print verbatim while looking for next '%'\" is a central piece of\nthe design here. One could certainly imagine a \"strict\" mode.\n\n> That being said, I have no idea whether we can do such a change now\n> without breaking existing usecases. As you rightfully argue the result\n> already is wrong, but with my proposal we'd completely refuse to do\n> anything. Which I'd argue is a good thing in the end.\n\nI can see the value of a strict mode, with command line options and\nconfig switches and whatnot, maybe even a changed default behavior at\nsome point. I'd rather punt on that for now. TBH, I'd be afraid to do a\nhard switch from \"0 means print it instead\" to \"0 means die\". I don't\ndisagree that it would be a better end-game though, at some point.\n\n> > We could let the caller use a temporary struct and only copy the data on\n> > success. Let's instead make our parsing function easy to use correctly\n> > by letting it only touch the output struct in the success case.\n>\n> s/success/&ful/\n\nThanks.\n\n> > +     struct padding_args ans = {\n> > +             .flush_type = no_flush,\n> > +             .truncate = trunc_none,\n> > +             .padding = 0,\n> > +     };\n> >\n> >       switch (*ch++) {\n> >       case '<':\n>\n> I honestly have no idea what `ans` stands for. You could call it\n> `result` to signify that it's what we'll ultimately bubble up to the\n> caller in the successful case.\n\nFair. :-) It's \"answer\", but \"result\" is much better. Thanks.\n\n\nMartin\n"},{"id":"514776","messageId":"CAN0heSpsGUJojoiFSrhTksBt0ZYoz17ycR=91Rs44S_SWbDjQw@mail.gmail.com","threadId":"63162","inReplyTo":"Z9vdUTQTnctm3965@pks.im","subject":"Re: [PATCH 6/8] pretty: refactor parsing of line-wrapping \"%w\" placeholder","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2025-03-20T16:11:41Z","receivedAt":"2025-03-20T16:11:58Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Thu, 20 Mar 2025 at 10:18, Patrick Steinhardt <ps@pks.im> wrote:\n>\n> On Wed, Mar 19, 2025 at 08:23:39AM +0100, Martin Ågren wrote:\n\n> > +     memset(rewrap, 0, sizeof(*rewrap));\n>\n> The `memset()` feels rather unnecessary as we only use the result at our\n> single caller in case we return successfully. And if we do, we know to\n> initialize all struct fields.\n\nThanks, good point. I'll drop it.\n\n\nMartin\n"},{"id":"514777","messageId":"CAN0heSovtPNpyHEUC8m48zyJkRxGQNwp5u6x=WDMdd+aNJnwjw@mail.gmail.com","threadId":"63162","inReplyTo":"Z9vdTmVbIJLa9PGO@pks.im","subject":"Re: [PATCH 5/8] pretty: after padding, reset padding info","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2025-03-20T16:11:31Z","receivedAt":"2025-03-20T16:12:02Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Thu, 20 Mar 2025 at 10:18, Patrick Steinhardt <ps@pks.im> wrote:\n>\n> On Wed, Mar 19, 2025 at 08:23:38AM +0100, Martin Ågren wrote:\n\n> Yeah, I agree. It's very surprising that we retain only a subset of\n> state, and that does feel like a bug to me.\n>\n> >       c->pad.flush_type = no_flush;\n> > +     c->pad.truncate = trunc_none;\n> > +     c->pad.padding = 0;\n> >       return total_consumed;\n> >  }\n>\n> This is using the same default values now as you started to use in the\n> preceding commit. It might make sense to introduce a macro or function\n> to initialize the structure so that we don't duplicate initialization.\n\nGood point. I'll make the preceding commit use a new\n`padding_args_clear()`, then reuse it here.\n\nBTW, we rely on initializing the struct with all-zeroes to put it in\nthis cleared state. Which is true, since the \"none\"/\"no\" enum members\nare indeed zero. That's not explicit though. I'm thinking of adding a\npreparatory patch to make `no_flush` and `trunc_none` be explicitly\nzero, and see if there are other such enum values in this file.\n\n\nMartin\n"},{"id":"514778","messageId":"CAN0heSosT5gVHZ3t7APJ0rGXD_agU8NQBE=t0KNK+C0huY5niw@mail.gmail.com","threadId":"63162","inReplyTo":"Z9vdVP4edeaRawsz@pks.im","subject":"Re: [PATCH 7/8] pretty: refactor parsing of magic","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2025-03-20T16:12:08Z","receivedAt":"2025-03-20T16:12:22Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Thu, 20 Mar 2025 at 10:18, Patrick Steinhardt <ps@pks.im> wrote:\n>\n> On Wed, Mar 19, 2025 at 08:23:40AM +0100, Martin Ågren wrote:\n> > +enum magic {\n> > +     NO_MAGIC,\n> > +     ADD_LF_BEFORE_NON_EMPTY,\n> > +     DEL_LF_BEFORE_EMPTY,\n> > +     ADD_SP_BEFORE_NON_EMPTY\n> > +};\n> > +\n>\n> It would be nice to give all of these enums a common prefix, e.g.:\n>\n>     enum magic {\n>             MAGIC_NONE,\n>             MAGIC_ADD_LF_BEFORE_NON_EMPTY,\n>             MAGIC_DEL_LF_BEFORE_EMPTY,\n>             MAGIC_ADD_SP_BEFORE_NON_EMPTY\n>     };\n>\n> Makes it easier to see that things belong together and it provides\n> proper namespacing.\n\nAgreed, good point.\n\n> On the other hand you simply retain existing names. I don't insist on\n> the refactoring, but still thing it would be nice as the enum has wider\n> scope now.\n\nRight. It's only file-scoped, but that's still a bigger scope... I'll\nrename them to give them all a common prefix as suggested.\n\n> It took me a bit to figure out why this is equivalent to what we had\n> before. But:\n>\n>   - If `parse_magic()` returns bigger than 1 we'd have exited early, so\n>     this return here is never hit.\n>\n>   - If it returns `0` we have hit `NO_MAGIC`, and we have another early\n>     return for this case.\n>\n> So we only end up here in case `consumed = parse_magic(...)` is 1, and\n> then we add the result from `format_and_pad_commit()` to that value.\n> Which means that the refactoring is true to the original spirit.\n\nIf there's anything in particular you think should be called out in the\ncommit message to assist future readers, just let me know. I'll take\nyour points above as inspiration for things to highlight better.\n\nThere's also the return value \"2\", which is a bit, well, magic. Or at\nleast fairly arbitrary. I kind of preferred it over switching to a\nsigned type in this one spot though.\n\nThanks for all your very helpful comments.\n\n\nMartin\n"},{"id":"514897","messageId":"20250324035001.GC690093@coredump.intra.peff.net","threadId":"63162","inReplyTo":"5f787ddac2d80391feadb8cf6be379fc8e58652f.1742367347.git.martin.agren@gmail.com","subject":"Re: [PATCH 2/8] pretty: simplify if-else to reduce code duplication","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-03-24T03:50:01Z","receivedAt":"2025-03-24T03:50:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 19, 2025 at 08:23:35AM +0100, Martin Ågren wrote:\n\n> First we look for \"auto,\", then we try \"always,\", then we fall back to\n> the default, which is to do exactly the same thing as we do for \"auto,\".\n> The amount of code duplication isn't huge, but still: reading this code\n> carefully requires spending at least *some* time on making sure the two\n> blocks of code are indeed identical.\n> \n> Rearrange the checks so that we end with the default case,\n> opportunistically consuming the \"auto,\" which may or may not be there.\n\nOK. The duplicated lines are not all that long, but I don't mind\ncollapsing the cases, especially with the explanatory comment that's\nthere.\n\n> In the \"always,\" case, we don't actually *do* anything, so if we were\n> into golfing, we'd just write the whole thing as a single\n> \n>   if (!skip_prefix(begin, \"always,\", &begin)) {\n>     ...\n>   }\n> \n> If we ever learn something new besides \"always,\" and \"auto,\" we'd need\n> to pull things apart again. Plus we still need somewhere to place the\n> comment. Let's focus on code de-duplication rather than golfing for now.\n\nYeah, I think what you wrote in the patch is much better than trying to\ngolf further.\n\nSo looks good to me.\n\n-Peff\n"},{"id":"514910","messageId":"Z-Eveqmb7Et6aHrO@pks.im","threadId":"63162","inReplyTo":"CAN0heSpN-k886+RsZ0+djLd974Mq57B4quZK1yKXRMxCnOvzZw@mail.gmail.com","subject":"Re: [PATCH 4/8] pretty: fix parsing of half-valid \"%<\" and \"%>\" placeholders","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-03-24T10:10:02Z","receivedAt":"2025-03-24T10:10:11Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Mar 20, 2025 at 05:11:09PM +0100, Martin Ågren wrote:\n> On Thu, 20 Mar 2025 at 10:18, Patrick Steinhardt <ps@pks.im> wrote:\n> >\n> > On Wed, Mar 19, 2025 at 08:23:37AM +0100, Martin Ågren wrote:\n> > > When parsing a \"%<\" or \"%>\", only store the parsed data after parsing\n> > > successfully. The added test would have failed before this commit. It\n> > > also shows how the existing behavior is hardly something someone can\n> > > rely on since the non-consumed modifier (\"%<(10,bad)\") shows up verbatim\n> > > in the pretty output.\n> >\n> > Ideally I'd expect us to die when seeing misformatted placeholders like\n> > this. This is way less confusing to the user as otherwise things _look_\n> > like they work, but we silently do the wrong thing.\n> \n> Right. I can see how it makes some kind of sense to print what we don't\n> understand when it's something short and simple like \"%X\". But for more\n> complex \"%X(first,second)\" it's kind of obvious that a misspelled\n> \"X(fist,second)\" isn't something you want in the output. The whole \"if\n> we can't parse, return zero as the number of consumed characters so that\n> we can print verbatim while looking for next '%'\" is a central piece of\n> the design here. One could certainly imagine a \"strict\" mode.\n> \n> > That being said, I have no idea whether we can do such a change now\n> > without breaking existing usecases. As you rightfully argue the result\n> > already is wrong, but with my proposal we'd completely refuse to do\n> > anything. Which I'd argue is a good thing in the end.\n> \n> I can see the value of a strict mode, with command line options and\n> config switches and whatnot, maybe even a changed default behavior at\n> some point. I'd rather punt on that for now. TBH, I'd be afraid to do a\n> hard switch from \"0 means print it instead\" to \"0 means die\". I don't\n> disagree that it would be a better end-game though, at some point.\n\nYup, I fully agree that this is a bit more of a risky change and that it\ndoesn't have to be part of this patch series.\n\nPatrick\n"}]}