Re: [PATCH 4/5] pretty: Add name_and_address_only parameter

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 25, 2023, 02:23 UTC
Message-ID: <xmqqedrjfj2x.fsf@gitster.g>
In-Reply-To: <20230119223858.29262-5-zev@bewilderbeest.net>

Zev Weiss <zev@bewilderbeest.net> writes:

Show 5 quoted lines

> This is meant to be used with pp_user_info() when using it to format
> email recipients generated by --to-cmd/--cc-cmd.  When set it omits
> the leading 'From: ', trailing linefeed, and the date suffix, and
> additionally will return the input string unmodified if
> split_ident_line() can't parse it (e.g. for a bare email address).

It is somewhat disturbing to see that this only takes effect when cmit_fmt_is_mail(pp->fmt) and completely ignored otherwise. Seeing pp->fmt and pp->name_and_address_only sitting next to each other, it looks like a layering error.

I wonder if you instead want a new value for pp->fmt that cmit_fmt_is_mail() considers an e-mail format but is different from what we used for usual e-mail format?

Show 14 quoted lines

> diff --git a/pretty.c b/pretty.c
> index 1e1e21878c83..e6798fadc107 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -509,8 +509,11 @@ void pp_user_info(struct pretty_print_context *pp,
>  		return;
>  
>  	line_end = strchrnul(line, '\n');
> -	if (split_ident_line(&ident, line, line_end - line))
> +	if (split_ident_line(&ident, line, line_end - line)) {
> +		if (pp->name_and_address_only)
> +			strbuf_addstr(sb, line);
>  		return;
> +	}

This error handling is also disturbing. What makes it correct to parrot the original input that does not parse correctly as an ident line to the output, only when name_and_address_only bit is on? It does not make any sense to do so when cmit_fmt_is_mail(pp->fmt) is false, especially.