Johannes Schindelin <Johannes.Schindelin@xxxxxx> 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.
