git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Eric Sunshine <sunshine@sunshineco.com>
Cc: Peter Krefting <peter@softwolves.pp.se>,
	 git@vger.kernel.org,  Jeff King <peff@peff.net>,
	 Christian Couder <christian.couder@gmail.com>,
	Michael Osipov <michael.osipov@innomotics.com>
Subject: Re: [PATCH v3] bisect: report the found commit with "show"
Date: Mon, 15 Apr 2024 14:24:00 -0700	[thread overview]
Message-ID: <xmqq7cgyl3pr.fsf@gitster.g> (raw)
In-Reply-To: <CAPig+cQu15HzZkeT3+oG3U7iFax5_GYUB=uqwuJxshw-PD=VHQ@mail.gmail.com> (Eric Sunshine's message of "Sat, 13 Apr 2024 21:28:40 -0400")

Eric Sunshine <sunshine@sunshineco.com> writes:

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

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


  reply	other threads:[~2024-04-15 21:24 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-04-13 20:14 [PATCH v3] bisect: report the found commit with "show" Peter Krefting
2024-04-14  1:28 ` Eric Sunshine
2024-04-15 21:24   ` Junio C Hamano [this message]
2024-04-15 21:33     ` Eric Sunshine
2024-04-16  5:13       ` Jeff King
2024-04-16 19:44         ` Peter Krefting

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqq7cgyl3pr.fsf@gitster.g \
    --to=gitster@pobox.com \
    --cc=christian.couder@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=michael.osipov@innomotics.com \
    --cc=peff@peff.net \
    --cc=peter@softwolves.pp.se \
    --cc=sunshine@sunshineco.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).