Re: [PATCH 7/8] pretty: refactor parsing of magic

[Patrick Steinhardt <ps@pks.im>]

On Wed, Mar 19, 2025 at 08:23:40AM +0100, Martin Ågren wrote:
> Similar to the previous commit, pull out our parsing of initial
> placeholder magic into a separate function. This helps make it a bit
> easier to get an overview of `format_commit_item()`. It also represents
> another small step towards separating the parsing of placeholders from
> subsequent usage of the parsed information.
> 
> This diff might be a bit easier to read with `-w`.
> 
> Signed-off-by: Martin Ågren <martin.agren@gmail.com>
> ---
>  pretty.c | 69 ++++++++++++++++++++++++++++++++++----------------------
>  1 file changed, 42 insertions(+), 27 deletions(-)
> 
> diff --git a/pretty.c b/pretty.c
> index c44ff87481..ddc7fd6aab 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -1929,17 +1929,17 @@ static size_t format_and_pad_commit(struct strbuf *sb, /* in UTF-8 */
>  	return total_consumed;
>  }
>  
> -static size_t format_commit_item(struct strbuf *sb, /* in UTF-8 */
> -				 const char *placeholder,
> -				 struct format_commit_context *context)
> +enum magic {
> +	NO_MAGIC,
> +	ADD_LF_BEFORE_NON_EMPTY,
> +	DEL_LF_BEFORE_EMPTY,
> +	ADD_SP_BEFORE_NON_EMPTY
> +};
> +

It would be nice to give all of these enums a common prefix, e.g.:

    enum magic {
            MAGIC_NONE,
            MAGIC_ADD_LF_BEFORE_NON_EMPTY,
            MAGIC_DEL_LF_BEFORE_EMPTY,
            MAGIC_ADD_SP_BEFORE_NON_EMPTY
    };

Makes it easier to see that things belong together and it provides
proper namespacing.

> +/* 2 for 'bad magic', otherwise whether we consumed 0 or 1 chars. */
> +static size_t parse_magic(const char *placeholder, enum magic *ret)
>  {
> -	size_t consumed, orig_len;
> -	enum {
> -		NO_MAGIC,
> -		ADD_LF_BEFORE_NON_EMPTY,
> -		DEL_LF_BEFORE_EMPTY,
> -		ADD_SP_BEFORE_NON_EMPTY
> -	} magic = NO_MAGIC;
> +	enum magic magic;
>  
>  	switch (placeholder[0]) {
>  	case '-':

On the other hand you simply retain existing names. I don't insist on
the refactoring, but still thing it would be nice as the enum has wider
scope now.

> @@ -1952,28 +1952,43 @@ static size_t format_commit_item(struct strbuf *sb, /* in UTF-8 */
>  		magic = ADD_SP_BEFORE_NON_EMPTY;
>  		break;
>  	default:
> -		break;
> +		*ret = NO_MAGIC;
> +		return 0;
>  	}
> -	if (magic != NO_MAGIC) {
> -		placeholder++;
>  
> -		switch (placeholder[0]) {
> -		case 'w':
> -			/*
> -			 * `%+w()` cannot ever expand to a non-empty string,
> -			 * and it potentially changes the layout of preceding
> -			 * contents. We're thus not able to handle the magic in
> -			 * this combination and refuse the pattern.
> -			 */
> -			return 0;
> -		};
> -	}
> +	switch (placeholder[1]) {
> +	case 'w':
> +		/*
> +		 * `%+w()` cannot ever expand to a non-empty string,
> +		 * and it potentially changes the layout of preceding
> +		 * contents. We're thus not able to handle the magic in
> +		 * this combination and refuse the pattern.
> +		 */
> +		*ret = NO_MAGIC;
> +		return 2;
> +	};
> +
> +	*ret = magic;
> +	return 1;
> +}
> +
> +static size_t format_commit_item(struct strbuf *sb, /* in UTF-8 */
> +				 const char *placeholder,
> +				 struct format_commit_context *context)
> +{
> +	size_t consumed, orig_len;
> +	enum magic magic;
> +
> +	consumed = parse_magic(placeholder, &magic);
> +	if (consumed > 1)
> +		return 0;
> +	placeholder += consumed;
>  
>  	orig_len = sb->len;
>  	if (context->pad.flush_type == no_flush)
> -		consumed = format_commit_one(sb, placeholder, context);
> +		consumed += format_commit_one(sb, placeholder, context);
>  	else
> -		consumed = format_and_pad_commit(sb, placeholder, context);
> +		consumed += format_and_pad_commit(sb, placeholder, context);
>  	if (magic == NO_MAGIC)
>  		return consumed;
>  
> @@ -1986,7 +2001,7 @@ static size_t format_commit_item(struct strbuf *sb, /* in UTF-8 */
>  		else if (magic == ADD_SP_BEFORE_NON_EMPTY)
>  			strbuf_insertstr(sb, orig_len, " ");
>  	}
> -	return consumed + 1;
> +	return consumed;

It took me a bit to figure out why this is equivalent to what we had
before. But:

  - If `parse_magic()` returns bigger than 1 we'd have exited early, so
    this return here is never hit.

  - If it returns `0` we have hit `NO_MAGIC`, and we have another early
    return for this case.

So we only end up here in case `consumed = parse_magic(...)` is 1, and
then we add the result from `format_and_pad_commit()` to that value.
Which means that the refactoring is true to the original spirit.

Patrick