git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Patrick Steinhardt <ps@pks.im>
Date
Mar 20, 2025, 09:18 UTC
Message-ID
<Z9vdUTQTnctm3965@pks.im>
In-Reply-To
<465c91155eb30197b5eac00d294dc6e7ea2dd310.1742367347.git.martin.agren@gmail.com>
On Wed, Mar 19, 2025 at 08:23:39AM +0100, Martin Ågren wrote:
Show 92 quoted lines
> 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.

Show 27 quoted lines
> +	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
Previous: Martin ÅgrenNext: Martin Ågren
Message 17 of 22 in “pretty: minor bugfixing, some refactorings”
  1. 0/8 pretty: minor bugfixing, some refactoringsMartin Ågren, Mar 19, 2025
  2. 1/8 pretty: tighten function signature to not take `void *`Martin Ågren, Mar 19, 2025
  3. Patrick SteinhardtMar 20, 2025
  4. 2/8 pretty: simplify if-else to reduce code duplicationMartin Ågren, Mar 19, 2025
  5. Patrick SteinhardtMar 20, 2025
  6. Martin ÅgrenMar 20, 2025
  7. Jeff KingMar 24, 2025
  8. 3/8 pretty: collect padding-related fields in separate structMartin Ågren, Mar 19, 2025
  9. 4/8 pretty: fix parsing of half-valid "%<" and "%>" placeholdersMartin Ågren, Mar 19, 2025
  10. Patrick SteinhardtMar 20, 2025
  11. Martin ÅgrenMar 20, 2025
  12. Patrick SteinhardtMar 24, 2025
  13. 5/8 pretty: after padding, reset padding infoMartin Ågren, Mar 19, 2025
  14. Patrick SteinhardtMar 20, 2025
  15. Martin ÅgrenMar 20, 2025
  16. 6/8 pretty: refactor parsing of line-wrapping "%w" placeholderMartin Ågren, Mar 19, 2025
  17. Patrick SteinhardtMar 20, 2025
  18. Martin ÅgrenMar 20, 2025
  19. 7/8 pretty: refactor parsing of magicMartin Ågren, Mar 19, 2025
  20. Patrick SteinhardtMar 20, 2025
  21. Martin ÅgrenMar 20, 2025
  22. 8/8 pretty: refactor parsing of decoration optionsMartin Ågren, Mar 19, 2025

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.