[PATCH] commit: More generous accepting of RFC-2822 footer lines.

[PATCH] commit: More generous accepting of RFC-2822 footer lines.

[David Brown]

From: David Brown <davidb@quicinc.com>

'git commit -s' will insert a blank line before the Signed-off-by
line at the end of the message, unless this last line is a
Signed-off-by line itself.  Common use has other trailing lines
at the ends of commit text, in the style of RFC2822 headers.

Be more generous in considering lines to be part of this footer.
This may occasionally leave out the blank line for cases where
the commit text happens to start with a word ending in a colon,
but this results in less fixups than the extra blank lines with
Acked-by, or other custom footers.

Signed-off-by: David Brown <davidb@quicinc.com>
---
 builtin-commit.c  |   17 ++++++++++++++++-
 t/t7501-commit.sh |   19 +++++++++++++++++++
 2 files changed, 35 insertions(+), 1 deletions(-)

diff --git a/builtin-commit.c b/builtin-commit.c
index 200ffda..f081e80 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -414,6 +414,21 @@ static void determine_author_info(void)
 	author_date = date;
 }
 
+static int is_rfc2822_footer(const char *line)
+{
+	int ch;
+
+	while ((ch = *line++)) {
+		if (ch == ':')
+			return 1;
+		if ((33 <= ch && ch <= 57) ||
+		    (59 <= ch && ch <= 126))
+			continue;
+		break;
+	}
+	return 0;
+}
+
 static int prepare_to_commit(const char *index_file, const char *prefix,
 			     struct wt_status *s)
 {
@@ -489,7 +504,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 		for (i = sb.len - 1; i > 0 && sb.buf[i - 1] != '\n'; i--)
 			; /* do nothing */
 		if (prefixcmp(sb.buf + i, sob.buf)) {
-			if (prefixcmp(sb.buf + i, sign_off_header))
+			if (!is_rfc2822_footer(sb.buf + i))
 				strbuf_addch(&sb, '\n');
 			strbuf_addbuf(&sb, &sob);
 		}
diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
index e2ef532..05542b4 100755
--- a/t/t7501-commit.sh
+++ b/t/t7501-commit.sh
@@ -247,6 +247,25 @@ $existing" &&
 
 '
 
+test_expect_success 'signoff gap' '
+
+	echo 3 >positive &&
+	git add positive &&
+	alt="Alt-RFC-822-Header: Value" &&
+	git commit -s -m "welcome
+
+$alt" &&
+	git cat-file commit HEAD | sed -e "1,/^\$/d" > actual &&
+	(
+		echo welcome
+		echo
+		echo $alt
+		git var GIT_COMMITTER_IDENT |
+		sed -e "s/>.*/>/" -e "s/^/Signed-off-by: /"
+	) >expected &&
+	test_cmp expected actual
+'
+
 test_expect_success 'multiple -m' '
 
 	>negative &&
-- 
1.6.5.1

Re: [PATCH] commit: More generous accepting of RFC-2822 footer lines.

[Shawn O. Pearce]

David Brown <davidb@codeaurora.org> wrote:
> From: David Brown <davidb@quicinc.com>
> 
> 'git commit -s' will insert a blank line before the Signed-off-by
> line at the end of the message, unless this last line is a
> Signed-off-by line itself.  Common use has other trailing lines
> at the ends of commit text, in the style of RFC2822 headers.
> 
> Be more generous in considering lines to be part of this footer.
> This may occasionally leave out the blank line for cases where
> the commit text happens to start with a word ending in a colon,
> but this results in less fixups than the extra blank lines with
> Acked-by, or other custom footers.

The nasty perl I use in Gerrit's commit-msg hook is a bit more
expressive.  Basically the rule is we insert a blank line before
the new footer unless all lines in the last paragraph (so all text
after the last "\n\n" sequence) match the regex "^[a-zA-Z0-9-]+:".
 
> +test_expect_success 'signoff gap' '
> +
> +	echo 3 >positive &&
> +	git add positive &&
> +	alt="Alt-RFC-822-Header: Value" &&
> +	git commit -s -m "welcome
> +
> +$alt" &&

I wonder if we shouldn't also have a test case for the message:

	msg="test

this is a test that
fixes: 42.
"

as the result would be expected to be:

	exp="test

this is a test that
fixes: 42.

Signed-off-by A. U. Thor <...>
"

But:

	msg="test

this is a test

fixes: 42
"

would produce:

	exp="test

this is a test

fixes: 42
Signed-off-by A. U. Thor <...>
"

-- 
Shawn.

Re: [PATCH] commit: More generous accepting of RFC-2822 footer lines.

[Junio C Hamano]

"Shawn O. Pearce" <spearce@spearce.org> writes:

> David Brown <davidb@codeaurora.org> wrote:
>> From: David Brown <davidb@quicinc.com>
>> 
>> 'git commit -s' will insert a blank line before the Signed-off-by
>> line at the end of the message, unless this last line is a
>> Signed-off-by line itself.  Common use has other trailing lines
>> at the ends of commit text, in the style of RFC2822 headers.
>> 
>> Be more generous in considering lines to be part of this footer.
>> This may occasionally leave out the blank line for cases where
>> the commit text happens to start with a word ending in a colon,
>> but this results in less fixups than the extra blank lines with
>> Acked-by, or other custom footers.
>
> The nasty perl I use in Gerrit's commit-msg hook is a bit more
> expressive.  Basically the rule is we insert a blank line before
> the new footer unless all lines in the last paragraph (so all text
> after the last "\n\n" sequence) match the regex "^[a-zA-Z0-9-]+:".

Together with your suggestion for tests, the above makes quite a lot of
sense to me.

There is one thing to be careful about.

When deciding to omit adding a new S-o-b, we deliberately check only the
last S-o-b to see if it matches what we are trying to add.  This is so
that a message from you, that has my patch that was reviewed and touched
up by you with your sign-off, i.e.

	S-o-b: Junio
        S-o-b: Shawn

will not be prevented to have another sign-off by me, so that I can
certify that I know that your change I received from you in the patch is
kosher.  IOW, this is not a "duplicate" check.  The order of S-o-b:
matters as it records the flow of the patch.

Re: [PATCH] commit: More generous accepting of RFC-2822 footer lines.

[David Brown]

On Wed, Oct 28, 2009 at 12:14:55AM -0700, Junio C Hamano wrote:

> When deciding to omit adding a new S-o-b, we deliberately check only the
> last S-o-b to see if it matches what we are trying to add.  This is so
> that a message from you, that has my patch that was reviewed and touched
> up by you with your sign-off, i.e.

This is good to know.  I'll leave the existing last-SoB test in
place then, and just use the sophisticated check for a block of
RFC2822 footers to determine if there should be a blank line.

Jeff also pointed out that I should probably also allow lines
starting with whitespace to be considered header lines.

David

[PATCH] commit: More generous accepting of RFC-2822 footer lines.

[David Brown]

From: David Brown <davidb@quicinc.com>

'git commit -s' will insert a blank line before the Signed-off-by
line at the end of the message, unless this last line is a
Signed-off-by line itself.  Common use has other trailing lines
at the ends of commit text, in the style of RFC2822 headers.

Be more generous in considering lines to be part of this footer.
If the last paragraph of the commit message reasonably resembles
RFC-2822 formatted lines, don't insert that blank line.

The new Signed-off-by line is still only suppressed when the
author's existing Signed-off-by is the last line of the message.

Signed-off-by: David Brown <davidb@quicinc.com>
---
 builtin-commit.c  |   43 ++++++++++++++++++++++++++++++++++++++++++-
 t/t7501-commit.sh |   41 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 83 insertions(+), 1 deletions(-)

diff --git a/builtin-commit.c b/builtin-commit.c
index 200ffda..c395cbf 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -414,6 +414,47 @@ static void determine_author_info(void)
 	author_date = date;
 }
 
+static int ends_rfc2822_footer(struct strbuf *sb)
+{
+	int ch;
+	int hit = 0;
+	int i, j, k;
+	int len = sb->len;
+	int first = 1;
+	const char *buf = sb->buf;
+
+	for (i = len - 1; i > 0; i--) {
+		if (hit && buf[i] == '\n')
+			break;
+		hit = (buf[i] == '\n');
+	}
+
+	while (i < len - 1 && buf[i] == '\n')
+		i++;
+
+	for (; i < len; i = k) {
+		for (k = i; k < len && buf[k] != '\n'; k++)
+			; /* do nothing */
+		k++;
+
+		if ((buf[k] == ' ' || buf[k] == '\t') && !first)
+			continue;
+
+		first = 0;
+
+		for (j = 0; i + j < len; j++) {
+			ch = buf[i + j];
+			if (ch == ':')
+				break;
+			if (isalnum(ch) ||
+			    (ch == '-'))
+				continue;
+			return 0;
+		}
+	}
+	return 1;
+}
+
 static int prepare_to_commit(const char *index_file, const char *prefix,
 			     struct wt_status *s)
 {
@@ -489,7 +530,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 		for (i = sb.len - 1; i > 0 && sb.buf[i - 1] != '\n'; i--)
 			; /* do nothing */
 		if (prefixcmp(sb.buf + i, sob.buf)) {
-			if (prefixcmp(sb.buf + i, sign_off_header))
+			if (!ends_rfc2822_footer(&sb))
 				strbuf_addch(&sb, '\n');
 			strbuf_addbuf(&sb, &sob);
 		}
diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
index e2ef532..d2de576 100755
--- a/t/t7501-commit.sh
+++ b/t/t7501-commit.sh
@@ -247,6 +247,47 @@ $existing" &&
 
 '
 
+test_expect_success 'signoff gap' '
+
+	echo 3 >positive &&
+	git add positive &&
+	alt="Alt-RFC-822-Header: Value" &&
+	git commit -s -m "welcome
+
+$alt" &&
+	git cat-file commit HEAD | sed -e "1,/^\$/d" > actual &&
+	(
+		echo welcome
+		echo
+		echo $alt
+		git var GIT_COMMITTER_IDENT |
+		sed -e "s/>.*/>/" -e "s/^/Signed-off-by: /"
+	) >expected &&
+	test_cmp expected actual
+'
+
+test_expect_success 'signoff gap 2' '
+
+	echo 4 >positive &&
+	git add positive &&
+	alt="fixed: 34" &&
+	git commit -s -m "welcome
+
+We have now
+$alt" &&
+	git cat-file commit HEAD | sed -e "1,/^\$/d" > actual &&
+	(
+		echo welcome
+		echo
+		echo We have now
+		echo $alt
+		echo
+		git var GIT_COMMITTER_IDENT |
+		sed -e "s/>.*/>/" -e "s/^/Signed-off-by: /"
+	) >expected &&
+	test_cmp expected actual
+'
+
 test_expect_success 'multiple -m' '
 
 	>negative &&
-- 
1.6.5.1

Re: [PATCH] commit: More generous accepting of RFC-2822 footer lines.

[Junio C Hamano]

David Brown <davidb@codeaurora.org> writes:

> From: David Brown <davidb@quicinc.com>
>
> 'git commit -s' will insert a blank line before the Signed-off-by
> line at the end of the message, unless this last line is a
> Signed-off-by line itself.  Common use has other trailing lines
> at the ends of commit text, in the style of RFC2822 headers.
>
> Be more generous in considering lines to be part of this footer.
> If the last paragraph of the commit message reasonably resembles
> RFC-2822 formatted lines, don't insert that blank line.

I do not think it is particularly readable to add Cc: at the end, and in a
sense this patch encourages that practice (without the patch, the end
result looks ugly and that has an effect to discourage people from adding
Cc: there).

But this is not a strong objection.  Applied.

Thanks.

Re: [PATCH] commit: More generous accepting of RFC-2822 footer lines.

[David Brown]

On Wed, Oct 28, 2009 at 11:06:50AM -0700, Junio C Hamano wrote:

> I do not think it is particularly readable to add Cc: at the end, and in a
> sense this patch encourages that practice (without the patch, the end
> result looks ugly and that has an effect to discourage people from adding
> Cc: there).

I wasn't actually even thinking of Cc: at the end.  I was
thinking more of things like Acked-by:, or Bugs-fixed:, or
Patch-applied-even-though-I-dont-like-it-by:, or
like that.

David

Re: [PATCH] commit: More generous accepting of RFC-2822 footer lines.

[SZEDER Gábor]

Hi,

> > From: David Brown <davidb@quicinc.com>
> >
> > 'git commit -s' will insert a blank line before the Signed-off-by
> > line at the end of the message, unless this last line is a
> > Signed-off-by line itself.  Common use has other trailing lines
> > at the ends of commit text, in the style of RFC2822 headers.
> >
> > Be more generous in considering lines to be part of this footer.
> > If the last paragraph of the commit message reasonably resembles
> > RFC-2822 formatted lines, don't insert that blank line.

I think this patch was a bit too generous.  If I make a one-line
commit message with git commit -s -m which has a colon in it, e.g.
'subsystem: what I did', then this patch removes the empty line
between the subject and the SOB line.


Best,
Gábor

[PATCH] commit: fix too generous RFC-2822 footer handling

[SZEDER Gábor]

Since commit c1e01b0c (commit: More generous accepting of RFC-2822
footer lines, 2009-10-28) RFC-2822-looking lines at the end of the
message are considered part of the footer and 'git commit -s -m'
doesn't add a newline between that footer and the new S-O-B line.
This new behaviour causes problems with subject-only commit messages
which happens to look like an RFC-2822 header (e.g. 'git commit -s -m
"subsystem: coolest feature ever"').  In such cases there won't be any
newline between the subject and the S-O-B line, and the S-O-B line
will show up at places where it should not (e.g. in the output of 'git
shortlog').

With this patch the newline will be always added if a commit message
has only a single line, even if it looks like an RFC-2822 header.

Signed-off-by: SZEDER Gábor <szeder@ira.uka.de>
---

 Maybe something like this?  Be careful when reviewing, it's 4AM
 here...


 builtin-commit.c  |    8 ++++++++
 t/t7501-commit.sh |    4 ++--
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/builtin-commit.c b/builtin-commit.c
index beddf01..4971156 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -429,6 +429,14 @@ static int ends_rfc2822_footer(struct strbuf *sb)
 		hit = (buf[i] == '\n');
 	}
 
+	for (j = i-1; j > 0; j--)
+		if (buf[j] == '\n') {
+			hit = 1;
+			break;
+		}
+	if (!hit)	/* one-line message */
+		return 0;
+
 	while (i < len - 1 && buf[i] == '\n')
 		i++;
 
diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
index d2de576..aaeedda 100755
--- a/t/t7501-commit.sh
+++ b/t/t7501-commit.sh
@@ -215,10 +215,10 @@ test_expect_success 'sign off (1)' '
 
 	echo 1 >positive &&
 	git add positive &&
-	git commit -s -m "thank you" &&
+	git commit -s -m "subsystem: coolest feature ever" &&
 	git cat-file commit HEAD | sed -e "1,/^\$/d" >actual &&
 	(
-		echo thank you
+		echo subsystem: coolest feature ever
 		echo
 		git var GIT_COMMITTER_IDENT |
 		sed -e "s/>.*/>/" -e "s/^/Signed-off-by: /"
-- 
1.6.5.2.201.g0f47

Re: [PATCH] commit: fix too generous RFC-2822 footer handling

[Junio C Hamano]

SZEDER Gábor <szeder@ira.uka.de> writes:

>  builtin-commit.c  |    8 ++++++++
>  t/t7501-commit.sh |    4 ++--
>  2 files changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/builtin-commit.c b/builtin-commit.c
> index beddf01..4971156 100644
> --- a/builtin-commit.c
> +++ b/builtin-commit.c
> @@ -429,6 +429,14 @@ static int ends_rfc2822_footer(struct strbuf *sb)
>  		hit = (buf[i] == '\n');
>  	}
>  
> +	for (j = i-1; j > 0; j--)
> +		if (buf[j] == '\n') {
> +			hit = 1;
> +			break;
> +		}
> +	if (!hit)	/* one-line message */
> +		return 0;
> +

That looks overly convoluted.  Why isn't the attached patch enough?

 - We inspected the last line of the message buffer, and 'i' is at the
   beginning of that last line;

 - At the line that begins at 'i', we found something that does not match
   the sob we are going to add;

 - We want a newline if it is a single liner (i.e. i == 0), or if that
   last one is not sob/acked-by and friends.

If you are anal and want to allow an author with a funny name "is allowed
as the first word", we _could_ encounter a single-liner commit like this:

        From: is allowed as the first word <author@example.xz>
	Subject: Signed-off-by: is allowed as the first word <author@example.xz>

        Signed-off-by: is allowed as the first word <author@example.xz>

and you may want to add "!i ||" in front of prefixcmp(), but I do not
think that is worth it.

 builtin-commit.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/builtin-commit.c b/builtin-commit.c
index c395cbf..cfa6b06 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -530,7 +530,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 		for (i = sb.len - 1; i > 0 && sb.buf[i - 1] != '\n'; i--)
 			; /* do nothing */
 		if (prefixcmp(sb.buf + i, sob.buf)) {
-			if (!ends_rfc2822_footer(&sb))
+			if (!i || !ends_rfc2822_footer(&sb))
 				strbuf_addch(&sb, '\n');
 			strbuf_addbuf(&sb, &sob);
 		}

Re: [PATCH] commit: fix too generous RFC-2822 footer handling

[SZEDER Gábor]

Hi,


On Tue, Nov 03, 2009 at 10:11:21PM -0800, Junio C Hamano wrote:
> That looks overly convoluted.

I figured that the function ends_rfc2822_footer() should tell us
whether the message, well, ends with an rfc2822 _footer_.  But since
it may say so even if there is only a single line in the commit
message, I thought this function should be fixed in the first place.

But yeah, that solution was unnecessarily complicated, after a good
night's sleep I would do it this way:

diff --git a/builtin-commit.c b/builtin-commit.c
index beddf01..c7dcbd0 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -428,6 +428,8 @@ static int ends_rfc2822_footer(struct strbuf *sb)
                        break;
                hit = (buf[i] == '\n');
        }
+       if (i == 0)     /* one-line message */
+               return 0;
 
        while (i < len - 1 && buf[i] == '\n')
                i++;

> Why isn't the attached patch enough?
> 
>  - We inspected the last line of the message buffer, and 'i' is at the
>    beginning of that last line;
> 
>  - At the line that begins at 'i', we found something that does not match
>    the sob we are going to add;
> 
>  - We want a newline if it is a single liner (i.e. i == 0), or if that
>    last one is not sob/acked-by and friends.

You are right in that there is no need to look for an rfc-2822
formatted footer when the commit message has only a single line.  But
ends_rfc2822_footer() still not completely behaves as its name would
suggest (i.e. it might match even if there is no footer).  Perhaps it
could just be renamed to ends_rfc282().


Best,
Gábor