git-mailinfo doesn't stop parsing at the end of the header

git-mailinfo doesn't stop parsing at the end of the header

[Philip Hofstetter]

Hello,

today, after working on a topic branch and trying to rebase it on top
of the updated master, the rebase failed, complaining about an invalid
email address.

Some investigating revealed an interesting quirk in git-mailinfo which
seems to be a bit too eager to extract author information: Instead of
just looking at the From:-Line in a mails header (git-rebase seems to
use git-am which in turn uses git-mailinfo), it searches for "from:"
*anywhere* in the mail and uses the last found information as the
source for the author information.

In this case, git-format-patch has generated a file that looks
something like this:

--------------8<---------------

From d28f21ea8ca64681ba7756417799ceea81ad6873 Mon Sep 17 00:00:00 2001
From: Foo Bar <foo@bar.com>
Date: Tue, 17 Nov 2009 15:27:25 +0100
Subject: blah, blah, blah

from:
- this is a
- list for stuff
---
 list/of/changed/files                   |    1 -
 list/of/changed/files2                  |    1 -
 2 files changed, 0 insertions(+), 2 deletions(-)

the actual diff down here

--------------8<---------------

And when you feed this into mailinfo, this is what you get:

pilif@celes ~/git % git mailinfo /dev/null /dev/null < somepatch.patch
Author:
Email:
Subject: blah, blah, blah
Date: Tue, 17 Nov 2009 15:27:25 +0100

pilif@celes ~/git %

and consequently, anything that depends on the correct author being
extracted then fails.

While I know it's rude to have a line beginning with "from:" (and it's
even ruder to have a line beginning with "from "), IMHO the header
ends at the first blank line and I see no reason to extract author
information past the header.

And if this is in fact intended behavior, it should probably not be
permitted to create a commit that later on can't be rebased or applied
using git-am.

I had a look at the source of git-mailinfo to fix it myself, but this
thing does too much for my minimal knowledge in C.

Philip

-- 
Sensational AG
Giesshübelstrasse 62c, Postfach 1966, 8021 Zürich
Tel. +41 43 544 09 60, Mobile  +41 79 341 01 99
info@sensational.ch, http://www.sensational.ch

Re: git-mailinfo doesn't stop parsing at the end of the header

[Jeff King]

On Wed, Nov 18, 2009 at 03:20:48PM +0100, Philip Hofstetter wrote:

> Some investigating revealed an interesting quirk in git-mailinfo which
> seems to be a bit too eager to extract author information: Instead of
> just looking at the From:-Line in a mails header (git-rebase seems to
> use git-am which in turn uses git-mailinfo), it searches for "from:"
> *anywhere* in the mail and uses the last found information as the
> source for the author information.

It is not quite "anywhere"; extra headers are respected at the very top
of the message body. This is intentional, to allow one to indicate that
a patch you are sending was authored by somebody else.

So the problem is slightly less severe; the body of your commit message
has to _start_ with "From:". Still, it is awfully ugly to hit a parsing
ambiguity like this when you are trying to do something as simple as
rebase.

Some solutions I can think of are:

  1. Improve the header-finding heuristic to actually look for something
     more sane, like "From:.*<.*@.*>" (I don't recall off the top of my
     head which other headers we handle in this position. Probably
     Date, too).

  2. Give mailinfo a "--strict" mode to indicate that it is directly
     parsing the output of format-patch, and not some random email. Use
     --strict when invoking "git am" via "git rebase".

> While I know it's rude to have a line beginning with "from:" (and it's
> even ruder to have a line beginning with "from "), IMHO the header
> ends at the first blank line and I see no reason to extract author
> information past the header.

As I explained above, there is a reason, but I don't think it's rude to
have either of those lines. You were, after all, writing a commit
message, not an email (and even if you were, it is a failure of the
storage format if it can't represent your data correctly). So I think
git is to blame here.

-Peff

Re: git-mailinfo doesn't stop parsing at the end of the header

[Jeff King]

On Wed, Nov 18, 2009 at 10:51:54AM -0500, Jeff King wrote:

> So the problem is slightly less severe; the body of your commit message
> has to _start_ with "From:". Still, it is awfully ugly to hit a parsing
> ambiguity like this when you are trying to do something as simple as
> rebase.
> 
> Some solutions I can think of are:
> 
>   1. Improve the header-finding heuristic to actually look for something
>      more sane, like "From:.*<.*@.*>" (I don't recall off the top of my
>      head which other headers we handle in this position. Probably
>      Date, too).
> 
>   2. Give mailinfo a "--strict" mode to indicate that it is directly
>      parsing the output of format-patch, and not some random email. Use
>      --strict when invoking "git am" via "git rebase".

Solution (2) seemed like a lot of work, so here is the relatively small
solution (1). I think looking for <.*@.*> is too restrictive, as people
may be using:

  From: bare@example.com

which should remain valid. I just look for an '@' instead.

Note that this validation also applies to actual headers. Should we turn
it off for them? As it is, it breaks t3400, which uses a bogus email
address. I suppose we should probably preserve such bogosities if they
are in the official headers.

diff --git a/builtin-mailinfo.c b/builtin-mailinfo.c
index c90cd31..6d69ef3 100644
--- a/builtin-mailinfo.c
+++ b/builtin-mailinfo.c
@@ -275,6 +275,17 @@ static inline int cmp_header(const struct strbuf *line, const char *hdr)
 			line->buf[len] == ':' && isspace(line->buf[len + 1]);
 }
 
+static int validate_header(const char *header, const struct strbuf *data)
+{
+	if (!strcmp(header, "From"))
+		return !!strchr(data->buf, '@');
+	if (!strcmp(header, "Date")) {
+		char buf[50];
+		return parse_date(data->buf, buf, sizeof(buf)) >= 0;
+	}
+	return 1;
+}
+
 static int check_header(const struct strbuf *line,
 				struct strbuf *hdr_data[], int overwrite)
 {
@@ -289,8 +300,10 @@ static int check_header(const struct strbuf *line,
 			 */
 			strbuf_add(&sb, line->buf + len + 2, line->len - len - 2);
 			decode_header(&sb);
-			handle_header(&hdr_data[i], &sb);
-			ret = 1;
+			if (validate_header(header[i], &sb)) {
+				ret = 1;
+				handle_header(&hdr_data[i], &sb);
+			}
 			goto check_header_out;
 		}
 	}
diff --git a/t/t5100-mailinfo.sh b/t/t5100-mailinfo.sh
index 0279d07..be06e0f 100755
--- a/t/t5100-mailinfo.sh
+++ b/t/t5100-mailinfo.sh
@@ -11,7 +11,7 @@ test_expect_success 'split sample box' \
 	'git mailsplit -o. "$TEST_DIRECTORY"/t5100/sample.mbox >last &&
 	last=`cat last` &&
 	echo total is $last &&
-	test `cat last` = 14'
+	test `cat last` = 16'
 
 check_mailinfo () {
 	mail=$1 opt=$2
diff --git a/t/t5100/info0015 b/t/t5100/info0015
new file mode 100644
index 0000000..c4d8d77
--- /dev/null
+++ b/t/t5100/info0015
@@ -0,0 +1,5 @@
+Author: A U Thor
+Email: a.u.thor@example.com
+Subject: check bogus body header (from)
+Date: Fri, 9 Jun 2006 00:44:16 -0700
+
diff --git a/t/t5100/info0016 b/t/t5100/info0016
new file mode 100644
index 0000000..f4857d4
--- /dev/null
+++ b/t/t5100/info0016
@@ -0,0 +1,5 @@
+Author: A U Thor
+Email: a.u.thor@example.com
+Subject: check bogus body header (date)
+Date: Fri, 9 Jun 2006 00:44:16 -0700
+
diff --git a/t/t5100/msg0015 b/t/t5100/msg0015
new file mode 100644
index 0000000..be5115b
--- /dev/null
+++ b/t/t5100/msg0015
@@ -0,0 +1,3 @@
+From: bogosity
+  - a list
+  - of stuff
diff --git a/t/t5100/msg0016 b/t/t5100/msg0016
new file mode 100644
index 0000000..1063f51
--- /dev/null
+++ b/t/t5100/msg0016
@@ -0,0 +1,4 @@
+Date: bogus
+
+and some content
+
diff --git a/t/t5100/patch0015 b/t/t5100/patch0015
new file mode 100644
index 0000000..ad64848
--- /dev/null
+++ b/t/t5100/patch0015
@@ -0,0 +1,8 @@
+---
+diff --git a/foo b/foo
+index e69de29..d95f3ad 100644
+--- a/foo
++++ b/foo
+@@ -0,0 +1 @@
++content
+
diff --git a/t/t5100/patch0016 b/t/t5100/patch0016
new file mode 100644
index 0000000..ad64848
--- /dev/null
+++ b/t/t5100/patch0016
@@ -0,0 +1,8 @@
+---
+diff --git a/foo b/foo
+index e69de29..d95f3ad 100644
+--- a/foo
++++ b/foo
+@@ -0,0 +1 @@
++content
+
diff --git a/t/t5100/sample.mbox b/t/t5100/sample.mbox
index 13fa4ae..de10312 100644
--- a/t/t5100/sample.mbox
+++ b/t/t5100/sample.mbox
@@ -650,3 +650,36 @@ index b0b5d8f..461c47e 100644
  		convert_to_utf8(line, charset.buf);
 -- 
 1.6.4.1
+From nobody Mon Sep 17 00:00:00 2001
+From: A U Thor <a.u.thor@example.com>
+Subject: check bogus body header (from)
+Date: Fri, 9 Jun 2006 00:44:16 -0700
+
+From: bogosity
+  - a list
+  - of stuff
+---
+diff --git a/foo b/foo
+index e69de29..d95f3ad 100644
+--- a/foo
++++ b/foo
+@@ -0,0 +1 @@
++content
+
+From nobody Mon Sep 17 00:00:00 2001
+From: A U Thor <a.u.thor@example.com>
+Subject: check bogus body header (date)
+Date: Fri, 9 Jun 2006 00:44:16 -0700
+
+Date: bogus
+
+and some content
+
+---
+diff --git a/foo b/foo
+index e69de29..d95f3ad 100644
+--- a/foo
++++ b/foo
+@@ -0,0 +1 @@
++content
+

[PATCH] git am/mailinfo: Don't look at in-body headers when rebasing

[Lukas Sandström]

When we are rebasing we know that the header lines in the
patch are good and that we don't need to pick up any headers
from the body of the patch.

This makes it possible to rebase commits whose commit message
start with "From" or "Date".

Test vectors by Jeff King.

Signed-off-by: Lukas Sandström <luksan@gmail.com>
---

Jeff King wrote:
>> Some solutions I can think of are:
>>
>>   1. Improve the header-finding heuristic to actually look for something
>>      more sane, like "From:.*<.*@.*>" (I don't recall off the top of my
>>      head which other headers we handle in this position. Probably
>>      Date, too).
>>
>>   2. Give mailinfo a "--strict" mode to indicate that it is directly
>>      parsing the output of format-patch, and not some random email. Use
>>      --strict when invoking "git am" via "git rebase".
> 
> Solution (2) seemed like a lot of work, so here is the relatively small
> solution (1). I think looking for <.*@.*> is too restrictive, as people
> may be using:
> 

This is an implementation of solution (2). Not much work, but I might
have missed something. git-mailinfo usally breaks when I touch it,
but the testsuite passes with this patch, including the extra test
vectors from Jeff.

The actual change is that mailinfo doesn't look for in-body headers
at all if --no-inbody-headers is passed. git-am now passes this option
to mailinfo when rebasing.

This won't handle the case when a "bad" patch is passed to git-am from
somewhere else than git rebase.

/Lukas


 builtin-mailinfo.c                   |    5 +++++
 git-am.sh                            |   13 ++++++++++---
 t/t5100-mailinfo.sh                  |    6 +++++-
 t/t5100/info0015                     |    5 +++++
 t/t5100/info0015--no-inbody-headers  |    5 +++++
 t/t5100/info0016                     |    5 +++++
 t/t5100/info0016--no-inbody-headers  |    5 +++++
 t/t5100/msg0015                      |    2 ++
 t/t5100/msg0015--no-inbody-headers   |    3 +++
 t/t5100/msg0016                      |    2 ++
 t/t5100/msg0016--no-inbody-headers   |    4 ++++
 t/t5100/patch0015                    |    8 ++++++++
 t/t5100/patch0015--no-inbody-headers |    8 ++++++++
 t/t5100/patch0016                    |    8 ++++++++
 t/t5100/patch0016--no-inbody-headers |    8 ++++++++
 t/t5100/sample.mbox                  |   33
+++++++++++++++++++++++++++++++++
 16 files changed, 116 insertions(+), 4 deletions(-)
 create mode 100644 t/t5100/info0015
 create mode 100644 t/t5100/info0015--no-inbody-headers
 create mode 100644 t/t5100/info0016
 create mode 100644 t/t5100/info0016--no-inbody-headers
 create mode 100644 t/t5100/msg0015
 create mode 100644 t/t5100/msg0015--no-inbody-headers
 create mode 100644 t/t5100/msg0016
 create mode 100644 t/t5100/msg0016--no-inbody-headers
 create mode 100644 t/t5100/patch0015
 create mode 100644 t/t5100/patch0015--no-inbody-headers
 create mode 100644 t/t5100/patch0016
 create mode 100644 t/t5100/patch0016--no-inbody-headers

diff --git a/builtin-mailinfo.c b/builtin-mailinfo.c
index c90cd31..a81526e 100644
--- a/builtin-mailinfo.c
+++ b/builtin-mailinfo.c
@@ -26,6 +26,7 @@ static struct strbuf charset = STRBUF_INIT;
 static int patch_lines;
 static struct strbuf **p_hdr_data, **s_hdr_data;
 static int use_scissors;
+static int use_inbody_headers = 1;

 #define MAX_HDR_PARSED 10
 #define MAX_BOUNDARIES 5
@@ -771,6 +772,8 @@ static int handle_commit_msg(struct strbuf *line)
 		return 0;

 	if (still_looking) {
+		if (!use_inbody_headers)
+			still_looking = 0;
 		strbuf_ltrim(line);
 		if (!line->len)
 			return 0;
@@ -1033,6 +1036,8 @@ int cmd_mailinfo(int argc, const char **argv,
const char *prefix)
 			use_scissors = 1;
 		else if (!strcmp(argv[1], "--no-scissors"))
 			use_scissors = 0;
+		else if (!strcmp(argv[1], "--no-inbody-headers"))
+			use_inbody_headers = 0;
 		else
 			usage(mailinfo_usage);
 		argc--; argv++;
diff --git a/git-am.sh b/git-am.sh
index c132f50..96869a2 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -289,7 +289,7 @@ split_patches () {
 prec=4
 dotest="$GIT_DIR/rebase-apply"
 sign= utf8=t keep= skip= interactive= resolved= rebasing= abort=
-resolvemsg= resume= scissors=
+resolvemsg= resume= scissors= no_inbody_headers=
 git_apply_opt=
 committer_date_is_author_date=
 ignore_date=
@@ -322,7 +322,7 @@ do
 	--abort)
 		abort=t ;;
 	--rebasing)
-		rebasing=t threeway=t keep=t scissors=f ;;
+		rebasing=t threeway=t keep=t scissors=f no_inbody_headers=t ;;
 	-d|--dotest)
 		die "-d option is no longer supported.  Do not use."
 		;;
@@ -448,6 +448,7 @@ else
 	echo "$utf8" >"$dotest/utf8"
 	echo "$keep" >"$dotest/keep"
 	echo "$scissors" >"$dotest/scissors"
+	echo "$no_inbody_headers" >"$dotest/no_inbody_headers"
 	echo "$GIT_QUIET" >"$dotest/quiet"
 	echo 1 >"$dotest/next"
 	if test -n "$rebasing"
@@ -495,6 +496,12 @@ t)
 f)
 	scissors=--no-scissors ;;
 esac
+if test "$(cat "$dotest/no_inbody_headers")" = t
+then
+	no_inbody_headers=--no-inbody-headers
+else
+	no_inbody_headers=
+fi
 if test "$(cat "$dotest/quiet")" = t
 then
 	GIT_QUIET=t
@@ -549,7 +556,7 @@ do
 	# by the user, or the user can tell us to do so by --resolved flag.
 	case "$resume" in
 	'')
-		git mailinfo $keep $scissors $utf8 "$dotest/msg" "$dotest/patch" \
+		git mailinfo $keep $no_inbody_headers $scissors $utf8 "$dotest/msg" "$dotest/patch" \
 			<"$dotest/$msgnum" >"$dotest/info" ||
 			stop_here $this

diff --git a/t/t5100-mailinfo.sh b/t/t5100-mailinfo.sh
index 0279d07..50e13c1 100755
--- a/t/t5100-mailinfo.sh
+++ b/t/t5100-mailinfo.sh
@@ -11,7 +11,7 @@ test_expect_success 'split sample box' \
 	'git mailsplit -o. "$TEST_DIRECTORY"/t5100/sample.mbox >last &&
 	last=`cat last` &&
 	echo total is $last &&
-	test `cat last` = 14'
+	test `cat last` = 16'

 check_mailinfo () {
 	mail=$1 opt=$2
@@ -30,6 +30,10 @@ do
 		if test -f "$TEST_DIRECTORY"/t5100/msg$mail--scissors
 		then
 			check_mailinfo $mail --scissors
+		fi &&
+		if test -f "$TEST_DIRECTORY"/t5100/msg$mail--use-first-header
+		then
+			check_mailinfo $mail --no-inbody-headers
 		fi
 	'
 done
diff --git a/t/t5100/info0015 b/t/t5100/info0015
new file mode 100644
index 0000000..0114f10
--- /dev/null
+++ b/t/t5100/info0015
@@ -0,0 +1,5 @@
+Author:
+Email:
+Subject: check bogus body header (from)
+Date: Fri, 9 Jun 2006 00:44:16 -0700
+
diff --git a/t/t5100/info0015--no-inbody-headers
b/t/t5100/info0015--no-inbody-headers
new file mode 100644
index 0000000..c4d8d77
--- /dev/null
+++ b/t/t5100/info0015--no-inbody-headers
@@ -0,0 +1,5 @@
+Author: A U Thor
+Email: a.u.thor@example.com
+Subject: check bogus body header (from)
+Date: Fri, 9 Jun 2006 00:44:16 -0700
+
diff --git a/t/t5100/info0016 b/t/t5100/info0016
new file mode 100644
index 0000000..38ccd0d
--- /dev/null
+++ b/t/t5100/info0016
@@ -0,0 +1,5 @@
+Author: A U Thor
+Email: a.u.thor@example.com
+Subject: check bogus body header (date)
+Date: bogus
+
diff --git a/t/t5100/info0016--no-inbody-headers
b/t/t5100/info0016--no-inbody-headers
new file mode 100644
index 0000000..f4857d4
--- /dev/null
+++ b/t/t5100/info0016--no-inbody-headers
@@ -0,0 +1,5 @@
+Author: A U Thor
+Email: a.u.thor@example.com
+Subject: check bogus body header (date)
+Date: Fri, 9 Jun 2006 00:44:16 -0700
+
diff --git a/t/t5100/msg0015 b/t/t5100/msg0015
new file mode 100644
index 0000000..9577238
--- /dev/null
+++ b/t/t5100/msg0015
@@ -0,0 +1,2 @@
+- a list
+  - of stuff
diff --git a/t/t5100/msg0015--no-inbody-headers
b/t/t5100/msg0015--no-inbody-headers
new file mode 100644
index 0000000..be5115b
--- /dev/null
+++ b/t/t5100/msg0015--no-inbody-headers
@@ -0,0 +1,3 @@
+From: bogosity
+  - a list
+  - of stuff
diff --git a/t/t5100/msg0016 b/t/t5100/msg0016
new file mode 100644
index 0000000..0d9adad
--- /dev/null
+++ b/t/t5100/msg0016
@@ -0,0 +1,2 @@
+and some content
+
diff --git a/t/t5100/msg0016--no-inbody-headers
b/t/t5100/msg0016--no-inbody-headers
new file mode 100644
index 0000000..1063f51
--- /dev/null
+++ b/t/t5100/msg0016--no-inbody-headers
@@ -0,0 +1,4 @@
+Date: bogus
+
+and some content
+
diff --git a/t/t5100/patch0015 b/t/t5100/patch0015
new file mode 100644
index 0000000..ad64848
--- /dev/null
+++ b/t/t5100/patch0015
@@ -0,0 +1,8 @@
+---
+diff --git a/foo b/foo
+index e69de29..d95f3ad 100644
+--- a/foo
++++ b/foo
+@@ -0,0 +1 @@
++content
+
diff --git a/t/t5100/patch0015--no-inbody-headers
b/t/t5100/patch0015--no-inbody-headers
new file mode 100644
index 0000000..ad64848
--- /dev/null
+++ b/t/t5100/patch0015--no-inbody-headers
@@ -0,0 +1,8 @@
+---
+diff --git a/foo b/foo
+index e69de29..d95f3ad 100644
+--- a/foo
++++ b/foo
+@@ -0,0 +1 @@
++content
+
diff --git a/t/t5100/patch0016 b/t/t5100/patch0016
new file mode 100644
index 0000000..ad64848
--- /dev/null
+++ b/t/t5100/patch0016
@@ -0,0 +1,8 @@
+---
+diff --git a/foo b/foo
+index e69de29..d95f3ad 100644
+--- a/foo
++++ b/foo
+@@ -0,0 +1 @@
++content
+
diff --git a/t/t5100/patch0016--no-inbody-headers
b/t/t5100/patch0016--no-inbody-headers
new file mode 100644
index 0000000..ad64848
--- /dev/null
+++ b/t/t5100/patch0016--no-inbody-headers
@@ -0,0 +1,8 @@
+---
+diff --git a/foo b/foo
+index e69de29..d95f3ad 100644
+--- a/foo
++++ b/foo
+@@ -0,0 +1 @@
++content
+
diff --git a/t/t5100/sample.mbox b/t/t5100/sample.mbox
index 13fa4ae..de10312 100644
--- a/t/t5100/sample.mbox
+++ b/t/t5100/sample.mbox
@@ -650,3 +650,36 @@ index b0b5d8f..461c47e 100644
  		convert_to_utf8(line, charset.buf);
 --
 1.6.4.1
+From nobody Mon Sep 17 00:00:00 2001
+From: A U Thor <a.u.thor@example.com>
+Subject: check bogus body header (from)
+Date: Fri, 9 Jun 2006 00:44:16 -0700
+
+From: bogosity
+  - a list
+  - of stuff
+---
+diff --git a/foo b/foo
+index e69de29..d95f3ad 100644
+--- a/foo
++++ b/foo
+@@ -0,0 +1 @@
++content
+
+From nobody Mon Sep 17 00:00:00 2001
+From: A U Thor <a.u.thor@example.com>
+Subject: check bogus body header (date)
+Date: Fri, 9 Jun 2006 00:44:16 -0700
+
+Date: bogus
+
+and some content
+
+---
+diff --git a/foo b/foo
+index e69de29..d95f3ad 100644
+--- a/foo
++++ b/foo
+@@ -0,0 +1 @@
++content
+
-- 
1.6.4.4

Re: [PATCH] git am/mailinfo: Don't look at in-body headers when rebasing

[Philip Hofstetter]

Hi,

On Wed, Nov 18, 2009 at 11:45 PM, Lukas Sandström <luksan@gmail.com> wrote:

> The actual change is that mailinfo doesn't look for in-body headers
> at all if --no-inbody-headers is passed. git-am now passes this option
> to mailinfo when rebasing.

after all the earlier discussion and a lot of thinking, I have to say,
that IMHO, this is the best option as it doesn't rely on heuristics
and now that you chose a descriptive command line switch, even the
small problem of "why exactly is this switch here?" seems to go away.

As I have no experience in git's codebase at all, I'll leave the
commenting on the patch itself to the people with clue, but
conceptionally, this feels much better than the method 1

> This won't handle the case when a "bad" patch is passed to git-am from
> somewhere else than git rebase.

of course, that leaves the question what "somewhere else" can contain.
If it's just manual calls to git-am, this is a non-issue as it's
easily fixed by the caller. If it's being called from other
higher-level operations though, you might run into the same issue
again.

Here too, I can't really provide any meaningful input though as I just
don't know well enough what really makes git tick.

Just my two cents :-)

Philip

[PATCH v2] git am/mailinfo: Don't look at in-body headers when rebasing

[Lukas Sandström]

When we are rebasing we know that the header lines in the
patch are good and that we don't need to pick up any headers
from the body of the patch.

This makes it possible to rebase commits whose commit message
start with "From" or "Date".

Test vectors by Jeff King.

Signed-off-by: Lukas Sandström <luksan@gmail.com>
---

Argh. I just realized that the change to t5100 in the previous patch
doesn't actually test the new option, since I forgot to change the
+		if test -f "$TEST_DIRECTORY"/t5100/msg$mail--use-first-header
line to "--no-inbody-headers", after my first attempt at an option name.

This time I checked that the tests actually fail when the test is broken.
Still passes, just one line changed since the previous version.

/Lukas

 builtin-mailinfo.c                   |    5 +++++
 git-am.sh                            |   13 ++++++++++---
 t/t5100-mailinfo.sh                  |    6 +++++-
 t/t5100/info0015                     |    5 +++++
 t/t5100/info0015--no-inbody-headers  |    5 +++++
 t/t5100/info0016                     |    5 +++++
 t/t5100/info0016--no-inbody-headers  |    5 +++++
 t/t5100/msg0015                      |    2 ++
 t/t5100/msg0015--no-inbody-headers   |    3 +++
 t/t5100/msg0016                      |    2 ++
 t/t5100/msg0016--no-inbody-headers   |    4 ++++
 t/t5100/patch0015                    |    8 ++++++++
 t/t5100/patch0015--no-inbody-headers |    8 ++++++++
 t/t5100/patch0016                    |    8 ++++++++
 t/t5100/patch0016--no-inbody-headers |    8 ++++++++
 t/t5100/sample.mbox                  |   33 +++++++++++++++++++++++++++++++++
 16 files changed, 116 insertions(+), 4 deletions(-)
 create mode 100644 t/t5100/info0015
 create mode 100644 t/t5100/info0015--no-inbody-headers
 create mode 100644 t/t5100/info0016
 create mode 100644 t/t5100/info0016--no-inbody-headers
 create mode 100644 t/t5100/msg0015
 create mode 100644 t/t5100/msg0015--no-inbody-headers
 create mode 100644 t/t5100/msg0016
 create mode 100644 t/t5100/msg0016--no-inbody-headers
 create mode 100644 t/t5100/patch0015
 create mode 100644 t/t5100/patch0015--no-inbody-headers
 create mode 100644 t/t5100/patch0016
 create mode 100644 t/t5100/patch0016--no-inbody-headers

diff --git a/builtin-mailinfo.c b/builtin-mailinfo.c
index c90cd31..a81526e 100644
--- a/builtin-mailinfo.c
+++ b/builtin-mailinfo.c
@@ -26,6 +26,7 @@ static struct strbuf charset = STRBUF_INIT;
 static int patch_lines;
 static struct strbuf **p_hdr_data, **s_hdr_data;
 static int use_scissors;
+static int use_inbody_headers = 1;

 #define MAX_HDR_PARSED 10
 #define MAX_BOUNDARIES 5
@@ -771,6 +772,8 @@ static int handle_commit_msg(struct strbuf *line)
 		return 0;

 	if (still_looking) {
+		if (!use_inbody_headers)
+			still_looking = 0;
 		strbuf_ltrim(line);
 		if (!line->len)
 			return 0;
@@ -1033,6 +1036,8 @@ int cmd_mailinfo(int argc, const char **argv, const char *prefix)
 			use_scissors = 1;
 		else if (!strcmp(argv[1], "--no-scissors"))
 			use_scissors = 0;
+		else if (!strcmp(argv[1], "--no-inbody-headers"))
+			use_inbody_headers = 0;
 		else
 			usage(mailinfo_usage);
 		argc--; argv++;
diff --git a/git-am.sh b/git-am.sh
index c132f50..96869a2 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -289,7 +289,7 @@ split_patches () {
 prec=4
 dotest="$GIT_DIR/rebase-apply"
 sign= utf8=t keep= skip= interactive= resolved= rebasing= abort=
-resolvemsg= resume= scissors=
+resolvemsg= resume= scissors= no_inbody_headers=
 git_apply_opt=
 committer_date_is_author_date=
 ignore_date=
@@ -322,7 +322,7 @@ do
 	--abort)
 		abort=t ;;
 	--rebasing)
-		rebasing=t threeway=t keep=t scissors=f ;;
+		rebasing=t threeway=t keep=t scissors=f no_inbody_headers=t ;;
 	-d|--dotest)
 		die "-d option is no longer supported.  Do not use."
 		;;
@@ -448,6 +448,7 @@ else
 	echo "$utf8" >"$dotest/utf8"
 	echo "$keep" >"$dotest/keep"
 	echo "$scissors" >"$dotest/scissors"
+	echo "$no_inbody_headers" >"$dotest/no_inbody_headers"
 	echo "$GIT_QUIET" >"$dotest/quiet"
 	echo 1 >"$dotest/next"
 	if test -n "$rebasing"
@@ -495,6 +496,12 @@ t)
 f)
 	scissors=--no-scissors ;;
 esac
+if test "$(cat "$dotest/no_inbody_headers")" = t
+then
+	no_inbody_headers=--no-inbody-headers
+else
+	no_inbody_headers=
+fi
 if test "$(cat "$dotest/quiet")" = t
 then
 	GIT_QUIET=t
@@ -549,7 +556,7 @@ do
 	# by the user, or the user can tell us to do so by --resolved flag.
 	case "$resume" in
 	'')
-		git mailinfo $keep $scissors $utf8 "$dotest/msg" "$dotest/patch" \
+		git mailinfo $keep $no_inbody_headers $scissors $utf8 "$dotest/msg" "$dotest/patch" \
 			<"$dotest/$msgnum" >"$dotest/info" ||
 			stop_here $this

diff --git a/t/t5100-mailinfo.sh b/t/t5100-mailinfo.sh
index 0279d07..ebc36c1 100755
--- a/t/t5100-mailinfo.sh
+++ b/t/t5100-mailinfo.sh
@@ -11,7 +11,7 @@ test_expect_success 'split sample box' \
 	'git mailsplit -o. "$TEST_DIRECTORY"/t5100/sample.mbox >last &&
 	last=`cat last` &&
 	echo total is $last &&
-	test `cat last` = 14'
+	test `cat last` = 16'

 check_mailinfo () {
 	mail=$1 opt=$2
@@ -30,6 +30,10 @@ do
 		if test -f "$TEST_DIRECTORY"/t5100/msg$mail--scissors
 		then
 			check_mailinfo $mail --scissors
+		fi &&
+		if test -f "$TEST_DIRECTORY"/t5100/msg$mail--no-inbody-headers
+		then
+			check_mailinfo $mail --no-inbody-headers
 		fi
 	'
 done
diff --git a/t/t5100/info0015 b/t/t5100/info0015
new file mode 100644
index 0000000..0114f10
--- /dev/null
+++ b/t/t5100/info0015
@@ -0,0 +1,5 @@
+Author:
+Email:
+Subject: check bogus body header (from)
+Date: Fri, 9 Jun 2006 00:44:16 -0700
+
diff --git a/t/t5100/info0015--no-inbody-headers b/t/t5100/info0015--no-inbody-headers
new file mode 100644
index 0000000..c4d8d77
--- /dev/null
+++ b/t/t5100/info0015--no-inbody-headers
@@ -0,0 +1,5 @@
+Author: A U Thor
+Email: a.u.thor@example.com
+Subject: check bogus body header (from)
+Date: Fri, 9 Jun 2006 00:44:16 -0700
+
diff --git a/t/t5100/info0016 b/t/t5100/info0016
new file mode 100644
index 0000000..38ccd0d
--- /dev/null
+++ b/t/t5100/info0016
@@ -0,0 +1,5 @@
+Author: A U Thor
+Email: a.u.thor@example.com
+Subject: check bogus body header (date)
+Date: bogus
+
diff --git a/t/t5100/info0016--no-inbody-headers b/t/t5100/info0016--no-inbody-headers
new file mode 100644
index 0000000..f4857d4
--- /dev/null
+++ b/t/t5100/info0016--no-inbody-headers
@@ -0,0 +1,5 @@
+Author: A U Thor
+Email: a.u.thor@example.com
+Subject: check bogus body header (date)
+Date: Fri, 9 Jun 2006 00:44:16 -0700
+
diff --git a/t/t5100/msg0015 b/t/t5100/msg0015
new file mode 100644
index 0000000..9577238
--- /dev/null
+++ b/t/t5100/msg0015
@@ -0,0 +1,2 @@
+- a list
+  - of stuff
diff --git a/t/t5100/msg0015--no-inbody-headers b/t/t5100/msg0015--no-inbody-headers
new file mode 100644
index 0000000..be5115b
--- /dev/null
+++ b/t/t5100/msg0015--no-inbody-headers
@@ -0,0 +1,3 @@
+From: bogosity
+  - a list
+  - of stuff
diff --git a/t/t5100/msg0016 b/t/t5100/msg0016
new file mode 100644
index 0000000..0d9adad
--- /dev/null
+++ b/t/t5100/msg0016
@@ -0,0 +1,2 @@
+and some content
+
diff --git a/t/t5100/msg0016--no-inbody-headers b/t/t5100/msg0016--no-inbody-headers
new file mode 100644
index 0000000..1063f51
--- /dev/null
+++ b/t/t5100/msg0016--no-inbody-headers
@@ -0,0 +1,4 @@
+Date: bogus
+
+and some content
+
diff --git a/t/t5100/patch0015 b/t/t5100/patch0015
new file mode 100644
index 0000000..ad64848
--- /dev/null
+++ b/t/t5100/patch0015
@@ -0,0 +1,8 @@
+---
+diff --git a/foo b/foo
+index e69de29..d95f3ad 100644
+--- a/foo
++++ b/foo
+@@ -0,0 +1 @@
++content
+
diff --git a/t/t5100/patch0015--no-inbody-headers b/t/t5100/patch0015--no-inbody-headers
new file mode 100644
index 0000000..ad64848
--- /dev/null
+++ b/t/t5100/patch0015--no-inbody-headers
@@ -0,0 +1,8 @@
+---
+diff --git a/foo b/foo
+index e69de29..d95f3ad 100644
+--- a/foo
++++ b/foo
+@@ -0,0 +1 @@
++content
+
diff --git a/t/t5100/patch0016 b/t/t5100/patch0016
new file mode 100644
index 0000000..ad64848
--- /dev/null
+++ b/t/t5100/patch0016
@@ -0,0 +1,8 @@
+---
+diff --git a/foo b/foo
+index e69de29..d95f3ad 100644
+--- a/foo
++++ b/foo
+@@ -0,0 +1 @@
++content
+
diff --git a/t/t5100/patch0016--no-inbody-headers b/t/t5100/patch0016--no-inbody-headers
new file mode 100644
index 0000000..ad64848
--- /dev/null
+++ b/t/t5100/patch0016--no-inbody-headers
@@ -0,0 +1,8 @@
+---
+diff --git a/foo b/foo
+index e69de29..d95f3ad 100644
+--- a/foo
++++ b/foo
+@@ -0,0 +1 @@
++content
+
diff --git a/t/t5100/sample.mbox b/t/t5100/sample.mbox
index 13fa4ae..de10312 100644
--- a/t/t5100/sample.mbox
+++ b/t/t5100/sample.mbox
@@ -650,3 +650,36 @@ index b0b5d8f..461c47e 100644
  		convert_to_utf8(line, charset.buf);
 --
 1.6.4.1
+From nobody Mon Sep 17 00:00:00 2001
+From: A U Thor <a.u.thor@example.com>
+Subject: check bogus body header (from)
+Date: Fri, 9 Jun 2006 00:44:16 -0700
+
+From: bogosity
+  - a list
+  - of stuff
+---
+diff --git a/foo b/foo
+index e69de29..d95f3ad 100644
+--- a/foo
++++ b/foo
+@@ -0,0 +1 @@
++content
+
+From nobody Mon Sep 17 00:00:00 2001
+From: A U Thor <a.u.thor@example.com>
+Subject: check bogus body header (date)
+Date: Fri, 9 Jun 2006 00:44:16 -0700
+
+Date: bogus
+
+and some content
+
+---
+diff --git a/foo b/foo
+index e69de29..d95f3ad 100644
+--- a/foo
++++ b/foo
+@@ -0,0 +1 @@
++content
+
-- 
1.6.4.4

Re: [PATCH v2] git am/mailinfo: Don't look at in-body headers when rebasing

[Jeff King]

On Thu, Nov 19, 2009 at 09:51:36AM +0100, Lukas Sandström wrote:

> When we are rebasing we know that the header lines in the
> patch are good and that we don't need to pick up any headers
> from the body of the patch.
> 
> This makes it possible to rebase commits whose commit message
> start with "From" or "Date".
> 
> Test vectors by Jeff King.

Thanks, it did end up being a pretty small change. Though I think we may
be better off with _both_ patches. Your patch protects the message
absolutely during rebasing, and my patch improves the heuristic when
applying non-rebase patches.

> @@ -771,6 +772,8 @@ static int handle_commit_msg(struct strbuf *line)
>  		return 0;
> 
>  	if (still_looking) {
> +		if (!use_inbody_headers)
> +			still_looking = 0;
>  		strbuf_ltrim(line);
>  		if (!line->len)
>  			return 0;

Hmm. But we still end up in this conditional for the very first line.
Which I guess happens to work because the first line we feed is
presumably the empty blank line (but I didn't check). Still, wouldn't it
be more clear as:

  if (use_inbody_headers && still_looking) {
     ...

in which case still_looking simply becomes irrelevant when the feature
is disabled?

> +From nobody Mon Sep 17 00:00:00 2001
> +From: A U Thor <a.u.thor@example.com>
> +Subject: check bogus body header (from)
> +Date: Fri, 9 Jun 2006 00:44:16 -0700
> +
> +From: bogosity
> +  - a list
> +  - of stuff
> +---

Since your feature is meant to prevent us looking at inbody headers no
matter if they are valid-looking or not, wouldn't a better test be to
actually have:

  From: Other Author <other@example.com>

Otherwise, you don't know if it is your feature blocking it, or my patch
(if it gets applied on top).

> +From nobody Mon Sep 17 00:00:00 2001
> +From: A U Thor <a.u.thor@example.com>
> +Subject: check bogus body header (date)
> +Date: Fri, 9 Jun 2006 00:44:16 -0700
> +
> +Date: bogus
> +
> +and some content
> +

And ditto for the Date here.

-Peff

[PATCH] git am/mailinfo: Don't look at in-body headers when rebasing

[Lukas Sandström]

When we are rebasing we know that the header lines in the
patch are good and that we don't need to pick up any headers
from the body of the patch.

This makes it possible to rebase commits whose commit message
start with "From" or "Date".

Test vectors by Jeff King.

Signed-off-by: Lukas Sandström <luksan@gmail.com>
---

Jeff King wrote:
> On Thu, Nov 19, 2009 at 09:51:36AM +0100, Lukas Sandström wrote:
> Thanks, it did end up being a pretty small change. Though I think we may
> be better off with _both_ patches. Your patch protects the message
> absolutely during rebasing, and my patch improves the heuristic when
> applying non-rebase patches.
> 

I looked a bit at using your extra safety check, but it would be a change
in behavior to only accept valid email adresses, and mailinfo
is riddled with corner cases. If this changed is made someone
will propably complain, saying that they rely on invalid dates in their
commit messages.

I don't know when to apply your stricter check whithout breaking any tests.

>> @@ -771,6 +772,8 @@ static int handle_commit_msg(struct strbuf *line)
>>  		return 0;
>>
>>  	if (still_looking) {
>> +		if (!use_inbody_headers)
>> +			still_looking = 0;
>>  		strbuf_ltrim(line);
>>  		if (!line->len)
>>  			return 0;
> 
> Hmm. But we still end up in this conditional for the very first line.
> Which I guess happens to work because the first line we feed is
> presumably the empty blank line (but I didn't check). Still, wouldn't it
> be more clear as:
> 
>   if (use_inbody_headers && still_looking) {
>      ...
> 
> in which case still_looking simply becomes irrelevant when the feature
> is disabled?

When rebasing the first line passed to handle_commit_msg will be the blank
line between the headers and commit message. This should be removed. I
rewrote this part to make it a bit more obvious.

> 
>> +From nobody Mon Sep 17 00:00:00 2001
>> +From: A U Thor <a.u.thor@example.com>
>> +Subject: check bogus body header (from)
>> +Date: Fri, 9 Jun 2006 00:44:16 -0700
>> +
>> +From: bogosity
>> +  - a list
>> +  - of stuff
>> +---
> 
> Since your feature is meant to prevent us looking at inbody headers no
> matter if they are valid-looking or not, wouldn't a better test be to
> actually have:
> 
>   From: Other Author <other@example.com>
> 
> Otherwise, you don't know if it is your feature blocking it, or my patch
> (if it gets applied on top).
> 

I kept the tests as is for now, since they show the problem
originally reported.

/Lukas


 builtin-mailinfo.c                   |   12 +++++++++++-
 git-am.sh                            |   13 ++++++++++---
 t/t5100-mailinfo.sh                  |    6 +++++-
 t/t5100/info0015                     |    5 +++++
 t/t5100/info0015--no-inbody-headers  |    5 +++++
 t/t5100/info0016                     |    5 +++++
 t/t5100/info0016--no-inbody-headers  |    5 +++++
 t/t5100/msg0015                      |    2 ++
 t/t5100/msg0015--no-inbody-headers   |    3 +++
 t/t5100/msg0016                      |    2 ++
 t/t5100/msg0016--no-inbody-headers   |    4 ++++
 t/t5100/patch0015                    |    8 ++++++++
 t/t5100/patch0015--no-inbody-headers |    8 ++++++++
 t/t5100/patch0016                    |    8 ++++++++
 t/t5100/patch0016--no-inbody-headers |    8 ++++++++
 t/t5100/sample.mbox                  |   33 +++++++++++++++++++++++++++++++++
 16 files changed, 122 insertions(+), 5 deletions(-)
 create mode 100644 t/t5100/info0015
 create mode 100644 t/t5100/info0015--no-inbody-headers
 create mode 100644 t/t5100/info0016
 create mode 100644 t/t5100/info0016--no-inbody-headers
 create mode 100644 t/t5100/msg0015
 create mode 100644 t/t5100/msg0015--no-inbody-headers
 create mode 100644 t/t5100/msg0016
 create mode 100644 t/t5100/msg0016--no-inbody-headers
 create mode 100644 t/t5100/patch0015
 create mode 100644 t/t5100/patch0015--no-inbody-headers
 create mode 100644 t/t5100/patch0016
 create mode 100644 t/t5100/patch0016--no-inbody-headers

diff --git a/builtin-mailinfo.c b/builtin-mailinfo.c
index c90cd31..3c4f075 100644
--- a/builtin-mailinfo.c
+++ b/builtin-mailinfo.c
@@ -26,6 +26,7 @@ static struct strbuf charset = STRBUF_INIT;
 static int patch_lines;
 static struct strbuf **p_hdr_data, **s_hdr_data;
 static int use_scissors;
+static int use_inbody_headers = 1;

 #define MAX_HDR_PARSED 10
 #define MAX_BOUNDARIES 5
@@ -774,10 +775,17 @@ static int handle_commit_msg(struct strbuf *line)
 		strbuf_ltrim(line);
 		if (!line->len)
 			return 0;
+	}
+
+	if (use_inbody_headers && still_looking) {
 		still_looking = check_header(line, s_hdr_data, 0);
 		if (still_looking)
 			return 0;
-	}
+	} else
+		/* Only trim the first (blank) line of the commit message
+		 * when ignoring in-body headers.
+		 */
+		still_looking = 0;

 	/* normalize the log message to UTF-8. */
 	if (metainfo_charset)
@@ -1033,6 +1041,8 @@ int cmd_mailinfo(int argc, const char **argv, const char *prefix)
 			use_scissors = 1;
 		else if (!strcmp(argv[1], "--no-scissors"))
 			use_scissors = 0;
+		else if (!strcmp(argv[1], "--no-inbody-headers"))
+			use_inbody_headers = 0;
 		else
 			usage(mailinfo_usage);
 		argc--; argv++;
diff --git a/git-am.sh b/git-am.sh
index c132f50..96869a2 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -289,7 +289,7 @@ split_patches () {
 prec=4
 dotest="$GIT_DIR/rebase-apply"
 sign= utf8=t keep= skip= interactive= resolved= rebasing= abort=
-resolvemsg= resume= scissors=
+resolvemsg= resume= scissors= no_inbody_headers=
 git_apply_opt=
 committer_date_is_author_date=
 ignore_date=
@@ -322,7 +322,7 @@ do
 	--abort)
 		abort=t ;;
 	--rebasing)
-		rebasing=t threeway=t keep=t scissors=f ;;
+		rebasing=t threeway=t keep=t scissors=f no_inbody_headers=t ;;
 	-d|--dotest)
 		die "-d option is no longer supported.  Do not use."
 		;;
@@ -448,6 +448,7 @@ else
 	echo "$utf8" >"$dotest/utf8"
 	echo "$keep" >"$dotest/keep"
 	echo "$scissors" >"$dotest/scissors"
+	echo "$no_inbody_headers" >"$dotest/no_inbody_headers"
 	echo "$GIT_QUIET" >"$dotest/quiet"
 	echo 1 >"$dotest/next"
 	if test -n "$rebasing"
@@ -495,6 +496,12 @@ t)
 f)
 	scissors=--no-scissors ;;
 esac
+if test "$(cat "$dotest/no_inbody_headers")" = t
+then
+	no_inbody_headers=--no-inbody-headers
+else
+	no_inbody_headers=
+fi
 if test "$(cat "$dotest/quiet")" = t
 then
 	GIT_QUIET=t
@@ -549,7 +556,7 @@ do
 	# by the user, or the user can tell us to do so by --resolved flag.
 	case "$resume" in
 	'')
-		git mailinfo $keep $scissors $utf8 "$dotest/msg" "$dotest/patch" \
+		git mailinfo $keep $no_inbody_headers $scissors $utf8 "$dotest/msg" "$dotest/patch" \
 			<"$dotest/$msgnum" >"$dotest/info" ||
 			stop_here $this

diff --git a/t/t5100-mailinfo.sh b/t/t5100-mailinfo.sh
index 0279d07..ebc36c1 100755
--- a/t/t5100-mailinfo.sh
+++ b/t/t5100-mailinfo.sh
@@ -11,7 +11,7 @@ test_expect_success 'split sample box' \
 	'git mailsplit -o. "$TEST_DIRECTORY"/t5100/sample.mbox >last &&
 	last=`cat last` &&
 	echo total is $last &&
-	test `cat last` = 14'
+	test `cat last` = 16'

 check_mailinfo () {
 	mail=$1 opt=$2
@@ -30,6 +30,10 @@ do
 		if test -f "$TEST_DIRECTORY"/t5100/msg$mail--scissors
 		then
 			check_mailinfo $mail --scissors
+		fi &&
+		if test -f "$TEST_DIRECTORY"/t5100/msg$mail--no-inbody-headers
+		then
+			check_mailinfo $mail --no-inbody-headers
 		fi
 	'
 done
diff --git a/t/t5100/info0015 b/t/t5100/info0015
new file mode 100644
index 0000000..0114f10
--- /dev/null
+++ b/t/t5100/info0015
@@ -0,0 +1,5 @@
+Author:
+Email:
+Subject: check bogus body header (from)
+Date: Fri, 9 Jun 2006 00:44:16 -0700
+
diff --git a/t/t5100/info0015--no-inbody-headers b/t/t5100/info0015--no-inbody-headers
new file mode 100644
index 0000000..c4d8d77
--- /dev/null
+++ b/t/t5100/info0015--no-inbody-headers
@@ -0,0 +1,5 @@
+Author: A U Thor
+Email: a.u.thor@example.com
+Subject: check bogus body header (from)
+Date: Fri, 9 Jun 2006 00:44:16 -0700
+
diff --git a/t/t5100/info0016 b/t/t5100/info0016
new file mode 100644
index 0000000..38ccd0d
--- /dev/null
+++ b/t/t5100/info0016
@@ -0,0 +1,5 @@
+Author: A U Thor
+Email: a.u.thor@example.com
+Subject: check bogus body header (date)
+Date: bogus
+
diff --git a/t/t5100/info0016--no-inbody-headers b/t/t5100/info0016--no-inbody-headers
new file mode 100644
index 0000000..f4857d4
--- /dev/null
+++ b/t/t5100/info0016--no-inbody-headers
@@ -0,0 +1,5 @@
+Author: A U Thor
+Email: a.u.thor@example.com
+Subject: check bogus body header (date)
+Date: Fri, 9 Jun 2006 00:44:16 -0700
+
diff --git a/t/t5100/msg0015 b/t/t5100/msg0015
new file mode 100644
index 0000000..9577238
--- /dev/null
+++ b/t/t5100/msg0015
@@ -0,0 +1,2 @@
+- a list
+  - of stuff
diff --git a/t/t5100/msg0015--no-inbody-headers b/t/t5100/msg0015--no-inbody-headers
new file mode 100644
index 0000000..be5115b
--- /dev/null
+++ b/t/t5100/msg0015--no-inbody-headers
@@ -0,0 +1,3 @@
+From: bogosity
+  - a list
+  - of stuff
diff --git a/t/t5100/msg0016 b/t/t5100/msg0016
new file mode 100644
index 0000000..0d9adad
--- /dev/null
+++ b/t/t5100/msg0016
@@ -0,0 +1,2 @@
+and some content
+
diff --git a/t/t5100/msg0016--no-inbody-headers b/t/t5100/msg0016--no-inbody-headers
new file mode 100644
index 0000000..1063f51
--- /dev/null
+++ b/t/t5100/msg0016--no-inbody-headers
@@ -0,0 +1,4 @@
+Date: bogus
+
+and some content
+
diff --git a/t/t5100/patch0015 b/t/t5100/patch0015
new file mode 100644
index 0000000..ad64848
--- /dev/null
+++ b/t/t5100/patch0015
@@ -0,0 +1,8 @@
+---
+diff --git a/foo b/foo
+index e69de29..d95f3ad 100644
+--- a/foo
++++ b/foo
+@@ -0,0 +1 @@
++content
+
diff --git a/t/t5100/patch0015--no-inbody-headers b/t/t5100/patch0015--no-inbody-headers
new file mode 100644
index 0000000..ad64848
--- /dev/null
+++ b/t/t5100/patch0015--no-inbody-headers
@@ -0,0 +1,8 @@
+---
+diff --git a/foo b/foo
+index e69de29..d95f3ad 100644
+--- a/foo
++++ b/foo
+@@ -0,0 +1 @@
++content
+
diff --git a/t/t5100/patch0016 b/t/t5100/patch0016
new file mode 100644
index 0000000..ad64848
--- /dev/null
+++ b/t/t5100/patch0016
@@ -0,0 +1,8 @@
+---
+diff --git a/foo b/foo
+index e69de29..d95f3ad 100644
+--- a/foo
++++ b/foo
+@@ -0,0 +1 @@
++content
+
diff --git a/t/t5100/patch0016--no-inbody-headers b/t/t5100/patch0016--no-inbody-headers
new file mode 100644
index 0000000..ad64848
--- /dev/null
+++ b/t/t5100/patch0016--no-inbody-headers
@@ -0,0 +1,8 @@
+---
+diff --git a/foo b/foo
+index e69de29..d95f3ad 100644
+--- a/foo
++++ b/foo
+@@ -0,0 +1 @@
++content
+
diff --git a/t/t5100/sample.mbox b/t/t5100/sample.mbox
index 13fa4ae..de10312 100644
--- a/t/t5100/sample.mbox
+++ b/t/t5100/sample.mbox
@@ -650,3 +650,36 @@ index b0b5d8f..461c47e 100644
  		convert_to_utf8(line, charset.buf);
 --
 1.6.4.1
+From nobody Mon Sep 17 00:00:00 2001
+From: A U Thor <a.u.thor@example.com>
+Subject: check bogus body header (from)
+Date: Fri, 9 Jun 2006 00:44:16 -0700
+
+From: bogosity
+  - a list
+  - of stuff
+---
+diff --git a/foo b/foo
+index e69de29..d95f3ad 100644
+--- a/foo
++++ b/foo
+@@ -0,0 +1 @@
++content
+
+From nobody Mon Sep 17 00:00:00 2001
+From: A U Thor <a.u.thor@example.com>
+Subject: check bogus body header (date)
+Date: Fri, 9 Jun 2006 00:44:16 -0700
+
+Date: bogus
+
+and some content
+
+---
+diff --git a/foo b/foo
+index e69de29..d95f3ad 100644
+--- a/foo
++++ b/foo
+@@ -0,0 +1 @@
++content
+
-- 
1.6.4.4

Re: git-mailinfo doesn't stop parsing at the end of the header

[Philip Hofstetter]

Hello,

On Wed, Nov 18, 2009 at 4:51 PM, Jeff King <peff@peff.net> wrote:
> On Wed, Nov 18, 2009 at 03:20:48PM +0100, Philip Hofstetter wrote:

>  1. Improve the header-finding heuristic to actually look for something
>     more sane, like "From:.*<.*@.*>" (I don't recall off the top of my
>     head which other headers we handle in this position. Probably
>     Date, too).

or at least don't prefer obviously invalid data over valid data that
has already been seen.

>  2. Give mailinfo a "--strict" mode to indicate that it is directly
>     parsing the output of format-patch, and not some random email. Use
>     --strict when invoking "git am" via "git rebase".

That would solve the problem too, though it feels like adding yet
another switch to guard against one specific issue. The purpose behind
options like this tends to get forgotten over time.

> As I explained above, there is a reason, but I don't think it's rude to
> have either of those lines. You were, after all, writing a commit
> message, not an email (and even if you were, it is a failure of the
> storage format if it can't represent your data correctly). So I think
> git is to blame here.

IMHO, another workable solution would be to reject a commit that later
can't be handled. That way the current attempts at getting an email
address can remain intact and the (much more) unlikely case that
somebody begins the commit message with from: will be caught before
damage is done.

So, just check that from-line for a valid email address at commit
time. If it is, ok. If not, treat it as an error and inform the user
that an invalid email address was given in the commit message.

Also, the error message by rebase (which is actually the message
printed by am) could have been a bit more helpful. If am fails during
a rebase, rebase could explicitly tell which commit am failed at. The
output I got made me suspect the problem to be in the first commit (as
that was the last one printed) when in fact it was in the second one
(which was not printed).

But that's just nit-picking.

Philip

Re: git-mailinfo doesn't stop parsing at the end of the header

[Jeff King]

On Wed, Nov 18, 2009 at 06:11:36PM +0100, Philip Hofstetter wrote:

> > As I explained above, there is a reason, but I don't think it's rude to
> > have either of those lines. You were, after all, writing a commit
> > message, not an email (and even if you were, it is a failure of the
> > storage format if it can't represent your data correctly). So I think
> > git is to blame here.
> 
> IMHO, another workable solution would be to reject a commit that later
> can't be handled. That way the current attempts at getting an email
> address can remain intact and the (much more) unlikely case that
> somebody begins the commit message with from: will be caught before
> damage is done.

I'm not sure I like that solution for a few reasons:

  1. It creates a bad user experience. You are not unreasonable for
     wanting to put some specific text in your commit message. Having
     git come back and say "oops, I might get confused by this later"
     just seems like an annoyance to the user.

  2. Mailinfo has to deal with data created by older versions of git. So
     in your case, the rebase was a bomb waiting to go off. If we can
     fix it so that an existing bomb doesn't go off, rather than not
     creating the bomb in the first place, then we are better off.

  3. Commit has to know about rules for mailinfo, even versions of
     mailinfo that will exist in the future. Probably the rules aren't
     going to change much, but it is a weakness.

  4. Commit messages can come from other places than "git commit". What
     should we do with a commit message like this that is imported from
     SVN? Reject the import? Munge the message?

Of course all of that presupposes that we can correctly handle the
existing data after the fact. Even with my patch, you still can't write
"From: foo@example.com" as the first line of your commit body. But that
is, IMHO, getting even more unlikely than your "From:" (which already
seems fairly unlikely).

I also think "git commit" would not be the right time for such a
feature. The problem is not that you have this text in your commit
message. The problem is that the "format-patch | am" transport is lossy.
You would do better to have format-patch say "Ah, this is going to
create a bogus email address" and somehow quote it appropriately.

-Peff

Re: git-mailinfo doesn't stop parsing at the end of the header

[Jakub Narebski]

Jeff King <peff@peff.net> writes:

> I also think "git commit" would not be the right time for such a
> feature. The problem is not that you have this text in your commit
> message. The problem is that the "format-patch | am" transport is lossy.
> You would do better to have format-patch say "Ah, this is going to
> create a bogus email address" and somehow quote it appropriately.

Doesn't mbox format have some way of escaping "From:" (or is it "From ")?
If I remember correctly it uses ">From " or something for that.
git-format-patch could do this also (perhaps only with --rebasing
option).

P.S. As git-format-patch / git-am have hidden --rebasing option,
perhaps git-mailinfo should have it as well (even if it is called
--strict).

-- 
Jakub Narebski
Poland
ShadeHawk on #git

Re: git-mailinfo doesn't stop parsing at the end of the header

[Jeff King]

On Wed, Nov 18, 2009 at 09:46:43AM -0800, Jakub Narebski wrote:

> > I also think "git commit" would not be the right time for such a
> > feature. The problem is not that you have this text in your commit
> > message. The problem is that the "format-patch | am" transport is lossy.
> > You would do better to have format-patch say "Ah, this is going to
> > create a bogus email address" and somehow quote it appropriately.
> 
> Doesn't mbox format have some way of escaping "From:" (or is it "From ")?
> If I remember correctly it uses ">From " or something for that.
> git-format-patch could do this also (perhaps only with --rebasing
> option).

It's for "From " lines, which are the mbox separator. This would be
somewhat different. It has nothing at all to do with the mail format
itself, but rather is about git treating the body of the message
specially. I don't think a quoting mechanism currently exists to handle
this.

-Peff

Re: git-mailinfo doesn't stop parsing at the end of the header

[Philip Hofstetter]

Hello,

On Wed, Nov 18, 2009 at 6:24 PM, Jeff King <peff@peff.net> wrote:

>  1. It creates a bad user experience. You are not unreasonable for
>     wanting to put some specific text in your commit message. Having
>     git come back and say "oops, I might get confused by this later"
>     just seems like an annoyance to the user.

agreed, though it's not that bad: when learning git, you will be
confronted with the fact that the commit message has a few things that
are special (well. it's doesn't break git, but the first line should
be < 56 chars in length for example).

Not being able to have From: lines in them that are not describing an
author would then just be one of them.

>  2. Mailinfo has to deal with data created by older versions of git. So
>     in your case, the rebase was a bomb waiting to go off. If we can
>     fix it so that an existing bomb doesn't go off, rather than not
>     creating the bomb in the first place, then we are better off.

This is a very good point. I didn't quite think about that.

>  4. Commit messages can come from other places than "git commit". What
>     should we do with a commit message like this that is imported from
>     SVN? Reject the import? Munge the message?

I would leave that to the tool that does the import. Probably it would
have to munge it. Yes.

I DO see though that implementing the check at commit time would lead
to problems popping up at other places.

> Of course all of that presupposes that we can correctly handle the
> existing data after the fact. Even with my patch, you still can't write
> "From: foo@example.com" as the first line of your commit body. But that

can't you? IMHO it would just attribute the commit to foo@example.com
which can be an equally bad, if not worse thing (I'm saying that
without the needed knowledge about git internals to really be sure, so
take this with a grain of salt)

I just have a bad feeling about trying out heuristics to see whether
thing thing after from: is an email address or not as email addresses
are notoriously hard to detect.

Typing a commit message and applying a patch from an email should be
separate things and should be handled separately. Currently they are
not and this is what's causing the problem in the first place.

Maybe that --strict thing is actually a good thing in the long run,
even though I don't quite like it either :-)

Interesting problem to have though.

Philip