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

From: Jonathan Tan <jonathantanmy@google.com>
Date: Oct 18, 2016, 02:02 UTC
Message-ID: <cb622421-1857-d136-2618-ec62d438790d@google.com>
In-Reply-To: <xmqq7f96sa9i.fsf@gitster.mtv.corp.google.com>

On 10/17/2016 06:42 PM, Junio C Hamano wrote:

Show 26 quoted lines

> Stefan Beller <sbeller@google.com> writes:
>
>> On Fri, Oct 14, 2016 at 10:38 AM, Jonathan Tan <jonathantanmy@google.com> wrote:
>>>
>>>  Existing trailers are extracted from the input message by looking for
>>> -a group of one or more lines that contain a colon (by default), where
>>> +a group of one or more lines in which at least one line contains a
>>> +colon (by default), where
>>
>> Please see commit
>> 578e6021c0819d7be1179e05e7ce0e6fdb2a01b7
>> for an example where I think this is overly broad.
>
> Hmph.  That's a merge.
>
>     Merge branch 'rs/c-auto-resets-attributes'
>
>     When "%C(auto)" appears at the very beginning of the pretty format
>     string, it did not need to issue the reset sequence, but it did.
>
>     * 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.

Show 27 quoted lines

>> Another made up example, that I'd want to feed
>> in commit -s eventually:
>>
>> --8<--
>> demonstrate colons in Java
>>
>> First paragraph is not interesting.
>>
>> Also if using another Language such as Java, where I point out
>> Class::function() to be problematic
>> --8<--
>>
>> This would lack the white space between the last paragraph and
>> the Sign off ?
>>
>> So for this patch I am mostly concerned about false positives hidden
>> in actual text.
>
> Yes.
>
> These are exactly why I mentioned "if certian number or percentage"
> in my earlier suggestion.
>
> I think in practice, "A paragraph with at least one Signed-off-by:
> line, and has no more than 3/4 of the (logical) lines that do not
> resemble how a typical RFC-822 header is formatted" or something
> along that line would give us a reasonable safety.

I think that "Signed-off-by:" is not guaranteed to be present. 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)

> Your Java example will fail the criteria in two ways, so we'd be
> safe ;-)