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

SZEDER Gábor [off-list ref] writes:

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 [off-list ref]
	Subject: Signed-off-by: is allowed as the first word [off-list ref]

        Signed-off-by: is allowed as the first word [off-list ref]

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);
 		}

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