From: Jonathan Tan Date: Wed, 19 Oct 2016 18:00:45 GMT Subject: Re: [PATCH v3 5/6] trailer: allow non-trailers in trailer block Message-ID: <31cae41d-701c-c657-f04e-3b617c0b1d39@google.com> In-Reply-To: On 10/18/2016 09:36 AM, Junio C Hamano wrote: > Jonathan Tan 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 > Signed-off-by: A U Thor > > 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. >> 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 >> 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 > [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 > > "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>