From: Jonathan Tan Date: Tue, 18 Oct 2016 02:02:46 GMT Subject: Re: [PATCH v3 5/6] trailer: allow non-trailers in trailer block Message-ID: In-Reply-To: On 10/17/2016 06:42 PM, Junio C Hamano wrote: > Stefan Beller writes: > >> On Fri, Oct 14, 2016 at 10:38 AM, Jonathan Tan 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. >> 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 ;-)