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

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 29, 2022, 17:29 UTC
Message-ID: <xmqq7d2rot48.fsf@gitster.g>
In-Reply-To: <08p469q7-2os4-p391-9nr4-10q7o9s4414s@tzk.qr>

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

Show 11 quoted lines

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

Show 11 quoted lines

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