Re: [PATCH 6/8] pretty: refactor parsing of line-wrapping "%w" placeholder

[Patrick Steinhardt <ps@pks.im>]

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