Re: [PATCH v3] bisect: report the found commit with "show"

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 15, 2024, 21:24 UTC
Message-ID: <xmqq7cgyl3pr.fsf@gitster.g>
In-Reply-To: <CAPig+cQu15HzZkeT3+oG3U7iFax5_GYUB=uqwuJxshw-PD=VHQ@mail.gmail.com>

Eric Sunshine <sunshine@sunshineco.com> writes:

Show 5 quoted lines

>> Pass some hard-coded options to "git show" to make the output similar
>> to the one we are replacing, such as showing a patch summary only.
>>
>> Signed-off-By: Peter Krefting <peter@softwolves.pp.se>
>> ---

Curious how you trimmed the trailers from the submitted patch ;-)

Although we do not use Cc: in this project, we do recommend use of the "Reported-by" trailer in Documentation/SubmittingPatches.

>> diff --git a/bisect.c b/bisect.c
>> ...

Show 6 quoted lines

> Style nit: On this project, multi-line comments are formatted like this:
>
>     /*
>      * This is a multi-line
>      * comment.
>      */

True.

> It also feels slightly odd to place each option on its own line in the
> call to strvec_pushl() but then place the terminating NULL on the same
> line as the oid_to_hex() call. But that's a minor and subjective point
> hardly worth mentioning.

I think that is because we may want to tweak the list of options, but no matter what change we make to them in the future, the object name as the last parameter is likely to remain the last one, and with that reasoning, I agree with the layout in the patch as posted.

What is more problematic is that the message is sent with

	Content-Type: text/plain; format=flowed; charset=US-ASCII

and the contents of the message is in that flawed format, possibly corrupting whitespaces in irrecoverable ways.

I _think_ "git am" (actually, "git mailsplit" that is called from it) did a reasonable job this time, but I do not have a lot of confidence in the resulting commit---I would not be surprised if it is not identical to what Peter wanted to give us.

Peter, if the resulting commit I push out later today botches some whitespaces due to this issue, please complain. I'll fix the multi-line comment thing on my end before pushing the today's integration result out.

Thanks.