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

[Junio C Hamano <gitster@pobox.com>]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> 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"?

It reads much better.

>> 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.

Yeah, (pp->from_ident != NULL) there serves two purposes.  Whatever
else is in that if() statement, the body of the statement depends on
the pp->from_ident being non-NULL and the control must not enter
there otherwise.  The other purpose is to guard the other half of
the if() statement, which happens to be ident_cmp() that looks at
the same pp->from_ident and depends on it being non-NULL.

I think the condition gets much cleaner by untangling it to

    - use_inbody_from() function does *not* check pp->from_ident; it
      just assumes it is not NULL 

    - the caller becomes

	if (pp->from_ident && use_inbody_from(...)) {
		... stuff that use pp->from_ident ...
	}

> 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.

I allowed wider line for function decl, by inertia, for
greppability, but I should fix that.  Thanks for noticing.