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

From: Jonathan Tan <jonathantanmy@google.com>
Date: Oct 19, 2016, 18:00 UTC
Message-ID: <31cae41d-701c-c657-f04e-3b617c0b1d39@google.com>
In-Reply-To: <xmqqbmyhr4vt.fsf@gitster.mtv.corp.google.com>

On 10/18/2016 09:36 AM, Junio C Hamano wrote:

Show 21 quoted lines

> Jonathan Tan <jonathantanmy@google.com> writes:
>
>>>     * 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.

Good point.

Show 5 quoted lines

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

I think we should - the use case I had in mind when I started this is the Android Git repository, which does not use Signed-off-by. For example, I quoted this commit in an earlier e-mail [1]:

https://android.googlesource.com/platform/frameworks/base/+/4c5281862f750cbc9d7355a07ef1a5545b9b3523

which has the footer:

   Bug: http://b/30562229
   Test: readelf --dyn-sym app_process32 and check that bsd_signal is 
exported
         readelf --dyn-sym app_process64 and check that bsd_signal is 
not exported
   Change-Id: Iec584070b42bc7fa43b114c0f884aff2db5a6858

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

Ah, sorry, I misread your original suggestion.

Would this work then:
- at least one trailer line generated by Git ("(cherry picked by" or
   "Signed-off-by: ") or configured in the "trailer" section in
   git config AND at least 25% logical trailer lines
OR
- 100% logical trailer lines

The first part is your original suggestion except that I think that it is more consistent to allow other trailer lines as well (but I do not feel too strongly about this). The second part would satisfy the Android Git use case above, and also retain existing behavior when "Signed-off-by" (for example) is added to an existing footer that does not contain "Signed-off-by" yet.

What do you think?

[1] Message ID <29cb0f55-f729-80af-cdca-64e927fa97c0@google.com>