Re: [PATCH v3 5/6] trailer: allow non-trailers in trailer block

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 18, 2016, 16:36 UTC
Message-ID: <xmqqbmyhr4vt.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <cb622421-1857-d136-2618-ec62d438790d@google.com>

Jonathan Tan <jonathantanmy@google.com> writes:

Show 9 quoted lines

>>     * rs/c-auto-resets-attributes:
>>       pretty: avoid adding reset for %C(auto) if output is empty
>>
>> And neither of the two colon containing line remotely resembles how
>> a typical RFC-822 header is formatted.  So that may serve as a hint
>> to how we can tighten it without introducing false negative.
>
> The only "offending" character is the space (according to RFC 822),
> but that sounds like a good rule to have.

I suspect that we should be willing to deviate from the letter of RFC to reject misidentification. I saw things like

	Thanks to: Jonathan Tan <jt@host.xz>
	Signed-off-by: A U Thor <au@th.or>

in the wild (notice the SP between Thanks and to), for example. Rejecting leading whitespace as a line that does *not* start the header (hence its colon does not count) may be a good compromise.

> I think that "Signed-off-by:" is not guaranteed to be
> present.

But do we really care about that case where there is no S-o-b:? I personally do not think so.

Show 7 quoted lines

> Defining a trailer line as "a line starting with a token,
> then optional whitespace, then separator", maybe the following rule:
> - at least one trailer line generated by Git ("(cherry picked by" or
> "Signed-off-by") or configured in the "trailer" section in gitconfig
> OR
> - at least 3/4 logical trailer lines (I'm wondering if this should be
> 100% trailer lines)

I'd strongly suggest turning that OR to AND. We will not safely be able to write a commit log message that describes how S-o-b lines are handled in its last paragraph otherwise.

I do not care too deeply about 3/4, but I meant to allow 75% cruft but no more than that, and the fact that the threashold is set at way more than 50% is important. IOW, if you have

	Ordinary log message here...

	S-o-b: A U Thor <au@th.or>
	[a short description that is typically a single liner
        in the real world use pattern we saw in the world, but
	could overflow to become multi line cruft]
	S-o-b: R E Layer <re@lay.er>

"last paragraph" is 5 lines long, among which 60% are cruft that is below the 75% threshold, and "am -s" can still add the S-o-b of the committer at the end of that existing last paragraph. Making it too strict would raise the false negative ratio.