From: Junio C Hamano <hidden> Date: 2016-06-15 23:06:27
Linus Torvalds [off-list ref] writes:
So I think that logic should basically be extended to saying
- if any line in the last chunk has a "Signed-off-by:", set a flag.
- at the end of the loop, if that flag wasn't set, return 0.
I am reluctant to special case S-o-b: too much, even though this is
about "am -s" and by definition S-o-b: is special, as that is what
we are adding after all.
How about a bit looser rule like this?
A block of text at the end of the message, each and every
line in which must match "^[^: ]+:[ ]" (that is,
a "keyword" that does not contain a whitespace nor a colon,
followed by a colon and whitespace, and arbitrary value thru
the end of line) is a signature block.
On Fri, Sep 4, 2015 at 5:54 PM, Junio C Hamano [off-list ref] wrote:
How about a bit looser rule like this?
A block of text at the end of the message, each and every
line in which must match "^[^: ]+:[ ]" (that is,
a "keyword" that does not contain a whitespace nor a colon,
followed by a colon and whitespace, and arbitrary value thru
the end of line) is a signature block.
No. That's still broken.
The thing is, and that was what the report was all about, not every
line _is_ of that format. We have commetns from the sign-off people.
Things like this:
Signed-off-by: Noam Camus [off-list ref]
Acked-by: Vineet Gupta [off-list ref]
[ Also removed pointless cast from "void *". - Linus ]
Signed-off-by: Linus Torvalds [off-list ref]
or
Signed-off-by: Andi Kleen [off-list ref]
[ Updated comments and changelog a bit. ]
Signed-off-by: Peter Zijlstra (Intel) [off-list ref]
Link: http://lkml.kernel.org/r/1424225886-18652-3-git-send-email-andi@firstfloor.org
Signed-off-by: Ingo Molnar [off-list ref]
so no, it is simply not true that "every line must match".
I'm not even seeing why you argue for that, since clearly having a
sign-off-line is actually a safer choice too. The "every line must
match" rule is bad, not just because it's not true like above, but
also because it can be true without it being a sign-off block.
For example, it's not at all unlikely that you have perfectly normal
comments that just list some subsystem and their changes. Which could
easily look like
Trivial fixes all over the tree
drm: fix whitespace
mm: speeling errors
kernel: indentation and codign style
The above looks like a perfectly sane commit log to me.
Do you seriously think that it makes for a better "sign-off block
test" than one that actually checks for "is there a sign-off line"?
I'd much rather have special cases like testing for specific keywords
or looking for things that look like emails, than make it about being
"every line has this very generic format".
Linus
From: Johannes Sixt <hidden> Date: 2016-06-15 23:06:27
Am 05.09.2015 um 02:54 schrieb Junio C Hamano:
Linus Torvalds [off-list ref] writes:
quoted
So I think that logic should basically be extended to saying
- if any line in the last chunk has a "Signed-off-by:", set a flag.
- at the end of the loop, if that flag wasn't set, return 0.
I am reluctant to special case S-o-b: too much, even though this is
about "am -s" and by definition S-o-b: is special, as that is what
we are adding after all.
How about a bit looser rule like this?
A block of text at the end of the message, each and every
line in which must match "^[^: ]+:[ ]" (that is,
a "keyword" that does not contain a whitespace nor a colon,
followed by a colon and whitespace, and arbitrary value thru
the end of line) is a signature block.
Why do we need a new rule? The old git-am had a logic that pleased
everyone, and it must have been implemented somewhere. Shouldn't it be
sufficient to just re-implement or re-use that logic?
-- Hannes
From: Jeff King <hidden> Date: 2016-06-15 23:06:27
On Sat, Sep 05, 2015 at 09:30:27AM +0200, Johannes Sixt wrote:
quoted
How about a bit looser rule like this?
A block of text at the end of the message, each and every
line in which must match "^[^: ]+:[ ]" (that is,
a "keyword" that does not contain a whitespace nor a colon,
followed by a colon and whitespace, and arbitrary value thru
the end of line) is a signature block.
Why do we need a new rule? The old git-am had a logic that pleased everyone,
and it must have been implemented somewhere. Shouldn't it be sufficient to
just re-implement or re-use that logic?
That was my thought, too; if there is a regression, let's start by
fixing that for the upcoming 2.6.0, and then we can worry about doing
something fancier[1] later.
And I think the original behavior really is what Linus is asking for: we
consider the final block, even with a "[]" comment, as a S-o-b block if
it has a Signed-off-by in it. So if we have the final block:
[some comment]
Signed-off-by: Somebody Else [off-list ref]
Running "am -s" with the current master yields:
[some comment]
Signed-off-by: Somebody Else [off-list ref]
Signed-off-by: Jeff King [off-list ref]
which is wrong. Running the same with v2.5.0 gets:
[some comment]
Signed-off-by: Somebody Else [off-list ref]
Signed-off-by: Jeff King [off-list ref]
So far so good. Now let's change our input to:
[some comment]
Reviewed-by: Somebody Else [off-list ref]
and run "am -s". Current "master" continues to stick the extra line in
there. No surprise. But now so does v2.5.0!
So I don't think the old behavior covered all cases well, either, and
there's room for improvement. But likewise, I don't recall seeing a lot
of complaints about it in practice. It seems like a sane thing to
restore it for the upcoming release, and then build from there.
-Peff
[1] I think part of the reason people are interested in "fancy" here is
that this topic extends beyond "git am". There's "commit -s", of
course, but there's also the generic "interpret-trailers" command,
which is supposed to be a generalization of the "--signoff" option.
It would be nice if the rules remained consistent for when we add a
trailer to an existing block, rather than special-casing signoffs.
But again, I think that's something to shoot for in the long run.
It's more important in the short term not to regress "am".