Re: [PATCH 1/2] pretty: separate out the logic to decide the use of in-body from

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Junio,

On Fri, 26 Aug 2022, Junio C Hamano wrote:

> When pretty-printing the log message for a given commit in e-mail
> format (e.g. "git format-patch"), we add in-body "From:" header when
> the author identity of the commit is different from the identity of
> the person who is "sending" the mail.

The quotes around "sending" made me stumble over this a bit. Maybe replace
it by saying "the person running the command"?

> Split out the logic into a helper function.  Because the "from_ident
> must be set" condition is there not because it is used in the
> ident_cmp() next, but because the codepath that is entered when this
> logic says "Yes, you should use in-body from" requires values there
> in from_ident member, so separate it out into an if() statement on
> its own to clarify it.

Even after reading this three times, I had trouble understanding it. I
then consulted the diff and started to grasp what you mean. I have no
good idea how to improve the wording, but maybe you can give it another
go? Or simply state that the condition was untangled a bit.

The diff looks good.

Ciao,
Dscho

P.S.: I do not know how strongly you feel these days about lines longer
than 80 columns, but personally I do not care about this rule, so I am
more than just fine with adding such a line here.

> diff --git a/pretty.c b/pretty.c
> index 6d819103fb..51e3fa5736 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -477,6 +477,15 @@ static void append_line_with_color(struct strbuf *sb, struct grep_opt *opt,
>  	}
>  }
>
> +static int use_inbody_from(const struct pretty_print_context *pp, const struct ident_split *ident)
> +{
> +	if (!pp->from_ident)
> +		return 0;
> +	if (ident_cmp(pp->from_ident, ident))
> +		return 1;
> +	return 0;
> +}
> +
>  void pp_user_info(struct pretty_print_context *pp,
>  		  const char *what, struct strbuf *sb,
>  		  const char *line, const char *encoding)
> @@ -503,7 +512,7 @@ void pp_user_info(struct pretty_print_context *pp,
>  		map_user(pp->mailmap, &mailbuf, &maillen, &namebuf, &namelen);
>
>  	if (cmit_fmt_is_mail(pp->fmt)) {
> -		if (pp->from_ident && ident_cmp(pp->from_ident, &ident)) {
> +		if (use_inbody_from(pp, &ident)) {
>  			struct strbuf buf = STRBUF_INIT;
>
>  			strbuf_addstr(&buf, "From: ");
> --
> 2.37.2-587-g47adba97a9
>
>