On Wed, Mar 19, 2025 at 08:23:39AM +0100, Martin Ågren wrote:
> Our parsing of a "%w" placeholder is quite a big chunk of code in
> the middle of our switch for handling a few different placeholders. We
> parse into three different variables, then use them to compare to and
> update existing values in the big `struct format_commit_context`.
>
> Pull out a helper function for parsing such a "%w" placeholder. Define a
> struct for collecting the three variables.
>
> Unlike recent commits, parsing and subsequent use are already a bit more
> separated in the sense that we don't parse directly into the big context
> struct. Thus, unlike the preceding commits, this does not fix any bugs
> that I'm aware of. There's still value in separating parsing and usage
> more clearly and simplifying `format_commit_one()`.
>
> Note that we use two different types for these values, `unsigned long`
> when parsing, `size_t` when eventually applying. Let's go for `size_t`
> in our struct. I don't know if there are platforms where assigning an
> `unsigned long` to a `size_t` could truncate the value, but since we
> already verify the values to be at most 16 KiB, we should be able to fit
> them into any sane `size_t`s.
>
> Signed-off-by: Martin Ågren <martin.agren@xxxxxxxxx>
> ---
> pretty.c | 120 +++++++++++++++++++++++++++++++++++--------------------
> 1 file changed, 76 insertions(+), 44 deletions(-)
>
> diff --git a/pretty.c b/pretty.c
> index f53e77ed86..c44ff87481 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -893,6 +893,10 @@ struct padding_args {
> int padding;
> };
>
> +struct rewrap_args {
> + size_t width, indent1, indent2;
> +};
> +
> struct format_commit_context {
> struct repository *repository;
> const struct commit *commit;
> @@ -902,7 +906,7 @@ struct format_commit_context {
> struct signature_check signature_check;
> const char *message;
> char *commit_encoding;
> - size_t width, indent1, indent2;
> + struct rewrap_args rewrap;
> int auto_color;
> struct padding_args pad;
>
> @@ -1034,18 +1038,21 @@ static void strbuf_wrap(struct strbuf *sb, size_t pos,
>
> static void rewrap_message_tail(struct strbuf *sb,
> struct format_commit_context *c,
> - size_t new_width, size_t new_indent1,
> - size_t new_indent2)
> + const struct rewrap_args *new_rewrap)
> {
> - if (c->width == new_width && c->indent1 == new_indent1 &&
> - c->indent2 == new_indent2)
> + const struct rewrap_args *old_rewrap = &c->rewrap;
> +
> + if (old_rewrap->width == new_rewrap->width &&
> + old_rewrap->indent1 == new_rewrap->indent1 &&
> + old_rewrap->indent2 == new_rewrap->indent2)
> return;
> +
> if (c->wrap_start < sb->len)
> - strbuf_wrap(sb, c->wrap_start, c->width, c->indent1, c->indent2);
> + strbuf_wrap(sb, c->wrap_start, old_rewrap->width,
> + old_rewrap->indent1, old_rewrap->indent2);
> +
> c->wrap_start = sb->len;
> - c->width = new_width;
> - c->indent1 = new_indent1;
> - c->indent2 = new_indent2;
> + c->rewrap = *new_rewrap;
> }
>
> static int format_reflog_person(struct strbuf *sb,
> @@ -1443,6 +1450,57 @@ static void free_decoration_options(const struct decoration_options *opts)
> free(opts->tag);
> }
>
> +static size_t parse_rewrap(const char *placeholder, struct rewrap_args *rewrap)
> +{
> + unsigned long width = 0, indent1 = 0, indent2 = 0;
> + char *next;
> + const char *start;
> + const char *end;
> +
> + memset(rewrap, 0, sizeof(*rewrap));
The `memset()` feels rather unnecessary as we only use the result at our
single caller in case we return successfully. And if we do, we know to
initialize all struct fields.
> + if (placeholder[1] != '(')
> + return 0;
This matches the `else` branch. It's nice that it's converted into an
early return.
> + start = placeholder + 2;
> + end = strchr(start, ')');
> +
> + if (!end)
> + return 0;
> + if (end > start) {
> + width = strtoul(start, &next, 10);
> + if (*next == ',') {
> + indent1 = strtoul(next + 1, &next, 10);
> + if (*next == ',') {
> + indent2 = strtoul(next + 1,
> + &next, 10);
> + }
> + }
> + if (*next != ')')
> + return 0;
> + }
> +
> + /*
> + * We need to limit the format here as it allows the
> + * user to prepend arbitrarily many bytes to the buffer
> + * when rewrapping.
> + */
> + if (width > FORMATTING_LIMIT ||
> + indent1 > FORMATTING_LIMIT ||
> + indent2 > FORMATTING_LIMIT)
> + return 0;
And all of the above matches the `if` branch, except that we don't
perform the rewrap itself.
Looks good.
Patrick
