Re: [PATCH 2/6] pretty: split oneline and email subject printing

From: "Kristoffer Haugsbakk" <code@khaugsbakk.name>
To: "Jeff King" <peff@peff.net>
Cc: git@vger.kernel.org
Subject: Re: [PATCH 2/6] pretty: split oneline and email subject printing
Date: Fri, 22 Mar 2024 23:00:32 +0100	[thread overview]
Message-ID: <69904aaf-7830-4b2a-8fd9-7f6fe55b45fe@app.fastmail.com> (raw)
In-Reply-To: <20240320002851.GB904136@coredump.intra.peff.net>

On Wed, Mar 20, 2024, at 01:28, Jeff King wrote:
> The pp_title_line() function is used for two formats: the oneline format
> and the subject line of the email format. But most of the logic in the
> function does not make any sense for oneline; it is about special
> formatting of email headers.
>
> Lumping the two formats together made sense long ago in 4234a76167
> (Extend --pretty=oneline to cover the first paragraph, 2007-06-11), when
> there was a lot of manual logic to paste lines together. But later,
> 88c44735ab (pretty: factor out format_subject(), 2008-12-27) pulled that
> logic into its own function.
>
> We can implement the oneline format by just calling that one function.
> This makes the intention of the code much more clear, as we know we only
> need to worry about those extra email options when dealing with actual
> email.
>
> While the intent here is cleanup, it is possible to trigger these cases
> in practice by running format-patch with an explicit --oneline option.
> But if you did, the results are basically nonsense. For example, with
> the preserve_subject flag:
>
>   $ printf "%s\n" one two three | git commit --allow-empty -F -
>   $ git format-patch -1 --stdout -k | grep ^Subject
>   Subject: =?UTF-8?q?one=0Atwo=0Athree?=
>   $ git format-patch -1 --stdout -k --oneline --no-signature
>   2af7fbe one
>   two
>   three
>
> Or with extra headers:
>
>   $ git format-patch -1 --stdout --cc=me --oneline --no-signature
>   2af7fbe one two three
>   Cc: me
>
> So I'd actually consider this to be an improvement, though you are
> probably crazy to use other formats with format-patch in the first place
> (arguably it should forbid non-email formats entirely, but that's a
> bigger change).

Makes sense. This make the code more focused.

> As a bonus, it eliminates some pointless extra allocations for the
> oneline output. The email code, since it has to deal with wrapping,
> formats into an extra auxiliary buffer. The speedup is tiny, though like
> "rev-list --no-abbrev --format=oneline" seems to improve by a consistent
> 1-2% for me.

Nice. That could add up when formatting a moderate amount of patches.

-- 
Kristoffer Haugsbakk