threads / patch / 21392

patchcommit: More generous accepting of RFC-2822 footer lines.

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

## tl;dr

11 messages between Oct 27, 2009 and Nov 4, 2009. Diffs are folded; open one to read it.

replies: 10people: 5as markdown or json

David Brown· Oct 27, 2009, 23:45 UTC · lore
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(-)
Show changes to 2 files +35 −1

builtin-commit.c, t/t7501-commit.sh

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
Shawn O. Pearce· Oct 28, 2009, 00:05 UTC · re: David Brown · lore

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

David Brown <davidb@codeaurora.org> wrote:
Show 12 quoted lines
> 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-]+:".
Show 8 quoted lines
> +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.
Junio C Hamano· Oct 28, 2009, 07:14 UTC · re: Shawn O. Pearce · lore

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

"Shawn O. Pearce" <spearce@spearce.org> writes:
Show 18 quoted lines
> 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.

David Brown· Oct 28, 2009, 14:23 UTC · re: Junio C Hamano · lore

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

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
David Brown· Oct 28, 2009, 17:13 UTC · re: David Brown · lore
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(-)
Show changes to 2 files +83 −1

builtin-commit.c, t/t7501-commit.sh

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
Junio C Hamano· Oct 28, 2009, 18:06 UTC · re: David Brown · lore

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

David Brown <davidb@codeaurora.org> writes:
Show 10 quoted lines
> 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.
David Brown· Oct 28, 2009, 18:17 UTC · re: Junio C Hamano · lore

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

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
SZEDER Gábor· Nov 3, 2009, 16:59 UTC · re: Junio C Hamano · lore

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

Hi,
Show 10 quoted lines
> > 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

SZEDER Gábor· Nov 4, 2009, 03:09 UTC · re: SZEDER Gábor · lore

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

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(-)
Show changes to 2 files +10 −2

builtin-commit.c, t/t7501-commit.sh

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
Junio C Hamano· Nov 4, 2009, 06:11 UTC · re: SZEDER Gábor · lore

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

SZEDER Gábor <szeder@ira.uka.de> writes:
Show 20 quoted lines
>  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(-)
Show changes to builtin-commit.c +1 −1
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);
 		}
SZEDER Gábor· Nov 4, 2009, 15:11 UTC · re: Junio C Hamano · lore

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

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:

Show changes to builtin-commit.c +2 −0
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

← back to recent threads